Skip to content

Use Ruby for local task inventory and pack validation - #803

Merged
justin808 merged 2 commits into
mainfrom
jg-codex/ruby-validation
Sep 9, 2026
Merged

justin808 merged 2 commits into
mainfrom
jg-codex/ruby-validation

Conversation

@justin808

@justin808 justin808 commented Sep 9, 2026 •

Copy link
Copy Markdown
Member

Why

The pack validation suite requires Python solely to run the local task-inventory helper's tests. Using Ruby for that helper and its tests keeps shared tooling on the runtime already used by the rest of the suite.

What changed

  • Port the task-inventory helper and its tests to Ruby. Preserve task filtering, archive handling, catalog fallback, timestamps, and JSON/TSV output.
  • Read SQLite through the existing sqlite3 command in read-only mode, with startup configuration disabled. No Ruby gem is added.
  • Remove the Python check and test invocation from bin/validate; document the SQLite prerequisite and the preference for Ruby in new shared helpers and validation tests.

This is separate from the recovery-helper conversion in #800. Python-based yamllint and historical benchmark scripts remain outside this change. The larger diff replaces both the implementation and its fixture suite together so the migration remains independently testable.

How to review and verify

  1. Compare the Ruby inventory's output and filtering with the replaced implementation.
  2. Run ruby skills/audit-chats/scripts/local_chat_inventory_test.rb. All 17 tests / 123 assertions pass, including execution without Python on PATH, read-only enforcement, unchanged database checksums, startup-file isolation, special-character paths, and catalog fallback.
  3. Check that bin/validate invokes the Ruby suite and has no Python requirement.

Test plan

  • Inventory behavior suite: 17 tests / 123 assertions passed.
  • Pinned RuboCop 1.87.0 passed for both Ruby files; shell syntax and diff checks passed.
  • Full hosted validation passed at 6dc366c20739d73c11a610a9c03e30b37fd12d80 after updating from main. The previous full run passed at c86437782d2800518c59ec1a26afec37d653977a.
  • Changelog classification: deferred_to_update_changelog.
Agent details

Commands and results

  • ruby skills/audit-chats/scripts/local_chat_inventory_test.rb: 17 tests / 123 assertions passed.
  • rubocop _1.87.0_ skills/audit-chats/scripts/local_chat_inventory.rb skills/audit-chats/scripts/local_chat_inventory_test.rb: passed.
  • bash -n bin/validate and git diff --check: passed.
  • Full local attempt stopped at the existing batch-status TERM/KILL timing test. The isolated timing test also fails on unchanged main (3.580 seconds against a 3.5-second limit); neither the helper nor its test changed here. Later full-suite sections were not reached. Full hosted validation subsequently passed at the committed head.
  • Final reviewed head: 6dc366c20739d73c11a610a9c03e30b37fd12d80; the independent integration audit verified equivalence to the tested implementation. Normal and C-locale inventory suites each passed.

Review

Independent codex review --uncommitted found no actionable regressions. It independently ran the 17-test suite and RuboCop. Tests use isolated SQLite fixtures; no private task database was read.

Before the base update, full CI and lint passed, and the hosted Claude review found no actionable issues, and no review threads remain open. CodeRabbit skipped automatic review and is advisory. Merged by guarded squash submission at a71885395892319ac8f77c508df4a4a1d9494c2e on 2026-09-09 at 10:40 UTC. Full CI and actual reviewer artifacts were complete before merge; no open review threads remained. An independent integration review verified that the PR patch is byte-for-byte unchanged against the new base 543a620668a9eea8d5e770acecab0684e2ca423d; only the upstream CodeRabbit setting changed.

Decision log

The existing SQLite CLI provides read-only JSON queries without a new Ruby gem. A separate YAML lint migration would change lint policy and is not included here.

QA Evidence

@github-actions github-actions Bot added the coderabbit:first-pass Triggers CodeRabbit's automatic first-pass pull-request review. label Sep 9, 2026
@coderabbitai

coderabbitai Bot commented Sep 9, 2026

Copy link
Copy Markdown

Important

  • 🔍 Trigger review

This repository does not receive automatic reviews because it has fewer than 10 stars.

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Advanced

Run ID: 68fc3c4c-510d-4d90-b053-8a1842f3c6bc


Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@claude

claude Bot commented Sep 9, 2026

Copy link
Copy Markdown

Reviewed the Python→Ruby port of the audit-chats local inventory helper. No correctness, security, or scope issues found.

Notes from the review:

  • SQL is built only from hardcoded literals and column names filtered through fixed whitelists intersected with the DB's actual schema, and paths are passed as separate Open3.capture3 argv elements — no SQL/shell injection surface.
  • -readonly -init /dev/null correctly enforces read-only access and blocks .sqliterc startup-file injection; both are exercised by tests (test_database_reads_preserve_files_and_connection_rejects_writes, test_inventory_runs_without_python_or_sqlite_startup_commands).
  • The port correctly distinguishes places needing Python's or-with-falsy-zero semantics (flag?/first_value) from places needing is None semantics (plain ||, since only nil/false are falsy in Ruby) — e.g. updated_at/recency_at vs. title/cwd. This is a common source of subtle Python→Ruby porting bugs and it's handled correctly throughout.
  • The title line-splitting regex correctly mirrors Python's str.splitlines() boundary set (verified against the Unicode/multiline test case).
  • sort_by! isn't stable like Python's sort, but the sort key always includes the unique id, so instability can't change output.
  • The expanded test suite (17 vs. 6 tests) is proportionate: the new tests specifically cover risk introduced by the new execution model (shelling out to sqlite3 vs. an in-process Python driver) — invalid UTF-8 output, corrupt DB, PATH/startup-file isolation, C-locale encoding — not unrelated padding.

Nice work on a faithful, well-tested port.

* origin/main:
  Disable CodeRabbit incremental reviews (#805)
@claude

claude Bot commented Sep 9, 2026

Copy link
Copy Markdown

Review

Went through the Python→Ruby port line-by-line against the deleted local_chat_inventory.py, plus bin/validate, AGENTS.md, docs/getting-started.md, and skills/audit-chats/SKILL.md.

Code quality / correctness: This is a careful, faithful port. Notably it correctly handles the trickiest Python→Ruby semantic gap: SQLite stores booleans as integers, and bool(0) is falsy in Python but 0 is truthy in Ruby. The flag? helper (local_chat_inventory.rb:61-64) and its comment make this explicit and apply it consistently everywhere the original used Python truthiness on SQLite ints (archived, pinned, missing_candidate, agent_path). By contrast, the updated_at/recency_at fallbacks correctly use plain || instead of flag?, since the Python original only guarded on is None there — the distinction is preserved correctly rather than applying flag? everywhere blanket-style.

Command construction uses Open3.capture3 with an argument array (not a shell string), so the --codex-home path can't cause shell injection even with special characters — and this is exercised by a test using a path with #, spaces, ?, %. -init /dev/null and -readonly are passed on every query, matching the PR's stated read-only/no-startup-file guarantees, and both are verified by dedicated tests (.sqliterc startup-command test, and a test asserting a DELETE through LocalChatInventory.query raises and leaves file checksums unchanged).

I checked candidate_ids dedup (array |= vs Python's set), the catalog-empty/fallback branching, the filter chain (missing_candidate/archived/agent_path/thread_source exclusions), and the sort (sort_by!.reverse! vs Python's stable sort(reverse=True)) — the sort key always includes the unique id as tiebreaker, so the stability difference is a non-issue in practice. All of this lines up with the original semantics.

Scope: Matches the stated Why — swap Python for Ruby in this one helper, no drive-by features. The test file grew relative to the Python version, but the additions target real risk in a language port (0/1 truthiness, UTF-8 validation from subprocess output, readonly enforcement, no-Python-on-PATH), not padding.

No functional bugs, security issues, or scope creep found.

@justin808
justin808 merged commit a718853 into main Sep 9, 2026
4 checks passed
@justin808
justin808 deleted the jg-codex/ruby-validation branch September 9, 2026 10:40
justin808 added a commit that referenced this pull request Sep 9, 2026
…very

* origin/main:
  Remove Python from pack validation (#803)
@justin808

Copy link
Copy Markdown
Member Author

Post-merge verification is clean. The merged tree at a71885395892319ac8f77c508df4a4a1d9494c2e exactly matches reviewed head 6dc366c20739d73c11a610a9c03e30b37fd12d80; no changes were lost. All four selected checks completed successfully before the merge, and no review threads remained open.

An independent integration check with companion PR #800 passed all 51 focused tests (248 assertions), including the Ruby inventory suite and both recovery suites. Full CI for that updated companion remains pending. Changelog handling remains deferred_to_update_changelog.

justin808 added a commit that referenced this pull request Sep 9, 2026
…usted-base-provenance

* origin/main:
  Make restart preparation bounded and verify every task recovers (#800)
  Remove Python from pack validation (#803)
  Disable CodeRabbit incremental reviews (#805)
justin808 added a commit that referenced this pull request Sep 9, 2026
…data-trust-boundary

* origin/main:
  Remove Python from pack validation (#803)
justin808 added a commit that referenced this pull request Sep 9, 2026
…-refill

* origin/main:
  Make restart preparation bounded and verify every task recovers (#800)
  Remove Python from pack validation (#803)
justin808 added a commit that referenced this pull request Sep 9, 2026
…ical-token-budgets

* origin/main:
  Make restart preparation bounded and verify every task recovers (#800)
  Remove Python from pack validation (#803)
  Disable CodeRabbit incremental reviews (#805)
justin808 added a commit that referenced this pull request Sep 9, 2026
…address-review

* origin/main:
  Report oversized PR diffs as blocked preflight coverage (#748)
  Make restart preparation bounded and verify every task recovers (#800)
  Remove Python from pack validation (#803)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

coderabbit:first-pass Triggers CodeRabbit's automatic first-pass pull-request review.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant