d04592de58c039b9f9d36c0ce071e4f6dc85fe00
2 Commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
Tom Boucher
|
0f60266042 |
fix(#2723): reconcile emitted manifest families as a set, not a count (#2750)
* fix(#2723): reconcile emitted manifest families as a set, not a shared count EXPECTED_MANIFEST_COUNT was a single literal 19 asserted against both the baseline (built at the base ref) and the current tree (built at PR HEAD). Those sides legitimately differ by one family whenever a PR adds or removes a runtime, so no value satisfied both: 19 rejected the current side, 20 rejected the baseline side. Every runtime-adding PR was hard-blocked. Replace the shared literal with three independent signals - the derived family set, the recorded fixture set, and the families present at the base ref - reconciled as sets in both directions. A family may appear or vanish only when the diff plausibly touches the runtime registry, and the failure names the family rather than a count. An absolute floor catches the uniformly shrunken universe a same-count self-check passes vacuously. Found by tracing #2005 (Qoder runtime) through the gate during the ADR-2719 dual-run window. * fix(#2723): read the baseline family set from the ref, not HEAD's registry Review found three defects in the first cut. Blocker: baselineManifestsAtRef enumerated MANIFEST_FAMILIES, which is imported at module load and therefore describes PR HEAD. A runtime REMOVED by the PR is already absent from that list, so the base ref was never asked for it, the baseline silently omitted a family that genuinely existed, and the dropped-family check could never fire in production - while its unit tests passed, because they inject the baseline directly. Enumerate from the ref with git ls-tree instead. Also: narrow the registry-signal set to the two surfaces that actually define the family set, since every extra path widens what excuses an unattributed delta; drop the ack bypass, which was a one-sided escape hatch making removals easier to wave through than additions; and gate the derived/fixtures inputs so malformed values return a verdict rather than an unhandled TypeError. * fix(#2723): filter prototype-shaped family names read from git output baselineFamilyNamesAtRef derives object keys from git ls-tree output rather than a trusted constant, so a fixture committed as __proto__.json would turn the manifests[name] assignment into a prototype write. Compared inline rather than through a Set, which is the form the prototype-pollution analysis recognizes. * fix(#2723): require an exact capability path depth and make the ref test hermetic The remote runner went red on both linux lanes with two real defects. The capability signal matched by prefix+suffix, so 'capabilities/capability.json' (no runtime segment) and 'capabilities/a/b/capability.json' (wrong depth) both attributed a family change and would have excused an unattributed delta. Anchored to an exact single-segment pattern. The ref-derivation test reached for this repo's root commit, which is not stable: the remote runner shallow-clones, so rev-list --max-parents=0 returns the grafted boundary carrying every fixture, and this repo has two root commits locally anyway. It now builds its own git repo containing a family absent from the current registry - the real discriminator, and one the root-commit version could never assert. Lint then caught a third: the test called t.after() without declaring t. * fix(#2723): stop asserting ref enumeration against the ambient checkout The remote runner returned [] for the repo's own HEAD while every hermetic temp-repo assertion in the same test passed. That is this function's documented behavior when git cannot read the ref - the runner works from a shallow clone under a bind-mounted workdir - so the assertion was testing the checkout rather than the code. Dropped it. The temp repo already proves the property that matters, and proves it more strongly: it contains a family absent from the current registry, which a registry-derived implementation could never report. The ambient path stays covered by the real-tree test, which skips explicitly when no base ref is resolvable. A git failure is not silently permissive downstream: baselineManifestsAtRef returns null on an empty family set and the real-tree test asserts the baseline is non-empty. |
||
|
Tom Boucher
|
9138271b5f |
test(#2723): differential emitted-attribution check, dual-run beside the golden (#2737)
* test(#2723): differential emitted-attribution check, dual-run beside the golden Phase 3 of #2719. The conservation law itself, running BESIDE golden-install-parity.test.cjs -- both green, fixtures untouched. Every emitted path whose hash moved between next HEAD and PR HEAD must be attributable, through the Phase 2 table, to a path the PR actually changed. Unattributable deltas fail with the paths NAMED. The only way through is a committed acknowledgment, never a flag -- a contributor facing a red gate sets a flag, which is what UPDATE_GOLDEN=1 is today. The central decision is that the law is a PURE function (no fs, git, installer, or clock), with I/O confined to a separate resolver. The naive one-big-integration-test shape would need ~38 installer spawns per assertion, so #2723's four failing-first criteria would not in practice have been written -- which is exactly how a phase ships promised-but-not-built. Pure, they are millisecond table tests, and the Stryker gate can actually bite. Buckets are conserved: every moved path lands in exactly one of attributed | unattributable | acked, property-tested at 400 runs. A path the provenance table cannot resolve surfaces as an error, never a silent skip. Asymmetries that are deliberate, each with a test: - an ADDED emitted key is a ripple too, not just a modified one - synthesized paths are exempt; code-derived ones are NOT (Phase 2 refused to mark them exempt precisely because exempt means permanently blind) - shrinkage needs no ack; growth does. Gating shrinkage would punish exactly what the size ratchet wants - a STALE ack is a hard failure -- an ack outliving its ripple pre-clears the next one on that path - a failed `git diff` is an explicit error, never an empty changedPaths set; reading it as "nothing changed" would make everything unattributable and produce a failure storm that reads like a real finding - prefix sources are SEGMENT-aware, so `agents/` does not attribute `agentsfoo/x.md` Baseline is cached, not committed, keyed on the next sha. A stale key is refused rather than used: absence fails loudly and gets fixed, whereas staleness produces a confident wrong answer. An explicitly pointed-at GSD_EMITTED_BASELINE that is stale is a hard stop; a stale cache falls through to the in-job build. No baseline-unavailable path returns -- ADR-2719 section 6 names that trap, since in node:test a bare return is a PASS. Both the conservation property and the staleness gate were mutation-verified (injecting a swallowed key fails 9 tests; disabling the staleness comparison fails 5). Fixtures, generators, the merge driver and the ADR status are untouched -- those are Phase 4 (#2724). Refs #2719 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01W5kQs6ZufZDySC6zDJfYP6 * test(#2723): compute stale acks once, after the size pass Self-review defect found while the reviewers were running. `staleAcks` was computed twice: once between the hash pass and the size pass, then again after. Only the second value was returned, so the first was dead code -- and the dead one was placed where it would have been WRONG. An acknowledgment can be consumed by either a hash move or a size growth. Computing staleness before the size pass reports a legitimate growth ack as stale, which is a false failure that pushes a contributor to delete the very ack that is doing its job. Now computed once, after both passes, with a regression test. Verified by mutation: restoring the early computation fails 2 tests. Refs #2719 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01W5kQs6ZufZDySC6zDJfYP6 * test(#2723): wire the attribution check to the real tree, not just synthetic input An isolated reviewer caught that the first cut was INTERFACE-ONLY: nothing read the ack file from disk, nothing shelled git, nothing built real manifests. Every test was true of hand-built inputs and none of the repo, so the acceptance criterion "both this check and golden-install-parity green on the same tree" was trivially true rather than meaningfully true. That is the promised-but-not-built failure this epic keeps finding in its predecessors, recurring one phase later for the wiring itself. Taken, not argued. Adds tests/helpers/emitted-runtime.cjs -- the only module that touches git, disk, or the installer -- and an integration test that runs the same pure law against reality: - CURRENT side: 19 real installer spawns via runMinimalInstall + buildParityManifest, the same machinery the golden harness uses. - BASELINE side: `git show origin/next:<fixture>`. That is next's RECORDED emitted state and it costs nothing. Deliberately NOT the working-tree fixtures, which are whatever this PR's author regenerated -- comparing against those would be vacuous. Phase 4 deletes the fixtures and swaps in resolveBaseline's cache path, already implemented and tested. - changed paths from real `git diff --name-only origin/next...HEAD`, with the git subprocess bounded at 30s per CLAUDE.md's unbounded-subprocess rule. - the real tests/emitted-drift-ack.json (absent is legal; present-but-empty or unparseable throws rather than being read as absent). Verified it can actually fail: an uncommitted edit to a shipped workflow moves emitted output but never appears in the committed diff, and the check names all 18 affected emitted paths with the message format ADR-2719 §1 specifies. Restores clean. Also from review: - readAckFile now has a real test exercising the SUT across absent / valid / empty / unparseable / unreadable. The previous test asserted fs behaviour rather than SUT behaviour, because no SUT ack-reading path existed yet. - formatReport's sampleLimit gains true limit-1/limit/limit+1 coverage at 19/20/21. A test was previously NAMED "(limit+1)" while testing no numeric limit at all, which is worse than no coverage because it reads as covered. Windows uses an explicit t.skip (install output is platform-specific there, mirroring the golden harness) -- never a bare return, which node:test scores as a PASS. Refs #2719 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01W5kQs6ZufZDySC6zDJfYP6 * test(#2723): cover the claude-local manifest family in the real-tree check Isolated adversarial review, MAJOR. The real-tree wiring enumerated Object.keys(RUNTIME_META) -- 18 entries -- while the emitted manifest set has 19 families. The 19th is claude-local: claude is the reference host and the only runtime with a distinct LOCAL "legacy flat-commands" layout (commands/gsd-*.md + agents/gsd-*.md at project scope), which golden-install-parity.test.cjs guards with a hand-coded test outside its RUNTIME_META loop (#2086). The family was dropped from BOTH sides, so the test's own self-check (current.length === baseline.length) passed vacuously at 18 === 18. A PR changing Claude's local-scope output would have failed the golden while this check reported ok -- and that disagreement is precisely what the dual-run window is designed to surface as a provenance-table hole. A wiring omission masquerading as one is the worst available failure here, because it would have been read as evidence about Phase 2 rather than a bug in Phase 3. Fixed by deriving MANIFEST_FAMILIES explicitly (18 global + claude-local at local scope) instead of inferring the set from RUNTIME_META. The self-check is also repaired: it now asserts both sides against the INDEPENDENT EXPECTED_MANIFEST_COUNT from the Phase 2 table, and asserts claude-local specifically. Comparing the two sides to each other can never catch a family missing from both -- the assertion has to come from outside. Verified by mutation: removing claude-local again fails the test. Also from the same review: - sourceSatisfiedBy returns the matched source string, so an empty-string source would return '' and the caller's `if (hit)` would silently discard a real match. Unreachable today (every rule source is a non-empty template) but a footgun for the next rule author; now `!== null`. - the purity fixture used a single-element changedPaths array, so an in-place sort would have been invisible. Now three elements in unsorted order. Refs #2719 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01W5kQs6ZufZDySC6zDJfYP6 * fix(#2723): resolve the base ref tolerantly instead of hard-requiring origin/next The first matrix run failed on both linux lanes: differential attribution over the real tree cannot resolve origin/next (Command failed: git rev-parse origin/next) Not a flake, and not an environment excuse -- a real defect in this diff. The gsd-test runner shallow-clones and merges base+head, so no origin/* remote- tracking refs exist in the container. My own fail-loud path fired correctly; what was wrong was hard-depending on that ref existing. GitHub Actions has the same shape by default, which is exactly why changeset-required.yml carries an explicit `git fetch origin "${BASE_REF}:refs/remotes/origin/${BASE_REF}"`. Now resolved through an ordered candidate list -- GSD_EMITTED_BASE (explicit lane override), then origin/$GITHUB_BASE_REF and $GITHUB_BASE_REF, then origin/next and next -- de-duplicated, each verified with `rev-parse --verify <ref>^{commit}`. When NO candidate resolves the test takes an explicit t.skip() naming every ref it tried and stating that the gate did not run here. That is the ADR-2719 section 6 distinction: t.skip is REPORTED as skipped, whereas a bare return is scored as a PASS. Hard-failing was the other option and is wrong -- it would make the suite permanently red wherever a base ref cannot exist by construction, which is a statement about the checkout, not a propagation finding. The candidate ordering is pinned by a unit test rather than left implicit, since the ordering IS the fix. Refs #2719 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01W5kQs6ZufZDySC6zDJfYP6 --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com> |