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>
SettingsLayer::{test_default, ensure_test_auth_methods} are `pub(crate)`
and only called from fabro-config's own in-crate tests, but their impl
block was gated on `cfg(any(test, feature = "test-support"))`. No
external crate enabled the `test-support` feature, so under
`--all-features` the methods compiled in without reachable callers and
clippy flagged them as dead code. Narrow the gate to `cfg(test)` and
drop the vestigial feature entry.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Reconciles origin's "emit terminal event from FINALIZE" refactor
(41c47dbe1, e8a89ac39, 904c8842f) with the local RunServices refactor.
finalize() now performs origin's single list_events walk for stage
durations + artifact count, origin's compute_final_patch, deduped
stages/billing via billing_from_checkpoint, and origin's terminal event
emission — but reads run_store/sandbox/emitter from the shared
RunServices instead of individual Retroed fields. services.emitter.notice
replaces origin's local emit_run_notice helper.
test_support's execute_and_emit_terminal (added by origin) now accesses
run_store/emitter via executed.engine.run.* since Executed bundles
EngineServices. execute/tests.rs drops the terminal-event status
assertion origin deleted — status is no longer set at EXECUTE end.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Move PullRequestDetail, PullRequestGithubDetail, PullRequestUser,
PullRequestRef, and MergeMethod into fabro-types. Register them as
fabro-api with_replacement targets so the OpenAPI client and the server
share one canonical type per concept.
PullRequestDetail composes a stored PullRequestRecord with a flattened
PullRequestGithubDetail mirroring GitHub's REST payload, removing the
hand-rolled pull_request_detail_json builder in the server. Change the
PullRequestRef wire field from `ref_name` to `ref` so the same Rust
type round-trips through both GitHub and our API without aliases.
The server now uses fabro_api::types::{Create,Merge,Close}* directly,
deleting the hand-defined request/response shadows and the
`body.method.parse::<...>()` call (the typed MergeMethod enum drives
deserialization). Drops fabro-cli's `i64::try_from(record.number)`
panic path and the AutoMergeMethod enum (replaced by MergeMethod).
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Integrates origin's worker-JWT-auth work (commits 8a6f83bb0..c847a828d)
with the config-boundary refactor that landed locally. Conflicts
resolved:
- commands/dump.rs: take origin's removal of the 500-line in-process
test block (replaced by real-server integration coverage).
- commands/run/runner.rs: keep local's dense WorkflowSettings import,
drop dead SettingsLayer import, pull in origin's ActorRef.
- manifest_builder.rs: adopt origin's lifted working_directory
resolution (fixes#159 - manifest git detection in nested repos),
but via local's resolve_working_directory_from_run API that takes
the dense RunNamespace. Update the regression test's
ManifestBuildInput literal to local's run_overrides/cli_overrides
field shape.
- server.rs: keep origin's jwt_auth_mode/jwt_auth_state/
test_user_subject/issue_test_user_jwt/issue_test_worker_token/
create_run_with_bearer/bearer_request test helpers, adapt
jwt_auth_state to local's create_test_app_state_with_session_key
signature (ServerSettings + RunLayer), keep local's dense
canonical_origin_settings that returns ServerSettings via
server_settings_from_toml. Rewrite
build_app_state_requires_session_secret_for_worker_tokens against
the dense AppStateConfig (resolved_settings +
resolved_runtime_settings_for_tests).
Post-merge verification: workspace builds clean, cargo +nightly
fmt --check all clean, cargo +nightly clippy --workspace
--all-targets -- -D warnings clean, cargo nextest run --workspace
4560 tests passed.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Let callers decide whether to wrap in Arc. Also consolidates the two
state() fetches in build_pr_body into one.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Sub-workflows were hardcoding Anthropic + EnvCredentialSource instead of
inheriting the parent run's provider and source, so vault-only auth and
non-default providers silently broke inside manager_loop.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Three test modules drifted to non-canonical style during the post-merge
CI fixup; reformatting brings them back in line with the pinned nightly
rustfmt config so `cargo +nightly-2026-04-14 fmt --check --all` is clean
again.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
- Parallelize `pr list` discovery loop via buffer_unordered; thread RunId
through the stream to drop the run_id.parse().expect(...) panic path.
- Skip computing the default model in create_run_pull_request when the
request already supplies one (common path from `fabro pr create`).
- Delete the dead user_config::storage_dir wrapper (test-only, zero
callers, stale deprecation note); point its tests at
local_server::storage_dir directly.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
- Return the existing PullRequestRecord on 409 from POST /runs/{id}/pull_request
so structured clients can recover the URL/number without a follow-up call.
The response now includes both the error envelope and a pull_request field
(same shape precedent as /install/finish's leftover_env_keys).
- Add server tests for merge/close error paths: 404 no_stored_record, 400
unsupported_host, 503 integration_unavailable, 400 invalid_merge_method, 502
github_not_found.
- Add a dedicated regression test proving the PR handlers use the
github_api_base_url captured at AppState construction, not a request-time
env read (SSRF defense invariant from the plan).
- Add an upgrade hint on unstructured 404s from the new PR client methods so
a new CLI against an old server sees "Upgrade the fabro server" instead of
an opaque failure.
- Refresh the stale CLI docs paragraph so it describes server-side GitHub
credentials, matching the post-refactor reality.
- Regenerate the TypeScript API client (had fallen behind the prior OpenAPI
schema additions) and add PullRequestRecord to ErrorResponse as an optional
field for the 409 case.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The view, merge, and close handlers each repeated the same ~25-line
prologue: open the run reader, load state, unwrap the stored record,
parse owner/repo with the host check, and load GitHub creds. Move it
into `load_pull_request_github_context` so each handler keeps only the
work that's unique to it. Net -31 lines with no behavior change.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
After Unit 3.1 of the config boundary refactor, fabro-types no longer has
any #[derive(Combine)] sites — the fabro-macros dep is unused. Likewise
`Duration as DurationLayer` was an artifact from when layer and vocabulary
types lived side-by-side; the resolved Duration type has no Layer form now,
so the alias was misleading.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Return `&'static str` from the mock provider's `name()` and drop
redundant `.to_string()` calls on `github.base_url()` (already owned).
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
FINALIZE loaded the run event log twice: once via build_conclusion_from_store
for stage durations, then again to count ArtifactCaptured events. Merged into
a single walk feeding both the conclusion and the artifact count.
Collapsed six near-identical pipeline::execute + emit_terminal + flush blocks
in test_support into one execute_and_emit_terminal helper. Also trimmed
narrative comments that described caller ordering, control flow, or the fix
commit rather than non-obvious invariants.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
