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>
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>
The playground's local manifest projection and builder now emit only
the resolved target path; workflow-name fallback, titles, and the
workflows-map key are unchanged.
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>
