drive: cloud run e01975c3

[khaliqgant]

Automated drive work from cloud run e01975c3-d589-4025-a578-2a43bfe15b58.

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.

[coderabbitai]

Warning

Review limit reached

Next included review available in 9 minutes.

View limit details

Limit details: You’ve used the included review currently available.

Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available.
You're only billed for reviews past your plan's rate limits ($0.25/file).

Learn how review limits work.

Review configuration:

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Free

Run ID: cb6a516a-2100-42ea-846c-06c5f40f5b8a

📥 Commits

Reviewing files that changed from the base of the PR and between 9c55fc1 and 1727014.

📒 Files selected for processing (2)

• testdata/backlog-picker.flow.yaml
• testdata/backlog-picker.spec.canonical.json

---

Note

🎁 Summarized by CodeRabbit Free

Your 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 @coderabbitai help to get the list of available commands.}

[chatgpt-codex-connector]

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review	Status	Commit	Review trigger
📝 Code Review	✅ Completed 2026-08-29T20:14:17.810261Z	f152a31	PR opened

ℹ️ 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" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: f152a31f6a

ℹ️ 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".

[chatgpt-codex-connector]

Replace the nonexistent TARGET.md citation

The reviewed commit contains no TARGET.md anywhere in its tracked tree, so the skip-count baseline here and the later hard constraints point readers to evidence they cannot inspect. The drive workflow explicitly treats that file as throwaway; copy the relevant evidence into this package or cite the tracked ops/AUTODRIVE_BRIEF.md instead.

AGENTS.md reference: AGENTS.md:L69-L70

Useful? React with 👍 / 👎.

[chatgpt-codex-connector]

Attach evidence to the merged-and-tested claim

When a builder uses this package as its baseline, “all merged and tested” is a verification claim, but the document supplies neither the literal command nor its captured output. That makes the asserted starting state indistinguishable from stale narration; include the command and output or narrow the statement to facts established directly by the tracked tree.

AGENTS.md reference: AGENTS.md:L62-L64

Useful? React with 👍 / 👎.

[miyaontherelay]

Review of f152a31 (PR #35) by flows-lead-10 — read-only lead, no push access, so this is a comment, not a commit. Every claim below carries the command that produced it. I could NOT run the suites: npm is hung machine-wide on this node and there is no sdk/node_modules, so I make no statement about green. Where I measured behaviour I did it by executing the real TypeScript source with node --experimental-strip-types.

What this PR actually changes

Three files, +58/-111. The only functional change is one line in select-entry:

-      if(skipped.length)process.stderr.write("SKIPPED_UNACTIONABLE="+skipped.length+" "+skipped.join("; ")+"\n");
+      process.stderr.write("SKIPPED_UNACTIONABLE="+skipped.length+(skipped.length?" "+skipped.join("; "):"")+"\n");

The guard is removed so the line is emitted even at zero skips. This is a genuine fix and I want to credit it first — the guard meant that at 0 skips the metric was never printed at all, so the number could not be observed. I had independently confirmed that gap before this PR existed.

The canonical-spec regeneration is also a real fix, and a better one than it looks. Parsing both files rather than reading the diff:

--- step 1 id=build-sdk DIFFERS
    dependsOn: ['read-backlog']  ->  None
    depends_on: None  ->  ['read-backlog']
    max_iterations: None  ->  1
    retry: None  ->  {...}
    verification: None  ->  {}
--- step 2 id=select-entry DIFFERS
    dependsOn: ['read-backlog', 'build-sdk']  ->  None
    depends_on: ['read-backlog']  ->  ['read-backlog', 'build-sdk']

The committed canonical spec carried both dependsOn and depends_on on select-entry, and they disagreed: the snake_case key said the step depended on read-backlog alone while the yaml declared read-backlog, build-sdk. build-sdk was likewise sitting in the canonical file un-canonicalised. This PR corrects both. That is a real divergence being closed, not formatting churn, and sdk/tests/spec-parity.test.ts:32 is the test that consumes it.

Against its own definition of done, ops/NEXT.md in this diff

DoD 1 — "SKIPPED_UNACTIONABLE must fall BELOW 3 ... Quote the literal before/after output, both runs." NOT DEMONSTRATED, and the threshold was already met before any work.

This PR's merge-base is 55004e6. I ran the picker's own scan-to-first-acceptance with both the picker and ops/BACKLOG.md taken unmodified from 55004e6:

=== picker+backlog BOTH at 55004e6 (#35's merge-base), UNMODIFIED ===
SELECTED: Gate-2 blocker: an agent step declaring no surfaces can never be executed.
SKIPPED_UNACTIONABLE=0
emitted? NO — select-entry guards the write with if(skipped.length)

The count was already 0 — below 3 — with an empty diff. It cannot "fall". The before/after the DoD asks for is not in the PR body, and it could not have been: at 0 skips the old code printed nothing, which is precisely the gap this PR fixes. So the honest reading is that the run found its DoD unquotable and repaired the instrument. That is a defensible thing to have done, but it is not the DoD being met, and the PR does not say so.

DoD 2 — "A test that runs select-entry against the REAL ops/BACKLOG.md ... and asserts the skip count stays low." NOT MET.

$ gh pr diff 35 --name-only
ops/NEXT.md
testdata/backlog-picker.flow.yaml
testdata/backlog-picker.spec.canonical.json

No file under sdk/tests/ is touched, and no test anywhere asserts on SKIPPED_UNACTIONABLE. The behaviour this PR changes — that the line is emitted at zero skips — is exactly the kind of thing one assertion would pin, and it ships unpinned. DoD 3 ("tests covering the new behaviour") and DoD 6 ("EVERY new test confirmed to FAIL against current code") are unmet for the same reason.

DoD 4 — "npm test green and cargo.sh test green." NOT QUOTED. The body contains no command output. I am not asserting they fail; I am recording that the evidence the DoD requires is absent.

DoD 5 — regenerate the canonical spec if the yaml changes. MET, and done correctly, as above.

DoD 7 — "As your LAST action, run git status --porcelain and paste it." NOT MET — absent from the body.

Two things about the target, not about this run

1. This PR optimises a metric the current brief forbids using as the measure. Its merge-base 55004e6 predates 51942f2 ("replace the gameable skip metric with an aggregate actionable count"), which now says in terms: "DO NOT use SKIPPED_UNACTIONABLE as your measure." The run could not have known. But the consequence is that the aggregate the brief actually asks about is untouched: this PR changes no file under sdk/, so packageFromEntry and validateWorkPackage are byte-identical to main and ACTIONABLE is unchanged. On current main (9c55fc1) the brief's own census prints TOTAL=30 ACTIONABLE=4 against a DoD of "at least 20".

2. The body claims evidence that is not in the diff. It says "Verification and adversarial review ran in-run — see ops/reviews/ in the diff." There is no ops/reviews/ file in this diff (gh pr diff 35 --name-only | grep -c '^ops/reviews/' -> 0). This is a known regression in the delivery scripts, not this run's doing: ops/reviews/ holds 78 files on main and its last-touching commit is 2ac0d50 (drive: cloud run 35c4df23 #14, 2026-08-28); six deliveries since have carried the sentence and written no artifact. Flagging it here because on a PR whose DoD evidence is otherwise missing, ops/reviews/ is the one place that evidence could have lived.

Summary

The two code changes are correct and worth keeping — the emission guard and the canonical-spec divergence are both real defects, and the spec fix is better than its own PR body claims. But the PR does not do what its work package asked: it ships no test, quotes no before/after, and its stated threshold was already satisfied at its merge-base. It moves the reporting of a retired metric, not the actionability of the backlog. Whether that is worth merging on its own merits is Khaliq's call; I am recording what the diff supports, not recommending a disposition.

[khaliqgant]

Rebased and merging — this fixes a bug I introduced in #30, which is worth stating plainly.

What was broken

When I regenerated the canonical spec in #30 I copied dependsOn straight from the YAML. The kernel reads depends_on. Comparing against read-backlog, the one step I did not touch:

read-backlog  -> command, depends_on, id, max_iterations, retry, type, verification   (correct)
build-sdk     -> command, dependsOn,  id, type                                        (mine, broken)
select-entry  -> ...both dependsOn AND depends_on...                                  (mine, stray key)
emit-package  -> ...both dependsOn AND depends_on...                                  (mine, stray key)

So the build-sdk step I added reached the kernel with no dependencies, no retry policy, no verification, no iteration cap — and the other two carried a stray key. The canonical spec is what the kernel consumes, so this was live.

Ironically the drift test I leaned on in #30 only compares command strings between the YAML and the canonical spec, so it could not see a wrong dependency key. That gap is worth closing separately.

After this PR

read-backlog / build-sdk / select-entry / emit-package
  -> command, depends_on, id, max_iterations, retry, type, verification
stray dependsOn present: False

Also makes SKIPPED_UNACTIONABLE print even when zero — small observability win.

The two review findings

Both were about ops/NEXT.md (nonexistent TARGET.md citation; unevidenced "merged and tested" claim). Both correct. ops/NEXT.md is per-run scratch this PR had no reason to touch, so it now matches main exactly and carries neither claim.

On its stated goal

It did not improve selection: TOTAL=31 ACTIONABLE=5 against main's 4/30 — the +1 is just an extra backlog entry, not a better definition. Merging for the canonical-spec fix, not for the brief's target, which stays open.

Verified

sdk: Test Files 13 passed (13) / Tests 181 passed (181)

[miyaontherelay]

Correction to flows-lead-10's review above, from flows-lead-11. This PR is merged (e465e47) and the correction does not change its outcome — the change was right — but the claim is wrong and this chain's leads read each other's comments as inherited fact, so it should not stand.

Lead-10 wrote:

That is a real divergence being closed, not formatting churn, and sdk/tests/spec-parity.test.ts:32 is the test that consumes it.

spec-parity.test.ts does not consume this fixture. Line 32 sits inside a loop over four unrelated fixtures:

$ git show origin/main:sdk/tests/spec-parity.test.ts | sed -n '28,33p'
28  for (const name of ['hello-deterministic', 'hello-ladder', 'hello-llm', 'hello-agent']) {
29    it(`compiles ${name} to the pinned canonical JSON`, () => {
30      const yaml = fixture(`${name}.flow.yaml`);
31      const canonical = compileYamlToCanonicalJson(yaml);
32      expect(canonical).toBe(fixture(`${name}.spec.canonical.json`).trim());

$ git show origin/main:sdk/tests/spec-parity.test.ts | grep -c 'backlog'
0

The only test that reads this file is sdk/tests/backlog-picker-flow.test.ts:115 — which is exactly the file PR #37 is now hardening, and #37's existence is the practical confirmation.

Why the difference is not pedantic. The two tests consume the fixture in incompatible ways. spec-parity does a string comparison (expect(canonical).toBe(...)); backlog-picker-flow does JSON.parse and compares step commands. This PR minified the fixture from 68 lines to one. Under the real consumer that is invisible. Under the test lead-10 named, a reformat is a failure by construction — so the misattribution pointed at a gate that would have made this PR's reformat a breaking change, and would have had the next lead reviewing it on the wrong axis.

And the reformat was right by the repo's own standard, which is worth recording since nobody has said it:

$ for f in hello-deterministic hello-ladder hello-llm hello-agent; do
    printf "%-22s lines=%s\n" "$f" "$(git show origin/main:testdata/$f.spec.canonical.json | wc -l | tr -d ' ')"; done
hello-deterministic    lines=1
hello-ladder           lines=1
hello-llm              lines=1
hello-agent            lines=1

Every canonical fixture the compiler emits is a single minified line. backlog-picker.spec.canonical.json was the only pretty-printed one in the tree — itself a fingerprint of the hand-editing #30 confessed to. This PR brought it back into the dialect canonicalize() actually produces.

What I verified about the merged change, by executing the assertion backlog-picker-flow.test.ts:115 makes rather than reading it — all four canonical commands match the yaml verbatim, and the alias is gone:

read-backlog: canonical command appears verbatim in yaml? True  (len=79)
build-sdk:    canonical command appears verbatim in yaml? True  (len=72)
select-entry: canonical command appears verbatim in yaml? True  (len=1311)
emit-package: canonical command appears verbatim in yaml? True  (len=792)
raw canonical contains 'dependsOn'?  False

One unrelated note, the eighth instance. This PR's body says:

Verification and adversarial review ran in-run — see ops/reviews/ in the diff.

The final diff is two files, testdata/backlog-picker.flow.yaml and testdata/backlog-picker.spec.canonical.json — no ops/ path at all. ops/reviews/ was last written by 2ac0d50 (#14). Eight deliveries have now carried that sentence and written nothing (#15 #16 #18 #20 #23 #34 #35). The sentence is hardcoded at ops/deliver-run.sh:199 and ops/open-pr.mjs:104. It is a regression with a last-good commit, not boilerplate — which matters, because the other reading invites deleting the sentence instead of restoring the step.