Conversation
Session-Id: 01a0bdd5-1542-7fe1-b85c-ada48bf177d9
|
Important
This repository does not receive automatic reviews because it has fewer than 10 stars. ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Advanced Run ID: Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
Surface build/typecheck evidence for the success claim in the PR body. Command: Captured output (exit 0): |
Session-Id: 01a0bdd5-1542-7fe1-b85c-ada48bf177d9
|
Follow-up after the schema validation check found an omitted generated artifact. Fix: committed regenerated packages/schema/flows.schema.json, constrained both authoring transportRetries and kernel max_transport_retries to integer/minimum 0, and added negative, fractional, and explicit-zero parity cases. New exact head: f3bd47f. Command: Captured output: Deterministic regeneration evidence from the same suite: |
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using high effort and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit f3bd47f. Configure here.
maintainability lens — FAILPR #501 — Maintainability reviewBlockers
Concerns
Notes
REVIEW_FAILED |
history lens — FAILBlocker
This also echoes DRIVE-LOG’s recorded #252 failure mode: retry behavior was described more strongly than the actual control flow supported. Here the mismatch is directly observable from the two separate completion paths, so it is not an aspirational deferral. Concern
NotesThe bounded transport budget, narrow failure classification, preserved idempotency key/pins, and terminal handling of ordinary nonzero exits align with recorded history and RFC semantics. The two commit subjects make no false test, evidence, or scope claims. REVIEW_FAILED |
structure lens — MISSING |
|
🎯 review-swarm: FAILED (M:fail H:fail S:missing) Lens transcripts posted as sibling comments above. |
Session-Id: 144d3b43-0019-4de3-988a-7cd9ba4fc148
…repair torn parks Review of #501 (history lens, Cursor Bugbot) found `completion_actions` retried every budget-eligible `crashed`/`lease_expired` without reading the agent step's `recovery_mode`; only the kernel-noticed death (`abandonment_actions`) honoured `manual`. The same dead attempt therefore parked or redispatched depending on who noticed it first, against RFC-0001 Appendix A rule 4. - `completion_actions` parks a `manual` agent step on a worker-reported transport loss (`disposition: park` + `wait.human`), at any transport budget. It takes the journaled `start_pins` so the diff is anchored on the kernel's pin, never the worker's `end_pins` claim. The `wait.human` is built by one shared `manual_park_wait` for both producers. - A park is two appends; dying between them left a permanent, unanswerable park (raised in independent review). `state::park_placeholder_wait_id` names the placeholder and `recovery_actions_filtered` journals the missing `wait.human` on resume, once. Closes the same latent gap on the abandonment path. - `all_backing_off_steps_return_timers` regains a retryable failure precondition (`crashed`; `worker_error` is terminal since the budget split). - `step.attempt.started.max_transport_retries` is always journaled, like `max_iterations`; the old `skip_serializing_if` omitted the explicit zero that explains why a lost process was not retried, and contradicted DESIGN.md. DESIGN.md reconciled and now ties the SDK classifier, the completion-reason alphabet and the kernel disposition together. Tests: core unit (both producers, torn-park repair, journal field), in-process engine crash injection between the two appends, and a real-daemon protocol test (crashed / lease_expired / budget 0, silence probe, SIGKILL + resume, human answer redispatches on the pinned revision). Mutation red/green and full kernel + SDK runs captured in kernel/evidence/501/. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Session-Id: 144d3b43-0019-4de3-988a-7cd9ba4fc148
…p the park sentinel crate-internal, literal mutation transcript Owner audit of 2e784ea asked for three things and a reviewer asked for two more verification captures; nothing in production behaviour changes. - Move the three new manual-recovery unit tests into `machine/recovery_tests.rs`; `machine/tests.rs` keeps the restored `all_backing_off_steps_return_timers` and lends its fixtures as `pub(super)`. No assertion weakened or removed. - `park_placeholder_wait_id` is `pub(crate)` again with no lib.rs re-export; it is a fold sentinel, not kernel API. - Replace the three mutation evidence files with one literal transcript (`mutation-transcript.txt`): pre-mutation sha256, the applied diffs, the red runs, `cp` restore, `sha256sum -c` OK, green runs. The README's "byte-for-byte" claim now points at the command that proves it. - Strip trailing whitespace from captured logs so `git diff --check` passes; README says so. - Add `green-sdk-authored-node-runtime.txt` (standalone suite under isolated Bun 1.4.0 + Node 22.23.2: 14/14) and `codex-live-probe.txt` (one bounded live run of codex-cli 0.154.0 through the direct unattended transport: success; also records the pre-existing `cwd` preflight/run mismatch). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Session-Id: 144d3b43-0019-4de3-988a-7cd9ba4fc148
The journal-facts section of kernel/evidence/501/codex-live-probe.txt named the tool but not the command. It now carries the script, its literal invocation and the output verbatim (AGENTS.md evidence rule). Evidence only. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Session-Id: 144d3b43-0019-4de3-988a-7cd9ba4fc148
|
Final-head handoff for
Exact-head local output is recorded in the PR body and committed under The independent final-head reviewer reported no blocker across maintainability/history/structure and re-executed the extracted recovery tests; its node-local full report is being relayed for portable attachment. The repository Review swarm workflow is currently disabled, so the old No merge performed; human review/merge remains required. |
…t-retry-hardening # Conflicts: # packages/sdk/src/cli/step-failure.ts # packages/sdk/src/failure-kinds.ts
Main's hosted-capability isolation pins software-factory.flow.ts by sha256; only the reviewed bytes are admitted as the extension base. Revert the branch's transportRetries additions there — adopting them requires a re-reviewed pin, which is a separate product decision. The transport-retry feature itself is unaffected.
|
Merge conflict resolution note (b266979): reverted the branch's Main's hosted-capability isolation (#552) pins that file by sha256 — The transport-retry feature itself is unchanged — only the example usage was reverted. If the factory flow should adopt retries, that requires a re-reviewed base-source pin; I left it out rather than updating a security pin to accommodate this branch. Verified: |

Root cause
Direct agent CLIs were always spawned with a piped stdin. The worker opened the
sidechannel around startup and closed the unused pipe after a delay, but Codex
had already detected non-TTY stdin and entered its additional-input lifecycle.
When that process exited, the SDK retained only a generic nonzero code and
stderr. The kernel then conflated semantic iteration with infrastructure
recovery: crashes consumed no semantic iteration and therefore had no separate
bound, while
f.agent()exposed no recovery controls.Change
bounded pre-spawn enrollment window. An unattended CLI starts with stdin
ignored/EOF; only an already-connected
drivepeer gets a pipe.signal, safe OS error code, retryability, and stderr tail.
crashed: signal close,close without status, selected transient spawn errors, and the exact historical
Codex stdin-lifecycle signature. Ordinary nonzero exits stay terminal
worker_error; timeout staystimeout.maxIterations,transportRetries, andrecoveryModetof.agent(),authored lowering, YAML/kernel compilation, reverse compilation, diagnostics,
docs, and examples.
is deterministic and bounded across both reported crashes and abandoned
leases; replacement attempts preserve the idempotency key and pinned revisions.
diagnostic, kernel, pin/idempotency, exhaustion, and crash/resume tests.
Compatibility and safety
maxIterationsremains 1 by default.transportRetriesdefaults to one additional classified infrastructureattempt, preserving the existing one-crash resume contract while removing the
former unbounded loop. Explicit
0disables transport recovery.authored specs and hashes; an explicitly authored zero round-trips.
reset,inspect, andmanualretain the RFC Appendix A recovery meanings.so a returned body
worker_errorcannot replay semantic side effects while agenuinely lost process remains resumable.
Verification evidence
Rust workspace
Command:
Captured result lines, in test-binary execution order (exit 0):
The toolchain also emitted non-fatal
rust-objcopywarnings because itslibLLVM.dylibis absent; the command exited 0 and every test binary abovecompleted.
SDK supported runtime suite
Command:
Captured output:
SDK typecheck and build
Command:
Captured output (exit 0):
Surface and packed-consumer gates
Commands:
Captured output:
packages/surfacebuild, main typecheck, and regression typecheck also exited 0.Review follow-up at exact head 139690f
The review delta closes the manual-recovery gap: worker-reported crashed or lease_expired completions now park under recoveryMode: manual before consulting the retry budget, including a zero budget. The wait.human append uses the journaled start pin, and resume repairs a process death between the park placeholder and wait append idempotently. The general timer test again requires a retryable crash, max_transport_retries is always journaled, and older journals still read through the serde default. Internal helpers remain crate-private.
Committed literal evidence is under kernel/evidence/501/, including mutation-transcript.txt (red mutations, checksummed restore, green rerun), green-kernel.txt, green-sdk-authored-node-runtime.txt, and codex-live-probe.txt.
Exact-head local commands and captured result excerpts:
Repository-wide gate blockers (reported, not hidden)
../ops/cargo.sh fmt --all -- --checkexits 1 on existing formatting driftacross files including
relayflowd/src/engine/remote.rs,relayflowd/src/lib.rs,crash fixtures, schema, memory, and state. First captured hunk:
../ops/cargo.sh clippy --workspace -- -D warningsexits 101 on three existingcollapsible_iffindings:installed Node 26.7.0:
packages/surface npm run typecheck:exampleshas existing URL-global errors:No merge is requested; this PR is ready for independent review at commit
139690f754805ee961bb7f12e2c5d0f4f8b72f46.Note
High Risk
Changes kernel agent completion, recovery mode, and journal semantics—incorrect logic could redispatch when a human park was intended or leave runs stuck without a repairable wait.
Overview
Closes a manual recovery gap: when an agent step uses
recoveryMode: manual, worker-reportedcrashed/lease_expiredcompletions now park withwait.human(using journaled start pins fordiff_ref) instead of taking the transport retry path—matching abandonment handling.manual_park_waitis shared so the two death paths cannot drift.Transport vs semantic retry is enforced in
completion_actions: only classified transport failures can consumemax_transport_retries; ordinaryworker_errorstays terminal.max_transport_retriesis always written onstep.attempt.started(including0).Resume repairs a torn manual park (park completion journaled but
wait.humanmissing) by appending the wait once from existing journal facts.Docs/README add
f.agentmaxIterations/transportRetries/recoveryMode, PTY pre-spawn drive enrollment, and richer failure diagnostics. Regression tests cover manual park, torn-park repair, and bounded transport retry;kernel/evidence/501/captures mutation and green-run logs.Reviewed by Cursor Bugbot for commit b266979. Bugbot is set up for automated code reviews on this repo. Configure here.