Consolidate the copies that review found across the feature:
- One GITHUB_CREDENTIAL_HELPER / GITHUB_CREDENTIAL_HELPER_KEY pair in
fabro-github, with apply_probe_git_env() for probe commands; the runtime
git bridge, server preflight probe, and live contract test all consume it
so the probes exercise exactly what the bridge configures.
- GitHubRepositoryAccess::resolve_verified_token() owns the
resolve-installations-then-mint choreography shared by server preflight,
workflow initialization, and the live test.
- A shared lookup_installation() helper backs both the shared-installation
resolution and the mint's installation lookup.
- The contents = read|write rule lives once as
RunIntegrationsGithubSettings::contents_permission_allows_repository_access.
- The preflight probe paces retries with fabro-sandbox's exported
replication_backoff() (3s/9s) instead of a contradicting 1s/2s loop, and
shares one run_ls_remote() runner with the existing remote-ref check.
Also: collapse the dead Ok(None) arm and repeated error blocks in the
preflight token check, drop the derivable bridge_entry_count(), privatize
resolve_permissions() behind resolve_integration(), make
GitHubRepositorySlug ordering/hashing allocation-free, and use EnvVars
constants for env names.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Brave stays the default. Shops that already vault VENICE_API_KEY
can drop BRAVE_SEARCH_API_KEY by setting
[server.integrations.search] provider = "venice".
Co-authored-by: Cursor <cursoragent@cursor.com>
- Add `additional_repositories` to the RunIntegrationsGithubSettings
OpenAPI schema and reuse the canonical Rust settings types through
`with_replacement`, with type-identity witnesses and JSON parity
tests for populated and empty repository sets.
- Regenerate the TypeScript API client.
- Document the feature in the GitHub integration and run-configuration
guides: exact layer replacement rules, single-token scope, gh/API
support, App-versus-PAT scope, the same-owner/same-installation
requirement, validation errors, supported Git URL forms, hard-failure
semantics for declared repositories, GH_TOKEN precedence, and the
security boundary (no second server-side repository intersection;
contents = "write" lets any stage push to any declared repository).
Correct the earlier claim that injecting GITHUB_TOKEN alone makes
arbitrary additional private clones work.
- Add a dated changelog entry and an opt-in live GitHub App e2e test
that verifies a scoped multi-repository token reads every declared
repository (and that a primary-only token cannot), with repositories
supplied through the test environment.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Add `additional_repositories` to `[run.integrations.github]`: a list of
full `owner/repository` slugs, beyond the implicit run origin, that the
minted GITHUB_TOKEN must cover.
- `GitHubRepositorySlug` gains FromStr, Display, string serde, and
case-insensitive Eq/Ord/Hash identity while preserving the submitted
spelling for display and serialization.
- The config layer keeps raw strings; the higher-precedence list
replaces the lower one wholesale, with `[]` as an explicit clear,
resolving independently from the `permissions` map.
- Resolution validates each entry with indexed error paths: slug
grammar, case-insensitive duplicates, one shared owner, the
499-repository cap, and a required `contents = "read"|"write"`
permission (templated values are re-checked at the runtime boundary).
- `RunIntegrationsGithubSettings` resolves permissions and repositories
together through `resolve_integration()` so consumers cannot pick up
one without the other; the field is omitted from serialization when
empty, keeping single-repository settings byte-identical.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The lineage field's `skip_serializing_if` behavior was asserted five times
across three crates. Keep the two assertions in fabro-types, which owns the
attribute, and drop the duplicates:
- Delete `run_created_omits_absent_workflow_version_id` from event/convert.rs,
a copy of the test above it that re-checked another crate's serde attribute.
convert.rs's own responsibility is covered by the existing field assertion.
- Delete `legacy_create_input_persists_without_workflow_version_id`, which ran
the full create() pipeline to prove a hardcoded `None` literal is `None`.
`CreateRunInput` has no such field, so no input could change the result.
- Fold `run_spec_omits_absent_workflow_version_id` into the adjacent legacy-spec
test, which already holds an all-`None` record.
- Drop the off-topic spec re-serialization from run_state.rs's retried_from test.
Add `test_support::test_workflow_version_id()` alongside `test_run_provenance()`
and use it everywhere, replacing eight copies of the same magic seed across five
crates plus two assertion sites that recomputed the hash inline. This also
subsumes retry.rs's private helper of the same shape.
Revert the `run_spec_json` parameterization in the projection round-trip test:
`RunProjection` is a `with_replacement` alias for the canonical type, so the
`Some` and `None` call sites exercise identical code.
Have the two run.created literals that mirror a `RunSpec` read the spec's
lineage field instead of hardcoding `None`, so the mirrors stay accurate once a
producer populates it.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The release push raced any commit that landed on main while the release
smoke ran (~15 minutes): git push was rejected as non-fast-forward and
the whole release failed, as seen on the v0.332.0-nightly.1 attempt.
Worse, the push was not atomic — if the tag ref had been accepted while
the main ref was rejected, the release would have shipped from an
orphan commit and main would never have received the version bump.
Make the push atomic (both refs or neither) and add a bounded rescue
loop: on rejection, drop the bump commit and tag this run created,
fast-forward onto the updated origin/main, recompute the version
against freshly fetched tags, and rebuild the bump commit on the new
tip. The fast-forward uses --ff-only so a genuinely diverged local main
(unpushed commits) fails loudly instead of being reset away.
The retried tag can include commits the smoke did not test; those
commits passed CI to land on main, and the Release workflow re-runs the
full test suite on the tagged commit before publishing anything.
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>
`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>
TemplateDiscoveryError only named a failing source through the Display
strings of its variants: parse and load failures forwarded transparently
to inner errors whose source naming varies (parent for some load
failures, the child path for dynamic dependencies, nothing for I/O
faults), so consumers that need the failing template's path had to
string-round-trip error messages. Carry the parent path on every
variant, exposing a total source_path() accessor, and render parse and
load failures with a parent-naming message above the preserved source
chain.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Dependency discovery pre-seeded roots into the path-keyed result map
and reused that map as the traversal-dedup set, so a loaded include
target whose path matched a root was recorded but never parsed (an
include chain that reaches the file anchoring a root silently skips its
content), and a second root occurrence at an already-seeded path was
dropped without parsing. Dedup traversal on the full
(path, root, content) occurrence instead, so every distinct authored
occurrence is parsed exactly once and identical duplicates parse once.
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>
The manifest_blob and definition_blob filter entries were copy-paste
twins that had to be edited identically; build them from one loop like
the elapsed-ms filters above so the pattern and placeholder cannot
drift apart.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Promote BlobHash to a named OpenAPI schema with the ^[0-9a-f]{64}$
pattern, reference it from WriteBlobResponse.hash and the blobHash path
parameter, and map it to fabro_types::BlobHash via with_replacement.
The server now serializes the domain type directly and the client gets
a parsed BlobHash by construction, removing the to_string/parse adapter
pair across the wire boundary. Adds the JSON-parity test required for
new replacements.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
This reverts commit 8090d7030984862564a929ee9264e93911014e00.
The cached canonical field was optimizing an unmeasured path: without
the (deferred) O(closure) dependency re-validation multiplier, the
repeated serialization is microseconds for realistic versions. Compute
canonical bytes on demand like the environment, automation, and MCP
stores do, rather than carrying a serde-skipped cache field, a
construction bootstrap, and doubled memory for it. Purely in-memory:
stored blobs and version IDs are unchanged.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
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>
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>
Add the WorkflowVersion domain resource with exactly entrypoint, files,
and workflow_dependencies, plus strict WorkflowPath validation and
deterministic canonical raw JSON. Semantic validation of graph imports,
templates, file references, workflow.toml rules, Dockerfile paths, and
exact child-workflow dependency bindings lives in the new
fabro-workflow-version crate, which validates the complete stored
dependency closure through the shared blob store before writing a root.
The authenticated create-only POST /api/v1/workflow-versions endpoint
ships with its OpenAPI contract, Rust type replacements, and generated
TypeScript client.
Squashed from the resource commits of the original combined branch;
the walker unification this builds on landed separately.
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>
Also add environment_images_mut and adopt it in the run compiler's
Dockerfile resolution, replacing the hand-rolled iteration over named
environments plus the run environment.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Two root-cause fixes for the sandbox failure where an inline Dockerfile
came back from the store as `ARG REDACTED` and the Daytona snapshot
build died on the unset variable.
Entropy redaction measures values, not assignment pairs. The detector
matched `NAME=value` as one token, so an uppercase name merged its
charset into a pure-hex value (which alone can never exceed 4.0 bits)
and pushed the pair over the 4.5-bit threshold — then replaced the
whole pair, destroying the name. `find_entropy_regions` now strips an
identifier-shaped `NAME=` prefix before measuring and redacts only the
value, matching the gitleaks layer's `key=REDACTED` shape.
Execution no longer reads redacted content. Every stored event passes
through the redaction sink, and `load_from_store` rehydrated the
worker's RunSpec from the projection folded from those events — so a
redactor false positive silently rewrote the spec the sandbox builds
from (and changed its snapshot identity). The creation path now writes
the exact spec bytes to the content-addressed blob store and records
`spec_blob` on run.created; `load_from_store` loads the spec from the
blob, keeping the event stream authoritative for run identity,
provenance, and event-recorded blob ids. Retry and fork carry the
source run's `spec_blob` forward, so derived runs stop inheriting the
redacted copy. Runs created before the blob existed fall back to the
folded spec.
The projection and every API surface keep serving the redacted fold;
blobs were already stored unredacted (the workflow bundle carries the
same bytes), so this adds no new exposure at rest.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The entropy redactor rewrites NAME=<hex> assignment pairs to a bare
REDACTED, and the worker rehydrates its executable RunSpec from the
projection folded from redacted stored events. Together these broke
Daytona snapshot builds for any run definition whose inline Dockerfile
pins a git SHA: the spec came back as `ARG REDACTED`, the build died on
the unset variable under `set -eu`, and the environment's snapshot
identity silently changed.
Pin the intended contracts with red tests:
- fabro-redact: an assignment whose value alone is below the entropy
threshold survives redaction (pure hex cannot exceed 4.0 bits; only
the name+value charset merge crosses 4.5), and a genuinely
high-entropy value is redacted without destroying the key name.
- fabro-workflow: the spec that load_from_store rehydrates round-trips
byte-identical through the store, including content that looks like
a secret — event redaction must not reach execution.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The executor incremented a node's visit count on entry and refused the
visit once the count reached the limit, so a node with max_visits=N
executed at most N-1 times. The documented contract in
stages-and-nodes.mdx is "Max times this node can execute in a run",
and both published examples describe bounded retry loops under that
reading. A graph with max_visits=2 on a designed
one-correction loop therefore failed as "stuck in a cycle" before the
correction could run.
Check the completed-visit count before entry instead: a node with
max_visits=N now executes exactly N times, and the refused entry is
not reported as a visit, so the error's count names the executions
that actually happened. Also correct the nlspec example prose, which
claimed the workflow "moves on with the best result" at the limit;
exceeding max_visits fails the run.
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>
