diff --git a/changelog.d/gst-435-collect-labels.md b/changelog.d/gst-435-collect-labels.md new file mode 100644 index 000000000..df39bd8c8 --- /dev/null +++ b/changelog.d/gst-435-collect-labels.md @@ -0,0 +1,12 @@ +**/ship's review collection no longer turns a reviewer's "no CRITICAL" summary into a CRITICAL finding.** In the HARN-15 runs (harness-evals #152 and the ship2 pass) the Red Team and fix-check readers opened with lines like "I found three INFORMATIONAL issues and no CRITICAL ones" and "there's nothing at CRITICAL". `collect` stored each of those lines as a CRITICAL finding, and `judge` would not let the lead drop it, so a clean pass read as blocked. The same happened to the fix-check closing line "Neither finding is CRITICAL." on gstack-evals#94. What reviewers see is unchanged: every brief renders byte for byte as before. + +### Fixed +- **A severity word that a negator directly scopes is no label.** "not CRITICAL", "isn't really High", "no CRITICAL or High findings", "found nothing CRITICAL", "nothing at CRITICAL", "Neither finding is CRITICAL", "none of which is High" and "below High" no longer count. The negation has to apply to the label itself: "Nothing guards the write path the failure of which is CRITICAL" and "None of the writes are atomic and are CRITICAL (queue.ts:78); High confidence" still collect as CRITICAL. A table row is never read for negation. A kept finding never takes a weaker label than the strongest one left on its line, so the table row "| 1 | `CRITICAL` | High | token leak in log.ts:3 |" stays CRITICAL. The native adversarial voice's FIXABLE still collects as CRITICAL. +- **A list item whose label first appears in an indented later paragraph collects as that item.** The ship2 native voice wrote "4. **The host lock is not atomic** (`qm-stack.ts:66-76`)", two detail bullets, and then an indented "Fix: … FIXABLE. Severity: Low." paragraph. The finding was stored as the Fix paragraph alone, without the line that says what it is. It now starts with the item's first line. A labeled item's own labeled later paragraph is still its own finding. +- **No blocker is lost.** Main's parser and this one were run over 2,250 recorded reader outputs: 25 raw outputs from the GSTA-189 and GSTA-196 replays, the 2,172 finding records from arms C6, L6, A3668 (both suites), G1 and L, and 53 HARN-15 and test-fixture outputs. None of the 260 records matched to a judged blocker changes. In the arms, two INVESTIGATE findings, each recorded in both arm G1 and arm L, lose a phantom label: one a CRITICAL from "no CRITICAL" inside its sentence, the other a High from "resolves below High". Every other change is a HARN-15 or GEVA-77 summary line that drops from CRITICAL to INFORMATIONAL or to nothing, or a native finding that gains its title. + +### Docs +- `docs/review-policy.md` (collect) describes the negation rule and the later-paragraph rule. + +### Tests +- `test/review-dispatch.test.ts`: the HARN-15 Red Team and fix-check replies, the GEVA-77 fix-check reply and the listed negation shapes collect no CRITICAL; fix-check 5's two table rows, its two INFORMATIONAL cases, other negations of something else and harness-evals #152's native FIXABLE findings keep their strongest label; the ship2 native reply collects 6 findings with item 4's title. Fixtures under `test/fixtures/review-ledger/` (already registered to that test). diff --git a/docs/review-policy.md b/docs/review-policy.md index 301d187e5..452d480ac 100644 --- a/docs/review-policy.md +++ b/docs/review-policy.md @@ -468,7 +468,12 @@ ones, and the arm lost blockers. `bin/gstack-review-dispatch` closes both ends: `merged.jsonl` with the label as written (a JSON line's `severity`, or the most severe of a prose line's Critical, High, Medium, Low, P0-P3, FIXABLE or INVESTIGATE wherever they stand, headings and table rows included; a top-level list item under a - label-only heading takes the stronger of the two). A `Severity:` or `Blocks:` line + label-only heading takes the stronger of the two). A label a negator directly scopes is + no label: "not CRITICAL", "no CRITICAL or High findings", "found nothing CRITICAL", + "Neither finding is CRITICAL", "below High" (GST-435). A negation of something else + ("Nothing guards the write path the failure of which is CRITICAL") leaves the label, a + table row is never read for negation, and a kept finding never takes a weaker label than + the strongest one left on its line. A `Severity:` or `Blocks:` line continues the finding above it until a list item, heading, bold paragraph opener or a second line for the same field starts another: `Severity` raises its label, `Blocks: yes` makes it CRITICAL and `Blocks: no` never lowers it; an inline @@ -476,6 +481,8 @@ ones, and the arm lost blockers. `bin/gstack-review-dispatch` closes both ends: adversarial voice's FIXABLE collects as CRITICAL (normalizer v2, GSTA-167). An unnumbered, unlabeled bullet with no file:line under a paragraph finding whose Severity or Blocks line has not come yet is that finding's detail, so the field line after it lands on the finding; a numbered item is its own finding. + An indented later paragraph that gives an unlabeled list item its first label (`Fix: ... FIXABLE. Severity: Low.`) collects + as that item, its first line included. A phantom finding costs a fold; a lost one costs the gate. A reply with no finding counts as a review only with the clean marker its own brief asks for: `NO FINDINGS` alone on a line for the specialists, the batched specialist, the targeted reader, the Red Team and fix-check, the `Recommendation:` diff --git a/lib/review-ledger.ts b/lib/review-ledger.ts index 0d945d0c4..3029e3ccc 100644 --- a/lib/review-ledger.ts +++ b/lib/review-ledger.ts @@ -8,8 +8,8 @@ * judgment each lead-judgment change: an added or promoted finding, with its reason * judge the verdict on the parent's judged set (ok, or the dropped and downgraded ids) * - * Findings reach merged.jsonl only through collect, with each voice's label as written (a prose line's most severe one, - * raised by the Severity line below it), except that a prose voice's `Blocks: yes` and the native voice's FIXABLE collect + * Findings reach merged.jsonl only through collect, with each voice's label as written (a prose line's most severe one that + * no negator directly scopes, raised by the Severity line below it), except that a prose voice's `Blocks: yes` and the native voice's FIXABLE collect * as CRITICAL (GSTA-167 normalizer v2). * The parent's judged set may add or promote a finding and may fold a duplicate into one at least * as severe; it never drops or downgrades one. verify replays every check from the ledger alone. @@ -59,6 +59,21 @@ const stronger = (a: string, b: string) => ((rank(a) ?? 0) >= (rank(b) ?? 0) ? a /** A prose voice's `Blocks: yes` (GSTA-167 normalizer v2): the finding is CRITICAL whatever its label; `Blocks: no` never lowers one. */ const BLOCKS_YES = /(? t.startsWith('|') || (t.match(/\|/g)?.length ?? 0) >= 2; +/** The label words on a line that no negator directly scopes; on a table row, every one. */ +const unnegatedLabels = (t: string) => [...t.matchAll(ALL_LABELS)].filter(m => tableRow(t) || !NEGATED_BEFORE.test(t.slice(0, m.index))).map(m => m[1]!); /** A file:line locus ("auth.ts:9"): prose naming one after a finding's fields starts the next finding. */ const LOCUS = /[\w./-]+\.\w+:\d+/; /** A line that is only a finding's `Severity` or `Blocks` field ("Severity: High", "- Blocks: yes", "Severity High"). */ @@ -68,12 +83,14 @@ export interface MergedLine { id: string; reader: string; label: string; raw: st /** * One reader's findings as written: a JSON object per line (severity kept as written); else each prose line carrying a severity or - * FIXABLE/INVESTIGATE label, at the most severe label on it, and each top-level list item under a label-only heading (the stronger + * FIXABLE/INVESTIGATE label no negator directly scopes ("no CRITICAL ones" is none; a table row is never read for negation), at + * the most severe such label on it, and each top-level list item under a label-only heading (the stronger * of the two labels). A `Severity` or `Blocks` field line continues the open finding, once per field: `Severity` raises its label, * `Blocks: yes` makes it CRITICAL, `Blocks: no` never lowers it; with no finding open it is one, carrying the paragraph's subject. * A list item, a heading, a bold or post-blank paragraph at the margin, or a line naming a file:line after the open finding's fields * starts the next finding, except an unlabeled, unnumbered bullet with no file:line under a paragraph finding whose fields have not - * come yet, which is its detail and joins its text (GSTA-196: 4 findings with detail bullets collected as 8). An inline + * come yet, which is its detail and joins its text (GSTA-196: 4 findings with detail bullets collected as 8). An indented later + * paragraph that first labels an unlabeled list item collects as that item, its first line included (GST-435). An inline * `Blocks: yes` is CRITICAL too. The native voice's FIXABLE collects as CRITICAL; other labels stay as written for lead judgment. When in doubt the parser merges rather than drops: a lost blocker costs the gate. */ export function parseFindings(reader: string, output: string): Array<{ label: string; raw: string }> { @@ -83,9 +100,11 @@ export function parseFindings(reader: string, output: string): Array<{ label: st let current: Finding | null = null; let head = ''; let gap = false; + let item: string | null = null; + let itemFinding: Finding | null = null; const role = reader.split('/')[0]; const named = (l: string) => (role === 'adversarial-native' && l.toUpperCase() === 'FIXABLE' ? 'CRITICAL' : l); - const strongest = (t: string) => [...t.matchAll(ALL_LABELS)].map(m => named(m[1]!)).reduce((a, b) => (a === undefined ? b : stronger(a, b)), undefined); + const strongest = (t: string) => unnegatedLabels(t).map(named).reduce((a, b) => (a === undefined ? b : stronger(a, b)), undefined); const push = (label: string, raw: string, fields: string[] = []): Finding => { const f = { label, raw, fields: new Set(fields), list: LIST_ITEM.test(raw) }; out.push(f); return f; }; for (const line of output.replace(/\r\n/g, '\n').split('\n')) { const t = line.trim(); @@ -110,6 +129,8 @@ export function parseFindings(reader: string, output: string): Array<{ label: st else current = null; continue; } + if (!indented && !LIST_ITEM.test(t)) { item = null; itemFinding = null; } + if (indented && item && !itemFinding && !LIST_ITEM.test(t) && (label || blocks)) { current = itemFinding = push(blocks ? 'CRITICAL' : label!, `${item} | ${t}`); continue; } const heading = LABEL_HEADING.exec(t); if (heading) { section = named(heading[1]!); current = null; head = ''; continue; } if (!indented && HEADING.test(t)) { section = null; current = null; } @@ -121,14 +142,16 @@ export function parseFindings(reader: string, output: string): Array<{ label: st } const detail = listItem && /^[-*+]\s/.test(t) && !locus && !label && !section && !!current && !current.list && !current.fields.size; if (detail && !blocks) { current!.raw += ` | ${t}`; continue; } + if (listItem && !detail) { item = t; itemFinding = null; } const fresh = (listItem && !detail) || (!indented && !detail && (t.startsWith('**') || afterGap)) || (!!current?.fields.size && locus); if (fresh) { current = null; head = t; } const inherited = section && listItem ? section : undefined; const found = label && inherited ? stronger(inherited, label) : label ?? inherited; - if (found) { current = push(blocks ? 'CRITICAL' : found, t); if (!indented) head = t; continue; } + if (found) { current = push(blocks ? 'CRITICAL' : found, t); if (!indented) head = t; if (item === t) itemFinding = current; continue; } if (blocks && !t.startsWith('{')) { if (current) { current.label = 'CRITICAL'; current.raw += ` | ${t}`; continue; } current = push('CRITICAL', head && head !== t ? `${head} | ${t}` : t); + if (item === t) itemFinding = current; continue; } if (!fresh && !current && (!LIST_ITEM.test(head) || locus)) head = t; diff --git a/test/fixtures/review-ledger/geva-77-fix-check.txt b/test/fixtures/review-ledger/geva-77-fix-check.txt new file mode 100644 index 000000000..a7f9b9c39 --- /dev/null +++ b/test/fixtures/review-ledger/geva-77-fix-check.txt @@ -0,0 +1,4 @@ +{"severity":"INFORMATIONAL","path":"CHANGELOG.md","line":5,"summary":"The reworded sentence describes /ship behaviour that gstack 1.91.358.0 does not have. Under bump never, Step 12 prints 'VERSION: kept', and Step 13's Kept branch (ship/sections/changelog.md:49-51) puts an entry 'where the repo's own rule puts unit-PR notes (its CHANGELOG preamble or a fragment directory it names), else under ## [Unreleased]'. Only NO_VERSION or changelog: null skip the entry. The preamble still names no location for unit-PR notes (for example the PR body), so a lane that reads Step 13 literally still reaches the [Unreleased] fallback on its next /ship. That half of the original finding is still open. Fix: say where the notes go ('unit-PR notes go in the PR body'), or set changelog: null in ship-policy.json so Step 13 really writes nothing."} +{"severity":"INFORMATIONAL","path":"CHANGELOG.md","line":5,"summary":"The fix leaves the original finding's fragment question open. changelog.d/gsta-30-laneb-quick-3-results.md is still in the tree, and CREW.md still says 'changelog entries go in changelog.d/ fragments'. The new sentence covers CHANGELOG.md only. docs/ship-policy.md:42 says bump never cannot be combined with changelog_fragments, so nothing folds that fragment at release, and a lane following CREW.md can keep adding fragments the preamble never mentions. Fix: name changelog.d/ in the preamble, either as banned or as folded by the Captain at release, and decide what happens to the GSTA-30 fragment."} + +Neither finding is CRITICAL. The change is one prose line, and pregate, tests (1264), typecheck, errors and wiki all pass after the fix. I checked the claim against the installed gstack 1.91.358.0: ship/sections/changelog.md and docs/ship-policy.md. \ No newline at end of file diff --git a/test/fixtures/review-ledger/harn15-152-p1-adversarial-native.txt b/test/fixtures/review-ledger/harn15-152-p1-adversarial-native.txt new file mode 100644 index 000000000..2d9f30e4e --- /dev/null +++ b/test/fixtures/review-ledger/harn15-152-p1-adversarial-native.txt @@ -0,0 +1,42 @@ +The range from 6c8fcfe to HEAD (8dc6584f5) has no blocking defect in today's code paths, because nothing sets `upstream.provider` yet. It does have one cheap authentication hole and two silent-miscount risks to deal with before the provider wiring lands. + +**Coverage:** I read the full source diff: `world/services/gbrain-tap.ts`, `world/services/server.ts`, `world/services/events.ts`, `scripts/v03/mechanism.ts`, the docs and the version files. I also read the helpers they depend on (`describeCall`, `savedFactsOf`, `grantFor`, `turnStarts`, `ingestedFacts`, `resolveActor`). + +**Fixtures were reviewed in summary mode only.** I read the stat and test names for the two new test files, `tests/gbrain-provider-off-identity.test.ts` (+145) and `tests/gbrain-tap-provider.test.ts` (+276), not their payloads: +- the off-path identity test replays the v0.3 pilot events and the denied batches, and checks the mechanism output is byte-identical; +- the provider test covers recall versus query, the cap, `isError`, timeout, forged `acting_user`, `x-qm-actor`, overlapping tokens, `qmDelivery` and the mechanism counts. + +Neither file tests an empty token or a malformed `content` entry. + +I checked docs line truncation with `wc -L` and grep on the paragraph tails; no paragraph lost its tail. No gstack workflow ran on this review voice. + +**Findings** + +1. **An empty provider token skips authentication (FIXABLE, medium).** At `gbrain-tap.ts` ~376-378, the bearer regex plus `.trim()` turns `Authorization: Bearer ` followed by any whitespace into `""`. I confirmed this with bun on `"Bearer "` and `"Bearer \t"`. + - If `provider.tokens` ever contains `""`, for example when the wiring fills it from an unset env var, any client can send a blank bearer. It skips `resolveActor`, gets logged as QM's provider, and is attributed to `provider.actor`. + - The overlap guard at `server.ts:102` doesn't refuse empty, whitespace-only or padded tokens. Padded tokens can never match after the trim, so QM gets a 401. + - It also doesn't check the sidecar's own tokens (`upstream.token`, `tokens.members`, `ingest.tokens`). + - Fix: in `setGbrain`, require a non-empty list of non-empty tokens with no surrounding whitespace, and require `bearer` to be non-empty in the tap. + +2. **A token mismatch produces a silent zero (INVESTIGATE, medium).** A provider request whose bearer isn't in the static list falls through to `resolveActor` and gets a 401 at ~379, before any event is logged. That covers two cases: a client-credentials access token refreshed mid-trial, and a capture write credential nobody listed (the docs admit the second). + - QM swallows provider errors, so the trial goes on. + - `mechanism --provider-aware` never sees an event, so neither `MECH-CALLER-MISSING` nor `MECH-PROVIDER-OP-UNKNOWN` fires, and the table reports "provider recall 0 of N" as a real result. + - This needs a call when the wiring is designed: log a rejected-auth gbrain event in provider mode, and/or have the mechanism refuse a QM-provider trial whose consumer turn has no provider recall event. + +3. **Fact delivery is judged by substring (INVESTIGATE, medium).** `qmDelivery` marks a fact delivered when `delivered.includes(f.fact)`. + - In correction tasks, the needed and superseded facts are often near-duplicates. If the needed text is a prefix or substring of the superseded one ("Launch moved to Friday" inside "Launch moved to Friday at 3pm" returns true), the needed fact counts as delivered when the cap kept only the superseded row. + - Short facts also match unrelated text. + - This inflates `provider_delivered_needed`, `search delivered it` and `model read delivered it` silently, and the correction tasks are where this table matters most. + - Fix: match on row boundaries (the `Saved fact (...): ` line or the row's end) rather than a bare substring. + +4. **A malformed result now crashes the request (FIXABLE, low).** `qmDelivery` reads `c.type` on each `result.content` entry. A `null` entry throws (confirmed: "null is not an object"). + - `tagged` runs inside the logging loop after upstream has already answered. So in provider mode, a reply that used to pass through becomes an uncaught exception: the harness gets a 500 instead of gbrain's reply, and the rest of the batch goes unlogged. + - Fix: guard with `c && c.type === 'text'`. + +5. **Timeout drops lean towards "delivered" (INVESTIGATE, low).** `dropped: 'timeout'` compares the tap-side batch `latency_ms`, which is a lower bound on QM's session time, with `timeout_ms`. Calls just under the threshold at the tap but over it at QM count as delivered. The docs disclose this, but the bias only ever goes towards "delivered", so the report should state that direction. + +6. **The trust premise is unverified (INVESTIGATE, low).** The design depends on the model never seeing QM core's client credentials in `MEMORY_PROVIDER_CONFIG`. If QM gives the model any shell, file or env tool inside the container that holds that config, the model can present the provider token, and its own calls get tagged `provider`. This needs checking against QM b6786d67's tool set before a paid cell. + +7. **Nit (FIXABLE).** In the model-call branch, `JSON.stringify(structured ?? '')` records an empty result as `text_chars` and `delivered_chars` of 2 (the string `""`). The mechanism doesn't read it, but the event data is wrong. + +Recommendation: Fix the empty or whitespace provider-token acceptance at world/services/server.ts:102 and gbrain-tap.ts ~376 before merge, because a blank token in `provider.tokens` would let any client sending a blank bearer skip `resolveActor` and be recorded as QM's provider; the fix is a two-line validation, and findings 2 and 3 can be settled with the wiring since nothing turns provider mode on yet. diff --git a/test/fixtures/review-ledger/harn15-152-p2-fix-check.txt b/test/fixtures/review-ledger/harn15-152-p2-fix-check.txt new file mode 100644 index 000000000..dc4576d02 --- /dev/null +++ b/test/fixtures/review-ledger/harn15-152-p2-fix-check.txt @@ -0,0 +1,17 @@ +The fixes are sound. None of them breaks a caller, opens or closes a guard wrongly, or changes behaviour with the option off, so there's nothing at CRITICAL. I have two INFORMATIONAL findings. The first is the one worth acting on: the tap half of the blank-bearer test never reaches the fixed line. + +``` +{"severity":"INFORMATIONAL","path":"tests/gbrain-tap-provider.test.ts","line":299,"summary":"The tap half of the blank-bearer test can't catch the bug it targets. Header normalization turns 'Bearer ' + ' ' into \"Bearer\", so the regex gives a null bearer and the old line (bearer !== null) also answers 401. I put the old line back in a scratch copy and the test still passed. An input that does reach the fix is 'Bearer \\u00a0' with tokens [''], because HTTP header normalization keeps U+00A0 while trim() strips it. With that input the old line authenticates the request as the provider, and the fixed line answers 401 (checked both ways in a scratch copy). The setGbrain half of the test does pin the server guard."} +{"severity":"INFORMATIONAL","path":"world/services/gbrain-tap.ts","line":404,"summary":"Fix 3 covers provider calls only. A model call whose HTTP status is not 2xx but whose body carries a JSON-RPC result still gets qmDelivery(result) with facts set to true. trialProviderMechanism (scripts/v03/mechanism.ts:117 on) counts model_returned and model_delivered without checking the call's outcome, so a model call QM's client would have thrown on counts as delivering the needed fact. This was already there before these commits, and the input is rare: gbrain would have to answer 500 with a result body."} +``` + +**What I checked:** +- **Blank bearer, fix 1:** `!!bearer` changes only the empty-string case. In the model path, `resolveActor` is unchanged. The `setGbrain` guard throws before `gbrain = u`, so a refused upstream leaves no partial state. The trial.ts:194 caller never sets `provider`, so it never reaches the new check. +- **Dropped calls, fix 2:** The `delivery?.dropped` skip comes after the `provider_searches++` count, so a dropped search still counts as a search. A dropped recall already had `delivered_chars` 0. Only the provider path ever sets `dropped`, so model calls aren't affected. +- **Failed calls, fix 3:** `ok` is `d.outcome === 'ok'` at both call sites, the all-denied branch included. Scope denials already came back as an `isError` result, so they were tool_error before this change as well. Without `provider`, `tagged` still returns `{}`, so events with the option off are unchanged. +- **Null content entry, fix 4:** `c?.type` short-circuits before `c.text` is read. The structuredContent paths don't iterate `content`. +- **Tests for fixes 2 to 5:** I traced the tests for fixes 2 to 4 and the code before these commits would fail each one. For the per-row test (fix 5), I worked through the sums: `provider_searches` is 2 from t2's two calls without `max_chars`, `model_calls` is 2 from the search plus the remember, and `provider_recalls` is 2, all from t1. + +I didn't rerun the full suite. The 41 passing tests and the clean tsc run are your results; I ran only the blank-bearer test and a scratch U+00A0 test, both in a scratch copy I've since removed. Your checkout is untouched. + +gstack: ok 1.91.346.0 (`--check` only, no workflow run; 2 behind latest 1.91.348.0) diff --git a/test/fixtures/review-ledger/harn15-ship2-p2-adversarial-native.txt b/test/fixtures/review-ledger/harn15-ship2-p2-adversarial-native.txt new file mode 100644 index 000000000..024c5bbf9 --- /dev/null +++ b/test/fixtures/review-ledger/harn15-ship2-p2-adversarial-native.txt @@ -0,0 +1,27 @@ +I found six problems in the range 2ae5923..HEAD (cdce7c76d) and none of them blocks. The one to fix before merge is the sandbox adoption at `qm-stack.ts:106`. When a sandbox create call fails, it can delete containers and volumes that belong to other QM stacks on the same host. The fix is one narrower filter. + +gstack: ok 1.91.366.0 require=claude,codex,patch (pins: skipped (no project); behind=3 (latest: 1.91.369.0)). That is the result of the `--check` run on this shared machine, which is above the 1.91.364.0 floor. This review did not run any gstack workflow. + +**Scope.** I read in full all non-fixture source that changed: `harnesses/qm/gstack/{qm-stack,s1-stage,smoke}.ts`, the gstack `README.md` and `CHANGELOG.md`. I reviewed the fixtures and tests in **summary mode only**. `tests/qm-gstack-smoke.test.ts` gained 39 lines and four tests: a successful import after a failed register reports no refusal, `lockHost` refuses a lock held by a live process and takes over a dead one, `receiptOut` keeps the committed receipt only for its own gstack ref, and a transport failure comes back as http 0 instead of throwing. I did not pull any payload bytes. With a scrubbed environment, `bun test tests/qm-gstack-smoke.test.ts` passed 16 of 16 and `bunx tsc --noEmit` was clean. + +**Findings** + +1. **A failed sandbox create adopts sandboxes from other stacks** (`qm-stack.ts:106-108`, `145-149`). `sandboxNames()` lists every container on the host with `label=qm.sandbox=1`. When the create call fails, or is cut off by `qmClient`'s 300 s timeout, `adoptNew()` takes ownership of every such container that appeared during that window. `down()` then runs `docker rm -f` on each one, plus `volume rm -f qm-home-*` and `network rm`. A live trial's QM stack uses the same label (`capture.sh:20`), so one failed s1 create during a trial deletes that trial's sandbox and its home volume. The lock does not help, because it only excludes other qm-gstack runs. The interface comment and the new README line ("never another stack's") claim more than the code does. Fix: add `--filter ancestor=` to the adoption query. s1's image `harness-eval/qm-sandbox:gstack-` is unique to it, and the project lock already rules out a second s1. Alternatively, filter on whatever label QM puts on sandboxes to tie them to their core. FIXABLE. Severity: Medium. Blocks: no, because it needs a concurrent QM stack, which the README already says not to run. + +2. **One bad container name skips the whole teardown** (`qm-stack.ts:105`, `108`, `123`). `own()` throws for any name that is not `qm-sbx-*`. `adoptNew()` calls it inside `down()`'s `finally` before `removeSandboxes()`, `dc down` and `unlock()`. One labelled container with an unexpected name aborts all three steps. Because `stopping` is already true, nothing retries, so pg, core and the owned sandboxes stay running. The same throw inside `createSandbox`'s catch leaves `creating` set, so the same failure repeats at teardown. Fix: skip names that don't match in `adoptNew` instead of throwing, or put each teardown step in its own `try`. FIXABLE. Severity: Low. Blocks: no. + +3. **A sandbox created after the call is cut off leaks** (`qm-stack.ts:108`, `147`). If the client times out while QM is still creating the container, `adoptNew()` takes its snapshot and sets `creating = null` before the container exists. Nothing tracks it afterwards, and `compose down --remove-orphans` doesn't remove it either, because core created it through the Docker socket and not through compose. Fix: keep the pre-create baseline until `down()`, and diff against it after `dc down` has stopped core. FIXABLE. Severity: Low. Blocks: no. + +4. **The host lock is not atomic** (`qm-stack.ts:66-76`). The lock code reads the file, checks the pid and then writes, with no `O_EXCL`. Two runs launched close together can both take the lock. The second run's opening `dc down -v --remove-orphans` then removes the first run's stack, which is the exact case the lock exists to stop. `release()` also deletes the file without checking it still holds its own pid, so the run that lost the race clears the winner's lock. Two smaller gaps: + - The lock directory comes from `tmpdir()`, so a run with `TMPDIR` set and one without lock in different places. + - A reused pid makes a dead lock look alive. + + Fix: create the file with `writeFileSync(path, pid, { flag: 'wx' })`. On `EEXIST`, check whether the holder is alive, then unlink and retry `wx` once. Release only if the file still holds this process's pid. Use a fixed directory. FIXABLE. Severity: Low. Blocks: no, since this is a manual tool. + +5. **A failed s1 run still overwrites the committed router file** (`s1-stage.ts:104`, `117`). The receipt now goes to `--work` on failure, but `routerOut` is still derived from `out` and written at line 117, before the stack runs. At the committed ref, a failed or aborted run overwrites `receipts/s1-router-21277291.md`. The content comes from `gstack-harness ... --print` run under the caller's `HOME`, so it may not be byte-identical, and the committed receipt's `router_sha256` would then no longer match the file. Fix: decide the destination for the router the same way as for the receipt, or write it after `ok` is known. FIXABLE. Severity: Low. Blocks: no. + +6. **`receiptOut` treats an unreadable receipt as permission to overwrite it** (`qm-stack.ts:156-160`). A committed receipt that is missing, has bad JSON, or lacks `gstack.ref` gives `committedRef === null`. Any default run at any ref then overwrites the committed path. Today both committed receipts carry the full sha `21277291c850...` under `gstack.ref`, which matches the newly normalised `ref`, so this is latent. INVESTIGATE: decide whether a parse failure should go to `work` instead. Severity: Low. Blocks: no. + +I checked these and found them sound: the lock now runs before the opening `compose down`; the signed `whoami` check (the committed smoke receipt shows `/v1/admin/whoami` answers 200 at this QM pin); running `prepare.sh` before the lock (it only creates directories and prints values); `refusal` being null after a successful import; `--ref` now resolving to a full commit sha; and the CHANGELOG count (61 skills plus `connect-chrome` plus the root file makes 63). + +Recommendation: Fix the sandbox adoption filter at qm-stack.ts:106 before merge because a failed s1 create runs `docker rm -f` and `volume rm -f` on every new `qm.sandbox=1` container on the host, including a live trial's; the change is one filter and everything else can ship as-is. \ No newline at end of file diff --git a/test/fixtures/review-ledger/harn15-ship2-p2-fix-check.txt b/test/fixtures/review-ledger/harn15-ship2-p2-fix-check.txt new file mode 100644 index 000000000..8f35182c6 --- /dev/null +++ b/test/fixtures/review-ledger/harn15-ship2-p2-fix-check.txt @@ -0,0 +1,12 @@ +None of the fixes blocks landing; I found four INFORMATIONAL defects and no CRITICAL. Three are in the one-run lock and sandbox-ownership fixes (3 and 6), and one is in the receipt-routing fix (9). + +Checks I ran: +- **Whoami (fix 5):** the endpoint exists. The committed smoke receipt shows `GET /v1/admin/whoami` at http 200 as `org_admin`. +- **Sandbox names (fix 6):** QM's `machineId` really is `qm-sbx-…`. The committed s1 receipt has `qm-sbx-sandbox-949f45aa-…-1b711b`, so `own()` accepts the success path. +- **Image pin check (fix 7):** passing `QM_SANDBOX_IMAGE=harness-eval/qm-sandbox:gstack-` to `prepare.sh` doesn't break s1. `Dockerfile.skills` builds `FROM qm-sandbox:dev`, so the image inherits the `harness-eval.commit` label. +- **Receipt ref match (fixes 8 and 9):** both committed receipts record the full 40-character `gstack.ref`, so the full sha from `--verify` matches them. + +{"severity":"INFORMATIONAL","path":"harnesses/qm/gstack/qm-stack.ts","line":108,"summary":"adoptNew() owns every qm.sandbox=1 container that appeared while create() ran. When the create call fails or returns no machineId (http 0, a 5xx, or a body without machineId), teardown runs `docker rm -f` and removes the qm-home-/qm-net- volume and network of any sandbox a concurrent trial, or a second stack on other ports, created in that window. The lock error invites a second stack on other ports, and README and the Stack doc comment still promise 'never another stack's'."} +{"severity":"INFORMATIONAL","path":"harnesses/qm/gstack/qm-stack.ts","line":123,"summary":"adoptNew() runs inside down()'s finally and calls own(), which throws for any qm.sandbox=1 container not named qm-sbx-*. If that happens during a cut-off createSandbox (SIGINT mid-call), removeSandboxes, compose down, unlock and the handler removal are all skipped. pg and core are left running and the lock file stays behind, and the next run refuses until it finds the dead pid. `creating` is also never reset, so later down() calls are no-ops because `stopping` is already true."} +{"severity":"INFORMATIONAL","path":"harnesses/qm/gstack/s1-stage.ts","line":114,"summary":"The one-run lock is taken inside upStack, after main has already mutated the shared --work directory. A second s1 run with the same --work is refused only after stage() has run rmSync on /context and docker build has re-tagged harness-eval/qm-sandbox:gstack-. The live run then reads its staged texts from /context (stagedText) and can hit ENOENT or rewritten files, and its receipt records the new image id. smoke.ts:114 likewise truncates the live run's fake-requests.ndjson before it is refused. This contradicts README's 'refuses a second run while the first is alive'."} +{"severity":"INFORMATIONAL","path":"harnesses/qm/gstack/s1-stage.ts","line":117,"summary":"Fix 9 sends a failed default run's receipt into --work, but routerOut is still computed beside `out` and written before the outcome is known. A failed default s1 run at the committed ref therefore overwrites receipts/s1-router-.md in the repository, and its --work receipt's gstack.router_file points at that committed file."} \ No newline at end of file diff --git a/test/fixtures/review-ledger/harn15-ship2-p2-red-team.txt b/test/fixtures/review-ledger/harn15-ship2-p2-red-team.txt new file mode 100644 index 000000000..b5116e3c5 --- /dev/null +++ b/test/fixtures/review-ledger/harn15-ship2-p2-red-team.txt @@ -0,0 +1,13 @@ +I found three INFORMATIONAL issues and nothing CRITICAL. All three are in the fix commit `cdce7c76d`, which fixes the earlier findings. The new code behind each one is the lock, the sandbox adoption and the receipt-overwrite gate. + +**Fixture coverage was reduced.** I read `tests/qm-gstack-smoke.test.ts` in summary mode only (+39/-1). It adds four tests: the outcome success path, `lockHost`, `receiptOut`, and the http 0 transport failure. No test covers `createSandbox`, `adoptNew` or the teardown order in `down()`. + +**Where the QM facts come from.** QM's container naming and labels come from the QM source copy at `~/.capy/work/local-sandbox.ts`. I assumed it matches the b6786d67 pin. + +**gstack check on this machine:** `gstack: ok 1.91.366.0 require=claude,codex,patch` (3 behind the latest, 1.91.369.0; above the 1.91.364.0 floor). + +```json +{"severity":"INFORMATIONAL","confidence":7,"path":"harnesses/qm/gstack/qm-stack.ts","line":108,"category":"red-team","summary":"adoptNew lists qm.sandbox=1 containers host-wide with no qm.org or qm.scope filter, and calls own(), which throws on any name outside qm-sbx-*. QM's scratch sandboxes (local-sandbox.ts:314-323, runContainer(qm-scratch-, undefined, false)) carry the same qm.sandbox=1 label. Failure 1: if any qm-scratch-* container appears on the host while a create call fails or returns no machineId, own() throws partway through the loop and `creating` is never reset. down() then calls adoptNew() first in its finally, so it throws again and skips removeSandboxes, compose down, unlock and the signal-handler removal. That brings back the teardown bypass outside-voice#3 reported, now triggered by another process's container. Failure 2: a live trial's freshly created qm-sbx-* sandbox (also org harness-eval) that appears in the same window, up to the 300 s client timeout, is adopted and removed with docker rm -f, its qm-home volume and its qm-net network. This contradicts the updated README line 'never another stack's' and brings back a narrower version of the label sweep that was dropped to protect live trials.","fix":"Narrow the diff to this call: filter on label=qm.org=harness-eval plus label=qm.scope=. QM names the container deterministically from the scope (localContainerName = qm-sbx-), so an exact name or scope match is possible. In adoptNew, skip names that fail the regex instead of throwing, and reset `creating` in a finally. In down(), wrap adoptNew in its own try so removeSandboxes, compose down and unlock always run. Add a fake-docker test where a qm-scratch-* name appears during a failed create, and assert that compose down still runs.","fingerprint":"harnesses/qm/gstack/qm-stack.ts:108:red-team","specialist":"red-team"} +{"severity":"INFORMATIONAL","confidence":7,"path":"harnesses/qm/gstack/qm-stack.ts","line":102,"category":"red-team","summary":"The host lock is taken inside upStack, after the caller has already modified the shared default --work directory. Both scripts default to fixed work directories (~/.capy/work/qm-gstack-smoke and qm-gstack-s1). Before reaching the lock, a second run truncates fake-requests.ndjson (smoke.ts:114, s1-stage.ts:135). s1 has also already run stage(), which does rmSync(work/context) and rewrites it (s1-stage.ts:114), rewritten the router, and re-tagged the overlay image. The run that holds the lock then reads the clobbered state after its turn. smoke's fake_model_requests counts lines in the truncated log. s1's stagedText() reads work/context/gstack/..., which is missing or from another ref if the second run failed after its rmSync, so the first run crashes with no receipt or records the wrong comparison. The lock only refuses the second run after this damage is done. The lock itself is also not atomic: it reads, checks liveness and then writes without O_EXCL, so two runs started together can both take it. It also lives under tmpdir(), so runs launched with different TMPDIR values (env -i or not) never see each other's locks.","fix":"Take the lock at the top of main(), before touching --work, and key it on the resolved work directory as well as the project and ports. Create each lock file with writeFileSync(path, pid, { flag: 'wx' }), and take over a dead owner's lock only through unlink-and-retry. Use a fixed lock directory such as /tmp or the work root, not tmpdir().","fingerprint":"harnesses/qm/gstack/qm-stack.ts:102:red-team","specialist":"red-team"} +{"severity":"INFORMATIONAL","confidence":6,"path":"harnesses/qm/gstack/qm-stack.ts","line":156,"category":"red-team","summary":"receiptOut lets a default run overwrite the committed receipt whenever the gstack ref matches, and nothing else is checked. The smoke's success gate is only result.registered (smoke.ts:154). Case 1: `smoke.ts --core-url ... --secret-file ...` at 21277291 replaces receipts/smoke-b6786d67.json with qm.commit null and an unchecked external core, while the file name and CHANGELOG still claim QM b6786d67. Case 2: a default run at the same ref where core can't reach GitHub, or QM's refusal changes, still registers the pack. It then rewrites the published refusal_kind bytes_cap receipt as clone_timeout or other and exits 0, so the README and CHANGELOG cite numbers the committed receipt no longer shows. s1 has the same gap: `ok` checks only the complete read, so a run whose over-cap read (the cited 16,384 cut) fails or changes still overwrites. Also, if pin.json later moves QM, the b6786d67-named file is overwritten with a receipt from the new QM build.","fix":"Overwrite the committed path only when the run's qm.commit equals the pinned commit (refuse when it is null, as with --core-url) and the outcome fields the docs cite (smoke: refusal_kind and refusal; s1: complete_read.identical and over_cap_read.cut) match the committed receipt. Otherwise write to / and print the difference. Or simply require an explicit --out to touch the committed receipts.","fingerprint":"harnesses/qm/gstack/qm-stack.ts:156:red-team","specialist":"red-team"} +``` \ No newline at end of file diff --git a/test/fixtures/review-ledger/harn15-ship2-red-team.txt b/test/fixtures/review-ledger/harn15-ship2-red-team.txt new file mode 100644 index 000000000..04981871d --- /dev/null +++ b/test/fixtures/review-ledger/harn15-ship2-red-team.txt @@ -0,0 +1,9 @@ +Red-team pass is done: I found three INFORMATIONAL issues and no CRITICAL ones. Both earlier specialist findings are already fixed in the working tree. `outcome()` now sets refusal to null when the import succeeds, and both suggested tests are in `tests/qm-gstack-smoke.test.ts`. That file passes 14/14 under `env -i`. + +**Coverage note:** the one test file (`tests/qm-gstack-smoke.test.ts`, +175, 14 tests) was reviewed in summary mode only. It covers request signing, the `upStack` dummy-key refusal, the smoke call sequence against a fake QM, refusal classification, pack sizing and the S1 staging helpers. All other changed files were read in full. Those are `qm-stack.ts`, `smoke.ts`, `s1-stage.ts`, `Dockerfile.skills`, both READMEs, the receipts, CHANGELOG, VERSION and package.json. I also checked `compose.yaml`, `prepare.sh`, `capture.sh` and `fake-anthropic.ts` at the integration boundaries. The only `sk-` token in the receipts is the `sk-ant-capture-dummy` prefix. + +``` +{"severity":"INFORMATIONAL","confidence":8,"path":"harnesses/qm/gstack/s1-stage.ts","line":102,"category":"red-team","summary":"If you rerun either script the way the README says, it overwrites the committed receipt with a different gstack commit, even when the run fails. --ref defaults to the checkout HEAD, but --out is a fixed repository path named only after the QM pin (smoke.ts:105, s1-stage.ts:102). Both scripts write the receipt unconditionally (smoke.ts:154, and s1-stage.ts:200 before the ok check). This machine's ~/.local/share/gstack is already at 12dc6782 (1.91.366.0), not the receipt's 21277291 (1.91.345.0). A default rerun today rewrites s1-staging-b6786d67.json against new gstack, adds s1-router-12dc6782.md next to it, and leaves the README's 4,727 / 29,581 / 17-of-63 / 12,186 figures pointing at a receipt that no longer shows them. To reproduce the published receipt, s1-stage's own refusal says to check out 21277291 in the live gstack install, which puts that machine below the crew's 1.91.364.0 floor.","fix":"Default --ref to the pinned 21277291 commit, and tell users in the README and in the refusal message to pass --gstack pointing at a separate `git worktree add 21277291` instead of checking out the live install. Put the gstack ref in the receipt file name, or refuse to overwrite an existing --out whose gstack.ref differs. On failure, write the receipt to / rather than to the committed path.","fingerprint":"harnesses/qm/gstack/s1-stage.ts:102:red-team","specialist":"red-team"} +{"severity":"INFORMATIONAL","confidence":7,"path":"harnesses/qm/gstack/s1-stage.ts","line":56,"category":"red-team","summary":"The router gstack-harness prints has a defect that the fixture hides: it lists `open-gstack-browser` twice. One line points at `connect-chrome/SKILL.md`, a symlink that only gstack's setup creates and that is not in git. stage() follows the symlink, so the fixture serves that path, but a QM deployment built from the gstack tree would return `[no such file` there. Under the README's counting rule, that is a 'fixture fault, rerun', so a real router defect would never be counted. The duplicate also inflates the published counts. The receipt's router_skills=62 and the CHANGELOG/README '62 router skills' are 61 distinct skills plus one alias (the manifest has connect-chrome and open-gstack-browser with the same sha256 2dd3227c..., 18,560 chars). The 63 staged files hold the same text twice. The 17 that fit inline is unaffected, because both copies are over the cap.","fix":"Report it to GSTA-36 as a gstack finding: `register generic --print` emits a setup-only alias path under another skill's name. In the fixture, mark the alias in the manifest (alias_of: open-gstack-browser) and state the counts as 61 distinct skills plus 1 alias, or drop the alias line from the staged router. Count a request that routes to connect-chrome as a router defect, not a fixture fault.","fingerprint":"harnesses/qm/gstack/s1-stage.ts:56:red-team","specialist":"red-team"} +{"severity":"INFORMATIONAL","confidence":4,"path":"harnesses/qm/gstack/qm-stack.ts","line":107,"category":"red-team","summary":"The health check cannot tell this run's core from another process on the same port. Core runs with network_mode: host (compose.yaml) and only pg publishes a port. If another process already holds 127.0.0.1:58080 and 55432 is free, `compose up -d` still exits 0, because depends_on waits only for pg. This run's core then fails to bind, and /healthz is answered by whatever holds the port. Every signed call goes to that foreign service and gets refused or garbled answers, and with the default --out (finding 1) that bad receipt replaces the committed one. The trial rig uses the same default port pair.","fix":"After the health check passes, call the signed GET /v1/admin/whoami and confirm a 200 before continuing. Or check that `docker compose ps core` is running and was not restarted.","fingerprint":"harnesses/qm/gstack/qm-stack.ts:107:red-team","specialist":"red-team"} +``` \ No newline at end of file diff --git a/test/review-dispatch.test.ts b/test/review-dispatch.test.ts index b76de4a38..898711076 100644 --- a/test/review-dispatch.test.ts +++ b/test/review-dispatch.test.ts @@ -4,7 +4,7 @@ import * as fs from 'node:fs'; import * as os from 'node:os'; import * as path from 'node:path'; import { BriefError, promptHash, renderBrief, sectionTexts } from '../lib/review-brief'; -import { collect, judge, parseFindings, type MergedLine } from '../lib/review-ledger'; +import { collect, judge, parseFindings, readJsonl, type MergedLine } from '../lib/review-ledger'; const ROOT = path.resolve(import.meta.dir, '..'); const BIN = path.join(ROOT, 'bin', 'gstack-review-dispatch'); @@ -418,6 +418,78 @@ describe('gstack-review-dispatch sent, collect, judge and verify: nothing change .toEqual([['CRITICAL', '**Race in a.ts:3 (Medium).** | - the writer holds no lock | Severity: Medium. Blocks: yes']]); }); + // Value: protects=a severity word a negator directly scopes makes no finding (GST-435 rule (b)), so a reader's own "no CRITICAL" summary + // never collects as a blocker judge then refuses to drop: HARN-15's Red Team and fix-check replies (harness-evals #152 and the + // ship2 run: "I found three INFORMATIONAL issues and no CRITICAL ones", "nothing CRITICAL", "nothing at CRITICAL") and GEVA-77's + // fix-check closing line ("Neither finding is CRITICAL.") each collected a phantom CRITICAL on main; fails_when=any of those lines + // collects CRITICAL, or a listed negation shape collects a label; why_new=any label word anywhere on a line was a finding; + // seam=parseFindings, collect and judge; no_claim=every English negation (the corpus replay in the PR bounds the rest) + test('a severity word a negator directly scopes is no label: the HARN-15 summaries and the GEVA-77 closing line collect no CRITICAL', () => { + const fixture = (name: string) => fs.readFileSync(path.join(ROOT, 'test/fixtures/review-ledger', name), 'utf8'); + const replies: Array<[string, string, number]> = [['red-team', 'harn15-ship2-red-team.txt', 3], ['red-team', 'harn15-ship2-p2-red-team.txt', 3], + ['fix-check', 'harn15-ship2-p2-fix-check.txt', 4], ['fix-check', 'harn15-152-p2-fix-check.txt', 2]]; + for (const [reader, name, json] of replies) { + const found = parseFindings(reader, fixture(name)); + expect([name, found.map(f => f.label)]).toEqual([name, Array(json + 1).fill('INFORMATIONAL')]); + expect(found[0]!.raw).toMatch(/(no|nothing( at)?) CRITICAL/); + } + const fc = fixture('geva-77-fix-check.txt'); + expect(parseFindings('fix-check', fc).map(f => f.label)).toEqual(['INFORMATIONAL', 'INFORMATIONAL']); + const d = tmp(); + const merged = path.join(d, 'merged.jsonl'); + const r = collect(path.join(d, 'l.jsonl'), 'fix-check', fc, merged); + expect([r.review, r.lines.map(l => [l.id, l.label])]).toEqual([true, [['fix-check#1', 'INFORMATIONAL'], ['fix-check#2', 'INFORMATIONAL']]]); + const judged = path.join(d, 'judged.jsonl'); + fs.writeFileSync(judged, r.lines.map(l => JSON.stringify({ id: l.id, severity: 'INFORMATIONAL' })).join('\n') + '\n'); + expect(judge(readJsonl(merged), readJsonl(judged)).ok).toBe(true); + for (const line of ['This is not CRITICAL.', "This isn't really CRITICAL.", 'It cannot be CRITICAL.', "This doesn't rise to CRITICAL.", 'This is not **CRITICAL**.', + 'There are no CRITICAL findings.', 'There are no `CRITICAL` findings.', 'No CRITICAL or High findings to report.', 'I found nothing CRITICAL.', "So there's nothing at CRITICAL.", + 'Neither finding is CRITICAL.', 'Neither of the two findings is CRITICAL.', 'Nothing here is CRITICAL.', 'None of these is High.', 'Two nits, neither of which is CRITICAL.', + 'Two nits, none of which is High.', 'The race sits below CRITICAL on the floor.', 'It ranks lower than High.']) + expect([line, parseFindings('outside-voice', line)]).toEqual([line, []]); + for (const reader of ['adversarial-native', 'red-team']) expect(parseFindings(reader, 'Nothing here is FIXABLE, and nothing is CRITICAL.')).toEqual([]); + expect(parseFindings('outside-voice', 'Severity: Low (not High)').map(f => f.label)).toEqual(['Low']); + expect(parseFindings('outside-voice', '1. **High**: retry loop never backs off (q.ts:78)\n\nOverall: one High finding, nothing CRITICAL.').map(f => f.label)).toEqual(['High', 'High']); + }); + + // Value: protects=rule (b) never reaches a word some other negation sits near, a table row, or the native voice's FIXABLE, and a kept + // finding takes the strongest label on its line (GST-435 rule (a)): fix-check 5's two table rows and its two INFORMATIONAL cases + // ("None of the writes are atomic and are CRITICAL (queue.ts:78); High confidence", "Nothing guards the write path the failure of + // which is CRITICAL") stay CRITICAL, and harness-evals #152's native (FIXABLE, medium) findings stay CRITICAL (adversarial.md); + // fails_when=any of them collects nothing or a weaker label; why_new=the #3677 draft's quote and negated-subject rules lowered these; + // seam=parseFindings; no_claim=that every kept line is a real finding (judge folds phantoms) + test('a negation of something else, a table row and the native FIXABLE keep their strongest label', () => { + const kept: Array<[string, string]> = [['| 1 | `CRITICAL` | High | token leak in log.ts:3 |', 'CRITICAL'], ['| log.ts:3 | `CRITICAL` | High | token leak |', 'CRITICAL'], + ['None of the writes are atomic and are CRITICAL (queue.ts:78); High confidence', 'CRITICAL'], ['Nothing guards the write path the failure of which is CRITICAL', 'CRITICAL'], + ['| a.ts:3 | race | not CRITICAL, Low |', 'CRITICAL'], ['| a.ts:3 | maps `FIXABLE` to `CRITICAL` in a comment | Low |', 'CRITICAL'], + ['Neither path checks auth so this is CRITICAL (a.ts:3)', 'CRITICAL'], ['None of the inputs are sanitized so this is CRITICAL (a.ts:3)', 'CRITICAL'], + ['Nothing checks the token which is CRITICAL (a.ts:3)', 'CRITICAL'], ['No bounds check lets CRITICAL data leak (a.ts:3)', 'CRITICAL'], + ['No defects in parsing but the upload handler is CRITICAL.', 'CRITICAL'], ['1. Without auth this is CRITICAL (a.ts:3)', 'CRITICAL'], + ['Not checking expiry at auth.ts:9 lets stale tokens through. Severity: High', 'High'], ["The lock isn't released on error (pool.ts:12): High", 'High'], + ['A retry storm could escalate up to CRITICAL.', 'CRITICAL'], ['Low: the parser now maps `FIXABLE` to `CRITICAL` in a comment (a.ts:3)', 'CRITICAL']]; + for (const [line, label] of kept) expect([line, parseFindings('outside-voice', line).map(f => f.label)]).toEqual([line, [label]]); + const native = fs.readFileSync(path.join(ROOT, 'test/fixtures/review-ledger/harn15-152-p1-adversarial-native.txt'), 'utf8'); + expect(parseFindings('adversarial-native', native).map(f => f.label)).toEqual(['CRITICAL', 'INVESTIGATE', 'INVESTIGATE', 'CRITICAL', 'INVESTIGATE', 'INVESTIGATE', 'CRITICAL']); + expect(parseFindings('adversarial-native', '**1. Race in a.ts:3 (`FIXABLE`).**\nSeverity: Low. Blocks: no').map(f => f.label)).toEqual(['CRITICAL']); + expect(parseFindings('red-team', '1. Race in a.ts:3 (FIXABLE).').map(f => f.label)).toEqual(['FIXABLE']); + }); + + // Value: protects=a list item whose label first appears in an indented later paragraph ("Fix: ... FIXABLE. Severity: Low.") collects as + // that item, title included (HARN-15 ship2 native item 4 "The host lock is not atomic" collected as a finding that began "Fix: create + // the file", so judge could not tell which finding it was); fails_when=the item's finding loses its first line or the reply collects + // more or fewer than 6; why_new=#3677's detail merge covered margin bullets only, not an item's later paragraphs; seam=parseFindings; + // no_claim=a labeled item's labeled later paragraph (it stays its own finding, as above) + test('an indented later paragraph that first labels a list item collects as that item, title included', () => { + const native = fs.readFileSync(path.join(ROOT, 'test/fixtures/review-ledger/harn15-ship2-p2-adversarial-native.txt'), 'utf8'); + const found = parseFindings('adversarial-native', native); + expect(found.map(f => [f.label, f.raw.slice(0, 3)])).toEqual([['CRITICAL', '1. '], ['CRITICAL', '2. '], ['CRITICAL', '3. '], ['CRITICAL', '4. '], ['CRITICAL', '5. '], ['Low', '6. ']]); + expect(found[3]!.raw).toStartWith('4. **The host lock is not atomic** (`qm-stack.ts:66-76`).'); + expect(found[3]!.raw).toContain(' | Fix: create the file with `writeFileSync(path, pid, { flag: \'wx\' })`.'); + expect(parseFindings('outside-voice', '1. **Race on the lock** (a.ts:3)\n - the writer holds no lock\n\n Fix: take it first. Severity: High. Blocks: no.').map(f => [f.label, f.raw])) + .toEqual([['High', '1. **Race on the lock** (a.ts:3) | Fix: take it first. Severity: High. Blocks: no.']]); + expect(parseFindings('outside-voice', '1. Lock race (a.ts:3)\n\n Fix: take it first. Blocks: yes\n2. Cache drift (c.ts:4)').map(f => f.label)).toEqual(['CRITICAL']); + }); + // Value: protects=a non-review the same dispatch later answers with a review is voided (counted, VERIFY clean), and a failed outside // voice recorded with --unavailable reads UNAVAILABLE (missing coverage, never a review) without blocking, while --unavailable is // refused for any other reader; fails_when=one outside-voice timeout or one refusal makes VERIFY dirty for good, or --unavailable