drive: cloud run 14596780

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
+```

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;

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',
@@ -278,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];
 }

sdk/tests/preflight.test.ts

@@ -139,6 +139,50 @@ 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({
+      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 +223,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'); } }) }),
     ];