diff --git a/CLAUDE.md b/CLAUDE.md index 51054bc..6bcdd28 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -136,7 +136,7 @@ The agent loop lives in `src/runtime/loop.py`. It is the highest-risk file in th - Single loop, spec-driven. No per-agent branching. - Decrement budgets **before** dispatch, not after, so a hung tool cannot overrun. - Stream partial assistant text as it arrives; buffer tool calls until complete. -- Termination reasons are an enum: `Completed | BudgetExceeded | Deadline | ConfirmationPending | ToolFailure | Refused`. Every run ends with exactly one. +- Termination reasons are an enum: `Completed | BudgetExceeded | Deadline | ConfirmationPending | ToolFailure | GatewayFailure | Refused`. Every run ends with exactly one. `GatewayFailure` is the model provider not answering (rate limited, request rejected, credential refused, unreachable) and is deliberately not `ToolFailure`: the two are different operational questions, and until 2026-09-22 the enum could not tell them apart. - Persist a full trajectory per run: every message, tool call, tool result, and budget snapshot. This is what makes debugging and evals possible — it is not optional telemetry. - Delegation is a tool call from the parent's perspective. Subagent runs get their own trajectory, linked by `parent_run_id`. @@ -220,21 +220,11 @@ depends on the curated-versus-self-serve decision and is not settled. | Layer | State | |---|---| | Surfaces | `POST /api/v1/agents/{id}/runs` (streams over SSE on `Accept: text/event-stream`), `GET /api/v1/runs/{id}`; the chat panel is the only answering path — the browser simulator is deleted | -| Orchestration | spec-driven loop, four bounds claimed before dispatch, six terminations, trajectories in `agent_runs`; delegation per §6 — a subagent is a tool call, runs as the caller, shares the parent budget, capped at depth 2, and writes its own trajectory linked by `parent_run_id` | -| Registry | 9 agents + 24 skills as rows; published versions immutable (append-only, trigger-enforced); runs pin the version they started with | +| Orchestration | spec-driven loop, four bounds claimed before dispatch, seven terminations, trajectories in `agent_runs`; delegation per §6 — a subagent is a tool call, runs as the caller, shares the parent budget, capped at depth 2, and writes its own trajectory linked by `parent_run_id` | +| Registry | 9 agents + 23 skills as rows; published versions immutable (append-only, trigger-enforced); runs pin the version they started with | | Tools | 19, two of which write (`move_application`, `assign_worker`), behind a bound single-use confirmation | | Knowledge | ACL-tagged ingest, hybrid dense + BM25 fused with RRF, pre-filtered | -| Gateway | tier → model + effort, token accounting, refusal as an outcome; one wire protocol — `openai`, the chat-completions shape that Groq (the default), Gemini, OpenRouter, Together, vLLM and a local Ollama all serve. The Anthropic path was removed; `MODEL_PROVIDER=anthropic`, a stale `ANTHROPIC_API_KEY` and a leftover `claude-*` id are each refused at startup rather than ignored | - -**Conversational writes are not agent tool calls.** Two skills — `create-position` -and `create-employee-role` — collect a record through the chat panel and then -write it with the same REST call the manual form uses, as the signed-in user. -They are therefore outside I4's confirmation-token mechanism, which governs -tools an AGENT invokes on a caller's behalf. The person is making the request -themselves, and the flow's review step ("Ready to create this position?") is -where they agree to it. Worth knowing rather than worth fixing: if a write is -ever moved from the panel into an agent tool, it acquires I4's bound single-use -confirmation at that point and not before. +| Gateway | tier → model + effort, token accounting, refusal as an outcome | **Deviations from this document, all deliberate and all flagged in code:** @@ -260,18 +250,7 @@ confirmation at that point and not before. Do not resolve these unilaterally. Flag them and ask. - **Who authors agents?** Curated (the team ships specs) vs. self-serve (tenants author their own). Self-serve requires prompt-injection hardening at the authoring boundary, per-tenant cost caps, an approval workflow, and a sandbox — roughly 3× the platform. Current assumption: **curated**, with the registry designed so self-serve is additive later. -- **Model hosting.** Self-hosted vs. API vs. mixed by tier. **Still open** — - but no longer expensive to change: `MODEL_BASE_URL` + the three `MODEL_*` ids - move the whole platform between Groq (the default), Gemini, OpenRouter, - Together, vLLM and a local Ollama without a code change, and `make eval-live` - runs the suite against whichever is configured. Decide it on the eval - evidence, and weigh the I7 case heaviest: a cheaper model that follows the - planted injection is a security regression, not a saving. - - The gap that the Anthropic removal opened here is closed: `openai/gpt-oss-120b` - on Groq has been through `make eval-live` and passes all three cases including - I7 (2026-09-07). The decision itself — self-hosted vs. API vs. mixed by tier — - is still open and still not mine to settle. +- **Model hosting.** Self-hosted vs. API vs. mixed by tier. - **Confirmation UX.** Inline in-chat vs. an approval queue. --- diff --git a/go-api/internal/domain/definitions_schema_test.go b/go-api/internal/domain/definitions_schema_test.go index 6961512..245f89e 100644 --- a/go-api/internal/domain/definitions_schema_test.go +++ b/go-api/internal/domain/definitions_schema_test.go @@ -733,6 +733,9 @@ func TestMigrationPairsAreComplete(t *testing.T) { // Phase 5: shared rate limit counters, so a limit means the same thing // behind one instance and behind ten. "000015_rate_limits.up.sql", + // A seventh termination reason. The CHECK in 000006 was chosen so + // this would be a migration rather than an ALTER TYPE; this is it. + "000016_gateway_failure_termination.up.sql", } if len(ups) != len(want) { t.Fatalf("%d migrations, want %d — update this list deliberately", len(ups), len(want)) diff --git a/go-api/internal/httpserver/runs.go b/go-api/internal/httpserver/runs.go index 23ee4ca..b453a9c 100644 --- a/go-api/internal/httpserver/runs.go +++ b/go-api/internal/httpserver/runs.go @@ -205,7 +205,7 @@ func buildRunResponse(res *runtime.ExecutionResult) runResponse { // reached its budget before finishing" is accurate and means nothing to // somebody who has never heard of a token budget. // -// Every one of the six is spelled out. A default that said "something went +// Every one of the seven is spelled out. A default that said "something went // wrong" would be the place where a Refused run and a ToolFailure became // indistinguishable to the person best placed to tell us which it was. func terminationMessage(t runtime.Termination) string { @@ -224,6 +224,12 @@ func terminationMessage(t runtime.Termination) string { "Nothing was changed." case runtime.TerminationRefused: return "The agent declined to answer this one." + case runtime.TerminationGatewayFailure: + // The one termination where "try again" is honest advice: the + // dominant cause is a rate limit that clears within a minute, and + // nothing about the question itself was the problem. + return "The model behind this agent did not answer — usually it is busy. " + + "Wait a minute and ask again. Nothing was changed." default: return "The agent did not finish." } diff --git a/go-api/internal/runtime/budget.go b/go-api/internal/runtime/budget.go index 7715759..410d705 100644 --- a/go-api/internal/runtime/budget.go +++ b/go-api/internal/runtime/budget.go @@ -22,13 +22,26 @@ const ( TerminationConfirmationPending Termination = "ConfirmationPending" TerminationToolFailure Termination = "ToolFailure" TerminationRefused Termination = "Refused" + + // GatewayFailure is the model provider failing to answer at all: rate + // limited, rejected the request, refused the credential, or unreachable. + // Added 2026-09-22 because until then every one of those was recorded as + // ToolFailure, and 131 of 318 production runs read as "a tool is broken" + // when no tool had failed — 45 of them were Groq's free-tier rate limit, + // which is a capacity decision, not a bug. The two are different questions + // to an operator ("what did we break" versus "what are we not paying + // for"), and an enum that could not tell them apart hid the answer for + // two weeks. Refused and Deadline keep their own reasons; this is the + // rest of the gateway's vocabulary. + TerminationGatewayFailure Termination = "GatewayFailure" ) -// Valid reports whether t is one of the six. +// Valid reports whether t is one of the seven. func (t Termination) Valid() bool { switch t { case TerminationCompleted, TerminationBudgetExceeded, TerminationDeadline, - TerminationConfirmationPending, TerminationToolFailure, TerminationRefused: + TerminationConfirmationPending, TerminationToolFailure, TerminationRefused, + TerminationGatewayFailure: return true } return false diff --git a/go-api/internal/runtime/delegate.go b/go-api/internal/runtime/delegate.go index 1d1d9c8..fae9e11 100644 --- a/go-api/internal/runtime/delegate.go +++ b/go-api/internal/runtime/delegate.go @@ -241,12 +241,16 @@ func (m *ModelExecutor) delegate( rec.AddChildren(collected.Runs) if res == nil { - msg := "the subagent returned nothing" + // The parent sees a delegation as a tool call, but the REASON it + // failed is still worth carrying: a subagent the provider rate limited + // should read as GatewayFailure in the parent's trajectory too, or the + // parent's operator goes looking for a tool that never broke. + msg, term := "the subagent returned nothing", TerminationToolFailure if err != nil { - msg = err.Error() + msg, term = err.Error(), terminationFor(err) } return delegationAnswer{ - Agent: sub.ID, Termination: string(TerminationToolFailure), Error: msg, + Agent: sub.ID, Termination: string(term), Error: msg, }, nil } diff --git a/go-api/internal/runtime/loop.go b/go-api/internal/runtime/loop.go index 91e47d3..29b0305 100644 --- a/go-api/internal/runtime/loop.go +++ b/go-api/internal/runtime/loop.go @@ -555,6 +555,12 @@ func (m *ModelExecutor) runTools( // a Refused run is one that must not be retried. Flattening them into a single // failure reason would make every one of those distinctions unanswerable from // the trajectory. +// +// Any other gateway error is GatewayFailure, not ToolFailure. The provider +// being rate limited, rejecting the request or refusing the key is not a tool +// failing, and calling it one sent two weeks of operators looking for a broken +// tool that did not exist. A non-gateway error — a tool the model invented, a +// result that could not be encoded — is still the tool layer's. func terminationFor(err error) Termination { var gwErr *gateway.Error if !errors.As(err, &gwErr) { @@ -566,7 +572,7 @@ func terminationFor(err error) Termination { case gateway.CodeTimeout: return TerminationDeadline default: - return TerminationToolFailure + return TerminationGatewayFailure } } @@ -657,6 +663,8 @@ func terminationMessage(t Termination) string { return "the run is waiting on a confirmation" case TerminationToolFailure: return "the run failed" + case TerminationGatewayFailure: + return "the model provider did not answer" default: return string(t) } diff --git a/go-api/internal/runtime/loop_test.go b/go-api/internal/runtime/loop_test.go index 076ad2b..e468450 100644 --- a/go-api/internal/runtime/loop_test.go +++ b/go-api/internal/runtime/loop_test.go @@ -277,6 +277,7 @@ func TestTerminationValidRejectsInvented(t *testing.T) { for _, ok := range []Termination{ TerminationCompleted, TerminationBudgetExceeded, TerminationDeadline, TerminationConfirmationPending, TerminationToolFailure, TerminationRefused, + TerminationGatewayFailure, } { if !ok.Valid() { t.Errorf("%q should be a valid termination", ok) @@ -1075,3 +1076,31 @@ func TestTheTrajectoryRecordsWhichChunksGroundedTheAnswer(t *testing.T) { t.Error("nothing in the trajectory says a retrieval happened") } } + +// The whole reason GatewayFailure exists: a provider that will not answer is +// not a tool that broke, and for two weeks the trajectory said it was. +func TestTerminationForSeparatesGatewayFromTool(t *testing.T) { + gw := func(code string) error { return &gateway.Error{Code: code, Message: "x"} } + cases := []struct { + name string + err error + want Termination + }{ + {"rate limited is the gateway's", gw(gateway.CodeRateLimited), TerminationGatewayFailure}, + {"invalid request is the gateway's", gw(gateway.CodeInvalidRequest), TerminationGatewayFailure}, + {"bad credential is the gateway's", gw(gateway.CodeUnauthorized), TerminationGatewayFailure}, + {"upstream is the gateway's", gw(gateway.CodeUpstream), TerminationGatewayFailure}, + {"not configured is the gateway's", gw(gateway.CodeNotConfigured), TerminationGatewayFailure}, + {"refused keeps its own reason", gw(gateway.CodeRefused), TerminationRefused}, + {"timeout keeps its own reason", gw(gateway.CodeTimeout), TerminationDeadline}, + {"a non-gateway error is still the tool layer's", errors.New("tool exploded"), TerminationToolFailure}, + {"a wrapped gateway error is still found", fmt.Errorf("delegating: %w", gw(gateway.CodeRateLimited)), TerminationGatewayFailure}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + if got := terminationFor(tc.err); got != tc.want { + t.Errorf("terminationFor(%v) = %s, want %s", tc.err, got, tc.want) + } + }) + } +} diff --git a/go-api/internal/runtime/trajectory.go b/go-api/internal/runtime/trajectory.go index ec44f71..da1e9cc 100644 --- a/go-api/internal/runtime/trajectory.go +++ b/go-api/internal/runtime/trajectory.go @@ -285,7 +285,7 @@ func (r *Recorder) SetModel(model string) { // Finish closes the trajectory with its termination reason and returns it. // -// A reason that is not one of the six is recorded as ToolFailure rather than +// A reason that is not one of the seven is recorded as ToolFailure rather than // stored as-is: an unrecognised termination is a bug in the loop, and writing // it verbatim would let that bug propagate into every eval and dashboard that // groups by this column. diff --git a/migrations/000016_gateway_failure_termination.down.sql b/migrations/000016_gateway_failure_termination.down.sql new file mode 100644 index 0000000..bb98bde --- /dev/null +++ b/migrations/000016_gateway_failure_termination.down.sql @@ -0,0 +1,10 @@ +-- Reverting narrows the vocabulary, so rows that used the seventh value are +-- folded back into ToolFailure FIRST — the classification they would have had +-- before 000016 — or the narrower CHECK cannot be re-added at all. This loses +-- the distinction the up migration introduced, which is what reverting means. +UPDATE agent_runs SET termination = 'ToolFailure' WHERE termination = 'GatewayFailure'; +ALTER TABLE agent_runs DROP CONSTRAINT agent_runs_termination_check; +ALTER TABLE agent_runs ADD CONSTRAINT agent_runs_termination_check CHECK (termination IN ( + 'Completed', 'BudgetExceeded', 'Deadline', + 'ConfirmationPending', 'ToolFailure', 'Refused' +)); diff --git a/migrations/000016_gateway_failure_termination.up.sql b/migrations/000016_gateway_failure_termination.up.sql new file mode 100644 index 0000000..342ed48 --- /dev/null +++ b/migrations/000016_gateway_failure_termination.up.sql @@ -0,0 +1,22 @@ +-- A seventh termination reason: GatewayFailure. +-- +-- 000006 chose a CHECK over an enum type so that "a new termination reason +-- should be a migration, but not one that requires ALTER TYPE". This is that +-- migration. +-- +-- Until now the model provider failing — rate limited, request rejected, key +-- refused, unreachable — was recorded as ToolFailure, because the enum had no +-- other place for it. On 2026-09-22 that was 131 of 318 production runs, of +-- which not one was a tool failing. The distinction is the difference between +-- "what did we break" and "what are we not paying for", and the column that +-- every dashboard and eval groups by could not make it. +-- +-- Existing rows are left as they are. Rewriting history from the trajectory +-- text would be guesswork against a message format that has changed twice; +-- the trajectory entries still carry the gateway.* error code for anyone who +-- needs to reclassify the past. +ALTER TABLE agent_runs DROP CONSTRAINT agent_runs_termination_check; +ALTER TABLE agent_runs ADD CONSTRAINT agent_runs_termination_check CHECK (termination IN ( + 'Completed', 'BudgetExceeded', 'Deadline', + 'ConfirmationPending', 'ToolFailure', 'Refused', 'GatewayFailure' +));