gate 2: pin the dispatch-ordering contract, and the path to proving execution - #36
Conversation
A run started with no worker parks with its step in `runnable`. Attaching a worker afterwards does NOT re-drive it — attach_worker (server/session.rs) registers the worker and nothing revisits parked runs. `run.resume` is the primitive that picks it back up. Measured, not assumed: after start, no worker: status=running steps=analyze-story=runnable attach alone: NO_DISPATCH after run.resume: DISPATCHED step=analyze-story This is why gate 2 could not be shown executing: the HN demo submits events while nothing is attached, so every woken run parks and stays parked. The kernel executes it fine — nothing was resuming it. Either attach the worker before submitting, or resume afterwards. This is a characterization test, not a regression test: it passes on main and asserts BOTH directions, so it fails if attach ever starts re-driving parked runs or if resume stops dispatching. I am not claiming it was seen to fail — the behaviour it pins is current behaviour, deliberately. Two wrong diagnoses preceded this and are worth recording so they are not repeated: first that the Covenant 2 attach preflight and validate_agent_pins deadlocked for a surface-less agent step (they do not — such a step dispatches when the worker attaches first), and then that attach failing to re-drive was itself a kernel defect (it is not — resume is the mechanism). The first was caught only by reverting the fix and finding the test still passed. Verified: sdk 182 passed, tsc clean. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Warning Review limit reachedNext included review available in 5 minutes. View limit detailsLimit 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. Review configuration: ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Free Run ID: 📒 Files selected for processing (1)
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 |
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. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: cd2f47778d
ℹ️ 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".
| dispatched, | ||
| new Promise<null>((resolve) => setTimeout(() => resolve(null), 4000)), | ||
| ]); | ||
| expect(passive, 'attaching a worker must not by itself re-drive a parked run').toBeNull(); |
There was a problem hiding this comment.
Stop requiring workers to leave parked runs stranded
When a gate-2 event wakes a run before its agent worker attaches, this assertion requires the run to remain parked until some separate component discovers its ID and explicitly resumes it. That is the silent-liveness failure identified in the parent commit’s ops/BACKLOG.md, whose stated fix is to re-elect parked steps on worker attachment; it also conflicts with RFC-0001’s requirement that proactive flows be liveness-checked rather than silently zero. A correct attach-time redispatch fix would now fail this test, so remove or invert the negative assertion and instead pin that attaching a compatible worker eventually dispatches the runnable step.
AGENTS.md reference: AGENTS.md:L3-L5
Useful? React with 👍 / 👎.
|
Review of The fixture it needs is present, so the test is not reaching for something that does not exist: The shape of the test is right, and specifically it is right in the way the previous attempt was wrong. It does not merely assert the good path — it first asserts the absence of dispatch after attach ( One substantive point, and it is about the repo's records rather than this code. This PR reframes the behaviour:
Those cannot both stand. One says the kernel should re-elect on attach; the other says attach deliberately does not, and resume is the intended primitive. This matters beyond tidiness: the backlog entry is the first entry in the file, it is what a gate-2 run would be handed, and a run reading it would implement the re-election that this PR's test now forbids. Worth one clarifying edit to that entry when this lands. I am not proposing wording — which of the two is the intended contract is your call, not mine. One small risk, not a blocker. The negative assertion is a 4000 ms timeout, so it establishes "no dispatch within 4 s", not "never". That is inherent to proving a negative against a live daemon and the budget looks sane against the 45 s case timeout, but it is the assertion most likely to flake first on a loaded CI box, and if it flakes it flakes green-to-red on correct code. Worth knowing when it eventually goes yellow. For the record, and so it is not lost: I had written the retracted pin-deadlock diagnosis into a local |
…review) Review was right. Asserting `passive === null` made today's behaviour a contract, which would block a kernel that re-elects parked steps when a worker attaches — a change that may well be the right one. The test now records that as an observation and passes either way: if attach alone dispatches, the requirement is already met and the test returns; if it does not, run.resume must be able to pick the run up. What is pinned is the requirement — a parked run can always be got to a late-attaching worker — not the mechanism. Verified: sdk live-kernel 8 passed, tsc clean. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Good catch — you are right, and I have changed it. Asserting The test now records it as an observation and passes either way:
What is pinned is the requirement — a parked run can always be got to a late-attaching worker — not the mechanism that achieves it. Renamed accordingly: "can always get a parked run to a late-attaching worker". Verified: sdk live-kernel 8 passed, tsc clean. |
|
Review of I think the revision is right, and it is right for a reason the review thread has not stated. The earlier version asserted The renamed test —
The old version failed a kernel that satisfied the requirement better, and it did so only when it was fast — so the assertion it pinned was decided by a 4 s race, not by the contract. The new one fails exactly when the step never reaches the worker, which is the thing gate 2 is about. That is a strictly better predicate. The one substantive finding: the header comment now contradicts the test it introduces. It is unchanged from
while the inline comment fourteen lines down says the opposite:
Settled-and-pinned versus open-and-unasserted, in one function. This is the same contradiction flows-lead-10 flagged between this PR and Not raised as findings, for the record so nobody re-raises them: the double I make no merge recommendation. |
The finding
Gate 2 has been unprovable because of an ordering requirement nobody had written down — not because the kernel cannot execute a woken run.
Measured on the real kernel, same flow, same pins:
A run started with no worker parks with its step in
runnable. Attaching a worker afterwards does not re-drive it —attach_worker(server/session.rs:102) registers the worker and nothing revisits parked runs.run.resumeis the primitive that picks it back up, and it works.The HN demo submits events while nothing is attached, so every woken run parks and stays parked. That is the whole of it. Gate 2 needs no kernel change: attach the worker before submitting, or resume afterwards.
What this PR contains
One characterization test pinning both directions of that contract. It is honest about what it is: it passes on main and is not a regression test for a fix. It fails if attach ever starts re-driving parked runs, or if resume stops dispatching.
Two wrong diagnoses I recorded in the commit
Worth keeping so they are not repeated:
validate_agent_pinsdeadlocked for a surface-less agent step. They do not — such a step dispatches fine when the worker attaches first. I had drafted a kernel fix for this and reverted it.The first survived a written fix and a passing test. It was caught only by reverting the fix and finding the test still passed — it had never reproduced the bug. That is the gate-1 asterisk repeating, and the reason this PR states plainly that its test was not seen to fail.
Verified
What I did not do
Change the demo to attach a worker before polling. That would let it prove execution, but a demo that spawns its own worker to satisfy its own claim needs your call on whether that counts as proving gate 2 — I did not want to manufacture a green.