docs(reborn): WU-B subagent durability sub-spec - #4582
Merged
Merged
Conversation
Sub-spec for WU-B per docs/plans/2026-06-06-subagent-compaction-impl.md. Blocks WU-C. Doc-only. Covers 4 in-memory stores (gate resolution, goal, tombstone, capability result) + 2 new tables (settlement event log, idempotency ledger). Decides typed-repo vs ScopedFilesystem per _contract-freeze-index.md §2. Introduces CapabilityResultStore + SubagentRestartReconciler traits. Specifies libSQL + PostgreSQL schemas, first-writer-wins semantics, scope propagation, migration/rollback under subagent.background_enabled toggle, and the dual-backend parity test (#4431 follow-on). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Contributor
|
Warning You have reached your daily quota limit. Please wait up to 24 hours and I will start processing your requests again! |
github-actions
Bot
added
scope: docs
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 87eb55484c
ℹ️ 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".
Contributor
There was a problem hiding this comment.
Pull request overview
This doc-only PR introduces the WU‑B “subagent durability sub-spec” that freezes the intended durable schemas and trait surfaces for Reborn subagent persistence (gate resolution, goal, tombstone, capability results) plus a settlement event log and idempotency ledger, to unblock WU‑C implementation work.
Changes:
- Adds a comprehensive durability spec covering per-store backend choices (ScopedFilesystem vs typed repos) and proposed SQL schemas.
- Specifies new trait surfaces (
CapabilityResultStore,SubagentRestartReconciler) and outlines replay/idempotency behavior. - Documents migration/rollback behavior under
subagent.background_enabledand a dual-backend parity test plan.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
henrypark133
commented
Jun 8, 2026
henrypark133
left a comment
henrypark133
left a comment
Collaborator
Author
There was a problem hiding this comment.
Code Review (multi-agent)
Intent: Document durable schema and trait shapes for four in-memory subagent stores and two new tables, blocking WU-C work.
Stats: 17 findings (from 39 raw, 17 after dedup) across 1 file. Reviewers run: security, bugs, performance, tests, conventions, local-patterns, maintainability, pattern-refactor. Reviewers failed: none. Body-only: 2
Security (2)
- Medium sanitized_reason redaction not specified (
docs/reborn/2026-06-08-subagent-durability-spec.md:218-231, confidence 65) — anchor: docs/reborn/2026-06-08-subagent-durability-spec.md:229
§1.5 sanitized_reason TEXT described only as 'redacted; NULL is valid.' No sanitization transform, layer, or source defined. May contain LLM output / user PII. Persists indefinitely. - Low delivery_node unvalidated text stored durably (
docs/reborn/2026-06-08-subagent-durability-spec.md:908-929, confidence 55) — anchor: docs/reborn/2026-06-08-subagent-durability-spec.md:913 (no diff position — body only)
§5.4 delivery_node TEXT NOT NULL described as 'ops debug: hostname or pod identity.' No validation. Compromised node writes arbitrary text into ledger.
Bugs (1)
- Medium capability_results libSQL missing explicit PRIMARY KEY (
docs/reborn/2026-06-08-subagent-durability-spec.md:636-660, confidence 78) — anchor: docs/reborn/2026-06-08-subagent-durability-spec.md:648
§4.6 declares UNIQUE INDEX on result_ref but no PRIMARY KEY column. Inconsistent with every other table in spec (all others declare PK explicitly). May confuse schema reviewers + migration tooling.
Performance (7)
- Medium Durable capacity cap requires SELECT COUNT on every spawn (
docs/reborn/2026-06-08-subagent-durability-spec.md:304-311, confidence 82) — anchor: docs/reborn/2026-06-08-subagent-durability-spec.md:311
§1.7 says enforce MAX_GATE_RECORDS=4096 via count query on every record_awaited_child. In-memory O(1) via cached total_states usize. Durable adds extra SELECT COUNT round-trip before every INSERT. - High Delete path missing tenant_id/user_id — full table scans (
docs/reborn/2026-06-08-subagent-durability-spec.md:291-297, confidence 88) — anchor: docs/reborn/2026-06-08-subagent-durability-spec.md:294
§1.6 DELETE statements use only gate_ref. PKs are (child_run_id, gate_ref) / (gate_ref, child_run_id). Without scope prefix, planner cannot use scoped indexes; sequential scans across tenants. - High Reconciler replay does 4 sequential DB round-trips per log entry (
docs/reborn/2026-06-08-subagent-durability-spec.md:862-897, confidence 90) — anchor: docs/reborn/2026-06-08-subagent-durability-spec.md:870
§5.3 issues 4 sequential async ops per entry: ledger try_insert, tombstone read, capability_result load, gate_store record. N children → 4N sequential round-trips blocking startup. - Medium InMemoryCapabilityResultStore explicitly unbounded — OOM risk (
docs/reborn/2026-06-08-subagent-durability-spec.md:618-623, confidence 85) — anchor: docs/reborn/2026-06-08-subagent-durability-spec.md:620
§4.5 'Mutex<HashMap<String, serde_json::Value>> (no BoundedRing)'. Removes existing BoundedRing cap. Megabyte-scale JSON payloads accumulate unbounded. - Medium write_capability_result serializes payload twice (
docs/reborn/2026-06-08-subagent-durability-spec.md:703-730, confidence 80) — anchor: docs/reborn/2026-06-08-subagent-durability-spec.md:710
§4.8 calls serialized_json_len(&output, ...) (full serialize for byte_len) then passes write.output.clone() to store which serializes again. Two full passes + allocations on megabyte payloads. - Medium Boot-time reconciler blocks all run acceptance synchronously (
docs/reborn/2026-06-08-subagent-durability-spec.md:918-930, confidence 78) — anchor: docs/reborn/2026-06-08-subagent-durability-spec.md:925
§5.6 'Before first run is accepted, composition boot path calls reconciler.replay(&scope).await for each active scope.' Synchronous sequential scan. O(N×M). No timeout guard. - Medium Partial index undelivered_terminal differs between backends (
docs/reborn/2026-06-08-subagent-durability-spec.md:114-116, confidence 75) — anchor: docs/reborn/2026-06-08-subagent-durability-spec.md:183
DUPLICATE of f-bug-4 from different angle. Performance impact: index-only scan libSQL vs heap fetch PostgreSQL on same query.
Tests (3)
- Medium §3.7 positive production-readiness Ready test for tombstone store unnamed (
docs/reborn/2026-06-08-subagent-durability-spec.md:494-495, confidence 80) — anchor: docs/reborn/2026-06-08-subagent-durability-spec.md:494
§3.7 says 'Add symmetric positive test that verified composition reports RebornLoopProductionStatus::Ready' but never names it. WU-C has no concrete acceptance criterion. - Medium §4.8 capability_result_store has rejection test but no symmetric Ready test (
docs/reborn/2026-06-08-subagent-durability-spec.md:760-768, confidence 80) — anchor: docs/reborn/2026-06-08-subagent-durability-spec.md:764
§4.8 names production_readiness_rejects_in_memory_capability_result_store. No symmetric positive test for production_verified(Required) yielding Ready. - Medium §3.6 first-writer-wins correction not verifiable by existing tombstone test (
docs/reborn/2026-06-08-subagent-durability-spec.md:461-465, confidence 75) — anchor: crates/ironclaw_reborn/src/subagent/tombstone_store.rs:62
Existing tombstone_store_is_idempotent_by_child_run writes same value twice — passes under both last-writer-wins AND first-writer-wins. Correction unguarded.
Local Patterns (2)
- Medium PostgreSQL idempotency ledger uses UUID; spec note says TEXT (
docs/reborn/2026-06-08-subagent-durability-spec.md:937-951, confidence 95) — anchor: docs/reborn/2026-06-08-subagent-durability-spec.md:1222
DUPLICATE of f-bug-7. trigger_records sibling uses trigger_id TEXT NOT NULL (postgres.rs:968). Codebase convention is TEXT. - Low delivered_at IS NULL references nonexistent column (
docs/reborn/2026-06-08-subagent-durability-spec.md:1221-1221, confidence 85) — anchor: docs/reborn/2026-06-08-subagent-durability-spec.md:1221
§8.3 + §5.3 reference WHERE delivered_at IS NULL on settlement log. §1.5 schema has only settled_at — no delivered_at column.
Maintainability (2)
- Medium §6.3 uses Pending/Delivered/DiscardedParentGone states absent from §5 ledger schema (
docs/reborn/2026-06-08-subagent-durability-spec.md:1082-1092, confidence 90) — anchor: docs/reborn/2026-06-08-subagent-durability-spec.md:1087
§6.3 references 'Delivered outcome', 'expired Pending entry', 'DiscardedParentGone tombstone' — three-state lifecycle. §5 ledger schema has no status column — binary INSERT-OR-IGNORE table. - Nit CapabilityRunId alias is misnamed and unearned (
docs/reborn/2026-06-08-subagent-durability-spec.md:546-549, confidence 65) — anchor: docs/reborn/2026-06-08-subagent-durability-spec.md:546 (no diff position — body only)
§4.3 defines type CapabilityRunId = TurnRunId. Docstring claims 'wraps LoopResultRef' — incorrect. All sibling traits use TurnRunId directly.
Applies 18 straightforward findings from multi-agent code review on PR #4582. Design-level items still pending discussion. Schema: - F2: PostgreSQL ledger run_id/child_run_id UUID → TEXT (§8.3 convention) - F3: align libSQL/PostgreSQL undelivered_terminal partial index - F5: add result_ref column to subagent_gate_settlement_log both backends - F6: capability_results uses explicit PRIMARY KEY (result_ref) - F8: scope predicates mandatory on UPDATE/DELETE templates in §1.6 - F9: define post-result-write flag update path (separate transaction) - F18: 8 MiB CHECK constraint on capability_results.payload (MUST) Contracts: - F1: CapabilityResultStore trait scope &ResourceScope → &TurnScope - F7: drop CapabilityRunId alias; use TurnRunId directly - F10: specify sanitized_reason source + sanitization transform - F11: specify delivery_node validation (length, allowlist, source) - F12: §6.3 restated in binary INSERT-OR-IGNORE ledger semantics - F13: resolve tombstone trait scope-param decision in spec Doc consistency: - F4: remove delivered_at IS NULL filter (column doesn't exist) Test plan: - F14: name positive production-readiness tests (goal, tombstone, capres) - F15: tombstone first-writer-wins distinguishing test - F16: agent_id cross-leakage parity test - F17: reconciler crash-between-ledger-insert-and-gate-write test Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Resolves two reconciler bugs surfaced by multi-agent review on PR #4582: D1 — Crash-between-ledger-insert-and-gate-write strands parent silently. Idempotency ledger goes two-phase. `delivered_at TIMESTAMPTZ NULL`: - INSERT OR IGNORE leaves `delivered_at = NULL` (pencil receipt; claim, mid-flight). - After successful gate-store write, UPDATE seals row with `delivered_at = NOW()` (pen receipt; final). - Pencil rows surviving a crash become `retryable` on next boot, not silently `skipped_idempotent` as before. Matches the existing `IdempotencyLedger::begin_or_replay` precedent in `crates/ironclaw_product_workflow/src/ledger.rs`. Both gate-store and seal UPDATE are idempotent at the row level so duplicate delivery cannot occur and missed delivery cannot occur. D9 — Orphan settlement-log rows produced perpetual `failed` count. Reconciler now checks `gate_store.gate_exists(scope, gate_ref)` first. If the gate is gone (parent cancelled, gate row deleted): write `SubagentResultTombstone { disposition: DiscardedParentGone }`, seal the ledger row, count as `skipped_orphan`. One pass per orphan; future passes skip via sealed ledger row. Settlement log stays append-only. ReplayReport gains `retryable: u32` and `skipped_orphan: u32` so each counter has one meaning. `failed > 0` is now operator-actionable only — no more phantom alerts. Spec changes: - §5.2 ReplayReport struct extended. - §5.3 algorithm rewritten: gate-exists check, then tombstone check, then pencil-claim, then deliver, then seal. Pencil read on insert-skip distinguishes sealed (skipped_idempotent) from pencil (retryable). - §5.4 + §5.5 ledger DDL: `delivered_at` becomes nullable. INSERT examples split into pencil + seal. - §5.8 test plan: orphan-gate test case added; existing test names updated. - §5.9 risks: stale-children GC bullet rewritten; capability-result- missing conclusion sentence updated. - §6.3 re-flip narrative updated to use sealed/pencil vocabulary. - "Decisions ratified up front" table gains rows 11–13. - Closing checklist gains 3 WU-C action items. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…nsive) Resolves cold-start / scaling concerns surfaced by multi-agent review on PR #4582. Comprehensive: D4 + D5 + 5 long-term concerns folded in. D4 — Reconciler replay batch-phased (no more N+1). §5.3 algorithm rewritten: Phase 0 bound input via LEFT JOIN against ledger (only pending pencil-or-missing rows enter the algorithm; replay's scan size stays proportional to outstanding work, not historical log size). Phase 1 batched preflight: one query for gates_exist_batch, one for read_tombstones_batch. Phase 2 multi-row ledger writes: 2a — orphan + tombstoned cleanup (one upsert-sealed batch) 2b — pencil claim (one INSERT OR IGNORE batch) Phase 3 parallel capability loads via `join_all` (capped at replay_pool size). Phase 4 per-row deliver + seal (sequential per row, each row hits a different parent's mailbox). Phases 0–3 are O(1) DB calls regardless of N. Net cost dominated by Phase 4's per-row delivery, ~5–30 ms per row depending on backend latency. 10–50× speedup over the previous N+1 form. D5 — Background replay + per-scope admission gate. §5.6 composition wire-up rewritten: - Replay dispatched via `tokio::spawn` from boot; foreground traffic accepts immediately (<100 ms cold start regardless of backlog). - Per-scope `ReplayState { completed_at, last_report }` tracks completion. Background-mode `SpawnSubagentPort` consults the gate before admitting; rejects with `SubagentSpawnError::ReplayInProgress` until per-scope replay completes. Foreground / blocking subagent calls NEVER consult this gate. - Dedicated `replay_pool` (default 4 DB connections, configurable via `RebornEventStoreConfig.replay_pool_size`) — replay never starves foreground writes during recovery storms. - Eager active-scope enumeration at boot via runs-table query. Bounded by active-runs count, not historical user count. Lazy per-scope replay deferred as future optimization. Long-term concerns folded in: - HA replicas: spec is HA-safe (correctness via Phase 2b INSERT OR IGNORE + single-winner seal UPDATE), HA-redundant (each replica runs replay independently — N× DB load at boot). Active-active leader election deferred to cross-cutting follow-up. Documented in §5.6 + §5.9. - Settlement log growth: Phase 0 LEFT JOIN bounds input — replay's scan size is independent of historical log size. Archival / materialized-view summarization deferred as ops follow-up. - Replay pool sizing: default 4 fine for typical fan-outs; tuning via P95 metric. Spec does not mandate auto-tuning. §5.7 NEW — Observability contract: - `RebornEventKind::SubagentReplayCompleted` event per scope. - 5 required metrics: replay_duration_seconds (histogram), replay_pending_rows (gauge), replay_outcomes_total{outcome=…} (counter), pencil_age_seconds (gauge), replay_in_progress (gauge). All labeled by (tenant_id, agent_id). - 3 required alerts: `failed > 0`, `pencil_age_seconds > 60`, `replay_duration_seconds{P95} > 30`. - OpenTelemetry spans: one per scope (`reborn.subagent.replay`) + child spans per phase. - WU-F WebUI surfaces `replay_in_progress` per-scope; background-spawn rejection during replay shown to user as "starting up, retrying in N seconds" affordance. Prerequisite for WU-G E2E + WU-F integration. Decisions table gains rows 14–19. Closing checklist gains 7 WU-C action items. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…A+A.B) Resolves hot-path overhead + multi-tenant scaling concerns for the spawn / capability-write / replay paths. D6-A — Durable per-scope capacity counter. Replace per-spawn SELECT COUNT(*) with a sidecar `subagent_gate_capacity_counter` table — one transactional UPDATE per spawn (no extra round-trip). Race-safe via SELECT FOR UPDATE (PG) / BEGIN IMMEDIATE (libSQL). Symmetric increment on INSERT, decrement on delivery / delete via GREATEST(undelivered - N, 0) safety net. E.A — Sharded counter for hot scopes (CAPACITY_COUNTER_BUCKETS = 16). Per-scope counter row becomes a write hotspot when one mega-tenant runs 10k+ concurrent background subagents under the same scope. Shard into K=16 rows per (tenant_id, user_id, agent_id) keyed by `bucket SMALLINT/INTEGER NOT NULL`. Spawn picks bucket via `hash(child_run_id) % K`. Cap check is `SUM(undelivered)` across all K buckets — index-only at K=16. `subagent_gate_awaited_children.counter_bucket` stores bucket-of-record for symmetric decrement on cleanup. Per-scope spawn throughput lifts from ~100/sec (single-row lock contention) to ~1600/sec on PostgreSQL. Drift bound: ≤ K-1 rows over cap under maximum concurrency. D8-A — CapabilityResultStore trait takes Vec<u8>, not serde_json::Value. Executor: `let bytes = serde_json::to_vec(&output)?;` ONCE. `byte_len` is `bytes.len() as u64` — derived for free. `bytes` is MOVED into the store, not cloned. Store INSERTs bytes directly into BLOB (libSQL) / JSONB (PostgreSQL) without re-serializing. `read()` returns Vec<u8>; caller deserializes lazily via `serde_json::from_slice` only when a Value is needed (prompt assembly, compaction). Eliminates 2× full-tree serialization + 1 Value clone per capability call. ~50% CPU reduction on capability-write hot path at production scale. Trait shape reflects what crosses the boundary (bytes, not a tree). Composes with future streaming variants (BoxStream<Bytes>). A.A — Reconciler replay jitter for fleet rollouts. `RebornEventStoreConfig.reconciler_replay_jitter_ms: u64` (default 5000). Each replica sleeps a uniform-random 0..jitter ms before launching its background replay task. Spreads the deploy-time reconciler stampede over a wider window — at 50-replica rollout, peak DB reconciler conn count drops from N×replay_pool to ~jitter-spread fraction. Foreground traffic NEVER pays the jitter cost. Set to 0 for single-node deployments. A.B — HA per-scope leader election (new §5.10, future, NOT WU-C scope). Documented direction: Postgres `pg_try_advisory_xact_lock` per scope. Replicas that lose election skip replay for that scope; still consume settlement events via gate-store mailbox as normal. Total fleet reconciler work drops from O(N × scopes) to O(scopes). Promotion trigger documented (P95 replay duration > 30s + sustained replay_in_progress aggregate > 60s). Lock is transaction-scope so auto-releases on leader crash — composes cleanly with D1's two-phase ledger. libSQL fallback: noop election (every replica is leader); libSQL deployments are typically single-node so redundancy is moot. Decisions table gains rows 20–24 (D6-A, E.A, D8-A, A.A, A.B). Closing checklist gains 4 WU-C action items + 1 follow-up note. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
henrypark133
commented
Jun 9, 2026
henrypark133
left a comment
henrypark133
left a comment
Collaborator
Author
There was a problem hiding this comment.
Code Review (multi-agent) — Round 2
Intent: Doc-only subagent durability sub-spec locking durable schema and trait shapes across libSQL and PostgreSQL.
Stats: 26 findings (from 42 raw, 26 after dedup) across 1 file. Reviewers run: all 8. Failed: none. Body-only: 11
Round 2 surfaced 5 real regressions introduced by prior fix commits — all High severity, all in the §5.3 algorithm + §1.6 SQL templates. Must address before merge.
Security (4)
- High agent_id = ? OR agent_id IS NULL allows cross-agent access when caller has non-NULL agent_id (
docs/reborn/2026-06-08-subagent-durability-spec.md:370-416, conf 75) — anchor: docs/reborn/2026-06-08-subagent-durability-spec.md:373
Settlement UPDATE + delivery-claim UPDATE use(agent_id = ? OR agent_id IS NULL). When caller's TurnScope has non-NULL agent_id, OR-branch matches rows where agent_id IS NULL (system-level rows). Ag... - Medium Delete-path for deliverable_queue + child_index omits user_id + agent_id (
docs/reborn/2026-06-08-subagent-durability-spec.md:444-448, conf 75) — anchor: docs/reborn/2026-06-08-subagent-durability-spec.md:444 (body only)
Delete-path DELETEs fromsubagent_gate_deliverable_queueandsubagent_gate_child_indexonly filtergate_ref + tenant_id. Omits user_id + agent_id. Contradicts §8.2 mandatory predicate rule. - Medium parent_run_context_json may persist sensitive strategy config (
docs/reborn/2026-06-08-subagent-durability-spec.md:99-101, conf 75) — anchor: docs/reborn/2026-06-08-subagent-durability-spec.md:100 (body only)
LoopRunContext stored as JSON blob may contain credentials, API keys, LLM provider tokens via strategy config. Spec doesn't require scrubbing or claim credential-freeness. - Medium sanitized_reason no DB-level length CHECK (
docs/reborn/2026-06-08-subagent-durability-spec.md:322-323, conf 50) — anchor: docs/reborn/2026-06-08-subagent-durability-spec.md:322 (body only)
Column TEXT NOT NULL no constraint. Sanitization 100% call-site convention.
Bugs (4)
- High Tombstoned-result test asserts failed==1 but algorithm routes to skipped_orphan (
docs/reborn/2026-06-08-subagent-durability-spec.md:1067-1088, conf 95) — anchor: docs/reborn/2026-06-08-subagent-durability-spec.md:1340
§5.3 Phase 2a counts pre-tombstoned children as skipped_orphan. §5.8 Tombstoned-result test asserts failed==1, redelivered==0. Direct contradiction. - High Phase 0 LEFT JOIN references s.run_id which does not exist (
docs/reborn/2026-06-08-subagent-durability-spec.md:1038-1046, conf 95) — anchor: docs/reborn/2026-06-08-subagent-durability-spec.md:1040
Settlement log schema hasparent_run_idnotrun_id. Phase 0 SQL joins ons.run_id = l.run_id. SQL error at runtime, blocks all replay. - High Capacity counter SQL uses agent_id = ? without NULL handling — bypasses cap for non-agent runs (
docs/reborn/2026-06-08-subagent-durability-spec.md:344-357, conf 90) — anchor: docs/reborn/2026-06-08-subagent-durability-spec.md:345
Counter SUM and UPDATE useagent_id = ?. NULL parameter evaluates to UNKNOWN in SQL → non-agent runs (agent_id IS NULL) always see SUM=0, bypass 4096 cap entirely. - High Delivery-claim DELETE removes all queue rows for gate, strands N-1 children (
docs/reborn/2026-06-08-subagent-durability-spec.md:400-415, conf 92) — anchor: docs/reborn/2026-06-08-subagent-durability-spec.md:413
DELETEWHERE gate_ref = ? AND tenant_id = ?wipes all N queue entries for a gate. Subsequent deliveries for other children silently lost.
Performance (2)
- Medium PostgreSQL cap-check reads unlocked buckets — drift bound is real not theoretical (
docs/reborn/2026-06-08-subagent-durability-spec.md:365-390, conf 78) — anchor: docs/reborn/2026-06-08-subagent-durability-spec.md:380
FOR UPDATE locks only one bucket. SUM reads other K-1 unlocked. Cap can drift K-1 over. - Low Eager scope enumeration query unspecified — no LIMIT, no timeout (
docs/reborn/2026-06-08-subagent-durability-spec.md:1210-1220, conf 65) — anchor: docs/reborn/2026-06-08-subagent-durability-spec.md:1215 (body only)
§5.6 says eager runs-table query. No SELECT shape, index hint, LIMIT, or timeout.
Tests (7)
- Medium §5.8 reconciler replay 5 tests not defined (
docs/reborn/2026-06-08-subagent-durability-spec.md:1306-1384, conf 85) — anchor: docs/reborn/2026-06-08-subagent-durability-spec.md:1306
All 5 reconciler replay scenarios (happy, double-replay, tombstoned, missing-result, crash-between) missing from crates/ironclaw_reborn_event_store/tests/. - Low §5.5 delivery_node validation test not named (
docs/reborn/2026-06-08-subagent-durability-spec.md:1233-1237, conf 75) — anchor: docs/reborn/2026-06-08-subagent-durability-spec.md:1233 (body only)
Allowlist + length + 'unknown' fallback never tested. - Medium §3.6 first-writer-wins tombstone test not defined (
docs/reborn/2026-06-08-subagent-durability-spec.md:644-649, conf 90) — anchor: crates/ironclaw_reborn/src/subagent/tombstone_store.rs:95
Required test write_tombstone_preserves_first_writer_when_second_write_has_different_disposition not in any test file. - Medium §3.7 positive readiness test for tombstone store not defined (
docs/reborn/2026-06-08-subagent-durability-spec.md:657-657, conf 90) — anchor: crates/ironclaw_reborn/tests/production_readiness.rs:152
production_readiness_accepts_filesystem_subagent_tombstone_store not implemented. - Medium §4.8 capability_result_store positive + negative readiness tests not defined (
docs/reborn/2026-06-08-subagent-durability-spec.md:938-938, conf 90) — anchor: docs/reborn/2026-06-08-subagent-durability-spec.md:938
Both production_readiness_rejects_in_memory_capability_result_store + production_readiness_accepts_production_verified_capability_result_store missing. - Medium §7 cross-agent scoped-query test guards security but deferred to WU-G (
docs/reborn/2026-06-08-subagent-durability-spec.md:1503-1590, conf 85) — anchor: docs/reborn/2026-06-08-subagent-durability-spec.md:1503
gate_resolution_scoped_query_excludes_rows_from_other_agents is security-critical (§1.7 cross-tenant invariant). Deferring to WU-G means WU-C ships without this guard. - Low §4.9 payload cap CapacityExceeded test not named (
docs/reborn/2026-06-08-subagent-durability-spec.md:944-944, conf 75) — anchor: docs/reborn/2026-06-08-subagent-durability-spec.md:944 (body only)
8 MiB cap enforced via CHECK + app-layer. No test verifies typed error.
Conventions (5)
- Medium DiscardedParentGone variant undefined (DUP f-bug-7) (
docs/reborn/2026-06-08-subagent-durability-spec.md:1077-1077, conf 90) — anchor: docs/reborn/2026-06-08-subagent-durability-spec.md:1077
Same as f-bug-7. - Medium InMemoryCapabilityResultStore type contradicted within §4.5 (
docs/reborn/2026-06-08-subagent-durability-spec.md:798-800, conf 95) — anchor: docs/reborn/2026-06-08-subagent-durability-spec.md:800
Line 798:Mutex<HashMap<String, serde_json::Value>>. Line 800:Mutex<HashMap<String, Vec<u8>>>. Trait mandates Vec. First is wrong, would reintroduce D8 regression. - Medium Double-replay test asserts skipped == 1 but ReplayReport has no
skippedfield (docs/reborn/2026-06-08-subagent-durability-spec.md:1330-1330, conf 95) — anchor: docs/reborn/2026-06-08-subagent-durability-spec.md:1330
§5.2 fields: redelivered, skipped_idempotent, retryable, skipped_orphan, failed. Noskipped. Test would fail compile or assert wrong counter. - Low scope_from_run_context called in §4.8 but never defined (
docs/reborn/2026-06-08-subagent-durability-spec.md:882-882, conf 65) — anchor: docs/reborn/2026-06-08-subagent-durability-spec.md:882 (body only)
Helper used in wire-up pseudocode but no definition anywhere in spec or codebase. WU-C must invent semantics. Could drop agent_id or use wrong user_id. - Nit pen-receipt vs pencil-receipt terminology (
docs/reborn/2026-06-08-subagent-durability-spec.md:1184-1184, conf 80) — anchor: docs/reborn/2026-06-08-subagent-durability-spec.md:1184 (body only)
SQL comments use 'pen-receipt seal'. Rest of spec uses 'pencil-receipt'. Inconsistent.
Local Patterns (2)
- Nit Heading style 'Section N — Title' deviates from sibling docs (
docs/reborn/2026-06-08-subagent-durability-spec.md:52-52, conf 75) — anchor: docs/reborn/2026-06-08-subagent-durability-spec.md:52 (body only)
Sibling docs (subagent-spawn/phase-1.md, phase-2.md, 2026-06-04-subagent-compaction-design.md) use bare## N. Title. This spec uses## Section N — Title. - Low subagent-spawn/README.md not updated to link this spec (
docs/reborn/2026-06-08-subagent-durability-spec.md:1-7, conf 75) — anchor: docs/reborn/subagent-spawn/README.md:34 (body only)
README still points to #4147 for durability work. This spec resolves it but README not updated.
Maintainability (2)
- Medium Duplicate §5.8 (DUP f-bug-8) (
docs/reborn/2026-06-08-subagent-durability-spec.md:1302-1306, conf 95) — anchor: docs/reborn/2026-06-08-subagent-durability-spec.md:1302
Same as f-bug-8. - Low §1.7 settlement-log dedup decision deferred but should be resolved here (
docs/reborn/2026-06-08-subagent-durability-spec.md:460-468, conf 75) — anchor: docs/reborn/2026-06-08-subagent-durability-spec.md:464 (body only)
§1.7 says 'Decide before WU-C opens'. Spec presents two options without choosing. WU-C implementer left with unresolved decision.
Round-2 multi-agent review surfaced 5 High-severity regressions
introduced by prior fix commits, plus medium consistency issues.
All resolved here.
SQL TEMPLATES (§1.6) — fix regressions in transaction shapes:
R3 — Conditional agent_id predicate. Replace blanket
`(agent_id = ? OR agent_id IS NULL)` (which lets agent-scoped
callers reach system-level rows) with placeholder
`<agent_predicate>` bound conditionally per caller's scope:
`agent_id = ?` when Some, `agent_id IS NULL` when None.
New §1.6 preamble paragraph documents the rule.
R4 — Capacity counter SUM + UPDATE use the conditional predicate too.
Bare `agent_id = ?` with NULL parameter evaluated to UNKNOWN,
silently bypassing the 4096 cap for non-agent runs.
R5 — Delivery-claim DELETE on deliverable_queue gains `child_run_id`.
Previously wiped ALL queue rows for a gate when only one child
was delivered — stranded N-1 siblings.
R10 — Delivery-claim UPDATE SET also flips `delivery_claimed = 1`
(prose was inconsistent with SQL).
R13 — Delete-path DELETEs on deliverable_queue + child_index gain
`user_id` predicate. awaited_children DELETE uses the
conditional agent_predicate.
R15 — Settlement log dedup decision resolved (was deferred). Ledger
UNIQUE + gate-store idempotency + Phase 0 LEFT JOIN make
duplicate log rows benign; no MIN(id) needed.
R16 — parent_run_context_json gains sensitivity audit requirement
(closing-checklist gate): WU-C MUST verify LoopRunContext is
credential-free or strip sensitive fields at write site.
ALGORITHM PSEUDOCODE (§5.3) — fix wrong column names + bounded fan-out:
R1 — Phase 0 LEFT JOIN uses `s.parent_run_id` (column actually exists;
schema does NOT have `s.run_id`).
R2 — Phase 0 filter uses `s.terminal_kind` (column actually exists;
schema does NOT have `s.event_kind`).
R11 — Phase 3 capability loads use `buffer_unordered(replay_pool_size)`
not `join_all`. Unbounded fan-out at 10k pending rows would
starve foreground writes on the 4-conn replay pool.
R12 — Phase 2a tombstone writes use `write_tombstones_batch` (single
round-trip), not a per-row `for` loop. Trait gains batch method.
TRAIT + VARIANT CONSISTENCY:
R8 — InMemoryCapabilityResultStore type is `Mutex<HashMap<String,
Vec<u8>>>` (was self-contradicted in §4.5 — D8-A regression).
R9 — SubagentResultDisposition variants documented: today's
`DiscardedByParentCancel` + WU-C addition `DiscardedParentGone`
(used by §5.3 Phase 2a orphan cleanup). §3.7 risks bullet
updated. Forward-compat with WU-D variants (`Delivered`,
`SettledByBackground`).
R17 — `scope_from_run_context` helper defined in §4.8 (was undefined).
Maps LoopRunContext → TurnScope; documents user_id resolution
via `explicit_owner_user_id()` + SYSTEM_RESERVED_ID sentinel.
DOC HYGIENE:
R6 — Tombstoned-result test assertion fixed: `skipped_orphan == 1,
failed == 0` (matches §5.3 algorithm; was `failed == 1`).
R7 — Double-replay test assertion fixed: `skipped_idempotent == 1`
(was `skipped == 1` — field does not exist on ReplayReport).
R14 — Duplicate `### 5.8` heading resolved. Test plan now §5.9, Risks
§5.10, HA leader election §5.11.
Pen→pencil terminology consistency in §5.4 + §5.5 SQL comments.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Round-2 Copilot + Codex automated reviewers caught issues skill
reviewers missed. All resolved here.
CF1 — subagent_gate_child_index + subagent_gate_deliverable_queue
gain `user_id TEXT NOT NULL` + `agent_id TEXT` columns per
Decision #5 (also fixes a latent SQL syntax error: prior R13
added user_id to DELETE predicates without the columns
existing). New `idx_sgci_scope` / `idx_sgdq_scope` indexes on
`(tenant_id, user_id, agent_id, child_run_id)` replace the
prior `tenant_child` indexes. Both libSQL + PostgreSQL.
CF2 — capability_results.created_at gains
`DEFAULT (datetime('now'))` in libSQL (PostgreSQL already had
`DEFAULT NOW()`). Needed because §4.6/§4.7 rely on this column
for `idx_capability_results_run` ordering and `list_by_run`
ORDER BY — silent inserter mistakes would break replay
ordering.
CF3 — CapabilityResultStore::write now takes
`invocation_id: InvocationId`. UNIQUE INDEX
`(tenant_id, user_id, run_id, capability_id, invocation_id)`
enforces true first-writer-wins idempotency. Previous design
minted a fresh UUID per call, so `INSERT OR IGNORE` could
never collide — idempotency claim was misleading. Now a
retry-after-transient-error returns the same `result_ref`.
Trait + in-memory impl note + §4.8 wire-up updated; both
backend schemas gain the column + unique index.
CF4 — §6.2 rollback step rewritten. Goal store stays on
FilesystemSubagentGoalStore (durable) when the toggle flips
OFF; the toggle gates only background-mode spawn admission,
NOT backend selection. Prior wording about
"re-selects InMemoryBoundedSubagentGoalStore" contradicted
§2.1.
CF5 — Decision #5 reworded. Scope columns are always PRESENT on
every durable table and reached via a scoped index
(`idx_*_scope`). PKs remain shape-appropriate per table
(e.g. `(gate_ref, child_run_id)`, `(result_ref)`) — scope
need not LEAD every PK. Matches actual schema guidance and
removes the false-positive interpretation that all PKs must
be scope-prefixed.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Two open threads from round-2 review now decided + applied. 1. InMemoryCapabilityResultStore gains bounded eviction. `INMEMORY_CAPABILITY_RESULT_STORE_MAX_ENTRIES = 1024` + `INMEMORY_CAPABILITY_RESULT_STORE_MAX_BYTES = 4 MiB` (FIFO by insertion order). Prevents local-dev / CI OOM on long sessions that accumulate megabyte-scale payloads. Production-readiness check still gates the impl to LocalDevTest mode regardless. 2. `gate_resolution_scoped_query_excludes_rows_from_other_agents` promoted from WU-G to WU-C. This is a security gate (cross-tenant / cross-agent leakage class via missing agent_id predicate), not an E2E gate. Shipping the gate-resolution backend in WU-C without this guard would mean releasing the durable code with no test that catches a missing agent_id WHERE clause — unacceptable per §1.7 + `_contract-freeze-index.md` §8. Closing checklist gains two WU-C action items. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
henrypark133
commented
Jun 9, 2026
henrypark133
left a comment
henrypark133
left a comment
Collaborator
Author
There was a problem hiding this comment.
Code Review (multi-agent) — Round 3 at head 707e2cd4d
Intent: Doc-only WU-B subagent durability sub-spec. Locks durable schema + trait shapes for 6 tables, reconciler algorithm, observability contract. Blocks WU-C.
Stats: 15 findings (from ~30 raw, 15 after dedup) in single file. Reviewers run: security, bugs, performance, tests, conventions, local-patterns, maintainability, pattern-refactor. Reviewers failed: none. All findings body-only — diff was truncated at 40KB of 142KB so inline anchoring is unreliable; use line numbers below.
Prior review state: 2 multi-agent skill review rounds + Codex + Copilot, 39 threads resolved. This round looks at the final spec for remaining internal-consistency gaps + unresolved spec-vs-trait drift.
High-severity
-
High (conf 95) — Three DELETE statements in §1.6 omit
agent_idpredicate (line 431-453). §1.6 itself mandates "Every UPDATE and DELETE MUST scope by the caller's full TurnScope". Three statements violate this:- line 432: delivery-claim path DELETE on
subagent_gate_deliverable_queue— missinguser_idAND<agent_predicate> - line 451: delete path DELETE on
subagent_gate_deliverable_queue— missing<agent_predicate> - line 452-453: delete path DELETE on
subagent_gate_child_index— missing<agent_predicate> - Impact: WU-C following these as written → cross-agent isolation violations. Agent-scoped caller's gate cleanup deletes other agents' rows under same
(tenant_id, user_id). §7.5 gate_resolution scoped-query test catches SELECT but not DELETE. - Fix: Add
AND <agent_predicate>to all three DELETEs; line 432 also needsAND user_id = ?.
- line 432: delivery-claim path DELETE on
-
High (conf 90) — Phase 0 LEFT JOIN missing
agent_idscope predicate (line 1082-1086). Filters onlyWHERE s.tenant_id=$1 AND s.user_id=$2. Whenscope.agent_id is Some(id), returns settlement-log rows belonging to other agents, causing reconciler to attempt delivery into wrong gate store.- Fix: Add
AND (s.agent_id = $3 OR s.agent_id IS NULL)per §1.6 conditional agent_id predicate.
- Fix: Add
-
High (conf 80) — Phase 4 issues per-row seal UPDATE; no batch like Phase 2a (line 1171-1194). Phase 3 prescribes
buffer_unordered. Phase 4 issues oneidempotency_ledger.seal(scope, row.key())per delivered row. 100 rows → 100 individual UPDATEs through 4-conn replay_pool → 25 sequential rounds. Phase 2a solves equivalent shape viawrite_tombstones_batch.- Fix: Add
idempotency_ledger.seal_batch(scope, Vec<LedgerKey>); require Phase 4 to collect keys + seal in single multi-row UPDATE.
- Fix: Add
-
High (conf 75) —
parent_run_context_jsoncredential audit is informational, not a merge-blocking checklist gate (line 470-474). Full LoopRunContext serializes as JSONB/TEXT. §1.7 flags audit required; closing checklist §9 has NO explicit item gating WU-C merge. If WU-C ships without audit and LoopRunContext contains LLM tokens / OAuth credentials / API keys, they persist plaintext in durable replicated SQL.- Fix: Add explicit checklist item:
[ ] LoopRunContext credential audit complete AND (a) zero sensitive fields confirmed + compile-time lint added, OR (b) write-site stripping verified — WU-C PR merge blocked on this item.
- Fix: Add explicit checklist item:
-
High (conf 90) — §5.9 reconciler test scenarios lack exact function names (line 1357-1437). 6 detailed scenarios prescribed but never named as
tests::<module>::<test_name>. WU-C has no canonical names to target.- Fix: Add
tests::reconciler_integration::reconciler_replays_undelivered_settled_child,::reconciler_is_idempotent_on_second_replay,::reconciler_skips_tombstoned_child,::reconciler_counts_failed_on_missing_capability_result,::reconciler_retries_pencil_receipt_from_crashed_prior_pass,::reconciler_skips_orphan_and_seals_ledger.
- Fix: Add
-
High (conf 85) — No named test for CapabilityResultStore payload >8 MiB → CapacityExceeded (line 984-985). §4.9 mandates
CapacityExceedederror. §7.3 parity matrix names onlywrite_returns_same_shape+read_after_write_returns_identical_bytes. App-layer guard bypass returns Backend error instead of CapacityExceeded.- Fix: Add
tests::parity::capability_result_store_write_rejects_payload_exceeding_8_mib_with_capacity_exceededon both backends.
- Fix: Add
-
High (conf 85) — No named test for
SubagentSpawnError::ReplayInProgressadmission gate (line 1301-1305). §5.6 step 5 mandates background-mode spawn rejection while ReplayState[scope].completed_at is None. New user-visible error path with no test.- Fix: Add
tests::reconciler_integration::background_spawn_rejected_with_replay_in_progress_while_reconciler_is_running.
- Fix: Add
Medium-severity (spec internal contradictions)
-
Medium (conf 100) — Decision 24 + closing checklist cite §5.10 for HA leader election; actual section is §5.11 (line 46, 1768). §5.10 = "Risks/open questions"; §5.11 = "HA leader election".
- Fix: Update both references from §5.10 → §5.11.
-
Medium (conf 95) —
join_allvsbuffer_unorderedcontradiction across 4 spec surfaces (lines 36, 1161, 1199, 1756). Decision #14, D4 prose, closing checklist all sayjoin_all. §5.3 pseudocode (line 1161) says "MUST usebuffer_unordered, NOTjoin_all". WU-C reading checklist/decision table → shipsjoin_all→ reintroduces connection-pool starvation regression the spec documents.- Fix: Update Decision #14, D4 prose, and closing checklist to
buffer_unordered(replay_pool_size)so all 4 surfaces agree with §5.3 pseudocode MUST.
- Fix: Update Decision #14, D4 prose, and closing checklist to
-
Medium (conf 95) — Phase 3 pseudocode calls
capability_result_store.load()but §4.3 trait defines.read()(line 1164, 1443). Noloadmethod on the locked trait. WU-C following pseudocode → compile error.- Fix: Replace both
.load(occurrences with.read(.
- Fix: Replace both
-
Medium (conf 85) — Reconciler calls 7 batch methods with no formal trait signatures in spec (lines 1097-1146).
gates_exist_batch,read_tombstones_batch,write_tombstones_batch,upsert_sealed_batch,insert_pencil_batch,read_batch,seal— pseudocode call sites only. Onlywrite_tombstones_batchgets rough prose signature (line 1199). WU-C will reverse-engineer 6 method signatures.- Fix: Add 'Batch method signatures' subsection to §5.2 (or §3) listing async-trait signatures for each batch method.
Medium-severity (bugs in spec semantics)
-
Medium (conf 85) —
skipped_orphancounter conflates pre-tombstoned rows with orphan rows (line 1130-1132).skipped_orphan = orphan_rows.len() + tombstoned_rows.len(). orphan_rows = gate gone; tombstoned_rows = gate live, child pre-tombstoned. Distinct situations. ReplayReport docstring says "gate cleaned up before delivery" — pre-tombstoned doesn't fit.- Fix: Either add separate
skipped_tombstoned: u32counter, or explicitly extend docstring to cover both cases.
- Fix: Either add separate
-
Medium (conf 80) —
subagent_idempotency_ledgerUNIQUE constraint missing scope columns (line 1218). §5.4/§5.5 defineUNIQUE (run_id, child_run_id, terminal_kind). §8.2 requires scoped uniqueness for cross-tenant isolation. UUID collision / data migration / deterministic UUID → second tenant silentlyON CONFLICT DO NOTHING'd.- Fix: Extend UNIQUE to
(tenant_id, user_id, agent_id, run_id, child_run_id, terminal_kind).
- Fix: Extend UNIQUE to
-
Medium (conf 75) —
capability_resultsUNIQUE idempotency index missing agent_id (line 847-848).idx_capability_results_invocationUNIQUE on(tenant_id, user_id, run_id, capability_id, invocation_id)— scope includes agent_id per §4.3 trait. Two agents producing same (tenant, user, run_id, capability_id, invocation_id) → second write silently dropped, first agent's result_ref returned to wrong caller.- Fix: Add agent_id to UNIQUE.
-
Medium (conf 90) —
scope_from_run_contextis identity wrapper; LoopRunContext.scope is already TurnScope (line 926-934). §4.8 specifies new helper inironclaw_reborn_compositionextracting fields from LoopRunContext to reconstruct TurnScope. ButLoopRunContext.scopeIS already TurnScope (host.rs:535). Mapping table contains factual error (run_context.tenant_iddoesn't exist as top-level field). Canonical pattern across codebase isrun_context.scope.clone().- Fix: Replace
scope_from_run_context(write.run_context)withwrite.run_context.scope.clone(). Remove helper + delete mapping table from §4.8.
- Fix: Replace
Recommended: All 7 High-severity items should be addressed before merge — they include real isolation violations (f-bug-1, f-bug-2), data-correctness regressions (f-perf-1, f-bug-3, f-bug-4, f-bug-5), and merge-blocker gaps (f-sec-1 credential audit, f-test-1/2/3 named tests). The 4 internal-contradiction Medium findings (f-conv-1/2/3/4) are mechanical fixes. f-maint-1 deletes dead code.
Round-3 multi-agent review at head 707e2cd found 12 straightforward fixes. All applied here. 3 design-level items deferred to discussion. SCOPE PREDICATE GAPS (security): R3-1 §1.6 three DELETE statements gain conditional <agent_predicate>: delivery-claim path + delete-path queue + delete-path child_index. Without these, agent-scoped callers can delete other agents' auxiliary rows under the same (tenant_id, user_id). R3-2 Phase 0 LEFT JOIN gains <agent_predicate_on_s> — agent-scoped replay must not surface settlement-log rows belonging to other agents. Performance shape paragraph documents the rule. R3-13 subagent_idempotency_ledger UNIQUE constraint extended to include (tenant_id, user_id, agent_id, ...). Without scope cols in the UNIQUE, a cross-tenant collision (UUID or migration artifact) would be silently ON CONFLICT DO NOTHING'd. R3-14 capability_results UNIQUE idempotency index gains agent_id. Two agents producing the same (tenant, user, run_id, capability_id, invocation_id) would otherwise have their second write silently dropped. PostgreSQL uses COALESCE(agent_id, '__non_agent__') for uniqueness across NULL-agent rows. SPEC-VS-TRAIT DRIFT: R3-9 buffer_unordered propagation: Decision #14 + D4 prose + closing checklist all now say buffer_unordered(replay_pool_size). Prior contradiction: §5.3 pseudocode MUSTed buffer_unordered but other surfaces still said join_all. WU-C reading checklist literally would reintroduce the pool-starvation regression. R3-10 Phase 3 pseudocode + §5.10 risks: .load() → .read() to match the §4.3 trait method name. Plus §5.10 capability_result_store.load reference updated. R3-15 scope_from_run_context helper deleted — LoopRunContext.scope IS already TurnScope. Replaced with &write.run_context.scope direct borrow. §4.8 "Scope source" paragraph documents the canonical pattern. CHECKLIST + TEST NAMES: R3-4 Credential audit promoted to MERGE-BLOCKING checklist item (top of list). WU-C MUST complete LoopRunContext audit + add compile- time lint OR verify write-site stripping before merging the durable gate-resolution backend. R3-5 Six reconciler test scenarios in §5.9 get canonical function names under tests::reconciler_integration::*. WU-C now has exact targets for redelivery, idempotency, tombstoned, missing-result, crash-between-insert-and-deliver, and orphan-gate paths. R3-6 §4.9 + §7.3 name the payload-size-cap test: capability_result_store_write_rejects_payload_exceeding_8_mib_with_capacity_exceeded. Asserts typed CapacityExceeded error, not raw Backend/Io error. R3-7 §5.6 names the admission-gate test: background_spawn_rejected_with_replay_in_progress_while_reconciler_is_running. Foreground / blocking subagent paths must succeed throughout. CROSS-REF FIXES: R3-8 Decision #24 + closing checklist HA leader election refs: §5.10 → §5.11 (R14 renumber missed these). DEFERRED FOR DISCUSSION: - R3-3 Phase 4 per-row seal vs seal_batch trait method - R3-11 Formal batch trait signatures subsection placement - R3-12 skipped_orphan counter — split vs combined for tombstoned Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Collaborator
Author
|
Round-3 review fixes applied in commit 12 straightforward fixes applied:
3 design-level items deferred for discussion:
Will discuss the 3 deferred items separately. |
Three coupled design-level changes from round-3 review now ratified
and applied.
R3-3 — Phase 4 seal_batch.
Spec §5.3 Phase 4 previously issued per-row `idempotency_ledger.seal`
on each successful delivery. At 100 children through a 4-conn
replay_pool that is 25 sequential rounds (~125-750 ms) on the seal
step alone.
Fix: Phase 4 now collects sealed-row keys into a `sealed_keys` vec
during the per-row loop, then issues ONE `seal_batch(scope, Vec<
LedgerKey>)` call at the end. Single multi-row UPDATE. Idempotent
per-row via the `delivered_at IS NULL` guard.
Single-row `seal` retained for orphan / tombstone paths in Phase 2a
(which already batch via `upsert_sealed_batch`) and for any future
operator-driven manual interventions on stuck rows.
R3-11 — Formal batch method signatures in §5.2.1.
§5.3 algorithm calls 8 batch methods. Only `write_tombstones_batch`
had a rough signature; the rest were implicit. WU-C would need to
reverse-engineer 7 method signatures from pseudocode call sites.
Fix: new §5.2.1 "Batch method signatures (reconciler-facing)"
subsection lists all 8 method signatures with full async-trait
syntax plus `LedgerKey` + `LedgerRow` struct definitions. Single-
row variants documented alongside batch variants for completeness.
WU-C now reads §5.2.1 literally as the trait surface contract.
R3-12 — Split skipped_orphan counter.
Old: `skipped_orphan = orphan_rows.len() + tombstoned_rows.len()`.
Two semantically distinct cases conflated:
- orphan = gate row gone (parent cancel + cleanup)
- tombstoned = gate live but child pre-tombstoned (parent
cancelled the specific child)
Different operational signals; merging them prevented operators
from distinguishing gate-cleanup spikes (high `skipped_orphan`
alone) from parent-cancel spikes (high `skipped_tombstoned`).
Fix: `ReplayReport` gains `skipped_tombstoned: u32`. Phase 2a
increments each counter independently. §5.7 metric label list
extended; §5.9 `reconciler_skips_tombstoned_child` test asserts
`skipped_tombstoned == 1, skipped_orphan == 0`. Decisions table
row 13 updated to six counters.
Closing checklist gains 3 new WU-C items (seal_batch impl,
batch-method trait surface, ReplayReport split).
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
henrypark133
commented
Jun 9, 2026
henrypark133
left a comment
henrypark133
left a comment
Collaborator
Author
There was a problem hiding this comment.
Code Review (multi-agent) — Round 4 at head e286fe449
Intent: Doc-only WU-B subagent durability sub-spec. Locks durable schema + trait shapes for 6 tables, reconciler algorithm, observability contract. Blocks WU-C.
Stats: 21 findings (from 21 raw, after dedup). All findings on the single changed file. Reviewers run: security, bugs, performance, tests, conventions, local-patterns, maintainability, pattern-refactor. Reviewers failed: none. Body-only: 12. Inline cap: 15.
User-override applied: Prior multi-agent review rounds (R1–R3) addressed many concerns. Findings below were checked against the 3 rounds of addressed-comment dossier. Where a finding restates a prior concern but the author's fix is incomplete (e.g., test patched in one location, missed in sibling location) or applies to NEW code added after the prior fix (two-phase ledger, idempotency ledger, Phase 0 LEFT JOIN), it is emitted as a new finding with explicit prior context. No prior-addressed concern is re-emitted without that context.
security
- High/90 — PostgreSQL idempotency ledger UNIQUE / ON CONFLICT broken for non-agent runs (NULL agent_id) — double-delivery possible (
docs/reborn/2026-06-08-subagent-durability-spec.md:L1350-1374(body-only))- §5.5 PostgreSQL ledger declares
UNIQUE (tenant_id, user_id, agent_id, run_id, child_run_id, terminal_kind). In Postgres, NULL is distinct under UNIQUE — so multiple rows withagent_id IS NULLand identical other columns are all considered unique. `ON CONFLICT (... agent_id ..… - Fix: Replace UNIQUE constraint with a partial UNIQUE INDEX using
COALESCE(agent_id, '__non_agent__'), matching the §1.4 pattern. Update ON CONFLICT to reference the same index.
- §5.5 PostgreSQL ledger declares
- High/85 — Phase 0 comment + §1.7 prose still describe forbidden OR-NULL agent_id pattern (note: prior fix covered templates not prose) (
docs/reborn/2026-06-08-subagent-durability-spec.md:L1176(body-only))- Phase 0 comment line 1176 reads
conditional: s.agent_id = $3 OR s.agent_id IS NULL per caller's scope. §1.7 line 480 says(agent_id = ? OR agent_id IS NULL). §1.6 (lines 338-341) explicitly forbids unconditional OR-NULL: it lets agent-scoped callers reach system-level rows. P… - Fix: Replace both descriptions with the conditional form:
agent_id = ?whenscope.agent_idisSome(id);agent_id IS NULLwhenNone. Never the OR-branch.
- Phase 0 comment line 1176 reads
- High/75 — LoopRunContext credential-audit closing-checklist item lacks MERGE-BLOCKING marker (
docs/reborn/2026-06-08-subagent-durability-spec.md:L469-476)- §1.7 acknowledges
parent_run_context_jsoncould persist credentials ifLoopRunContextincludes strategy/run_profile material that resolves to provider tokens. Audit deferred to WU-C as a closing-checklist gate, but unlike the explicitMERGE-BLOCKING:marker on §1's other it… - Fix: Add
**MERGE-BLOCKING:**marker to the LoopRunContext credential-audit closing-checklist item. Require a linked audit doc or compile-timeassert_credential_free!macro in WU-C's PR description.
- §1.7 acknowledges
- Medium/65 — sanitized_reason 'truncated message prefix' branch may persist user PII (
docs/reborn/2026-06-08-subagent-durability-spec.md:L330(body-only))- §1.5 allows
sanitized_reasonto be 'a fixed-length truncated prefix of the failure message (max 256 chars), with non-ASCII stripped'. If failure messages embed user task text ('Failed to book flight for John Smith…'), the first 256 ASCII chars carry PII. Stripping non-ASCII doe… - Fix: Restrict sanitized_reason to the
LoopFailureKinddiscriminator only. Prohibit truncated message prefixes. If failure messages must be debuggable, store in a separate access-restricted column.
- §1.5 allows
- Medium/60 — delivery_node has no DB-level CHECK constraints — bypassed app validation writes arbitrary content (
docs/reborn/2026-06-08-subagent-durability-spec.md:L1386-1390(body-only))- Length + allowlist invariants enforced only at application layer. Column is
TEXT NOT NULLwith no CHECK. A deployment skipping app validation writes arbitrarily long / arbitrary-char values, bloating every ledger row and enabling log injection in any system that renders the col… - Fix: Add
CHECK (length(delivery_node) <= 128)+CHECK (delivery_node ~ '^[A-Za-z0-9._-]+$')(PostgreSQL) /GLOBequivalent (libSQL) to both DDLs.
- Length + allowlist invariants enforced only at application layer. Column is
bugs
- Critical/90 — Phase 3 buffer_unordered destroys ordering; Phase 4 zip pairs wrong payload with wrong child (
docs/reborn/2026-06-08-subagent-durability-spec.md:L1257-1272)buffer_unorderedemits futures in completion order, not input order. Phase 3 collects via.buffer_unordered(replay_pool_size).collect::<Vec<_>>(). Phase 4 then doesto_attempt.zip(load_results)positionally — pairing row A's identity with row B's payload. Result: `gate_stor…- Fix: Replace
buffer_unorderedwithbuffered(preserves input order, same concurrency bound), OR carry(row_index, future)through Phase 3 and match by identity in Phase 4.
- High/95 — Idempotency ledger seal UPDATE missing user_id + agent_id — violates §1.6 scope-predicate rule (NEW code, not covered by prior F8/R3 fix) (
docs/reborn/2026-06-08-subagent-durability-spec.md:L1338-1341)- Both seal UPDATEs (§5.4 libSQL line 1338, §5.5 PostgreSQL line 1378) filter on
tenant_id + run_id + child_run_id + terminal_kindonly. UNIQUE constraint is(tenant_id, user_id, agent_id, run_id, child_run_id, terminal_kind). §1.6 mandates full scope predicate on every UPDATE/… - Fix: Add
AND user_id = ? AND <agent_predicate>to both seal UPDATE WHERE clauses.
- Both seal UPDATEs (§5.4 libSQL line 1338, §5.5 PostgreSQL line 1378) filter on
- High/85 — Phase 0 LEFT JOIN missing scope columns on ledger side — cross-tenant suppressed replay (
docs/reborn/2026-06-08-subagent-durability-spec.md:L1170-1179(body-only))- Phase 0 LEFT JOIN matches
s.parent_run_id = l.run_id AND s.child_run_id = l.child_run_id AND s.terminal_kind = l.terminal_kindwith no scope columns. WHERE clause scopessbut the JOIN can match a ledger row from any tenant/user/agent sharing the triple. A sealed ledger row f… - Fix: Add scope columns to ON clause:
AND s.tenant_id = l.tenant_id AND s.user_id = l.user_id AND s.agent_id IS NOT DISTINCT FROM l.agent_id.
- Phase 0 LEFT JOIN matches
- Medium/90 — First reconciler test asserts non-existent
skippedfield (prior fix only patched the second test) (docs/reborn/2026-06-08-subagent-durability-spec.md:L1474-1476)- Prior fix (prior-id 3377634497, commit 88a10fc) corrected
skipped == 1→skipped_idempotent == 1inreconciler_is_idempotent_on_second_replay(line 1484). The companion testreconciler_replays_undelivered_settled_childat line 1475 STILL assertsskipped == 0. `ReplayR… - Fix: Update the first test's step 6 to assert all three skip counters:
skipped_idempotent == 0, skipped_orphan == 0, skipped_tombstoned == 0.
- Prior fix (prior-id 3377634497, commit 88a10fc) corrected
- Medium/80 — Phase 2a passes Vec to upsert_sealed_batch which expects Vec (
docs/reborn/2026-06-08-subagent-durability-spec.md:L1224-1226(body-only))- Phase 2a calls
idempotency_ledger.upsert_sealed_batch(scope, orphan_rows ++ tombstoned_rows, delivery_node=self.node_id). Per §5.2.1 the signature is(&TurnScope, Vec<LedgerKey>, String). The arguments passed are settlement-log rows. Conversion from row toLedgerKeyis neve… - Fix: Specify the conversion explicitly: `(orphan_rows ++ tombstoned_rows).map(|r| LedgerKey { tenant_id: r.tenant_id, user_id: r.user_id, agent_id: r.agent_id, run_id: r.parent_run_id, child_run_id: r.chil…
- Phase 2a calls
performance
- Medium/75 — Phase 0 LEFT JOIN scans full settlement log per scope — unbounded growth, no partial index (
docs/reborn/2026-06-08-subagent-durability-spec.md:L1163-1180(body-only))- §5.10 acknowledges settlement log grows indefinitely. Phase 0 scans all rows for a scope to LEFT JOIN against ledger. After months of operation a high-churn tenant returns a large intermediate set before the ledger JOIN reduces it. Archival is deferred to ops follow-up; no partia…
- Fix: Either (a) add a
replayed BOOLEANwatermark on settlement log + partial indexWHERE NOT replayed, OR (b) commit to a concrete archival SLA in §5.10 (e.g., archive sealed rows older than 30 days) a…
- Medium/72 — Active-scope enumeration at boot has no upper bound or timeout (
docs/reborn/2026-06-08-subagent-durability-spec.md:L1393-1416)- §5.6 D17 specifies eager enumeration of all non-terminal runs at boot, claiming bounded by active-runs count. No
max_active_scopes_at_bootcap, no query timeout. Post-rollout or spike scenarios with thousands of active scopes spawn thousands oftokio::spawntasks at once. - Fix: Add config knob
max_active_scopes_at_boot(default ~1000) with overflow scopes lazily replayed on first traffic. Add a 5s query timeout on the enumerationSELECT DISTINCTwithwarn!on slow enum…
- §5.6 D17 specifies eager enumeration of all non-terminal runs at boot, claiming bounded by active-runs count. No
- Medium/68 — Phase 4 fully sequential per-row delivery — N×gate_store_latency dominates replay completion (
docs/reborn/2026-06-08-subagent-durability-spec.md:L1271-1295(body-only))- Per-row delivery runs sequentially with 5–30ms gate-store writes. 100 rows = 0.5–3s; 4096 rows ≈ 2 min Phase 4 wallclock. Background admission gate stays closed for the entire duration → spurious
ReplayInProgresserrors. Spec mentions JoinSet sharding as a future option but def… - Fix: Bound Phase 4 with
JoinSetcapped atmin(replay_pool_size, pending_rows)rather than purely sequential. Tie the bound to the §5.7 P95<30s alert threshold.
- Per-row delivery runs sequentially with 5–30ms gate-store writes. 100 rows = 0.5–3s; 4096 rows ≈ 2 min Phase 4 wallclock. Background admission gate stays closed for the entire duration → spurious
- Medium/65 — InMemoryCapabilityResultStore aggregate caps not paired with explicit per-payload bound (
docs/reborn/2026-06-08-subagent-durability-spec.md:L820-825(body-only))- §4.5 sets
MAX_ENTRIES=1024andMAX_BYTES=4 MiBaggregate; §4.9 sets 8 MiB per-result via SQL CHECK. In-memory enforcement of the 8 MiB per-result limit is not explicit. A single 7.9 MiB write passes the per-result limit but exceeds 4 MiB aggregate and evicts all 1023 prior en… - Fix: State in §4.5 that the in-memory impl rejects writes >8 MiB with
CapacityExceeded(consistent with §4.9), AND that aggregate-cap eviction is FIFO. Add explicit text covering large-payload eviction b…
- §4.5 sets
- Low/60 — capability_results list_by_run includes tombstoned rows? idx_capability_results_run not partial on tombstoned_at (
docs/reborn/2026-06-08-subagent-durability-spec.md:L844-849(body-only))- Spec doesn't say whether
list_by_runexcludes tombstoned results. Index includes all rows. Reconciler/drain_settled callers must filter tombstoned post-fetch. - Fix: Clarify §4.3 inclusion behaviour. If tombstoned excluded, change
idx_capability_results_runto partialWHERE tombstoned_at IS NULL.
- Spec doesn't say whether
tests
- Medium/75 — reconciler_counts_failed_on_missing_capability_result test missing pencil-receipt-survives assertion (
docs/reborn/2026-06-08-subagent-durability-spec.md:L1500-1506)- Test asserts
failed == 1, redelivered == 0only. §5.10 + Phase 4 explicitly require the ledger pencil row remain withdelivered_at IS NULLso the next boot retries. Without an assertion on ledger state, an implementation that drops or seals the row on failure silently disable… - Fix: Add a 4th assertion step that reads the ledger row for (run_id, child_run_id, terminal_kind) and asserts it exists with delivered_at IS NULL.
- Test asserts
- Medium/75 — delivery_node validation invariants (max 128 + allowlist + substitute 'unknown') have no named test (
docs/reborn/2026-06-08-subagent-durability-spec.md:L1386-1391)- §5.5 mandates input-validation rules for
delivery_node(max 128, allowlist[A-Za-z0-9._-]+, substitute literal"unknown"on invalid, never crash). No test in §5.9, §7.3, or closing checklist exercises (a) oversized value substitution, (b) disallowed-chars substitution, (c) … - Fix: Add named test
delivery_node_invalid_substituted_to_unknownthat passes oversized + disallowed-chars + empty values and asserts (i) reconciler does not Err, (ii) written ledger row holds 'unknown'.
- §5.5 mandates input-validation rules for
conventions
- Medium/85 — §1.6 INSERT pseudocode for child_index and deliverable_queue omits required user_id/agent_id columns (
docs/reborn/2026-06-08-subagent-durability-spec.md:L364-367)- §1.3 schemas declare
user_id TEXT NOT NULLandagent_id TEXTon bothsubagent_gate_child_index(lines 134-143) andsubagent_gate_deliverable_queue. §1.6 INSERT pseudocode at lines 365-367 lists only(tenant_id, child_run_id, gate_ref). Verbatim execution → NOT NULL cons… - Fix: Update both INSERT snippets to include
user_idandagent_idcolumns and bind placeholders.
- §1.3 schemas declare
local-patterns
- Medium/75 — SubagentIdempotencyLedger trait has split scope contract — LedgerKey embeds scope but 6/7 methods also take &TurnScope (
docs/reborn/2026-06-08-subagent-durability-spec.md:L1104-1158)LedgerKeyalready carriestenant_id,user_id,agent_id.try_inserttakes onlyLedgerKey. Butread,seal,upsert_sealed_batch,insert_pencil_batch,read_batch,seal_batchall take BOTHscope: &TurnScopeANDLedgerKey. Implementations face two sources of…- Fix: Pick one shape for the whole trait. Recommend dropping
scope: &TurnScopefrom the 6 other methods (matchtry_insert) and derive scope from the keys.
maintainability
- Medium/75 — §5.4/§5.5 SQL path comments reference migrations/*.sql but §8.5 mandates inline Rust constants in migrations.rs (
docs/reborn/2026-06-08-subagent-durability-spec.md:L1307-1349(body-only))- Code blocks at §5.4/§5.5 carry path comments pointing at
migrations/<n>_subagent_idempotency_ledger.sql. §8.5 ('Migration-script convention') states DDL lives incrates/ironclaw_reborn_event_store/src/{libsql,postgres}/migrations.rsas Rust&[(i64, &str, &str)]constants — … - Fix: Remove
migrations/*.sqlpath comments. Replace with// DDL belongs in INCREMENTAL_MIGRATIONS entry (N, "subagent_idempotency_ledger", ...) per §8.5.
- Code blocks at §5.4/§5.5 carry path comments pointing at
- Low/75 — subagent_idempotency_ledger has no PRIMARY KEY column — only table in spec without explicit PK (
docs/reborn/2026-06-08-subagent-durability-spec.md:L1311-1325(body-only))- Every other table declares PK (gate_settlement_log → id INTEGER PRIMARY KEY AUTOINCREMENT, capability_results → result_ref, etc.). Ledger has only UNIQUE. SQLite implicit rowid becomes de-facto PK — inconsistent and confuses migration tooling/schema reviewers.
- Fix: Add
id INTEGER NOT NULL PRIMARY KEY AUTOINCREMENT(libSQL) /id BIGSERIAL NOT NULL PRIMARY KEY(PostgreSQL). Keep the UNIQUE constraint.
Verdict: REQUEST_CHANGES — 1 Critical (buffer_unordered ordering bug silently delivers wrong payload to wrong child) + 6 High findings, most in NEW code added after R3 fixes. f-sec-2 (forbidden OR-NULL in §1.7 prose) is a documentation gap on an architecturally-correct invariant — author's prior reply confirms templates carry conditional predicates; only the prose lags. Treat as note, not block.
R4-1 CRITICAL — Phase 3 buffer_unordered → buffered. buffer_unordered emits futures in completion order, not input order. Phase 4's to_attempt.zip(load_results) pairs row identity with payload positionally → SILENT cross-child payload delivery on every replay with >1 pending row. gate_store.record_background_settlement called with row_A.parent_run_id + row_A.child_run_id + payload_B. Fix: .buffered(replay_pool_size) — same concurrency bound, preserves input order. Decision #14, D4 prose, closing checklist all updated. Schema + SQL invariants: R4-2 Seal UPDATEs (§5.4 libSQL + §5.5 PostgreSQL) add user_id + <agent_predicate> — were missing despite §1.6 mandate. R4-5 §1.6 INSERT pseudocode for child_index + deliverable_queue add user_id + agent_id columns (CF1 added schema cols but not pseudocode → would NOT NULL violation on verbatim execution). f-sec-1 PostgreSQL ledger UNIQUE uses COALESCE(agent_id, '__non_agent__') — NULL agent_id rows were silently double-INSERTable. f-sec-2 §1.7 prose no longer describes forbidden (agent_id = ? OR agent_id IS NULL) pattern; references §1.6 conditional convention instead. f-bug-3 Phase 0 LEFT JOIN matches scope cols on ledger side too — cross-tenant UUID collision could otherwise suppress this tenant's replay. f-bug-5 Phase 2a maps Vec<SettlementLogRow> → Vec<LedgerKey> before upsert_sealed_batch — type mismatch fixed. f-perf-1 + partial-index DDL — idx_subagent_idempotency_ledger_pending (partial on delivered_at IS NULL) added to §5.4 + §5.5 so Phase 0 scan stays bounded by outstanding work. Trait surface + tests: R4-4 reconciler_replays_undelivered_settled_child step 6 fixed: `skipped == 0` → 3 real counter fields. R4-6 SubagentIdempotencyLedger trait drops redundant `scope: &TurnScope` arg from 6 methods — LedgerKey embeds scope (single source of truth). R4-7 reconciler_counts_failed_on_missing_capability_result gains pencil-receipt-survives assertion (delivered_at IS NULL). R4-8 delivery_node_invalid_substituted_to_unknown test named in §5.5 + §5.9 (4 cases: oversized, control chars, disallowed chars, empty). R4-9 §5.6 active-scope enumeration gains max_active_scopes_at_boot (default 1000) cap + 5s timeout + overflow → lazy fallback + operator metrics. f-maint-1 §5.4/§5.5 SQL path comments reference inline Rust constants in migrations.rs per §8.5 (not separate .sql files). R4-3 (MERGE-BLOCKING marker on credential audit): verified already applied via R3-4 (line 1928). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
- R5-1: PG capability_results.payload BYTEA not JSONB (byte-exact round-trip contract; JSONB normalization breaks parity + byte_len) - R5-2: capability-write idempotency conflict target = invocation unique index, insert-then-select read-back (was result_ref, which never conflicts on retry) - R5-3: lazy per-scope replay ships in WU-C as admission-gate trigger (capped scopes were rejected with ReplayInProgress forever) - R5-4: flat tombstone ScopedPath (no thread segment — settlement log carries no thread_id); read_tombstone also gains scope param - R5-5: Phase 3 = exists_batch existence check, no payload loads; delivery via new redeliver_settled_child (record_background_settlement was undefined and payload-shaped) - R5-6: libSQL MAX() not GREATEST in counter decrements - R5-7: tombstoned rows resolve live gate row (capacity-leak fix); new resolve_undeliverable_batch - R5-8: ReplayState keyed by (tenant_id, user_id, agent_id) - R5-9: SubagentReplayCompleted contract-freeze callout - R5-10: CapacityExceeded maps to CapabilityOutcome::Failed, never aborts the loop - minors: 11-method count, pseudocode scope-arg drift, six-counter comment, PG ON CONFLICT expression target, MountView user-isolation verification, stale §5.10 throughput bullet - plan: drop stale duplicate WU-C 'Files modified' block (contradicted the corrected block above it) Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Collaborator
Author
|
Round-5 review fixes applied in commit Blocking fixes:
Major fixes:
Minors: reconciler method count now 11 ( All findings from the codex, Copilot, and multi-agent reviews above were verified against the current file before resolving — the schema-level flags (scope columns on aux tables, |
Audit of existing host plumbing (request_cancel, RunCancellationHandle, children_of, event projection) shows the only gap is model-visible action surface. Ratifies decisions 33-35: two thin WU-D actions (subagent_cancel, subagent_status) over existing machinery; parent- requested cancel delivers a Cancelled settlement (never tombstones — DiscardedByParentCancel stays reserved for the parent-run-cancel cascade); status is metadata-only so settle-time delivery remains the sole sanitization choke point. Child-pushed progress notes deferred pending WU-G evidence. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
theredspoon
pushed a commit
to theredspoon/ironclaw
that referenced
this pull request
Jun 21, 2026
* docs(reborn): WU-B subagent durability sub-spec Sub-spec for WU-B per docs/plans/2026-06-06-subagent-compaction-impl.md. Blocks WU-C. Doc-only. Covers 4 in-memory stores (gate resolution, goal, tombstone, capability result) + 2 new tables (settlement event log, idempotency ledger). Decides typed-repo vs ScopedFilesystem per _contract-freeze-index.md §2. Introduces CapabilityResultStore + SubagentRestartReconciler traits. Specifies libSQL + PostgreSQL schemas, first-writer-wins semantics, scope propagation, migration/rollback under subagent.background_enabled toggle, and the dual-backend parity test (nearai#4431 follow-on). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * docs(reborn): WU-B straightforward review fixes Applies 18 straightforward findings from multi-agent code review on PR nearai#4582. Design-level items still pending discussion. Schema: - F2: PostgreSQL ledger run_id/child_run_id UUID → TEXT (§8.3 convention) - F3: align libSQL/PostgreSQL undelivered_terminal partial index - F5: add result_ref column to subagent_gate_settlement_log both backends - F6: capability_results uses explicit PRIMARY KEY (result_ref) - F8: scope predicates mandatory on UPDATE/DELETE templates in §1.6 - F9: define post-result-write flag update path (separate transaction) - F18: 8 MiB CHECK constraint on capability_results.payload (MUST) Contracts: - F1: CapabilityResultStore trait scope &ResourceScope → &TurnScope - F7: drop CapabilityRunId alias; use TurnRunId directly - F10: specify sanitized_reason source + sanitization transform - F11: specify delivery_node validation (length, allowlist, source) - F12: §6.3 restated in binary INSERT-OR-IGNORE ledger semantics - F13: resolve tombstone trait scope-param decision in spec Doc consistency: - F4: remove delivered_at IS NULL filter (column doesn't exist) Test plan: - F14: name positive production-readiness tests (goal, tombstone, capres) - F15: tombstone first-writer-wins distinguishing test - F16: agent_id cross-leakage parity test - F17: reconciler crash-between-ledger-insert-and-gate-write test Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * docs(reborn): WU-B two-phase ledger + orphan handling (D1+D9) Resolves two reconciler bugs surfaced by multi-agent review on PR nearai#4582: D1 — Crash-between-ledger-insert-and-gate-write strands parent silently. Idempotency ledger goes two-phase. `delivered_at TIMESTAMPTZ NULL`: - INSERT OR IGNORE leaves `delivered_at = NULL` (pencil receipt; claim, mid-flight). - After successful gate-store write, UPDATE seals row with `delivered_at = NOW()` (pen receipt; final). - Pencil rows surviving a crash become `retryable` on next boot, not silently `skipped_idempotent` as before. Matches the existing `IdempotencyLedger::begin_or_replay` precedent in `crates/ironclaw_product_workflow/src/ledger.rs`. Both gate-store and seal UPDATE are idempotent at the row level so duplicate delivery cannot occur and missed delivery cannot occur. D9 — Orphan settlement-log rows produced perpetual `failed` count. Reconciler now checks `gate_store.gate_exists(scope, gate_ref)` first. If the gate is gone (parent cancelled, gate row deleted): write `SubagentResultTombstone { disposition: DiscardedParentGone }`, seal the ledger row, count as `skipped_orphan`. One pass per orphan; future passes skip via sealed ledger row. Settlement log stays append-only. ReplayReport gains `retryable: u32` and `skipped_orphan: u32` so each counter has one meaning. `failed > 0` is now operator-actionable only — no more phantom alerts. Spec changes: - §5.2 ReplayReport struct extended. - §5.3 algorithm rewritten: gate-exists check, then tombstone check, then pencil-claim, then deliver, then seal. Pencil read on insert-skip distinguishes sealed (skipped_idempotent) from pencil (retryable). - §5.4 + §5.5 ledger DDL: `delivered_at` becomes nullable. INSERT examples split into pencil + seal. - §5.8 test plan: orphan-gate test case added; existing test names updated. - §5.9 risks: stale-children GC bullet rewritten; capability-result- missing conclusion sentence updated. - §6.3 re-flip narrative updated to use sealed/pencil vocabulary. - "Decisions ratified up front" table gains rows 11–13. - Closing checklist gains 3 WU-C action items. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * docs(reborn): WU-B reconciler perf + cold-start shape (D4+D5 comprehensive) Resolves cold-start / scaling concerns surfaced by multi-agent review on PR nearai#4582. Comprehensive: D4 + D5 + 5 long-term concerns folded in. D4 — Reconciler replay batch-phased (no more N+1). §5.3 algorithm rewritten: Phase 0 bound input via LEFT JOIN against ledger (only pending pencil-or-missing rows enter the algorithm; replay's scan size stays proportional to outstanding work, not historical log size). Phase 1 batched preflight: one query for gates_exist_batch, one for read_tombstones_batch. Phase 2 multi-row ledger writes: 2a — orphan + tombstoned cleanup (one upsert-sealed batch) 2b — pencil claim (one INSERT OR IGNORE batch) Phase 3 parallel capability loads via `join_all` (capped at replay_pool size). Phase 4 per-row deliver + seal (sequential per row, each row hits a different parent's mailbox). Phases 0–3 are O(1) DB calls regardless of N. Net cost dominated by Phase 4's per-row delivery, ~5–30 ms per row depending on backend latency. 10–50× speedup over the previous N+1 form. D5 — Background replay + per-scope admission gate. §5.6 composition wire-up rewritten: - Replay dispatched via `tokio::spawn` from boot; foreground traffic accepts immediately (<100 ms cold start regardless of backlog). - Per-scope `ReplayState { completed_at, last_report }` tracks completion. Background-mode `SpawnSubagentPort` consults the gate before admitting; rejects with `SubagentSpawnError::ReplayInProgress` until per-scope replay completes. Foreground / blocking subagent calls NEVER consult this gate. - Dedicated `replay_pool` (default 4 DB connections, configurable via `RebornEventStoreConfig.replay_pool_size`) — replay never starves foreground writes during recovery storms. - Eager active-scope enumeration at boot via runs-table query. Bounded by active-runs count, not historical user count. Lazy per-scope replay deferred as future optimization. Long-term concerns folded in: - HA replicas: spec is HA-safe (correctness via Phase 2b INSERT OR IGNORE + single-winner seal UPDATE), HA-redundant (each replica runs replay independently — N× DB load at boot). Active-active leader election deferred to cross-cutting follow-up. Documented in §5.6 + §5.9. - Settlement log growth: Phase 0 LEFT JOIN bounds input — replay's scan size is independent of historical log size. Archival / materialized-view summarization deferred as ops follow-up. - Replay pool sizing: default 4 fine for typical fan-outs; tuning via P95 metric. Spec does not mandate auto-tuning. §5.7 NEW — Observability contract: - `RebornEventKind::SubagentReplayCompleted` event per scope. - 5 required metrics: replay_duration_seconds (histogram), replay_pending_rows (gauge), replay_outcomes_total{outcome=…} (counter), pencil_age_seconds (gauge), replay_in_progress (gauge). All labeled by (tenant_id, agent_id). - 3 required alerts: `failed > 0`, `pencil_age_seconds > 60`, `replay_duration_seconds{P95} > 30`. - OpenTelemetry spans: one per scope (`reborn.subagent.replay`) + child spans per phase. - WU-F WebUI surfaces `replay_in_progress` per-scope; background-spawn rejection during replay shown to user as "starting up, retrying in N seconds" affordance. Prerequisite for WU-G E2E + WU-F integration. Decisions table gains rows 14–19. Closing checklist gains 7 WU-C action items. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * docs(reborn): WU-B hot-path perf + multi-tenant scaling (D6+D8+E.A+A.A+A.B) Resolves hot-path overhead + multi-tenant scaling concerns for the spawn / capability-write / replay paths. D6-A — Durable per-scope capacity counter. Replace per-spawn SELECT COUNT(*) with a sidecar `subagent_gate_capacity_counter` table — one transactional UPDATE per spawn (no extra round-trip). Race-safe via SELECT FOR UPDATE (PG) / BEGIN IMMEDIATE (libSQL). Symmetric increment on INSERT, decrement on delivery / delete via GREATEST(undelivered - N, 0) safety net. E.A — Sharded counter for hot scopes (CAPACITY_COUNTER_BUCKETS = 16). Per-scope counter row becomes a write hotspot when one mega-tenant runs 10k+ concurrent background subagents under the same scope. Shard into K=16 rows per (tenant_id, user_id, agent_id) keyed by `bucket SMALLINT/INTEGER NOT NULL`. Spawn picks bucket via `hash(child_run_id) % K`. Cap check is `SUM(undelivered)` across all K buckets — index-only at K=16. `subagent_gate_awaited_children.counter_bucket` stores bucket-of-record for symmetric decrement on cleanup. Per-scope spawn throughput lifts from ~100/sec (single-row lock contention) to ~1600/sec on PostgreSQL. Drift bound: ≤ K-1 rows over cap under maximum concurrency. D8-A — CapabilityResultStore trait takes Vec<u8>, not serde_json::Value. Executor: `let bytes = serde_json::to_vec(&output)?;` ONCE. `byte_len` is `bytes.len() as u64` — derived for free. `bytes` is MOVED into the store, not cloned. Store INSERTs bytes directly into BLOB (libSQL) / JSONB (PostgreSQL) without re-serializing. `read()` returns Vec<u8>; caller deserializes lazily via `serde_json::from_slice` only when a Value is needed (prompt assembly, compaction). Eliminates 2× full-tree serialization + 1 Value clone per capability call. ~50% CPU reduction on capability-write hot path at production scale. Trait shape reflects what crosses the boundary (bytes, not a tree). Composes with future streaming variants (BoxStream<Bytes>). A.A — Reconciler replay jitter for fleet rollouts. `RebornEventStoreConfig.reconciler_replay_jitter_ms: u64` (default 5000). Each replica sleeps a uniform-random 0..jitter ms before launching its background replay task. Spreads the deploy-time reconciler stampede over a wider window — at 50-replica rollout, peak DB reconciler conn count drops from N×replay_pool to ~jitter-spread fraction. Foreground traffic NEVER pays the jitter cost. Set to 0 for single-node deployments. A.B — HA per-scope leader election (new §5.10, future, NOT WU-C scope). Documented direction: Postgres `pg_try_advisory_xact_lock` per scope. Replicas that lose election skip replay for that scope; still consume settlement events via gate-store mailbox as normal. Total fleet reconciler work drops from O(N × scopes) to O(scopes). Promotion trigger documented (P95 replay duration > 30s + sustained replay_in_progress aggregate > 60s). Lock is transaction-scope so auto-releases on leader crash — composes cleanly with D1's two-phase ledger. libSQL fallback: noop election (every replica is leader); libSQL deployments are typically single-node so redundancy is moot. Decisions table gains rows 20–24 (D6-A, E.A, D8-A, A.A, A.B). Closing checklist gains 4 WU-C action items + 1 follow-up note. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * docs(reborn): WU-B round-2 review fixes (R1-R17) Round-2 multi-agent review surfaced 5 High-severity regressions introduced by prior fix commits, plus medium consistency issues. All resolved here. SQL TEMPLATES (§1.6) — fix regressions in transaction shapes: R3 — Conditional agent_id predicate. Replace blanket `(agent_id = ? OR agent_id IS NULL)` (which lets agent-scoped callers reach system-level rows) with placeholder `<agent_predicate>` bound conditionally per caller's scope: `agent_id = ?` when Some, `agent_id IS NULL` when None. New §1.6 preamble paragraph documents the rule. R4 — Capacity counter SUM + UPDATE use the conditional predicate too. Bare `agent_id = ?` with NULL parameter evaluated to UNKNOWN, silently bypassing the 4096 cap for non-agent runs. R5 — Delivery-claim DELETE on deliverable_queue gains `child_run_id`. Previously wiped ALL queue rows for a gate when only one child was delivered — stranded N-1 siblings. R10 — Delivery-claim UPDATE SET also flips `delivery_claimed = 1` (prose was inconsistent with SQL). R13 — Delete-path DELETEs on deliverable_queue + child_index gain `user_id` predicate. awaited_children DELETE uses the conditional agent_predicate. R15 — Settlement log dedup decision resolved (was deferred). Ledger UNIQUE + gate-store idempotency + Phase 0 LEFT JOIN make duplicate log rows benign; no MIN(id) needed. R16 — parent_run_context_json gains sensitivity audit requirement (closing-checklist gate): WU-C MUST verify LoopRunContext is credential-free or strip sensitive fields at write site. ALGORITHM PSEUDOCODE (§5.3) — fix wrong column names + bounded fan-out: R1 — Phase 0 LEFT JOIN uses `s.parent_run_id` (column actually exists; schema does NOT have `s.run_id`). R2 — Phase 0 filter uses `s.terminal_kind` (column actually exists; schema does NOT have `s.event_kind`). R11 — Phase 3 capability loads use `buffer_unordered(replay_pool_size)` not `join_all`. Unbounded fan-out at 10k pending rows would starve foreground writes on the 4-conn replay pool. R12 — Phase 2a tombstone writes use `write_tombstones_batch` (single round-trip), not a per-row `for` loop. Trait gains batch method. TRAIT + VARIANT CONSISTENCY: R8 — InMemoryCapabilityResultStore type is `Mutex<HashMap<String, Vec<u8>>>` (was self-contradicted in §4.5 — D8-A regression). R9 — SubagentResultDisposition variants documented: today's `DiscardedByParentCancel` + WU-C addition `DiscardedParentGone` (used by §5.3 Phase 2a orphan cleanup). §3.7 risks bullet updated. Forward-compat with WU-D variants (`Delivered`, `SettledByBackground`). R17 — `scope_from_run_context` helper defined in §4.8 (was undefined). Maps LoopRunContext → TurnScope; documents user_id resolution via `explicit_owner_user_id()` + SYSTEM_RESERVED_ID sentinel. DOC HYGIENE: R6 — Tombstoned-result test assertion fixed: `skipped_orphan == 1, failed == 0` (matches §5.3 algorithm; was `failed == 1`). R7 — Double-replay test assertion fixed: `skipped_idempotent == 1` (was `skipped == 1` — field does not exist on ReplayReport). R14 — Duplicate `### 5.8` heading resolved. Test plan now §5.9, Risks §5.10, HA leader election §5.11. Pen→pencil terminology consistency in §5.4 + §5.5 SQL comments. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * docs(reborn): WU-B Copilot + Codex review fixes (CF1-CF5) Round-2 Copilot + Codex automated reviewers caught issues skill reviewers missed. All resolved here. CF1 — subagent_gate_child_index + subagent_gate_deliverable_queue gain `user_id TEXT NOT NULL` + `agent_id TEXT` columns per Decision #5 (also fixes a latent SQL syntax error: prior R13 added user_id to DELETE predicates without the columns existing). New `idx_sgci_scope` / `idx_sgdq_scope` indexes on `(tenant_id, user_id, agent_id, child_run_id)` replace the prior `tenant_child` indexes. Both libSQL + PostgreSQL. CF2 — capability_results.created_at gains `DEFAULT (datetime('now'))` in libSQL (PostgreSQL already had `DEFAULT NOW()`). Needed because §4.6/§4.7 rely on this column for `idx_capability_results_run` ordering and `list_by_run` ORDER BY — silent inserter mistakes would break replay ordering. CF3 — CapabilityResultStore::write now takes `invocation_id: InvocationId`. UNIQUE INDEX `(tenant_id, user_id, run_id, capability_id, invocation_id)` enforces true first-writer-wins idempotency. Previous design minted a fresh UUID per call, so `INSERT OR IGNORE` could never collide — idempotency claim was misleading. Now a retry-after-transient-error returns the same `result_ref`. Trait + in-memory impl note + §4.8 wire-up updated; both backend schemas gain the column + unique index. CF4 — §6.2 rollback step rewritten. Goal store stays on FilesystemSubagentGoalStore (durable) when the toggle flips OFF; the toggle gates only background-mode spawn admission, NOT backend selection. Prior wording about "re-selects InMemoryBoundedSubagentGoalStore" contradicted §2.1. CF5 — Decision #5 reworded. Scope columns are always PRESENT on every durable table and reached via a scoped index (`idx_*_scope`). PKs remain shape-appropriate per table (e.g. `(gate_ref, child_run_id)`, `(result_ref)`) — scope need not LEAD every PK. Matches actual schema guidance and removes the false-positive interpretation that all PKs must be scope-prefixed. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * docs(reborn): WU-B resolve 2 leftover review items Two open threads from round-2 review now decided + applied. 1. InMemoryCapabilityResultStore gains bounded eviction. `INMEMORY_CAPABILITY_RESULT_STORE_MAX_ENTRIES = 1024` + `INMEMORY_CAPABILITY_RESULT_STORE_MAX_BYTES = 4 MiB` (FIFO by insertion order). Prevents local-dev / CI OOM on long sessions that accumulate megabyte-scale payloads. Production-readiness check still gates the impl to LocalDevTest mode regardless. 2. `gate_resolution_scoped_query_excludes_rows_from_other_agents` promoted from WU-G to WU-C. This is a security gate (cross-tenant / cross-agent leakage class via missing agent_id predicate), not an E2E gate. Shipping the gate-resolution backend in WU-C without this guard would mean releasing the durable code with no test that catches a missing agent_id WHERE clause — unacceptable per §1.7 + `_contract-freeze-index.md` §8. Closing checklist gains two WU-C action items. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * docs(reborn): WU-B round-3 review fixes (R3-1..R3-15) Round-3 multi-agent review at head 707e2cd found 12 straightforward fixes. All applied here. 3 design-level items deferred to discussion. SCOPE PREDICATE GAPS (security): R3-1 §1.6 three DELETE statements gain conditional <agent_predicate>: delivery-claim path + delete-path queue + delete-path child_index. Without these, agent-scoped callers can delete other agents' auxiliary rows under the same (tenant_id, user_id). R3-2 Phase 0 LEFT JOIN gains <agent_predicate_on_s> — agent-scoped replay must not surface settlement-log rows belonging to other agents. Performance shape paragraph documents the rule. R3-13 subagent_idempotency_ledger UNIQUE constraint extended to include (tenant_id, user_id, agent_id, ...). Without scope cols in the UNIQUE, a cross-tenant collision (UUID or migration artifact) would be silently ON CONFLICT DO NOTHING'd. R3-14 capability_results UNIQUE idempotency index gains agent_id. Two agents producing the same (tenant, user, run_id, capability_id, invocation_id) would otherwise have their second write silently dropped. PostgreSQL uses COALESCE(agent_id, '__non_agent__') for uniqueness across NULL-agent rows. SPEC-VS-TRAIT DRIFT: R3-9 buffer_unordered propagation: Decision #14 + D4 prose + closing checklist all now say buffer_unordered(replay_pool_size). Prior contradiction: §5.3 pseudocode MUSTed buffer_unordered but other surfaces still said join_all. WU-C reading checklist literally would reintroduce the pool-starvation regression. R3-10 Phase 3 pseudocode + §5.10 risks: .load() → .read() to match the §4.3 trait method name. Plus §5.10 capability_result_store.load reference updated. R3-15 scope_from_run_context helper deleted — LoopRunContext.scope IS already TurnScope. Replaced with &write.run_context.scope direct borrow. §4.8 "Scope source" paragraph documents the canonical pattern. CHECKLIST + TEST NAMES: R3-4 Credential audit promoted to MERGE-BLOCKING checklist item (top of list). WU-C MUST complete LoopRunContext audit + add compile- time lint OR verify write-site stripping before merging the durable gate-resolution backend. R3-5 Six reconciler test scenarios in §5.9 get canonical function names under tests::reconciler_integration::*. WU-C now has exact targets for redelivery, idempotency, tombstoned, missing-result, crash-between-insert-and-deliver, and orphan-gate paths. R3-6 §4.9 + §7.3 name the payload-size-cap test: capability_result_store_write_rejects_payload_exceeding_8_mib_with_capacity_exceeded. Asserts typed CapacityExceeded error, not raw Backend/Io error. R3-7 §5.6 names the admission-gate test: background_spawn_rejected_with_replay_in_progress_while_reconciler_is_running. Foreground / blocking subagent paths must succeed throughout. CROSS-REF FIXES: R3-8 Decision #24 + closing checklist HA leader election refs: §5.10 → §5.11 (R14 renumber missed these). DEFERRED FOR DISCUSSION: - R3-3 Phase 4 per-row seal vs seal_batch trait method - R3-11 Formal batch trait signatures subsection placement - R3-12 skipped_orphan counter — split vs combined for tombstoned Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * docs(reborn): WU-B round-3 design decisions (R3-3 + R3-11 + R3-12) Three coupled design-level changes from round-3 review now ratified and applied. R3-3 — Phase 4 seal_batch. Spec §5.3 Phase 4 previously issued per-row `idempotency_ledger.seal` on each successful delivery. At 100 children through a 4-conn replay_pool that is 25 sequential rounds (~125-750 ms) on the seal step alone. Fix: Phase 4 now collects sealed-row keys into a `sealed_keys` vec during the per-row loop, then issues ONE `seal_batch(scope, Vec< LedgerKey>)` call at the end. Single multi-row UPDATE. Idempotent per-row via the `delivered_at IS NULL` guard. Single-row `seal` retained for orphan / tombstone paths in Phase 2a (which already batch via `upsert_sealed_batch`) and for any future operator-driven manual interventions on stuck rows. R3-11 — Formal batch method signatures in §5.2.1. §5.3 algorithm calls 8 batch methods. Only `write_tombstones_batch` had a rough signature; the rest were implicit. WU-C would need to reverse-engineer 7 method signatures from pseudocode call sites. Fix: new §5.2.1 "Batch method signatures (reconciler-facing)" subsection lists all 8 method signatures with full async-trait syntax plus `LedgerKey` + `LedgerRow` struct definitions. Single- row variants documented alongside batch variants for completeness. WU-C now reads §5.2.1 literally as the trait surface contract. R3-12 — Split skipped_orphan counter. Old: `skipped_orphan = orphan_rows.len() + tombstoned_rows.len()`. Two semantically distinct cases conflated: - orphan = gate row gone (parent cancel + cleanup) - tombstoned = gate live but child pre-tombstoned (parent cancelled the specific child) Different operational signals; merging them prevented operators from distinguishing gate-cleanup spikes (high `skipped_orphan` alone) from parent-cancel spikes (high `skipped_tombstoned`). Fix: `ReplayReport` gains `skipped_tombstoned: u32`. Phase 2a increments each counter independently. §5.7 metric label list extended; §5.9 `reconciler_skips_tombstoned_child` test asserts `skipped_tombstoned == 1, skipped_orphan == 0`. Decisions table row 13 updated to six counters. Closing checklist gains 3 new WU-C items (seal_batch impl, batch-method trait surface, ReplayReport split). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * docs(reborn): WU-B round-4 review fixes (R4-1 critical + 12 more) R4-1 CRITICAL — Phase 3 buffer_unordered → buffered. buffer_unordered emits futures in completion order, not input order. Phase 4's to_attempt.zip(load_results) pairs row identity with payload positionally → SILENT cross-child payload delivery on every replay with >1 pending row. gate_store.record_background_settlement called with row_A.parent_run_id + row_A.child_run_id + payload_B. Fix: .buffered(replay_pool_size) — same concurrency bound, preserves input order. Decision #14, D4 prose, closing checklist all updated. Schema + SQL invariants: R4-2 Seal UPDATEs (§5.4 libSQL + §5.5 PostgreSQL) add user_id + <agent_predicate> — were missing despite §1.6 mandate. R4-5 §1.6 INSERT pseudocode for child_index + deliverable_queue add user_id + agent_id columns (CF1 added schema cols but not pseudocode → would NOT NULL violation on verbatim execution). f-sec-1 PostgreSQL ledger UNIQUE uses COALESCE(agent_id, '__non_agent__') — NULL agent_id rows were silently double-INSERTable. f-sec-2 §1.7 prose no longer describes forbidden (agent_id = ? OR agent_id IS NULL) pattern; references §1.6 conditional convention instead. f-bug-3 Phase 0 LEFT JOIN matches scope cols on ledger side too — cross-tenant UUID collision could otherwise suppress this tenant's replay. f-bug-5 Phase 2a maps Vec<SettlementLogRow> → Vec<LedgerKey> before upsert_sealed_batch — type mismatch fixed. f-perf-1 + partial-index DDL — idx_subagent_idempotency_ledger_pending (partial on delivered_at IS NULL) added to §5.4 + §5.5 so Phase 0 scan stays bounded by outstanding work. Trait surface + tests: R4-4 reconciler_replays_undelivered_settled_child step 6 fixed: `skipped == 0` → 3 real counter fields. R4-6 SubagentIdempotencyLedger trait drops redundant `scope: &TurnScope` arg from 6 methods — LedgerKey embeds scope (single source of truth). R4-7 reconciler_counts_failed_on_missing_capability_result gains pencil-receipt-survives assertion (delivered_at IS NULL). R4-8 delivery_node_invalid_substituted_to_unknown test named in §5.5 + §5.9 (4 cases: oversized, control chars, disallowed chars, empty). R4-9 §5.6 active-scope enumeration gains max_active_scopes_at_boot (default 1000) cap + 5s timeout + overflow → lazy fallback + operator metrics. f-maint-1 §5.4/§5.5 SQL path comments reference inline Rust constants in migrations.rs per §8.5 (not separate .sql files). R4-3 (MERGE-BLOCKING marker on credential audit): verified already applied via R3-4 (line 1928). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * docs(reborn): WU-B round-5 review fixes (R5-1..R5-10) - R5-1: PG capability_results.payload BYTEA not JSONB (byte-exact round-trip contract; JSONB normalization breaks parity + byte_len) - R5-2: capability-write idempotency conflict target = invocation unique index, insert-then-select read-back (was result_ref, which never conflicts on retry) - R5-3: lazy per-scope replay ships in WU-C as admission-gate trigger (capped scopes were rejected with ReplayInProgress forever) - R5-4: flat tombstone ScopedPath (no thread segment — settlement log carries no thread_id); read_tombstone also gains scope param - R5-5: Phase 3 = exists_batch existence check, no payload loads; delivery via new redeliver_settled_child (record_background_settlement was undefined and payload-shaped) - R5-6: libSQL MAX() not GREATEST in counter decrements - R5-7: tombstoned rows resolve live gate row (capacity-leak fix); new resolve_undeliverable_batch - R5-8: ReplayState keyed by (tenant_id, user_id, agent_id) - R5-9: SubagentReplayCompleted contract-freeze callout - R5-10: CapacityExceeded maps to CapabilityOutcome::Failed, never aborts the loop - minors: 11-method count, pseudocode scope-arg drift, six-counter comment, PG ON CONFLICT expression target, MountView user-isolation verification, stale §5.10 throughput bullet - plan: drop stale duplicate WU-C 'Files modified' block (contradicted the corrected block above it) Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * docs(reborn): WU-B add §9 parent-initiated child cancel + inspect Audit of existing host plumbing (request_cancel, RunCancellationHandle, children_of, event projection) shows the only gap is model-visible action surface. Ratifies decisions 33-35: two thin WU-D actions (subagent_cancel, subagent_status) over existing machinery; parent- requested cancel delivers a Cancelled settlement (never tombstones — DiscardedByParentCancel stays reserved for the parent-run-cancel cascade); status is metadata-only so settle-time delivery remains the sole sanitization choke point. Child-pushed progress notes deferred pending WU-G evidence. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Doc-only PR per WU-B in
docs/plans/2026-06-06-subagent-compaction-impl.md. Blocks WU-C. Locks the durable schema + trait shapes for the four in-memory subagent stores (gate resolution, goal, tombstone, capability result) + two new tables (settlement event log, idempotency ledger) + reconciler + observability contract.Commit log
87eb55484421c285b710ea9624201047888ef8fc0094788a10fc3ad2d2a7289707e2cd4dWhat this sub-spec locks down
CapabilityResultStore(&TurnScope+invocation_id+Vec<u8>),SubagentRestartReconciler,SubagentResultTombstoneStore(with batch + scope arg).SubagentRestartReconciler::replay— 5 phases: bound input (LEFT JOIN), batched preflight, batched ledger writes, parallel capability loads (buffer_unordered), per-row deliver+seal.delivered_at) → gate write → pen receipt (UPDATEdelivered_at = NOW()). Crash-between-insert-and-deliver is recoverable on next boot.gate_existsreturns false. Never strands operator with phantom failures.tokio::spawnreplay; foreground unblocked; per-scope admission gate for background mode only.invocation_idkey, not freshly-minted UUID).tenant_id + user_id + conditional <agent_predicate>.agent_id = ?for Some-caller,agent_id IS NULLfor None-caller. Never blanket OR.Review activity
Closing checklist status
All required
[ ]items in the closing checklist are deliverables for WU-C (durable stores implementation PR). This sub-spec PR has no remaining[ ]items pointing back at itself.WU-C handoff
WU-C is the follow-up implementation PR. Per
docs/plans/2026-06-06-subagent-compaction-impl.mdWU-C reads §1-§8 of this spec literally — schemas + trait signatures + algorithm + observability contract are all locked. Closing checklist is the WU-C todo list.🤖 Generated with Claude Code