You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
Section-level annotations escape the REQ-010.3.2 dangling-reference net (src/changed.ts:190-194). Annotation IDs may name sections, but filtering unknown-ID violations only checks baseline requirement IDs. Deleting an entire section can therefore hide an unchanged dangling section annotation. Build baseline/current section-ID sets too and add a regression test.
SIMPLIFY
Narrow the CLI catch to IncrementalCheckError; the current condition catches every Error and hides programming defects.
Remove the unreachable empty merge-base guard or move its friendly wording into the Git action diagnostic.
Import shared .2119.yml and verdict-directory constants rather than duplicating literals.
Avoid computing all review targets unconditionally on plain checks where they are discarded.
SPEC / TEST HONESTY
The existing tests are generally genuinely falsifying: the merge-base fixture distinguishes tip from merge-base, block granularity has sibling negative controls, fail-closed fixtures would pass under a silent fallback, and config-change coverage genuinely requires universal scoping.
Weaker spots:
The network shim blocks only named top-level commands and does not prove that object-reading Git commands cannot lazily fetch.
Duplicate --changed flags are implemented but untested.
REQ-010.1.2 describes the union of baseline-to-HEAD and worktree differences, while the implementation computes one net baseline-to-worktree diff.
SUGGESTIONS
Fix the section-ID gap and test it.
Narrow the CLI catch and remove the dead merge-base guard.
Detect ambiguous refs explicitly instead of treating all Git stderr as fatal.
Batch baseline blob reads rather than spawning once per file.
Carry requirement IDs structurally on violations instead of parsing message text.
Import shared constants and avoid eager review-target calculation.
REQ-010.1.1: Git can perform implicit network access. In a partial clone, object-reading commands can lazily fetch missing promisor objects. Set GIT_NO_LAZY_FETCH=1 for every Git invocation and fail clearly if an object is unavailable.
REQ-010.1.2: changed paths are a net diff, not the specified union. A committed path restored to baseline by staged/unstaged changes disappears. Union baseline-to-HEAD, staged, unstaged, and untracked path sets.
REQ-010.1.3: baseline configuration can disappear when .2119.yml is a symlink. Baseline materialization skips symlinks, causing baseline config to silently use defaults. Reconstruct directly-read symlinks or fail closed.
REQ-010.2.2: annotation changes are ignored for non-enforced requirements. Evidence comparison uses review targets, which exclude non-enforced keywords, although coverage still records their annotations.
REQ-010.2.5 / 010.3.1: any historical verdict for an ID incorrectly affects the requirement. Only baseline/current review IDs should map verdict changes to a requirement; unrelated malformed files should remain standalone scoped violations.
REQ-010.2.6 / 010.3.1: configuration-driven annotation discovery is filtered out. When config changes, new unknown-annotation violations in unchanged evidence files must be retained.
SIMPLIFY
Remove the unused annotations field from CheckContext.
Use one Git execution helper so environment, buffering, and diagnostics cannot diverge.
Simplify the CLI catch condition.
SPEC / TEST HONESTY
MUST-FIX: no-network coverage does not exercise partial-clone lazy fetching.
MUST-FIX: changed-path coverage does not test cancelling committed/staged/unstaged states.
MUST-FIX: fail-closed coverage lacks a directly-read symlink baseline fixture.
MUST-FIX: annotation evidence coverage lacks a non-enforced requirement.
MUST-FIX: verdict coverage lacks an obsolete-verdict negative control.
I read the full diff, the two governing specs, and all touched source; the build is clean and all 124 tests pass. I then probed the implementation empirically in scratch repos. Three defects are confirmed with reproductions; the rest are simplification and test-honesty observations.
MUST-FIX
1. Benign git stderr warnings hard-fail every check --changed run (src/changed.ts:110, in gitText) gitText treats any stderr output as failure (result.error || result.status !== 0 || warning). That policy exists to catch ambiguous refs (verified: git rev-parse --verify amb^{commit} exits 0 and only warns on stderr), but it is applied to all six git invocations. Confirmed repro: a repo with core.autocrlf=input (the standard Linux/macOS setting; Windows uses autocrlf=true) and one modified CRLF file →
check --changed: Cannot read unstaged changes: warning: in the working copy of 'data.txt', CRLF will be replaced by LF...
exit=2
git diff --name-only succeeded and returned the complete path list; the feature is nonetheless unusable in a very common repo configuration. Fix: keep stderr-sensitivity only on the rev-parse resolve call (where it's the ambiguity signal, and where check-changed.test.ts already pins it); rely on exit status for merge-base/diff/ls-files. Effort: small.
2. A gitignored file matching evidence globs makes every requirement affected on every run, surfacing out-of-scope pre-existing violations (materializeBaseline / evidenceKey view asymmetry, src/changed.ts:123-186, 285-298)
The current view (walk()) lists all files including gitignored ones; the materialized baseline contains only tracked blobs. Any ignored file matching shared_evidence or [review] globs therefore exists in exactly one view, so evidenceKey differs forever. Confirmed repro: a repo with an ignored tests/helpers/cache.js under a shared_evidence glob, with zero working-tree changes, reports requirementCount: 2 and fails on a pre-existing, unrelated REQ-002.2.4 uncovered-requirement violation. This violates REQ-010.3.1 ("ignoring pre-existing violations outside that scope") and the overview's promise to agree with the verdict-freshness hash model. Note the fix has a soundness tension: copying ignored files into the baseline would silence real edits to ignored evidence (git can't attest them unchanged). The defensible fix is an explicit REQ-010.1.3-style diagnostic — fail (or at minimum warn) with "ignored file X matches evidence globs; incremental scoping cannot compare it" — rather than silently degenerating to full-scope failure output. Effort: small-medium.
3. A duplicate document ID introduced by a changed spec file escapes --changed entirely (src/check.ts:56-61 + lint filter at src/changed.ts:236)
The REQ-001.1.7 violation is attributed to the later-sorting file. When a change makes an earlier-sorting spec declare a doc ID that an unchanged file already owns, the violation lands on the unchanged file and changedPaths.has(...) filters it out. Confirmed repro (rename FIX-001-a.md → FIX-002-a.md with matching content edits): plain check fails with REQ-001.1.7 as the sole violation; check --changed HEADpasses (exit 0). This is not a pre-existing violation — it is introduced entirely by the change — so REQ-010.3.1 requires it to be reported. Fix: for cross-file lint rules, include the violation when either implicated file changed (the message already names the other path), or emit REQ-001.1.7 against both files. Effort: small.
SIMPLIFY
4. evidenceKey reimplements the evidence-parts assembly that computeReviewTargets already has (src/changed.ts:285-298 vs src/review.ts:41-72). Both build evidenceBlockParts(...) + fileParts(sharedEvidence) for test coverage and fileParts(instructions + globs) for review coverage. The REQ-010 overview explicitly warns against "inventing a second dependency model" — extract one evidenceParts(req, ctx) helper used by both, deleting the duplicate. (It can't be replaced by comparing review IDs alone: non-enforced requirements have no review target, and the tests correctly pin that case.) Effort: small-medium.
5. materializeBaseline spawns one git cat-file process per blob. On a 10k-file repo that is 10k process spawns per check — likely minutes. git cat-file --batch (one process, object IDs via stdin) preserves the exact fail-closed semantics. Effort: medium.
6. allReviewTargets is now computed unconditionally in buildContext (src/check.ts:72), including on all three hook events (hook.ts:127,139,163) and when reviews: false. That's re-reading and re-hashing every evidence block on every latency-sensitive after-edit hook, for data only buildChangedContext consumes. Compute it lazily or only in the --changed path. Effort: small.
7. messageRequirementId recovers requirement IDs by prefix-matching violation message text (src/changed.ts:319-324). It works today (I verified every producing site formats ${id} ..., and REQ-002.2.3 is handled via its own quoted-ID regex), but any message rewording silently breaks scoping. Adding an optional requirementId field to Violation deletes both parsers. Effort: small-medium.
SPEC/TEST HONESTY
8. REQ-010.1.2's "deletions retained as changed paths" clause is only indirectly tested. The three deletion cases in check-changed.test.ts (delete covering test → uncovered) pass through the baseline-vs-current evidence comparison, which never consults the changed-path set — you could add --diff-filter=d to all four git listings and those assertions would still pass. The only assertion that genuinely depends on deletion retention is the REQ-010.2.5 verdict-removal case, and only for the unstaged mode. Suggest adding staged and committed verdict-file deletions as direct probes. Effort: small.
9. The "no network access" test is honest where it matters, decorative elsewhere. The git shim genuinely binds behavior: it fails any git call lacking GIT_NO_LAZY_FETCH=1 and any fetch/pull/ls-remote, and asserts the expected diff invocations occurred — removing NO_NETWORK_ENV would fail the test. The Node --require network blocker, by contrast, patches modules the CLI never uses and doesn't constrain child processes; it's harmless but adds no verification. No change required.
10. Positive notes. The ambiguous-ref test would catch a naive fix to finding #1 (good — fix it surgically). The REQ-010.2.1 variants pin the affected set properly (requirementCount === 1 plus sibling-not-uncovered together imply the right requirement was selected). The gitignore tests use real git check-ignore in both polarities, including the parent-ignored and nested-.gitignore adversarial cases from round 1 — these would genuinely fail if the rules regressed.
Fix Make review audits automation-friendly #2 (ignored-evidence diagnostic) — small-medium; decide the soundness posture explicitly rather than inheriting it from view asymmetry.
#6 (lazy allReviewTargets) — small; removes a per-edit hook tax unrelated to the feature.
Two documentation-level notes, tiny effort: any submodule in the baseline tree makes --changed error unconditionally (defensible fail-closed, since walk() descends into checked-out submodules, but worth documenting or scoping the error to submodule paths that can match config globs), and shallow CI clones (fetch-depth: 1) will fail at merge-base with a message about "unrelated histories" that could also mention shallowness.
The REQ-003.2.2 spec rewording (from "never gitignored" to "pass/fail/init MUST leave verdicts unignored") makes the requirement enforceable by the named commands rather than promising control over future user edits, and the re-recorded verdict files are consistent with that amendment — no objection.
MUST-FIX — Changed duplicate spec IDs can pass with zero violations. Duplicate-ID violations are attached only to the later sorted spec in check.ts, then incremental filtering retains them only if that attached file changed in changed.ts. Adding specs/FIX-001-a.md with the same document ID as unchanged specs/FIX-001-z.md therefore attributes the violation to the unchanged z file and filters it out. I reproduced check --changed <base> --json exiting 0 with no violations. This violates REQ-010.3.1.
MUST-FIX — Deleting the only spec in a configless repository hides its dangling annotation. The scoped context correctly identifies the deleted requirement, but inherited notInitialized: true causes requireInitialized to exit before reporting it. I reproduced a repository containing only specs/REQ-001-a.md and an annotated test: deleting the spec produces the “2119 is not set up” diagnostic instead of the required REQ-002.2.3 violation. This violates REQ-010.3.2. Incremental mode should recognize that the baseline was initialized or otherwise allow deletion diagnostics through.
MUST-FIX — The sole REQ-003.2.2 test does not verify “plain JSON.”cli.test.ts checks only command status and Git ignore behavior. It never reads or parses the verdict produced by pass/fail, nor confirms init preserves an existing verdict. The test remains green if those commands write arbitrary or corrupted bytes to the .json file, so the newly recorded passing judgment is not honest for the complete requirement.
2. SIMPLIFY
SUGGESTION S1 — Centralize evidence dependency construction.changed.ts reimplements annotation-block, shared-evidence, review-glob, and instruction-file selection already embodied by computeReviewTargets. Extracting a canonical “evidence parts/fingerprint for requirement” helper would prevent the two implementations drifting again. Expected effort: medium.
SUGGESTION S2 — Avoid materializing every baseline blob through one Git process per file.changed.ts reads the entire tree and launches git cat-file separately for every blob. It also rejects any gitlink at changed.ts, even when unrelated to configured specs or evidence; I reproduced an unchanged, otherwise valid repository failing solely because it contained vendor/sub as a gitlink. Consider batch object reads and explicit, consistent submodule exclusion. Expected effort: medium.
3. SPEC/TEST HONESTY
MUST-FIX — REQ-010.3.1 lacks a cross-file lint mutant. The test at check-changed.test.ts covers a local missing-keyword violation only. It does not reject the duplicate-document-ID escape described above, where the changed file causes a violation attributed to an unchanged peer.
MUST-FIX — REQ-010.3.2 covers only deletion within a retained, explicitly configured spec set.check-changed.test.ts does not cover deleting the entire final spec in a repository using default configuration, allowing the notInitialized early exit to hide the required dangling-annotation violation.
MUST-FIX — REQ-003.2.2’s “plain JSON” conjunct is untested. Add assertions that each generated verdict is a directly located, parseable JSON object with the expected fields, and that init leaves existing verdict JSON intact.
The remaining REQ-010 tests use real Git repositories and CLI execution and appear capable of failing when their covered behavior is removed.
4. SUGGESTIONS
SUGGESTION S2: Make baseline reading batch-based and define consistent submodule handling. Expected effort: medium.
SUGGESTION S1: Reuse one canonical evidence-fingerprint implementation for review IDs and incremental scoping. Expected effort: medium.
The two-round review cap is complete. Round 1 findings were fixed before round 2. Round 2 found six concrete gaps; all were accepted, fixed in c22d933, and the exact local gate was rerun. No third model review was run because the workflow caps re-review at two rounds.
Round 1 — Fable 5
Accepted — section-level dangling annotations. Baseline section IDs are now retained so deleting a section reports unchanged annotations that became invalid; regression coverage was added.
Accepted — narrow the CLI catch.check --changed now catches only IncrementalCheckError, leaving programming defects visible.
Accepted — dead merge-base guard. Removed the unreachable empty-output guard and retained the actionable Git diagnostic.
Accepted — shared constants. Reused the configuration and verdict-directory constants.
Deferred — avoid eager review-target calculation. Valid performance cleanup, but it changes general context construction outside this one-flag feature.
Accepted — prove no implicit network access. Every Git invocation sets GIT_NO_LAZY_FETCH=1; the final test harness also allowlists local-only Git plumbing, blocks Node TCP/HTTP/DNS/UDP and non-Git child processes, and exercises both successful and failed local ref resolution.
Accepted — duplicate --changed coverage. Added a usage-error case.
Accepted — changed-path union. The implementation now unions merge-base-to-HEAD, staged, unstaged, and untracked paths, with cancelling-change coverage.
Accepted — ambiguous-ref handling. Ambiguity is rejected at rev-parse; benign stderr from later successful Git commands is allowed after the round-2 CRLF regression.
Deferred — batch baseline object reads. Worthwhile large-repository optimization, not a correctness condition for this flag.
Deferred — structural requirement IDs on violations. Useful hardening, but the existing producers and regression tests pin current parsing behavior.
Accepted — union semantics. Replaced the net-only diff with committed/staged/unstaged/untracked union semantics.
Accepted — baseline symlinks. Directly read baseline symlinks are reconstructed safely, with an explicit readable-config test.
Accepted — non-enforced annotation evidence. Affected-requirement hashing now uses annotation blocks independently of review eligibility.
Accepted — historical verdict isolation. Only current or baseline review IDs affect a requirement; unassignable malformed verdicts remain standalone violations.
Accepted — the six corresponding test-honesty gaps. Added partial-clone/no-network, cancelling-change, symlink, non-enforced annotation, historical-verdict, and config-only discovery probes, with scoped negative controls.
Accepted — one Git execution helper and narrow CLI catch. Git environment/diagnostics are centralized and the CLI catch is typed.
Rejected — remove CheckContext.annotations. It is used by incremental evidence hashing, so it is not unused.
Deferred — exact-count strengthening. The new fixtures use requirement counts/negative controls where needed to prove the scoped ID, without broad churn to unrelated tests.
Deferred — temporary-repository cleanup. Test hygiene follow-up, not a product defect.
Rejected — reject every unsupported check flag in this PR. The approved scope is the single --changed flag; broad CLI argument-policy changes need their own spec.
Deferred — batch baseline object reads. Performance follow-up as above.
Round 2 — Fable 5
Accepted — benign Git stderr warnings. Only ref resolution treats successful stderr as ambiguity; a real CRLF/autocrlf warning now remains successful.
Accepted — ignored evidence asymmetry. Incremental mode now fails explicitly when an ignored requirement dependency cannot be compared to Git, naming the path and remedy.
Accepted — duplicate document IDs could escape. Cross-file duplicate violations are emitted for both implicated specs, so changing either side is visible.
Deferred — central evidence-dependency helper. Good maintainability follow-up; behavioral regression tests now pin both dependency models.
Deferred — cat-file --batch. Performance optimization, outside this correctness fix.
Deferred — lazy allReviewTargets. General hook-performance optimization, outside the flag's behavior.
Deferred — structural requirementId on violations. Future robustness refactor, not needed to close a demonstrated escape.
Accepted — direct deletion-retention probes. Verdict deletion is now tested in committed, staged, and unstaged states.
No action — Node blocker was called decorative. It was harmless, and subsequent fresh-context judgment strengthened it into an active DNS/UDP/non-Git-process guard.
Deferred — shallow-clone/submodule documentation. The diagnostics are already fail-closed; documentation can follow separately.
Round 2 — Sol 5.6
Accepted — duplicate-ID escape and missing mutant. Fixed the attribution and added the cross-file regression.
Accepted — deleting the only configless spec. Incremental initialization now considers both current and baseline contexts, allowing the dangling annotation diagnostic through.
Accepted — REQ-003.2.2 JSON honesty. Tests parse pass/fail verdicts, assert their fields/hash/timestamp, and prove init preserves an existing verdict.
Deferred — central evidence helper. Same maintainability follow-up as Fable's finding.
Deferred — batch object reads. Same performance follow-up as Fable's finding.
Rejected — exclude submodules. REQ-010.1.3 requires fail-closed behavior when baseline content cannot be read completely; silently excluding gitlinks would weaken that contract.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Plan
Task plan: panopticon://tasks/c8bf33d6ba6c4fe889633916186f32aa/artifacts/plan.md
Verification