mirror of
https://github.com/garrytan/gstack.git
synced 2026-11-02 06:54:50 +01:00
v1.92.36.0 fix(review): keep every severity label, so a blocker after "no CRITICAL issues except" is no longer lost (GSTA-167 S1) (#3803)
* test(review): a blocker in an exception clause after a negated severity word collects at its label (GSTA-167 S1, failing first) Adds the six exception-clause probes that collect nothing on8cef24ce, GST-435's label-keeping probes and fixtures (PR #3703, read as text), and turns GST-766's negated-label test into a kept-label test. Co-authored-by: capy-ai[bot] <230910855+capy-ai[bot]@users.noreply.github.com> * fix(review): revert GST-766's negation drop; collect keeps every label as before8cef24ce, stated stays (GSTA-167 S1) Co-authored-by: capy-ai[bot] <230910855+capy-ai[bot]@users.noreply.github.com> * fix: pre-landing review fixes (GSTA-167 S1: a closing-summary phantom stays at its label in the judged set and folds only into a finding at least as severe) Co-authored-by: capy-ai[bot] <230910855+capy-ai[bot]@users.noreply.github.com> --------- Co-authored-by: capy-ai[bot] <230910855+capy-ai[bot]@users.noreply.github.com>
This commit is contained in:
1 parent
4f5b043b3e
commit
216493fb32
14 files changed
+263
-46
No files matched your search
@@ -1,5 +1,18 @@
|
||||
# Changelog
|
||||
|
||||
## [1.92.36.0] - 2026-11-01
|
||||
|
||||
**/ship's review collector keeps every severity label again, so a blocker named after "no CRITICAL issues except" is no longer lost.** v1.92.29.0 (GST-766) skipped any severity word that followed "no", "nothing", "none" and similar words, to stop a reviewer's closing line like "found nothing CRITICAL" from counting as a finding. That also threw away real blockers named in an exception clause: "No CRITICAL issues except the token leak in log.ts:3", "Nothing CRITICAL apart from the auth bypass (auth.ts:9)" and four more wordings collected nothing at all, so the gate never saw them. The collector now reads labels exactly as it did before v1.92.29.0. The reviewer's own Severity and Blocks fields are still recorded beside the label (`stated`), as in v1.92.29.0.
|
||||
|
||||
### Known
|
||||
- **A reviewer's closing summary collects at its label again.** Lines like "found nothing CRITICAL" or "Neither finding is CRITICAL." collect as CRITICAL, the same as before v1.92.29.0. They are known phantoms. Each stays CRITICAL in the judged set (lead judgment can fold it only into a finding at least as severe), so it counts toward what blocks. A lost blocker costs more than an extra one.
|
||||
|
||||
### Tests
|
||||
- `test/review-dispatch.test.ts`: the six exception-clause wordings collect CRITICAL for every reader. GST-766's test that a negated label collects nothing now checks that each of those lines keeps its label. GST-435's label-keeping probes (table rows, FIXABLE, negated subjects, closing summaries kept as known phantoms) are added with their HARN-15 and GEVA-77 fixtures under `test/fixtures/review-ledger/`.
|
||||
|
||||
### Docs
|
||||
- `docs/review-policy.md` (collect) says a negated severity word still collects at its label.
|
||||
|
||||
## [1.92.35.0] - 2026-11-01
|
||||
|
||||
- **A `.gstack/pregate.json` that only raises `timeout_s` no longer changes lane verdicts, `warn: ["lanes"]` no longer lets a failing test pass, and a `[]` declaration no longer skips a deleted test's suite** (GST-775, from gbrain-evals #272 and GBEV-22).
|
||||
|
||||
@@ -1,4 +1,4 @@
|
||||
# gstack digest v1.92.35.0 — regenerate/re-copy after upgrading gstack
|
||||
# gstack digest v1.92.36.0 — regenerate/re-copy after upgrading gstack
|
||||
|
||||
Behavioral rules from gstack (https://github.com/garrytan/gstack), compressed
|
||||
for agent hosts without a full skill install. The full skills add workflows,
|
||||
|
||||
@@ -469,7 +469,7 @@ ones, and the arm lost blockers. `bin/gstack-review-dispatch` closes both ends:
|
||||
3. `collect` hashes each reader's raw output and appends its findings to
|
||||
`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 outside a negation (below), headings and table rows included; a top-level list item under a
|
||||
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
|
||||
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,
|
||||
@@ -478,11 +478,11 @@ 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.
|
||||
A label a negation word governs ("found nothing CRITICAL", "no new **High** or Critical issues", "No P0/P1 issues":
|
||||
no, nothing, not, none, zero, without or never, optionally followed by new, remaining, other, further, additional, more
|
||||
or at, through Markdown, and carried to labels joined by a comma, slash, or, nor or and) is no finding unless it opens a
|
||||
field (`High:`, GST-766). A line whose labels are all negated closes the open finding and heads the next, so a finding
|
||||
worded around a negated label still arrives through its Severity or Blocks field. Each prose finding's line also
|
||||
A negated severity word still collects at its label ("found nothing CRITICAL", "No P0/P1 issues"), so a reader's
|
||||
closing summary is a known phantom: it stays at that label in the judged set, lead judgment can fold it only into a
|
||||
finding at least as severe, and it counts toward what blocks. GST-766 skipped negated labels and lost
|
||||
real blockers named in an exception clause ("No CRITICAL issues except the token leak in log.ts:3"); GSTA-167
|
||||
reverted that. Each prose finding's line also
|
||||
records the Severity and Blocks its own text states as `stated`, beside the collected label and never in its place;
|
||||
a classified finding that carries `stated` shows it in its deferred-review issue next to the label ("CRITICAL (the
|
||||
reviewer wrote Severity: Low, Blocks: no)"). /ship's Step 9.4 does not copy `stated` into its findings file yet: that
|
||||
|
||||
+1
-21
@@ -59,14 +59,6 @@ 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 = /(?<![A-Za-z0-9])Blocks:?\**:?\s*\**\s*yes(?![A-Za-z0-9])/i;
|
||||
const ALL_LABELS = new RegExp(ANY_LABEL.source, 'g');
|
||||
/**
|
||||
* A label a negation word governs ("found nothing CRITICAL", "no new **High** or Critical issues", "No P0/P1 issues") is no finding
|
||||
* (GST-766), unless it opens a field ("High:"). Such a line still closes the open finding and heads the next, so a prose finding
|
||||
* phrased around a negated label reaches merged.jsonl through its required Severity or Blocks field.
|
||||
*/
|
||||
const NEGATED = /(?<![A-Za-z0-9-])(?:no|nothing|not|none|(?<!\bby\s)zero|without|never)\s+(?:(?:new|remaining|other|further|additional|more|at)\s+)?$/i;
|
||||
const NEGATION_LIST = /^\s*(?:,|\/|or|nor|and|,\s*(?:or|nor|and))\s*$/i;
|
||||
const MARKUP = /[*_`[\]]/g;
|
||||
/** 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"). */
|
||||
@@ -105,18 +97,7 @@ export function parseFindings(reader: string, output: string): Array<{ label: st
|
||||
let treeSeen = false;
|
||||
const role = reader.split('/')[0];
|
||||
const named = (l: string) => (role === 'adversarial-native' && l.toUpperCase() === 'FIXABLE' ? 'CRITICAL' : l);
|
||||
const asserted = (t: string) => {
|
||||
const all = [...t.matchAll(ALL_LABELS)];
|
||||
let end = -1, negated = false;
|
||||
return all.filter(m => {
|
||||
const at = m.index!;
|
||||
negated = !/^\s*:/.test(t.slice(at + m[0].length, at + m[0].length + 8).replace(MARKUP, ''))
|
||||
&& (NEGATED.test(t.slice(Math.max(0, at - 60), at).replace(MARKUP, '')) || (negated && NEGATION_LIST.test(t.slice(end, at).replace(MARKUP, ''))));
|
||||
end = at + m[0].length;
|
||||
return !negated;
|
||||
});
|
||||
};
|
||||
const strongest = (t: string) => asserted(t).map(m => named(m[1]!)).reduce<string | undefined>((a, b) => (a === undefined ? b : stronger(a, b)), undefined);
|
||||
const strongest = (t: string) => [...t.matchAll(ALL_LABELS)].map(m => named(m[1]!)).reduce<string | undefined>((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();
|
||||
@@ -151,7 +132,6 @@ export function parseFindings(reader: string, output: string): Array<{ label: st
|
||||
if (typeof o.severity === 'string') { current = push(o.severity, t); continue; }
|
||||
} catch { /* prose that starts with a brace */ }
|
||||
}
|
||||
if (!label && !blocks && !(section && listItem) && ANY_LABEL.test(t)) { current = null; head = t; continue; }
|
||||
const detail = listItem && /^[-*+]\s/.test(t) && !locus && !label && !section && !!current && !current.list && !current.fields.size;
|
||||
if (detail && !blocks) { current!.raw += ` | ${t}`; continue; }
|
||||
const fresh = (listItem && !detail) || (!indented && !detail && (t.startsWith('**') || afterGap)) || (!!current?.fields.size && locus);
|
||||
|
||||
+1
-1
@@ -1,6 +1,6 @@
|
||||
{
|
||||
"name": "gstack",
|
||||
"version": "1.92.35",
|
||||
"version": "1.92.36",
|
||||
"description": "Garry's Stack — Claude Code skills + fast headless browser. One repo, one install, entire AI engineering workflow.",
|
||||
"license": "MIT",
|
||||
"type": "module",
|
||||
|
||||
@@ -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: <v> 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.
|
||||
@@ -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 (...): <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.
|
||||
@@ -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)
|
||||
@@ -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=<sandboxImage>` to the adoption query. s1's image `harness-eval/qm-sandbox:gstack-<ref8>` 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.
|
||||
@@ -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-<ref8>` 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 <work>/context and docker build has re-tagged harness-eval/qm-sandbox:gstack-<ref8>. The live run then reads its staged texts from <work>/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-<ref8>.md in the repository, and its --work receipt's gstack.router_file points at that committed file."}
|
||||
@@ -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-<key>, 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=<the scope being created>. QM names the container deterministically from the scope (localContainerName = qm-sbx-<slug(scopeId)>), 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 <work>/ 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"}
|
||||
```
|
||||
@@ -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 <dir> 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 <work>/ 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"}
|
||||
```
|
||||
+116
-16
@@ -4,7 +4,7 @@ import * as fs from 'node:fs';
|
||||
import * as os from 'node:os';
|
||||
import * as path from 'node:path';
|
||||
import { BriefError, checkReviewedTree, promptHash, renderBrief, sectionTexts, workspaceParagraph } 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');
|
||||
@@ -249,10 +249,23 @@ describe('gstack-review-dispatch sent, collect, judge and verify: nothing change
|
||||
expect(judge(m, [{ id: 'r#1', severity: 'INFORMATIONAL', reason: 'a real defect' }]).ok).toBe(true);
|
||||
});
|
||||
|
||||
// Value: protects=a reader's closing prose that negates a severity word ("found nothing CRITICAL") is not collected as a finding,
|
||||
// while a label stated as a finding still is; fails_when=collect records the negated word as a CRITICAL the judge cannot drop;
|
||||
// why_new=GST-766: GEVA-92's fix-check#5 on gstack-evals #108 (1.92.17.0) became a phantom CRITICAL; seam=parseFindings; no_claim=prose voices' summaries that state a count
|
||||
test('a negated severity word is not a finding: GEVA-92 fix-check#5 and its negated forms', () => {
|
||||
// Value: protects=a blocker named in an exception clause after a negated severity word collects at its label (GSTA-167 S1: GST-766's
|
||||
// negation drop collected nothing for each of these on 8cef24ce, where 8cef24ce^ collected CRITICAL); fails_when=any probe collects
|
||||
// nothing or a weaker label; why_new=GST-766 skipped any label a negation word governs and lost these blockers; seam=parseFindings;
|
||||
// no_claim=that the reader's negated wording is clear (lead judgment reads the raw line)
|
||||
test('a blocker in an exception clause after a negated severity word collects at its label', () => {
|
||||
for (const prose of ['No CRITICAL issues except the token leak in log.ts:3', 'There is no CRITICAL issue other than the SQL injection at db.ts:12',
|
||||
'Nothing CRITICAL apart from the auth bypass (auth.ts:9).', 'No new CRITICAL findings, but the token leak in log.ts:3 still blocks.',
|
||||
'None CRITICAL besides the race in queue.ts:40, which loses writes.', '- **No CRITICAL** issues remain except F2 (store.ts:22 corrupts data).'])
|
||||
for (const reader of ['outside-voice', 'fix-check', 'red-team', 'adversarial-native'])
|
||||
expect([prose, reader, parseFindings(reader, prose).map(f => f.label)]).toEqual([prose, reader, ['CRITICAL']]);
|
||||
});
|
||||
|
||||
// Value: protects=a negated severity word still collects at its label, so a reader's closing prose ("found nothing CRITICAL") is a
|
||||
// known phantom lead judgment folds and never a dropped line (GSTA-167 S1 reverts GST-766's negation drop); fails_when=any negated
|
||||
// form collects nothing or a weaker label; why_new=GST-766's drop lost blockers named in exception clauses; seam=parseFindings;
|
||||
// no_claim=that a negated line is a real finding (judge folds phantoms)
|
||||
test('a negated severity word still collects at its label: GEVA-92 fix-check#5 and its negated forms are known phantoms', () => {
|
||||
const fixCheck = [
|
||||
'REVIEWED_TREE: bedd511efa0fa16c9277821073c68e438dc760d2 /workspace/gstack-evals',
|
||||
'{"severity":"INFORMATIONAL","path":"wiki/board/README.md","line":229,"summary":"Section 9\'s opening still says every ruling is recorded in the approvals file."}',
|
||||
@@ -262,19 +275,22 @@ describe('gstack-review-dispatch sent, collect, judge and verify: nothing change
|
||||
'- The new secondary: 47/60, 15/60, 5/40, 7/40, 4/40 and 11.5/40.',
|
||||
'- The minimum p of 2^-15.',
|
||||
].join('\n');
|
||||
expect(parseFindings('fix-check', fixCheck).map(f => f.label)).toEqual(['INFORMATIONAL', 'INFORMATIONAL']);
|
||||
for (const prose of ['Found no CRITICAL issues in the fix range.', 'Nothing CRITICAL or HIGH remains.', 'This is not CRITICAL.', 'I found zero Critical findings, and none High.', 'The retry path is safe; without CRITICAL impact.',
|
||||
'No new CRITICAL issues introduced by the fix.', 'No remaining CRITICAL or HIGH findings.', '**No** CRITICAL issues found.', 'No **CRITICAL** issues found.', 'No `CRITICAL` issue.', 'No [P1] issues remain.', 'Nothing at CRITICAL severity remains.',
|
||||
'No CRITICAL regressions introduced by the fix.', 'Nothing CRITICAL to report.', 'There is nothing CRITICAL in the fix range.', 'No CRITICAL — the fix is sound.', 'Found no CRITICAL or HIGH regressions.', 'No HIGH-risk changes.',
|
||||
'No CRITICAL/HIGH findings.', 'No P0/P1 issues.', 'No CRITICAL, HIGH or MEDIUM issues.', 'Checked collect() at lib/review-ledger.ts:189; no CRITICAL issues remain.', 'No CRITICAL issues; served on localhost:3000.'])
|
||||
expect([prose, parseFindings('fix-check', prose)]).toEqual([prose, []]);
|
||||
expect(parseFindings('r', 'Not CRITICAL, but High: the cache drops a row (c.ts:4)').map(f => f.label)).toEqual(['High']);
|
||||
expect(parseFindings('r', 'No CRITICAL, High: the cache drops a row (c.ts:4)').map(f => f.label)).toEqual(['High']);
|
||||
expect(parseFindings('r', 'Nothing CRITICAL / HIGH: race in q.ts:7').map(f => f.label)).toEqual(['HIGH']);
|
||||
expect(parseFindings('fix-check', fixCheck).map(f => f.label)).toEqual(['INFORMATIONAL', 'INFORMATIONAL', 'CRITICAL']);
|
||||
const kept: Array<[string, string]> = [['Found no CRITICAL issues in the fix range.', 'CRITICAL'], ['Nothing CRITICAL or HIGH remains.', 'CRITICAL'], ['This is not CRITICAL.', 'CRITICAL'],
|
||||
['I found zero Critical findings, and none High.', 'Critical'], ['The retry path is safe; without CRITICAL impact.', 'CRITICAL'], ['No new CRITICAL issues introduced by the fix.', 'CRITICAL'],
|
||||
['No remaining CRITICAL or HIGH findings.', 'CRITICAL'], ['**No** CRITICAL issues found.', 'CRITICAL'], ['No **CRITICAL** issues found.', 'CRITICAL'], ['No `CRITICAL` issue.', 'CRITICAL'],
|
||||
['No [P1] issues remain.', 'P1'], ['Nothing at CRITICAL severity remains.', 'CRITICAL'], ['No CRITICAL regressions introduced by the fix.', 'CRITICAL'], ['Nothing CRITICAL to report.', 'CRITICAL'],
|
||||
['There is nothing CRITICAL in the fix range.', 'CRITICAL'], ['No CRITICAL — the fix is sound.', 'CRITICAL'], ['Found no CRITICAL or HIGH regressions.', 'CRITICAL'], ['No HIGH-risk changes.', 'HIGH'],
|
||||
['No CRITICAL/HIGH findings.', 'CRITICAL'], ['No P0/P1 issues.', 'P0'], ['No CRITICAL, HIGH or MEDIUM issues.', 'CRITICAL'],
|
||||
['Checked collect() at lib/review-ledger.ts:189; no CRITICAL issues remain.', 'CRITICAL'], ['No CRITICAL issues; served on localhost:3000.', 'CRITICAL']];
|
||||
for (const [prose, label] of kept) expect([prose, parseFindings('fix-check', prose).map(f => f.label)]).toEqual([prose, [label]]);
|
||||
expect(parseFindings('r', 'Not CRITICAL, but High: the cache drops a row (c.ts:4)').map(f => f.label)).toEqual(['CRITICAL']);
|
||||
expect(parseFindings('r', 'No CRITICAL, High: the cache drops a row (c.ts:4)').map(f => f.label)).toEqual(['CRITICAL']);
|
||||
expect(parseFindings('r', 'Nothing CRITICAL / HIGH: race in q.ts:7').map(f => f.label)).toEqual(['CRITICAL']);
|
||||
expect(parseFindings('r', 'No CRITICAL lock guards the write in Makefile:7\nSeverity: CRITICAL').map(f => f.label)).toEqual(['CRITICAL']);
|
||||
expect(parseFindings('r', '1. auth.ts:3 LOW: log line too verbose\nNo CRITICAL lock guards the write in Makefile:7\nSeverity: CRITICAL').map(f => [f.label, f.raw])).toEqual([['LOW', '1. auth.ts:3 LOW: log line too verbose'], ['CRITICAL', 'No CRITICAL lock guards the write in Makefile:7 | Severity: CRITICAL']]);
|
||||
expect(parseFindings('r', '1. auth.ts:3 LOW: log line too verbose\nNo CRITICAL issues remain.').map(f => f.label)).toEqual(['LOW']);
|
||||
expect(parseFindings('outside-voice', '- a.ts:3 race. Severity: Low. Blocks: no\n Separately, nothing CRITICAL about b.ts:9, but it leaks fds.\n Severity: High').map(f => [f.label, f.raw])).toEqual([['Low', '- a.ts:3 race. Severity: Low. Blocks: no'], ['High', 'Separately, nothing CRITICAL about b.ts:9, but it leaks fds. | Severity: High']]);
|
||||
expect(parseFindings('r', '1. auth.ts:3 LOW: log line too verbose\nNo CRITICAL issues remain.').map(f => f.label)).toEqual(['LOW', 'CRITICAL']);
|
||||
expect(parseFindings('outside-voice', '- a.ts:3 race. Severity: Low. Blocks: no\n Separately, nothing CRITICAL about b.ts:9, but it leaks fds.\n Severity: High').map(f => [f.label, f.raw])).toEqual([['Low', '- a.ts:3 race. Severity: Low. Blocks: no'], ['CRITICAL', 'Separately, nothing CRITICAL about b.ts:9, but it leaks fds. | Severity: High']]);
|
||||
expect(parseFindings('r', '1. A CRITICAL race in queue.ts:78 that no lock covers').map(f => f.label)).toEqual(['CRITICAL']);
|
||||
expect(parseFindings('r', '- A divide by zero CRITICAL bug in parse()').map(f => f.label)).toEqual(['CRITICAL']);
|
||||
expect(parseFindings('adversarial-native', 'FIXABLE: no timeout on the fetch in a.ts:3').map(f => f.label)).toEqual(['CRITICAL']);
|
||||
@@ -484,6 +500,90 @@ describe('gstack-review-dispatch sent, collect, judge and verify: nothing change
|
||||
expect(json.lines.map(l => [l.label, l.stated])).toEqual([['INFORMATIONAL', undefined]]);
|
||||
});
|
||||
|
||||
// Value: protects=collect keeps every label on a prose line, at the strongest one (GST-435 rule (a)): each line that three review
|
||||
// rounds found losing its label under a negation-drop draft (an exception clause, a negated subject, "else"/"other", a blocker named in an
|
||||
// earlier sentence), fix-check 5's table rows and INFORMATIONAL cases, and harness-evals #152's native (FIXABLE, medium) findings collect
|
||||
// at their label (adversarial.md); fails_when=any of them collects nothing or a weaker label; why_new=GST-435's negation drops lost a
|
||||
// blocker in each round and were removed; seam=parseFindings; no_claim=that every kept line is a real finding (judge folds phantoms)
|
||||
test('collect keeps every label at the strongest on its line: no wording drops a blocker', () => {
|
||||
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'],
|
||||
['No input validation is CRITICAL (upload.ts:4)', 'CRITICAL'], ['No CSRF token is a CRITICAL gap at form.ts:12', 'CRITICAL'],
|
||||
['Nothing escaped here is CRITICAL: stored XSS at view.ts:8', 'CRITICAL'], ['No timeout here is High (fetch.ts:3)', 'High'],
|
||||
['Upgraded: not High, CRITICAL (a.ts:3)', 'CRITICAL'], ['Severity: not Medium, CRITICAL', 'CRITICAL'],
|
||||
['Nothing else is CRITICAL except the token leak in log.ts:3.', 'CRITICAL'], ['No other issue is CRITICAL besides the auth bypass in auth.ts:9.', 'CRITICAL'],
|
||||
['None of the other findings are CRITICAL, but the SQL injection in db.ts:12 is.', 'CRITICAL'],
|
||||
['Nothing here is CRITICAL apart from the unescaped query (db.ts:12).', 'CRITICAL'], ['Nothing else is CRITICAL except the token leak.', 'CRITICAL'],
|
||||
['Fix the token leak in log.ts:3 first; nothing else is CRITICAL.', 'CRITICAL'], ['The token leak in log.ts:3 blocks the merge. Nothing else is CRITICAL.', 'CRITICAL'],
|
||||
['1. Token leak in log.ts:3 must be fixed before landing.\n\nNothing else is CRITICAL.', 'CRITICAL'], ['The token leak blocks the merge.\nNo other issue is CRITICAL.', 'CRITICAL'],
|
||||
['The token leak in log.ts:3 blocks the merge. None of the other findings are CRITICAL.', 'CRITICAL'],
|
||||
['The token leak in log.ts:3 must block the merge. The other two are nits. Neither finding is CRITICAL.', 'CRITICAL'],
|
||||
['Apart from the token leak in log.ts:3, I found two nits. Neither finding is CRITICAL.', 'CRITICAL'],
|
||||
['Besides the auth bypass at auth.ts:9 there are three nits. None of the three findings are CRITICAL.', 'CRITICAL'],
|
||||
['The token leak in log.ts:3 must block the merge. Nothing here is CRITICAL.', 'CRITICAL'], ['No auth check guards the admin route, so nothing CRITICAL.', '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']);
|
||||
expect(parseFindings('adversarial-native', 'No rate limit is FIXABLE (login.ts:9)').map(f => f.label)).toEqual(['CRITICAL']);
|
||||
});
|
||||
|
||||
// Value: protects=a reader's own closing summary is a known phantom that keeps its label (HARN-15's Red Team and fix-check replies,
|
||||
// GEVA-77's "Neither finding is CRITICAL.", "found nothing CRITICAL"): it collects, and judge refuses to drop or lower it; the
|
||||
// follow-up tags such lines summary_candidate so judge may dismiss a tagged one on record; fails_when=a summary line collects nothing
|
||||
// or a weaker label, which would mean a drop path is back; why_new=GST-435 removed the drop after it lost blockers in three review
|
||||
// rounds; seam=parseFindings, collect and judge; no_claim=that a phantom is free (it stays CRITICAL in the judged set)
|
||||
test('known phantoms: closing summaries keep their label until the summary_candidate follow-up', () => {
|
||||
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, ['CRITICAL', ...Array(json).fill('INFORMATIONAL')]]);
|
||||
expect(found[0]!.raw).toMatch(/(no|nothing( at)?) CRITICAL/);
|
||||
}
|
||||
const fc = fixture('geva-77-fix-check.txt');
|
||||
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'], ['fix-check#3', 'CRITICAL']]]);
|
||||
expect(r.lines[2]!.raw).toStartWith('Neither finding is CRITICAL.');
|
||||
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<MergedLine>(merged), readJsonl(judged)).ok).toBe(false);
|
||||
for (const line of ['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.', 'Nothing here is CRITICAL.', 'Neither fix is CRITICAL.', 'Nothing in this diff is CRITICAL.', 'This is not CRITICAL.'])
|
||||
expect([line, parseFindings('outside-voice', line).map(f => f.label)]).toEqual([line, ['CRITICAL']]);
|
||||
expect(parseFindings('outside-voice', 'Severity: Low (not High)').map(f => f.label)).toEqual(['High']);
|
||||
});
|
||||
|
||||
// Value: protects=item layouts collect as on main: an indented JSON finding keeps its JSON severity, an indented label heading sets
|
||||
// the section for the next item, and a later Blocks: yes raises the finding a Severity field line or JSON line opened;
|
||||
// fails_when=any of these collects a lower label or a different finding than main; why_new=GST-435's paragraph-split draft broke each
|
||||
// of these and was deferred; seam=parseFindings; no_claim=that main's reading is the right one (known, deferred to the follow-up:
|
||||
// HARN-15 ship2 native item 4 collects without its title, and an indented label heading rates the next item, not the one above it)
|
||||
test('item layouts collect as on main; the later-paragraph split and indented label headings are known deferred shapes', () => {
|
||||
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)).toEqual(['CRITICAL', 'CRITICAL', 'CRITICAL', 'CRITICAL', 'CRITICAL', 'Low']);
|
||||
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)).toEqual(['High']);
|
||||
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']);
|
||||
expect(parseFindings('red-team', '1. Unescaped query\n {"severity":"critical","summary":"Low-level race in q.ts"}').map(f => [f.label, f.raw]))
|
||||
.toEqual([['critical', '{"severity":"critical","summary":"Low-level race in q.ts"}']]);
|
||||
expect(parseFindings('red-team', '1. Unescaped query\n **Critical**\n2. Token leak in the logger').map(f => [f.label, f.raw])).toEqual([['Critical', '2. Token leak in the logger']]);
|
||||
expect(parseFindings('red-team', '1. Unescaped query\n ## High\n2. Token leak in the logger').map(f => [f.label, f.raw])).toEqual([['High', '2. Token leak in the logger']]);
|
||||
expect(parseFindings('red-team', '1. Race in queue\n- Severity: Low\n this one blocks. Blocks: yes').map(f => f.label)).toEqual(['CRITICAL']);
|
||||
expect(parseFindings('red-team', '1. Unescaped query\n {"severity":"low","summary":"race"}\n this blocks the merge. Blocks: yes').map(f => [f.label, f.raw]))
|
||||
.toEqual([['CRITICAL', '{"severity":"low","summary":"race"} | this blocks the merge. Blocks: yes']]);
|
||||
});
|
||||
|
||||
// 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
|
||||
|
||||
Reference in new issue
Block a user