LockError::Task is only constructed while waiting for the refresh
sidecar lock, so "auth store lock" pointed at the wrong file.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
AgentPermissions duplicated fabro_types::PermissionLevel: same variants,
same kebab-case wire form, same crate. PermissionLevel is strictly richer
(Hash, strum, clap::ValueEnum) and is already the with_replacement target
for the OpenAPI PermissionLevel schema, whose values are identical to the
AgentPermissions schema this branch deletes.
Delete AgentPermissions and type the [cli.exec.agent] permissions setting
as PermissionLevel. This drops the adapter match in `fabro exec` and the
`as AgentPermissionLevel` alias that existed only to tell the two names
apart. The TOML wire form is unchanged.
Removing run.agent.permissions also changed the serialized run spec, but
two fabro-cli inline snapshots still carried "permissions": null. They
failed on this branch and passed on main. Accept the updated snapshots.
Also tighten the removed-setting test to assert the exact unknown-field
message, rename its module to run_agent now that it covers more than
fabro_tools, and drop three doc references to the removed setting.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Review pass over the reuse change. No intended behavior changes.
- share one definition of the initial generation from fabro-types instead
of three copies across fabro-types, fabro-agent, and the supervisor
- give each child one SubAgentHandle instead of threading the supervisor's
state, callback, and notification sender through five functions, and
collapse the repeated signal-then-drain pairs into publish()
- move `reusable` inside SubAgentStatus::Finished so a closed agent can no
longer be marked reusable
- clear the lifecycle draining flag with an RAII guard, so one panicking
callback cannot silence every later lifecycle event
- tear down a session that failed to initialize right away rather than
holding it and its sandbox until the parent closes the agent
- look agents up through SupervisorState::agent/agent_mut instead of five
copies of the same not-found error
- drop the unreachable cleanup_started branch and the test-only emit_event
whose only caller was its own test
- render subagent starts from one ProgressEvent and one display method,
deriving the spawn/turn distinction from the generation
- set projected subagent status through one helper instead of four
identical reducer arms
- drive the generation-pinned wait test through spawn/send_input rather
than hand-writing private supervisor state
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The staleness check treated "different from the token that failed" as
"usable". A long-lived process could read an entry a sibling rotated an
hour earlier, whose access token had since expired, install it, and
return Ok. The caller retries once and does not refresh again, so that
surfaced a 401. Require the stored token to be unexpired; an expired one
now falls through and rotates with the refresh token just read.
Also:
- Give the non-Unix `acquire_refresh_lock` a no-op passthrough, matching
the other lock helpers off Unix. Returning an error there broke
re-installing a stored dev token, which needs no lock because it never
writes.
- Rename `Client::refresh_lock` to `local_refresh_lock`. Two different
locks were sharing one word four lines apart.
- Gate `LockError::Task` on Unix, where its only construction site is.
- Give the concurrency test a no-proxy transport connector. Building
clients without one goes through `connect_target_transport`, which
does not disable proxy discovery, against localhost.
- Assert the rotated refresh token reaches the store, which is the
invariant behind single-use rotation.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The cross-process refresh lock added a third copy of the open-file,
try-lock, then block-on-contention sequence. Collapse all three into
one `open_locked_file` helper parameterized by `LockMode`, which
removes `open_lock_file` and `lock_error`.
Lock calls are now qualified as `FileExt` calls throughout, since
`std::fs::File` has inherent locking methods with different return
types that take precedence over trait methods.
Also give `acquire_refresh_lock` one signature on all platforms by
defining `RefreshLockGuard` for non-Unix targets too, instead of
returning `Result<(), _>` there and `Result<RefreshLockGuard, _>` on
Unix.
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>
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>
Copilot review flagged two backward-compatibility breaks with events
persisted by pre-model-keyed releases; both are stored data that can
never be rewritten, so accept the old shapes on read:
- FailoverProps: original_provider/original_model/attempt are Option
again with serde defaults. New events always set them; failover events
recorded before model-keyed fallbacks lack them. Restores the
historical-event test.
- RunModelSettings: temporary custom deserializer accepts the legacy
flat-array fallbacks shape inside stored run.created events, keying
the chain under the requested model name when one is set. Remove once
pre-0.311 run logs are out of the support window.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Consolidation pass over the fallback feature, no intended behavior
changes beyond noted validation and event-shape cleanups:
- Unify the two parallel notice types: FallbackPlanNotice is gone;
ModelFallbackNotice now owns the runtime NoNearbyReasoningLevel case
and the shared ChainEmpty wording. Notices emit through a new
Emitter::notice_scoped with their own level, and each distinct notice
is emitted once per run instead of on every LLM call.
- Move canonical_model_id onto Catalog so chain keys are written and
read through one function; reject provider-qualified fallback keys,
which could never match at dispatch and were silently dead config.
- Type FallbackTarget as ProviderId/ModelId, removing repeated
ProviderId::new re-wrapping at every use site.
- Derive FallbackPlan's current route from a position index instead of
storing current/requested_controls copies; advance() no longer has
unreachable None branches.
- Bundle the agent invocation's live state (session, bridge, lease,
forwarder, accounting) into LiveAgentInvocation; failover_agent_session
drops from 21 parameters to 7 and the six copies of the
abort/discard/classify teardown collapse into two methods.
- Share one route_request builder between one_shot and its failover
loop; complete_one_shot_request takes the request by value instead of
deep-cloning the message payload per call.
- Event::Failover carries FailoverProps directly; the props' original
route and attempt fields are now required, and reasoning efforts are
typed ReasoningEffort instead of strings.
- Reuse RunModelSettings/RunModelControls in fabro-api via
with_replacement, add the missing controls property to the OpenAPI
schema, regenerate the TS client, and add the type-identity/JSON
parity test.
- Smaller cleanups: ReasoningEffort::closest_supported uses enum
discriminants; ModelFallbackPolicy gains len(); resolve_model_fallbacks
takes a provider slice; duplicate-target filtering lives only in the
resolver.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Addresses review feedback on the fallback notice work.
A provider-only fallback such as `openrouter` needs the primary model's
catalog entry to find the closest capability match. When the primary is
itself a passthrough selector there is no entry, so every provider-only
candidate was skipped with "provider `X` has no compatible model" even
when that provider had plenty. Adds a `PrimaryNotInCatalog` notice that
names the missing primary instead of blaming the provider.
Also from review:
- `code()` was a wildcard fallthrough, which docs/internal/events-strategy.md
forbids for new variants. Now exhaustive.
- `NoConfiguredOffering` discarded the `providers` list that
`NoEligibleOffering` hands it. The notice now names the providers that do
offer the model.
- `ModelFallbackNotice::reference` was a rendered `String`; it is now the
`ModelRef` it came from, which also drops the per-candidate double
allocation the previous refactor introduced.
- `ResolvedStartLlm` unpacked and repacked `ResolvedFallbackChain`
field-for-field; it now holds it directly.
- Added `FallbackTarget: Display` as `provider:model`, replacing two
hand-written `"{}:{}"` format strings.
- `Catalog::select` still inlined the `require_provider` body.
- Emission moved to `ModelFallbackNotice::emit_all`, covered by a new test
proving notices reach the event stream with the right level, code, and
message. Nothing tested that hand-off before.
Documented in `resolve_fallback_chain` why an unknown provider stays a hard
error while an unconfigured one is skipped, and that an unqualified unknown
selector pins to the primary's provider.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The doc comment promised blank names were ignored, but the guard only
rejected the empty string. Stylesheet selectors match class names exactly,
so a padded name would sit in `classes` and match no rule.
No current caller can pass one: the parser splits on whitespace, and the
subgraph and import paths strip everything but alphanumerics and hyphens.
This makes the public contract on the shared type match what it claims.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Follow-up cleanup on the portable fallback chain work.
- Add `Catalog::require_provider` and `Catalog::provider_id`, replacing the
`catalog_provider_id` free function in `start.rs` and two copies of the same
`provider(..).ok_or_else(UnknownProvider)` block inside the catalog.
- Add `FallbackTarget::new` and use it for the six struct literals that each
stringified a provider and model by hand.
- Extract per-candidate resolution into `resolve_fallback_candidate`, returning
a `FallbackCandidate` that is either a target or the skip reason. This flattens
`resolve_fallback_chain` from four levels of nesting to one loop and splits the
qualified/unqualified model arms into separate match patterns.
- Drop the `seen` HashSet and its per-candidate key clones in favor of a
`contains` check on the chain being built; fallback chains hold a handful of
entries.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
One row per separator rule, so a regression names the input that broke
instead of pointing at a combined fixture string. Also record why
`add_class` keeps insertion order: `fidelity` falls back to the first class
for the thread ID.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Node classes were built in two places. The parser split the `class`
attribute on commas and whitespace, but the import transform re-split the
raw attribute on commas only. A space-separated class on an import
placeholder became a single class name, so stylesheet rules did not match.
That included the `class="fast shared"` example in the imports docs.
- add `Node::add_class`, replacing the duplicate append helpers in
`SemanticState` and `ImportTransform`
- read `node.classes` in `placeholder_config` instead of re-parsing the raw
attribute, so class splitting happens in exactly one place
- name the separator rule `split_class_attr`, splitting on commas and then
whitespace so empty entries need no trimming
- drop the unused `Node::class` accessor that invited the re-parse
- keep the comma-compatibility note in the DOT attribute reference only
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
- ExecStreamingRequest: drop #[non_exhaustive] and the six Option-taking
builder setters; call sites use struct literals over ::new(), matching
GrepOptions/WalkOptions, and providers can destructure exhaustively
- Docker: pass ExecStreamingRequest through docker_exec_shell_streaming
instead of seven positional args; revert the no-op StartExecOptions
- Daytona: stdin temp-file cleanup is now best-effort (mirrors
DaytonaSession::close) so a failed delete cannot fail a completed
command or double-delete from Drop; upload overlaps session creation;
one shared DAYTONA_CLEANUP_TIMEOUT
- write_process_stdin tolerates ConnectionReset/ConnectionAborted so a
command that stops reading stdin does not fail on TCP Docker daemons
- Local sandbox aborts the stdin writer after process exit instead of
joining unbounded
- Cap stdin_source payloads at 10 MiB, mirroring the for_each bound
- Add Node::context_key_attr() tri-state so the handler and lint rule
share one definition of a valid context-key attribute
- inert_attribute canonicalizes handler types via StageHandler, fixing
false warnings for command attrs on tool nodes
- Share resolve_flat_context_value between command stdin and for_each;
resolve_json_value takes Value by value, removing a deep clone
- Reuse MockSandbox in command handler stdin tests instead of extending
SpySandbox with a hand-rolled streaming override
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Addresses three Copilot review comments on #653.
`item_label` comes from a model or a workflow author, and it reaches the
terminal through the CLI progress display. A label could carry ANSI
escapes, newlines, or bidi overrides and rewrite what the operator sees.
It could also be whitespace-only, giving a branch a blank identity.
Add `text::sanitize_display_label`: strip ANSI sequences, drop control
and bidi-reordering characters, trim, and elide past 80 characters.
Return an empty string when nothing printable survives so callers fall
back to an identity they control.
Apply it where the label is created, so events, the store, and the web
UI all get a clean value instead of each consumer having to remember.
`parallel_branch_display` sanitizes again, because a run recorded before
this commit still has raw labels in its event log.
`emit_branch_retrying` now sets `stage.retrying`'s `index` from the
branch stage's execution ordinal, matching the envelope `stage_id` and
the meaning every other emitter gives that field. The branch's position
in the fan-out is already on `parallel.branch.started`.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Simplification pass over the for_each branch. No behavior change.
Share what was duplicated:
- Node::prompt_or_label replaces the "prompt, else label" fallback that
agent, prompt, and the for_each item injector each wrote out.
- context::lookup_flat replaces the "exact key, then strip context."
lookup that condition.rs had twice and the for_each source had again.
- is_llm_handler_type replaces the inline agent/prompt match, so the
runtime and the for_each_contract rule agree by construction.
- find_join_node now takes branch ids, so a for_each fan-out passes its
template target instead of needing find_join_for_target.
- collect_events moves to test_support; parallel and integration tests
shared one copy already.
- One ScriptedHandler replaces four test handlers that differed only in
what they returned.
Straighten the branch retry loop:
- Reserve the branch scope once before the loop instead of guarding it
with an Option, which removes three expect() calls.
- acquire_branch_permit and backoff_or_cancel replace the cancel-aware
select! blocks the loop repeated verbatim.
- Keep the match arms in Executor::execute_with_retry order so the two
loops stay easy to compare.
Drop redundant state:
- BranchPlan::is_for_each derives from template_target_id.
- for_each_contract checks node type before source shape, so one
mistake reports one diagnostic.
Prove the new wire fields survive the OpenAPI boundary: the fabro-api
round-trip fixture now carries index and item_label.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
