Replace the hand-rolled byte scanner in strip_css_comments with a
str::find loop over "/*" and "*/".
Drop the quote and backslash tracking. The stylesheet language has no
string literals: parse_declarations ends a value at the first ';' or
'}' with no quote awareness, and values flow into AttrValue::String
verbatim, so a quoted model name is just an unknown model. Tracking
quotes here also created a failure mode the simple scan does not have.
An unpaired apostrophe, as in `model: don't`, disabled comment
stripping for the rest of the input and then blamed a well-formed
comment for the parse error.
Also drop the Cow and its copied_through watermark. They avoided one
allocation on a graph attribute of a few hundred bytes, parsed once per
workflow load, in a function whose caller already clones the attribute
and whose parser allocates a String per property and per value.
Extract excerpt() for the error snippets. The two existing call sites
sliced raw bytes at index 20, which panics when a multi-byte character
straddles the cutoff; model_stylesheet is arbitrary user text, so that
was reachable.
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>
The 10 MiB cap on resolved stdin_source values is tight for wide
fan-in: a context.parallel.results batch from a large for_each round
carries tens of structured agent outputs, and a merge step that feeds
them to a deterministic command hits the ceiling as a hard
deterministic failure. Raise the ceiling to 30 MiB; it still bounds
peak memory and remote uploads, just with headroom matched to the
fan-out sizes for_each already allows.
Co-Authored-By: Claude Fable 5 <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>
These were untracked working-tree files unrelated to for_each item
injection. They were swept in by an over-broad `git add` and do not
belong on this branch.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Addresses a Copilot review comment on #653.
The source array is runtime data, usually produced by a model, so its
length is not something a workflow author reviewed. Two changes, so an
over-long array degrades into a clear error rather than memory pressure.
Cap the item count at 1000. Above that the stage fails deterministically
before `parallel.started`, alongside the other for_each contract
violations, and the message says how to reduce the array.
Fork the parent context inside the branch task, after it acquires a
`max_parallel` slot, instead of at dispatch time. Live context copies now
track `max_parallel` rather than item count. Only the branch's own
preamble entry is moved into the task, so the shared stash is not cloned
per branch either.
The reviewer also suggested replacing spawn-all with `max_parallel`
workers pulling from a queue. Not done here: with the fork deferred, a
pending task holds little beyond its item, and reshaping the dispatch
loop would change cancellation and scope-reservation ordering, which
deserves its own review.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
build_branch_plan read the runtime array before run_branches looked at
`simulated`, so every for_each workflow failed under --dry-run with
"for_each source '...' was not found in workflow context". Nothing had
populated the key yet: upstream LLM nodes take Handler::simulate, which
returns no context updates.
A dry run now stands in one placeholder item when the source is absent or
unusable, and simulates the template target once. Graph-shape mistakes
still fail, since catching those is the point of a dry run.
Also from review:
- ITEM_FENCE_PREFIX replaces the bare "untrusted-" literal that
render_item_data and its test each spelled out.
- ItemRecordingHandler no longer guesses an item label by substring
search. Nothing asserted it, and the third item's label "2" matched
stray hex from the random fence tag about two thirds of the time.
- The twin test reads keys::PARALLEL_RESULTS instead of the raw string.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`parallel_group_id` used `oneOf: [$ref StageId, null]`, and the generator
drops a sibling description in that position, so the TypeScript client
documented the field as "Canonical stage execution identifier in
`node_id@visit` form" — the shared StageId text, which says nothing about
what this field means. Switching to `allOf` lets the field's own
description through.
Dropping `type: "null"` also makes the contract match the server, which
omits both fields rather than sending null (`skip_serializing_if` on
`Option`, pinned by list_run_stages_exposes_parallel_branch_identity).
The Rust types are unchanged — still `Option<StageId>` and `Option<u32>`,
which accept an explicit null on input either way — so this only narrows
what clients are told to expect on the wire. Wording updated to match,
and reworded to avoid an apostrophe the generator escapes into the
JSDoc.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Conflicts were between this branch's parallel-branch identity work and
main's stage billing, review targets, and live stage timing.
- Stage fixtures: main added `billing` to each per-file `makeStage`; this
branch had hoisted one builder into `lib/test-utils`. Kept the hoisted
builder and gave it `billing: makeBilledTokenCounts()`, so both intents
hold and the field list stays in one place. `stage-sidebar.test.ts` also
builds raw `RunStage` wire payloads, so it keeps importing
`makeBilledTokenCounts` directly.
- Import lists (`run_projection.rs`, `fabro-api/src/lib.rs`,
`run_state.rs`, `stage_projection_round_trip.rs`): unioned both sides —
`ParallelBranchId` alongside `timing`, `ReviewTarget`,
`ReviewTargetKind`, `AttrValue`, `Node`, and
`StageToolBatchProjection`.
- `fabro-server` tests: git interleaved two unrelated new tests into one
body. Split them back into
`list_run_stages_exposes_parallel_branch_identity` and
`run_billing_includes_live_stage_timing_in_rows_and_totals`.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Restore the documented SDK env credential facade without reintroducing run fallback behavior. Fail closed on GitHub permission resolution, require worker storage at the CLI boundary, and align interpolation names and generated docs.
Branches bypass the engine's stage.started/stage.completed lifecycle, so
no SWR key invalidated the stages list while a fork ran. The new live
branch rows stayed frozen at their first observed state until an
incidental refetch. Map parallel.* events to the stages list, run events,
and graph keys.
Also:
- Label branch rows with formatStageLabel so a re-entered branch renders
as `review_glm@2`, matching the sidebar and waterfall.
- Build branch rows in one pass and count live outcomes in one loop.
- Name ParallelBranchId in the OpenAPI spec and reuse fabro_types::
ParallelBranchId, replacing two copies of an inline string format.
- Hoist makeStage and textContent into lib/test-utils so widening Stage
cannot leave per-file fixtures stale (tests are excluded from
typecheck, so the two component-test copies had already gone stale).
- Query stat tiles by data-stat instead of an exact Tailwind class.
- Reuse append_scoped_stage_event's body via append_event_with_scope and
add test_branch_event instead of poking envelope fields.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Splitting on the first colon in FromStr broke bare model IDs that
legitimately contain one. A reference like "llama3:8b" parsed as
provider "llama3" selector "8b", and since "llama3" is not a provider
the lookup failed instead of passing the ID through to the pinned
provider. Verified against origin/main: canonical_session_model with
"future-model:latest" pinned to openrouter returned the passthrough
before and a 400 after.
This is not fixable by choosing a different separator. Bedrock
inference-profile ARNs contain both colons and slashes, and
docs/public/integrations/bedrock.mdx tells users to put arbitrary
inference-profile IDs in api_id. Only the registry can tell a provider
prefix from a model ID that happens to contain the separator.
FromStr now leaves colon-bearing tokens bare, and ModelRef::qualify
promotes only those whose prefix names a known provider. resolve()
applies it, so the fallback path is covered; sessions.rs applies it
before its own match so it keeps its tailored ambiguity messages.
ModelRegistry is now implemented for Catalog in fabro-types, replacing
the CatalogModelRegistry wrapper that existed only in start.rs, so both
call sites share one registry view.
Covered by regression tests at both surfaces, plus qualify unit tests
for ollama tags and Bedrock ARNs. The pre-existing passthrough test
canonical_session_model_preserves_unknown_passthrough_on_selected_provider
passes again.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Conflict in apps/fabro-web/app/components/interview-dock.tsx. Main moved
the dock onto the shared collapsible `RunDockShell` and replaced the
local button constants with shared ones.
Kept main's structure whole and re-applied the review target rendering
onto it: the question paragraph in the shell's `body` becomes the linked
`ReviewTargetQuestion` when the target passes `safeReviewTarget`, and
plain text otherwise. Both now share main's paragraph classes through
`QUESTION_TEXT`, so the two renderings stay visually identical.
`peek` keeps using `question.text`, which is the correct plain-text
collapsed summary for a review target question.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The review question sentence was written in four places and the URL
safety rules in three. Collapse each to one definition.
- Add `ReviewTarget::question_text_with_link` as the single definition of
the question wording. `question_text()` and the Slack header both use
it, so a wording change is now one edit.
- Delete `ReviewTargetKind::noun()`. The enum already derives
`strum::Display` with the same snake_case output.
- Share one `review_target_line` helper between the console interviewer
and the CLI attach client, which held a byte-identical copy. Print only
the URL: `question.text` already carries the label and the noun.
- Trim the web-side check to the URL scheme, host, and credentials, which
are what a raw `href` can act on. Label length and control characters
cannot affect the DOM and stay server-side.
- Split validation from presentation in the web UI. `safeReviewTarget`
returns the target or null, and each caller picks its own fallback, so
an unsafe target now falls back to the same Markdown rendering as a
question with no target.
- Derive the resource noun from `kind` in the web UI instead of
hardcoding "document".
- Use `ReviewTargetKind.DOCUMENT` and the shared `isRecord` guard when
parsing events, instead of a raw string and a hand-rolled object check
that accepted arrays.
- Drop `deny_unknown_fields` from the wire struct. The OpenAPI schema
leaves `additionalProperties` permissive, so an added field would
otherwise make persisted events unreadable.
- Import `ReviewTarget` by name, and stop naming Slack in a fabro-types
error message.
- Document that `review_target=true` replaces the gate's `label`.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The process environment is no longer a configuration source. `{{ vars.NAME }}`
(non-sensitive, server-stored) and `{{ secrets.NAME }}` (vault-backed) cover
both cases, and reading the worker's ambient environment made a run's inputs
depend on how its process happened to be launched.
`Namespace::Env` is kept but wired to nothing, so `{{ env.NAME }}` still
parses and fails with a message naming its replacement rather than reaching
a consumer as literal text. `ResolveCtx::with_env` is gone, so no call site
can opt back in.
Two long-standing warts were env-only and go with it:
- `InterpString::resolve_or_source`, the "fall back to the raw template
source on failure" path, which let an unresolved token reach a sandbox or
the GitHub API as literal `{{ ... }}` text. Its own comment noted it was
slated for hard-error semantics.
- `RunEnvironmentSettings::resolve_env`'s matching source fallback for
env-only values.
Both carried `#[expect(clippy::disallowed_methods)]` escape hatches. Every
run-boundary resolver — sandbox env, prepare steps, MCP transports, GitHub
permissions, Slack channels, run goal files, provider extra_headers — now
fails closed instead.
Hooks lose their `allowed_env_vars` allowlist, `resolve_header`, and
`HeaderResolveError` along with the `E: Env` generic threaded through the
executor. They keep `{{ vars.* }}`, which `RunSettings::substitute_variables`
already substitutes server-side at run creation.
`allowed_env_vars` is removed from the OpenAPI spec and the generated
TypeScript client. The docs example showing `{{ env.* }}` in
`[server.slatedb.s3].bucket` was already wrong — that field is a plain
String and never interpolated — and is now a literal.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Command node `script` attributes were literal text: a `{{ inputs.x }}`
reached bash verbatim, and the only signal was a `detemplated_attribute`
warning. Scripts now substitute `{{ goal }}`, `{{ inputs.NAME }}`, and
`{{ vars.NAME }}` at run creation, alongside goals and prompts.
Scripts use `InterpString` token substitution rather than the MiniJinja
pass that renders prompts. Shell source is full of brace syntax that must
survive untouched — jq filters, awk programs, Go templates, brace
expansion — and `InterpString` claims only the narrow token forms,
leaving everything else literal.
`env` and `secrets` are deliberately not wired and now fail loudly
instead of passing through as text. A script reads the environment with
`$NAME`, which needs no interpolation, and a resolved secret would be
baked into the `CommandStarted` event that records the script verbatim.
The error points at `[environments.<slug>.env]` for the secret case.
`ResolveCtx` gains opt-in `with_inputs` and `with_goal`. Namespace
availability stays scope-determined per call site, so every existing
config-layer context leaves both unwired and keeps its current behavior.
`goal` names a single value rather than a namespace of them, so it has
no dotted form: only the exact body `goal` produces a token and
`{{ goal.title }}` stays literal.
Values substitute verbatim without shell quoting, matching
`[[run.prepare.steps]].script` where the snippet is the author's to
quote. Substituted text is never rescanned.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The model indicator on a stage page hovered to provider, model, and
reasoning effort only. Seeing what a stage actually spent meant leaving
for the Billing tab, which reports per node rather than per visit.
The stage list had no token data to show, so add a per-visit `billing`
block to `GET /runs/{id}/stages`. The Billing tab's pricing rule (a
provider-reported cost wins, otherwise the server catalog prices the
tokens) was private to `billing_rollup`; move it to
`StageProjection::billed_usage` and drive both call sites from it so the
two views cannot drift.
The popover's buckets use the Billing tab's labels verbatim. It stays
scoped to one visit, so a looped node's row on the Billing tab is the sum
of what each of its visits shows here.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
A node with no `shape` defaulted to `box`, which resolves to the agent
handler. That made a shapeless `script` node run as an LLM call prompted
with its own label, while the `script` was reported as inert — wrong
behavior behind a warning.
`script` is read by the command handler and by nothing else, so a
shapeless node that sets it is unambiguously a command node. `shape()`
now infers `parallelogram` in that case. An explicit `shape` still wins.
Two rules keep the inference honest:
- `script_prompt_conflict` — setting both `script` and `prompt` is an
error. No handler reads both. It fires regardless of shape so that
adding one cannot downgrade the error to a warning.
- `command_requires_script` — a command node without a script is an
error. Without this the original trap just moves: a node meant as a
command that omits its script silently becomes an agent again.
Also drops the `tool_command` alias in favor of `script` alone, routing
the six read sites through a new `Node::script()` accessor.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The DOT parser created a node for every edge endpoint, and nothing
recorded whether a node came from a declaration or was synthesized from
an edge. The edge_target_exists rule only checked whether the node id
was present in the graph, which was always true by then, so a misspelled
endpoint became an attribute-free node that defaulted to shape=box — an
LLM stage. Validation emitted a prompt_on_llm_nodes warning and exited 0.
Node now carries `implicit`, set only when the parser synthesizes the
node from an edge endpoint. A declaration anywhere in the workflow
clears it, so order does not matter and subgraph declarations count.
Node::new leaves it false, so programmatic construction and graphs
deserialized from older checkpoints read as declared.
edge_target_exists treats an endpoint as valid only when it exists and
is declared, reporting each undeclared node once. The near-identical
missing-source and missing-target branches collapse into one path. The
import transform copies the flag onto spliced nodes so an edge-only node
inside an imported fragment is caught too.
parse_and_validate_human_gate had two edge-only nodes and now declares
them; it was an instance of the bug rather than a casualty of the fix.
No shipped workflow, docs example, or CLI fixture relied on the old
behavior.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
