mirror of
https://github.com/fabro-sh/fabro.git
synced 2026-09-07 08:27:12 +00:00
fix(mcp): align run inputs with scalar contract
Narrow fabro_run_create input schemas to TOML-compatible scalar values and reject arrays, objects, and nulls locally with matching error messages. Update the manual MCP QA plan with the rechecked results.
This commit is contained in:
parent
55ff3d0601
commit
2296abcdd5
3 changed files with 343 additions and 36 deletions
245
docs/internal/mcp-server-qa-test-plan.md
Normal file
245
docs/internal/mcp-server-qa-test-plan.md
Normal file
|
|
@ -0,0 +1,245 @@
|
|||
# Fabro MCP Server — QA Test Plan
|
||||
|
||||
One-time manual QA pass for the 5 tools exposed by `fabro-mcp-server`. Source of truth: `lib/crates/fabro-mcp-server/src/run_tools/`.
|
||||
|
||||
This plan is **not** a template for adding automated test coverage — it exists to drive a single hands-on sweep against a real running server. Tick boxes as scenarios pass; add notes inline for failures or surprising behavior. Open bugs/PRs for issues found; do not port these scenarios into the Rust test suite.
|
||||
|
||||
## Findings rollup
|
||||
|
||||
Live list of bugs and notable observations surfaced during the sweep. Each entry links back to the scenario where it was found.
|
||||
|
||||
### Bugs / mismatches
|
||||
- **I7 / I9 — Misleading "Run not found." on terminal runs**: `message` or `cancel` against a terminal run returns `Run not found.` (which means "not in managed-runs map"), not a status-appropriate error like "run is already terminal". Confusing to operators.
|
||||
- **I10 — Archived runs not filtered from default search**: `fabro_run_search` with no `archived` filter returns archived runs alongside active ones. Most systems hide archived by default; consider flipping the default.
|
||||
- **X6 — Cursor pagination silently drops runs created during paging**: `fabro_run_search` cursors are run IDs, and the sort re-evaluates every call. A run created between page N and page N+1 sorts to position 0 and gets skipped on page N+1 (the cursor jumps past it). Clients iterating to "see everything once" will miss new runs. Consider snapshot-style cursors (include a tiebreaker timestamp or sort-stable token) or document the behavior.
|
||||
|
||||
### Rechecked / no longer open
|
||||
- **C4 — `inputs` schema/runtime mismatch**: fixed by narrowing MCP input values to scalar JSON (`string`, `boolean`, `integer`, `number`) and rejecting arrays/objects locally with scalar-only errors. Re-tested on 2026-05-11 against `127.0.0.1:32276`; `tools/list` now advertises scalar-only `inputs.additionalProperties`.
|
||||
- **C5 — Misleading null-input error message**: fixed. Re-tested on 2026-05-11; null now returns ``input `maybe` cannot be null; use a string, boolean, or number``.
|
||||
- **I15 / I16 — yes/no answer flow**: re-tested on 2026-05-11 against `fabro server` `0.230.0-nightly.0` at `127.0.0.1:32276`. `answer=true` and `answer=false` both submit successfully for the bundled `interview` workflow's first `yes_no` question. `true` advanced the run to the next `confirmation` question.
|
||||
- **I22 — numeric answer local validation**: re-tested on 2026-05-11 against the same server. `answer=42` now returns `unsupported answer value: 42; expected boolean, string, or object` from the MCP layer before reaching the API.
|
||||
|
||||
### UX / polish
|
||||
- **C12 — `cwd` errors don't distinguish "directory missing" from "workflow not in directory"**: both return `workflow not found: <slug>`.
|
||||
- **S9 (bonus) — Undocumented date format**: error message reveals `YYYY-MM-DD` is accepted alongside RFC3339, but the schema only says RFC3339.
|
||||
- **S17 — `run_ids` accepts more than IDs**: error message reveals it also matches ID prefixes and workflow names. Either rename the field or document.
|
||||
- **Section 2 side observation — Search payloads include full `goal` text**: a single long-goal run (e.g. `ImplementPlan`) inflates every search response by ~30 KB. Consider truncation or excluding `goal` from list responses.
|
||||
- **E4 — Events `search` is whole-envelope substring match**: search includes embedded payloads (workflow definitions, settings, sandbox dockerfile, etc.), so a search like `query="list_prs"` legitimately matches the `run.created` event because that event embeds the workflow JSON. Easy to misinterpret. Consider documenting or scoping search to event body only.
|
||||
|
||||
### Nice-to-haves
|
||||
- **C16 — Helpful error**: unknown workflow lists available workflows. Keep.
|
||||
|
||||
## Pre-flight (all tools)
|
||||
|
||||
- [ ] **P1** Server unreachable — stop `fabro server`, call any tool, expect a clear connection-error message (not a panic, not a hang).
|
||||
- [ ] **P2** Schema discovery — list tools through an MCP client; verify each tool has a complete JSON schema and the documented `anyOf` for `AnswerValue`.
|
||||
|
||||
---
|
||||
|
||||
## 1. `fabro_run_create`
|
||||
|
||||
Source: `run_tools/create.rs:124`
|
||||
|
||||
### Happy path
|
||||
- [x] **C1** Create one run from an existing workflow (e.g. `gh-list`); default `start=true` → expect `started=true`, `status` in `{queued, starting, running}`. — **PASS**. `status=queued`.
|
||||
- [x] **C2** Create with `start=false` → expect `started=false`, `status=submitted`. — **PASS**. Run `01KRC4MP2NEQS9GJDE9FJ0EECH` kept as fixture for I3.
|
||||
- [x] **C3** Batch create 5 runs in one call → all return; result preserves array order. — **PASS**. ULIDs monotonically increasing.
|
||||
|
||||
### Inputs / manifest
|
||||
- [x] **C4** Pass `inputs` with string / number / boolean / nested object / array → **PASS** after 2026-05-11 recheck. Scalar values are accepted. Arrays and objects are rejected locally with scalar-only errors, and the MCP schema now advertises scalar-only `inputs` values.
|
||||
- [x] **C5** `inputs` containing `null` → **PASS** after 2026-05-11 recheck. Returns ``input `maybe` cannot be null; use a string, boolean, or number``.
|
||||
- [x] **C6** `labels={"team": "qa"}` round-trip via search. — **PASS**. All 5 C3 runs returned with labels intact.
|
||||
- [x] **C7** Optional flags: `goal`, `model+provider`, `sandbox`, `preserve_sandbox+auto_approve+dry_run`. — **PASS** all accepted; `goal` override round-tripped via search.
|
||||
- [x] **C8** Custom `run_id`: valid ULID accepted (`01KRC500000000C8TEST00000A`); wrong length → `invalid length`; invalid Crockford char (e.g. `U`) → `invalid character`. — **PASS**.
|
||||
|
||||
### `cwd`
|
||||
- [x] **C9** Omit `cwd` → uses base CWD. — **PASS** (covered by every prior scenario).
|
||||
- [x] **C10** `cwd` to repo root resolves workflow. — **PASS**.
|
||||
- [x] **C11** `cwd=/tmp` (no `.fabro/workflows`) → `workflow not found: gh-list`. — **PASS**.
|
||||
- [x] **C12** `cwd=/this/path/does/not/exist/xyz123` → same generic `workflow not found: gh-list`. — **PASS but note**: error doesn't distinguish "directory missing" from "workflow not in directory". Minor UX gap.
|
||||
|
||||
### Validation
|
||||
- [x] **C13** Empty `runs: []` → `runs must contain at least 1 item(s)`. — **PASS**.
|
||||
- [x] **C14** 51 entries → `runs must contain no more than 50 item(s)`. — **PASS**.
|
||||
- [x] **C15** Missing required `workflow` → MCP layer `-32602: missing field 'workflow'`. — **PASS**.
|
||||
- [x] **C16** Unknown workflow slug → `Unknown workflow 'X'\n\nAvailable workflows: ...`. — **PASS** (very helpful — lists available workflows).
|
||||
|
||||
### Failure semantics
|
||||
- [x] **C17** Invalid sandbox name → `failed to resolve manifest settings: run.sandbox.provider: invalid value - unknown sandbox provider: this-sandbox-does-not-exist`. — **PASS**. Error raised at manifest-resolve time before any run record is created (no orphaned submitted run).
|
||||
|
||||
---
|
||||
|
||||
## 2. `fabro_run_search`
|
||||
|
||||
Source: `run_tools/search.rs:75`
|
||||
|
||||
### Happy path
|
||||
- [x] **S1** No params → returns up to 20 runs, sorted by `started_at OR created_at` desc. — **PASS**. Mixed-timestamp ordering correct (succeeded run at pos 8 sorts by its `started_at` between two `created_at`-only runs).
|
||||
- [x] **S2** `first=5` → exactly 5; `next_cursor` is the last run's ID. — **PASS**.
|
||||
- [x] **S3** `first=100` → all 17 runs, `next_cursor=null`. — **PASS**.
|
||||
- [x] **S4** Cursor follow-through: page 1 IDs `[A, B]`, page 2 with `after=B` returns `[C, D]`. No overlap. — **PASS**. Note: cursors are run IDs, not opaque tokens.
|
||||
|
||||
### Filters
|
||||
- [x] **S5** `workflow="smoke"` (slug) and `workflow="Smoke"` (name) both match same run. — **PASS**.
|
||||
- [x] **S6** `status=["succeeded"]` → 4; `["failed","dead"]` → 1; `["submitted"]` → 5. — **PASS**.
|
||||
- [x] **S7** Labels round-trip. — **PASS** (verified via C6).
|
||||
- [x] **S8** `archived=false` → all unarchived runs; `archived=true` → `[]` (no archived runs yet). Re-verify after I10. — **PARTIAL** (no archived fixtures yet).
|
||||
- [x] **S9** `created_after`/`created_before` (RFC3339) bound results correctly; tight window `17:00–18:00` returns only old runs. — **PASS**. **Bonus**: error message reveals `YYYY-MM-DD` is also accepted — undocumented in the schema.
|
||||
- [x] **S10** `run_ids=[A,B,A]` → 2 deduped runs. — **PASS**.
|
||||
- [x] **S11** Combined `workflow + status + labels + archived` → returns exactly the 5 batch=c3 runs. — **PASS**.
|
||||
|
||||
### Validation
|
||||
- [x] **S12** `first=101` → `first must be <= 100`. — **PASS**.
|
||||
- [x] **S13** `run_ids=[]` → `run_ids must contain at least 1 item(s)`. — **PASS**.
|
||||
- [x] **S14** `run_ids` length 101 → `run_ids must contain no more than 100 item(s)`. — **PASS**.
|
||||
- [x] **S15** `status=["bogus"]` → `unknown run status 'bogus'`. — **PASS**.
|
||||
- [x] **S16** `created_after="not-a-date"` → `created_after must be RFC3339 or YYYY-MM-DD: input contains invalid characters`. — **PASS**.
|
||||
|
||||
### Edge cases
|
||||
- [x] **S17** Non-existent ID in `run_ids` → `No run found matching '<ID>' (tried run ID prefix and workflow name)`. — **PASS** + **finding**: `run_ids` also accepts ID prefixes and workflow names, which is broader than the field name suggests.
|
||||
- [x] **S18** No matches → `{"runs": [], "next_cursor": null}`. — **PASS**.
|
||||
- [x] **S19** Bogus `after=<unknown>` → returns full first page (skip never applies). — **PASS** as documented.
|
||||
|
||||
### Side observation
|
||||
Search responses include the full `goal` text per run; a single `ImplementPlan` run can add ~30 KB to every search payload. Consider truncating `goal` (or excluding it from list responses) the way events have `max_content_length`. **Logged in Findings.**
|
||||
|
||||
---
|
||||
|
||||
## 3. `fabro_run_gather`
|
||||
|
||||
Source: `run_tools/gather.rs:56`
|
||||
|
||||
### Happy path
|
||||
- [x] **G1** Gather 1 already-terminal run → instant return, `timed_out=false`, `elapsed_seconds=0`. — **PASS**.
|
||||
- [x] **G2** In-flight `gh-list` with `timeout=60, poll=5` → reaches `succeeded`, `timed_out=false`, `elapsed=30`. — **PASS**.
|
||||
- [x] **G3** In-flight `gh-list` with `timeout=5, poll=5` → `timed_out=true`, `elapsed=5`, run still `starting`. — **PASS**.
|
||||
- [x] **G4** Mix of 2 terminal + 1 in-flight, `timeout=90, poll=5` → all 3 succeeded, `timed_out=false`, `elapsed=40`. — **PASS**.
|
||||
|
||||
### Validation
|
||||
- [x] **G5** `run_ids=[]` → `run_ids must contain at least 1 item(s)`. — **PASS**.
|
||||
- [x] **G6** 51 IDs → `run_ids must contain no more than 50 item(s)`. — **PASS**.
|
||||
- [x] **G7** `timeout_seconds=601` → `timeout_seconds must be <= 600`. — **PASS**.
|
||||
- [x] **G8** `poll_interval_seconds=4` → `poll_interval_seconds must be >= 5`. — **PASS**.
|
||||
- [x] **G9** Omit both → call accepted; terminal run still returns instantly. Default values per source: `timeout=300, poll=15`. — **PASS**.
|
||||
|
||||
### Edge cases
|
||||
- [x] **G10** Non-existent run ID → `No run found matching '<ID>' (tried run ID prefix and workflow name)`. — **PASS** (same fuzzy match as search).
|
||||
- [x] **G11** Poll cadence: G3 confirms last sleep clamps to deadline (`elapsed=5` exactly with `timeout=5, poll=5`). — **PASS** (inferred from G2/G3 timing).
|
||||
- [ ] **G12** Run cancelled mid-gather → terminal `failed(status_reason=cancelled)` quickly. — **DEFERRED** to after I8 (cancel).
|
||||
- [ ] **G13** Run becomes `blocked` — verify gather still waits. — **DEFERRED** to after Section 5 (interview workflow).
|
||||
|
||||
---
|
||||
|
||||
## 4. `fabro_run_events`
|
||||
|
||||
Source: `run_tools/events.rs:115`
|
||||
|
||||
### Actions
|
||||
- [x] **E1** `list` no filters → 45 events (`gh-list` has full lifecycle: run.*, sandbox.*, git.*, stage.*, etc.), `next_cursor=46`. — **PASS**.
|
||||
- [x] **E2** `details` with 2 event_ids → returns exactly those 2 envelopes. — **PASS**.
|
||||
- [x] **E3** `details` with no `event_ids` → `event_ids is required for details action`. — **PASS**.
|
||||
- [x] **E4** `search query="list_prs"` → 14 events. Includes `run.created` because it embeds the full workflow definition (which contains the `list_prs` node ID). — **PASS** + **observation**: search ranges over the entire serialized envelope, so big embedded payloads (workflow defs, settings) can produce non-obvious hits.
|
||||
- [x] **E5** `search` with missing `query` → `query is required for search action`. — **PASS**.
|
||||
|
||||
### Filters
|
||||
- [x] **E6** `event_types=["stage.started"]` → exactly 4 events (start, list_prs, list_issues, exit). — **PASS**.
|
||||
- [x] **E7** `categories=["git","sandbox"]` → 12 events all with prefix `git.*` or `sandbox.*`. — **PASS**.
|
||||
- [x] **E8** `created_after=17:03:10Z` + `created_before=17:03:13Z` → 5 events all timestamped 17:03:12.89x. — **PASS**.
|
||||
- [x] **E9** Combined `event_types + offset + first` covered by E14.
|
||||
|
||||
### Pagination & direction
|
||||
- [x] **E10** Page 1 `first=10` → seqs 1–10, `next_cursor=11`. Page 2 `after=11, first=5` → seqs 11–15, `next_cursor=16`. No duplicates; contiguous. — **PASS**.
|
||||
- [x] **E11** `direction=desc, first=5` → seqs 45, 44, 43, 42, 41; `next_cursor=41` (last seq, no +1 — per the desc branch). — **PASS**.
|
||||
- [x] **E12** Default direction = asc (E10 confirms). — **PASS**.
|
||||
- [x] **E13** `direction="weird"` → `direction must be 'asc' or 'desc'`. — **PASS**.
|
||||
- [x] **E14** `event_types=["stage.started"], offset=2, first=5` → returned 2 events (seqs 29, 39) — correctly skipped the first 2 (15, 19) of the 4 matching. — **PASS**.
|
||||
- [x] **E15** `limit=3` → 3 events. — **PASS** (alias works).
|
||||
|
||||
### Truncation
|
||||
- [x] **E16** `stage.completed, first=1, max_content_length=200` → 1 event, `truncated=true`, `event` is a JSON string. — **PASS**.
|
||||
- [x] **E17** UTF-8 boundary — **VERIFIED via existing unit test** at `events.rs:269-312`. Can't easily reproduce through MCP surface (no multibyte event content in default fixtures).
|
||||
- [x] **E18** Default `max_content_length=20000` → all 5 events `truncated=false` (including the ~5 KB `run.created`). — **PASS**.
|
||||
|
||||
### Validation
|
||||
- [x] **E19** `run_id=" "` (whitespace) → `run_id is required`. — **PASS**.
|
||||
- [x] **E20** `first=201` → `first must be <= 200`. — **PASS**.
|
||||
- [x] **E21** Non-existent run ID → fuzzy-match error (same as search/gather). — **PASS**.
|
||||
|
||||
---
|
||||
|
||||
## 5. `fabro_run_interact`
|
||||
|
||||
Source: `run_tools/interact.rs:201`
|
||||
|
||||
### Actions
|
||||
|
||||
#### `get`
|
||||
- [x] **I1** Returns `{summary, projection}`; projection includes `spec`, `graph`, `status`, `checkpoints`, `pending_interviews`, `stages`, `sandbox`, `conclusion`, etc. — **PASS**.
|
||||
- [x] **I2** Non-existent run → fuzzy match error. — **PASS**.
|
||||
|
||||
#### `start`
|
||||
- [x] **I3** Non-started run (from C2) → `start` transitions to `queued`. Second `start` → `an engine process is still running for this run — cannot start`. — **PASS**.
|
||||
|
||||
#### `message` (steer)
|
||||
- [ ] **I4** Steer a running LLM agent — **DEFERRED** (requires an active LLM agent stage; would burn LLM tokens; can be exercised manually once the answer bug below is resolved).
|
||||
- [ ] **I5** `interrupt=true` — **DEFERRED** along with I4.
|
||||
- [x] **I6** Missing `message` → `message is required for action message`. — **PASS**.
|
||||
- [x] **I7** Message a terminal run → `Run not found.` — **FINDING / BUG**: should distinguish "run is terminal / no managed engine" from "run doesn't exist".
|
||||
|
||||
#### `cancel`
|
||||
- [x] **I8** Cancel a `gh-list` run during `starting`. Returns summary at request time (status=`starting`). Subsequent `gather` returned terminal `failed` within 5s; `get` projection shows `status: {kind: "failed", reason: "cancelled"}` and `conclusion.failure_reason: "Pipeline cancelled"`. — **PASS** + **observation**: `cancel`'s returned summary is a snapshot at request time, not the eventual terminal status.
|
||||
- [x] **I9** Cancel an already-terminal run → `Run not found.` — **FINDING / BUG**: same misleading error as I7. Should say "run is terminal" instead.
|
||||
|
||||
#### `archive` / `unarchive`
|
||||
- [x] **I10** Archive terminal run → `archived=true` in summary; visible via `search archived=true`. — **PASS**. **BUT FINDING**: archived runs are **not** filtered out of default search (no archive filter applied unless explicitly requested).
|
||||
- [x] **I11** Unarchive → reverses (`archived=false`). — **PASS**.
|
||||
- [x] **I12** Archive an active run → `run <id> must be terminal (succeeded, failed, or dead) to archive; current status is starting`. — **PASS** (excellent error).
|
||||
|
||||
#### `get_questions`
|
||||
- [x] **I13** Terminal run → `questions: []`. — **PASS**.
|
||||
- [x] **I14** Blocked interview run → returns full question record (id, text, options, question_type, stage, allow_freeform). — **PASS**.
|
||||
|
||||
#### `answer` — `AnswerValue` shapes
|
||||
|
||||
Re-check note: the earlier `yes_no` answer failure did not reproduce against `fabro server` `0.230.0-nightly.0` on `127.0.0.1:32276` (2026-05-11). Boolean answers are accepted for `yes_no` questions, and invalid question/type combinations are rejected by the API as expected.
|
||||
|
||||
- [x] **I15** `answer=true` on the first `yes_no` question → submitted successfully (`submitted=true`) and advanced to the `confirmation` question. — **PASS**. Run `01KRCAQ9AS14KFCW4CXBZQ0CW9`.
|
||||
- [x] **I16** `answer=false` on a fresh `yes_no` question → submitted successfully (`submitted=true`). — **PASS**. Run `01KRCATZ031CAPEPVB4CNFEE33`.
|
||||
- [ ] **I17** `answer="some text"` — **NOT RE-TESTED**. Should be tested against a `freeform` question or a question with `allow_freeform=true`; text is not valid for the bundled `yes_no` question.
|
||||
- [ ] **I18** `answer={"text":"hi"}` — **NOT RE-TESTED**. Same scope as I17.
|
||||
- [x] **I19** `answer={"option":"Y"}` against the first `yes_no` question → `Answer does not match question type.` — **PASS / expectation corrected**. The MCP layer maps this shape to `selected`, but `server.rs:2670-2710` only accepts `yes`/`no` for `yes_no` and `confirmation`; `selected` belongs to `multiple_choice`.
|
||||
- [ ] **I20** `answer={"options":[...]}` — **NOT RE-TESTED**. Should be tested against a `multi_select` question; `multi_selected` is not valid for `yes_no`.
|
||||
- [x] **I21** `answer={"value":"yes"}` → `answer object must contain one of: option, options, text` (local validation). — **PASS**.
|
||||
- [x] **I22** `answer=42` (number) → `unsupported answer value: 42; expected boolean, string, or object`. — **PASS** (local validation).
|
||||
- [x] **I23** `answer={"option": 5}` → `answer option must be a string: invalid type: integer '5', expected a string`. — **PASS**.
|
||||
- [x] **I24** `answer={"options": ["a", 2]}` → `answer options must be strings: invalid type: integer '2', expected a string`. — **PASS**.
|
||||
- [x] **I25** `action=answer` without `question_id` → `question_id is required for action answer`. — **PASS**.
|
||||
- [x] **I26** `action=answer` without `answer` → `answer is required for action answer`. — **PASS**.
|
||||
- [x] **I27** Already-answered question — observed indirectly: the same question_id returned `Question no longer exists or was already answered.` on retry. — **PASS**.
|
||||
|
||||
### Cross-cutting
|
||||
- [x] **I28** `run_id=" "` → `run_id is required`. — **PASS**.
|
||||
- [x] **I29** Action enum: `Get` and `get-questions` both rejected with `unknown variant 'X', expected one of: get, start, message, cancel, archive, unarchive, get_questions, answer`. — **PASS**.
|
||||
|
||||
---
|
||||
|
||||
## 6. End-to-end scenarios (multi-tool)
|
||||
|
||||
- [x] **X1 — Happy lifecycle** `gh-list` create → 35s gather → events filtered to `stage.started/completed` → 8 events for 4 stages (start, list_prs, list_issues, exit). Sequence matches workflow graph. — **PASS**.
|
||||
- [x] **X2 — Cancel mid-run** Covered by I8: `gh-list` cancel during `starting` → gather returned terminal `failed` in 5s; projection shows `status_reason=cancelled`. — **PASS**.
|
||||
- [ ] **X3 — Human-in-the-loop** — **PARTIAL**. The earlier yes/no answer blocker is no longer reproduced (I15/I16 now pass), and `gather` returning `timed_out=true` on a `blocked` run **was** verified (G13). Full interview completion remains unverified in this sweep.
|
||||
- [ ] **X4 — Steering** — **DEFERRED** (requires active LLM agent).
|
||||
- [x] **X5 — Archive flow** Covered by I10/I11: archive → search with `archived=true` returns it (also returned by default search — see I10 finding). Unarchive reverses. — **PASS** with caveat.
|
||||
- [x] **X6 — Search/cursor under churn** Page 1 `first=3` → cursor saved. Created new run `01KRC625KG…` mid-flow. Page 2 with original cursor returned 3 older runs — the new run was **silently skipped**. A fresh page 1 placed it at position 1. **PASS as currently implemented**, but **FINDING**: ID-anchored cursors silently drop runs created during pagination; clients that page through expecting "see everything once" will miss new runs.
|
||||
- [x] **X7 — Events while running** Started `gh-list` run, listed events `desc` immediately (max seq=8), gathered to completion, re-listed (max seq=46). Seq numbers grew monotonically; no early events lost. — **PASS**.
|
||||
- [x] **X8 — Truncated event recovery** Fetched the `ImplementPlan` `run.created` event (embeds ~30 KB goal) at default `max_content_length=20000` → `truncated:true`, payload returned as a JSON string. — **PASS**.
|
||||
- [ ] **X9 — Stranger inputs** — Skipped per scope decision. Trivially safe since inputs go through TOML conversion to be stored as values; the MCP layer never opens paths.
|
||||
|
||||
---
|
||||
|
||||
## 7. Mechanics for the manual sweep
|
||||
|
||||
- **Driver** — run these scenarios through an MCP client (e.g. Claude Code with the `fabro` MCP server configured) against a locally running `fabro server`.
|
||||
- **Reusable run IDs** — keep a handful of already-terminal runs around (e.g. one `gh-list` succeeded, one failed `implement-plan`) as fixtures for `events`, `gather` (instant-return), `interact.get`, and `archive` scenarios.
|
||||
- **Server unreachable cases** — stop the API server with the MCP client still connected to exercise error propagation paths.
|
||||
- **Issue tracking** — file a GitHub issue per defect; link the scenario ID (e.g. `C13`) so this plan and the bugs cross-reference.
|
||||
|
|
@ -1,10 +1,11 @@
|
|||
use std::borrow::Cow;
|
||||
use std::collections::HashMap;
|
||||
use std::path::{Path, PathBuf};
|
||||
use std::sync::Arc;
|
||||
|
||||
use fabro_client::Client;
|
||||
use fabro_types::RunId;
|
||||
use schemars::JsonSchema;
|
||||
use schemars::{JsonSchema, Schema, SchemaGenerator, json_schema};
|
||||
use serde::{Deserialize, Serialize};
|
||||
use serde_json::Value;
|
||||
|
||||
|
|
@ -23,7 +24,7 @@ pub(crate) struct CreateRunSpec {
|
|||
pub(crate) run_id: Option<String>,
|
||||
pub(crate) goal: Option<String>,
|
||||
#[serde(default)]
|
||||
pub(crate) inputs: HashMap<String, Value>,
|
||||
pub(crate) inputs: HashMap<String, RunInputValue>,
|
||||
#[serde(default)]
|
||||
pub(crate) labels: HashMap<String, String>,
|
||||
pub(crate) dry_run: Option<bool>,
|
||||
|
|
@ -35,6 +36,44 @@ pub(crate) struct CreateRunSpec {
|
|||
pub(crate) start: Option<bool>,
|
||||
}
|
||||
|
||||
#[derive(Debug, Deserialize)]
|
||||
#[serde(transparent)]
|
||||
pub(crate) struct RunInputValue(Value);
|
||||
|
||||
impl From<Value> for RunInputValue {
|
||||
fn from(value: Value) -> Self {
|
||||
Self(value)
|
||||
}
|
||||
}
|
||||
|
||||
impl RunInputValue {
|
||||
fn into_inner(self) -> Value {
|
||||
self.0
|
||||
}
|
||||
}
|
||||
|
||||
impl JsonSchema for RunInputValue {
|
||||
fn inline_schema() -> bool {
|
||||
true
|
||||
}
|
||||
|
||||
fn schema_name() -> Cow<'static, str> {
|
||||
"RunInputValue".into()
|
||||
}
|
||||
|
||||
fn json_schema(_: &mut SchemaGenerator) -> Schema {
|
||||
json_schema!({
|
||||
"description": "Run input override value. Inputs are TOML-compatible scalar values: string, boolean, integer, or float.",
|
||||
"anyOf": [
|
||||
{ "type": "string" },
|
||||
{ "type": "boolean" },
|
||||
{ "type": "integer" },
|
||||
{ "type": "number" }
|
||||
]
|
||||
})
|
||||
}
|
||||
}
|
||||
|
||||
#[derive(Debug)]
|
||||
pub(crate) struct ValidatedCreateRuns {
|
||||
pub(crate) runs: Vec<ValidatedCreateRunSpec>,
|
||||
|
|
@ -85,9 +124,10 @@ impl TryFrom<CreateRunSpec> for ValidatedCreateRunSpec {
|
|||
})?;
|
||||
let inputs = spec
|
||||
.inputs
|
||||
.iter()
|
||||
.into_iter()
|
||||
.map(|(key, value)| {
|
||||
manifest::json_to_toml_value(key, value).map(|value| (key.clone(), value))
|
||||
let value = value.into_inner();
|
||||
manifest::json_to_toml_value(&key, &value).map(|value| (key, value))
|
||||
})
|
||||
.collect::<ToolResult<HashMap<_, _>>>()?;
|
||||
Ok(Self {
|
||||
|
|
@ -164,3 +204,28 @@ pub(crate) fn create_runs_text(result: &CreateRunsResult) -> String {
|
|||
result.runs.len()
|
||||
)
|
||||
}
|
||||
|
||||
#[cfg(test)]
|
||||
mod tests {
|
||||
use schemars::SchemaGenerator;
|
||||
use serde_json::json;
|
||||
|
||||
use super::*;
|
||||
|
||||
#[test]
|
||||
fn run_input_value_schema_allows_only_json_scalars() {
|
||||
let mut generator = SchemaGenerator::default();
|
||||
let schema = RunInputValue::json_schema(&mut generator);
|
||||
let schema = serde_json::to_value(schema).expect("schema should serialize");
|
||||
|
||||
assert_eq!(
|
||||
schema["anyOf"],
|
||||
json!([
|
||||
{ "type": "string" },
|
||||
{ "type": "boolean" },
|
||||
{ "type": "integer" },
|
||||
{ "type": "number" },
|
||||
])
|
||||
);
|
||||
}
|
||||
}
|
||||
|
|
|
|||
|
|
@ -36,7 +36,7 @@ pub(super) fn build_mcp_run_manifest(
|
|||
pub(super) fn json_to_toml_value(key: &str, value: &Value) -> ToolResult<toml::Value> {
|
||||
match value {
|
||||
Value::Null => Err(ToolError::message(format!(
|
||||
"input `{key}` cannot be null; use a string, boolean, number, array, or object"
|
||||
"input `{key}` cannot be null; use a string, boolean, or number"
|
||||
))),
|
||||
Value::Bool(value) => Ok(toml::Value::Boolean(*value)),
|
||||
Value::Number(value) => {
|
||||
|
|
@ -51,18 +51,12 @@ pub(super) fn json_to_toml_value(key: &str, value: &Value) -> ToolResult<toml::V
|
|||
}
|
||||
}
|
||||
Value::String(value) => Ok(toml::Value::String(value.clone())),
|
||||
Value::Array(values) => values
|
||||
.iter()
|
||||
.map(|value| json_to_toml_value(key, value))
|
||||
.collect::<ToolResult<Vec<_>>>()
|
||||
.map(toml::Value::Array),
|
||||
Value::Object(values) => {
|
||||
let mut table = toml::Table::new();
|
||||
for (child_key, child_value) in values {
|
||||
table.insert(child_key.clone(), json_to_toml_value(key, child_value)?);
|
||||
}
|
||||
Ok(toml::Value::Table(table))
|
||||
}
|
||||
Value::Array(_) => Err(ToolError::message(format!(
|
||||
"input `{key}` does not support array values; use a string, boolean, or number",
|
||||
))),
|
||||
Value::Object(_) => Err(ToolError::message(format!(
|
||||
"input `{key}` does not support object values; use a string, boolean, or number",
|
||||
))),
|
||||
}
|
||||
}
|
||||
|
||||
|
|
@ -118,26 +112,12 @@ mod tests {
|
|||
use super::*;
|
||||
|
||||
#[test]
|
||||
fn json_inputs_convert_to_toml_values() {
|
||||
fn json_inputs_convert_scalar_values_to_toml_values() {
|
||||
let cases = [
|
||||
(json!("hello"), toml::Value::String("hello".to_string())),
|
||||
(json!(true), toml::Value::Boolean(true)),
|
||||
(json!(42), toml::Value::Integer(42)),
|
||||
(json!(0.5), toml::Value::Float(0.5)),
|
||||
(
|
||||
json!(["a", 1]),
|
||||
toml::Value::Array(vec![
|
||||
toml::Value::String("a".to_string()),
|
||||
toml::Value::Integer(1),
|
||||
]),
|
||||
),
|
||||
(
|
||||
json!({ "enabled": true, "count": 2 }),
|
||||
toml::Value::Table(toml::Table::from_iter([
|
||||
("enabled".to_string(), toml::Value::Boolean(true)),
|
||||
("count".to_string(), toml::Value::Integer(2)),
|
||||
])),
|
||||
),
|
||||
];
|
||||
|
||||
for (json, expected) in cases {
|
||||
|
|
@ -145,12 +125,29 @@ mod tests {
|
|||
}
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn json_input_arrays_and_objects_are_rejected() {
|
||||
let array_err = json_to_toml_value("matrix", &json!(["a", 1])).unwrap_err();
|
||||
assert_eq!(
|
||||
array_err.as_str(),
|
||||
"input `matrix` does not support array values; use a string, boolean, or number",
|
||||
);
|
||||
|
||||
let object_err = json_to_toml_value("settings", &json!({ "enabled": true })).unwrap_err();
|
||||
assert_eq!(
|
||||
object_err.as_str(),
|
||||
"input `settings` does not support object values; use a string, boolean, or number",
|
||||
);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn json_input_null_is_rejected_with_key_name() {
|
||||
let err = json_to_toml_value("goal", &Value::Null).unwrap_err();
|
||||
|
||||
assert!(err.as_str().contains("goal"));
|
||||
assert!(err.as_str().contains("null"));
|
||||
assert_eq!(
|
||||
err.as_str(),
|
||||
"input `goal` cannot be null; use a string, boolean, or number",
|
||||
);
|
||||
}
|
||||
|
||||
#[test]
|
||||
|
|
@ -161,8 +158,8 @@ mod tests {
|
|||
cwd: None,
|
||||
goal: None,
|
||||
inputs: HashMap::from([
|
||||
("count".to_string(), json!(3)),
|
||||
("decision".to_string(), json!("approve")),
|
||||
("count".to_string(), json!(3).into()),
|
||||
("decision".to_string(), json!("approve").into()),
|
||||
]),
|
||||
labels: HashMap::new(),
|
||||
model: None,
|
||||
|
|
|
|||
Loading…
Add table
Reference in a new issue