Skip to content

fix(#4939): answer a caller error for a missing phase dir - #5030

Merged
trek-e merged 2 commits into
open-gsd:nextfrom
drakeo338:fix/4939-bug-decision-coverage-a-phase-dir-that-d
Oct 18, 2026
Merged

trek-e merged 2 commits into
open-gsd:nextfrom
drakeo338:fix/4939-bug-decision-coverage-a-phase-dir-that-d

Conversation

@drakeo338

@drakeo338 drakeo338 commented Sep 27, 2026 •

Copy link
Copy Markdown
Contributor

Fix PR


Linked Issue

Fixes #4939

The linked issue must have the confirmed-bug label. If it doesn't, ask a maintainer to confirm the bug before continuing.


What was broken

decision-coverage-plan/-verify never checked the phase-dir argument was a directory. Passing the phase number instead made loadPlanContents load 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/covered null), and so does a directory with no PLAN file ("phase directory holds no plans"). Verify returns a non-blocking warning with honored/total null instead, and treats a directory with SUMMARY files but no plans as readable. Omitting the argument is unchanged.

Root cause

loadPlanContents answers [] 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 next 4d8c0ae, Linux, Node v26.5.1), after npm 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 new phase-dir-no-plans E1/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 with node scripts/benchmark-compact-content.cjs --write on this base; it moved with next).
  • 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 (against next): 140 passed, 0 failed.

Regression test added?

  • Yes — added a test that would have caught this bug

Platforms tested

  • Linux
  • macOS
  • Windows (including backslash path handling)
  • N/A (not platform-specific)

Runtimes tested

  • Claude Code
  • Antigravity
  • OpenCode
  • Other: ___
  • N/A (not runtime-specific)

Standards followed

Bugfix within existing behavior, mirrors #2770/#4794's caller-error pattern; per docs/contributor-standards.md, no new ADR needed.

Checklist

  • Issue linked above with Fixes #NNN — PR will be auto-closed if missing
  • Linked issue has the confirmed-bug label
  • Fix is scoped to the reported bug — no unrelated changes included
  • Regression test added (or explained why not)
  • All existing tests pass (npm 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 "...") — or no-changelog label applied
  • No unnecessary dependencies added

Breaking changes

None

@trek-e trek-e left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 used no-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 isReadableDirectory in 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 status phase_dir_not_found. #5030 adds isExistingDirectory() 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 found vs phase_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

  1. 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 for check.decision-coverage-verify asserting reason, honored:null, total:null.
  2. 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.
  3. 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} into decision_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: note reason and that total/covered/honored are null on caller-error outcomes.
  4. Nit (pre-existing, same function, same defect class) — src/check-command-router.cts:514-531: verify's could-not-parse branch still answers total: 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: null total/honored there 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.

@trek-e trek-e added the review: changes requested PR reviewed — changes required before merge label Sep 27, 2026
drakeo338 added a commit to drakeo338/gsd-core that referenced this pull request Sep 28, 2026
…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
@drakeo338

Copy link
Copy Markdown
Contributor Author

Pushed bcaadbd with all four findings addressed and the current next merged in; reason stays 'phase directory not found' for every stat outcome, following the contextKind pattern, while the message names the actual one.

@drakeo338

Copy link
Copy Markdown
Contributor Author

@trek-e all four findings are addressed (bcaadbd) and the branch is up to date with next. Could you take another look when you have a moment?

@trek-e trek-e left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 (target cmdDecisionCoverageVerify, branch: next, upstream): LOW, 2 affected symbols — the sole caller routeCheckCommand (src/check-command-router.cts:1892-1995) and its file; process Gsd-coreRouteCheckCommand; graph_incomplete: false. (An earlier unscoped query was ambiguous across branches.)
  • get_symbol_context (target cmdDecisionCoveragePlan, src/check-command-router.cts): unscoped query ambiguous across branches, no callers/callees — inconclusive; the plan guard shares the routeCheckCommand caller.
  • recall_decision (query "decision coverage gate caller error null total covered unmeasured"): no on-topic decision. governing_rules on src/check-command-router.cts: ADR-443 only (unrelated).
  • find_code_review_issues (online, max_candidates: 50) on the src/check-command-router.cts diff (tests/docs excluded): 0 issues, _graph_state: ready; weak evidence.

Findings

  1. Minor [judgment] src/check-command-router.cts (plan guard ~:443, verify guard ~:557): reason stays "phase directory not found" even when the stat outcome is EACCES/EIO/file, so a machine consumer branching on reason cannot tell missing from unreadable (only the message text differs). You chose this deliberately; either emit distinct reason values or state in the contract docs that consumers must not branch on it.
  2. Nit [judgment] tests/decisions.test.cjs (runGateWithStatFailure): t.mock.restoreAll() is not in a finally, so a throw from routeCheckCommand leaves fs.writeSync mocked until the test ends.
  3. 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.

@drakeo338

Copy link
Copy Markdown
Contributor Author

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 trek-e left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@drakeo338
drakeo338 force-pushed the fix/4939-bug-decision-coverage-a-phase-dir-that-d branch from 011bb78 to 0413ddb Compare September 30, 2026 20:05
drakeo338 added a commit to drakeo338/gsd-core that referenced this pull request Sep 30, 2026
drakeo338 added a commit to drakeo338/gsd-core that referenced this pull request Sep 30, 2026
…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
@drakeo338

Copy link
Copy Markdown
Contributor Author

Rebased onto next (f4be7ed) and regenerated the compact-content benchmark baseline with the repo's --write generator instead of hand-merging it (0413ddb).

@trek-e trek-e added the needs-review: merge-conflict Has a merge conflict that must be resolved label Sep 30, 2026
@drakeo338
drakeo338 force-pushed the fix/4939-bug-decision-coverage-a-phase-dir-that-d branch from 0413ddb to 8ef455f Compare October 1, 2026 10:03
@drakeo338

Copy link
Copy Markdown
Contributor Author

Rebased onto next again; the phase-dir check now lives in the shared decision-coverage support module that both gates call, tests pass (8ef455f).

@drakeo338
drakeo338 force-pushed the fix/4939-bug-decision-coverage-a-phase-dir-that-d branch from 8ef455f to 6818eb9 Compare October 1, 2026 22:42
@drakeo338

Copy link
Copy Markdown
Contributor Author

Rebased onto next (2cc432e) again and regenerated the compact-content benchmark baseline, decisions tests pass (6818eb9).

@drakeo338
drakeo338 force-pushed the fix/4939-bug-decision-coverage-a-phase-dir-that-d branch from 6818eb9 to 59dadff Compare October 1, 2026 22:42

@trek-e trek-e left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 has Fixes #4939. Not a draft. Title fix(#4939): …, branch fix/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-conflict is STALE (see Merge). review: changes requested present.

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): returned ambiguous: 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 caller routeCheckCommand.
  • get_symbol_context (phaseDirProblem): found: false (new symbol; index is of next). 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_rules on 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_issues not run. detect_changes (4 changed files): affected_symbols: [] for all, i.e. index (of next) has no overlap with the PR-added symbols; not informative.
  • find_code for 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 stdout has "total": 0, "honored": 0; tests/check-router-cutover-equivalence.test.cjs:82 does a byte-for-byte assert.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.

@drakeo338
drakeo338 force-pushed the fix/4939-bug-decision-coverage-a-phase-dir-that-d branch from 59dadff to 02fa818 Compare October 2, 2026 14:23
@drakeo338

Copy link
Copy Markdown
Contributor Author

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.

@drakeo338
drakeo338 force-pushed the fix/4939-bug-decision-coverage-a-phase-dir-that-d branch from 02fa818 to d8bbaf9 Compare October 2, 2026 23:49

@trek-e trek-e left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.cts
  • src/gate-decision-coverage-plan.cts
  • src/gate-decision-coverage-verify.cts
  • gsd-core/references/verifier-phase-gates.md
  • tests/fixtures/compact-content-benchmark-baseline.json (regenerate with node scripts/benchmark-compact-content.cjs --write after 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.

@drakeo338
drakeo338 force-pushed the fix/4939-bug-decision-coverage-a-phase-dir-that-d branch from 71b04e9 to 2015d6f Compare October 3, 2026 21:43
@drakeo338

Copy link
Copy Markdown
Contributor Author

@trek-e merged next in (merge commit). The three src/ files keep #5171's typed evidence and exit-status model with the #4939 caller errors on top: null totals on a caller error, ENOENT/ENOTDIR as "not found", other stat failures through the unreadable path. The baseline is next's version regenerated with the script. I also trimmed the plan-phase.md note to fit the size cap.

@trek-e trek-e left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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-Growth trailer 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, behind next by 34, git merge-tree --write-tree origin/next refs/pr/5030 exit 0 (clean). The needs-review: merge-conflict label 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_node target statEvidence and gateUnreadable: "not found" (the index does not parse .cts and describes next); source verified with git grep on refs/pr/5030: statEvidence maps ABSENT_CODES to none and any other code to unreadable.
  • codegraph_impact / codegraph_callers targets evaluateDecisionCoveragePlan / evaluateDecisionCoverageVerify: no indexed data (.cts); callers from source and prior reviews: routeCheckCommand only.
  • codegraph_callees target phaseDirProblem: new symbol, absent from the base index; callee from source: statEvidence.

Memtrace unavailable: monthly query quota exhausted (20972/20933, resets 2026-10-20)

Findings

  1. Blocker [fact] CI matrix never executed for this head. action_required runs (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.
  2. 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.
  3. Nit [judgment] The branch carries two merge commits (Merge upstream/next ...) and is 34 behind. Run git fetch origin next && git rebase origin/next, resolve, regenerate the compact-content baseline with node scripts/benchmark-compact-content.cjs --write after the build, and push.
  4. Nit [judgment] docs/INVENTORY.md:589 description of gate-decision-coverage-verify.cjs omits 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.

@trek-e trek-e removed the needs-review: merge-conflict Has a merge conflict that must be resolved label Oct 7, 2026
@drakeo338
drakeo338 force-pushed the fix/4939-bug-decision-coverage-a-phase-dir-that-d branch from 82fdb30 to 4bf3543 Compare October 7, 2026 11:51
@drakeo338

Copy link
Copy Markdown
Contributor Author

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 node scripts/benchmark-compact-content.cjs --write after the build. The nine action_required matrix runs need a maintainer's approval to start; I can't release them from a fork.

@trek-e trek-e left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

  1. 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 all completed action_required. Only the 4 policy workflows (Target, Template Format, Title, Issue Link) plus GitGuardian and Socket x2 ran, all SUCCESS.
  2. 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/.
  3. 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-tree exit 0, clean). Baseline regenerated (commit 4bf3543).
  4. 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

  1. Blocker | CI | PR-level: 9 matrix workflows at action_required for 4bf3543; a PR whose matrix never executed is not approvable. Fix: maintainer releases fork runs; surface results verbatim.
  2. 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.
  3. 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.
  4. 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 unreadable arm (UNAVAILABLE/69) through the router; the cutover goldens only cover not-found/is-file (exitCode 0). A swap of gateUnreadable for gateVerdict('block') in phaseDirProblem consumers would keep JSON identical except outcome. U1q/U2q assert outcome: 'unreadable' so this is partly covered at the unit level; add a router-level exit assertion or an injected-stat cutover arm.
  5. 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 gives phase path is not a directory); the later sentence covers unreadable but not the file case. Fix: list the file reason.
  6. 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.

@drakeo338

Copy link
Copy Markdown
Contributor Author

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 trek-e left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

  1. Blocker CI: 9 matrix workflows unexecuted (action_required).
  2. Major Scope: verify could-not-parse total/honored 0 -> null (gate-decision-coverage-verify.cts, U2d/U2e, two goldens, verifier-phase-gates.md, changeset clause) is not in #4939. Remove or split out.
  3. 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.
  4. Nit 15 behind next with merge commits again; git fetch origin && git rebase origin/next (clean).
  5. Nit PR body cites head 72e2bc33f and 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 and CONTEXT.md predicates 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.

@drakeo338

Copy link
Copy Markdown
Contributor Author

Pushed 9a9b488:

  • Major 2: the verify could-not-parse change is reverted (source, U2d/U2e, both goldens, verifier-phase-gates.md, changeset).
  • Major 3: a phase dir with no PLAN file is now reason: "phase directory holds no plans" with null totals in both gates (U1r, U2r). For verify, it needs no PLAN and no SUMMARY file, since the R4 fixture is summary-only.
  • Nit 5: body updated.
  • Nit 4: I'll rebase onto next next.

@drakeo338
drakeo338 force-pushed the fix/4939-bug-decision-coverage-a-phase-dir-that-d branch from 9a9b488 to d540cfa Compare October 14, 2026 22:12

@trek-e trek-e left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 #4939 present, not draft, title fix(#4939): ..., branch fix/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-5030 merges 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/total null, 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 diff has 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 next now (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

  1. Maintainer-side gate, not a contributor defect [fact] CI matrix never executed for head b25ae44; the runs are held at action_required pending maintainer release, which is withheld while Minor 2 / Nits 3-5 are open (release after they are fixed). 9 runs at action_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.
  2. Minor [judgment] No cutover arm and no router/decisions.test.cjs case for phase directory holds no plans on 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.
  3. 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 with node scripts/benchmark-compact-content.cjs --write after build, push.
  4. Nit [fact] PR body cites "head 9a9b488"; current head is b25ae44. Update the body to the head tested.
  5. 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.

@drakeo338
drakeo338 force-pushed the fix/4939-bug-decision-coverage-a-phase-dir-that-d branch from b25ae44 to 5e49777 Compare October 17, 2026 15:31
@drakeo338

Copy link
Copy Markdown
Contributor Author

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 trek-e left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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-bug on #4939). Branch, title and Fixes #4939 are valid.
  • Emitted-attribution growth is unacknowledged (Blocker, fact). gsd-core/workflows/plan-phase.md is 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 no Emitted-Drift-Ack-Growth trailer.

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 log trailers 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

  1. [fact] gsd-core/workflows/plan-phase.md grows by 224 bytes with no Emitted-Drift-Ack-Growth: plan-phase.md — <reason> trailer on any commit in this PR. This fails tests/emitted-attribution.test.cjs (differential attribution over the real tree) on the macOS conformance lane and both Ubuntu test lanes, and fails Required 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.

@trek-e trek-e added the ci: failing CI checks are failing label Oct 17, 2026
@drakeo338
drakeo338 force-pushed the fix/4939-bug-decision-coverage-a-phase-dir-that-d branch from fcbb6bd to 7c94d1a Compare October 17, 2026 18:16
@drakeo338

Copy link
Copy Markdown
Contributor Author

Added the Emitted-Drift-Ack-Growth: plan-phase.md trailer to the PR commit (rebased onto next as one commit, 7c94d1a); tests/emitted-attribution.test.cjs passes locally against next.

@trek-e trek-e left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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. The Emitted-Drift-Ack-Growth trailer 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 has Fixes #4939. Not a draft. Title fix(#4939): ...; branch fix/4939-bug-decision-coverage-a-phase-dir-that-d is valid.
  • Template: Fix PR template sections present. Changeset .changeset/wise-dogs-frolic.md: type Fixed, pr: 5030, bold lead, symptom-led, trailing (#4939). It names all three reason strings that the gates answer plus the no-plans one. CHANGELOG.md untouched.
  • Docs: Fixed is 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 offTokens 28447 -> 28509, onTokens 25179 -> 25241 (+62 each), aggregate +62, reductionPct 11.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-found was 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), CodeGraph codegraph node -f / codegraph_explore for the test helpers (writeMethodFailurePreload, runTools({preload}) signatures match the new calls), git grep on origin/next for every other consumer of the two gate verbs, git merge-tree, git patch-id for 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 superseded Issue link required run (39057404101) shows CANCELLED; its replacement 39057422783 is SUCCESS.
  • Latest-per-name check-runs that are not success/skipped: none listed.
  • action_required runs 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

  1. [fact] CI matrix never executed for 7c94d1a181: 10 workflows sit at action_required (IDs above), including Tests, 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, the emitted-attribution fix, 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)
@drakeo338
drakeo338 force-pushed the fix/4939-bug-decision-coverage-a-phase-dir-that-d branch from 7c94d1a to dedc9bd Compare October 18, 2026 14:59
@drakeo338

Copy link
Copy Markdown
Contributor Author

Rebased onto next 4d8c0ae as dedc9bd (trailer kept, compact baseline regenerated), rewrapped the verifier-phase-gates sentence (Nit 3) and refreshed the Testing section (Nit 4); touched suites and emitted-attribution (140/0) pass locally.

@trek-e trek-e left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Head / CI note (verified at approval): reviewed at dedc9bd8df; the current head 6877d571ee6c70b72a6df922c8d853455b463ccf is that commit plus a merge of next only (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, including Tests, Install Smoke, Mutation Testing and Security 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.

@trek-e
trek-e merged commit 9d1ca20 into open-gsd:next Oct 18, 2026
44 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ci: failing CI checks are failing review: changes requested PR reviewed — changes required before merge

Projects

None yet

Development

Successfully merging this pull request may close these issues.

bug(decision-coverage): a phase dir that does not exist (e.g. the phase NUMBER passed as arg 1) loads 0 plans and reports covered: 0 as if measured

2 participants