commit 99c4a2f2f91d337c3e5f7ed0b24dce03c10f46fa Author: Fabro Date: Sun May 3 23:08:41 2026 -0400 init run ⚒️ Generated with [Fabro](https://fabro.sh) diff --git a/graph.fabro b/graph.fabro new file mode 100644 index 000000000..2d53bb7b7 --- /dev/null +++ b/graph.fabro @@ -0,0 +1,37 @@ +digraph ImplementPlan { + graph [ + goal="Implement and simplify", + model_stylesheet=" + * { model: claude-opus-4-6; } + " + ] + rankdir=LR + + start [shape=Mdiamond, label="Start"] + exit [shape=Msquare, label="Exit"] + + toolchain [label="Toolchain", shape=parallelogram, script="command -v cargo >/dev/null || { curl --proto '=https' --tlsv1.2 -sSf https://sh.rustup.rs | sh -s -- -y && sudo ln -sf $HOME/.cargo/bin/* /usr/local/bin/; }; cargo --version 2>&1", max_retries=0] + preflight_compile [label="Preflight Compile", shape=parallelogram, script="cargo check -q --workspace 2>&1", max_retries=0] + preflight_lint [label="Preflight Lint", shape=parallelogram, script="cargo +nightly-2026-04-14 clippy -q --workspace -- -D warnings 2>&1", max_retries=0] + fix_lints [label="Fix Lints", prompt="The preflight lint step failed. Read the build output from context and fix all clippy lint warnings.", max_visits=3] + implement [label="Implement", prompt="Read the plan file referenced in the goal and implement every step. Make all the code changes described in the plan. Use red/green TDD."] + simplify_opus [label="Simplify (Opus)", prompt="@prompts/simplify.md"] + simplify_gpt [label="Simplify (GPT-54)", prompt="@prompts/simplify.md", model="gpt-54"] + verify [label="Verify", shape=parallelogram, script="cargo +nightly-2026-04-14 clippy -q --workspace -- -D warnings 2>&1 && cargo nextest run --cargo-quiet --workspace --status-level fail 2>&1", goal_gate=true, retry_target="fixup"] + fixup [label="Fixup", prompt="The verify step failed. Read the build output from context and fix all clippy lint warnings and test failures.", max_visits=3] + fmt [label="Format", shape=parallelogram, script="cargo +nightly-2026-04-14 fmt --all 2>&1", max_retries=0] + + start -> toolchain + toolchain -> preflight_compile [condition="outcome=succeeded"] + toolchain -> exit + preflight_compile -> preflight_lint [condition="outcome=succeeded"] + preflight_compile -> exit + preflight_lint -> implement [condition="outcome=succeeded"] + preflight_lint -> fix_lints + fix_lints -> preflight_lint + implement -> simplify_opus -> simplify_gpt -> verify + verify -> fmt [condition="outcome=succeeded"] + verify -> fixup + fixup -> verify + fmt -> exit +} diff --git a/run.json b/run.json new file mode 100644 index 000000000..296852ec9 --- /dev/null +++ b/run.json @@ -0,0 +1,521 @@ +{ + "spec": { + "run_id": "01KQRFAT7326ADECC8AAWBXK6M", + "settings": { + "project": { + "name": null, + "description": null, + "directory": ".", + "metadata": {} + }, + "workflow": { + "name": null, + "description": null, + "graph": "workflow.fabro", + "metadata": {} + }, + "run": { + "goal": { + "type": "inline", + "value": "# Patch silent-degrade sites in fabro-workflow / fabro-sandbox\n\n> **Note on filename:** `make-a-plan-to-abstract-hamming.md` is the harness-prescribed\n> path for this plan and does not reflect the content. Future readers should treat the\n> file body as authoritative.\n\n## Context\n\nThe user noticed that when `worktree_mode = always` is set and the cwd is not a Git repo,\n`resolve_worktree_base_sha` (`lib/crates/fabro-workflow/src/pipeline/initialize.rs:74-76`)\nreturns `Ok(None)` on `\"not a git repository\"`, the caller's `else` branch (line 506-508)\nwraps the bare sandbox, and `options.run_options.git` is reset to `None` — with no\n`Emitter::notice` and no `tracing::warn!`. The user asked for X, got not-X, and was told nothing.\n\nA short audit surfaced several more sites with the same anti-pattern. Goal: every\n*genuine* silent-degrade site emits a stable signal that reaches `fabro logs` / SSE / retro.\nSandbox-internal sites without an Emitter are tracing-only, with the event-stream\nfollow-up tracked separately. Behavior is unchanged — the fallback still happens; it just\nannounces itself.\n\n## Revised fix pattern\n\nThis pattern was rewritten in response to reviewer feedback (Event::trace already logs\nwarn-level notices; failover already has a typed event; not every `Ok(None)` is a degradation).\n\n1. **Default:** at the fallback site, call\n `emitter.notice(RunNoticeLevel::Warn, \"\", \"\")`.\n This routes through `Event::RunNotice` whose `Event::trace()` arm\n (`event/events.rs:696-710`) already emits a `warn!(code, message, \"Run notice\")` — so\n a notice alone covers both `server.log` and the run feed.\n2. **Add a separate `tracing::warn!` only when** there are structured diagnostic fields\n absent from the notice trace (`error = %err`, `refspec`, `provider`, `model`,\n `worktree_mode`, etc.). Plain restatements of the notice message do not justify a\n second log line.\n3. **Use a typed event when one already exists** (e.g. `Event::Failover` for LLM provider\n failover, `Event::RetroFailed` for retro problems). Don't introduce a parallel\n `RunNotice` for behavior already represented by a typed event.\n4. **Gate the warning on user intent.** If the fallback path is the *expected* outcome\n for the user's configuration (e.g. local in-place, no-clone sandbox), do not warn.\n Only warn when the user implicitly or explicitly asked for the non-fallback path.\n5. **Stable code naming:** `_` lowercase snake. Codes are a contract;\n pick once.\n6. **Severity:** `RunNoticeLevel::Warn` for \"user asked for X, didn't get X.\"\n `RunNoticeLevel::Info` for benign post-conditions like sandbox preserved. LLM\n failover uses `Event::Failover` (already typed), not `RunNotice`.\n\nReference: `dirty_worktree` notice at `pipeline/initialize.rs:156-161`; `git_diff_failed`\nat `lifecycle/git.rs:306-310`; existing `Event::Failover` emit at `handler/llm/api.rs:510-520`.\n\n## Per-site patches\n\n### 1. Worktree skipped on non-git cwd *(the original)*\n\n`lib/crates/fabro-workflow/src/pipeline/initialize.rs:506-520`\n\nIn the `else` branch at line 506 (where `resolve_worktree_base_sha` returned `Ok(None)`):\n\n- `tracing::warn!(worktree_mode = ?options.worktree_mode, \"worktree requested but cwd is not a git repository; running without a worktree\")`\n — keeps the structured `worktree_mode` field that's not in the notice payload.\n- `options.emitter.notice(RunNoticeLevel::Warn, \"worktree_skipped_no_git\", \"Worktree mode requested but no Git repository was found; running without a worktree.\")`\n\n### 2. Sandbox `setup_git` returned `Ok(None)` **when git was expected**\n\n`lib/crates/fabro-workflow/src/pipeline/initialize.rs:602-626`\n\nThe `Ok(None) => {}` arm covers two real cases:\n\na. The sandbox had an origin / git was expected → `Ok(None)` is a degradation.\nb. The sandbox is clone-less / no-origin → `Ok(None)` is the normal outcome.\n\nThe surrounding code already discriminates: line 596 only calls `ensure_git_available`\nwhen `sandbox.origin_url().is_some()`. Reuse that signal:\n\n```rust\nOk(None) => {\n if sandbox.origin_url().is_some() {\n options.emitter.notice(\n RunNoticeLevel::Warn,\n \"sandbox_git_unavailable\",\n \"Sandbox could not set up Git despite a configured origin; running without checkpointing or PR support.\",\n );\n }\n}\n```\n\nNo additional `tracing::warn!` — the notice trace covers it; no extra structured fields\nworth emitting.\n\n### 3. Checkpoint push failure\n\n`lib/crates/fabro-workflow/src/lifecycle/git.rs:277-289`\n\nExisting `tracing::warn!(refspec, error, ...)` at line 280-284 carries structured\nfields and stays. Add a notice in the same `Err(err)` arm, before `false`:\n\n```rust\nself.emitter.emit(&Event::RunNotice {\n level: RunNoticeLevel::Warn,\n code: \"git_push_failed\".to_string(),\n message: format!(\"Failed to push run branch {branch}: {err}\"),\n});\n```\n\n(Matches the local style at `lifecycle/git.rs:306-310` which already builds `Event::RunNotice`\ndirectly because `self.emitter` is a `&Emitter`.)\n\n### 4. Parallel base checkpoint failure\n\n`lib/crates/fabro-workflow/src/handler/parallel.rs:200-209`\n\nExisting `tracing::warn!(error = %e, ...)` at line 206 stays. Add a notice between the\n`warn!` and `None`:\n\n```rust\nservices.run.emitter.notice(\n RunNoticeLevel::Warn,\n \"parallel_base_checkpoint_failed\",\n format!(\"Could not checkpoint base state before parallel branches: {e}\"),\n);\n```\n\nUpdate the file-top imports to include `RunNoticeLevel` from `crate::event`\n(see `pipeline/initialize.rs` for the same import shape).\n\n### 5. GitHub token mint failure\n\n`lib/crates/fabro-workflow/src/pipeline/initialize.rs:238-247`\n\nAlready emits `notice(\"github_token_failed\", …)`. The notice message embeds `{e}` as a\nplain string, but the structured `error` field is absent from the notice trace\n(`events.rs:704-706` only carries `code` and `message`). Per the revised rule\n(structured fields not in the notice trace justify a separate `tracing::warn!`), add\na structured warn line immediately before the existing `emitter.notice(...)`:\n\n```rust\ntracing::warn!(error = %e, \"Failed to mint GitHub token\");\n```\n\n### 6. LLM provider failover surfaced through `one_shot` path\n\n`lib/crates/fabro-workflow/src/handler/llm/api.rs:283-402`\n\nThe existing `chat()` path at line 510-520 emits `Event::Failover` per attempt with\n`stage`, `from_provider/model`, `to_provider/model`, `error`. The `one_shot` path\n(line 283-402) does not, because:\n\n- `AgentApiBackend` has no `emitter` field (struct definition at 117-125).\n- The `CodergenBackend::one_shot` trait method has no emitter parameter (signature at 283-288).\n\nApproach: plumb `&Arc` into the trait, then emit the existing `Event::Failover`\n(no new code; reuses what `chat()` already does).\n\nSteps:\n\n1. Change `CodergenBackend::one_shot` signature in the trait at `handler/agent.rs:36-…`\n (default method at `handler/agent.rs:50`) to add `emitter: &Arc` and a\n `&StageScope` parameter (mirroring `chat()`'s emit at `api.rs:510-520`). Then update\n every implementor — find them with:\n ```\n rg -n \"async fn one_shot\\(\" lib/crates/fabro-workflow\n ```\n At time of writing this finds:\n - Trait default — `handler/agent.rs:50`\n - `AgentApiBackend::one_shot` — `handler/llm/api.rs:283`\n - `BackendRouter::one_shot` — `handler/llm/cli.rs:808`\n (`AgentCliBackend` uses the trait default, not its own impl — leave as-is.)\n - Test stubs in `handler/prompt.rs:277, 337, 394`\n - Integration test stub in `tests/it/integration.rs:6219`\n Re-run the rg before editing in case more impls have been added.\n2. Caller `handler/prompt.rs:107-109` passes `&services.run.emitter` and the prompt's\n `stage_scope`.\n3. Inside the `one_shot` failover loop in `api.rs:349-399`, emit `Event::Failover` per\n attempt, exactly as the `chat()` loop at line 510-520 does. Each iteration of the\n `for target in fallback_chain` loop emits one event before attempting the call.\n4. **Delete the existing `tracing::warn!` at `api.rs:361-369`.** `Event::Failover::trace()`\n at `events.rs:1083-1090` already emits `warn!(stage, from_provider, from_model,\n to_provider, to_model, error, ...)` — identical fields. Keeping both produces a\n duplicate WARN per attempt. (The `chat()` path correctly does not have a\n parallel `tracing::warn!`; this is making `one_shot` consistent with it.) No new\n `RunNotice` code; `agent.failover` is the canonical event name (`event/names.rs:113`).\n\nTests: extend whatever exercises the one-shot failover branch to assert an\n`agent.failover` event is recorded.\n\n### 7. Sandbox stdout/stderr drain failure (tracing-only, scope-bounded)\n\n`lib/crates/fabro-sandbox/src/local.rs:282-295`\n\n`fabro-sandbox` has no `Emitter` access at this depth, and the event-stream surface\nis `SandboxEventCallback`. Plumbing a new `SandboxEvent::PipeReadFailed` through\n`fabro-types` + `event_name` + `EventBody` is intentionally out of scope for this batch\n(decided with the user). Do the tracing-only fix here:\n\n```rust\nlet stdout_task = tokio::spawn(async move {\n let mut buf = String::new();\n if let Some(ref mut r) = stdout_pipe {\n if let Err(err) = r.read_to_string(&mut buf).await {\n tracing::warn!(error = %err, stream = \"stdout\", \"Failed to drain child stdout\");\n }\n }\n buf\n});\n// same shape for stderr_task\n```\n\nGoal-narrowing acknowledgment: this site is fixed in `server.log` only — event-stream\nvisibility is a follow-up.\n\n## Out of scope (verified — adequately surfaced today)\n\n- **MCP server failed (`fabro-agent/src/session.rs:253-265`)** — emits\n `AgentEvent::McpServerFailed` *and* `tracing::warn!`. Adequate.\n- **Retro failures (`pipeline/retro.rs:19, 28, 40`)** — emits `Event::RetroFailed` *and*\n `tracing::warn!`. Adequate.\n- **`pipeline/finalize.rs:72` (`state_result.ok()`)** / `pipeline/pull_request.rs:205`\n / `pipeline/initialize.rs:340-346` — internal projection / explicit user config; not\n silent-degrade.\n\n## Follow-ups (deliberately deferred)\n\n- Plumb `SandboxEvent::PipeReadFailed` through the existing `SandboxEventCallback`\n (variant + `event_name` + `EventBody` mapping per `docs/internal/events-strategy.md`)\n so site 7's truncation reaches the run feed.\n\n## Files to modify\n\n1. `lib/crates/fabro-workflow/src/pipeline/initialize.rs` (sites 1, 2, 5)\n2. `lib/crates/fabro-workflow/src/lifecycle/git.rs` (site 3)\n3. `lib/crates/fabro-workflow/src/handler/parallel.rs` (site 4 — also import `RunNoticeLevel`)\n4. `lib/crates/fabro-workflow/src/handler/agent.rs` (site 6 — `CodergenBackend` trait + default `one_shot` signature)\n5. `lib/crates/fabro-workflow/src/handler/llm/api.rs` (site 6 — `AgentApiBackend::one_shot` impl + `Event::Failover` emit + delete duplicate `tracing::warn!`)\n6. `lib/crates/fabro-workflow/src/handler/llm/cli.rs` (site 6 — `BackendRouter::one_shot` forward params)\n7. `lib/crates/fabro-workflow/src/handler/prompt.rs` (site 6 — caller plumbing + test stubs at 277/337/394)\n8. `lib/crates/fabro-workflow/tests/it/integration.rs` (site 6 — test stub at 6219)\n9. `lib/crates/fabro-sandbox/src/local.rs` (site 7 — tracing only)\n\nRe-run `rg -n \"async fn one_shot\\(\" lib/crates/fabro-workflow` before editing site 6 to\ncatch any new `one_shot` impls added since this plan.\n\n## Stable codes added\n\n- `worktree_skipped_no_git` — Warn (site 1)\n- `sandbox_git_unavailable` — Warn (site 2, gated on `origin_url.is_some()`)\n- `git_push_failed` — Warn (site 3)\n- `parallel_base_checkpoint_failed` — Warn (site 4)\n\n(Site 5 reuses the existing `github_token_failed` notice; only adds a structured\n`tracing::warn!`. Site 6 reuses the existing `agent.failover` event; no new stable\nnotice code or `RunNotice` code is introduced.)\n\n## Verification\n\n1. Build: `cargo build --workspace`\n2. Unit tests: `cargo nextest run -p fabro-workflow -p fabro-sandbox`\n3. New unit tests, one per behavioral change:\n - **Worktree skip** — extend the existing\n `resolve_worktree_plan_uses_local_worktree_without_pre_run_git_context`\n (`pipeline/initialize.rs:957`) into an `init`-level test using a non-git scratch\n dir; assert a `RunNotice` with code `worktree_skipped_no_git` is emitted.\n - **Sandbox git unavailable (gated)** — two cases: (a) sandbox with `origin_url =\n Some(...)` returning `Ok(None)` from `setup_git` emits `sandbox_git_unavailable`;\n (b) sandbox with `origin_url = None` returning `Ok(None)` emits *no* notice.\n Confirms the gating works.\n - **Push failure** — extend lifecycle/git tests to fake a failing `git_push_ref`\n and assert `git_push_failed` notice + the push_results entry.\n - **Parallel base checkpoint failure** — `handler/parallel.rs` calls the free\n function `checked_git_checkpoint(...)` (line 188) on the sandbox; there is no\n creator interface to fake. Test by constructing the parallel handler with a\n scripted sandbox where the git probe succeeds (so `git_state` is `Some(_)`) and\n the actual `git commit` / checkpoint command fails, populate `services.git_state`,\n and assert the `parallel_base_checkpoint_failed` notice fires. Pattern off\n existing parallel-handler tests in the same file.\n - **One-shot LLM failover** — `AgentApiBackend::one_shot` constructs its\n `Client::from_source(self.source.as_ref())` internally (`api.rs:289`), so the\n existing seam is the `Arc`. Test approach: provide a stub\n `CredentialSource` returning credentials that point at an `httpmock` server,\n program mock A to return a failover-eligible status (e.g. 529 / overloaded for\n Anthropic), program mock B to return success, and assert exactly one\n `Event::Failover` was emitted with the right `from_*` / `to_*` properties.\n Mirror an existing `httpmock`-based test from `fabro-llm` integration tests if\n one already covers `failover_eligible` mapping.\n - **GitHub token mint warn** — site 5 only adds a `tracing::warn!`; the user-facing\n notice is unchanged. If existing tests cover the `Err(e)` arm of `mint_token`\n (likely in `pipeline::initialize::tests` or `fabro-github` tests), assert the\n warn line via `tracing-test` / `tracing_subscriber::fmt::TestWriter`. Otherwise,\n this is covered by code inspection plus the manual smoke run; do not add a new\n test just for the warn line.\n - **Sandbox pipe drain** — a closed pipe reads as `Ok(0)` (EOF), not `Err`, so a\n direct unit test of the inline closure is awkward. Two acceptable approaches:\n (a) extract the read loop into a `drain_pipe(reader: &mut R, stream: &str)`\n helper and unit-test it with a custom `AsyncRead` impl whose `poll_read` returns\n `Poll::Ready(Err(io::Error::other(\"simulated\")))`; or (b) keep the inline\n `if let Err(...) = ...` and verify only by manual smoke (run a command that\n terminates abnormally and confirm the WARN line in `server.log`). Prefer (a) if\n the small refactor is cheap; otherwise (b) is fine — record the choice in the\n PR description.\n4. **Manual smoke test (concrete)** for the worktree case end-to-end. Build the\n workflow inline so the test does not depend on repo workflows or LLM credentials —\n only the sandbox needs to start:\n ```bash\n tmp=$(mktemp -d)\n mkdir -p \"$tmp/.fabro/workflows/baresmoke\"\n cat > \"$tmp/.fabro/workflows/baresmoke/workflow.toml\" <<'EOF'\n _version = 1\n\n [workflow]\n graph = \"workflow.fabro\"\n EOF\n cat > \"$tmp/.fabro/workflows/baresmoke/workflow.fabro\" <<'EOF'\n digraph BareSmoke {\n graph [goal=\"non-git smoke test for worktree_skipped_no_git\"]\n rankdir=LR\n\n start [shape=Mdiamond, label=\"Start\"]\n exit [shape=Msquare, label=\"Exit\"]\n\n hello [label=\"Hello\", shape=parallelogram, script=\"echo hello\"]\n\n start -> hello -> exit\n }\n EOF\n cd \"$tmp\"\n fabro run baresmoke --no-retro --auto-approve\n ```\n - `fabro run ` resolves `/.fabro/workflows//workflow.toml`, so the\n workflow must land at that exact path.\n - The `script` node uses the same shape as the existing `smoke` workflow\n (`.fabro/workflows/smoke/workflow.fabro`) — `parallelogram` + `script=\"...\"` —\n which runs purely in the sandbox shell with no LLM calls. `goal_gate=true` is\n intentionally omitted: the real `smoke` workflow pairs it with `retry_target=exit`\n in graph attrs, and using `goal_gate` without `retry_target` trips the\n `goal_gate_has_retry` validation warning. This smoke only needs to reach\n initialization and exec one command, so the gate isn't needed.\n - `mktemp -d` is intentionally non-git, so this exercises the worktree-skipped path\n even with `worktree_mode = always` (the local-sandbox default).\n - Confirm `/logs/server.log` has `code=\"worktree_skipped_no_git\" ... \"Run notice\"`.\n - Confirm `fabro logs ` (or the SSE/UI run feed) shows the notice.\n5. Format and lint:\n - `cargo +nightly-2026-04-14 fmt --all`\n - `cargo +nightly-2026-04-14 clippy --workspace --all-targets -- -D warnings`\n\n## Reviewer feedback acknowledgments (round 1 P1/P2 incorporated)\n\n- LLM failover patch redesigned around the existing `Event::Failover` and a\n trait-signature change to `CodergenBackend::one_shot`; `self.emitter` was a fiction.\n- \"Always emit both notice and `tracing::warn!`\" rule replaced with a structured-fields\n predicate; relies on `Event::trace()` for the warn-level log of every notice.\n- `setup_git` `Ok(None)` warning is now gated on `sandbox.origin_url().is_some()`.\n- LLM failover severity contradiction removed; `RunNotice`-vs-`Event::Failover`\n distinction now explicit.\n- Site 7 reframed as deliberate scope narrowing with a follow-up; goal text updated.\n- Test plan now covers sites 4, 6, and 7.\n- Manual smoke command made self-contained with a copied fixture workflow.\n\n## Reviewer feedback acknowledgments (round 2)\n\n- **Failover duplicate WARN**: `Event::Failover::trace()` (`events.rs:1083-1090`) already\n emits `warn!` with the same fields as the existing `tracing::warn!` at `api.rs:361-369`.\n Plan now explicitly deletes that line as part of site 6.\n- **Trait file**: `handler/agent.rs` (where `CodergenBackend` and the default `one_shot`\n live) added to files-to-modify.\n- **Implementor list**: replaced the hand-written list with an `rg` recipe; the only\n real impls today are the trait default, `AgentApiBackend`, and `BackendRouter`. Test\n stubs are now called out separately. `AgentCliBackend` does not have its own `one_shot`.\n- **Parallel emitter handle**: now `services.run.emitter`, with the `RunNoticeLevel`\n import call-out.\n- **Pipe-drain test**: closed pipes read as EOF; replaced the \"pre-closed reader\"\n shorthand with a real choice between (a) extract a `drain_pipe` helper testable with\n a custom `AsyncRead`, or (b) drop the automated test and rely on manual smoke.\n\n## Reviewer feedback acknowledgments (round 3)\n\n- **Smoke workflow doesn't exist**: this repo's workflows are `gh-triage`, `hello`,\n `implement-issue`, `implement-plan`, `smoke` — no `repl`, and `hello` requires LLM\n credentials. Smoke recipe rewritten to build a minimal command-only workflow inline\n using the same `parallelogram` + `script=\"...\"` shape used by `.fabro/workflows/smoke/`,\n so it runs purely in the sandbox shell without LLM creds.\n- **GitHub token rule contradiction**: an embedded `{e}` in a notice message is not a\n structured field. Site 5 reinstated with `tracing::warn!(error = %e, ...)` to honor\n the structured-fields rule. Removed the contradicting \"out of scope\" entry.\n- **Failover test injection**: `AgentApiBackend::one_shot` constructs\n `Client::from_source(self.source.as_ref())` internally, so the seam is the existing\n `Arc`. Test recipe spelled out with stub `CredentialSource` +\n `httpmock` returning failover-eligible from A and success from B.\n- **Parallel test wording**: there is no checkpoint-creator interface — the handler\n calls free function `checked_git_checkpoint(...)`. Test recipe rewritten to drive\n a scripted sandbox where the git probe succeeds and the checkpoint command fails,\n with `services.git_state = Some(_)`.\n\n## Reviewer feedback acknowledgments (round 4)\n\n- **Files-to-modify**: site 5 also lives in `pipeline/initialize.rs`; entry corrected\n to `(sites 1, 2, 5)`.\n- **Site 5 verification**: added an explicit verification entry stating the new\n `tracing::warn!` is covered by code inspection plus the manual smoke run, with an\n optional `tracing-test` assertion if existing token-mint test scaffolding exists.\n- **Smoke `goal_gate` validation warning**: `goal_gate=true` removed from the inline\n `hello` node, with an explanation note. Real `smoke` works because it pairs\n `goal_gate` with `retry_target=exit` in graph attrs; we don't need the gate at all\n for this verification.\n- **\"No new code\" wording**: clarified — site 6 introduces trait plumbing and an\n `Event::Failover` emit, but no new stable `RunNotice` code (`agent.failover` was\n already canonical).\n" + }, + "working_dir": null, + "metadata": {}, + "inputs": {}, + "model": { + "provider": "anthropic", + "name": "claude-sonnet-4-6", + "fallbacks": [] + }, + "git": { + "author": null + }, + "prepare": { + "commands": [], + "timeout_ms": 300000 + }, + "execution": { + "mode": "normal", + "approval": "prompt", + "retros": true + }, + "checkpoint": { + "exclude_globs": [] + }, + "sandbox": { + "provider": "daytona", + "preserve": false, + "devcontainer": false, + "env": {}, + "local": { + "worktree_mode": "always" + }, + "docker": { + "image": "buildpack-deps:noble", + "network_mode": null, + "memory_limit": 4000000000, + "cpu_quota": 200000, + "env_vars": {}, + "skip_clone": false + }, + "daytona": { + "auto_stop_interval": 30, + "labels": { + "repo": "fabro-sh/fabro" + }, + "snapshot": { + "name": "fabro-v8", + "cpu": 8, + "memory_gb": 16, + "disk_gb": 20, + "dockerfile": { + "type": "inline", + "value": "FROM ubuntu:24.04\n\nRUN apt-get update && apt-get install -y --no-install-recommends curl git ca-certificates build-essential pkg-config libssl-dev unzip python3 && rm -rf /var/lib/apt/lists/*\n\n# GitHub CLI\nRUN curl -fsSL https://cli.github.com/packages/githubcli-archive-keyring.gpg | dd of=/usr/share/keyrings/githubcli-archive-keyring.gpg && echo \"deb [arch=$(dpkg --print-architecture) signed-by=/usr/share/keyrings/githubcli-archive-keyring.gpg] https://cli.github.com/packages stable main\" | tee /etc/apt/sources.list.d/github-cli.list > /dev/null && apt-get update && apt-get install -y --no-install-recommends gh && rm -rf /var/lib/apt/lists/*\n\n# Rust\nRUN curl --proto '=https' --tlsv1.2 -sSf https://sh.rustup.rs | sh -s -- -y\nENV PATH=\"/root/.cargo/bin:${PATH}\"\nRUN rustup toolchain install nightly-2026-04-14 --profile minimal --component clippy,rustfmt\nRUN cargo install cargo-nextest --locked\nENV CARGO_INCREMENTAL=0\n\n# Bun\nRUN curl -fsSL https://bun.sh/install | bash\nENV PATH=\"/root/.bun/bin:${PATH}\"\n\nWORKDIR /root\n" + } + }, + "network": null, + "skip_clone": false + } + }, + "notifications": {}, + "interviews": { + "provider": null, + "slack": null, + "discord": null, + "teams": null + }, + "agent": { + "permissions": null, + "mcps": {} + }, + "hooks": [], + "scm": { + "provider": null, + "owner": null, + "repository": null, + "github": null + }, + "pull_request": { + "enabled": true, + "draft": false, + "auto_merge": false, + "merge_strategy": "squash" + }, + "artifacts": { + "include": [] + } + } + }, + "graph": { + "name": "ImplementPlan", + "nodes": { + "simplify_gpt": { + "id": "simplify_gpt", + "attrs": { + "model": { + "String": "gpt-5.4" + }, + "provider": { + "String": "openai" + }, + "label": { + "String": "Simplify (GPT-54)" + }, + "prompt": { + "String": "# Simplify: Code Review and Cleanup\n\nReview changes vs. origin for reuse, quality, and efficiency. Fix any issues found.\n\n## Phase 1: Identify Changes\n\nRun git diff (or git diff HEAD if there are staged changes) to see what changed. If there are no git changes, review the most recently modified files that the user mentioned or that you edited earlier in this conversation.\n\n## Phase 2: Launch Three Review Agents in Parallel\n\nUse the Agent tool to launch all three agents concurrently in a single message. Pass each agent the full diff so it has the complete context.\n\n### Agent 1: Code Reuse Review\n\nFor each change:\n\n1. Search for existing utilities and helpers that could replace newly written code. Use Grep to find similar patterns elsewhere in the codebase — common locations are utility directories, shared modules, and files adjacent to the changed ones.\n2. Flag any new function that duplicates existing functionality. Suggest the existing function to use instead.\n3. Flag any inline logic that could use an existing utility — hand-rolled string manipulation, manual path handling, custom environment checks, ad-hoc type guards, and similar patterns are common candidates.\n\nNote: This is a greenfield app, so focus on maximizing simplicity and don't worry about changing things to achieve it.\n\n### Agent 2: Code Quality Review\n\nReview the same changes for hacky patterns:\n\n1. Redundant state: state that duplicates existing state, cached values that could be derived, observers/effects that could be direct calls\n2. Parameter sprawl: adding new parameters to a function instead of generalizing or restructuring existing ones\n3. Copy-paste with slight variation: near-duplicate code blocks that should be unified with a shared abstraction\n4. Leaky abstractions: exposing internal details that should be encapsulated, or breaking existing abstraction boundaries\n5. Stringly-typed code: using raw strings where constants, enums (string unions), or branded types already exist in the codebase\n\nNote: This is a greenfield app, so be aggressive in optimizing quality.\n\n### Agent 3: Efficiency Review\n\nReview the same changes for efficiency:\n\n1. Unnecessary work: redundant computations, repeated file reads, duplicate network/API calls, N+1 patterns\n2. Missed concurrency: independent operations run sequentially when they could run in parallel\n3. Hot-path bloat: new blocking work added to startup or per-request/per-render hot paths\n4. Unnecessary existence checks: pre-checking file/resource existence before operating (TOCTOU anti-pattern) — operate directly and handle the error\n5. Memory: unbounded data structures, missing cleanup, event listener leaks\n6. Overly broad operations: reading entire files when only a portion is needed, loading all items when filtering for one\n\n## Phase 3: Fix Issues\n\nWait for all three agents to complete. Aggregate their findings and fix each issue directly. If a finding is a false positive or not worth addressing, note it and move on — do not argue with the finding, just skip it.\n\nWhen done, briefly summarize what was fixed (or confirm the code was already clean)." + } + } + }, + "exit": { + "id": "exit", + "attrs": { + "provider": { + "String": "anthropic" + }, + "label": { + "String": "Exit" + }, + "model": { + "String": "claude-opus-4-6" + }, + "shape": { + "String": "Msquare" + } + } + }, + "fixup": { + "id": "fixup", + "attrs": { + "provider": { + "String": "anthropic" + }, + "prompt": { + "String": "The verify step failed. Read the build output from context and fix all clippy lint warnings and test failures." + }, + "label": { + "String": "Fixup" + }, + "model": { + "String": "claude-opus-4-6" + }, + "max_visits": { + "Integer": 3 + } + } + }, + "preflight_lint": { + "id": "preflight_lint", + "attrs": { + "provider": { + "String": "anthropic" + }, + "shape": { + "String": "parallelogram" + }, + "label": { + "String": "Preflight Lint" + }, + "script": { + "String": "cargo +nightly-2026-04-14 clippy -q --workspace -- -D warnings 2>&1" + }, + "max_retries": { + "Integer": 0 + }, + "model": { + "String": "claude-opus-4-6" + } + } + }, + "fmt": { + "id": "fmt", + "attrs": { + "model": { + "String": "claude-opus-4-6" + }, + "label": { + "String": "Format" + }, + "provider": { + "String": "anthropic" + }, + "shape": { + "String": "parallelogram" + }, + "script": { + "String": "cargo +nightly-2026-04-14 fmt --all 2>&1" + }, + "max_retries": { + "Integer": 0 + } + } + }, + "toolchain": { + "id": "toolchain", + "attrs": { + "label": { + "String": "Toolchain" + }, + "shape": { + "String": "parallelogram" + }, + "script": { + "String": "command -v cargo >/dev/null || { curl --proto '=https' --tlsv1.2 -sSf https://sh.rustup.rs | sh -s -- -y && sudo ln -sf $HOME/.cargo/bin/* /usr/local/bin/; }; cargo --version 2>&1" + }, + "model": { + "String": "claude-opus-4-6" + }, + "provider": { + "String": "anthropic" + }, + "max_retries": { + "Integer": 0 + } + } + }, + "implement": { + "id": "implement", + "attrs": { + "provider": { + "String": "anthropic" + }, + "prompt": { + "String": "Read the plan file referenced in the goal and implement every step. Make all the code changes described in the plan. Use red/green TDD." + }, + "model": { + "String": "claude-opus-4-6" + }, + "label": { + "String": "Implement" + } + } + }, + "simplify_opus": { + "id": "simplify_opus", + "attrs": { + "provider": { + "String": "anthropic" + }, + "label": { + "String": "Simplify (Opus)" + }, + "prompt": { + "String": "# Simplify: Code Review and Cleanup\n\nReview changes vs. origin for reuse, quality, and efficiency. Fix any issues found.\n\n## Phase 1: Identify Changes\n\nRun git diff (or git diff HEAD if there are staged changes) to see what changed. If there are no git changes, review the most recently modified files that the user mentioned or that you edited earlier in this conversation.\n\n## Phase 2: Launch Three Review Agents in Parallel\n\nUse the Agent tool to launch all three agents concurrently in a single message. Pass each agent the full diff so it has the complete context.\n\n### Agent 1: Code Reuse Review\n\nFor each change:\n\n1. Search for existing utilities and helpers that could replace newly written code. Use Grep to find similar patterns elsewhere in the codebase — common locations are utility directories, shared modules, and files adjacent to the changed ones.\n2. Flag any new function that duplicates existing functionality. Suggest the existing function to use instead.\n3. Flag any inline logic that could use an existing utility — hand-rolled string manipulation, manual path handling, custom environment checks, ad-hoc type guards, and similar patterns are common candidates.\n\nNote: This is a greenfield app, so focus on maximizing simplicity and don't worry about changing things to achieve it.\n\n### Agent 2: Code Quality Review\n\nReview the same changes for hacky patterns:\n\n1. Redundant state: state that duplicates existing state, cached values that could be derived, observers/effects that could be direct calls\n2. Parameter sprawl: adding new parameters to a function instead of generalizing or restructuring existing ones\n3. Copy-paste with slight variation: near-duplicate code blocks that should be unified with a shared abstraction\n4. Leaky abstractions: exposing internal details that should be encapsulated, or breaking existing abstraction boundaries\n5. Stringly-typed code: using raw strings where constants, enums (string unions), or branded types already exist in the codebase\n\nNote: This is a greenfield app, so be aggressive in optimizing quality.\n\n### Agent 3: Efficiency Review\n\nReview the same changes for efficiency:\n\n1. Unnecessary work: redundant computations, repeated file reads, duplicate network/API calls, N+1 patterns\n2. Missed concurrency: independent operations run sequentially when they could run in parallel\n3. Hot-path bloat: new blocking work added to startup or per-request/per-render hot paths\n4. Unnecessary existence checks: pre-checking file/resource existence before operating (TOCTOU anti-pattern) — operate directly and handle the error\n5. Memory: unbounded data structures, missing cleanup, event listener leaks\n6. Overly broad operations: reading entire files when only a portion is needed, loading all items when filtering for one\n\n## Phase 3: Fix Issues\n\nWait for all three agents to complete. Aggregate their findings and fix each issue directly. If a finding is a false positive or not worth addressing, note it and move on — do not argue with the finding, just skip it.\n\nWhen done, briefly summarize what was fixed (or confirm the code was already clean)." + }, + "model": { + "String": "claude-opus-4-6" + } + } + }, + "verify": { + "id": "verify", + "attrs": { + "model": { + "String": "claude-opus-4-6" + }, + "shape": { + "String": "parallelogram" + }, + "goal_gate": { + "Boolean": true + }, + "provider": { + "String": "anthropic" + }, + "retry_target": { + "String": "fixup" + }, + "label": { + "String": "Verify" + }, + "script": { + "String": "cargo +nightly-2026-04-14 clippy -q --workspace -- -D warnings 2>&1 && cargo nextest run --cargo-quiet --workspace --status-level fail 2>&1" + } + } + }, + "fix_lints": { + "id": "fix_lints", + "attrs": { + "label": { + "String": "Fix Lints" + }, + "model": { + "String": "claude-opus-4-6" + }, + "max_visits": { + "Integer": 3 + }, + "prompt": { + "String": "The preflight lint step failed. Read the build output from context and fix all clippy lint warnings." + }, + "provider": { + "String": "anthropic" + } + } + }, + "preflight_compile": { + "id": "preflight_compile", + "attrs": { + "model": { + "String": "claude-opus-4-6" + }, + "provider": { + "String": "anthropic" + }, + "label": { + "String": "Preflight Compile" + }, + "max_retries": { + "Integer": 0 + }, + "script": { + "String": "cargo check -q --workspace 2>&1" + }, + "shape": { + "String": "parallelogram" + } + } + }, + "start": { + "id": "start", + "attrs": { + "model": { + "String": "claude-opus-4-6" + }, + "label": { + "String": "Start" + }, + "shape": { + "String": "Mdiamond" + }, + "provider": { + "String": "anthropic" + } + } + } + }, + "edges": [ + { + "from": "start", + "to": "toolchain", + "attrs": {} + }, + { + "from": "toolchain", + "to": "preflight_compile", + "attrs": { + "condition": { + "String": "outcome=succeeded" + } + } + }, + { + "from": "toolchain", + "to": "exit", + "attrs": {} + }, + { + "from": "preflight_compile", + "to": "preflight_lint", + "attrs": { + "condition": { + "String": "outcome=succeeded" + } + } + }, + { + "from": "preflight_compile", + "to": "exit", + "attrs": {} + }, + { + "from": "preflight_lint", + "to": "implement", + "attrs": { + "condition": { + "String": "outcome=succeeded" + } + } + }, + { + "from": "preflight_lint", + "to": "fix_lints", + "attrs": {} + }, + { + "from": "fix_lints", + "to": "preflight_lint", + "attrs": {} + }, + { + "from": "implement", + "to": "simplify_opus", + "attrs": {} + }, + { + "from": "simplify_opus", + "to": "simplify_gpt", + "attrs": {} + }, + { + "from": "simplify_gpt", + "to": "verify", + "attrs": {} + }, + { + "from": "verify", + "to": "fmt", + "attrs": { + "condition": { + "String": "outcome=succeeded" + } + } + }, + { + "from": "verify", + "to": "fixup", + "attrs": {} + }, + { + "from": "fixup", + "to": "verify", + "attrs": {} + }, + { + "from": "fmt", + "to": "exit", + "attrs": {} + } + ], + "attrs": { + "model_stylesheet": { + "String": "\n * { model: claude-opus-4-6; }\n " + }, + "goal": { + "String": "# Patch silent-degrade sites in fabro-workflow / fabro-sandbox\n\n> **Note on filename:** `make-a-plan-to-abstract-hamming.md` is the harness-prescribed\n> path for this plan and does not reflect the content. Future readers should treat the\n> file body as authoritative.\n\n## Context\n\nThe user noticed that when `worktree_mode = always` is set and the cwd is not a Git repo,\n`resolve_worktree_base_sha` (`lib/crates/fabro-workflow/src/pipeline/initialize.rs:74-76`)\nreturns `Ok(None)` on `\"not a git repository\"`, the caller's `else` branch (line 506-508)\nwraps the bare sandbox, and `options.run_options.git` is reset to `None` — with no\n`Emitter::notice` and no `tracing::warn!`. The user asked for X, got not-X, and was told nothing.\n\nA short audit surfaced several more sites with the same anti-pattern. Goal: every\n*genuine* silent-degrade site emits a stable signal that reaches `fabro logs` / SSE / retro.\nSandbox-internal sites without an Emitter are tracing-only, with the event-stream\nfollow-up tracked separately. Behavior is unchanged — the fallback still happens; it just\nannounces itself.\n\n## Revised fix pattern\n\nThis pattern was rewritten in response to reviewer feedback (Event::trace already logs\nwarn-level notices; failover already has a typed event; not every `Ok(None)` is a degradation).\n\n1. **Default:** at the fallback site, call\n `emitter.notice(RunNoticeLevel::Warn, \"\", \"\")`.\n This routes through `Event::RunNotice` whose `Event::trace()` arm\n (`event/events.rs:696-710`) already emits a `warn!(code, message, \"Run notice\")` — so\n a notice alone covers both `server.log` and the run feed.\n2. **Add a separate `tracing::warn!` only when** there are structured diagnostic fields\n absent from the notice trace (`error = %err`, `refspec`, `provider`, `model`,\n `worktree_mode`, etc.). Plain restatements of the notice message do not justify a\n second log line.\n3. **Use a typed event when one already exists** (e.g. `Event::Failover` for LLM provider\n failover, `Event::RetroFailed` for retro problems). Don't introduce a parallel\n `RunNotice` for behavior already represented by a typed event.\n4. **Gate the warning on user intent.** If the fallback path is the *expected* outcome\n for the user's configuration (e.g. local in-place, no-clone sandbox), do not warn.\n Only warn when the user implicitly or explicitly asked for the non-fallback path.\n5. **Stable code naming:** `_` lowercase snake. Codes are a contract;\n pick once.\n6. **Severity:** `RunNoticeLevel::Warn` for \"user asked for X, didn't get X.\"\n `RunNoticeLevel::Info` for benign post-conditions like sandbox preserved. LLM\n failover uses `Event::Failover` (already typed), not `RunNotice`.\n\nReference: `dirty_worktree` notice at `pipeline/initialize.rs:156-161`; `git_diff_failed`\nat `lifecycle/git.rs:306-310`; existing `Event::Failover` emit at `handler/llm/api.rs:510-520`.\n\n## Per-site patches\n\n### 1. Worktree skipped on non-git cwd *(the original)*\n\n`lib/crates/fabro-workflow/src/pipeline/initialize.rs:506-520`\n\nIn the `else` branch at line 506 (where `resolve_worktree_base_sha` returned `Ok(None)`):\n\n- `tracing::warn!(worktree_mode = ?options.worktree_mode, \"worktree requested but cwd is not a git repository; running without a worktree\")`\n — keeps the structured `worktree_mode` field that's not in the notice payload.\n- `options.emitter.notice(RunNoticeLevel::Warn, \"worktree_skipped_no_git\", \"Worktree mode requested but no Git repository was found; running without a worktree.\")`\n\n### 2. Sandbox `setup_git` returned `Ok(None)` **when git was expected**\n\n`lib/crates/fabro-workflow/src/pipeline/initialize.rs:602-626`\n\nThe `Ok(None) => {}` arm covers two real cases:\n\na. The sandbox had an origin / git was expected → `Ok(None)` is a degradation.\nb. The sandbox is clone-less / no-origin → `Ok(None)` is the normal outcome.\n\nThe surrounding code already discriminates: line 596 only calls `ensure_git_available`\nwhen `sandbox.origin_url().is_some()`. Reuse that signal:\n\n```rust\nOk(None) => {\n if sandbox.origin_url().is_some() {\n options.emitter.notice(\n RunNoticeLevel::Warn,\n \"sandbox_git_unavailable\",\n \"Sandbox could not set up Git despite a configured origin; running without checkpointing or PR support.\",\n );\n }\n}\n```\n\nNo additional `tracing::warn!` — the notice trace covers it; no extra structured fields\nworth emitting.\n\n### 3. Checkpoint push failure\n\n`lib/crates/fabro-workflow/src/lifecycle/git.rs:277-289`\n\nExisting `tracing::warn!(refspec, error, ...)` at line 280-284 carries structured\nfields and stays. Add a notice in the same `Err(err)` arm, before `false`:\n\n```rust\nself.emitter.emit(&Event::RunNotice {\n level: RunNoticeLevel::Warn,\n code: \"git_push_failed\".to_string(),\n message: format!(\"Failed to push run branch {branch}: {err}\"),\n});\n```\n\n(Matches the local style at `lifecycle/git.rs:306-310` which already builds `Event::RunNotice`\ndirectly because `self.emitter` is a `&Emitter`.)\n\n### 4. Parallel base checkpoint failure\n\n`lib/crates/fabro-workflow/src/handler/parallel.rs:200-209`\n\nExisting `tracing::warn!(error = %e, ...)` at line 206 stays. Add a notice between the\n`warn!` and `None`:\n\n```rust\nservices.run.emitter.notice(\n RunNoticeLevel::Warn,\n \"parallel_base_checkpoint_failed\",\n format!(\"Could not checkpoint base state before parallel branches: {e}\"),\n);\n```\n\nUpdate the file-top imports to include `RunNoticeLevel` from `crate::event`\n(see `pipeline/initialize.rs` for the same import shape).\n\n### 5. GitHub token mint failure\n\n`lib/crates/fabro-workflow/src/pipeline/initialize.rs:238-247`\n\nAlready emits `notice(\"github_token_failed\", …)`. The notice message embeds `{e}` as a\nplain string, but the structured `error` field is absent from the notice trace\n(`events.rs:704-706` only carries `code` and `message`). Per the revised rule\n(structured fields not in the notice trace justify a separate `tracing::warn!`), add\na structured warn line immediately before the existing `emitter.notice(...)`:\n\n```rust\ntracing::warn!(error = %e, \"Failed to mint GitHub token\");\n```\n\n### 6. LLM provider failover surfaced through `one_shot` path\n\n`lib/crates/fabro-workflow/src/handler/llm/api.rs:283-402`\n\nThe existing `chat()` path at line 510-520 emits `Event::Failover` per attempt with\n`stage`, `from_provider/model`, `to_provider/model`, `error`. The `one_shot` path\n(line 283-402) does not, because:\n\n- `AgentApiBackend` has no `emitter` field (struct definition at 117-125).\n- The `CodergenBackend::one_shot` trait method has no emitter parameter (signature at 283-288).\n\nApproach: plumb `&Arc` into the trait, then emit the existing `Event::Failover`\n(no new code; reuses what `chat()` already does).\n\nSteps:\n\n1. Change `CodergenBackend::one_shot` signature in the trait at `handler/agent.rs:36-…`\n (default method at `handler/agent.rs:50`) to add `emitter: &Arc` and a\n `&StageScope` parameter (mirroring `chat()`'s emit at `api.rs:510-520`). Then update\n every implementor — find them with:\n ```\n rg -n \"async fn one_shot\\(\" lib/crates/fabro-workflow\n ```\n At time of writing this finds:\n - Trait default — `handler/agent.rs:50`\n - `AgentApiBackend::one_shot` — `handler/llm/api.rs:283`\n - `BackendRouter::one_shot` — `handler/llm/cli.rs:808`\n (`AgentCliBackend` uses the trait default, not its own impl — leave as-is.)\n - Test stubs in `handler/prompt.rs:277, 337, 394`\n - Integration test stub in `tests/it/integration.rs:6219`\n Re-run the rg before editing in case more impls have been added.\n2. Caller `handler/prompt.rs:107-109` passes `&services.run.emitter` and the prompt's\n `stage_scope`.\n3. Inside the `one_shot` failover loop in `api.rs:349-399`, emit `Event::Failover` per\n attempt, exactly as the `chat()` loop at line 510-520 does. Each iteration of the\n `for target in fallback_chain` loop emits one event before attempting the call.\n4. **Delete the existing `tracing::warn!` at `api.rs:361-369`.** `Event::Failover::trace()`\n at `events.rs:1083-1090` already emits `warn!(stage, from_provider, from_model,\n to_provider, to_model, error, ...)` — identical fields. Keeping both produces a\n duplicate WARN per attempt. (The `chat()` path correctly does not have a\n parallel `tracing::warn!`; this is making `one_shot` consistent with it.) No new\n `RunNotice` code; `agent.failover` is the canonical event name (`event/names.rs:113`).\n\nTests: extend whatever exercises the one-shot failover branch to assert an\n`agent.failover` event is recorded.\n\n### 7. Sandbox stdout/stderr drain failure (tracing-only, scope-bounded)\n\n`lib/crates/fabro-sandbox/src/local.rs:282-295`\n\n`fabro-sandbox` has no `Emitter` access at this depth, and the event-stream surface\nis `SandboxEventCallback`. Plumbing a new `SandboxEvent::PipeReadFailed` through\n`fabro-types` + `event_name` + `EventBody` is intentionally out of scope for this batch\n(decided with the user). Do the tracing-only fix here:\n\n```rust\nlet stdout_task = tokio::spawn(async move {\n let mut buf = String::new();\n if let Some(ref mut r) = stdout_pipe {\n if let Err(err) = r.read_to_string(&mut buf).await {\n tracing::warn!(error = %err, stream = \"stdout\", \"Failed to drain child stdout\");\n }\n }\n buf\n});\n// same shape for stderr_task\n```\n\nGoal-narrowing acknowledgment: this site is fixed in `server.log` only — event-stream\nvisibility is a follow-up.\n\n## Out of scope (verified — adequately surfaced today)\n\n- **MCP server failed (`fabro-agent/src/session.rs:253-265`)** — emits\n `AgentEvent::McpServerFailed` *and* `tracing::warn!`. Adequate.\n- **Retro failures (`pipeline/retro.rs:19, 28, 40`)** — emits `Event::RetroFailed` *and*\n `tracing::warn!`. Adequate.\n- **`pipeline/finalize.rs:72` (`state_result.ok()`)** / `pipeline/pull_request.rs:205`\n / `pipeline/initialize.rs:340-346` — internal projection / explicit user config; not\n silent-degrade.\n\n## Follow-ups (deliberately deferred)\n\n- Plumb `SandboxEvent::PipeReadFailed` through the existing `SandboxEventCallback`\n (variant + `event_name` + `EventBody` mapping per `docs/internal/events-strategy.md`)\n so site 7's truncation reaches the run feed.\n\n## Files to modify\n\n1. `lib/crates/fabro-workflow/src/pipeline/initialize.rs` (sites 1, 2, 5)\n2. `lib/crates/fabro-workflow/src/lifecycle/git.rs` (site 3)\n3. `lib/crates/fabro-workflow/src/handler/parallel.rs` (site 4 — also import `RunNoticeLevel`)\n4. `lib/crates/fabro-workflow/src/handler/agent.rs` (site 6 — `CodergenBackend` trait + default `one_shot` signature)\n5. `lib/crates/fabro-workflow/src/handler/llm/api.rs` (site 6 — `AgentApiBackend::one_shot` impl + `Event::Failover` emit + delete duplicate `tracing::warn!`)\n6. `lib/crates/fabro-workflow/src/handler/llm/cli.rs` (site 6 — `BackendRouter::one_shot` forward params)\n7. `lib/crates/fabro-workflow/src/handler/prompt.rs` (site 6 — caller plumbing + test stubs at 277/337/394)\n8. `lib/crates/fabro-workflow/tests/it/integration.rs` (site 6 — test stub at 6219)\n9. `lib/crates/fabro-sandbox/src/local.rs` (site 7 — tracing only)\n\nRe-run `rg -n \"async fn one_shot\\(\" lib/crates/fabro-workflow` before editing site 6 to\ncatch any new `one_shot` impls added since this plan.\n\n## Stable codes added\n\n- `worktree_skipped_no_git` — Warn (site 1)\n- `sandbox_git_unavailable` — Warn (site 2, gated on `origin_url.is_some()`)\n- `git_push_failed` — Warn (site 3)\n- `parallel_base_checkpoint_failed` — Warn (site 4)\n\n(Site 5 reuses the existing `github_token_failed` notice; only adds a structured\n`tracing::warn!`. Site 6 reuses the existing `agent.failover` event; no new stable\nnotice code or `RunNotice` code is introduced.)\n\n## Verification\n\n1. Build: `cargo build --workspace`\n2. Unit tests: `cargo nextest run -p fabro-workflow -p fabro-sandbox`\n3. New unit tests, one per behavioral change:\n - **Worktree skip** — extend the existing\n `resolve_worktree_plan_uses_local_worktree_without_pre_run_git_context`\n (`pipeline/initialize.rs:957`) into an `init`-level test using a non-git scratch\n dir; assert a `RunNotice` with code `worktree_skipped_no_git` is emitted.\n - **Sandbox git unavailable (gated)** — two cases: (a) sandbox with `origin_url =\n Some(...)` returning `Ok(None)` from `setup_git` emits `sandbox_git_unavailable`;\n (b) sandbox with `origin_url = None` returning `Ok(None)` emits *no* notice.\n Confirms the gating works.\n - **Push failure** — extend lifecycle/git tests to fake a failing `git_push_ref`\n and assert `git_push_failed` notice + the push_results entry.\n - **Parallel base checkpoint failure** — `handler/parallel.rs` calls the free\n function `checked_git_checkpoint(...)` (line 188) on the sandbox; there is no\n creator interface to fake. Test by constructing the parallel handler with a\n scripted sandbox where the git probe succeeds (so `git_state` is `Some(_)`) and\n the actual `git commit` / checkpoint command fails, populate `services.git_state`,\n and assert the `parallel_base_checkpoint_failed` notice fires. Pattern off\n existing parallel-handler tests in the same file.\n - **One-shot LLM failover** — `AgentApiBackend::one_shot` constructs its\n `Client::from_source(self.source.as_ref())` internally (`api.rs:289`), so the\n existing seam is the `Arc`. Test approach: provide a stub\n `CredentialSource` returning credentials that point at an `httpmock` server,\n program mock A to return a failover-eligible status (e.g. 529 / overloaded for\n Anthropic), program mock B to return success, and assert exactly one\n `Event::Failover` was emitted with the right `from_*` / `to_*` properties.\n Mirror an existing `httpmock`-based test from `fabro-llm` integration tests if\n one already covers `failover_eligible` mapping.\n - **GitHub token mint warn** — site 5 only adds a `tracing::warn!`; the user-facing\n notice is unchanged. If existing tests cover the `Err(e)` arm of `mint_token`\n (likely in `pipeline::initialize::tests` or `fabro-github` tests), assert the\n warn line via `tracing-test` / `tracing_subscriber::fmt::TestWriter`. Otherwise,\n this is covered by code inspection plus the manual smoke run; do not add a new\n test just for the warn line.\n - **Sandbox pipe drain** — a closed pipe reads as `Ok(0)` (EOF), not `Err`, so a\n direct unit test of the inline closure is awkward. Two acceptable approaches:\n (a) extract the read loop into a `drain_pipe(reader: &mut R, stream: &str)`\n helper and unit-test it with a custom `AsyncRead` impl whose `poll_read` returns\n `Poll::Ready(Err(io::Error::other(\"simulated\")))`; or (b) keep the inline\n `if let Err(...) = ...` and verify only by manual smoke (run a command that\n terminates abnormally and confirm the WARN line in `server.log`). Prefer (a) if\n the small refactor is cheap; otherwise (b) is fine — record the choice in the\n PR description.\n4. **Manual smoke test (concrete)** for the worktree case end-to-end. Build the\n workflow inline so the test does not depend on repo workflows or LLM credentials —\n only the sandbox needs to start:\n ```bash\n tmp=$(mktemp -d)\n mkdir -p \"$tmp/.fabro/workflows/baresmoke\"\n cat > \"$tmp/.fabro/workflows/baresmoke/workflow.toml\" <<'EOF'\n _version = 1\n\n [workflow]\n graph = \"workflow.fabro\"\n EOF\n cat > \"$tmp/.fabro/workflows/baresmoke/workflow.fabro\" <<'EOF'\n digraph BareSmoke {\n graph [goal=\"non-git smoke test for worktree_skipped_no_git\"]\n rankdir=LR\n\n start [shape=Mdiamond, label=\"Start\"]\n exit [shape=Msquare, label=\"Exit\"]\n\n hello [label=\"Hello\", shape=parallelogram, script=\"echo hello\"]\n\n start -> hello -> exit\n }\n EOF\n cd \"$tmp\"\n fabro run baresmoke --no-retro --auto-approve\n ```\n - `fabro run ` resolves `/.fabro/workflows//workflow.toml`, so the\n workflow must land at that exact path.\n - The `script` node uses the same shape as the existing `smoke` workflow\n (`.fabro/workflows/smoke/workflow.fabro`) — `parallelogram` + `script=\"...\"` —\n which runs purely in the sandbox shell with no LLM calls. `goal_gate=true` is\n intentionally omitted: the real `smoke` workflow pairs it with `retry_target=exit`\n in graph attrs, and using `goal_gate` without `retry_target` trips the\n `goal_gate_has_retry` validation warning. This smoke only needs to reach\n initialization and exec one command, so the gate isn't needed.\n - `mktemp -d` is intentionally non-git, so this exercises the worktree-skipped path\n even with `worktree_mode = always` (the local-sandbox default).\n - Confirm `/logs/server.log` has `code=\"worktree_skipped_no_git\" ... \"Run notice\"`.\n - Confirm `fabro logs ` (or the SSE/UI run feed) shows the notice.\n5. Format and lint:\n - `cargo +nightly-2026-04-14 fmt --all`\n - `cargo +nightly-2026-04-14 clippy --workspace --all-targets -- -D warnings`\n\n## Reviewer feedback acknowledgments (round 1 P1/P2 incorporated)\n\n- LLM failover patch redesigned around the existing `Event::Failover` and a\n trait-signature change to `CodergenBackend::one_shot`; `self.emitter` was a fiction.\n- \"Always emit both notice and `tracing::warn!`\" rule replaced with a structured-fields\n predicate; relies on `Event::trace()` for the warn-level log of every notice.\n- `setup_git` `Ok(None)` warning is now gated on `sandbox.origin_url().is_some()`.\n- LLM failover severity contradiction removed; `RunNotice`-vs-`Event::Failover`\n distinction now explicit.\n- Site 7 reframed as deliberate scope narrowing with a follow-up; goal text updated.\n- Test plan now covers sites 4, 6, and 7.\n- Manual smoke command made self-contained with a copied fixture workflow.\n\n## Reviewer feedback acknowledgments (round 2)\n\n- **Failover duplicate WARN**: `Event::Failover::trace()` (`events.rs:1083-1090`) already\n emits `warn!` with the same fields as the existing `tracing::warn!` at `api.rs:361-369`.\n Plan now explicitly deletes that line as part of site 6.\n- **Trait file**: `handler/agent.rs` (where `CodergenBackend` and the default `one_shot`\n live) added to files-to-modify.\n- **Implementor list**: replaced the hand-written list with an `rg` recipe; the only\n real impls today are the trait default, `AgentApiBackend`, and `BackendRouter`. Test\n stubs are now called out separately. `AgentCliBackend` does not have its own `one_shot`.\n- **Parallel emitter handle**: now `services.run.emitter`, with the `RunNoticeLevel`\n import call-out.\n- **Pipe-drain test**: closed pipes read as EOF; replaced the \"pre-closed reader\"\n shorthand with a real choice between (a) extract a `drain_pipe` helper testable with\n a custom `AsyncRead`, or (b) drop the automated test and rely on manual smoke.\n\n## Reviewer feedback acknowledgments (round 3)\n\n- **Smoke workflow doesn't exist**: this repo's workflows are `gh-triage`, `hello`,\n `implement-issue`, `implement-plan`, `smoke` — no `repl`, and `hello` requires LLM\n credentials. Smoke recipe rewritten to build a minimal command-only workflow inline\n using the same `parallelogram` + `script=\"...\"` shape used by `.fabro/workflows/smoke/`,\n so it runs purely in the sandbox shell without LLM creds.\n- **GitHub token rule contradiction**: an embedded `{e}` in a notice message is not a\n structured field. Site 5 reinstated with `tracing::warn!(error = %e, ...)` to honor\n the structured-fields rule. Removed the contradicting \"out of scope\" entry.\n- **Failover test injection**: `AgentApiBackend::one_shot` constructs\n `Client::from_source(self.source.as_ref())` internally, so the seam is the existing\n `Arc`. Test recipe spelled out with stub `CredentialSource` +\n `httpmock` returning failover-eligible from A and success from B.\n- **Parallel test wording**: there is no checkpoint-creator interface — the handler\n calls free function `checked_git_checkpoint(...)`. Test recipe rewritten to drive\n a scripted sandbox where the git probe succeeds and the checkpoint command fails,\n with `services.git_state = Some(_)`.\n\n## Reviewer feedback acknowledgments (round 4)\n\n- **Files-to-modify**: site 5 also lives in `pipeline/initialize.rs`; entry corrected\n to `(sites 1, 2, 5)`.\n- **Site 5 verification**: added an explicit verification entry stating the new\n `tracing::warn!` is covered by code inspection plus the manual smoke run, with an\n optional `tracing-test` assertion if existing token-mint test scaffolding exists.\n- **Smoke `goal_gate` validation warning**: `goal_gate=true` removed from the inline\n `hello` node, with an explanation note. Real `smoke` works because it pairs\n `goal_gate` with `retry_target=exit` in graph attrs; we don't need the gate at all\n for this verification.\n- **\"No new code\" wording**: clarified — site 6 introduces trait plumbing and an\n `Event::Failover` emit, but no new stable `RunNotice` code (`agent.failover` was\n already canonical).\n" + }, + "rankdir": { + "String": "LR" + } + } + }, + "workflow_slug": "implement-plan", + "source_directory": "/Users/bhelmkamp/p/fabro-sh/fabro", + "provenance": { + "server": { + "version": "0.221.0-nightly.1" + }, + "client": { + "user_agent": "fabro-cli/0.221.0-nightly.1", + "name": "fabro-cli", + "version": "0.221.0-nightly.1" + }, + "subject": { + "kind": "user", + "identity": { + "issuer": "https://github.com", + "subject": "19" + }, + "login": "brynary", + "auth_method": "github" + } + }, + "manifest_blob": "023a3052b05955b9e6f7f593d4968c6d796be3b56b29c53e15e382fb4c9d5a61", + "definition_blob": "1c91f85075f82e2131d90f2aa492d4af0ed9898678b479796d2176dd3790af8f", + "git": { + "origin_url": "https://github.com/fabro-sh/fabro", + "branch": "main", + "sha": "253af1150818e93a49eade3c262440295256d278", + "dirty": "clean", + "push_outcome": { + "type": "not_attempted" + } + }, + "in_place": false + }, + "graph_source": "digraph ImplementPlan {\n graph [\n goal=\"Implement and simplify\",\n model_stylesheet=\"\n * { model: claude-opus-4-6; }\n \"\n ]\n rankdir=LR\n\n start [shape=Mdiamond, label=\"Start\"]\n exit [shape=Msquare, label=\"Exit\"]\n\n toolchain [label=\"Toolchain\", shape=parallelogram, script=\"command -v cargo >/dev/null || { curl --proto '=https' --tlsv1.2 -sSf https://sh.rustup.rs | sh -s -- -y && sudo ln -sf $HOME/.cargo/bin/* /usr/local/bin/; }; cargo --version 2>&1\", max_retries=0]\n preflight_compile [label=\"Preflight Compile\", shape=parallelogram, script=\"cargo check -q --workspace 2>&1\", max_retries=0]\n preflight_lint [label=\"Preflight Lint\", shape=parallelogram, script=\"cargo +nightly-2026-04-14 clippy -q --workspace -- -D warnings 2>&1\", max_retries=0]\n fix_lints [label=\"Fix Lints\", prompt=\"The preflight lint step failed. Read the build output from context and fix all clippy lint warnings.\", max_visits=3]\n implement [label=\"Implement\", prompt=\"Read the plan file referenced in the goal and implement every step. Make all the code changes described in the plan. Use red/green TDD.\"]\n simplify_opus [label=\"Simplify (Opus)\", prompt=\"@prompts/simplify.md\"]\n simplify_gpt [label=\"Simplify (GPT-54)\", prompt=\"@prompts/simplify.md\", model=\"gpt-54\"]\n verify [label=\"Verify\", shape=parallelogram, script=\"cargo +nightly-2026-04-14 clippy -q --workspace -- -D warnings 2>&1 && cargo nextest run --cargo-quiet --workspace --status-level fail 2>&1\", goal_gate=true, retry_target=\"fixup\"]\n fixup [label=\"Fixup\", prompt=\"The verify step failed. Read the build output from context and fix all clippy lint warnings and test failures.\", max_visits=3]\n fmt [label=\"Format\", shape=parallelogram, script=\"cargo +nightly-2026-04-14 fmt --all 2>&1\", max_retries=0]\n\n start -> toolchain\n toolchain -> preflight_compile [condition=\"outcome=succeeded\"]\n toolchain -> exit\n preflight_compile -> preflight_lint [condition=\"outcome=succeeded\"]\n preflight_compile -> exit\n preflight_lint -> implement [condition=\"outcome=succeeded\"]\n preflight_lint -> fix_lints\n fix_lints -> preflight_lint\n implement -> simplify_opus -> simplify_gpt -> verify\n verify -> fmt [condition=\"outcome=succeeded\"]\n verify -> fixup\n fixup -> verify\n fmt -> exit\n}\n", + "start": null, + "status": { + "kind": "starting" + }, + "status_updated_at": "2026-05-04T03:08:28.283299Z", + "pending_control": null, + "checkpoint": null, + "checkpoints": [], + "conclusion": null, + "retro": null, + "retro_prompt": null, + "retro_response": null, + "sandbox": { + "provider": "daytona", + "working_directory": "/home/daytona/workspace", + "identifier": "fabro-01KQRFAT7326ADECC8AAWBXK6M", + "repo_cloned": true, + "clone_origin_url": "https://github.com/fabro-sh/fabro", + "clone_branch": "main" + }, + "final_patch": null, + "pull_request": null, + "superseded_by": null, + "pending_interviews": {}, + "stages": {} +} \ No newline at end of file