* test(#3885): failing-first coverage for the depth bound and the manufactured wave verdict ADR-3473 §8.5 says a swallowed failure may not become an authoritative-looking answer. Three families do exactly that today; this commit pins each one RED. Measured on this tree, 2026-08-27: intel query, .planning/intel/file-roles.json nested 12000 deep -> exit 1, "Error: Maximum call stack size exceeded" searchJsonEntries / matchesInValue carry no depth parameter at all. The MAX_JSON_SEARCH_DEPTH = 48 bound existed in the retired SDK lineage (sdk/src/query/intel.ts at 11918dcc3^) and the surviving .cts lineage never received it. same fixture nested 48 and 49 deep -> both return total=1 at exit 0, truncated=undefined Nothing distinguishes "searched to the bottom" from "stopped looking". query phase-plan-index, a plan whose depends_on names an unresolvable token -> warnings: ["Plan 03-02: declared wave: 2 but depends_on DAG places it in wave 1"] The token is never mentioned. computeDependencyLevels drops the edge with `if (!resolvedDep) continue;`, every plan becomes a root, and the tool then reports the author's correct wave: as the thing that is wrong. countPhasePlansAndSummaries with fs.readdirSync throwing EACCES -> hasContext:false, indistinguishable from a phase that simply has no CONTEXT.md. context_read_error is undefined. The shapes these tests assert against, chosen here so the implementation has a target rather than inventing one later: `truncated: boolean` on the intel query result, `unresolved: Array<{plan, token}>` from computeDependencyLevels, and `context_read_error: string | null` per analyzed phase. Deliberately green, and they must stay that way — each stops the fix from over-firing: depth 48 is found and NOT flagged truncated (the ceiling is inclusive) a shallow miss reports no truncation (noise control, N1) 10,000 siblings at depth 2 are unaffected (the bound is DEPTH, N2) a genuine wave: mismatch on a fully-resolved DAG still warns (N3) a genuinely missing directory is absent, not an error the emitted depends_on display mapping still passes an unresolved token through verbatim — already pinned by the existing #3785 test, so no duplicate was added T31 asserts at the consumer's output per ADR-3180 Decision 4(b): it runs the real CLI and reads the emitted JSON, because a unit assertion on computeDependencyLevels would have passed throughout #3427's life. Design: .gsd/phase/feat-3885-no-silent-swallow/40-design.md Test matrix: .gsd/phase/feat-3885-no-silent-swallow/50-test-matrix.md Refs #3885 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * enhance(#3885): no silent swallow, and no verdict manufactured from dropped data Implements ADR-3473 §8.5. A failure or a gap in the input stops being absorbed into an output that reads as authoritative. The recursion bound, restored but NOT verbatim (src/intel.cts) MAX_JSON_SEARCH_DEPTH = 48 is threaded through searchJsonEntries and matchesInValue, which carried no depth parameter at all. The bound existed in the retired SDK lineage (sdk/src/query/intel.ts at 11918dcc3^) and the surviving .cts lineage never received it — §8.3's "a consolidation may not delete an invariant along with the surface that held it", demonstrated. Measured before: a .planning/intel file nested 12000 deep exits 1 with "Error: Maximum call stack size exceeded". Reachable from a project document. The original returned a bare `false` at the ceiling. Restoring that verbatim would trade a crash for a silent "no match" when the truth is "I stopped looking" — the same class this epic exists to close, and ADR-3473 Decision 4 forbids it. So the bound carries a truncation signal: nesting 47 -> found, truncated false nesting 48 -> found, truncated false (the ceiling is inclusive) nesting 49 -> not found, truncated TRUE nesting 12000 -> exit 0, truncated TRUE, no RangeError A shallow document that simply has no match reports truncated FALSE — the flag means "I stopped early", never "I found nothing", or it would be noise. The bound is on DEPTH: 10,000 siblings at depth 2 are unaffected. The dropped edge is named, and stops being blamed on the author (src/phase.cts) computeDependencyLevels dropped every unresolvable depends_on token with a bare `continue`. Each drop makes a plan a root, so the whole phase collapses to wave 1 — and cmdPhasePlanIndex then reported the author's CORRECT wave: as the thing that was wrong. Before: warnings: ["Plan 03-02: declared wave: 2 but depends_on DAG places it in wave 1"] After: warnings: ["Plan 03-02: depends_on token \"nonexistent-token-3427\" does not resolve to any plan in this phase — edge dropped, wave placement for this plan may be unreliable"] The suppression is PER PLAN, never blanket: a plan with a fully-resolved DAG and a genuinely wrong wave: still gets the mismatch warning. resolveDependencyId stays two-tier — the shortFormToId third tier is §8.3/Phase 6's rule and is deliberately not built here. The emitted depends_on display mapping still passes an unresolved token through verbatim (#3785). No artifact from failed inputs (gsd-core/workflows/review.md, #3352) A failed lane leaves no result file, so "every lane failed" is exactly "the aggregate JSONL has zero lines" — the gate condition already existed as a byproduct. REVIEWS.md is no longer written in that case, and the commit step is skipped with it. A budget-SKIPPED lane also leaves no file and is NOT counted as a failure. Per-lane output and non-empty .err are preserved to .review-diagnostics/ before `rm -rf "{run_dir}"` destroys the only record that the lanes failed at all; the commit step names one file, never a glob, so the diagnostics are not swept in. Unreadable is not absent (roadmap.cts, gap-checker.cts, init.cts x2) Four callers collapsed an EACCES on a phase directory into [] and reported hasContext:false — byte-identical to a phase that simply has no CONTEXT.md. Each now names the directory it could not read. A genuinely missing directory stays absent rather than becoming an error, which is what keeps the fix from over-firing. Fatal errno folded into a retry set: audited, no defect found Reported as a verified negative rather than padded with a change. withPlanningLock was fixed by #1884/PR #3472; acquireStateLock by #3776; atomicRenameWithRetry and estimate-cli's renameWithRetry are correct by construction — bounded set {EPERM,EBUSY,EACCES}, bounded attempts, and they return or rethrow the final error rather than swallowing it. estimate-cli's sole caller surfaces that rethrow as write_error in its JSON output. Manufacturing a diff to make the checkbox look worked-on is the Goodhart outcome Decision 6 exists to prevent. Disclosed: R46 (the commit step names one file, never a glob) is a real regression guard but is NOT independently failing-first — the commit fence is byte-identical pre- and post-fix, so it only fails pre-fix through its shared extraction dependency. Recorded rather than claimed as fail-first. Design: .gsd/phase/feat-3885-no-silent-swallow/40-design.md Test matrix: .gsd/phase/feat-3885-no-silent-swallow/50-test-matrix.md Refs #3885 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(#3885): escape untrusted tokens, and stop cleanup destroying unpreserved evidence Two review findings, both real, both in my own change. An isolated adversarial review found the evidence-preservation block never checked mkdir/cp exit status while `rm -rf "{run_dir}"` ran unconditionally in a SEPARATE fenced block. A disk-full or unwritable phase directory therefore still destroyed the only copy of the failed lanes' output — reintroducing the exact #3352 data loss this item exists to stop, inside the fix for it. Preservation and cleanup are now one block, because each fenced block is a separate execution and a shell variable cannot carry between them. mkdir -p and each cp are exit-checked; cleanup runs only when preservation succeeded, and a failure warns naming the intact run directory. "Nothing to preserve" is not a failure and still cleans up. Driven three ways: success removes run_dir, failure leaves it intact with the warning, nothing-to-preserve removes it. The failure is induced by a file-vs-directory conflict rather than chmod 0o000, which root bypasses. The new unresolved-depends_on warning embedded a user-authored token verbatim: warnings: ["Plan 03-02: depends_on token \"evil Plan 03-01: FORGED WARNING\" does not resolve ..."] The JSON wire form is safe, and the security reviewer judged it non-exploitable for that reason. It is escaped anyway through formatDiagnosticToken — the helper #3884 added one phase earlier for exactly this class. warnings[] is an array a consumer naturally prints line by line, and not reusing the sibling fix is the generative-fix-divergence shape this epic exists to close. The same treatment is applied to context_read_error / phase_dir_read_error, which embed a phase directory path a repository can choose, and to the fs error message, which echoes the raw path itself. Known limit L5 recorded: the bound is on DEPTH only. A 300,000-element shallow array yields a 14.5MB reply with truncated:false. Correct per §8.5 and per negative space N2, disclosed rather than left to be discovered. Refs #3885 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(#3885): unreadable is not absent in intel.cts either, and a corrupt snapshot is not "no snapshot" Blocker from the round-2 isolated review, and it is my own inconsistency: this phase applied "unreadable is not absent" to phase directories and left it broken in the file it was already editing. chmod 000 .planning/intel/file-roles.json gsd-tools intel query <term> -> {"matches":[],"total":0,"truncated":false} exit 0 safeReadJson swallowed every read failure and returned null, so an EACCES was byte-indistinguishable from an absent file AND from a genuine no-match. Now it separates three states: ENOENT stays silently absent, because not every project has every intel file and intelQuery loops over all of them expecting misses; EACCES/EIO and malformed JSON are both surfaced naming the file. A corrupt intel file previously read as "no matches" too — same defect, same fix. Threading that outcome through the other three callers found something worse than the reported case. intelDiff returned no_baseline:true for a corrupt or unreadable snapshot — not a silent failure but an actively FALSE verdict, telling the caller they never took a snapshot when they did. That is §8.5's headline case, so it is fixed and tested rather than noted. intelStatus and intelApiSurface collapsed the same way; intelApiSurface additionally printed a "not yet populated" banner that was simply untrue. Every row is failing-first, including the absent-file ones — the field is new, so it does not exist pre-fix at all. Those rows are not pre-fix pins; they pin that the fix does not OVER-fire on the ordinary absent case, which is what would turn this into noise on every project lacking an intel file. IO failure is injected by monkeypatching fs and restoring in finally, never chmod 0o000 — root bypasses mode bits, so the reviewer's manual chmod repro is not reproducible as a test. Refs #3885 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * test(#3885): build the pathological intel fixture as text, not by stringifying a nested object The remote runner came back red on Linux with two failures, both T4: deeplyNestedIntelDoesNotOverflowTheStack, while the same test passed on macOS. The product was never at fault. writeNestedFixture(12000) built a 12,000-deep JavaScript OBJECT and then JSON.stringify'd it. JSON.stringify recurses once per level, so it overflowed the TEST PROCESS's stack — the error was thrown before the CLI was ever spawned. Linux's container stack is smaller than macOS's, which is the whole of the platform difference. Measured, with the same document built as JSON TEXT so nothing in the building process recurses: depth=100 rc=0 truncated=true depth=5000 rc=0 truncated=true depth=12000 rc=0 truncated=true depth=60000 rc=0 truncated=true V8 parses this shape iteratively; only stringify recurses. The bound works at every depth tried. The fixture is now built by string concatenation. That is also the more faithful input — a real deeply nested JSON document on disk is exactly what the bound guards, where a stringified object was only ever a way to produce one. The depth stays 12000. Lowering it would have made the test pass by weakening it to accommodate a fixture bug, and 12000 is a legitimate pathological input the product handles. T4 remains a genuine fail-first: rebuilt against the parent of the commit that added the bound, the string-built depth-12000 fixture still drives the CLI to rc=1 with "Error: Maximum call stack size exceeded". A comment records why the fixture is text, so it is not "simplified" back into a macOS-green / Linux-red test. Refs #3885 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * chore(#3885): backfill the changeset PR number Refs #3885 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * test(#3885): normalize path separators before splicing into the workflow's bash CI red on one lane — test (windows-latest, 24, shard 3/3). macOS, Linux and the remote runner were all green. AssertionError: commit must name the single REVIEWS.md file; got: --files C:UsersRUNNER~1AppDataLocalTempgsd-3352-phasedir-mOKmuy/03-REVIEWS.md Every backslash in C:\Users\RUNNER~1\AppData\Local\Temp\... was eaten. The harness spliced an OS-native temp path into the extracted bash, and bash consumes \U, \A, \L and \T as escapes on an unquoted expansion. The same loss broke RUN_DIR, so "rm -rf" targeted a path that never existed and the run directory survived — which is the other two assertions. This is a fixture defect, not a product one, and that was checked rather than assumed. In production the phase directory is toPosixPath-normalized at every call site that serializes it (bin/lib/init.cjs:951, 1381, 1461, 1529, 1595), and the run directory is created by "mktemp -d" running inside the bash block itself (gsd-core/workflows/review.md:163), which emits POSIX-style output even under Git-Bash on Windows. Neither ever carries a backslash where the workflow reads it. The file's pre-existing #3034 harness splices raw native paths too, but only ever inside double-quoted assignments, so it never tripped this — my new harness followed that convention faithfully into the one place where it does not hold. Both now splice through toPosixPath from shell-command-projection, the established seam, which is a no-op on POSIX and mirrors what production does. No assertion was weakened. "commit must name the single REVIEWS.md file" and "the run dir must still be destroyed" still assert exactly that; only how the fixture supplies its path changed. Nothing is skipped on Windows — a t.skip() here would have hidden the question of whether the exposure was real, which is the question that mattered. Driven both ways: a synthetic C:\Users\RUNNER~1\... input reproduces the exact CI string when unfixed and yields C:/Users/RUNNER~1/... when fixed; a POSIX input produces a byte-identical shape, proving the normalization is idempotent. Refs #3885 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * test(#3885): stop the harness making the deleted run dir its own cwd Windows shard 3/3 stayed red after the separator fix, on two assertions the separator fix never touched: AssertionError: the run dir must still be destroyed AssertionError: nothing to preserve is not a failure — run dir must still be removed The separators were a real bug and fixing them fixed the --files assertion. They were not this bug, and two CI cycles went into the wrong axis before I stopped converting path forms and looked at what the harness actually does. runWriteReviewsFlow passed cwd: runDir to runHook, so the child bash process's working directory WAS the directory the block under test then removes with rm -rf "$RUN_DIR". POSIX allows a process to delete its own cwd — verified locally, cd "$d"; rm -rf "$d" removes it cleanly — and Windows does not: a live process's working directory cannot be removed. So on Windows the directory survived and both assertions failed, on macOS and Linux it vanished and they passed. Nothing to do with slashes. Harness-only. Production never cd's into the run directory; every reference is by absolute path, and RUN_DIR is created by mktemp -d inside the bash block itself (gsd-core/workflows/review.md:165) rather than injected. review.md is unchanged. Fix: the child now runs with its cwd in an unrelated temp directory that the block under test never deletes. Neither assertion was weakened, and nothing is skipped on Windows — the tests in this file carry no platform guard and run there unconditionally, which is how this surfaced at all. Honest limit: the Windows failure mode cannot be reproduced on macOS, because POSIX permits the very thing Windows refuses. The diagnosis is grounded in that documented divergence and in the fact that only the Windows lane failed, but the green outcome on windows-latest is unverified until CI runs it. Refs #3885 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> --------- Co-authored-by: sim <sim@local> Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
This commit is contained in:
Tom Boucher
5
.changeset/bold-jays-travel.md
Normal file
5
.changeset/bold-jays-travel.md
Normal file
@@ -0,0 +1,5 @@
|
||||
---
|
||||
type: Changed
|
||||
pr: 3925
|
||||
---
|
||||
**Diagnostics stop reporting a clean result when they had to drop data to get one.** `intel query`'s recursive search now stops at 48 levels and marks the result `truncated: true` instead of quietly matching arbitrarily deep (a match past the ceiling now reports truncated rather than found, and no longer crashes with a stack overflow past ~12000 levels); `phase-plan-index` now names an unresolved `depends_on` token in its own warning instead of blaming the plan's declared `wave:` for a dependency edge the tool itself dropped, and that warning's own token is escaped so an attacker-authored token cannot forge a second warning line; and a code-review run where every lane failed no longer writes a `REVIEWS.md` synthesized from nothing, preserving each lane's raw output first. (#3885)
|
||||
@@ -251,6 +251,38 @@ Scope and limits, so the output is not read as more than it is:
|
||||
Every path the scan does recover is checked — there is no cap. The standalone
|
||||
`verify-summary` verb keeps its historical default of checking the first two.
|
||||
|
||||
### `phase-plan-index`: unresolved `depends_on` tokens (ADR-3473 §8.5, #3427/#3885)
|
||||
|
||||
A plan's `depends_on:` token must resolve to another plan in the same phase (by
|
||||
exact id or by canonical id). When a token resolves to neither, the edge is
|
||||
dropped and the dependent plan becomes a DAG root — `phase-plan-index` now
|
||||
names this in `warnings[]` instead of silently discarding it:
|
||||
|
||||
```text
|
||||
Plan 03-02: depends_on token "typo-plan-id" does not resolve to any plan in this
|
||||
phase — edge dropped, wave placement for this plan may be unreliable
|
||||
```
|
||||
|
||||
The token is escaped (quoted, control characters and embedded newlines
|
||||
backslash-escaped) before it is embedded in the warning, so a `depends_on`
|
||||
value crafted to contain a newline or a quote cannot forge a second,
|
||||
fabricated warning entry when `warnings[]` is printed one-per-line.
|
||||
|
||||
**The wave-mismatch warning is suppressed for an affected plan.** Normally a
|
||||
plan whose declared `wave:` disagrees with the computed DAG wave gets its own
|
||||
warning (`"declared wave: N but depends_on DAG places it in wave M"`). When
|
||||
the disagreement is caused by a dropped edge on that same plan, that warning
|
||||
would blame the author for a mismatch the tool itself manufactured by losing
|
||||
an edge — so it does not fire for that plan; the unresolved-token warning above
|
||||
stands in its place. A plan with **no** dropped edges and a genuinely wrong
|
||||
`wave:` still gets the mismatch warning as before.
|
||||
|
||||
This does not repair `waves` / `wave` themselves — those fields stay computed
|
||||
from the DAG with the edge missing, since the edge cannot be invented. A
|
||||
consumer using `wave` for scheduling (`WAVE_FILTER`, the wave-safety check)
|
||||
is still working from the degraded assignment; only the diagnostic surfaces
|
||||
the loss.
|
||||
|
||||
---
|
||||
|
||||
## Roadmap Commands
|
||||
@@ -543,6 +575,42 @@ This command is strictly read-only — no config writes, no disk mutation. See [
|
||||
|
||||
---
|
||||
|
||||
## Intel Commands
|
||||
|
||||
```bash
|
||||
node gsd-tools.cjs intel query <term>
|
||||
```
|
||||
|
||||
Searches every JSON intel file under `.planning/intel/` (keys and values, including
|
||||
`arch-decisions.json`) for `<term>`. No-ops with `{ enabled: false }` when the `intel`
|
||||
capability is not active (`intel.enabled` in config).
|
||||
|
||||
**Output JSON:**
|
||||
|
||||
```json
|
||||
{ "matches": [{ "source": "file-roles.json", "entries": [...] }], "term": "…", "total": 3, "truncated": false }
|
||||
```
|
||||
|
||||
| Field | Type | Description |
|
||||
|---|---|---|
|
||||
| `total` | number | Count of matched entries across every intel file |
|
||||
| `truncated` | boolean | `true` when the recursive walk of at least one intel file hit the 48-level depth ceiling before finishing — see below |
|
||||
|
||||
### The 48-level recursion ceiling and `truncated` (ADR-3473 §8.5, #3885)
|
||||
|
||||
The search recurses into nested objects/arrays up to **48 levels deep** (the bound is on
|
||||
depth, not breadth or total node count — a wide-but-shallow structure is unaffected). A
|
||||
match at or above the ceiling is not returned, and `truncated` is set to `true` on the
|
||||
result so a caller can tell "I stopped looking" apart from "there is no match here."
|
||||
|
||||
`truncated: false` means the walk reached the bottom of every branch it visited — it does
|
||||
**not** by itself mean anything was found; check `total` for that. Before this fix, a
|
||||
search past the ceiling threw an uncaught `RangeError: Maximum call stack size exceeded`
|
||||
instead of returning a diagnosable result; a shallower search (depth ≤ 48) is unaffected
|
||||
and its result is unchanged.
|
||||
|
||||
---
|
||||
|
||||
## Model Resolution
|
||||
|
||||
```bash
|
||||
|
||||
@@ -198,6 +198,7 @@
|
||||
- [Stated Failing Direction](#167-stated-failing-direction)
|
||||
- [Runtime Identity](#168-runtime-identity)
|
||||
- ["Failure Is a Value" — Strict Argv Rejection and the `--pick` Absence Contract](#3884-failure-is-a-value--strict-argv-rejection-and-the---pick-absence-contract)
|
||||
- [No Silent Swallow, No Verdict From Dropped Data](#3885-no-silent-swallow-no-verdict-from-dropped-data)
|
||||
|
||||
---
|
||||
|
||||
@@ -3738,6 +3739,81 @@ nullglob hazard) is untouched. See
|
||||
|
||||
---
|
||||
|
||||
### 3885. No Silent Swallow, No Verdict From Dropped Data
|
||||
|
||||
**Purpose:** ADR-3473 §8.5 states the rule directly: a failure or a gap in the
|
||||
input must not be absorbed into an output that reads as authoritative. A
|
||||
routine that drops data it could not read or could not resolve, and then
|
||||
reports a clean result anyway, turns a diagnosable gap into a confidently
|
||||
wrong answer. This closes four instances of that collapse found across
|
||||
`gsd-tools`.
|
||||
|
||||
**`intel query` no longer crashes past ~12000 levels of nesting (#3427).**
|
||||
`searchJsonEntries` / `matchesInValue` recursed with no depth bound at all —
|
||||
an intel JSON file nested deeply enough overflowed the call stack with an
|
||||
uncaught `RangeError` instead of a diagnosis. The original SDK-era bound
|
||||
(`MAX_JSON_SEARCH_DEPTH = 48`, lost in the ADR-0174 consolidation) is
|
||||
restored, paired with a `truncated` result field: a match at or above the
|
||||
ceiling is not returned, and the result says so rather than reporting a bare
|
||||
"not found" that is indistinguishable from a genuine miss. A match at depth
|
||||
48 (inclusive) or shallower is unaffected; the bound is on nesting depth, not
|
||||
breadth or total node count, so a shallow object with many siblings still
|
||||
works unchanged.
|
||||
|
||||
**`phase-plan-index` no longer blames the author for an edge the tool itself
|
||||
dropped (#3427).** A `depends_on:` token that resolves to no plan in the
|
||||
phase (typo, or a stale cross-phase reference) silently dropped that edge,
|
||||
making the dependent plan a DAG root — its own docstring recorded the intent
|
||||
as "ignore this edge, never a throw." The tool then compared the resulting
|
||||
degraded wave against the plan's declared `wave:` and reported the *author's
|
||||
correct* declaration as a mismatch. The unresolved token is now named in its
|
||||
own `warnings[]` entry (plan and token together), and the wave-mismatch
|
||||
warning is suppressed for that plan only — a plan with no dropped edges and a
|
||||
genuinely wrong `wave:` still warns as before. The token is escaped
|
||||
(quoted, control characters and embedded newlines backslash-escaped) before
|
||||
it is embedded in the warning text, so a `depends_on` value crafted to
|
||||
contain a newline or a quote cannot forge a second, fabricated warning entry.
|
||||
|
||||
**A code-review run where every lane failed no longer writes `REVIEWS.md`
|
||||
from nothing (#3352).** `review.md`'s aggregation step wrote `REVIEWS.md`
|
||||
regardless of whether any lane actually produced results — a run where every
|
||||
lane failed still emitted a completed-looking review artifact, and the
|
||||
per-lane outputs and `.err` files that would have explained the failure were
|
||||
then destroyed by the run's own cleanup. `REVIEWS.md` is now withheld when
|
||||
the aggregate has zero lines (every lane failed, not merely skipped under a
|
||||
lower budget), the run reports the failure instead, and per-lane outputs and
|
||||
non-empty `.err` files are preserved beside the phase's artifacts before
|
||||
cleanup runs.
|
||||
|
||||
**Unreadable directories are distinguished from absent ones (#3473 B5).**
|
||||
Four call sites collapsed an `EACCES`/`EIO` on a phase directory into the
|
||||
same "nothing here" result as a directory that genuinely does not exist —
|
||||
`countPhasePlansAndSummaries` (`hasContext:false`), `runGapAnalysis`, and two
|
||||
guarded blocks in `init.cts` (`context_path` absent). Each now distinguishes
|
||||
"could not read" from "does not exist" and names the discarded path and
|
||||
error in a dedicated field (`context_read_error` / `phase_dir_read_error`)
|
||||
rather than silently reading as absent.
|
||||
|
||||
**Audited, no defect found:** every retry-set / swallowed-catch call site
|
||||
this phase's rule covers that had not already been fixed by a prior PR
|
||||
(`withPlanningLock`, `acquireStateLock`, `atomicRenameWithRetry`,
|
||||
`renameWithRetry`) was reviewed and found to already fail loudly on a fatal
|
||||
errno rather than folding it into a retry.
|
||||
|
||||
**Known limits:**
|
||||
- A match deeper than 48 levels is still not surfaced by `intel query` — it
|
||||
is reported as truncated rather than as absent, but the value itself is
|
||||
not returned. Raising the ceiling is a separate decision.
|
||||
- `phase-plan-index`'s `waves` / `wave` fields remain computed from the
|
||||
degraded DAG when an edge is dropped — this phase stops the tool from
|
||||
manufacturing a false verdict about it, but does not invent the missing
|
||||
edge. A consumer that schedules work from `wave` (e.g. `--wave N`
|
||||
filtering) is still working from the degraded assignment.
|
||||
- `review.md`'s evidence preservation is bounded by what a lane actually
|
||||
wrote — a lane that produced no output at all leaves nothing to preserve.
|
||||
|
||||
---
|
||||
|
||||
_Generated by `scripts/gen-features.cjs` — add a fragment under `docs/features/` and run `--write`._
|
||||
|
||||
<!-- FEATURES:END -->
|
||||
|
||||
@@ -0,0 +1,76 @@
|
||||
---
|
||||
id: 3885
|
||||
title: No Silent Swallow, No Verdict From Dropped Data
|
||||
group: v1.7.0 Features
|
||||
---
|
||||
|
||||
**Purpose:** ADR-3473 §8.5 states the rule directly: a failure or a gap in the
|
||||
input must not be absorbed into an output that reads as authoritative. A
|
||||
routine that drops data it could not read or could not resolve, and then
|
||||
reports a clean result anyway, turns a diagnosable gap into a confidently
|
||||
wrong answer. This closes four instances of that collapse found across
|
||||
`gsd-tools`.
|
||||
|
||||
**`intel query` no longer crashes past ~12000 levels of nesting (#3427).**
|
||||
`searchJsonEntries` / `matchesInValue` recursed with no depth bound at all —
|
||||
an intel JSON file nested deeply enough overflowed the call stack with an
|
||||
uncaught `RangeError` instead of a diagnosis. The original SDK-era bound
|
||||
(`MAX_JSON_SEARCH_DEPTH = 48`, lost in the ADR-0174 consolidation) is
|
||||
restored, paired with a `truncated` result field: a match at or above the
|
||||
ceiling is not returned, and the result says so rather than reporting a bare
|
||||
"not found" that is indistinguishable from a genuine miss. A match at depth
|
||||
48 (inclusive) or shallower is unaffected; the bound is on nesting depth, not
|
||||
breadth or total node count, so a shallow object with many siblings still
|
||||
works unchanged.
|
||||
|
||||
**`phase-plan-index` no longer blames the author for an edge the tool itself
|
||||
dropped (#3427).** A `depends_on:` token that resolves to no plan in the
|
||||
phase (typo, or a stale cross-phase reference) silently dropped that edge,
|
||||
making the dependent plan a DAG root — its own docstring recorded the intent
|
||||
as "ignore this edge, never a throw." The tool then compared the resulting
|
||||
degraded wave against the plan's declared `wave:` and reported the *author's
|
||||
correct* declaration as a mismatch. The unresolved token is now named in its
|
||||
own `warnings[]` entry (plan and token together), and the wave-mismatch
|
||||
warning is suppressed for that plan only — a plan with no dropped edges and a
|
||||
genuinely wrong `wave:` still warns as before. The token is escaped
|
||||
(quoted, control characters and embedded newlines backslash-escaped) before
|
||||
it is embedded in the warning text, so a `depends_on` value crafted to
|
||||
contain a newline or a quote cannot forge a second, fabricated warning entry.
|
||||
|
||||
**A code-review run where every lane failed no longer writes `REVIEWS.md`
|
||||
from nothing (#3352).** `review.md`'s aggregation step wrote `REVIEWS.md`
|
||||
regardless of whether any lane actually produced results — a run where every
|
||||
lane failed still emitted a completed-looking review artifact, and the
|
||||
per-lane outputs and `.err` files that would have explained the failure were
|
||||
then destroyed by the run's own cleanup. `REVIEWS.md` is now withheld when
|
||||
the aggregate has zero lines (every lane failed, not merely skipped under a
|
||||
lower budget), the run reports the failure instead, and per-lane outputs and
|
||||
non-empty `.err` files are preserved beside the phase's artifacts before
|
||||
cleanup runs.
|
||||
|
||||
**Unreadable directories are distinguished from absent ones (#3473 B5).**
|
||||
Four call sites collapsed an `EACCES`/`EIO` on a phase directory into the
|
||||
same "nothing here" result as a directory that genuinely does not exist —
|
||||
`countPhasePlansAndSummaries` (`hasContext:false`), `runGapAnalysis`, and two
|
||||
guarded blocks in `init.cts` (`context_path` absent). Each now distinguishes
|
||||
"could not read" from "does not exist" and names the discarded path and
|
||||
error in a dedicated field (`context_read_error` / `phase_dir_read_error`)
|
||||
rather than silently reading as absent.
|
||||
|
||||
**Audited, no defect found:** every retry-set / swallowed-catch call site
|
||||
this phase's rule covers that had not already been fixed by a prior PR
|
||||
(`withPlanningLock`, `acquireStateLock`, `atomicRenameWithRetry`,
|
||||
`renameWithRetry`) was reviewed and found to already fail loudly on a fatal
|
||||
errno rather than folding it into a retry.
|
||||
|
||||
**Known limits:**
|
||||
- A match deeper than 48 levels is still not surfaced by `intel query` — it
|
||||
is reported as truncated rather than as absent, but the value itself is
|
||||
not returned. Raising the ceiling is a separate decision.
|
||||
- `phase-plan-index`'s `waves` / `wave` fields remain computed from the
|
||||
degraded DAG when an edge is dropped — this phase stops the tool from
|
||||
manufacturing a false verdict about it, but does not invent the missing
|
||||
edge. A consumer that schedules work from `wave` (e.g. `--wave N`
|
||||
filtering) is still working from the degraded assignment.
|
||||
- `review.md`'s evidence preservation is bounded by what a lane actually
|
||||
wrote — a lane that produced no output at all leaves nothing to preserve.
|
||||
@@ -68,6 +68,14 @@ lane you selected must appear there. That list is the contract: lanes are joined
|
||||
is rendered, so a missing reviewer means that lane did not produce a review — never that
|
||||
aggregation ran early.
|
||||
|
||||
**If every selected lane failed, `REVIEWS.md` is not written at all** (ADR-3473 §8.5, #3885) — the
|
||||
run reports the failure instead of synthesizing a review artifact from zero lane results. This is
|
||||
not specific to parallel dispatch (a sequential run where every lane fails behaves the same way),
|
||||
but concurrency gives you more ways to lose every lane in one pass. Each lane's raw output and any
|
||||
non-empty `.err` file are preserved beside the phase's artifacts before the run's own cleanup runs,
|
||||
so a missing `REVIEWS.md` is diagnosable, not silent — see the table below for what a *partial*
|
||||
failure (some, not all, lanes down) looks like instead.
|
||||
|
||||
Section order in `REVIEWS.md`, and line order in the run's `gsd-review-lane-results.jsonl`, are
|
||||
unchanged from sequential dispatch. They follow reviewer-selection order, not completion order,
|
||||
so a diff of two runs shows no reordering churn.
|
||||
|
||||
@@ -481,6 +481,61 @@ Display progress:
|
||||
</step>
|
||||
|
||||
<step name="write_reviews">
|
||||
**#3352 (ADR-3473 §8.5): no artifact from failed inputs.** Before rendering anything, gate on
|
||||
whether any lane actually produced a result — "every lane failed" is exactly "the aggregate
|
||||
JSONL has zero lines" (§`invoke_reviewers`'s aggregation loop already builds this file as a
|
||||
byproduct; a lane that never started or was budget-skipped contributes no line either way).
|
||||
|
||||
```bash
|
||||
RUN_DIR="{run_dir}"
|
||||
JSONL="$RUN_DIR/gsd-review-lane-results.jsonl"
|
||||
LANE_LINES=0
|
||||
[ -f "$JSONL" ] && LANE_LINES=$(wc -l < "$JSONL" | tr -d ' ')
|
||||
|
||||
TOTAL_LANE_FAILURE="false"
|
||||
ALL_LANES_SKIPPED="false"
|
||||
if [ "${LANE_LINES:-0}" -eq 0 ]; then
|
||||
# Zero lines means every dispatched lane left no result JSON — either every one
|
||||
# was budget-skipped (N5: a skip is not a failure) or every one actually failed
|
||||
# to run. Re-derive the dispatched-slug set the same way invoke_reviewers did
|
||||
# (SELECTED_REVIEWERS is a shell block boundary — recompute, do not assume the
|
||||
# earlier step's local DISPATCH_SLUGS variable survived into this block).
|
||||
DISPATCH_SLUGS=""
|
||||
for SLUG in $(echo "$SELECTED_REVIEWERS" | tr ',' ' '); do
|
||||
case " $DISPATCH_SLUGS " in
|
||||
*" $SLUG "*) continue ;;
|
||||
esac
|
||||
DISPATCH_SLUGS="$DISPATCH_SLUGS $SLUG"
|
||||
done
|
||||
# Distinguish by whether every dispatched slug's stub markdown says "skipped":
|
||||
# a skip stub always does (see run_review_lane's budget branch, which writes
|
||||
# this exact text before returning without ever invoking the lane); a real
|
||||
# failure stub does not. If a slug has no stub at all, it is not a skip.
|
||||
DISPATCHED_COUNT=0
|
||||
SKIPPED_COUNT=0
|
||||
for SLUG in $DISPATCH_SLUGS; do
|
||||
DISPATCHED_COUNT=$((DISPATCHED_COUNT + 1))
|
||||
STUB="$RUN_DIR/gsd-review-$SLUG.md"
|
||||
if [ -f "$STUB" ] && grep -q "review skipped: prompt budget" "$STUB" 2>/dev/null; then
|
||||
SKIPPED_COUNT=$((SKIPPED_COUNT + 1))
|
||||
fi
|
||||
done
|
||||
if [ "$DISPATCHED_COUNT" -gt 0 ] && [ "$SKIPPED_COUNT" -eq "$DISPATCHED_COUNT" ]; then
|
||||
ALL_LANES_SKIPPED="true"
|
||||
else
|
||||
TOTAL_LANE_FAILURE="true"
|
||||
fi
|
||||
fi
|
||||
```
|
||||
|
||||
- **If `ALL_LANES_SKIPPED=true`:** do NOT write `REVIEWS.md` and do NOT run the commit below —
|
||||
there is nothing to review. Report to the user that every selected lane was budget-skipped
|
||||
(not a failure) and stop; do not proceed to `present_results`' summary claiming a review ran.
|
||||
- **If `TOTAL_LANE_FAILURE=true`:** do NOT write `REVIEWS.md` and do NOT run the commit below.
|
||||
Report the total lane failure to the user (name the lanes that were dispatched and point at
|
||||
their `.err`/stub files preserved under `.review-diagnostics/` by `present_results`) and stop.
|
||||
- **Otherwise** (at least one lane produced a result — R1, unchanged): proceed exactly as below.
|
||||
|
||||
Combine all review responses into `{phase_dir}/{padded_phase}-REVIEWS.md`:
|
||||
|
||||
After all reviewers complete, collect trim metadata files written during the run. For each reviewer that was trimmed (i.e. a `.metadata.json` file exists and `hardFailed` or `omitted` is non-empty, or `projectMdShrunk` is true, or `planTruncationPct > 0`), include a `trimmed_reviewers` block in the frontmatter. Omit the key entirely if no reviewer was trimmed.
|
||||
@@ -564,14 +619,30 @@ trimmed_reviewers: # only present if at least one reviewer was trimmed
|
||||
{where reviewers disagreed — worth investigating}
|
||||
```
|
||||
|
||||
Commit:
|
||||
Commit (only reached when `TOTAL_LANE_FAILURE` and `ALL_LANES_SKIPPED` are both `false` — the
|
||||
gate above):
|
||||
```bash
|
||||
gsd_run query commit "docs: cross-AI review for phase {N}" --files {phase_dir}/{padded_phase}-REVIEWS.md
|
||||
```
|
||||
</step>
|
||||
|
||||
<step name="present_results">
|
||||
Display summary:
|
||||
**If `write_reviews` set `TOTAL_LANE_FAILURE=true` or `ALL_LANES_SKIPPED=true`, skip the success
|
||||
summary below entirely** — no `REVIEWS.md` was written or committed, so there is nothing to
|
||||
present as complete. Report instead:
|
||||
|
||||
```
|
||||
### GSD ► REVIEW FAILED
|
||||
|
||||
Phase {N}: every selected reviewer lane {failed to produce a result|was budget-skipped} — no
|
||||
REVIEWS.md was written.
|
||||
|
||||
{If the preserve+cleanup block below reports success: "Diagnostics preserved:
|
||||
{phase_dir}/.review-diagnostics/". If it reports failure: relay its own warning verbatim —
|
||||
it names the intact run directory holding the un-preserved evidence instead.}
|
||||
```
|
||||
|
||||
Otherwise (at least one lane succeeded), display summary:
|
||||
|
||||
```
|
||||
### GSD ► REVIEW COMPLETE
|
||||
@@ -587,10 +658,52 @@ To incorporate feedback into planning:
|
||||
/gsd:plan-phase {N} --reviews
|
||||
```
|
||||
|
||||
Clean up — remove the run's temp directory now that REVIEWS.md is committed:
|
||||
**#3352 (ADR-3473 §8.5, R3): preserve per-lane evidence before destroying it.** Regardless of
|
||||
which branch above ran, the run's temp directory is the only record that a lane failed at all —
|
||||
copy it beside the phase's artifacts BEFORE cleanup. A lane that produced no output at all (L4)
|
||||
leaves nothing to preserve; that is a smaller diagnostics folder, not a fabricated one, and is
|
||||
NOT a preservation failure. This copy is deliberately NOT part of the commit above (N6) — that
|
||||
step names only `{padded_phase}-REVIEWS.md` explicitly, never a directory glob, so
|
||||
`.review-diagnostics/` is never swept into it.
|
||||
|
||||
Preservation and cleanup MUST run in the same fenced block below (a shell variable cannot
|
||||
survive across separate fences — each is its own process). `mkdir -p` and every `cp` are
|
||||
exit-status checked; `rm -rf "$RUN_DIR"` runs ONLY if nothing was preserved (nothing to
|
||||
preserve is not a failure) or everything that needed preserving was copied successfully. If
|
||||
preservation fails partway, `$RUN_DIR` is left intact and a message names it as the location of
|
||||
the un-preserved evidence — a leftover temp directory is far cheaper than destroyed evidence:
|
||||
|
||||
```bash
|
||||
rm -rf "{run_dir}"
|
||||
shopt -s nullglob 2>/dev/null; setopt NULL_GLOB 2>/dev/null
|
||||
|
||||
RUN_DIR="{run_dir}"
|
||||
DIAG_DIR="{phase_dir}/.review-diagnostics"
|
||||
|
||||
_DIAG_MD=( "$RUN_DIR"/gsd-review-*.md )
|
||||
_DIAG_ERR=()
|
||||
for f in "$RUN_DIR"/gsd-review-*.err; do
|
||||
[ -s "$f" ] && _DIAG_ERR+=("$f")
|
||||
done
|
||||
|
||||
_PRESERVE_OK=true
|
||||
if [ ${#_DIAG_MD[@]} -gt 0 ] || [ ${#_DIAG_ERR[@]} -gt 0 ]; then
|
||||
if mkdir -p "$DIAG_DIR"; then
|
||||
if [ ${#_DIAG_MD[@]} -gt 0 ] && ! cp "${_DIAG_MD[@]}" "$DIAG_DIR/"; then
|
||||
_PRESERVE_OK=false
|
||||
fi
|
||||
if [ ${#_DIAG_ERR[@]} -gt 0 ] && ! cp "${_DIAG_ERR[@]}" "$DIAG_DIR/"; then
|
||||
_PRESERVE_OK=false
|
||||
fi
|
||||
else
|
||||
_PRESERVE_OK=false
|
||||
fi
|
||||
fi
|
||||
|
||||
if [ "$_PRESERVE_OK" = "true" ]; then
|
||||
rm -rf "$RUN_DIR"
|
||||
else
|
||||
echo "WARNING: evidence preservation to $DIAG_DIR failed — leaving the un-preserved run directory intact at: $RUN_DIR" >&2
|
||||
fi
|
||||
```
|
||||
</step>
|
||||
|
||||
|
||||
@@ -20,7 +20,7 @@ import fs from 'node:fs';
|
||||
import path from 'node:path';
|
||||
// eslint-disable-next-line @typescript-eslint/no-require-imports
|
||||
import io = require('./io.cjs');
|
||||
const { output, error } = io;
|
||||
const { output, error, formatDiagnosticToken } = io;
|
||||
import { escapeRegex } from './pattern.cjs';
|
||||
// eslint-disable-next-line @typescript-eslint/no-require-imports
|
||||
import planningWorkspace = require('./planning-workspace.cjs');
|
||||
@@ -67,6 +67,14 @@ interface GapResult {
|
||||
table: string;
|
||||
summary: string;
|
||||
counts: GapCounts;
|
||||
/**
|
||||
* #3885 (ADR-3473 §8.5): null when the phase directory is genuinely absent
|
||||
* (guarded by `fs.existsSync` before the read, so readdirSync is never even
|
||||
* attempted) or was read successfully. A message naming the phase directory
|
||||
* when it EXISTS but `readdirSync` failed (EACCES/EIO/...) — never
|
||||
* collapsed to the same `[]` an absent directory produces.
|
||||
*/
|
||||
phase_dir_read_error: string | null;
|
||||
}
|
||||
|
||||
interface RunGapAnalysisOptions {
|
||||
@@ -338,6 +346,7 @@ function runGapAnalysis(cwd: string, phaseDir: string, options: RunGapAnalysisOp
|
||||
table: '',
|
||||
summary: 'workflow.post_planning_gaps disabled — skipping post-planning gap analysis',
|
||||
counts: { total: 0, covered: 0, uncovered: 0 },
|
||||
phase_dir_read_error: null,
|
||||
};
|
||||
}
|
||||
|
||||
@@ -364,9 +373,16 @@ function runGapAnalysis(cwd: string, phaseDir: string, options: RunGapAnalysisOp
|
||||
// Read the phase directory once; reuse the listing for both context detection
|
||||
// and plan-file enumeration (avoids redundant readdirSync calls).
|
||||
let phaseDirFiles: string[] = [];
|
||||
// #3885 (ADR-3473 §8.5): the existsSync guard above already means a catch
|
||||
// here is NEVER "genuinely absent" (ENOENT) — this directory exists, so any
|
||||
// failure to list it is a real read error (EACCES/EIO/...) and must be
|
||||
// named, not folded into the same `[]` an absent directory produces.
|
||||
let phaseDirReadError: string | null = null;
|
||||
try {
|
||||
if (fs.existsSync(absPhaseDir)) phaseDirFiles = fs.readdirSync(absPhaseDir);
|
||||
} catch { /* unreadable */ }
|
||||
} catch (err) {
|
||||
phaseDirReadError = `Could not read phase directory ${formatDiagnosticToken(absPhaseDir)}: ${formatDiagnosticToken((err as Error)?.message ?? String(err))}`;
|
||||
}
|
||||
|
||||
// #3511-class: scope the raw listing to this phase dir before the
|
||||
// phase-numbered -CONTEXT.md predicate. `phaseDirFiles` itself stays raw —
|
||||
@@ -433,6 +449,7 @@ function runGapAnalysis(cwd: string, phaseDir: string, options: RunGapAnalysisOp
|
||||
table: formatGapTable(rows) + '\n' + coverageSummary + '\n\n' + mismatchMsg,
|
||||
summary: coverageSummary + '; extracted 0 of N — possible format mismatch',
|
||||
counts: { total: rows.length, covered, uncovered },
|
||||
phase_dir_read_error: phaseDirReadError,
|
||||
};
|
||||
}
|
||||
return {
|
||||
@@ -441,6 +458,7 @@ function runGapAnalysis(cwd: string, phaseDir: string, options: RunGapAnalysisOp
|
||||
table: mismatchMsg,
|
||||
summary: 'extracted 0 of N — possible format mismatch',
|
||||
counts: { total: 0, covered: 0, uncovered: 0 },
|
||||
phase_dir_read_error: phaseDirReadError,
|
||||
};
|
||||
}
|
||||
|
||||
@@ -458,6 +476,7 @@ function runGapAnalysis(cwd: string, phaseDir: string, options: RunGapAnalysisOp
|
||||
table: '## Post-Planning Gap Analysis\n\nNo requirements or decisions to check.\n',
|
||||
summary: 'no requirements or decisions to check',
|
||||
counts: { total: 0, covered: 0, uncovered: 0 },
|
||||
phase_dir_read_error: phaseDirReadError,
|
||||
};
|
||||
}
|
||||
|
||||
@@ -478,6 +497,7 @@ function runGapAnalysis(cwd: string, phaseDir: string, options: RunGapAnalysisOp
|
||||
table: formatGapTable(rows) + '\n' + summary + '\n',
|
||||
summary,
|
||||
counts: { total: rows.length, covered, uncovered },
|
||||
phase_dir_read_error: phaseDirReadError,
|
||||
};
|
||||
}
|
||||
|
||||
|
||||
33
src/init.cts
33
src/init.cts
@@ -81,7 +81,7 @@ const {
|
||||
import verifyCommandGrounding = require('./verify-command-grounding.cjs');
|
||||
const { harvestPriorVerifyCommands } = verifyCommandGrounding;
|
||||
|
||||
const { output, error, ERROR_REASON } = io;
|
||||
const { output, error, ERROR_REASON, formatDiagnosticToken } = io;
|
||||
const { loadConfig, loadConfigResolved } = configLoader;
|
||||
const { resolveModelInternal, resolveGranularityInternal, assertValidGranularityOverride } = modelResolver;
|
||||
const { findPhaseInternal, listMilestonePhaseDirs, listAllPhaseDirs } = phaseLocator;
|
||||
@@ -1214,8 +1214,21 @@ function cmdInitPlanPhase(
|
||||
if (patternsFile) {
|
||||
result['patterns_path'] = toPosixPath(path.join(phaseDirFull, patternsFile));
|
||||
}
|
||||
} catch {
|
||||
/* intentionally empty */
|
||||
} catch (err) {
|
||||
// #3885 (ADR-3473 §8.5): this branch means `phaseInfo['directory']` was
|
||||
// set (the phase was already resolved to an on-disk directory) yet
|
||||
// `readdirSync` still failed — ENOENT here would be a genuine race
|
||||
// (the directory vanished between resolution and this read) and stays
|
||||
// a silent degrade like the prior behavior; any other errno
|
||||
// (EACCES/EIO/...) is an unreadable-not-absent directory and must be
|
||||
// named, or every conditional field this block sets (context_path,
|
||||
// research_path, verification_path, uat_path, reviews_path,
|
||||
// patterns_path) silently reads as "none of these exist".
|
||||
const code = (err as NodeJS.ErrnoException)?.code;
|
||||
if (code !== 'ENOENT') {
|
||||
result['context_read_error'] =
|
||||
`Could not read phase directory ${formatDiagnosticToken(phaseDirFull)}: ${formatDiagnosticToken((err as Error)?.message ?? String(err))}`;
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
@@ -2091,8 +2104,18 @@ function cmdInitPhaseOp(cwd: string, phase: string, raw: boolean): void {
|
||||
if (reviewsFile) {
|
||||
result['reviews_path'] = toPosixPath(path.join(phaseDirFull, reviewsFile));
|
||||
}
|
||||
} catch {
|
||||
/* intentionally empty */
|
||||
} catch (err) {
|
||||
// #3885 (ADR-3473 §8.5): see the parallel site in cmdInitPlanPhase —
|
||||
// ENOENT here is a genuine race (directory vanished after resolution)
|
||||
// and stays a silent degrade; any other errno (EACCES/EIO/...) means
|
||||
// the directory exists but could not be read, and must be named rather
|
||||
// than silently reported the same as "none of context_path/
|
||||
// research_path/verification_path/uat_path/reviews_path exist".
|
||||
const code = (err as NodeJS.ErrnoException)?.code;
|
||||
if (code !== 'ENOENT') {
|
||||
result['context_read_error'] =
|
||||
`Could not read phase directory ${formatDiagnosticToken(phaseDirFull)}: ${formatDiagnosticToken((err as Error)?.message ?? String(err))}`;
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
|
||||
176
src/intel.cts
176
src/intel.cts
@@ -21,6 +21,9 @@ import { platformWriteSync, platformReadSync, platformEnsureDir } from './shell-
|
||||
// eslint-disable-next-line @typescript-eslint/no-require-imports
|
||||
import capabilityStateMod = require('./capability-state.cjs');
|
||||
const { isCapabilityActive } = capabilityStateMod;
|
||||
// eslint-disable-next-line @typescript-eslint/no-require-imports
|
||||
import ioMod = require('./io.cjs');
|
||||
const { formatDiagnosticToken } = ioMod;
|
||||
|
||||
// ─── Constants ───────────────────────────────────────────────────────────────
|
||||
|
||||
@@ -34,6 +37,17 @@ const INTEL_FILES: Record<string, string> = {
|
||||
stack: 'stack.json',
|
||||
};
|
||||
|
||||
/**
|
||||
* ADR-3473 §8.5 / #3885: recursion bound for the intel JSON search walk.
|
||||
* Restored from the retired SDK lineage (`sdk/src/query/intel.ts` at `11918dcc3^`),
|
||||
* lost in the ADR-0174 consolidation. Unlike the original, hitting the ceiling is
|
||||
* NOT reported as a silent "no match" — the walk that stops early sets a
|
||||
* `truncated` flag threaded back up to `intelQuery`'s result (Decision 4: a
|
||||
* routine that discards an input says so). The bound is on DEPTH only; breadth
|
||||
* (sibling count at any given depth) is unaffected.
|
||||
*/
|
||||
const MAX_JSON_SEARCH_DEPTH = 48;
|
||||
|
||||
// ─── Internal helpers ────────────────────────────────────────────────────────
|
||||
|
||||
/**
|
||||
@@ -93,14 +107,39 @@ interface IntelData {
|
||||
|
||||
/**
|
||||
* Safely read and parse a JSON intel file.
|
||||
* Returns null if file doesn't exist or can't be parsed.
|
||||
*
|
||||
* Returns null for THREE distinct on-disk states, only one of which is a
|
||||
* "quiet" case (#3885, ADR-3473 §8.5):
|
||||
* - ABSENT (ENOENT, via platformReadSync returning null): silent — not
|
||||
* every project has every intel file, and callers already loop over
|
||||
* the full INTEL_FILES set expecting misses. Never pushed to `errors`.
|
||||
* - UNREADABLE (EACCES/EIO/... — platformReadSync rethrows anything that
|
||||
* isn't ENOENT): surfaced, naming the file, when `errors` is supplied.
|
||||
* - MALFORMED (JSON.parse throws on a file that WAS read successfully):
|
||||
* also surfaced, naming the file — a corrupt intel file used to read
|
||||
* identically to "no matches", which is the same defect one layer down.
|
||||
*
|
||||
* `errors` is an optional accumulator so callers that want to thread the
|
||||
* outcome into their own result shape can pass an array and read it back
|
||||
* after the call; callers that omit it keep the prior fold-to-null shape.
|
||||
*/
|
||||
function safeReadJson(filePath: string): IntelData | null {
|
||||
function safeReadJson(filePath: string, errors?: string[]): IntelData | null {
|
||||
let raw: string | null;
|
||||
try {
|
||||
raw = platformReadSync(filePath);
|
||||
} catch (err) {
|
||||
if (errors) {
|
||||
errors.push(`Could not read ${formatDiagnosticToken(filePath)}: ${formatDiagnosticToken((err as Error)?.message ?? String(err))}`);
|
||||
}
|
||||
return null;
|
||||
}
|
||||
if (raw === null) return null; // ENOENT — genuinely absent, not an error.
|
||||
try {
|
||||
const raw = platformReadSync(filePath);
|
||||
if (raw === null) return null;
|
||||
return JSON.parse(raw) as IntelData;
|
||||
} catch {
|
||||
} catch (err) {
|
||||
if (errors) {
|
||||
errors.push(`Could not parse ${formatDiagnosticToken(filePath)}: ${formatDiagnosticToken((err as Error)?.message ?? String(err))}`);
|
||||
}
|
||||
return null;
|
||||
}
|
||||
}
|
||||
@@ -124,18 +163,34 @@ interface SearchMatch {
|
||||
value: unknown;
|
||||
}
|
||||
|
||||
interface SearchWalkResult {
|
||||
matches: SearchMatch[];
|
||||
truncated: boolean;
|
||||
}
|
||||
|
||||
/**
|
||||
* Mutable walk state shared across one searchJsonEntries invocation's
|
||||
* recursive matchesInValue calls. Set to true the moment ANY branch of the
|
||||
* walk actually hits MAX_JSON_SEARCH_DEPTH and stops recursing further —
|
||||
* never inferred from an empty result (a shallow miss must not set this).
|
||||
*/
|
||||
interface SearchWalkState {
|
||||
truncated: boolean;
|
||||
}
|
||||
|
||||
/**
|
||||
* Search for a term (case-insensitive) in a JSON object's keys and string values.
|
||||
* Returns an array of matching entries.
|
||||
* Returns matching entries plus whether the walk hit MAX_JSON_SEARCH_DEPTH.
|
||||
*/
|
||||
function searchJsonEntries(data: IntelData, term: string): SearchMatch[] {
|
||||
if (!data || typeof data !== 'object') return [];
|
||||
function searchJsonEntries(data: IntelData, term: string): SearchWalkResult {
|
||||
if (!data || typeof data !== 'object') return { matches: [], truncated: false };
|
||||
|
||||
const entries = data.entries || data;
|
||||
if (!entries || typeof entries !== 'object') return [];
|
||||
if (!entries || typeof entries !== 'object') return { matches: [], truncated: false };
|
||||
|
||||
const lowerTerm = term.toLowerCase();
|
||||
const matches: SearchMatch[] = [];
|
||||
const state: SearchWalkState = { truncated: false };
|
||||
|
||||
for (const [key, value] of Object.entries(entries)) {
|
||||
if (key === '_meta') continue;
|
||||
@@ -146,27 +201,42 @@ function searchJsonEntries(data: IntelData, term: string): SearchMatch[] {
|
||||
continue;
|
||||
}
|
||||
|
||||
// Check string value match (recursive for objects)
|
||||
if (matchesInValue(value, lowerTerm)) {
|
||||
// Check string value match (recursive for objects/arrays, bounded by depth)
|
||||
if (matchesInValue(value, lowerTerm, 0, state)) {
|
||||
matches.push({ key, value });
|
||||
}
|
||||
}
|
||||
|
||||
return matches;
|
||||
return { matches, truncated: state.truncated };
|
||||
}
|
||||
|
||||
/**
|
||||
* Recursively check if a term appears in any string value.
|
||||
*
|
||||
* `depth` counts container unwraps already performed (starts at 0 for the
|
||||
* entry's own value). Strings never fail the ceiling check themselves — only
|
||||
* a container (object/array) refuses to recurse one level deeper once
|
||||
* `depth > MAX_JSON_SEARCH_DEPTH`, at which point `state.truncated` is set so
|
||||
* the caller can report "I stopped looking" rather than a bare "no match".
|
||||
* The bound is on nesting depth only, never on sibling breadth.
|
||||
*/
|
||||
function matchesInValue(value: unknown, lowerTerm: string): boolean {
|
||||
function matchesInValue(value: unknown, lowerTerm: string, depth: number, state: SearchWalkState): boolean {
|
||||
if (typeof value === 'string') {
|
||||
return value.toLowerCase().includes(lowerTerm);
|
||||
}
|
||||
if (Array.isArray(value)) {
|
||||
return value.some(v => matchesInValue(v, lowerTerm));
|
||||
if (depth > MAX_JSON_SEARCH_DEPTH) {
|
||||
state.truncated = true;
|
||||
return false;
|
||||
}
|
||||
return value.some(v => matchesInValue(v, lowerTerm, depth + 1, state));
|
||||
}
|
||||
if (value && typeof value === 'object') {
|
||||
return Object.values(value).some(v => matchesInValue(v, lowerTerm));
|
||||
if (depth > MAX_JSON_SEARCH_DEPTH) {
|
||||
state.truncated = true;
|
||||
return false;
|
||||
}
|
||||
return Object.values(value).some(v => matchesInValue(v, lowerTerm, depth + 1, state));
|
||||
}
|
||||
return false;
|
||||
}
|
||||
@@ -177,6 +247,26 @@ interface IntelQueryResult {
|
||||
matches: Array<{ source: string; entries: SearchMatch[] }>;
|
||||
term: string;
|
||||
total: number;
|
||||
/**
|
||||
* True iff ANY searched intel file's walk hit MAX_JSON_SEARCH_DEPTH and
|
||||
* stopped early. Never true merely because nothing was found (a shallow
|
||||
* miss is not a truncation) — always present as a boolean, never undefined.
|
||||
*/
|
||||
truncated: boolean;
|
||||
/**
|
||||
* #3885 (ADR-3473 §8.5): one diagnostic string per intel file that was
|
||||
* present on disk but could not be read (EACCES/EIO/...) or could not be
|
||||
* parsed (malformed JSON) — each naming the file via formatDiagnosticToken.
|
||||
* A file that is simply ABSENT (ENOENT) never contributes an entry here;
|
||||
* that is the normal, expected case (not every project has every intel
|
||||
* file). Always present as an array, never undefined — empty when every
|
||||
* searched file was either absent or read cleanly, mirroring `truncated`'s
|
||||
* always-present convention. Plural (unlike `context_read_error`'s
|
||||
* singular nullable-string shape in roadmap.cts/init.cts) because a single
|
||||
* query fans out across every file in INTEL_FILES and more than one can
|
||||
* independently fail.
|
||||
*/
|
||||
read_errors: string[];
|
||||
}
|
||||
|
||||
/**
|
||||
@@ -188,27 +278,38 @@ function intelQuery(term: string, planningDir: string): IntelQueryResult | Disab
|
||||
|
||||
const matches: Array<{ source: string; entries: SearchMatch[] }> = [];
|
||||
let total = 0;
|
||||
let truncated = false;
|
||||
const readErrors: string[] = [];
|
||||
|
||||
// Search all JSON intel files
|
||||
for (const [_key, filename] of Object.entries(INTEL_FILES)) {
|
||||
const filePath = intelFilePath(planningDir, filename);
|
||||
const data = safeReadJson(filePath);
|
||||
const data = safeReadJson(filePath, readErrors);
|
||||
if (!data) continue;
|
||||
|
||||
const found = searchJsonEntries(data, term);
|
||||
const { matches: found, truncated: fileTruncated } = searchJsonEntries(data, term);
|
||||
if (fileTruncated) truncated = true;
|
||||
if (found.length > 0) {
|
||||
matches.push({ source: filename, entries: found });
|
||||
total += found.length;
|
||||
}
|
||||
}
|
||||
|
||||
return { matches, term, total };
|
||||
return { matches, term, total, truncated, read_errors: readErrors };
|
||||
}
|
||||
|
||||
interface IntelStatusFileEntry {
|
||||
exists: boolean;
|
||||
updated_at: string | null;
|
||||
stale: boolean;
|
||||
/**
|
||||
* #3885 (ADR-3473 §8.5): non-null iff the file exists but could not be
|
||||
* read or parsed — naming the file. `stale` still defaults to true in
|
||||
* that case (an unknown-freshness file is conservatively treated as
|
||||
* stale, unchanged from prior behaviour), but this field says WHY rather
|
||||
* than leaving the reader to assume the file was simply never refreshed.
|
||||
*/
|
||||
read_error: string | null;
|
||||
}
|
||||
|
||||
interface IntelStatusResult {
|
||||
@@ -233,7 +334,7 @@ function intelStatus(planningDir: string): IntelStatusResult | DisabledResponse
|
||||
const exists = fs.existsSync(filePath);
|
||||
|
||||
if (!exists) {
|
||||
files[filename] = { exists: false, updated_at: null, stale: true };
|
||||
files[filename] = { exists: false, updated_at: null, stale: true, read_error: null };
|
||||
overallStale = true;
|
||||
continue;
|
||||
}
|
||||
@@ -241,7 +342,8 @@ function intelStatus(planningDir: string): IntelStatusResult | DisabledResponse
|
||||
let updatedAt: string | null = null;
|
||||
|
||||
// All intel files are JSON — read _meta.updated_at
|
||||
const data = safeReadJson(filePath);
|
||||
const readErrors: string[] = [];
|
||||
const data = safeReadJson(filePath, readErrors);
|
||||
if (data && data._meta && data._meta.updated_at) {
|
||||
updatedAt = data._meta.updated_at;
|
||||
}
|
||||
@@ -253,7 +355,7 @@ function intelStatus(planningDir: string): IntelStatusResult | DisabledResponse
|
||||
}
|
||||
|
||||
if (stale) overallStale = true;
|
||||
files[filename] = { exists: true, updated_at: updatedAt, stale };
|
||||
files[filename] = { exists: true, updated_at: updatedAt, stale, read_error: readErrors[0] ?? null };
|
||||
}
|
||||
|
||||
return { files, overall_stale: overallStale };
|
||||
@@ -268,14 +370,20 @@ interface IntelDiffResult {
|
||||
/**
|
||||
* Show changes since the last full refresh by comparing file hashes.
|
||||
*/
|
||||
function intelDiff(planningDir: string): IntelDiffResult | { no_baseline: true } | DisabledResponse {
|
||||
function intelDiff(planningDir: string): IntelDiffResult | { no_baseline: true; read_error: string | null } | DisabledResponse {
|
||||
if (!isIntelCapabilityActive(planningDir)) return disabledResponse();
|
||||
|
||||
const snapshotPath = intelFilePath(planningDir, '.last-refresh.json');
|
||||
const snapshot = safeReadJson(snapshotPath);
|
||||
const readErrors: string[] = [];
|
||||
const snapshot = safeReadJson(snapshotPath, readErrors);
|
||||
|
||||
if (!snapshot) {
|
||||
return { no_baseline: true };
|
||||
// #3885 (ADR-3473 §8.5): `no_baseline: true` stays true for BOTH a
|
||||
// genuinely-never-snapshotted project (ENOENT, read_error stays null)
|
||||
// AND a present-but-unreadable/malformed snapshot — collapsing those
|
||||
// two into a bare `no_baseline: true` would manufacture "you've never
|
||||
// run a refresh" out of a read failure. `read_error` names which one.
|
||||
return { no_baseline: true, read_error: readErrors[0] ?? null };
|
||||
}
|
||||
|
||||
const prevHashes = (snapshot.hashes as Record<string, string> | undefined) || {};
|
||||
@@ -457,6 +565,12 @@ interface IntelApiSurfaceResult {
|
||||
written: string;
|
||||
symbolCount: number;
|
||||
stale: boolean;
|
||||
/**
|
||||
* #3885 (ADR-3473 §8.5): non-null iff api-map.json exists but could not
|
||||
* be read or parsed — naming the file. Null when api-map.json is simply
|
||||
* absent (the normal "not yet populated" case) or was read cleanly.
|
||||
*/
|
||||
read_error: string | null;
|
||||
}
|
||||
|
||||
/**
|
||||
@@ -472,7 +586,9 @@ function intelApiSurface(planningDir: string): IntelApiSurfaceResult | DisabledR
|
||||
const apiMapPath = path.join(intelPath, INTEL_FILES.apis);
|
||||
const outputPath = path.join(intelPath, 'API-SURFACE.md');
|
||||
|
||||
const data = safeReadJson(apiMapPath);
|
||||
const readErrors: string[] = [];
|
||||
const data = safeReadJson(apiMapPath, readErrors);
|
||||
const readError = readErrors[0] ?? null;
|
||||
const entries = (data && data.entries && typeof data.entries === 'object')
|
||||
? Object.entries(data.entries)
|
||||
: [];
|
||||
@@ -493,7 +609,13 @@ function intelApiSurface(planningDir: string): IntelApiSurfaceResult | DisabledR
|
||||
lines.push('');
|
||||
|
||||
if (symbolCount === 0) {
|
||||
lines.push('> **Incomplete:** api-map.json has no entries (intel extraction is regex/JS-only or not yet populated).');
|
||||
if (readError) {
|
||||
// #3885: a read/parse failure is NOT "not yet populated" — say so,
|
||||
// rather than manufacturing the wrong reason for the empty surface.
|
||||
lines.push(`> **Incomplete:** ${readError}`);
|
||||
} else {
|
||||
lines.push('> **Incomplete:** api-map.json has no entries (intel extraction is regex/JS-only or not yet populated).');
|
||||
}
|
||||
lines.push('> Treat absence here as "unknown", not "does not exist".');
|
||||
lines.push('');
|
||||
} else {
|
||||
@@ -517,7 +639,7 @@ function intelApiSurface(planningDir: string): IntelApiSurfaceResult | DisabledR
|
||||
|
||||
platformWriteSync(outputPath, lines.join('\n'));
|
||||
|
||||
return { written: outputPath, symbolCount, stale };
|
||||
return { written: outputPath, symbolCount, stale, read_error: readError };
|
||||
}
|
||||
|
||||
interface IntelPatchMetaResult {
|
||||
|
||||
@@ -20,7 +20,7 @@ import fs from 'node:fs';
|
||||
import path from 'node:path';
|
||||
// eslint-disable-next-line @typescript-eslint/no-require-imports -- io.cjs is an export= CommonJS module
|
||||
import ioMod = require('./io.cjs');
|
||||
const { output, error, ERROR_REASON } = ioMod;
|
||||
const { output, error, ERROR_REASON, formatDiagnosticToken } = ioMod;
|
||||
// eslint-disable-next-line @typescript-eslint/no-require-imports
|
||||
import stateContract = require('./state-contract.cjs');
|
||||
const { publishStateContract } = stateContract;
|
||||
@@ -634,17 +634,26 @@ function computeDependencyLevels(
|
||||
rawPlans: RawPlan[],
|
||||
planMap: Map<string, RawPlan>,
|
||||
canonicalToId: Map<string, string>,
|
||||
): { level: Map<string, number>; visited: number; order: string[] } {
|
||||
): { level: Map<string, number>; visited: number; order: string[]; unresolved: Array<{ plan: string; token: string }> } {
|
||||
const level = new Map<string, number>();
|
||||
const inDeg = new Map<string, number>();
|
||||
const adj = new Map<string, string[]>();
|
||||
// #3427 / ADR-3473 §8.5: a depends_on token that resolves via neither
|
||||
// planMap nor canonicalToId is a dropped edge. Naming it here (rather than
|
||||
// silently `continue`-ing past it) lets cmdPhasePlanIndex surface the
|
||||
// token's own warning instead of manufacturing a wave-mismatch verdict from
|
||||
// the resulting damaged graph (#3427).
|
||||
const unresolved: Array<{ plan: string; token: string }> = [];
|
||||
|
||||
for (const p of rawPlans) {
|
||||
if (!inDeg.has(p.id)) inDeg.set(p.id, 0);
|
||||
if (!adj.has(p.id)) adj.set(p.id, []);
|
||||
for (const dep of p.dependsOn) {
|
||||
const resolvedDep = resolveDependencyId(dep, planMap, canonicalToId);
|
||||
if (!resolvedDep) continue;
|
||||
if (!resolvedDep) {
|
||||
unresolved.push({ plan: p.id, token: String(dep) });
|
||||
continue;
|
||||
}
|
||||
if (!adj.has(resolvedDep)) adj.set(resolvedDep, []);
|
||||
(adj.get(resolvedDep) as string[]).push(p.id);
|
||||
inDeg.set(p.id, (inDeg.get(p.id) ?? 0) + 1);
|
||||
@@ -679,7 +688,7 @@ function computeDependencyLevels(
|
||||
}
|
||||
}
|
||||
|
||||
return { level, visited, order: queue };
|
||||
return { level, visited, order: queue, unresolved };
|
||||
}
|
||||
|
||||
function cmdPhasePlanIndex(cwd: string, phase: string, raw: boolean): void {
|
||||
@@ -858,7 +867,7 @@ function cmdPhasePlanIndex(cwd: string, phase: string, raw: boolean): void {
|
||||
rawPlans.map((p) => [extractCanonicalPlanId(p.id).toLowerCase(), p.id]),
|
||||
);
|
||||
|
||||
const { level, visited, order } = computeDependencyLevels(rawPlans, planMap, canonicalToId);
|
||||
const { level, visited, order, unresolved } = computeDependencyLevels(rawPlans, planMap, canonicalToId);
|
||||
|
||||
if (visited < rawPlans.length) {
|
||||
const cycleNodes = rawPlans.filter((p) => !level.has(p.id)).map((p) => p.id);
|
||||
@@ -894,6 +903,20 @@ function cmdPhasePlanIndex(cwd: string, phase: string, raw: boolean): void {
|
||||
let hasCheckpoints = false;
|
||||
const warnings: string[] = [];
|
||||
|
||||
// #3427 / ADR-3473 §8.5: name every dropped depends_on edge (plan AND
|
||||
// token) rather than letting it silently collapse the plan to a DAG root.
|
||||
// A plan with at least one unresolved token gets ITS OWN warning here and
|
||||
// the wave-mismatch verdict below is suppressed for that plan ONLY — a
|
||||
// plan with no dropped edges and a genuinely wrong `wave:` still warns
|
||||
// (N3, D6, T25).
|
||||
const plansWithUnresolvedTokens = new Set<string>();
|
||||
for (const { plan, token } of unresolved) {
|
||||
plansWithUnresolvedTokens.add(plan);
|
||||
warnings.push(
|
||||
`Plan ${plan}: depends_on token ${formatDiagnosticToken(token)} does not resolve to any plan in this phase — edge dropped, wave placement for this plan may be unreliable`,
|
||||
);
|
||||
}
|
||||
|
||||
for (const rawPlan of rawPlans) {
|
||||
if (!rawPlan.autonomous) {
|
||||
hasCheckpoints = true;
|
||||
@@ -911,7 +934,17 @@ function cmdPhasePlanIndex(cwd: string, phase: string, raw: boolean): void {
|
||||
|
||||
const computedWave = (level.get(rawPlan.id) ?? 0) + levelOffset;
|
||||
const effectiveWave = computedWave;
|
||||
if (rawPlan.declaredWave !== null && rawPlan.declaredWave !== computedWave) {
|
||||
// #3427 (D5/N3): suppress the wave-mismatch verdict for a plan that has
|
||||
// at least one unresolved depends_on token — its own dropped-edge
|
||||
// warning above already explains the degraded wave placement, so the
|
||||
// mismatch here would blame the author for a DAG the tool itself
|
||||
// couldn't build. A plan with NO unresolved tokens still gets a genuine
|
||||
// mismatch reported (N3, T25) — the suppression is per-plan, never blanket.
|
||||
if (
|
||||
rawPlan.declaredWave !== null &&
|
||||
rawPlan.declaredWave !== computedWave &&
|
||||
!plansWithUnresolvedTokens.has(rawPlan.id)
|
||||
) {
|
||||
warnings.push(
|
||||
`Plan ${rawPlan.id}: declared wave: ${rawPlan.declaredWave} but depends_on DAG places it in wave ${computedWave}`,
|
||||
);
|
||||
|
||||
@@ -13,7 +13,7 @@ import { escapeRegex } from './pattern.cjs';
|
||||
import { splitLines, detectEol, joinLines } from './text-lines.cjs';
|
||||
// eslint-disable-next-line @typescript-eslint/no-require-imports
|
||||
import ioMod = require('./io.cjs');
|
||||
const { output, error } = ioMod;
|
||||
const { output, error, formatDiagnosticToken } = ioMod;
|
||||
// eslint-disable-next-line @typescript-eslint/no-require-imports
|
||||
import phaseIdMod = require('./phase-id.cjs');
|
||||
const { normalizePhaseName, phaseMarkdownRegexSource, matchPhaseDirs, stripProjectCodePrefix, OPTIONAL_PHASE_TAG_SOURCE, roadmapPhaseLookupSources, isSentinelPhaseId, scopeToPhase } = phaseIdMod;
|
||||
@@ -58,6 +58,15 @@ interface PhasePlansAndSummaries {
|
||||
summaryCount: number;
|
||||
hasContext: boolean;
|
||||
hasResearch: boolean;
|
||||
/**
|
||||
* #3885 (ADR-3473 §8.5): null when the phase directory's readdirSync
|
||||
* succeeded OR was genuinely absent (ENOENT — a real "not built yet"
|
||||
* answer, not an error). A message naming the phase directory when
|
||||
* readdirSync failed for any other reason (EACCES/EIO/...), so an
|
||||
* unreadable directory is never silently reported the same as one that was
|
||||
* successfully read and genuinely has no CONTEXT.md.
|
||||
*/
|
||||
contextReadError: string | null;
|
||||
}
|
||||
|
||||
interface PhaseSearchResult {
|
||||
@@ -118,7 +127,20 @@ function countPhasePlansAndSummaries(phaseDir: string): PhasePlansAndSummaries {
|
||||
// hasContext and hasResearch are not plan-scan concerns — read the directory
|
||||
// once and share the listing for all non-plan metadata that cmdRoadmapAnalyze needs.
|
||||
let phaseFiles: string[] = [];
|
||||
try { phaseFiles = fs.readdirSync(phaseDir); } catch { /* empty */ }
|
||||
// #3885 (ADR-3473 §8.5): distinguish "genuinely absent" (ENOENT) from
|
||||
// "could not read" (EACCES/EIO/...) — the collapse of both to an empty
|
||||
// listing is exactly the defect class this item closes. Mirrors
|
||||
// core-utils.cts's getPhaseFileStats / phase-locator.cts's
|
||||
// listMilestonePhaseDirs SCOPE.UNREADABLE discriminator.
|
||||
let contextReadError: string | null = null;
|
||||
try {
|
||||
phaseFiles = fs.readdirSync(phaseDir);
|
||||
} catch (err) {
|
||||
const code = (err as NodeJS.ErrnoException)?.code;
|
||||
if (code !== 'ENOENT') {
|
||||
contextReadError = `Could not read phase directory ${formatDiagnosticToken(phaseDir)}: ${formatDiagnosticToken((err as Error)?.message ?? String(err))}`;
|
||||
}
|
||||
}
|
||||
// #3511: scope the raw listing to this phase dir before the
|
||||
// phase-numbered-artifact predicates (hasContext/hasResearch) — planCount/
|
||||
// summaryCount above stay on scanPhasePlans's own unscoped listing since a
|
||||
@@ -130,6 +152,7 @@ function countPhasePlansAndSummaries(phaseDir: string): PhasePlansAndSummaries {
|
||||
summaryCount,
|
||||
hasContext: findContextMdIn(scopedFiles) !== null,
|
||||
hasResearch: scopedFiles.some(f => f.endsWith('-RESEARCH.md') || f === 'RESEARCH.md'),
|
||||
contextReadError,
|
||||
};
|
||||
}
|
||||
|
||||
@@ -364,6 +387,8 @@ type AnalyzePhase = {
|
||||
has_research: boolean;
|
||||
disk_status: string;
|
||||
roadmap_complete: boolean;
|
||||
/** #3885 (ADR-3473 §8.5): see PhasePlansAndSummaries.contextReadError. */
|
||||
context_read_error: string | null;
|
||||
};
|
||||
|
||||
/**
|
||||
@@ -417,6 +442,10 @@ function collectAnalyzePhases(content: string, phasesDir: string, phaseDirNames:
|
||||
let summaryCount = 0;
|
||||
let hasContext = false;
|
||||
let hasResearch = false;
|
||||
// #3885 (ADR-3473 §8.5): null unless dirMatch resolves and its readdirSync
|
||||
// hit a non-ENOENT error — no directory at all is `disk_status:
|
||||
// 'no_directory'`, a real (if uninteresting) answer, not a read error.
|
||||
let contextReadError: string | null = null;
|
||||
|
||||
// DEAD catch removed (#2245 audit): matchPhaseDirs(...) is a pure
|
||||
// array lookup on an already-resolved string array, and
|
||||
@@ -432,6 +461,7 @@ function collectAnalyzePhases(content: string, phasesDir: string, phaseDirNames:
|
||||
summaryCount = counts.summaryCount;
|
||||
hasContext = counts.hasContext;
|
||||
hasResearch = counts.hasResearch;
|
||||
contextReadError = counts.contextReadError;
|
||||
|
||||
// ADR-3180 §7.4 (issue #3186, disk-strict, #3168 fix): route "is this
|
||||
// phase complete" through the canonical owner (`isPhaseComplete`),
|
||||
@@ -478,6 +508,7 @@ function collectAnalyzePhases(content: string, phasesDir: string, phaseDirNames:
|
||||
has_research: hasResearch,
|
||||
disk_status: diskStatus,
|
||||
roadmap_complete: roadmapComplete,
|
||||
context_read_error: contextReadError,
|
||||
});
|
||||
}
|
||||
|
||||
@@ -494,12 +525,18 @@ function collectAnalyzePhases(content: string, phasesDir: string, phaseDirNames:
|
||||
let tSummaryCount = 0;
|
||||
let tHasContext = false;
|
||||
let tHasResearch = false;
|
||||
let tContextReadError: string | null = null;
|
||||
if (dirMatchA) {
|
||||
const counts = countPhasePlansAndSummaries(path.join(phasesDir, dirMatchA));
|
||||
tPlanCount = counts.planCount;
|
||||
tSummaryCount = counts.summaryCount;
|
||||
tHasContext = fs.existsSync(path.join(phasesDir, dirMatchA, 'CONTEXT.md'));
|
||||
tHasResearch = fs.existsSync(path.join(phasesDir, dirMatchA, 'RESEARCH.md'));
|
||||
// #3885 (ADR-3473 §8.5): reuse the SAME countPhasePlansAndSummaries call's
|
||||
// discriminator — this row's hasContext/hasResearch are read via a direct
|
||||
// existsSync (which cannot itself distinguish EACCES from absent), but
|
||||
// an unreadable phase directory is still surfaced via the sibling call.
|
||||
tContextReadError = counts.contextReadError;
|
||||
}
|
||||
phases.push({
|
||||
number: tr.id,
|
||||
@@ -513,6 +550,7 @@ function collectAnalyzePhases(content: string, phasesDir: string, phaseDirNames:
|
||||
has_research: tHasResearch,
|
||||
disk_status: dirMatchA ? 'ok' : 'no_directory',
|
||||
roadmap_complete: false,
|
||||
context_read_error: tContextReadError,
|
||||
});
|
||||
}
|
||||
return phases;
|
||||
|
||||
@@ -2,7 +2,7 @@
|
||||
"version": 1,
|
||||
"paths": {
|
||||
"review.md": {
|
||||
"reason": "#3034 adds opt-in concurrent reviewer-lane dispatch to the invoke_reviewers step. The growth is the feature plus the reasoning that has to travel with it, because this block is shipped shell that is read by an agent at runtime rather than by a compiler: the strict-equality guard and its fail-safe polarity (deliberately inverted relative to the commit_docs guard, so it does not read as an inconsistency to be tidied away), why per-lane result files replaced a shared append (concurrent O_APPEND is atomic only below PIPE_BUF, and write_reviews parses that JSONL to render the models:/model_sources: frontmatter), why aggregation walks selection order rather than completion order, why the slug list is de-duplicated before dispatch, and why the loop body was hoisted into one shared function instead of forking into two dispatch bodies. No prose was moved into an eagerly @-imported reference to shrink the measured file -- total loaded context grew by exactly this diff."
|
||||
"reason": "#3034 adds opt-in concurrent reviewer-lane dispatch to the invoke_reviewers step. The growth is the feature plus the reasoning that has to travel with it, because this block is shipped shell that is read by an agent at runtime rather than by a compiler: the strict-equality guard and its fail-safe polarity (deliberately inverted relative to the commit_docs guard, so it does not read as an inconsistency to be tidied away), why per-lane result files replaced a shared append (concurrent O_APPEND is atomic only below PIPE_BUF, and write_reviews parses that JSONL to render the models:/model_sources: frontmatter), why aggregation walks selection order rather than completion order, why the slug list is de-duplicated before dispatch, and why the loop body was hoisted into one shared function instead of forking into two dispatch bodies. No prose was moved into an eagerly @-imported reference to shrink the measured file -- total loaded context grew by exactly this diff. #3885 (ADR-3473 §8.5) adds a further, additive growth on top of the above: a gate in write_reviews that refuses to render REVIEWS.md when every dispatched lane left no result (distinguishing an all-budget-skipped run, which is not a failure, from a genuine total lane failure, since both leave the aggregate JSONL empty), a conditional guard on the commit step so a run that hit that gate never commits an artifact it did not produce, an updated present_results branch that reports the failure/skip case instead of the success banner, and a per-lane evidence-preservation step in present_results that copies the run's `gsd-review-*.md` and non-empty `.err` files into `{phase_dir}/.review-diagnostics/` before `rm -rf` destroys the run directory -- the only record a failed run left. None of this is committed alongside REVIEWS.md (the commit step still names only that one file, never a directory glob), so the preserved diagnostics cannot leak into the REVIEWS.md commit. No prose was moved into an eagerly @-imported reference to shrink the measured file."
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
@@ -815,3 +815,91 @@ describe('#3189 — normalizePhaseReqIds drops non-ID prose after range expansio
|
||||
});
|
||||
});
|
||||
|
||||
// ─────────────────────────────────────────────────────────────────────────────
|
||||
// #3885 (ADR-3473 §8.5) / item 5 — runGapAnalysis's phase-directory readdirSync
|
||||
// swallows an unreadable directory into the same `[]` a genuinely absent one
|
||||
// produces (src/gap-checker.cts, runGapAnalysis):
|
||||
// try { if (fs.existsSync(absPhaseDir)) phaseDirFiles = fs.readdirSync(absPhaseDir); }
|
||||
// catch { /* unreadable */ }
|
||||
// The existsSync guard means a caught error here is NEVER ENOENT-shaped
|
||||
// absence — the directory exists, so any readdirSync failure (EACCES, EIO,
|
||||
// ...) must be named, not folded into the same result a phase with a
|
||||
// genuinely context-less/plan-less directory produces.
|
||||
//
|
||||
// `runGapAnalysis` gains `phase_dir_read_error: string | null` — null on a
|
||||
// successful read (this is the ONLY branch reachable, since an ABSENT
|
||||
// directory never reaches readdirSync at all, per the existsSync guard), and
|
||||
// a message naming the phase directory on any other readdirSync failure.
|
||||
// `runGapAnalysis` is exported directly (see decisions.test.cjs's identical
|
||||
// direct-call style for the same function), so these drive it in-process.
|
||||
// Injected via monkeypatching `fs.readdirSync` (t.mock.method, auto-restored
|
||||
// per test) — NEVER chmod 0o000, which root bypasses with zero coverage.
|
||||
describe('#3885 (ADR-3473 §8.5): runGapAnalysis distinguishes unreadable from absent (gap-checker.cts caller)', () => {
|
||||
const fs = require('fs');
|
||||
const path = require('path');
|
||||
const { createTempProject, cleanup } = require('./helpers.cjs');
|
||||
const { runGapAnalysis } = require('../gsd-core/bin/lib/gap-checker.cjs');
|
||||
|
||||
let tmpDir;
|
||||
let phaseDir;
|
||||
|
||||
function setup() {
|
||||
tmpDir = createTempProject();
|
||||
phaseDir = path.join(tmpDir, '.planning', 'phases', '03-api');
|
||||
fs.mkdirSync(phaseDir, { recursive: true });
|
||||
fs.writeFileSync(path.join(phaseDir, '03-01-PLAN.md'), '# Plan\nSome plan text.\n');
|
||||
}
|
||||
|
||||
function injectReaddirFailure(t, targetPath, code) {
|
||||
const resolved = path.resolve(targetPath);
|
||||
const origReaddirSync = fs.readdirSync.bind(fs);
|
||||
t.mock.method(fs, 'readdirSync', (p, ...rest) => {
|
||||
if (path.resolve(String(p)) === resolved) {
|
||||
const err = new Error(`${code}: simulated failure, scandir '${p}'`);
|
||||
err.code = code;
|
||||
throw err;
|
||||
}
|
||||
return origReaddirSync(p, ...rest);
|
||||
});
|
||||
}
|
||||
|
||||
test('readablePhaseDirWithoutContextReportsNoReadError (MUST STAY GREEN)', () => {
|
||||
setup();
|
||||
try {
|
||||
const result = runGapAnalysis(tmpDir, phaseDir);
|
||||
assert.strictEqual(result.phase_dir_read_error ?? null, null,
|
||||
'a readable phase directory must report no read error');
|
||||
} finally {
|
||||
cleanup(tmpDir);
|
||||
}
|
||||
});
|
||||
|
||||
test('unreadablePhaseDirIsNotReportedAsAbsent', (t) => {
|
||||
setup();
|
||||
try {
|
||||
injectReaddirFailure(t, phaseDir, 'EACCES');
|
||||
const result = runGapAnalysis(tmpDir, phaseDir);
|
||||
assert.strictEqual(typeof result.phase_dir_read_error, 'string',
|
||||
`an unreadable phase directory must be reported as an error, not silently absent; got: ${JSON.stringify(result.phase_dir_read_error)}`);
|
||||
assert.ok(result.phase_dir_read_error.includes('03-api'),
|
||||
`the reported error must name the discarded input (the phase directory); got: ${result.phase_dir_read_error}`);
|
||||
} finally {
|
||||
cleanup(tmpDir);
|
||||
}
|
||||
});
|
||||
|
||||
test('missingPhaseDirStaysAbsentNotAnError (MUST STAY GREEN)', () => {
|
||||
tmpDir = createTempProject();
|
||||
const missingPhaseDir = path.join(tmpDir, '.planning', 'phases', '09-nonexistent');
|
||||
try {
|
||||
// Genuinely absent — never created — so `fs.existsSync` short-circuits
|
||||
// before readdirSync is ever attempted; no patch needed.
|
||||
const result = runGapAnalysis(tmpDir, missingPhaseDir);
|
||||
assert.strictEqual(result.phase_dir_read_error ?? null, null,
|
||||
`a genuinely absent phase directory must not be reported as a read error; got: ${result.phase_dir_read_error}`);
|
||||
} finally {
|
||||
cleanup(tmpDir);
|
||||
}
|
||||
});
|
||||
});
|
||||
|
||||
|
||||
@@ -3246,6 +3246,130 @@ describe('#3057 B3: cmdInitVerifyWork — verification staleness-check indetermi
|
||||
});
|
||||
});
|
||||
|
||||
// ─────────────────────────────────────────────────────────────────────────────
|
||||
// #3885 (ADR-3473 §8.5) / item 5 — cmdInitPlanPhase / cmdInitPhaseOp swallow an
|
||||
// unreadable phase directory into "none of the conditional fields resolved",
|
||||
// indistinguishable from a phase directory that genuinely has no
|
||||
// CONTEXT.md/RESEARCH.md/VERIFICATION.md/UAT.md/REVIEWS.md/PATTERNS.md.
|
||||
//
|
||||
// Mechanism (src/init.cts, both cmdInitPlanPhase and cmdInitPhaseOp):
|
||||
// try { const files = fs.readdirSync(phaseDirFull); ... }
|
||||
// catch { /* intentionally empty */ }
|
||||
// guarded by `if (phaseInfo?.['directory'])` — the directory was already
|
||||
// resolved to exist on disk, so a caught error here is never a genuine
|
||||
// "phase has no directory yet" absence.
|
||||
//
|
||||
// Both commands gain `context_read_error` on their result: absent (key
|
||||
// omitted, matching prior shape) when readdirSync succeeds or fails with
|
||||
// ENOENT (a genuine race — the directory vanished after resolution, and
|
||||
// stays a silent degrade like the prior behavior); a message naming the
|
||||
// phase directory on any other errno (EACCES/EIO/...).
|
||||
//
|
||||
// Neither command returns its result object (`output(result, raw)` writes
|
||||
// via `fs.writeSync(1, ...)` — see the cmdInitVerifyWork capture helper
|
||||
// above for why `process.stdout.write` cannot see it), and both are
|
||||
// exercised through the real CLI dispatcher elsewhere in this file via
|
||||
// `runGsdTools`, a real subprocess a parent-process fs monkeypatch cannot
|
||||
// reach — so these drive the exported functions directly, in-process,
|
||||
// mirroring the cmdInitVerifyWork capture pattern immediately above.
|
||||
// Injected via `t.mock.method(fs, 'readdirSync', ...)` (auto-restored) —
|
||||
// NEVER chmod 0o000, which root bypasses with zero coverage.
|
||||
describe('#3885 (ADR-3473 §8.5): init callers distinguish unreadable from absent phase directories', () => {
|
||||
const initMod = require(path.join(__dirname, '..', 'gsd-core', 'bin', 'lib', 'init.cjs'));
|
||||
let projectDir;
|
||||
|
||||
beforeEach(() => {
|
||||
projectDir = createFixture();
|
||||
seedPhase(projectDir, '03-api', {
|
||||
'03-01-PLAN.md': '# Plan',
|
||||
});
|
||||
writePlanningDocs(projectDir);
|
||||
});
|
||||
|
||||
afterEach(() => {
|
||||
cleanup(projectDir);
|
||||
});
|
||||
|
||||
function captureFd1(t, run) {
|
||||
const chunks = [];
|
||||
const origWriteSync = fs.writeSync.bind(fs);
|
||||
t.mock.method(fs, 'writeSync', (fd, data, offset, length) => {
|
||||
if (fd === 2) return Buffer.isBuffer(data) ? data.length : String(data).length;
|
||||
if (fd !== 1) return origWriteSync(fd, data, offset, length);
|
||||
const chunk = Buffer.isBuffer(data)
|
||||
? data.subarray(offset ?? 0, length === undefined ? data.length : (offset ?? 0) + length).toString('utf8')
|
||||
: String(data);
|
||||
chunks.push(chunk);
|
||||
return Buffer.byteLength(chunk, 'utf8');
|
||||
});
|
||||
run();
|
||||
const captured = chunks.join('');
|
||||
assert.ok(captured.length > 0, 'command produced no stdout output');
|
||||
return JSON.parse(captured);
|
||||
}
|
||||
|
||||
function injectReaddirFailure(t, targetPath, code) {
|
||||
const resolved = path.resolve(targetPath);
|
||||
const origReaddirSync = fs.readdirSync.bind(fs);
|
||||
t.mock.method(fs, 'readdirSync', (p, ...rest) => {
|
||||
if (path.resolve(String(p)) === resolved) {
|
||||
const err = new Error(`${code}: simulated failure, scandir '${p}'`);
|
||||
err.code = code;
|
||||
throw err;
|
||||
}
|
||||
return origReaddirSync(p, ...rest);
|
||||
});
|
||||
}
|
||||
|
||||
const phaseDirAbs = () => path.join(projectDir, '.planning', 'phases', '03-api');
|
||||
|
||||
describe('cmdInitPlanPhase', () => {
|
||||
test('readablePhaseDirReportsNoReadError (MUST STAY GREEN)', (t) => {
|
||||
const output = captureFd1(t, () => initMod.cmdInitPlanPhase(projectDir, '03', false));
|
||||
assert.strictEqual(output.context_read_error ?? null, null);
|
||||
});
|
||||
|
||||
test('unreadablePhaseDirIsNotReportedAsAbsent', (t) => {
|
||||
injectReaddirFailure(t, phaseDirAbs(), 'EACCES');
|
||||
const output = captureFd1(t, () => initMod.cmdInitPlanPhase(projectDir, '03', false));
|
||||
assert.strictEqual(typeof output.context_read_error, 'string',
|
||||
`an unreadable phase directory must be reported, not silently absent; got: ${JSON.stringify(output.context_read_error)}`);
|
||||
assert.ok(output.context_read_error.includes('03-api'),
|
||||
`the reported error must name the discarded input (the phase directory); got: ${output.context_read_error}`);
|
||||
});
|
||||
|
||||
test('raceConditionEnoentStaysAGenuineSilentDegrade (MUST STAY GREEN)', (t) => {
|
||||
injectReaddirFailure(t, phaseDirAbs(), 'ENOENT');
|
||||
const output = captureFd1(t, () => initMod.cmdInitPlanPhase(projectDir, '03', false));
|
||||
assert.strictEqual(output.context_read_error ?? null, null,
|
||||
`ENOENT must stay a silent degrade (genuine race), not reported as an error; got: ${output.context_read_error}`);
|
||||
});
|
||||
});
|
||||
|
||||
describe('cmdInitPhaseOp', () => {
|
||||
test('readablePhaseDirReportsNoReadError (MUST STAY GREEN)', (t) => {
|
||||
const output = captureFd1(t, () => initMod.cmdInitPhaseOp(projectDir, '03', false));
|
||||
assert.strictEqual(output.context_read_error ?? null, null);
|
||||
});
|
||||
|
||||
test('unreadablePhaseDirIsNotReportedAsAbsent', (t) => {
|
||||
injectReaddirFailure(t, phaseDirAbs(), 'EACCES');
|
||||
const output = captureFd1(t, () => initMod.cmdInitPhaseOp(projectDir, '03', false));
|
||||
assert.strictEqual(typeof output.context_read_error, 'string',
|
||||
`an unreadable phase directory must be reported, not silently absent; got: ${JSON.stringify(output.context_read_error)}`);
|
||||
assert.ok(output.context_read_error.includes('03-api'),
|
||||
`the reported error must name the discarded input (the phase directory); got: ${output.context_read_error}`);
|
||||
});
|
||||
|
||||
test('raceConditionEnoentStaysAGenuineSilentDegrade (MUST STAY GREEN)', (t) => {
|
||||
injectReaddirFailure(t, phaseDirAbs(), 'ENOENT');
|
||||
const output = captureFd1(t, () => initMod.cmdInitPhaseOp(projectDir, '03', false));
|
||||
assert.strictEqual(output.context_read_error ?? null, null,
|
||||
`ENOENT must stay a silent degrade (genuine race), not reported as an error; got: ${output.context_read_error}`);
|
||||
});
|
||||
});
|
||||
});
|
||||
|
||||
// ─────────────────────────────────────────────────────────────────────────────
|
||||
// roadmap analyze command
|
||||
// ─────────────────────────────────────────────────────────────────────────────
|
||||
|
||||
@@ -425,6 +425,252 @@ describe('intelQuery', () => {
|
||||
});
|
||||
});
|
||||
|
||||
// ─── #3885 (ADR-3473 §8.5): recursion bound + truncation signal ─────────────
|
||||
//
|
||||
// searchJsonEntries/matchesInValue lost the MAX_JSON_SEARCH_DEPTH=48 bound in
|
||||
// the ADR-0174 SDK retirement. A deeply nested .planning/intel/*.json document
|
||||
// overflows the call stack (uncaught RangeError, exit 1) instead of failing
|
||||
// gracefully. Restoring the bound verbatim would trade the crash for a SILENT
|
||||
// "no match" at depth 49+ — exactly the defect class this epic exists to close
|
||||
// one layer down (ADR-3473 Decision 4: a routine that discards an input says
|
||||
// so, naming the input). The required fix therefore pairs the bound with a
|
||||
// truncation signal.
|
||||
//
|
||||
// DESIGN DECISION (chosen by this test file — not yet implemented): a
|
||||
// top-level `truncated: boolean` field on the IntelQueryResult returned by
|
||||
// intelQuery. `truncated` is true iff ANY searched intel file's walk hit the
|
||||
// depth ceiling; false/absent otherwise. Chosen because it is the shape a
|
||||
// JSON consumer (or the CLI's raw stdout) can read directly without parsing
|
||||
// per-entry structure.
|
||||
describe('#3885 (ADR-3473 §8.5): intel query recursion bound + truncation signal', () => {
|
||||
let tmpDir;
|
||||
let planningDir;
|
||||
let surfacedConfigDir;
|
||||
let savedEnv;
|
||||
|
||||
beforeEach(() => {
|
||||
tmpDir = createTempProject();
|
||||
planningDir = path.join(tmpDir, '.planning');
|
||||
enableIntel(planningDir);
|
||||
surfacedConfigDir = makeSurfacedConfigDir();
|
||||
savedEnv = saveSurfacedEnv();
|
||||
delete process.env.GSD_RUNTIME;
|
||||
process.env.CLAUDE_CONFIG_DIR = surfacedConfigDir;
|
||||
delete process.env.GSD_WORKSTREAM;
|
||||
delete process.env.GSD_PROJECT;
|
||||
});
|
||||
|
||||
afterEach(() => {
|
||||
savedEnv.restore();
|
||||
cleanup(surfacedConfigDir);
|
||||
cleanup(tmpDir);
|
||||
});
|
||||
|
||||
// Build an object nested `n` levels deep: {a: {a: {a: ... {leaf: 'needle-here'}}}}.
|
||||
function nest(n) {
|
||||
let o = { leaf: 'needle-here' };
|
||||
for (let i = 0; i < n; i++) o = { a: o };
|
||||
return o;
|
||||
}
|
||||
|
||||
function writeNestedFixture(depth) {
|
||||
writeIntelJson(planningDir, 'file-roles.json', { entries: { top: nest(depth) } });
|
||||
}
|
||||
|
||||
// Build the SAME shape as nest(depth) — {a:{a:{a:...{leaf:'needle-here'}}}}
|
||||
// wrapped in {entries:{top:...}} — but as JSON TEXT via string .repeat(),
|
||||
// with zero recursion in the test process. Required for pathological depths
|
||||
// (thousands+): nest() + JSON.stringify() both recurse per level, so at
|
||||
// depth 12000 THE TEST'S OWN FIXTURE BUILDER overflows the test process's
|
||||
// stack before the CLI is ever spawned (V8's JSON.stringify recurses; its
|
||||
// JSON.parse does not). Do not "simplify" this back to
|
||||
// JSON.stringify(nest(n)) for large depths — that reintroduces a
|
||||
// macOS-green / Linux-red test, since Linux's default container stack is
|
||||
// smaller than macOS's.
|
||||
function writeNestedFixtureText(depth) {
|
||||
const intelPath = path.join(planningDir, 'intel');
|
||||
fs.mkdirSync(intelPath, { recursive: true });
|
||||
const json =
|
||||
'{"entries":{"top":' + '{"a":'.repeat(depth) + '{"leaf":"needle-here"}' + '}'.repeat(depth) + '}}';
|
||||
fs.writeFileSync(path.join(intelPath, 'file-roles.json'), json, 'utf8');
|
||||
}
|
||||
|
||||
// T2 — MUST STAY GREEN before and after the fix: the ceiling is inclusive.
|
||||
test('T2: matchAtDepth48IsFoundUntruncated', () => {
|
||||
writeNestedFixture(48);
|
||||
const result = intelQuery('needle-here', planningDir);
|
||||
assert.strictEqual(result.total, 1, 'depth-48 match must still be found');
|
||||
assert.notStrictEqual(result.truncated, true, 'depth-48 (at the ceiling) must not report truncation');
|
||||
});
|
||||
|
||||
// T3 — RED today: measured on this tree (e20744eac), depth 49 currently
|
||||
// returns total=1 (found), exit 0, with no `truncated` field at all.
|
||||
// Required: NOT returned as a match, AND the result reports truncation —
|
||||
// never a bare "no match".
|
||||
test('T3: matchAtDepth49IsNotSilentlyAbsent', () => {
|
||||
writeNestedFixture(49);
|
||||
const result = intelQuery('needle-here', planningDir);
|
||||
assert.strictEqual(result.total, 0, 'a match past the depth ceiling must not be reported as found');
|
||||
assert.deepStrictEqual(result.matches, [], 'no per-file match entries past the ceiling');
|
||||
assert.strictEqual(result.truncated, true, 'the result must say the search was truncated — never a bare "no match"');
|
||||
});
|
||||
|
||||
// T4 — RED today: measured via the real CLI on this tree — depth 12000
|
||||
// exits 1 with stderr "Error: Maximum call stack size exceeded". Required:
|
||||
// exit 0, bounded result, truncated:true, and the RangeError text must
|
||||
// never appear.
|
||||
test('T4: deeplyNestedIntelDoesNotOverflowTheStack', () => {
|
||||
writeNestedFixtureText(12000);
|
||||
const result = runGsdTools(['intel', 'query', 'needle-here'], tmpDir);
|
||||
assert.strictEqual(result.success, true, `must exit 0 (no stack overflow), got: ${result.error}`);
|
||||
assert.ok(
|
||||
!/Maximum call stack size exceeded/.test(result.error || ''),
|
||||
`stderr must never contain the raw RangeError text, got: ${result.error}`,
|
||||
);
|
||||
const output = JSON.parse(result.output);
|
||||
assert.strictEqual(output.truncated, true, 'a 12000-deep document must report truncation');
|
||||
});
|
||||
|
||||
// T5 — MUST STAY GREEN (N1): a shallow miss is not truncated. Stops the
|
||||
// flag becoming noise on every ordinary "not found" result.
|
||||
test('T5: shallowMissReportsNoTruncation', () => {
|
||||
writeIntelJson(planningDir, 'file-roles.json', {
|
||||
entries: { 'src/foo.ts': { type: 'typescript' } },
|
||||
});
|
||||
const result = intelQuery('needle-here', planningDir);
|
||||
assert.strictEqual(result.total, 0);
|
||||
assert.notStrictEqual(result.truncated, true, 'a shallow miss must not report truncation');
|
||||
});
|
||||
|
||||
// T6 — MUST STAY GREEN (N2): the bound is on DEPTH, not breadth. 10,000
|
||||
// shallow siblings must not be mistaken for hitting the depth ceiling.
|
||||
test('T6: wideShallowDocumentIsUnaffectedByTheDepthBound', () => {
|
||||
const entries = {};
|
||||
for (let i = 0; i < 10000; i++) {
|
||||
entries[`sibling-${i}`] = { note: `filler ${i}` };
|
||||
}
|
||||
entries.target = { nested: { leaf: 'needle-here' } };
|
||||
writeIntelJson(planningDir, 'file-roles.json', { entries });
|
||||
|
||||
const result = intelQuery('needle-here', planningDir);
|
||||
assert.strictEqual(result.total, 1, '10,000-sibling breadth must not suppress a shallow match');
|
||||
assert.notStrictEqual(result.truncated, true, 'breadth alone must never trigger the depth-truncation flag');
|
||||
});
|
||||
});
|
||||
|
||||
// ─── #3885 (ADR-3473 §8.5): safeReadJson no-silent-swallow ──────────────────
|
||||
//
|
||||
// `safeReadJson` folded THREE distinct on-disk states into one bare `null`:
|
||||
// absent (ENOENT), unreadable (EACCES/EIO/...), and malformed (JSON.parse
|
||||
// throws). An unreadable or malformed intel file read as "no matches" —
|
||||
// byte-indistinguishable from a project that simply lacks that intel file.
|
||||
// This closes the gap: absent stays silent (the common, expected case —
|
||||
// intelQuery already loops over every INTEL_FILES entry expecting misses),
|
||||
// while unreadable/malformed are surfaced via the always-present
|
||||
// `read_errors: string[]` field on IntelQueryResult, naming the file.
|
||||
describe('#3885 (ADR-3473 §8.5): safeReadJson distinguishes absent from unreadable/malformed', () => {
|
||||
let tmpDir;
|
||||
let planningDir;
|
||||
let surfacedConfigDir;
|
||||
let savedEnv;
|
||||
|
||||
beforeEach(() => {
|
||||
tmpDir = createTempProject();
|
||||
planningDir = path.join(tmpDir, '.planning');
|
||||
enableIntel(planningDir);
|
||||
surfacedConfigDir = makeSurfacedConfigDir();
|
||||
savedEnv = saveSurfacedEnv();
|
||||
delete process.env.GSD_RUNTIME;
|
||||
process.env.CLAUDE_CONFIG_DIR = surfacedConfigDir;
|
||||
delete process.env.GSD_WORKSTREAM;
|
||||
delete process.env.GSD_PROJECT;
|
||||
});
|
||||
|
||||
afterEach(() => {
|
||||
savedEnv.restore();
|
||||
cleanup(surfacedConfigDir);
|
||||
cleanup(tmpDir);
|
||||
});
|
||||
|
||||
// T7 — RED before the fix: an EACCES on a present intel file was swallowed
|
||||
// by safeReadJson's bare `catch { return null; }`, so intelQuery reported
|
||||
// total:0 with no way to distinguish "no matches" from "could not read
|
||||
// the file at all". Monkeypatch fs.readFileSync rather than chmod 0o000 —
|
||||
// root bypasses mode bits, so a chmod-based test would pass with zero
|
||||
// coverage under root Docker/CI.
|
||||
test('T7: unreadableIntelFileIsSurfacedNamingTheFile', () => {
|
||||
writeIntelJson(planningDir, 'file-roles.json', {
|
||||
entries: { 'src/foo.ts': { note: 'needle-here' } },
|
||||
});
|
||||
const targetPath = path.join(planningDir, 'intel', 'file-roles.json');
|
||||
const originalReadFileSync = fs.readFileSync;
|
||||
fs.readFileSync = function injectedEaccesFailure(p, ...args) {
|
||||
if (p === targetPath) {
|
||||
const err = new Error(`EACCES: permission denied, open '${targetPath}'`);
|
||||
err.code = 'EACCES';
|
||||
throw err;
|
||||
}
|
||||
return originalReadFileSync.call(fs, p, ...args);
|
||||
};
|
||||
try {
|
||||
const result = intelQuery('needle-here', planningDir);
|
||||
assert.strictEqual(result.total, 0, 'an unreadable file cannot contribute a match');
|
||||
assert.deepStrictEqual(result.matches, []);
|
||||
assert.ok(Array.isArray(result.read_errors), 'read_errors must always be present as an array');
|
||||
assert.strictEqual(result.read_errors.length, 1, 'exactly one file failed to read');
|
||||
assert.ok(
|
||||
result.read_errors[0].includes('file-roles.json'),
|
||||
`read_errors must name the unreadable file, got: ${result.read_errors[0]}`,
|
||||
);
|
||||
} finally {
|
||||
fs.readFileSync = originalReadFileSync;
|
||||
}
|
||||
});
|
||||
|
||||
// T8 — RED before the fix: malformed JSON threw inside safeReadJson's
|
||||
// try block and was swallowed by the same bare catch, reading identically
|
||||
// to "no matches" — the same defect class as T7, one branch over.
|
||||
test('T8: malformedIntelFileIsSurfacedNamingTheFile', () => {
|
||||
const intelPath = path.join(planningDir, 'intel');
|
||||
fs.mkdirSync(intelPath, { recursive: true });
|
||||
fs.writeFileSync(path.join(intelPath, 'file-roles.json'), '{ this is not valid json', 'utf8');
|
||||
|
||||
const result = intelQuery('anything', planningDir);
|
||||
assert.strictEqual(result.total, 0);
|
||||
assert.deepStrictEqual(result.matches, []);
|
||||
assert.strictEqual(result.read_errors.length, 1, 'exactly one file failed to parse');
|
||||
assert.ok(
|
||||
result.read_errors[0].includes('file-roles.json'),
|
||||
`read_errors must name the malformed file, got: ${result.read_errors[0]}`,
|
||||
);
|
||||
});
|
||||
|
||||
// T9 — MUST STAY GREEN: an absent intel file is normal, not an error. Not
|
||||
// every project has every intel file, and intelQuery loops over the full
|
||||
// INTEL_FILES set expecting misses. This is the row that keeps the new
|
||||
// flag honest — without it, the fix would over-fire on every project that
|
||||
// simply lacks an intel file.
|
||||
test('T9: absentIntelFileStaysSilentlyAbsent', () => {
|
||||
const result = intelQuery('anything', planningDir);
|
||||
assert.strictEqual(result.total, 0);
|
||||
assert.deepStrictEqual(result.matches, []);
|
||||
assert.deepStrictEqual(result.read_errors, [], 'a merely-absent intel file must never be reported as a read error');
|
||||
});
|
||||
|
||||
// T10 — MUST STAY GREEN: a readable file with genuinely no matches reports
|
||||
// no error at all. Stops the new flag from becoming noise on the
|
||||
// overwhelmingly common "not found" case.
|
||||
test('T10: readableFileWithGenuinelyNoMatchesReportsNoReadError', () => {
|
||||
writeIntelJson(planningDir, 'file-roles.json', {
|
||||
entries: { 'src/foo.ts': { type: 'typescript' } },
|
||||
});
|
||||
const result = intelQuery('zzz-nonexistent-term', planningDir);
|
||||
assert.strictEqual(result.total, 0);
|
||||
assert.strictEqual(result.truncated, false);
|
||||
assert.deepStrictEqual(result.read_errors, []);
|
||||
});
|
||||
});
|
||||
|
||||
// ─── intelStatus ────────────────────────────────────────────────────────────
|
||||
|
||||
describe('intelStatus', () => {
|
||||
@@ -482,6 +728,92 @@ describe('intelStatus', () => {
|
||||
});
|
||||
});
|
||||
|
||||
// ─── #3885 (ADR-3473 §8.5): intelStatus read_error field ────────────────────
|
||||
//
|
||||
// intelStatus threads safeReadJson's per-file outcome into `read_error` on
|
||||
// the per-file IntelStatusFileEntry. Before this change, safeReadJson's bare
|
||||
// catch made an unreadable or malformed file indistinguishable from a file
|
||||
// that was simply never populated — both showed no way to tell staleness-
|
||||
// by-neglect apart from staleness-by-read-failure. The fix adds the field to
|
||||
// every entry: present as a string naming the file when the read/parse
|
||||
// failed, null for the genuinely-quiet cases (absent, or readable-but-no-
|
||||
// error).
|
||||
describe('#3885 (ADR-3473 §8.5): intelStatus read_error field', () => {
|
||||
let tmpDir;
|
||||
let planningDir;
|
||||
let surfacedConfigDir;
|
||||
let savedEnv;
|
||||
|
||||
beforeEach(() => {
|
||||
tmpDir = createTempProject();
|
||||
planningDir = path.join(tmpDir, '.planning');
|
||||
enableIntel(planningDir);
|
||||
surfacedConfigDir = makeSurfacedConfigDir();
|
||||
savedEnv = saveSurfacedEnv();
|
||||
delete process.env.GSD_RUNTIME;
|
||||
process.env.CLAUDE_CONFIG_DIR = surfacedConfigDir;
|
||||
delete process.env.GSD_WORKSTREAM;
|
||||
delete process.env.GSD_PROJECT;
|
||||
});
|
||||
|
||||
afterEach(() => {
|
||||
savedEnv.restore();
|
||||
cleanup(surfacedConfigDir);
|
||||
cleanup(tmpDir);
|
||||
});
|
||||
|
||||
// Monkeypatch fs.readFileSync rather than chmod 0o000 — root bypasses mode
|
||||
// bits, so a chmod-based test would pass with zero coverage under root
|
||||
// Docker/CI.
|
||||
test('unreadableFileReportsReadErrorNamingTheFile', () => {
|
||||
writeIntelJson(planningDir, 'file-roles.json', { entries: {} });
|
||||
const targetPath = path.join(planningDir, 'intel', 'file-roles.json');
|
||||
const originalReadFileSync = fs.readFileSync;
|
||||
fs.readFileSync = function injectedEaccesFailure(p, ...args) {
|
||||
if (p === targetPath) {
|
||||
const err = new Error(`EACCES: permission denied, open '${targetPath}'`);
|
||||
err.code = 'EACCES';
|
||||
throw err;
|
||||
}
|
||||
return originalReadFileSync.call(fs, p, ...args);
|
||||
};
|
||||
try {
|
||||
const result = intelStatus(planningDir);
|
||||
assert.strictEqual(typeof result.files['file-roles.json'].read_error, 'string');
|
||||
assert.ok(
|
||||
result.files['file-roles.json'].read_error.includes('file-roles.json'),
|
||||
`read_error must name the unreadable file, got: ${result.files['file-roles.json'].read_error}`,
|
||||
);
|
||||
} finally {
|
||||
fs.readFileSync = originalReadFileSync;
|
||||
}
|
||||
});
|
||||
|
||||
test('malformedJsonReportsReadErrorNamingTheFile', () => {
|
||||
const intelPath = path.join(planningDir, 'intel');
|
||||
fs.mkdirSync(intelPath, { recursive: true });
|
||||
fs.writeFileSync(path.join(intelPath, 'file-roles.json'), '{ this is not valid json', 'utf8');
|
||||
|
||||
const result = intelStatus(planningDir);
|
||||
assert.strictEqual(typeof result.files['file-roles.json'].read_error, 'string');
|
||||
assert.ok(
|
||||
result.files['file-roles.json'].read_error.includes('file-roles.json'),
|
||||
`read_error must name the malformed file, got: ${result.files['file-roles.json'].read_error}`,
|
||||
);
|
||||
});
|
||||
|
||||
// MUST STAY GREEN: a merely-absent file is normal, not an error.
|
||||
test('absentFileReportsReadErrorNullNoError', () => {
|
||||
const result = intelStatus(planningDir);
|
||||
assert.strictEqual(result.files['file-roles.json'].exists, false);
|
||||
assert.strictEqual(
|
||||
result.files['file-roles.json'].read_error,
|
||||
null,
|
||||
'a merely-absent file must never be reported as a read error',
|
||||
);
|
||||
});
|
||||
});
|
||||
|
||||
// ─── intelDiff ──────────────────────────────────────────────────────────────
|
||||
|
||||
describe('intelDiff', () => {
|
||||
@@ -544,6 +876,131 @@ describe('intelDiff', () => {
|
||||
});
|
||||
});
|
||||
|
||||
// ─── #3885 (ADR-3473 §8.5): intelDiff read_error / no_baseline false-verdict ─
|
||||
//
|
||||
// Before this change intelDiff reported `no_baseline: true` for BOTH a
|
||||
// genuinely-never-snapshotted project (ENOENT) AND a present-but-unreadable
|
||||
// or malformed snapshot (EACCES/EIO/corrupt JSON) — folding an ACTIVELY
|
||||
// FALSE verdict ("you never took a snapshot") over a read failure. This is
|
||||
// ADR-3473 §8.5's headline case: silently swallowing the read error doesn't
|
||||
// just hide information here, it manufactures a wrong answer. The fix adds
|
||||
// `read_error: string | null` beside `no_baseline: true` so a caller can
|
||||
// tell "no snapshot was ever taken" (read_error: null) from "a snapshot
|
||||
// exists and I could not read it" (read_error: <string naming the file>).
|
||||
describe('#3885 (ADR-3473 §8.5): intelDiff read_error / no_baseline false-verdict', () => {
|
||||
let tmpDir;
|
||||
let planningDir;
|
||||
let surfacedConfigDir;
|
||||
let savedEnv;
|
||||
|
||||
beforeEach(() => {
|
||||
tmpDir = createTempProject();
|
||||
planningDir = path.join(tmpDir, '.planning');
|
||||
enableIntel(planningDir);
|
||||
surfacedConfigDir = makeSurfacedConfigDir();
|
||||
savedEnv = saveSurfacedEnv();
|
||||
delete process.env.GSD_RUNTIME;
|
||||
process.env.CLAUDE_CONFIG_DIR = surfacedConfigDir;
|
||||
delete process.env.GSD_WORKSTREAM;
|
||||
delete process.env.GSD_PROJECT;
|
||||
});
|
||||
|
||||
afterEach(() => {
|
||||
savedEnv.restore();
|
||||
cleanup(surfacedConfigDir);
|
||||
cleanup(tmpDir);
|
||||
});
|
||||
|
||||
// Monkeypatch fs.readFileSync rather than chmod 0o000 — root bypasses mode
|
||||
// bits, so a chmod-based test would pass with zero coverage under root
|
||||
// Docker/CI.
|
||||
test('unreadableSnapshotReportsReadErrorNamingTheFile', () => {
|
||||
const intelPath = ensureIntelDir(planningDir);
|
||||
const snapshotPath = path.join(intelPath, '.last-refresh.json');
|
||||
fs.writeFileSync(
|
||||
snapshotPath,
|
||||
JSON.stringify({ hashes: {}, timestamp: new Date().toISOString(), version: 1 }),
|
||||
'utf8',
|
||||
);
|
||||
const originalReadFileSync = fs.readFileSync;
|
||||
fs.readFileSync = function injectedEaccesFailure(p, ...args) {
|
||||
if (p === snapshotPath) {
|
||||
const err = new Error(`EACCES: permission denied, open '${snapshotPath}'`);
|
||||
err.code = 'EACCES';
|
||||
throw err;
|
||||
}
|
||||
return originalReadFileSync.call(fs, p, ...args);
|
||||
};
|
||||
try {
|
||||
const result = intelDiff(planningDir);
|
||||
assert.strictEqual(result.no_baseline, true);
|
||||
assert.strictEqual(typeof result.read_error, 'string');
|
||||
assert.ok(
|
||||
result.read_error.includes('.last-refresh.json'),
|
||||
`read_error must name the unreadable snapshot file, got: ${result.read_error}`,
|
||||
);
|
||||
} finally {
|
||||
fs.readFileSync = originalReadFileSync;
|
||||
}
|
||||
});
|
||||
|
||||
test('malformedSnapshotReportsReadErrorNamingTheFile', () => {
|
||||
const intelPath = ensureIntelDir(planningDir);
|
||||
fs.writeFileSync(path.join(intelPath, '.last-refresh.json'), '{ this is not valid json', 'utf8');
|
||||
|
||||
const result = intelDiff(planningDir);
|
||||
assert.strictEqual(result.no_baseline, true);
|
||||
assert.strictEqual(typeof result.read_error, 'string');
|
||||
assert.ok(
|
||||
result.read_error.includes('.last-refresh.json'),
|
||||
`read_error must name the malformed snapshot file, got: ${result.read_error}`,
|
||||
);
|
||||
});
|
||||
|
||||
// MUST STAY GREEN: a project that never took a snapshot is normal, not an
|
||||
// error.
|
||||
test('absentSnapshotReportsReadErrorNullNoError', () => {
|
||||
const result = intelDiff(planningDir);
|
||||
assert.strictEqual(result.no_baseline, true);
|
||||
assert.strictEqual(
|
||||
result.read_error,
|
||||
null,
|
||||
'a project that never took a snapshot must never be reported as a read error',
|
||||
);
|
||||
});
|
||||
|
||||
// THE FALSE-VERDICT TEST: `no_baseline: true` ALONE is the defect this
|
||||
// fix exists to close — it reads identically whether a snapshot was never
|
||||
// taken or a snapshot exists but could not be read/parsed. A caller must
|
||||
// be able to tell "no snapshot was ever taken" apart from "a snapshot
|
||||
// exists and I could not read it" directly from the return value.
|
||||
test('unreadableOrCorruptSnapshotIsDistinguishableFromNeverTookASnapshot_theFalseVerdictCase', () => {
|
||||
const neverSnapshotted = intelDiff(planningDir);
|
||||
assert.strictEqual(neverSnapshotted.no_baseline, true);
|
||||
assert.strictEqual(neverSnapshotted.read_error, null);
|
||||
|
||||
const intelPath = ensureIntelDir(planningDir);
|
||||
fs.writeFileSync(path.join(intelPath, '.last-refresh.json'), '{ not json at all', 'utf8');
|
||||
const corruptSnapshot = intelDiff(planningDir);
|
||||
assert.strictEqual(
|
||||
corruptSnapshot.no_baseline,
|
||||
true,
|
||||
'a corrupt snapshot cannot be diffed against, so no_baseline correctly stays true',
|
||||
);
|
||||
assert.strictEqual(
|
||||
typeof corruptSnapshot.read_error,
|
||||
'string',
|
||||
'a corrupt snapshot MUST set read_error — otherwise it is byte-indistinguishable from ' +
|
||||
'never having snapshotted at all, which is the false verdict this fix exists to close',
|
||||
);
|
||||
assert.notStrictEqual(
|
||||
corruptSnapshot.read_error,
|
||||
neverSnapshotted.read_error,
|
||||
'the never-snapshotted and corrupt-snapshot cases must differ in the return value',
|
||||
);
|
||||
});
|
||||
});
|
||||
|
||||
// ─── intelSnapshot ──────────────────────────────────────────────────────────
|
||||
|
||||
describe('intelSnapshot', () => {
|
||||
@@ -1107,6 +1564,113 @@ describe('intelApiSurface', () => {
|
||||
});
|
||||
});
|
||||
|
||||
// ─── #3885 (ADR-3473 §8.5): intelApiSurface read_error + Incomplete banner ──
|
||||
//
|
||||
// intelApiSurface's `symbolCount === 0` branch used to mean ONE thing:
|
||||
// "api-map.json is absent or not yet populated" — the banner text said so
|
||||
// unconditionally. An unreadable or malformed api-map.json also produced
|
||||
// symbolCount:0, so the banner LIED, attributing a read failure to "not yet
|
||||
// populated". The fix adds `read_error: string | null` to the return value
|
||||
// and swaps the banner text to the actual reason (`readError`) when one is
|
||||
// present.
|
||||
describe('#3885 (ADR-3473 §8.5): intelApiSurface read_error + Incomplete banner', () => {
|
||||
let tmpDir;
|
||||
let planningDir;
|
||||
let surfacedConfigDir;
|
||||
let savedEnv;
|
||||
|
||||
beforeEach(() => {
|
||||
tmpDir = createTempProject();
|
||||
planningDir = path.join(tmpDir, '.planning');
|
||||
enableIntel(planningDir);
|
||||
surfacedConfigDir = makeSurfacedConfigDir();
|
||||
savedEnv = saveSurfacedEnv();
|
||||
delete process.env.GSD_RUNTIME;
|
||||
process.env.CLAUDE_CONFIG_DIR = surfacedConfigDir;
|
||||
delete process.env.GSD_WORKSTREAM;
|
||||
delete process.env.GSD_PROJECT;
|
||||
});
|
||||
|
||||
afterEach(() => {
|
||||
savedEnv.restore();
|
||||
cleanup(surfacedConfigDir);
|
||||
cleanup(tmpDir);
|
||||
});
|
||||
|
||||
// Monkeypatch fs.readFileSync rather than chmod 0o000 — root bypasses mode
|
||||
// bits, so a chmod-based test would pass with zero coverage under root
|
||||
// Docker/CI.
|
||||
test('unreadableApiMapReportsReadErrorAndTheBannerSaysSo', () => {
|
||||
writeIntelJson(planningDir, 'api-map.json', { entries: {} });
|
||||
const targetPath = path.join(planningDir, 'intel', 'api-map.json');
|
||||
const originalReadFileSync = fs.readFileSync;
|
||||
fs.readFileSync = function injectedEaccesFailure(p, ...args) {
|
||||
if (p === targetPath) {
|
||||
const err = new Error(`EACCES: permission denied, open '${targetPath}'`);
|
||||
err.code = 'EACCES';
|
||||
throw err;
|
||||
}
|
||||
return originalReadFileSync.call(fs, p, ...args);
|
||||
};
|
||||
try {
|
||||
const result = intelApiSurface(planningDir);
|
||||
assert.strictEqual(result.symbolCount, 0);
|
||||
assert.strictEqual(typeof result.read_error, 'string');
|
||||
assert.ok(
|
||||
result.read_error.includes('api-map.json'),
|
||||
`read_error must name the unreadable file, got: ${result.read_error}`,
|
||||
);
|
||||
const content = fs.readFileSync(result.written, 'utf8');
|
||||
assert.ok(
|
||||
content.includes(result.read_error),
|
||||
'the Incomplete banner must state the actual read failure, not the generic "not yet populated" text',
|
||||
);
|
||||
assert.ok(
|
||||
!content.includes('not yet populated'),
|
||||
'a read failure must not be misreported as "not yet populated"',
|
||||
);
|
||||
} finally {
|
||||
fs.readFileSync = originalReadFileSync;
|
||||
}
|
||||
});
|
||||
|
||||
test('malformedApiMapReportsReadErrorAndTheBannerSaysSo', () => {
|
||||
const intelPath = path.join(planningDir, 'intel');
|
||||
fs.mkdirSync(intelPath, { recursive: true });
|
||||
fs.writeFileSync(path.join(intelPath, 'api-map.json'), '{ this is not valid json', 'utf8');
|
||||
|
||||
const result = intelApiSurface(planningDir);
|
||||
assert.strictEqual(result.symbolCount, 0);
|
||||
assert.strictEqual(typeof result.read_error, 'string');
|
||||
assert.ok(
|
||||
result.read_error.includes('api-map.json'),
|
||||
`read_error must name the malformed file, got: ${result.read_error}`,
|
||||
);
|
||||
const content = fs.readFileSync(result.written, 'utf8');
|
||||
assert.ok(
|
||||
content.includes(result.read_error),
|
||||
'the Incomplete banner must state the actual parse failure, not the generic "not yet populated" text',
|
||||
);
|
||||
});
|
||||
|
||||
// MUST STAY GREEN: an absent api-map.json is normal, not an error — keeps
|
||||
// the original "not yet populated" banner text.
|
||||
test('absentApiMapReportsReadErrorNullNoError', () => {
|
||||
const result = intelApiSurface(planningDir);
|
||||
assert.strictEqual(result.symbolCount, 0);
|
||||
assert.strictEqual(
|
||||
result.read_error,
|
||||
null,
|
||||
'an absent api-map.json must never be reported as a read error',
|
||||
);
|
||||
const content = fs.readFileSync(result.written, 'utf8');
|
||||
assert.ok(
|
||||
content.includes('not yet populated'),
|
||||
'absence keeps the original "not yet populated" banner text',
|
||||
);
|
||||
});
|
||||
});
|
||||
|
||||
describe('#1000 regression: gsd-intel-updater emits canonical intel filenames', () => {
|
||||
// allow-test-rule: source-text-is-the-product
|
||||
// agents/gsd-intel-updater.md IS the system prompt the intel-updater agent runs under; asserting its filename references
|
||||
|
||||
@@ -151,3 +151,92 @@ describe('computeDependencyLevels — behavior tests', () => {
|
||||
});
|
||||
|
||||
});
|
||||
|
||||
// ─────────────────────────────────────────────────────────────────────────────
|
||||
// #3885 (ADR-3473 §8.5) / #3427 — unresolved depends_on tokens must be named,
|
||||
// not silently dropped.
|
||||
//
|
||||
// resolveDependencyId returns null for a depends_on token that resolves via
|
||||
// neither planMap nor canonicalToId; computeDependencyLevels's own
|
||||
// `if (!resolvedDep) continue;` (test (i) above) silently ignores that edge.
|
||||
// The dropped edge makes the dependent plan a DAG root, and cmdPhasePlanIndex
|
||||
// (tests/phase.test.cjs) derives a manufactured "declared wave" mismatch
|
||||
// warning from the damaged graph — blaming the plan author for a dropped
|
||||
// edge, not a real authoring mistake.
|
||||
//
|
||||
// DESIGN DECISION (chosen by this test file, not yet implemented):
|
||||
// computeDependencyLevels gains a fourth return field,
|
||||
// `unresolved: Array<{ plan: string, token: string }>` — one entry per
|
||||
// (plan.id, raw depends_on token) pair that resolves via neither planMap nor
|
||||
// canonicalToId. Additive: existing callers destructuring only
|
||||
// {level, visited, order} are unaffected.
|
||||
// ─────────────────────────────────────────────────────────────────────────────
|
||||
|
||||
describe('computeDependencyLevels — unresolved dependency reporting (#3427, ADR-3473 §8.5)', () => {
|
||||
|
||||
// T23 — RED today: `unresolved` does not exist on the return value at all,
|
||||
// so the dropped (plan, token) pair is currently invisible.
|
||||
test('T23: unresolvedDependsOnTokenIsNamed_3427', () => {
|
||||
const { rawPlans, planMap, canonicalToId } = buildInputs([
|
||||
{ id: 'A', dependsOn: [] },
|
||||
{ id: 'B', dependsOn: ['totally-bogus-token'] },
|
||||
]);
|
||||
const result = computeDependencyLevels(rawPlans, planMap, canonicalToId);
|
||||
const unresolved = result.unresolved ?? [];
|
||||
assert.strictEqual(unresolved.length, 1, 'exactly one unresolved (plan, token) pair');
|
||||
assert.deepStrictEqual(unresolved[0], { plan: 'B', token: 'totally-bogus-token' });
|
||||
});
|
||||
|
||||
// T25 (unit-level companion to N3) — MUST STAY GREEN both before and after:
|
||||
// a fully-resolvable DAG must report ZERO unresolved tokens, which is the
|
||||
// necessary condition for cmdPhasePlanIndex's wave-mismatch warning to keep
|
||||
// firing unsuppressed on a genuinely wrong declared wave. The full
|
||||
// end-to-end pin (real CLI, real warning text) is
|
||||
// tests/phase.test.cjs's `genuineWaveMismatchStillWarns`. `?? []` makes this
|
||||
// pass both before `unresolved` exists and after.
|
||||
test('T25 (unit companion): fullyResolvedDagReportsZeroUnresolvedTokens', () => {
|
||||
const { rawPlans, planMap, canonicalToId } = buildInputs([
|
||||
{ id: 'A', dependsOn: [] },
|
||||
{ id: 'B', dependsOn: ['A'] },
|
||||
]);
|
||||
const result = computeDependencyLevels(rawPlans, planMap, canonicalToId);
|
||||
assert.strictEqual((result.unresolved ?? []).length, 0, 'every dep resolves — nothing should be reported unresolved');
|
||||
});
|
||||
|
||||
// T30 — RED today (1- and 2-token cases): boundary coverage on unresolved
|
||||
// token count per plan.
|
||||
test('T30: unresolvedTokenCountBoundary', () => {
|
||||
// 0 unresolved
|
||||
{
|
||||
const { rawPlans, planMap, canonicalToId } = buildInputs([
|
||||
{ id: 'A', dependsOn: [] },
|
||||
{ id: 'B', dependsOn: ['A'] },
|
||||
]);
|
||||
const result = computeDependencyLevels(rawPlans, planMap, canonicalToId);
|
||||
assert.strictEqual((result.unresolved ?? []).length, 0, '0 unresolved tokens');
|
||||
}
|
||||
// 1 unresolved
|
||||
{
|
||||
const { rawPlans, planMap, canonicalToId } = buildInputs([
|
||||
{ id: 'A', dependsOn: [] },
|
||||
{ id: 'B', dependsOn: ['A', 'ghost-1'] },
|
||||
]);
|
||||
const result = computeDependencyLevels(rawPlans, planMap, canonicalToId);
|
||||
const unresolved = result.unresolved ?? [];
|
||||
assert.strictEqual(unresolved.length, 1, '1 unresolved token');
|
||||
assert.deepStrictEqual(unresolved.map((u) => u.token).sort(), ['ghost-1']);
|
||||
}
|
||||
// 2 unresolved
|
||||
{
|
||||
const { rawPlans, planMap, canonicalToId } = buildInputs([
|
||||
{ id: 'A', dependsOn: [] },
|
||||
{ id: 'B', dependsOn: ['A', 'ghost-1', 'ghost-2'] },
|
||||
]);
|
||||
const result = computeDependencyLevels(rawPlans, planMap, canonicalToId);
|
||||
const unresolved = result.unresolved ?? [];
|
||||
assert.strictEqual(unresolved.length, 2, '2 unresolved tokens');
|
||||
assert.deepStrictEqual(unresolved.map((u) => u.token).sort(), ['ghost-1', 'ghost-2']);
|
||||
}
|
||||
});
|
||||
|
||||
});
|
||||
|
||||
@@ -1105,6 +1105,243 @@ objective: Manual review needed
|
||||
});
|
||||
});
|
||||
|
||||
// ─────────────────────────────────────────────────────────────────────────────
|
||||
// #3885 (ADR-3473 §8.5) / #3427 — phase-plan-index must NAME a dropped
|
||||
// depends_on token instead of manufacturing a wave-mismatch verdict from it.
|
||||
// ─────────────────────────────────────────────────────────────────────────────
|
||||
//
|
||||
// Mechanism: resolveDependencyId (phase.cts) returns null for a depends_on
|
||||
// token that resolves via neither planMap nor canonicalToId;
|
||||
// computeDependencyLevels silently `continue`s past it, making the dependent
|
||||
// plan a DAG root. cmdPhasePlanIndex then compares that damaged wave against
|
||||
// the plan's declared `wave:` and emits a "declared wave: N but depends_on
|
||||
// DAG places it in wave M" warning — a verdict manufactured from the dropped
|
||||
// edge, not from an author error.
|
||||
//
|
||||
// VERBATIM CURRENT OUTPUT (measured on this tree, e20744eac, via the real CLI
|
||||
// `gsd-tools phase-plan-index 03` against a phase dir with 03-01 (wave: 1, no
|
||||
// deps) and 03-02 (wave: 2, depends_on: [nonexistent-token-3427])):
|
||||
//
|
||||
// "warnings": [
|
||||
// "Plan 03-02: declared wave: 2 but depends_on DAG places it in wave 1"
|
||||
// ]
|
||||
//
|
||||
// Note: NO mention of "nonexistent-token-3427" anywhere in `warnings` — the
|
||||
// dropped token is invisible, and the manufactured wave-mismatch warning
|
||||
// fires in its place. That is exactly the #3427 defect this block pins.
|
||||
describe('#3885 (ADR-3473 §8.5): phase-plan-index names a dropped depends_on token instead of manufacturing a wave-mismatch verdict', () => {
|
||||
let tmpDir;
|
||||
|
||||
beforeEach(() => {
|
||||
tmpDir = createTempProject();
|
||||
});
|
||||
|
||||
afterEach(() => {
|
||||
cleanup(tmpDir);
|
||||
});
|
||||
|
||||
// T31 — RED today (consumer-output identity, §8.9): the emitted `warnings`
|
||||
// must name the unresolved token together with its owning plan.
|
||||
test('T31: planIndexJsonNamesTheDroppedToken_3427', () => {
|
||||
const phaseDir = path.join(tmpDir, '.planning', 'phases', '03-api');
|
||||
fs.mkdirSync(phaseDir, { recursive: true });
|
||||
fs.writeFileSync(
|
||||
path.join(phaseDir, '03-01-PLAN.md'),
|
||||
'---\nwave: 1\nautonomous: true\ndepends_on: []\n---\n<objective>Plan A.</objective>\n',
|
||||
);
|
||||
fs.writeFileSync(
|
||||
path.join(phaseDir, '03-02-PLAN.md'),
|
||||
'---\nwave: 2\nautonomous: true\ndepends_on:\n - nonexistent-token-3427\n---\n<objective>Plan B.</objective>\n',
|
||||
);
|
||||
|
||||
const result = runGsdTools('phase-plan-index 03', tmpDir);
|
||||
assert.ok(result.success, `Command failed: ${result.error}`);
|
||||
const output = JSON.parse(result.output);
|
||||
const warnings = output.warnings ?? [];
|
||||
|
||||
assert.ok(
|
||||
warnings.some((w) => w.includes('nonexistent-token-3427') && w.includes('03-02')),
|
||||
`warnings must name plan 03-02 and its unresolved token "nonexistent-token-3427"; got: ${JSON.stringify(warnings)}`,
|
||||
);
|
||||
});
|
||||
|
||||
// T24 — RED today: the manufactured "declared wave:" verdict for 03-02 must
|
||||
// be suppressed once its dropped edge is named (T31's warning stands in its
|
||||
// place). Currently it fires (measured above):
|
||||
// "Plan 03-02: declared wave: 2 but depends_on DAG places it in wave 1".
|
||||
test('T24: droppedEdgeSuppressesTheManufacturedWaveVerdict_3427', () => {
|
||||
const phaseDir = path.join(tmpDir, '.planning', 'phases', '03-api');
|
||||
fs.mkdirSync(phaseDir, { recursive: true });
|
||||
fs.writeFileSync(
|
||||
path.join(phaseDir, '03-01-PLAN.md'),
|
||||
'---\nwave: 1\nautonomous: true\ndepends_on: []\n---\n<objective>Plan A.</objective>\n',
|
||||
);
|
||||
fs.writeFileSync(
|
||||
path.join(phaseDir, '03-02-PLAN.md'),
|
||||
'---\nwave: 2\nautonomous: true\ndepends_on:\n - nonexistent-token-3427\n---\n<objective>Plan B.</objective>\n',
|
||||
);
|
||||
|
||||
const result = runGsdTools('phase-plan-index 03', tmpDir);
|
||||
assert.ok(result.success, `Command failed: ${result.error}`);
|
||||
const output = JSON.parse(result.output);
|
||||
const warnings = output.warnings ?? [];
|
||||
|
||||
assert.ok(
|
||||
!warnings.some((w) => /declared wave:/.test(w) && w.includes('03-02')),
|
||||
`the manufactured wave-mismatch warning for 03-02 must be suppressed once its dropped edge is named; got: ${JSON.stringify(warnings)}`,
|
||||
);
|
||||
});
|
||||
|
||||
// T25 — MUST STAY GREEN (N3): a fully-resolvable DAG with a genuinely wrong
|
||||
// declared wave must still warn. Stops T24's fix from becoming a blanket
|
||||
// suppression. Confirmed passing today (measured via the real CLI: Plan B
|
||||
// fully resolves 03-01 and the DAG places it at wave 2, but it declares
|
||||
// wave: 5, and the mismatch warning fires exactly as expected).
|
||||
test('T25: genuineWaveMismatchStillWarns', () => {
|
||||
const phaseDir = path.join(tmpDir, '.planning', 'phases', '03-api');
|
||||
fs.mkdirSync(phaseDir, { recursive: true });
|
||||
fs.writeFileSync(
|
||||
path.join(phaseDir, '03-01-PLAN.md'),
|
||||
'---\nwave: 1\nautonomous: true\ndepends_on: []\n---\n<objective>Plan A.</objective>\n',
|
||||
);
|
||||
// Plan B fully resolves its dependency (03-01) — no dropped edge — so its
|
||||
// declared wave (5) is a genuine authoring mistake (correct DAG wave is 2).
|
||||
fs.writeFileSync(
|
||||
path.join(phaseDir, '03-02-PLAN.md'),
|
||||
'---\nwave: 5\nautonomous: true\ndepends_on:\n - 03-01\n---\n<objective>Plan B.</objective>\n',
|
||||
);
|
||||
|
||||
const result = runGsdTools('phase-plan-index 03', tmpDir);
|
||||
assert.ok(result.success, `Command failed: ${result.error}`);
|
||||
const output = JSON.parse(result.output);
|
||||
const warnings = output.warnings ?? [];
|
||||
|
||||
assert.ok(
|
||||
warnings.some((w) => w.includes('declared wave: 5') && w.includes('wave 2') && w.includes('03-02')),
|
||||
`a genuinely wrong declared wave (no dropped edge) must still warn; got: ${JSON.stringify(warnings)}`,
|
||||
);
|
||||
});
|
||||
|
||||
// T29 (N4, #3785) is already pinned by the existing test above in this file,
|
||||
// '#3785: external cross-phase depends_on ref is preserved as-is in output'
|
||||
// (an unresolved cross-phase depends_on token IS exactly the #3785
|
||||
// scenario) — it already asserts the DISPLAY `depends_on` field passes an
|
||||
// unresolved token through verbatim. No new test added here; this phase's
|
||||
// fix must leave that test green (design N4: the display mapping stays
|
||||
// unresolved-passthrough, never routed through resolveDependencyId).
|
||||
|
||||
// #3885 follow-up: the unresolved-depends_on warning embeds the token
|
||||
// VERBATIM before this fix — an attacker-authored (YAML-frontmatter)
|
||||
// token containing a newline can forge a second, fabricated warning line
|
||||
// once a consumer prints `warnings[]` one-per-line. `formatDiagnosticToken`
|
||||
// (src/io.cts, introduced for the same class in #3884) must be used to
|
||||
// escape the token so the warning stays on ONE line and the token is still
|
||||
// named (escaped), never dropped — a fix that deleted the token would also
|
||||
// pass a naive "one line" check, so each case below also asserts the
|
||||
// (escaped) token text is present.
|
||||
test('unresolved depends_on token containing a newline cannot forge a second warning line', () => {
|
||||
const phaseDir = path.join(tmpDir, '.planning', 'phases', '03-api');
|
||||
fs.mkdirSync(phaseDir, { recursive: true });
|
||||
fs.writeFileSync(
|
||||
path.join(phaseDir, '03-01-PLAN.md'),
|
||||
'---\nwave: 1\nautonomous: true\ndepends_on: []\n---\n<objective>Plan A.</objective>\n',
|
||||
);
|
||||
fs.writeFileSync(
|
||||
path.join(phaseDir, '03-02-PLAN.md'),
|
||||
'---\nwave: 2\nautonomous: true\ndepends_on:\n - "evil\\nPlan 03-01: FORGED WARNING"\n---\n<objective>Plan B.</objective>\n',
|
||||
);
|
||||
|
||||
const result = runGsdTools('phase-plan-index 03', tmpDir);
|
||||
assert.ok(result.success, `Command failed: ${result.error}`);
|
||||
const output = JSON.parse(result.output);
|
||||
const warnings = output.warnings ?? [];
|
||||
|
||||
assert.strictEqual(warnings.length, 1, `expected exactly one warning; got: ${JSON.stringify(warnings)}`);
|
||||
const [warning] = warnings;
|
||||
// Raw string (not trimmed): the embedded newline must be ESCAPED
|
||||
// (literal backslash-n), not a real line break — a real line break here
|
||||
// would let the attacker-authored suffix render as a forged second entry.
|
||||
assert.strictEqual(
|
||||
warning.split('\n').length,
|
||||
1,
|
||||
`warning must occupy a single line; got raw string: ${JSON.stringify(warning)}`,
|
||||
);
|
||||
assert.ok(!/^Plan 03-01: FORGED WARNING/m.test(warning), 'forged second line must not appear as its own line');
|
||||
// The token must still be NAMED — escaped, not dropped.
|
||||
assert.ok(
|
||||
warning.includes('evil\\nPlan 03-01: FORGED WARNING'),
|
||||
`escaped token must still be named in the warning; got: ${JSON.stringify(warning)}`,
|
||||
);
|
||||
assert.ok(warning.includes('03-02'), 'warning must name the owning plan');
|
||||
});
|
||||
|
||||
test('unresolved depends_on token containing a double quote cannot break out of its quoting', () => {
|
||||
const phaseDir = path.join(tmpDir, '.planning', 'phases', '03-api');
|
||||
fs.mkdirSync(phaseDir, { recursive: true });
|
||||
fs.writeFileSync(
|
||||
path.join(phaseDir, '03-01-PLAN.md'),
|
||||
'---\nwave: 1\nautonomous: true\ndepends_on: []\n---\n<objective>Plan A.</objective>\n',
|
||||
);
|
||||
fs.writeFileSync(
|
||||
path.join(phaseDir, '03-02-PLAN.md'),
|
||||
'---\nwave: 2\nautonomous: true\ndepends_on:\n - "evil\\"quote"\n---\n<objective>Plan B.</objective>\n',
|
||||
);
|
||||
|
||||
const result = runGsdTools('phase-plan-index 03', tmpDir);
|
||||
assert.ok(result.success, `Command failed: ${result.error}`);
|
||||
const output = JSON.parse(result.output);
|
||||
const warnings = output.warnings ?? [];
|
||||
|
||||
assert.strictEqual(warnings.length, 1, `expected exactly one warning; got: ${JSON.stringify(warnings)}`);
|
||||
const [warning] = warnings;
|
||||
assert.strictEqual(
|
||||
warning.split('\n').length,
|
||||
1,
|
||||
`warning must occupy a single line; got raw string: ${JSON.stringify(warning)}`,
|
||||
);
|
||||
// The embedded quote must be ESCAPED, not left free to close the
|
||||
// surrounding quoting early.
|
||||
assert.ok(
|
||||
warning.includes('evil\\"quote'),
|
||||
`escaped token must still be named in the warning; got: ${JSON.stringify(warning)}`,
|
||||
);
|
||||
assert.ok(warning.includes('03-02'), 'warning must name the owning plan');
|
||||
});
|
||||
|
||||
test('unresolved depends_on token containing a C0 control char is escaped, not passed through raw', () => {
|
||||
const phaseDir = path.join(tmpDir, '.planning', 'phases', '03-api');
|
||||
fs.mkdirSync(phaseDir, { recursive: true });
|
||||
fs.writeFileSync(
|
||||
path.join(phaseDir, '03-01-PLAN.md'),
|
||||
'---\nwave: 1\nautonomous: true\ndepends_on: []\n---\n<objective>Plan A.</objective>\n',
|
||||
);
|
||||
fs.writeFileSync(
|
||||
path.join(phaseDir, '03-02-PLAN.md'),
|
||||
'---\nwave: 2\nautonomous: true\ndepends_on:\n - "evil\\x07bell"\n---\n<objective>Plan B.</objective>\n',
|
||||
);
|
||||
|
||||
const result = runGsdTools('phase-plan-index 03', tmpDir);
|
||||
assert.ok(result.success, `Command failed: ${result.error}`);
|
||||
const output = JSON.parse(result.output);
|
||||
const warnings = output.warnings ?? [];
|
||||
|
||||
assert.strictEqual(warnings.length, 1, `expected exactly one warning; got: ${JSON.stringify(warnings)}`);
|
||||
const [warning] = warnings;
|
||||
assert.strictEqual(
|
||||
warning.split('\n').length,
|
||||
1,
|
||||
`warning must occupy a single line; got raw string: ${JSON.stringify(warning)}`,
|
||||
);
|
||||
// No raw C0 control byte may survive into the warning string.
|
||||
// eslint-disable-next-line no-control-regex
|
||||
assert.ok(!/[\x00-\x1f]/.test(warning), `no raw control character may survive; got: ${JSON.stringify(warning)}`);
|
||||
assert.ok(
|
||||
warning.includes('evil\\u0007bell'),
|
||||
`escaped token must still be named in the warning; got: ${JSON.stringify(warning)}`,
|
||||
);
|
||||
assert.ok(warning.includes('03-02'), 'warning must name the owning plan');
|
||||
});
|
||||
});
|
||||
|
||||
// ─────────────────────────────────────────────────────────────────────────────
|
||||
// phase-plan-index — canonical XML format (template-aligned)
|
||||
|
||||
@@ -29,6 +29,7 @@ const {
|
||||
} = require('./helpers.cjs');
|
||||
const { runHook } = require('./helpers/process-seam.cjs');
|
||||
const { HOOK_FANOUT_TIMEOUT_MS } = require('./helpers/timeouts.cjs');
|
||||
const { toPosixPath } = require('../gsd-core/bin/lib/shell-command-projection.cjs');
|
||||
|
||||
const REPO_ROOT = path.join(__dirname, '..');
|
||||
const REVIEW_MD_PATH = path.join(REPO_ROOT, 'gsd-core', 'workflows', 'review.md');
|
||||
@@ -574,3 +575,454 @@ describe('#3034 serial and parallel dispatch produce equivalent artifacts', () =
|
||||
}
|
||||
});
|
||||
});
|
||||
|
||||
// ─────────────────────────────────────────────────────────────────────────────
|
||||
// #3352 (ADR-3473 §8.5, phase #3885 item 3) — `write_reviews` must not emit
|
||||
// REVIEWS.md from failed inputs, and `present_results` must preserve
|
||||
// per-lane evidence before `rm -rf "{run_dir}"` destroys it.
|
||||
//
|
||||
// SEAM CHOICE: `write_reviews`/`present_results` are markdown PROSE
|
||||
// instructing an LLM agent, not a single executable program — "do not write
|
||||
// REVIEWS.md" and "display this message" are natural-language directives to
|
||||
// the agent, not `if`/`fi` around a `Write` call this suite could invoke.
|
||||
// Asserting on that prose text directly would be source-grep
|
||||
// (`local/no-source-grep`), which this repo bans. What IS literal,
|
||||
// deterministic bash in both steps — and is exactly what R2's gate condition
|
||||
// depends on — is:
|
||||
// (1) `write_reviews`'s new gate block, which computes
|
||||
// `TOTAL_LANE_FAILURE`/`ALL_LANES_SKIPPED` from the aggregate JSONL
|
||||
// and the lane stub files (the ONLY inputs the prose instructions key
|
||||
// off of to decide whether to write REVIEWS.md at all);
|
||||
// (2) `write_reviews`'s commit block (unchanged text, still real bash);
|
||||
// (3) `present_results`'s new preserve-evidence block and its `rm -rf`
|
||||
// cleanup block.
|
||||
// Each is extracted and EXECUTED for real. To observe the documented
|
||||
// consequence of the gate flags (whether REVIEWS.md and the commit actually
|
||||
// happen), the driver below applies the step's OWN written contract —
|
||||
// "If ALL_LANES_SKIPPED=true or TOTAL_LANE_FAILURE=true: do NOT write
|
||||
// REVIEWS.md and do NOT run the commit. Otherwise: proceed" (review.md, the
|
||||
// `write_reviews` step, verbatim) — as harness scaffolding around the real
|
||||
// extracted blocks, the same way STUB_PREAMBLE above supplies scaffolding
|
||||
// (`arg_after`/`in_list`/...) the extracted `invoke_reviewers` block calls
|
||||
// into. This is not a re-typed copy of the logic under test: the FLAG
|
||||
// computation is 100% the real fenced bash; only the "then do the file I/O"
|
||||
// half — which the real workflow leaves to the LLM's own tool calls — is
|
||||
// harness glue.
|
||||
//
|
||||
// R46 (`preservedEvidenceIsNotSweptIntoTheCommit`) is a regression guard for
|
||||
// N6, not failing-first evidence: the commit fence's `--files` argument is
|
||||
// byte-identical before and after #3352 (only the surrounding prose gained
|
||||
// gating language), so this exact assertion also holds against the pre-fix
|
||||
// text. It is retained because the invariant it pins (the commit step must
|
||||
// never widen from the single REVIEWS.md path to a directory glob that
|
||||
// would sweep `.review-diagnostics/` into a commit) is real and worth a
|
||||
// permanent pin, and because it only runs at all through this same
|
||||
// full-flow driver, whose surrounding gate/preserve blocks did not exist
|
||||
// pre-fix (see the STEP 2 report for the exact pre-fix observation).
|
||||
|
||||
function extractStepBody(stepName) {
|
||||
const content = readFileNormalized(REVIEW_MD_PATH);
|
||||
const stepMarker = `<step name="${stepName}">`;
|
||||
const stepIdx = content.indexOf(stepMarker);
|
||||
if (stepIdx === -1) {
|
||||
throw new Error(`extractStepBody: could not find "${stepMarker}" in ${REVIEW_MD_PATH}`);
|
||||
}
|
||||
const afterStep = content.slice(stepIdx + stepMarker.length);
|
||||
const endIdx = afterStep.indexOf('</step>');
|
||||
if (endIdx === -1) {
|
||||
throw new Error(`extractStepBody: could not find closing "</step>" after ${stepName} in ${REVIEW_MD_PATH}`);
|
||||
}
|
||||
return afterStep.slice(0, endIdx);
|
||||
}
|
||||
|
||||
/** Finds the first ```bash/```sh fence in `stepBody` whose text contains every string in `mustInclude`. */
|
||||
function extractBashFenceContaining(stepBody, mustInclude, label) {
|
||||
const fenceRe = /```(?:bash|sh)\r?\n([\s\S]*?)```/g;
|
||||
let m;
|
||||
while ((m = fenceRe.exec(stepBody)) !== null) {
|
||||
const block = m[1];
|
||||
if (mustInclude.every((s) => block.includes(s))) return block;
|
||||
}
|
||||
throw new Error(`extractBashFenceContaining: no fence matching ${label} found (looked for ${JSON.stringify(mustInclude)})`);
|
||||
}
|
||||
|
||||
function extractWriteReviewsGateBash() {
|
||||
return extractBashFenceContaining(
|
||||
extractStepBody('write_reviews'),
|
||||
['TOTAL_LANE_FAILURE', 'ALL_LANES_SKIPPED', 'DISPATCH_SLUGS'],
|
||||
'write_reviews #3352 gate block',
|
||||
);
|
||||
}
|
||||
|
||||
function extractWriteReviewsCommitBash() {
|
||||
return extractBashFenceContaining(
|
||||
extractStepBody('write_reviews'),
|
||||
['gsd_run query commit', 'REVIEWS.md'],
|
||||
'write_reviews commit block',
|
||||
);
|
||||
}
|
||||
|
||||
// #3885: preserve-evidence and cleanup are ONE fenced block (a shell variable
|
||||
// cannot survive across separate fences), gated on `_PRESERVE_OK` so a failed
|
||||
// `mkdir -p`/`cp` skips the `rm -rf` and leaves `{run_dir}` intact instead of
|
||||
// destroying the only copy of the evidence it exists to protect.
|
||||
function extractPresentResultsPreserveAndCleanupBash() {
|
||||
return extractBashFenceContaining(
|
||||
extractStepBody('present_results'),
|
||||
['DIAG_DIR', 'nullglob', 'rm -rf', '{run_dir}', '_PRESERVE_OK'],
|
||||
'present_results #3352/#3885 preserve+cleanup block',
|
||||
);
|
||||
}
|
||||
|
||||
/**
|
||||
* Runs the real extracted write_reviews gate+commit blocks and the real
|
||||
* present_results preserve+cleanup block, in the documented order, against
|
||||
* a fixture run dir. `opts.jsonlLines` seeds the aggregate JSONL (omit/empty
|
||||
* for "every lane failed"); `opts.lanes` seeds per-slug `gsd-review-<slug>.md`
|
||||
* / `.md`'s sibling `.err`. See the seam-choice comment above this describe
|
||||
* block for what is real bash vs. harness glue.
|
||||
*
|
||||
* `opts.blockDiagDirWithFile`: #3885 root-safe failure injection. Root
|
||||
* bypasses `chmod 0o000` entirely (a Docker/CI default), so permission bits
|
||||
* cannot induce a `mkdir -p`/`cp` failure deterministically. A filesystem
|
||||
* TYPE conflict is root-safe instead: pre-creating a plain FILE at the exact
|
||||
* path `mkdir -p` needs to create as a DIRECTORY makes `mkdir -p` fail with
|
||||
* "not a directory" for every caller, root included, because it is not a
|
||||
* permissions check at all.
|
||||
*/
|
||||
function runWriteReviewsFlow(t, opts) {
|
||||
const scriptDir = createTempDir('gsd-3352-script-');
|
||||
const runDir = createTempDir('gsd-3352-rundir-');
|
||||
const phaseDir = createTempDir('gsd-3352-phasedir-');
|
||||
t.after(() => {
|
||||
cleanup(scriptDir);
|
||||
cleanup(runDir);
|
||||
cleanup(phaseDir);
|
||||
});
|
||||
|
||||
if (opts.blockDiagDirWithFile) {
|
||||
fs.writeFileSync(path.join(phaseDir, '.review-diagnostics'), 'blocking file, not a directory\n');
|
||||
}
|
||||
|
||||
if (opts.jsonlLines && opts.jsonlLines.length > 0) {
|
||||
fs.writeFileSync(
|
||||
path.join(runDir, 'gsd-review-lane-results.jsonl'),
|
||||
opts.jsonlLines.map((l) => JSON.stringify(l)).join('\n') + '\n',
|
||||
);
|
||||
}
|
||||
for (const [slug, files] of Object.entries(opts.lanes || {})) {
|
||||
if (files.md !== undefined && files.md !== null) {
|
||||
fs.writeFileSync(path.join(runDir, `gsd-review-${slug}.md`), files.md);
|
||||
}
|
||||
if (files.err !== undefined && files.err !== null) {
|
||||
fs.writeFileSync(path.join(runDir, `gsd-review-${slug}.err`), files.err);
|
||||
}
|
||||
}
|
||||
|
||||
const gateBlock = extractWriteReviewsGateBash();
|
||||
const commitBlock = extractWriteReviewsCommitBash();
|
||||
const preserveAndCleanupBlock = extractPresentResultsPreserveAndCleanupBash();
|
||||
|
||||
const tracePath = path.join(scriptDir, 'commit-trace.log');
|
||||
const reviewsMdPath = path.join(phaseDir, '03-REVIEWS.md');
|
||||
|
||||
// #3885 Windows fix: splice POSIX-form paths into the bash source text, not
|
||||
// the OS-native ones `createTempDir()` returns. This is a fixture fix, not
|
||||
// a product one — the real workflow never hits this seam with a backslash
|
||||
// path in the first place: `{run_dir}` is created by `mktemp -d` running
|
||||
// INSIDE the bash block itself (always POSIX-style, even under Git Bash on
|
||||
// Windows), and `{phase_dir}` is the `phase_dir` field from
|
||||
// `gsd_run query init.review`, which gsd-core/bin/lib/init.cjs already
|
||||
// pipes through this exact same `toPosixPath()` before it is ever
|
||||
// serialized (see the `phase_dir: ... toPosixPath(...)` call sites there).
|
||||
// Splicing a native `C:\Users\...` string directly into unquoted bash
|
||||
// source (e.g. the pre-existing, unquoted
|
||||
// `--files {phase_dir}/{padded_phase}-REVIEWS.md` commit line) hits bash's
|
||||
// own unquoted-backslash removal and silently drops every separator — a
|
||||
// test-harness-only failure mode that following the fixture's own
|
||||
// production analogue eliminates.
|
||||
const runDirPosix = toPosixPath(runDir);
|
||||
const phaseDirPosix = toPosixPath(phaseDir);
|
||||
|
||||
const substitute = (block) => block
|
||||
.split('{run_dir}').join(runDirPosix)
|
||||
.split('{phase_dir}').join(phaseDirPosix)
|
||||
.split('{padded_phase}').join('03')
|
||||
.split('{N}').join('3');
|
||||
|
||||
const script = [
|
||||
'#!/usr/bin/env bash',
|
||||
'set -u',
|
||||
`SELECTED_REVIEWERS='${opts.selected}'`,
|
||||
`TRACE='${tracePath}'`,
|
||||
'gsd_run() { printf "%s\\n" "$*" >> "$TRACE"; }',
|
||||
substitute(gateBlock),
|
||||
'echo "GATE:TOTAL_LANE_FAILURE=$TOTAL_LANE_FAILURE"',
|
||||
'echo "GATE:ALL_LANES_SKIPPED=$ALL_LANES_SKIPPED"',
|
||||
'if [ "$TOTAL_LANE_FAILURE" = "false" ] && [ "$ALL_LANES_SKIPPED" = "false" ]; then',
|
||||
// Harness glue standing in for the agent's own Write tool call (see the
|
||||
// seam-choice comment above): the prose says "combine ... into
|
||||
// REVIEWS.md" only when the gate above did not fire.
|
||||
` touch '${reviewsMdPath}'`,
|
||||
substitute(commitBlock),
|
||||
'fi',
|
||||
substitute(preserveAndCleanupBlock),
|
||||
].join('\n');
|
||||
|
||||
fs.writeFileSync(path.join(scriptDir, 'flow.sh'), script, { mode: 0o755 });
|
||||
|
||||
// cwd is deliberately scriptDir, NOT runDir: every path this flow touches
|
||||
// ($RUN_DIR, $DIAG_DIR, {phase_dir}) is already absolute in the extracted
|
||||
// bash text, so the child's cwd is not load-bearing for anything the
|
||||
// blocks do — but on Windows a process cannot delete a directory that is
|
||||
// its OWN current working directory (unlike POSIX, where rm -rf on your
|
||||
// cwd succeeds). Setting cwd to runDir here was a harness artifact with no
|
||||
// production analogue (review.md never `cd`s into $RUN_DIR — see
|
||||
// gsd-core/workflows/review.md's use of $RUN_DIR, always by absolute
|
||||
// path), and it silently defeated `rm -rf "$RUN_DIR"` under Git-Bash on
|
||||
// windows-latest: the directory's *contents* were removed but the
|
||||
// still-open-as-cwd directory entry itself survived, so
|
||||
// `fs.existsSync(runDir)` kept reporting true. This was the actual defect
|
||||
// behind the two Windows-only failures — not a path-separator mismatch.
|
||||
const result = runHook(path.join(scriptDir, 'flow.sh'), [], {
|
||||
interpreter: 'bash',
|
||||
cwd: scriptDir,
|
||||
timeoutMs: HOOK_FANOUT_TIMEOUT_MS,
|
||||
});
|
||||
|
||||
const stdout = result.stdout || '';
|
||||
const diagDir = path.join(phaseDir, '.review-diagnostics');
|
||||
const commitTrace = fs.existsSync(tracePath)
|
||||
? readFileNormalized(tracePath).split('\n').filter((l) => l.trim() !== '')
|
||||
: [];
|
||||
|
||||
return {
|
||||
outcome: result.outcome,
|
||||
exitCode: result.exitCode,
|
||||
stderr: result.stderr,
|
||||
totalLaneFailure: /GATE:TOTAL_LANE_FAILURE=(\S+)/.exec(stdout)?.[1],
|
||||
allLanesSkipped: /GATE:ALL_LANES_SKIPPED=(\S+)/.exec(stdout)?.[1],
|
||||
reviewsMdExists: fs.existsSync(reviewsMdPath),
|
||||
runDirExists: fs.existsSync(runDir),
|
||||
diagDir,
|
||||
diagDirExists: fs.existsSync(diagDir),
|
||||
commitTrace,
|
||||
runDir,
|
||||
phaseDir,
|
||||
};
|
||||
}
|
||||
|
||||
const BUDGET_SKIP_MD = (slug) => `${slug} review skipped: prompt budget (500 tokens) too small for the minimum review set.`;
|
||||
const LANE_FAILURE_MD = (slug) => `${slug} review failed: exit 1`;
|
||||
|
||||
describe('#3352 every lane failed writes no REVIEWS.md', () => {
|
||||
test('totalLaneFailureWritesNoReviewsMd_3352', (t) => {
|
||||
const result = runWriteReviewsFlow(t, {
|
||||
selected: 'codex,gemini,claude',
|
||||
jsonlLines: [],
|
||||
lanes: {
|
||||
codex: { md: LANE_FAILURE_MD('codex'), err: 'stack trace: codex crashed\n' },
|
||||
gemini: { md: LANE_FAILURE_MD('gemini'), err: 'stack trace: gemini crashed\n' },
|
||||
claude: { md: LANE_FAILURE_MD('claude'), err: 'stack trace: claude crashed\n' },
|
||||
},
|
||||
});
|
||||
assert.equal(result.outcome, 'exited');
|
||||
assert.equal(result.totalLaneFailure, 'true', `expected TOTAL_LANE_FAILURE=true; stderr=${result.stderr}`);
|
||||
assert.equal(result.allLanesSkipped, 'false');
|
||||
assert.equal(result.reviewsMdExists, false, 'no REVIEWS.md may be written when every lane failed');
|
||||
assert.deepEqual(result.commitTrace, [], 'the commit must not run when every lane failed');
|
||||
});
|
||||
});
|
||||
|
||||
describe('#3352 one successful lane still writes REVIEWS.md (R1, must stay green)', () => {
|
||||
test('oneSuccessfulLaneStillWritesReviews', (t) => {
|
||||
const result = runWriteReviewsFlow(t, {
|
||||
selected: 'codex,gemini,claude',
|
||||
jsonlLines: [{ slug: 'codex' }],
|
||||
lanes: {
|
||||
codex: { md: '# Codex review\nlooks good\n' },
|
||||
gemini: { md: LANE_FAILURE_MD('gemini'), err: 'stack trace: gemini crashed\n' },
|
||||
claude: { md: LANE_FAILURE_MD('claude'), err: 'stack trace: claude crashed\n' },
|
||||
},
|
||||
});
|
||||
assert.equal(result.totalLaneFailure, 'false');
|
||||
assert.equal(result.allLanesSkipped, 'false');
|
||||
assert.equal(result.reviewsMdExists, true, 'at least one lane succeeded — REVIEWS.md must still be written');
|
||||
assert.equal(result.commitTrace.length, 1, 'the commit must run exactly once');
|
||||
});
|
||||
});
|
||||
|
||||
describe('#3352 a budget-skipped lane is not a failure (N5)', () => {
|
||||
test('budgetSkippedLanesAreNotFailures', (t) => {
|
||||
const result = runWriteReviewsFlow(t, {
|
||||
selected: 'codex,gemini,claude',
|
||||
jsonlLines: [],
|
||||
lanes: {
|
||||
codex: { md: BUDGET_SKIP_MD('codex') },
|
||||
gemini: { md: BUDGET_SKIP_MD('gemini') },
|
||||
claude: { md: BUDGET_SKIP_MD('claude') },
|
||||
},
|
||||
});
|
||||
assert.equal(result.allLanesSkipped, 'true', `every lane was budget-skipped, not failed; stderr=${result.stderr}`);
|
||||
assert.equal(result.totalLaneFailure, 'false', 'a budget skip must never be classified as a failure');
|
||||
assert.equal(result.reviewsMdExists, false, 'nothing to review — REVIEWS.md still must not be written');
|
||||
assert.deepEqual(result.commitTrace, []);
|
||||
});
|
||||
});
|
||||
|
||||
describe('#3352 successful-lane count boundary (0/1/2)', () => {
|
||||
test('successfulLaneCountBoundary', (t) => {
|
||||
const zero = runWriteReviewsFlow(t, {
|
||||
selected: 'codex,gemini',
|
||||
jsonlLines: [],
|
||||
lanes: {
|
||||
codex: { md: LANE_FAILURE_MD('codex'), err: 'boom\n' },
|
||||
gemini: { md: LANE_FAILURE_MD('gemini'), err: 'boom\n' },
|
||||
},
|
||||
});
|
||||
assert.equal(zero.totalLaneFailure, 'true');
|
||||
assert.equal(zero.reviewsMdExists, false, '0 successful lanes: no REVIEWS.md');
|
||||
|
||||
const one = runWriteReviewsFlow(t, {
|
||||
selected: 'codex,gemini',
|
||||
jsonlLines: [{ slug: 'codex' }],
|
||||
lanes: {
|
||||
codex: { md: '# Codex\nok\n' },
|
||||
gemini: { md: LANE_FAILURE_MD('gemini'), err: 'boom\n' },
|
||||
},
|
||||
});
|
||||
assert.equal(one.totalLaneFailure, 'false');
|
||||
assert.equal(one.reviewsMdExists, true, '1 successful lane: REVIEWS.md is written');
|
||||
|
||||
const two = runWriteReviewsFlow(t, {
|
||||
selected: 'codex,gemini',
|
||||
jsonlLines: [{ slug: 'codex' }, { slug: 'gemini' }],
|
||||
lanes: {
|
||||
codex: { md: '# Codex\nok\n' },
|
||||
gemini: { md: '# Gemini\nok\n' },
|
||||
},
|
||||
});
|
||||
assert.equal(two.totalLaneFailure, 'false');
|
||||
assert.equal(two.reviewsMdExists, true, '2 successful lanes: REVIEWS.md is written');
|
||||
});
|
||||
});
|
||||
|
||||
describe('#3352 per-lane evidence survives cleanup (R3)', () => {
|
||||
test('laneEvidenceSurvivesCleanup_3352', (t) => {
|
||||
const result = runWriteReviewsFlow(t, {
|
||||
selected: 'codex,gemini,claude',
|
||||
jsonlLines: [],
|
||||
lanes: {
|
||||
codex: { md: LANE_FAILURE_MD('codex'), err: 'stack trace: codex crashed\n' },
|
||||
gemini: { md: '', err: '' }, // ran, wrote nothing, no error either — nothing to preserve for this slug's .err
|
||||
claude: { md: LANE_FAILURE_MD('claude'), err: 'stack trace: claude crashed\n' },
|
||||
},
|
||||
});
|
||||
|
||||
assert.equal(result.runDirExists, false, 'the run dir must still be destroyed');
|
||||
assert.equal(result.diagDirExists, true, 'diagnostics must have been preserved somewhere under phase_dir');
|
||||
|
||||
const codexMd = path.join(result.diagDir, 'gsd-review-codex.md');
|
||||
const codexErr = path.join(result.diagDir, 'gsd-review-codex.err');
|
||||
const claudeErr = path.join(result.diagDir, 'gsd-review-claude.err');
|
||||
assert.ok(fs.existsSync(codexMd), 'codex .md evidence must be preserved');
|
||||
assert.equal(fs.readFileSync(codexMd, 'utf-8'), LANE_FAILURE_MD('codex'));
|
||||
assert.ok(fs.existsSync(codexErr), 'codex non-empty .err evidence must be preserved');
|
||||
assert.ok(fs.existsSync(claudeErr), 'claude non-empty .err evidence must be preserved');
|
||||
|
||||
const geminiErr = path.join(result.diagDir, 'gsd-review-gemini.err');
|
||||
assert.equal(fs.existsSync(geminiErr), false, 'an empty .err must not be copied as if it were real evidence');
|
||||
});
|
||||
});
|
||||
|
||||
describe('#3885 nothing to preserve still cleans up (must stay green)', () => {
|
||||
test('nothingToPreserveStillCleansUp_3885', (t) => {
|
||||
// No .md/.err fixtures written at all: `_DIAG_MD`/`_DIAG_ERR` are both
|
||||
// empty, so this is the "nothing to preserve" branch, not a failure —
|
||||
// `_PRESERVE_OK` starts (and stays) `true`, so cleanup proceeds exactly
|
||||
// as if preservation had succeeded.
|
||||
const result = runWriteReviewsFlow(t, {
|
||||
selected: 'codex,gemini,claude',
|
||||
jsonlLines: [],
|
||||
lanes: {},
|
||||
});
|
||||
|
||||
assert.equal(result.runDirExists, false, 'nothing to preserve is not a failure — run dir must still be removed');
|
||||
assert.equal(result.diagDirExists, false, 'no diagnostics directory should be created when there is nothing to copy');
|
||||
});
|
||||
});
|
||||
|
||||
describe('#3885 failed preservation leaves run_dir intact (no silent swallow)', () => {
|
||||
test('failedPreservationLeavesRunDirIntact_3885', (t) => {
|
||||
// Root-safe failure injection: pre-create a plain FILE at the exact path
|
||||
// `mkdir -p "$DIAG_DIR"` needs to create as a directory. This is a
|
||||
// filesystem TYPE conflict, not a permission check, so it fails `mkdir -p`
|
||||
// even when the test runs as root in Docker/CI (where `chmod 0o000`
|
||||
// would be silently bypassed and this test would pass with zero real
|
||||
// coverage — see #3885's brief).
|
||||
const result = runWriteReviewsFlow(t, {
|
||||
selected: 'codex,gemini,claude',
|
||||
jsonlLines: [],
|
||||
blockDiagDirWithFile: true,
|
||||
lanes: {
|
||||
codex: { md: LANE_FAILURE_MD('codex'), err: 'stack trace: codex crashed\n' },
|
||||
gemini: { md: LANE_FAILURE_MD('gemini'), err: 'stack trace: gemini crashed\n' },
|
||||
claude: { md: LANE_FAILURE_MD('claude'), err: 'stack trace: claude crashed\n' },
|
||||
},
|
||||
});
|
||||
|
||||
assert.equal(
|
||||
result.runDirExists,
|
||||
true,
|
||||
'a failed mkdir -p on DIAG_DIR must skip rm -rf and leave run_dir intact — this is the #3885 regression guard',
|
||||
);
|
||||
// The warning is emitted by the substituted bash script, which names
|
||||
// $RUN_DIR in its POSIX-spliced form (see the #3885 comment in
|
||||
// runWriteReviewsFlow) — compare against that same form rather than the
|
||||
// OS-native `result.runDir`.
|
||||
assert.ok(
|
||||
result.stderr.includes(toPosixPath(result.runDir)),
|
||||
`the failure warning must name the intact run_dir holding the un-preserved evidence; got stderr: ${result.stderr}`,
|
||||
);
|
||||
// The original per-lane evidence is still readable at its original
|
||||
// location, uncorrupted, because it was never moved or destroyed.
|
||||
assert.equal(
|
||||
fs.readFileSync(path.join(result.runDir, 'gsd-review-codex.md'), 'utf-8'),
|
||||
LANE_FAILURE_MD('codex'),
|
||||
);
|
||||
});
|
||||
});
|
||||
|
||||
describe('#3352 preserved evidence is never swept into the commit (N6)', () => {
|
||||
test('preservedEvidenceIsNotSweptIntoTheCommit', (t) => {
|
||||
const result = runWriteReviewsFlow(t, {
|
||||
selected: 'codex',
|
||||
jsonlLines: [{ slug: 'codex' }],
|
||||
lanes: {
|
||||
codex: { md: '# Codex\nok\n', err: '' },
|
||||
},
|
||||
});
|
||||
assert.equal(result.commitTrace.length, 1, 'exactly one commit call must run');
|
||||
const commitArgs = result.commitTrace[0];
|
||||
// The commit fence splices `{phase_dir}` into unquoted bash source as a
|
||||
// POSIX-form path (see the #3885 comment in runWriteReviewsFlow) — build
|
||||
// the expected string the same way rather than via `path.join`, which on
|
||||
// win32 would re-insert native backslashes the substituted script never
|
||||
// produces.
|
||||
assert.ok(
|
||||
commitArgs.includes(`${toPosixPath(result.phaseDir)}/03-REVIEWS.md`),
|
||||
`commit must name the single REVIEWS.md file; got: ${commitArgs}`,
|
||||
);
|
||||
assert.ok(
|
||||
!commitArgs.includes('.review-diagnostics'),
|
||||
`commit must never name the diagnostics directory; got: ${commitArgs}`,
|
||||
);
|
||||
// The commit step's --files value is a single path, never a glob: a
|
||||
// directory glob would expand to MULTIPLE argv tokens by the time
|
||||
// `gsd_run` sees them, so more than one path after "--files" is itself
|
||||
// the defect this row guards against.
|
||||
const filesIdx = commitArgs.indexOf('--files ');
|
||||
const afterFiles = commitArgs.slice(filesIdx + '--files '.length).trim();
|
||||
assert.equal(afterFiles.split(/\s+/).length, 1, `--files must carry exactly one path; got: ${afterFiles}`);
|
||||
});
|
||||
});
|
||||
|
||||
@@ -4146,3 +4146,166 @@ describe('bug #3641: bracket-convention windows are visible to validate (V005/V0
|
||||
'bracket convention is a superset — the legacy label stays recognized');
|
||||
});
|
||||
});
|
||||
|
||||
// ─────────────────────────────────────────────────────────────────────────────
|
||||
// #3885 (ADR-3473 §8.5) / item 5 — `countPhasePlansAndSummaries` swallows an
|
||||
// unreadable phase directory into "absent", indistinguishable from a phase
|
||||
// that genuinely has no CONTEXT.md.
|
||||
//
|
||||
// Mechanism (src/roadmap.cts, countPhasePlansAndSummaries):
|
||||
// try { phaseFiles = fs.readdirSync(phaseDir); } catch { /* empty */ }
|
||||
// An EACCES/EIO collapses `phaseFiles` to `[]`, which makes
|
||||
// `findContextMdIn(scopedFiles)` return null — identical to a phase dir that
|
||||
// was successfully read and genuinely has no CONTEXT.md. `cmdRoadmapAnalyze`
|
||||
// (the only exported consumer) surfaces this as `has_context: false` on the
|
||||
// phase's entry in `phases[]`, with nothing distinguishing "could not read"
|
||||
// from "nothing there".
|
||||
//
|
||||
// DESIGN DECISION (chosen by this test file, not yet implemented): each
|
||||
// `AnalyzePhase` gains a `context_read_error: string | null` field — null on
|
||||
// success (including a genuinely missing/ENOENT directory), and a message
|
||||
// string naming the phase directory when the readdirSync call fails with any
|
||||
// non-ENOENT error (EACCES, EIO, ...). Mirrors the SCOPE.UNREADABLE
|
||||
// discriminator `src/core-utils.cts`'s `getPhaseFileStats` already uses to
|
||||
// keep "unreadable" separate from "absent" on the sibling phase-stats path.
|
||||
//
|
||||
// `countPhasePlansAndSummaries` itself is not exported from roadmap.cjs, so
|
||||
// these tests drive the ONLY exported consumer, `cmdRoadmapAnalyze`, in
|
||||
// process — injecting the fs failure by monkeypatching `fs.readdirSync`
|
||||
// (restored in `finally`) and capturing `output()`'s raw fd-1 write by
|
||||
// monkeypatching `fs.writeSync` (io.cjs writes via `fs.writeSync(1, ...)`,
|
||||
// bypassing console.log, so `captureConsole()` cannot see it). NEVER
|
||||
// `chmod 0o000` — root bypasses mode bits, so that trick passes with zero
|
||||
// coverage in root Docker/CI.
|
||||
describe('#3885 (ADR-3473 §8.5): countPhasePlansAndSummaries distinguishes unreadable from absent (roadmap.cts caller)', () => {
|
||||
let tmpDir;
|
||||
let roadmapLib;
|
||||
|
||||
beforeEach(() => {
|
||||
tmpDir = createTempProject();
|
||||
roadmapLib = require('../gsd-core/bin/lib/roadmap.cjs');
|
||||
fs.writeFileSync(
|
||||
path.join(tmpDir, '.planning', 'ROADMAP.md'),
|
||||
'# Roadmap\n\n### Phase 3: API\n**Goal:** Build API\n',
|
||||
);
|
||||
});
|
||||
|
||||
afterEach(() => {
|
||||
cleanup(tmpDir);
|
||||
});
|
||||
|
||||
// Runs `roadmapLib.cmdRoadmapAnalyze(cwd, false)` while capturing the raw
|
||||
// fd-1 bytes `output()` writes via `fs.writeSync`, and returns the parsed
|
||||
// JSON result. Restores `fs.writeSync` in `finally` even if analyze throws.
|
||||
function runAnalyzeCapturingStdout(cwd) {
|
||||
const chunks = [];
|
||||
const origWriteSync = fs.writeSync;
|
||||
fs.writeSync = function patchedWriteSync(fd, buffer, offset, length) {
|
||||
if (fd !== 1) return origWriteSync.apply(fs, arguments);
|
||||
const buf = Buffer.isBuffer(buffer) ? buffer : Buffer.from(buffer);
|
||||
const start = offset ?? 0;
|
||||
const len = length ?? (buf.length - start);
|
||||
chunks.push(Buffer.from(buf.subarray(start, start + len)));
|
||||
return len;
|
||||
};
|
||||
try {
|
||||
roadmapLib.cmdRoadmapAnalyze(cwd, false);
|
||||
} finally {
|
||||
fs.writeSync = origWriteSync;
|
||||
}
|
||||
return JSON.parse(Buffer.concat(chunks).toString('utf8'));
|
||||
}
|
||||
|
||||
// Monkeypatches `fs.readdirSync` so a call whose FIRST argument resolves to
|
||||
// `targetPath` throws an error shaped like `code`; every other path is
|
||||
// delegated to the real implementation. Returns a restorer — callers MUST
|
||||
// invoke it in `finally`.
|
||||
function injectReaddirFailure(targetPath, code) {
|
||||
const resolved = path.resolve(targetPath);
|
||||
const origReaddirSync = fs.readdirSync;
|
||||
fs.readdirSync = function patchedReaddirSync(p, ...rest) {
|
||||
if (path.resolve(String(p)) === resolved) {
|
||||
const err = new Error(`${code}: simulated failure, scandir '${p}'`);
|
||||
err.code = code;
|
||||
throw err;
|
||||
}
|
||||
return origReaddirSync.call(fs, p, ...rest);
|
||||
};
|
||||
return () => { fs.readdirSync = origReaddirSync; };
|
||||
}
|
||||
|
||||
function findPhase3(analyzeOutput) {
|
||||
const phase = analyzeOutput.phases.find((p) => p.number === '3');
|
||||
assert.ok(phase, `phase 3 must appear in analyze output; got: ${JSON.stringify(analyzeOutput.phases)}`);
|
||||
return phase;
|
||||
}
|
||||
|
||||
// T61 — MUST STAY GREEN: a readable phase directory with no CONTEXT.md
|
||||
// reports has_context:false, and (using `?? null` so this passes both
|
||||
// before and after the fix) no read-error signal.
|
||||
test('T61: readableDirWithoutContextReportsFalse', () => {
|
||||
const phaseDir = path.join(tmpDir, '.planning', 'phases', '03-api');
|
||||
fs.mkdirSync(phaseDir, { recursive: true });
|
||||
fs.writeFileSync(path.join(phaseDir, '03-01-PLAN.md'), '---\nwave: 1\n---\n## Task 1\n');
|
||||
|
||||
const output = runAnalyzeCapturingStdout(tmpDir);
|
||||
const phase = findPhase3(output);
|
||||
assert.strictEqual(phase.has_context, false, 'no CONTEXT.md on disk — has_context must be false');
|
||||
assert.strictEqual(phase.context_read_error ?? null, null, 'a readable, genuinely context-less dir must report no read error');
|
||||
});
|
||||
|
||||
// T62 — RED today: measured on this tree, an EACCES on the phase
|
||||
// directory's readdirSync collapses to has_context:false /
|
||||
// disk_status:"empty" with nothing distinguishing it from a phase that was
|
||||
// successfully read and genuinely has no CONTEXT.md. Required: the failure
|
||||
// must be reported, naming the phase directory.
|
||||
test('T62: unreadablePhaseDirIsNotReportedAsAbsent', () => {
|
||||
const phaseDir = path.join(tmpDir, '.planning', 'phases', '03-api');
|
||||
fs.mkdirSync(phaseDir, { recursive: true });
|
||||
fs.writeFileSync(path.join(phaseDir, '03-01-PLAN.md'), '---\nwave: 1\n---\n## Task 1\n');
|
||||
|
||||
const restore = injectReaddirFailure(phaseDir, 'EACCES');
|
||||
let output;
|
||||
try {
|
||||
output = runAnalyzeCapturingStdout(tmpDir);
|
||||
} finally {
|
||||
restore();
|
||||
}
|
||||
const phase = findPhase3(output);
|
||||
assert.strictEqual(
|
||||
typeof (phase.context_read_error ?? null),
|
||||
'string',
|
||||
`an unreadable phase directory must be reported as an error, not silently absent; got context_read_error=${JSON.stringify(phase.context_read_error)}`,
|
||||
);
|
||||
assert.ok(
|
||||
(phase.context_read_error || '').includes('03-api'),
|
||||
`the reported error must name the discarded input (the phase directory); got: ${phase.context_read_error}`,
|
||||
);
|
||||
});
|
||||
|
||||
// T64 — MUST STAY GREEN: a genuinely missing directory (ENOENT) is absent,
|
||||
// not an error — this is the row that keeps T62's fix from over-firing on
|
||||
// every ordinary "no directory yet" phase. Injected the same way as T62/T63
|
||||
// (readdirSync throws ENOENT for the exact phase-dir path) so the assertion
|
||||
// exercises the discriminator itself, not merely "no error was ever
|
||||
// thrown".
|
||||
test('T64: missingDirIsGenuinelyAbsent', () => {
|
||||
const phaseDir = path.join(tmpDir, '.planning', 'phases', '03-api');
|
||||
fs.mkdirSync(phaseDir, { recursive: true });
|
||||
fs.writeFileSync(path.join(phaseDir, '03-01-PLAN.md'), '---\nwave: 1\n---\n## Task 1\n');
|
||||
|
||||
const restore = injectReaddirFailure(phaseDir, 'ENOENT');
|
||||
let output;
|
||||
try {
|
||||
output = runAnalyzeCapturingStdout(tmpDir);
|
||||
} finally {
|
||||
restore();
|
||||
}
|
||||
const phase = findPhase3(output);
|
||||
assert.strictEqual(
|
||||
phase.context_read_error ?? null,
|
||||
null,
|
||||
`ENOENT must be treated as genuinely absent, not reported as an error; got: ${phase.context_read_error}`,
|
||||
);
|
||||
});
|
||||
});
|
||||
|
||||
Reference in New Issue
Block a user