From 95edcfa6539db3eda689f585fc5eb1e8824b8452 Mon Sep 17 00:00:00 2001 From: Fabro Date: Sat, 11 Jul 2026 20:45:45 +0000 Subject: [PATCH] =?UTF-8?q?checkpoint=20=E2=9A=92=EF=B8=8F=20Generated=20w?= =?UTF-8?q?ith=20[Fabro](https://fabro.sh)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- run.json | 354 ++++++++++++++++-- stages/004-preflight_lint@1/output.log | 1 + .../004-preflight_lint@1/script_timing.json | 8 + stages/004-preflight_lint@1/status.json | 6 + stages/005-implement@1/prompt.md | 250 +++++++++++++ stages/005-implement@1/provider_used.json | 6 + 6 files changed, 602 insertions(+), 23 deletions(-) create mode 100644 stages/004-preflight_lint@1/output.log create mode 100644 stages/004-preflight_lint@1/script_timing.json create mode 100644 stages/004-preflight_lint@1/status.json create mode 100644 stages/005-implement@1/prompt.md create mode 100644 stages/005-implement@1/provider_used.json diff --git a/run.json b/run.json index 78271bbbf..2c4f609e1 100644 --- a/run.json +++ b/run.json @@ -505,7 +505,7 @@ "kind": "running" }, "status_updated_at": "2026-07-11T20:41:55.540863608Z", - "last_event_at": "2026-07-11T20:43:50.604255351Z", + "last_event_at": "2026-07-11T20:45:45.631329167Z", "pending_control": null, "checkpoints": [ { @@ -690,9 +690,9 @@ } }, { - "seq": 0, + "seq": 49, "checkpoint": { - "timestamp": "2026-07-11T20:45:42.139295461Z", + "timestamp": "2026-07-11T20:45:45.297541987Z", "current_node": "preflight_lint", "completed_nodes": [ "start", @@ -702,26 +702,26 @@ ], "node_retries": {}, "context_values": { - "internal.run_id": "01KX9EM9QF65A43PB064TF7TWY", - "failure_class": "", - "command.output": "blob://sha256/12ae32cb1ec02d01eda3581b127c1fee3b0dc53572ed6baf239721a03d82e126", - "thread.toolchain.current_node": "preflight_compile", - "graph.goal": "# PR 4 — Resolve hook interpolation at the run boundary (and enable secrets in hooks)\n\n**Self-contained implementation plan.** Everything needed to implement this is\nin this file plus the repository. Independent — no preconditions; can land\nanytime.\n\n**Redaction context (fixed; do not build on it):** run-output redaction in\nthis codebase is **content-based only** — entropy + credential-pattern\ndetection (`fabro_redact::redact_string` / `redact_json_value`), applied\nwhere events are serialized and where exec-output tails are captured. There\nis no per-run exact-value secret registry (a registration approach was\nconsidered and rejected). Secrets resolved for hooks by this PR get exactly\nthe coverage every other boundary-resolved secret (MCP env, run env, prepare\nsteps) already has: credential-shaped values are redacted from event and\ntail surfaces if echoed; a low-entropy secret value is not — an accepted,\ndocumented trade. Do not add any registration or exact-match machinery.\n\n> **Token notation.** Interpolation tokens are written in this file without\n> their enclosing double curly braces, so the file is safe to pass directly as\n> a workflow goal (the goal templater would otherwise try to expand them).\n> Read `secrets.NAME`, `env.NAME` as the double-curly-brace token form used in\n> the codebase, and write the real double-brace syntax in the code, tests, and\n> docs you produce.\n\n## Context and goal\n\nHooks are user-defined callbacks on workflow lifecycle events (`run_start`,\n`stage_start`, `pre_tool_use`, `sandbox_ready`, …) that can observe or gate a\nrun (decisions: proceed / block / skip / override). Four types: **command**\n(shell, runs in the sandbox by default or host-side with `sandbox = false`),\n**http** (POST from the worker), **prompt** and **agent** (LLM evaluation in\nthe worker). Their configurable string fields — `command`, `url`, header\nvalues, `prompt`, `model` — are typed `InterpString` and may carry `env.NAME`\ntokens.\n\nEvery other secret/env-consuming subsystem (MCP transport env, run-environment\nenv, prepare steps, docker config) resolves its InterpStrings **once, at the\nrun boundary** in `RunSession::new`, through one shared lookup closure. Hooks\nare the single exception: the hook **executor** resolves tokens **at fire\ntime**, and only the `env` namespace is wired there — a `secrets.NAME` token\nin a hook currently fails closed with \"unavailable namespace\". This fire-time\nresolution is a fossil, not a decision: it dates from the original hooks\nimplementation, before the boundary-resolution pattern existed, and was\ncarried forward unexamined. A previous attempt to add secrets support built a\nparallel fire-time secrets-resolution and redaction-registration subsystem\ninside the hooks crate to accommodate it; that PR was closed, and the accepted\ndirection is to remove the root special case instead.\n\n**Goal:** resolve all hook InterpStrings once at the run boundary through the\nshared closures, hand the hooks subsystem fully-resolved strings, and delete\nthe executor's resolution layer. Consequences, all intended:\n\n- `secrets.NAME` becomes usable in hook `command`, `url`, and `prompt`/`model`\n — the user-facing capability — with the same content-based redaction\n coverage every other boundary-resolved secret already has (see the\n redaction context above).\n- The invariant \"all env/secrets resolution happens at the run boundary\" holds\n with **zero exceptions**, so no future secrets/redaction work needs a hooks\n special case.\n- The hooks crate never learns about vaults or secrets at all.\n\nExplicitly out of scope / preserved:\n\n- The fire-time **context** mechanism is untouched: hooks receive per-firing\n data (event, node id, tool name, …) out-of-band via the `FABRO_HOOK_CONTEXT`\n env var, not via interpolation. There is no context namespace in\n InterpString; nothing interpolates per-firing data today, so boundary\n resolution loses no capability that exists.\n- Matcher semantics, blocking/decision merging, sandbox-vs-host dispatch,\n timeouts: unchanged.\n\n## Verified current state (as of main `9daca83b3`, 2026-07-09 — re-verify before starting; line numbers are anchors, not gospel)\n\n`lib/crates/fabro-hooks/src/executor.rs`:\n\n- `resolve_interp(value, env)` (~`:76`) — resolves an `InterpString` against\n process env at fire time; doc comment says only `env` is wired and other\n namespaces fail closed. Used for `command` and `url`.\n- `resolve_prompt_and_model` (~`:186`) — same, for prompt/agent hooks.\n- `resolve_header(value, allowed_env_vars, env)` (~`:132`) — header\n values additionally gate `env.NAME` behind the hook's `allowed_env_vars`\n list (`HeaderResolveError::NotAllowed`); resolution errors block the hook.\n- `safe_url_source_for_log` — logs the **unresolved** URL source (never the\n resolved URL) so env-sourced URL material is not logged.\n- Fail-closed dispositions today: a resolution error in `command` blocks; in\n `url`/headers/prompt/model the hook logs an error and does not fire (http\n headers produce a block); transport-level failures stay fail-open.\n\n`lib/crates/fabro-types/src/settings/run.rs`:\n\n- `HookDefinition { name, event, command: Option, hook_type,\n matcher, blocking, timeout_ms, sandbox }` (~`:2173`); `HookType::{Command,\n Http { url, headers, allowed_env_vars, tls }, Prompt { prompt, model },\n Agent { prompt, model } }` (~`:2149`). `vars.NAME` tokens in all these\n fields are already substituted at run creation (`substitute_variables`\n walks hooks), so only `env`/`secrets` tokens remain by boundary time.\n\n`lib/crates/fabro-workflow/src/operations/start.rs`:\n\n- The boundary pattern to mirror: `runtime_mcp_server(server, process_env_var,\n secret_lookup)` (~`:718`) and `runtime_setup_commands(...)` (~`:745`) —\n config type in, resolved runtime type out, hard error on missing names.\n- Hooks are currently passed through to the runner **unresolved** (find the\n hook wiring where `resolved.hooks` reaches `HookSettings`).\n\nEnvironment-timing note (verified): no `env::set_var` in production worker\npaths, so worker process env is identical at boundary time and fire time —\nresolving earlier does not change resolved values. Command hooks with\n`sandbox = true` already resolve against **worker** env and ship the resolved\nstring into the sandbox; that stays true, just earlier.\n\n## Design\n\n1. **Runtime hook type.** Add a resolved runtime form (e.g.\n `RuntimeHookDefinition`, plain `String` fields, mirroring\n `HookDefinition`/`HookType` shape) plus a boundary constructor\n `runtime_hooks(hooks, process_env_var, secret_lookup) -> Result>`\n in `operations/start.rs` alongside `runtime_mcp_server` /\n `runtime_setup_commands`. The config/wire type `HookDefinition` is\n unchanged — no API or manifest change. For http hooks, carry the\n **unresolved url source string** on the runtime type as well, for safe\n logging (preserves the `safe_url_source_for_log` guarantee).\n2. **Header policy enforced at the boundary, unchanged in substance:**\n - a `secrets.NAME` token in a header value is rejected **before any vault\n lookup**, with the existing guidance shape: secrets are not allowed in\n HTTP hook headers; use secret interpolation in a hook command, prompt,\n or url instead;\n - `env.NAME` in header values stays gated by `allowed_env_vars`\n (non-allowlisted name → error naming the variable; allowlisted-but-unset\n → missing-variable error);\n - `command`/`url`/`prompt`/`model` resolve `env` + `secrets` with hard\n errors on missing names.\n Any resolution error **fails the run at startup** (consistent with how\n missing secrets in MCP/prepare config behave).\n3. **Slim the executor.** `HookRunner`/`HookExecutor` take the runtime type;\n delete `resolve_interp`, `resolve_header`, `resolve_prompt_and_model`,\n `HeaderResolveError`, and the `Env` type parameters from execution paths.\n The executor formats, dispatches, and merges decisions — it resolves\n nothing.\n4. **No hook-side redaction work needed.** Hook output and block/skip reasons\n flow into events, and event serialization already applies the content-based\n redaction pass (`event/redaction.rs`, `redact_json_value`). That is the\n full extent of coverage by design — do not add redaction machinery for\n hook values (see the redaction context at the top).\n\n### Behavior changes (state these plainly in the PR description)\n\n- **Fail timing moves earlier.** A hook referencing a missing env var or\n secret today fails when (and only if) the hook fires; after this PR the run\n fails at startup, including for hooks that would never have fired. Both are\n fail-closed; startup surfacing is stricter and reports config errors\n immediately instead of mid-run.\n- **Eager resolution.** Hook secrets resolve even if the hook never fires;\n values are held in worker memory for the run, like every other\n boundary-resolved secret.\n- Env snapshot timing is theoretically observable but a practical no-op (see\n the environment-timing note above).\n\n## Implementation\n\n1. Boundary: `RuntimeHookDefinition` + `runtime_hooks(...)` with header\n policy; wire into `RunSession::new` next to the other `runtime_*`\n resolvers; hard-fail the run on any resolve error.\n2. `fabro-hooks`: switch `HookSettings`/runner/executor to the runtime type;\n delete the resolution layer; keep matcher/blocking/dispatch/\n `FABRO_HOOK_CONTEXT`/timeout code untouched.\n3. Migrate tests:\n - executor tests asserting resolution behavior (missing env var blocks at\n fire time; header allowlist gating; unavailable-namespace errors) become\n boundary tests asserting startup failure / rejection with the same error\n content;\n - executor execution tests (dispatch, decisions, timeouts, sandbox-vs-host)\n switch to literal strings.\n4. New end-to-end tests (worker level, hermetic temp-dir vaults):\n - command hook with a `secrets.NAME` token resolves from the vault and\n proceeds;\n - missing hook secret fails the run at startup, error names the secret;\n - `secrets.NAME` in an http-hook header fails at startup with the guidance\n message, and the endpoint is never called;\n - http hook with a secret-valued URL resolves and fires (mock server\n asserts the call);\n - a blocking command hook whose block reason echoes a resolved\n **credential-shaped** secret value (use a distinctive high-entropy test\n marker, never a realistic credential) has that value redacted in stored\n events by the existing content-based pass — proving hook secrets get the\n standard coverage. Do not assert redaction of low-entropy values; that\n is out of coverage by design;\n - a hook that references only env still works host-side and sandbox-side.\n5. Docs (`docs/public/` hooks page): secrets usable in hook command / url /\n prompt; headers reject secret tokens with the guidance; missing names fail\n at run start.\n\n## Scope boundaries — deliberately NOT in this PR\n\n- **No redaction machinery inside `fabro-hooks`** — restated as a boundary:\n hook output flows into events, and event serialization already applies the\n content-based pass. If you find yourself adding a redactor, a registry, or\n a secrets type to the hooks crate, you have left this PR's design.\n- **The event-serialization redaction pass in `event/redaction.rs`** — leave\n as-is; do not extend, scope, or restructure it for hook fields.\n- **Exec-output tails and any `fabro-sandbox` redaction signatures** — leave\n as-is; they already apply content-based redaction.\n- **Read-side server handlers and the event-detail `redacted` flag** — leave\n as-is; a separate change owns read paths.\n- **Typed wrapper types for secret values** — separate planned work. The\n runtime hook type carries plain resolved `String`s in this PR.\n\nIf work outside these boundaries seems genuinely required for this PR to\ncompile or pass its tests, stop and state that in the PR description rather\nthan expanding scope.\n\n## Acceptance / verification\n\n- `cargo +nightly-2026-04-14 fmt --check --all`\n- `cargo +nightly-2026-04-14 clippy --workspace --all-targets -- -D warnings`\n- `cargo nextest run --workspace` (touched: `fabro-hooks`, `fabro-workflow`,\n `fabro-types` if the runtime type lands there)\n- `cargo dev docs check`\n- No OpenAPI/wire change (config types untouched).\n\n## Conventions\n\n- Never print or log a resolved secret value, including from tests; preserve\n unresolved-source-only URL logging.\n- Plain-English commit messages, PR text, and comments; no internal planning\n identifiers or plan-file names in anything that ships.\n- PR description must include the two behavior changes above, framed as\n intended semantics (fail-fast config errors), and state the capability\n added (vault secrets in hooks) with the header exclusion.\n- If implementation uncovers a genuine need for per-firing interpolation in\n hook strings (none is known), stop and surface it rather than re-adding a\n fire-time resolver.\n", - "graph.rankdir": "LR", - "internal.work_dir": "/home/daytona/workspace/fabro", - "internal.thread_id": "preflight_compile", - "internal.retry_count.start": 0, - "internal.retry_count.preflight_compile": 0, "internal.fidelity": "compact", - "thread.preflight_compile.current_node": "preflight_lint", - "internal.retry_count.preflight_lint": 0, - "internal.node_visit_count": 1, + "internal.retry_count.toolchain": 0, "graph.model_stylesheet": "\n * { model: claude-opus-4-8; }\n ", - "current_node": "preflight_lint", - "thread.start.current_node": "toolchain", + "graph.rankdir": "LR", + "internal.retry_count.preflight_lint": 0, + "thread.preflight_compile.current_node": "preflight_lint", + "command.output": "blob://sha256/12ae32cb1ec02d01eda3581b127c1fee3b0dc53572ed6baf239721a03d82e126", + "graph.goal": "# PR 4 — Resolve hook interpolation at the run boundary (and enable secrets in hooks)\n\n**Self-contained implementation plan.** Everything needed to implement this is\nin this file plus the repository. Independent — no preconditions; can land\nanytime.\n\n**Redaction context (fixed; do not build on it):** run-output redaction in\nthis codebase is **content-based only** — entropy + credential-pattern\ndetection (`fabro_redact::redact_string` / `redact_json_value`), applied\nwhere events are serialized and where exec-output tails are captured. There\nis no per-run exact-value secret registry (a registration approach was\nconsidered and rejected). Secrets resolved for hooks by this PR get exactly\nthe coverage every other boundary-resolved secret (MCP env, run env, prepare\nsteps) already has: credential-shaped values are redacted from event and\ntail surfaces if echoed; a low-entropy secret value is not — an accepted,\ndocumented trade. Do not add any registration or exact-match machinery.\n\n> **Token notation.** Interpolation tokens are written in this file without\n> their enclosing double curly braces, so the file is safe to pass directly as\n> a workflow goal (the goal templater would otherwise try to expand them).\n> Read `secrets.NAME`, `env.NAME` as the double-curly-brace token form used in\n> the codebase, and write the real double-brace syntax in the code, tests, and\n> docs you produce.\n\n## Context and goal\n\nHooks are user-defined callbacks on workflow lifecycle events (`run_start`,\n`stage_start`, `pre_tool_use`, `sandbox_ready`, …) that can observe or gate a\nrun (decisions: proceed / block / skip / override). Four types: **command**\n(shell, runs in the sandbox by default or host-side with `sandbox = false`),\n**http** (POST from the worker), **prompt** and **agent** (LLM evaluation in\nthe worker). Their configurable string fields — `command`, `url`, header\nvalues, `prompt`, `model` — are typed `InterpString` and may carry `env.NAME`\ntokens.\n\nEvery other secret/env-consuming subsystem (MCP transport env, run-environment\nenv, prepare steps, docker config) resolves its InterpStrings **once, at the\nrun boundary** in `RunSession::new`, through one shared lookup closure. Hooks\nare the single exception: the hook **executor** resolves tokens **at fire\ntime**, and only the `env` namespace is wired there — a `secrets.NAME` token\nin a hook currently fails closed with \"unavailable namespace\". This fire-time\nresolution is a fossil, not a decision: it dates from the original hooks\nimplementation, before the boundary-resolution pattern existed, and was\ncarried forward unexamined. A previous attempt to add secrets support built a\nparallel fire-time secrets-resolution and redaction-registration subsystem\ninside the hooks crate to accommodate it; that PR was closed, and the accepted\ndirection is to remove the root special case instead.\n\n**Goal:** resolve all hook InterpStrings once at the run boundary through the\nshared closures, hand the hooks subsystem fully-resolved strings, and delete\nthe executor's resolution layer. Consequences, all intended:\n\n- `secrets.NAME` becomes usable in hook `command`, `url`, and `prompt`/`model`\n — the user-facing capability — with the same content-based redaction\n coverage every other boundary-resolved secret already has (see the\n redaction context above).\n- The invariant \"all env/secrets resolution happens at the run boundary\" holds\n with **zero exceptions**, so no future secrets/redaction work needs a hooks\n special case.\n- The hooks crate never learns about vaults or secrets at all.\n\nExplicitly out of scope / preserved:\n\n- The fire-time **context** mechanism is untouched: hooks receive per-firing\n data (event, node id, tool name, …) out-of-band via the `FABRO_HOOK_CONTEXT`\n env var, not via interpolation. There is no context namespace in\n InterpString; nothing interpolates per-firing data today, so boundary\n resolution loses no capability that exists.\n- Matcher semantics, blocking/decision merging, sandbox-vs-host dispatch,\n timeouts: unchanged.\n\n## Verified current state (as of main `9daca83b3`, 2026-07-09 — re-verify before starting; line numbers are anchors, not gospel)\n\n`lib/crates/fabro-hooks/src/executor.rs`:\n\n- `resolve_interp(value, env)` (~`:76`) — resolves an `InterpString` against\n process env at fire time; doc comment says only `env` is wired and other\n namespaces fail closed. Used for `command` and `url`.\n- `resolve_prompt_and_model` (~`:186`) — same, for prompt/agent hooks.\n- `resolve_header(value, allowed_env_vars, env)` (~`:132`) — header\n values additionally gate `env.NAME` behind the hook's `allowed_env_vars`\n list (`HeaderResolveError::NotAllowed`); resolution errors block the hook.\n- `safe_url_source_for_log` — logs the **unresolved** URL source (never the\n resolved URL) so env-sourced URL material is not logged.\n- Fail-closed dispositions today: a resolution error in `command` blocks; in\n `url`/headers/prompt/model the hook logs an error and does not fire (http\n headers produce a block); transport-level failures stay fail-open.\n\n`lib/crates/fabro-types/src/settings/run.rs`:\n\n- `HookDefinition { name, event, command: Option, hook_type,\n matcher, blocking, timeout_ms, sandbox }` (~`:2173`); `HookType::{Command,\n Http { url, headers, allowed_env_vars, tls }, Prompt { prompt, model },\n Agent { prompt, model } }` (~`:2149`). `vars.NAME` tokens in all these\n fields are already substituted at run creation (`substitute_variables`\n walks hooks), so only `env`/`secrets` tokens remain by boundary time.\n\n`lib/crates/fabro-workflow/src/operations/start.rs`:\n\n- The boundary pattern to mirror: `runtime_mcp_server(server, process_env_var,\n secret_lookup)` (~`:718`) and `runtime_setup_commands(...)` (~`:745`) —\n config type in, resolved runtime type out, hard error on missing names.\n- Hooks are currently passed through to the runner **unresolved** (find the\n hook wiring where `resolved.hooks` reaches `HookSettings`).\n\nEnvironment-timing note (verified): no `env::set_var` in production worker\npaths, so worker process env is identical at boundary time and fire time —\nresolving earlier does not change resolved values. Command hooks with\n`sandbox = true` already resolve against **worker** env and ship the resolved\nstring into the sandbox; that stays true, just earlier.\n\n## Design\n\n1. **Runtime hook type.** Add a resolved runtime form (e.g.\n `RuntimeHookDefinition`, plain `String` fields, mirroring\n `HookDefinition`/`HookType` shape) plus a boundary constructor\n `runtime_hooks(hooks, process_env_var, secret_lookup) -> Result>`\n in `operations/start.rs` alongside `runtime_mcp_server` /\n `runtime_setup_commands`. The config/wire type `HookDefinition` is\n unchanged — no API or manifest change. For http hooks, carry the\n **unresolved url source string** on the runtime type as well, for safe\n logging (preserves the `safe_url_source_for_log` guarantee).\n2. **Header policy enforced at the boundary, unchanged in substance:**\n - a `secrets.NAME` token in a header value is rejected **before any vault\n lookup**, with the existing guidance shape: secrets are not allowed in\n HTTP hook headers; use secret interpolation in a hook command, prompt,\n or url instead;\n - `env.NAME` in header values stays gated by `allowed_env_vars`\n (non-allowlisted name → error naming the variable; allowlisted-but-unset\n → missing-variable error);\n - `command`/`url`/`prompt`/`model` resolve `env` + `secrets` with hard\n errors on missing names.\n Any resolution error **fails the run at startup** (consistent with how\n missing secrets in MCP/prepare config behave).\n3. **Slim the executor.** `HookRunner`/`HookExecutor` take the runtime type;\n delete `resolve_interp`, `resolve_header`, `resolve_prompt_and_model`,\n `HeaderResolveError`, and the `Env` type parameters from execution paths.\n The executor formats, dispatches, and merges decisions — it resolves\n nothing.\n4. **No hook-side redaction work needed.** Hook output and block/skip reasons\n flow into events, and event serialization already applies the content-based\n redaction pass (`event/redaction.rs`, `redact_json_value`). That is the\n full extent of coverage by design — do not add redaction machinery for\n hook values (see the redaction context at the top).\n\n### Behavior changes (state these plainly in the PR description)\n\n- **Fail timing moves earlier.** A hook referencing a missing env var or\n secret today fails when (and only if) the hook fires; after this PR the run\n fails at startup, including for hooks that would never have fired. Both are\n fail-closed; startup surfacing is stricter and reports config errors\n immediately instead of mid-run.\n- **Eager resolution.** Hook secrets resolve even if the hook never fires;\n values are held in worker memory for the run, like every other\n boundary-resolved secret.\n- Env snapshot timing is theoretically observable but a practical no-op (see\n the environment-timing note above).\n\n## Implementation\n\n1. Boundary: `RuntimeHookDefinition` + `runtime_hooks(...)` with header\n policy; wire into `RunSession::new` next to the other `runtime_*`\n resolvers; hard-fail the run on any resolve error.\n2. `fabro-hooks`: switch `HookSettings`/runner/executor to the runtime type;\n delete the resolution layer; keep matcher/blocking/dispatch/\n `FABRO_HOOK_CONTEXT`/timeout code untouched.\n3. Migrate tests:\n - executor tests asserting resolution behavior (missing env var blocks at\n fire time; header allowlist gating; unavailable-namespace errors) become\n boundary tests asserting startup failure / rejection with the same error\n content;\n - executor execution tests (dispatch, decisions, timeouts, sandbox-vs-host)\n switch to literal strings.\n4. New end-to-end tests (worker level, hermetic temp-dir vaults):\n - command hook with a `secrets.NAME` token resolves from the vault and\n proceeds;\n - missing hook secret fails the run at startup, error names the secret;\n - `secrets.NAME` in an http-hook header fails at startup with the guidance\n message, and the endpoint is never called;\n - http hook with a secret-valued URL resolves and fires (mock server\n asserts the call);\n - a blocking command hook whose block reason echoes a resolved\n **credential-shaped** secret value (use a distinctive high-entropy test\n marker, never a realistic credential) has that value redacted in stored\n events by the existing content-based pass — proving hook secrets get the\n standard coverage. Do not assert redaction of low-entropy values; that\n is out of coverage by design;\n - a hook that references only env still works host-side and sandbox-side.\n5. Docs (`docs/public/` hooks page): secrets usable in hook command / url /\n prompt; headers reject secret tokens with the guidance; missing names fail\n at run start.\n\n## Scope boundaries — deliberately NOT in this PR\n\n- **No redaction machinery inside `fabro-hooks`** — restated as a boundary:\n hook output flows into events, and event serialization already applies the\n content-based pass. If you find yourself adding a redactor, a registry, or\n a secrets type to the hooks crate, you have left this PR's design.\n- **The event-serialization redaction pass in `event/redaction.rs`** — leave\n as-is; do not extend, scope, or restructure it for hook fields.\n- **Exec-output tails and any `fabro-sandbox` redaction signatures** — leave\n as-is; they already apply content-based redaction.\n- **Read-side server handlers and the event-detail `redacted` flag** — leave\n as-is; a separate change owns read paths.\n- **Typed wrapper types for secret values** — separate planned work. The\n runtime hook type carries plain resolved `String`s in this PR.\n\nIf work outside these boundaries seems genuinely required for this PR to\ncompile or pass its tests, stop and state that in the PR description rather\nthan expanding scope.\n\n## Acceptance / verification\n\n- `cargo +nightly-2026-04-14 fmt --check --all`\n- `cargo +nightly-2026-04-14 clippy --workspace --all-targets -- -D warnings`\n- `cargo nextest run --workspace` (touched: `fabro-hooks`, `fabro-workflow`,\n `fabro-types` if the runtime type lands there)\n- `cargo dev docs check`\n- No OpenAPI/wire change (config types untouched).\n\n## Conventions\n\n- Never print or log a resolved secret value, including from tests; preserve\n unresolved-source-only URL logging.\n- Plain-English commit messages, PR text, and comments; no internal planning\n identifiers or plan-file names in anything that ships.\n- PR description must include the two behavior changes above, framed as\n intended semantics (fail-fast config errors), and state the capability\n added (vault secrets in hooks) with the header exclusion.\n- If implementation uncovers a genuine need for per-firing interpolation in\n hook strings (none is known), stop and surface it rather than re-adding a\n fire-time resolver.\n", + "internal.retry_count.preflight_compile": 0, + "internal.node_visit_count": 1, + "internal.retry_count.start": 0, + "internal.work_dir": "/home/daytona/workspace/fabro", "outcome": "succeeded", + "failure_class": "", "failure_signature": "", - "internal.retry_count.toolchain": 0 + "thread.toolchain.current_node": "preflight_compile", + "internal.run_id": "01KX9EM9QF65A43PB064TF7TWY", + "thread.start.current_node": "toolchain", + "current_node": "preflight_lint", + "internal.thread_id": "preflight_compile" }, "node_outcomes": { "preflight_lint": { @@ -772,11 +772,123 @@ } }, "next_node_id": "implement", + "git_commit_sha": "047a3472f1ae96d171f23c1ae4ce498b4d64002e", + "node_visits": { + "start": 1, + "preflight_compile": 1, + "preflight_lint": 1, + "toolchain": 1 + } + }, + "diff": { + "summary": { + "files_changed": 0, + "additions": 0, + "deletions": 0 + } + } + }, + { + "seq": 0, + "checkpoint": { + "timestamp": "2026-07-11T20:45:45.781536842Z", + "current_node": "implement", + "completed_nodes": [ + "start", + "toolchain", + "preflight_compile", + "preflight_lint", + "implement" + ], + "node_retries": {}, + "context_values": { + "internal.run_id": "01KX9EM9QF65A43PB064TF7TWY", + "failure_class": "deterministic", + "thread.preflight_lint.current_node": "implement", + "command.output": "blob://sha256/12ae32cb1ec02d01eda3581b127c1fee3b0dc53572ed6baf239721a03d82e126", + "thread.toolchain.current_node": "preflight_compile", + "graph.goal": "# PR 4 — Resolve hook interpolation at the run boundary (and enable secrets in hooks)\n\n**Self-contained implementation plan.** Everything needed to implement this is\nin this file plus the repository. Independent — no preconditions; can land\nanytime.\n\n**Redaction context (fixed; do not build on it):** run-output redaction in\nthis codebase is **content-based only** — entropy + credential-pattern\ndetection (`fabro_redact::redact_string` / `redact_json_value`), applied\nwhere events are serialized and where exec-output tails are captured. There\nis no per-run exact-value secret registry (a registration approach was\nconsidered and rejected). Secrets resolved for hooks by this PR get exactly\nthe coverage every other boundary-resolved secret (MCP env, run env, prepare\nsteps) already has: credential-shaped values are redacted from event and\ntail surfaces if echoed; a low-entropy secret value is not — an accepted,\ndocumented trade. Do not add any registration or exact-match machinery.\n\n> **Token notation.** Interpolation tokens are written in this file without\n> their enclosing double curly braces, so the file is safe to pass directly as\n> a workflow goal (the goal templater would otherwise try to expand them).\n> Read `secrets.NAME`, `env.NAME` as the double-curly-brace token form used in\n> the codebase, and write the real double-brace syntax in the code, tests, and\n> docs you produce.\n\n## Context and goal\n\nHooks are user-defined callbacks on workflow lifecycle events (`run_start`,\n`stage_start`, `pre_tool_use`, `sandbox_ready`, …) that can observe or gate a\nrun (decisions: proceed / block / skip / override). Four types: **command**\n(shell, runs in the sandbox by default or host-side with `sandbox = false`),\n**http** (POST from the worker), **prompt** and **agent** (LLM evaluation in\nthe worker). Their configurable string fields — `command`, `url`, header\nvalues, `prompt`, `model` — are typed `InterpString` and may carry `env.NAME`\ntokens.\n\nEvery other secret/env-consuming subsystem (MCP transport env, run-environment\nenv, prepare steps, docker config) resolves its InterpStrings **once, at the\nrun boundary** in `RunSession::new`, through one shared lookup closure. Hooks\nare the single exception: the hook **executor** resolves tokens **at fire\ntime**, and only the `env` namespace is wired there — a `secrets.NAME` token\nin a hook currently fails closed with \"unavailable namespace\". This fire-time\nresolution is a fossil, not a decision: it dates from the original hooks\nimplementation, before the boundary-resolution pattern existed, and was\ncarried forward unexamined. A previous attempt to add secrets support built a\nparallel fire-time secrets-resolution and redaction-registration subsystem\ninside the hooks crate to accommodate it; that PR was closed, and the accepted\ndirection is to remove the root special case instead.\n\n**Goal:** resolve all hook InterpStrings once at the run boundary through the\nshared closures, hand the hooks subsystem fully-resolved strings, and delete\nthe executor's resolution layer. Consequences, all intended:\n\n- `secrets.NAME` becomes usable in hook `command`, `url`, and `prompt`/`model`\n — the user-facing capability — with the same content-based redaction\n coverage every other boundary-resolved secret already has (see the\n redaction context above).\n- The invariant \"all env/secrets resolution happens at the run boundary\" holds\n with **zero exceptions**, so no future secrets/redaction work needs a hooks\n special case.\n- The hooks crate never learns about vaults or secrets at all.\n\nExplicitly out of scope / preserved:\n\n- The fire-time **context** mechanism is untouched: hooks receive per-firing\n data (event, node id, tool name, …) out-of-band via the `FABRO_HOOK_CONTEXT`\n env var, not via interpolation. There is no context namespace in\n InterpString; nothing interpolates per-firing data today, so boundary\n resolution loses no capability that exists.\n- Matcher semantics, blocking/decision merging, sandbox-vs-host dispatch,\n timeouts: unchanged.\n\n## Verified current state (as of main `9daca83b3`, 2026-07-09 — re-verify before starting; line numbers are anchors, not gospel)\n\n`lib/crates/fabro-hooks/src/executor.rs`:\n\n- `resolve_interp(value, env)` (~`:76`) — resolves an `InterpString` against\n process env at fire time; doc comment says only `env` is wired and other\n namespaces fail closed. Used for `command` and `url`.\n- `resolve_prompt_and_model` (~`:186`) — same, for prompt/agent hooks.\n- `resolve_header(value, allowed_env_vars, env)` (~`:132`) — header\n values additionally gate `env.NAME` behind the hook's `allowed_env_vars`\n list (`HeaderResolveError::NotAllowed`); resolution errors block the hook.\n- `safe_url_source_for_log` — logs the **unresolved** URL source (never the\n resolved URL) so env-sourced URL material is not logged.\n- Fail-closed dispositions today: a resolution error in `command` blocks; in\n `url`/headers/prompt/model the hook logs an error and does not fire (http\n headers produce a block); transport-level failures stay fail-open.\n\n`lib/crates/fabro-types/src/settings/run.rs`:\n\n- `HookDefinition { name, event, command: Option, hook_type,\n matcher, blocking, timeout_ms, sandbox }` (~`:2173`); `HookType::{Command,\n Http { url, headers, allowed_env_vars, tls }, Prompt { prompt, model },\n Agent { prompt, model } }` (~`:2149`). `vars.NAME` tokens in all these\n fields are already substituted at run creation (`substitute_variables`\n walks hooks), so only `env`/`secrets` tokens remain by boundary time.\n\n`lib/crates/fabro-workflow/src/operations/start.rs`:\n\n- The boundary pattern to mirror: `runtime_mcp_server(server, process_env_var,\n secret_lookup)` (~`:718`) and `runtime_setup_commands(...)` (~`:745`) —\n config type in, resolved runtime type out, hard error on missing names.\n- Hooks are currently passed through to the runner **unresolved** (find the\n hook wiring where `resolved.hooks` reaches `HookSettings`).\n\nEnvironment-timing note (verified): no `env::set_var` in production worker\npaths, so worker process env is identical at boundary time and fire time —\nresolving earlier does not change resolved values. Command hooks with\n`sandbox = true` already resolve against **worker** env and ship the resolved\nstring into the sandbox; that stays true, just earlier.\n\n## Design\n\n1. **Runtime hook type.** Add a resolved runtime form (e.g.\n `RuntimeHookDefinition`, plain `String` fields, mirroring\n `HookDefinition`/`HookType` shape) plus a boundary constructor\n `runtime_hooks(hooks, process_env_var, secret_lookup) -> Result>`\n in `operations/start.rs` alongside `runtime_mcp_server` /\n `runtime_setup_commands`. The config/wire type `HookDefinition` is\n unchanged — no API or manifest change. For http hooks, carry the\n **unresolved url source string** on the runtime type as well, for safe\n logging (preserves the `safe_url_source_for_log` guarantee).\n2. **Header policy enforced at the boundary, unchanged in substance:**\n - a `secrets.NAME` token in a header value is rejected **before any vault\n lookup**, with the existing guidance shape: secrets are not allowed in\n HTTP hook headers; use secret interpolation in a hook command, prompt,\n or url instead;\n - `env.NAME` in header values stays gated by `allowed_env_vars`\n (non-allowlisted name → error naming the variable; allowlisted-but-unset\n → missing-variable error);\n - `command`/`url`/`prompt`/`model` resolve `env` + `secrets` with hard\n errors on missing names.\n Any resolution error **fails the run at startup** (consistent with how\n missing secrets in MCP/prepare config behave).\n3. **Slim the executor.** `HookRunner`/`HookExecutor` take the runtime type;\n delete `resolve_interp`, `resolve_header`, `resolve_prompt_and_model`,\n `HeaderResolveError`, and the `Env` type parameters from execution paths.\n The executor formats, dispatches, and merges decisions — it resolves\n nothing.\n4. **No hook-side redaction work needed.** Hook output and block/skip reasons\n flow into events, and event serialization already applies the content-based\n redaction pass (`event/redaction.rs`, `redact_json_value`). That is the\n full extent of coverage by design — do not add redaction machinery for\n hook values (see the redaction context at the top).\n\n### Behavior changes (state these plainly in the PR description)\n\n- **Fail timing moves earlier.** A hook referencing a missing env var or\n secret today fails when (and only if) the hook fires; after this PR the run\n fails at startup, including for hooks that would never have fired. Both are\n fail-closed; startup surfacing is stricter and reports config errors\n immediately instead of mid-run.\n- **Eager resolution.** Hook secrets resolve even if the hook never fires;\n values are held in worker memory for the run, like every other\n boundary-resolved secret.\n- Env snapshot timing is theoretically observable but a practical no-op (see\n the environment-timing note above).\n\n## Implementation\n\n1. Boundary: `RuntimeHookDefinition` + `runtime_hooks(...)` with header\n policy; wire into `RunSession::new` next to the other `runtime_*`\n resolvers; hard-fail the run on any resolve error.\n2. `fabro-hooks`: switch `HookSettings`/runner/executor to the runtime type;\n delete the resolution layer; keep matcher/blocking/dispatch/\n `FABRO_HOOK_CONTEXT`/timeout code untouched.\n3. Migrate tests:\n - executor tests asserting resolution behavior (missing env var blocks at\n fire time; header allowlist gating; unavailable-namespace errors) become\n boundary tests asserting startup failure / rejection with the same error\n content;\n - executor execution tests (dispatch, decisions, timeouts, sandbox-vs-host)\n switch to literal strings.\n4. New end-to-end tests (worker level, hermetic temp-dir vaults):\n - command hook with a `secrets.NAME` token resolves from the vault and\n proceeds;\n - missing hook secret fails the run at startup, error names the secret;\n - `secrets.NAME` in an http-hook header fails at startup with the guidance\n message, and the endpoint is never called;\n - http hook with a secret-valued URL resolves and fires (mock server\n asserts the call);\n - a blocking command hook whose block reason echoes a resolved\n **credential-shaped** secret value (use a distinctive high-entropy test\n marker, never a realistic credential) has that value redacted in stored\n events by the existing content-based pass — proving hook secrets get the\n standard coverage. Do not assert redaction of low-entropy values; that\n is out of coverage by design;\n - a hook that references only env still works host-side and sandbox-side.\n5. Docs (`docs/public/` hooks page): secrets usable in hook command / url /\n prompt; headers reject secret tokens with the guidance; missing names fail\n at run start.\n\n## Scope boundaries — deliberately NOT in this PR\n\n- **No redaction machinery inside `fabro-hooks`** — restated as a boundary:\n hook output flows into events, and event serialization already applies the\n content-based pass. If you find yourself adding a redactor, a registry, or\n a secrets type to the hooks crate, you have left this PR's design.\n- **The event-serialization redaction pass in `event/redaction.rs`** — leave\n as-is; do not extend, scope, or restructure it for hook fields.\n- **Exec-output tails and any `fabro-sandbox` redaction signatures** — leave\n as-is; they already apply content-based redaction.\n- **Read-side server handlers and the event-detail `redacted` flag** — leave\n as-is; a separate change owns read paths.\n- **Typed wrapper types for secret values** — separate planned work. The\n runtime hook type carries plain resolved `String`s in this PR.\n\nIf work outside these boundaries seems genuinely required for this PR to\ncompile or pass its tests, stop and state that in the PR description rather\nthan expanding scope.\n\n## Acceptance / verification\n\n- `cargo +nightly-2026-04-14 fmt --check --all`\n- `cargo +nightly-2026-04-14 clippy --workspace --all-targets -- -D warnings`\n- `cargo nextest run --workspace` (touched: `fabro-hooks`, `fabro-workflow`,\n `fabro-types` if the runtime type lands there)\n- `cargo dev docs check`\n- No OpenAPI/wire change (config types untouched).\n\n## Conventions\n\n- Never print or log a resolved secret value, including from tests; preserve\n unresolved-source-only URL logging.\n- Plain-English commit messages, PR text, and comments; no internal planning\n identifiers or plan-file names in anything that ships.\n- PR description must include the two behavior changes above, framed as\n intended semantics (fail-fast config errors), and state the capability\n added (vault secrets in hooks) with the header exclusion.\n- If implementation uncovers a genuine need for per-firing interpolation in\n hook strings (none is known), stop and surface it rather than re-adding a\n fire-time resolver.\n", + "graph.rankdir": "LR", + "internal.work_dir": "/home/daytona/workspace/fabro", + "internal.thread_id": "preflight_lint", + "internal.retry_count.start": 0, + "internal.retry_count.preflight_compile": 0, + "internal.fidelity": "compact", + "thread.preflight_compile.current_node": "preflight_lint", + "internal.retry_count.preflight_lint": 0, + "internal.retry_count.implement": 0, + "internal.node_visit_count": 1, + "graph.model_stylesheet": "\n * { model: claude-opus-4-8; }\n ", + "current_node": "implement", + "thread.start.current_node": "toolchain", + "outcome": "failed", + "failure_signature": "implement|deterministic|api_deterministic|openai|authentication", + "internal.retry_count.toolchain": 0 + }, + "node_outcomes": { + "preflight_lint": { + "status": "succeeded", + "context_updates": { + "command.output": "blob://sha256/12ae32cb1ec02d01eda3581b127c1fee3b0dc53572ed6baf239721a03d82e126" + }, + "notes": "Script completed: cargo +nightly-2026-04-14 clippy -q --workspace --all-targets -- -D warnings 2>&1", + "usage": null, + "timing": { + "wall_time_ms": 0, + "inference_time_ms": 0, + "tool_time_ms": 111530, + "active_time_ms": 111530 + } + }, + "implement": { + "status": "failed", + "failure": { + "message": "LLM error: Authentication error for openai: Your authentication token has been invalidated. Please try signing in again.", + "category": "deterministic", + "signature": "api_deterministic|openai|authentication" + }, + "usage": null + }, + "start": { + "status": "succeeded", + "usage": null + }, + "preflight_compile": { + "status": "succeeded", + "context_updates": { + "command.output": "blob://sha256/12ae32cb1ec02d01eda3581b127c1fee3b0dc53572ed6baf239721a03d82e126" + }, + "notes": "Script completed: cargo check -q --workspace 2>&1", + "usage": null, + "timing": { + "wall_time_ms": 0, + "inference_time_ms": 0, + "tool_time_ms": 105518, + "active_time_ms": 105518 + } + }, + "toolchain": { + "status": "succeeded", + "context_updates": { + "command.output": "blob://sha256/20eeffec02497fbda7b51f51b06fe29c1d639551eee4d5ea9845fc1f86bd77e1" + }, + "notes": "Script completed: 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", + "usage": null, + "timing": { + "wall_time_ms": 0, + "inference_time_ms": 0, + "tool_time_ms": 1299, + "active_time_ms": 1299 + } + } + }, + "next_node_id": "simplify_fable", "node_visits": { "start": 1, "preflight_compile": 1, "toolchain": 1, - "preflight_lint": 1 + "preflight_lint": 1, + "implement": 1 } }, "diff": {} @@ -890,6 +1002,181 @@ }, "state": "succeeded" }, + "implement@1": { + "first_event_seq": 52, + "prompt": null, + "response": null, + "completion": null, + "provider_used": { + "mode": "agent", + "provider": "openai", + "model": "gpt-5.5", + "reasoning_effort": "xhigh" + }, + "diff": null, + "script_invocation": null, + "script_timing": null, + "parallel_results": null, + "output": null, + "started_at": "2026-07-11T20:45:45.299664832Z", + "handler": "agent", + "usage": { + "input_tokens": 0, + "output_tokens": 0, + "total_tokens": 0, + "reasoning_tokens": 0, + "cache_read_tokens": 0, + "cache_write_tokens": 0 + }, + "skills": { + "available": [ + { + "name": "rust-style-guide", + "description": "Apply this Rust style guide when writing, reviewing, refactoring, or configuring Rust code for this project. Covers Rust 2024/MSRV, library vs application conventions, public API design, errors, panics, ownership and cloning, async/Tokio/concurrency, tracing, rustfmt/Clippy, testing with nextest, and unsafe/macro policy. Also use when setting up new Rust projects, investigating Rust performance, verifying library releases, or reviewing Rust code changes." + } + ], + "activated": [] + }, + "permission_level": "full", + "agent_tools": [ + { + "name": "apply_patch", + "description": "Use the `apply_patch` tool to edit files. This is a FREEFORM tool, so do not wrap the patch in JSON.", + "source": { + "kind": "native" + }, + "category": "write", + "invoked": false + }, + { + "name": "close_agent", + "description": "Close a running subagent that is no longer needed.", + "source": { + "kind": "native" + }, + "category": "subagent", + "invoked": false + }, + { + "name": "glob", + "description": "Find files by file names using a glob pattern. Use path to choose the search root. Prefer this over shell find or ls when locating repository files.", + "source": { + "kind": "native" + }, + "category": "read", + "invoked": false + }, + { + "name": "grep", + "description": "Search file contents with a regex pattern. Use path to choose the search root, glob_filter to limit matching files, case_insensitive for case folding, and max_results to cap output.", + "source": { + "kind": "native" + }, + "category": "read", + "invoked": false + }, + { + "name": "read_file", + "description": "Read files before editing them. Returns line-numbered text and supports offset/limit for large files. Use this instead of shell cat, head, tail, or sed when inspecting repository files.", + "source": { + "kind": "native" + }, + "category": "read", + "invoked": false + }, + { + "name": "request_user_input", + "description": "Ask the human one or more questions and wait for their answers before continuing this stage.", + "source": { + "kind": "native" + }, + "category": "other", + "invoked": false + }, + { + "name": "send_input", + "description": "Send a follow-up message to a running subagent when new information or corrected instructions are needed.", + "source": { + "kind": "native" + }, + "category": "subagent", + "invoked": false + }, + { + "name": "shell", + "description": "Execute shell commands for terminal operations, package managers, tests and builds. Use dedicated tools for file reads, file edits, filename searches, and content searches. Provide timeout_ms for long-running commands.", + "source": { + "kind": "native" + }, + "category": "shell", + "invoked": false + }, + { + "name": "spawn_agent", + "description": "Spawn a subagent for independent work or context isolation. Use it for tasks that can proceed separately, and avoid duplicating the same work in the parent session.", + "source": { + "kind": "native" + }, + "category": "subagent", + "invoked": false + }, + { + "name": "update_plan", + "description": "Update the multi-step plan for the current task. Submit the entire plan; existing steps are reconciled by exact step text.", + "source": { + "kind": "native" + }, + "category": "other", + "invoked": false + }, + { + "name": "use_skill", + "description": "Load a skill's instructions by name. Call this when the user's request matches an available skill.", + "source": { + "kind": "skill" + }, + "category": "other", + "invoked": false + }, + { + "name": "wait", + "description": "Wait for a subagent to complete, then use the result to synthesize the outcome for the user.", + "source": { + "kind": "native" + }, + "category": "subagent", + "invoked": false + }, + { + "name": "web_fetch", + "description": "Fetch content from a URL that starts with http:// or https://. Pass a prompt to extract specific information or summarize the page; omit prompt to return the page content.", + "source": { + "kind": "native" + }, + "category": "other", + "invoked": false + }, + { + "name": "web_search", + "description": "Search the web using Brave Search when current external information is needed. Returns result titles, URLs, and descriptions; use web_fetch for a specific URL.", + "source": { + "kind": "native" + }, + "category": "other", + "invoked": false + }, + { + "name": "write_file", + "description": "Create new files, or overwrite an existing file only when replacement is explicitly intended. Prefer edit_file for targeted changes to existing files because write_file overwrites the full file content.", + "source": { + "kind": "native" + }, + "category": "write", + "invoked": false + } + ], + "state": "running" + }, "preflight_compile@1": { "first_event_seq": 32, "prompt": null, @@ -942,7 +1229,12 @@ "first_event_seq": 42, "prompt": null, "response": null, - "completion": null, + "completion": { + "outcome": "succeeded", + "notes": "Script completed: cargo +nightly-2026-04-14 clippy -q --workspace --all-targets -- -D warnings 2>&1", + "failure_reason": null, + "timestamp": "2026-07-11T20:45:42.138415261Z" + }, "provider_used": null, "diff": null, "script_invocation": { @@ -950,11 +1242,27 @@ "command": "exec 2>&1\ncargo +nightly-2026-04-14 clippy -q --workspace --all-targets -- -D warnings 2>&1", "language": "shell" }, - "script_timing": null, + "script_timing": { + "output": "blob://sha256/12ae32cb1ec02d01eda3581b127c1fee3b0dc53572ed6baf239721a03d82e126", + "exit_code": 0, + "duration_ms": 111530, + "termination": "exited", + "output_bytes": 0, + "live_streaming": false + }, "parallel_results": null, "output": null, + "output_bytes": 0, + "live_streaming": false, + "termination": "exited", "started_at": "2026-07-11T20:43:50.603822193Z", "handler": "command", + "timing": { + "wall_time_ms": 111534, + "inference_time_ms": 0, + "tool_time_ms": 111530, + "active_time_ms": 111530 + }, "usage": { "input_tokens": 0, "output_tokens": 0, @@ -963,7 +1271,7 @@ "cache_read_tokens": 0, "cache_write_tokens": 0 }, - "state": "running" + "state": "succeeded" } } } \ No newline at end of file diff --git a/stages/004-preflight_lint@1/output.log b/stages/004-preflight_lint@1/output.log new file mode 100644 index 000000000..d87ba9545 --- /dev/null +++ b/stages/004-preflight_lint@1/output.log @@ -0,0 +1 @@ +blob://sha256/12ae32cb1ec02d01eda3581b127c1fee3b0dc53572ed6baf239721a03d82e126 \ No newline at end of file diff --git a/stages/004-preflight_lint@1/script_timing.json b/stages/004-preflight_lint@1/script_timing.json new file mode 100644 index 000000000..da07cc649 --- /dev/null +++ b/stages/004-preflight_lint@1/script_timing.json @@ -0,0 +1,8 @@ +{ + "output": "blob://sha256/12ae32cb1ec02d01eda3581b127c1fee3b0dc53572ed6baf239721a03d82e126", + "exit_code": 0, + "duration_ms": 111530, + "termination": "exited", + "output_bytes": 0, + "live_streaming": false +} \ No newline at end of file diff --git a/stages/004-preflight_lint@1/status.json b/stages/004-preflight_lint@1/status.json new file mode 100644 index 000000000..de37e9987 --- /dev/null +++ b/stages/004-preflight_lint@1/status.json @@ -0,0 +1,6 @@ +{ + "outcome": "succeeded", + "notes": "Script completed: cargo +nightly-2026-04-14 clippy -q --workspace --all-targets -- -D warnings 2>&1", + "failure_reason": null, + "timestamp": "2026-07-11T20:45:42.138415261Z" +} \ No newline at end of file diff --git a/stages/005-implement@1/prompt.md b/stages/005-implement@1/prompt.md new file mode 100644 index 000000000..6ce1ac07a --- /dev/null +++ b/stages/005-implement@1/prompt.md @@ -0,0 +1,250 @@ +Goal: # PR 4 — Resolve hook interpolation at the run boundary (and enable secrets in hooks) + +**Self-contained implementation plan.** Everything needed to implement this is +in this file plus the repository. Independent — no preconditions; can land +anytime. + +**Redaction context (fixed; do not build on it):** run-output redaction in +this codebase is **content-based only** — entropy + credential-pattern +detection (`fabro_redact::redact_string` / `redact_json_value`), applied +where events are serialized and where exec-output tails are captured. There +is no per-run exact-value secret registry (a registration approach was +considered and rejected). Secrets resolved for hooks by this PR get exactly +the coverage every other boundary-resolved secret (MCP env, run env, prepare +steps) already has: credential-shaped values are redacted from event and +tail surfaces if echoed; a low-entropy secret value is not — an accepted, +documented trade. Do not add any registration or exact-match machinery. + +> **Token notation.** Interpolation tokens are written in this file without +> their enclosing double curly braces, so the file is safe to pass directly as +> a workflow goal (the goal templater would otherwise try to expand them). +> Read `secrets.NAME`, `env.NAME` as the double-curly-brace token form used in +> the codebase, and write the real double-brace syntax in the code, tests, and +> docs you produce. + +## Context and goal + +Hooks are user-defined callbacks on workflow lifecycle events (`run_start`, +`stage_start`, `pre_tool_use`, `sandbox_ready`, …) that can observe or gate a +run (decisions: proceed / block / skip / override). Four types: **command** +(shell, runs in the sandbox by default or host-side with `sandbox = false`), +**http** (POST from the worker), **prompt** and **agent** (LLM evaluation in +the worker). Their configurable string fields — `command`, `url`, header +values, `prompt`, `model` — are typed `InterpString` and may carry `env.NAME` +tokens. + +Every other secret/env-consuming subsystem (MCP transport env, run-environment +env, prepare steps, docker config) resolves its InterpStrings **once, at the +run boundary** in `RunSession::new`, through one shared lookup closure. Hooks +are the single exception: the hook **executor** resolves tokens **at fire +time**, and only the `env` namespace is wired there — a `secrets.NAME` token +in a hook currently fails closed with "unavailable namespace". This fire-time +resolution is a fossil, not a decision: it dates from the original hooks +implementation, before the boundary-resolution pattern existed, and was +carried forward unexamined. A previous attempt to add secrets support built a +parallel fire-time secrets-resolution and redaction-registration subsystem +inside the hooks crate to accommodate it; that PR was closed, and the accepted +direction is to remove the root special case instead. + +**Goal:** resolve all hook InterpStrings once at the run boundary through the +shared closures, hand the hooks subsystem fully-resolved strings, and delete +the executor's resolution layer. Consequences, all intended: + +- `secrets.NAME` becomes usable in hook `command`, `url`, and `prompt`/`model` + — the user-facing capability — with the same content-based redaction + coverage every other boundary-resolved secret already has (see the + redaction context above). +- The invariant "all env/secrets resolution happens at the run boundary" holds + with **zero exceptions**, so no future secrets/redaction work needs a hooks + special case. +- The hooks crate never learns about vaults or secrets at all. + +Explicitly out of scope / preserved: + +- The fire-time **context** mechanism is untouched: hooks receive per-firing + data (event, node id, tool name, …) out-of-band via the `FABRO_HOOK_CONTEXT` + env var, not via interpolation. There is no context namespace in + InterpString; nothing interpolates per-firing data today, so boundary + resolution loses no capability that exists. +- Matcher semantics, blocking/decision merging, sandbox-vs-host dispatch, + timeouts: unchanged. + +## Verified current state (as of main `9daca83b3`, 2026-07-09 — re-verify before starting; line numbers are anchors, not gospel) + +`lib/crates/fabro-hooks/src/executor.rs`: + +- `resolve_interp(value, env)` (~`:76`) — resolves an `InterpString` against + process env at fire time; doc comment says only `env` is wired and other + namespaces fail closed. Used for `command` and `url`. +- `resolve_prompt_and_model` (~`:186`) — same, for prompt/agent hooks. +- `resolve_header(value, allowed_env_vars, env)` (~`:132`) — header + values additionally gate `env.NAME` behind the hook's `allowed_env_vars` + list (`HeaderResolveError::NotAllowed`); resolution errors block the hook. +- `safe_url_source_for_log` — logs the **unresolved** URL source (never the + resolved URL) so env-sourced URL material is not logged. +- Fail-closed dispositions today: a resolution error in `command` blocks; in + `url`/headers/prompt/model the hook logs an error and does not fire (http + headers produce a block); transport-level failures stay fail-open. + +`lib/crates/fabro-types/src/settings/run.rs`: + +- `HookDefinition { name, event, command: Option, hook_type, + matcher, blocking, timeout_ms, sandbox }` (~`:2173`); `HookType::{Command, + Http { url, headers, allowed_env_vars, tls }, Prompt { prompt, model }, + Agent { prompt, model } }` (~`:2149`). `vars.NAME` tokens in all these + fields are already substituted at run creation (`substitute_variables` + walks hooks), so only `env`/`secrets` tokens remain by boundary time. + +`lib/crates/fabro-workflow/src/operations/start.rs`: + +- The boundary pattern to mirror: `runtime_mcp_server(server, process_env_var, + secret_lookup)` (~`:718`) and `runtime_setup_commands(...)` (~`:745`) — + config type in, resolved runtime type out, hard error on missing names. +- Hooks are currently passed through to the runner **unresolved** (find the + hook wiring where `resolved.hooks` reaches `HookSettings`). + +Environment-timing note (verified): no `env::set_var` in production worker +paths, so worker process env is identical at boundary time and fire time — +resolving earlier does not change resolved values. Command hooks with +`sandbox = true` already resolve against **worker** env and ship the resolved +string into the sandbox; that stays true, just earlier. + +## Design + +1. **Runtime hook type.** Add a resolved runtime form (e.g. + `RuntimeHookDefinition`, plain `String` fields, mirroring + `HookDefinition`/`HookType` shape) plus a boundary constructor + `runtime_hooks(hooks, process_env_var, secret_lookup) -> Result>` + in `operations/start.rs` alongside `runtime_mcp_server` / + `runtime_setup_commands`. The config/wire type `HookDefinition` is + unchanged — no API or manifest change. For http hooks, carry the + **unresolved url source string** on the runtime type as well, for safe + logging (preserves the `safe_url_source_for_log` guarantee). +2. **Header policy enforced at the boundary, unchanged in substance:** + - a `secrets.NAME` token in a header value is rejected **before any vault + lookup**, with the existing guidance shape: secrets are not allowed in + HTTP hook headers; use secret interpolation in a hook command, prompt, + or url instead; + - `env.NAME` in header values stays gated by `allowed_env_vars` + (non-allowlisted name → error naming the variable; allowlisted-but-unset + → missing-variable error); + - `command`/`url`/`prompt`/`model` resolve `env` + `secrets` with hard + errors on missing names. + Any resolution error **fails the run at startup** (consistent with how + missing secrets in MCP/prepare config behave). +3. **Slim the executor.** `HookRunner`/`HookExecutor` take the runtime type; + delete `resolve_interp`, `resolve_header`, `resolve_prompt_and_model`, + `HeaderResolveError`, and the `Env` type parameters from execution paths. + The executor formats, dispatches, and merges decisions — it resolves + nothing. +4. **No hook-side redaction work needed.** Hook output and block/skip reasons + flow into events, and event serialization already applies the content-based + redaction pass (`event/redaction.rs`, `redact_json_value`). That is the + full extent of coverage by design — do not add redaction machinery for + hook values (see the redaction context at the top). + +### Behavior changes (state these plainly in the PR description) + +- **Fail timing moves earlier.** A hook referencing a missing env var or + secret today fails when (and only if) the hook fires; after this PR the run + fails at startup, including for hooks that would never have fired. Both are + fail-closed; startup surfacing is stricter and reports config errors + immediately instead of mid-run. +- **Eager resolution.** Hook secrets resolve even if the hook never fires; + values are held in worker memory for the run, like every other + boundary-resolved secret. +- Env snapshot timing is theoretically observable but a practical no-op (see + the environment-timing note above). + +## Implementation + +1. Boundary: `RuntimeHookDefinition` + `runtime_hooks(...)` with header + policy; wire into `RunSession::new` next to the other `runtime_*` + resolvers; hard-fail the run on any resolve error. +2. `fabro-hooks`: switch `HookSettings`/runner/executor to the runtime type; + delete the resolution layer; keep matcher/blocking/dispatch/ + `FABRO_HOOK_CONTEXT`/timeout code untouched. +3. Migrate tests: + - executor tests asserting resolution behavior (missing env var blocks at + fire time; header allowlist gating; unavailable-namespace errors) become + boundary tests asserting startup failure / rejection with the same error + content; + - executor execution tests (dispatch, decisions, timeouts, sandbox-vs-host) + switch to literal strings. +4. New end-to-end tests (worker level, hermetic temp-dir vaults): + - command hook with a `secrets.NAME` token resolves from the vault and + proceeds; + - missing hook secret fails the run at startup, error names the secret; + - `secrets.NAME` in an http-hook header fails at startup with the guidance + message, and the endpoint is never called; + - http hook with a secret-valued URL resolves and fires (mock server + asserts the call); + - a blocking command hook whose block reason echoes a resolved + **credential-shaped** secret value (use a distinctive high-entropy test + marker, never a realistic credential) has that value redacted in stored + events by the existing content-based pass — proving hook secrets get the + standard coverage. Do not assert redaction of low-entropy values; that + is out of coverage by design; + - a hook that references only env still works host-side and sandbox-side. +5. Docs (`docs/public/` hooks page): secrets usable in hook command / url / + prompt; headers reject secret tokens with the guidance; missing names fail + at run start. + +## Scope boundaries — deliberately NOT in this PR + +- **No redaction machinery inside `fabro-hooks`** — restated as a boundary: + hook output flows into events, and event serialization already applies the + content-based pass. If you find yourself adding a redactor, a registry, or + a secrets type to the hooks crate, you have left this PR's design. +- **The event-serialization redaction pass in `event/redaction.rs`** — leave + as-is; do not extend, scope, or restructure it for hook fields. +- **Exec-output tails and any `fabro-sandbox` redaction signatures** — leave + as-is; they already apply content-based redaction. +- **Read-side server handlers and the event-detail `redacted` flag** — leave + as-is; a separate change owns read paths. +- **Typed wrapper types for secret values** — separate planned work. The + runtime hook type carries plain resolved `String`s in this PR. + +If work outside these boundaries seems genuinely required for this PR to +compile or pass its tests, stop and state that in the PR description rather +than expanding scope. + +## Acceptance / verification + +- `cargo +nightly-2026-04-14 fmt --check --all` +- `cargo +nightly-2026-04-14 clippy --workspace --all-targets -- -D warnings` +- `cargo nextest run --workspace` (touched: `fabro-hooks`, `fabro-workflow`, + `fabro-types` if the runtime type lands there) +- `cargo dev docs check` +- No OpenAPI/wire change (config types untouched). + +## Conventions + +- Never print or log a resolved secret value, including from tests; preserve + unresolved-source-only URL logging. +- Plain-English commit messages, PR text, and comments; no internal planning + identifiers or plan-file names in anything that ships. +- PR description must include the two behavior changes above, framed as + intended semantics (fail-fast config errors), and state the capability + added (vault secrets in hooks) with the header exclusion. +- If implementation uncovers a genuine need for per-firing interpolation in + hook strings (none is known), stop and surface it rather than re-adding a + fire-time resolver. + + +## Completed stages +- **toolchain**: succeeded + - 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` + - Output: + ``` + cargo 1.96.0 (30a34c682 2026-05-25) + ``` +- **preflight_compile**: succeeded + - Script: `cargo check -q --workspace 2>&1` + - Output: (empty) +- **preflight_lint**: succeeded + - Script: `cargo +nightly-2026-04-14 clippy -q --workspace --all-targets -- -D warnings 2>&1` + - Output: (empty) + + +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. Be sure to use the rust-style-guide skill to help you follow this repo's Rust style conventions. \ No newline at end of file diff --git a/stages/005-implement@1/provider_used.json b/stages/005-implement@1/provider_used.json new file mode 100644 index 000000000..c57772db6 --- /dev/null +++ b/stages/005-implement@1/provider_used.json @@ -0,0 +1,6 @@ +{ + "mode": "agent", + "provider": "openai", + "model": "gpt-5.5", + "reasoning_effort": "xhigh" +} \ No newline at end of file