WorkflowVersion::new serialized the whole version just to enforce the
size limit and threw the bytes away, the store re-serialized them to
write the blob, and every read re-serialized a third time for the
canonicality comparison. Cache the canonical bytes on the struct at
construction (skipped during serde) and expose them as an infallible
borrow; the now-unconstructable InvalidShape store error variant goes
away with it.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The field's shape (WorkflowPath -> WorkflowVersionId) can only ever
hold pinned child workflow versions, so the generic name was squatting
on a word a future non-workflow dependency kind (a pinned model, tool,
or data snapshot) would want. Since the field name is part of the
canonical bytes that version IDs hash, this rename is only possible
before the first version is stored — claim the specific name now.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The method iterates EnvironmentImageLayer configs, but out of context
"image layers" reads as Docker image layers — a bad collision in
exactly the domain where it appears. Name it for what it yields.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The manifest bundler and workflow-version validation each maintained
their own walk over the graph reference vocabulary (goal files, inline
goal/prompt templates, imports, child workflows, @-file attributes), so
a new reference-bearing attribute had to be added twice or the two
would drift. Add fabro_template::visit_graph_references as the single
per-graph walker — it validates that file references are template-free
and emits typed events — and rewrite both consumers on top of it,
keeping their own IO, resolution, and recursion.
Same for dockerfiles: SettingsLayer::image_layers[_mut] is now the one
definition of where images live in a settings layer, replacing the
three hand-rolled traversals in the run compiler, manifest bundler, and
workflow-version validation.
Behavior note: the manifest bundler now also follows
stack.child_dotfile (already in the shared vocabulary and treated as a
child-workflow reference by workflow-version validation); nothing in
the engine emits or reads that attribute today.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Move the WorkflowVersion wire type and its structural invariants (file
count and size limits, entrypoint presence, unique keys, path
collisions, canonical form) into fabro-types, so fabro-api replaces the
generated schema type without pulling graph parsing or the template
engine into every API consumer. The wire shape is unchanged.
fabro-workflow-version keeps the expensive graph/config/template
validation behind a ValidatedWorkflowVersion newtype and now owns
WorkflowVersionStore: put only accepts validated versions and get still
re-validates blobs read from shared storage. fabro-store goes back to
being domain-agnostic persistence.
Move the static-reference attribute vocabulary (ReferenceKind,
AttributeScope, reference_kind_for_attribute) to fabro_types::graph and
template-syntax validation to fabro-template, so fabro-graphviz no
longer depends on the template engine.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
- Validate WorkflowVersion structure once at construction so
canonical_bytes only serializes and enforces the size limit
- Collapse the three pairwise path-collision loops into one over the
combined file and dependency keys
- Make WorkflowPath::is_ancestor_of allocation-free and remove unused
resolve_from_root and error accessors
- Parse WorkflowVersionId via serde into/try_from, delegating length and
charset checks to RunBlobId
- Derive ReferenceKind's Display with strum instead of a hand-written
match
- Drop the fabro-workflow static_reference re-export shim; consumers
import from fabro-graphviz directly
- Collapse duplicate JSON-rejection arms in the workflow-versions
handler
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Route the root workflow through collect_workflow_entry so relative root
arguments are lexically normalized before reading, matching the pre-refactor
behavior: `..` segments no longer resolve through symlinks to a file other
than the one the manifest key names, and `~`-prefixed references are
rejected again. Adds a symlink regression test for the root argument.
Also:
- collect_workflow_entry/collect_workflow_location return the manifest key,
so bundle() no longer recomputes the root key
- hold one FilesystemTemplateStore on the bundler instead of rebuilding it
per template reference
- drop the unused Clone derive on WorkflowScanInput
- replace the hand-rolled JSON literal in the characterization test with an
insta snapshot per the testing strategy
- share one write_file fixture helper between the lib and bundler test
modules
- remove the bundler git-push test; the bundler has no git code path, so the
test could not fail
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Skipping a malformed key under blobs/sha256 during listing now emits a
warn! so operators get a signal when the CAS namespace contains garbage,
matching the projection-cache warmup skip path. Also removes the
runs_share_database_blob_store test, which asserted Arc pointer identity
of internal wiring rather than any observable behavior.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
- Collapse RunDatabase::open_writer/open_reader wrappers into one
pub(crate) build, with a Database::open_run_database helper that
gathers the shared-store dependencies in one place
- Stop fetching the blob store on open_run's active-cache hit path
- Share a raw-db test fixture between the two BlobStore raw-key tests
- Evict the cached writer in open_run_reader_is_read_only so the test
exercises the real reader construction path
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>
Apply cleanup review findings on the collector extraction:
- Deduplicate the lexical path-normalization loop: normalize_absolute_path
now delegates to lexically_normalize_access_path, and it plus
manifest_path_from_absolute live in working_tree.rs so the module
dependency points one way (projection -> collector). Drop the redundant
re-normalization in collect_bundled_file.
- Extract collect_bundled_template_includes to replace the copy-pasted
goal/prompt template-closure sequence, seed_config_document for the
duplicated config seeding, and read_source_input for the duplicated
config reader closures (with the user-settings is_file check hoisted).
- Replace ~100 lines of trivial getters on the Collected* output structs
with pub(super) fields; keep the CollectedPath newtype encapsulated.
- Assemble the manifest by value, moving collected sources into the wire
types instead of deep-copying every file a second time; drop two full
DraftDocument clones that only satisfied the borrow checker; stop
recomputing manifest paths per file in template-dependency verification.
- Resolve the root workflow once in assemble_current_manifest, removing an
unreachable duplicate error path; flatten single-use CollectionNamespace
into a finalize_documents free function.
No behavior change; fabro-manifest tests, clippy, and fmt pass.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Stage the pending task reminder as a Message and add
Message::to_llm_message so durable history and the round-staged turn
share one turn-to-wire conversion. Replace the one-off
BlockingAfterFirstOutputProvider with request capture and an
EventsThenPending variant on ScriptedStreamProvider, add a shared
make_session_with_provider_and_tools helper, and assert the reminder
tests against task_reminder::TASK_REMINDER_TEXT instead of a
substring.
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>
Drop ManifestTarget.identifier (the raw token the user typed) and
ManifestGoal.path (the original goal-file path) from the OpenAPI
manifest schema, the Rust manifest builder, the regenerated Rust and
TypeScript client types, and every canonical test fixture. Neither
field had a production reader: the server selects the workflow by
target.path and consumes only the resolved goal type and text.
Target path, goal type/text, manifest versioning, and submitted-byte
persistence are unchanged. Old request bodies that still carry the
removed properties remain accepted through unknown-field tolerance,
pinned by a dedicated public-route regression test.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
GitHub repository slug and git ref selector syntax now has one owner:
fabro-types::repository defines GitHubRepositorySlug with a try_new
constructor and the is_valid_github_ref_selector predicate.
fabro-automation keeps its public type path as a re-export of the same
type and delegates its existing parser and ref validation to the shared
grammar, preserving its exact error variants and messages. Server
checkout and materialization code imports the type from its canonical
owner. No wire, API, or behavior change.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Review asked twice whether `permit.send` can leave an agent Running with
no turn on its way. It cannot, and the reasoning is not local to the call,
so state it there.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The subagent event forwarder left its loop on any `recv` error, including
`Lagged`. A lagged broadcast receiver stays usable, so one transient lag
silenced the child for the rest of its life while the task completed
normally and shutdown joined it without noticing. Session reuse widens
that window from a single turn to the whole parent session.
Also borrow each result's output when rendering a parent notification
instead of cloning it.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
