Neither method has callers anywhere in the workspace: resolve_reference
splits on '/' directly, and the path-collision validator now checks
ancestor prefixes against a path set. parent() also constructed Self
without going through validate(), so dropping it removes an unvalidated
construction path from the wire type's public API.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The round-trip fixtures only used empty workflow_dependencies, so no
WorkflowVersionId value ever appeared on the wire in a fabro-api
assertion and CreateWorkflowVersionResponse had no coverage at all.
Put a real 64-hex id in the fixture, round-trip the response type, and
pin serialization to the schema's ^[0-9a-f]{64}$ pattern including
lowercase normalization of case-insensitive input.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
WorkflowVersionId bolted a lowercase-only byte scan onto BlobHash
parsing, giving the same 64-hex concept two parse behaviors across
entry points. Identity is the decoded 32-byte digest and canonical
serialization always emits lowercase, so accepting either case on
input is lossless — the stored-blob canonicality check still rejects
non-canonical bytes independently. Delegate straight to BlobHash.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
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>
DependencyInvalid fell through to the curated 500 even though the
OpenAPI contract promises 422 workflow_version_dependency_not_found for
an absent, invalid, or non-canonical dependency. Route it to that
response alongside DependencyNotFound; the top-level message only names
the caller-supplied path and id, so no internal chain leaks. Drop the
InvalidVersion/InvalidShape arms, which were unreachable from the only
call site.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The adjacent-pair scan over the byte-sorted path list missed
file/directory collisions whenever a sibling path sorted between the
ancestor and its descendant (any byte below '/' after the shared
prefix, e.g. "assets.txt" between "assets" and "assets/item.txt").
Replace it with an exhaustive ancestor-prefix lookup over a path set,
which also catches equal paths across files and workflow dependencies.
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>
import and stack.child_workflow are node attributes — the import
transform and manager loop only read them from nodes — but the
reference classifier matched them at any scope, unlike goal and prompt
which already carry scope guards. That meant template expansion treated
a graph- or edge-level attribute with one of those names as a live
static reference (hard error on template syntax) even though nothing
reads it there. Guard both arms to node scope so classification matches
what the engine actually consumes.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
stack.child_dotfile was the original attractor-spec name for the
manager-loop child workflow path, later renamed to stack.child_workflow
with the old name kept as a backward-compatible alias. The alias was
deliberately removed in #281 (engine, manifest fallback, and docs), but
reference_kind_for_attribute — introduced the same day in #290 — was
written with the alias baked in, resurrecting it in the shared
vocabulary.
The engine only reads stack.child_workflow, so a graph using the alias
silently does nothing at runtime while validation treats it as live:
workflow-version validation would demand a dependency pin for a child
that never spawns, and the manifest bundler would bundle its target.
Finish what #281 started and drop the alias.
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>
250ms was too aggressive: a server that was reachable but slightly slow
to answer /health made `fabro doctor` report a failed health check.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Regenerates the Axios client from the reduced OpenAPI spec and removes
the six stale pre-run-push-outcome model files the generator leaves
behind, along with their barrel and generator-manifest entries.
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>
