The AWS access key ID is not a secret — swap its PasswordInput for a
regular text input so operators can read and edit it directly.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Add a Supported platforms table to the quick-start so Intel Mac and
Windows users learn they're unsupported before running the installer.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
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>
Apply the five findings from the external review: reject archived sources
with 409 (was contradictory); emit RunSupersededBy only on archive success
(was self-contradicting with the ordering rationale); look up working_directory
from the run's RunSpec instead of hand-waving AppState.repo_path; plumb
superseded_by through RunSummary + OpenAPI to honor the 'helps fabro ps' claim;
reconcile test scenarios to the archive-first ordering.
Also add GET /runs/{id}/timeline to Unit 2 so --list display moves server-side
alongside the mutating rewind call (web-UI parity). Normalize all status
codes from 412 to 409 to match fabro-server's CONFLICT convention.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Record the five decisions from the targeted Unit 2 review: 207 Multi-Status
for archive-failure partial success, graceful-degradation mapping for TOCTOU
precondition races, archive-first event ordering, accept-orphan retry posture,
and a new superseded_by projection field. Also add spawn_blocking and
operations-layer composite guidance from the review's autofixes.
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>
