Commit graph

1491 commits

Author SHA1 Message Date
Bryan Helmkamp
8aeaef5f33 fix(install): mount wizard at root route 2026-04-24 12:59:16 -04:00
Bryan Helmkamp
533785cd4c fix(install): expose local object store root 2026-04-24 12:38:04 -04:00
Bryan Helmkamp
7d791c531b fix(install): use single-line install token input
Refresh the embedded SPA assets so the Rust bundle serves the updated install screen.
2026-04-24 12:20:17 -04:00
Bryan Helmkamp
68c9ddec5e
build(spa): refresh embedded web assets
Regenerate the tracked SPA bundle so the TypeScript build job's refresh-and-diff gate stays clean.
2026-04-24 12:02:39 -04:00
Bryan Helmkamp
4215ed3c16
refactor(workflow): make rewind fork and archive
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.
2026-04-24 11:42:37 -04:00
Bryan Helmkamp
23a317d4a1
test(server): align PR endpoint test with LLM credential source 2026-04-24 11:31:04 -04:00
Bryan Helmkamp
baa20ca375
Merge remote-tracking branch 'origin/main' 2026-04-24 11:23:34 -04:00
Bryan Helmkamp
0a297b26bf
refactor(pr): simplify server-side PR plumbing
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.
2026-04-24 11:17:01 -04:00
Bryan Helmkamp
9ede01698c
refactor(server): narrow RunPrInputs to goal; drop PullRequestGithubContext.{owner,repo}
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>
2026-04-24 10:38:55 -04:00
Bryan Helmkamp
f19c0e2e91
test(fabro-llm): remove trybuild compile-fail test 2026-04-24 09:30:55 -04:00
Bryan Helmkamp
16184a6f96
refactor(github): drop merge_method.as_str() shim in PUT /pulls/:n/merge body
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>
2026-04-24 09:23:18 -04:00
Bryan Helmkamp
961cb4c0c9
refactor(server): pr_test_app_with_completed_run for create-PR happy-path tests
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>
2026-04-24 09:20:26 -04:00
Bryan Helmkamp
c4404fc931
refactor(server): pr_test_app_with_minimal_run for create_run holdouts
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>
2026-04-24 09:17:54 -04:00
Bryan Helmkamp
12ca595f54
refactor(github): take GitHubContext by reference in public API
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>
2026-04-24 09:15:25 -04:00
Bryan Helmkamp
896f0bb8ad
refactor(agent): add Session::from_source convenience constructor
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>
2026-04-24 09:08:37 -04:00
Bryan Helmkamp
b4bc9a0506
refactor(auth): extract ApiCredential::from_api_key
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>
2026-04-24 09:05:54 -04:00
Bryan Helmkamp
7e83cd38e5
refactor(pipeline): split build_pr_body signature; drop RunServices::for_cli
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>
2026-04-24 09:02:13 -04:00
Bryan Helmkamp
042fca0dec
refactor(server): add pr_test_app fixture for PR endpoint tests
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>
2026-04-24 09:00:30 -04:00
Bryan Helmkamp
b01e08a52d
refactor(workflow): drop Concluded.run_id and pushed_branch
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>
2026-04-24 08:57:50 -04:00
Bryan Helmkamp
336553e30f
remove(cli): drop pr list command entirely
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>
2026-04-24 08:56:07 -04:00
Bryan Helmkamp
35763dc437
refactor(workflow): inline RunServices::for_test into test_default
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>
2026-04-24 08:54:56 -04:00
Bryan Helmkamp
58ce594f72
refactor: inline empty_pull_request_diff_error into RunPrInputs::extract
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>
2026-04-24 08:48:43 -04:00
Bryan Helmkamp
1781788798
refactor: extract RunPrInputs validation; drop dead is_app_public
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>
2026-04-24 08:39:58 -04:00
Bryan Helmkamp
60e2027ac3
refactor: extend GitHubContext to remaining callers; add OpenPullRequestRequest::from_run_state
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>
2026-04-24 08:31:14 -04:00
Bryan Helmkamp
176c0c0159
refactor: GitHubContext + drop dead CommandContext methods
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>
2026-04-24 00:54:11 -04:00
Bryan Helmkamp
cc51d8c396
refactor: collapse maybe_open_pull_request args; route auth detection through Classified
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>
2026-04-24 00:15:31 -04:00
Bryan Helmkamp
4e9dd6fb52
Merge origin/main (preserving RunServices architecture)
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>
2026-04-24 00:10:01 -04:00
Bryan Helmkamp
600720423c
Merge remote-tracking branch 'origin/main'
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.
2026-04-24 00:01:09 -04:00
Bryan Helmkamp
15d5cac09b
refactor: simplify is_not_found_error and inline single-use ApiError factories
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>
2026-04-23 23:47:21 -04:00
Bryan Helmkamp
817ff40cec
refactor(auth): drive env_source key lookup from Provider metadata
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>
2026-04-23 23:45:29 -04:00
Bryan Helmkamp
45d0cc4415
refactor(server): drop TOCTOU branch_exists pre-check on PR create
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>
2026-04-23 22:53:31 -04:00
Bryan Helmkamp
233d47391d
refactor(client): replace add_pr_upgrade_hint substring match with tagged failure
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>
2026-04-23 22:51:05 -04:00
Bryan Helmkamp
0bf2dd30ef
refactor(server): collapse 409 conflict into standard ApiError
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>
2026-04-23 22:45:29 -04:00
Bryan Helmkamp
e65d9a92e2
fix sleep_inhibitor lints under --all-features clippy
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>
2026-04-23 22:45:07 -04:00
Bryan Helmkamp
c806de16f3
perf(workflow): load event log once in retro
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>
2026-04-23 22:37:30 -04:00
Bryan Helmkamp
9881d69d6a
perf(workflow): run devcontainer Command::Parallel concurrently
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>
2026-04-23 22:37:25 -04:00
Bryan Helmkamp
6b5e89a6f1
perf(workflow): parallelize final patch and finalize commit
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>
2026-04-23 22:37:19 -04:00
Bryan Helmkamp
7c95f6fa8e
refactor(workflow): use RunServices builders for child engines
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>
2026-04-23 22:37:13 -04:00
Bryan Helmkamp
1ea72ca2e8
refactor(workflow): adopt Emitter::notice in artifact lifecycle
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
2026-04-23 22:37:05 -04:00
Bryan Helmkamp
7d63e2c298
refactor(github): replace stringly-typed PR errors with typed enum
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>
2026-04-23 22:31:23 -04:00
Bryan Helmkamp
906868c30c
drop unused test-support feature from fabro-config
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>
2026-04-23 22:29:29 -04:00
Bryan Helmkamp
b9911fc6e7
Merge origin/main
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>
2026-04-23 21:46:24 -04:00
Bryan Helmkamp
f46803b367
refactor(api): unify PR detail types via with_replacement
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>
2026-04-23 21:41:11 -04:00
Bryan Helmkamp
099dd881a8
merge origin/main into local main
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>
2026-04-23 21:13:28 -04:00
Bryan Helmkamp
827bd72af2
refactor(workflow): gate RunServices builder helpers to tests
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
2026-04-23 21:03:29 -04:00
Bryan Helmkamp
ca56c15f2f
refactor(llm): return Self from Client::from_source
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>
2026-04-23 21:03:21 -04:00
Bryan Helmkamp
9a0b64e53c
fix(workflow): reuse parent credential source in sub-workflows
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>
2026-04-23 21:03:12 -04:00
Bryan Helmkamp
65533f486b
refactor(auth): move auth_issue_message to resolve
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
2026-04-23 21:03:05 -04:00
Bryan Helmkamp
f0fadffb5e
apply rustfmt to settings consumer tests
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>
2026-04-23 21:02:02 -04:00
Bryan Helmkamp
c973550e4e
refactor(pr): simplify post-refactor PR command code
- 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>
2026-04-23 20:59:32 -04:00