Add DisplaySafeUrl under fabro-util::redact so URL Display and Debug output redact credentials by default. Migrate token-bearing GitHub, OAuth, server, LLM, sandbox, and workflow paths to use the wrapper at logging/error boundaries while keeping raw URLs explicit for wire and shell transit.
Color the install URL and add a separate block that prints the install
token on its own line so users can copy it without parsing the query
string. The token block shows in both the URL and reverse-proxy branches.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Add fabro-static::EnvVars as the shared registry for fixed environment variable names and migrate env reads, clap env bindings, and subprocess/test allowlists to use it.
Add clippy bans for raw std::env lookup APIs so future dynamic env facades must be documented explicitly.
Rewind now creates a resumable replacement run from the selected checkpoint, archives the source run, and records run.superseded_by for auditability. Fork, rewind, and timeline listing now share server-backed git-store plumbing, with generated API clients and docs updated for the new contract.
Reuse the existing merge strategy type across CLI/API/GitHub paths, consolidate repeated PR command setup, and serialize server-side PR creation per run to avoid duplicate external work.
Two code-reuse findings from the simplify review:
1. PullRequestGithubContext carried owner/repo String fields obtained by
re-parsing record.html_url, even though PullRequestRecord already
carries typed non-optional owner/repo fields. Dropped the redundant
fields; the 3 PR handlers read via &ctx.record.owner /
&ctx.record.repo instead. The incidental non-github.com URL
rejection is preserved as an explicit one-line host-validation
call (documented by the rejects_non_github_record_url tests).
2. RunPrInputs held run_spec: &RunSpec purely to read goal()
downstream. Narrowed to goal: &str stored directly; the server
handler passes inputs.goal to OpenPullRequestRequest::from_run_state,
which no longer needs the full RunSpec. Fewer fields, clearer
dependency at the call site.
Also tightened the from_run_state doc comment (was narrating peer
callers' behavior rather than the method's contract).
Verified: workspace fmt clean, clippy --all-targets -D warnings clean,
cargo nextest run --workspace 4581 passed, 182 skipped.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
MergeMethod derives serde(rename_all = "snake_case"), so json!({
"merge_method": method }) emits the same `"squash"` / `"merge"` /
`"rebase"` strings as the as_str() round-trip. Inlining the typed value
removes the only remaining manual string conversion in the merge path.
Verified: workspace fmt clean, clippy --all-targets -D warnings clean,
cargo nextest run --workspace 4581 passed (fabro-github merge_pr unit
tests still pass — they assert against status codes not payload bytes,
but the twin-mode integration test create_merge_and_verify_state
exercises the on-the-wire JSON shape).
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The 4 callers of create_completed_run_ready_for_pull_request all paired
it with pr_test_app(...) and used identical defaults for base_branch
("main"), run_branch ("fabro/run/42"), and diff. Only repo_origin_url
varied per test. Bundle into pr_test_app_with_completed_run(token,
github_base_url, repo_origin_url) -> (state, app, run_id); each call
site shrinks from 12 lines to 1 helper invocation.
Verified: workspace fmt clean, clippy --all-targets -D warnings clean,
cargo nextest run -p fabro-server 439 passed.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Add an async sibling helper that bundles state + app + a fresh
create_run(&app, MINIMAL_DOT) into one (state, app, run_id) tuple.
Updated the 2 PR tests that had built this triple manually
(merge/close not_found_when_record_missing). The third holdout at
line 10148 keeps its own setup — it has an intervening
assert_eq!(state.github_api_base_url, github.base_url()) that
documents a load-bearing invariant about app state construction.
Verified: workspace fmt clean, clippy --all-targets -D warnings clean,
cargo nextest run -p fabro-server 439 passed.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Public functions now take ctx: &GitHubContext<'_> instead of by-value
GitHubContext<'_>. Matches the surrounding &str / &GitHubCredentials
convention. The type stays Copy so internal call sites that pass `ctx`
through still work without explicit reborrows.
Touched: 8 fabro-github functions + matching _with_client variants,
plus call sites in fabro-server, fabro-workflow, fabro-sandbox, and
fabro-github's integration + unit tests. Pure mechanical change.
Verified: workspace fmt clean, clippy --all-targets -D warnings clean,
cargo nextest run --workspace 4581 passed, 182 skipped.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Session keeps llm_client: Client as its internal model — a session is
bounded (≤ 1 hour) and its cached client stays fresh within that
window. Session::new(client, ...) remains the primitive (used by the
server-mediated agent adapter path in fabro-cli/exec.rs, which builds
a Client with a custom ProviderAdapter, no source involved).
Add Session::from_source(source, ...) for callers that hold a source
directly — resolves a Client via Client::from_source and delegates to
new. Lets workflow-level callers that store Arc<dyn CredentialSource>
build a Session without hand-resolving first.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The "Anthropic uses x-api-key header, everyone else uses Bearer" logic
was written three times: env_source (env-based construction), resolve
(vault-based construction), and provider_auth (CLI key validation).
Any future header rename would need three edits.
Add ApiCredential::from_api_key(provider, key) as a canonical
constructor. Each callsite now builds via the helper and overrides only
the fields specific to its path (env base URLs, vault-sourced org/project
IDs, codex mode, etc.).
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
build_pr_body and maybe_open_pull_request now take the two things they
actually need — run_store: &RunStoreHandle and llm_source: &dyn
CredentialSource — instead of services: &RunServices. The workflow
PULL_REQUEST phase decomposes services at the callsite; the standalone
fabro pr create command passes its own directly.
This removes RunServices::for_cli, a stub constructor that fabricated
an emitter, sandbox, and provider just to satisfy the RunServices type
for two fields it cared about. The "leaky fake" is gone.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Add a pr_test_app(token, github_base_url) -> (state, app, run_id)
helper that bundles the create_github_token_app_state +
build_router(...) + fixtures::RUN_1 triple every PR-endpoint test
shared. Updated 15 call sites; the 3 tests that derive run_id from
create_run(&app, MINIMAL_DOT).await keep their own setup since they
need the app before the run_id exists.
Verified: workspace fmt clean, clippy --all-targets -D warnings clean,
cargo nextest run --workspace 4581 passed, 182 skipped.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Both fields were derivable from run_options (run_options.run_id and
run_options.git.as_ref().and_then(|g| g.run_branch.clone())), so they
were a second place to keep in sync with the canonical source.
Drop both from Concluded, populate Finalized's copies from run_options
at the pull_request phase boundary. Add RunOptions::run_branch() helper
so the "reach into optional git opts" pattern reads as a single call.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Abandoning `fabro pr list`. Deletes:
- lib/crates/fabro-cli/src/commands/pr/list.rs
- lib/crates/fabro-cli/tests/it/cmd/pr_list.rs
- PrListArgs struct + PrCommand::List variant + dispatch arm + name
- The `client()` accessor + `client` field on ServerSummaryLookup
(`pr list` was the only consumer)
- `### fabro pr list` section in docs/reference/cli.mdx
- `list` row + alias from the `fabro pr --help` snapshot test
Server side untouched: there was no `/pull_requests` endpoint to
remove. Historical changelog and plan docs left as-is — they record
when the command shipped, not its current existence.
Verified: workspace fmt clean, clippy --all-targets -D warnings clean,
cargo nextest run --workspace 4581 passed (down from 4584 by the
3 deleted pr_list tests), 182 skipped.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The for_test helper was a second layer of indirection — EngineServices
::test_default() called it, and it was the only caller. Inlining
collapses two test-scaffolding functions into one. The thread+runtime
scaffolding stays (it's still needed because create_run is async and
tokio tests can't block_on directly), just moves up one level.
Also drops the StubCredentialSource struct at module scope; it moves
inside test_default() since that's its only use.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The helper had only one meaningful caller after the previous diff/conclusion
validation collapse. Inlining keeps the diff-validation message + error code
in the same place as the rest of RunPrInputs::extract's validation branches.
RunPrInputs is already grouped with the other PR helpers in server.rs.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Two cleanups:
1. Extracted the 8 sequential let-Some-else-return validations from
create_run_pull_request into a server-local RunPrInputs struct with
an extract(&run_state, force) -> Result<RunPrInputs, ApiError>
constructor. The handler shrinks from ~85 lines of validation +
build to a single match RunPrInputs::extract(...) followed by
creds + model + request build. All error codes/messages preserved.
2. Deleted is_app_public from fabro-github plus its 3 unit tests and
the now-unused MockHeaderCheck::Missing / with_req_header_missing
test-helper variants. No production caller remained after the
server-side install flow stopped checking app visibility client-side.
Verified: workspace fmt clean, clippy --all-targets -D warnings clean,
cargo nextest run --workspace 4584 passed (down from 4587 by the 3
deleted is_app_public tests), 182 skipped.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Two cleanups:
1. Threaded GitHubContext through the remaining fabro-github functions
that pair credentials with the API base URL: branch_exists,
resolve_clone_credentials, resolve_authenticated_url. Each loses its
trailing `base_url: &str` and replaces `creds: &GitHubCredentials`
with `ctx: GitHubContext<'_>`. is_app_public was skipped — it doesn't
take credentials. Updated production callers in fabro-sandbox/daytona
and fabro-workflow/sandbox_git, plus integration and unit tests.
2. Added OpenPullRequestRequest::from_run_state on the workflow struct.
Bundles the validated unpacked-from-RunState pieces into a draft PR
request with the server's defaults (`draft = true`, `auto_merge =
None`). Server's create_run_pull_request handler now calls the
constructor instead of inlining a 12-field struct literal — the
handler reads as a sequence of validations followed by one named
request build, not as plumbing.
Verified: workspace fmt clean, clippy --all-targets -D warnings clean,
cargo nextest run --workspace 4587 passed, 182 skipped.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Three cleanups in one pass:
1. Bundle GitHub creds + base URL into a GitHubContext<'_>:
Defined in fabro-github and threaded through create_pull_request,
enable_auto_merge, get_pull_request, merge_pull_request, and
close_pull_request (plus their _with_client variants). Each function
loses its trailing `base_url: &str` and replaces `creds:
&GitHubCredentials` with `ctx: GitHubContext<'_>`. Bundle propagates
into OpenPullRequestRequest as a single `github` field instead of
the prior split `creds` + `github_api_base_url`.
2. Delete dead CommandContext::storage_dir() and ::server_settings():
Origin added these for client-side PR commands that no longer exist
after the server-side migration. Field `server_settings` removed
from CommandContext (only the deleted method read it). Same field
pruned from ResolvedCommandSettings; one test that verified the
underlying loader behavior was rewired to read LoadedSettings
directly via load_resolved_settings_from_toml.
3. Audit *_error helpers in fabro-server: no remaining single-use
factories. The previous inlining pass left a tidy surface. No diff.
Verified: workspace fmt clean, clippy --all-targets -D warnings clean,
cargo nextest run --workspace 4587 passed, 182 skipped.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Two surface cleanups:
1. maybe_open_pull_request now takes one OpenPullRequestRequest<'_>
struct instead of 12 positional args. The struct lives next to the
function (matches *Options pattern in pipeline/types.rs); fields are
named so call sites read top-down — eliminates the wall of
strings/bools that the workflow pipeline, server handler, and tests
were passing positionally. Renamed `base_url` -> `github_api_base_url`
so it doesn't read like a sibling of `base_branch`.
2. transport_error in cli/exec.rs replaced two substring matches
(`message.contains("fabro auth login") || message.contains(
"Authentication required.")`) with `exit::exit_class_for(err) ==
Some(ExitClass::AuthRequired)` — refresh_access_token already attaches
ExitClass::AuthRequired via .classify(), and main.rs uses the same
structural check.
Verified: workspace fmt clean, clippy --all-targets -D warnings clean,
cargo nextest run --workspace 4587 passed, 182 skipped.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Reconciles 61 origin commits (settings/config architectural reshape:
sparse layers → dense snapshots via builders, WorkflowSettings rename,
RunLayer/CliLayer moves, workflow builders, drop of public load wrappers)
with our LLM credential + RunServices refactor.
Our architecture preserved where it conflicted with origin's:
- RunServices / EngineServices stay (services.rs does not exist on
origin, which inlined the fields onto Initialized). Origin's new
Initialized fields (inputs, run_store, emitter, sandbox, registry,
env, dry_run, llm_client, provider) are absorbed through RunServices
and EngineServices instead of being inlined.
- llm_source: Arc<dyn CredentialSource> stays on AppState and
RunServices. Origin had a parallel ProviderCredentials struct in
fabro-server; our CredentialSource trait is more general and
complies with docs-internal/llm-client-resolution.md. Point-of-use
Client::from_source(...) rebuild preserves OAuth refresh.
- CommandContext.llm_source() uses self.storage_dir (origin's direct
field) instead of self.machine_settings (our side's field, removed
by origin).
- standalone_llm_source in fabro-agent drops the dead Result wrap and
uses fabro_config::user::default_storage_dir (origin's entrypoint)
instead of the removed load_settings_user/resolve_storage_root.
Absorbed from origin wholesale:
- SettingsLayer → WorkflowSettings rename everywhere
- Dense run settings: RunOptions.settings is WorkflowSettings, inputs
read via settings.run.inputs directly (not Option<RunLayer>)
- AppState.manifest_run_defaults / manifest_run_settings
- fabro_config re-exports of CliLayer/RunLayer/CliOutputLayer/etc.
- Lifecycle terminal-event changes, finalize dedup, list_events
consolidation — already brought in on the previous merge, kept
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Merge origin's fabro-config types boundary refactor (dense settings
migration: WorkflowSettings/UserSettings/ServerSettings moved to
fabro-types; SettingsLayer made pub(crate) inside fabro-config) into
local PR-refactor branch.
Conflict resolution intent:
- lib/crates/fabro-cli/src/commands/pr/{create,mod}.rs — kept HEAD's
server-side PR command implementations; origin still carried the
pre-refactor client-side helpers (build_github_credentials,
load_pr_record, branch_exists pre-check) that local commits had
already migrated to the server.
- lib/crates/fabro-cli/src/user_config.rs — took origin's resolution
(load_resolved_settings_from_toml + storage_dir_from_document tests),
which implements the same dead-storage_dir-wrapper cleanup local had
done via local_server::storage_dir.
- lib/crates/fabro-server/src/server.rs — kept HEAD's PullRequestRecord
import alongside origin's added ServerSettings; rewrote test helpers
github_token_settings + create_github_token_app_state to use origin's
ServerSettingsBuilder + AppStateConfig dense-settings shape (replaces
HEAD's parse_settings_layer + Arc<RwLock<SettingsLayer>>); switched
RunSpec.settings fixture from SettingsLayer::default() to
WorkflowSettings::default() per origin's RunSpec retype.
- Suppressed dead_code on CommandContext::storage_dir() and
::server_settings() (added by origin for use by client-side PR
commands that no longer exist after local's server-side migration);
gated load_resolved_settings_from_toml on cfg(test).
Verified post-merge: workspace fmt clean, clippy --all-targets
-D warnings clean, cargo nextest run --workspace 4587 passed,
182 skipped.
is_not_found_error now takes &anyhow::Error and uses api_failure_for to
discover the HTTP status structurally — works on errors after
map_api_error/classify_api_error rather than only on the raw progenitor
variant. Call site at delete_store_run inverts to map first, then check.
Inline seven single-use error factories at their sole call sites:
no_stored_pull_request_error, pull_request_already_exists_error,
missing_repo_origin_error, missing_base_branch_error,
missing_run_branch_error, run_not_finished_error,
run_not_successful_error.
Keep github_pull_request_not_found_error (3 call sites) and
empty_pull_request_diff_error (2 call sites) as named helpers.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
EnvCredentialSource::credential_for hardcoded "ANTHROPIC_API_KEY",
"OPENAI_API_KEY", etc. in match arms, while configured_providers read
the same names from Provider::api_key_env_vars(). Renaming any env var
required editing both sites.
Pull the primary key lookup from api_key_env_vars() so the Provider
enum owns the env-var-name → provider mapping. Provider-specific extras
(ANTHROPIC_BASE_URL, OPENAI codex mode, etc.) stay inline — they aren't
about the API key itself.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
POST /runs/{id}/pull_request was calling fabro_github::branch_exists to
distinguish a missing head ref before invoking maybe_open_pull_request.
GitHub's POST /pulls already returns 422 for an unknown ref, so the
pre-check was an extra round-trip on the happy path (and would race a
concurrent branch deletion anyway).
Drop the check plus the now-unused missing_remote_branch_error helper;
let GitHub's validation error bubble up as BAD_GATEWAY. Replace the
(owner, repo) binding with an if-let Err on parse_github_owner_repo_from_url
since we only needed it for the unsupported_host validation side-effect.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Attach ApiFailure to classified anyhow errors via a transparent
TaggedFailure source wrapper (mirrors fabro-util's Classified pattern),
so callers can discover HTTP status + structured code via downcast
without parsing error strings.
add_pr_upgrade_hint now branches on api_failure_for(&err) — appending
the upgrade hint only when the server returned a 404 with no structured
code (i.e. progenitor's unstructured "route not found"). Structured 404s
with a code like "no_stored_record" pass through unchanged.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Replace hand-built 409 response that inlined {errors,pull_request} with
a regular ApiError::with_code(CONFLICT, ..., "pull_request_exists") and
drop the optional pull_request field that had been added to the
ErrorResponse OpenAPI schema solely to carry the existing record.
Clients receiving a 409 can GET /runs/{id}/pull_request to retrieve the
stored record when they need it — the detail string still includes the
existing html_url, which is the field most clients branch on.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The `sleep_inhibitor` feature pulled in 20 pedantic/nightly lints that
CI (default features) never exercised. Narrow all `pub` items in the
module to `pub(crate)`/`pub(super)`, replace the `use
super::iokit_bindings::*` wildcard with explicit imports, use `&raw
mut` for FFI pointer borrows, drop the always-`Some` wrapping in
`DummySleepInhibitor::acquire`, and bring `crate::sleep_inhibitor`
into scope at the three call sites so they don't trip
`clippy::absolute_paths`.
Verified: `cargo +nightly-2026-04-14 clippy --workspace --all-targets
--all-features -- -D warnings` clean, `cargo nextest run --workspace
--all-features` 4563 tests passed.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
run_retro fetched list_events twice — once for stage_durations and
again for run_retro_agent's payload. Load once at the top and reuse.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Command::Parallel entries were previously flattened into the same
sequential for-loop as Shell/Args, defeating the devcontainer spec's
parallel-safe guarantee. Extract a run_shell helper and dispatch on
Command kind: Shell/Args await one command, Parallel uses try_join_all.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
compute_final_patch (up to 30s git diff) and write_finalize_commit
(network push to meta branch) are independent — run via tokio::join!
so worst-case wall time is max(diff, push) instead of their sum.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Reverses the #[cfg(test)] gating on RunServices::with_run_store /
with_emitter / with_sandbox / with_cancel_requested — manager_loop and
parallel handlers have production callers that were unpacking 7 fields
into locals just to reconstruct RunServices::new(...).
manager_loop builds its child via
parent_run.with_run_store(...).with_cancel_requested(None).
parallel builds each branch via parent_run.with_sandbox(...).
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Introduce PullRequestApiError with a structured NotFound variant and an
Other(String) catch-all for non-classified failures. Update
get_pull_request, merge_pull_request, and close_pull_request to return
the new type so callers can branch on shape rather than substring.
Server PR handlers now match Err(PullRequestApiError::NotFound { .. })
to map a missing GitHub PR to the existing github_pull_request_not_found
ApiError, removing three err.contains("not found") substring checks.
The Display impl for NotFound preserves the prior message format
("Pull request #N not found in owner/repo") so logging and the
catch-all BAD_GATEWAY response keep their human-readable text.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
