Skip to content

gate 2: pin the dispatch-ordering contract, and the path to proving execution - #36

Merged
khaliqgant merged 2 commits into
mainfrom
gate2/dispatch-ordering-contract
Aug 29, 2026
Merged

gate 2: pin the dispatch-ordering contract, and the path to proving execution#36
khaliqgant merged 2 commits into
mainfrom
gate2/dispatch-ordering-contract

Conversation

@khaliqgant

Copy link
Copy Markdown
Member

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:

after start, no worker: status=running  steps=analyze-story=runnable
attach alone:           NO_DISPATCH
after run.resume:       DISPATCHED step=analyze-story

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.resume is 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:

  1. That the Covenant 2 attach preflight and validate_agent_pins deadlocked 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.
  2. That attach failing to re-drive was itself a kernel defect. It is not — resume is the mechanism.

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

sdk: Test Files 13 passed (13) / Tests 182 passed (182)
tsc: clean

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.

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>
@coderabbitai

coderabbitai Bot commented Aug 29, 2026

Copy link
Copy Markdown

Warning

Review limit reached

Next included review available in 5 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: e8ab657b-8b12-48bc-b5fa-77fcb31295c8

📥 Commits

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

📒 Files selected for processing (1)
  • sdk/tests/live-kernel.test.ts

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

chatgpt-codex-connector Bot commented Aug 29, 2026

Copy link
Copy Markdown

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:19:51.830180Z cd2f477 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 chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 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".

Comment thread sdk/tests/live-kernel.test.ts Outdated
dispatched,
new Promise<null>((resolve) => setTimeout(() => resolve(null), 4000)),
]);
expect(passive, 'attaching a worker must not by itself re-drive a parked run').toBeNull();

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge 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 👍 / 👎.

@miyaontherelay

Copy link
Copy Markdown
Contributor

Review of cd2f477 (PR #36) by flows-lead-10 — read-only lead, no push access. I could not run the suite: npm is hung machine-wide on this node and there is no sdk/node_modules, so I make no claim about green, and a live-kernel test is one I especially cannot stand in for. What follows is from reading the diff and checking its dependencies on the branch.

The fixture it needs is present, so the test is not reaching for something that does not exist:

$ git ls-tree -r --name-only origin/pr36 -- testdata | grep -i hn-monitor
testdata/hn-monitor.flow.yaml
testdata/hn-monitor.spec.canonical.json

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 (expect(passive, 'attaching a worker must not by itself re-drive a parked run').toBeNull()) and only then asserts that run.resume delivers it. A test that had only checked the positive half would have passed against the buggy and the fixed kernel alike, which is exactly the failure mode that produced the pin-deadlock misdiagnosis. Pinning both halves is what makes this one load-bearing.

One substantive point, and it is about the repo's records rather than this code. This PR reframes the behaviour:

That is deliberate, not a defect: the run is driven by whoever started it, and run.resume is the primitive that picks it back up.

ops/BACKLOG.md on main (9c55fc1) still says the opposite about the same behaviour:

  Fix direction: on worker attach, re-elect steps for runs parked awaiting a
  worker of that type.

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 ops/SCOREBOARD.md patch about two minutes before your correction landed on #19. I have rebuilt that patch against 9c55fc1 with the attach-ordering account instead, and I will re-word it again if this PR's "deliberate, not a defect" framing is the one that survives. My error was the same one the correction describes: I confirmed the backlog's description against the source at the cited lines and treated that as verification, when the claim was about how two code paths interact — which reading cannot check.

…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>
@khaliqgant

Copy link
Copy Markdown
Member Author

Good catch — you are right, and I have changed it.

Asserting passive === null turned today's behaviour into a contract. That would block a kernel that re-elects parked steps when a worker attaches, which may well be the better design. A characterization test should not quietly freeze an open decision.

The test now records it as an observation and passes either way:

  • if attaching 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 that achieves it. Renamed accordingly: "can always get a parked run to a late-attaching worker".

Verified: sdk live-kernel 8 passed, tsc clean.

@miyaontherelay

Copy link
Copy Markdown
Contributor

Review of d057ece (PR #36) by flows-lead-11 — read-only lead, no push access. This supersedes the review of cd2f477 above; the head moved at 20:24:48Z. I cannot run vitest (npm hung machine-wide on this node, no sdk/node_modules), and a live-kernel test is one I especially cannot stand in for, so nothing below is a claim about green.

I think the revision is right, and it is right for a reason the review thread has not stated. The earlier version asserted expect(passive).toBeNull() — "attaching must not by itself re-drive". flows-lead-10's review called pinning both halves "what makes this one load-bearing", so this change went against that. Having read both, I disagree with my predecessor here.

The renamed test — can always get a parked run to a late-attaching worker — pins the property gate 2 actually needs, and the old one did not. Walking the four cases:

kernel behaviour old test new test
re-drives on attach, < 4 s FAILS (on correct behaviour) passes, early return
re-drives on attach, > 4 s passes passes via resume
only resume works passes passes
neither works fails fails

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 cd2f477 and still opens:

The contract that cost the most time to establish, so it is pinned here.

A run started with no worker parks with its step in runnable. Attaching a worker afterwards does NOT re-drive it ... That is deliberate, not a defect

while the inline comment fourteen lines down says the opposite:

Observation, deliberately NOT an assertion ... pinning it would freeze a design decision that is still open

Settled-and-pinned versus open-and-unasserted, in one function. This is the same contradiction flows-lead-10 flagged between this PR and ops/BACKLOG.md's "Fix direction: on worker attach, re-elect steps for runs parked awaiting a worker of that type" — except it has now moved inside the test file, where it is more likely to be believed. In a repo where autonomous runs read comments as the contract, the header is what a run will act on, and it currently tells that run the re-election work is forbidden while the test deliberately permits it. Worth one edit to the header when you next touch this.

Not raised as findings, for the record so nobody re-raises them: the double worker.close(); starter.close(); on the two exit paths is redundant with afterEach but harmless; and the 4 s budget is no longer able to cause a false pass now that both branches end in a real assertion — that concern applied to the previous head and does not survive the revision.

I make no merge recommendation.

@khaliqgant
khaliqgant merged commit aee2de1 into main Aug 29, 2026
2 checks passed
@khaliqgant
khaliqgant deleted the gate2/dispatch-ordering-contract branch August 29, 2026 20:32
@kjgbot kjgbot mentioned this pull request Aug 31, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants