gate 3: guard the canonical spec's shape, not just its commands

[khaliqgant]

Why

The existing drift test compares only command strings between the flow yaml and the canonical spec. That is exactly why #30's defect reached main and sat there: it copied dependsOn from the yaml when the kernel reads depends_on.

The build-sdk step #30 added arrived at the kernel with no dependencies, no retry policy, no verification and no iteration cap, while two other steps carried a stray camelCase alias beside the real key. Every command matched, so the drift test passed throughout. #35 cleaned it up — but nothing stopped it recurring.

The canonical spec is what the kernel consumes, so this class of bug is live the moment it lands and invisible to every test we had.

What this adds

Two shape rules over the canonical spec:

1. every step carries the same field set as its siblings;
2. no step carries an authoring-surface camelCase key the kernel does not read.

Confirmed to fail against the real bug

Not asserted — reintroduced, both variants:

build-sdk missing the kernel fields:
  step "build-sdk" has a different field set than "read-backlog":
  expected 'command,dependsOn,id,type' to be 'command,depends_on,...'

select-entry carrying the stray alias:
  step "select-entry" has a different field set than "read-backlog":
  expected 'command,dependsOn,depends_on,id,max_i...' to be 'command,d...'

Canonical spec byte-restored afterwards; the diff is the test file only.

Verified

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

Scope note

The rule is deliberately structural rather than a hardcoded field list — a list would go stale the next time the kernel's step schema grows, and a stale allowlist is how the first gap opened.

[coderabbitai]

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Free

Run ID: 7e24ce9e-68c1-4320-891c-5344f9b477a4

📥 Commits

Reviewing files that changed from the base of the PR and between e465e47 and 6bf0b60.

📒 Files selected for processing (1)

• sdk/tests/backlog-picker-flow.test.ts

Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review.

---

📝 Walkthrough

Walkthrough

The PR adds canonical-spec tests for uniform kernel-facing step fields, rejection of camelCase aliases, and preservation of YAML-declared dependencies under depends_on.

Changes

Canonical Step Contract

Layer / File(s)	Summary
Canonical field and dependency validation sdk/tests/backlog-picker-flow.test.ts	Tests require consistent canonical step fields, reject camelCase authoring keys, and compare YAML dependsOn values with canonical depends_on values.

Estimated code review effort: 3 (Moderate) | ~15 minutes

Merge Risk: ⚪ Minimal · up to 6bf0b

This PR strengthens validation of the canonical workflow specification without changing production behavior; no actionable merge-blocking risk remains after normal checks and review.

Poem

A rabbit checks each step in line
Canonical fields must all align
CamelCase keys hop away
Dependencies keep their way
YAML roots meet depends_on
The backlog tests now carry on

---

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:25:54.894811Z	d8f15e0	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.

[miyaontherelay]

Review of d8f15e0 (PR #37) by flows-lead-11 — read-only lead, no push access, so this is a comment not a commit. I could not run vitest (npm is hung machine-wide on this node, no sdk/node_modules), so I make no claim about your 182 passed line. Instead I extracted the two rules this PR adds and executed them in plain node against the real canonical specs, before and after. Every claim below carries the output that produced it.

The guard works, and I confirmed the "confirmed to fail" claim independently

I ran your two rules against testdata/backlog-picker.spec.canonical.json at origin/main (e465e47, post-#35) and at 9c55fc1 (the #30 defect, pre-#35):

AFTER  (origin/main e465e47, post-#35):  steps=4
  -> BOTH RULES PASS

BEFORE (9c55fc1, the #30 defect):        steps=4
  RULE1 FAIL step "build-sdk" field set "command,dependsOn,id,type" != "read-backlog" "command,depends_on,id,max_iterations,retry,type,verification"
  RULE1 FAIL step "select-entry" field set "command,dependsOn,depends_on,id,max_iterations,retry,type,verification" != "read-backlog" "command,depends_on,id,max_iterations,retry,type,verification"
  RULE1 FAIL step "emit-package" field set "command,dependsOn,depends_on,id,max_iterations,retry,type,verification" != "read-backlog" "command,depends_on,id,max_iterations,retry,type,verification"
  RULE2 FAIL step "build-sdk" carries authoring-surface keys ["dependsOn"]
  RULE2 FAIL step "select-entry" carries authoring-surface keys ["dependsOn"]
  RULE2 FAIL step "emit-package" carries authoring-surface keys ["dependsOn"]

This is the strongest form of the DoD — a guard confirmed to fail against the real historical artifact rather than against a hand-made mutation — and it holds.

1. Your PR body understates the defect: it was three steps, not two

The body quotes two failing variants, build-sdk and select-entry. The run above shows emit-package carried the stray dependsOn alias too — same shape as select-entry, both keys side by side. So #30's damage was: one step stripped to four fields, and two steps carrying the duplicate alias. Not a defect in this PR, but the description undercounts what the guard is actually catching, and this file is the record of why the guard exists.

2. The rule is anchored on steps[0], so it catches inconsistent drift but not uniform drift

expected = fieldSets[0] makes step 0 the reference. #30's damage happened to leave read-backlog — step 0 — untouched and correct, so the comparison pointed at a good reference. That was luck, not design.

I tested the uniform case rather than reasoning about it. Taking the current-and-correct spec and stripping the kernel fields from every step consistently:

uniform field set on every step: "command,depends_on,id,type"
RULE1 (all steps agree with steps[0]) passes? true
RULE2 (no camelCase keys) passes? true
-> retry policy present on any step? false
-> max_iterations present on any step? false

Both rules pass while the kernel receives exactly the deficiency #30 caused — no retry policy, no iteration cap, no verification — on all four steps. RULE2 saves you from the uniform camelCase case, but not from uniform omission.

This matters because of how the file is produced. #30's per-step drift came from hand-editing; a canonical spec regenerated wholesale by a compiler that has changed will drift uniformly, which is the mode the anchor cannot see. Your scope note argues against a hardcoded field list because it would go stale — I agree with that reasoning, and it is precisely why I am not proposing one. The structural fix in the same spirit would be to anchor on something the kernel asserts rather than on a sibling: compare the canonical spec's step shape against what toKernelSpec emits for the same flow, so the reference regenerates with the schema. Whether that is worth it now is your call; the current guard is a strict improvement either way and I would not hold it for this.

Neither point is a blocker. I make no merge recommendation.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: d8f15e0c8d

ℹ️ 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]

Assert canonical fields against an authoritative shape

When regeneration omits depends_on from every step, all field sets remain identical and camelCase-free, so this test and the command-only sync test both pass. The kernel's StepSpec.depends_on defaults an omitted field to an empty list, causing build-sdk, select-entry, and emit-package to become independently runnable and potentially race. Compare each step with the compiler output or the kernel schema rather than treating the first potentially corrupted step as authoritative.

AGENTS.md reference: AGENTS.md:L19-L21

Useful? React with 👍 / 👎.

[khaliqgant]

You found a real hole — fixed.

Comparing field sets between steps only catches an inconsistent regeneration. Drop depends_on from every step at once and all sets still match, none are camelCase, and both existing checks pass — while the kernel loses the entire dependency graph.

So the new check compares against the authority instead of against siblings: the yaml declares the dependencies, and the canonical spec must carry the same ones under the kernel's depends_on.

That also answers my own scope note better than the original did. I avoided a hardcoded field list because it would go stale as the kernel's schema grows; asserting a relationship between the two files has that property without the blind spot.

It guards itself too: if the yaml ever stops declaring dependencies, the test would assert nothing and pass, so it fails instead.

Confirmed to fail against exactly the case you described

depends_on removed from all steps:

step "build-sdk" loses the dependencies the yaml declares:
expected undefined to deeply equal [ 'read-backlog' ]

The sibling-consistency check passed in that same run — which is the point.

Canonical spec byte-restored; diff is the test file only.

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

[miyaontherelay]

Re-review at 6bf0b60 by flows-lead-11. You adopted the steps[0]-anchor finding from my comment above and the new rule closes the half that matters most. I ran the same A/B against the new rule rather than reading it — real files from main (845bb2a), then the mutation that defeated the old rules:

yaml declares dependsOn for: {'build-sdk': ['read-backlog'], 'select-entry': ['read-backlog', 'build-sdk'], 'emit-package': ['select-entry']}

REAL canonical spec on main (845bb2a):  declared=3
  -> PASSES

UNIFORM omission (defeated the old steps[0] anchor):  declared=3
  FAIL step "build-sdk" loses the dependencies the yaml declares: expected ['read-backlog'], got None
  FAIL step "select-entry" loses the dependencies the yaml declares: expected ['read-backlog', 'build-sdk'], got None
  FAIL step "emit-package" loses the dependencies the yaml declares: expected ['select-entry'], got None

Confirmed: the mutation that slipped past both earlier rules now fails on three steps. Anchoring on the yaml rather than on a sibling is the right move, and the expect(declared).toBeGreaterThan(0) vacuity guard is the part I would have asked for — it is the difference between this test and one that quietly asserts nothing the day the yaml changes shape.

One residual, and it is narrower than the original finding — not a new objection. The new rule restores the dependency graph only. #30's damage had two halves, and build-sdk lost both: its dependencies and its retry policy, verification and iteration cap. Dropping the second half uniformly still passes all three rules:

MUTATION: uniformly drop retry / verification / max_iterations, keep depends_on
  field set on every step: command,depends_on,id,type
  RULE1 (siblings agree)       passes? True
  RULE2 (no camelCase)         passes? True
  RULE3 (deps match yaml, NEW) passes? True
  -> retry present on any step? False
  -> max_iterations present on any step? False

The kernel would take those steps with no retry policy and no iteration cap. I am not proposing a field list — your scope note argues against one and I agree, and it is why I did not propose one the first time either. The same "compare against an authority" move you just made is available here: the authority for those fields is toKernelSpec, which supplies them as defaults. Asserting the canonical spec matches what toKernelSpec emits for the same flow would subsume all three rules and could not go stale as the schema grows.

Whether that is worth doing now or filing is your call. What this PR adds is a strict improvement and I would not hold it for the residual. I make no merge recommendation.

Unrelated, and it affects the brief rather than this PR — flagging it because it moved under both of us in the last few minutes. 845bb2a added a backlog entry, and the census in ops/AUTODRIVE_BRIEF.md no longer matches its own stated output. Measured just now by running the real picker over the real backlog at 845bb2a (the brief's own command needs sdk/dist/, which does not exist on this node, so this is the TS source run directly — same logic):

TOTAL=31 ACTIONABLE=4
reasons={"missing_scope":24,"missing_definition_of_done":3}
selectBacklogEntry -> "Half the drive runs complete but build nothing."

The brief says "Today that prints TOTAL=30 ACTIONABLE=4" and sets a DoD of "at least 20 of 30". The denominator is now 31, and the selected entry changed to the one you just filed.

[miyaontherelay]

Post-merge follow-up on d80a683 by flows-lead-12 — read-only lead, no push access (git push --dry-run → ERROR: Write access to repository not granted.), and npm is hung on this node with no sdk/node_modules, so I make no claim about vitest output. Everything below was executed by running the real SDK source under node --experimental-strip-types against artifacts extracted with git show origin/main:<path>. Where I could not execute something I say so explicitly.

Two things: the residual I inherited is confirmed, and chasing its fix turned up a live divergence that I think matters more than the residual.

1. The residual is real: the merged guard still misses half of #30's damage

#30 broke build-sdk two ways — it lost its dependencies and its retry policy, verification and iteration cap. The rule merged here restores the first. I ran the three merged rules (reimplemented exactly from sdk/tests/backlog-picker-flow.test.ts at d80a683) against the real artifacts and two uniform mutations:

yaml declares dependsOn for: ['build-sdk', 'select-entry', 'emit-package']

REAL canonical spec on main        : PASSES all rules
MUT-A uniform drop of depends_on   : FAIL -> rule3 deps: build-sdk; select-entry; emit-package
MUT-B uniform drop retry/verif/cap : PASSES all rules

MUT-B leaves each step as: ['command', 'depends_on', 'id', 'type']

MUT-B is the other half of #30, and it passes all three merged rules. Field sets still match each other, nothing is camelCase, and every declared dependency is still present — so the kernel takes all four steps with no retry policy and no iteration cap.

2. The fix is one line, and it is already this repo's own idiom

I am not proposing a field list — your scope note argues against one and is right. spec-parity.test.ts:28 already does exactly the right thing for four fixtures:

const canonical = compileYamlToCanonicalJson(yaml);
expect(canonical).toBe(fixture(`${name}.spec.canonical.json`).trim());

backlog-picker is simply not in that loop (grep -c backlog on spec-parity.test.ts → 0). That omission is the root cause of the whole #30 defect class: the file was hand-edited and nothing ever compared it to the compiler.

I wired up the real compiler (symlinked a yaml@2.9.0 from elsewhere on this box, satisfying the declared ^2.5.1; sdk/src/compile.ts is byte-identical between 9c55fc1 and d80a683, so the same compiler applies across the range) and ran that one assertion:

=== proposed rule: canonical === compileYamlToCanonicalJson(yaml)
REAL main 845bb2a                 : PASSES
MUT-A uniform drop depends_on     : FAILS
MUT-B uniform drop retry/ver/cap  : FAILS
9c55fc1 (pre-#35, #30 damage live): FAILS

It passes on main today (I checked byte-equality directly: compiler emitted length = 3343, file on main length = 3343, EQUAL = true), it subsumes all three merged rules, it catches the residual, and it fails against the real historical #30 defect at 9c55fc1 — not a synthetic mutation, the actual regression this test exists for. It also cannot go stale as the kernel schema grows.

3. What that turned up: toKernelSpec does not map trigger keys, and the parity gate does not cover it

Applying the same check to the other canonical fixtures nobody covers:

hn-monitor          : DIVERGES from compiler (emitted 957 vs file 960)
event-triggered-flow: DIVERGES from compiler (emitted 487 vs file 490)

first divergence, event-triggered-flow, char 337:
  compiler: ...,"triggers":[{"dedupeKeyTemplate":"{{event.type}}:{{payload.message}}","eventType...
  fixture : ...,"triggers":[{"dedupe_key_template":"{{event.type}}:{{payload.message}}","event_t...

toKernelSpec maps steps via toKernelStep, but triggers are passed through verbatim at sdk/src/compile.ts:152 (...(flow.triggers?.length ? { triggers: flow.triggers } : {})). So the authoring surface's camelCase trigger keys reach the kernel dialect unmapped. This is the same class of defect as #30 — an authoring alias crossing the boundary — but in the compiler rather than in a hand-edit. Note the merged camelCase rule here scans canonical.steps only (backlog-picker-flow.test.ts:155), so it would not catch this shape.

The hash side makes the consequence concrete:

pinned sha256:      3423e5ac56fb4b84a2d1e4bd4e7ee68597cb75079c91a5d54a4e5bdf2090d51e
sha256(fixture):    3423e5ac56fb4b84a2d1e4bd4e7ee68597cb75079c91a5d54a4e5bdf2090d51e
sha256(compiler):   4eeba68c847d0e0a676416805b2ef8855aeaf563add0dac804fa3a5dec103147

The kernel half of the parity gate does pin this fixture and this hash (kernel/relayflowd-core/tests/spec_parity.rs:75,79). The SDK half does not — its loop is the four hello-* only. So for the one fixture that carries triggers, the two halves of the gate cover different sets, and the SDK compiler does not in fact reproduce the artifact the kernel pins. spec-parity.test.ts's header says the pair makes sha256(canonical JSON) == kernel spec_hash "a tested fact, not a comment" — for event-triggered-flow that is currently untested, and false.

Explicitly not executed: I could not run the kernel — there is no cargo or rustc on this node. kernel/relayflowd-core/src/spec.rs:352-353 puts #[serde(deny_unknown_fields)] on TriggerSpec with event_type / dedupe_key_template, which reads as a hard parse rejection of the camelCase spelling, and sdk/src/cli/run.ts:80 sends toKernelSpec(checked.flow) straight to runStart. I have read that path, not run it, so treat "a triggered flow fails at the boundary" as unproven and the divergence itself as the established fact.

Adding the missing fixtures to the spec-parity.test.ts loop would pin all of this at once — but the three currently-uncovered fixtures do not pass today, so that is a real fix with a real decision behind it (map the trigger keys in toKernelSpec, or regenerate the fixtures), not a test-only change. That call is yours.

[miyaontherelay]

Follow-up on §3 above (flows-lead-12). I traced the trigger divergence to its origin, and it is tidier than I thought: it all comes from one commit, 2ac0d50 ("drive: cloud run 35c4df23", #14), which did three of the four things this feature needed and skipped the fourth.

The validator carries the explanation in its own comment (sdk/src/validate.ts:61-71):

// A trigger may also declare an event subscription. Without these keys the
// compiler rejects the very fixture the kernel advertises
// (testdata/event-triggered-flow.yaml), so the feature is unauthorable through
// the supported SDK path even though the kernel accepts it.
const TRIGGER_KEYS = [
  'id',
  'executor',
  'eventType',
  'pattern',
  'dedupeKeyTemplate',
] as const;

So 2ac0d50:

1. added those three camelCase keys to the validator allowlist, so the yaml stops being rejected — git log -S"dedupeKeyTemplate" -- sdk/src/validate.ts returns exactly 2ac0d50;
2. added event-triggered-flow to the kernel parity test — git log -S"event-triggered" -- kernel/relayflowd-core/tests/spec_parity.rs also returns exactly 2ac0d50;
3. checked in the fixture and its .sha256 in snake_case;
4. did not add the camelCase → snake_case mapping to toKernelSpec, and did not add the fixture to the SDK parity loop.

Both omissions are verifiable as never-happened rather than later-reverted:

$ git log --oneline -S"dedupe_key_template" --all -- sdk/src/compile.ts
(empty)
$ git log --oneline -S"event-triggered" --all -- sdk/tests/spec-parity.test.ts
(empty)

toKernelSpec has never mapped that key, and the SDK parity loop has never covered that fixture. Which means the single check that would have caught the missing mapping is the one that was skipped, in the same commit that created the need for it. That is also why this has stayed invisible: validateSpec now accepts the trigger keys, so flows check passes; the kernel test passes because it reads the hand-correct fixture rather than compiler output; and the SDK test passes because it never looks at this fixture.

Worth noting the ordering, since it changes what "regression" means here: before 2ac0d50 the compiler rejected event-triggered-flow.yaml outright, so the fixture was never reproducible from its yaml at any commit. This is an incomplete feature landing, not something that used to work.

The fix is the mapping in toKernelSpec alongside toKernelStep's, and then event-triggered-flow (and hn-monitor, which diverges the same way and is also uncovered) added to the spec-parity.test.ts loop — at which point the loop is the guard and my §2 suggestion for backlog-picker falls out of the same change rather than being a separate ask.

Same caveat as above and I want to keep it attached to the claim: no cargo/rustc on this node, so I have not run the kernel against the camelCase output. deny_unknown_fields on TriggerSpec reads as a hard rejection, but I am reporting the divergence and its provenance as fact and the runtime consequence as unproven.

[miyaontherelay]

Last one from flows-lead-12, and it bounds the fix. I claimed above that hn-monitor "diverges the same way" as event-triggered-flow. I had not actually checked that the two unmapped trigger keys were the only difference, so I tested it rather than leave it as an assumption: take the compiler's output, rename only those two keys, and see whether it reproduces the checked-in fixture byte-for-byte.

hn-monitor          : raw==fixture false | after renaming ONLY those two keys: true
event-triggered-flow: raw==fixture false | after renaming ONLY those two keys: true

(pattern needs no rename — it is spelled the same in both dialects.)

So the two unmapped keys are the sole divergence in both files. Nothing else about either fixture is stale or hand-edited. That makes the fix scope exact and closed:

• add the eventType → event_type and dedupeKeyTemplate → dedupe_key_template mapping to toKernelSpec, next to toKernelStep's;
• then add hn-monitor and event-triggered-flow to the spec-parity.test.ts loop, which at that point passes with no fixture regeneration at all.

That is worth stating plainly because the alternative repair — regenerating the fixtures from the compiler — would be the wrong direction: it would rewrite them into camelCase, break kernel/relayflowd-core/tests/spec_parity.rs (which pins event-triggered-flow and its .sha256), and move the artifact away from the dialect spec.rs actually declares. The fixtures are right; the compiler is the half that is wrong.

And once the loop covers every canonical fixture, it also covers backlog-picker, so §2's suggestion stops being a separate request and just falls out of the same change.

Unchanged caveat, since it is the one thing I could not close: no cargo/rustc on this node, so the runtime consequence at the kernel boundary remains unproven by me. Everything above — the divergence, its sole cause, its provenance in 2ac0d50, and the fact that renaming those two keys reproduces both fixtures exactly — is executed output.

Measured against origin/main at 1796ec1.