From eb4891b1b07fa01e820809b4a8391602b91118c8 Mon Sep 17 00:00:00 2001 From: Bryan Helmkamp Date: Fri, 22 May 2026 21:51:45 -0400 Subject: [PATCH] refactor(agent): simplify reviewed changes Use raw sandbox reads for memory and skills, keep line-numbered reads focused on display, and share retry-delay handling across agent and LLM code. Trim task tool descriptions, bound multi-file read concurrency, restore Docker's text read path, and add the reviewed implementation plan docs. --- ...05-21-wall-and-active-time-metrics-plan.md | 163 +++++++ ...001-fix-mcp-create-schema-mismatch-plan.md | 309 ++++++++++++ ...ove-session-sandboxes-feature-flag-plan.md | 55 +++ .../2026-05-22-ask-fabro-sidebar-wiring.md | 160 +++++++ docs/public/reference/sdk.mdx | 2 + ...5-22-agent-context-observability-events.md | 438 ++++++++++++++++++ ...2026-05-22-run-agent-fabro-tools-opt-in.md | 140 ++++++ ...6-05-22-unified-agent-transcript-events.md | 291 ++++++++++++ .../2026-05-23-llm-input-token-counting.md | 381 +++++++++++++++ lib/crates/fabro-agent/README.md | 6 +- lib/crates/fabro-agent/src/memory.rs | 2 +- lib/crates/fabro-agent/src/session.rs | 20 +- lib/crates/fabro-agent/src/skills.rs | 2 +- lib/crates/fabro-agent/src/todo_tools.rs | 179 +------ lib/crates/fabro-agent/src/tools.rs | 26 +- lib/crates/fabro-llm/src/retry.rs | 28 +- lib/crates/fabro-sandbox/src/docker.rs | 22 +- lib/crates/fabro-sandbox/src/sandbox.rs | 8 +- lib/crates/fabro-sandbox/src/worktree.rs | 10 - 19 files changed, 2028 insertions(+), 214 deletions(-) create mode 100644 docs/plans/2026-05-21-wall-and-active-time-metrics-plan.md create mode 100644 docs/plans/2026-05-22-001-fix-mcp-create-schema-mismatch-plan.md create mode 100644 docs/plans/2026-05-22-002-remove-session-sandboxes-feature-flag-plan.md create mode 100644 docs/plans/2026-05-22-ask-fabro-sidebar-wiring.md create mode 100644 docs/superpowers/plans/2026-05-22-agent-context-observability-events.md create mode 100644 docs/superpowers/plans/2026-05-22-run-agent-fabro-tools-opt-in.md create mode 100644 docs/superpowers/plans/2026-05-22-unified-agent-transcript-events.md create mode 100644 docs/superpowers/plans/2026-05-23-llm-input-token-counting.md diff --git a/docs/plans/2026-05-21-wall-and-active-time-metrics-plan.md b/docs/plans/2026-05-21-wall-and-active-time-metrics-plan.md new file mode 100644 index 000000000..055ae856c --- /dev/null +++ b/docs/plans/2026-05-21-wall-and-active-time-metrics-plan.md @@ -0,0 +1,163 @@ +--- +title: "feat: Wall and active time metrics" +type: feature +status: active +date: 2026-05-21 +--- + +# feat: Wall and active time metrics + +## Summary + +Rename runtime duration concepts from ambiguous duration/runtime/elapsed fields +to explicit wall-time fields, then add first-class active timing. + +Definitions: + +- `wall_time_ms`: elapsed clock time from start to finish. +- `inference_time_ms`: Fabro-observed LLM request/stream elapsed time. +- `tool_time_ms`: tool or command execution elapsed time. +- `active_time_ms`: `inference_time_ms + tool_time_ms`. + +This is greenfield API churn. Do not preserve old public run/stage timing +fields, aliases, or compatibility shims for `duration_ms`, `runtime_secs`, or +`elapsed_secs` on run/stage runtime surfaces. + +Run-level active time is total work performed: sum active timing across stage +visits. Parallel work is summed, so run active time can exceed run wall time. + +## Key Changes + +- Add a shared timing value object in `fabro-types` for stage/run active timing: + - `wall_time_ms` + - `inference_time_ms` + - `tool_time_ms` + - serialized `active_time_ms` derived by constructors/accessors from + `inference_time_ms + tool_time_ms`; do not let callers supply an + independent active-time value. +- Replace run/stage public timing fields: + - stage/run terminal event props use `wall_time_ms` plus the active timing + breakdown. + - `StageProjection` stores the timing breakdown instead of stage + `duration_ms`. + - `RunTimestamps` keeps timestamps only; move elapsed values into a separate + run timing object. + - `/runs/{id}/stages` and `/runs/{id}/billing` expose timing in milliseconds, + not `runtime_secs`. +- Keep `duration_ms` only for unrelated subsystem-specific operational events + where the name is still local and unambiguous, such as sandbox setup, + metadata snapshot, devcontainer lifecycle, and hook execution. The cleanup + target is public run/stage runtime semantics. +- Update OpenAPI and regenerate the Rust and TypeScript API clients after + schema edits. + +## Timing Behavior + +- `prompt` nodes: + - inference = elapsed time spent in the one-shot LLM backend call. + - tool = 0. +- native `agent` nodes: + - inference = sum of elapsed time spent opening/consuming LLM streams for new + turns in the stage. + - tool = sum of elapsed time spent executing agent tool calls. + - retry backoff and waiting for steering are wall time, not active time. +- opaque external/ACP agent nodes: + - inference = 0 for v1 because Fabro cannot reliably separate model time from + process runtime. + - tool = external agent process wall time. +- `command` nodes: + - inference = 0. + - tool = command wall time from the sandbox command result. +- `human`, `wait`, `conditional`, `fan-in`, `start`, and `exit`: + - inference = 0. + - tool = 0. +- `parallel` container nodes: + - active = 0 on the container stage. + - child/branch stages carry work timing so rollups do not double count. + +## Implementation + +- In `fabro-types`, introduce the timing structs and replace the relevant fields + in `Outcome`, `NodeResult` consumers, `StageProjection`, `Conclusion`, + `RunTimestamps`, `RunCompletedProps`, `RunFailedProps`, + `StageCompletedProps`, `StageFailedProps`, `RunBillingStage`, and + `RunBillingTotals`. +- In `fabro-workflow`, rename run/stage execution fields from `duration_ms` to + `wall_time_ms` and thread timing through lifecycle events, terminal events, + conclusion building, pull request summaries, timeline/billing rollups, and + test support fixtures. +- In `fabro-agent`, add timing data to agent events or session results so + `fabro-workflow` can aggregate: + - LLM stream/request elapsed time per assistant response. + - tool call elapsed time per tool completion. + - preserve token billing behavior separately from timing. +- In `fabro-store`, update event projection to write stage `started_at`, timing + breakdowns, and run summary timing from the new event props. +- In `fabro-server`, replace runtime billing aggregation with a timing rollup + owned by workflow/projection code. Billing endpoints may include timing, but + billing logic should not define timing semantics. +- In `apps/fabro-web`, update run list/detail/stages/billing views and tests to + render wall time and active time from the new fields. +- Remove all run/stage public API references to old timing names from + `docs/public/api-reference/fabro-api.yaml` and regenerated clients. + +## Test Plan + +- `fabro-types`: + - run and stage event round trips serialize the new timing payloads. + - old public run/stage timing properties are absent from serialized fixtures. + - API-facing timing structs round trip through generated schemas. +- `fabro-store`: + - `stage.started` records `started_at`. + - stage terminal events store `wall_time_ms` and active breakdowns. + - run summaries expose timestamp fields and run timing without + `elapsed_secs`. + - retried stages reset per-attempt live wall-time state correctly. +- `fabro-workflow`: + - prompt stages report inference-only active timing. + - command stages report tool-only active timing. + - native agent stages sum LLM turn timing and tool timing. + - human/wait/conditional/fan-in/start/exit stages report zero active timing. + - parallel stage rollups sum child active work and avoid container double + counting. + - repeated node visits sum timing by node in rollups. +- `fabro-server`: + - `/runs/{id}/stages`, `/runs/{id}/billing`, run detail, and run list return + new timing fields only. + - aggregate billing/timing totals sum active work across completed runs. + - OpenAPI conformance passes after regeneration. +- `apps/fabro-web`: + - run list/detail/billing/stages render wall time and active time. + - in-flight wall-time ticking still uses `started_at`. + - no UI code reads `runtime_secs`, `elapsed_secs`, or run/stage + `duration_ms`. + +## Validation + +Run focused checks first: + +```bash +cargo nextest run -p fabro-types -p fabro-store -p fabro-workflow -p fabro-server +cd apps/fabro-web && bun test && bun run typecheck +``` + +Then run full workspace checks before merging: + +```bash +cargo build --workspace +cargo nextest run --workspace +cargo +nightly-2026-04-14 fmt --check --all +cargo +nightly-2026-04-14 clippy --workspace --all-targets -- -D warnings +git diff --check +``` + +## Assumptions + +- Inference time is Fabro-observed LLM request/stream elapsed time, not + provider-reported model-only compute time. +- LLM retry backoff, queueing outside a request/stream, human waits, steering + waits, and scheduler gaps are wall time but not active time. +- Active timing is finalized-event based in v1; live active-time ticking can be + added later if it becomes necessary. +- No compatibility layer is required for existing API clients or stored run + event data. diff --git a/docs/plans/2026-05-22-001-fix-mcp-create-schema-mismatch-plan.md b/docs/plans/2026-05-22-001-fix-mcp-create-schema-mismatch-plan.md new file mode 100644 index 000000000..18f953949 --- /dev/null +++ b/docs/plans/2026-05-22-001-fix-mcp-create-schema-mismatch-plan.md @@ -0,0 +1,309 @@ +--- +title: fix: Align fabro_run_create MCP schema and accepted input +type: fix +status: active +date: 2026-05-22 +--- + +# fix: Align fabro_run_create MCP schema and accepted input + +## Overview + +Fix the mismatch where MCP clients see `fabro_run_create` as accepting +`runs: string[]`, while the running server currently deserializes +`runs` as an array of `CreateRunSpec` objects. The fix should make the +tool robust for agents that follow the advertised string shorthand while +also preserving the richer object form used by existing tests and callers. + +## Problem Frame + +During manual MCP testing, this call failed before tool validation: + +```json +{ "runs": ["sleeper"] } +``` + +The server returned a deserialization error because it expected a +`CreateRunSpec` object. This is a poor agent-facing failure mode: the +client-visible schema implied the call was valid, but the runtime contract +rejected it. The object form still worked: + +```json +{ "runs": [{ "workflow": "sleeper", "auto_approve": true, "start": true }] } +``` + +## Requirements Trace + +- R1. `fabro_run_create` must accept the string shorthand advertised to MCP + clients, treating each string as the workflow selector. +- R2. Existing object-form `CreateRunSpec` calls must keep working with all + current optional fields. +- R3. MCP `tools/list` must advertise a truthful schema for `runs` so clients + can discover both accepted forms, or at minimum no longer advertise only a + shape that fails at runtime. +- R4. Local validation errors must remain tool errors and must still happen + before auth or network access. +- R5. Docs and QA notes should show the supported shapes so future manual + testing does not rediscover the mismatch. + +## Scope Boundaries + +- Do not change the HTTP run creation API. +- Do not change manifest resolution semantics for object-form create specs. +- Do not add new create-run options beyond accepting string shorthand. +- Do not make `client_message_id`, pair APIs, or other MCP tools part of this + fix. + +## Context & Research + +### Relevant Code and Patterns + +- `lib/crates/fabro-tool/src/create.rs` owns `FabroRunCreateParams`, + `CreateRunSpec`, validation, and create-run execution. +- `lib/crates/fabro-mcp-server/src/server.rs` registers the MCP tool using + `Parameters`. +- `lib/crates/fabro-cli/tests/it/cmd/mcp.rs` already exercises object-form + `fabro_run_create`, schema listing, and pre-auth validation. +- `docs/internal/mcp-server-qa-test-plan.md` already records past + schema/runtime mismatches, especially the `inputs` schema narrowing. +- `docs/public/agents/mcp.mdx` lists the MCP tools but does not show concrete + `fabro_run_create` input examples. + +### Institutional Learnings + +- No `docs/solutions/` directory exists in this checkout. +- The MCP QA plan shows this class of bug has appeared before: schema/runtime + agreement for MCP parameters needs explicit tests, not only happy-path calls. + +### External References + +- None. The issue is internal schema/deserialization parity; local patterns are + sufficient. + +## Key Technical Decisions + +- Accept both string shorthand and object specs. This is additive, fixes the + observed failed call directly, and preserves existing rich create semantics. +- Normalize inputs before validation. Convert raw string/object specs into the + existing `ValidatedCreateRunSpec` path so all downstream manifest and run + creation behavior stays centralized. +- Add schema assertions near the MCP boundary. Unit validation alone cannot + catch a client-visible `tools/list` regression. + +## Open Questions + +### Resolved During Planning + +- Should the object form remain supported? Yes. Existing tests and the MCP + design require optional create settings like `dry_run`, `auto_approve`, + `labels`, `parent_id`, and `start`. +- Should string shorthand be treated as workflow selector only? Yes. It maps + cleanly to the one required object-form field. + +### Deferred to Implementation + +- Exact schema shape: use `anyOf`/`oneOf`, inline schemas, or a manual + `JsonSchema` implementation depending on how `schemars` and `rmcp` emit the + final `tools/list` schema. + +## Implementation Units + +- [ ] **Unit 1: Characterize and lock the current schema mismatch** + +**Goal:** Add failing coverage that proves `fabro_run_create` advertises and +accepts the intended `runs` item shapes. + +**Requirements:** R1, R3, R4 + +**Dependencies:** None + +**Files:** +- Modify: `lib/crates/fabro-mcp-server/src/server.rs` +- Modify: `lib/crates/fabro-cli/tests/it/cmd/mcp.rs` + +**Approach:** +- Add a server-level schema test for `fabro_run_create`, similar to the + existing `fabro_run_pair` schema leakage test. +- Assert the schema for `runs` does not collapse to string-only if the runtime + requires object fields. +- Add an MCP stdio integration case that calls `fabro_run_create` with + `runs: ["simple.fabro"]` against an unreachable server and verifies local + parameter validation succeeds far enough to require backend/auth, not fail + with `expected struct CreateRunSpec`. + +**Execution note:** Characterization-first. Capture the failing schema/runtime +contract before changing deserialization. + +**Patterns to follow:** +- `fabro_run_pair_tool_is_registered_with_stage_based_schema` in + `lib/crates/fabro-mcp-server/src/server.rs`. +- `mcp_create_validation_errors_happen_before_auth_or_network` in + `lib/crates/fabro-cli/tests/it/cmd/mcp.rs`. + +**Test scenarios:** +- Integration: `tools/list` for `fabro_run_create` exposes `runs` as an array + whose items include the object form with a required `workflow` field. +- Error path: calling `fabro_run_create` with `runs: ["simple.fabro"]` no + longer returns an MCP deserialization error mentioning `CreateRunSpec`. +- Error path: malformed non-string/non-object run items still fail before + auth/network with an actionable tool or MCP parameter error. + +**Verification:** +- The new tests fail against the current behavior and identify the mismatch + without requiring a live Fabro API server. + +- [ ] **Unit 2: Add string shorthand normalization for run create specs** + +**Goal:** Make `runs: ["workflow"]` behave like +`runs: [{ "workflow": "workflow" }]`. + +**Requirements:** R1, R2, R4 + +**Dependencies:** Unit 1 + +**Files:** +- Modify: `lib/crates/fabro-tool/src/create.rs` +- Test: `lib/crates/fabro-tool/src/create.rs` +- Test: `lib/crates/fabro-cli/tests/it/cmd/mcp.rs` + +**Approach:** +- Introduce a raw input representation for create specs that can deserialize + either a string workflow selector or the current object form. +- Normalize both raw forms into the existing validated create spec structure + before calling manifest resolution or backend methods. +- Preserve all existing object-form field handling and validation. +- Treat blank string workflows as invalid local input with a clear tool error. + +**Patterns to follow:** +- `AnswerValue` in `lib/crates/fabro-tool/src/interact.rs` for custom + schema/deserialization where the MCP surface needs a flexible input value. +- `RunInputValue` in `lib/crates/fabro-tool/src/create.rs` for schema-driven + input constraints and local conversion. + +**Test scenarios:** +- Happy path: `runs: ["simple.fabro"]` creates the same validated spec as + `runs: [{ "workflow": "simple.fabro" }]`. +- Happy path: object form with `dry_run`, `auto_approve`, `labels`, and + `start` continues to pass through unchanged. +- Edge case: `runs: [" "]` returns a local validation error naming the + workflow value. +- Error path: `runs: []` and 51 entries retain the existing min/max errors. +- Integration: string shorthand reaches the backend path in the MCP integration + harness, proving it is not rejected by the MCP deserializer. + +**Verification:** +- Existing object-form MCP create tests still pass. +- String shorthand can start a run in the same manual scenario that previously + failed. + +- [ ] **Unit 3: Make the advertised MCP schema client-friendly** + +**Goal:** Ensure MCP clients can discover the actual supported input contract. + +**Requirements:** R2, R3 + +**Dependencies:** Unit 2 + +**Files:** +- Modify: `lib/crates/fabro-tool/src/create.rs` +- Modify: `lib/crates/fabro-mcp-server/src/server.rs` +- Test: `lib/crates/fabro-mcp-server/src/server.rs` +- Test: `lib/crates/fabro-cli/tests/it/cmd/mcp.rs` + +**Approach:** +- Prefer a schema where `runs.items` clearly advertises both supported forms: + a workflow string shorthand and the object-form create spec. +- If `schemars` emits `$defs` that client tooling misinterprets, inline the + relevant schema or provide a manual `JsonSchema` implementation for the raw + create-spec input. +- Keep the schema descriptive rather than loosening it to arbitrary JSON. + +**Patterns to follow:** +- Manual `JsonSchema` implementations in `RunInputValue` and `AnswerValue`. +- Existing MCP schema assertions in `mcp.rs` that verify property schemas are + objects and startup listing remains fast. + +**Test scenarios:** +- Happy path: `tools/list` schema for `fabro_run_create` contains the string + shorthand branch. +- Happy path: `tools/list` schema for `fabro_run_create` contains the object + branch with `workflow`. +- Error path: schema does not advertise unsupported array/object input values + for `inputs`; existing scalar-only assertion remains true. +- Integration: listing tools still does not construct the API client. + +**Verification:** +- An MCP client inspecting `tools/list` can infer at least one valid shape that + the runtime accepts. + +- [ ] **Unit 4: Update docs and QA checklist** + +**Goal:** Record the supported `fabro_run_create` shapes and the regression + test so future manual testing uses the right contract. + +**Requirements:** R5 + +**Dependencies:** Unit 2, Unit 3 + +**Files:** +- Modify: `docs/public/agents/mcp.mdx` +- Modify: `docs/internal/mcp-server-qa-test-plan.md` + +**Approach:** +- Add a small `fabro_run_create` example showing both shorthand and object + form, with object form recommended when options are needed. +- Add a QA note that this schema/runtime mismatch was fixed and should remain + covered by schema-discovery and shorthand-call tests. + +**Patterns to follow:** +- Existing terse MCP tool table in `docs/public/agents/mcp.mdx`. +- Existing resolved issue notes at the top of + `docs/internal/mcp-server-qa-test-plan.md`. + +**Test scenarios:** +- Test expectation: none -- documentation-only unit. + +**Verification:** +- Public docs show an input shape that works when pasted into an MCP client. +- QA plan names the regression and where it is covered. + +## System-Wide Impact + +- **Interaction graph:** MCP clients call `tools/list`, infer parameter shape, + and then call `tools/call`; this fix aligns both surfaces with the same + deserializer. +- **Error propagation:** Invalid local input should continue to return MCP tool + errors without killing the stdio server. Framework-level JSON type errors + should only remain for truly unsupported JSON shapes. +- **State lifecycle risks:** No persistent data migration. The only durable + effect is successful run creation for shorthand calls that previously failed. +- **API surface parity:** HTTP run creation remains unchanged. This is an MCP + tool input compatibility fix. +- **Integration coverage:** Unit tests cover normalization; MCP stdio tests + cover real schema discovery and tool-call deserialization. +- **Unchanged invariants:** Object-form create specs remain the full-fidelity + path for labels, parent links, options, and overrides. + +## Risks & Dependencies + +| Risk | Mitigation | +|------|------------| +| Schema becomes too loose and agents send unsupported values | Use an explicit string-or-object schema and keep local validation narrow | +| Object-form callers regress while adding shorthand | Keep existing tests and add object-form pass-through assertions | +| MCP client tooling still summarizes the schema poorly | Make runtime accept the string shorthand so the summarized `string[]` shape still works | +| Validation accidentally moves after auth/network setup | Keep validation tests using an unreachable server target | + +## Documentation / Operational Notes + +- This fix should be called out as an MCP UX/compatibility fix, not an HTTP API + change. +- Manual verification should include the exact previously failing call: + `fabro_run_create({ "runs": ["sleeper"] })`. + +## Sources & References + +- Related code: `lib/crates/fabro-tool/src/create.rs` +- Related code: `lib/crates/fabro-mcp-server/src/server.rs` +- Related tests: `lib/crates/fabro-cli/tests/it/cmd/mcp.rs` +- Related QA doc: `docs/internal/mcp-server-qa-test-plan.md` +- Related docs: `docs/public/agents/mcp.mdx` diff --git a/docs/plans/2026-05-22-002-remove-session-sandboxes-feature-flag-plan.md b/docs/plans/2026-05-22-002-remove-session-sandboxes-feature-flag-plan.md new file mode 100644 index 000000000..1c8dc9f82 --- /dev/null +++ b/docs/plans/2026-05-22-002-remove-session-sandboxes-feature-flag-plan.md @@ -0,0 +1,55 @@ +# Remove `features.session_sandboxes` + +## Summary + +Remove the `session_sandboxes` feature flag and the now-empty `[features]` settings namespace entirely. Behavior should be as if `session_sandboxes = true` was always set: Ask Fabro is never disabled by a feature flag, and UI controls previously hidden behind the flag are always shown. + +## Key Changes + +- Remove the settings namespace from config: + - Delete `FeaturesNamespace`, `FeaturesLayer`, `resolve_features`, and `[features]` defaults. + - Remove `features` from resolved `ServerSettings` and `UserSettings`. + - Remove `features` from the top-level settings parser allow-list, so old `[features]` config is rejected as unknown. +- Remove the runtime gate: + - Simplify Ask Fabro readiness to check only sandbox presence/runtime and LLM configuration. + - Remove `AskFabroUnavailableReason::FeatureDisabled` and the "Ask Fabro is disabled" tooltip. +- Update frontend behavior: + - Run detail page no longer handles `FEATURE_DISABLED`. + - Start page always renders the project/branch controls and no longer fetches system info just for this flag. +- Remove public API surfaces: + - `/api/v1/settings` `ServerSettings` no longer includes `features`. + - `/api/v1/system/info` no longer includes `features`. + - OpenAPI removes `FeaturesNamespace`, `SystemFeatures`, `ServerSettings.features`, `SystemInfoResponse.features`, and `feature_disabled`. + - Regenerate Rust API types and TypeScript Axios client. +- Update current docs: + - Remove `[features]` from active configuration docs, generated options docs, API docs, and unknown-key guidance. + - Do not touch unrelated meanings of "features" such as Cargo features, LLM model features, or devcontainer features. + +## Test Plan + +- Update or remove tests that assert `features.session_sandboxes` in config, settings, system info, and Ask Fabro readiness. +- Add or adjust coverage for: + - Ask Fabro unavailable reasons are only `no_sandbox`, `sandbox_not_ready`, or `llm_unconfigured`. + - Settings parsing rejects top-level `[features]`. + - `/api/v1/settings` response contains only `server` at the top level. + - `/api/v1/system/info` has no `features` field. + - Start page renders project/branch controls without consulting `SystemInfo.features`. +- Run: + - `cargo build -p fabro-api` + - `cd lib/packages/fabro-api-client && bun run generate` + - `cargo dev docs refresh && cargo dev docs check` + - `cargo nextest run -p fabro-config -p fabro-api -p fabro-server -p fabro-cli` + - `cd apps/fabro-web && bun test && bun run typecheck` + - `cargo +nightly-2026-04-14 fmt --check --all` + - `cargo +nightly-2026-04-14 clippy --workspace --all-targets -- -D warnings` + +## Acceptance Checks + +- `rg -n "session_sandboxes|FeaturesNamespace|SystemFeatures|feature_disabled|Ask Fabro is disabled" lib apps docs/public` returns no relevant matches. +- `rg -n "\\[features\\]" docs/public lib/crates/fabro-config/src lib/crates/fabro-types/src lib/crates/fabro-server/src apps/fabro-web/app` returns no settings-namespace matches. +- Existing sandbox runtime behavior remains unchanged; only the feature flag and schema surface are removed. + +## Assumptions + +- This is intentionally a breaking config/API cleanup: existing user config containing `[features]` should fail validation until removed. +- Historical internal plans may still contain old text unless they are part of active public docs; implementation should prioritize product code, generated clients, and current docs. diff --git a/docs/plans/2026-05-22-ask-fabro-sidebar-wiring.md b/docs/plans/2026-05-22-ask-fabro-sidebar-wiring.md new file mode 100644 index 000000000..bb9c4ff96 --- /dev/null +++ b/docs/plans/2026-05-22-ask-fabro-sidebar-wiring.md @@ -0,0 +1,160 @@ +# Ask Fabro Sidebar Wiring — Implementation Plan + +> **For agentic workers:** Use superpowers:subagent-driven-development or superpowers:executing-plans. Steps use `- [ ]` checkboxes. + +**Goal:** Ship the Ask Fabro sidebar on run pages — wired to real session APIs, with the agent able to inspect its owning run. + +**Architecture:** Two phases. Phase 1 (Rust): give Ask Fabro agent sessions the read-only `fabro_run_get` + `fabro_run_events` tools, scoped to the owning run, via the existing run-tool service path. Reuse `register_named_fabro_run_tools`; do not add a second subset registration helper. Phase 2 (web): replace the scripted sidebar adapter with real session calls; drop `?ask=1`, gate on `run.ask_fabro.available`. + +**Tech Stack:** Rust (fabro-server, fabro-workflow, fabro-tool), React 19 + assistant-ui (fabro-web), generated API clients. + +**Decisions locked:** +- Reuse `fabro-tool` — no new tool. +- Subset = `fabro_run_get` + `fabro_run_events`, read-only run inspection. +- Backend = existing `FabroRunToolServices` registration path. Prefer in-process server/store access when adding new same-process backends; avoid new loopback URL plumbing. +- Scoped to owning run — enforced by a same-run worker token; the API 403s cross-run calls. +- File/shell tools stay read-only in Ask Fabro sessions. +- Gate the sidebar on `run.ask_fabro.available`; drop `?ask=1`. + +--- + +## Background (current state) + +- API endpoints exist (`296fbddec`): `SessionDetail`, `/sessions/{id}/events`, `/sessions/{id}/attach`, turn submission w/ `x-fabro-turn-id`, `Run.ask_fabro` readiness. Web helpers exist: `session-stream.ts`, `sessionsApi`. +- Sidebar (`ask-fabro-sidebar.tsx`) is a prototype: scripted adapter (`chats-runtime.ts`/`chats-script.ts`), no API calls, gated behind `?ask=1` (`run-detail.tsx:363`). +- Ask Fabro sessions built by `build_agent_session` (`fabro-server/.../handler/sessions.rs`): profile + run sandbox + a read-only gate. +- `fabro-tool`: tools built on the `FabroToolBackend` trait. `FabroRunToolServices`, `register_fabro_run_tools`, `register_named_fabro_run_tools`, and `execute_fabro_run_tool` live in `fabro-workflow` (`handler/llm/api.rs`, `services.rs`). +- Worker tokens: `worker_token.rs` — `issue_worker_token(keys, &run_id)` mints a base same-run token; `AppState::worker_token_keys()` exposes the keys. `server.rs` already mints tokens this way for dispatched workers. +- `register_fabro_run_tools` is `pub(crate)`; `fabro-server` already depends on `fabro-workflow`. + +--- + +## File structure + +**Phase 1 — Rust** +- Modify: `lib/crates/fabro-server/src/server/handler/sessions.rs` — register the named read-only run tools and allowlist them in the gate. + +**Phase 2 — Web** +- Create: `apps/fabro-web/app/lib/ask-fabro-runtime.ts` — real session adapter. +- Modify: `apps/fabro-web/app/components/chats/ask-fabro-sidebar.tsx` — use real adapter, take `runId`. +- Modify: `apps/fabro-web/app/routes/run-detail.tsx` — drop `?ask=1`, gate on `run.ask_fabro`. +- Delete (verify orphaned first): `apps/fabro-web/app/lib/chats-script.ts` + scripted paths in `chats-runtime.ts`. + +--- + +## Phase 1: Run tools for Ask Fabro sessions + +### Task 1 — Reuse named run-tool registration + +- [ ] Use `register_named_fabro_run_tools` from `fabro-workflow/src/handler/llm/api.rs`. +- [ ] Register only `fabro_tool::FABRO_RUN_EVENTS_TOOL_NAME` and `fabro_tool::FABRO_RUN_GET_TOOL_NAME`. +- [ ] Do not add another subset helper or duplicate the tool-catalog filtering loop. +- [ ] `cargo build --workspace`; `cargo nextest run -p fabro-workflow agent_run`. + +### Task 2 — Wire scoped run tools into `build_agent_session` + +The session's run-inspection backend is scoped to the owning run. The same-run token remains the authorization backstop for HTTP-backed calls, and any future in-process backend must enforce the same run-id check before executing a tool. + +**Files:** `fabro-server/src/server/handler/sessions.rs` + +- [ ] Change `build_profile` to return `Box` (currently `Arc`); the caller registers tools on `&mut` then `Arc::from`s. +- [ ] In `build_agent_session`, after `build_profile`, before `Session::from_record`: + ```rust + let worker_token = issue_worker_token(state.worker_token_keys(), &run_id) + .map_err(|err| AskFabroBuildError::Agent(anyhow::Error::new(err)))?; + // fabro_client::Client = generated reqwest client from the `fabro-client` crate. + let api_client = fabro_client::Client::new_with_client( + state.self_server_target()?, + reqwest_client_with_bearer(&worker_token), + ); + let backend = fabro_tool::fabro_client::FabroClient::new(Arc::new(api_client)); + let services = FabroRunToolServices { + backend: Arc::new(backend), + current_run_id: run_id, + base_cwd: PathBuf::new(), // unused by events/get + user_settings_path: PathBuf::new(), // unused by events/get + }; + register_named_fabro_run_tools( + profile.tool_registry_mut(), + &services, + &[fabro_tool::FABRO_RUN_EVENTS_TOOL_NAME, fabro_tool::FABRO_RUN_GET_TOOL_NAME], + ); + ``` + Reference impls: the worker-token mint in `server.rs`; `FabroRunToolServices` construction in `fabro-cli/src/commands/run/runner.rs`. +- [ ] Ensure the session prompt/context names the owning run id so the agent passes the correct `run_id` to the tools. (Backstop: wrong id → API 403, agent self-corrects.) +- [ ] `cargo build --workspace`. +- [ ] Commit: `feat(fabro-server): give Ask Fabro sessions run-control tools`. + +### Task 3 — Allowlist the two run tools in the session gate + +`build_ask_fabro_tool_approval` (`sessions.rs`) currently denies everything not `ReadOnly`-approved. The two read-only run tools should be allowed; file/shell stay read-only. + +- [ ] Update the closure: + ```rust + Arc::new(move |tool_name: &str, _args: &Value| { + if matches!(tool_name, "fabro_run_get" | "fabro_run_events") { + return Ok(()); // read-only run-inspection tools, scoped by run id + } + if is_tool_auto_approved(PermissionLevel::ReadOnly, tool_name) { + Ok(()) + } else { + Err(format!("{tool_name} tool denied by Ask Fabro tool policy")) + } + }) + ``` +- [ ] Tests: `fabro_run_get` and `fabro_run_events` approved; `write_file` and shell denied; `read_file` approved. +- [ ] `cargo +nightly-2026-04-14 fmt --all && cargo +nightly-2026-04-14 clippy --workspace --all-targets -- -D warnings`. +- [ ] `cargo nextest run -p fabro-server --features test-support api::sessions`. +- [ ] Commit: `feat(fabro-server): allow run tools through the Ask Fabro session gate`. + +### Task 4 — E2E coverage + +- [ ] E2E test (twin), mirroring `tests/it/api/sessions.rs`: create a run with a sandbox, open an Ask Fabro session, submit a turn asking about run stages — assert the agent calls a read-only run tool and the turn completes. +- [ ] Cross-run guard test: a tool call with a different `run_id` is rejected (worker-token scope). +- [ ] `cargo nextest run -p fabro-server --features test-support --test it api::sessions`. +- [ ] Commit: `test(fabro-server): Ask Fabro run-tool E2E coverage`. + +--- + +## Phase 2: Wire the sidebar + +### Task 5 — Real session adapter + +**Files:** Create `apps/fabro-web/app/lib/ask-fabro-runtime.ts` + +- [ ] assistant-ui adapter parameterized by `runId`: + - First turn: `sessionsApi.createRunSession(runId, { model })` (model from `run.ask_fabro.default_model`); persist session id in `sessionStorage` keyed by `runId` so reopen resumes. + - Open with existing session id: `sessionsApi.getSession(id)` → render `SessionDetail.messages`, then `attachSessionEvents(id, { sinceSeq: last_seq })`. + - Send: `streamSessionTurn(id, { input })`; map streamed `EventEnvelope`s (incl. `run.session.*` tool-call events) to assistant-ui messages. +- [ ] Route tool-call events through the existing `tool-fallback.tsx` renderer. +- [ ] `bun test app/lib/ask-fabro-runtime.test.ts` (mock SSE as `session-stream.test.ts` does). +- [ ] Commit: `feat(web): real session adapter for Ask Fabro sidebar`. + +### Task 6 — Sidebar uses the adapter + +- [ ] `ask-fabro-sidebar.tsx`: accept a `runId` prop; replace `createScriptedAdapter` with `ask-fabro-runtime`; remove `EMPTY_CHAT`/`scriptIndexRef`. +- [ ] `rg createScriptedAdapter` — if `chats-script.ts`/scripted paths are orphaned, delete them. +- [ ] `bun run typecheck`. +- [ ] Commit: `feat(web): drive Ask Fabro sidebar from session API`. + +### Task 7 — Drop `?ask=1`, gate on readiness + +- [ ] `run-detail.tsx`: remove `askEnabled`/`searchParams.get("ask")` (lines ~363-368, 635-648, 724-730). +- [ ] Render the Ask Fabro button always; `disabled={!run.ask_fabro.available}`. Disabled tooltip from `unavailable_reason`: `no_sandbox`/`sandbox_not_ready` → "Run sandbox isn't ready"; `llm_unconfigured` → "No LLM configured". +- [ ] Pass `runId={params.id}` to ``. +- [ ] `bun run typecheck && bun test`. +- [ ] Commit: `feat(web): enable Ask Fabro sidebar on run pages`. + +--- + +## Tests to run before each PR + +- Rust: `cargo +nightly-2026-04-14 fmt --check --all` · `cargo +nightly-2026-04-14 clippy --workspace --all-targets -- -D warnings` · `cargo build --workspace` · `cargo nextest run -p fabro-server -p fabro-workflow` +- Web: `cd apps/fabro-web && bun run typecheck && bun test` + +## Unresolved questions + +1. **`fabro_run_get` payload size** — `get` may return a large `RunProjection` (all stage data). If it blows agent context, consider a trimmed projection. Verify before Task 4. +2. **Session reuse** — one Ask Fabro session per run reused across sidebar opens (plan assumes this via `sessionStorage`), or fresh each open? +3. **Capability scope** — Ask Fabro is read-only in this plan. Mutating run-control tools should be a separate product decision. +4. **Phase split** — Phase 1 + 2 as two PRs (Phase 2 works without 1; agent just lacks run tools), or ship together so the feature only appears once useful? diff --git a/docs/public/reference/sdk.mdx b/docs/public/reference/sdk.mdx index 0acdcc6ac..6208145d9 100644 --- a/docs/public/reference/sdk.mdx +++ b/docs/public/reference/sdk.mdx @@ -130,6 +130,8 @@ The `Sandbox` trait abstracts where tools execute — local filesystem, Docker c ```rust #[async_trait] pub trait Sandbox: Send + Sync { + async fn read_file_bytes(&self, path: &str) -> Result, String>; + async fn read_file_text(&self, path: &str) -> Result; async fn read_file(&self, path: &str, offset: Option, limit: Option) -> Result; async fn write_file(&self, path: &str, content: &str) -> Result<(), String>; async fn delete_file(&self, path: &str) -> Result<(), String>; diff --git a/docs/superpowers/plans/2026-05-22-agent-context-observability-events.md b/docs/superpowers/plans/2026-05-22-agent-context-observability-events.md new file mode 100644 index 000000000..dfc0b712f --- /dev/null +++ b/docs/superpowers/plans/2026-05-22-agent-context-observability-events.md @@ -0,0 +1,438 @@ +# Agent Context Observability Events Implementation Plan + +> **For agentic workers:** REQUIRED SUB-SKILL: Use superpowers:subagent-driven-development (recommended) or superpowers:executing-plans to implement this plan task-by-task. Steps use checkbox (`- [ ]`) syntax for tracking. + +**Goal:** Add durable API/backend events that report loaded memory files, discovered and activated skills, and per-server MCP tool names for agent runs. + +**Architecture:** Keep this API-backend scoped. Emit typed `AgentEvent` variants from the existing `fabro-agent` initialization and skill activation paths, convert them through `fabro-workflow` into durable `fabro-types` run events, and document the event contracts. Do not add run projection fields in this pass; consumers can read the event stream/history. + +**Tech Stack:** Rust, Serde, Fabro agent/session events, Fabro workflow event conversion, Fabro MCP connection manager, `cargo nextest`. + +--- + +## Scope + +Implement these event changes: + +- Add `agent.memory.loaded` with memory file paths, byte counts, loaded byte counts, truncation flags, provider profile, total loaded bytes, and budget bytes. +- Add `agent.skills.discovered` with source directories, provider profile, and sorted skill summaries. +- Add persisted `agent.skill.activated` for slash skill expansion and successful `use_skill` tool calls. +- Enrich `agent.mcp.ready` with names-only tool summaries: qualified tool name and original server tool name. + +Do not implement ACP-native equivalents in this pass. Do not include memory file contents in any event payload. Do not include MCP tool descriptions or schemas. + +## Existing Patterns To Follow + +- Read `docs/internal/events-strategy.md` before changing event variants, names, conversion, or progress JSONL behavior. +- Read `docs/internal/testing-strategy.md` before adding or reorganizing tests. +- Follow the current `AgentEvent` flow: + - `lib/crates/fabro-agent/src/types.rs` + - `lib/crates/fabro-agent/src/session.rs` + - `lib/crates/fabro-workflow/src/handler/llm/api.rs` + - `lib/crates/fabro-workflow/src/event/convert.rs` + - `lib/crates/fabro-workflow/src/event/names.rs` + - `lib/crates/fabro-types/src/run_event/agent.rs` + - `lib/crates/fabro-types/src/run_event/mod.rs` +- Follow Rust import style from `AGENTS.md`: import types by name, import functions through their parent module, and avoid glob imports in production code. + +## File Map + +- Modify `lib/crates/fabro-types/src/run_event/agent.rs`: add new prop structs and extend `AgentMcpReadyProps`. +- Modify `lib/crates/fabro-types/src/run_event/mod.rs`: add `EventBody` variants for the new event names. +- Modify `lib/crates/fabro-agent/src/types.rs`: add internal `AgentEvent` variants, trace output, and noise filtering decisions. +- Modify `lib/crates/fabro-agent/src/memory.rs`: return memory content plus metadata instead of bare strings. +- Modify `lib/crates/fabro-agent/src/session.rs`: emit memory, skills, skill activation, and enriched MCP events. +- Modify `lib/crates/fabro-agent/src/skills.rs`: emit tool-sourced skill activation from `use_skill`. +- Modify `lib/crates/fabro-mcp/src/connection_manager.rs`: expose or support deterministic names-only tool summaries per server. +- Modify `lib/crates/fabro-workflow/src/event/convert.rs`: convert new agent events to durable event bodies. +- Modify `lib/crates/fabro-workflow/src/event/names.rs`: add event names. +- Modify `lib/crates/fabro-workflow/src/event/events.rs` only if the agent event name mapping also lives there for these variants. +- Modify `lib/crates/fabro-workflow/src/event/stored_fields.rs` only if a new event needs non-standard stored fields; otherwise rely on existing `Event::Agent` handling. +- Modify `docs/internal/events.md`: document new event shapes and the richer MCP payload. +- Add or update tests in `lib/crates/fabro-agent`, `lib/crates/fabro-mcp`, `lib/crates/fabro-types`, and `lib/crates/fabro-workflow`. + +--- + +### Task 1: Add Typed Durable Event Contracts + +**Files:** +- Modify: `lib/crates/fabro-types/src/run_event/agent.rs` +- Modify: `lib/crates/fabro-types/src/run_event/mod.rs` +- Test: existing `fabro-types` run event serde tests, or add focused coverage near the existing run event tests. + +- [ ] **Step 1: Add agent memory props** + +Add event prop structs with this shape: + +```rust +pub struct AgentMemoryLoadedProps { + pub provider_profile: String, + pub files: Vec, + pub total_loaded_bytes: usize, + pub budget_bytes: usize, + pub visit: u32, +} + +pub struct AgentMemoryFileProps { + pub path: String, + pub byte_count: usize, + pub loaded_bytes: usize, + pub truncated: bool, +} +``` + +- [ ] **Step 2: Add skill props** + +Add skill discovery and activation props: + +```rust +pub struct AgentSkillsDiscoveredProps { + pub provider_profile: String, + pub source_dirs: Vec, + pub skills: Vec, + pub visit: u32, +} + +pub struct AgentSkillSummary { + pub name: String, + pub description: String, +} + +pub enum AgentSkillActivationSource { + Slash, + Tool, +} + +pub struct AgentSkillActivatedProps { + pub skill_name: String, + pub source: AgentSkillActivationSource, + pub visit: u32, +} +``` + +Use serde names `slash` and `tool` for `AgentSkillActivationSource`. If a local enum string pattern already exists, follow that pattern. + +- [ ] **Step 3: Extend MCP ready props** + +Extend `AgentMcpReadyProps` with a backwards-compatible field: + +```rust +#[serde(default, skip_serializing_if = "Vec::is_empty")] +pub tools: Vec, +``` + +Add: + +```rust +pub struct AgentMcpToolSummary { + pub name: String, + pub original_name: String, +} +``` + +- [ ] **Step 4: Add EventBody variants** + +Add `EventBody` variants using these serialized event names: + +- `agent.memory.loaded` +- `agent.skills.discovered` +- `agent.skill.activated` + +Keep existing `agent.mcp.ready` name unchanged and only enrich its props. + +- [ ] **Step 5: Add serde tests** + +Cover: + +- New event names serialize to the expected dot names. +- `AgentSkillActivationSource` serializes as `slash` and `tool`. +- Old `agent.mcp.ready` JSON without `tools` still deserializes with `tools == []`. + +--- + +### Task 2: Add Internal Agent Events And Conversion + +**Files:** +- Modify: `lib/crates/fabro-agent/src/types.rs` +- Modify: `lib/crates/fabro-workflow/src/event/convert.rs` +- Modify: `lib/crates/fabro-workflow/src/event/names.rs` +- Modify: `lib/crates/fabro-workflow/src/event/events.rs` if needed by the existing name mapping. +- Test: `lib/crates/fabro-workflow` event conversion tests. + +- [ ] **Step 1: Add internal AgentEvent variants** + +Add variants equivalent to: + +```rust +MemoryLoaded { + provider_profile: String, + files: Vec, + total_loaded_bytes: usize, + budget_bytes: usize, +} + +SkillsDiscovered { + provider_profile: String, + source_dirs: Vec, + skills: Vec, +} + +SkillActivated { + skill_name: String, + source: SkillActivationSource, +} + +McpServerReady { + server_name: String, + tool_count: usize, + tools: Vec, +} +``` + +Prefer small shared internal structs near `AgentEvent` if that matches the existing file organization. + +- [ ] **Step 2: Persist skill activation** + +Do not classify `SkillActivated` as streaming noise. The existing `SkillExpanded` event is currently filtered before persistence; replace slash expansion emissions with `SkillActivated { source: Slash }` or keep `SkillExpanded` internal-only if removing it would create unnecessary churn. + +- [ ] **Step 3: Add trace behavior** + +Update `AgentEvent::trace` so the new events emit concise tracing summaries: + +- memory loaded: profile, file count, total loaded bytes, budget bytes +- skills discovered: profile, skill count, source dir count +- skill activated: name and source +- MCP ready: server, count, and summary count + +- [ ] **Step 4: Convert to durable events** + +Update `fabro-workflow` event conversion so the new agent events map to the new `fabro-types` props and include `visit`. + +- [ ] **Step 5: Add conversion tests** + +Cover each new event with a focused conversion assertion that checks: + +- durable event name +- `visit` +- core fields +- no memory content in the converted payload + +--- + +### Task 3: Emit Memory Loaded Metadata + +**Files:** +- Modify: `lib/crates/fabro-agent/src/memory.rs` +- Modify: `lib/crates/fabro-agent/src/session.rs` +- Test: relevant `fabro-agent` memory/session tests. + +- [ ] **Step 1: Change memory discovery return type** + +Change memory discovery from bare `Vec` to a document type carrying both prompt content and event metadata: + +```rust +pub struct MemoryDocument { + pub path: String, + pub content: String, + pub byte_count: usize, + pub loaded_bytes: usize, + pub truncated: bool, +} +``` + +Keep existing behavior unchanged: + +- provider profile filename candidates stay the same +- root-to-working-dir walk stays the same +- content dedupe stays the same +- empty files are skipped +- total budget remains 32 KiB +- truncated content keeps the existing truncation marker + +- [ ] **Step 2: Preserve prompt assembly behavior** + +Adjust session/profile prompt assembly to pass only memory contents where prompt assembly expects memory text. The system prompt should be byte-for-byte equivalent except where existing tests allow non-semantic ordering differences. + +- [ ] **Step 3: Emit agent.memory.loaded** + +In `Session::initialize()`, emit `AgentEvent::MemoryLoaded` immediately after memory discovery, before skills and MCP initialization. + +Emit the event even when no memory files are loaded. That lets consumers distinguish "no memory" from "not reported." + +- [ ] **Step 4: Add memory tests** + +Cover: + +- loaded file path appears in event metadata +- `byte_count` is the original file byte count +- `loaded_bytes` reflects bytes actually loaded into the prompt budget +- `truncated` is true only for truncated files +- event payload never contains memory file contents +- empty discovery still emits a memory-loaded event with `files == []` + +--- + +### Task 4: Emit Skills Discovered And Skill Activated + +**Files:** +- Modify: `lib/crates/fabro-agent/src/session.rs` +- Modify: `lib/crates/fabro-agent/src/skills.rs` +- Test: relevant `fabro-agent` skill/session tests. + +- [ ] **Step 1: Emit skills discovered** + +After `discover_skills(...)`, emit `AgentEvent::SkillsDiscovered` with: + +- `provider_profile` +- `source_dirs` +- sorted `skills: [{ name, description }]` + +Emit the event even when no skills are discovered. + +- [ ] **Step 2: Emit slash activation** + +Where slash skill expansion currently emits or creates `SkillExpanded`, emit: + +```rust +AgentEvent::SkillActivated { + skill_name, + source: SkillActivationSource::Slash, +} +``` + +- [ ] **Step 3: Emit tool activation** + +In `make_use_skill_tool`, use `ToolContext::emit_agent_event(...)` after a requested skill is found and before returning the skill template. Emit: + +```rust +AgentEvent::SkillActivated { + skill_name: name.to_string(), + source: SkillActivationSource::Tool, +} +``` + +Do not emit activation for failed `use_skill` lookups. + +- [ ] **Step 4: Add skill tests** + +Cover: + +- discovery event includes all discovered skills sorted by name +- discovery event includes configured source directories +- empty discovery emits `skills == []` +- slash expansion emits `source == slash` +- successful `use_skill` emits `source == tool` +- failed `use_skill` does not emit activation + +--- + +### Task 5: Enrich agent.mcp.ready With Names-Only Tool Summaries + +**Files:** +- Modify: `lib/crates/fabro-mcp/src/connection_manager.rs` +- Modify: `lib/crates/fabro-agent/src/session.rs` +- Test: relevant `fabro-mcp` or `fabro-agent` MCP tests. + +- [ ] **Step 1: Add deterministic tool summaries** + +Expose a helper on `McpConnectionManager` or compute in `Session` from `all_tools()`: + +- filter tools by `server_name` +- return qualified tool name as `name` +- return server-provided tool name as `original_name` +- sort by qualified `name` + +- [ ] **Step 2: Enrich ready emissions** + +When emitting `AgentEvent::McpServerReady`, include the tool summaries for that server. Keep existing `server_name` and `tool_count`. + +- [ ] **Step 3: Add MCP tests** + +Cover: + +- ready event includes only tools from the ready server +- summaries are sorted by qualified name +- `name` is the Fabro-qualified MCP tool name +- `original_name` is the server-provided tool name +- descriptions and input schemas are not included + +--- + +### Task 6: Update Event Documentation + +**Files:** +- Modify: `docs/internal/events.md` + +- [ ] **Step 1: Document new events** + +Add sections for: + +- `agent.memory.loaded` +- `agent.skills.discovered` +- `agent.skill.activated` + +For `agent.memory.loaded`, explicitly state that file contents are excluded. + +- [ ] **Step 2: Update MCP ready docs** + +Update `agent.mcp.ready` to show: + +```json +{ + "server_name": "github", + "tool_count": 2, + "tools": [ + { + "name": "mcp__github__create_issue", + "original_name": "create_issue" + } + ], + "visit": 1 +} +``` + +- [ ] **Step 3: Record skill event replacement** + +If `agent.skill.expanded` remains in internal code or docs, mark it internal-only or replaced by `agent.skill.activated`. + +--- + +### Task 7: Verify + +**Files:** +- No new files unless test placement requires it. + +- [ ] **Step 1: Run focused tests** + +Run: + +```bash +cargo nextest run -p fabro-agent -p fabro-workflow -p fabro-types -p fabro-mcp +``` + +- [ ] **Step 2: Run formatting** + +Run: + +```bash +cargo +nightly-2026-04-14 fmt --all +``` + +- [ ] **Step 3: Run clippy for touched crates or workspace** + +Prefer the workspace command if time permits: + +```bash +cargo +nightly-2026-04-14 clippy --workspace --all-targets -- -D warnings +``` + +- [ ] **Step 4: Final sanity checks** + +Confirm: + +- memory events never contain file contents +- skills discovered and memory loaded are emitted even for empty lists +- skill activation is persisted rather than filtered as streaming noise +- `agent.mcp.ready` remains backwards-compatible for old events without `tools` +- docs match the serialized event names and payload shapes + diff --git a/docs/superpowers/plans/2026-05-22-run-agent-fabro-tools-opt-in.md b/docs/superpowers/plans/2026-05-22-run-agent-fabro-tools-opt-in.md new file mode 100644 index 000000000..8730936d9 --- /dev/null +++ b/docs/superpowers/plans/2026-05-22-run-agent-fabro-tools-opt-in.md @@ -0,0 +1,140 @@ +# Run Agent Fabro Tools Opt-In Implementation Plan + +> **For agentic workers:** REQUIRED SUB-SKILL: Use superpowers:subagent-driven-development (recommended) or superpowers:executing-plans to implement this plan task-by-task. Steps use checkbox (`- [ ]`) syntax for tracking. + +**Goal:** Add `[run.agent] fabro_tools = true/false`, defaulting to `false`, so workflow agents only get Fabro run tools and the `agent:run_tools` worker JWT scope when a run opts in. + +**Architecture:** Treat `run.agent.fabro_tools` as the source of truth in resolved run settings. The server reads the effective run setting before spawning `__run-worker` and issues the worker token with or without `agent:run_tools`. The CLI worker decodes the already-present worker token and registers Fabro run tools only when the scope contains both `run:worker` and `agent:run_tools`. Do not add a second worker-side env flag or hidden CLI argument for this capability; the signed JWT scope is the worker-side authority. + +**Tech Stack:** Rust, Serde TOML config layers, Fabro worker JWT scopes, `cargo nextest`. + +--- + +## File Map + +- Modify `lib/crates/fabro-types/src/settings/run.rs`: add resolved `RunAgentSettings::fabro_tools`. +- Modify `lib/crates/fabro-config/src/layers/run.rs`: add optional layered `[run.agent] fabro_tools` and options metadata. +- Modify `lib/crates/fabro-config/src/resolve/run.rs`: resolve missing config to `false`. +- Modify `lib/crates/fabro-config/src/tests/resolve_run.rs`: cover default, true, false, and layer override behavior. +- Modify `lib/crates/fabro-server/src/worker_token.rs`: make the base worker scope constructor available to production code and retain `run_worker_with_agent_run_tools`. +- Modify `lib/crates/fabro-server/src/server.rs`: compute the opt-in flag from run settings and choose worker JWT scopes. +- Modify `lib/crates/fabro-server/src/server/tests.rs`: cover default and opted-in worker token scopes. +- Modify `lib/crates/fabro-cli/src/commands/run/runner.rs`: register `FabroRunToolServices` from the decoded worker token scope. +- Modify docs generator/reference docs: `lib/crates/fabro-dev/src/commands/docs_options_reference.rs`, `docs/public/reference/user-configuration.mdx`, and `docs/public/execution/run-configuration.mdx`. + +--- + +## Task 1: Add Resolved Run Config + +**Files:** +- Modify: `lib/crates/fabro-types/src/settings/run.rs` +- Modify: `lib/crates/fabro-config/src/layers/run.rs` +- Modify: `lib/crates/fabro-config/src/resolve/run.rs` +- Test: `lib/crates/fabro-config/src/tests/resolve_run.rs` + +- [ ] Add resolver tests for default `false`, explicit `true`, explicit `false`, and higher-layer override behavior. +- [ ] Add `fabro_tools: bool` to `RunAgentSettings`. +- [ ] Add `fabro_tools: Option` to `RunAgentLayer` with `#[serde(default, skip_serializing_if = "Option::is_none")]` and options metadata. +- [ ] Resolve `agent.fabro_tools.unwrap_or(false)`. +- [ ] Run `cargo nextest run -p fabro-config run_agent_fabro_tools`. + +--- + +## Task 2: Gate Worker JWT Scope + +**Files:** +- Modify: `lib/crates/fabro-server/src/worker_token.rs` +- Modify: `lib/crates/fabro-server/src/server.rs` +- Test: `lib/crates/fabro-server/src/server/tests.rs` + +- [ ] Add or keep constructors: + +```rust +WorkerScopeSet::run_worker() +WorkerScopeSet::run_worker_with_agent_run_tools() +``` + +- [ ] Update worker command tests: + - default run token scopes are exactly `run:worker` + - opted-in run token scopes are exactly `run:worker agent:run_tools` +- [ ] Do not set a separate worker env var for Fabro tools. +- [ ] Load the effective setting from the run spec/settings available at worker-spawn time. If the current spawn path only exposes full projected run state, prefer a narrow run-spec/settings accessor or cached run record field over scanning/projecting full run history just to read this static setting. +- [ ] Pass the boolean into worker-token scope selection. +- [ ] Run `cargo nextest run -p fabro-server worker_command`. + +--- + +## Task 3: Gate CLI Worker Tool Registration From JWT Scope + +**Files:** +- Modify: `lib/crates/fabro-cli/src/commands/run/runner.rs` +- Test: `lib/crates/fabro-cli/src/commands/run/runner.rs` + +- [ ] Add focused tests for `fabro_run_tools_enabled_from_worker_token`: + - invalid token -> false + - missing `scope` claim -> false + - `run:worker` only -> false + - `agent:run_tools` only -> false + - unknown extra scope -> false + - `run:worker agent:run_tools` -> true +- [ ] Decode only the unsigned claim locally for registration convenience. The server remains responsible for signature and scope enforcement. +- [ ] Gate `build_fabro_run_tool_services(...)` on `fabro_run_tools_enabled_from_worker_token(worker_token)`. +- [ ] Keep token presence as a second local guard inside `build_fabro_run_tool_services`. +- [ ] Run: + +```bash +cargo nextest run -p fabro-cli fabro_run_tools_enabled_token_requires_run_tools_scope +cargo nextest run -p fabro-cli --test it runner +``` + +--- + +## Task 4: Update Docs And Generated Reference Text + +**Files:** +- Modify: `lib/crates/fabro-dev/src/commands/docs_options_reference.rs` +- Modify: `docs/public/reference/user-configuration.mdx` +- Modify: `docs/public/execution/run-configuration.mdx` + +- [ ] Add `fabro_tools = true` to the `[run.agent]` generated sample. +- [ ] Document that the setting defaults to `false`. +- [ ] Document that the setting controls built-in Fabro run-management tools and is separate from ordinary agent `permissions` and `[run.agent.mcps]`. +- [ ] Run: + +```bash +cargo dev docs refresh +cargo dev docs check +``` + +--- + +## Full Verification + +```bash +cargo nextest run -p fabro-config +cargo nextest run -p fabro-server +cargo nextest run -p fabro-cli +cargo +nightly-2026-04-14 fmt --check --all +cargo +nightly-2026-04-14 clippy -p fabro-types -p fabro-config -p fabro-server -p fabro-cli -p fabro-dev --all-targets -- -D warnings +``` + +## Acceptance Criteria + +- Default run: + - resolved `run.agent.fabro_tools == false` + - worker JWT scope is `run:worker` + - `StartServices.fabro_run_tools == None` +- Opted-in run: + - resolved `run.agent.fabro_tools == true` + - worker JWT scope is `run:worker agent:run_tools` + - `StartServices.fabro_run_tools` is present +- No private worker env var or hidden CLI flag controls Fabro tool registration. +- Server-side authorization remains the enforcement point for the worker token signature, run id, and scopes. + +## Assumptions And Defaults + +- `fabro_tools` is a per-run opt-in setting only; this plan does not add a separate server-wide allow/deny policy. +- Defaulting to `false` intentionally changes existing behavior: runs that need Fabro run tools must set `[run.agent] fabro_tools = true`. +- `run.agent.permissions` remains about ordinary agent tool permissions and does not imply Fabro API access. +- `[run.agent.mcps]` remains independent; MCP tools are not enabled or disabled by `fabro_tools`. +- `fabro mcp start` and standalone MCP exposure of Fabro tools are out of scope. diff --git a/docs/superpowers/plans/2026-05-22-unified-agent-transcript-events.md b/docs/superpowers/plans/2026-05-22-unified-agent-transcript-events.md new file mode 100644 index 000000000..ddc138a2f --- /dev/null +++ b/docs/superpowers/plans/2026-05-22-unified-agent-transcript-events.md @@ -0,0 +1,291 @@ +# Unified Agent Transcript Events Implementation Plan + +> **For agentic workers:** REQUIRED SUB-SKILL: Use `superpowers:subagent-driven-development` or `superpowers:executing-plans` to implement this plan task-by-task. + +**Goal:** Make the ordered Fabro event stream sufficient to recreate an API-mode agent session without adding a parallel transcript event family. + +**Architecture:** Extend existing `agent.message`, `agent.tool.started`, and `agent.tool.completed` event semantics. Messages are communication (`system`, `user`, `reasoning`, `agent`). Tool calls and tool results are actions, not messages. Persist only committed events; partial stream deltas, retries, and interrupted output are not replay sources. + +**Out of scope:** Request metadata, compaction semantics, and broad store refactors. + +--- + +## Key Decisions + +- Use one shared Fabro transcript model in `fabro-types`; do not create parallel DTOs for events, API, store projection, and runtime history. +- Treat reasoning as a first-class message kind, not a tool call and not part of the visible agent answer. +- Keep model-role semantics (`kind`) separate from audit/source semantics (`source`). +- Keep tool calls/results as enriched action lifecycle records. +- Use event `seq` as the ordering source of truth. +- Keep run/session lifecycle events for lifecycle only; transcript replay comes from `agent.message` and `agent.tool.*`. +- Preserve provider replay payloads as structured parts, not strings. + +## Type Ownership + +Promote provider-neutral replay primitives from `fabro-llm` into `fabro-types`, then make `fabro-llm` import or re-export the canonical types. + +Canonical shared types: + +- `ContentPart` +- `ThinkingData` +- `ToolCall` +- `ToolResult` +- `TranscriptMessage` +- `MessageKind` +- `MessageSource` +- `PairMessageRef` +- existing `Principal` for actor attribution + +Name the durable transcript type `TranscriptMessage`, not bare `Message`, to avoid import ambiguity with `fabro_agent::Message` and `fabro_llm::types::Message`. Do not add `AgentTranscriptPart` as a second `{ kind, data }` model if `ContentPart` can own the role. Event props must embed the canonical `ToolCall`, `ToolResult`, and `ContentPart` types directly. OpenAPI replacements should point generated API types at these canonical Rust types and include type identity / JSON parity tests. + +## Interface Changes + +Add shared transcript types in `fabro-types`: + +```rust +TranscriptMessage { + id, + turn_id, + kind, // system | user | reasoning | agent + source, // system_prompt | turn_input | followup | steer | pair | injected_system | injected_user | loop_detection + actor: Option, + pair: Option, + content: Vec, + provider, + model, + response_id, + usage, +} + +PairMessageRef { + pair_id, + message_id, + client_message_id, +} +``` + +`kind` captures provider/model-role semantics for replay. `source` captures audit/UI origin. Steering is a source, not a role: steering that currently replays to the LLM as user-role input must be stored as `kind=user, source=steer`. + +Extend existing durable events: + +- `agent.message` + - Add `message: TranscriptMessage`. + - This becomes the canonical replay source for committed system, user, reasoning, and agent messages. + - Keep narrow `text`, `model`, `billing`, and `tool_call_count` fields until web/server/client consumers are migrated. +- `agent.tool.started` + - Add `tool_call: ToolCall`. + - Add `turn_id` and `parent_message_id`. + - Keep narrow `tool_name`, `tool_call_id`, and `arguments` fields until consumers are migrated. +- `agent.tool.completed` + - Add `tool_result: ToolResult`. + - Add `turn_id`. + - Keep narrow `tool_name`, `tool_call_id`, `output`, and `is_error` fields until consumers are migrated. + +Provider replay requirements: + +- OpenAI `openai_reasoning` and `openai_message` opaque items remain exact `ContentPart::Other` payloads. +- Anthropic thinking and redacted thinking remain `ContentPart::Thinking` payloads with signatures preserved. +- Gemini `thoughtSignature` remains `ToolCall.provider_metadata`. +- Reasoning messages can contain cleartext, redacted, signed, encrypted, or opaque provider parts, but implementation must not collapse these into plain strings. + +Identity requirements: + +- Add a canonical `MessageId` in `fabro-types`. +- `fabro-agent::Session` mints a `TurnId` for every `run_single_input()` invocation unless the caller supplies one. +- Ask Fabro passes its existing API `TurnId` into the agent session before processing. +- Workflow API-mode stages let the agent session mint a `TurnId`. +- The assistant/agent message id is minted before emitting tool calls. Tool calls emitted from that response use `parent_message_id = agent_message.id`. + +## Implementation Tasks + +### 1. Add Typed Event Contracts + +Modify: + +- `lib/crates/fabro-types/src/run_event/agent.rs` +- `lib/crates/fabro-types/src/run_event/session.rs` +- `lib/crates/fabro-types/src/run_event/mod.rs` +- `docs/public/api-reference/fabro-api.yaml` if exposed wire shapes change + +Tasks: + +- Move or re-home provider-neutral `ContentPart`, `ThinkingData`, `ToolCall`, and `ToolResult` into `fabro-types`. +- Add canonical `TranscriptMessage`, `MessageKind`, `MessageSource`, and `PairMessageRef` types in `fabro-types`. +- Extend `AgentMessageProps` to carry the canonical message payload. +- Extend tool started/completed props to carry canonical tool call/result payloads plus turn/message linkage. +- Keep serde defaults where needed so old event payloads continue to deserialize. +- Add `fabro-api` replacement tests for type identity and JSON parity when OpenAPI schemas map to canonical Rust types. + +### 2. Emit Committed Messages From `fabro-agent` + +Modify: + +- `lib/crates/fabro-agent/src/types.rs` +- `lib/crates/fabro-agent/src/session.rs` +- `lib/crates/fabro-agent/src/history.rs` + +Tasks: + +- Replace or extend the narrow assistant-only `AgentEvent::AssistantMessage` path with a general committed `AgentEvent::Message`. +- Emit `kind=system, source=system_prompt` after the exact rendered system prompt is assembled. +- Emit `kind=user, source=turn_input` after skill expansion/wrapping, using the exact user message sent to the model. +- Emit `kind=user, source=followup` for follow-up inputs. +- Emit `kind=user, source=steer` for steering-as-user. +- Emit `kind=user, source=loop_detection` for loop-detection steering. +- Emit `kind=system, source=injected_system` for injected system messages. +- Emit `kind=user, source=injected_user` for injected user-role messages. +- Emit `kind=user, source=pair` for pair chat messages that enter LLM history, with `pair` populated. +- Emit `kind=system, source=pair` for pair join/leave or other pair system messages that enter LLM history, with `pair` populated. +- Emit `kind=reasoning` only for completed provider reasoning blocks that must be preserved for replay, preserving exact structured parts. +- Emit `kind=agent` after provider `Finish`, using the completed response content. +- Do not emit committed messages for deltas, retries, or interrupted partial output. +- Ensure all message events carry `turn_id`, `source`, and optional `actor`/`pair` metadata where applicable. + +### 3. Enrich Tool Action Events + +Modify: + +- `lib/crates/fabro-agent/src/session.rs` +- `lib/crates/fabro-agent/src/tool_execution.rs` +- provider adapters only where extra metadata is not currently surfaced + +Tasks: + +- Preserve `ToolCall.tool_type`, `raw_arguments`, and `provider_metadata`. +- Preserve `ToolResult` structured output, error state, and supported media/artifact fields. +- Link every tool call to the owning agent message with `parent_message_id`. +- Mint the agent message id before tool execution so tool events can link correctly. +- Keep tool calls/results out of message events. + +### 4. Persist Unified Events In Both API Paths + +Modify: + +- `lib/crates/fabro-workflow/src/handler/llm/api.rs` +- `lib/crates/fabro-workflow/src/event/convert.rs` +- `lib/crates/fabro-workflow/src/event/names.rs` +- `lib/crates/fabro-server/src/server/handler/sessions.rs` + +Tasks: + +- Convert the unified agent message event through the existing workflow `Event::Agent` path. +- Convert Ask Fabro/server session agent events into the same durable `agent.message` and `agent.tool.*` shapes. +- Keep `run.session.created`, `run.session.turn.started`, and terminal turn events as lifecycle events. +- Keep old `run.session.user_message`, `run.session.assistant_message`, and `run.session.tool_call.*` projection support until all producers and consumers are migrated. +- Prefer a shared event persistence helper for workflow and server session paths so redaction behavior is consistent. +- Avoid creating new transcript-specific event families. + +Migration order: + +1. Add canonical types and event deserialization support. +2. Update projection to read both old narrow run-session events and new unified agent events. +3. Switch workflow and Ask Fabro producers to emit unified events while retaining compatibility fields. +4. Update web/server/client consumers to prefer unified payloads with narrow-field fallback. +5. Only then consider deprecating narrow transcript-bearing run-session events. + +### 5. Define Pair Transcript Relationship + +Modify: + +- `lib/crates/fabro-workflow/src/steering_hub.rs` +- `lib/crates/fabro-types/src/pair.rs` +- `lib/crates/fabro-server/src/server/handler/sessions.rs` +- web consumers of pair transcript events + +Tasks: + +- Treat `agent.pair.user_message` and `agent.pair.system_message` as UI/audit projection events only. +- Do not use pair transcript events as replay-authoritative session history. +- For any pair message that affects LLM history, emit the corresponding canonical `agent.message` event with `source=pair` and a populated `PairMessageRef`. +- Store pair user chat as `kind=user, source=pair`. +- Store pair join/leave or other pair system items that enter model context as `kind=system, source=pair`. +- Keep existing pair API transcript types as projections over pair events and canonical message references, not as a second replay model. + +### 6. Rebuild Session Projection From Events + +Modify: + +- `lib/crates/fabro-store/src/run_sessions.rs` +- `lib/crates/fabro-types/src/session.rs` +- `lib/crates/fabro-agent/src/history.rs` + +Tasks: + +- Project runtime context from ordered `agent.message` and `agent.tool.*` events scoped by envelope `session_id`. +- Add a session-id/sequence index or incremental per-session transcript projection before relying on replay for hydration. Do not scan the full run event history and inspect every event payload for each session load. +- Preserve provider-specific reasoning, opaque provider items, response ids, usage, and tool metadata. +- Keep best-effort fallback projection for legacy narrow session events. +- Ignore pair transcript events for replay except as a legacy fallback path; canonical `agent.message` with `source=pair` is the replay source. +- Ensure `Session::from_record()` can hydrate without dropping provider parts needed for same-provider replay. +- Preserve injected history sources: rendered system prompt, wrapped user input, follow-up input, steering, injected system messages, injected user-role messages, and loop-detection steering. + +### 7. Redaction And Security Policy + +Modify: + +- workflow event persistence path +- server session event persistence path +- event redaction utilities + +Tasks: + +- Define raw replay fields explicitly: provider opaque parts, raw tool arguments, provider metadata, and structured tool outputs. +- Apply one shared redaction policy before durable storage for both workflow and server sessions. +- Preserve replay-critical opaque provider fields unless they match an existing secret redaction rule. +- Do not omit fields needed for same-provider replay silently; if a field must be redacted, preserve the shape and mark the value redacted. +- Add tests covering raw tool arguments and provider metadata through both persistence paths. + +### 8. Consumer Compatibility + +Modify: + +- web event consumers that currently read narrow `properties.text` +- web pair transcript consumers that read `agent.pair.*` +- server/API projections that expose session detail or event detail +- generated clients if OpenAPI changes + +Tasks: + +- Keep narrow compatibility fields in emitted events until consumers are updated. +- Update consumers to prefer `properties.message` and fall back to narrow fields. +- Keep pair transcript rendering backed by pair projection events, while ensuring session replay and hydration consume canonical `agent.message` events. +- Add web/server tests that render both old and new event shapes. +- Document the deprecation path for narrow transcript fields after consumer migration. + +## Test Plan + +- `fabro-types`: serde round trips for `agent.message`, enriched `agent.tool.started`, and enriched `agent.tool.completed`. +- Type ownership: + - canonical `ToolCall`, `ToolResult`, `ContentPart`, `TranscriptMessage`, usage, and event prop types are reused rather than duplicated + - OpenAPI replacement tests prove type identity and JSON parity where API schemas expose these shapes +- `fabro-agent`: committed system/user/reasoning/agent messages emit once, while partial deltas and interrupted streams do not create committed messages. +- `fabro-agent`: followups, steering-as-user, injected system messages, and loop-detection steering emit committed messages with the correct `kind`, `source`, and `turn_id`. +- Role/source mapping: + - steering-as-user emits `kind=user, source=steer` + - loop-detection steering emits `kind=user, source=loop_detection` + - injected user-role messages emit `kind=user, source=injected_user` + - pair user chat emits `kind=user, source=pair` with `PairMessageRef` + - pair join/leave context emits `kind=system, source=pair` with `PairMessageRef` +- Identity/linkage: tool calls include the parent agent message id minted before tool execution. +- Provider replay: + - OpenAI encrypted reasoning and opaque message items survive event replay. + - Anthropic thinking signatures survive event replay. + - Gemini thought signatures survive enriched tool call replay. +- `fabro-store`: session projection from event `seq` order recreates runtime history including provider parts and tool metadata. +- Pair projection: pair transcript events render in the pair UI/audit surface but do not create duplicate replay history when the canonical `source=pair` message exists. +- Migration: old narrow run-session events and new unified events both hydrate session detail without duplicate transcript entries. +- `fabro-server`: Ask Fabro stores the wrapped model input, not only the raw UI question. +- Redaction: workflow and server session persistence apply the same redaction behavior to raw arguments, provider metadata, and tool outputs. +- Consumer compatibility: existing UI/server consumers render old narrow fields and new unified message payloads. +- API conformance: OpenAPI-generated Rust/TypeScript clients still match the spec after schema updates. + +## Acceptance Criteria + +- A completed API-mode session can be reconstructed from the event stream without losing committed system, user, reasoning, agent, tool call, or tool result state. +- New transcript state is stored through existing semantic events, not a separate transcript event family. +- Tool calls remain actions, not messages. +- Partial output remains non-authoritative for replay. +- The implementation introduces one canonical set of replay types, not duplicated event/API/runtime DTOs. +- Steering, pair, and injected inputs preserve provider-role semantics in `kind` and audit/source semantics in `source`. +- Pair transcript events are UI/audit projection events, not a replay-authoritative transcript source. +- Ask Fabro migration is backward compatible for existing session events and projections. diff --git a/docs/superpowers/plans/2026-05-23-llm-input-token-counting.md b/docs/superpowers/plans/2026-05-23-llm-input-token-counting.md new file mode 100644 index 000000000..bc9e4687e --- /dev/null +++ b/docs/superpowers/plans/2026-05-23-llm-input-token-counting.md @@ -0,0 +1,381 @@ +# LLM Input Token Counting Implementation Plan + +> **For agentic workers:** REQUIRED SUB-SKILL: Use superpowers:subagent-driven-development (recommended) or superpowers:executing-plans to implement this plan task-by-task. Steps use checkbox (`- [ ]`) syntax for tracking. + +**Goal:** Add an optional LLM adapter capability that returns current input/context token counts, using provider-native counting when available and a deterministic local estimate otherwise. + +**Architecture:** Token counting belongs in `fabro-llm` because each provider adapter owns the final provider-specific request serialization. `Client::count_input_tokens` will resolve and validate the request through the same provider path as `complete` and `stream`, try the adapter count API when requested, and fall back to a local estimate only for explicitly fallback-eligible failures. The returned value reports input/context size only, not billing usage. + +**Tech Stack:** Rust, async-trait, serde/serde_json, fabro-http, httpmock, existing `fabro-llm` provider adapters. + +--- + +## Scope And Decisions + +- Build the reusable `fabro-llm` capability only. Session-level context breakdown, API endpoints, and UI rendering are follow-up work. +- Count input/context tokens only: model-visible messages, system/developer instructions, tools, tool choice, response schemas, and structured input content. +- Do not reuse `TokenCounts`; it includes output, reasoning, cache-read, and cache-write billing buckets. +- Prefer provider-native counting when callers choose it, but do not hide deterministic configuration, credential, request-shape, model-availability, content-filter, or context-length errors behind a local estimate. +- `PreferProvider` falls back only for unsupported adapters, timeout/network errors, rate limits, and provider 5xx/server errors. +- `RequireProvider` never returns a local estimate. It returns a provider count or an error. +- `EstimateOnly` still resolves and validates the provider/model, but does not call the adapter or send request content upstream. +- Privacy: `PreferProvider` and `RequireProvider` send the model-visible request to the upstream provider's token-count endpoint. That includes messages, system/developer instructions, tools, schemas, structured content, and media metadata/content according to provider serialization. `EstimateOnly` is the privacy-preserving mode. + +## File Structure + +- Create `lib/crates/fabro-llm/src/token_count.rs` + - Public token-counting result/preference types. + - Deterministic local estimator. + - Unit tests for estimator behavior. +- Modify `lib/crates/fabro-llm/src/lib.rs` + - Export the new module and public types. +- Modify `lib/crates/fabro-llm/src/provider.rs` + - Add the optional adapter method with a default unsupported implementation. +- Modify `lib/crates/fabro-llm/src/client.rs` + - Add `Client::count_input_tokens`. + - Add tests for fallback and preference behavior. +- Modify provider adapter files under `lib/crates/fabro-llm/src/providers/` + - Anthropic: count via `/messages/count_tokens`. + - Gemini: count via `models/{model}:countTokens`. + - OpenAI: count via `/responses/input_tokens`. + - OpenAI-compatible and Fabro-server adapters keep the default unsupported path. + +## Task 1: Add Public Types And Local Estimator + +**Files:** +- Create: `lib/crates/fabro-llm/src/token_count.rs` +- Modify: `lib/crates/fabro-llm/src/lib.rs` + +- [ ] Define these public types in `token_count.rs`: + +```rust +use serde::{Deserialize, Serialize}; + +use crate::types::{ContentPart, Request, ToolDefinition, Warning}; + +#[derive(Debug, Clone, Copy, PartialEq, Eq, Serialize, Deserialize)] +#[serde(rename_all = "snake_case")] +pub enum InputTokenCountPreference { + PreferProvider, + RequireProvider, + EstimateOnly, +} + +#[derive(Debug, Clone, Copy, PartialEq, Eq, Serialize, Deserialize)] +#[serde(rename_all = "snake_case")] +pub enum InputTokenCountMethod { + ProviderApi, + LocalEstimate, +} + +#[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize)] +pub struct InputTokenCount { + pub input_tokens: i64, + pub method: InputTokenCountMethod, + pub provider: String, + pub model: String, + #[serde(default)] + pub warnings: Vec, +} +``` + +- [ ] Add `estimate_input_tokens(request: &Request, provider: impl Into) -> InputTokenCount`. + - Use `InputTokenCountMethod::LocalEstimate`. + - Set `model` from `request.model`. + - Add deterministic warning codes as needed: + - `local_token_estimate`: every local estimate. + - `media_token_estimate`: media content was counted by a fixed heuristic or embedded byte estimate. + - `opaque_context_estimate`: opaque provider-specific context was serialized or approximated without provider semantics. + - `provider_options_estimate`: provider options may affect model-visible context and were counted by JSON-size heuristic. + - De-duplicate warnings by code so repeated media or opaque parts do not produce noisy results. + +- [ ] Implement deterministic estimator helpers: + - `estimate_text_tokens(text)`: `text.chars().count().div_ceil(4)`; empty text counts as 0. + - `estimate_json_tokens(value)`: compact `serde_json::to_string(value)` length rounded up at 4 chars/token. + - Message overhead: 4 tokens per message plus 1 token per content part. + - Tool overhead: 8 tokens per tool plus estimated name, description, and schema JSON. + - Tool choice and response format: estimate their serialized JSON values. + - `provider_options`: estimate serialized JSON and add `provider_options_estimate`. + - Images: 2,000 token media floor plus metadata text/URL estimate. + - Audio/documents: estimate embedded byte length at 4 bytes/token; URL-only media uses a 2,000 token media floor plus metadata. + - File IDs and URL-only media: count the ID/URL text plus the 2,000 token media floor and add `media_token_estimate`. + - Embedded media bytes: count byte length divided by 4, rounded up, and add `media_token_estimate`. + - Gemini cached content options: estimate serialized `provider_options.gemini.cached_content` and add `provider_options_estimate`. + - `ContentPart::Other`: estimate serialized JSON and add `opaque_context_estimate`. + - OpenAI opaque previous-response/message/reasoning items in `ContentPart::Other`: estimate serialized JSON and add `opaque_context_estimate`. + +- [ ] Export the module and public types from `lib.rs`: + +```rust +pub mod token_count; + +pub use token_count::{ + InputTokenCount, InputTokenCountMethod, InputTokenCountPreference, estimate_input_tokens, +}; +``` + +- [ ] Add estimator tests in `token_count.rs`: + - text-only request returns a positive local estimate + - adding a tool increases the estimate + - adding response format schema increases the estimate + - image/document/audio content gets a media warning or media-sized estimate + - provider options produce `provider_options_estimate` + - opaque `ContentPart::Other` produces `opaque_context_estimate` + - estimator is deterministic for the same request + +Run: + +```bash +cargo nextest run -p fabro-llm token_count +``` + +Expected: estimator tests pass. + +## Task 2: Add Adapter Capability And Client Fallback + +**Files:** +- Modify: `lib/crates/fabro-llm/src/provider.rs` +- Modify: `lib/crates/fabro-llm/src/client.rs` + +- [ ] Extend `ProviderAdapter` with this default method: + +```rust +async fn count_input_tokens( + &self, + _request: &Request, +) -> Result, Error> { + Ok(None) +} +``` + +- [ ] Add imports in `provider.rs` for `InputTokenCount`. + +- [ ] Add `Client::count_input_tokens(&self, request: &Request, preference: InputTokenCountPreference) -> Result`. + - Call `self.validate_request_controls(request)?`. + - Resolve provider with `self.resolve_provider(request)?`. + - Call `provider.validate_request(request)?`. + - If preference is `EstimateOnly`, return `estimate_input_tokens(request, provider.name())`. + - If preference is `PreferProvider` or `RequireProvider`, call `provider.count_input_tokens(request).await`. + - Return provider result when it is `Ok(Some(count))`. + - In `PreferProvider`, return local estimate with warning code `provider_token_count_unsupported` when it is `Ok(None)`. + - In `RequireProvider`, return `Err(Error::Configuration { .. })` when it is `Ok(None)`. + - In `PreferProvider`, fallback to local estimate with warning code `provider_token_count_failed` only when the adapter returns: + - `Error::Network` + - `Error::RequestTimeout` + - `Error::Provider { kind: ProviderErrorKind::RateLimit, .. }` + - `Error::Provider { kind: ProviderErrorKind::Server, .. }` + - In `PreferProvider`, return the original error for: + - provider resolution errors + - `provider.validate_request` errors + - provider 400 invalid request / `ProviderErrorKind::InvalidRequest` + - authentication / access denied + - not found / model unavailable + - context-length and content-filter errors + - quota exceeded and every other provider error kind not explicitly listed as fallback-eligible + - configuration, unsupported tool choice, interrupt, invalid tool call, no-object, and stream errors + - In `RequireProvider`, return the original adapter error for every error kind; never fallback. + - Do not run completion/stream middleware for token counting. + +- [ ] Add client tests using a mock adapter: + - provider result is returned when adapter returns `Ok(Some(_))` + - `PreferProvider` unsupported adapter returns local estimate with `provider_token_count_unsupported` + - `RequireProvider` unsupported adapter returns `Err` + - `PreferProvider` falls back for timeout, network, rate-limit, and server errors + - `PreferProvider` returns `Err` for invalid request, auth, access denied, not found, context length, content filter, quota exceeded, configuration, and unsupported tool choice errors + - `RequireProvider` returns `Err` for fallback-eligible provider errors + - `EstimateOnly` does not call the adapter method + - validation errors still return `Err` + +Run: + +```bash +cargo nextest run -p fabro-llm client::tests::count_input_tokens +``` + +Expected: new client tests pass. + +## Task 3: Implement Anthropic Provider Counting + +**Files:** +- Modify: `lib/crates/fabro-llm/src/providers/anthropic.rs` + +- [ ] Reuse the existing request translation from `build_api_request(adapter, request, false).await` so count requests match normal Anthropic serialization. + +- [ ] Add a private count request/response shape: + +```rust +#[derive(serde::Serialize)] +struct CountTokensRequest { + model: String, + messages: Vec, + #[serde(skip_serializing_if = "Option::is_none")] + system: Option, + #[serde(skip_serializing_if = "Option::is_none")] + tools: Option>, + #[serde(skip_serializing_if = "Option::is_none")] + tool_choice: Option, + #[serde(skip_serializing_if = "Option::is_none")] + thinking: Option, +} + +#[derive(serde::Deserialize)] +struct CountTokensResponse { + input_tokens: i64, +} +``` + +- [ ] Add `count_tokens_url()` returning `"{base_url}/messages/count_tokens"`. + +- [ ] Implement `ProviderAdapter::count_input_tokens` for Anthropic: + - Build the translated API request. + - Send only count-supported input fields: `model`, `messages`, `system`, `tools`, `tool_choice`, and `thinking`. + - Do not send generation-only fields from the normal request body: `max_tokens`, `temperature`, `top_p`, `stop_sequences`, `output_config`, `speed`, `metadata`, or `stream`. + - Apply the same auth, `anthropic-version`, default headers, and beta headers needed for the translated request. + - Parse `input_tokens`. + - Return `InputTokenCount { method: ProviderApi, provider: self.provider_name.clone(), model: request.model.clone(), warnings: vec![] }`. + +- [ ] Add `httpmock` tests: + - request path is `/messages/count_tokens` + - body includes translated `model`, `messages`, `system`, and `tools` + - reasoning-effort requests that normally produce `output_config` do not include `output_config` in the count body + - extended thinking configured through provider options is included as `thinking` when the normal translated request includes it + - response `{ "input_tokens": 123 }` returns `ProviderApi` with `123` + - non-2xx provider response surfaces from the adapter so the client fallback test can handle it + +Run: + +```bash +cargo nextest run -p fabro-llm providers::anthropic::tests::count_input_tokens +``` + +Expected: Anthropic count tests pass. + +## Task 4: Implement Gemini Provider Counting + +**Files:** +- Modify: `lib/crates/fabro-llm/src/providers/gemini.rs` + +- [ ] Reuse `build_api_request(request).await` to build the normal Gemini `generateContent` body. + +- [ ] Add a private response shape: + +```rust +#[derive(serde::Deserialize)] +#[serde(rename_all = "camelCase")] +struct CountTokensResponse { + total_tokens: i64, +} +``` + +- [ ] Implement `ProviderAdapter::count_input_tokens` for Gemini: + - Resolve API model with `common::api_model_id(self.catalog.as_deref(), &request.model)`. + - POST to `"{base_url}/models/{api_model}:countTokens"`. + - Body is `{ "generateContentRequest": api_body }`. + - Apply `x-goog-api-key` and default headers as in `complete`. + - Parse `totalTokens`. + - Return `InputTokenCount` with `ProviderApi`. + +- [ ] Add `httpmock` tests: + - request path is `/models/:countTokens` + - body nests the generated request under `generateContentRequest` + - body does not include top-level `contents`, because Gemini requires `contents` and `generateContentRequest` to be mutually exclusive + - response `{ "totalTokens": 456 }` returns `456` + - tool declarations and system instructions are included through the reused translator + +Run: + +```bash +cargo nextest run -p fabro-llm providers::gemini::tests::count_input_tokens +``` + +Expected: Gemini count tests pass. + +## Task 5: Implement OpenAI Provider Counting + +**Files:** +- Modify: `lib/crates/fabro-llm/src/providers/openai.rs` + +- [ ] Reuse `build_request_body_with_catalog(request, false, self.codex_mode, self.catalog.as_deref()).await`, then filter the body to the `/responses/input_tokens` allow-list. +- [ ] Keep only these top-level fields when present: `conversation`, `input`, `instructions`, `model`, `parallel_tool_calls`, `previous_response_id`, `reasoning`, `text`, `tool_choice`, `tools`, and `truncation`. +- [ ] Strip generation/storage/response-shaping fields from the count request body: `background`, `include`, `max_output_tokens`, `max_tool_calls`, `metadata`, `prompt`, `prompt_cache_key`, `safety_identifier`, `service_tier`, `store`, `stream`, `temperature`, `top_logprobs`, `top_p`, `user`, `stop`, and any Codex-only generated fields not in the allow-list. + +- [ ] Add a private response shape matching the current Responses input-token API: + +```rust +#[derive(serde::Deserialize)] +struct InputTokensResponse { + input_tokens: i64, + object: String, +} +``` + +- [ ] Implement `ProviderAdapter::count_input_tokens` for OpenAI: + - POST to `"{base_url}/responses/input_tokens"`. + - Use `self.build_request(&url).json(&filtered_request_body)` so auth/org/project/default headers match completion behavior. + - Validate `object == "response.input_tokens"`; otherwise return a network parse error. + - Parse top-level `input_tokens`. + - Return `InputTokenCount` with `ProviderApi`. + +- [ ] Add `httpmock` tests: + - request path is `/responses/input_tokens` + - request body is exactly the allow-listed count body for messages, instructions, tools, reasoning, and response format + - request body strips `store`, `include`, `stream`, `max_output_tokens`, `metadata`, `temperature`, `top_p`, and `stop` + - response `{ "object": "response.input_tokens", "input_tokens": 789 }` returns `789` + - response with the wrong `object` returns an error + - `codex_mode` count uses the same serialization choices as Codex-mode streaming requests + +Run: + +```bash +cargo nextest run -p fabro-llm providers::openai::tests::count_input_tokens +``` + +Expected: OpenAI count tests pass. + +## Task 6: Integration, Docs, And Final Verification + +**Files:** +- Modify: `lib/crates/fabro-llm/README.md` + +- [ ] Add a short README section showing: + - `client.count_input_tokens(&request, InputTokenCountPreference::PreferProvider).await` + - `RequireProvider` for callers that need provider semantics and must not accept estimates + - `EstimateOnly` as the privacy-preserving mode + - provider fallback behavior + - privacy/data exposure for provider-native counting: model-visible request content is sent to the provider token-count endpoint + - the distinction between `InputTokenCount` and billing `TokenCounts` + +- [ ] Run formatting check: + +```bash +cargo +nightly-2026-04-14 fmt --check --all +``` + +Expected: passes. If it fails only because new files need formatting, run `cargo +nightly-2026-04-14 fmt --all` and re-check. + +- [ ] Run focused tests: + +```bash +cargo nextest run -p fabro-llm +``` + +Expected: all `fabro-llm` tests pass. + +- [ ] Run workspace build: + +```bash +cargo build --workspace +``` + +Expected: workspace builds successfully. + +## Acceptance Criteria + +- Callers can ask the LLM client for input/context token count without sending a completion request. +- Anthropic, Gemini, and OpenAI adapters use provider-native counting endpoints. +- Unsupported adapters and provider count failures return deterministic local estimates only when the failure is explicitly fallback-eligible. +- `RequireProvider` never returns a local estimate. +- Provider count requests are filtered to fields accepted by each provider's count endpoint. +- README documents privacy implications of provider-native counting. +- The result cannot be mistaken for billing totals because it uses new input-token-specific types. +- Existing completion and streaming behavior is unchanged. diff --git a/lib/crates/fabro-agent/README.md b/lib/crates/fabro-agent/README.md index 975cb456c..5cac1be82 100644 --- a/lib/crates/fabro-agent/README.md +++ b/lib/crates/fabro-agent/README.md @@ -81,7 +81,9 @@ Built-in profiles: ```rust pub trait Sandbox: Send + Sync { - async fn read_file(&self, path: &str, offset: Option, limit: Option) -> Result; + async fn read_file_bytes(&self, path: &str) -> Result, String>; + async fn read_file_text(&self, path: &str) -> Result; + async fn read_file(&self, path: &str, offset: Option, limit: Option) -> Result; // line-numbered display async fn write_file(&self, path: &str, content: &str) -> Result<(), String>; async fn exec_command(&self, command: &str, timeout_ms: u64, ...) -> Result; async fn grep(&self, pattern: &str, path: &str, options: &GrepOptions) -> Result, String>; @@ -232,4 +234,4 @@ profile.register_subagent_tools(manager, factory, 0); - **Tool output truncation** -- Per-tool character and line limits with head/tail or tail-only truncation modes - **Environment variable filtering** -- `LocalSandbox` strips secrets (`*_API_KEY`, `*_SECRET`, `*_TOKEN`, `*_PASSWORD`, `*_CREDENTIAL`) from subprocess environments - **Command timeouts** -- Configurable per-command with process group cleanup (SIGTERM then SIGKILL) -- **Project doc discovery** -- Automatically discovers `AGENTS.md`, `CLAUDE.md`, `GEMINI.md`, or `.codex/instructions.md` based on provider, with a 32KB budget \ No newline at end of file +- **Project doc discovery** -- Automatically discovers `AGENTS.md`, `CLAUDE.md`, `GEMINI.md`, or `.codex/instructions.md` based on provider, with a 32KB budget diff --git a/lib/crates/fabro-agent/src/memory.rs b/lib/crates/fabro-agent/src/memory.rs index 16f9b5739..263702c4b 100644 --- a/lib/crates/fabro-agent/src/memory.rs +++ b/lib/crates/fabro-agent/src/memory.rs @@ -46,7 +46,7 @@ pub async fn discover_memory( return Err(Error::Interrupted(InterruptReason::Cancelled)); } let path = format!("{dir}/{filename}"); - let read_result = env.read_file(&path, None, None).await; + let read_result = env.read_file_text(&path).await; if cancel_token.is_cancelled() { return Err(Error::Interrupted(InterruptReason::Cancelled)); } diff --git a/lib/crates/fabro-agent/src/session.rs b/lib/crates/fabro-agent/src/session.rs index d17922515..ad84a96fd 100644 --- a/lib/crates/fabro-agent/src/session.rs +++ b/lib/crates/fabro-agent/src/session.rs @@ -1,6 +1,6 @@ use std::collections::{HashMap, VecDeque}; use std::sync::{Arc, Mutex, RwLock}; -use std::time::{Duration, SystemTime}; +use std::time::SystemTime; use fabro_auth::CredentialSource; use fabro_llm::client::Client; @@ -1008,22 +1008,6 @@ impl Session { } } - fn retry_delay_for_error( - retry_policy: &RetryPolicy, - err: &LlmError, - attempt: u32, - ) -> Option { - if let Some(retry_after) = err.retry_after() { - let retry_after = Duration::from_secs_f64(retry_after); - if retry_after > retry_policy.backoff.max_delay { - return None; - } - Some(retry_after) - } else { - Some(retry_policy.backoff.delay_for_attempt(attempt + 1)) - } - } - #[must_use] pub fn followup_queue_handle(&self) -> Arc>> { self.followup_queue.clone() @@ -1421,7 +1405,7 @@ impl Session { let can_retry = err.retryable() && stream_attempt < STREAM_CONSUME_RETRIES; let retry_attempt = u32::try_from(stream_attempt).unwrap_or(u32::MAX); let retry_delay = can_retry - .then(|| Self::retry_delay_for_error(&retry_policy, &err, retry_attempt)) + .then(|| retry::retry_delay(&retry_policy, &err, retry_attempt)) .flatten(); if let Some(delay) = retry_delay { diff --git a/lib/crates/fabro-agent/src/skills.rs b/lib/crates/fabro-agent/src/skills.rs index f7b4e3eaf..98ac6def2 100644 --- a/lib/crates/fabro-agent/src/skills.rs +++ b/lib/crates/fabro-agent/src/skills.rs @@ -255,7 +255,7 @@ pub async fn discover_skills( if cancel_token.is_cancelled() { return Err(Error::Interrupted(InterruptReason::Cancelled)); } - let read_result = env.read_file(&path, None, None).await; + let read_result = env.read_file_text(&path).await; if cancel_token.is_cancelled() { return Err(Error::Interrupted(InterruptReason::Cancelled)); } diff --git a/lib/crates/fabro-agent/src/todo_tools.rs b/lib/crates/fabro-agent/src/todo_tools.rs index 7c704eb3b..aea811a13 100644 --- a/lib/crates/fabro-agent/src/todo_tools.rs +++ b/lib/crates/fabro-agent/src/todo_tools.rs @@ -57,133 +57,16 @@ fn parse_status(value: &str, allow_deleted: bool) -> Result Ok(status) } -const TASK_CREATE_DESCRIPTION: &str = r#"Use this tool to create a structured task list for your current coding session. This helps you track progress, organize complex tasks, and demonstrate thoroughness to the user. -It also helps the user understand the progress of the task and overall progress of their requests. +const TASK_CREATE_DESCRIPTION: &str = "Create pending tasks in the current session. \ +Use concise subjects, descriptions, optional activeForm text for in-progress display, \ +and metadata when callers need structured labels."; -## When to Use This Tool +const TASK_UPDATE_DESCRIPTION: &str = "Update an existing task's status, text, owner, \ +metadata, or dependencies. Valid statuses are pending, in_progress, completed, and \ +deleted; deleted removes a task from active work."; -Use this tool proactively in these scenarios: - -- Complex multi-step tasks - When a task requires 3 or more distinct steps or actions -- Non-trivial and complex tasks - Tasks that require careful planning or multiple operations -- Plan mode - When using plan mode, create a task list to track the work -- User explicitly requests todo list - When the user directly asks you to use the todo list -- User provides multiple tasks - When users provide a list of things to be done, either numbered or comma-separated -- After receiving new instructions - Immediately capture user requirements as tasks -- When you start working on a task - Mark it as in_progress BEFORE beginning work -- After completing a task - Mark it as completed and add any new follow-up tasks discovered during implementation - -## When NOT to Use This Tool - -Skip using this tool when: - -- There is only a single, straightforward task -- The task is trivial and tracking it provides no organizational benefit -- The task can be completed in less than 3 trivial steps -- The task is purely conversational or informational - -NOTE that you should not use this tool if there is only one trivial task to do. In this case you are better off just doing the task directly. - -## Task Fields - -- **subject**: A brief, actionable title in imperative form, such as "Fix authentication bug in login flow" -- **description**: What needs to be done -- **activeForm** (optional): Present continuous form shown in the spinner when the task is in_progress, such as "Fixing authentication bug". If omitted, the spinner shows the subject instead. - -All tasks are created with status `pending`. - -## Tips - -- Create tasks with clear, specific subjects that describe the outcome -- After creating tasks, use TaskUpdate to set up dependencies with addBlocks or addBlockedBy if needed -- Check TaskList first to avoid creating duplicate tasks"#; - -const TASK_UPDATE_DESCRIPTION: &str = r#"Use this tool to update a task in the task list. - -## When to Use This Tool - -**Mark tasks as resolved:** - -- When you have completed the work described in a task -- When a task is no longer needed or has been superseded -- Mark tasks as in_progress when you start working on them -- Mark tasks as completed immediately after finishing them -- ONLY mark a task as completed when you have FULLY accomplished it -- If you encounter errors, blockers, or cannot finish, keep the task as in_progress -- When blocked, create a new task describing what needs to be resolved -- Never mark a task as completed if tests are failing, implementation is partial, unresolved errors remain, or required files/dependencies could not be found - -**Delete tasks:** - -- When a task is no longer relevant or was created in error -- Set status to `deleted` to permanently remove a task - -**Update task details:** - -- When requirements change or become clearer -- When establishing dependencies between tasks -- When assigning task ownership - -## Fields You Can Update - -- **status**: Task status. See Status Workflow below. -- **subject**: Change the task title in imperative form, such as "Run tests" -- **description**: Change the task description -- **activeForm**: Present continuous form shown in the spinner when in_progress, such as "Running tests" -- **owner**: Change the task owner -- **metadata**: Merge metadata keys into the task. Set a key to null to delete it. -- **addBlocks**: Mark tasks that cannot start until this one completes -- **addBlockedBy**: Mark tasks that must complete before this one can start - -## Status Workflow - -Status progresses: `pending` -> `in_progress` -> `completed`. - -Use `deleted` to permanently remove a task. - -## Examples - -Mark task as in progress when starting work: -```json -{"taskId": "1", "status": "in_progress"} -``` - -Mark task as completed after finishing work: -```json -{"taskId": "1", "status": "completed"} -``` - -Delete a task: -```json -{"taskId": "1", "status": "deleted"} -``` - -Set up task dependencies: -```json -{"taskId": "2", "addBlockedBy": ["1"]} -```"#; - -const TASK_LIST_DESCRIPTION: &str = r"Use this tool to list all tasks in the task list. - -## When to Use This Tool - -- To see what tasks are available to work on -- To check overall progress on the project -- To find tasks that are blocked and need dependencies resolved -- After completing a task, to check for newly unblocked work or the next available task -- Prefer working on tasks in ID order, lowest ID first, when multiple tasks are available because earlier tasks often set up context for later ones - -## Output - -Returns a summary of each task: - -- **id**: Task identifier to use with TaskUpdate -- **subject**: Brief description of the task -- **status**: pending, in_progress, or completed -- **owner**: Owner if assigned -- **blockedBy**: List of open task IDs that must be resolved first. Tasks with blockedBy entries should not be started until dependencies resolve. - -Use TaskUpdate to change task status, owner, details, or dependencies."; +const TASK_LIST_DESCRIPTION: &str = "List tasks for the current session, including \ +status, owner, and blocking dependencies."; /// Deterministic todo id derived from `::`. Codex identifies /// a plan step by the exact step text, so the projection ID is the @@ -731,7 +614,7 @@ mod tests { } #[test] - fn anthropic_task_tool_descriptions_include_claude_code_guidance() { + fn anthropic_task_tool_descriptions_are_concise() { let runtime = Arc::new(TodoRuntime::new()); let create = make_task_create_tool(runtime.clone()); let update = make_task_update_tool(runtime.clone()); @@ -741,43 +624,23 @@ mod tests { create .definition .description - .contains("structured task list") + .contains("Create pending tasks") ); - assert!( - create - .definition - .description - .contains("## When to Use This Tool") - ); - assert!(create.definition.description.contains("## Task Fields")); - assert!(create.definition.description.contains("TaskUpdate")); - assert!(create.definition.description.contains("TaskList")); - - assert!(update.definition.description.contains("## Status Workflow")); - assert!( - update - .definition - .description - .contains("ONLY mark a task as completed") - ); - assert!( - update - .definition - .description - .contains("status to `deleted`") - ); - + assert!(create.definition.description.contains("activeForm")); + assert!(update.definition.description.contains("pending")); + assert!(update.definition.description.contains("deleted")); assert!( list.definition .description - .contains("## When to Use This Tool") - ); - assert!(list.definition.description.contains("blocked")); - assert!( - list.definition - .description - .contains("Prefer working on tasks in ID order") + .contains("blocking dependencies") ); + + let total_description_bytes = create.definition.description.len() + + update.definition.description.len() + + list.definition.description.len(); + assert!(total_description_bytes < 600); + assert!(!create.definition.description.contains("##")); + assert!(!update.definition.description.contains("```")); } #[tokio::test] diff --git a/lib/crates/fabro-agent/src/tools.rs b/lib/crates/fabro-agent/src/tools.rs index 9efa86c41..4c605641f 100644 --- a/lib/crates/fabro-agent/src/tools.rs +++ b/lib/crates/fabro-agent/src/tools.rs @@ -6,13 +6,14 @@ use fabro_llm::client::Client; use fabro_llm::types::{Message, Request, ToolDefinition}; use fabro_model::ModelHandle; use fabro_static::EnvVars; -use futures::future::join_all; +use futures::{StreamExt, stream}; use crate::config::SessionOptions; use crate::sandbox::GrepOptions; use crate::tool_registry::{RegisteredTool, ToolRegistry}; const MAX_WEB_FETCH_BYTES: usize = 100 * 1024; +const MAX_READ_MANY_FILES_CONCURRENCY: usize = 8; /// Configuration for the optional LLM-based summarizer used by `web_fetch`. #[derive(Clone)] @@ -387,27 +388,34 @@ pub(crate) fn make_read_many_files_tool() -> RegisteredTool { }, executor: Arc::new(|args, ctx| { Box::pin(async move { - let paths: Vec<&str> = args["paths"] + let paths: Vec = args["paths"] .as_array() .ok_or_else(|| "paths must be an array".to_string())? .iter() .map(|p| { p.as_str() .ok_or_else(|| "each path must be a string".to_string()) + .map(str::to_string) }) .collect::>()?; - let reads = paths.iter().map(|path| { - let env = Arc::clone(&ctx.env); - async move { (*path, env.read_file(path, None, None).await) } - }); - let results = join_all(reads).await; + let results = stream::iter(paths) + .map(|path| { + let env = Arc::clone(&ctx.env); + async move { + let result = env.read_file(&path, None, None).await; + (path, result) + } + }) + .buffered(MAX_READ_MANY_FILES_CONCURRENCY) + .collect::>() + .await; let mut output = String::new(); for (path, result) in results { match result { Ok(content) => { - ctx.env.mark_agent_read(path); + ctx.env.mark_agent_read(&path); let _ = write!(output, "=== {path} ===\n{content}\n\n"); } Err(err) => { @@ -732,7 +740,7 @@ mod tests { }, ) .await; - assert_eq!(result.unwrap(), "3 | line3\n4 | line4\n"); + assert_eq!(result.unwrap(), "2 | line2\n3 | line3\n"); } #[tokio::test] diff --git a/lib/crates/fabro-llm/src/retry.rs b/lib/crates/fabro-llm/src/retry.rs index ef8d049ae..b036c73b4 100644 --- a/lib/crates/fabro-llm/src/retry.rs +++ b/lib/crates/fabro-llm/src/retry.rs @@ -33,16 +33,8 @@ where return Err(err); } - // Check Retry-After - let delay = if let Some(retry_after) = err.retry_after() { - let retry_after_dur = Duration::from_secs_f64(retry_after); - if retry_after_dur > policy.backoff.max_delay { - return Err(err); - } - retry_after_dur - } else { - // Convert from 0-indexed (fabro-llm convention) to 1-indexed (BackoffPolicy) - policy.backoff.delay_for_attempt(attempt + 1) + let Some(delay) = retry_delay(policy, &err, attempt) else { + return Err(err); }; warn!( @@ -64,6 +56,22 @@ where } } +/// Return the delay for a retryable attempt, or `None` when `Retry-After` +/// exceeds the configured maximum delay. +#[must_use] +pub fn retry_delay(policy: &RetryPolicy, err: &Error, attempt: u32) -> Option { + if let Some(retry_after) = err.retry_after() { + let retry_after_dur = Duration::from_secs_f64(retry_after); + if retry_after_dur > policy.backoff.max_delay { + return None; + } + Some(retry_after_dur) + } else { + // Convert from 0-indexed (fabro-llm convention) to 1-indexed (BackoffPolicy). + Some(policy.backoff.delay_for_attempt(attempt + 1)) + } +} + #[cfg(test)] mod tests { use std::sync::Arc; diff --git a/lib/crates/fabro-sandbox/src/docker.rs b/lib/crates/fabro-sandbox/src/docker.rs index 2e22e884d..c7036687a 100644 --- a/lib/crates/fabro-sandbox/src/docker.rs +++ b/lib/crates/fabro-sandbox/src/docker.rs @@ -32,7 +32,7 @@ use crate::sandbox::{StdioProcessControl, optional_timeout, resolve_path}; use crate::{ CommandOutputCallback, DEFAULT_EXEC_OUTPUT_TAIL_BYTES, DirEntry, ExecResult, ExecStreamingResult, GrepOptions, Sandbox, SandboxEvent, SandboxEventCallback, StderrCollector, - StdioProcess, StdioProcessHandle, StdioProcessTermination, shell_quote, + StdioProcess, StdioProcessHandle, StdioProcessTermination, format_lines_numbered, shell_quote, }; pub(crate) const WORKING_DIRECTORY: &str = "/workspace"; @@ -1657,6 +1657,26 @@ impl Sandbox for DockerSandbox { self.download_file_bytes(path).await } + async fn read_file( + &self, + path: &str, + offset: Option, + limit: Option, + ) -> crate::Result { + let container_path = self.resolve_container_path(path); + let (stdout, stderr, exit_code) = self + .docker_exec(vec!["cat".to_string(), container_path.clone()], None, None) + .await?; + + if exit_code != 0 { + return Err(crate::Error::message(format!( + "Failed to read {container_path}: {stderr}" + ))); + } + + Ok(format_lines_numbered(&stdout, offset, limit)) + } + async fn write_file(&self, path: &str, content: &str) -> crate::Result<()> { self.upload_bytes_to_container(path, content.as_bytes()) .await diff --git a/lib/crates/fabro-sandbox/src/sandbox.rs b/lib/crates/fabro-sandbox/src/sandbox.rs index e5da8d796..7138a5d06 100644 --- a/lib/crates/fabro-sandbox/src/sandbox.rs +++ b/lib/crates/fabro-sandbox/src/sandbox.rs @@ -493,12 +493,12 @@ pub type SandboxEventCallback = Arc; /// Formats file content with line numbers for display. /// -/// Applies optional offset (0-based lines to skip) and limit (max lines to -/// return). Line numbers are 1-based and right-aligned. +/// Applies optional offset (1-based starting line number) and limit (max lines +/// to return). Line numbers are 1-based and right-aligned. #[must_use] pub fn format_lines_numbered(content: &str, offset: Option, limit: Option) -> String { let all_lines: Vec<&str> = content.lines().collect(); - let skip = offset.unwrap_or(0); + let skip = offset.unwrap_or(1).saturating_sub(1); let take = limit.unwrap_or(all_lines.len()); let selected: Vec<&str> = all_lines.into_iter().skip(skip).take(take).collect(); let width = (skip + selected.len()).to_string().len().max(1); @@ -1485,7 +1485,7 @@ mod tests { #[test] fn format_lines_numbered_with_offset_limit() { - let result = format_lines_numbered("a\nb\nc\nd\ne", Some(1), Some(2)); + let result = format_lines_numbered("a\nb\nc\nd\ne", Some(2), Some(2)); assert!(result.contains("2 | b")); assert!(result.contains("3 | c")); assert!(!result.contains("1 | a")); diff --git a/lib/crates/fabro-sandbox/src/worktree.rs b/lib/crates/fabro-sandbox/src/worktree.rs index fb0eacf0a..09d866d04 100644 --- a/lib/crates/fabro-sandbox/src/worktree.rs +++ b/lib/crates/fabro-sandbox/src/worktree.rs @@ -287,16 +287,6 @@ impl Sandbox for WorktreeSandbox { self.inner.read_file_bytes(&resolved).await } - async fn read_file( - &self, - path: &str, - offset: Option, - limit: Option, - ) -> crate::Result { - let resolved = self.resolve_path(path); - self.inner.read_file(&resolved, offset, limit).await - } - async fn write_file(&self, path: &str, content: &str) -> crate::Result<()> { let resolved = self.resolve_path(path); self.inner.write_file(&resolved, content).await