Repository navigation
Conversation
trek-e
left a comment
There was a problem hiding this comment.
Review: changes requested
Zero-tolerance policy: every finding below (including Minor/Nit) must be resolved before approval. The queued CI runs for this head are being released so you have a signal to push against.
Head: pr-5030 (f726c57), author drakeo338, branch fix/4939-bug-decision-coverage-a-phase-dir-that-d.
Files: .changeset/wise-dogs-frolic.md, src/check-command-router.cts (+30), tests/decisions.test.cjs (+72).
Summary
Adds isExistingDirectory() (src/check-command-router.cts:171-177) and a guard in both
cmdDecisionCoveragePlan (:425-433) and cmdDecisionCoverageVerify (:539-545): a phase-dir
argument that is GIVEN but is not a directory now answers reason: "phase directory not found"
with total/covered (plan) or total/honored (verify) null instead of a measured-looking
covered: 0. Plan gate still fails closed (passed:false); verify stays non-blocking. Omitted
phase dir unchanged (pinned by #4130 tests at tests/decisions.test.cjs:2579-2598, 2647-2657).
Correct, minimal, fail-closed. Only minor/nit findings. Lint clean, build clean.
Acceptance checklist (issue #4939 + triage Agent Brief)
- Plan gate: phase NUMBER in phase-dir slot no longer reports
covered: 0— src:430-433; test tests/decisions.test.cjs (#4939 "plan gate — the phase NUMBER…"). - Treated as a caller error, fail closed, distinct reason —
passed:false, skipped:false, reason:'phase directory not found'. (Issue's example usedno-plans-found+ numeric totals; the PR's null totals follow the #4794 "null not 0" convention — stricter, acceptable.) - Message names the argument and says to pass the phase DIRECTORY — src:431.
- Verify gate parity as non-blocking warning — src:542-545; test "#4939: verify gate…".
- Regression tests for nonexistent-dir on both gates — present.
- Correct form unchanged (control test "#4939: control").
- [~] "exists but holds no plans" variant: triage brief made it optional ("if that distinction matters"); not implemented. Not required by the reproduction.
Prior findings status
No prior reviews or comments on the PR (reviews: [], comments: []). First review.
Root-cause verdict
Root cause per triage: loadPlanContents() (src:156-165) returns [] for a missing path,
identical to a genuinely empty scan. The PR distinguishes the case at both consumers before the
scan rather than changing loadPlanContents's return type — this is what the triage Agent Brief
prescribed and covers every consumer (get_impact upstream: only routeCheckCommand; no parallel
SDK handler found via find_code). Not special-casing/swallowing: the unmeasured state is surfaced
as a caller error with null metrics. Guard is placed after context/decision validation, so a
context with no trackable decisions still green-skips first (correct: nothing to check).
Memtrace Evidence
- get_impact(cmdDecisionCoveragePlan, upstream, node a84c023d, branch HEAD): LOW risk, 1 affected —
routeCheckCommand(src:1835), process Gsd-coreRouteCheckCommand. (First unscoped call returned ambiguous: 4 candidates across HEAD and fix/4378-seed-id-collision.) - get_symbol_context(loadPlanContents / cmdDecisionCoverageVerify): ambiguous — multiple declarations (HEAD + stale branch copies); node-id reissue returned "node UUID … is ambiguous in the requested graph scope". No traversal performed.
- recall_decision("decision coverage gate caller error phase directory missing null total covered"): no on-topic decision (hits: coverage-parity gate, resolves-only scope, ADR-612 bracket phase-ID, #3755 consensus gate).
- find_code_review_issues(diff, online, max 60):
_graph_state: ready, 0 issues (ast 0, yaml 0, cross_module 0, online 0). - find_code (is-directory helper): no existing canonical helper to reuse (closest
isReadableDirectoryin src/runtime-artifact-layout.cts:171, different module). - npm run lint: clean. npm run build: complete.
Overlap with #5003 (issue #4987) and origin/next
- #5003 adds
isPhaseDirAbsent()in src/verification.cts (ENOENT/ENOTDIR → absent; other errnos fall through) and statusphase_dir_not_found. #5030 addsisExistingDirectory()in src/check-command-router.cts that maps ALL stat errors to "not found". No textual conflict (different files), but two new private is-dir predicates with divergent errno semantics and divergent vocab (phase directory not foundvsphase_dir_not_found). Each is consistent with its own file's existing vocabulary (check gates use prose reasons; verification uses snake_case statuses), so vocab divergence is acceptable; the errno divergence is Nit 2 below. - origin/next: no existing fix for #4939; PR three-dot diff is clean.
Findings
- Minor — tests/decisions.test.cjs (#4939 describe, "a FILE in the phase-dir slot" test covers plan gate only). The verify branch src/check-command-router.cts:542 has no FILE-in-slot test, and no verify-side control (dir given → numeric honored). A regression that swaps verify's predicate back to
fs.existsSync(true for a file) would not be caught. Fix: add the FILE-in-slot case forcheck.decision-coverage-verifyassertingreason,honored:null,total:null. - Nit — src/check-command-router.cts:171-177 swallows every stat error (EACCES, ELOOP, EIO) as "not a directory", and the messages at :431/:543 then assert the directory "does not exist or is not a directory" — a wrong diagnosis for a permissions failure. Fail-closed outcome is right; the text is not. Fix: follow the #4794 pattern already in this function (src:369-380,
contextKind) — capture the stat outcome (missing / not a directory / unreadable) and name it in the message. - Nit — documented output contracts not updated for the new nullable fields: gsd-core/references/verifier-phase-gates.md:41-48 still documents verify as
{ skipped, blocking:false, total, honored, not_honored, message }with numeric totals and instructs writing{honored,total}intodecision_coverage; gsd-core/workflows/plan-phase.md:1360-1363 same for the plan gate. (Fixed changesets are exempt from the docs rule, so not blocking.) Fix: notereasonand thattotal/covered/honoredare null on caller-error outcomes. - Nit (pre-existing, same function, same defect class) — src/check-command-router.cts:514-531: verify's
could-not-parsebranch still answerstotal: decisions.length, honored: 0(measured-looking zero), while the plan gate (#4794, :397-403) and this PR's new verify branch (:543) answer null. The function now has two inconsistent "unmeasured" shapes. Fix: nulltotal/honoredthere too.
Changeset: type Fixed, pr: 5030 correct, bold lead + em dash + symptom-led, trailing (#4939). OK.
Branch name, conventional commits, Fixes #4939, template sections: OK. Test is behavioral (spawns the CLI), no source-grep, no chmod, no wall-clock. No limit → boundary rule N/A; not a parser → fast-check N/A.
Security: argument is interpolated only into a JSON string value emitted via output() (JSON.stringify); no shell/argv/path-write use. No injection, traversal or secrets.
CI: only policy checks have run (all pass); test matrix not yet run (fork approval) — informational.
Verdict
minor — correct root-cause fix with adequate tests for the reported case; four minor/nit items above
(one test gap, one misleading error text, two contract/consistency gaps). No blockers.
…ed counts The phase-dir guard folded every stat failure into "does not exist or is not a directory", so a permission or I/O error was misdiagnosed as a missing path. It now says whether the path is missing, is a file, or could not be read, naming the error code in the last case. Verify's could-not-parse branch still answered total: decisions.length, honored: 0 for a check that never ran. It now answers null for both, the same shape as the plan gate and the new phase-dir branch. Adds the verify-gate FILE-in-slot test and a verify control, and injects the EACCES case by mocking fs.statSync in-process. Documents the null fields on caller errors in plan-phase.md and verifier-phase-gates.md, and refreshes the compact-content benchmark baseline for that growth. Emitted-Drift-Ack-Growth: plan-phase.md — documents that total/covered are null on the decision-coverage gate's caller-error outcomes (review of open-gsd#5030), net +229 bytes
|
Pushed bcaadbd with all four findings addressed and the current next merged in; |
trek-e
left a comment
There was a problem hiding this comment.
Re-review — #5030 (fixes #4939), head 834d06a94a
Prior findings — all resolved on this head
The FILE-test verify case, the stat-outcome message, the contract docs, and the verify could-not-parse null are all fixed.
Classification & gate compliance
confirmed-bug → Bug-Fix track; Fixes #4939, conventional title/branch, not a draft. No security defect found (no new argv/subprocess/path surface).
Memtrace Evidence
get_impact(targetcmdDecisionCoverageVerify,branch: next, upstream): LOW, 2 affected symbols — the sole callerrouteCheckCommand(src/check-command-router.cts:1892-1995) and its file; processGsd-coreRouteCheckCommand;graph_incomplete: false. (An earlier unscoped query was ambiguous across branches.)get_symbol_context(targetcmdDecisionCoveragePlan,src/check-command-router.cts): unscoped queryambiguousacross branches, no callers/callees — inconclusive; the plan guard shares therouteCheckCommandcaller.recall_decision(query "decision coverage gate caller error null total covered unmeasured"): no on-topic decision.governing_rulesonsrc/check-command-router.cts: ADR-443 only (unrelated).find_code_review_issues(online,max_candidates: 50) on thesrc/check-command-router.ctsdiff (tests/docs excluded): 0 issues,_graph_state: ready; weak evidence.
Findings
- Minor
[judgment]src/check-command-router.cts(plan guard ~:443, verify guard ~:557):reasonstays"phase directory not found"even when the stat outcome is EACCES/EIO/file, so a machine consumer branching onreasoncannot tell missing from unreadable (only the message text differs). You chose this deliberately; either emit distinctreasonvalues or state in the contract docs that consumers must not branch on it. - Nit
[judgment]tests/decisions.test.cjs(runGateWithStatFailure):t.mock.restoreAll()is not in afinally, so a throw fromrouteCheckCommandleavesfs.writeSyncmocked until the test ends. - Nit
[judgment]The compact-content baseline numbers were not independently recomputed in this pass; please confirm they were regenerated from this head.
CI
On this head only the policy checks had run and mergeStateStatus is BLOCKED; the fork test matrix was held at action_required. I am releasing those runs now; nothing has failed.
Verdict
Changes requested: findings 1–3 are open and the matrix has not executed.
|
Pushed 011bb78: reason is now distinct per stat outcome (not found, not a directory, unreadable) with docs and tests updated, mock restore moved into finally, and the benchmark baseline regenerated. |
trek-e
left a comment
There was a problem hiding this comment.
Merge conflict gate: this branch does not merge cleanly into current next (f4be7ed), so review is paused until it is resolved. The following path(s) conflict (verified with git merge-tree --write-tree origin/next refs/pr/5030):
tests/fixtures/compact-content-benchmark-baseline.json
This is a generated benchmark baseline; regenerate it after the rebase.
Rebase onto next (git fetch origin next && git rebase origin/next), resolve, regenerate any generated files rather than hand-merging them, and push. The full review runs once the branch merges cleanly.
011bb78 to
0413ddb
Compare
…ed counts The phase-dir guard folded every stat failure into "does not exist or is not a directory", so a permission or I/O error was misdiagnosed as a missing path. It now says whether the path is missing, is a file, or could not be read, naming the error code in the last case. Verify's could-not-parse branch still answered total: decisions.length, honored: 0 for a check that never ran. It now answers null for both, the same shape as the plan gate and the new phase-dir branch. Adds the verify-gate FILE-in-slot test and a verify control, and injects the EACCES case by mocking fs.statSync in-process. Documents the null fields on caller errors in plan-phase.md and verifier-phase-gates.md, and refreshes the compact-content benchmark baseline for that growth. Emitted-Drift-Ack-Growth: plan-phase.md — documents that total/covered are null on the decision-coverage gate's caller-error outcomes (review of open-gsd#5030), net +229 bytes
0413ddb to
8ef455f
Compare
|
Rebased onto next again; the phase-dir check now lives in the shared decision-coverage support module that both gates call, tests pass (8ef455f). |
8ef455f to
6818eb9
Compare
6818eb9 to
59dadff
Compare
trek-e
left a comment
There was a problem hiding this comment.
Review — fix(#4939): answer a caller error for a missing phase dir (re-review)
Head reviewed: 59dadff141abae9d27e5057d7977c037503747b1 (two commits: 40f41bfdac, 59dadff141). Prior trek-e review at 011bb78243 was a COMMENTED merge-conflict pause; earlier CHANGES_REQUESTED reviews at f726c577b7 and 834d06a94a.
Prior findings
| # | Finding (earlier reviews) | Status at head |
|---|---|---|
| 1 | Verify-side FILE-in-slot test + control missing | Resolved: tests/decisions.test.cjs #4939: verify gate — a FILE in the phase-dir slot… and …control: the phase DIRECTORY… |
| 2 | Stat-outcome message wrongly says "does not exist" for EACCES | Resolved: phaseDirProblem src/decision-coverage-support.cts (new fn, ~:63-82) names could not be read (code); EACCES tests for both gates |
| 3 | Contract docs not updated | Resolved: gsd-core/references/verifier-phase-gates.md:41-54, gsd-core/workflows/plan-phase.md:1367-1372 |
| 4 | Verify could-not-parse still answers total: decisions.length, honored: 0 |
Code resolved (src/gate-decision-coverage-verify.cts:57-62 now null/null) BUT see Blocker 1 (existing tests/golden not updated) |
| 5 | reason collapsed all stat outcomes |
Resolved: distinct phase directory not found / phase path is not a directory / phase directory unreadable |
| 6 | t.mock.restoreAll() not in finally |
Resolved: tests/decisions.test.cjs runGateWithStatFailure uses try/finally |
| 7 | Baseline not independently recomputed | Not independently re-verified by me (generator script read blocked by Memtrace-first guard). Delta +81 offTokens/+81 onTokens on plan-phase only is consistent with the 6 lines added to plan-phase.md. Author states it was regenerated with --write. Confirm via CI. |
Summary
Root-cause fix is sound: both decision-coverage gates now require a given phase-dir argument to be an existing directory (phaseDirProblem, shared in src/decision-coverage-support.cts) before loadPlanContents can answer []. The author also moved the check into the shared support module that both gates call (one owner for the concept). One new defect class introduced: the verify could-not-parse shape change (total/honored -> null) breaks existing tests and a golden that were not updated.
Classification & gate compliance
- Linked issue #4939: OPEN, labels
bug,confirmed-bug-> Bug-Fix track. Body hasFixes #4939. Not a draft. Titlefix(#4939): …, branchfix/4939-bug-decision-coverage-a-phase-dir-that-d. - Changeset
.changeset/wise-dogs-frolic.md: type Fixed,pr: 5030, bold lead + em dash, symptom-led. OK. - Docs gate: Fixed -> exempt from
docs/; contract docs updated anyway. No new references/workflows files -> no INVENTORY change. - No competing open PR (
gh pr list --search "4939 in:body"-> only #5030). - Labels on PR:
needs-review: merge-conflictis STALE (see Merge).review: changes requestedpresent.
Merge state (verified this turn)
gh pr view: mergeable: MERGEABLE, mergeStateStatus: BEHIND, headRefOid 59dadff141abae9d27e5057d7977c037503747b1. git merge-tree --write-tree origin/next refs/pr/5030 exit 0 (tree 1c109bccba…), no conflicts. Behind origin/next by 2 (957faa55b6 docs #5175, b18e8c53b2 CI timeout history #5163), neither touches PR files; rebase is clean. The merge-conflict label should be removed.
Root-cause verdict
loadPlanContents answers [] for a missing path, indistinguishable from zero coverage. Guard placed after the context/decision checks and before the scan, only when phase-dir argument was given (omitted case pinned by #4130 tests). Fail-closed on plan gate, advisory on verify. Correct, not special-casing. ENOENT/ENOTDIR -> not found, everything else -> unreadable (fail-safe).
Blast radius
Callers: routeCheckCommand only (check.decision-coverage-plan / -verify). Output-shape change is the real radius: verify could-not-parse now null/null (consumers: gsd-core/references/verifier-phase-gates.md decision_coverage pass-through; existing golden + unit tests, below). Risk tier: Low for call graph, Medium for output contract.
Test quality
New #4939 describe in tests/decisions.test.cjs: behavioral (spawn CLI / in-process router), asserts specific reason/null totals/omitted uncovered, fs.statSync/writeSync mocked (no chmod), no wall-clock, no source-grep, restore in finally. Gaps are Minor (see 3, 4). Fail-first: the plan/verify NUMBER tests assert reason/null which base cannot produce (base returns covered: 0).
Co-change completeness
Not run: get_cochange_context not invoked (documented to time out on this repo). Manually found absent-from-diff files that assert the changed output: tests/gate-decision-coverage-verify.unit.test.cjs (U2d, U2e) and tests/fixtures/gate-cutover/decision-coverage-verify-could-not-parse.json (see Blocker 1).
Memtrace Evidence
get_impact(evaluateDecisionCoveragePlan, upstream, depth 3): returnedambiguous: true, 2 declarations (src fn + test constant),risk: UNKNOWN, no traversal. Not a clean result. Earlier reviews of the same symbol (node-scoped) report LOW, sole callerrouteCheckCommand.get_symbol_context(phaseDirProblem):found: false(new symbol; index is ofnext). No data.find_symbol(evaluateDecisionCoveragePlan)located src/gate-decision-coverage-plan.cts:35-150.recall_decision(query "decision coverage gate caller error phase directory not found null total covered unmeasured"): returned 10 hits, none on-topic (coverage-parity gate, ADR-612, #3755 consensus gate, resolves-only scope).governing_ruleson src/gate-decision-coverage-plan.cts:governed: false, no rules.find_code_review_issues(online, max 40): I invoked it with a placeholder diff, not the real PR diff;_graph_state: ready, 0 issues. This is NOT evidence of a clean review; a prior reviewer's run on the real diff (online, max 60) returned 0 issues.find_yaml_rule_matches/find_cross_module_issuesnot run.detect_changes(4 changed files):affected_symbols: []for all, i.e. index (ofnext) has no overlap with the PR-added symbols; not informative.find_codefor consumers of the verify could-not-parse shape surfaced tests/gate-decision-coverage-verify.unit.test.cjs and tests/fixtures/gate-cutover/decision-coverage-verify-could-not-parse.json; confirmed by Read (below).
Findings
Blocker 1 [judgment, confirmed by reading] The verify could-not-parse change to total: null, honored: null (src/gate-decision-coverage-verify.cts:57-62) contradicts three existing expectations the PR did not update, so they fail at this head:
- tests/gate-decision-coverage-verify.unit.test.cjs:111-121 (U2d) expects
total: 1, honored: 0. - tests/gate-decision-coverage-verify.unit.test.cjs:124-143 (U2e) expects
total: 0, honored: 0. - tests/fixtures/gate-cutover/decision-coverage-verify-could-not-parse.json
stdouthas"total": 0, "honored": 0; tests/check-router-cutover-equivalence.test.cjs:82 does a byte-for-byteassert.strictEqual(actual.stdout, golden.stdout)for the E2 arm that runs this fixture.
Fix: update U2d/U2e titles+expectations and regenerate the golden (via the repo's golden generation, not by hand). The PR body's "189 passed, 0 failed" is for the author's local run and does not cover these files; the CI matrix has not executed (below).
Blocker 2 [judgment] CI matrix never executed for this head: 9 fork runs sit at action_required (see CI). A PR whose matrix never ran is not approvable.
Minor 3 [judgment] tests/decisions.test.cjs: the ENOENT/ENOTDIR -> "not found" vs other-errno -> "unreadable" boundary is pinned only on the EACCES side. No test pins that ENOTDIR (e.g. <file>/child) maps to phase directory not found; a swap of the two branches in phaseDirProblem would pass. Add an ENOTDIR (and ENOENT) injection case for both gates.
Minor 4 [judgment] Docs: gsd-core/workflows/plan-phase.md:1367-1370 and gsd-core/references/verifier-phase-gates.md:42-46 contain over-long lines in otherwise wrapped prose (cosmetic); and the docs list 'could-not-parse' among caller errors while the plan-phase list also names "context path is not a file" but not "missing context path argument" (that arm still returns total: 0, covered: 0), so the stated "total/covered null on caller error" is incomplete for that reason. Either list it with its actual shape or state the exception.
Verdict
CHANGES REQUESTED (blocker). Substance of the #4939 fix is correct and prior findings 1-3,5,6 are resolved, but the verify could-not-parse null change breaks U2d, U2e and the E2 golden (unrelated tests not updated), and the matrix has not run. Stale needs-review: merge-conflict label: branch merges cleanly (behind 2, clean rebase).
CI (fresh this turn)
Head 59dadff141abae9d27e5057d7977c037503747b1, mergeStateStatus: BEHIND. statusCheckRollup (7): all SUCCESS (validate-target, Pull request template format, validate-title, Issue link required, GitGuardian, Socket x2). Pending-approval queue for this head: pending=9 — 36936713383 Default Flip Documentation; 36936713307 Validate Branch Name; 36936713442 Changeset Required; 36936713459 Docs Required; 36936713470 Install Smoke; 36936713407 Tests; 36936713503 Security Scan; 36936713613 Mutation Testing; 36936712973 Dependabot Auto-Merge. No failing check; none executed.
59dadff to
02fa818
Compare
|
U2d/U2e and both could-not-parse goldens now expect null totals (goldens regenerated), ENOENT/ENOTDIR cases pin "not found" for both gates, and the doc lines are wrapped with the caller-error list completed; also rebased onto current next. |
02fa818 to
d8bbaf9
Compare
trek-e
left a comment
There was a problem hiding this comment.
Substance at head d8bbaf9395 is clean (the tree is identical to the one reviewed at 02fa81834a), but this branch now conflicts with next after #5171 (typed gate evidence) landed, so it cannot be updated or merged:
src/decision-coverage-support.ctssrc/gate-decision-coverage-plan.ctssrc/gate-decision-coverage-verify.ctsgsd-core/references/verifier-phase-gates.mdtests/fixtures/compact-content-benchmark-baseline.json(regenerate withnode scripts/benchmark-compact-content.cjs --writeafter the build; do not hand-merge)
Please merge origin/next into the branch, resolve the three src/ files against #5171's evidence/exit-status changes, and push. A re-review of the resolved delta will follow.
71b04e9 to
2015d6f
Compare
|
@trek-e merged |
trek-e
left a comment
There was a problem hiding this comment.
Review — fix(#4939): answer a caller error for a missing phase dir
Head reviewed: 82fdb30d44472a5e450617fba0531e2ae39c735c. Full diff read: yes.
Summary
Both decision-coverage gates now require a given phase-dir argument to be an existing directory via the shared phaseDirProblem (src/decision-coverage-support.cts) before loadPlanContents can answer []. The prior merge conflict with #5171 is resolved; the substance is correct. The test matrix has not executed.
Classification & gate compliance
- #4939 OPEN,
confirmed-bug-> bug-fix track;Fixes #4939; conventional title; branch name valid; not a draft. - Changeset
.changeset/wise-dogs-frolic.md: Fixed,pr: 5030, bold lead, symptom-led,(#4939). CHANGELOG untouched. - Docs: Fixed is exempt; contract docs (verifier-phase-gates.md, plan-phase.md) updated;
Emitted-Drift-Ack-Growthtrailer present for plan-phase.md. No new references/workflows files, so no INVENTORY change. - No competing PR for #4939.
- Merge state (this turn):
mergeable: MERGEABLE,mergeStateStatus: BEHIND, behindnextby 34,git merge-tree --write-tree origin/next refs/pr/5030exit 0 (clean). Theneeds-review: merge-conflictlabel is stale.
Functional checklist / root-cause verdict
Root cause: loadPlanContents answers [] for a path that is not there, indistinguishable from a phase whose plans cite nothing. The fix distinguishes this in the shared support module, only when the argument is given, after context/decision validation. ENOENT/ENOTDIR -> phase directory not found; other errnos -> phase directory unreadable (fail-safe); a file -> phase path is not a directory. Plan gate stays fail-closed with null totals; verify stays non-blocking with null honored/total. All #4939 acceptance items verified in code and tests; the regression tests assert reason and null totals, which base cannot produce (fail-first).
Blast radius
Only routeCheckCommand reaches the changed gates. Output-contract change: verify could-not-parse now null/null (unit U2d/U2e and both goldens updated; docs updated). Risk: Low (call graph), Medium (output contract).
Co-change completeness
Changeset, contract docs, goldens and unit cases are updated. Missing: gate-unit and cutover coverage of the new phase-dir outcomes (Finding 2).
CodeGraph Evidence
codegraph_nodetargetstatEvidenceandgateUnreadable: "not found" (the index does not parse.ctsand describesnext); source verified withgit grepon refs/pr/5030:statEvidencemaps ABSENT_CODES to none and any other code to unreadable.codegraph_impact/codegraph_callerstargetsevaluateDecisionCoveragePlan/evaluateDecisionCoverageVerify: no indexed data (.cts); callers from source and prior reviews:routeCheckCommandonly.codegraph_calleestargetphaseDirProblem: new symbol, absent from the base index; callee from source:statEvidence.
Memtrace unavailable: monthly query quota exhausted (20972/20933, resets 2026-10-20)
Findings
- Blocker [fact] CI matrix never executed for this head.
action_requiredruns (9): 37172613252 Docs Required; 37172613279 Default Flip Documentation; 37172613284 Mutation Testing; 37172613208 Dependabot Auto-Merge; 37172613213 Validate Branch Name; 37172613258 Install Smoke; 37172613257 Tests; 37172613292 Security Scan; 37172613380 Changeset Required. Only the 7 policy checks ran (all SUCCESS). Not approvable until the matrix runs green. - Minor [judgment] tests/gate-decision-coverage-plan.unit.test.cjs, tests/gate-decision-coverage-verify.unit.test.cjs, tests/fixtures/gate-cutover/arms.cjs: no case or arm pins the phase-dir-not-found shape, so the golden equivalence suite does not guard the new output; only tests/decisions.test.cjs does. Add unit cases and E1/E2 arms with generated goldens.
- Nit [judgment] The branch carries two merge commits (
Merge upstream/next ...) and is 34 behind. Rungit fetch origin next && git rebase origin/next, resolve, regenerate the compact-content baseline withnode scripts/benchmark-compact-content.cjs --writeafter the build, and push. - Nit [judgment] docs/INVENTORY.md:589 description of
gate-decision-coverage-verify.cjsomits the new caller-error outcomes.
Verdict
Request changes. The #4939 fix substance is correct and prior findings are resolved, but Finding 1 (matrix not executed) blocks, and Findings 2-4 are open.
82fdb30 to
4bf3543
Compare
|
Rebased onto current next (no merge commits): the branch is now three commits on top of 83aba44. The docs/INVENTORY.md conflict is resolved, the gate unit and cutover coverage and both INVENTORY descriptions are kept, and the baseline was regenerated with |
trek-e
left a comment
There was a problem hiding this comment.
Automated maintainer re-review of head 4bf35431b3. Zero-tolerance policy: every finding at any severity must be resolved before approval.
Review — fix(#4939): answer a caller error for a missing phase dir (RE-REVIEW)
Head reviewed: 4bf35431b305f35fe2e2d3a86a3cabadf167969c (gh pr view, this turn). mergeable: MERGEABLE, mergeStateStatus: BEHIND, reviewDecision: CHANGES_REQUESTED. Three commits on 83aba44, no merge commits.
Summary
Both decision-coverage gates call the shared phaseDirProblem (src/decision-coverage-support.cts:66-80) before loadPlanContents, only when a phase-dir argument was given. ENOENT/ENOTDIR -> phase directory not found; file -> phase path is not a directory; other stat errors -> phase directory unreadable (outcome unreadable, UNAVAILABLE). Plan gate fails closed with null total/covered; verify stays non-blocking with null honored/total. Verify could-not-parse now null/null (parity with #4794). Substance correct. Remaining blocker is CI: the matrix has not executed.
Prior findings status (review of 82fdb30, 2026-10-07 01:23)
- Blocker, CI matrix never executed: UNRESOLVED (not author-fixable).
gh run list --commit 4bf35431: Tests, Install Smoke, Mutation Testing, Security Scan, Docs Required, Changeset Required, Validate Branch Name, Default Flip Documentation, Dependabot Auto-Merge allcompleted action_required. Only the 4 policy workflows (Target, Template Format, Title, Issue Link) plus GitGuardian and Socket x2 ran, all SUCCESS. - Minor, no unit cases/cutover arms for new outcomes: RESOLVED. tests/gate-decision-coverage-plan.unit.test.cjs U1o/U1p/U1q; tests/gate-decision-coverage-verify.unit.test.cjs U2o/U2p/U2q; tests/fixtures/gate-cutover/arms.cjs:542-544 (E1 x2), :556-557 (E2 x2) with four new goldens under tests/fixtures/gate-cutover/.
- Nit, merge commits and 34 behind: PARTIAL. Merge commits gone (3 linear commits). Now behind by 1 (ed0f3fd, touches only tests/ci-timeout-budget-history.jsonl;
git merge-treeexit 0, clean). Baseline regenerated (commit 4bf3543). - Nit, docs/INVENTORY.md:589 omitted new outcomes: RESOLVED (both gate rows at docs/INVENTORY.md:590-591 now list the three reasons and null totals).
Acceptance checklist (issue #4939, verbatim items)
- "The gate should treat '0 plans loaded' as a caller error, the same way #2770 ... and #4794": MET. src/gate-decision-coverage-plan.cts:~160-169 (block,
reason, null totals). Verified: nonexistent dir path. - "
cmdDecisionCoverageVerify... has the same shape, as a non-blocking warning": MET. src/gate-decision-coverage-verify.cts:~108-115 (gateVerdict('advisory', false, ...)). - Agent Brief: distinct fail-closed reason: MET (not
no-plans-found; prose reason per the file's vocabulary, acceptable). - Brief: regression tests for both gates asserting caller-error shape: MET (tests/decisions.test.cjs new describe, plus unit and golden arms).
- Brief: "exists but holds no plans" distinction: optional per brief ("if that distinction matters"); not implemented. Not required by the reproduction.
- Correct form unchanged: MET (control tests in tests/decisions.test.cjs; omitted phase-dir left to scan, #4130 behavior).
Findings
- Blocker | CI | PR-level: 9 matrix workflows at
action_requiredfor 4bf3543; a PR whose matrix never executed is not approvable. Fix: maintainer releases fork runs; surface results verbatim. - Nit | process | branch is BEHIND next by 1 (ed0f3fd, jsonl-only, clean). Fix:
git fetch origin && git rebase origin/next(no conflicts expected); re-verify baseline unaffected. - Minor | co-change | docs/CONFIGURATION.md:1440-1445 ("Invoking the plan gate directly") documents the missing-context caller error but not the new phase-dir caller errors (
phase directory not found/phase path is not a directory/phase directory unreadable). Fixed changesets are docs-exempt, but the contract prose lives here. Fix: add one sentence. - Minor | tests | tests/decisions.test.cjs (EACCES cases) and unit U1q/U2q assert outcome/JSON but no case asserts the process exit status for the
unreadablearm (UNAVAILABLE/69) through the router; the cutover goldens only cover not-found/is-file (exitCode 0). A swap ofgateUnreadableforgateVerdict('block')in phaseDirProblem consumers would keep JSON identical exceptoutcome. U1q/U2q assertoutcome: 'unreadable'so this is partly covered at the unit level; add a router-level exit assertion or an injected-stat cutover arm. - Nit | changeset | .changeset/wise-dogs-frolic.md names only
reason: "phase directory not found"for "both gates" yet the gates answer three reasons (a file givesphase path is not a directory); the later sentence covers unreadable but not the file case. Fix: list the file reason. - Nit | PR body | "Ran
npm test: 189 passed" is a stale local claim; Platforms list Linux only. Runner verdict (gsd-test) is not shown in the PR; confirm out of band.
Unverified (could not run, per rules): compact-content baseline numbers (+56 tokens off/on, plausible for the 4 lines added to plan-phase.md) and the gsd-test verdict; no source files executed.
Blast radius
codegraph callers: evaluateDecisionCoveragePlan / evaluateDecisionCoverageVerify -> cmdDecisionCoveragePlan/Verify in the router (routeCheckCommand) and tests only. loadPlanContents callers: the two gates only, both guarded now. phaseDirProblem new, called only from the two gates. Index describes next (.cts not parsed for the new symbol), so counts come from source. Output-contract change: verify could-not-parse total/honored 0 -> null (U2d/U2e and both could-not-parse goldens updated; docs/references/verifier-phase-gates.md updated). Risk Low (graph), Medium (contract).
Root cause
loadPlanContents answers [] for an absent path, identical to a plan set citing nothing. Fix distinguishes the case in one shared module before the scan; no catch swallows a sentinel; every caller (both gates) handles the new result; one owner (statEvidence). Not special-cased.
Security
phase-dir arg already contained by resolveContainedPath before use; statEvidence follows symlinks like the prior scan did; stat/scan TOCTOU degrades to unreadable from the scan (fail-safe). Non-ENOENT/ENOTDIR stat errors never read as "missing". No new argv/prompt surface.
Gate/format compliance
confirmed-bug issue, bug track, Fixes #4939, conventional title, not draft, branch valid, changeset Fixed pr: 5030 with trailing (#4939), docs updated (verifier-phase-gates.md, plan-phase.md, INVENTORY.md), Emitted-Drift-Ack-Growth trailer present for plan-phase.md. CONTEXT.md: no decision-coverage vocabulary, no update needed. No new references/workflows files so no manifest change.
Co-change gaps
docs/CONFIGURATION.md (Finding 3); CTS/CJS parity: gates are .cts compiled, no hand-written CJS twin.
Behind-by / rebase
BEHIND by 1 (ed0f3fd tests/ci-timeout-budget-history.jsonl); clean per git merge-tree exit 0.
Verdict
Substance clean / approvable on code; changes requested only for CI (matrix unexecuted, orchestrator releases), plus Minor 3-4 and Nits under zero tolerance.
Memtrace Evidence
Memtrace quota was exhausted for this review run, so the graph-backed checks were NOT executed; this is stated rather than reported as passed:
get_impact: not run (quota). Blast radius above comes from codegraph where present.get_symbol_context: not run (quota). Callers/callees were read via codegraph where present.recall_decision: not run (quota). The recorded-decision gate was NOT evaluated; this is not a finding that no decision governs the change.find_code_review_issues: not run (quota). The deterministic AST/YAML/cross-module pass did not execute; findings above are judgment-sourced unless marked deterministic.
CI runs for this head are left queued (action_required) because Minor-or-higher findings are open; release will follow once they are addressed. The fix push re-triggers CI once.
|
Merged current next, documented the three phase-dir caller errors in the configuration docs, and pinned exit 69 for the unreadable phase dir in the decisions test |
trek-e
left a comment
There was a problem hiding this comment.
Review — fix(#4939): answer a caller error for a missing phase dir
Head b9de0e8dd98c488abb4889d589a3e9fb06dad940. Zero-tolerance policy applies.
Summary
Shared phaseDirProblem gates both decision-coverage gates before loadPlanContents. Prior Minor findings (CONFIGURATION.md, exit-69 test) and the changeset nit are resolved.
Classification & gate compliance
confirmed-bug, Fixes #4939, conventional title, not draft, changeset Fixed pr: 5030, docs updated. No competing PR.
Root-cause verdict
loadPlanContents answers [] for a path that is not there, indistinguishable from zero coverage; fixed at the shared owner, not special-cased.
Blast radius
Only routeCheckCommand reaches the gates; verify output contract changes (Major 1).
Test quality
Regression tests assert reason + null totals (fail on base); exit 69 pinned through the router; unit and cutover goldens added. No source-grep or timing assertions.
Findings
- Blocker CI: 9 matrix workflows unexecuted (
action_required). - Major Scope: verify
could-not-parsetotal/honored0 -> null (gate-decision-coverage-verify.cts, U2d/U2e, two goldens, verifier-phase-gates.md, changeset clause) is not in #4939. Remove or split out. - Major #4939 asks that "holds no plans" / "0 plans loaded" be a caller error; an existing phase dir with zero plans still reports
covered: 0(decision-coverage-support.cts:66-80, plan:160-169, verify:108-115). Implement with tests for both gates. - Nit 15 behind
nextwith merge commits again;git fetch origin && git rebase origin/next(clean). - Nit PR body cites head
72e2bc33fand ticks "All existing tests pass" beside 9 listed failures.
Verdict
Changes requested.
Memtrace Evidence
The Memtrace MCP server was unavailable for this review (it failed to connect: memtrace not found in PATH), so no graph-backed check ran. None of the four checks below is a clean result.
get_impact: not run. Blast radius comes from reading the diff and grepping callers.get_symbol_context: not run. Callers and callees of the changed symbols were checked by grep.recall_decision: not run. The recorded-decision gate did not execute, so this does not show that no decision governs the change. ADRs andCONTEXT.mdpredicates were checked by grep instead.find_code_review_issues: not run. The deterministic review pass did not run. A manual diff review stood in for it, so the findings above are judgment, not graph facts.
|
Pushed 9a9b488:
|
9a9b488 to
d540cfa
Compare
trek-e
left a comment
There was a problem hiding this comment.
Review — fix(#4939): answer a caller error for a missing phase dir (re-review)
Head b25ae44f45277da34ae9287313a529314e7f79f8 (gh pr view this turn: mergeable MERGEABLE, mergeStateStatus BLOCKED, reviewDecision CHANGES_REQUESTED). Zero-tolerance policy applies.
Summary
Prior review (b9de0e8) raised: Blocker CI unexecuted, Major 2 (verify could-not-parse scope creep), Major 3 (existing dir with zero plans still covered: 0), Nit 4 (merge commits / behind), Nit 5 (stale body). Delta since: Major 2 and Major 3 are resolved in code and tests. The CI Blocker is unchanged, Nit 4 is only partly resolved, and the body still cites a stale head.
Classification & gate compliance
- Issue #4939: confirmed-bug -> Bug-Fix track.
Fixes #4939present, not draft, titlefix(#4939): ..., branchfix/4939-bug-decision-coverage-a-phase-dir-that-d. - Changeset
.changeset/wise-dogs-frolic.md: Fixed,pr: 5030, bold lead, symptom-led, trailing (#4939). Describes the no-plans outcome and the could-not-parse clause is gone. - Docs: Fixed is exempt, but CONFIGURATION.md, INVENTORY.md rows, verifier-phase-gates.md and plan-phase.md were updated. No new references/workflows files, so no manifest change.
- Merge commits: 5
Merge remote-tracking branch 'upstream/next' into rebased-5030merges remain (see Nit 3).
Must-Have Acceptance Checklist (issue #4939 + Agent Brief)
- [PASS] "treat '0 plans loaded' as a caller error": nonexistent dir (src/gate-decision-coverage-plan.cts:~160-166), not-a-directory, unreadable, and now existing dir with no PLAN file (:~172-176) all fail closed with null totals.
- [PASS] Verify gate parity as non-blocking warning: src/gate-decision-coverage-verify.cts:~107-113 and :~121-125 (advisory,
honored/totalnull,not_honored: []). - [PASS] Distinct fail-closed reason, message names the argument and says pass the phase DIRECTORY.
- [PASS] Regression tests on both gates: tests/decisions.test.cjs new describe, unit U1o-U1r / U2o-U2r, cutover arms E1/E2.
- [PASS] Correct form unchanged: control tests in decisions.test.cjs; omitted phase dir left to the scan (#4130).
Prior-finding status
- Major 2 (verify could-not-parse null change): RESOLVED.
gh pr diffhas no change to the could-not-parse branch, U2d/U2e, its goldens, or a could-not-parse clause in verifier-phase-gates.md/changeset. - Major 3 (existing dir with zero plans): RESOLVED in src. Plan gate blocks when
phaseDir && planContents.value.length === 0; verify requires no PLAN and no SUMMARY (keeps R4 summary-only fixture readable). Tests U1r/U2r pin it. Callers that relied on an empty dir were updated (tests/representative-corpus.test.cjs, tests/sdk-removal-query-family-dispatch.test.cjs); the other gate test files I checked build a PLAN or SUMMARY in the phase dir. - Blocker (CI unexecuted): NOT RESOLVED (below).
- Nit 4 (merge commits / behind): PARTIAL. Not behind
nextnow (0 commits), but 5 merge commits are back; author wrote "I'll rebase next". - Nit 5 (body): PARTIAL. Body now cites head 9a9b488, which is not the current head b25ae44; the "All existing tests pass" box is unticked beside 9 listed failures, which is honest.
Root-cause / functional verdict
Root cause: loadPlanContents answers [] for an absent or empty phase dir, indistinguishable from a phase whose plans cite nothing. The fix gates on a single shared owner (phaseDirProblem, src/decision-coverage-support.cts:~66-80) before the scan and on an empty scan result after it, in both gates. Not special-casing. ENOENT/ENOTDIR -> not found; other errno -> unreadable (exit 69). Both gate outputs are consistent with the #4794 null-not-zero convention.
Blast radius
Only routeCheckCommand reaches the two gates. Behavior change: an existing-but-empty phase dir now blocks the plan gate (was a covered: 0 block with uncovered list; still a block, different shape) and produces a verify warning with null counts. Risk Low (call graph), Medium (output contract; documented).
Co-change completeness
Changeset, docs, INVENTORY rows, unit cases, goldens and the two incidental tests are updated. Gap: no cutover E1/E2 arm or router-level test for phase directory holds no plans (Minor 2).
Memtrace Evidence
- get_impact: UNAVAILABLE — Memtrace MCP failed to connect (ENOENT).
- get_symbol_context: UNAVAILABLE — Memtrace MCP failed to connect (ENOENT).
- recall_decision: UNAVAILABLE — Memtrace MCP failed to connect (ENOENT). Not evidence that no decision governs the change.
- find_code_review_issues: UNAVAILABLE — Memtrace MCP failed to connect (ENOENT). Manual diff review stood in; findings are judgment, not graph facts.
- Used instead:
gh pr diff 5030,git diff origin/next...refs/prrev/5030, per-file inspection of the 14 other test files that invoke the gates, repo grep for callers.
Findings
- Maintainer-side gate, not a contributor defect [fact] CI matrix never executed for head b25ae44; the runs are held at
action_requiredpending maintainer release, which is withheld while Minor 2 / Nits 3-5 are open (release after they are fixed). 9 runs ataction_required(not released by me): 39040650641 Docs Required; 39040650668 Dependabot Auto-Merge; 39040650678 Changeset Required; 39040650777 Tests; 39040650723 Security Scan; 39040650727 Default Flip Documentation; 39040650753 Mutation Testing; 39040650768 Validate Branch Name; 39040650736 Install Smoke. Only the 7 policy checks ran (all SUCCESS). Latest-per-name non-success check-runs list was empty. Not approvable until the matrix runs green. Because the matrix has not run, the compact-content baseline (tests/fixtures/compact-content-benchmark-baseline.json, regenerated before the last 5 merges) and the new unit/cutover goldens are unverified. - Minor [judgment] No cutover arm and no router/decisions.test.cjs case for
phase directory holds no planson either gate (only unit U1r/U2r). Add E1/E2 arms in tests/fixtures/gate-cutover/arms.cjs with generated goldens, and a spawn-the-CLI case in tests/decisions.test.cjs, matching how not-found/is-file are pinned. - Nit [fact] Five merge commits (
Merge remote-tracking branch 'upstream/next' into rebased-5030, b25ae44, cc1f6e2, f35c000, 00bb0d0, 3d0d126) plus cd44be7; the branch was previously asked to be linear. Fix:git fetch origin && git rebase origin/next, regenerate the baseline withnode scripts/benchmark-compact-content.cjs --writeafter build, push. - Nit [fact] PR body cites "head 9a9b488"; current head is b25ae44. Update the body to the head tested.
- Nit [judgment] tests/decisions.test.cjs describe title still says "a phase directory that does not exist" while it now covers not-a-directory, unreadable, and (via unit) no-plans outcomes; retitle.
Verdict
Changes requested. Substance of Majors 2 and 3 is fixed and correct; the matrix is unexecuted (maintainer-side; CI is released once the findings are fixed), and Minor 2 and Nits 3-5 are open and must be fixed.
b25ae44 to
5e49777
Compare
|
Rebased onto next as one linear commit (5e49777), added the no-plans E1/E2 cutover arms with goldens and a CLI case for both gates, retitled the describe, and updated the body to that head. |
trek-e
left a comment
There was a problem hiding this comment.
Re-review — fix(#4939): answer a caller error for a phase dir with nothing to scan
Head reviewed: fcbb6bd15344c25c8d59bad3eff2ce4aeb0d7f2b (the previously reviewed head 5e49777061 plus a merge of next at f215401d09; the PR's own files are unchanged by that merge).
Summary
All findings from the earlier review are resolved in code (the no-plans E1/E2 cutover arms, goldens and CLI cases are present; one linear commit; PR body and describe title updated). The CI matrix has now executed on this head, and it fails on a defect in this PR's own diff that a static read does not surface.
Classification & gate compliance
- Bug-Fix track (
confirmed-bugon #4939). Branch, title andFixes #4939are valid. - Emitted-attribution growth is unacknowledged (Blocker, fact).
gsd-core/workflows/plan-phase.mdis a shipped runtime-loaded workflow and this PR adds 4 lines to it (git diff --stat origin/next...HEAD -- gsd-core/workflows/plan-phase.md= 4 insertions). The single PR commit (5e49777061) carries noEmitted-Drift-Ack-Growthtrailer.
Must-Have Acceptance Checklist
Carried from the earlier review: the issue's acceptance criteria pass at code level. They cannot be treated as met while the required test run is red (below).
Memtrace Evidence
- get_impact: UNAVAILABLE — Memtrace MCP failed to connect (ENOENT).
- get_symbol_context: UNAVAILABLE — Memtrace MCP failed to connect (ENOENT).
- recall_decision: UNAVAILABLE — Memtrace MCP failed to connect (ENOENT). The recorded-decision gate was NOT evaluated.
- find_code_review_issues: UNAVAILABLE — Memtrace MCP failed to connect (ENOENT). No deterministic AST pass ran.
- Substitute evidence:
gh pr diff 5030,git diff --stat origin/next...refs/prrev/5030,git logtrailers on the PR commit, and the CI job logs below.
CI (head fcbb6bd15344c25c8d59bad3eff2ce4aeb0d7f2b)
Tests run 39051228833 concluded failure. Latest-per-name failures: conformance test (macos-latest, 24), Required tests, test (ubuntu-latest, 24, shard 3/3), test (ubuntu-latest, 24). Install Smoke and Mutation Testing succeeded. All three failing lanes report the same single failure, verbatim:
not ok 106 - differential attribution over the real tree
location: tests/emitted-attribution.test.cjs:2284:1
emitted-attribution failed against merge-base(f215401d096a…, HEAD):
1 file(s) grew without an acknowledgment:
plan-phase.md grew 224 bytes (96115 -> 96339)
Add a trailer to a commit in this PR (never a new file). Use the `Emitted-Drift-Ack-Growth` trailer for growth …
Emitted-Drift-Ack-Growth: plan-phase.md — <why this growth is deliberate>
The test file tests/emitted-attribution.test.cjs is not in this PR's changed-file list, and the growth is measured against the merge-base with next, so the 224-byte growth comes from this PR's own plan-phase.md hunk.
Findings
Blocker
- [fact]
gsd-core/workflows/plan-phase.mdgrows by 224 bytes with noEmitted-Drift-Ack-Growth: plan-phase.md — <reason>trailer on any commit in this PR. This failstests/emitted-attribution.test.cjs(differential attribution over the real tree) on the macOS conformance lane and both Ubuntu test lanes, and failsRequired tests. Fix: add the trailer to a commit in this PR (do not add a file, and do not regenerate anything), then re-run.
Verdict
Changes requested. One Blocker (unacknowledged emitted-prompt growth, red Required tests). The earlier findings are resolved; this is the only remaining item.
fcbb6bd to
7c94d1a
Compare
|
Added the |
trek-e
left a comment
There was a problem hiding this comment.
Status note (head
7c94d1a181f0): Blocker 1 below is a maintainer-side gate (the fork matrix has not been released), not a defect in your diff. TheEmitted-Drift-Ack-Growthtrailer from the earlier review is now present and that finding is resolved. CI runs stay unreleased while Nits 2-4 are open, because your fix push re-triggers the full matrix. The open findings you need to act on are Nits 2, 3 and 4.
Review — fix(#4939): answer a caller error for a missing phase dir (re-review)
Head reviewed: 7c94d1a181f026efee0fe0d42fe58d29162f87ce (gh pr view this turn: mergeable: MERGEABLE, mergeStateStatus: BEHIND, reviewDecision: CHANGES_REQUESTED). One commit. Zero-tolerance policy applies: every finding below, at any severity, must be resolved before approval.
Summary
Both decision-coverage gates call the shared phaseDirProblem (src/decision-coverage-support.cts:66-80) before loadPlanContents, only when a phase-dir argument was given, and treat an empty scan as the same caller error afterwards. ENOENT/ENOTDIR -> phase directory not found; a file -> phase path is not a directory; any other stat errno -> phase directory unreadable (outcome unreadable, exit 69); a directory with no PLAN file (verify: no PLAN and no SUMMARY) -> phase directory holds no plans. Plan gate stays fail-closed (passed:false, total/covered null, uncovered omitted); verify stays non-blocking (honored/total null, not_honored: []). The substance is correct. The only remaining blocker is that the CI matrix has not executed on this head.
Prior findings (last trek-e review: CHANGES_REQUESTED at fcbb6bd153, 2026-10-17T17:29Z)
| # | Finding | Status at 7c94d1a181 |
|---|---|---|
| 1 | Blocker: gsd-core/workflows/plan-phase.md grew 224 bytes with no Emitted-Drift-Ack-Growth trailer (red tests/emitted-attribution.test.cjs, Required tests) |
Resolved in the commit message: the single PR commit ends with Emitted-Drift-Ack-Growth: plan-phase.md — documents the null totals ..., in the final trailer paragraph, no new file. Whether the lane is now green is unverified: the matrix has not run (Finding 1). |
Delta since the reviewed head: git patch-id --stable of the PR diff is identical for fcbb6bd153 and 7c94d1a181 (3e1080dc13e0, 21 files, +599/-10). The new head is a rebase plus the trailer; no code changed. Earlier-round findings (verify-side FILE test, stat-outcome message, contract docs, per-outcome reason, finally restore, could-not-parse revert, no-plans outcome, E1/E2 arms, CONFIGURATION.md, INVENTORY rows, exit-69 test, linear history) remain resolved in the tree I read. Author follow-ups on the PR thread are all answered by pushes; no unanswered question from the author or anyone else.
Classification & gate compliance
- #4939: labels
bug,confirmed-bug-> Bug-Fix track. Body hasFixes #4939. Not a draft. Titlefix(#4939): ...; branchfix/4939-bug-decision-coverage-a-phase-dir-that-dis valid. - Template: Fix PR template sections present. Changeset
.changeset/wise-dogs-frolic.md: typeFixed,pr: 5030, bold lead, symptom-led, trailing(#4939). It names all threereasonstrings that the gates answer plus the no-plans one. CHANGELOG.md untouched. - Docs:
Fixedis docs-exempt; docs/CONFIGURATION.md, docs/INVENTORY.md rows, gsd-core/references/verifier-phase-gates.md and gsd-core/workflows/plan-phase.md updated anyway. No new references/ or workflows/ file, so no INVENTORY manifest change. CONTEXT.md carries no decision-coverage vocabulary; no update needed. No ADR needed (bug fix inside existing behavior, mirrors #2770/#4794). - Slash syntax / hyphenated tags: no slash commands or angle-bracket tags added.
- Compact-content baseline: plan-phase
offTokens28447 -> 28509,onTokens25179 -> 25241 (+62 each), aggregate +62,reductionPct11.46 / 15.66: arithmetically self-consistent with a 224-byte growth. The generator output itself was not re-run (CI is the executor). - One concern per PR: yes. The two incidental test edits (tests/representative-corpus.test.cjs:219, tests/sdk-removal-query-family-dispatch.test.cjs:94) add a PLAN file because a plan-less phase dir is now a caller error; they are required by the change.
Must-Have Acceptance Checklist (issue #4939 + Agent Brief)
- [PASS, code level] "The gate should treat '0 plans loaded' as a caller error": nonexistent dir, file, unreadable dir and existing-dir-with-no-PLAN all fail closed with null totals (src/gate-decision-coverage-plan.cts:160-177).
- [PASS, code level]
cmdDecisionCoverageVerify"same shape, as a non-blocking warning": src/gate-decision-coverage-verify.cts:107-113 (stat arm) and :121-125 (no plan and no summary),gateVerdict('advisory', false, ...). - [PASS] Distinct fail-closed reason, message names the argument and says to pass the phase DIRECTORY (prose reasons per the file's vocabulary; the issue's
no-plans-foundwas an "for example"). - [PASS] Regression tests on both gates: tests/decisions.test.cjs new describe (in-process and CLI), unit U1o-U1r / U2o-U2r, cutover E1/E2 arms with goldens.
- [PASS] Correct form unchanged: control tests in tests/decisions.test.cjs; omitted phase dir left to the scan (#4130 pinned).
- Nothing in the checklist can be recorded as met by execution while the required test run has not executed (Finding 1).
Root-cause / functional verdict
Root cause: loadPlanContents answers [] for an absent or empty phase dir, indistinguishable from a phase whose plans cite nothing. The fix distinguishes the cases at one shared owner (phaseDirProblem, built on statEvidence) before the scan and on an empty scan result after it, in both gates. Not special-casing, no error swallowing. Fail-first: the NUMBER/FILE/no-plans tests assert reason and null totals, which the base cannot produce (base answers covered: 0 with all decisions uncovered; the exit-69 test cannot pass on base either). Correctness checks: guard sits after context/decision validation, so a context with no trackable decisions still green-skips first; phaseDir is already contained by resolveContainedPath before any stat; omitted phase-dir path is untouched; verify requires no PLAN and no SUMMARY so the summary-only R4 fixtures stay readable; nested plans/ is covered by the existing scanPhasePlans path.
Security lens: the argument is only stat-ed after containment, then echoed inside a JSON string via output() (JSON.stringify); no new argv, subprocess, path-write or prompt surface. Symlinks are followed by statSync exactly as the prior scan did. A stat/scan TOCTOU degrades to unreadable (fail-safe). Non-ENOENT/ENOTDIR errors never read as "missing". No finding.
Blast radius
Call graph: only routeCheckCommand reaches evaluateDecisionCoveragePlan / evaluateDecisionCoverageVerify; phaseDirProblem is new and called only from the two gates; loadPlanContents callers are the same two gates, both guarded. Output contract: a plan-less existing dir now blocks the plan gate with null totals (previously a covered: 0 block with an uncovered list) and yields a verify warning with null counts (previously honored: 0). Documented in verifier-phase-gates.md and plan-phase.md. I checked the other tests that invoke the gates on origin/next (check-decision-coverage-config, gate-decision-coverage-verify-stderr, check-gap-analysis-plan-post-e2e, check-decision-coverage-frontmatter-evidence): each builds a PLAN or SUMMARY in the phase dir, so none relies on an empty dir. gate-consumer-exit-awareness spans only the shell block of plan-phase.md/verifier-phase-gates.md, which the PR does not alter. Risk: Low (call graph), Medium (output contract, documented).
Co-change completeness
Changeset, CONFIGURATION.md, INVENTORY rows, verifier-phase-gates.md, plan-phase.md, compact baseline, unit cases, cutover arms + 6 goldens, and the two incidental tests are all in the diff. Merge-base drift (Finding 2) touches none of the PR's files (checked: git diff --name-only refs/prrev/5030...origin/next has no overlap; git merge-tree --write-tree origin/next refs/prrev/5030 exits clean, tree cbcdd068fd). No .cts/.cjs hand-written twin exists for the gates.
Test quality
Behavioral (spawns the CLI or calls routeCheckCommand in process); persisted/emitted artifact asserted by reason, null totals, omitted uncovered, message text and exit status (0 for delivered caller errors, 69 for unreadable) through a --require preload, no chmod, no wall-clock, no source-grep, restoreAll in finally. Controls assert the specific verdict (covered: 2 / honored: 2, reason undefined). ENOENT and ENOTDIR (not-found side) and EACCES (unreadable side) pinned for both gates, so a branch swap in phaseDirProblem is caught. No limit/boundary and no parser in this change, so limit-1/limit/limit+1 and fast-check do not apply. Goldens are byte-compared by tests/check-router-cutover-equivalence.test.cjs; the ctxdir fixture directory (tests/fixtures/gate-cutover/arms.cjs:101) holds only keep.txt, consistent with the no-plans arms. Not executed here (CI is the executor).
Memtrace Evidence
- get_impact: UNAVAILABLE — Memtrace MCP failed to connect (ENOENT).
- get_symbol_context: UNAVAILABLE — Memtrace MCP failed to connect (ENOENT).
- recall_decision: UNAVAILABLE — Memtrace MCP failed to connect (ENOENT). Not evidence that no recorded decision governs the change; ADR/CONTEXT.md predicates were checked by grep instead (no decision-coverage vocabulary, no governing ADR for this change).
- find_code_review_issues: UNAVAILABLE — Memtrace MCP failed to connect (ENOENT). No deterministic AST/YAML/cross-module pass ran; findings below are judgment unless marked
[fact]. - Substituted evidence:
gh pr diff/git diff origin/next...refs/prrev/5030(full diff read), CodeGraphcodegraph node -f/codegraph_explorefor the test helpers (writeMethodFailurePreload,runTools({preload})signatures match the new calls),git grepon origin/next for every other consumer of the two gate verbs,git merge-tree,git patch-idfor the delta, GitHub compare API.
CI (fresh this turn, head 7c94d1a181f026efee0fe0d42fe58d29162f87ce)
statusCheckRollup(latest per name): validate-target, Pull request template format, validate-title, Issue link required, GitGuardian, Socket x2 all SUCCESS. A supersededIssue link requiredrun (39057404101) shows CANCELLED; its replacement 39057422783 is SUCCESS.- Latest-per-name check-runs that are not success/skipped: none listed.
action_requiredruns for this head_sha (10, NOT released by me): 39057406708 Tests; 39057406611 Install Smoke; 39057406700 Mutation Testing; 39057406629 Security Scan; 39057406654 Docs Required; 39057406630 Changeset Required; 39057406685 Validate Branch Name; 39057406643 Default Flip Documentation; 39057422431 Default Flip Documentation; 39057406602 Dependabot Auto-Merge.- No failing check. The test matrix has never executed on this head.
Findings
Blocker
- [fact] CI matrix never executed for
7c94d1a181: 10 workflows sit ataction_required(IDs above), includingTests,Install Smoke,Mutation Testing,Security Scan. A PR whose matrix never ran is not approvable. Not author-fixable from a fork; the maintainer releases the runs, results to be surfaced verbatim. Until it runs green, theemitted-attributionfix, the regenerated compact baseline and the new goldens are unverified.
Nit
2. [fact] Branch is BEHIND next by 10 commits (head of next: 274f8cae13, #5314). No overlap with PR files; merge is clean. Fix: git fetch origin && git rebase origin/next, then re-run npm run build and node scripts/benchmark-compact-content.cjs --write only if the baseline moved, push (the trailer must survive the rebase).
3. [judgment] gsd-core/references/verifier-phase-gates.md:41-47: the added sentence ends a wrapped line with `not_honored` is `[]`: and continues with unwrapped prose ("nothing was checked, so ... A non-zero DECISION_EXIT ...") on one long line; the colon introduces nothing. Rewrap and use a period/semicolon.
4. [judgment] PR body is stale and partly contradictory: it lists emitted-attribution among 9 failures "on a clean next" while the same body says emitted-attribution passes against next (140 passed) and the commit now carries the trailer; next Tests is green. It also anchors "one commit on next aeabede" although next has moved, and reports only Linux/Claude Code. Update the Testing section to the current head and drop the unexplained "9 failures also fail on a clean next" claim (or leave All existing tests pass unchecked with a current reason).
Verdict
Changes requested. Substance of the #4939 fix is correct and complete against the issue's acceptance criteria; the prior Blocker (missing Emitted-Drift-Ack-Growth trailer) is resolved in the commit. Open: Blocker 1 (matrix unexecuted, maintainer releases the 10 action_required runs) and Nits 2-4 under zero tolerance. Substance clean, awaiting CI.
…g to scan decision-coverage-plan and decision-coverage-verify used to report a measured zero (covered: 0, every decision uncovered) when the phase-dir argument named nothing they could scan: the phase NUMBER, a file, an unreadable path, or a directory with no PLAN file. Both gates now check the phase dir through one shared helper (phaseDirProblem) before the scan and treat an empty scan as the same caller error after it. Totals are null, the plan gate still blocks, verify stays a non-blocking warning, and the message names the argument and asks for the phase DIRECTORY. - not found / not a directory / holds no plans: delivered caller error (exit 0); unreadable: UNAVAILABLE (exit 69). - Pinned in the gate unit suites, tests/decisions.test.cjs (in-process and through the CLI), and E1/E2 cutover arms with goldens. - Docs: CONFIGURATION.md, INVENTORY.md, verifier-phase-gates.md, plan-phase.md; changeset added; compact-content baseline regenerated. Emitted-Drift-Ack-Growth: plan-phase.md — documents the null totals the decision-coverage plan gate now answers for a caller error, including a bad phase dir or one with no plans (open-gsd#4939)
7c94d1a to
dedc9bd
Compare
trek-e
left a comment
There was a problem hiding this comment.
Head / CI note (verified at approval): reviewed at
dedc9bd8df; the current head6877d571ee6c70b72a6df922c8d853455b463ccfis that commit plus a merge ofnextonly (the PR's own changes are unchanged). I released the nine fork runs that were waiting for approval on this head, and CI is now complete: 37 distinct checks, none with a non-success latest run, includingTests,Install Smoke,Mutation TestingandSecurity Scan.
Review — fix(#4939) (re-review, approve)
Summary
The substance of the fix is correct and complete against the issue's acceptance criteria. The earlier Blocker (the missing Emitted-Drift-Ack-Growth trailer, which only the real CI run could catch) is resolved: the commit carries the trailer, and emitted-attribution passes in the executed matrix. Nits 3 and 4 from the earlier review (the wrapped sentence in verifier-phase-gates.md, and the stale PR body) are resolved. The only items in the latest review were the maintainer-side CI release, which is now done, and being behind next, which the refresh cleared.
Must-Have Acceptance Checklist (issue #4939)
Every acceptance criterion passes; see the full checklist in the isolated re-review of this head.
Memtrace Evidence
- get_impact: UNAVAILABLE — Memtrace MCP failed to connect (ENOENT)
- get_symbol_context: UNAVAILABLE — Memtrace MCP failed to connect (ENOENT)
- recall_decision: UNAVAILABLE — Memtrace MCP failed to connect (ENOENT)
- find_code_review_issues: UNAVAILABLE — Memtrace MCP failed to connect (ENOENT)
- Substitute evidence: isolated manual re-review of the diff at
dedc9bd8df; the executed CI matrix on the current head; issue #4939 acceptance criteria; the prior review text.
Findings
None at any severity.
Verdict
Approved on the exact head 6877d571ee6c70b72a6df922c8d853455b463ccf, with CI complete and green.
Fix PR
Linked Issue
Fixes #4939
What was broken
decision-coverage-plan/-verifynever checked the phase-dir argument was a directory. Passing the phase number instead madeloadPlanContentsload zero plans, and both gates read that as real zero coverage.What this fix does
Both gates now require the argument to be an existing directory that holds plans. A given value that is not a directory fails closed as a caller error (
reason: "phase directory not found",total/coverednull), and so does a directory with no PLAN file ("phase directory holds no plans"). Verify returns a non-blocking warning withhonored/totalnull instead, and treats a directory with SUMMARY files but no plans as readable. Omitting the argument is unchanged.Root cause
loadPlanContentsanswers[]for a missing path, indistinguishable from a phase whose plans genuinely cite nothing.Testing
How I verified the fix
On head dedc9bd (one commit on
next4d8c0ae, Linux, Node v26.5.1), afternpm ci --ignore-scripts && npm run build:lib:node --test tests/decisions.test.cjs: 203 passed, 0 failed.node --test tests/check-router-cutover-equivalence.test.cjs: 221 passed, 0 failed (includes the newphase-dir-no-plansE1/E2 arms).node --test tests/gate-decision-coverage-*.test.cjs: 37 passed, 0 failed.node --test tests/*compact*.test.*: 100 passed, 0 failed (baseline regenerated withnode scripts/benchmark-compact-content.cjs --writeon this base; it moved withnext).node --test tests/representative-corpus.test.cjs tests/sdk-removal-query-family-dispatch.test.cjs: 12 passed, 0 failed.npm run lint:ci: passes.node --test tests/emitted-attribution.test.cjs(againstnext): 140 passed, 0 failed.Regression test added?
Platforms tested
Runtimes tested
Standards followed
Bugfix within existing behavior, mirrors #2770/#4794's caller-error pattern; per
docs/contributor-standards.md, no new ADR needed.Checklist
Fixes #NNN— PR will be auto-closed if missingconfirmed-buglabelnpm test): not run in full locally on this head; the suites the change touches are listed above, and the full matrix runs in CI.changeset/fragment added if this is a user-facing fix (npm run changeset -- --type Fixed --pr <NNN> --body "...") — orno-changeloglabel appliedBreaking changes
None