9faacc0c153a88f939ef07ced74d56dc468f0708
3 Commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
Tom Boucher
|
9faacc0c15 |
test(#3148): bound the long tail and delete the unbounded-spawn allowlist (#3192)
* test(#3148): bound the long tail and delete the allowlist Migrates the final 170 unbounded sync spawn sites across 49 files, then removes the allowlist entirely. local/no-unbounded-spawn now runs with no exemption surface across tests/**: there is no file to add a name to. drift-detection's throw-native git() helper routes to gitOrThrow -- bare runGit would have taken 16 call sites quiet on failure. commands.test.cjs has two independently-scoped runGsdTools/runCli helpers, one already bounded and one not; they are kept distinct rather than unified, the same trap as the two same-named git() helpers in Wave 1. runNpm's bound was erasable. Its options spread callerOptions after the defaults, so an explicit timeout:undefined silently dropped the 180000ms bound -- the rule flagged it and was right; it was not a false positive. Fixed by destructuring with a default, with a test that fails when the default is removed. Two sites stay on a raw spawn with an explicit timeout because the seam cannot express them: one needs shell:true for npm.cmd on Windows, one redirects stdout to a real fd. Both are the rule's own documented second option, not an escape from it. Closure verified rather than asserted: the derivation scan reports 0 unbounded spawn helpers and 0 unbounded direct git call sites, and a temporary file carrying an unbounded spawn still errors with the allowlist gone. Closes #3064. * test(#3148): close a hole in the guard's own eslint-disable ban The ban listed only the top level of tests/, so it was blind to 37 .cjs files under tests/helpers, qa, observability, fixtures and dispatch. With the allowlist deleted this test is the sole remaining way to detect someone silencing the rule inline, so the gap was load-bearing: a nested file could carry an unbounded spawn plus an eslint-disable and pass everything. Proven before and after. A probe planted under tests/helpers with both was invisible to the guard and clean under eslint; after making the listing recursive the guard fails on it. The scanned set goes from 771 files to 808. Pre-existing since the guard shipped, but this wave is what promoted it to sole defense, so it is fixed here rather than filed. Also converts the last hand-rolled throw check to throwIfFailed and the last re-derived legacy shape to compose toLegacyResult, which makes the epic's none-remain claim true rather than nearly true. toLegacyResult itself is not widened -- eight callers depend on its shape and one consumer does not justify changing a shared contract. * fix(#3148): correct seam incoherence at the bound and a slow review-lane error path Two real failures from the remote runner, both fixed at the cause. The seam could return outcome TIMED_OUT together with exitCode 0. At the exact bound spawnSync reports ETIMEDOUT while the child has already exited with a real status, and toSeamResult classified on the error code while passing status straight through -- an incoherent pair its own boundary test was written to catch, and did. A status that is not null is direct evidence the child exited on its own, so it now decides the outcome before the error-code branches run. process-seam.cjs was deliberately untouched by every earlier wave; this is a defect in the module itself, kept surgical, with a unit test that fails against the old logic. review-lane with an unknown subcommand fell through to its usage error only after loading the capability registry and building a per-lane plan, which spawns one child process per lane -- up to twelve. The error path took ~1288ms instead of ~119ms, and under bench load it outran a caller's spawn timeout and was killed before writing anything, which is the empty stdout and stderr CI saw. It now fails fast before any of that work begins. This is the epic's first production change. It is user-facing, so it carries a changeset rather than a no-changelog label. * test(#3148): replace a real-race timeout test with a deterministic one E9 raced git rev-parse against a 1ms bound and assumed git always lost. On a warm container git finishes first, spawnSync returns status 0 with no error at all, the seam correctly classifies EXITED, and gitOrThrow correctly does not throw -- so the test failed on both lanes. A probe confirms a genuine timeout always carries status null, so this was never the seam misbehaving. Raising the bound would only lengthen the odds, which is the same defect with better luck. The test now drives gitOrThrow against a stubbed runGit that returns a synthetic TIMED_OUT result, so it asserts exactly what it always meant to -- that a timeout propagates as a throw -- with no timing dependence. Five consecutive runs are identical where the old one varied. I wrote this test in Wave 0; it is a real-race test by construction and CLAUDE.md says to replace those rather than re-run them. * chore(#3148): backfill changeset PR number 3192 --------- Co-authored-by: sim <sim@local> |
||
|
Tom Boucher
|
0e6fa2e2cf |
enhance(#3118): close the dead injectables and the shell projection follow-on — Wave 4 (#3124)
* 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="<dir>:$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. |
||
|
Tom Boucher
|
51b6e3c35c |
test(#2136): add localToday regression + mirror fake/inline clock stubs
Adds a dedicated regression (tests/fix-2136-clock-local-today.test.cjs): - realClock.localToday() returns the LOCAL calendar day under a pinned instant + TZ (America/Chicago → 2020-06-14, the issue's repro instant). - realClock.today() is unchanged (UTC) — internal/cosmetic stamps stay UTC. - localToday === today on a UTC host (no spurious divergence). - makeFakeClock mirrors localToday (drop-in Clock substitute). - field-level: a 'sync' transition with a split clock (today≠localToday) writes the LOCAL day into Last Activity, proving the wiring uses localToday. Mirrors localToday() in the fake clock helper and the inline fixedClock stubs in state-transition.test.cjs / state-rebuild.test.cjs (the Clock interface now requires it). |