From 1c6b7b09db1c8d56d937226543a3a5e97caed932 Mon Sep 17 00:00:00 2001 From: dormouse-bot <287024035+dormouse-bot@users.noreply.github.com> Date: Wed, 23 Sep 2026 13:17:37 +0000 Subject: [PATCH 1/3] fix(security-audit): lift the deciding lines above the truncation point MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Run 35842217451 composed a 226,302-character report and posted 31,662 of it. What reached the issue was `VERDICT: INCONCLUSIVE` naming `audit-ci-secrets.md` and a note telling the reader to read its `UNVERIFIABLE` checks — while that domain's one `UNVERIFIABLE` line, and the whole of `## Application security`, `## Hosted accounts` and `## Summary`, sat past the cut. The clamp keeps the head, and the lines that decide a verdict are not positionally predictable, so lift them: every fragment's verdict line plus its `UNVERIFIABLE`, `FAIL:`, `BLOCKER` and `WARNING` lines are emitted ahead of the report, where the clamp cannot reach them. Matched at line start after an optional heading or bullet marker, so the `FAIL IF` vocabulary the fragments are written in is not lifted as a finding. --- .github/workflows/security-audit.yaml | 36 +++++++++++++++++++++++++- docs/specs/security-audit.md | 1 + docs/specs/security-audit.rationale.md | 2 ++ scripts/security-audit.test.mjs | 28 ++++++++++++++++++-- scripts/spec-word-budgets.json | 2 +- 5 files changed, 65 insertions(+), 4 deletions(-) diff --git a/.github/workflows/security-audit.yaml b/.github/workflows/security-audit.yaml index 701aa0a6f..ed3bef6f8 100644 --- a/.github/workflows/security-audit.yaml +++ b/.github/workflows/security-audit.yaml @@ -493,6 +493,30 @@ jobs: echo "- **The audit wrote no verdict.** \`audit-status.txt\` was absent, empty, or not \`PASS\`/\`FAIL\`, so the run ended without deciding. Start at the report's own markers: \`UNVERIFIABLE\` is a check the agent could not determine, \`_Incomplete …_\` heads a domain that was cut off mid-report, and a domain section reading \`_No report …_\` is one that never reported — which is what an expired wait deadline looks like." >> "$NOTES" fi + # The notes above index the report by marker, and the clamp below + # keeps the head — so on a report too long to post, the markers they + # send the reader to are the part that gets cut. Run 35842217451 + # posted `VERDICT: INCONCLUSIVE` naming `audit-ci-secrets.md`, and + # its one `UNVERIFIABLE` line, along with two whole later domains + # and their verdicts, fell past 32,000 characters of a 226,302- + # character body. Lift every line that carries a verdict or a + # non-passing finding into the head, where the clamp cannot reach + # it; the full sections still follow for whatever survives. + # + # Matched at line start after an optional heading or bullet marker, + # which is what keeps `### FAIL IF results` and every `- PASS:` + # bullet quoting a **FAIL IF** clause out of the lift. `FAIL:` takes + # its colon for the same reason; a domain-level failure arrives as + # `VERDICT: FAIL` and is caught by the `VERDICT:` alternative. + DECIDING=$(mktemp) + # Unquoted on purpose: `AUDIT_FRAGMENTS` is a filename list. + # shellcheck disable=SC2086 + for f in $AUDIT_FRAGMENTS; do + [ -s "$f" ] || continue + grep -hE '^[[:space:]]*(#+[[:space:]]*)?([-*][[:space:]]*)?(VERDICT:|FAIL:|UNVERIFIABLE|BLOCKER|WARNING)' "$f" | + cut -c1-500 | sed "s|^|- \`$f\`: |" + done > "$DECIDING" + if [ "$STATUS" = "FAIL" ]; then TITLE="[security-audit] FAIL on $(date -u +%Y-%m-%d)" HEADLINE="Audit failed at $DATE." @@ -516,6 +540,16 @@ jobs: echo "- **Inconclusive for a reason this step could not name.** That is itself a bug in the reporting step." echo fi + if [ -s "$DECIDING" ]; then + echo "### Lines that decided this verdict" + echo + echo "_Lifted out of the fragments so truncation cannot cut them. Each domain's full section follows for as far as the body reaches._" + echo + head -n 40 "$DECIDING" + LIFTED=$(wc -l < "$DECIDING") + [ "$LIFTED" -gt 40 ] && echo "- _…and $((LIFTED - 40)) more; read them in the transcript._" + echo + fi if [ -s audit-report.md ]; then cat audit-report.md else @@ -557,7 +591,7 @@ jobs: done fi } > audit-comment.md - rm -f "$NOTES" + rm -f "$NOTES" "$DECIDING" # Truncate before posting, non-fatally. See docs/specs/security-audit.md -> "Outcomes and reporting". node scripts/clamp-issue-body.mjs audit-comment.md \ diff --git a/docs/specs/security-audit.md b/docs/specs/security-audit.md index 996b04dc1..b5c2a68f4 100644 --- a/docs/specs/security-audit.md +++ b/docs/specs/security-audit.md @@ -80,6 +80,7 @@ Source of truth: `2. Wait without ending your turn`, `3. Merge`, and `4. The ver - **With no `audit-report.md` the reporting step publishes each fragment verbatim under its own heading**, unmerged (rationale). - **Must return `VERDICT: INCONCLUSIVE` from a domain with any undetermined check unless it found a failure.** Only all-determined passing checks permit `VERDICT: PASS`; a domain's inconclusive verdict prevents a merged pass. - **`STATUS` is assigned in exactly two places**: where the status file is parsed, and in the single escalation block, **which orders `FAIL` > `MISSING` > `PASS`** — a dissent can raise `MISSING` to `FAIL` and never the reverse, and a `FAIL` alongside missing or unreadable fragments still reports them. +- **Every fragment's verdict line and its `UNVERIFIABLE`, `FAIL:`, `BLOCKER` and `WARNING` lines are lifted into the head ahead of the report**, matched at line start after an optional heading or bullet marker so the fragments' `FAIL IF` vocabulary is not read as a finding. Truncation keeps the head, so otherwise the markers the notes point at are what gets cut (rationale). - **The report is truncated to 32,000 characters before posting**, head kept, by `scripts/clamp-issue-body.mjs` (self-tested by `scripts/clamp-issue-body-selftest.mjs`). The call is non-fatal; the `audit-transcript` artifact holds the report in full; `.github/workflows/workflow-audit.yaml` truncates its commit list the same way (rationale). - **Every run uploads the `audit-transcript` artifact, which is world-readable and not secret-masked** — 14-day retention, deep-linked from failure issues (rationale). diff --git a/docs/specs/security-audit.rationale.md b/docs/specs/security-audit.rationale.md index ce88d36f7..fb14ba4f4 100644 --- a/docs/specs/security-audit.rationale.md +++ b/docs/specs/security-audit.rationale.md @@ -64,6 +64,8 @@ Collapsing the inconclusive case into `FAIL`, as the step originally did, filed GitHub rejects an over-long issue body outright; that rejection lands on a `set -e` step *after* the verdict is decided, and the finding then reaches no issue and no comment — only a red run and an artifact that expires. Truncation keeps the head because that is where the verdict and the links are, and the clamp call is non-fatal so a failure of the helper cannot reopen the window it closes. +Keeping the head is not the same as keeping what decides. Run 35842217451 (2026-09-23) composed a 226,302-character body: the posted issue carried `VERDICT: INCONCLUSIVE` for `audit-ci-secrets.md` and a note sending the reader to its `UNVERIFIABLE` checks, while that domain's one `UNVERIFIABLE` line sat past the cut along with the whole of `## Application security`, `## Hosted accounts` and `## Summary`. The deciding lines are not positionally predictable, so the clamp cannot be taught to keep them; lifting them into the head, which the clamp keeps by construction, is what makes the notes' pointers resolve. The lift matches at line start after an optional heading or bullet marker because every fragment is written in `FAIL IF` vocabulary — `### FAIL IF results` heads the passing list, and a `- PASS:` bullet quotes the clause it passed — so a bare `FAIL` match would lift the passing evidence as findings. Bounded at 40 lines and 500 characters a line so the lift cannot itself exhaust the budget it protects; the real report produced 8 lines and 1,571 characters. + Issue prose per combination of conditions cannot be kept correct by fixing combinations. Four consecutive review rounds found the same defect in different clothes — an arm whose text was true only of the states that could reach it, made false by the next gate that widened. A note claiming nothing about the other conditions cannot be invalidated by a new one. Gating a fragment guard on the status produced the same defect three times: gated on `PASS`, one empty fragment silenced the dissent check; widened to `!= FAIL`, an orchestrator that wrote `FAIL` itself silenced both, so a domain that left no report beside a real finding appeared nowhere at all. Recording what is true of a run and deciding its verdict are separate jobs. diff --git a/scripts/security-audit.test.mjs b/scripts/security-audit.test.mjs index 50deac2c6..cf992e0eb 100644 --- a/scripts/security-audit.test.mjs +++ b/scripts/security-audit.test.mjs @@ -75,6 +75,27 @@ const cases = [ notes: ['the merge never ran', '## audit-supply-chain.md', 'VERDICT: PASS', '## audit-application.md', '_No report — this domain produced no fragment._', '## audit-ci-secrets.md\n\n_Incomplete — this domain never closed its report'], counts: { '_Incomplete — this domain never closed its report': 1 } }, + // Run 35842217451 composed a 226,302-character body; the clamp keeps the + // head, so what reached the issue was `VERDICT: INCONCLUSIVE` for + // `audit-ci-secrets.md` without the one `UNVERIFIABLE` line the note sends + // the reader to, and without two later domains' sections at all. Every + // verdict and non-passing finding is lifted into the head ahead of the + // report, where the clamp cannot reach it. + { name: 'the lines that decided the verdict outlive truncation', status: 'PASS', + verdicts: ['PASS', 'INCONCLUSIVE', 'PASS', 'PASS'], + evidence: [null, '- UNVERIFIABLE: token scope needs a live credential', null, null], + report: `# Fixture report\n${'filler paragraph. '.repeat(3000)}\n`, + expected: 'INCONCLUSIVE', clamped: true, + notes: ['- `audit-ci-secrets.md`: - UNVERIFIABLE: token scope needs a live credential', + '- `audit-hosted.md`: VERDICT: PASS'] }, + // The lift reads line starts, so the `FAIL IF` vocabulary every fragment is + // written in must not read as a finding: a passing clause quoting one, and + // the heading the list sits under, are both PASS evidence. + { name: 'a passing FAIL IF clause is not lifted as a finding', status: 'PASS', + verdicts: ['PASS', 'PASS', 'PASS', 'PASS'], + evidence: ['### FAIL IF results\n- PASS: **FAIL IF** a secret leaks — none does.', null, null, null], + unfinished: [0], expected: 'INCONCLUSIVE', + counts: { 'FAIL IF': 0 } }, ]; for (const scenario of cases) { test(`reporting: ${scenario.name}`, (t) => { @@ -86,13 +107,14 @@ for (const scenario of cases) { if (args[0] === 'issue' && args[1] === 'list') process.stdout.write('23\\n'); `); if (scenario.status !== undefined) writeFileSync(join(dir, 'audit-status.txt'), scenario.status); - if (scenario.report !== null) writeFileSync(join(dir, 'audit-report.md'), '# Fixture report\n'); + if (scenario.report !== null) writeFileSync(join(dir, 'audit-report.md'), scenario.report ?? '# Fixture report\n'); scenario.verdicts.forEach((verdict, i) => { if (verdict === null) return; const sentinel = scenario.unfinished?.includes(i) ? '' : `${SENTINEL}\n${scenario.trailingBlank ? '\n' : ''}`; - writeFileSync(join(dir, fragments[i]), `VERDICT: ${verdict}\nEvidence\n${sentinel}`); + const evidence = scenario.evidence?.[i] ?? 'Evidence'; + writeFileSync(join(dir, fragments[i]), `VERDICT: ${verdict}\n${evidence}\n${sentinel}`); }); const result = spawnSync('bash', ['-c', reporting], { cwd: dir, env, encoding: 'utf8' }); assert.equal(result.status, scenario.expected === 'PASS' ? 0 : 1, result.stderr); @@ -101,6 +123,8 @@ for (const scenario of cases) { if (scenario.expected !== 'PASS') { const body = readFileSync(join(dir, 'audit-comment.md'), 'utf8'); assert.match(body, scenario.expected === 'FAIL' ? /Audit failed/ : /Audit reached no usable verdict/); + // Without this the truncation case passes vacuously on a body that fit. + if (scenario.clamped) assert.match(body, /_Truncated to fit: the full body is \d+ characters\./); for (const note of scenario.notes ?? []) assert.ok(body.includes(note), `missing note: ${note}`); for (const [note, n] of Object.entries(scenario.counts ?? {})) { assert.equal(body.split(note).length - 1, n, `wrong occurrence count for: ${note}`); diff --git a/scripts/spec-word-budgets.json b/scripts/spec-word-budgets.json index 555b27d60..1ab002e1c 100644 --- a/scripts/spec-word-budgets.json +++ b/scripts/spec-word-budgets.json @@ -18,7 +18,7 @@ "docs/specs/relay.md": 10150, "docs/specs/remote-api.md": 4700, "docs/specs/remote-security-model.md": 4750, - "docs/specs/security-audit.md": 1950, + "docs/specs/security-audit.md": 2050, "docs/specs/security-ci.md": 2700, "docs/specs/security-hosted.md": 600, "docs/specs/security-local.md": 3150, From 573ea8bb9c8bf8740cd84f7a0244c49bdb5f4855 Mon Sep 17 00:00:00 2001 From: dormouse-bot <287024035+dormouse-bot@users.noreply.github.com> Date: Wed, 23 Sep 2026 13:33:37 +0000 Subject: [PATCH 2/3] Keep the lift from silencing the step, and cap only the findings MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The review on this PR caught that the lift's `grep` exits 1 when a fragment carries no marker line, and this step's `set -eo pipefail` turned that into an abort before `audit-comment.md` was composed and before any `gh issue` call — no report at all, which is worse than the truncation the lift fixes. Reproduced against the shipped step with a markerless `audit-application.md`: exit 1, no body written, no issue call. Both passes now end in `|| true`, and a new case pins it. Verdicts and findings are collected separately so the cap falls on the findings alone. One verdict line per fragment is bounded by the fragment count; a single domain's findings are not, so the shared `head -n 40` could have let the first domain's findings push the last domain's verdict out — the loss the lift exists to prevent. The spec now states the bound and the unconditional verdict claim above the fold. --- .github/workflows/security-audit.yaml | 39 +++++++++++++++++++------- docs/specs/security-audit.md | 2 +- docs/specs/security-audit.rationale.md | 2 +- scripts/security-audit.test.mjs | 19 ++++++++++++- 4 files changed, 49 insertions(+), 13 deletions(-) diff --git a/.github/workflows/security-audit.yaml b/.github/workflows/security-audit.yaml index ed3bef6f8..d9f1644ef 100644 --- a/.github/workflows/security-audit.yaml +++ b/.github/workflows/security-audit.yaml @@ -503,19 +503,35 @@ jobs: # non-passing finding into the head, where the clamp cannot reach # it; the full sections still follow for whatever survives. # + # Verdicts and findings are collected separately so the cap can fall + # on the findings alone: one verdict line per fragment is bounded by + # the fragment count, while a single domain's findings are not, and + # a shared cap would let the first domain's findings push the last + # domain's verdict out — the loss this lift exists to prevent. + # # Matched at line start after an optional heading or bullet marker, # which is what keeps `### FAIL IF results` and every `- PASS:` # bullet quoting a **FAIL IF** clause out of the lift. `FAIL:` takes # its colon for the same reason; a domain-level failure arrives as - # `VERDICT: FAIL` and is caught by the `VERDICT:` alternative. - DECIDING=$(mktemp) + # `VERDICT: FAIL` and is caught by the verdict pass. + # + # `|| true` on both: `grep` exits 1 when it matches nothing, and + # under this step's `set -eo pipefail` that would end the step here, + # before `audit-comment.md` is composed and before any `gh issue` + # call — turning a truncated report into no report at all. A + # nonempty fragment with no marker line is exactly the state the + # verdict loop above already reports as unreadable. + VERDICTS=$(mktemp) + FINDINGS=$(mktemp) # Unquoted on purpose: `AUDIT_FRAGMENTS` is a filename list. # shellcheck disable=SC2086 for f in $AUDIT_FRAGMENTS; do [ -s "$f" ] || continue - grep -hE '^[[:space:]]*(#+[[:space:]]*)?([-*][[:space:]]*)?(VERDICT:|FAIL:|UNVERIFIABLE|BLOCKER|WARNING)' "$f" | - cut -c1-500 | sed "s|^|- \`$f\`: |" - done > "$DECIDING" + { grep -hE '^[[:space:]]*(#+[[:space:]]*)?([-*][[:space:]]*)?VERDICT:' "$f" || true; } | + cut -c1-500 | sed "s|^|- \`$f\`: |" >> "$VERDICTS" + { grep -hE '^[[:space:]]*(#+[[:space:]]*)?([-*][[:space:]]*)?(FAIL:|UNVERIFIABLE|BLOCKER|WARNING)' "$f" || true; } | + cut -c1-500 | sed "s|^|- \`$f\`: |" >> "$FINDINGS" + done if [ "$STATUS" = "FAIL" ]; then TITLE="[security-audit] FAIL on $(date -u +%Y-%m-%d)" @@ -540,14 +556,17 @@ jobs: echo "- **Inconclusive for a reason this step could not name.** That is itself a bug in the reporting step." echo fi - if [ -s "$DECIDING" ]; then + if [ -s "$VERDICTS" ] || [ -s "$FINDINGS" ]; then echo "### Lines that decided this verdict" echo echo "_Lifted out of the fragments so truncation cannot cut them. Each domain's full section follows for as far as the body reaches._" echo - head -n 40 "$DECIDING" - LIFTED=$(wc -l < "$DECIDING") - [ "$LIFTED" -gt 40 ] && echo "- _…and $((LIFTED - 40)) more; read them in the transcript._" + cat "$VERDICTS" + head -n 40 "$FINDINGS" + LIFTED=$(wc -l < "$FINDINGS") + if [ "$LIFTED" -gt 40 ]; then + echo "- _…and $((LIFTED - 40)) more findings; read them in the transcript._" + fi echo fi if [ -s audit-report.md ]; then @@ -591,7 +610,7 @@ jobs: done fi } > audit-comment.md - rm -f "$NOTES" "$DECIDING" + rm -f "$NOTES" "$VERDICTS" "$FINDINGS" # Truncate before posting, non-fatally. See docs/specs/security-audit.md -> "Outcomes and reporting". node scripts/clamp-issue-body.mjs audit-comment.md \ diff --git a/docs/specs/security-audit.md b/docs/specs/security-audit.md index b5c2a68f4..dac2073e3 100644 --- a/docs/specs/security-audit.md +++ b/docs/specs/security-audit.md @@ -80,7 +80,7 @@ Source of truth: `2. Wait without ending your turn`, `3. Merge`, and `4. The ver - **With no `audit-report.md` the reporting step publishes each fragment verbatim under its own heading**, unmerged (rationale). - **Must return `VERDICT: INCONCLUSIVE` from a domain with any undetermined check unless it found a failure.** Only all-determined passing checks permit `VERDICT: PASS`; a domain's inconclusive verdict prevents a merged pass. - **`STATUS` is assigned in exactly two places**: where the status file is parsed, and in the single escalation block, **which orders `FAIL` > `MISSING` > `PASS`** — a dissent can raise `MISSING` to `FAIL` and never the reverse, and a `FAIL` alongside missing or unreadable fragments still reports them. -- **Every fragment's verdict line and its `UNVERIFIABLE`, `FAIL:`, `BLOCKER` and `WARNING` lines are lifted into the head ahead of the report**, matched at line start after an optional heading or bullet marker so the fragments' `FAIL IF` vocabulary is not read as a finding. Truncation keeps the head, so otherwise the markers the notes point at are what gets cut (rationale). +- **Every fragment's verdict line is lifted into the head ahead of the report, followed by its `UNVERIFIABLE`, `FAIL:`, `BLOCKER` and `WARNING` lines, capped at 40 and 500 characters a line.** Both are matched at line start after an optional heading or bullet marker, so the fragments' `FAIL IF` vocabulary is not read as a finding. **Only the findings are capped**, so no domain's findings can push another's verdict out. **Neither pass may fail the step when it matches nothing** — under `set -eo pipefail` that would post no report at all (rationale). - **The report is truncated to 32,000 characters before posting**, head kept, by `scripts/clamp-issue-body.mjs` (self-tested by `scripts/clamp-issue-body-selftest.mjs`). The call is non-fatal; the `audit-transcript` artifact holds the report in full; `.github/workflows/workflow-audit.yaml` truncates its commit list the same way (rationale). - **Every run uploads the `audit-transcript` artifact, which is world-readable and not secret-masked** — 14-day retention, deep-linked from failure issues (rationale). diff --git a/docs/specs/security-audit.rationale.md b/docs/specs/security-audit.rationale.md index fb14ba4f4..346c94811 100644 --- a/docs/specs/security-audit.rationale.md +++ b/docs/specs/security-audit.rationale.md @@ -64,7 +64,7 @@ Collapsing the inconclusive case into `FAIL`, as the step originally did, filed GitHub rejects an over-long issue body outright; that rejection lands on a `set -e` step *after* the verdict is decided, and the finding then reaches no issue and no comment — only a red run and an artifact that expires. Truncation keeps the head because that is where the verdict and the links are, and the clamp call is non-fatal so a failure of the helper cannot reopen the window it closes. -Keeping the head is not the same as keeping what decides. Run 35842217451 (2026-09-23) composed a 226,302-character body: the posted issue carried `VERDICT: INCONCLUSIVE` for `audit-ci-secrets.md` and a note sending the reader to its `UNVERIFIABLE` checks, while that domain's one `UNVERIFIABLE` line sat past the cut along with the whole of `## Application security`, `## Hosted accounts` and `## Summary`. The deciding lines are not positionally predictable, so the clamp cannot be taught to keep them; lifting them into the head, which the clamp keeps by construction, is what makes the notes' pointers resolve. The lift matches at line start after an optional heading or bullet marker because every fragment is written in `FAIL IF` vocabulary — `### FAIL IF results` heads the passing list, and a `- PASS:` bullet quotes the clause it passed — so a bare `FAIL` match would lift the passing evidence as findings. Bounded at 40 lines and 500 characters a line so the lift cannot itself exhaust the budget it protects; the real report produced 8 lines and 1,571 characters. +Keeping the head is not the same as keeping what decides. Run 35842217451 (2026-09-23) composed a 226,302-character body: the posted issue carried `VERDICT: INCONCLUSIVE` for `audit-ci-secrets.md` and a note sending the reader to its `UNVERIFIABLE` checks, while that domain's one `UNVERIFIABLE` line sat past the cut along with the whole of `## Application security`, `## Hosted accounts` and `## Summary`. The deciding lines are not positionally predictable, so the clamp cannot be taught to keep them; lifting them into the head, which the clamp keeps by construction, is what makes the notes' pointers resolve. The lift matches at line start after an optional heading or bullet marker because every fragment is written in `FAIL IF` vocabulary — `### FAIL IF results` heads the passing list, and a `- PASS:` bullet quotes the clause it passed — so a bare `FAIL` match would lift the passing evidence as findings. Bounded at 40 lines and 500 characters a line so the lift cannot itself exhaust the budget it protects; the real report produced 8 lines and 1,571 characters. The cap falls on the findings alone because one verdict line per fragment is bounded by the fragment count while a single domain's findings are not, so a shared cap would let the first domain's findings push the last domain's verdict out — the loss the lift exists to prevent. The first draft of the lift piped `grep` without `|| true`: a nonempty fragment carrying no marker line — the unreadable-verdict state the guard loop above already reports — made the pipeline exit 1, and `set -eo pipefail` ended the step before `audit-comment.md` was composed, so nothing was posted at all. Caught by the review on the PR that introduced it and pinned by a markerless-fragment case. Issue prose per combination of conditions cannot be kept correct by fixing combinations. Four consecutive review rounds found the same defect in different clothes — an arm whose text was true only of the states that could reach it, made false by the next gate that widened. A note claiming nothing about the other conditions cannot be invalidated by a new one. diff --git a/scripts/security-audit.test.mjs b/scripts/security-audit.test.mjs index cf992e0eb..3087e6d6d 100644 --- a/scripts/security-audit.test.mjs +++ b/scripts/security-audit.test.mjs @@ -96,6 +96,17 @@ const cases = [ evidence: ['### FAIL IF results\n- PASS: **FAIL IF** a secret leaks — none does.', null, null, null], unfinished: [0], expected: 'INCONCLUSIVE', counts: { 'FAIL IF': 0 } }, + // A fragment carrying no marker line at all — the unreadable-verdict state + // the guard loop above already reports. The lift's `grep` matches nothing + // and exits 1; without `|| true` this step's `set -eo pipefail` ends it + // before the body is composed, so the reader gets a red run and an artifact + // instead of a truncated report. `raw` bypasses the verdict-line prefix + // every other fixture fragment carries. + { name: 'a fragment with no marker line still gets reported', status: 'PASS', + verdicts: ['PASS', 'PASS', 'PASS', 'PASS'], + raw: [null, null, '# audit-application.md\n\nI reviewed the specs but could not finish.\n', null], + expected: 'INCONCLUSIVE', posts: true, + notes: ["A domain's verdict could not be read", '- `audit-hosted.md`: VERDICT: PASS'] }, ]; for (const scenario of cases) { test(`reporting: ${scenario.name}`, (t) => { @@ -114,12 +125,18 @@ for (const scenario of cases) { ? '' : `${SENTINEL}\n${scenario.trailingBlank ? '\n' : ''}`; const evidence = scenario.evidence?.[i] ?? 'Evidence'; - writeFileSync(join(dir, fragments[i]), `VERDICT: ${verdict}\n${evidence}\n${sentinel}`); + writeFileSync(join(dir, fragments[i]), + scenario.raw?.[i] ?? `VERDICT: ${verdict}\n${evidence}\n${sentinel}`); }); const result = spawnSync('bash', ['-c', reporting], { cwd: dir, env, encoding: 'utf8' }); assert.equal(result.status, scenario.expected === 'PASS' ? 0 : 1, result.stderr); const calls = readFileSync(join(dir, 'gh-calls.jsonl'), 'utf8').trim().split('\n').map(JSON.parse); assert.equal(calls.some((args) => args[0] === 'issue' && args[1] === 'close'), scenario.expected === 'PASS'); + // The step aborting before it posts leaves every `notes` assertion below + // unreachable, so name the post itself. + if (scenario.posts) { + assert.ok(calls.some((args) => args[0] === 'issue' && args[1] === 'comment'), 'no issue comment was posted'); + } if (scenario.expected !== 'PASS') { const body = readFileSync(join(dir, 'audit-comment.md'), 'utf8'); assert.match(body, scenario.expected === 'FAIL' ? /Audit failed/ : /Audit reached no usable verdict/); From b24c2b5df2983242aad2ead9f8e00a8bfdd633db Mon Sep 17 00:00:00 2001 From: Ned Twigg Date: Wed, 23 Sep 2026 09:52:15 -0700 Subject: [PATCH 3/3] Pin the findings-only cap with a test, and state the two-block lift accurately Applies the two review suggestions: the spec now says verdicts and findings are two blocks, both cut to 500 characters, with only the findings carrying the 40-line cap; a new case puts 42 findings in the first fragment and goes red when the cap is shared. Co-Authored-By: Claude Opus 5.5 (1M context) --- docs/specs/security-audit.md | 2 +- scripts/security-audit.test.mjs | 8 ++++++++ 2 files changed, 9 insertions(+), 1 deletion(-) diff --git a/docs/specs/security-audit.md b/docs/specs/security-audit.md index dac2073e3..610509353 100644 --- a/docs/specs/security-audit.md +++ b/docs/specs/security-audit.md @@ -80,7 +80,7 @@ Source of truth: `2. Wait without ending your turn`, `3. Merge`, and `4. The ver - **With no `audit-report.md` the reporting step publishes each fragment verbatim under its own heading**, unmerged (rationale). - **Must return `VERDICT: INCONCLUSIVE` from a domain with any undetermined check unless it found a failure.** Only all-determined passing checks permit `VERDICT: PASS`; a domain's inconclusive verdict prevents a merged pass. - **`STATUS` is assigned in exactly two places**: where the status file is parsed, and in the single escalation block, **which orders `FAIL` > `MISSING` > `PASS`** — a dissent can raise `MISSING` to `FAIL` and never the reverse, and a `FAIL` alongside missing or unreadable fragments still reports them. -- **Every fragment's verdict line is lifted into the head ahead of the report, followed by its `UNVERIFIABLE`, `FAIL:`, `BLOCKER` and `WARNING` lines, capped at 40 and 500 characters a line.** Both are matched at line start after an optional heading or bullet marker, so the fragments' `FAIL IF` vocabulary is not read as a finding. **Only the findings are capped**, so no domain's findings can push another's verdict out. **Neither pass may fail the step when it matches nothing** — under `set -eo pipefail` that would post no report at all (rationale). +- **Every fragment's verdict line is lifted into the head ahead of the report, then every fragment's `UNVERIFIABLE`, `FAIL:`, `BLOCKER` and `WARNING` lines** — two blocks, not one per domain. Both passes match at line start after an optional heading or bullet marker, so the fragments' `FAIL IF` vocabulary is not read as a finding, and both cut lines to 500 characters. **Only the findings block carries the 40-line cap**, so no domain's findings can push another's verdict out. **Neither pass may fail the step when it matches nothing** — under `set -eo pipefail` that would post no report at all (rationale). - **The report is truncated to 32,000 characters before posting**, head kept, by `scripts/clamp-issue-body.mjs` (self-tested by `scripts/clamp-issue-body-selftest.mjs`). The call is non-fatal; the `audit-transcript` artifact holds the report in full; `.github/workflows/workflow-audit.yaml` truncates its commit list the same way (rationale). - **Every run uploads the `audit-transcript` artifact, which is world-readable and not secret-masked** — 14-day retention, deep-linked from failure issues (rationale). diff --git a/scripts/security-audit.test.mjs b/scripts/security-audit.test.mjs index 3087e6d6d..fb1cc5262 100644 --- a/scripts/security-audit.test.mjs +++ b/scripts/security-audit.test.mjs @@ -107,6 +107,14 @@ const cases = [ raw: [null, null, '# audit-application.md\n\nI reviewed the specs but could not finish.\n', null], expected: 'INCONCLUSIVE', posts: true, notes: ["A domain's verdict could not be read", '- `audit-hosted.md`: VERDICT: PASS'] }, + // The cap falls on the findings alone, so one domain's findings cannot push + // a later domain's verdict out of the head — the loss the lift exists to + // prevent. 42 findings in the first fragment is two past the cap. + { name: 'findings past the cap do not push out a later verdict', status: 'PASS', + verdicts: ['PASS', 'PASS', 'PASS', 'PASS'], + evidence: [Array.from({ length: 42 }, (_, i) => `WARNING: finding ${i}`).join('\n'), null, null, null], + unfinished: [0], expected: 'INCONCLUSIVE', + notes: ['- `audit-hosted.md`: VERDICT: PASS', 'more findings; read them in the transcript'] }, ]; for (const scenario of cases) { test(`reporting: ${scenario.name}`, (t) => {