release: v0.10.1 — CLI security ship set + backlog - #237
Merged
Conversation
betterleaks >=1.1.2 writes the literal `null`, not `[]`, to its JSON report
for a clean scan via the `dir`/`git` subcommands both implementations use.
`JSON.parse("null")` is `null` and `Array.isArray(null)` is false, so a clean
scan fell into the sable-o4k stale-binary version-mismatch branch and printed
a spurious warning for every clean file scanned.
Return an empty result set for a null report, before the array check, so the
stale-binary guard still fires for genuinely non-array output.
Extract the Python parse into `_parse_report` to mirror Node's `parseResults`,
so both are directly testable without invoking the binary.
Reported by @justJackjon in #217, with an accurate root-cause analysis.
Note the older `betterleaks detect --report-format json` form writes `[]`, so
the null only appears via the subcommand form we actually invoke.
`rafter secrets --diff <ref>` with no changed files printed its "No files changed since <ref>" status line to stdout and then called outputScanResults, which printed a second success line. Under --json that human line landed on stdout ahead of the JSON payload, so the documented output contract (stdout = JSON, status = stderr) was violated and the output would not parse. Route the status line to stderr in both implementations and let outputScanResults own the single stdout success line. Python additionally exited early without emitting any result envelope, so a zero-file `--json` scan produced no JSON at all; it now mirrors Node. Also drop a pre-existing unused `execSync` import in scan.ts.
The MCP server registers three resources (mcp/server.ts:313-332): rafter://config, rafter://policy, and rafter://docs. rafter://docs was added later and no doc was updated, so four places still advertised two. Surfaced while triaging external PR #215, which mirrored the stale count into recipes/cursor.md by following what was already written.
`agent status` probes `betterleaks` on PATH before falling through to the legacy-gitleaks branch, so the assertion only holds when betterleaks is absent. The test passed on CI and failed on any dev box with betterleaks installed. Run the CLI with PATH pointed at an empty directory so the fallthrough is reached deterministically.
* docs: expand Cursor recipe to match platform recipe standard Fill out Prerequisites, MCP tools reference with usage examples, Troubleshooting, and Uninstall so recipes/cursor.md aligns with gemini-cli.md for #33. Co-authored-by: Cursor <cursoragent@cursor.com> * Update cursor.md * Update cursor.md docs: address review - add rafter://docs, scan_secrets engines --------- Co-authored-by: Cursor <cursoragent@cursor.com>
…report fix(scanners): treat betterleaks null report as empty result set (#217)
The suite only triggered on `pull_request` into `prod`, so every contributor PR targeting `main` merged with no automated verification — tests ran later, at release time, when main was promoted to prod. #215, #216 and #218 all sat mergeable with an empty status-check rollup. Add `main` to the trigger and gate the jobs: PRs into prod always run (that is the release gate, unchanged), PRs into main run only for outside contributions. Our own work — Rome-1's PRs, or any branch living in the Raftersecurity repo — is reviewed and tested locally before it is pushed, so re-running the full matrix would only burn runner minutes. Uses `pull_request`, not `pull_request_target`: fork PRs run with a read-only token and no secrets. Tests that need RAFTER_API_KEY already skip when it is absent, so a fork PR gets a clean green rather than a spurious failure.
…l10k) (#220) * fix: retry transient report-read failures during scan polling (sable-l10k) A paying customer's GitHub Actions run died on: ::error::Rafter scan poll failed: HTTP 500 — Failed to fetch report from storage: Object not found A report is not durable the instant a scan flips to completed, so a poll can hit a 5xx on a scan that is perfectly readable seconds later. Every poll path treated any non-2xx as fatal and exited immediately — while the transport-error branch three lines above already retried. That asymmetry was the bug: a curl-level failure was survivable, an HTTP-level one was not. Reproduced in a real GitHub Actions run against a mock backend that injects one transient 500 into an otherwise healthy poll sequence; the run died on a scan that completed on the very next poll. All three surfaces now share one contract (documented in CLI_SPEC.md): - 5xx / 408 / 404 mid-poll and transport errors are transient. Retried up to 5 consecutive times with 2s/4s/8s/16s backoff; the counter resets on any successful poll. - Other 4xx (401/403/429) are not retried. A 404 on the FIRST poll is still a genuinely missing scan, exit 2. - On giving up, the message names the scan id, the `rafter get <id>` retry, and the dashboard. Storage-layer wording survives as supporting detail rather than as the whole explanation. The Python loop was additionally calling .json() on the 500 body, reading no status, falling out of the loop and writing the error payload out as if it were results — a silent wrong answer rather than a loud failure. Also here, from the security review of this diff: - Strip newlines and cap length on server-controlled `.error` before it reaches `::error::`/`::warning::`. A body containing a newline could forge workflow commands (`::add-mask::`, `::stop-commands::`). Same class as the pre-existing sinks; this diff widened it from 2 to 6. - Add --connect-timeout/--max-time to curl and an axios timeout, so a hung server cannot stall inside a request that the retry loop only checks between attempts. - Source the action's `status` output from the poll step when the results step never runs, so the new `unreadable` status reaches consumers instead of an empty string. Coverage: 8 vitest + 9 pytest cases pinning both halves of the contract, plus two end-to-end CI jobs that drive the composite action against a localhost mock backend — no API key, no credit spend. * test: restore real timers inside each test (Node 18 afterEach hang) Leaving vitest's fake timers installed past the end of the test body hangs the afterEach hook on Node 18 — the cross-platform matrix caught it on both ubuntu and macos. Matches the in-test restore the existing scan-remote tests already use; the afterEach restore stays as a fallback for failed assertions. * fix: address adversarial review of the poll-retry change (sable-l10k) An independent reviewer was asked to argue against merging #220. It found a regression I introduced, a contract my own spec text got wrong, and a test suite that did not test the mechanism it existed to protect. All real. REGRESSION I INTRODUCED — a failed results fetch reported `completed`. Sourcing the action's `status` output from `steps.results.outputs.status || steps.poll.outputs.status` meant that when the results fetch exhausted its retries, the empty results output fell back to the poll step's `completed`. A consumer gating on `status == 'completed'` saw a clean scan, and the artifact upload published the error body as rafter-results.json. Exactly the silent-wrong-answer class this PR set out to remove. Both give-up paths in fetch_results now record `status=unreadable`, and the artifact upload is gated on the results step rather than the poll step. THE TESTS DID NOT TEST THE BACKOFF. Setting BASE_BACKOFF_MS to 0 left all 8 vitest cases green, and the Python fixture patched out time.sleep entirely, making the schedule unobservable by construction. Backoff IS the fix — retrying five times inside a millisecond gives an eventually-consistent store no time to converge. Both suites now pin the exact sequence (10s poll, then 2/4/8/16), and both verify the consecutive counter resets on a successful poll. THE ACTION VIOLATED THE CONTRACT THIS PR WROTE. CLI_SPEC said transport errors were retried on the same budget as 5xx; in the action they retried on a flat 10s and never touched the counter, so an unreachable backend burned the whole timeout and then reported "scan did not complete within N minutes" — a timeout message for a DNS failure. They now share the budget and exit with status=unreachable. The spec is also corrected where the action genuinely cannot match the CLI: it has no first-poll concept, so every 404 there is lag (bounded by the 5-failure budget, not the full timeout). UNBOUNDED CLI LOOP. The consecutive counter was constructed per call, and resets on success, so a backend alternating 200/500 forever never exhausted it — and the CLI has no wall-clock deadline. Added a total budget (20 per invocation) that does not reset, keeping the useful reset-on-success semantics without the hole. THE REMEDY WE RECOMMEND WAS THE ONE PATH NOT FIXED. The give-up message says "retry with rafter get <id>", which re-enters at the first poll — which had no retry, so it died on the raw storage jargon we had just stopped printing. Worse for 404: the loop retried it five times, then recommended a command that reports "not found" with a different exit code. The first poll now retries transient 5xx while still treating 404 as fatal. PARITY BREAKS (this repo requires strict Node/Python parity): - Python accepted only 200; Node accepts any 2xx. On a 202 they returned opposite outcomes — Python exit 1, Node exit 0 with an empty payload. - Node wrote the retry notice into the ora spinner, which renders nothing on a non-TTY. The diagnostic was invisible in CI, the one place it matters. It goes to stderr now, matching Python. - Node retried ANY error lacking a `.response`, including TypeErrors thrown from our own code. Narrowed to genuine HTTP-layer errors. - Python raised PollGaveUpError for non-transient statuses too, conflating "tried five times" with "did not try". Split out PollFatalError. - Neither runtime truncated server error text; both now cap it like the action does. ALSO: sanitized the three remaining unsanitized `.error`/response echo sites in action.yml, guarded TIMEOUT_MINUTES before bash arithmetic evaluates it, and restored the remaining-budget denominator the poll log line had lost. COVERAGE for the two things a "simplification" would silently break: a CI job injecting a mid-poll 404, a CI job failing the results fetch specifically, and six new assertions in the action.yml drift detector (404 in the transient set, transport errors counted, give-up message actionable, both unreadable writes, artifact gating, exponential backoff). Each was mutation-tested to confirm it fails when the property is removed. CHANGELOG documents the two behavior changes this ships: timeout-minutes is now a real wall-clock deadline rather than a poll count, and Python's non-transient mid-poll failures now exit 1 instead of 0. * fix: nested error body crashed the retry it was supposed to trigger (sable-l10k) A verification pass over the previous fix commit found a defect in code that commit introduced, plus three places where a fix was thinner than it looked. THE BLOCKER — I added a truncate() helper that assumed the server's error field is a string. A backend answering {"error": {"message": "..."}} on a 500 made it call .split() on a dict (Python: AttributeError, uncaught, straight to a traceback) and .replace() on an object (Node: "s.replace is not a function", and crucially NO retry — 2 calls, not 5). So the one shape of error body most likely to appear on a real 500 turned a retryable failure into an immediate hard failure with a nonsense message, inside the very code meant to make transient failures survivable. Both runtimes now coerce before truncating. THE TOTAL-CAP TEST DID NOT TEST THE TOTAL CAP. Deleting the total clause from FailureBudget.exhausted left all 14 Node tests green: the mock queue drained, axios returned undefined, and the resulting TypeError was converted by the loop's own catch into the exact exit code the test asserted. The test now uses an endless flapping mock with a hard ceiling, so a missing cap fails loudly and immediately rather than passing on an unrelated crash. Verified by mutation both ways. (Python's equivalent was already genuine.) THE RECOMMENDED REMEDY STILL DID NOT RETRY. The last commit made the first poll retry, but `rafter get <id>` WITHOUT --interactive takes a different path entirely — a single un-retried request in both runtimes. So the command the give-up message recommends was still defeated by the failure that produced the message. It now shares the same retry budget. THE MESSAGE LIED ABOUT ITS OWN ATTEMPT COUNT. Both runtimes hardcoded "after 5 attempts" while exhaustion can equally come from the total budget of 20 — a flapping backend produced "after 5 attempts" following 20 failures over four minutes. It now reports the real count. Relatedly, the CLI blamed the report ("could not read the report … retry with rafter get") even when nothing ever reached the server; it now distinguishes unreachable-API from unreadable-report, which the action already did. ALSO: - Narrowed isTransientPollError: Node's own TypeError [ERR_INVALID_CHAR] carries .code, so an API key read from a file with a trailing newline was retried five times and reported as a flaky backend. Dropped .code; real axios timeouts still retry via .request/.isAxiosError. - Validate the server-supplied scan_id before it reaches $GITHUB_OUTPUT. A newline there forges step outputs, including status=completed. The CHANGELOG claimed sanitization was complete "at every site" when this one was open; the claim is now true rather than trimmed. - Three more drift assertions (sanitization present, TIMEOUT_MINUTES guarded, scan_id validated) — the first of which was itself broken on first write and only caught by mutation-testing it. - Tests for the nested-error crash, the truncation cap, the real attempt count, and the unreachable-API message, in both runtimes. --------- Co-authored-by: achebe <hello@rafter.so>
…10k (#221) The assignment was to explain why 8 of 17 checks skipped on PR #220. The skip pattern is real, but it is not why the bug reached a customer. Testing that claim rather than assuming it is what turned up the rest. WOULD ANY EXISTING CHECK HAVE CAUGHT IT, IF IT HAD RUN? No. Checked out main at 0996492 (the buggy tree) and ran the entire suite against it: 2065 passed, and the only failures were three files that fail for environmental reasons here and are unrelated to polling. tests/scan-remote.test.ts and test_scan_remote.py both cover the poll loop — with the HTTP layer mocked, and neither ever injected a non-2xx mid-poll. They pass against the bug. And nothing executed github-action/action.yml at all, which is the file the customer's error came from. test-action.yml drives the ROOT action.yml, a different action that scans locally. Of the jobs in test-github-action.yml, two run hand-copied reimplementations of the action's bash (their own headers admit the duplication) and one greps the YAML as text. So the workflow that fires on github-action/** changes ran, and still executed none of the code. The gap was COVERAGE, and #220 closed it. The skip pattern would not have mattered. Three things found on the way there do: 1. publish-python had no `needs:`. publish-node has needed the test jobs since it was written; the Python half published to PyPI in parallel with the tests, ungated. A red suite blocked the npm release and shipped the PyPI one anyway — in a dual-implementation product where the two versions must match, that diverges them at the registry, the one place users cannot see it. Now gated. (publish.yaml also runs no pytest anywhere; filed separately.) 2. backend-api rendered identically whether it tested the backend or nothing. Its only real step is gated on RAFTER_API_KEY, which has never been set on this repo, so "backend-api ✓" has always meant "checked out and built". It now says so, loudly, in the log and the step summary. 3. test-node and test-python were skipped on internal PRs into main. On #220 — which changed both clients — neither ran. They now run on every PR. The premise that our own work is tested locally first is also weaker than it looks: this repo has test files that fail locally for environmental reasons, so "green on my machine" is not a signal anyone can act on. Cost is ~4 minutes of wall clock (234s and 100s, in parallel). The expensive part, the 6-way cross-platform grid with 3 macOS runners, stays gated — this reverses part of #219 narrowly and deliberately, not wholesale. Also established, not changed here: main has no branch protection at all. The only ruleset targets refs/heads/prod and contains no required-status-checks rule, so no check is required anywhere and a red PR can merge into main. That is a policy call, not a workflow fix. Co-authored-by: achebe <hello@rafter.so>
requests' SessionRedirectMixin.rebuild_auth strips Authorization on a host change and leaves arbitrary custom headers intact; axios/follow-redirects does the same. So `x-api-key` rides a 302 to whatever host it points at. Nothing in this CLI needs to follow a redirect, so nothing does any more. PRE-EXISTING, not introduced by #220. The retry loop added there raises exposure from one transmission to as many as five, which is why it matters more today than it did last week, but it is not the cause. The action's curl paths were never affected — verified, no -L or --location on any of the eight curl invocations in github-action/action.yml. Every authenticated call site, not just the poll path: 14 in Node behind a shared `apiClient` (axios instance, maxRedirects: 0), 13 in Python behind api_get/api_post (allow_redirects forced False, so no call site can opt back in). Deliberately left alone: update-checker's npm registry call and the Slack/Discord webhook posts, none of which carry the key. WHAT THE SECURITY REVIEW CAUGHT, and it would have shipped a broken CLI: API/API_BASE end in "/" and 11 call sites concatenated "/static/...", building https://rafter.so/api//static/scan. Production answers that with a 308 to the single-slash form. It worked only because the client followed the redirect — so refusing redirects turned every core command into a hard failure. Verified against the live API: the double-slash URL 308s, the single-slash one reaches the endpoint. Both runtimes now build URLs through apiUrl()/api_url(), and a test in each fails on any `${API}/` or `{API_BASE}/` construction. Every other test mocks the transport, which is why nothing caught this. Also from that review: - The message told users to point --rafter-url at the final URL. That flag does not exist in the CLI — it is a GitHub Action input. Removed the instruction rather than shipping advice nobody can follow. - A redirect Location is attacker-controlled if the endpoint is. Header values cannot carry CR/LF but ESC is legal, so the raw value could rewrite the user's terminal. Both runtimes strip non-printables and cap at 200 chars, asserted with an ANSI sequence in the fixture. - The source-scanning guards only caught the most literal bypass. They now also match the .request() form and flag any second axios.create() / requests.Session() built outside the api utils. - The Node test shim made axios and apiClient the same mock, so a regression to bare axios would still have passed. create() now returns a distinct object and the tests watch that instance; mutation-tested by reverting one call site to bare axios, which the guard catches. A refused redirect now explains itself instead of surfacing a bare 302. Co-authored-by: achebe <hello@rafter.so>
python/tests/ executed in exactly one workflow in this repo — test-comprehensive.yml — and that workflow is not on the release path. publish.yaml had no Python test job at all, and validate-release.yml's test-build ran `python -m build` without pytest, so the pre-release gate asserted the Python package compiles and never that it works. A Python-only regression reached PyPI green, and PyPI is not somewhere you can quietly unship from. - publish.yaml: add test-python, mirroring test-comprehensive.yml's job. It runs in parallel with test-node, so it costs no wall clock the release was not already spending. - publish.yaml: gate BOTH publish jobs on it. The two registries publish from one push and version parity is enforced elsewhere, so a suite that goes red after npm has already published leaves the two runtimes at different versions on the two indexes. The divergence is the failure mode whichever half breaks. - publish.yaml: give test-node a Python toolchain. The node release path looked safe because publish-node has a `needs:` clause, but the job it needed was quietly weaker than the same job in test-comprehensive.yml: tests/cross-runtime-parity.test.ts gates its whole describe block on `python3 -c "import typer"` and describe.skip is silent, so all 40 parity assertions were skipped and the suite reported green. Verified by stubbing python3: "40 skipped", exit 0. Those tests are what enforce the dual-implementation contract — the ones a release least wants to skip. - validate-release.yml: run pytest in test-build. ~80s. Note for PR #221, which touches the same job: it adds `needs: [test-node, test-package]` to publish-python to stop a red NODE suite shipping the PyPI package. That is a different hole. Keep both sets of needs if the two land together. Co-authored-by: mayor <hello@rafter.so>
The composite action's results step coerced every jq failure into findings_count=0: a body that was not JSON, a 200 carrying an error object, or a parseable payload with no vulnerabilities key passed every severity threshold and rendered "No security findings detected". Validate the shape first; on failure exit 1 with status=unreadable, an actionable message, and no count outputs at all. Once the shape holds the counts cannot fail, so the fallbacks are removed rather than moved. rafter issues create from-scan (Node and Python) had the same shape: data.vulnerabilities || [] turned a processing or failed scan, an error object, or a keyless payload into "No findings to create issues for". Both now refuse with exit 1 and name the scan status. An empty array is still a clean result. Regression coverage: two end-to-end CI jobs drive the real action against the mock backend (a 3-way matrix of unreadable shapes asserting the build fails with status=unreadable and an EMPTY findings-count, plus an exact counts job asserting 3/1/1/0/1); three drift assertions; unit tests on the real functions in both runtimes. Every new guard was mutation-verified by hand: restoring the old behaviour fails the tests that exist to catch it.
…ped jobs (sable-5d3q, sable-wl3p) (#226) * ci: fail when the suite shrinks or a file's tests all skip; name skipped jobs (sable-5d3q, sable-wl3p) A green run must have run something. sable-cazq was 40 parity tests describe.skip'd on a missing interpreter and the release path exited 0; a total-count floor would not have caught it, a per-file rule does. .github/scripts/test_floor.py reads the vitest JSON or pytest JUnit report and fails the job when executed tests fall below a floor (~95% of today's counts, lowered in the same PR that removes tests) or any file's tests all skipped, and lists every skipped test by name in the step summary. Wired into test-node and test-python on the PR path and the release path. The gate job's skip of the extended matrix on internal PRs now emits a ::warning:: and a summary section naming each job that did not run, so a grey check reads as skipped rather than passed. * ci: derive the skipped-jobs notice from the workflow file instead of a hand-typed list (it omitted e2e-node within the hour)
…229) The action's threshold-eval and PR-comment-tip tests each carried their own transcription of the bash they tested, so they could pass in full while action.yml was broken. The logic now lives once in github-action/lib/severity.sh, sourced by both action.yml steps and by both tests. Behaviour unchanged. A new end-to-end job runs the real action with findings above the threshold and asserts status=completed AND outcome=failure, which proves the counts reach the gate; drift check 17 asserts action.yml sources the library in both steps and carries no inline copy, so the tests cannot be silently detached again. node/tests/issues.test.ts mirrored four modules (dedup, issue-builder, from-text, from-scan) and tested the mirrors; one had already drifted. The copies are removed and every describe imports the shipped function. Three source functions gained `export` for that. Every guard was mutated by hand: each mutation fails named tests that were untouched by the same mutation before this change.
…rf-6pqx, rf-3rsj, sable-c6an, rf-7dda, rf-er8a, rf-fuwy) (#235) * fix(risk-rules): newline is a statement separator + heredoc-aware sanitizer (rf-6pqx, rf-3rsj, #230) A newline was tokenized as whitespace, so a multi-line Bash command was one segment. When the first word was a text-exec (echo/printf/grep), every later line was redacted as its operand, so `echo x\nrm -rf /` classified LOW and bypassed the CRITICAL hard-block, the deny-list, the approval list, and approve-dangerous mode. Reported independently by kerckhoffs (se-ijzs) and the external reporter J/JJB1 (se-odv4 finding 1); confirmed in both runtimes. The one-line "add \n to CHAIN_OPS" is a NO-OP: the tokenizer eats \n in the whitespace branch before the operator branch ever runs. The real fix: - Exclude \n/\r from the whitespace skip so a newline reaches the operator branch and ends the segment like ";" (both runtimes). Matching still runs over the whole reassembled string, so `curl x | bash` is unaffected. - Strip heredoc BODIES that a command consumes as DATA (`cat > doc.md <<EOF`, `grep <<EOF`) BEFORE tokenizing. Without this the newline split makes ordinary documentation writes classify CRITICAL (rf-3rsj) — the same over-block public #230 reported. A body EXECUTED by a shell/eval owner (`bash <<EOF`, `sh <<'X'`, `sudo bash <<EOF`) is KEPT and scanned, so it stays blocked (se-y6vo). bash terminator semantics: exact match for plain `<<`, tab-strip for `<<-`. - Delete `\<newline>` line continuations before parsing, so `r\<NL>m -rf /` (which bash runs as `rm -rf /`) forms the token `rm` and is caught; preserved inside single quotes (se-y6vo). - Fail closed on an unterminated quote: `$'a\'b'` desyncs the sanitizer's quote state and swallowed a trailing payload; an unterminated parse now matches the raw string instead (se-y6vo). Test: rf-6pqx-newline-heredoc-battery.json (29 cases, both directions) is run byte-for-byte by both runtimes (node/tests/risk-rules-newline-heredoc.test.ts, python/tests/test_risk_rules_newline_heredoc.py) so a clean result in one cannot hide a bug in the other. Existing suites unchanged: 137 node + 43 python pass. Co-designed with kerckhoffs (se-ijzs, shared battery) and achebe (#230 heredoc). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01BbKepZbxosd5TEJas9Mmhg * fix(risk-rules): two heredoc cases that were MORE permissive than main Sits directly on fix/rf-6pqx-newline-heredoc @ db9a809. Cherry-pick or merge; it touches only the two functions named below and adds four cases to the shared battery. Measured on that branch against origin/main, both runtimes: case main #234 cat <<< "hello" > a.txt \n rm -rf / critical LOW cat <<EOF | bash \n rm -rf / \n EOF critical LOW In both, the sanitized string handed to the matcher had the `rm -rf /` DELETED — not down-tiered, removed. 1. `<<<` WAS BEING READ AS `<<`. HEREDOC_START has no boundary guards, so on `cat <<< "hello"` it matches starting at the SECOND `<`: delimiter `hello`, no terminator line ever found, every following line stripped. Any here-string followed by a newline therefore suppressed everything after it, hard block included. This is a general suppression primitive rather than a missed case, and `<<<` is ordinary shell. Fixed with `(?<!<)` / `(?!<)`. Verified the guards leave real introducers matching: `cat > a.md <<'EOF'` still matches at the right offset with delimiter EOF. 2. A HEREDOC PIPED INTO A SHELL IS EXECUTED. `heredocOwnerExecutes` reads only the text BEFORE the introducer, so it never sees `| bash` and `cat` looks like a plain writer. main got this right by accident — it scanned the body as bare words — so the heredoc pass has to keep it deliberately. Added `heredocOutputPipedToShell`, which walks the pipeline stages after the introducer with the same env-assignment and TAIL_WRAPPERS skipping the owner check already uses, so `| sudo bash` resolves too. Keeping a body is the safe direction: it can only over-block, and only for a shape that should block anyway. Four cases added to rf-6pqx-newline-heredoc-battery.json, which both runtimes read byte-for-byte, so neither can go green while the other is broken. Each mutation-verified: reverting either fix turns exactly two battery cases red. Not addressed here, filed as sable-c6an (P0, pre-existing on main and on this branch alike, so not a blocker for this one): `bash -c "$(echo rm -rf /)"` and `echo "rm -rf /" | bash` both classify low, because a word that is entirely a command substitution leaves the piece's literal text empty and the `-c` branch reads only that text. Same root as (2) — whether this segment's OUTPUT becomes code. A fix is on fix/rf-3rsj-heredoc-tokenizer @ 2e2d53b. Suites on this branch after the change: node 269 + 33 pass, python 304 pass. My own 18-case heredoc battery goes 12/18 -> 14/18 against it; the four still red are all sable-c6an. * fix(risk-rules): fold the sable-c6an bypass fix into the ship lineage Mayor caught an integration gap: sable-c6an's fix existed ONLY on fix/rf-3rsj-heredoc-tokenizer @ 2e2d53b, which is not an ancestor of this branch. The ship lineage was db9a809 plus the two-regression fix, and neither carried it — so merging #234 as it stood would have shipped the heredoc work and left the live P0 behind. This branch is now the assembled artifact: goldwasser's db9a809 + the two permissive regressions + sable-c6an, all three verified together rather than separately. The fix ports the same distinction the tokenizer branch used, onto this branch's structure: codeCarrying — this segment RUNS a command string it was handed executesOutput — this segment's STDOUT becomes code somewhere else (`… | bash`, or a substitution used as a -c script) Three sites follow from it. The `-c` branch now sanitizes the piece's SUBSTITUTIONS as well as its literal text — a word that is entirely a substitution leaves `text` empty, which is how the whole script argument was vanishing. Text-exec operands and quoted prose arguments are code when the segment's output is executed. And `sanitize` now keeps the operator that ends each segment, so a segment can be asked whether the next one is a shell. The two must stay separate or `bash -c "echo 'rm -rf /'"` breaks: that runs echo, and echo prints. It has an existing test asserting low, it is in the battery as an explicit guard row, and it still passes. VERIFIED ON THE ASSEMBLED ARTIFACT, not on the pieces: - all nine cases mayor asked for: six critical (2 regressions + 4 c6an), #230 documentation still low, print-only script still low, rf-6pqx newline still critical - kerckhoffs's differential, BOTH runtimes: CLEAN (70 generated rows, candidate never weaker than main except a pure data heredoc) - kerckhoffs's battery, BOTH runtimes: ALL PASS - shared battery grown 29 -> 37; every added row proven RED on db9a809 and green here, except the print-only guard row which must stay low on both - repo suites: node 320 pass, python 304 pass The differential is the check that would have caught the original regression, and it is now the one I would gate the merge on rather than any battery — a battery only asks the questions someone thought to ask. * test(risk-rules): commit the differential-vs-main gate; stop tracking the node_modules symlink The stop-ship regressions on db9a809 passed the battery because its only here-string case was single-line. A battery only asks the questions someone thought to ask; the differential asks a generated corpus and compares against main, so an unanticipated permissive move is caught structurally. - python/tests/test_rf6pqx_differential.py runs the gate against the origin/main baseline (via git show) and FAILS LOUDLY if the baseline is unobtainable. - rf6pqx_differential.py / rf6pqx-differential.ts are the committed corpus+compare (authored by kerckhoffs), runnable in both runtimes. - Drop the accidentally-tracked node/node_modules symlink. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01BbKepZbxosd5TEJas9Mmhg * fix(agent): init writes a resolvable absolute hook command; verify executes the gate (rf-er8a, rf-fuwy) rf-er8a — `agent init` wrote a bare `rafter hook pretool`. Under the documented `npx @rafter-security/cli agent init --all` there is no global `rafter`, so the command exits 127; agents block only on exit 2, so the gate is inert while settings.json looks configured. init now pins BOTH the node interpreter and the resolved CLI entrypoint (dist/index.js carries `#!/usr/bin/env node`, so an absolute entrypoint alone still fails where node is off PATH), across every integration write path, and refuses to print a clean "initialized" when the written hook is in an npx cache or does not execute-and-enforce. rf-fuwy — `agent verify` reported an inert gate as healthy, byte-identical to a working one, because it only checked that the hook was CONFIGURED. It now EXECUTES each configured command via `sh -c` with a benign payload (expect allow) and the canonical CRITICAL payload (expect deny); 127 / non-JSON / wrong decision is a hard FAIL. A hook deliberately disabled (RAFTER_DISABLE_HOOKS / global) is reported as its own state, not a failure. The opt-in --probe no longer spawns verify's own entrypoint (which always resolved) — it runs the configured command. The misleading "resolve like Claude Code" comment is corrected. Tests: node/tests/verify-hook-liveness.test.ts (positive/negative/127 controls for the executor). End-to-end: init writes an absolute command that verify reports "installed and enforcing" even with rafter off PATH — the exact state that previously read as a healthy but inert gate. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01BbKepZbxosd5TEJas9Mmhg * fix(hook): a project .env cannot disable the hook (rf-7dda) dotenv.config() at CLI startup loads $CWD/.env — the cloned repo's own file when rafter runs inside an agent hook. dotenv does not override an already-set var but DOES introduce an unset one, so a repo shipping RAFTER_DISABLE_HOOKS=1 (or any RAFTER_DISABLE_* / RAFTER_HOOK_* value) could switch off the victim's command policy and secret scanning — defeating hook-control's contract that the disable signal is honored only from the machine owner's environment. guardSecurityEnvFromDotenv() runs dotenv then drops any RAFTER_DISABLE_* / RAFTER_HOOK_* var that was not already in the real environment. Owner values and legitimate .env keys (RAFTER_API_KEY, …) are untouched. Python does not load a cwd .env on the hook path; a regression test pins that. Verified: battery goes red on the unguarded sequence, green with the guard, in both runtimes (parity). se-80vx / rf-7dda. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01WSumMwjhyvqi8qdZkuQrGD * test(risk-rules): make the committed node differential actually run, and floor the battery Four gaps on the ship branch, all of the same shape: a check that is present is not a check that runs. 1. THE NODE DIFFERENTIAL WAS NEVER INVOKED. `node/tests/rf6pqx-differential.ts` is committed next to its Python twin, but vitest collects `*.test.ts` and nothing wrapped it — so the repo had two differentials on disk and one running, which from a file listing is indistinguishable from having both. Wrapped it in a real test that mirrors test_rf6pqx_differential.py, including the rule that an unobtainable baseline FAILS rather than skips. Verified non-vacuous: pointed at db9a809's classifier it reports all 14 permissive regressions and goes red; on this branch it is CLEAN. 2. THE BATTERY HAD DRIFTED INTO TWO LISTS. The repo's JSON and the copy in the rig working dir had both reached 37 cases with 7 DIFFERING — each looked complete. The repo file is now the union at 44 (gaining `eval "$(…)"`, the sudo-sh pipe, the backtick c6an form and both redirect-target guards), and both unit suites read it byte-for-byte. 3. THE BATTERY HAD NO FLOOR. It is the gate that catches a MISSING fix — the differential structurally cannot, because a fix absent on both sides is not a permissive move, which is exactly how a differential ran CLEAN against a branch that had lost sable-c6an. That makes its contents load-bearing, so classifier_battery_floor.py asserts it has not shrunk AND still gates both directions: want-critical rows catch a missing fix, want-low rows stop a fix being bought with an over-block. #230 was an over-block report, so a battery with no low rows would have been happy to ship it. Non-vacuous both ways: truncating the battery or raising either floor fails it. 4. A PARITY GAP I CREATED. `node/tests/risk-rules-heredoc.test.ts` landed here without its Python mirror — swept in by the same `git add -A` that committed the node_modules symlink. Mirror restored, 18 tests each side. Also closes the ignore gap behind that symlink: `.gitignore` had `node_modules/` with a trailing slash, which matches DIRECTORIES ONLY, so a symlink walked straight past it. Both forms listed now, so the accident cannot recur — f1132ad removed the file but left the hole open. * ci: fetch main so the differential gate can actually run #235 is red on exactly the failure the gate was designed to produce: could not obtain main baseline for the differential gate: fatal: invalid object name 'main'. That is the gate working — it FAILS rather than skips when the baseline is missing, because a differential that silently skips is a vacuous check. But actions/checkout fetches only the PR ref, so `origin/main` is never in the clone and the gate could not have passed in CI at all. It was green locally, where main is always present, which is why nobody saw it until the branch ran. A depth-1 fetch is enough — the gate reads one blob, not history — so this costs a second rather than a full-history clone. Reproduced both halves against a shallow clone: `git show origin/main:python/rafter_cli/core/risk_rules.py` exits 128 before the fetch and prints the 506-line baseline after it. Added to test-node and test-python, the two jobs that run the gate. * fix(agent): tests assert rf-er8a's absolute-command contract; verify retries a transient spawn (test-node CI) The rf-er8a change (init writes an absolute `<node> <entrypoint> hook …` command instead of a bare `rafter hook …`) correctly broke five existing tests that asserted the old bare spelling, and my verify-hook-liveness test asserted an exact exit status that flaked under CI process pressure. Both are fixed so test-node goes green — the classifier and gate tests were already passing. - agent-commands / cursor-deep-support / platform-integration: assert the resolvable-absolute contract (command ends with `hook <sub>` and its first token is an absolute path) instead of `=== "rafter hook pretool"`, and match the dedup filters on the `hook <sub>` tail. This tests the property rf-er8a guarantees, not the old string. - runConfiguredHook: retry a transient spawn failure (EAGAIN under load returned a null status in CI, so the deny case got 0 while allow/127 got null). A resource hiccup must not be reported as an inert gate — a false alarm is worse than a slow check. - verify-hook-liveness: assert the DECISION (deny/allow/none), which carries the meaning, rather than the exact exit code. Local: node 210 + the 7 previously-failing tests now pass (agent-commands 81, cursor-deep 13, platform-integration 77, verify-hook-liveness 4); tsc clean. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01BbKepZbxosd5TEJas9Mmhg * fix(agent): retry the hook probe on a signal kill too, and make the liveness tests retry (achebe review) Follow-up to 63aa475, from achebe's measured review: - runConfiguredHook retried only on `result.error`, but a SIGKILL (which the OOM killer sends under the same process pressure that produces the EAGAIN this retry exists for) sets `result.signal` with a null status and no `error`, so it fell straight through and was reported as an inert gate — the exact false alarm, by another route. Retry now fires on `result.error || result.signal`. A persistent failure still returns decision:null, so a genuinely inert hook is still reported inert (no fail-open). - The liveness tests spawn real subprocesses and are inherently sensitive to process pressure; a per-test `retry: 3` keeps a transient spawn hiccup from flaking the security gate's own tests red without masking a real failure (which reproduces on every attempt). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01BbKepZbxosd5TEJas9Mmhg * test(agent): drop the liveness test-level retry — the production retry carries it (achebe review) Reverting the `retry: 3` added in ab0f695. My justification ("a retry can't mask a real failure, which reproduces every attempt") only holds for a DETERMINISTIC failure; for an intermittent one — exactly the failure mode of code spawning real subprocesses — a retry turns a 1-in-4 race green ~99% of the time, a vacuous pass on the very test that asserts the security gate is alive. And it is now redundant: runConfiguredHook retries the transient spawn error/signal itself (same fix), so the production code carries the process-pressure case the test-retry was added for. Keeping both leaves only the weaker mechanism, able to hide a future intermittent real failure. If the test flakes after the production retry, that is signal about runConfiguredHook, and we want to hear it. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01BbKepZbxosd5TEJas9Mmhg * fix(exec): --force no longer skips approval; approval needs a person at a TTY; add --dry-run (rf-ss67) `rafter agent exec --force "<quoted cmd>"` ran any HIGH-tier command unprompted: the PreToolUse hook classified the quoted argument as prose (risk-rules "unrecognized evaluator" limitation), and exec then skipped its own approval prompt because of --force. Reported in the secbolt audit (se-ezvc / se-6g2q). The approval model is now: only a person at an interactive terminal can approve, because every other channel — a flag, an env var, a piped "yes" — can be produced by the agent whose command is being gated. - --force is a hidden no-op kept so old invocations parse; it prints a notice and falls into the normal approval flow. - A command that requires approval is prompted only when stdin is a TTY; otherwise it is denied (exit 1) with a message saying to run it at a terminal or have the machine owner adjust commandPolicy in ~/.rafter/config.json. A piped "yes" is therefore not an approval. - --dry-run, advertised in three shipped docs but absent from exec, now exists: prints the verdict, risk tier and reason, runs nothing; exit 0 allowed, 1 blocked, 2 requires approval. - The documented `-- <command>` form is accepted (variadic argument); words are re-quoted (shellQuote / shlex.join) so the classifier evaluates the string the shell will run. Both runtimes. Docs: CLI_SPEC agent exec section, skill guardrails and cli-reference (the --force "ack flag" advice is gone), CHANGELOG. Tests: the file mode is the witness in every approval test — the command must not have run. Node: forced high-risk command denied and not executed; --force still parses; piped yes is not an approval; dry-run allowed/approval /blocked exit codes with nothing executed; `--` form; joinCommandParts quoting. Python: the same seven. Residual (filed as a follow-up): the hook still cannot see through `rafter agent exec "<cmd>"` — that lives in the sanitizer #235 rewrites — and a pty wrapper could fake a terminal. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01CKZyh4yEZBJnrjmoyPeSpG * test(exec): assert the denial line itself, not the notice that also names a terminal With the TTY gate removed by hand, the forced-approval tests still passed: the --force notice mentions "interactive terminal" too, and an EOFError on input() happened to exit 1. They now assert "Command denied: approval needs an interactive terminal" and a clean stderr, so removing the gate fails them by name in both runtimes. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01CKZyh4yEZBJnrjmoyPeSpG * chore: remove the inert node/.gitignore decoy It contained `node/node_modules`. Patterns in a nested .gitignore resolve relative to that file's own directory, so it matched node/node/node_modules — a path that does not exist. It never ignored anything. On ccd54a5 it was the replacement for achebe's working root-.gitignore fix and did not work: fresh repo, that OID's ignore files, a real symlink at node/node_modules -> git check-ignore said NOT IGNORED. The root fix (both forms, e90c8c5) is back and check-ignore attributes the match to .gitignore:17 — never to the nested file. Deleting it because an inert guard is worse than no guard: its only remaining function is to look like protection and stop the next person checking whether there is any. * chore(release): bump to v0.10.1 The bump lives in this PR rather than in a separate commit on main so that merging the ship set yields a releasable main in one step. main-into-prod is what fires publish, and npm rejects a republish of an existing version — so a main left at 0.10.0 would disclose the fixes and publish nothing. #233 touches neither version file, so this does not conflict with it in either merge order. --------- Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
validate-versions runs only on PRs into prod, so nothing before #237 could catch this. It requires each ClawHub skill manifest's own 'version:' line to match package.json. BOTH manifests needed it, not just the one the gate named. The gate loops over node/resources/rafter-security-skill.md and python/rafter_cli/resources/rafter-security-skill.md and exits on the FIRST mismatch, so the python one was invisible in the failure output and would have turned the check red again on the next run. Not touched, because this gate does not cover them: the eight SKILL.md manifests under resources/skills/ (rafter and rafter-code-review at 0.7.0, rafter-secure-design and rafter-skill-review at 0.1.0, in both runtimes). They version independently of the package. A gate that checks two manifests and not ten is a gate with a blind spot; recurrence is tracked separately.
test-build failed on #237 with: Error: Command failed: git show origin/main:node/src/core/risk-rules.ts The rf-6pqx differential fails loudly when main's classifier is unobtainable — by design, because a differential that silently skips is a vacuous gate. But actions/checkout fetches only the PR ref, so origin/main does not exist in this job and the gate cannot run at all. Same defect class as the ClawHub version desync fixed in 28e0095: #235 added this exact fetch to test-comprehensive.yml and not to validate-release.yml, and validate-release only runs on PRs into prod — so no PR into main could ever surface it. This ADDS gate coverage rather than relaxing it: without the fetch the check is red and evaluates nothing; with it the differential actually runs. On a main-into-prod PR the head IS main, so it evaluates to no permissive move, which is the correct answer rather than a skipped one.
…ite job the differential baseline (#238) Two separate things blocking #237, both larger than they first looked. 1. THE VERSION GATE NAMED ONE FILE BECAUSE IT EXITS AT THE FIRST. validate-versions loops over TWO manifests and `exit 1`s on the first mismatch, so it reported only node/resources/rafter-security-skill.md. python/rafter_cli/resources/rafter-security-skill.md was equally stale at 0.10.0 and would have failed the very next run. Both bumped to 0.10.1; gate simulated over both files, OK on each. Four more manifests carry a version and are NOT in the loop (rafter/SKILL.md and rafter-code-review at 0.7.0, rafter-secure-design and rafter-skill-review at 0.1.0, each duplicated node/python). Left alone deliberately — they are not release-versioned — but a gate that checks two of six is a gate with a blind spot, and prod history already contains "fix(release): bump ClawHub skill resource versions to 0.8.9". Recurrence is the finding; bead to follow. 2. FIVE OTHER CHECKS WERE FAILING, ALL OF THEM MINE. test-build and all four cross-platform legs failed on tests/rf6pqx-differential.test.ts: Command failed: git show origin/main:node/src/core/risk-rules.ts fatal: invalid object name 'origin/main'. The differential FAILS rather than skips when the baseline is missing, which is right — but that makes the baseline a precondition of every job that runs the suite, and I had added the fetch to exactly the two jobs whose failure I happened to be looking at. So I swept all four workflows for jobs running a FULL suite instead of patching the instances again: test-comprehensive test-node already had it test-comprehensive test-python already had it test-comprehensive cross-platform ADDED validate-release test-build ADDED publish test-node ADDED publish test-python ADDED The last two are the ones that matter most: publish.yaml runs the full node and python suites, and publish-node/publish-python are gated on them. Without this the release would have passed every check on #237 and then FAILED AT THE MOMENT ROME MERGED — the worst place to find it. e2e-node, secret-detection-accuracy and backend-api run single targeted files, so they never collect the differential and need nothing. Checked rather than assumed. Local: the differential and battery suites pass in both runtimes.
vitest@4.1.0 declares engines.node ^20.0.0 || ^22.0.0 || >=24.0.0. The Node 18 matrix leg was running the suite on a runner that does not support that platform. It did not refuse outright — it ran, reported 89 files passed and 1 failed, and none of that said anything about the shipped code. A leg claiming Node 18 coverage while running an unsupported runner is a check that cannot do what it claims. engines in package.json is NOT touched. It stays >=18 because the BUILT ARTIFACT does work on Node 18 (achebe measured 200/200 against dist), so the declaration remains honest. Dropping engines on this evidence would have been acting on a false cause. Real Node 18 coverage, if wanted, is a smoke test of the built artifact under 18 — that is what users actually run, and it is the only Node 18 signal that would mean anything. Not added here; tracked separately. Note the failing assertion this surfaced is a SEPARATE defect and is not fixed by this commit: the liveness fixture uses a hook that never reads stdin, so the payload write can EPIPE under process pressure. Measured 40 failures/960 with that fixture versus 0/960 with one that consumes stdin — and Node 24, a supported runner, failed it MORE than Node 18. Tracked on its own bead.
This was referenced Sep 9, 2026
Closed
Rome-1
changed the title
Raftersecurity
approved these changes
Sep 9, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Release 0.10.1 — the security ship set plus the six-week backlog. Merging this fires the npm/PyPI publish.
Why this PR exists at all
prodis the publish trigger; pushes tomaindo not publish.prodis 13 commits behind and the last release was 2026-07-29. Everything below has been sitting unshipped.What ships
The security set (squashed onto main as
6d6aa33):bash -c "$(echo rm -rf /)"classified low. Two tokens, no policy file, no clone. Pre-existing and live in published 0.10.0.envdisabled the PreToolUse hooknpxquickstart installed a hook that could not run (exit 127 → warning, not block)agent verify, even--probe, reported that dead gate as healthyagent exec --forceskipped approval entirely;--forceis now deprecated and no longer skips.rafter.ymlcould lower the machine owner's command policy; the global is now a floorPlus the backlog already on main, including
efefff0"stop sending the API key across redirects",3856a9d"an unreadable scan report is not a clean scan", and7e58d4c(the polling retry that reached Action users only because the Action pins@main).Verification
Gates committed in this release, not just run once: a differential-vs-main, a 44-case battery with a floor (a differential is structurally blind to a missing fix — that is how a clean differential once ran against a branch missing sable-c6an), and node/python parity.
Independently verified at the artifact level by four parties. The mayor re-ran 11 python and 8 node behavioural cases against every candidate head, because the branch name was wrong three times during assembly:
Red controls confirmed: the same harness returns the broken answers against pre-fix trees, so it distinguishes fixed from broken rather than passing regardless.
The full comprehensive matrix runs on this PR (it is gated to PRs into
prod, which is the release gate) and was additionally dispatched against the branch beforehand.Known and deliberately NOT fixed here
bash <<< 'rm -rf /'classifies high, not critical. Pre-existing, bounded (high still requires approval), deliberately out of scope.rafter agent execas an evaluator.cursor-deep-support.test.tsasserts a substring that GitHub's checkout path happens to supply. Green for the wrong reason; not user-facing.After merging
Publish fires on push to
prod. Verify the artifact, not the workflow exit code:npm view @rafter-security/cli versionmust actually read0.10.1.