fix(plugin): preserve committed diff scan integrity - #623
Conversation
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
@codex review |
|
Codex Review: Didn't find any major issues. Nice work! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
|
@codex review |
|
Codex Review: Didn't find any major issues. You're on a roll. Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
kmbroai
left a comment
There was a problem hiding this comment.
Reviewed head 4e478501a9283fa0aed4e1e99ab55db00529d134 for correctness, necessity, and simplification.
Necessity and correctness
This fixes a real integrity boundary: a selected commit/range must remain bound to the bytes inspected, including Git replacement behavior, rather than merely to revision names. The narrow temporary Git view is a reasonable approach because it freezes replacement configuration without copying an entire repository or holding the global SQLite writer lock while streaming changed blobs.
I traced the strict cat-file parser, digest/object identity construction, replacement-ref capture, committed-scan registration and completion, and the inspect/reacquire/revalidate transaction path. The parser checks object identity, framing, sizes, missing objects, and unexpected output; changed blobs are streamed through private temporary files. The locked recheck compares the frozen object identity rather than repeating expensive content reads. If lock contention intervenes, external inspection is repeated before the transaction proceeds. These are necessary parts of the fix, not redundant defensive checks.
The compatibility path is also appropriately narrow: recovering a legacy NULL digest does not generally authorize changing an already submitted selection. Current equivalent-workspace joins and state-free inspection retain their separate behavior. I did not reproduce a new correctness blocker at this head.
Simplification opportunities
This PR combines the object protocol, replacement-view construction, writer-lock scheduling, legacy recovery, and MCP timeout/fallback behavior. Keep the strict parser and immutable inspection result small and independently testable; avoid turning them into a general repository snapshot framework. The transaction helper should own the unlock/inspect/relock/revalidate invariant once, with callers only applying scan-specific updates.
If splitting is still practical, isolate the parser/replacement-view tests from the transaction/legacy compatibility changes while retaining the end-to-end integrity tests. The five-minute timeout belongs only to operations that actually perform the larger work; the implementation's unchanged shorter default is preferable to a blanket timeout increase.
Coordinate this with the restoration/inventory stack (#626 and #630) and assign a fresh integrated plugin version. Several independent branches currently reuse prospective version numbers, which is not a safe installation-cache contract.
Verification
Ran the focused workbench, compact-diff, and bundled timeout/fallback suites: 105 passed, 0 failed. These include committed replacement/object handling, stale selections, concurrent/legacy recovery, and actual bundled MCP paths. Execution was on Linux with Git 2.55 and cached dependencies; this is not independent validation of Git 2.39, native Windows, or a live model scan.
|
@codex review |
|
Codex Review: Didn't find any major issues. Another round soon, please! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
Summary
Bind Git-backed diff scans to the exact bytes inspected so replacement refs, concurrent target changes, missing objects, and stale legacy state cannot silently change a scan's selection.
Changes
0.1.23.Testing
bun test --timeout 30000 --seed 449 tests-ts/git-batch-object-validation.test.ts tests-ts/compact-diff-scan.test.ts tests-ts/prompt-only-start-timeout.test.ts: 105 passed, 0 failed, 1,529 expectations.bun test --seed 449 tests-ts/compact-diff-scan.test.ts tests-ts/prompt-only-start-timeout.test.ts: 18 passed, 0 failed, 350 expectations.bun test --seed 449 tests-ts/release-automation.test.ts tests-ts/skeleton.test.ts: 221 passed, 0 failed, 544 expectations after merging the latestmain.bun test --timeout 30000 --seed 449 ./tests-ts: 1,763 passed, 28 skipped, 0 failed, 23,448 expectations across 1,791 tests and 93 files.0.1.22to0.1.23; the upgraded installed runtime executed the fix and retained the safety configuration key.pnpm run types, Prettier checks, Python AST parsing, Ruff E9/F checks, andgit diff --check: passed.pnpm run build,pnpm pack,pnpm run test:package, andpnpm run check:package: passed; the installed package contained 113 bundled plugin files and 265 archive entries.Risk and rollout
The committed-target path now relies on canonical object coordinates and a frozen replacement mapping; working-tree behavior remains fully revalidated under the database writer transaction. The Git subprocess environment continues to honor trusted local configuration while forcing local-only object access.
The plugin identity change to
0.1.23forces cached0.1.22installations to refresh before using the corrected bundled runtime. Existing legacy rows are not backfilled.Public disclosure review