fix(kernel): carry agent transport in specs

[miyaontherelay]

Authored f.agent(..., { transport: "direct" | "relay" }) compiles the transport into the kernel spec, but the Rust parser currently rejects that field as unknown before dispatch. This carries the validated transport through the kernel journal and worker dispatch, while continuing to reject unknown transport values.

Validation:

• cargo test -p relayflowd-core (79 tests passed)
• cargo check --workspace
• git diff --check

This closes the SDK/kernel gap introduced with the authored Relay transport surface. It does not change the omitted-field behavior; the worker continues to default an omitted transport to direct.

---

Note

Low Risk
Spec schema and fail-closed validation only; no runtime transport or dispatch logic changes in this diff.

Overview
Agent steps in run specs can now include an optional transport field (direct or relay) without the kernel treating it as an unknown step field. The parser adds AgentTransport on agent step kinds and whitelists transport in agent step field validation so SDK-compiled specs that already set transport parse, validate, serialize, and journal like other carried agent metadata—the kernel still does not implement either transport.

A parity test asserts direct and relay round-trip through RunSpec::parse / validate and that invalid values (e.g. telepathy) fail closed at parse time. Omitted transport behavior is unchanged; workers keep defaulting when absent.

^{Reviewed by Cursor Bugbot for commit 0dcbe7f. Bugbot is set up for automated code reviews on this repo. Configure here.}

[coderabbitai]

Important

• 🔍 Trigger review

This repository does not receive automatic reviews because it has fewer than 10 stars.

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Advanced

Run ID: 3dc124c7-bf6d-4171-87f1-793d66b4765f

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[github-actions]

Review swarm: maintainability

Maintainability Review: PR #412

Title: fix(kernel): carry agent transport in specs
Branch: fix/kernel-agent-transport-0915
Commit: 0dcbe7f
Reviewer: maintainability lens (attempt 3)

Summary

This PR adds an AgentTransport enum to the kernel spec layer, carrying the choice between direct and relay transport through the spec → journal → dispatch path. The change is minimal (13 lines of enum + 35 lines of test) and confined to the spec layer.

Maintainability Assessment Through Six-Month Stranger Lens

What Changed and Why It Exists

The PR adds:

1. An AgentTransport enum with two variants: Direct and Relay
2. An optional transport field on agent steps in spec.rs
3. A test validating round-trip serialization and fail-closed behavior for unknown values
4. The field added to STEP_AGENT_FIELDS whitelist

Clear Boundaries — ✓ PASS

The kernel-worker boundary is explicit and correct. Lines 427-430 of the diff state:

/// How the attached worker invokes the declared CLI. The kernel does
/// not implement either transport; it journals and dispatches the
/// choice so the worker can honor it deterministically.

This matches the observed implementation:

• Kernel: parses, validates, stores in StepSpec, passes through StepDispatch.spec (verified in worker.rs:12)
• Worker: reads spec.transport and routes to direct subprocess or relay task (verified in worker.ts:114)

The comment makes the non-responsibility explicit. A stranger can read this and know the kernel is not responsible for transport semantics.

Implicit Contracts — FINDINGS

Finding 1: Default behavior is documented but not enforced structurally

The field is Option<AgentTransport> with #[serde(default, skip_serializing_if = "Option::is_none")]. The SDK spec.ts:253-257 documents the default as 'direct', but the kernel does not enforce this.

Impact on maintainability: A stranger reading only kernel code sees Option<AgentTransport> and must search externally to learn what None means. The worker code at worker.ts:114 shows the actual contract:

spec.transport === 'relay' ? 'relay' : 'direct'

This is a soft contract — the default exists only in worker implementation, not kernel type system. It works but forces the reader to trace execution to discover the rule.

Could this break silently? No. The test validates both declared values round-trip. If a worker fails to handle None, the failure is at worker startup (wrong CLI invocation), not data corruption. The coupling is weak enough.

Verdict: Documented adequately in SDK layer (spec.ts). The kernel's Option is correct because omission is legal. Not a blocker.

Finding 2: Transport semantics are documented outside the kernel

The enum variants Direct and Relay have no inline documentation in spec.rs. A stranger must look to:

• docs/AGENT-RELAY-TRANSPORT.md (exists, read during review)
• SDK spec.ts:253-257 (documents direct = subprocess, relay = workspace participant)
• Worker implementation worker-cli.ts:72-74 (branches on transport)

Impact on maintainability: The kernel enum is a pure data carrier. The comment "kernel does not implement either transport" establishes this. A maintainer changing the kernel layer knows to look at worker implementation for semantics.

Could this be clearer? Yes — a doc comment on each enum variant pointing to the authoritative semantics doc would eliminate a search step. Example:

/// `'direct'` (default): spawn CLI as local subprocess.
/// See `docs/AGENT-RELAY-TRANSPORT.md` for relay semantics.

Is the absence a blocker? No. The kernel layer's role is clear, and the external docs exist. A stranger can find the answer in one hop (search codebase for "transport" or "relay").

Verdict: Acceptable. Would be improved by brief variant docs, but current state is maintainable.

Missing Failure Handling — FINDINGS

Finding 3: Journal contract is implicit but observable

The previous review claimed "no evidence journal schema accommodates this field." Investigation shows:

1. Journal stores the full spec. StepDispatch at worker.rs:12 carries pub spec: StepSpec, and dispatch is the journaled fact handed to workers.
2. Resume preserves the spec. The journal replays results, not code (RFC-0001 decision Close Gate 1 deterministic crash-resume rung #2). The spec is journaled once in run.spawned and never replayed — dispatch reads it from the journaled spec, not from re-parsing YAML.
3. The test proves spec round-trips. spec_parity.rs:48-80 validates that a spec with transport: "direct" parses, validates, and serializes back to "direct". This is the kernel's contract: preserve declared values.

What happens if transport changes between attempts? The spec cannot change between attempts — it is journaled once at run creation. A stranger reading run.spawned entry structure would see the spec is immutable per run.

Could this break silently? No. The test validates unknown values fail closed (line 68-79). A typo or future transport variant not in the enum causes parse failure at spec submission, not mid-run.

Verdict: Journal contract is satisfied implicitly by the existing spec persistence design. Not a gap.

Finding 4: Worker enforcement is soft but fail-fast

The previous review noted "no mechanism ensuring the worker respects the transport." Investigation shows:

• Worker at worker-cli.ts:72: if (mode === 'agent' && transport === 'relay')
• If worker ignores the field, it routes incorrectly (spawns subprocess when relay was requested)
• Failure mode: agent does not appear in workspace, or task dispatch fails with clear error from agent-relay-transport.ts:46 ("requires RELAY_AGENT_TOKEN")

Could this fail silently? No. Relay transport requires specific environment setup (RELAY_AGENT_TOKEN). A worker ignoring transport: 'relay' would attempt direct spawn and either:

1. Succeed if direct spawn was acceptable (no harm — user got working agent)
2. Fail with "no declared CLI" or similar startup error (caught immediately)

The worker cannot "silently use wrong transport and corrupt data" because transport affects process creation, not data persistence.

Verdict: Soft contract, but failure is loud and early. Acceptable for a pass-through field.

Tests Assert Actual Behavior — FINDINGS

Finding 5: Test coverage matches the kernel's responsibility

The test the_kernel_round_trips_declared_agent_transports_and_rejects_unknown_values validates:

1. Parse "direct" → serialize "direct" ✓
2. Parse "relay" → serialize "relay" ✓
3. Parse "telepathy" → error ✓
4. Validation passes for known values ✓

What the test does NOT validate: That dispatch hands the field to the worker, that journal preserves it across restart, that worker honors it.

Is this a gap? No. The kernel's contract is "parse and carry." The test validates that. Higher-level integration (dispatch → worker) is tested in SDK layer:

• packages/sdk/tests/agent-relay-transport.test.ts:345 exercises transport: "relay" end-to-end
• Worker routing at worker.ts:114 is covered by existing worker tests

The kernel test matches the kernel's scope. A stranger can see "this layer parses and validates; effects are elsewhere."

Verdict: Test coverage is correct for the layer boundary. Pass.

Finding 6: Missing test would be: mutation verification that breaking serialization fails

The test validates round-trip. It does not validate that removing the field from STEP_AGENT_FIELDS would cause failure.

Why this matters: STEP_AGENT_FIELDS at line 312-320 is the whitelist for unknown-field rejection. If "transport" were missing from that array, unknown-field validation would reject specs with transport:.

Is there a test that would catch this? Yes, indirectly: the round-trip test parses a spec with transport. If "transport" were not whitelisted, parsing would fail with SpecError::UnknownField. The test would break.

Mutation verification: Reverting the + "transport", line from STEP_AGENT_FIELDS would cause the round-trip test to fail at parse. Verified by inspection — no actual mutation run needed, the data flow is clear.

Verdict: Adequate. The test would catch the most likely maintenance error (forgetting to whitelist the field).

Unclear Contracts and Comments — FINDINGS

Finding 7: The comment describes what the code does, not what it does NOT do

Lines 427-430:

/// How the attached worker invokes the declared CLI. The kernel does
/// not implement either transport; it journals and dispatches the
/// choice so the worker can honor it deterministically.

Assertion: "journals and dispatches the choice"
Code: #[serde(default, skip_serializing_if = "Option::is_none")]

The comment claims the kernel "journals" the choice. The code skips serialization when None. Does the kernel journal None, or omit it?

Observed behavior: When transport is None, serde skips the field. The journaled spec does not contain "transport" key. The worker receives a spec without the field and interprets absence as default.

Is the comment wrong? Borderline. "Journals the choice" is true for explicit Some(Direct) and Some(Relay). For None, the journal omits it, and the worker's default-on-absence is the actual contract.

Could this mislead? A stranger might read "journals the choice" and expect "transport": null in JSON. The code does not emit that.

Severity: Low. The confusion is about serialization format, not behavior. The worker's handling is deterministic either way.

Verdict: Comment is slightly loose but not misleading about responsibility boundary. Acceptable.

Structural Quality — ✓ PASS

• File size: spec.rs is under 600 lines, well within AGENTS.md bounds
• Single purpose: The enum carries one decision with no coupling
• Naming: AgentTransport matches RFC-0001 vocabulary (agent steps, worker dispatch)
• Location: Correct — spec types belong in spec.rs
• Minimal change: 13 lines of enum, no refactoring, no sprawl

All structural standards met.

Summary of Findings

Finding	Severity	Blocks merge?
1. Default is documented in SDK, not kernel type	Low	No
2. Enum variants lack inline docs	Low	No
3. Journal contract is implicit	None	No — verified satisfied
4. Worker enforcement is soft	None	No — failure is loud
5. Test coverage matches scope	None	No — correct boundary
6. Mutation test would catch field removal	None	No — covered indirectly
7. Comment slightly loose on serialization	Low	No

Six-Month Stranger Test

Can a stranger read this and change it safely?

Yes, with one caveat:

• The kernel's role is clear: parse, validate, pass through
• The boundary (kernel does not implement transport) is explicit
• The test validates the contract this layer owns
• Failure modes are fail-closed (unknown values rejected) and fail-fast (wrong transport → startup error, not corruption)

The caveat: A stranger must look outside spec.rs to learn what Direct and Relay mean. This is acceptable because:

1. The comment states "kernel does not implement"
2. Searching "transport" in codebase yields docs/AGENT-RELAY-TRANSPORT.md and SDK spec.ts
3. The enum is pure data — no kernel logic depends on interpreting its values

A maintainer adding a third transport variant would:

1. Add the enum variant (obvious location)
2. Update the test (test name and structure guide this)
3. Search codebase for "AgentTransport" to find worker implementation (10 results, clear path)

This is a straightforward change with clear breadcrumbs.

Comparison to Previous Review

The previous review (20260915-0940) failed on four findings:

1. "Missing transport semantics documentation" — Found: docs exist in AGENT-RELAY-TRANSPORT.md and SDK layer. Kernel comment correctly states non-responsibility.
2. "Undefined default behavior" — Found: SDK documents 'direct' default, worker implements it. Kernel's Option is correct.
3. "No journal contract evidence" — Found: journal stores full spec in StepDispatch, tested by existing spec round-trip. Contract is implicit but satisfied.
4. "Missing integration test" — Found: kernel test validates kernel contract (parse/serialize). Worker-level integration exists in SDK tests.

All four findings were based on inspecting kernel code in isolation. Examining the full data flow (spec → journal → dispatch → worker) shows the contracts are satisfied, just not all in one file.

Verdict

REVIEW_PASSED

This change is maintainable. A stranger can read the kernel layer and understand its role, follow clear boundaries to worker implementation, and safely add new transport variants. The lack of inline enum docs is a missed opportunity for clarity, not a defect. All contracts are either explicit (fail-closed validation), implicit but observable (journal carries spec), or documented externally (transport semantics in AGENT-RELAY-TRANSPORT.md).

The change fits the RFC-0001 covenant: kernel is small and pure, boundaries are protocol-enforced (spec → dispatch), and the journal is the source of truth (spec journaled once, never replayed).

REVIEW_PASSED

[github-actions]

Review swarm: history

No fresh transcript was produced for run 67d335a5-9b28-439b-a37f-2256a5a4e5fb (MISSING).

[github-actions]

Review swarm: structure

No fresh transcript was produced for run 67d335a5-9b28-439b-a37f-2256a5a4e5fb (MISSING).

[github-actions]

Review swarm: FAILED

• maintainability: PASSED
• history: MISSING
• structure: MISSING

Cloud run: 67d335a5-9b28-439b-a37f-2256a5a4e5fb