A hook matcher regex that failed to compile was dropped silently in compile_matchers, while matches() carried a comment claiming the failure had "already warned" — which never happened. A typo'd matcher made a hook silently never fire, with no signal.
Emit a warning at construction (naming the pattern and error), flatten the nested conditionals, and correct the stale comment. Add a test asserting an invalid pattern never matches.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Automated simplify review of the secret-redaction/hook-secrets change:
- Redact events once at the RunEventSink::RedactSecrets node (skipping
entirely when no secrets are registered) instead of threading an
Option<SecretRedactor> into every sink leaf. This removes the full
serialize -> content-scan -> reparse pass that ran per event per fanout
leaf on the streaming hot path for runs with no declared secrets, and
deletes append_run_event_with_redactor from RunStoreBackend,
RunStoreHandle, LocalRunStoreBackend, and HttpRunStore (whose trait
default silently dropped the redactor).
- Collapse the foo/foo_with_redactor/foo_inner triples in fabro-sandbox
(default_redacted_output_tail, display_for_log, redacted_output_tail,
ExecResult methods) and fabro-workflow event redaction into single
signatures taking &SecretRedactor; an empty redactor is an identity.
- Drop the redundant run_id parameter from redacted_run_event (derived
from event.run_id).
- Fold HookRunner::new_with_secrets into HookRunner::new.
- Replace the hand-written decision_label map with strum::IntoStaticStr
on HookDecision, aligned with its serde tag per repo strum policy.
- Replace the duplicated 5-boolean reqwest error log blocks in the HTTP
hook executor with error = %e.without_url().
- Add HookType::interp_strings() as the single source of truth for
fire-time interpolatable fields; use it in secret_names_for_definition.
- Guard first_secret_name with InterpString::references to avoid a Vec
allocation per header per HTTP hook fire.
## Problem
Loading the web UI from a remote server took **~11 seconds to first
render on every refresh**. A HAR capture against a remote deployment
showed the page downloading **13.5 MB of JavaScript across 356 files,
uncompressed, on every single page load** — even though the assets are
content-hashed and served with `Cache-Control: immutable`.
Four compounding causes:
1. **`Pragma: no-cache` defeated the browser cache.** The
security-headers middleware stamped `Pragma: no-cache` onto every
response, including hashed assets that set a year-long immutable
`Cache-Control`. Browsers treat a response `Pragma: no-cache` as
`Cache-Control: no-cache` and check it *before* `max-age` (Chromium
zeroes freshness on it), and since assets carried no validators,
"revalidate" degraded into a full re-download. Empirically visible in
the HAR: Google-Fonts woff2s served from cache (`transfer = 0`) during
the same page load where all 356 of our assets re-downloaded in full.
2. **No response compression.** The server had no compression layer;
13.5 MB of JS compresses to ~2.5 MB with brotli.
3. **The HTML force-loaded every chunk.** `writeIndexHtml` emitted a
`<script type="module">` tag for all 356 outputs. Only 2.9 MB is
statically reachable from the entry; the other ~10.7 MB is
dynamic-import-only code (syntax grammars, Graphviz WASM, xterm, diff
file tree) that was being downloaded eagerly at high priority.
4. **The immutable heuristic over-matched.** Any dash in a filename
counted as a content hash, so stable-named files
(`pierre-diffs-worker/worker-portable.js`, `apple-touch-icon.png`) would
be pinned in browser caches for a year across deploys once fix 1 made
immutable caching effective.
## Changes
- **`security_headers`**: apply the `no-store`/`Pragma: no-cache`
defaults only when the handler didn't set its own `Cache-Control`. API
responses keep the conservative defaults.
- **Compression**: `tower-http` `CompressionLayer` (brotli + gzip) on
both the main router and the install-mode router (install mode serves
the same SPA bundle through a separate router). Default predicate keeps
SSE (`text/event-stream`), gRPC, images, and tiny bodies
identity-encoded. Quality pinned to `Precise(4)` — tower-http's default
defers to the codec default, and brotli's default is quality 11 (seconds
of CPU per multi-megabyte asset).
- **Entry-only HTML**: `writeIndexHtml` emits script tags only for `kind
=== "entry-point"` outputs. The module graph pulls static imports (depth
1, so no waterfall); dynamic `import()` chunks load on demand.
- **Cache-control classifier + validators**: only files matching the
bundler's actual output shape (`assets/<stem>-<hash8>.js|css`, lowercase
base-36) get `immutable`. Everything else is `no-cache` **with a strong
ETag** and `If-None-Match` → `304` support, so index.html / app.css /
the pierre worker revalidate in one cheap conditional request instead of
a full re-download.
## Impact (measured on the built bundle)
| | Before | After |
|---|---|---|
| Cold load, ~1 MB/s link | 13.5 MB raw ≈ **11–14 s** | ~0.8 MB
compressed eager payload ≈ **~1 s** |
| Refresh | full re-download, same 11–14 s | served from cache + one 304
≈ **instant** |
| Eager JS on first render | 13.56 MB / 356 files | 2.88 MB raw (0.79 MB
gzip) / 6 files |
## Verification
- 959 fabro-server tests pass (incl. new coverage); fmt + clippy clean;
`bun run typecheck` passes (the 5 pre-existing bun test failures
reproduce identically on `main` — missing `@pierre/diffs/dist/worker`
fixture + flaky InstallApp timing tests).
- New integration tests pin compression through **both** serving shapes
that matter: regular routes and the SPA fallback service, each via tower
`oneshot` **and** over a real TCP connection through hyper (raw-socket
assertions, so no client auto-decompression can mask a regression).
- Live-verified against a debug server: hashed assets get `immutable` +
brotli and no `Pragma`; mutable assets get `no-cache` + ETag and answer
conditionals with `304`; API responses keep `no-store`.
- Headless Chrome boots the rebuilt SPA from the entry-only HTML and
fully renders the UI.
## Notes for reviewers
- The ETag is skipped for immutable assets deliberately — they never
revalidate, so hashing multi-MB bodies per request would be pure
overhead.
- Install mode previously had **no** compression and shares the same
bundle; it gets the same layer via a shared `compression_layer()`
helper.
- `bun test` has a pre-existing suite (`production build copies Pierre
worker assets`) that fails without `@pierre/diffs/dist/worker` present
locally; unrelated to this change.
🤖 Generated with [Claude Code](https://claude.com/claude-code)
---------
Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
## Summary
Fixes the graph that was always being rendered as `left-to-right` even
when the workflow's `rankdir` is `top-to-bottom`
## Test plan
- [x] `bun run typecheck` (fabro-web)
- [x] `bun test` (fabro-web, full suite — 625 pass)
- [x] Manually load a run whose workflow declares `rankdir TB` and
confirm the graph renders top-to-bottom on first load, with the
toolbar's LR/TB buttons still working as manual overrides
🤖 Generated with [Claude Code](https://claude.com/claude-code)
---------
Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>
This change enables scrolling the stages sidebar on the run's
overview/stages page. Without it, for long runs with lots of stages, the
entire page scrolls, hiding the graph while it's running.
https://github.com/user-attachments/assets/c5405a5b-8480-46f8-8d7c-4cd4914f6228
---------
Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
## What
Adds the `organization_projects: write` permission to the GitHub App
manifest used when Fabro auto-creates a GitHub App, in **both** install
flows:
- `lib/crates/fabro-server/src/install.rs` (web-UI install)
- `lib/crates/fabro-cli/src/commands/install.rs` (CLI install)
A test assertion in the CLI install tests guards the new permission.
## Why
The GitHub Projects V2 tracker mints a scoped installation token
requesting `{ "issues": "write", "organization_projects": "write" }`
(`create_installation_access_token_for_projects`,
`fabro-github/src/lib.rs`). GitHub only lets an installation token
request a **subset** of the permissions the app was granted at install
time — and `organization_projects` was never in the manifest. So on any
auto-created Fabro app, the token request comes back **422** and the
tracker fails before it can make a single GraphQL call.
`issues: write` (also requested by that helper) is already covered by
the manifest; `organization_projects` was the missing piece.
## Note on rollout
Manifest `default_permissions` are applied at **app-creation time**, so
this only affects **newly** auto-created apps. Existing apps need the
permission added manually in their settings, and each installation must
approve it.
## Follow-up (not in this PR)
The `422` branch in `mint_installation_token_with_jwt` reports "GitHub
App does not have access to repository {repo}" — which misattributes a
missing-permission failure to repository access. Worth softening the
message to mention permissions too; left out here to keep this PR
focused on the scope change.
## Test
- `cargo nextest run -p fabro-cli --
manifest_includes_callback_urls_and_setup_url` passes.
🤖 Generated with [Claude Code](https://claude.com/claude-code)
Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
## What
Adds the `vulnerability_alerts: write` fine-grained permission to the
GitHub App manifest used when Fabro auto-creates a GitHub App, in
**both** install flows:
- `lib/crates/fabro-server/src/install.rs` (web-UI install)
- `lib/crates/fabro-cli/src/commands/install.rs` (CLI install)
`write` on `vulnerability_alerts` grants both read and write of
Dependabot alerts (write implies read for fine-grained permissions).
The two manifest builders are byte-for-byte identical by design, so both
are updated together. A test assertion in the CLI install tests guards
the new permission.
## Why
We need auto-created Fabro apps to be able to read and manage Dependabot
alerts.
## Note on rollout
Manifest `default_permissions` are applied at **app-creation time**, so
this only affects **newly** auto-created apps. Any app already created
won't pick this up automatically — the owner must add the permission in
the app's settings, and each existing installation must approve the new
permission request.
## Test
- `cargo nextest run -p fabro-cli --
manifest_includes_callback_urls_and_setup_url` passes.
🤖 Generated with [Claude Code](https://claude.com/claude-code)
Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
## Summary
Move server-managed environments from sibling TOML files into SQLite,
matching the storage model already used by variables and secrets.
This adds:
- an `environments` SQLite table with DB-level validation for IDs,
revisions, providers, network modes, booleans, and JSON fields
- a SQLite-backed `EnvironmentStore` with cached synchronous reads,
transactional create/replace/delete, synthetic unpersisted `local`, and
`default` as an ordinary seeded row users can delete
- one-time legacy import from `environments/*.toml` next to the active
server `settings.toml`, including relative Dockerfile path inlining and
backup rename to `environments.imported-<timestamp>.bak`
- install/test/CLI seeding of `default` directly into SQLite instead of
writing `environments/default.toml`
- docs updates for API/SQLite-managed server environments and legacy
import behavior
The REST API shape is unchanged; path Dockerfile sources remain rejected
over the environments API.
## Testing
- `cargo nextest run -p fabro-db -p fabro-environment` - 15 passed
- `cargo nextest run -p fabro-server --features test-support
environments` - 16 passed
- `cargo nextest run -p fabro-server --features test-support install` -
60 passed
- `cargo nextest run -p fabro-server --features test-support
create_run_rejects_disabled_sandbox_provider` - 1 passed
- `cargo nextest run -p fabro-server --features test-support
system_sandbox_provider` - 2 passed
- `cargo nextest run -p fabro-cli install` - 132 passed
- `cargo +nightly-2026-04-14 fmt --check --all`
- `cargo +nightly-2026-04-14 clippy --workspace --all-targets -- -D
warnings`
## What
Per-step environment in `run.prepare.steps[].env` was parsed and then
**dropped** before it reached the resolved run settings, so prepare
steps could never see their declared env. This PR carries that env all
the way through to the executor, resolves prepare-step interpolation at
the run boundary, and fixes an argv-quoting bug.
Three things:
1. **Per-step env is carried through.** `RunPrepareSettings` now holds
`steps: Vec<PreparedStep>` (command plus per-step `env`) instead of a
flat `commands: Vec<String>`. The per-step env reaches `exec_command`,
which already accepts per-command env vars, and is merged on top of the
base sandbox environment.
2. **Interpolation resolves at the run boundary.** Prepare-step
`script`/`command` and per-step `env` values are carried in source form
out of the portable config resolve layer (so `fabro validate` stays
portable and never requires env to be set). Their `{{ env.* }}` tokens
resolve in the process that actually runs the steps, via
`RunPrepareSettings::resolve_step_env` — mirroring the existing MCP
transport env resolution. A missing env var is a **hard error**
(fail-closed); there is no fallback to the unresolved literal.
3. **Argv is shell-quoted.** Argv-style prepare steps were assembled
with `join(" ")`, so an argument containing spaces or quotes was
re-split by the shell. They are now shell-quoted per element with the
shared `shell_quote()` helper. `script` steps stay verbatim because they
are raw shell snippets.
## How
- `RunPrepareSettings.commands: Vec<String>` becomes
`RunPrepareSettings.steps: Vec<PreparedStep>` where `PreparedStep {
command, env }`. The server-side `{{ vars.* }}` substitution pass now
walks each step's command and env.
- New `RunPrepareSettings::resolve_step_env(env_lookup)` resolves `{{
env.* }}` in each step's command and env values, returning a hard error
on a missing var (and a loud `Unavailable` error for reserved
`secrets`/`inputs` tokens).
- The run boundary (`fabro_workflow::operations::start`) gains
`runtime_setup_commands`, the prepare-step counterpart to
`runtime_mcp_server`. `LifecycleOptions` now carries `Vec<SetupCommand>`
(command + env), and the initialize phase passes each step's env to
`exec_command`.
- `resolve_prepare` shell-quotes each argv element and carries per-step
env in source form. The stale lint suppression on the resolved fields is
rewritten to describe the deliberate source preservation that now
resolves at the run boundary.
- The shell-quoting helper moves to a shared `fabro_util::shell` module
(backed by `shlex`); `fabro_sandbox::shell_quote` delegates to it so the
config resolve layer and sandbox code share one audited implementation.
- The OpenAPI `RunPrepareSettings` schema and the generated TypeScript
client are updated to the new `steps`/`PreparedStep` shape.
## Testing
- `cargo build --workspace`
- `cargo +nightly-2026-04-14 fmt --check --all`
- `cargo +nightly-2026-04-14 clippy --workspace --all-targets -- -D
warnings`
- `cargo nextest run` for `fabro-util`, `fabro-types`, `fabro-config`,
`fabro-sandbox`, `fabro-api`, `fabro-workflow`, `fabro-server`,
`fabro-cli` (provider keys stripped) — all green.
- `cd lib/packages/fabro-api-client && bun run typecheck` — clean.
New tests cover: per-step env carried through resolution; script/command
+ env resolved at the run boundary; a missing env var is a hard error
(in both the command and a per-step env value); reserved `secrets`
tokens surface as `Unavailable`; argv elements are shell-quoted (an arg
with spaces/quotes is correctly quoted) while a `script` stays verbatim;
and an end-to-end check that per-step env reaches the executed setup
command (with a negative control proving the success is attributable to
the per-step env).
🤖 Generated with [Claude Code](https://claude.com/claude-code)
---------
Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
## What
Makes hook interpolation typed end-to-end and fail-closed, and removes
the bespoke template engine on HTTP-hook headers.
- **Typed end-to-end.** Hook `command`, `url`, header values, `prompt`,
and `model` are now carried as a typed `InterpString` from the config
resolve layer all the way to the executor. The executor resolves each
segment at hook fire time from the typed value instead of collapsing it
to a `String` and re-parsing it. This mirrors the MCP transport env
resolution boundary (`resolve_transport_env` / `runtime_mcp_server`).
- **Narrow header tokens.** HTTP-hook headers previously ran through
MiniJinja with an env allowlist
(`TemplateContext::with_env_lookup_allowed`). They now resolve through
the same narrow `{{ ns.NAME }}` token resolver as every other hook field
— no template engine, no allowlist.
- **Fail-closed everywhere.** A missing or out-of-scope `{{ env.* }}` /
`{{ secrets.* }}` token in a command, URL, header, prompt, or model is
now a hard error that blocks the hook rather than firing it with a
half-resolved or empty value. Previously command hooks failed closed but
http/prompt/agent hooks failed open (warned and proceeded), which could
dispatch an HTTP request with an empty credential header or run an LLM
call against a half-rendered prompt. Transport-level outcomes (non-2xx
responses, connection errors, unparseable bodies) stay fail-open.
A follow-up cleanup commit removes the template engine's `env` namespace
(`with_env_lookup` / `with_env_lookup_allowed` / the `EnvLookup`
object), which the header path was the last consumer of.
## How
- `fabro-types` and `fabro-hooks` `HookType` / `HookDefinition` now type
the interpolatable fields as `InterpString`. `InterpString` serializes
as its raw source, so persisted run specs and checkpoints round-trip
unchanged.
- The `fabro-config` resolve layer clones the typed `InterpString`
through instead of calling `as_source()`, so the fields no longer leak
unresolved template text — the old "source preservation" `#[expect]`
annotations on the hook resolvers are gone.
- The executor's single `resolve_interp` helper resolves a typed
`InterpString` and is shared by the command, http, prompt, and agent
paths; resolution failure maps to `HookDecision::Block`, which the
runner already reports loudly (error for blocking hooks, warn for
non-blocking).
## Testing
- New unit tests: fire-time resolution from the typed value (no
re-parse), narrow-token header resolution, and fail-closed behavior for
HTTP url, HTTP header, and prompt hooks on a missing variable (the hook
does not fire and the resolution error surfaces).
- Existing hook tests updated and kept green.
- Gates: `cargo build --workspace`, `cargo +nightly-2026-04-14 fmt
--check --all`, `cargo +nightly-2026-04-14 clippy --workspace
--all-targets -- -D warnings`, and `cargo nextest run` for the touched
crates (`fabro-hooks`, `fabro-types`, `fabro-config`, `fabro-template`,
`fabro-workflow`, `fabro-server`, and the `fabro-cli` hook/config
tests), all green.
🤖 Generated with [Claude Code](https://claude.com/claude-code)
---------
Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
## What
Adds the **mcp-servers HTTP API**: `GET/POST /api/v1/mcp-servers` and
`GET/PUT/DELETE /api/v1/mcp-servers/{id}` on top of the merged
`fabro-mcp-store` foundation and OpenAPI spec.
This includes the AppState wiring needed for the catalog to work end to
end: `McpServerStore` construction from `{active-config-dir}/mcps/`, an
`AppState` accessor, the `fabro-server` dependency, and route
registration for list/create/get/replace/delete handlers.
The API mirrors the automations concurrency pattern with ETags on
read/write responses and required `If-Match` headers for replace/delete.
## Resolved before merge
- **Credential-omitting read model:** read responses now return
`McpServerView` / `McpTransportView`, so stored env/header values are
not exposed by GET/list/create/replace responses. Responses include only
`env_keys` / `header_keys`; persisted values remain available to runtime
execution.
- **Manifest catalog references:** run manifest validation, graph
rendering, preflight, and run creation now resolve server-managed MCP
catalog references such as `[run.agent.mcps.<name>] id = "..."`.
- **Schema strictness:** unknown MCP transport fields are rejected,
aligning the reused Rust domain type with the OpenAPI
`additionalProperties: false` contract.
- **Create response headers:** the `POST /mcp-servers` 201 response now
documents its `ETag` header in OpenAPI.
## Follow-up intentionally left out
Credential-literal validation remains structural only: create/replace
currently accept literal env/header values and persist them for runtime
use. The warn-vs-hard-reject UX is a separate follow-up for the settings
UI; it is not a response-omission issue.
## Testing
Current PR checks are green:
- Rust: format, clippy, generated docs, Linux tests
- TypeScript: build, test, typecheck
Local checks run during the simplify/CI-fix pass:
- `cargo +nightly-2026-04-14 fmt --check --all`
- `cargo +nightly-2026-04-14 clippy --locked --workspace --all-targets
-- -D warnings`
- `cargo nextest run -p fabro-config run_agent_mcps`
- `cargo nextest run -p fabro-mcp-store`
- `cargo nextest run -p fabro-api --test mcp_server_round_trip`
- `cargo build -p fabro-api`
- `cargo nextest run -p fabro-server --features test-support
system_sandbox_provider`
- `cargo nextest run -p fabro-server --features test-support --test it
mcp_servers`
---------
Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
## Summary
This moves workflow-visible variables from JSON file storage into
SQLite-backed storage, establishing the first durable SQL table while
preserving the existing variable API behavior.
## What Changed
- Added a `fabro-db` crate with bundled SQLite, an embedded migration
for the `variables` table, and a `Database` owner for `connect()`,
`migrate()`, `health_check()`, and pool access.
- Replaced the `fabro-variable` JSON file store with an async
SQLx-backed `VariableStore` that preserves sorted listing,
case-sensitive names, empty string values, name validation, and
description-preserving upserts.
- Wired server startup to create `<storage>/db/fabro.sqlite3`, run
SQLite migrations, import legacy variables when needed, and pass the
shared pool into server state.
- Grouped live server stores under `AppStores` so runs, variables,
vault, environments, and automations share one state boundary while
artifacts remain separate.
- Updated variable handlers, run creation, validation, and test support
for async SQLite-backed variable access.
- Added schema, store-level, legacy import, and API-level persistence
coverage for variables.
## Legacy JSON Migration
On startup, Fabro looks for `<storage>/variables.json`. If it is
missing, startup is a no-op for legacy variables.
If the file exists, Fabro parses and validates the full file before
mutating SQLite. Valid entries are inserted with `ON CONFLICT(name) DO
NOTHING`, so existing SQLite values remain authoritative and only
missing names are imported from the legacy file.
After a successful import transaction, the source file is renamed to a
timestamped backup such as `variables.json.imported-<timestamp>.bak`. A
later startup naturally skips the import because the original source
path no longer exists. Invalid JSON or invalid variable names leave the
source file in place for operator repair.
Variable values are not logged during import. Logs include only safe
metadata such as source/backup paths, row counts, and variable names.
## Verification
- `cargo nextest run -p fabro-db -p fabro-variable`
- `cargo nextest run -p fabro-server --features test-support variables`
- `cargo +nightly-2026-04-14 fmt --check --all`
- `cargo +nightly-2026-04-14 clippy --workspace --all-targets -- -D
warnings`
---
[](https://github.com/EveryInc/compound-engineering-plugin)
Generated with GPT-5 via [Codex](https://openai.com/codex)
## Summary
- Updates transitive Rust dependency `tar` from `0.4.45` to `0.4.46` in
`Cargo.lock`.
- Expected to resolve Dependabot alert:
https://github.com/fabro-sh/fabro/security/dependabot/30
- Dependency path: `fabro-sandbox` -> `tar`.
## Grouping
- Kept this separate from the web alerts because it is a Rust
lockfile-only patch with a separate verification path.
## Verification
- `cargo tree -i tar` resolves `tar v0.4.46`.
- `cargo build --workspace`
- `cargo nextest run --workspace` (6860 passed, 185 skipped; nextest
reported 1 leaky test warning as non-fatal)
- `git diff --check`
## Residual alerts
- React Router alerts 31-37 are intentionally handled in a separate web
PR.
Co-authored-by: Release Repro <release-repro@example.com>
