5869febb16620837083fa5596bc85db7841cf32d
14 Commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
Dennis Alexis Valin Dittrich
|
5869febb16 |
enhance(#4155): invalidate verification results when covered inputs change (#4290)
* enhance(#4155): invalidate verification results when covered inputs change readVerificationStatus() now recomputes a deterministic sha256 fingerprint over a VERIFICATION.md's declared covered_files (phase PLAN/SUMMARY, requirements, implementation files in the verified change set) and returns stale on any mismatch, fail-closed when a covered file is missing, unreadable, or escapes the project root. Legacy reports with no fingerprint metadata keep the prior SUMMARY-mtime staleness check unchanged. The verifier computes covered_digest via the new verification.fingerprint CLI command rather than by hand, since a digest is deterministic math, not an LLM-estimated value. * chore(#4155): backfill fork PR number in changeset * fix(#4155): trim gsd-verifier.md fingerprint instructions to fit LARGE tier byte cap * fix(#4155): address CodeRabbit findings on fingerprint fail-closed behavior Partial fingerprint metadata (one of covered_files/covered_digest present, the other missing or malformed) now fails closed to stale instead of silently downgrading to the legacy mtime-only check. computeCoveredDigest also canonicalizes with realpathSync before re-confining, so an in-root symlink whose target escapes the project root can no longer produce a matching digest. gsd-verifier.md restores the completeness requirement and checklist item trimmed by the earlier size-budget fix, within the LARGE tier byte cap. * chore(#4155): acknowledge gsd-verifier.md growth for the #4155 fingerprint instructions Emitted-Drift-Ack-Growth: gsd-verifier.md — adds the covered-input fingerprint instructions and frontmatter fields the #4155 verification staleness mechanism requires; trimmed to stay within the LARGE tier byte cap * fix(#4155): address gemini adversarial review findings computeCoveredDigest now threads the caller-supplied opts.fs seam through its confinement and read paths instead of always using raw node:fs — a caller like planning-inspect.cts's containmentEnforcingVerificationFs (GAP 2, #2790 follow-up) was silently bypassed for covered-input reads. The project-root anchor itself still canonicalizes through real fs (it is a trusted value the caller derived, not attacker-influenced covered-input data); only per-file candidate reads go through the injected seam. Covered-file paths are now canonicalized (./ prefixes, redundant slashes, internal .. segments) before becoming dedup/sort/hash keys or confinement subjects — closes both a spurious-stale false positive (two spellings of the same file hashing differently) and a confinement gap (an internal .. segment that doesn't start the string). gsd-verifier.md now states covered-file paths are project-root-relative, not phaseDir-relative, closing an ambiguity that would have made a real verifier agent's first fingerprint invocation fail closed. defaultFsImpl's methods now late-bind through fs.<method> rather than capturing function references at module load — the earlier direct-capture form was invisible to existing tests' t.mock.method(fs, 'statSync', ...) seams, a real regression caught by the full suite (not the reviewer). * fix(#4155): catch a plan/summary added to the phase dir after verification but never declared The content digest only recomputes hashes for paths the verifier actually declared in covered_files — it had no way to notice a plan or summary added to the phase directory after verification if that new file was never declared, silently regressing behind the legacy mtime check it replaces (which scans the live directory, not a declared list). findUncoveredCurrentArtifact re-scans the live phase directory for every current *-PLAN.md/*-SUMMARY.md and requires each to be represented in covered_files, closing that gap; a directory scan failure fails closed to stale rather than silently skipping the check. CONTEXT.md's Verification Module entry corrected to describe the fingerprint path's stricter fail-closed FS-error contract (routes to stale) instead of the module's original degrade-to-safe one (missing / not-stale), which only the legacy path still keeps. * refactor(#4155): extract canonicalizeCoveredFiles, add real nested-project e2e test computeCoveredDigest and cmdVerificationFingerprint each normalized/deduped/ sorted covered_files independently — one shared helper now backs both (gemini review's ponytail-lens finding). Adds one CLI-to-readVerificationStatus test against a genuine .planning/phases/NN-x/ project with an implementation file outside .planning/ entirely, closing the review finding that prior #4155 unit fixtures put phaseDir directly under an ownerless tmpdir (findProjectRoot falls back to phaseDir itself there) and never exercised real multi-level path resolution. * fix(#4155): route computeCoveredDigest through real fs, fail closed on unreadable plans/ Two independent review rounds (opus critical-reviewer + opus ponytail + agy, run twice) found two instances of the same fail-open class: - computeCoveredDigest's per-file reads routed through the caller's injected fsImpl. planning-inspect.cts passes a `.planning/`-confined containment fs into readVerificationStatus's opts.fs, so any covered implementation file outside `.planning/` (mandatory per the issue) made the confinement wrapper throw, which was caught and turned into a stale digest -- reporting every fingerprinted phase permanently stale via `planning.inspect`, regardless of actual drift. Per-file reads now always use real node:fs, matching the pre-existing treatment of root canonicalization; the realRel-vs-realRoot check is the real confinement boundary for this data and needs no seam. - allCurrentArtifactsCovered's try/catch never fired (scanPhasePlans reports readdir failures via a `scope` field, it never throws), so an unreadable nested plans/ dir was silently treated as "zero artifacts, all covered" instead of failing closed. Now branches on scope !== SCOPE.COMPLETE. Also, per ponytail's second-round findings: reverted an unwarranted FINGERPRINT_VERSION bump and digest length-prefix from the first fix (no v1 digest has ever existed -- the feature is unreleased -- and the prefix closed a collision that grants no capability beyond what a writer of covered_files already has more cheaply); removed a verifier-facing escape-hatch instruction whose own example was a case that should trigger staleness, not bypass it; corrected CONTEXT.md references to the renamed allCurrentArtifactsCovered and a stale "unconditional" rescan claim; simplified the isStale derivation, removed dead FsLike members, and tightened test coverage. Regression tests for both fail-open bugs are included and were each confirmed to fail against the pre-fix code before the fix landed. full test suite: 2558/2560 pass, 2 skipped, 0 fail * fix(#4155): trim gsd-verifier.md under the LARGE size cap Fork CI caught what my local runs missed: the superseded/nested-plans instruction added earlier pushed gsd-verifier.md to 49299 bytes, 147 over the LARGE tier's 49152-byte hard cap (tests/agent-size-budget.test.cjs). Tightened the #4155 instruction's wording and dropped a redundant inline comment tag; no content lost. * chore(#4155): point changeset at the upstream PR number pr: 19 was the fork PR opened for internal review-lane CI; now that open-gsd/gsd-core#4290 exists, the changeset field must match it per CONTRIBUTING.md's release-notes convention. --------- Co-authored-by: Test <test@test.com> Co-authored-by: Tom Boucher <trekkie@nomorestars.com> |
||
|
Tom Boucher
|
4e8927b0b9 |
fix(#3707): degrade the fold for every UAT gap class, and stop line endings hiding rows from the audit and the acceptance gate (#3903)
* test(#3707): failing-first coverage for reverting the fence-shortfall fold shield Pins the post-revert contract: a phase whose only gap is a fence shortfall must degrade the fold and withhold the milestone percentages, like every other gap class. Five of the eight rows are CONTROLS that pass before the change, and they carry more weight than the failing row. The failure mode of this revert is degrading TOO MUCH: a revert that sets foldScope outside the headingsSeen > 0 branch would withhold every percentage in the project, and only the no-gap control catches that. Another control catches a revert that collapses the two scopes into one and loses the distinction between what a phase reports and what the fold folds -- uat.scope must stay TRUNCATED for every gap either way, which it already is. The row that pinned the shielded behavior is rewritten rather than deleted. Deleting a test because the behavior it asserts is being reversed leaves the reversal unguarded. * fix(#3707): degrade the fold for every UAT gap class, reverting the fence-shortfall shield Maintainer decision. The two orthogonal engines split on this during #3707 and neither filed it as blocking, so it shipped in the shape the engine that raised the objection endorsed after verifying seven fixtures. The call has now gone the other way, restoring the fail-safe direction chosen twice already on this issue. The shield exempted one gap class from the fold's teeth. It could not do that safely: shortfallBlocks is a single tally incremented at exactly one site and spans BOTH a harmless fenced documentation sample AND a genuinely fence-straddled result: blocked row. Exempting it therefore could not exempt only the harmless case -- it also published a milestone percentage over a real, unread outstanding row. SCOPE.TRUNCATED means the scan could not SEE part of the evidence, which is exactly that case. scope and foldScope now agree: every gap class degrades both. The accepted over-report documented in uat.cts is unchanged and still documented there; what changed is only that it no longer buys an exemption from the fold. The comment block above it argued FOR the shield and is rewritten, because a comment defending behavior the code no longer has is worse than no comment. shortfallBlocks leaves this function's destructure but is untouched upstream, where audit-uat still consumes it. * fix(#3707): correct the caller comment, add the changeset, and name what the order tests guard Review found a SECOND comment still documenting the removed shield -- the caller's, beside the worstScope fold, stating that foldScope differs from scope for exactly one case which must not raise phase_scope_degraded or withhold the milestone's percentages. That is now the opposite of what the code does. I rewrote the buildUatRows comment in the previous commit and asserted in its message that a comment defending behavior the code no longer has is worse than no comment, then left exactly that one standing a few hundred lines away. The change had no changeset. It is user-visible: a milestone's percentage goes from published to withheld whenever any phase has a fence-shortfall-only gap. PR gates hard-fail a user-facing code diff without one. The two scopes are now identical at every return site. They are NOT collapsed -- that would change the return shape and the caller on what is meant to be a one-condition revert, and the seam is worth keeping if the distinction is ever wanted again -- but the declaration now says plainly that they agree by decision rather than by accident, so a reader does not have to re-derive it. The two order-independence tests were renamed. foldScope is monotonic with no reset path, so file order is structurally irrelevant and those rows could never have failed for the ordering reason their names promised. They do guard something real -- a multi-file phase degrading when any one file has a shortfall-only gap -- so they now say that instead. * test(#3707): failing-first coverage for the lone-CR UAT false-clean The parser splits on newline only, and the heading tokenizer agrees with it, so a lone carriage return is not a line boundary anywhere in it. CommonMark treats a lone CR as a line ending, so such a row renders to a human reader while being invisible to BOTH sides of the parser's symmetry invariant: no item, no shortfall, no headingsSeen. A phase hiding a result: blocked row this way reports 100 percent with zero diagnostics. Found by the security review of the fold-shield revert. It is the one false-clean class that revert does not reach, and it is the same bug class this issue exists to fix -- an unreadable row reported as clean. Nine rows. The LF control is what proves this is a separator defect rather than a content defect: identical bodies, one separator apart, and only one of them hides the row. CRLF and CR-inside-a-fence controls guard the coming normalization against double-counting or tearing content that legitimately contains a carriage return. Two further manifestations turned up while writing them: a leading CR breaks column-0 anchoring of the first heading, and an all-CR document flags a shortfall it cannot attribute to any row. * fix(#3707): treat a lone carriage return as a line ending in the UAT parser A lone CR was not a line boundary anywhere in the parser -- it split on newline only, and the heading tokenizer agreed with it. CommonMark treats a lone CR as a line ending, so such a row rendered to a human reader while being invisible to BOTH sides of the parser's symmetry invariant: no item, no shortfall, no headingsSeen. A phase hiding a result: blocked row that way reported 100 percent with zero diagnostics. Line endings are now normalized once at document ingress -- CRLF and lone CR both to newline -- at the two independent entry points, rather than teaching each split site about CR. Every downstream scan, offset and span therefore reads one convention. That single-frame property is deliberate: this issue already cost a HIGH when two scans read the same document through different frames. MY OWN END-TO-END TEST WAS WRONG and is replaced rather than weakened. It asserted that a lone-CR document must withhold its percentage, which reasons from the pre-fix symptom: after the fix the row is not hidden, it is surfaced, and this module deliberately keeps visible outstanding UAT work separate from completion percentages -- only unreadable evidence degrades scope. The success of the fix is what made the assertion false. The implementing agent refused to satisfy both it and the architecture and asked instead of bending either; it was right. What replaces it is a stronger contract: a lone-CR document and its LF twin, built from one source, must produce identical audit output -- scope, percent, every unresolved row by identity, and the diagnostic set. That is what 'a line-ending convention must not change what the audit reports' actually means, and it carries a non-vacuity check so it cannot pass with both sides empty. shortfallBlocks keeps being returned, now documented as currently unconsumed. An earlier reviewer told me audit-uat still consumed it and I passed that on as an instruction; it was wrong, and it was caught by checking rather than by me. * fix(#3707): normalize at the document read boundary, not at two call sites The lone-CR fix was half-applied and both review engines caught it independently. cmdAuditUat has four document ingresses, not the two I normalized: VERIFICATION.md and deferred-items.md still handed raw text to newline-only splitters, and the frontmatter extract in the UAT loop read raw content while its parser read normalized -- one audit entry mixing the two frames the fix exists to unify. Measured: a phase written twice from one source gave total_files 2 / total_items 4 under LF and results [] / total_items 0 under lone CR, with zero diagnostics. Normalizing two call sites and declaring it done is exactly why two were missed, so this moves it to the read boundary: every document now enters through a helper that normalizes, in audit-uat, in planning-inspect's readDocument, and in the shared verification-status read. Future parsers downstream get normalized text by construction rather than because someone remembered. That last seam also fixes an under-reporting case of the same root: a lone-CR VERIFICATION.md saying status: passed was read as missing, telling the user a verify step that had completed never ran. The parity test's load-bearing assertion is now marked as such. Four of its five equality checks still pass with the bug present -- only the unresolved-row identity differs -- so trimming that one as redundant would make the row vacuous. Second changeset added: the CR fix is user-visible independently of the fold revert, and one fragment covering both would have described neither. * test(#3707): failing-first coverage for the U+2028 and duplicate-result false-cleans Two more of the same class, both found by the security review of this branch and both reproduced before writing a line. normalizeLineEndings folds only carriage returns, but a JS /m anchor also treats U+2028 and U+2029 as line terminators while split on newline does not. That is the identical asymmetry the carriage-return bug exploited, one separator over, and worse in one respect: these are not CommonMark line endings, so a reader still sees the column-0 result: blocked that the tool discards. Measured: a scalar-internal result: pass placed after U+2028 wins over the real blocked line and the row disappears with no gap raised. Separately, and independent of any separator, a block with two column-0 result: lines resolves to the first with no ambiguity signalled. Prepending result: pass to a block therefore deletes an outstanding row silently; reversing the order surfaces it. Order deciding meaning is the defect, so the pair of rows pins the contract as ambiguity-is-a-gap rather than last-one-wins, leaving the fix room to implement the gap sensibly. Four controls: an ordinary marker in the same position (proving separator not content), legitimate U+2028 inside prose that must not be torn, a single result line, and a result line inside a fence that must not count as a second occurrence. * fix(#3707): scan result lines by split, not by a multiline anchor Two more false-cleans from the security review, both closed by the same change. A JS /m anchor treats U+2028 and U+2029 as line terminators while split on newline does not. A scalar-internal result: pass placed after one of those separators therefore matched as a line start and beat the real column-0 result: blocked, and the row vanished at 100 percent with no gap. Worse than the carriage-return case in one respect: these are not CommonMark line endings, so a reader still saw the blocked row the tool discarded. Separately, the non-global match returned the leftmost hit, so a block with two column-0 result: lines silently resolved to the first. Prepending result: pass deleted an outstanding row; reversing the order surfaced it. Order deciding meaning was the defect. Both close by scanning lines produced by split rather than by anchoring a regex inside the whole document: each line is tested on its own, and a count other than exactly one is reported as a parse gap instead of resolved to either candidate. I asked for U+2028 to be folded in normalizeLineEndings and that was wrong. Folding is length-preserving, so it would have made the U+2028 fixture byte-identical to the genuine two-result-line fixture -- while one requires a confident item and the other requires an ambiguity gap. No implementation can satisfy both once the distinguishing character is erased. The agent proved that and deviated rather than forcing it, which is why normalizeLineEndings still folds only carriage returns, now with a comment saying why. * fix(#3707): bound the ambiguity scan at the next heading-shaped line The split-based result scan regressed four pre-existing #3078/#3707 guards, each off by exactly one gap. My diagnosis was wrong. I read the off-by-one as double counting -- zero-result blocks taking both the new path and the pre-existing one -- and said to change the ambiguity condition from not-equal-one to greater-than-one. The agent checked and refused: the zero path was never duplicated. The real cause is double ATTRIBUTION. A block is sliced to the next TOKENIZED heading, so when the next row is untokenized -- hidden by a straddling fence, or indented and already counted by the shortfall scan -- that row's own result: line is absorbed into the previous block. The scan then saw two result lines across what are really two rows and raised a second, redundant gap on top of the one already counted elsewhere. Had the greater-than-one change gone in, the counts would have matched while the double attribution stayed. That is the compensating-adjustment failure I had asked it to refuse, and it did. The scan is now bounded at the first following heading-shaped line, either indent class, so a genuine same-block ambiguity is untouched while spillover from a row counted elsewhere is excluded. * fix(#3707): keep the U+2028 immunity, revert the ambiguity detection The ambiguity half of this change regressed the suite twice and is coming out. Attempt one double-attributed: a block is sliced to the next TOKENIZED heading, so when the real next row is untokenized its result: line was absorbed into the previous block and raised a second gap on a row already counted elsewhere. Four guards broke. Attempt two bounded the scan at the next heading-shaped line and broke thirty. An indented ### N. inside a block scalar is legitimate scalar CONTENT, not a heading, and truncating there defeats every #3078 guard that exists to stop scalar bodies being read as rows. Telling a genuinely hidden indented row apart from indented scalar text is a classification countUnattributedIndentedRows already owns; a raw regex does not have that information. What survives is the half that is sound and was never implicated in either regression: the result scan tests each line produced by split rather than anchoring a regex with the multiline flag over the whole block. split never treats U+2028 or U+2029 as a delimiter, so those separators can no longer manufacture a line start and steal a row. Everything else returns to first-match-wins, byte-identical to origin/next. The two tests pinning ambiguity-as-a-gap are removed with it, since the contract is no longer implemented here. The defect they described is real, pre-existing and independent of any separator -- result: pass before result: blocked silently deletes an outstanding row -- and it needs its own change with a scalar-aware counter rather than being wedged into a branch already carrying three fixes. * fix(#3707): correct the shared-seam rationale and restore U+2028 trailing text The revert left a stale rationale in core-utils, justifying the decision not to fold U+2028 by claiming uat.cts must tell a fake line start apart from a real second column-0 result: declaration that gets flagged as ambiguous. Nothing flags ambiguity any more; that behavior was reverted and the same file says so a few lines away. The decision is still right, the stated reason was false. This is the third stale comment this branch has shipped and had to fix, and the worst placed of them: core-utils is a shared leaf that every future document consumer will read for guidance. Rewritten to the true reason -- the scan tests each split line individually rather than anchoring over the block, so an exotic separator cannot manufacture a line start and folding is unnecessary. Also a real behavior delta I had not noticed. Dropping the multiline flag left the pattern's trailing .*$ in place, and dot never matches U+2028, so a genuine column-0 result: blocked whose TRAILING text contained one stopped parsing entirely -- a visible parse gap rather than a false clean, so fail-safe, but a regression against origin/next that nothing pinned. The trailing portion now matches any character and a test pins it by identity against its plain-LF twin. Plus the JSDoc orphaned when normalizeLineEndings moved to core-utils, and the changeset, which described neither the separator fix nor planning-inspect surfacing lone-CR rows. * fix(#3707): harden the acceptance gate, which had both halves of the same bug uat-predicate is a SECOND, independent UAT parser, and it is the one that decides phase uat-passed. It read raw bytes and anchored a multiline regex over unsplit text -- exactly the two defects this branch closed one module away in uat.cts. The consequence is worse than the audit surface it mirrors. Measured on identical bytes: a U+2028 scalar injection made the gate return passed true while planning inspect reported the same row as blocked and outstanding. The hardened surface and the gate disagreed, and the gate was the permissive one -- so a phase could be accepted over a row the audit could see and the gate could not. Both raw reads now go through the shared normalize seam and both scans test lines produced by split rather than anchoring over the document. First-match-wins, matching uat.cts; no ambiguity counting is reintroduced. Tests assert the AGREEMENT between the two surfaces rather than each separately, because divergence is the defect. Also finishes the same root cause one module over: phase complete's advisory pre-scan read raw bytes, so a lone-CR VERIFICATION.md lost its human_needed or gaps_found warning -- the fix verification.cts already got on this branch. And narrows the core-utils rationale I reworded last commit, which claimed consumers already avoid multiline anchors. uat.cts still has five over unsplit text. That is the fourth comment on this branch to assert something the code does not do, so it now states only what is true of core-utils itself. * fix(#3707): give structure and attribution different line frames, normalize the close audit Two more from review, and the first was a regression I introduced one commit earlier. Converting the gate's heading scan to split-then-match removed a detection origin/next had: a ### N. heading delimited by U+2028 was found by the old multiline scan and was not found after. So hardening the result scan quietly weakened the heading scan, and the gate stopped blocking on rows origin/next blocked -- the permissive direction, on the surface that decides acceptance. The insight I had missed is that the two scans need DIFFERENT frames. Heading detection is structure: there is no distinction to preserve, so it splits on newline or either exotic separator and finds a heading however it is delimited. The result scan is attribution: the newline-only frame is exactly what stops a scalar-internal result: from being read as a column-0 line, so it stays. One frame applied uniformly was the error. Second, a THIRD unnormalized parser family: the milestone-close audit read every artifact raw. A lone-CR VERIFICATION.md degraded to status unknown and was skipped, and deferred entries vanished outright -- measured as three items requiring decisions under LF and one under CR, on identical bytes. All nine scanner reads now normalize; six of them had the identical defect beyond the three review named. The acknowledge path stays deliberately raw, since it splices by byte offset, and now says so. Also pins the cross-newline result: divergence, and replaces three raw U+2028 literals in test source with escapes. A raw separator in a fixture is one formatter away from becoming an ordinary-character control that still passes -- vacuous in the only test pinning the separator fix. * fix(#3707): share one frame between the acknowledge writer and the audit reader Normalizing the audit scanners left the writer and the reader on different frames. cmdAuditAcknowledge derives its stored snapshot values from raw content -- correct for the SPLICE, which rewrites by byte offset -- but scanUatGaps and scanContextQuestions now recompute those same values from normalized content. For a lone-CR artifact the two can never match, so an acknowledgement never suppresses its item and it resurfaces on every audit: acknowledge became a silent no-op. Fail-safe in direction, since the item stays visible rather than being wrongly suppressed, but it is the writer and reader disagreeing about what a line is -- the exact class this branch exists to eliminate, and the fourth instance of it here. The derive functions now read a normalized copy while the splice keeps raw bytes and raw offsets, so both sides share one frame and the byte-offset rewrite is untouched. Round trip pinned for lone-CR and LF, with an existing LF marker asserted still recognised so the change cannot silently invalidate acknowledgements already in users' files. Also tightens an assertion that pinned this branch's own heading fix with a proxy: notStrictEqual against 'passed' also passes on 'pass', which IS a passing token, so it could not have caught a regression attributing a passing result to the recovered heading. It now pins the exact token. * chore(#3707): backfill changeset pr numbers Both fragments still carried the pr: 0 placeholder, which failed changeset-lint and docs-lint on PR 3903. The review had flagged the backfill as pending and I opened the PR without doing it. --------- Co-authored-by: sim <sim@local> |
||
|
Tom Boucher
|
59e7a677fe | fix(#3511): scope every phase-directory scan to the phase it belongs to (#3535) | ||
|
Tom Boucher
|
3893d1ff69 |
fix(#3518): pin uat_path to the phase's own UAT artifact via the shared phase-pinned resolver (#3525)
* fix(#3518): pin uat_path to the phase's own UAT artifact via the shared phase-pinned resolver Both uat_path projectors in src/init.cts picked the phase's UAT file with a bare .find() over unsorted readdir order — no phase-membership check, no ordering — so a stray cross-phase 04-UAT.md in phase 03's directory could become phase 03's uat_path, filesystem-dependently (creation order on APFS, hash order on ext4/XFS): two machines on the same commit could emit different uat_path values for the same phase. Route both sites through a new resolveUatFile in src/verification.cts, the UAT counterpart of #3357/#3492's resolveVerificationFile, sharing the exact selection rule via one extracted core (resolvePhaseArtifactFile): the phase's own <token>-UAT.md always wins; otherwise the alphabetically-first dashed candidate (deterministic everywhere); a bare UAT.md only via allowBare when no dashed candidate exists. resolveVerificationFile now delegates to the same core — behavior byte-identical. Guarded by: two end-to-end repro tests in tests/init.test.cjs (plan-phase and phase-op, red on the pre-fix readdir pick), resolveUatFile contract anchors and a src/-wide call-site guard in tests/verification-status.test.cjs. * chore(#3518): add changeset fragment for PR #3525 --------- Co-authored-by: sim <sim@local> |
||
|
Tom Boucher
|
ddf852873c |
fix(#3357): one phase-pinned resolver for verification-report discovery (#3513)
A phase directory can hold more than one `*-VERIFICATION.md` — an ad-hoc `03-CORRECTION-VERIFICATION.md` worksheet beside the real `03-VERIFICATION.md`. Discovery took the alphabetically-first match, so the worksheet won and the phase could report `missing` while a passing report sat next to it. The issue named two copies. There were seven, in four grammars: two `.sort()[0]` sites in the verification module, three `.find()` over UNSORTED readdir order (phase status, and `verification_path` twice — filesystem-dependent, so two machines on one commit could disagree), and two in shell. All seven now route through one exported `resolveVerificationFile`; the shell copies via a new `verification resolve-file` verb rather than hand-rolling the rule an eighth time. Fixing five of seven would have been worse than fixing none: the verify-work workflow is a WRITER that stamps `status: passed` onto the file it picks, so canonical-aware readers plus an alphabetical writer means the human_needed→passed canonicalization silently no-ops forever while the worksheet gets stamped. That divergence did not exist on next. The first resolver was itself a regression — it preferred ANY canonically-shaped name over the phase's own report, so a stray cross-phase or sentinel-numbered file outranked it. The global-canonical preference was removed rather than narrowed; the rule is pinned to the phase token via `PHASE_NUMBER_TOKEN_SOURCE`, its existing owner. Fixed in passing: the transition workflow's awk guarded on `NR==1` instead of `FNR==1`, so across a multi-file glob it armed only on the first file — a leading worksheet with no frontmatter blocked transition even when the canonical report passed. Also removed a U+00AD soft hyphen introduced earlier on this branch. Five broader-grammar AGGREGATE scans are deliberately out of scope — a different defect class (phase-unscoped scanning), tracked as #3511. Closes #3357 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> |
||
|
Tom Boucher
|
2537c286f9 |
refactor(#1762): clarify verification-missing/unknown routing text (#3439)
* test(#1762): assert reassuring verification-routing wording (RED) Regression test for #1762 — asserts readVerificationStatus's missing/ unknown next_action text reassures the user that execute-phase resumes at the verification gates without redoing work, instead of reading as a blind re-run instruction. Fails against current wording; the fix lands in the next commit. * fix(#1762): reassure verification-missing/unknown routing text is safe to run readVerificationStatus's 'missing' and 'unknown' next_action text read as "redo the implementation", when execute-phase's own discover_and_group_plans step (#2868) already resumes at the verification gates and skips execute_waves/checkpoint_handling entirely when every plan already has a SUMMARY.md. Reword both to say so explicitly, and soften the 'unknown' message to acknowledge a non-standard status may be an intentional marker rather than presuming re-verification is always the fix. Also updates progress.md's Route V.missing / V.unknown to consume the dynamic $VERIFICATION_NEXT_ACTION (matching V.gaps/V.human) instead of a hand-duplicated string, so the reassurance lands there too without a second copy to keep in sync. No change to next_command routing, status values, or the isPhaseComplete completeness predicate (still gated on status === 'passed' per the #2957 DISK-STRICT decision) — advisory text only. * fix(#1762): drop duplicated reassurance clause in progress.md routing text Review finding: the SUMMARY.md reassurance was stated once via the interpolated $VERIFICATION_NEXT_ACTION and again as a parenthetical on the command line, in both Route V.missing and Route V.unknown. Trimmed the parenthetical so $VERIFICATION_NEXT_ACTION stays the single source of truth. * docs(#1762): add changeset fragment Type Changed with a docs-exempt marker — advisory routing text only, no docs page documents this specific message today. * test(#1762): acknowledge progress.md emitted-content growth Route V.missing/V.unknown now render a small block referencing ${VERIFICATION_NEXT_ACTION} instead of a bare command line, growing progress.md by 376 bytes. Deliberate per the differential attribution check (ADR-2719). * chore: drop spent progress.md ack from #3218's emitted-drift fragment The #3218 growth it described is already on next (inert, per the lint's own note that spent acks 'can no longer clear anything'). Its presence collided with the new #1762 fragment naming the same bare filename, which lint-emitted-drift-ack rejects outright ('two ack sources may never name the same path'). #3218's other two acks (plan-phase.md, plan-review-convergence.md) are untouched. * docs(#1762): backfill changeset PR number --------- Co-authored-by: sim <sim@local> |
||
|
Tom Boucher
|
e201cde73c |
refactor(#3186): one shared phase-completion predicate, disk-strict (#3306)
* docs(#3186): record the disk-strict completion decision in ADR-3180 7.4 The maintainer decided #2957 on 2026-08-08: disk state is authoritative and a ROADMAP checkbox is a human annotation with no machine authority. Section 7.4 still carried the OPEN QUESTION and was marked blocked, so the contract said one thing and the tracker another. Recorded per section 7's own rule - a behavior not stated there is not decided, and amending a rule is an ADR amendment rather than a code change with a comment. The decision comment names Phase 4's PR as the carrier of this edit and makes it an acceptance criterion that the text be in the tree before implementation begins, so this lands first, alone, ahead of any code. Also clears the stale blocked-on-2957 row in the guard roster. * refactor(#3186): one shared phase-completion predicate, disk-strict isPhaseComplete in verification.cts becomes the single owner. It calls readVerificationStatus UNCONDITIONALLY - plan count is not a precondition - so a zero-plan phase with a passing VERIFICATION.md is complete. That is #3168: init gated the read on a plan count and synthesized a not_required sentinel, so phase.complete succeeded while init.manager reported incomplete for the same phase. The guard, built and run before scope was fixed per Amendment 3, found 9 re-derivations where the ADR named 3. Four were unnamed, including one in the prompt layer: mvp-phase.md ORed a ticked checkbox with disk status, which under disk-strict is the divergence itself. Per the #2957 decision, a ticked ROADMAP checkbox is a human annotation with no machine authority. The overrides in roadmap analyze and init manager are deleted rather than generalized; the user's checkbox stays in ROADMAP.md, only its authority goes. scanPhasePlans.completed and buildWorkstreamInventory are deliberately NOT folded - they answer 'are all plans summarized', which is a different question, and folding them would either over-report completion or invert the dependency direction between Phase 1's owner and this one. Verified on the remote runner. * fix(#3186): close seven review findings and record the missing-verdict rule The isolated review reproduced a write-path regression I introduced: migrating cmdRoadmapUpdatePlanProgress dropped its summaryCount>=planCount gate, so a phase with a fresh passing verification plus a newly-added unsummarized plan reported complete AND wrote a checkbox into ROADMAP.md while phase complete refused. The owner stays right per 7.4 - plan count is not a completion precondition - so the gate is restored at the write site as an explicit composition, mirroring the separate 2648 unexecuted-plan gate cmdPhaseComplete already carries. The spec axis was right that my 0.x-split reasoning was too permissive. The 2957 decision names buildStateFrontmatter as one of the three that must converge, and buildWorkstreamInventory combined a summaries-met local with verification data to decide the same verdict - Decision 4(c)'s named bypass, and it reproduced 3168 in a third surface. Both now route through the owner. The raw scanPhasePlans helper stays: it answers are-plans-summarized, which genuinely is a different question. Maintainer decision recorded in 7.4: a missing verdict is not a passing one, so an absent VERIFICATION.md means not complete everywhere. That retires 2645's verifier-disabled tolerance and inverts its Goodhart incentive - deleting the evidence now lowers completion instead of raising it. Guard hardened: block-form count gates and algebraic restatements are caught, and the header now discloses its remaining limits instead of overclaiming. Verified on the remote runner. * fix(#3186): route state sync through the owner and catch bare completed reads The matrix found 52 failures. 51 were fixtures asserting the old semantics: a phase with plans and summaries but no VERIFICATION.md used to count complete and correctly no longer does. Each fixture now carries a passing verification where that is what the test was actually about, rather than having its assertion weakened. The 52nd was a real 10th re-derivation the guard could not see. cmdStateSync destructured scanPhasePlans().completed directly - a bare field read, not a comparison - and used it as a completion verdict, so state sync and state json disagreed on completed_phases for identical disk state. Routed through the owner. Guard gains shape (d): any read of .completed off a scanPhasePlans() result outside plan-scan.cts, in chained, destructured and indirect forms, function scoped with no line window. It cannot tell a summaries-met read from a completion read - that is data flow - so it flags every one and requires a written-reason exemption, which is the same discipline shapes a-c already use. The blind spot is disclosed in the header rather than overclaimed. The emitted-attribution failure was also mine, not pre-existing: the mvp-phase.md checkbox-OR removal moves emitted bytes, acknowledged in tests/emitted-drift-acks. Verified on the remote runner. * test(#3186): give the nested-plans sync fixture a passing verification Last 3 matrix failures were one failure echoing up two describe levels. Phase 01-alpha had plans and summaries but no VERIFICATION.md, so under disk-strict completed stayed 0 and no Progress change was emitted - correct new behavior, not a regression. Added the passing verification rather than dropping the Progress expectation, so the test still covers what #3257 is about: that a nested plans/ layout is counted and not undercounted. Probe against the built lib confirms Progress: 0% -> 50% alongside Total Plans in Phase: 0 -> 3. * chore(#3186): backfill changeset PR number pr:0 placeholder replaced with the real number now that #3306 exists. --------- Co-authored-by: sim <sim@local> |
||
|
Tom Boucher
|
53ea8e0664 |
fix(#3057): make a guard's failure distinguishable from its benign result — Wave 1 (#3088)
* fix(#3057): refuse the write when the duplicate scan cannot complete writeManifest documents itself as a fail-closed duplicate guard: if any existing manifest shares plan_id with a different, non-terminal job_id it must refuse, because dispatching again would duplicate the external job. It could not honour that. The scan reads every sibling manifest looking for the duplicate, and an unreadable or unparseable sibling was `continue`d past. If the corrupt file was the one holding the live duplicate, the scan found nothing and a duplicate external job dispatched. The asymmetry is what gives it away: a malformed TARGET refused with malformed_existing because clobbering is unacceptable, while a malformed SIBLING was skipped — yet siblings are the only thing the duplicate check reads. Adds a scan_incomplete verdict that refuses and names the offending file, so an operator can quarantine or repair it. Fail-closed alone would let one stale corrupt manifest wedge every dispatch for that planning dir permanently; naming the file is what makes refusing survivable. malformed_existing is untouched, so the target/sibling distinction stays visible. The docstring is updated — it previously stated a rule the function did not keep. memFs() gains an optional failReads map so these branches are reachable at all; they had zero coverage because the fake could not express a per-file read fault. The signature is additive and every existing caller is unchanged. The regression is proved by a pair, not a single test. A control writes a readable sibling holding a genuine non-terminal duplicate and asserts duplicate_plan_id, establishing the scenario is real; the regression then makes that same path unreadable and asserts scan_incomplete. A first draft of this test used a corrupt-JSON fixture containing no plan_id at all while its comment claimed otherwise — it duplicated the unparseable-sibling case and proved nothing, which is the defect class this phase exists to remove. Refs #3051 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(#3057): make a guard's failure distinguishable from its benign result Wave 1 of the negative-space backfill: the branches where a guard that could not verify something reported the same value it reports when everything is fine. That indistinguishability is the defect; every fix here makes the two states tellable apart, and every test proves it with a pair — one for the failure, one for the benign case. A single test cannot establish that two states are distinguishable, which is the whole property being fixed. state.cts phaseInventoryProvider returned null for both a real disk-scan failure and a genuinely empty phases dir, so `state rebuild` could report success while phase-table reconciliation never ran. It now returns a discriminated result and the CLI surfaces phase_inventory_scan_failed plus a reason. The reason field turned out never to have been wired into the emitted JSON at all — it existed only as an internal variable — so a test could only assert on the operator-facing note. It is a real field now. state.cts treated an unreadable lock body the same as an empty one, applying the 1-second stealable floor. A lock we cannot read is not a lock we know is stale; an unreadable body is now held to the deadman ceiling like a live holder. verification.cts findStaleVerificationSummary returned null on any fs, scan or clock failure — meaning "not stale". It now returns a discriminated StaleCheckResult and the caller records that the check was indeterminate. git-base-branch resolveBaseBranch returned 'main' both when no candidate branch existed and when every git tier timed out. A diagnostics variant now reports whether the answer was verified, and the CLI writes an unverified-fallback note to stderr. The stdout contract five workflows parse is untouched. worktree-safety snapshotWorktreeInventory left exists:true when statSync threw, so a guard that could not check reported the worktree present; exists is now tri-state and a stat failure surfaces as an 'unverified' finding. planWorktreePrune reported 'no_worktrees' for a parse failure, which is not the same as an empty list — and it drives a prune. It now reports 'parse_failed'. Fixing the inventory change exposed a second fail-open in verify.cts: the validate-health consumer silently dropped findings whose kind it did not recognise, so the new kind would have vanished. That is closed too — worth noting that the survey enumerated producers of degraded verdicts, not consumers that discard them. worktree-base-ref and state-transition gain the distinguishing signal without changing what they do: headAbsenceVerified, and a phase-inventory scan meta. Whether those guards should ACT differently is a product question this change does not answer, and both are flagged rather than quietly settled. rescueSummaryArtifacts is left alone: rescuing on an uncertain cat-file is deliberate per #2556. It now has tests proving it, and a recorded negative finding — git cat-file -e returns 128 for both "absent from HEAD" and a fatal error, so "uncertain" and "certain-and-fine" are not separable at the git level. Refs #3051 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * test(#3057): assert typed values, not rendered text Ten assertions in the rebuild CLI suite matched substrings of produced output — STATE.md body fields, a markdown table row, an audit-log heading, and JSON keys read as text. CONTRIBUTING prohibits that: if the code under test produces text, the test asserts on its structured surface instead. No production surface had to be built. Every one already existed and was already compiled into bin/lib: stateExtractField for body fields, parseMarkdownTable for the phase table, collectSection for the audit-log section, and result.data.log — already a typed RebuildLogEntry[]. The tests were matching rendered text sitting next to the structured data. One of those assertions was passing for the wrong reason. `stdout.includes ('rebuilt')` matched the JSON KEY name, not a value: the dry-run path emits `mutated` and the real path emits `rebuilt`, so it would have passed whether the value was true or false. It now asserts the value. external-job's refusal already had to name the offending file — that naming is why the fail-closed variant is survivable rather than a permanent wedge — but the tests proved it by substring of a prose message. The failure result now carries offendingPath as its own field and the tests assert it by value. The human message is unchanged; operators read it. Array membership is left alone. `phaseIds.includes('99')` and `result.updated.includes('Completed Phases')` are membership checks on real arrays, not text matching, and converting them would weaken nothing and clarify nothing. Refs #3051 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * test(#3057): execute acquireStateLock instead of grepping its source The non-EEXIST lock test asserted on the TEXT of the built .cjs and never called acquireStateLock. It carried an allow-test-rule: architectural-invariant exemption to permit that. A source grep proves a literal is present in a file, not that the behaviour works — it is weaker than a liveness test, which at least runs the code, and it was the only coverage the fatal-errno path had. Replaced with tests that inject the errno through fs and assert what actually happens: a fatal EACCES propagates out of acquireStateLock with zero backoff sleeps, while EAGAIN/EINTR/EINVAL/EIO/ENOENT/ESTALE/EPERM/EBUSY retry once and succeed. The exemption is removed and its allowlist entry with it. One old assertion is deliberately not carried over: it checked the retryable errnos were expressed as a Set rather than an inline literal. That is a shape check with no runtime signature; the behavioural tests fail if the code reverts to the old inline check, which is the regression it was really guarding. The #3057 lock-body tests move into that same file rather than a new one, which is what lint-test-file-count asks for and puts every acquireStateLock test in one place. Refs #3051 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(#3057): surface an indeterminate staleness check to its callers An isolated review caught an inconsistency inside this wave. Two of the three "add the distinguishing signal" fixes wire through to something a user sees: git base-branch writes an unverified-fallback diagnostic to stderr, and an unverifiable worktree surfaces as a W020 finding. The third set staleCheckIndeterminate on readVerificationStatus's result and nothing read it. A signal nobody consumes leaves the fail-open exactly as silent as before: the staleness check could fail and the operator saw precisely what they would see if the answer were genuinely "not stale". That is the defect this issue exists to remove, so it is not defensible as scaffolding when its two siblings in the same change already wire through. All five callers now surface it, each through the channel it already had rather than a mechanism imposed uniformly: phase complete adds it to its existing warnings array and, on the blocked path, as an additive note on the error text; init and roadmap carry it as a field on output they already emit; the UAT report carries it without ever gating passed/blockers; workstream inventory takes an injectable writeDiagnostic mirroring the git base-branch idiom, because its return shape had nowhere to hang a per-phase field without rippling the builder's types. The routing decision is unchanged everywhere. What changes is only that a caller and an operator can now tell a failed check from a completed one. That diagnostic carries structured meta rather than being asserted by regex — the default still writes only the human message to stderr, but tests assert phaseDir and reason by value. Two earlier assertions in this branch were converted the same way; this was the last raw-text assertion left. Also records a scope correction: the completePhaseCore guards now compare stateReplaceField's result to the body instead of testing truthiness, so a field whose substitution produced identical text no longer reports as updated. That is a real behaviour fix, not the signal-only change this file was described as carrying, and its tests cover both the changed and unchanged cases. Refs #3051 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * test(#3057): bound two heavy subprocesses for a loaded bench, not an idle one The remote matrix surfaced three failures unrelated to this branch's changes. All were bad tests, and a re-run would have hidden every one of them. The reviewer-flags parse block bounded bash -> node -> a full gsd-tools cold start at 5 seconds. On a bench running thirty thousand tests in parallel that is not a hang, it is a busy machine. Raised to 30s, matching the convention sibling suites already use for script invocations, with a comment saying what the budget covers so nobody tightens it back. Two further copies of the same 5-second spawn in the same file had the identical defect and are raised too — they were not in the failure report, but they will be next time. The fragment-propagation test bounded npm run regen:derived — a full build plus eight generators, the heaviest subprocess in the suite — at five minutes, and node22 was killed near the end. The captured output proves it: every generator had written its files and gen:install-tree had emitted all fifteen runtimes before the kill. Raised to fifteen minutes. That failure read as `null !== 0`, which says nothing. status null means killed, not a non-zero exit, and the two want different responses: one is a timeout to size correctly, the other is a real build break. The assertion now distinguishes them and names the signal. Neither test's assertions were weakened and no retry was added. A retry here would suppress exactly the signal the timeout exists to produce. Refs #3051 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * test(#3057): capture fd 1 through the mock tracker, not a raw reassignment The phase suite reported zero test results on both lanes while running for five and a half minutes and exiting 1. No assertion text, no stderr, four events for the whole file: enqueue, start, dequeue, complete. That shape is not a failing assertion — it is the runner being unable to read the child at all, because it parses its event stream from the child's stdout. The cause was the capture helper reassigning fs.writeSync directly. Proven rather than assumed: a standalone probe patched fs.writeSync and called process.stdout.write, and the interception fired only when fd 1 resolved to a FILE, not when it was a pipe. The remote runner captures the event stream to a file, so a helper that was invisible against a pipe swallowed the reporter's own output on the bench. That is also why the two sibling suites wired the same way in this change pass cleanly — they use the mock tracker, the seam io.test.cjs established for this exact function. The helper now uses t.mock.method with an explicit restore after each call, so teardown belongs to node:test rather than a second hand-rolled implementation, and the interception cannot outlive the one synchronous call it wraps even if that call throws. Ten call sites thread the test context through; three test callbacks gained the parameter they lacked. The three B3 tests are untouched — same assertions, same fault injection. Only how the context reaches the helper changed. Refs #3051 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * test(#3057): capture phase-complete output from a subprocess, not fd 1 Two attempts to make in-process fd-1 interception safe both failed on the bench. The suite reported zero test results on either lane while exiting 1 — four events for the whole file — because the runner parses its event stream from the child's stdout, and process.stdout.write routes through fs.writeSync whenever fd 1 resolves to a file, which is how the runner captures. Patching that seam anywhere in a file can therefore destroy the file's own reporting, and tightening the window only moved the runtime from 326s to 125s without recovering a single event. So the interception is gone rather than tuned. The helper now spawns gsd-tools as a real subprocess and reads stdout the way the OS already gives it to us, which is what the rest of the suite does. It asserts the command succeeded before parsing, so a genuine failure can no longer present as a JSON parse error. The two fault-injecting tests could not survive that move as written: a subprocess cannot see a mock installed in the parent. Instead of reinstating the interception they now produce the fault on disk — the summary artifact is created as a dangling symlink, so the staleness check's real statSync throws inside the child. That is a more honest fixture than a mock in any case, since it is a condition a user's tree can actually be in. Skipped on Windows, matching the existing symlink precedent in the write-guard suite. Three further call sites turned out to depend on parent-process writeFileSync mocks the subprocess could not see. Those call the CJS function directly, which is what they always wanted — they never needed stdout at all. Refs #3051 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(#3057): one name for one signal, one encoding for one distinction Standards review found four things this branch introduced, all of them inconsistencies with itself rather than with the repo. One upstream bit reached its consumers under three names — verification_stale_check_indeterminate in two modules, the same value with "stale" dropped in a third, and stderr only in the fourth. Standardised on the long name wherever it is a field. The workstream inventory keeps its stderr channel, since its return shape has nowhere to hang a per-phase field without rippling the builder's types, but it now says the same word for the same thing. worktree-safety encoded one three-way distinction two ways in a single file: a named union for a finding's kind, and boolean|null for an inventory entry's existence. The second is now a named union too. Two assertions matched human prose because the blocked and non-blocked completion paths carried no typed field for the signal. Both now assert typed values. The first round of this fix added the field but left the regex beside it, which is the banned pattern sitting next to its own replacement; the second removed it and added an assertion on the reason enum so nothing was lost. The remaining two were reasoned away before being fixed, and both reasons were bad. "No typed surface exists" is the condition CONTRIBUTING says to fix by adding one — it took three lines. "The file already does this dozens of times" is not licence to add instance number thirty-one; a convention that violates a documented rule is debt, not precedent. Vocabulary differing across DIFFERENT modules is left alone: CONTEXT.md rejects a single shared result envelope, so per-module shapes are precedented, and a baseline smell does not outrank a documented standard. A census of every line this branch adds to a test file now finds no regex or substring assertion on produced prose: 87 strictEqual, 25 ok (all non-empty or shape guards), 12 equal, 3 throws (all typed err.code predicates), 3 deepStrictEqual, 2 notStrictEqual. Refs #3051 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * chore(#3057): backfill changeset pr number to 3088 --------- Co-authored-by: sim <sim@local> Co-authored-by: Claude Opus 5 <noreply@anthropic.com> |
||
|
Tom Boucher
|
9a76ca6783 |
fix(#1882): distinguish unterminated frontmatter from absent frontmatter (#2712)
* fix(#1882): distinguish unterminated frontmatter from absent frontmatter
extractFrontmatter returned {} both for a document with no frontmatter and for
one whose fence was opened and never closed, so a file truncated mid-write was
byte-identical to a legitimate no-metadata file. Verified live through
`gsd-tools frontmatter get`: both printed {} with exit 0 and nothing on stderr.
Per ADR-1411's "corrupt is not absent" amendment the {} return is preserved
exactly -- no caller may break -- and the cause is surfaced out-of-band as a
deduplicated, unconditional stderr diagnostic. That mechanism lands as a shared
leaf module rather than a per-site copy because three sibling findings in the
same epic need it identically; four hand-rolled copies of one behaviour is the
generative-fix-divergence defect class.
The discriminator is deliberately not "opened but never closed". A Markdown
document whose first line is a thematic break takes that exact branch, so
flagging on the missing fence alone reports corruption on good Markdown -- the
failure mode this class of check has shipped with before. The unterminated
region is instead run through extractFrontmatter's own parser (extracted as
parseYamlRegion so the probe and the real parse can never diverge) and reported
only when it yields at least one key.
Also folds an inline defect found while working: src/config-loader.cts carried
two NUL bytes in the JSDoc added by this epic's Phase 1 (
|
||
|
Tom Boucher
|
27c2279a39 |
fix(#2617): project verification next_command onto the runtime's command surface (#2700)
* fix(#2617): project verification next_command onto the runtime's command surface `src/verification.cts` stored and synthesized hard-coded `/gsd:…` command strings with no runtime context, and `phase complete` relayed that raw field straight into its verification-blocked error. On a Codex project the suggested next step was `/gsd:execute-phase`, a surface Codex does not install — it installs `$gsd-execute-phase`. The colon form is wrong twice over: `runtime-slash.cts` documents that "the colon form is never emitted", so EVERY runtime — not just Codex — was being handed a deprecated shape. Fixed at the one routing seam rather than per caller: - The routing table now stores BARE command names (`execute-phase`), never a prefixed literal. A prefixed literal in the table is what leaked. - A single `projectNextCommand(bare, runtime, tail)` helper runs every return path through `formatGsdSlash`, preserving the argument tail (`01 --gaps`) untouched. An empty command stays empty, so "no next step" never becomes a bare prefix. - `readVerificationStatus` accepts `opts.runtime`; `cmdVerificationStatus` and `phase complete` pass `resolveRuntime(cwd)`. The default is `claude`, which yields the canonical `/gsd-` hyphen form. All four routed states are covered: missing, unknown, gaps_found, stale. `init.cts` keeps its own projector deliberately. It already formats correctly, and its command CONTENT differs from the router's on purpose (it appends the phase number to `execute-phase`, and routes `human_needed` to `verify-work`). Consolidating them would silently change `init`'s user-visible output, which this issue did not ask for — so the divergence is left intact and the new tests instead pin the property that matters on both surfaces: no raw colon form escapes. Failing-first record: `origin/next:src/verification.cts` carried the four `/gsd:` literals (lines 101, 108, 382, 392), and 11 existing assertions in tests/verification-status.test.cjs asserted the colon form. Those 11 are corrected in this commit — they passed before the fix and fail after it, which is precisely the regression this closes. Tests are folded into the module's primary suite rather than added as a third file (`lint-test-file-count` caps the `verification` module at two, and consolidating is its documented remedy — growing the allowlist is not). The `phase complete` assertion reads `res.error`, not `res.stderr`: `runGsdTools` exposes a clean non-zero exit's stderr as `error`, and reading the wrong field yields '' and makes the whole check vacuous — which is how this user-visible path stayed untested. Closes #2617 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QT3ibz5qJuDuGqpTGRYVGf * test(#2617): scope the new hooks to their describes; cover gaps_found through the CLI Two findings from the orthogonal review of the first commit, both in the tests this change added. 1. The folded block's `beforeEach`/`afterEach` were declared at MODULE scope. node:test applies module-scope hooks to every test in the file, so hooks added for the #2617 suites also wrapped the ~40 pre-existing tests in verification-status.test.cjs — making an unrelated block a single point of failure for them (currently benign, but a throwing hook would have failed suites it has nothing to do with). They now install inside their own describes via a small `useProjectionPhaseDir()` helper, with a comment recording why. 2. The live-CLI `phase complete` test exercised only the `missing` state, so a regression in any other routed branch would have shown up in the router's return object but not in the text a user actually reads. Added a `gaps_found` case per runtime, asserting the projected `plan-phase <N> --gaps` reaches the blocked-completion error. Whole file verified green: 48 tests, 48 pass — the ~40 pre-existing ones included. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QT3ibz5qJuDuGqpTGRYVGf * test(#2617): correct the last colon-form assertion in phase.test.cjs The remote run surfaced one more stale assertion outside tests/verification-status.test.cjs: the `phase complete` canonical-gate suite matched the blocked-completion message against `/\/gsd:verify-work 0?1/`. That project fixture configures no runtime, so it takes the `claude` default, which now yields the canonical `/gsd-verify-work 01` hyphen form. The colon form this asserted is exactly the deprecated shape #2617 removes — `runtime-slash.cts` documents that "the colon form is never emitted". Like the eleven corrected in the first commit, this assertion passed before the fix and fails after it, which is the regression record rather than a test being loosened: the surrounding assertions (failure reason, `stale` wording, and that neither ROADMAP.md nor STATE.md was mutated) are untouched. Verified against the real CLI: the emitted message is now "Phase 1 verification is incomplete: Verification is stale. Re-run verify-work before transition. Next: /gsd-verify-work 01". Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QT3ibz5qJuDuGqpTGRYVGf * fix(#2617): collapse the two verification projectors into one seam The orthogonal review found that `init.cts` carried a second, independently maintained `verificationNextCommand()` that had drifted from the router's table in CONTENT, not just formatting: state router (before) init.cts missing execute-phase execute-phase <N> unknown execute-phase execute-phase <N> human_needed "" (no command) verify-work <N> The `human_needed` row is the sharp one: two GSD surfaces disagreed about whether a next command existed at all, and the router's own next_action told the user to "re-run the verify step until status is passed" while naming no command to run. init's answers were the useful ones, so the router adopts them and init now delegates to it — satisfying the issue's "keep one verification-routing seam" direction. `verificationNextCommand()` is deleted. Appending the phase number surfaced a trap the old bare commands hid. `extractPhaseToken` also returns project-code forms (`PROJ-07`), which are indistinguishable by shape from an ordinary directory name — `gsd-651-parent` yields `gsd-651` — so deriving the argument blindly emits `execute-phase gsd-651`. The number is therefore appended only when it is unambiguously numeric, or when the caller supplies it explicitly. `init` does supply it: its `phaseDir` is unresolved in several branches, where the router could not derive one at all. dir `01-example` -> $gsd-execute-phase 01, $gsd-verify-work 01 dir `gsd-651-parent` -> $gsd-execute-phase, $gsd-verify-work Suites verified green against the built lib: verification-status 50/50, phase 268/268, init 143/143, init-manager 40/40. `npm run lint:ci` clean. User-visible change beyond the reported bug, as agreed: `query verification.status` and `phase complete` now append the phase number for missing/unknown, and emit `verify-work <N>` for human_needed where they previously emitted nothing. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QT3ibz5qJuDuGqpTGRYVGf * chore(#2617): backfill changeset PR number (#2700) --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
Tom Boucher
|
2c54f219c9 |
fix(#2348): derive verification staleness from git commit time, not mtime (#2394)
readVerificationStatus() decided a phase's verification was `stale` (a *-SUMMARY.md newer than the *-VERIFICATION.md) by comparing filesystem mtimes. mtimes are assigned at checkout time and are not preserved by `git clone` / `cp -R`, and any unrelated `touch` / reformat / editor-save re-stales a valid report — so a committed phase declaring `status: passed` could silently read `stale` on a fresh clone purely from checkout order, falsely rewriting a ROADMAP row and blocking milestone close (#2022 gate). Each file's effective "last changed" time is now its git commit time when the file is committed AND clean, and its mtime otherwise (uncommitted or working-tree-dirty). Both are real wall-clock change times, so a summary committed after — or edited after — the verification reads stale, while a clean fresh clone stays passed. Git commit time is content-tied and clone- stable; mtime is retained only where it is the true last-changed signal. Implementation: - Two bounded git calls per phase (never one-per-file): `git log --first-parent --format=%ct --name-only` for commit times, and `git diff --name-only HEAD` to drop dirty files. readVerificationStatus runs per-phase in the init/roadmap listing loops, so per-file spawning would fan out to P×(S+1) git processes ("Unbounded Subprocesses"). - `--first-parent` so merge commits report their file lists (plain `--name-only` omits merge diffs and would under-date merge-landed content). - The dirty-check fails SAFE: if `git diff` is inconclusive (errors / exits non-zero) the commit times are discarded so every file falls back to mtime, never trusting a possibly-stale commit time (no false "not stale"). - Paths matched back by `/`-bounded suffix (root vs nested `plans/` can't collide) and passed after `--` (dash-named files can't be read as flags). - A phase with no summaries skips git entirely; the scan short-circuits on the first stale summary. A `phaseCleanCommitTimesMs` seam keeps the unit tests hermetic (no git spawn); the resolver's two-call error handling is unit-tested via an injected execGit; two real-git integration tests lock the end-to-end path, the committed-then-edited (dirty) regression, and the `--` argv guard. |
||
|
Jeremy McSpadden
|
77c7b4fc9d |
fix(#1522): enforce canonical verification before phase transition (#1548)
* fix: require fresh phase verification before transition * no-mistakes(review): Fix canonical verification closeout gates * no-mistakes(review): Fix verify-work frontmatter promotion command * no-mistakes(review): Fix stale verification gates * no-mistakes(review): Fix canonical verification routing gates * no-mistakes(review): Fix verification dependency and runtime routing gates * no-mistakes(review): Block stale verification bypasses * fix: handle large init manager outputs in verification workflows * chore: update changeset pr number * fix(verify-work): use fresh verification.status for stale gate The stale check after UAT used phase_completion.verification_status from session-start INIT while human_needed promotion already queried fresh verification.status. Align the stale gate with the canonical query so mid-session verification refresh is not ignored. * fix(init): skip roadmap-checked phases when selecting next_phase Roadmap-only phases without a disk directory were still promoted to next_phase when their checkbox was already checked. Exclude checkboxComplete phases so progress routing does not point at work the roadmap already marks done. * fix: gaps_found not overridden by stale, transition uses canonical verification - verification.cts: check gaps_found before stale so gap-closure routing is not masked by a newer summary mtime - phase.cts: remove redundant findStaleVerificationSummary — readVerificationStatus already handles stale detection - transition.md: replace raw grep on file content with verification.status query to avoid false-positive blocks from body text matching * ci: retrigger tests after rebase * fix(transition): replace gsd_run advisory check with awk frontmatter extraction The runtime launcher is not defined until the update_roadmap_and_state step bash block (~line 165). The early verify_completion block used gsd_run to query verification.status, which violated the runtime-launcher-parity test: 'preamble appears AFTER the first gsd_run reference'. Replace the gsd_run call with an awk-based frontmatter extractor that reads only the status: field between the two --- fences. This avoids both the preamble-ordering constraint and the original false-positive grep bug where body text like 'previous_status: gaps_found' would match a full-text regex. The phase.complete gate at update_roadmap_and_state is the canonical enforcement point; this early check is advisory only. Also update workflow-size-baseline.json for the updated transition.md size. Fixes: runtime-launcher-parity test (B) Co-authored-by: Codesmith <codesmith-bot@users.noreply.github.com> * fix: re-check verification under planning lock in phase complete Move readVerificationStatus into withPlanningLock so stale verification cannot slip through when a SUMMARY.md is written between the gate and the roadmap/state mutation. Return the blocked status from the lock callback and emit the error after release to avoid leaving .lock behind. * fix(transition): gate on canonical verification.status including stale Replace awk frontmatter read with verification.status query so transition blocks when summaries are newer than VERIFICATION.md, matching phase.complete and other workflows (autonomous, progress, verify-work). * Fix workflow verification gates for yolo transition and stale routing Require VERIFY_STATUS passed before yolo/interactive transition advance. Route stale verification recovery to verify-work, matching canonical projection. * fix(transition): use verification.status query for stale-aware advisory check The awk-based check read raw frontmatter status: passed, which misses the stale case where summaries are newer than the VERIFICATION.md file even though the frontmatter still says passed. The stale status is computed from file modification times, not stored in frontmatter. Move the preamble to the verify_completion bash block (the first block with a gsd_run call) so gsd_run query verification.status can be used for the advisory check. This gives the full readVerificationStatus logic including mtime-based staleness detection, matching the enforcement gate at phase.complete. Capture full JSON (VERIFY_JSON) so next_action can be included in the advisory output alongside the status. Also update workflow-size-baseline.json for the updated transition.md size. Co-authored-by: Codesmith <codesmith-bot@users.noreply.github.com> * ci: trigger test matrix for 525b946 Co-authored-by: Codesmith <codesmith-bot@users.noreply.github.com> * fix(transition): restore awk frontmatter extraction for pre-shim verification check The gsd_run launcher shim is not defined until line ~163 of transition.md, so the verification debt check at line ~80 cannot use gsd_run. Restore the awk-based frontmatter extraction that correctly reads status without needing the runtime, and restore the shim at its proper location before phase.complete. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix(#1522): clarify transition verification gate wording * fix(#1522): update transition workflow size baseline * fix(#1522): update workflow-size-baseline after rebase onto next Co-authored-by: Codesmith <codesmith-bot@users.noreply.github.com> * fix(#1522): guard findStaleVerificationSummary FS calls + thread opts.fs seam (review) Address review blocker B1 on #1548: findStaleVerificationSummary ran fs.readdirSync and two fs.statSync calls unguarded between readVerificationStatus's try/catch sections, so a TOCTOU race (a SUMMARY listed by scanPhasePlans then removed before statSync) or any FS error threw uncaught into callers NOT under the planning lock (init.manager / init.progress / uat-predicate). Wrap the body in try/catch degrading to 'not stale', and thread the injectable opts.fs seam (add statSync to FsLike, pass fsImpl from the caller) for parity with readVerificationStatus's no-throw contract and testability. Also adds the Verification Module glossary entry to CONTEXT.md (review B3). --------- Co-authored-by: Cursor Agent <cursoragent@cursor.com> Co-authored-by: Codesmith <codesmith-bot@users.noreply.github.com> Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com> Co-authored-by: Tom Boucher <trekkie@nomorestars.com> |
||
|
Tom Boucher
|
ec2ecdf28b |
refactor(#1283): T3 — migrate 9 multi-leaf callers off the core spine (batch 2) (#1285)
Migrate 9 files' entire core surface to the leaf modules directly (behaviour-identical — leaves are the objects core re-exports by reference): config, docs, gap-checker, graphify-command-router (namespace core.output -> io.output), init (17 core symbols -> 8 leaves), profile-output, uat, verification, workstream. All 9 now import zero core symbols and are removed from the allowlist (18 -> 9). Stale core.* docstrings corrected. core.cts re-exports untouched (still serve the remaining 9 files); teardown is T-final. No behaviour change. Closes #1283 Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com> |
||
|
Tom Boucher
|
4056d830bc |
refactor(#651): consolidate verification-status routing into one queryable seam (#755)
* refactor(#651): consolidate verification-status routing into one queryable seam The passed/gaps_found/human_needed verification status was re-encoded as bare strings across three prose surfaces (gsd-verifier emits, execute-phase routes, ship gates), each independently deciding the per-status next action with no parity coupling — the DEFECT.GENERATIVE-FIX class. Give the enum one home: src/verification.cts (-> bin/lib/verification.cjs) exposing `gsd_run query verification.status <phaseDir>` returning a typed {status, next_action, next_command}. ship.md and execute-phase.md now consume the query instead of re-deriving the routing in prose; gsd-verifier.md points at the shared vocabulary as the single emitter (values unchanged). Also fixes the latent broad-grep status misread (DEFECT.FRONTMATTER-SCALAR- BROAD-GREP): execute-phase.md read `grep "^status:"` over the whole report, so a body `status:` line could misroute a valid phase. Extraction is now frontmatter-scoped in one place. A parity test fails if a verifier status gains no route. Lands the two CONTEXT.md DEFECT entries captured on the issue. Closes #651 Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * chore(#651): set changeset pr to 755 Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com> |