A Daytona "Sandbox state change in progress" rejection surfacing
through the pipeline lifecycle path ("Pipeline lifecycle operation
failed") matched no transient-infra hint, so the run failure was
categorized deterministic. The condition is a provider lifecycle
transition that finishes on its own — the definition of transient
infrastructure — and the deterministic label misinforms retry
machinery and anyone reading the failure.
Add two transient-infra hints: the provider rejection ("state change
in progress") and the bounded-wait timeout an activation reports when
a stop transition outlives its budget ("sandbox stop still in
progress").
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Sandbox exec logs previously required command_len fingerprinting to tell a
push from a credential refresh or a checkpoint commit. The shared git
helpers now instrument their futures with a git_op span, so Daytona's and
Docker's `exec_command: entered` lines inherit the operation label and the
log renders as `git_op{op=push}: exec_command: entered timeout_ms=...`.
Ops: push (git_push_via_exec), refresh-credentials (both providers'
refresh_push_credentials), checkpoint-commit (checked_git_checkpoint),
fetch (fetch_source_run_ref), and metadata-push (the run-metadata snapshot
write). Spans are attached with #[tracing::instrument] — attached to the
future, never an entered() guard held across an await — so they follow the
task across worker threads. No trait or signature changes.
Plan: .ai/plans/git-push-token-resilience.md (PR 3: item 10).
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Run 01M0DH033P2XSTHAGVBHG6922F completed 2.8 hours of work, then failed
terminally because four consecutive publish pushes hit GitHub's
token-replication lag (404 "Repository not found") — the push path had no
retry, the failure was misclassified as deterministic, and the same
fresh-mint-then-push pattern silently disabled metadata snapshots. This
generalizes the clone retry machinery to pushes and makes attempt detail
durable.
- clone_retry -> git_retry: the classifier's boolean becomes a
CredentialContext derived from the token snapshot (fresh App tokens retry
404s as replication lag, mature ones as transient infra, static
credentials fail fast), and the attempt/backoff limits become a RetryPlan
with layered optional bounds. Clone behavior is preserved: Docker keeps
its absolute five-minute deadline, Daytona keeps no deadline.
- Pushes take a scoped CredentialLease before the first attempt: it owns
the embed mutex for the whole operation, pins the single successful
resolve, retries only failed resolves, falls back to the last embedded
token when a mint fails, and force-re-embeds the pinned token once after
the first auth-shaped failure (drift repair). The margin invariant
(REFRESH_MARGIN > every push plan's max_elapsed) guarantees the pinned
token outlives the operation; a unit test asserts it.
- Sandbox::git_push_ref now takes a RetryPlan and returns PushReport /
PushError with per-attempt records (classification, redacted output tail,
token generation/provenance/age, credential action, refresh errors).
Checkpoint pushes use a 90-second budget; the terminal publish push gets
5 attempts over at most 4 minutes.
- The single durable git.push event per push gains a nested attempts array
(GitPushAttemptProps, token snapshot flattened to flat fields); stored
events without it still deserialize. Publish push failures now carry an
explicit failure category — exhausted transient retries stay
transient_infra instead of deterministic — plus one bounded cause line
per attempt and the last successful push time in the message.
- Metadata snapshot degradation records why it degraded: push failures with
retryable classifications leave the writer eligible to re-probe at each
later checkpoint, and a successful snapshot clears the degraded state and
re-arms the warning. Permanent failures keep today's latch.
Plan: .ai/plans/git-push-token-resilience.md (PR 2: items 1, 2, 4, 7 and
the metadata re-probe).
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Every push previously re-minted a fresh GitHub App installation token and
embedded it in the origin URL, so pushes routinely landed inside GitHub's
token-replication lag window (run 01M0DH033P2XSTHAGVBHG6922F failed
terminally on four consecutive fresh-token 404s). Reusing mature tokens
removes the failure trigger and saves two GitHub API calls plus one sandbox
exec per push.
- New fabro_github::token_source::InstallationTokenSource: one cached,
single-flight source per origin repo. Static credentials pass through
(generation 0); App credentials mint through the cache and reuse tokens
until REFRESH_MARGIN (10 min) before expiry. Every resolve returns a
non-secret TokenSnapshot (generation + Minted/Reused/Static provenance),
and the source logs mints at INFO and reuses at DEBUG.
- Docker and Daytona share the source through PushCredentialState: an embed
mutex serializes compare -> set-url -> record, a matching generation skips
the set-url exec, and the generation is recorded only after a successful
exec. The clone still mints its own token, but now seeds the source cache
(generation 1) and the last-embedded state, so a refresh mint failure
falls back to the known embedded token instead of believing nothing was
ever embedded.
- RefreshOutcome now reports the remote action (embedded/unchanged/none)
separately from the token snapshot; git_push_via_exec logs token age and
provenance with each push, and refresh failures log the last embedded
generation.
- The run-metadata writer resolves through the sandbox's shared source
instead of minting per snapshot (with its own cached source on resume).
- The ACP refresh-ahead loop reschedules from the embedded token's
expires_at minus the margin instead of a fixed 45-minute interval, which
a cached source would have broken for long turns; static credentials stop
the loop.
Plan: .ai/plans/git-push-token-resilience.md (PR 1: items 3 and 6).
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
`RunSpec` has 13 fields and no `Default`, so every test that needed one
spelled out all 13 even when it cared about one or two. That put 64
hand-rolled `RunSpec { .. }` literals in `lib/`, and made a single
additive field cost a mechanical edit at roughly 30 sites.
Add `test_run_spec()` to `fabro-types`' feature-gated `test_support`
module: fixed `fixtures::RUN_1`, default settings, a minimal `test`
graph, `test_run_provenance()`, and every optional field unset. Tests
now spread it and only spell out what they assert on.
Adopt it at the 13 literals where the spread removes real duplication,
including the crate-local `test_run_spec` helpers in `fabro-store` and
`fabro-workflow`, which are now defined in terms of the shared fixture.
Tests that populate every field on purpose — the exhaustive `RunSpec`
serde round-trip in particular — keep spelling it out.
No production code and no behavior changes.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
reference_kind_for_attribute returned the full ReferenceKind, which
includes the config-sourced Dockerfile kind the classifier can never
yield, so the shared graph walker carried a silent `continue` and an
`unreachable!` for impossible kinds; each new config-sourced kind widens
those filler arms, and a classifier extension that reuses an existing
kind would be dropped by the walker without validation, visitation, or a
compiler error. Return a GraphReferenceKind subset instead (converting
into ReferenceKind for validation), making the walker's matches total
with every arm meaningful.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Config {{ env.NAME }} interpolation was removed workspace-wide (tokens
still parse only to fail with a migration message), but several doc
comments and the server-secrets strategy doc still presented it as a
live mechanism, including run goal file paths where the new
workflow-version validation now makes the contradiction user-visible.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
materialize_blob_ref checked is_local_execution for every blob
reference, but the sandbox and run directory are invariant across a
resolution pass, so each check after the first was a redundant (and on
Docker/Daytona, remote) round-trip. The check is now memoized in a
per-pass SandboxLocality threaded through resolve_execution_value.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Finish the blob-hash vocabulary unification at the defining signatures:
RunStoreBackend::read_blob, RunStoreHandle, LocalRunStoreBackend, the
HTTP backend impl, RunDatabase::read_blob, and BlobStore::read/exists
all said `id`, which kept re-teaching the old vocabulary at every impl
site and inlay hint.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Move the static-reference vocabulary out of fabro-workflow so every
consumer shares one definition: ReferenceKind, AttributeScope, and
reference_kind_for_attribute land in fabro-types::graph, and
validate_static_reference plus a new visit_graph_references walker land
in fabro-template. The manifest bundler drops its ad-hoc graph scan and
walks references through the shared walker.
Unifying the walkers forces three semantic alignments, each matching
what the engine actually executes rather than what the old scanners
happened to match:
- stack.child_dotfile is no longer classified as a child-workflow
reference; the engine never resolved it as one.
- import and stack.child_workflow only count at node scope; graph- and
edge-level occurrences were scanned but never executed.
- @@-escaped goals flow through the shared walker's escape handling
instead of the bundler's own prefix stripping.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Structural cleanup of the durable pull request creation feature, from a
three-agent review (reuse, quality, efficiency) of the branch:
- Move the supervisor out of handler/ into server/pull_request_supervisor.rs,
collapse its double bookkeeping into one task-id map, and fold the five
copy-pasted failure arms into attempt_pull_request_creation.
- Tag pull_request.failed events with the creation id they resolve, so a
publish-stage failure can never fail an unrelated explicit creation. The
reducer gains PullRequestCreation::succeed/fail transition methods.
- Scan pending creations through a narrow projection-cache accessor instead
of materializing every run summary, raise the scan interval to 30s (notify
covers the live path), and cap retries for runs whose worker cannot even
record a failure.
- Answer "creation already pending" POSTs before taking the per-run create
lock, which a worker can hold for the whole creation.
- Replace the hand-rolled per-run lock map with fabro_store::KeyedMutex.
- Reuse cheap Arc'd projections (cached_run_projection) on the poll endpoint
and in the worker instead of deep-cloning run summaries and diffs.
- Merge ExistingPullRequest into fabro_github::CreatedPullRequest and
extract one reconcile_existing_pull_request helper for both call sites.
- Give the client poll loop a 15-minute deadline; document that Retry-After
and the poll interval are the same constant.
- Resolve a wedged pending creation (run already has a pull request) as a
durable failure instead of skipping it forever.
- Tests: shared wait_for_pull_request_creation helper, a pinned generation-
failure assertion, and a new pipeline test proving reconciliation adopts
an existing PR without an LLM call or create request.
Verified: cargo build --workspace, cargo nextest run --workspace (7,767
passed), nightly clippy -D warnings, fmt --check, insta (no pending), bun
typecheck in fabro-api-client.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The manifest builder's best-effort pre-run push converted every result
into a PreRunPushOutcome that was serialized into GitContext, expanded
into five OpenAPI union arms, and generated into API clients — but no
production path ever read it; every field read was a test.
Delete the concept while preserving the behavior:
- Drop the PreRunPushOutcome enum and GitContext.push_outcome from
fabro-types; GitContext keeps origin_url, branch, optional sha, and
dirty, which remain real execution inputs and provenance.
- Rename the manifest outcome builder to push_manifest_branch_best_effort,
a side-effect-only helper with the same decision rules: skip without an
origin, skip on configured-repository mismatch, skip when the branch is
already synced, otherwise push noninteractively and discard the result
without failing manifest creation or logging raw Git stderr.
- Prove the push through repository state instead of the deleted enum: a
branch ahead of a local bare origin is pushed during manifest build, a
mismatched configured repository is not, and a failing remote helper
still cannot fail manifest creation.
- Remove push_outcome from GitContext in OpenAPI, delete the five-arm
union schemas, and drop the fabro-api type replacement and re-export.
- Keep one regression proving historical run.created events with a nested
push_outcome still deserialize through ordinary unknown-field tolerance
and reserialize to the reduced shape. No migration or event rewrite.
Old JSON carrying the removed field stays readable. Newly generated
clients omit a field older servers required, so new-client-to-old-server
compatibility is intentionally not promised for this pre-1.0 contract.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Second pass, from the remaining review findings.
- Wrap the stall watchdog in a `StallWatchdog` type. The call site kept
two parallel `Option`s derived from the same condition and threaded out
an `Option<(CancellationToken, JoinHandle<()>)>`. `monitor_for_stall`
also took two same-typed `CancellationToken` params pointing opposite
directions, where swapping them compiles and yields a run that silently
never stalls.
- Rename `WorkflowAgentQuestionRuntime::stage_id` and
`PendingAgentQuestionBatch::stage_id` to `node_id`. They hold
`node.id`, and the previous commit put them two lines from
`stage_scope.stage_id()`, which returns a real `StageId`.
- Widen the two real-time interview tests. `node_timeout_excludes_
human_input_wait` allowed 20ms of active work against a 50ms budget,
which is tight enough to flake under parallel nextest load. The blocked
wait still outruns the timeout, so both still fail if the pause
regresses.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
One conflict, in StructuredOutputError::repair_message. main (#709)
added a `previous_error` parameter and richer validation-error
rendering; this branch had replaced the inline expectation match with
OutputSchemaKind::expectation().
Resolved by keeping both: main's new signature and section assembly,
calling schema.expectation() for the expectation text. The method
already supersedes main's inline match and carries this branch's intent
of embedding the resolved JSON Schema instead of naming it.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Follow-up cleanup on the human-input timeout work.
- Drop the `unresolved_interviews` counter from `InterviewBlockState`. It
duplicated `blocked_stages`, which is non-empty exactly when the run is
blocked.
- Publish block state before emitting `run.blocked` / `run.unblocked` in
both directions, so a listener reading `subscribe()` from an event
callback never sees state that disagrees with the event. The watchdog
still gets a full fresh deadline because it restarts on the unblock
transition.
- Stop panicking in `InterviewBlockState::resolve`. It runs from `Drop`,
where a panic during unwind aborts the process.
- Replace the emitter's `activity_revision` watch channel with a
monotonic timestamp. `record_activity` runs on every agent stream
delta, and the channel woke the watchdog task and re-armed its timer
per event. The watchdog now samples `last_activity()` when its deadline
fires and re-arms only if the run was active, so the hot path is one
clock read and one relaxed store.
- Remove the now-unused `last_event_at()` and `epoch_millis()`.
- Collapse the duplicated blocked/unblocked `select!` arms in
`monitor_for_stall` and `timeout_excluding_interview_wait` into one
loop each, using a branch precondition to park the timer while blocked.
- Handle a dropped block-state sender in
`timeout_excluding_interview_wait` by falling back to a plain deadline
instead of panicking, which also removes a potential busy loop.
- Only compute `stage_id` when the node actually has a timeout.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Both contract helpers dispatched on OutputSchemaKind, so they belong on
the type. Moves expectation() and agent_prompt() into an impl block and
drops the free functions.
Splits the combined agent test: assertions no longer run inside the
backend's run(), where a failure surfaces as a panic from execute().
Adds coverage for the Routing branch of the contract, which was
previously untested.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Resolve the model catalog table conflict in docs/public/core-concepts/models.mdx
by keeping both changes: this branch's `kimi` -> `moonshot` provider rename for
the Kimi rows, and main's new DeepSeek V4 rows.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Addresses Copilot review feedback on the repeated-failure check.
serde_json runs with preserve_order, and jsonschema builds the
additionalProperties `unexpected` list by walking the instance in
document order. So the same leftover keys emitted in a different order
produced a different Vec and compared as a different problem, which
suppressed the "unchanged from your previous repair" nudge.
Sorting at capture also makes the MAX_UNEXPECTED_PROPERTIES truncation
pick the same subset every time instead of an order-dependent one, and
stabilizes the rendered message.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Path safety now lives in one place. The NUL-byte and drive-letter rules
move from a server-only helper into the store's own filename validation,
so uploads reject those paths at write time instead of only the ZIP read
path catching them. The download still re-checks, because artifacts
stored before the rule existed can still carry an unsafe path, but it
now skips a bad path rather than failing the whole archive.
Promote is_boundary_stage to RunProjection and drop the three identical
private copies. The ZIP download used a node-name match instead, which
would have dropped artifacts from a working node that happened to be
named "start".
Compress the archive. Entries were Stored while the response was also
excluded from transfer compression, so text artifacts moved at full
size. async_zip gains the deflate feature; async-compression and flate2
were already in the lock file.
Log archive failures unconditionally. The send-succeeded guard meant a
client that had already disconnected left no record at all, which is the
case where the log is the only evidence.
Also: collapse the duplicate 500 arms, drop the dead stage-ID tiebreaker
and the cached order in the selection map, name the accessible label
after the visible one, share the run URL prefix between the two download
href builders, and document the mid-stream truncation behavior in the
OpenAPI description.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Follow-up review of the repair-error work. Behavior is the same or better;
the machinery is smaller.
Fixes a false "unchanged from your previous repair" nudge. same_problem_as
fell through to `_ => true`, so any two non-Required issues at the same
instance path, schema path and keyword compared equal. A model that removed
one unexpected property and added another was told it had changed nothing.
SchemaValidationIssue already derives PartialEq, so the 17-line comparison
is now `previous.contains(issue)`.
Drops the hand-written Type and Enum rendering. jsonschema already renders
both, and its messages name the offending value, which the hand-written
ones did not. Also switches masked() back to to_string(): masking replaced
the bad value with a placeholder, working against the goal of an actionable
message, and buys no privacy since the full response is already in the
prompt.
Resolves the schema fragment when the issue is captured rather than
threading Option<&OutputSchemaKind> through rendering. That reverts the
command.rs change and drops the test-only messages() shim. The fragment is
now attached only to Other, where it adds information; for required, type,
enum and additionalProperties it just repeated the prose.
Also: caps the model-controlled unexpected-property list so a wide object
cannot turn the repair prompt into megabytes; drops evaluation_path, which
was dead except under $ref, where it printed a pointer that does not
resolve; drops the keyword field, already named by the schema path; and
records the previous error only after the agent session accepted the
repair, since failover rebuilds the session from the original prompt.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The parallel stage summary rendered a Duration tile directly below
StageMetaBar, which already shows the same stage's duration with a live
ticking clock and a started-at tooltip. The two disagreed while running:
the meta bar counted up, the tile showed the static word "running". The
cancelled-stage bug lived only in the duplicate.
Drop the tile. The meta bar owns duration for every stage renderer, and
it was already correct for cancelled, pending and skipped stages. That
removes the three-way duration branch, the "--" sentinel decode, and the
ACTIVE_STAGE_STATES and formatDurationMs imports.
With the tile gone, ParallelOverview.durationMs is dead, as were
successCount, failureCount and isComplete — the renderer counts the
branch rows it draws. ParallelOverview reduces to branch identity.
For run event write failures, log the first at error with run_id and
event name, the rest at debug, and summarize new losses at flush. A
broken sink fails for every event, so a bare error would emit one
"investigate me" line per event for the life of the run.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
