From 0e6fa2e2cfacfd472e6c31596903cf3ef6e03250 Mon Sep 17 00:00:00 2001 From: Tom Boucher Date: Thu, 6 Aug 2026 23:57:05 -0400 Subject: [PATCH] =?UTF-8?q?enhance(#3118):=20close=20the=20dead=20injectab?= =?UTF-8?q?les=20and=20the=20shell=20projection=20follow-on=20=E2=80=94=20?= =?UTF-8?q?Wave=204=20(#3124)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * test(#3118): failing-first coverage for the dead injectables and the shell projection Adds the counter-tests Wave 4 closes against, before any fix: - antigravityWatermark had zero test references. The four existing tests that look like watermark coverage hand the fallback a literal mark and never call the producer, so nothing pinned whether a real run's mark is correct. Covers all six branches plus the non-object cache classes. - Pins the fail-open: a transcript read that throws reports lines:0, indistinguishable from a genuinely empty transcript, and the consumer then replays a previous run's review as this run's. - Pins the export-line escaping across the repair, persist and win32 bash lanes, including the parity assertion that they must not diverge. - sliceCurrentPositionSection: empty-vs-absent, fenced heading, second occurrence, H3, CRLF. - Proves deps.progressProvider is inert by supplying a throwing stub to all ten transition intents. Verification through the remote runner only. Refs #3118 * fix(#3118): distinguish an unreadable transcript from an empty one antigravityWatermark's final read can throw on a transcript that indisputably exists. It returned lines:0, which is the same value a genuinely empty transcript produces, so the caller could not tell the two apart. antigravityTranscriptFallback derives its skip from that count. A mark of {convId:'c1', lines:0} for a conversation that pre-dates the run makes it skip nothing and return the last PLANNER_RESPONSE in a transcript written before this run started — a previous review presented as this one's, which is exactly what the function's own 'never stale' docstring promises cannot happen. The unreadable case now sets unreadable:true and the fallback declines for a same-conv-id unreadable mark. An absent or empty transcript is untouched: those genuinely have zero prior lines. * fix(#3118): escape the export line for the file it lands in, not the echo Three lanes emit export PATH=":$PATH". repair escaped it with escapePosixDoubleQuoted; persist and the win32 Git Bash lane escaped it with escapeSingleQuotedShellLiteral instead. The single-quoting is correct for the echo, so nothing runs when the user pastes the command. But the bytes appended to ~/.bashrc are the export line itself, and inside double quotes in an rc file a $(...) or a backtick in the directory name is command substitution that runs on every new shell. Those characters are legal in a path on both POSIX and Windows, so the path was reachable. projectPathExportLine is now the single source of that line and escapes for its final rc-file context; each lane still applies its own transport escaping on top. fish keeps the single-quote escaper — its value really does stay single-quoted. The cmd.exe lane interpolated into a cmd double-quoted string with no cmd-level escaping, so a quote closed the region and &cmd& ran. A quote is reserved on Windows and cannot appear in a real path, so there is no correct command to suggest: the win32 lanes now fail closed for one. Metacharacter-free paths render byte-identically on every lane. * fix(#3118): drop a stray carriage return and a deps field nobody reads locateCurrentPosition subtracted a fixed one byte to exclude the newline before the next heading, which assumes LF. On a CRLF document the slice kept an unpaired trailing carriage return. It now walks back over the newline and over a preceding carriage return if there is one. StateTransitionDeps also required a progressProvider that 33 sites supplied and no site ever called. A required field nothing reads widens the module's interface without changing its implementation, which is the shape epic #3051 cites as its reason for refusing blanket injection. Removed along with the ProgressRecord alias that existed only as its return type; state-document.cts's unrelated interface of the same name is untouched. * fix(#3118): stop an empty span duplicating bytes, and name the empty results Three findings from the isolated review pass. locateCurrentPosition could return end < start when the section was empty and the next heading followed with no blank line between. Every mutator splices with slice(0,start) + body + slice(end), so an inverted span duplicated the region between them — a blank line silently inserted into STATE.md on every transition, two bytes on CRLF. The span is now clamped, and an empty section is a zero-length span, which is what it always meant. The win32 fail-closed path left the installer printing 'Add it with one of:' with nothing under it. An empty shellActions folded two different facts together, so projectPathActionProjection now carries a frozen PATH_ACTION_REASON and the installer branches on it. Two empty results with different causes staying distinguishable is the subject of the epic this belongs to. fish_add_path parses a leading dash as an option, so a directory named -v printed 'No paths to add' instead of being added. Verified against fish 4.8.1: the end-of-options separator fixes it. Replaces the console-prose test the second fix first arrived with — a regex over captured stdout is what CONTRIBUTING prohibits, and the typed reason is the surface it asks for instead. * fix(#3118): escape TOML control characters, and stop a test name overstating Five findings from the two review axes. escapeTomlDoubleQuotedString escaped only backslash and quote. TOML basic strings also require U+0000-U+0008, U+000A-U+001F and U+007F to be escaped, so a value carrying a raw newline or NUL wrote a config.toml no parser accepts — rejecting the whole file, not just that value. Four of its call sites write real config. Tab stays raw; the grammar exempts it. The byte-identity test claimed every lane was unchanged for an ordinary path, which is false: fish now takes the end-of-options separator on every path, not only hostile ones. Renamed, and the one intended delta now has its own named test instead of hiding inside a claim that read as broader than it was. Also: exact-equality assertions in place of substring checks that could pass on a subtly wrong escape, newline and null-byte cases for all five quoting primitives, and a temp dir registered with t.after so it is removed when an assertion fails. * docs(#3118): add the changeset fragments * fix(#3118): degrade instead of throwing on a null conversation cache A cache file whose whole content is the literal null — what a truncated or zeroed write leaves behind — made both antigravityWatermark and antigravityTranscriptFallback throw. JSON.parse('null') succeeds, so the try/catch wrapping the parse never fired, and resolveConvId then called hasOwnProperty on null. Both functions advertise the opposite; the existing test next to them is named 'a missing cache or transcript degrades to empty, never throws'. Parsing successfully is not the same fact as the payload being usable, and a guard that only wraps the parse cannot tell them apart. resolveConvId is now total for any non-object input, so one guard covers both callers. Caught by the null case in this wave's own cache matrix. * test(#3118): correct a stale fish expectation and a parity comparison The pre-existing 'POSIX persist mode escapes single quotes' test pinned fish_add_path without the end-of-options separator this wave adds, so it asserted behavior that is no longer correct. A repo-wide scan found one such hardcoded expectation; every other site derives its expectation from the projection. The new parity test compared the token from a POSIX path against the win32 lane, which posix-normalizes its input first — two different inputs, so the tokens differed for a reason that had nothing to do with the parity it claims to check. It now derives the win32 expectation from the same input the lane receives. * docs(#3118): reword a comment the injection scanner reads as an instruction The scanner pattern act\s+as\s+(?:a|an|the)\s+ carries no word boundary, so 'the same fact as the payload' matched on the tail of 'fact'. Reworded per the documented remedy for this collision. The missing boundary is a scanner defect rather than a prose problem — any contributor writing 'fact as the' trips it — but the pattern is gate plumbing, which the sibling epic owns, so it is surfaced rather than changed here. * chore(#3118): backfill changeset pr number to 3124 * chore(#3118): backfill changeset pr number to 3124 * fix(#2784): make the negation scan single-pass and index it correctly Three defects in the negation suppression added by #3127, all in one block, none of which had a test. The pair scan was verbs.some(nouns.some(...)) with a slice and a split per pair, so it grew cubically with clause length: 1.1ms before that PR and 8462ms after, on 800 verb+noun pairs in one clause. api-coverage's property test generates documents large enough to reach the runner's 600s file cap, which is why it hangs as 'fail 0, cancelled 1' rather than failing an assertion. Every (verb, noun) window is a subset of the single widest one, so one scan of that window answers the same question in a linear pass. Verified equivalent against the old predicate over 20,000 generated clauses. Both checks also subtracted clause.start from offsets that collectTerm- Matches already returns clause-local. The first clause on a line has start 0 so it worked there and nowhere else: later clauses went negative, and slice reads a negative index from the end, so suppression silently examined unrelated text. The comment claimed 'without any API integration' was suppressed. It is not — the qualifier sits outside the two-word lookback and the noun precedes the verb. Widening the window would trade a false positive that costs one declaration line for a false negative that slips a real integration past a blocking gate, so the behavior stands and the comment now says so. Pinned by a test. The qualifier sets were also rebuilt for every line of every document. --- .changeset/bold-birds-wave.md | 5 + .changeset/daring-tigers-squeak.md | 5 + .changeset/silly-eagles-swim.md | 5 + CONTEXT.md | 4 +- bin/install.js | 21 +- docs/how-to/install-on-your-runtime.md | 2 +- pwned_cmdsub | 0 src/api-coverage.cts | 121 ++++- src/milestone.cts | 2 +- src/phase.cts | 1 - src/review-lane-runner.cts | 32 +- src/shell-command-projection.cts | 95 +++- src/state-transition.cts | 24 +- src/state.cts | 14 +- tests/api-coverage.test.cjs | 93 ++++ tests/fix-2136-clock-local-today.test.cjs | 2 +- tests/review-lane-runner.test.cjs | 136 ++++++ ...shell-command-projection-dispatch.test.cjs | 403 ++++++++++++++++- tests/state-rebuild.test.cjs | 2 - tests/state-transition.test.cjs | 415 +++++++++++++++++- 20 files changed, 1296 insertions(+), 86 deletions(-) create mode 100644 .changeset/bold-birds-wave.md create mode 100644 .changeset/daring-tigers-squeak.md create mode 100644 .changeset/silly-eagles-swim.md create mode 100644 pwned_cmdsub diff --git a/.changeset/bold-birds-wave.md b/.changeset/bold-birds-wave.md new file mode 100644 index 000000000..56ee4f5f8 --- /dev/null +++ b/.changeset/bold-birds-wave.md @@ -0,0 +1,5 @@ +--- +type: Fixed +pr: 3124 +--- +The api-coverage detector's negation-suppression check no longer takes superlinear time on long prose, which was hanging the verification gate (#2784, #3127). It also no longer fails to suppress a negated pair ("this phase integrates no external API") when the negation sits in any clause other than the first on a line — a latent offset bug made negation suppression a no-op for every clause after the first. diff --git a/.changeset/daring-tigers-squeak.md b/.changeset/daring-tigers-squeak.md new file mode 100644 index 000000000..313afc89e --- /dev/null +++ b/.changeset/daring-tigers-squeak.md @@ -0,0 +1,5 @@ +--- +type: Fixed +pr: 3124 +--- +**`fish_add_path` no longer skips a directory whose name starts with a dash** — fish parses a leading-dash token as an option, so the suggested command silently added nothing; it now passes the end-of-options separator. Also fixes a `config.toml` written unparseable when a value carried a newline or NUL, an installer PATH hint that printed a header with nothing under it, and a reviewer lane that crashed instead of degrading when its conversation cache file held the literal `null`. (#3118) diff --git a/.changeset/silly-eagles-swim.md b/.changeset/silly-eagles-swim.md new file mode 100644 index 000000000..5309864ab --- /dev/null +++ b/.changeset/silly-eagles-swim.md @@ -0,0 +1,5 @@ +--- +type: Security +pr: 3124 +--- +**A directory name containing `$(…)` or a backtick no longer becomes a live command in your shell startup file** — the PATH-persistence suggestion escaped its `export PATH="…"` line for the `echo` that carries it, not for the rc file it lands in, so a substitution in the target directory survived into `~/.bashrc` and ran on every new shell. (#3118) diff --git a/CONTEXT.md b/CONTEXT.md index 81629d0cb..1b48ff39f 100644 --- a/CONTEXT.md +++ b/CONTEXT.md @@ -121,7 +121,7 @@ Module owning the single declared contract for a **reviewer lane** — one exter Module owning the projection from a **declared reviewer lane** plus resolved configuration to a concrete **invocation plan** — the value a lane is actually run from (ADR-2782 Phase 5b, #2799). Pure: no filesystem, network, subprocess or clock; configuration arrives through a `configGet` seam. Interface: `resolveLanePlan({lane, configGet, runDir, repoRoot, effortArgs}) → {ok, plan} | {ok:false, reason, detail}`, `LANE_UNAVAILABLE` (frozen reason enum — a lane that will not run reports WHY, because the ambiguity between "failed" and "ran cleanly with nothing to report" is the defect class this epic closes), plus `isEmptyReview`, `normalizeHost` and `fileRefPrompt`. TOTAL — a malformed lane yields an unavailable result, never a throw, because third-party overlay manifests reach this seam. `invoke.args` is an **argv template** over a closed four-member placeholder vocabulary (`{{model}}`, `{{effort}}`, `{{output}}`, `{{prompt}}`), not a prefix: the injected pieces do not all go in the same place — `codex` injects the model after its `exec` subcommand and the output file later still, while five lanes end with a bare `-` that must stay last. Each lane's plan was derived FROM its former bash leg, and a golden table asserts all twelve; that table is the strangler-fig substitute for a parallel run. ### Reviewer Lane Runner -Module owning **execution** of an invocation plan (ADR-2782 Phase 5b, #2799): probe, spawn or HTTP call, empty-output policy, and dispatch of the three first-party `handler` modules D6 names (`antigravity`, `openai-compatible`, `opencode`). Replaces ~640 lines of hand-authored per-CLI bash in `invoke_reviewers`. Interface: `runLane`, `probeLane`, `checkEgressHost`, `writeReviewOrStub`, and the handler entry points (`handleOpencodeOutput`, `antigravityArgv`, `antigravityPrompt`, `antigravityWatermark`, `antigravityTranscriptFallback`, `antigravityDiagnostic`, `stampBlindReview`, `runOpenAiCompatible`). Every dependency is injected, so behaviour is testable without a network or a spawn. **Every subprocess call passes `timeout` + `killSignal` + `maxBuffer`** (`DEFECT.UNBOUNDED-SUBPROCESS`): a frozen synchronous spawn cannot be interrupted and hangs a whole CI chunk to its kill with `# fail 0`. Three runtime dependencies disappear here — `jq`, `curl`, and external `timeout`/`gtimeout` — which also closes two platform holes: five lanes were unavailable on stock Windows/Git-Bash for want of `jq`, and the Antigravity lane ran unbounded on stock macOS, which ships neither killer. Owns ADR-2782 D5 rules 2–4: the egress destination is **re-resolved at invocation** and a changed host blocks the lane rather than silently redirecting it; absence of a consent record allows, since first-party lanes are never consent-gated. +Module owning **execution** of an invocation plan (ADR-2782 Phase 5b, #2799): probe, spawn or HTTP call, empty-output policy, and dispatch of the three first-party `handler` modules D6 names (`antigravity`, `openai-compatible`, `opencode`). Replaces ~640 lines of hand-authored per-CLI bash in `invoke_reviewers`. Interface: `runLane`, `probeLane`, `checkEgressHost`, `writeReviewOrStub`, and the handler entry points (`handleOpencodeOutput`, `antigravityArgv`, `antigravityPrompt`, `antigravityWatermark`, `antigravityTranscriptFallback`, `antigravityDiagnostic`, `stampBlindReview`, `runOpenAiCompatible`). Every dependency is injected, so behaviour is testable without a network or a spawn. **Every subprocess call passes `timeout` + `killSignal` + `maxBuffer`** (`DEFECT.UNBOUNDED-SUBPROCESS`): a frozen synchronous spawn cannot be interrupted and hangs a whole CI chunk to its kill with `# fail 0`. Three runtime dependencies disappear here — `jq`, `curl`, and external `timeout`/`gtimeout` — which also closes two platform holes: five lanes were unavailable on stock Windows/Git-Bash for want of `jq`, and the Antigravity lane ran unbounded on stock macOS, which ships neither killer. Owns ADR-2782 D5 rules 2–4: the egress destination is **re-resolved at invocation** and a changed host blocks the lane rather than silently redirecting it; absence of a consent record allows, since first-party lanes are never consent-gated. `antigravityWatermark`'s final read can throw (permissions, mid-write truncation) on a transcript that indisputably exists, which is not the same fact as a genuinely empty or absent one; that case now sets `unreadable: true` on the returned mark rather than folding into `lines: 0`, and `antigravityTranscriptFallback` declines (`''`) for a same-conv-id unreadable mark instead of skipping zero lines and replaying a stale pre-run response (#3118). ### Resolution Provenance Cross-seam principle (ADR-1411, epic #1411): context resolution — config loading, project-root anchoring, workstream resolution — must report its provenance, not fall open silently to defaults. A resolver anchors deterministically to the project root (one walk-up module, no dependence on an arbitrary descendant cwd), returns *what* it resolved **and** *where it came from* (`source`/`degraded`), and surfaces a diagnostic when a *configured* input resolves empty (`not configured` and `configured-but-empty` are distinguishable). The resolution-side analog of ADR-227 (input-validation shape). Target seams: Config Loader Module (`loadConfig` → `ConfigResolution { config, source, degraded }`), Project-Root Resolution Module (single nearest-`.planning/` walk-up, retiring ad-hoc resolvers like `resolvePlanningCwd`), I/O Module (`Resolution { value, configured, reason, warnings }` output envelope). A configured input resolving empty without a reason is a CI-guarded regression. **P1 (nearest-.planning/ heuristic) shipped in #1413; P2 (loadConfigResolved + agent-skills diagnostic) shipped in #1415 / closes #1366**: `loadConfigResolved` now implements the Config Loader seam target; `cmdAgentSkills` uses `findProjectRoot` + `loadConfigResolved` and emits `configured`/`reason`/`source`/`degraded` in its `--json` IR. **Corrupt is not absent (ADR-1411 amendment 2026-07-26, epic #1879 Phase 0 / #2674):** the principle above governs a resolution *miss*; input that is present but *not usable* (a `SyntaxError`, an errno such as `EACCES`/`EIO`, or a malformed structure with no exception at all) is a distinct class that must stay distinguishable from genuine absence. The defect in that class is not the fallback — ADR-227 requires malformed input to be coerced rather than propagated, and this ADR already permits a fallback — it is that the fallback is **invisible**. So every current return value is preserved and the cause is made visible by one of two mechanisms: **in-band**, where the result already carries a provenance envelope, name the cause in it (`ConfigResolution` gains a `reason`; `Resolution`'s four documented values all describe a miss, so new unusable-input values are introduced with the first adopter) and also expose it on the surface callers actually use, since a `reason` no caller reads is an unreachable field; **out-of-band**, where the read returns a bare sentinel or a plausible default it cannot extend, keep that value and emit a deduplicated `stderr` diagnostic keyed on resolved-path + errno, reusing the `_warnedUnknownConfigKeys` guard pattern. The diagnostic is unconditional — a deliberate divergence from ADR-227's never-implemented `GSD_DEBUG` opt-in, since an opt-in nobody sets is the same silence. Throwing is **not** the cluster's answer — it stays confined to ADR-227's genuinely-fatal carve-out, decided per call, never inferred from the return shape. @@ -883,7 +883,7 @@ The prompt-level data/instruction isolation seam for untrusted web/document ingr ## Shell Command Projection Module (expanded glossary entry, 2026-05-13) -Module owning all OS-facing I/O for the tool: runtime-aware command-text rendering (hook commands, PATH action lines, shim scripts), subprocess dispatch (execGit, execNpm, execTool, probeTty, isSpawnTimeout), and platform file I/O (platformWriteSync, platformReadSync, platformEnsureDir). Single seam for platform-conditional logic — one place to fix any shell or file write regression across Windows, macOS, and Linux. `isSpawnTimeout` is the single shared "did this subprocess time out" predicate (error.code==='ETIMEDOUT' only — cross-platform-safe; does not require signal==='SIGTERM'), consumed by worktree-safety.cts, worktree-base-ref.cts, commands.cts, and this module's own dispatchGsdCommand (#3050 — "Generative Fix Divergence"). Lives in `gsd-core/bin/lib/shell-command-projection.cjs`. See ADR-0009 (superseded "does not execute" constraint) and ADR-0010 (superseded File Operation Engine). +Module owning all OS-facing I/O for the tool: runtime-aware command-text rendering (hook commands, PATH action lines, shim scripts), subprocess dispatch (execGit, execNpm, execTool, probeTty, isSpawnTimeout), and platform file I/O (platformWriteSync, platformReadSync, platformEnsureDir). Single seam for platform-conditional logic — one place to fix any shell or file write regression across Windows, macOS, and Linux. `isSpawnTimeout` is the single shared "did this subprocess time out" predicate (error.code==='ETIMEDOUT' only — cross-platform-safe; does not require signal==='SIGTERM'), consumed by worktree-safety.cts, worktree-base-ref.cts, commands.cts, and this module's own dispatchGsdCommand (#3050 — "Generative Fix Divergence"). `projectPathExportLine(targetDir)` is the single source of the `export PATH=":$PATH"` line for all three PATH-persistence lanes (repair, persist, win32 Git Bash) — it double-quote-escapes for the line's final rc-file context before any lane single-quotes it for its own `echo` transport, closing the #3118 command-substitution injection where a lane re-escaped the line itself and let a `$(…)` in the target dir execute on every new shell; the win32 cmd.exe lane fails closed (empty actions) whenever the target dir contains `"`, since that character is reserved on Windows and would otherwise close cmd's quoted region, and now tags that empty result with a typed `PATH_ACTION_REASON` (`win32_reserved_quote`) so it stays distinguishable from the unrelated "no target directory given" empty result (`no_target_dir`, #3118); the fish lane emits `fish_add_path -- ''` (`--` end-of-options separator, verified empirically against fish 4.8.1), since a leading-dash target dir is otherwise misparsed by fish's argparse-based option scanning regardless of quoting; `escapeTomlDoubleQuotedString` now escapes TOML's required control characters (U+0000-U+0008, U+000A-U+001F, U+007F), not just backslash and quote, closing a raw-newline/CR/NUL config.toml parse failure (#3118). Lives in `gsd-core/bin/lib/shell-command-projection.cjs`. See ADR-0009 (superseded "does not execute" constraint) and ADR-0010 (superseded File Operation Engine). Invariants: - Result shape: all exec* functions return `{ exitCode, stdout, stderr }`; never throw on non-zero exit code. diff --git a/bin/install.js b/bin/install.js index a57df30af..e1b794463 100755 --- a/bin/install.js +++ b/bin/install.js @@ -14,6 +14,7 @@ const { projectPathActionProjection, projectPortableHookBaseDir, projectPersistentPathExportActions, + PATH_ACTION_REASON, projectShellCommandText, projectCodexHookTomlCommand, shellHookOmitsBashRunner, @@ -13138,14 +13139,26 @@ function maybeSuggestPathExport(globalBin, homeDir) { console.log(''); console.log(` ${yellow}⚠${reset} ${bold}${globalBin}${reset} is not on your PATH.`); - console.log(` Add it with one of:`); const projected = projectPersistentPathExportActions({ targetDir: globalBin, platform: process.platform, }); - for (const action of projected.shellActions) { - const labelPrefix = action.label ? `${action.label}: ` : ''; - console.log(` ${cyan}${labelPrefix}${action.command}${reset}`); + if (projected.reason === PATH_ACTION_REASON.WIN32_RESERVED_QUOTE) { + // #3118 review MINOR: a win32 targetDir containing `"` makes + // projectPathActionProjection return [] (no command can quote it safely + // on Windows) — printing the "Add it with one of:" header with nothing + // under it is a silent dead-end. Name the cause instead. + console.log(` No command can be suggested: the path contains a ${cyan}"${reset} character, which cannot appear in a Windows path.`); + } else if (projected.shellActions.length === 0) { + // #3118: no target directory to talk about (reason === NO_TARGET_DIR, or + // no reason at all) — there is nothing to print beyond the "not on your + // PATH" line above. + } else { + console.log(` Add it with one of:`); + for (const action of projected.shellActions) { + const labelPrefix = action.label ? `${action.label}: ` : ''; + console.log(` ${cyan}${labelPrefix}${action.command}${reset}`); + } } console.log(''); } diff --git a/docs/how-to/install-on-your-runtime.md b/docs/how-to/install-on-your-runtime.md index e9503daa8..6c15dceea 100644 --- a/docs/how-to/install-on-your-runtime.md +++ b/docs/how-to/install-on-your-runtime.md @@ -538,7 +538,7 @@ If the command is not found after restart, verify the install directory matches If the installer's global bin directory is not on your `PATH`, it prints a one-time warning with a copy-paste command for your shell. The suggestion list covers `zsh`, `bash`, and `fish` (plus PowerShell, cmd.exe, and Git Bash on Windows). For fish, run the line it prints: ```fish -fish_add_path '/path/to/global/bin' +fish_add_path -- '/path/to/global/bin' ``` If the directory is already on your PATH but the installer still warns, open a new fish session (`exec fish`) to pick up the change. diff --git a/pwned_cmdsub b/pwned_cmdsub new file mode 100644 index 000000000..e69de29bb diff --git a/src/api-coverage.cts b/src/api-coverage.cts index 723f864f3..4afb4bd74 100644 --- a/src/api-coverage.cts +++ b/src/api-coverage.cts @@ -257,6 +257,31 @@ const SURFACE_DESCRIPTOR_WORDS = new Set([ * is deliberately absent (an external API IS external). */ const INTERNAL_DESCRIPTORS = new Set(['internal', 'in-house', 'local', 'first-party', 'private']); +/** #2784: negation suppression. A clause that pairs an integration verb with an + * API noun but the verb itself is directly negated (e.g. "does not integrate", + * "integrates no external API") is suppressed. Two windows are checked: a + * negation qualifier within 2 words directly before the verb, or "no"/"zero"/ + * "none" between the verb and a following noun. + * KNOWN LIMIT (deliberate, not a bug to fix later): a negation further than 2 + * words before the verb, or a clause where the noun precedes the verb, is NOT + * suppressed — e.g. "Ships without any API integration." still reports + * detected:true, because "without" sits outside the verb's 2-word lookback + * and the noun precedes the verb. This is intentional: detectApiIntegration + * is fail-closed — an unsuppressed false positive costs a one-line + * COVERAGE.md declaration, while widening the suppression window risks a + * false negative that silently lets a real external-API phase past a + * blocking gate. + * Hoisted to module scope (#3127 regression fix): this was previously + * allocated fresh on every source line, which is wasted work on documents + * with many lines. */ +const NEGATION_QUALIFIERS = new Set([ + 'no', 'not', 'without', 'zero', 'neither', 'nor', 'none', "don't", "doesn't", "didn't", "won't", "can't", "cannot", +]); +/** Negation tokens checked BETWEEN a verb and a later noun (narrower than + * NEGATION_QUALIFIERS — "not" and "without" are checked only immediately + * before the verb, via NEGATION_QUALIFIERS above). */ +const NEGATION_NOUN_TOKENS = new Set(['no', 'zero', 'none']); + /** A capitalized compound modifier ("Resolver-only", "Read-only", "E-commerce" * — lowercase letter right after the hyphen) is an adjective phrase, not a * service name. Real hyphenated services capitalize the second segment @@ -484,23 +509,37 @@ export function detectApiIntegration( // // #2784: negation suppression. A clause that pairs an integration verb with // an API noun but the verb itself is directly negated (e.g. "does not - // integrate", "integrates no external API", "without any API integration") - // is suppressed. The check is scoped to the verb's immediate context (the - // word directly before the verb, or the word directly between verb and noun) - // — NOT a blanket clause-wide scan, because "without changing runtime - // dependencies" in a long clause does NOT negate the integration. - const NEGATION_QUALIFIERS = new Set([ - 'no', 'not', 'without', 'zero', 'neither', 'nor', 'none', "don't", "doesn't", "didn't", "won't", "can't", "cannot", - ]); + // integrate", "integrates no external API") is suppressed. The check is + // scoped to the verb's immediate context (the word directly before the + // verb, or the word directly between verb and noun) — NOT a blanket + // clause-wide scan, because "without changing runtime dependencies" in a + // long clause does NOT negate the integration. + // KNOWN LIMIT (deliberate, not a bug to fix later): a negation further than + // 2 words before the verb, or a clause where the noun precedes the verb, is + // NOT suppressed — e.g. "Ships without any API integration." is NOT + // suppressed today (pinned by a test in tests/api-coverage.test.cjs). + // detectApiIntegration is fail-closed by design: an unsuppressed false + // positive costs a one-line COVERAGE.md declaration, while widening the + // window trades that for a silent false negative on a blocking gate. if (verbRe && nounRe) { for (const clause of clauses) { const verbs = collectTermMatches(verbRe, clause.text); if (verbs.length === 0) continue; // #2784: check if any verb is immediately preceded by a negation // qualifier (within 2 words before the verb match). + // + // OFFSET NOTE: `v.start`/`n.start` (from collectTermMatches below and + // above) are already CLAUSE-LOCAL — collectTermMatches was called with + // `clause.text`, not the full line — and so is `clauseText` + // (`clause.text.toLowerCase()`). They must be used AS-IS to index into + // `clauseText`; do not re-base them against `clause.start` (that field + // is the clause's offset within the LINE, a different coordinate space, + // used only to map line-level spans like `extraNouns`/`masked` into a + // clause). Subtracting `clause.start` here double-offsets the slice + // bounds for every clause after the first on a line (#3127 follow-up). const clauseText = clause.text.toLowerCase(); const hasNegatedVerb = verbs.some((v) => { - const before = clauseText.slice(Math.max(0, v.start - clause.start - 20), v.start - clause.start); + const before = clauseText.slice(Math.max(0, v.start - 20), v.start); const beforeWords = before.split(/\s+/).filter(Boolean).slice(-2); return beforeWords.some((w: string) => NEGATION_QUALIFIERS.has(w.replace(/[^a-z']/g, ''))); }); @@ -513,15 +552,63 @@ export function detectApiIntegration( } } if (nounTerms.size === 0) continue; - // Check for negation between verb and noun - const hasNegatedNoun = verbs.some((v) => { - return nouns.some((n) => { - if (n.start <= v.start) return false; - const between = clauseText.slice(v.start - clause.start + v.term.length, n.start - clause.start); + // Check for negation between verb and noun. + // + // #3127 regression: the original form of this check was + // O(verbs × nouns), re-slicing and re-splitting the clause text for + // every (verb, noun) pair — effectively cubic in clause length (a + // clause of N repeated "integrate api" pairs did O(N^2) pair checks, + // each doing an O(N) slice/split). On a clause with 800 repeated + // pairs this took ~8.5s; fast-check's property test then generated + // documents large enough to hang the whole test file past node:test's + // 600s timeout. It ALSO subtracted `clause.start` from `v.start`/ + // `n.start` before slicing `clauseText` — but `v.start`/`n.start` are + // already local to `clause.text` (collectTermMatches was called with + // clause.text, not the full line), and `clauseText` is exactly + // `clause.text.toLowerCase()`. So that subtraction double-offset the + // slice bounds for every clause after the first on a line, sliding + // (and for negative results, JS's negative-index slice() wraparound + // non-monotonically re-mapping) the window to characters unrelated to + // the verb/noun pair — an independent latent bug, fixed here as part + // of establishing a well-defined O(1) predicate (a piecewise/clamped + // window has no single "widest span" to reason about at all). + // + // EXACT-EQUIVALENCE, single pass: the predicate is "does any pair + // (v, n) with n.start > v.start have a negation token in the span + // (v.end, n.start)". Every such span is a SUBSET of the widest + // possible span for a given noun: [min(v.end) over verbs valid for + // that noun, n.start). And since that window only widens as a + // noun's start increases (more verbs become valid, and the noun + // bound itself grows), the single widest span across the WHOLE + // clause is anchored at the noun with the maximum start, using the + // minimum verb-end among verbs valid for THAT noun (not the global + // minimum verb-end, which could belong to a verb that starts after + // this noun and so is never a valid pairing with it — a mismatch + // that would either miss or falsely include a negation). If that one + // substring contains no negation token, no narrower pair-specific + // substring can either; if it does, the (minVerb, maxNoun) pair + // itself contains it. This drops the check to O(verbs + nouns). + let hasNegatedNoun = false; + if (nouns.length > 0) { + let minVerbStart = Infinity; + for (const v of verbs) if (v.start < minVerbStart) minVerbStart = v.start; + let maxNounStart = -Infinity; + for (const n of nouns) if (n.start > maxNounStart) maxNounStart = n.start; + if (maxNounStart > minVerbStart) { + let minQualifyingVerbEnd = Infinity; + for (const v of verbs) { + if (v.start < maxNounStart) { + const vEnd = v.start + v.term.length; + if (vEnd < minQualifyingVerbEnd) minQualifyingVerbEnd = vEnd; + } + } + const between = clauseText.slice(minQualifyingVerbEnd, maxNounStart); const betweenWords = between.split(/\s+/).filter(Boolean); - return betweenWords.some((w: string) => ['no', 'zero', 'none'].includes(w.replace(/[^a-z']/g, ''))); - }); - }); + hasNegatedNoun = betweenWords.some((w: string) => + NEGATION_NOUN_TOKENS.has(w.replace(/[^a-z']/g, '')), + ); + } + } if (hasNegatedVerb || hasNegatedNoun) continue; for (const vTerm of new Set(verbs.map((t) => t.term))) { for (const nTerm of nounTerms) emitPair(vTerm, nTerm, rawLine); diff --git a/src/milestone.cts b/src/milestone.cts index e9babe30e..14e77aa25 100644 --- a/src/milestone.cts +++ b/src/milestone.cts @@ -792,7 +792,7 @@ function cmdMilestoneComplete(cwd: string, version: string, options: MilestoneCo version, nextMilestoneCommand: formatGsdSlash('new-milestone', resolveRuntime(cwd)) as string, }, - { clock: realClock, progressProvider: () => null, sourcePath: statePath }, + { clock: realClock, sourcePath: statePath }, ); writeStateMd(statePath, result.content, cwd); } diff --git a/src/phase.cts b/src/phase.cts index 6f6466c48..be149bcab 100644 --- a/src/phase.cts +++ b/src/phase.cts @@ -2686,7 +2686,6 @@ function cmdPhaseComplete(cwd: string, phaseNum: string, raw: boolean): void { }, { clock: realClock, - progressProvider: () => null, // completePhase derives progress from the roadmap, not disk roadmapProvider: () => roadmapContent, sourcePath: statePath, }, diff --git a/src/review-lane-runner.cts b/src/review-lane-runner.cts index c26daf2d6..6120bc7d5 100644 --- a/src/review-lane-runner.cts +++ b/src/review-lane-runner.cts @@ -330,7 +330,7 @@ interface TranscriptEntry { export function antigravityWatermark( workspace: string, deps: RunnerDeps, -): { convId: string; lines: number } { +): { convId: string; lines: number; unreadable?: boolean } { const cachePath = `${deps.homeDir}/.gemini/antigravity-cli/cache/last_conversations.json`; if (!deps.exists(cachePath)) return { convId: '', lines: 0 }; let cache: Record; @@ -346,18 +346,31 @@ export function antigravityWatermark( try { return { convId, lines: deps.readFile(tx).split(/\r?\n/).filter((l) => l.trim()).length }; } catch { - return { convId, lines: 0 }; + // #3118: this conv-id pre-dates this run, so its transcript exists but this run cannot verify + // its line count. Reporting `lines: 0` would assert a fact we could not check — flag it instead + // so the fallback can decline rather than silently skip zero and replay a stale response. + return { convId, lines: 0, unreadable: true }; } } -/** Workspace lookup is case-insensitive — the leg's jq did `ascii_downcase` on both sides. */ -function resolveConvId(cache: Record, workspace: string): string { - if (Object.prototype.hasOwnProperty.call(cache, workspace)) { - const direct = cache[workspace]; +/** + * Workspace lookup is case-insensitive — the leg's jq did `ascii_downcase` on both sides. + * + * #3118: a successful `JSON.parse` does not by itself make the payload a usable object — + * `JSON.parse('null')` succeeds and returns `null`, so a truncated/zeroed cache file slips past + * the callers' parse-only try/catch. `typeof null === 'object'`, so the guard below must exclude + * `null` explicitly. Arrays are excluded too (not a workspace map), which also falls out of the + * `Object.entries`/`hasOwnProperty` lookups below returning nothing for array input. + */ +function resolveConvId(cache: unknown, workspace: string): string { + if (cache === null || typeof cache !== 'object') return ''; + const record = cache as Record; + if (Object.prototype.hasOwnProperty.call(record, workspace)) { + const direct = record[workspace]; if (typeof direct === 'string' && direct) return direct; } const target = workspace.toLowerCase(); - for (const [k, v] of Object.entries(cache)) { + for (const [k, v] of Object.entries(record)) { if (k.toLowerCase() === target && typeof v === 'string' && v) return v; } return ''; @@ -376,7 +389,7 @@ function transcriptPath(homeDir: string, convId: string): string { */ export function antigravityTranscriptFallback( workspace: string, - mark: { convId: string; lines: number }, + mark: { convId: string; lines: number; unreadable?: boolean }, deps: RunnerDeps, ): string { const cachePath = `${deps.homeDir}/.gemini/antigravity-cli/cache/last_conversations.json`; @@ -389,6 +402,9 @@ export function antigravityTranscriptFallback( } const convId = resolveConvId(cache, workspace); if (!convId) return ''; + // #3118: the watermark could not read this conversation's transcript, so there is no trustworthy + // skip for it. Declining is the fail-closed answer; skipping 0 would replay a prior run's review. + if (mark.unreadable === true && convId === mark.convId) return ''; const tx = transcriptPath(deps.homeDir, convId); if (!deps.exists(tx)) return ''; let lines: string[]; diff --git a/src/shell-command-projection.cts b/src/shell-command-projection.cts index 3ccaad438..f516daf8a 100644 --- a/src/shell-command-projection.cts +++ b/src/shell-command-projection.cts @@ -358,8 +358,35 @@ export function projectLegacySettingsHookCommand({ }); } +// Implements the TOML v1.0.0 basic-string escaping grammar (toml.md, "Basic +// strings" section, https://toml.io/en/v1.0.0#string): a basic string must +// escape the quotation mark, backslash, and control characters other than +// tab (U+0000-U+0008, U+000A-U+001F, U+007F). Compact escapes are used where +// TOML defines them (\b \t \n \f \r \" \\); every other character in the +// required ranges falls back to \uXXXX. See #3118 — an earlier version +// escaped only backslash and quote, so a raw newline/CR/NUL in a value +// produced an unparseable config.toml. +const TOML_COMPACT_ESCAPES: Record = { + '\x08': '\\b', + '\x09': '\\t', + '\x0A': '\\n', + '\x0C': '\\f', + '\x0D': '\\r', +}; + +// U+0000-U+0008, U+000A-U+001F, U+007F — control characters other than tab +// (U+0009), which the grammar permits unescaped. +const TOML_MUST_ESCAPE_CONTROL_CHARS = /[\x00-\x08\x0A-\x1F\x7F]/g; + export function escapeTomlDoubleQuotedString(value: unknown): string { - return String(value).replace(/\\/g, '\\\\').replace(/"/g, '\\"'); + return String(value) + .replace(/\\/g, '\\\\') + .replace(/"/g, '\\"') + .replace(TOML_MUST_ESCAPE_CONTROL_CHARS, (ch) => { + const compact = TOML_COMPACT_ESCAPES[ch]; + if (compact) return compact; + return `\\u${ch.codePointAt(0)!.toString(16).padStart(4, '0')}`; + }); } export function projectCodexHookTomlCommand({ absoluteRunner, scriptPath, platform = process.platform }: { @@ -388,12 +415,35 @@ export function escapeSingleQuotedShellLiteral(value: unknown): string { return String(value).replace(/'/g, "'\\''"); } +/** + * The `export PATH=":$PATH"` line every persistence lane appends, plus the escaped directory + * token it embeds. One builder because three lanes emit this line: a lane that re-escapes it + * locally is how #3118 shipped a `$(…)` into ~/.bashrc, where it ran on every new shell. The + * escaping is for the line's FINAL context — a double-quoted string in an rc file — not for + * whatever transport (an `echo`, a paste) it passes through on the way there. + */ +export function projectPathExportLine(targetDir: unknown): { escapedDir: string; line: string } { + const escapedDir = escapePosixDoubleQuoted(String(targetDir)); + return { escapedDir, line: `export PATH="${escapedDir}:$PATH"` }; +} + interface ShellAction { label: string | null; shell: string; command: string; } +/** + * Why a PATH suggestion produced no actions. An empty `shellActions` alone folds two different + * facts together — "no target directory was given" and "this target directory cannot be + * expressed as a shell command" — and a caller that cannot tell them apart prints a header with + * nothing under it (#3118). + */ +export const PATH_ACTION_REASON = Object.freeze({ + NO_TARGET_DIR: 'no_target_dir', + WIN32_RESERVED_QUOTE: 'win32_reserved_quote', +}); + export function renderShellActionLines(shellActions: ShellAction[] = []): string[] { return shellActions.map((action) => { if (!action || !action.command) return ''; @@ -409,15 +459,23 @@ export function projectPathActionProjection({ mode?: string; targetDir?: string | null; platform?: string; -}): { shellActions: ShellAction[]; actionLines: string[] } { - if (!targetDir) return { shellActions: [], actionLines: [] }; +}): { shellActions: ShellAction[]; actionLines: string[]; reason?: string } { + if (!targetDir) return { shellActions: [], actionLines: [], reason: PATH_ACTION_REASON.NO_TARGET_DIR }; + + // #3118: `"` is reserved on Windows, so a path containing one cannot exist — and it would close + // cmd's quoted region in the `powershell -Command "…"` lane below, turning the rest into cmd + // input. There is no correct command to suggest for an impossible path: fail closed rather than + // emit one whose quoting can be broken. + if (platform === 'win32' && String(targetDir).includes('"')) return { shellActions: [], actionLines: [], reason: PATH_ACTION_REASON.WIN32_RESERVED_QUOTE }; const isWin32 = platform === 'win32'; let shellActions: ShellAction[]; if (isWin32) { const psTargetDir = escapePowerShellSingleQuoted(targetDir); - const bashTargetDir = escapeSingleQuotedShellLiteral(posixNormalize(String(targetDir))); + const bashExportLine = escapeSingleQuotedShellLiteral( + projectPathExportLine(posixNormalize(String(targetDir))).line, + ); shellActions = [ { label: 'PowerShell', @@ -432,40 +490,49 @@ export function projectPathActionProjection({ { label: 'Git Bash', shell: 'bash', - command: `echo 'export PATH="${bashTargetDir}:$PATH"' >> ~/.bashrc`, + command: `echo '${bashExportLine}' >> ~/.bashrc`, }, ]; } else if (mode === 'persist') { - const bashTargetDir = escapeSingleQuotedShellLiteral(String(targetDir)); + const exportLine = escapeSingleQuotedShellLiteral(projectPathExportLine(targetDir).line); + const fishTargetDir = escapeSingleQuotedShellLiteral(String(targetDir)); shellActions = [ { label: 'zsh', shell: 'zsh', - command: `echo 'export PATH="${bashTargetDir}:$PATH"' >> ~/.zshrc`, + command: `echo '${exportLine}' >> ~/.zshrc`, }, { label: 'bash', shell: 'bash', - command: `echo 'export PATH="${bashTargetDir}:$PATH"' >> ~/.bashrc`, + command: `echo '${exportLine}' >> ~/.bashrc`, }, // #323: fish has no `export`/`$PATH`-list syntax. `fish_add_path` is the // fish-native API (>= fish 3.2, 2021) that persists to the universal // variable store and de-duplicates. The directory is single-quoted with // the same POSIX literal escaping as the zsh/bash siblings — `'\''` is // also a valid escaped single quote in fish between quote spans. + // + // #3118 review MINOR: a `targetDir` with a leading `-` (e.g. `-v`) is a + // legal directory name, but fish's argparse-based option scanning + // treats a leading-dash token as a flag REGARDLESS of quoting, so + // `fish_add_path '-v'` misparses it and prints "No paths to add, not + // setting anything." (exit 1) instead of adding the path. `--` is + // fish's standard end-of-options separator; verified empirically + // against a real fish 4.8.1 install that `fish_add_path -- '-v'` + // succeeds where the unseparated form fails. { label: 'fish', shell: 'fish', - command: `fish_add_path '${bashTargetDir}'`, + command: `fish_add_path -- '${fishTargetDir}'`, }, ]; } else { - const posixTargetDir = escapePosixDoubleQuoted(targetDir); shellActions = [ { label: null, shell: 'posix', - command: `export PATH="${posixTargetDir}:$PATH"`, + command: projectPathExportLine(targetDir).line, }, ]; } @@ -479,13 +546,15 @@ export function projectPathActionProjection({ export function projectPersistentPathExportActions({ targetDir, platform = process.platform }: { targetDir?: string | null; platform?: string; -}): { shellActions: ShellAction[] } { +}): { shellActions: ShellAction[]; reason?: string } { const projected = projectPathActionProjection({ mode: 'persist', targetDir, platform, }); - return { shellActions: projected.shellActions }; + return projected.reason === undefined + ? { shellActions: projected.shellActions } + : { shellActions: projected.shellActions, reason: projected.reason }; } diff --git a/src/state-transition.cts b/src/state-transition.cts index 362180719..8b7c2fbdf 100644 --- a/src/state-transition.cts +++ b/src/state-transition.cts @@ -297,10 +297,7 @@ export const STATE_MD_SECTIONS = { // Intent + deps + result types (ADR-1769 §3) // ---------------------------------------------------------------------------- -export type ProgressRecord = Record; - export type StateTransitionDeps = { - progressProvider: () => ProgressRecord | null; clock: { today: () => string; localToday: () => string; nowIso: () => string }; /** * Roadmap content provider for transitions that re-derive milestone-wide @@ -601,7 +598,26 @@ function locateCurrentPosition(body: string): { start: number; end: number } | n const start = h.offset + hl.length + 1; let end = body.length; for (let j = idx + 1; j < hs.length; j++) { - if (STOP_H2_PLUS(hs[j].level)) { end = hs[j].offset - 1; break; } + if (STOP_H2_PLUS(hs[j].level)) { + // Exclude the newline that separates this section from the next + // heading. Walk back over a bare `\n`, then over a `\r` if one + // immediately precedes it (CRLF), so a CRLF document's slice does not + // retain a stray unpaired trailing `\r` (#3118). + let e = hs[j].offset; + if (e > 0 && body[e - 1] === '\n') { + e -= 1; + if (e > 0 && body[e - 1] === '\r') e -= 1; + } + // Clamp so the span can never invert (#3118 review): when the section + // is empty and the next heading follows with no blank line between, + // walking back over the newline(s) can land `e` before `start`. An + // inverted span makes every mutator's `body.slice(0, start) + + // sectionBody + body.slice(end)` reassembly duplicate the bytes in + // `[end, start)`. A zero-length span (`end === start`) is the correct + // representation of an empty-but-present section. + end = Math.max(e, start); + break; + } } return { start, end }; } diff --git a/src/state.cts b/src/state.cts index 836a3fdb3..0d4561b6b 100644 --- a/src/state.cts +++ b/src/state.cts @@ -467,7 +467,7 @@ function cmdStatePatch(cwd: string, patches: Record, raw: boolea // and the resync-progress decision stay in this adapter. let results: { updated: string[]; failed: string[] } = { updated: [], failed: [] }; readModifyWriteStateMd(statePath, (content) => { - const result = transitionCore(content, { kind: 'patch', patches }, { clock: realClock, progressProvider: () => null }); + const result = transitionCore(content, { kind: 'patch', patches }, { clock: realClock }); results = (result.data as { updated: string[]; failed: string[] }) ?? results; return result.content; }, cwd, { resync: shouldResync }); @@ -505,7 +505,7 @@ function cmdStateUpdate(cwd: string, field: string | undefined, value: string | const result = transitionCore( content, { kind: 'update', field: field as string, value: value as string }, - { clock: realClock, progressProvider: () => null }, + { clock: realClock }, ); updated = (result.data as { updated: boolean } | undefined)?.updated === true; return result.content; @@ -556,7 +556,6 @@ function cmdStateAdvancePlan(cwd: string, raw: boolean): void { const intent: StateTransitionIntent = { kind: 'advancePlan' }; const deps: StateTransitionDeps = { clock: realClock, - progressProvider: () => null, sourcePath: statePath, }; @@ -2435,7 +2434,6 @@ function cmdStateBeginPhase(cwd: string, phaseNumber: string | number, phaseName }; const deps: StateTransitionDeps = { clock: realClock, - progressProvider: () => null, // beginPhase doesn't consult disk progress; syncStateFrontmatter's scan is authoritative sourcePath: statePath, }; @@ -2713,7 +2711,6 @@ function cmdStatePlannedPhase(cwd: string, phaseNumber: string | number, planCou }; const deps: StateTransitionDeps = { clock: realClock, - progressProvider: () => null, sourcePath: statePath, }; @@ -2751,7 +2748,7 @@ function cmdStateMilestoneSwitch(cwd: string, version: string | undefined, name: // milestoneSwitch rebuilds frontmatter directly and must not run the // steady-state syncStateFrontmatter post-sync. const intent: StateTransitionIntent = { kind: 'milestoneSwitch', version, name: resolvedName }; - const deps: StateTransitionDeps = { clock: realClock, progressProvider: () => null, sourcePath: statePath }; + const deps: StateTransitionDeps = { clock: realClock, sourcePath: statePath }; const lockPath = acquireStateLock(statePath); try { @@ -2990,7 +2987,7 @@ function cmdStateSync(cwd: string, options: StateSyncOptions | undefined, raw: b const syncResult = transitionCore( modified, { kind: 'sync', totalPlansInPhase: highestIncompletePhase ? highestIncompletePhaseplanCount : null, percent }, - { clock: realClock, progressProvider: () => null }, + { clock: realClock }, ); modified = syncResult.content; const coreChanges = (syncResult.data as { changes?: string[] } | undefined)?.changes ?? []; @@ -3066,7 +3063,7 @@ function cmdStatePrune(cwd: string, options: StatePruneOptions, raw: boolean): v // This adapter owns currentPhase derivation (#1760 `Phase`/`Current Phase` // fallback above), dry-run, and STATE-ARCHIVE.md writes. const runPruneCore = (content: string): { newContent: string; archivedSections: PrunedSection[] } => { - const result = transitionCore(content, { kind: 'prune', cutoff }, { clock: realClock, progressProvider: () => null }); + const result = transitionCore(content, { kind: 'prune', cutoff }, { clock: realClock }); return { newContent: result.content, archivedSections: ((result.data as { archivedSections?: PrunedSection[] } | undefined)?.archivedSections) ?? [], @@ -3184,7 +3181,6 @@ function cmdStateRebuild(cwd: string, options: StateRebuildOptions, raw: boolean }; const deps: StateTransitionDeps = { - progressProvider: () => null, clock: realClock, phaseInventoryProvider, // Without this, `state rebuild --dry-run` reported a truncated STATE.md anonymously: the diff --git a/tests/api-coverage.test.cjs b/tests/api-coverage.test.cjs index b96e53e9f..905ba5789 100644 --- a/tests/api-coverage.test.cjs +++ b/tests/api-coverage.test.cjs @@ -131,6 +131,31 @@ describe('detectApiIntegration — pure detector (#1562)', () => { }); } + // ── #2784/#3127: negation suppression + its #3127 perf regression ──────── + // PR #3127 added clause-local negation suppression (hasNegatedVerb / + // hasNegatedNoun) but the noun-side check was O(verbs × nouns) with a + // slice+split per pair — effectively cubic in clause length. A clause with + // a few hundred repeated "integrate api" pairs took seconds; fast-check's + // property test then generated documents large enough to hang the whole + // file past node:test's 600s timeout (fail 0 / cancelled 1). These two + // tests pin the O(verbs + nouns) fix at a size (~500 pairs) chosen so that + // a re-regression to the old quadratic-per-pair behavior makes this file + // exceed the runner's own timeout — that hang, not a wall-clock assertion + // (CLAUDE.md forbids those), is the failure signal for a re-regression. + test('suppresses a negated pair regardless of how many terms precede it', () => { + const pairs = Array(500).fill('integrate api').join(' '); + const scope = pairs + ' but we integrate no api here'; + const r = detectApiIntegration(scope); + assert.strictEqual(r.detected, false); + }); + + test('still detects a genuine pair in a very long clause', () => { + const pairs = Array(500).fill('integrate api').join(' '); + const r = detectApiIntegration(pairs); + assert.strictEqual(r.detected, true); + assert.ok(r.signals.length > 0); + }); + test('fenced code blocks are stripped — trigger inside a code fence does not fire', () => { const scope = [ 'Refactor the helpers.', @@ -165,6 +190,74 @@ describe('detectApiIntegration — pure detector (#1562)', () => { assert.strictEqual(detectApiIntegration(splitLine).detected, false); assert.strictEqual(detectApiIntegration('integrates the api').detected, true); }); + + // ── #3127 follow-up: hasNegatedVerb offset regression coverage ─────────── + // The `hasNegatedVerb` window previously subtracted `clause.start` a SECOND + // time from an already clause-local `v.start`, which only accidentally + // worked for the first clause on a line (clause.start === 0) and silently + // mis-windowed (via negative-index slice wraparound) every later clause. + test('suppresses a pair when the verb is directly negated', () => { + const r = detectApiIntegration('This phase does not integrate any external API.'); + assert.strictEqual(r.detected, false); + }); + + test('suppresses a pair when a negation sits between verb and noun', () => { + const r = detectApiIntegration('This phase integrates no external API.'); + assert.strictEqual(r.detected, false); + }); + + test('still detects a genuine integration', () => { + const r = detectApiIntegration('We integrate the Stripe API.'); + assert.strictEqual(r.detected, true); + const compound = r.signals.find((s) => s.verb !== '(surface)'); + assert.ok(compound, 'expected a compound verb+noun signal'); + assert.strictEqual(compound.verb, 'integrate'); + assert.strictEqual(compound.noun, 'api'); + }); + + test('a distant negation does not suppress a genuine pair', () => { + // #2784's own documented example: "without changing runtime dependencies" + // inside a long clause must NOT negate an unrelated genuine pairing later + // in the SAME clause — the negation-window checks are scoped to the + // verb's immediate context, not a blanket clause-wide scan. + const r = detectApiIntegration( + 'The migration proceeds without changing runtime dependencies while we integrate the Stripe API for payment processing across the whole checkout flow.' + ); + assert.strictEqual(r.detected, true); + }); + + test('suppression works for a clause that is not the first on the line', () => { + // THE REGRESSION TEST for the double-offset bug: before the fix, this + // failed (wrongly detected === true) because the negation window for the + // second clause was computed against the wrong index base (v.start was + // re-based against clause.start even though it was already clause-local), + // producing an empty/garbage "before" window via negative-index slice() + // wraparound so the verb-adjacent negation was never seen. + const r = detectApiIntegration('We ship a thing; this phase does not integrate an external API.'); + assert.strictEqual(r.detected, false); + }); + + test('suppression in an early clause does not mask a genuine pair in a later clause', () => { + // Intended semantics: negation suppression is scoped PER CLAUSE (matching + // the "no cross-clause binding" design elsewhere in this module — see the + // CLAUSE_BOUNDARY_CHARS note). A negated pair in clause 1 must not swallow + // an independent, unnegated, genuine pair in clause 2: each clause is + // evaluated on its own, and `detected` is true if ANY clause fires. + const r = detectApiIntegration('This phase integrates no external API; we also integrate the Stripe API.'); + assert.strictEqual(r.detected, true); + }); + + test('a negation outside the lookback window does not suppress — fail-closed by design', () => { + // This pins DELIBERATE fail-closed behavior, not an aspiration: "without" + // sits outside hasNegatedVerb's 2-word lookback before the verb, and the + // noun ("integration") precedes the verb ("Ships") so hasNegatedNoun's + // verb-to-noun window never applies either. Widening the lookback to catch + // this phrase would trade a cheap false positive (a one-line COVERAGE.md + // declaration) for a silent false negative — a real external-API phase + // slipping past a blocking gate — which is the dangerous direction. + const r = detectApiIntegration('Ships without any API integration.'); + assert.strictEqual(r.detected, true); + }); }); // ────────────────────────────────────────────────────────────────────────────── diff --git a/tests/fix-2136-clock-local-today.test.cjs b/tests/fix-2136-clock-local-today.test.cjs index d824b229a..108871f0b 100644 --- a/tests/fix-2136-clock-local-today.test.cjs +++ b/tests/fix-2136-clock-local-today.test.cjs @@ -112,7 +112,7 @@ describe('#2136 last_activity is stamped from localToday, not today', () => { const result = transitionCore( input, { kind: 'sync', totalPlansInPhase: 5, percent: 60 }, - { clock: splitClock, progressProvider: () => null }, + { clock: splitClock }, ); assert.strictEqual(stateExtractField(result.content, 'Last Activity'), '2020-06-14', 'Last Activity must use localToday() (2020-06-14), not today() (2020-06-15)'); diff --git a/tests/review-lane-runner.test.cjs b/tests/review-lane-runner.test.cjs index 9ae705a8c..c0df48862 100644 --- a/tests/review-lane-runner.test.cjs +++ b/tests/review-lane-runner.test.cjs @@ -19,6 +19,7 @@ const { writeReviewOrStub, handleOpencodeOutput, stampBlindReview, + antigravityWatermark, antigravityTranscriptFallback, runOpenAiCompatible, } = require('../gsd-core/bin/lib/review-lane-runner.cjs'); @@ -342,6 +343,141 @@ describe('runner — antigravity handler (#2073 / #2176)', () => { assert.equal(antigravityTranscriptFallback(ROOT, { convId: '', lines: 0 }, d), ''); }); + // ── antigravityWatermark (#3118) ─────────────────────────────────────────── + // + // Every test above hands the fallback a HAND-WRITTEN mark. None of them calls + // `antigravityWatermark`, so none says anything about whether the mark a real run produces is + // correct. The producer had zero test references before this block; the fail-open below lived + // entirely in that gap. + describe('antigravityWatermark — the mark a real run actually produces', () => { + test('returns an empty mark when the conversation cache is absent', () => { + assert.deepEqual(antigravityWatermark(ROOT, deps()), { convId: '', lines: 0 }); + }); + + test('returns an empty mark when the cache is not valid JSON', () => { + const d = deps({ files: { [CACHE]: 'NOT JSON' } }); + assert.deepEqual(antigravityWatermark(ROOT, d), { convId: '', lines: 0 }); + }); + + test('returns an empty mark when the workspace has no conversation', () => { + const d = deps({ files: { [CACHE]: JSON.stringify({ '/somewhere/else': 'c9' }) } }); + assert.deepEqual(antigravityWatermark(ROOT, d), { convId: '', lines: 0 }); + }); + + for (const [label, body] of [ + ['a number', '0'], + ['a string', '"just a string"'], + ['an array', '[]'], + ['null', 'null'], + ['a boolean', 'true'], + ]) { + test(`a conversation cache that is ${label} yields an empty mark`, () => { + // Valid JSON that is not an object still reaches hasOwnProperty / Object.entries. + const d = deps({ files: { [CACHE]: body } }); + assert.deepEqual(antigravityWatermark(ROOT, d), { convId: '', lines: 0 }); + }); + } + + test('ignores a non-string conversation id', () => { + const d = deps({ files: { [CACHE]: JSON.stringify({ [ROOT]: 42 }) } }); + assert.equal(antigravityWatermark(ROOT, d).convId, ''); + }); + + test('ignores an empty-string conversation id', () => { + const d = deps({ files: { [CACHE]: JSON.stringify({ [ROOT]: '' }) } }); + assert.equal(antigravityWatermark(ROOT, d).convId, ''); + }); + + test('resolves the workspace case-insensitively', () => { + const d = deps({ files: { [CACHE]: JSON.stringify({ '/REPO': 'c1' }), [TX('c1')]: entry('x') } }); + assert.equal(antigravityWatermark('/repo', d).convId, 'c1'); + }); + + test('keeps the conversation id when the transcript does not exist yet', () => { + // Distinct from the cases above: the conversation is KNOWN, it simply has no transcript. + const d = deps({ files: { [CACHE]: JSON.stringify({ [ROOT]: 'c1' }) } }); + assert.deepEqual(antigravityWatermark(ROOT, d), { convId: 'c1', lines: 0 }); + }); + + test('counts the non-blank transcript lines', () => { + const files = { [CACHE]: JSON.stringify({ [ROOT]: 'c1' }), [TX('c1')]: [entry('a'), entry('b')].join('\n') }; + assert.equal(antigravityWatermark(ROOT, deps({ files })).lines, 2); + }); + + test('an empty transcript is zero lines, not an unreadable one', () => { + // Negative space for the fix: a genuinely empty transcript must NOT degrade. + const files = { [CACHE]: JSON.stringify({ [ROOT]: 'c1' }), [TX('c1')]: '' }; + const mark = antigravityWatermark(ROOT, deps({ files })); + assert.equal(mark.lines, 0); + assert.notEqual(mark.unreadable, true, 'an empty transcript is readable, just empty'); + }); + + test('whitespace-only transcript lines are not counted', () => { + const files = { [CACHE]: JSON.stringify({ [ROOT]: 'c1' }), [TX('c1')]: '\n \n\t\n' }; + const mark = antigravityWatermark(ROOT, deps({ files })); + assert.equal(mark.lines, 0); + assert.notEqual(mark.unreadable, true); + }); + + test('counts CRLF transcript lines the same as LF', () => { + const lf = { [CACHE]: JSON.stringify({ [ROOT]: 'c1' }), [TX('c1')]: [entry('a'), entry('b')].join('\n') }; + const crlf = { [CACHE]: JSON.stringify({ [ROOT]: 'c1' }), [TX('c1')]: [entry('a'), entry('b')].join('\r\n') }; + assert.equal( + antigravityWatermark(ROOT, deps({ files: lf })).lines, + antigravityWatermark(ROOT, deps({ files: crlf })).lines, + ); + }); + + test('does not report zero lines when the transcript could not be read', () => { + // THE FAIL-OPEN. The transcript EXISTS and its conversation pre-dates this run, so its + // content is definitionally stale — but the read threw, so the count is unknown. Returning + // `lines: 0` is indistinguishable from "genuinely empty" and asserts a fact the function + // could not verify. + const files = { [CACHE]: JSON.stringify({ [ROOT]: 'c1' }), [TX('c1')]: 'unused' }; + const d = deps({ files }); + const realRead = d.readFile; + d.readFile = (p) => { + if (p === TX('c1')) throw new Error('EACCES'); + return realRead(p); + }; + + const mark = antigravityWatermark(ROOT, d); + assert.equal(mark.convId, 'c1', 'the conversation id was resolved and stays trustworthy'); + assert.equal(mark.unreadable, true, 'an unreadable transcript must be distinguishable from an empty one'); + }); + + test('the fallback declines when the watermark could not be established', () => { + // The CONSEQUENCE of the branch above. The watermark read fails; the fallback's own read + // then succeeds (transient EACCES, a concurrent writer, a partial flush). With `lines: 0` + // and a matching convId the fallback skips nothing and returns a PREVIOUS run's review as + // this run's — the precise failure the "never stale" docstring promises cannot happen. + const files = { [CACHE]: JSON.stringify({ [ROOT]: 'c1' }), [TX('c1')]: entry('STALE FROM LAST RUN') }; + const marking = deps({ files }); + const realRead = marking.readFile; + marking.readFile = (p) => { + if (p === TX('c1')) throw new Error('EACCES'); + return realRead(p); + }; + const mark = antigravityWatermark(ROOT, marking); + + assert.equal( + antigravityTranscriptFallback(ROOT, mark, deps({ files })), + '', + 'an unverified watermark must not license replaying the transcript', + ); + }); + + test('the fallback still skips exactly the pre-run lines when the mark is sound', () => { + // Negative space for the fix: a sound mark must keep working end-to-end, producer included. + const files = { [CACHE]: JSON.stringify({ [ROOT]: 'c1' }), [TX('c1')]: entry('old') }; + const mark = antigravityWatermark(ROOT, deps({ files })); + assert.equal(mark.lines, 1); + + files[TX('c1')] = [entry('old'), entry('THIS RUN')].join('\n'); + assert.equal(antigravityTranscriptFallback(ROOT, mark, deps({ files })), 'THIS RUN'); + }); + }); + test('the blind-review marker is anchored to the head of the output', () => { assert.ok(stampBlindReview('REVIEWED-WITHOUT-REPO-ACCESS\nbody').startsWith('> [reviewed-without-repo-access]')); }); diff --git a/tests/shell-command-projection-dispatch.test.cjs b/tests/shell-command-projection-dispatch.test.cjs index 8b042a554..fdab2fcc3 100644 --- a/tests/shell-command-projection-dispatch.test.cjs +++ b/tests/shell-command-projection-dispatch.test.cjs @@ -17,6 +17,17 @@ const { platformEnsureDir, dispatchGsdCommand, resolveGsdToolsPath, + projectPathActionProjection, + projectPathExportLine, + posixNormalize, + PATH_ACTION_REASON, + renderShellActionLines, + formatManagedHookScriptToken, + escapeTomlDoubleQuotedString, + escapePowerShellSingleQuoted, + escapePosixDoubleQuoted, + escapeSingleQuotedShellLiteral, + retryRenameSync, } = require(path.join(__dirname, '..', 'gsd-core', 'bin', 'lib', 'shell-command-projection.cjs')); const { createTempGitProject, createTempDir, cleanup } = require('./helpers.cjs'); @@ -961,7 +972,9 @@ describe('bug #3441: PATH guidance is projected from typed shell action IR', () assert.equal(projected.shellActions[1].command.includes("/tmp/O'\\''Neil/bin"), true); // #323: fish entry single-quotes the dir with the same POSIX literal // escaping (`'\''` is also a valid escaped quote in fish unquoted context). - assert.equal(projected.shellActions[2].command, "fish_add_path '/tmp/O'\\''Neil/bin'"); + // #3118: `--` is fish's end-of-options separator, added so a leading-dash + // directory name is not misparsed as a flag; every fish_add_path lane carries it now. + assert.equal(projected.shellActions[2].command, "fish_add_path -- '/tmp/O'\\''Neil/bin'"); }); test('maybeSuggestPathExport renders commands projected by path-action seam', () => { @@ -1619,3 +1632,391 @@ test('platformWriteSync survives a concurrent collision on the same target path' }); }); } + +// ──────────────────────────────────────────────────────────────────────── +// #3118 — failing-first coverage for a path-export projection API that does +// not exist yet in this module (projectPathExportLine, escapeCmdDoubleQuotedArgument). +// These tests are EXPECTED TO FAIL until that API lands. +// ──────────────────────────────────────────────────────────────────────── + +describe('path export projection — escaping (#3118)', () => { + const laneCommand = (mode, targetDir, platform, shell) => { + const { shellActions } = projectPathActionProjection({ mode, targetDir, platform }); + const a = shellActions.find((x) => x.shell === shell); + return a ? a.command : null; + }; + + // The fish lane is the ONE deliberate output change for ordinary paths: fish_add_path + // misparses a leading-dash directory name without the `--` end-of-options separator + // (#3118 review finding), so every path — hostile or ordinary — now gets `--` on the + // fish lane. Every other lane (posix export, zsh/bash echo, PowerShell, cmd) remains + // byte-identical for an ordinary path. + test('leaves an ordinary path unchanged on every lane except fish', () => { + const repairLinux = projectPathActionProjection({ mode: 'repair', targetDir: '/usr/local/bin', platform: 'linux' }); + assert.deepEqual(repairLinux.shellActions, [ + { label: null, shell: 'posix', command: 'export PATH="/usr/local/bin:$PATH"' }, + ]); + assert.deepEqual(repairLinux.actionLines, [ + 'export PATH="/usr/local/bin:$PATH"', + ]); + + const persistLinux = projectPathActionProjection({ mode: 'persist', targetDir: '/usr/local/bin', platform: 'linux' }); + assert.deepEqual(persistLinux.shellActions, [ + { label: 'zsh', shell: 'zsh', command: `echo 'export PATH="/usr/local/bin:$PATH"' >> ~/.zshrc` }, + { label: 'bash', shell: 'bash', command: `echo 'export PATH="/usr/local/bin:$PATH"' >> ~/.bashrc` }, + { label: 'fish', shell: 'fish', command: `fish_add_path -- '/usr/local/bin'` }, + ]); + assert.deepEqual(persistLinux.actionLines, [ + `zsh: echo 'export PATH="/usr/local/bin:$PATH"' >> ~/.zshrc`, + `bash: echo 'export PATH="/usr/local/bin:$PATH"' >> ~/.bashrc`, + `fish: fish_add_path -- '/usr/local/bin'`, + ]); + + const repairWin32 = projectPathActionProjection({ mode: 'repair', targetDir: 'C:/Users/dev/bin', platform: 'win32' }); + assert.deepEqual(repairWin32.shellActions, [ + { + label: 'PowerShell', + shell: 'powershell', + command: `[Environment]::SetEnvironmentVariable('PATH', 'C:/Users/dev/bin;' + [Environment]::GetEnvironmentVariable('PATH', 'User'), 'User')`, + }, + { + label: 'cmd.exe', + shell: 'cmd', + command: `powershell -Command "[Environment]::SetEnvironmentVariable('PATH', 'C:/Users/dev/bin;' + [Environment]::GetEnvironmentVariable('PATH', 'User'), 'User')"`, + }, + { + label: 'Git Bash', + shell: 'bash', + command: `echo 'export PATH="C:/Users/dev/bin:$PATH"' >> ~/.bashrc`, + }, + ]); + }); + + test('the fish lane gains the end-of-options separator for every path', () => { + const command = laneCommand('persist', '/usr/local/bin', 'linux', 'fish'); + assert.equal(command, `fish_add_path -- '/usr/local/bin'`); + }); + + test('does not write a command substitution into the rc file', () => { + const { escapedDir } = projectPathExportLine('/tmp/a$(id)b'); + assert.equal(escapedDir, '/tmp/a\\$(id)b'); + }); + + test('does not write a backtick substitution into the rc file', () => { + const { escapedDir } = projectPathExportLine('/tmp/a`whoami`b'); + assert.equal(escapedDir, '/tmp/a\\`whoami\\`b'); + }); + + test('keeps the persisted rc line quote-balanced', () => { + const { escapedDir } = projectPathExportLine('/tmp/a"b'); + assert.equal(escapedDir, '/tmp/a\\"b'); + }); + + test('escapes a backslash in the persisted line', () => { + const { escapedDir } = projectPathExportLine('/tmp/a\\b'); + assert.equal(escapedDir, '/tmp/a\\\\b'); + }); + + // #3118 review finding (now fixed): the other four escapers' target contexts + // (PowerShell single-quoted, POSIX double-quoted, POSIX single-quoted, and the win32 + // JSON.stringify-based hook token) all treat a raw newline/CR/NUL as literal data that + // stays inside the quoting/escaping without breaking out, so those four DO survive + // intact and are pinned exactly below. escapeTomlDoubleQuotedString is exercised + // separately below — it now escapes TOML's required control-character ranges rather + // than passing them through raw. + test('the escapers survive newlines and null bytes', () => { + const cases = [ + ['\n', '\n', 'a\\nb'], + ['\r\n', '\r\n', 'a\\r\\nb'], + ['\0', '\0', 'a\\u0000b'], + ]; + for (const [raw, , tomlExpected] of cases) { + const value = `a${raw}b`; + assert.equal(escapePowerShellSingleQuoted(value), `a${raw}b`); + assert.equal(escapePosixDoubleQuoted(value), `a${raw}b`); + assert.equal(escapeSingleQuotedShellLiteral(value), `a${raw}b`); + assert.equal(formatManagedHookScriptToken(value, { platform: 'win32' }), JSON.stringify(`a${raw}b`)); + assert.equal(escapeTomlDoubleQuotedString(value), tomlExpected); + } + }); + + // TOML v1.0.0 basic-string grammar (https://toml.io/en/v1.0.0#string): a basic string + // must escape the quotation mark, backslash, and control characters other than tab + // (U+0000-U+0008, U+000A-U+001F, U+007F). Compact escapes (\b \t \n \f \r \" \\) are + // used where TOML defines them; every other required character falls back to \uXXXX. + // Tab (U+0009) is explicitly excluded from the "must escape" set, so it passes through + // unescaped. Exact expected outputs below were derived by running the built function + // (see dispatch report table). + test('escapes control characters to a parseable TOML basic string', () => { + assert.equal(escapeTomlDoubleQuotedString('\n'), '\\n'); + assert.equal(escapeTomlDoubleQuotedString('\r'), '\\r'); + assert.equal(escapeTomlDoubleQuotedString('\r\n'), '\\r\\n'); + assert.equal(escapeTomlDoubleQuotedString('\0'), '\\u0000'); + assert.equal(escapeTomlDoubleQuotedString('\t'), '\t'); + assert.equal(escapeTomlDoubleQuotedString('\b'), '\\b'); + assert.equal(escapeTomlDoubleQuotedString('\f'), '\\f'); + assert.equal(escapeTomlDoubleQuotedString('\x1F'), '\\u001f'); + assert.equal(escapeTomlDoubleQuotedString('\x7F'), '\\u007f'); + // Ordering pin: backslash escaped FIRST (doubled), then quote, then control chars — + // so a backslash introduced by an earlier escape step is never re-escaped. + assert.equal(escapeTomlDoubleQuotedString('a\\b"c\nd'), 'a\\\\b\\"c\\nd'); + }); + + test('leaves an ordinary value byte-identical', () => { + const value = 'C:/Users/dev/gsd.js'; + assert.equal(escapeTomlDoubleQuotedString(value), value); + }); + + test('produces a TOML basic string that parses back to the original', () => { + // No built-in or dependency TOML parser is available in this environment (Node has + // no built-in TOML support, and neither `toml` nor `@iarna/toml` nor `smol-toml` is a + // project dependency), so this round-trips through a minimal inline unescape that + // implements only the escape forms escapeTomlDoubleQuotedString can produce + // (\\ \" \b \t \n \f \r \uXXXX) — sufficient to invert this encoder, not a general + // TOML parser. + const unescapeTomlBasicString = (escaped) => { + let out = ''; + for (let i = 0; i < escaped.length; i += 1) { + const ch = escaped[i]; + if (ch !== '\\') { + out += ch; + continue; + } + const next = escaped[i + 1]; + if (next === 'u') { + out += String.fromCodePoint(parseInt(escaped.slice(i + 2, i + 6), 16)); + i += 5; + continue; + } + const compact = { b: '\b', t: '\t', n: '\n', f: '\f', r: '\r', '"': '"', '\\': '\\' }[next]; + if (compact === undefined) throw new Error(`unsupported escape: \\${next}`); + out += compact; + i += 1; + } + return out; + }; + + const hostileInputs = ['\n', '\r', '\r\n', '\0', '\t', '\b', '\f', '\x1F', '\x7F', 'a\\b"c\nd']; + for (const input of hostileInputs) { + const escaped = escapeTomlDoubleQuotedString(input); + const tomlLine = `k = "${escaped}"`; + const quoted = tomlLine.slice(tomlLine.indexOf('"') + 1, tomlLine.lastIndexOf('"')); + assert.equal(unescapeTomlBasicString(quoted), input, `round-trip failed for ${JSON.stringify(input)}`); + } + }); + + // A newline in the target directory lands inside a single-quoted `echo '...'` argument + // in the persisted bash/zsh rc lines. POSIX single quotes preserve a literal newline as + // data — they do NOT terminate on a bare newline — so the whole `echo '...' >> ~/.bashrc` + // stays ONE shell command; the newline just makes the persisted PATH assignment a broken, + // two-line value inside .bashrc, not a second executable command. (Contrast: had the + // value been embedded unquoted or inside double quotes followed by an unescaped command + // separator, a newline COULD start a new command — that is not the case here.) + test('a newline in the target directory cannot start a new rc-file command', () => { + const command = laneCommand('persist', '/tmp/a\nmalicious', 'linux', 'bash'); + assert.equal(command, `echo 'export PATH="/tmp/a\nmalicious:$PATH"' >> ~/.bashrc`); + // The newline sits between the opening and closing `'` of the echo argument — it is + // literal data inside one quoted token, not a shell command terminator. + const singleQuoteSpan = command.slice(command.indexOf(`'`) + 1, command.lastIndexOf(`'`)); + assert.ok(singleQuoteSpan.includes('\n'), 'the newline must be inside the single-quoted region'); + }); + + test('keeps the existing apostrophe escaping', () => { + const command = laneCommand('persist', "/tmp/o'brien", 'linux', 'bash'); + assert.ok(command.includes(`'\\''`), 'bash lane must retain the POSIX single-quote escape sequence'); + }); + + test('escapes an apostrophe and a dollar in one path', () => { + const command = laneCommand('persist', "/tmp/o'b$(id)", 'linux', 'bash'); + assert.ok(command.includes(`'\\''`), 'must still contain the POSIX single-quote escape'); + assert.ok(command.includes('\\$('), 'must also contain the escaped dollar-paren'); + }); + + test('all three lanes escape the export line identically', () => { + const { escapedDir: token } = projectPathExportLine('/tmp/a$(id)b'); + const repairCommand = laneCommand('repair', '/tmp/a$(id)b', 'linux', 'posix'); + const persistCommand = laneCommand('persist', '/tmp/a$(id)b', 'linux', 'bash'); + const win32TargetDir = 'C:/a$(id)b'; + const win32Command = laneCommand('repair', win32TargetDir, 'win32', 'bash'); + // #3118: the win32 bash lane posix-normalizes targetDir before escaping it, so its token + // must be compared against the SAME (posix-normalized) input, not against the '/tmp/...' + // token above — those are two different inputs and legitimately escape differently. + const { escapedDir: win32Token } = projectPathExportLine(posixNormalize(win32TargetDir)); + assert.ok(repairCommand.includes(token), 'repair posix lane must use the same escaped token'); + assert.ok(persistCommand.includes(token), 'persist bash lane must use the same escaped token'); + assert.ok(win32Command.includes(win32Token), 'win32 bash lane must use the same escaped token'); + }); + + test('does not let a quote break out of the cmd lane', () => { + const { shellActions, actionLines } = projectPathActionProjection({ + mode: 'repair', + targetDir: 'C:/a"&calc&"b', + platform: 'win32', + }); + // `"` is a reserved character that cannot appear in any Windows path, so + // there is no valid command to suggest; the projection fails closed + // rather than emitting a cmd line whose quoting can be broken. + assert.deepEqual(shellActions, []); + assert.deepEqual(actionLines, []); + }); + + test('still projects win32 lanes for a legal path', () => { + const { shellActions } = projectPathActionProjection({ + mode: 'repair', + targetDir: 'C:/Users/dev/bin', + platform: 'win32', + }); + assert.equal(shellActions.length, 3); + assert.deepEqual(shellActions.map((a) => a.shell), ['powershell', 'cmd', 'bash']); + }); + + test('keeps the PowerShell doubling', () => { + assert.equal(escapePowerShellSingleQuoted("o'brien"), "o''brien"); + }); + + test('an absent target directory produces no actions', () => { + for (const targetDir of [null, undefined, '']) { + assert.deepEqual( + projectPathActionProjection({ mode: 'repair', targetDir, platform: 'linux' }), + { shellActions: [], actionLines: [], reason: PATH_ACTION_REASON.NO_TARGET_DIR }, + ); + } + }); + + test('reports why a win32 quote produced no actions', () => { + const result = projectPathActionProjection({ mode: 'repair', targetDir: 'C:/a"b', platform: 'win32' }); + assert.deepEqual(result.shellActions, []); + assert.equal(result.reason, PATH_ACTION_REASON.WIN32_RESERVED_QUOTE); + }); + + // Two empty results with different causes must stay distinguishable — that + // is the whole subject of epic #3051. + test('reports a missing target directory as a different cause', () => { + const result = projectPathActionProjection({ mode: 'repair', targetDir: null, platform: 'win32' }); + assert.equal(result.reason, PATH_ACTION_REASON.NO_TARGET_DIR); + }); + + test('a successful projection carries no reason', () => { + const result = projectPathActionProjection({ mode: 'repair', targetDir: '/usr/local/bin', platform: 'linux' }); + assert.equal(Object.prototype.hasOwnProperty.call(result, 'reason'), false); + }); + + test('the reason vocabulary is closed', () => { + assert.deepEqual(Object.keys(PATH_ACTION_REASON).sort(), ['NO_TARGET_DIR', 'WIN32_RESERVED_QUOTE']); + assert.ok(Object.isFrozen(PATH_ACTION_REASON)); + }); + + test('leaves the fish lane escaping unchanged', () => { + const command = laneCommand('persist', '/tmp/a$(id)b', 'linux', 'fish'); + assert.equal(command, `fish_add_path -- '/tmp/a$(id)b'`); + }); + + test('emits the fish end-of-options separator for an ordinary path', () => { + // #3118 review MINOR: a leading-dash `targetDir` (e.g. `-v`) is a legal + // directory name, but fish's argparse-based option scanning misparses an + // unseparated leading-dash token as a flag regardless of quoting — + // verified empirically against a real fish 4.8.1 install that + // `fish_add_path '-v'` fails ("No paths to add, not setting anything.") + // while `fish_add_path -- '-v'` succeeds. `--` is fish's standard + // end-of-options separator and is a no-op for ordinary paths. + const command = laneCommand('persist', '/tmp/x', 'linux', 'fish'); + assert.equal(command, `fish_add_path -- '/tmp/x'`); + }); + + test('an empty platform string falls back to the host', () => { + assert.equal( + formatManagedHookScriptToken('/x/y.js', { platform: '' }), + formatManagedHookScriptToken('/x/y.js'), + ); + }); + + test('projects no token off win32', () => { + assert.equal(formatManagedHookScriptToken('/x/y.js', { platform: 'linux' }), null); + }); + + test('projects a JSON-quoted posix-normalized token on win32', () => { + assert.equal( + formatManagedHookScriptToken('C:\\x\\y.js', { platform: 'win32' }), + JSON.stringify('C:/x/y.js'), + ); + }); + + test('returns no lines when called with no arguments', () => { + assert.deepEqual(renderShellActionLines(), []); + }); + + test('drops entries without a command', () => { + assert.deepEqual( + renderShellActionLines([null, { label: 'a' }, { label: 'b', command: 'c' }]), + ['b: c'], + ); + }); + + test('renders an unlabeled action as the bare command', () => { + assert.deepEqual(renderShellActionLines([{ label: null, command: 'x' }]), ['x']); + }); + + test('escapes a backslash-quote pair in the right order', () => { + // Input: a \ " b (a literal backslash immediately followed by a quote). + const input = ['a', '\\', '"', 'b'].join(''); + // Expected: backslash first doubles to two backslashes, THEN the quote + // gets its own backslash — so the quote ends up preceded by 3 backslashes. + const expected = ['a', '\\', '\\', '\\', '"', 'b'].join(''); + assert.equal(escapeTomlDoubleQuotedString(input), expected); + }); + + test('coerces non-string values', () => { + const escapers = [ + escapeTomlDoubleQuotedString, + escapePowerShellSingleQuoted, + escapePosixDoubleQuoted, + escapeSingleQuotedShellLiteral, + ]; + for (const escaper of escapers) { + for (const value of [null, undefined, 0, [], {}]) { + assert.doesNotThrow(() => escaper(value)); + assert.equal(typeof escaper(value), 'string'); + } + } + }); + + test('returns empty for an empty value', () => { + const escapers = [ + escapeTomlDoubleQuotedString, + escapePowerShellSingleQuoted, + escapePosixDoubleQuoted, + escapeSingleQuotedShellLiteral, + ]; + for (const escaper of escapers) { + assert.equal(escaper(''), ''); + } + }); + + describe('retryRenameSync (#3118)', () => { + afterEach(() => { + mock.restoreAll(); + }); + + test('rethrows when the rename cannot be retried to success', () => { + mock.method(fs, 'renameSync', () => { + const e = new Error('EPERM'); + e.code = 'EPERM'; + throw e; + }); + assert.throws(() => retryRenameSync('/a', '/b')); + }); + + test('returns silently on a successful rename', (t) => { + const dir = createTempDir('gsd-3118-rename-'); + t.after(() => cleanup(dir)); + const from = path.join(dir, 'source.txt'); + const to = path.join(dir, 'dest.txt'); + fs.writeFileSync(from, 'content'); + + retryRenameSync(from, to); + + assert.ok(fs.statSync(to).isFile()); + assert.equal(fs.existsSync(from), false); + }); + }); +}); diff --git a/tests/state-rebuild.test.cjs b/tests/state-rebuild.test.cjs index 03a5e6c1f..2ca691972 100644 --- a/tests/state-rebuild.test.cjs +++ b/tests/state-rebuild.test.cjs @@ -30,7 +30,6 @@ const fixedClock = Object.freeze({ nowIso: () => '2026-06-29T12:00:00.000Z', }); -const noProgress = () => null; // #3057 B1: `phaseInventoryProvider` returns a discriminated result, never a // bare array-or-null — `{ ok: true, phases: [] }` is the genuinely-empty // benign case ("nothing to reconcile"), distinct from `{ ok: false, reason }` @@ -39,7 +38,6 @@ const noProgress = () => null; const noPhases = () => ({ ok: true, phases: [] }); const baseDeps = Object.freeze({ - progressProvider: noProgress, clock: fixedClock, phaseInventoryProvider: noPhases, }); diff --git a/tests/state-transition.test.cjs b/tests/state-transition.test.cjs index 89b95e3b4..4d78531c0 100644 --- a/tests/state-transition.test.cjs +++ b/tests/state-transition.test.cjs @@ -17,6 +17,7 @@ const { FIELD_CLASSIFICATION, getFieldClassification, STATE_MD_SECTIONS, + sliceCurrentPositionSection, } = require('../gsd-core/bin/lib/state-transition.cjs'); const { stateExtractField } = require('../gsd-core/bin/lib/state-document.cjs'); @@ -26,8 +27,6 @@ const fixedClock = Object.freeze({ nowIso: () => '2026-06-27T12:00:00.000Z', }); -const noProgress = () => null; - describe('ADR-1769 substrate: field-classification table', () => { const allowedSources = new Set(['body', 'disk', 'external', 'curated', 'free']); const allowedPreservation = new Set([ @@ -140,7 +139,7 @@ describe('ADR-1769 Phase 1: beginPhase transition — tracer bullet', () => { const result = transitionCore( input, { kind: 'beginPhase', phaseNumber: 3, phaseName: 'Test Phase', planCount: 5 }, - { clock: fixedClock, progressProvider: noProgress }, + { clock: fixedClock }, ); assert.ok(result.updated.includes('Status'), `updated should include Status; got ${JSON.stringify(result.updated)}`); @@ -181,7 +180,7 @@ function firstTimeBody() { describe('ADR-1769 Phase 1: beginPhase first-time body field updates', () => { const intent = { kind: 'beginPhase', phaseNumber: 3, phaseName: 'Test Phase', planCount: 5 }; - const deps = { clock: fixedClock, progressProvider: noProgress }; + const deps = { clock: fixedClock }; test('updates Current Phase to N', () => { const result = transitionCore(firstTimeBody(), intent, deps); @@ -260,7 +259,7 @@ function resumeBody() { describe('ADR-1769 Phase 1: #3127 idempotency guard — resume path', () => { const intent = { kind: 'beginPhase', phaseNumber: 3, phaseName: 'Test Phase', planCount: 5 }; - const deps = { clock: fixedClock, progressProvider: noProgress }; + const deps = { clock: fixedClock }; test('Status is still refreshed on resume (Last Activity Date tracks execute-phase runs)', () => { const result = transitionCore(resumeBody(), intent, deps); @@ -299,7 +298,7 @@ describe('ADR-1769 Phase 1: #3127 idempotency guard — resume path', () => { describe('ADR-1769 Phase 1: Current Position section mutation (first-time begin)', () => { const intent = { kind: 'beginPhase', phaseNumber: 3, phaseName: 'Test Phase', planCount: 5 }; - const deps = { clock: fixedClock, progressProvider: noProgress }; + const deps = { clock: fixedClock }; test('Current Position Phase line reflects the new phase (EXECUTING)', () => { const result = transitionCore(firstTimeBody(), intent, deps); @@ -356,7 +355,7 @@ describe('ADR-1769 Phase 1: Current Position section mutation (first-time begin) describe('ADR-1769 Phase 1: Current Position section mutation (resume path)', () => { const intent = { kind: 'beginPhase', phaseNumber: 3, phaseName: 'Test Phase', planCount: 5 }; - const deps = { clock: fixedClock, progressProvider: noProgress }; + const deps = { clock: fixedClock }; test('Resume updates only the Last activity line in Current Position (preserves Plan, Phase, Status)', () => { const result = transitionCore(resumeBody(), intent, deps); @@ -372,7 +371,7 @@ describe('ADR-1769 Phase 1: Current Position section mutation (resume path)', () }); describe('ADR-1769 Phase 1: property tests (RULESET.TESTS.property-based-testing)', () => { - const deps = { clock: fixedClock, progressProvider: noProgress }; + const deps = { clock: fixedClock }; test('for any non-negative integer phaseNumber and any STATE.md body with a non-whitespace Status value, beginPhase produces content whose body Status carries "Executing Phase N"', () => { // Note: filters out whitespace-only statusSuffix because state-document.cjs's @@ -419,7 +418,7 @@ describe('ADR-1769 Phase 1: property tests (RULESET.TESTS.property-based-testing }); describe('ADR-1769 Phase 2: advancePlan transition', () => { - const deps = { clock: fixedClock, progressProvider: noProgress }; + const deps = { clock: fixedClock }; test('advances Current Plan from N to N+1 (legacy format)', () => { const input = [ @@ -483,7 +482,7 @@ describe('ADR-1769 Phase 2: advancePlan transition', () => { }); describe('ADR-1769 Phase 2: advancePlan with frontmatter (#1255 pattern — codex review)', () => { - const deps = { clock: fixedClock, progressProvider: noProgress }; + const deps = { clock: fixedClock }; test('advances plan correctly when STATE.md has YAML frontmatter (body Status not YAML status)', () => { const input = [ @@ -560,7 +559,7 @@ const ROADMAP_3_OF_5 = [ ].join('\n'); describe('ADR-1769 Phase 3: completePhase transition — body field updates', () => { - const deps = { clock: fixedClock, progressProvider: noProgress, roadmapProvider: () => ROADMAP_3_OF_5 }; + const deps = { clock: fixedClock, roadmapProvider: () => ROADMAP_3_OF_5 }; test('Current Phase advances to nextPhaseNum, preserving "of total" and appending the next name', () => { const intent = { @@ -645,7 +644,7 @@ describe('ADR-1769 Phase 3: completePhase transition — body field updates', () }); describe('ADR-1769 Phase 3: completePhase progress derivation (roadmap)', () => { - const deps = { clock: fixedClock, progressProvider: noProgress, roadmapProvider: () => ROADMAP_3_OF_5 }; + const deps = { clock: fixedClock, roadmapProvider: () => ROADMAP_3_OF_5 }; test('Completed Phases is re-derived from the roadmap progress table', () => { const result = transitionCore( @@ -667,7 +666,7 @@ describe('ADR-1769 Phase 3: completePhase progress derivation (roadmap)', () => }); test('when roadmapProvider yields null, existing Completed Phases / Progress are preserved (no crash)', () => { - const nullDeps = { clock: fixedClock, progressProvider: noProgress, roadmapProvider: () => null }; + const nullDeps = { clock: fixedClock, roadmapProvider: () => null }; const result = transitionCore( completePhaseBody(), { kind: 'completePhase', phaseNum: '3', nextPhaseNum: '4', nextPhaseName: null, isLastPhase: false, planCount: 3, summaryCount: 3 }, @@ -685,7 +684,7 @@ describe('ADR-1769 Phase 3: completePhase progress derivation (roadmap)', () => // 'updated' even when nothing changed. test('FAILURE path (roadmap unavailable): Completed Phases / Progress are NOT marked updated — left-as-is is distinguishable from recomputed', () => { - const nullDeps = { clock: fixedClock, progressProvider: noProgress, roadmapProvider: () => null }; + const nullDeps = { clock: fixedClock, roadmapProvider: () => null }; const result = transitionCore( completePhaseBody(), { kind: 'completePhase', phaseNum: '3', nextPhaseNum: '4', nextPhaseName: null, isLastPhase: false, planCount: 3, summaryCount: 3 }, @@ -716,7 +715,7 @@ describe('ADR-1769 Phase 3: completePhase progress derivation (roadmap)', () => }); describe('ADR-1769 Phase 3: completePhase edge cases', () => { - const deps = { clock: fixedClock, progressProvider: noProgress, roadmapProvider: () => ROADMAP_3_OF_5 }; + const deps = { clock: fixedClock, roadmapProvider: () => ROADMAP_3_OF_5 }; test('falls back to the "Phase:" field when "Current Phase:" is absent (stateReplaceFieldWithFallback)', () => { const input = [ @@ -813,7 +812,7 @@ function plannedPhaseBody() { } describe('ADR-1769 Phase 4: plannedPhase transition — body field updates', () => { - const deps = { clock: fixedClock, progressProvider: noProgress }; + const deps = { clock: fixedClock }; test('Status advances to "Ready to execute" when the existing value is a template default (Planning)', () => { const result = transitionCore(plannedPhaseBody(), { kind: 'plannedPhase', phaseNumber: 3, planCount: 4 }, deps); @@ -888,7 +887,7 @@ describe('ADR-1769 Phase 4: plannedPhase transition — body field updates', () }); describe('ADR-1769 Phase 4: milestoneSwitch transition — milestone reset', () => { - const deps = { clock: fixedClock, progressProvider: noProgress }; + const deps = { clock: fixedClock }; function milestoneBody() { return [ @@ -963,7 +962,7 @@ describe('ADR-1769 Phase 4: milestoneSwitch transition — milestone reset', () // ADR-1769 Phase 5: milestoneComplete describe('ADR-1769 Phase 5: milestoneComplete transition — closure write', () => { - const deps = { clock: fixedClock, progressProvider: noProgress }; + const deps = { clock: fixedClock }; const intent = { kind: 'milestoneComplete', version: 'v1.0', nextMilestoneCommand: '/gsd:new-milestone' }; function preCloseBody() { @@ -1095,7 +1094,7 @@ describe('ADR-1769 Phase 5: milestoneComplete transition — closure write', () // ADR-1769 Phase 6: patch describe('ADR-1769 Phase 6: patch transition — field updates', () => { - const deps = { clock: fixedClock, progressProvider: noProgress }; + const deps = { clock: fixedClock }; test('applies each patched field and reports the updated set', () => { const input = [ @@ -1148,7 +1147,7 @@ describe('ADR-1769 Phase 6: patch transition — field updates', () => { // ADR-1769 Phase 7: update, prune, sync describe('ADR-1769 Phase 7: update transition — single body field', () => { - const deps = { clock: fixedClock, progressProvider: noProgress }; + const deps = { clock: fixedClock }; test('replaces a body field and reports updated:true', () => { const input = '# Project State\n\n**Status:** Planning\n**Current Plan:** 2\n'; @@ -1173,7 +1172,7 @@ describe('ADR-1769 Phase 7: update transition — single body field', () => { }); describe('ADR-1769 Phase 7: prune transition — section pruning', () => { - const deps = { clock: fixedClock, progressProvider: noProgress }; + const deps = { clock: fixedClock }; test('archives Decisions entries at or below the cutoff phase', () => { const input = [ @@ -1223,7 +1222,7 @@ describe('ADR-1769 Phase 7: prune transition — section pruning', () => { }); describe('ADR-1769 Phase 7: sync transition — body writes + #1761', () => { - const deps = { clock: fixedClock, progressProvider: noProgress }; + const deps = { clock: fixedClock }; test('updates Total Plans in Phase + Progress bar + Last Activity when bounded', () => { const input = [ @@ -1660,3 +1659,375 @@ describe('bug #21 — STATE.md template must carry YAML frontmatter', () => { }); }); } + +// ──────────────────────────────────────────────────────────────────────── +// #3118: sliceCurrentPositionSection — locator characterization tests. +// ──────────────────────────────────────────────────────────────────────── + +describe('sliceCurrentPositionSection (#3118)', () => { + test('slices the section up to the following heading', () => { + const body = [ + '# Project State', + '', + '## Current Position', + '', + 'Phase: 3 (Test Phase) — EXECUTING', + '', + '## Accumulated Context', + '', + '- A decision worth keeping.', + '', + ].join('\n'); + const result = sliceCurrentPositionSection(body); + assert.ok(result.includes('Phase: 3 (Test Phase) — EXECUTING')); + assert.ok(!result.includes('A decision worth keeping')); + }); + + test('slices to end of document for a trailing section', () => { + const body = [ + '# Project State', + '', + '## Accumulated Context', + '', + '- Earlier decision.', + '', + '## Current Position', + '', + 'Phase: 5 (Final Phase) — EXECUTING', + '', + ].join('\n'); + const result = sliceCurrentPositionSection(body); + assert.ok(result.includes('Phase: 5 (Final Phase) — EXECUTING')); + }); + + test('returns null when the section is absent', () => { + const body = [ + '# Project State', + '', + '## Accumulated Context', + '', + '- Some decision.', + '', + '## Deferred Items', + '', + '- Something deferred.', + '', + ].join('\n'); + assert.strictEqual(sliceCurrentPositionSection(body), null); + }); + + test('matches the heading case- and space-insensitively', () => { + const body = [ + '# Project State', + '', + '## CURRENT POSITION', + '', + 'Phase: 2 — EXECUTING', + '', + ].join('\n'); + assert.notStrictEqual(sliceCurrentPositionSection(body), null); + }); + + test('ignores a Current Position heading inside a code fence', () => { + // The locator is fence-aware via `tokenizeHeadings` — a `##` line inside + // a ``` fence is not a real heading, so this document has zero *real* + // Current Position headings. + const body = [ + '# Project State', + '', + '## Accumulated Context', + '', + '```markdown', + '## Current Position', + '', + 'Phase: 9 — should not be seen', + '```', + '', + ].join('\n'); + assert.strictEqual(sliceCurrentPositionSection(body), null); + }); + + test('distinguishes an empty section from an absent one', () => { + // An empty section and an absent one are different answers, and a caller + // that folds them together reintroduces the collapse this epic removes. + const body = [ + '# Project State', + '', + '## Current Position', + '## Accumulated Context', + '', + '- A decision.', + '', + ].join('\n'); + const result = sliceCurrentPositionSection(body); + assert.strictEqual(typeof result, 'string'); + assert.notStrictEqual(result, null); + assert.strictEqual(result.trim(), ''); + }); + + test('does not match an H3 Current Position', () => { + const body = [ + '# Project State', + '', + '### Current Position', + '', + 'Phase: 2 — EXECUTING', + '', + ].join('\n'); + assert.strictEqual(sliceCurrentPositionSection(body), null); + }); + + test('slices the first Current Position when the document has two', () => { + // `findIndex` picks the first heading match and nothing pinned that + // behavior down before this test. + const body = [ + '# Project State', + '', + '## Current Position', + '', + 'Phase: 3 — FIRST OCCURRENCE', + '', + '## Accumulated Context', + '', + '- unrelated', + '', + '## Current Position', + '', + 'Phase: 9 — SECOND OCCURRENCE', + '', + ].join('\n'); + const result = sliceCurrentPositionSection(body); + assert.ok(result.includes('FIRST OCCURRENCE')); + assert.ok(!result.includes('SECOND OCCURRENCE')); + }); + + test('returns null for an empty document', () => { + assert.strictEqual(sliceCurrentPositionSection(''), null); + }); + + test('slices a CRLF document identically', () => { + // Only `\n` in a regex/split is the recurring CRLF defect class in this + // repo (#1658 and successors) — verify the CRLF fixture, normalized back + // to LF, matches the LF fixture's result byte-for-byte. + const lines = [ + '# Project State', + '', + '## Current Position', + '', + 'Phase: 3 (Test Phase) — EXECUTING', + '', + '## Accumulated Context', + '', + '- A decision worth keeping.', + '', + ]; + const lfResult = sliceCurrentPositionSection(lines.join('\n')); + const crlfResult = sliceCurrentPositionSection(lines.join('\r\n')); + // Strict form (#3118): normalize CRLF->LF and require exact equality with + // the LF result. The looser `.replace(/\r/g, '')` form (previously used + // here) strips ALL `\r` bytes including a stray unpaired trailing `\r` + // left by the pre-fix `end = hs[j].offset - 1` slice — that loose + // assertion is what let the CRLF-slice-defect ship undetected. + assert.strictEqual(crlfResult.replace(/\r\n/g, '\n'), lfResult); + }); + + test('returns an empty string when the section is empty and the next heading follows immediately', () => { + const body = ['# STATE', '', '## Current Position', '## Next Section', 'content'].join('\n'); + const result = sliceCurrentPositionSection(body); + assert.strictEqual(result, ''); + }); + + test('does not duplicate bytes when a transition mutates an empty adjacent section', () => { + // Regression for #3118 review MAJOR: `locateCurrentPosition`'s newline + // walk-back could land `end` before `start` when the section is empty + // and the next heading follows with no blank line between. Every + // mutator's `body.slice(0, start) + sectionBody + body.slice(end)` + // reassembly then duplicated the bytes in the inverted `[end, start)` + // range — a spurious blank line (LF) or `\r\n` (CRLF) inserted into + // STATE.md on every transition. + const lfBody = ['# STATE', '', '## Current Position', '## Next Section', 'content'].join('\n'); + const lfResult = transitionCore( + lfBody, + { kind: 'beginPhase', phaseNumber: 3, phaseName: null, planCount: null }, + { clock: fixedClock }, + ); + assert.ok( + !lfResult.content.includes('## Current Position\n\n## Next Section'), + `expected no inserted blank line; got ${JSON.stringify(lfResult.content)}`, + ); + assert.strictEqual( + lfResult.content, + '# STATE\n\n## Current Position\n## Next Section\ncontent', + ); + assert.strictEqual(lfResult.content.length, lfBody.length); + + const crlfBody = ['# STATE', '', '## Current Position', '## Next Section', 'content'].join('\r\n'); + const crlfResult = transitionCore( + crlfBody, + { kind: 'beginPhase', phaseNumber: 3, phaseName: null, planCount: null }, + { clock: fixedClock }, + ); + assert.ok( + !crlfResult.content.includes('## Current Position\r\n\r\n## Next Section'), + `expected no inserted CRLF; got ${JSON.stringify(crlfResult.content)}`, + ); + assert.strictEqual( + crlfResult.content, + '# STATE\r\n\r\n## Current Position\r\n## Next Section\r\ncontent', + ); + assert.strictEqual(crlfResult.content.length, crlfBody.length); + }); +}); + +// ──────────────────────────────────────────────────────────────────────── +// #3118: deps.progressProvider is a required StateTransitionDeps field with +// 33 supply sites and zero call sites, and is being removed. Prove it +// behaviorally: no transition ever invokes it. +// ──────────────────────────────────────────────────────────────────────── + +describe('state transitions do not consult a progress provider (#3118)', () => { + test('no transition invokes deps.progressProvider', () => { + // An exploding stub is the behavioral form of "this field is inert"; + // asserting the declaration is absent would be source-grep theater. + const exploding = () => { throw new Error('progressProvider must never be called'); }; + const clock = fixedClock; + + assert.doesNotThrow(() => transitionCore( + firstTimeBody(), + { kind: 'beginPhase', phaseNumber: 3, phaseName: 'Test Phase', planCount: 5 }, + { clock, progressProvider: exploding }, + )); + + assert.doesNotThrow(() => transitionCore( + [ + '# Project State', + '', + '**Current Plan:** 02', + '**Total Plans in Phase:** 05', + '**Status:** Executing Phase 3', + '**Last Activity:** 2026-06-26', + '', + '## Current Position', + '', + 'Plan: 2 of 5', + 'Status: Executing Phase 3', + '', + ].join('\n'), + { kind: 'advancePlan' }, + { clock, progressProvider: exploding }, + )); + + assert.doesNotThrow(() => transitionCore( + completePhaseBody(), + { kind: 'completePhase', phaseNum: '3', nextPhaseNum: '4', nextPhaseName: 'Design Phase', isLastPhase: false, planCount: 3, summaryCount: 3 }, + { clock, progressProvider: exploding, roadmapProvider: () => ROADMAP_3_OF_5 }, + )); + + assert.doesNotThrow(() => transitionCore( + plannedPhaseBody(), + { kind: 'plannedPhase', phaseNumber: 3, planCount: 4 }, + { clock, progressProvider: exploding }, + )); + + const milestoneBody = [ + '---', + 'gsd_state_version: 1.0', + 'milestone: v1.0', + 'milestone_name: Old Milestone', + 'status: executing', + 'current_phase: "3"', + 'progress:', + ' total_phases: 5', + ' completed_phases: 2', + ' percent: 40', + '---', + '', + '# Project State', + '', + '## Current Position', + '', + 'Phase: 3 — EXECUTING', + 'Plan: 2 of 5', + 'Status: Executing Phase 3', + 'Last activity: 2026-06-20 — mid-flight', + '', + ].join('\n'); + assert.doesNotThrow(() => transitionCore( + milestoneBody, + { kind: 'milestoneSwitch', version: 'v2.0', name: 'New Milestone' }, + { clock, progressProvider: exploding }, + )); + + const preCloseBody = [ + '# Project State', + '', + '**Status:** Executing Phase 5', + '**Last Activity:** 2026-06-20', + '**Last Activity Description:** mid-flight', + '', + '## Current Position', + '', + 'Phase: 5 — EXECUTING', + 'Plan: 2 of 3', + 'Status: Executing Phase 5', + 'Last activity: 2026-06-20 — running', + '', + '## Operator Next Steps', + '', + '- Re-run /gsd:complete-milestone v1.0', + '', + ].join('\n'); + assert.doesNotThrow(() => transitionCore( + preCloseBody, + { kind: 'milestoneComplete', version: 'v1.0', nextMilestoneCommand: '/gsd:new-milestone' }, + { clock, progressProvider: exploding }, + )); + + assert.doesNotThrow(() => transitionCore( + [ + '# Project State', + '', + '**Status:** Planning', + '**Current Plan:** 2', + '**Total Plans in Phase:** 5', + '', + ].join('\n'), + { kind: 'patch', patches: { Status: 'Paused', 'Current Plan': '3' } }, + { clock, progressProvider: exploding }, + )); + + assert.doesNotThrow(() => transitionCore( + '# Project State\n\n**Status:** Planning\n**Current Plan:** 2\n', + { kind: 'update', field: 'Current Plan', value: '3' }, + { clock, progressProvider: exploding }, + )); + + assert.doesNotThrow(() => transitionCore( + [ + '# Session State', + '', + '## Decisions', + '', + '- [Phase 1]: Old', + '- [Phase 3]: Older', + '- [Phase 9]: Recent', + '', + ].join('\n'), + { kind: 'prune', cutoff: 7 }, + { clock, progressProvider: exploding }, + )); + + assert.doesNotThrow(() => transitionCore( + [ + '# Project State', + '', + '**Total Plans in Phase:** 2', + '**Last Activity:** 2026-06-20', + '**Progress:** [████░░░░░░] 40%', + '', + ].join('\n'), + { kind: 'sync', totalPlansInPhase: 5, percent: 60 }, + { clock, progressProvider: exploding }, + )); + }); +});