Commit graph

100 commits

Author SHA1 Message Date
Bryan Helmkamp
b19dda6aa3
Merge pull request #661 from fabro-sh/fix/small-default-skips-providers-without-small-model
Fix run title generation picking a flagship model, and sharpen the prompt
2026-07-28 17:38:27 -04:00
Bryan Helmkamp
8f9b36c0b8
fix(test): propagate storage setup errors 2026-07-28 17:23:31 -04:00
Bryan Helmkamp
e91343bbeb
refactor: address run title review findings 2026-07-28 17:14:03 -04:00
Bryan Helmkamp
df0bd58819
Merge remote-tracking branch 'origin/main' into fix/small-default-skips-providers-without-small-model
# Conflicts:
#	lib/apps/fabro-server/src/test_support.rs
2026-07-28 17:06:20 -04:00
Bryan Helmkamp
1aa7a153b0
Merge pull request #652 from fabro-sh/fix/publish-failures
Make publish failures terminal
2026-07-28 17:03:18 -04:00
Bryan Helmkamp
e6cf174578
Merge origin/main into feat/human-review-target
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>
2026-07-28 16:48:02 -04:00
Bryan Helmkamp
450a1701b2
Merge pull request #670 from fabro-sh/fix/sandbox-activate-before-access
Reactivate stopped sandboxes at workflow boundaries
2026-07-28 15:54:57 -04:00
Release Repro
e21e6bcdf9
refactor(sandbox): harden activation recovery 2026-07-28 15:38:35 -04:00
Bryan Helmkamp
8e066ecf7b
refactor: remove duplicated review target rendering and validation
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>
2026-07-28 15:14:23 -04:00
Bryan Helmkamp
73f48eeddb
refactor: simplify publish pipeline and collapse duplicated stage errors
Follow-up cleanup on the publish-failures change.

Error model:
- Collapse `Error::{Engine, Publish, Handler}` into one `Error::Stage` with an
  `ErrorStage` discriminator. The three shared a field shape and had to be
  edited together in four match groups; nine near-identical constructors
  become two private helpers.
- Add `Error::failure_reason()`, replacing the same error -> FailureReason
  mapping written out in four places.
- Publish errors are now terminal. Publish runs once, after execution, so no
  caller could ever act on the retryable classification.

Publish phase:
- Fix: a branch that was pushed is now still reported when pull request
  creation fails afterwards. `PublishOutcome` records what happened and
  carries the error separately, instead of hiding both behind a `Result`.
- Drop `PublishOutcome::NoChanges`, which no consumer distinguished from
  `Published { pr_url: None }`.
- Move publish onto `Concluded` as methods and replace three near-identical
  precondition guards with one `publish_target()`.

Pull requests:
- `maybe_open_pull_request` -> `open_pull_request` returning the record
  directly. Both callers already reject empty diffs, so the `Ok(None)` path
  was unreachable.
- Drop `CreatedPullRequest.head_sha`, which echoed back its own input.

GitHub client:
- Delete `branch_exists`, which had no callers and duplicated
  `branch_head_sha`. Give `branch_head_sha` the `_with_client` split every
  sibling has and port the tests to `MockHttpClient`.
- Collapse the copy-pasted credential match in `resolve_clone_credentials`.

Events:
- `PullRequestCreated.head_sha` is `Option<String>` instead of using an empty
  string to mean absent.
- Centralize the run-branch refspec in `lifecycle::git::push_run_branch`, so
  `git.push` reports a branch name from both emitters as documented.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
2026-07-28 15:09:10 -04:00
Bryan Helmkamp
34656aa7a4
Merge pull request #672 from fabro-sh/refactor/validation-catalog-cleanup
fix(tool): finish the catalog-free validation work from #651
2026-07-28 14:59:28 -04:00
Bryan Helmkamp
c083fa5209
Merge pull request #658 from fabro-sh/feat/stage-model-popover-billing
Show stage tokens and cost in the model popover
2026-07-28 14:58:51 -04:00
Release Repro
acff084afd
refactor: simplify stage billing reuse 2026-07-28 14:51:47 -04:00
Bryan Helmkamp
40372340c0
fix(tool): keep fabro_run_create validation catalog-free
PR #651 stopped `fabro validate` and `fabro create` from judging model and
provider availability locally, but left the `fabro_run_create` tool path
doing exactly that. Both of its callers build a *client-side* catalog and
then POST the manifest to the server, so an agent naming a server-owned
model got `Model selection failed: unknown model provider '...'` while the
same workflow succeeded through the CLI.

- `build_run_tool_manifest` now validates structurally, matching the CLI.
  It no longer takes a catalog at all.
- The MCP builder drops its `load_llm_catalog_settings` +
  `Catalog::from_builtin_with_overrides` pair, and `WorkerRunManifestBuilder`
  drops its catalog field, becoming a unit struct.
- `validate_manifest_with_catalog` had no callers left, so it is gone.
  `validate_manifest` documents why every remaining caller is catalog-free.

The new test fails with the pre-fix client-side check, reproducing the
reported error exactly.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
2026-07-28 14:40:54 -04:00
Bryan Helmkamp
ee89caf649
Merge pull request #647 from fabro-sh/fix/live-active-time-accumulation
fix(timing): accumulate active time for in-flight stages
2026-07-28 14:32:31 -04:00
Bryan Helmkamp
09ef90ba1e
refactor: pass the catalog by value and correct the RenderMode docs
- Take `Arc<Catalog>` by value again through the validation entry points.
  `AppState::catalog()` returns an owned `Arc`, so `&state.catalog()` was
  cloning, borrowing the temporary, then cloning again at the leaf. Every
  consumer ends up owning the `Arc`, so by-value is the honest shape and it
  drops one clone per call. The one caller holding the catalog in a field
  now says `Arc::clone(&self.catalog)` explicitly.
- Correct the `RenderMode` doc comment. It claimed `Strict` is "used by
  run-create", but run-create renders `Structural` and promotes the
  resulting warnings to errors itself; `Strict` has no production caller.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
2026-07-28 14:30:48 -04:00
Bryan Helmkamp
c5d3345144
Merge pull request #651 from fabro-sh/fix/offline-validation-catalog-free
fix(cli): keep offline validation catalog-free
2026-07-28 14:28:35 -04:00
Bryan Helmkamp
a10ffb02b5
Merge pull request #654 from fabro-sh/fix/reject-edge-only-nodes
Reject nodes referenced by an edge but never declared
2026-07-28 14:28:11 -04:00
Bryan Helmkamp
96f70a9aea
fix(timing): simplify live timing projections 2026-07-28 14:20:59 -04:00
Bryan Helmkamp
04c931e9d3
Simplify edge-only node validation 2026-07-28 14:14:32 -04:00
Bryan Helmkamp
8592a34968
refactor: collapse the split validation paths
Follow-up cleanup on the catalog-free validation split. Same behavior,
fewer parallel code paths.

- Make the catalog an explicit `Option<&Catalog>` on `pipeline::validate`
  instead of a `validate` / `validate_with_catalog` pair, so each call
  site states whether catalog rules run.
- Collapse `preprocess_and_validate`, `preprocess_and_validate_structural`,
  and `preprocess` into one function that takes `TransformOptions`. Its
  `model_resolution` field is now the single source of truth for catalog
  awareness, which drops a 12-argument signature and the
  `too_many_arguments` allow.
- Replace the duplicated resolve-and-preprocess block in
  `operations::validate` with one `validate_in_scope` helper, and drop the
  HashSet -> Vec -> HashSet round trip on the catalog path.
- Extract `configured_default_provider`, previously duplicated between
  `operations::create` and `operations::validate`.
- Delete `validate_manifest_with_environment_defaults`, which had no
  callers outside its own module.
- Share the `server-model.fabro` fixture between the two CLI tests instead
  of inlining it twice. The validate test now asserts the rendered output
  through the usual snapshot helper, which also removes a hand-rolled
  `std::fs::write` and its clippy allow.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
2026-07-28 14:13:45 -04:00
Bryan Helmkamp
465bdd6c27
Redirect test storage through with_storage_override
The test helper set server.storage.root directly, leaving the derived
local object-store roots (artifacts, slatedb) pointing at the real
~/.fabro/storage. Route the redirect through
ServerSettings::with_storage_override so every derived root moves to the
test directory together.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
2026-07-28 13:50:16 -04:00
Bryan Helmkamp
010c8d50c1
Add structured review targets to human gates 2026-07-28 13:24:12 -04:00
Bryan Helmkamp
7906af3f3f
fix(sandbox): reactivate stopped run sandboxes 2026-07-28 12:50:20 -04:00
Bryan Helmkamp
9e744f8072
Keep server tests off the developer's real ~/.fabro/storage
Test settings usually omit `[server.storage] root`, so it resolved to the
production default. Handlers that walk that tree read whatever the machine
happened to have.

That is why all_spec_routes_are_routable was slow. Timing every request in
it showed 91% of the runtime in two routes:

  6304ms  GET /api/v1/system/resources
  4574ms  GET /api/v1/system/df
   583ms  POST /api/v1/system/prune/runs
  ...
  the remaining 134 operations: 8ms combined

Both size Fabro-managed storage. On this machine that meant 193MB and 90,795
entries under scratch/, so the test's duration tracked how long the developer
had been running Fabro locally. Run-creating tests were writing there too.

Redirect settings that still carry the production default to a `storage`
directory beside the test vault, alongside the existing `server.env` and
`settings.toml` siblings. A test that chose its own root keeps it.

all_spec_routes_are_routable drops from ~15s to 0.6s, and the full workspace
run from ~33s to ~21s. All 7402 tests pass.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
2026-07-27 18:45:27 -04:00
Bryan Helmkamp
99dd7718c0
Keep server tests off the developer's real ~/.fabro/storage
Test settings usually omit `[server.storage] root`, so it resolved to the
production default. Handlers that walk that tree read whatever the machine
happened to have.

That is why all_spec_routes_are_routable was slow. Timing every request in
it showed 91% of the runtime in two routes:

  6304ms  GET /api/v1/system/resources
  4574ms  GET /api/v1/system/df
   583ms  POST /api/v1/system/prune/runs
  ...
  the remaining 134 operations: 8ms combined

Both size Fabro-managed storage. On this machine that meant 193MB and 90,795
entries under scratch/, so the test's duration tracked how long the developer
had been running Fabro locally. Run-creating tests were writing there too.

Redirect settings that still carry the production default to a `storage`
directory beside the test vault, alongside the existing `server.env` and
`settings.toml` siblings. A test that chose its own root keeps it.

all_spec_routes_are_routable drops from ~15s to 0.6s, and the full workspace
run from ~33s to ~21s. All 7402 tests pass.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
2026-07-27 18:39:25 -04:00
Bryan Helmkamp
713d340db7
Render the run title prompt with Jinja instead of string replacement
The prompt used hand-rolled `{placeholder}` substitution via `str::replace`.
The app already has a MiniJinja layer for exactly this, and every other
checked-in prompt uses it, so use it here too.

`prompts/run_title.md` becomes `prompts/run_title.md.j2` with `{{ inputs.* }}`
variables, rendered through `fabro_template::render_named`. Strict undefined
handling now catches a variable the template asks for and the caller does not
supply, which the old `.replace()` chain silently left as literal text.

`build_title_prompt` returns `Result` accordingly. A checked-in template that
will not render is a bug rather than a transient failure, so the caller logs
it at `warn` — louder than the `debug` used for a generation miss — and keeps
the deterministic title.

Re-checked against claude-haiku-4-5: same titles as before the change.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
2026-07-27 17:23:16 -04:00
Bryan Helmkamp
88ca4b1d03
Teach the run title model the title shape we want
Move the run title prompt into `src/prompts/run_title.md` and load it with
`include_str!`, matching the playground and ask-fabro prompts. Placeholder
substitution replaces `format!`, so the literal `{"title":"..."}` in the
prompt no longer needs brace escaping.

The old instructions only said "concise" and "preserve ticket IDs", so a
work order run titled itself with the raw file path. The prompt now asks for
a pull-request-shaped title: leading verb, identifier in canonical uppercase,
then a description with paths, date prefixes, and extensions stripped and
slug hyphens turned back into words. Three worked examples carry the shape.

Checked against claude-haiku-4-5 at the existing 64-token budget:

  Implement Conveyor Work Order docs/planning/orders/2026-07-22-wrk-004-operational-diagnostics.md
    -> Implement WRK-004: Operational diagnostics
  Fix flaky checkout test (input branch release-9.2)
    -> Fix flaky checkout test on release-9.2

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
2026-07-27 17:11:52 -04:00
Bryan Helmkamp
7841a77f2c
feat(web): show stage tokens and cost in the model popover
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>
2026-07-27 16:06:19 -04:00
Bryan Helmkamp
c501c67185
Show each diagnostic's suggested fix in CLI output
Diagnostics have carried a `fix` field all along, but the CLI renderer
never printed it — the suggestion was only reachable through --json. The
actionable half of every validation failure was invisible to the person
running the command.

print_diagnostics now emits the fix as a dim-labelled continuation line
under any diagnostic that has one, at both error and warning severity.
Gating it behind --verbose would defeat the point, and printing it only
for errors would read as "this warning has no fix" — the warning
suggestions are useful on their own. Diagnostics that set no fix simply
omit the line.

The severity match moved into print_diagnostic so the fix line is
appended once in the loop rather than copied into all five arms; the
rest of the diff is reindentation.

print_diagnostics is shared by validate, preflight, graph, exec, and
dry-run, so this covers all five. Eleven inline snapshots across four
files gain a fix line; every change is additive.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
2026-07-27 14:06:31 -04:00
Bryan Helmkamp
59b1c2e59f
Reject nodes referenced by an edge but never declared
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>
2026-07-27 13:53:54 -04:00
Bryan Helmkamp
1c82bd9008
fix(workflow): make publish failures terminal 2026-07-27 11:25:18 -04:00
Bryan Helmkamp
0b24649e76
fix(cli): keep offline validation catalog-free 2026-07-26 09:25:47 -04:00
Bryan Helmkamp
dd9f75fb05
fix(timing): harden live active projections 2026-07-25 23:43:43 -04:00
Bryan Helmkamp
c4971b93d3
fix(timing): accumulate active time for in-flight stages
`active_time_ms` was only ever computed from terminal stage events, so a
stage still running contributed zero to the run rollup. A run parked in one
long agent stage reported 2m 8s of active time against 16m 53s of wall
clock — the two finished stages — while the running stage had been doing
continuous inference and tool work for over 14 minutes.

`live_run_timing` summed `filter_map(|stage| stage.timing)`, and
`stage.timing` is only written at finalization. Wall time ticked live off
`start_time`; active time did not tick at all.

Stage projections now accumulate brackets from the event log:

- Closing an inference bracket folds its span into `live_inference_ms`
  instead of discarding it, including across retries, matching the
  in-process stopwatch.
- Tool calls open a batch on the first outstanding call and close it when
  the last one drains, so tools running concurrently within a turn count
  once — the same span `execute_tool_calls` is bracketed by. Summing
  per-call durations would over-count parallel tool use. Subagent tool
  events are excluded; they run inside the root call's span already.
- `StageProjection::live_timing(now)` composes accumulators with any open
  bracket, per handler: agent stages use the brackets, prompt and command
  stages count elapsed time as inference and tool respectively, and
  handlers that wait on a human, timer, condition, or child branches
  report zero.

Active is clamped to wall per stage. A worker killed mid-turn leaves its
bracket open forever, and without the clamp it would tick up unbounded.
The clamp does not need to detect the dead worker: a stage cannot have been
active longer than it has existed. `watchdog.timeout` remains the authority
on whether a run is stuck. The clamp is deliberately not applied at run
level, where concurrent branches can legitimately sum past run wall time.

Timing is derived from events rather than emitted by the worker, so this
needs no event-schema change and applies to runs already stored.
`StageProjection.timing` keeps its terminal-only meaning, and the
authoritative breakdown still replaces the live estimate at terminal
events.

The billing endpoint had the same hole behind its `wall_only` fallback:
running stages reported zero inference/tool/active. Not visible in the
product, which renders only `wall_time_ms`, but wrong for any other
consumer of `GET /runs/{id}/billing`.

Parallel branch stages lose their breakdown permanently, even after
completion, because `parallel.branch.completed` carries only `duration_ms`.
That is a separate data-loss bug, tracked in #644.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
2026-07-25 14:54:38 -04:00
Bryan Helmkamp
695a981f42
feat(web): tell open tabs when a new build ships
A tab left open across a deploy keeps running the previous build's
JavaScript indefinitely. index.html is fetched only on a full page load,
all later navigation is client-side, and hashed bundles are served
`immutable`, so nothing reveals that the code is stale. This produced a
false-positive bug report where two correctly-deployed fixes appeared to
be missing.

Publishes a build id and offers a reload when the running document falls
behind. The toast never reloads on its own; the only automatic reload is
recovery from a chunk that no longer exists.

Build id derivation
-------------------
The obvious approach — hash the emitted asset filenames, which already
embed content hashes — does not work: Bun's minified identifier naming is
not deterministic. Building an unchanged tree twice produces byte-different
output roughly one run in three (same length, ~100k differing bytes, all of
it mangled names). Output hashes therefore move with no source change,
which would fire the toast on redeploys of identical code and train people
to ignore it.

The id is instead derived from the bundle's source inputs, so it changes if
and only if something we control changed. Verified stable across eight
consecutive builds while the entry hash flipped between both variants.

This non-determinism also means two builds of the same commit embed
different bytes into the server binary, which is worth addressing
separately for reproducible builds.

Detection
---------
SWR with `refreshInterval` + `revalidateOnFocus`, per the repo's React
effects policy. SWR does not poll while the document is hidden, so
background tabs stay quiet without extra gating. Unknown state on either
side — missing meta tag, failed fetch, 503 during a dev rebuild — never
produces a prompt.

Stylesheet hashing
------------------
Tailwind's output was stable-named and therefore served `no-cache`, letting
a tab revalidate into new CSS while running old JS. Tailwind purges unused
classes per build, so classes the old bundle still emits could silently
lose their styles. It is now content-hashed and moves with the build.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
2026-07-25 14:45:26 -04:00
Bryan Helmkamp
e2df6e68c7
Merge pull request #640 from fabro-sh/codex/workspace-glob-semantics
Unify workspace glob semantics across sandboxes and artifacts
2026-07-25 12:05:17 -04:00
Bryan Helmkamp
dec67ec92e
fix(glob): harden artifact traversal 2026-07-25 11:56:57 -04:00
Bryan Helmkamp
d931ae6105
fix(agent): simplify GPT-5.6 tool routing 2026-07-25 11:49:40 -04:00
Bryan Helmkamp
c7ad387d3e
feat(agent): add gpt56 profile for GPT-5.6 Sol, Terra, and Luna
Codex drives the GPT-5.6 models with a much narrower tool set than the
other OpenAI models: a shell, `apply_patch`, and `update_plan`. It has no
file-read, file-write, grep, glob, or fetch tool at all -- reading and
searching go through the shell, and every write goes through
`apply_patch`. Offering 5.6 fabro's extra tools advertises affordances its
instructions never mention, so this adds a profile that registers only
what Codex does.

The profile is selected per model via `agent_profile = "gpt56"` on the six
5.6 rows (three each on `openai` and `openrouter`), following the existing
Kimi-over-a-gateway pattern. Every other model on those providers keeps
its provider default, with no code branch and no version sniffing.

- `ToolVocabulary::Codex` renames `shell` to `shell_command`; a strum
  alias keeps `from_any_name` resolving it to `NativeTool::Shell`, so
  permissions, categories, and telemetry still key on the canonical name.
- `shell_command` gains `workdir`, passed to the `cwd` argument
  `execute_shell_command` already accepted, with Codex's "always set
  `workdir`, do not `cd`" guidance.
- `prompts/gpt56.md.j2` is adapted from Codex's 5.6 `base_instructions`,
  which are byte-identical across Sol, Terra, and Luna. A header comment
  records provenance and the departures fabro's harness forces.

This is an alignment-only pass: it matches Codex's tool contract while
keeping direct tool calls. Codex actually drives 5.6 in code mode, with a
single `exec` tool taking JavaScript and every other tool reached through
a `tools` object inside a V8 isolate. That is deliberately out of scope.

Luna's `multi_agent_version: v1` (vs v2 on Sol and Terra) is also out of
scope. It only changes the sub-agent tool set, which fabro registers from
the caller rather than the profile, and fabro's current set matches
neither version exactly.

Two server cancel-timing tests are adjusted. `gpt-5.6-sol` is the
`openai` provider's default model, so runs that name no model now build a
3-tool profile instead of an 8-tool one and reach their first stage
sooner. `full_http_lifecycle_cancel` asserted `status.kind == "blocked"`
at the instant of cancel, which the worker is free to change the moment it
is signaled; it now accepts either live state, matching the tolerance its
own comment already documents for `pending_control`.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
2026-07-25 10:42:32 -04:00
Bryan Helmkamp
c81ea69c73
Merge origin/main into feat/inference-observability 2026-07-24 22:55:15 -04:00
Bryan Helmkamp
d4f619bc2a
fix: clean up inference observability 2026-07-24 22:50:00 -04:00
Bryan Helmkamp
fea249b4b6
Merge pull request #631 from fabro-sh/feat/kimi-agent-profile
feat(agent): add a Kimi agent profile for Moonshot and gateway routes
2026-07-24 22:20:50 -04:00
Bryan Helmkamp
6659ae768a
feat(events): make inference in-flight state observable
During a long LLM turn the durable event stream was silent: between
`agent.tool.completed` and the next `agent.message` nothing was emitted,
so "the model is generating" and "the worker is wedged" were
indistinguishable from the run store, SSE, or the UI.

The signal already existed. `AssistantTextStart` fired at exactly the
right point — after `build_request()`, after compaction, immediately
before the stream opens — then was classified as streaming noise and
thrown away. This promotes it rather than inventing a new one.

Two events, each asserting only what is provable when it is emitted:

- `agent.llm.started` carries the *requested* provider/model. No usage,
  no cost, no context window: none of it exists yet, and failover can
  re-target, so `agent.message` stays authoritative for what answered.
- `agent.llm.first_output` is edge-triggered on the first output of an
  attempt and names what arrived. `ToolCall` is required, not optional:
  a turn that opens with a tool call produces no text or reasoning
  delta, so a latch keyed on those two would stay silent for exactly
  the tool-heavy rounds where liveness matters most.

`agent.llm.retry` now also fires on the one previously invisible
mid-turn path — a stream that ends without a finish event, which
replays the turn and discards its output with nothing to show for it.
Its `attempt` field was already fed by two independent counters, so an
optional `phase` (open | consume) names which loop it counts.

`StageProjection.inference` projects the open bracket. `Some` means
"the event log contains an unclosed inference bracket", not "the model
is computing now" — a SIGKILLed worker leaves it open, which is the
truthful statement of what we know, and `watchdog.timeout` remains the
authority on actually-stuck.

The close is the subtle part. Terminal cancel and wall-clock timeout
tear the session down through `discard_session` without emitting a
message, error, or interrupt, so a session-lifecycle backstop is
required. It has to be `agent.session.ended`, not
`agent.session.deactivated`: deactivation is emitted by `lease.release()`
*before* the forwarder drains queued agent events, so a queued
`agent.llm.started` can arrive after it and re-open the bracket. But
`agent.session.ended` carries no stage identity, so the close takes
ordering from the event and identity from the projection, scanning for
brackets the ending session opened. A normal stage lookup there finds
no target and silently no-ops.

Presentation states what the log proves and nothing more: no progress
bar or ETA (no completion estimate exists), "reasoning" only when the
provider sent reasoning output, elapsed counted since the request
opened, and no live animation once the run is terminal.

Scope is session-backed agent stages. One-shot completions call
`client.complete` directly and never build a session; covering them
means moving the emit point into `fabro-llm`, filed as a follow-up.

`agent.output.start` was never persisted — it existed in a name map,
an `unreachable!` arm, and docs — so the rename carries no migration
risk. Corrects `events.md`, which documented it as a real emitted
event, and the v2 proposal, which mapped it to `message.part.started`
despite it firing before the request opens.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
2026-07-24 21:47:51 -04:00
Bryan Helmkamp
7501ada9e6
Merge pull request #629 from fabro-sh/fix/compaction-empty-summary-guard
fix(agent): refuse to truncate history on a degenerate compaction summary
2026-07-24 21:27:51 -04:00
Release Repro
af647aba4b
fix(cli): avoid duplicate compaction error prefix 2026-07-24 21:24:31 -04:00
Release Repro
7eef7652d3
fix(agent): harden compaction failure handling
Accept every nonblank summary instead of applying an arbitrary length heuristic. Preserve typed compaction failures and their source chains, suppress repeat attempts within one input, and clear the CLI compaction indicator when the existing agent error event arrives.
2026-07-24 20:57:57 -04:00
Bryan Helmkamp
fbff6f5774
refactor(agent): model built-in tools as an enum with per-profile vocabularies
Tool names were string literals matched in several places, which made
renaming a tool for one profile unsafe: `tool_category` falls back to `Shell`
for an unrecognized name, so exposing `Read` instead of `read_file` would have
silently demanded shell-level approval for every file read.

Introduce `NativeTool`, the closed set of tools fabro implements, with strum
string conversions per the repo convention. A tool is an identity; a name is
one rendering of it. `ToolVocabulary` names the renderings -- fabro's own, and
Kimi Code's -- and `NativeTool::from_any_name` resolves a name in any
vocabulary back to the identity. Permissions, categories, and telemetry go
through that resolution, so behavior no longer depends on which profile is
running.

`known_tool_category` is now an exhaustive match on the enum rather than a
string match, so a new built-in tool has to state its category instead of
silently inheriting the unknown-tool default. Tools that are uncategorized
today stay uncategorized: giving them a category would change the CLI
permission gate, which is a behavior change rather than a cleanup.

MCP, skill, and run-scoped tools keep arbitrary string names, so
`ToolDefinition.name` and the registry keys stay `String`. The enum covers the
closed set only.

With that in place, the Kimi profile exposes its tools under Kimi Code's
vocabulary -- Read, Write, Edit, Bash, Grep, Glob, WebSearch, FetchURL -- and
its prompt and tool descriptions use those names. Tools with no Kimi Code
counterpart of the same shape keep fabro's names. Ask Fabro's tool policy
resolves through the canonical name so a Kimi-model run is not denied its
whole tool set.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
2026-07-24 20:04:11 -04:00
Bryan Helmkamp
c08e5c5490
feat(agent): add a Kimi agent profile for Moonshot and gateway routes
Kimi models ran on the OpenAI profile, which exists to look like Codex. Give
them their own profile derived from Kimi Code's system prompt.

Routing is per model, not per provider, because Kimi models are served both
directly by Moonshot and through gateways. `kimi` sets agent_profile at the
provider level; the Kimi model rows on `openrouter` set it individually, so a
gateway route behaves like the direct one while other OpenRouter models keep
the provider's OpenAI profile.

The profile targets a measured failure. Across two observed K3 implementation
stages, 32 of 35 tool failures were the same thing: writes to files the model
had not read, rejected by the workspace read-before-write guard, or
`old_string` values reconstructed from memory rather than taken from a read.
Kimi Code drills this rule in its own tool descriptions, so the profile does
too -- `edit_file` and `write_file` carry Kimi-specific descriptions naming the
guard and the failure text the model will see, alongside a "Reading Before
Writing" section in the system prompt. Profiles own their tool registries, so
this re-describes the tools for Kimi only; every other profile is untouched and
the executors and JSON schemas are shared unchanged.

Tool names stay fabro's existing snake_case. Whether Kimi Code's PascalCase
vocabulary measurably helps is untested, and renaming would also mean updating
the name-keyed categories in tool_permissions.rs, where an unknown tool falls
back to Shell. That is a separate change to make on evidence.

The prompt is a subtractive port: capabilities fabro does not have -- plan
mode, background tasks, cron, subagent swarms, the cwd tree listing -- are
dropped rather than promised. The shell timeout default matches Kimi Code's 60s
and memory discovery reads AGENTS.md, which is the only instruction file Kimi
Code looks for.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
2026-07-24 19:50:58 -04:00
Bryan Helmkamp
63c952e380
Merge pull request #628 from fabro-sh/refactor/agent-md-prompts
Some checks are pending
Rust / Format (push) Waiting to run
Rust / Clippy (push) Waiting to run
Rust / Generated Docs (push) Waiting to run
Rust / Test (Linux) (push) Waiting to run
Rust / Test (macOS) (push) Waiting to run
TypeScript / Typecheck (push) Waiting to run
TypeScript / Test (push) Waiting to run
TypeScript / Build (push) Waiting to run
refactor(agent): render profile system prompts with minijinja templates
2026-07-24 19:31:48 -04:00