[amiddavid]

Behavior change (needs a human developer's yes)

• Who it affects: users whose subagents end with a StructuredOutput call. Hosted and local.
• Old behavior: the proxy pinged such a thread up to 2 times after the final answer. All of those pings were wasted.
• New behavior: the proxy sends no pings for it.
• Also changes: a SubagentHandback call now reads waiting_for = human (was own_tool). It only changes the label. It does not change pings.
• How to turn it off: set cache.keepalive_terminal_tools: [] in the config. The old behavior returns. The default list is [StructuredOutput].

Summary

• StructuredOutput always ended the thread: 226 of 226 on the hosted proxy in 7 days (waitcause: a subagent's StructuredOutput call is its final answer, not a tool wait #857).
• A new waiting_for value, done, marks such a thread.
• The keep-alive plans no pings for a done thread.

What changed

• internal/waitcause: done value, DefaultTerminalTools, IsDone, ClassifyWaitingForTerminal. ClassifyWaitingFor keeps its signature and uses the default list.
• Only the last tool_use counts. A terminal tool followed by another call stays own_tool.
• SubagentHandback (last call) is classified like end_turn with nothing open.
• keepalive: Options.TerminalTools. pingable() returns false for a done thread. The sweep's wait cause uses the same list.
• config: cache.keepalive_terminal_tools. nil is the default list. [] is off. proxy.Options.TerminalTools carries it. It is process-wide, also in hosted mode.
• Docs: docs/how-to/cache-keepalive.md, docs/data-dependence.md. Comment in dash/event.go.
• CacheConfig now holds a slice, so two tests that compared it with == now use reflect.DeepEqual.

How it was tested

On the eval box, with -race:

• go vet ./...: clean.
• ./internal/waitcause ./keepalive ./components ./config ./tenant ./cmd ./apply: pass.
• ./dash/...: pass. ./proxy/...: pass.
• New tests: classifier cases (terminal alone, terminal last, terminal not last, handback, failed call, empty list, custom list). Keeper tests: done thread gets no ping and holds no credential; terminal tool not last is pinged; empty list pings; custom list.
• Mutation: removed the terminal-tool check in IsDone. 4 tests failed. Restored: they pass.

Data dependence

Metadata only. The rule reads the tool name of the last tool_use block. The proxy already reads it. It works at every storage level (off, metadata only, everything). docs/data-dependence.md row "Wait-cause and lifecycle signals" is updated.

Other readers of waiting_for

• dash (schema, store, query, breakdown dimension): plain text column. A new value shows as its own bucket done. No change needed.
• Plugin band and mod (context-guru-plugin/, codex-marketplace): no reader of waiting_for.
• extract_llm_sweep held answer and shadow log: store the string as-is.
• scripts/analysis/wait_causes.py: has its own classifier on transcripts. Not changed here.

Closes #857

🤖 Generated with Claude Code

[amiddavid]

Reviewer	Kind	Started by	Context	Review	Round	Head	Verdict
Reviewer 1 (glm, user of @amiddavid)	GLM rits/zai-org/glm-5-3 via Claude Code 2.1.316	offload-review skill, @amiddavid's session	fresh	1 of 2	1 of 2	affb47a60bfd	blocking

Verdict: blocking

The change is sound in its core. The classifier, the keeper gate, and the tests are careful.
Two gaps remain: the off switch does not work for hosted tenants, and a truncated response
window can mark a live thread as done.

Findings

1. config/config.go:173 — a hosted tenant's keepalive_terminal_tools is parsed, then silently dropped

   • What fails: hosted mode, a tenant document sets cache: keepalive_terminal_tools: [] to turn
     the rule off. buildTenantConfig maps the tenant's cache: block through cachePolicy
     (cmd/context-guru-proxy/main.go:1124). proxy.CachePolicy has no terminal field, so the value
     is lost. proxy.Options.TerminalTools (main.go:690) holds only the host process config. The
     tenant keeps the #857 behavior, cannot turn it off, and gets no warning. The PR body itself
     says the change affects "Hosted and local" users, and names cache.keepalive_terminal_tools: []
     as the off switch — an instruction that does nothing for half its audience.
   • Proof: grep KeepAliveTerminalTools over the repo: read only at main.go:690 (the host's own
     config). cachePolicy (main.go:1117-1118) is the only path a tenant document's cache: block
     takes, and it drops the field. docs/how-to/cache-keepalive.md:74 says "Set
     keepalive_terminal_tools: [] to turn this off" with no host-only caveat. The key also
     survives a settings-page save (ApplyForm writes only its own tuning keys), so a tenant can
     carry it indefinitely without effect.
   • Fix: pick one. (a) State in docs/how-to that the key is host-config only, and that hosted
     tenants cannot change it. (b) Carry the list per tenant, the way cachePolicy carries the
     rest of the cache: block.

2. keepalive/keeper.go:310 — a spliced sniff window can mark a live thread done, so it gets no pings

   • What fails: a streamed response longer than the sniffer's window (64 KiB head + 64 KiB tail,
     proxy/usage.go:681). The response calls StructuredOutput with over 64 KiB of arguments, then
     one more tool with over 64 KiB of arguments. The second call's content_block_start event
     falls in the dropped middle, so responseToolUseNames never sees it. The last visible name is
     StructuredOutput, so IsDone answers true. The thread really waits on the second tool's
     result, but the keeper plans no pings. When the result arrives, the thread pays a cold cache
     rewrite. The dashboard row also reads waiting_for = done, which is wrong.
   • Proof: proxy/usage.go:603-606 documents ToolUseNames as best-effort over the bounded window,
     "not exhaustive". responseToolUseNames (proxy/usage.go:606) reads names only from events
     inside sniffer.bytes(). Before this PR the same truncation could only mislabel wait_tool;
     IsDone turns the last visible name into a terminal verdict. The PR's own test
     "StructuredOutput NOT last stays own_tool" states the intent that only the true last call
     counts; the window breaks that intent silently.
   • Bound: money only, at most the strategy's ping value per thread. No data loss, no fail-open
     break. The scenario is rare (it needs two tool calls of over 64 KiB each in one response).
   • Fix: responseUsageWhy already knows the spliced flag. Carry it next to ToolUseNames
     (a field or a sentinel), and make IsDone answer false for a response whose name list may be
     incomplete. A spliced response then keeps today's ping behavior.

Non-blocking

• keepalive/keeper.go:944 — a thread refused because it is done is counted with reason
  ReasonBelowLimits. An operator reading the refuse stats sees "below limits" for a thread that
  was fine. A dedicated reason would read true.
• internal/waitcause/waitcause.go:81-85 — IsDone's recover() cannot fire; nothing in the
  function can panic. terminalSet also builds a map on every call, now on the record path of
  every request. A package-level set for the default list drops both.
• internal/waitcause/waitcause.go:151 — done wins over open background and teammate counts, and
  no test covers StructuredOutput with openBackground > 0. If the 226-of-226 measurement
  included threads with open background tasks, say so in the PR. If it did not, background should
  win, as it already does for SubagentHandback.
• proxy/proxy.go:2032-2039 — on the streamed path, when the final expand round's usage does not
  parse, ToolUseNames keeps the earlier round's list while StopReason is refreshed. The shape
  predates this PR, but IsDone now acts on the stale list. The usual failure direction is
  missed-done (pings sent), which is safe.
• The wiring config → proxy.Options.TerminalTools → keeper and capture has no test. The library
  behavior is tested; the three assignments are not.

What I checked

• The full diff, and the code around each change: ClassifyWaitingForTerminal's order of checks,
  IsDone, pingable, Record's refuse path, the sweep's held answer, the capture closures.
• Fail open: IsDone is pure and cannot panic; a false answer only skips pings. The request path
  is untouched.
• The off switch at each layer: nil vs [] vs a custom list, in the classifier tests, the keeper
  tests, and terminalSet.
• Reversibility and data dependence: tool names only; the data-dependence row is updated and
  matches the code.
• The config round trip: CacheConfig is never marshaled, so nil cannot become [] behind an
  operator's back. A form save writes only its own tuning keys, so a stored
  keepalive_terminal_tools survives.
• The dash side: waiting_for is a plain TEXT column with no enum; no UI or query code enumerates
  its values, so done lands as its own bucket.
• ClassifyWaitingFor (old signature) has no production caller left; both live callers pass the
  configured list.
• main.go's two config paths (single-tenant host config vs hosted tenant documents), which is
  finding 1.
• The PR body: it names the behavior change, the off switch, and the data dependence, as the repo
  rules require.

[amiddavid]

Reviewer	Kind	Started by	Context	Review	Round	Head	Verdict
Reviewer 2 (glm, user of @amiddavid)	GLM rits/zai-org/glm-5-3 via Claude Code 2.1.316	offload-review skill, @amiddavid's session	fresh	2 of 2	1 of 2	affb47a60bfd	blocking

Verdict: blocking

Findings

1. config/config.go:173 — a tenant document accepts keepalive_terminal_tools and silently ignores it, so the documented off switch does not work in hosted mode

   • What fails: a hosted-mode tenant (or an operator editing a tenant document, which is where hosted cache: config lives — buildTenantConfig, cmd/context-guru-proxy/main.go:1117) sets keepalive_terminal_tools: [] to get the pre-#857 pings back. The strict loader accepts the key, because CacheConfig is shared by tenant documents. cachePolicy (main.go:1124) maps the block onto proxy.CachePolicy and drops the field, and proxy.Options.TerminalTools comes only from the host's own top-level config (main.go:690). The tenant's setting has no effect and no error is shown. docs/how-to/cache-keepalive.md:74 says "Set keepalive_terminal_tools: [] to turn this off" without saying the key is host-level only; the PR body does say "process-wide, also in hosted mode", but the operator doc does not.
   • Proof: grep shows KeepAliveTerminalTools is read in exactly one place (main.go:690, from the host's cfg.Cache); buildTenantConfig → cachePolicy(cfg.Cache) never touches it; the tenant document goes through config.LoadBytes, which knows the key. So the key is valid in a tenant document and dead there.
   • Fix: say in docs/how-to/cache-keepalive.md that the key is host-level config only and is ignored in a tenant document (the operator sets it in the main config file). Better: reject the key in tenant documents, so a silently-ignored setting cannot be stored at all. This is a doc/config decision — it needs the owner's word, not mine.

2. config/config.go:173 — no test pins the nil-versus-[] semantics of the new YAML key

   • What fails: the off switch rests on one distinction: an absent key loads as nil (default list on) and keepalive_terminal_tools: [] loads as a non-nil empty list (rule off). No test loads a YAML document and asserts either value. The keeper tests (keepalive/terminal_test.go:53) build []string{} by hand, so a later change in the loader, the form round-trip, or a normalization that turns [] into nil would flip every "off" deployment back to the default rule with no failing test.
   • Proof: grep -rn "TerminalTools\|terminal_tools" config/*_test.go tenant/*_test.go cmd/ proxy/*_test.go matches only main.go:690 — no test anywhere loads the key from YAML.
   • Fix: add a config test: load a document with the key absent and assert cfg.Cache.KeepAliveTerminalTools == nil; load one with keepalive_terminal_tools: [] and assert it is non-nil and empty; load [Finish] and assert the value. One test function, three cases.

Non-blocking

• internal/waitcause/waitcause.go:80 — the recover in IsDone is dead code: a length check and a map read cannot panic. Harmless, but it suggests a panic risk that is not there.
• internal/waitcause/waitcause.go:54 — DefaultTerminalTools is an exported mutable var. Any caller could mutate it and change process behavior. A function or an unexported var would be safer.
• internal/waitcause/waitcause.go:138 — ClassifyWaitingFor kept its signature but changed behavior (it now applies the default terminal list, so a last StructuredOutput reads done). Its doc comment does not say so.
• A configured list of only empty strings ([""]) turns the terminal match off but leaves the SubagentHandback relabel on, while [] turns both off. An edge case of a malformed list; terminalSet drops empty names but len(terminal) > 0 still gates the handback rule.
• scripts/analysis/wait_causes.py:58 — WAITING_FOR_VALUES does not know done. The PR says the script is unchanged on purpose; fine, noted for whoever runs it on post-#857 transcripts.
• The default list applies to every provider and every host, though the 226-of-226 measurement is hosted Claude traffic. On OpenAI a function named StructuredOutput would also end the pings. The failure direction is the cheap one (a forgone ping, not a wrong one), and the PR names the behavior change at the top, so this is only a note.
• The proxy-layer wiring (dashcapture's terminal, newKeeper reading h.opts.TerminalTools) has no test of its own. The behavior it carries is tested one layer down, so I did not make it a finding.

What I checked

• Read CLAUDE.md and docs/design.md (pipeline, fail-open, keep-alive rules, data dependence).
• Read the full diff c2a06dd5..HEAD and the code around every hunk: internal/waitcause/waitcause.go (whole file), keepalive/keeper.go (Record, pingable, Sweep, Options, entry fields), keepalive/substitute.go, keepalive/terminal_test.go plus its helpers (testKeeperWith, kaPolicy, fakeDispatcher.hold/holds/n), proxy/dashcapture.go (newCapture, finish closures), proxy/keepalive.go (newKeeper/newKeeperWith and the strategy-rebuild path), proxy/proxy.go, cmd/context-guru-proxy/main.go (Options wiring, buildTenantConfig, cachePolicy), config/config.go, config/form.go (ApplyForm merge — a settings save preserves the new key in a stored document), both changed tests and why they had to change (CacheConfig became non-comparable; I grepped for other ==/!= on Cache/Config and found none).
• Traced the classification logic by hand: terminal-last → done; terminal not last → own_tool; handback-last → end_turn-like path with toolUseNames nil and usageOK forced true; empty list skips both rules; failed wins over done. Confirmed no consumer switches on WaitingFor values (dash schema/store/query store the string as-is; no UI file reads it).
• Confirmed a done thread never reaches a ping: pingable() is false at Record, so no entry (and no credential hold) is created; Sweep, Plans and the extra-slot path all sit behind the same gate. A done thread that resumes is tracked again on its next response, because turns outlives the entry.
• Checked lock discipline: pingable runs under k.mu in Sweep/Plans; sweepWaitCause copies the entry fields under the lock; capture.terminal is written at capture creation and read on the writer goroutine after a channel handoff.
• Checked the PR body (description only, no comments): the behavior change is named at the top with who it affects, old and new behavior, and the off switch, per RELEASING.md.
• Did not run builds or tests (not approved; Go is not on this machine).

[amiddavid]

Behavior change: approved by the owner (@amiddavid), 2026-11-02, in the session that started this PR.

The approved change:

• Who it affects: local and hosted users whose subagents end with a StructuredOutput call.
• Old behavior: the keep-alive sent up to 2 pings after the subagent's final answer. No tool result ever comes back, so those pings were wasted.
• New behavior: a thread whose last tool call is in the terminal-tool list (default [StructuredOutput]) gets no pings. A last SubagentHandback is treated like end_turn.
• How to turn it off: cache.keepalive_terminal_tools: [] in the host config.

The owner gave this yes in words, in the session. This comment records it on the PR so the integrator can read it. The round-2 review fixes do not widen the change: they make the rule fire in fewer cases (a cut-off response is never done).

[amiddavid]

Round 1 answers. All fixes are in 4cb67cfa.

Blocking

• R1-1, R2-1 (tenant key ignored): fixed. buildTenantConfig now refuses a tenant document that sets cache.keepalive_terminal_tools. The key is new, so no stored document has it. docs/how-to/cache-keepalive.md says the key is host config only. Test: TestTenantDocumentRefusesTerminalTools.
• R1-2 (spliced window): fixed. Usage.ToolUseIncomplete is set from the spliced flag in responseUsageWhy. It travels in keepalive.Record and into the capture. When it is true, the terminal list is empty for that response. The thread is never done and is pinged as before. Tests: TestIncompleteToolNamesAreNeverDone, TestToolUseIncompleteFollowsSpliced. Mutation (guard disabled) made the keeper test fail.
• R2-2 (YAML test): fixed. config/terminal_tools_test.go: absent gives nil, [] gives a non-nil empty list, [Finish] gives that list.

Non-blocking

• Own refuse reason: fixed. New ReasonDone = done. Added to the dash comment, the band/mod text (model.ts and its test), and both cg.py copies.
• Dead recover, map per call: fixed. IsDone is plain. The default set is built once.
• Exported default list: fixed. DefaultTerminalTools() returns a copy.
• ClassifyWaitingFor doc: fixed.
• List of only empty strings: fixed. It acts like []. The SubagentHandback relabel uses the cleaned set. Test: TestEmptyNamesListActsLikeEmpty.
• done against open background: kept as is, test added. A finished subagent gets no later notification on its own thread, so nothing it waits on can wake it. Test case: StructuredOutput with openBackground 2 and openTeammate 1 gives done.
• Wiring test: not added. The three assignments (config to proxy.Options to keeper and capture) sit inside main's large setup function. A test would need that whole setup. The tenant refusal and the YAML tests cover the ends.
• Stale ToolUseNames after an expand round (proxy.go:2032): left. The failure direction is a missed done (pings sent), which is safe.
• scripts/analysis/wait_causes.py: out of scope. It has its own transcript classifier, and keepalive: choose the ping count from what the thread waits for (measured on two sources) #858 covers it.

[amiddavid]

Reviewer	Kind	Started by	Context	Review	Round	Head	Verdict
Reviewer 1 (glm, user of @amiddavid)	GLM rits/zai-org/glm-5-3 via Claude Code 2.1.316	offload-review skill, @amiddavid's session	resumed	1 of 2	2 of 2	4cb67cfaf4d9	blocking

Verdict: blocking

Round 1 asked for two fixes. Both were addressed, and the spliced-window fix is correct end to
end. But the tenant-key fix landed in the wrong layer: buildTenantConfig also rebuilds the
single-tenant host's own document, so the new refusal breaks /api/adapt there, and it fires
too late (request time, as a 401) for a hosted document that does carry the key.

Findings

1. cmd/context-guru-proxy/main.go:1116 — the tenant-key refusal breaks /api/adapt on a single-tenant proxy, and misses the hosted save path
   • What fails, single-tenant: the operator sets cache.keepalive_terminal_tools in the proxy's
     config file — the documented host use (docs/how-to/cache-keepalive.md:74 says "host
     configuration only", and main.go:692 reads it from that same file). main.go:609-612 makes
     that file the BaseConfig, and main.go:652 wires BuildConfig: buildTenantConfig with no
     hosted-mode gate. Every /api/adapt apply or revert (proxy/adaptapply.go:321, 345) edits
     the BaseConfig and rebuilds it through buildTenantConfig, which now refuses the key. So
     every apply on that proxy answers 400: "cache.keepalive_terminal_tools is host
     configuration; a tenant document cannot set it" — for the host's OWN document. The plugin's
     /context-guru:save-more flow is such an apply. A saved override also stops reapplying at
     startup (adaptapply.go:378 warns and skips). Before this commit, all of that worked.
   • What fails, hosted: the refusal sits at build time, not save time. tenant.Registry.Update
     validates a stored document with config.Validate (tenant/tenant.go:703-706), which accepts
     the key. The account editor takes a raw document. A manager who pastes one with the key
     passes save validation and the document is stored. The tenant's every request then fails in
     TenantSource.tenancy, and statusOf maps the plain error to 401 unauthorized
     (proxy/tenancy.go:417-420): the tenant is bricked behind a misleading auth error. The
     commit's premise "the key is new, so no stored document has it" holds only until a manager
     pastes one.
   • Proof: BaseConfig = the config file's bytes (main.go:609-612); BuildConfig is
     buildTenantConfig unconditionally (main.go:651-652), also when tenants == nil;
     reloadLocal calls h.opts.BuildConfig on every local apply and revert
     (adaptapply.go:290, 321, 345). config.Validate (config/config.go:381-390) runs
     LoadBytes + Build only — nothing refuses the key before storage.
   • Fix: enforce the restriction where a TENANT document is saved, not where any document is
     built. Wrap the Validate handed to tenant.Open (main.go:519) to also refuse the key, and
     let buildTenantConfig accept it again (or pass a tenant/local flag). A manager then gets a
     400 on the settings page at save time, and the local adapt path — which rebuilds the host's
     own document — keeps working.

Round 1 findings: status

• R1-1 (tenant key ignored): addressed, but the fix is finding 1. The refusal is real and
  tested (TestTenantDocumentRefusesTerminalTools), and the docs now say host-only. The layer
  is wrong.
• R1-2 (spliced window can read done): fixed, verified end to end.
  responseUsageWhy sets Usage.ToolUseIncomplete from the sniffer's spliced flag
  (proxy/usage.go:373). It travels into keepalive.Record.ToolUseIncomplete and the capture
  (proxy/keepalive.go:392, proxy/dashcapture.go:509-511), and terminalFor empties the list for
  that response (keepalive/keeper.go:272-277). Both tests cover it. The flag and the name list
  always come from the same round, including the sticky-usage multi-round paths. The direction
  is conservative: any stream over the 64 KiB head window keeps today's pings.
• Own refuse reason: fixed. ReasonDone = "done" (keepalive/manualhold.go:62), set in
  Record's refuse path, and added to the dash comment, the band and mod text (model.ts and its
  test), and both cg.py copies. The strings match the keeper's.
• Dead recover, map per call, exported mutable list: fixed. IsDone is plain; the default
  set is built once; DefaultTerminalTools() returns a copy, with a test.
• done vs open background: kept, with a reason and a test. The reason — a finished
  subagent gets no later notification on its own thread — is sound, and
  "done beats an open background task" now pins it. I accept it.
• Stale ToolUseNames after an expand round: left, safe direction. Accepted.
• Wiring test: declined with a reason. The ends (YAML semantics, tenant refusal) are now
  tested; the three assignments sit inside main's setup. Accepted.

Reviewer 2, round 1

• R2-1 (tenant key ignored): same as my R1-1. Addressed; see finding 1 for the defect the
  fix adds.
• R2-2 (no YAML test for nil vs []): fixed. config/terminal_tools_test.go loads absent
  (nil), [] (non-nil, empty) and [Finish], and asserts each. I checked the cases and the
  loader path; they are right.
• Reviewer 2's non-blocking notes are all addressed or answered above, except the OpenAI
  cross-provider note (a function named StructuredOutput on OpenAI also ends pings). That
  stands unanswered; the failure direction is a forgone ping, so it stays a note.

Non-blocking

• terminalFor (keepalive/keeper.go:273) allocates a fresh empty slice per recorded response
  with an incomplete name list. One small allocation on a rare path; a package-level empty slice
  would drop it.
• The owner's approval of the behavior change is recorded on the PR (comment, 2026-11-02
  15:55:52Z), with who it affects and the off switch. That satisfies the repo's rule.

What I checked

• Last review: round 1, affb47a, 2026-11-02T15:52:03Z.
• New commits read: 1 — 4cb67cfa "fix(waitcause): round 1 review fixes for the terminal-tool
  rule (#857)".
• Read in full: 2 reviews, 2 comments, 0 inline comments (from the three files
  .review-in-reviews.txt, .review-in-comments.txt, .review-in-inline.txt).
• New items since my last review, and what I did about each:
  • Reviewer 2's round-1 review (15:52:52Z): read in full; both findings checked — one shared
    with mine, one (the YAML test) verified as fixed.
  • Owner's approval comment (15:55:52Z): read; checked it names the change, the audience and
    the off switch.
  • Author's round-1 answers (16:16:29Z): read; each claim re-verified in the code (see
    "Round 1 findings: status").
  • Commit 4cb67cfa: full diff read, plus the code around every hunk.
• The refusal's blast radius: every caller of buildTenantConfig (main.go:526 tenant source,
  main.go:652 BuildConfig), the whole /api/adapt local path (adaptWriteLocal,
  adaptRevertLocal, the startup reapply), the hosted save path (tenant.Registry.Update →
  config.Validate), and how a build error reaches the tenant (statusOf → 401). This is
  finding 1.
• The spliced fix at every responseUsageWhy call site (sniffed stream, buffered, SSE splicer),
  and the pairing of ToolUseNames with ToolUseIncomplete across the sticky multi-round
  usage merge.
• The waitcause rewrite by hand: nil → shipped set, [] and [""] → rule off (done and
  handback both), custom list → replaced, handback still gated on the cleaned set. No stale
  reference to the old DefaultTerminalTools var remains.
• The new tests' premises: stateOf exists (keepalive/manualhold_test.go:30), preset: off is
  a real preset (config/config.go:486), components.NopEmitter{} is the same emitter
  config.Validate uses.
• Did not run builds or tests (not approved; Go is not on this machine).

[amiddavid]

Reviewer	Kind	Started by	Context	Review	Round	Head	Verdict
Reviewer 2 (glm, user of @amiddavid)	GLM rits/zai-org/glm-5-3 via Claude Code 2.1.316	offload-review skill, @amiddavid's session	resumed	2 of 2	2 of 2	4cb67cfaf4d9	blocking

Verdict: blocking

Round 1 is answered well: the spliced-window guard is right, the YAML semantics are pinned, the
smaller items are all fixed. But the tenant-refusal fix has the wrong blast radius: it also fires
on the single-tenant /api/adapt path, which rebuilds the host's own config document — the one
place the key is legitimate.

Findings

1. cmd/context-guru-proxy/main.go:1116 — the tenant-document refusal breaks every /api/adapt write on a single-tenant proxy whose config file sets keepalive_terminal_tools

   • What fails: an operator follows the new docs and puts keepalive_terminal_tools: [Foo] in the config file passed with --config. Startup is fine (loadConfig reads it, proxy.Options.TerminalTools is set). Then any /api/adapt/* write — settings, compaction, tools, the plugin's save-more — calls adaptWrite → adaptWriteLocal (proxy/adaptapply.go:131), which starts from h.opts.BaseConfig (the same file, main.go:608-613), edits it (the edits preserve unknown keys), and rebuilds it through BuildConfig = buildTenantConfig (adaptapply.go:290-292). buildTenantConfig now refuses any document that sets the key, so the whole apply fails with HTTP 400, and the error message is wrong for this case: the document is not "a tenant document", it is the host's own config, where the key is valid. The operator cannot apply any adapt change until they remove the key from the file and restart.
   • Proof: main.go:652 wires BuildConfig: buildTenantConfig for the single-tenant host too, and main.go:608-613 puts the --config file's bytes into BaseConfig when cfgPath is set. reloadLocal (adaptapply.go:287-292) is the only consumer of BuildConfig on that path and passes it the edited BaseConfig. The refusal at main.go:1113-1117 checks only cfg.Cache.KeepAliveTerminalTools != nil, with no way to tell a tenant registry rebuild from a local host rebuild.
   • Fix: scope the refusal to tenant documents only. The tenant registry path is proxy.NewTenantSource(reg, emitter, buildTenantConfig, ...) (main.go:525) — wrap the check there (a small func(doc []byte, e components.Emitter) that loads, refuses on the key, then calls buildTenantConfig), and leave BuildConfig permissive, since it rebuilds the host's own document. A test should cover both: a tenant doc with the key is refused; a local adapt rebuild of a doc with the key succeeds.

2. docs/dashboard.md:143 — the ping_reason value list does not include done

   • What fails: the round-2 commit added the new refuse reason everywhere it enumerated the values (dash/cachestate.go comment, model.ts long and short forms plus its test, both cg.py copies) but missed this doc. docs/dashboard.md says "ping_reason is one of:" and lists ten values; done is a real, reachable value (ReasonDone is set by Record's refuse path and read back through /api/cachestate), so the doc's exhaustive list is now wrong by one.
   • Proof: keepalive/manualhold.go adds ReasonDone = "done"; keeper.go:963-965 sets it via setReasonLocked; the reasons map is what the Ping button's explanation and /api/cachestate read (manualhold.go's own doc). The commit updated four of the six places that enumerate the values; docs/dashboard.md:143-153 and the plugin Go test below are the two it missed.
   • Fix: add a done line to the list in docs/dashboard.md (one sentence: the thread's last response ended with a terminal tool, so nothing is pinged).

Non-blocking

• context-guru-plugin/ping_test.go:236 — TestPingReasonWords verifies words for every ping_reason but its table does not include done. The words exist (both cg.py copies, model.ts); only the test's table misses the new value.
• With ToolUseIncomplete true, the whole #857 rule set is off for that response, including the SubagentHandback relabel: a spliced response ending in a handback reads own_tool, not human. That is the pre-#857 label, the safe direction, and consistent between the keeper and the dash row. Note only.

Earlier findings, and the answers

• R2-1 / R1-1 (tenant key silently ignored): fixed in intent, but the fix is finding 1. The refusal plus the doc line answer the original gap for hosted tenant documents. The author's reason "the key is new, so no stored document has it" is right for stored tenant docs — but the adapt path reads the host's live config file, not a stored tenant doc, which is exactly the case the refusal breaks.
• R1-2 (spliced window can mark a live thread done): fixed, correctly. I verified the flag on every path: responseUsageWhy sets ToolUseIncomplete = spliced; the sniffed window passes sn.spliced() (proxy.go:2463), the full-body paths pass false (proxy.go:2132, and responseUsage is test-only now). The streamed path takes its whole Usage — names and flag together — from the window call (proxy.go:2021-2035), so a long stream whose middle is dropped is also covered. terminalFor turns the flag into an empty list for that response only, in both the keeper and the dash capture. Tests cover both layers.
• R2-2 (YAML nil-vs-[] test): fixed. config/terminal_tools_test.go pins absent → nil, [] → non-nil empty, [Finish] → the list.
• Reviewer 1, "done vs open background" (kept, with a reason): the reason is right, I accept it. A thread that ended with StructuredOutput never gets another request — a background task's notification lands on the main agent's thread, not the finished subagent's — so done is the truer label and the no-ping decision is unchanged either way. The new test case (openBackground 2, openTeammate 1 → done) pins it.
• Reviewer 1, stale ToolUseNames after an unparsed expand round (left): accepted. I traced it: the stale list is the previous round's, whose last name is the expand tool itself, not a terminal tool, so a false done needs a shape the continuation loop does not produce. The realistic direction is a missed done (one ping sent), which only spends money.
• Reviewer 1, wiring test not added: accepted. The ends are covered (YAML load, tenant refusal); the middle is three assignments inside main's setup.
• My round-1 non-blocking items: all fixed (dead recover removed, default set built once, DefaultTerminalTools() returns a copy, ClassifyWaitingFor doc updated, [""] now acts like [] with a test).
• Owner approval: recorded on the PR (comment at 15:55:52Z), naming who it affects, the old and new behavior, and the off switch. The round-2 fixes do not widen the approved change — the refusal in finding 1 is a config-validation change of its own, which is why it needs the owner's look, not just a code fix.

What I checked

• Last review: round 1, affb47a60bfd872a9e60c1fd2140cbf4b5aa0b88, 2026-11-02T15:52:52Z.
• New commits read: 1 — 4cb67cfa "fix(waitcause): round 1 review fixes for the terminal-tool rule (#857)".
• Read in full: 2 reviews, 2 comments, 0 inline comments (from .review-in-reviews.txt, .review-in-comments.txt, .review-in-inline.txt; the inline file is empty).
• New items since my last review: the fix commit (full diff read, every hunk); the owner's approval comment (checked it names the change and the off switch); the author's round-1 answers comment (checked each claim against the code — all verified except the tenant-refusal blast radius, finding 1).
• Verified the spliced fix end to end: Usage.ToolUseIncomplete in responseUsageWhy, its three production callers and their spliced arguments, the streamed path's usage merge (proxy.go:2021-2135), Record.ToolUseIncomplete → terminalFor → kaEntry.terminal, and the dash capture's override.
• Verified the refusal's reach: BuildConfig vs BaseConfig (proxy/proxy.go:207-212), adaptWrite → adaptWriteLocal → reloadLocal → buildTenantConfig, and NewTenantSource's use of the same builder — which is finding 1.
• Verified the reason plumbing: ReasonDone, the refuse-path ordering (over budget beats done; done beats below limits), stateOf test helper, and every place that enumerates ping_reason values (grep "pings used" across Go, TS, Python and docs) — which is finding 2 and the non-blocking test gap.
• Checked resolveTerminal semantics by hand: nil → shipped set (built once), [] and [""] → empty set → both rules off, custom list → fresh set; DefaultTerminalTools() returns a copy (test covers it).
• Did not run builds or tests (not approved; Go is not on this machine).

[amiddavid]

Round 2 answers. All fixes are in e734c87f.

• Tenant-key refusal also hit the host (blocking, both reviewers): fixed.
  • buildTenantConfig accepts the key again, so the local adapt apply and revert work.
  • tenant.Open now gets validateTenantDoc (config.Validate plus the host-only key check). A manager gets a 400 at save time.
  • proxy.NewTenantSource now gets buildHostedTenantConfig, which refuses the key and then calls buildTenantConfig.
  • Tests: TestTenantDocumentRefusesTerminalTools (save and build) and TestHostConfigWithTerminalToolsStillBuilds (the local rebuild).
  • Mutation: with the check disabled, the first test fails.
  • Not added: a test for a request on a stored tenant document with the key. It needs the full hosted setup. No stored document can have the key, because the save path now refuses it.
• docs/dashboard.md missing done (blocking): fixed. The ping_reason list has a line for it.
• TestPingReasonWords (non-blocking): fixed. It has a done row. It ran on the box and passes.

[amiddavid]

Reviewer	Kind	Started by	Context	Review	Round	Head	Verdict
Reviewer 1 (glm, user of @amiddavid)	GLM rits/zai-org/glm-5-3 via Claude Code 2.1.316	offload-review skill, @amiddavid's session	resumed	1 of 2	3 of 3	e734c87f37ef	approve

Verdict: approve

The round-2 finding is fixed at the right layer, and the fix holds on every path I traced. No
findings this round.

Round 2 findings: status

• My finding 1 (tenant-key refusal breaks single-tenant /api/adapt, misses the hosted save
  path): fixed. buildTenantConfig accepts the key again, so the local adapt apply and revert
  rebuild the host's own document without refusal — TestHostConfigWithTerminalToolsStillBuilds
  pins it. The restriction moved to the two places a TENANT document passes through:
  • Save time: tenant.Open gets validateTenantDoc (main.go:519), which is config.Validate
    plus the key check. tenant.Registry.Update (tenant/tenant.go:703-706) runs it on every
    stored-document write, so a manager who pastes a document with the key gets a 400 on the
    settings page, not a stored brick.
  • Build time: NewTenantSource gets buildHostedTenantConfig (main.go:526), which refuses the
    key and then calls buildTenantConfig — defense in depth for the per-request rebuild.
    I verified the dispatch that makes this layering sound: adaptWrite sends who.Local to
    adaptWriteLocal (the only BuildConfig/reloadLocal consumer), and who.Local is set only
    when reg == nil, that is single-tenant (proxy/adaptapply.go:46-49, 131). Hosted adapt writes
    go through reg.Update, which now validates. refuseTenantHostKeys returns nil on a parse
    error by design, and both its callers report the parse error themselves first.
• Reviewer 2, round-2 finding 1 (same blast-radius defect): fixed the same way. Checked
  myself; nothing to add.
• Reviewer 2, round-2 finding 2 (docs/dashboard.md missing done): fixed. The
  ping_reason list has the done line (docs/dashboard.md:148).
• Reviewer 2, round-2 non-blocking (TestPingReasonWords table): fixed. The done row is
  in (context-guru-plugin/ping_test.go:237).

Findings

None.

Non-blocking

• errTerminalToolsHostOnly (main.go:1123) is a plain error. If buildHostedTenantConfig ever
  fired at request time, statusOf would map it to a bare 401 (proxy/tenancy.go:417-420). It is
  unreachable now — the save path refuses the key first — so this is only a latent sharp edge.
  A proxy.StatusError would make the message reach the caller if that ever changes.

What I checked

• Last review: round 2, 4cb67cf, 2026-11-02T16:24:23Z.
• New commits read: 1 — e734c87f "fix(proxy): refuse keepalive_terminal_tools in a tenant
  document only (#857)".
• Read in full: 3 reviews, 3 comments, 0 inline comments (from the three files
  .review-in-reviews.txt, .review-in-comments.txt, .review-in-inline.txt; the inline file
  is empty).
• New items since my last review, and what I did about each:
  • Reviewer 2's round-2 review (16:24:47Z): read in full. Finding 1 matched my round-2 finding;
    finding 2 (docs/dashboard.md) and the test-table note I checked myself — both fixed in
    e734c87f.
  • Author's round-2 answers (16:50:04Z): read; every claim re-verified in the code (see
    "Round 2 findings: status"). The declined request-time test is fine: the save path now
    refuses the key, so no stored document can carry it.
  • Commit e734c87f: full diff read, plus the code around every hunk.
• The new layering on every path a document takes: single-tenant startup (loadConfig →
  proxy.Options.TerminalTools), single-tenant adapt (adaptWrite → who.Local →
  adaptWriteLocal → reloadLocal → BuildConfig = buildTenantConfig, key accepted),
  hosted save (registry.Update → validateTenantDoc), hosted per-request build
  (TenantSource.build → buildHostedTenantConfig), and the migration hook
  (tenant.Open's Validate, used by fixDeprecatedExtractLLMConfigs — no stored document can
  have the new key).
• The tests added in this commit: TestTenantDocumentRefusesTerminalTools now covers both the
  save-time check and the hosted build; TestHostConfigWithTerminalToolsStillBuilds covers the
  local rebuild. Their fixtures use preset: off, a real preset.
• The ping_reason documentation set is now complete: dash/cachestate.go's comment, model.ts
  long and short forms plus its test, both cg.py copies, docs/dashboard.md, and the plugin's
  TestPingReasonWords.
• Did not run builds or tests (not approved; Go is not on this machine).

[amiddavid]

Reviewer	Kind	Started by	Context	Review	Round	Head	Verdict
Reviewer 2 (glm, user of @amiddavid)	GLM rits/zai-org/glm-5-3 via Claude Code 2.1.316	offload-review skill, @amiddavid's session	resumed	2 of 2	3 of 3	e734c87f37ef	approve

Verdict: approve

Both round-2 findings are fixed at the right layer, with tests on both sides of the split. I found
no new defect in e734c87f.

Findings

None.

Round-2 findings: status

1. Tenant-key refusal broke /api/adapt on a single-tenant host (my finding 1; also reviewer 1, round 2, finding 1, single-tenant half): fixed.
   • buildTenantConfig accepts the key again (main.go:1110-1122, the refusal is gone), so the
     local adapt apply and revert rebuild the host's own document as before. BuildConfig at
     main.go:652 is still buildTenantConfig, and TestHostConfigWithTerminalToolsStillBuilds
     pins the case I asked for: a local rebuild of a host config with the key succeeds.
2. Hosted save path let the key through to request time (reviewer 1, round 2, finding 1, hosted half — I checked it myself): fixed.
   • tenant.Open now gets validateTenantDoc (main.go:519), which runs config.Validate plus
     refuseTenantHostKeys. Registry.Update validates every ConfigYAML patch with the
     injected function (tenant/tenant.go:703-706), and both tenant-doc write paths go through
     Update: the account editor, and the hosted /api/adapt write (proxy/adaptapply.go:146).
     A manager pasting a document with the key now gets a 400 naming the key at save time, not a
     401 at request time. NewTenantSource also gets buildHostedTenantConfig, which refuses
     the key at build — belt and braces for documents that predate the save check, which cannot
     exist since the key is new.
   • I agree with reviewer 1's round-2 analysis of the 401 shape; the fix removes the reachable
     path to it. A stored document with the key is now possible only through a direct database
     edit, which is not a supported path.
3. docs/dashboard.md missing done (my finding 2): fixed. The ping_reason list has a
   done line (dashboard.md:148) with wording that matches the code's meaning.
4. TestPingReasonWords missing done (my non-blocking): fixed. The table has a done row;
   the expected words match cg.py's "this agent has finished".

Non-blocking

• validateTenantDoc parses the document twice (config.Validate runs LoadBytes+Build, then
  refuseTenantHostKeys runs LoadBytes again). It runs only on a tenant-document save, so the
  cost is nothing. A note only.
• buildHostedTenantConfig's build-time refusal still surfaces as a 401 through statusOf if a
  stored document ever carries the key. Unreachable through every supported write path; noted so
  nobody mistakes it for a live hole.

What I checked

• Last review: round 2, 4cb67cfaf4d9dec3a642cbe05d2af8a242bd64ea, 2026-11-02T16:24:47Z.
• New commits read: 1 — e734c87f "fix(proxy): refuse keepalive_terminal_tools in a tenant
  document only (#857)".
• Read in full: 4 reviews, 3 comments, 0 inline comments (from .review-in-reviews.txt,
  .review-in-comments.txt, .review-in-inline.txt; the inline file is empty).
• New items since my last review, and what I did about each:
  • Commit e734c87f: full diff read, plus the code around every hunk.
  • Reviewer 1's round-2 review (16:24:23Z): read in full; its hosted-save half (the 401 bricking)
    checked in the code — Registry.Update's validation, the account editor's write path, and
    the hosted adapt write path all go through the injected Validate.
  • Author's round-2 answers (16:50:04Z): read; every claim re-verified in the code. The declined
    request-time test is fine: the save path refuses the key, so no stored document can carry it,
    and the build-side refusal is tested directly.
• The refusal's new blast radius: buildTenantConfig's callers (main.go:652 BuildConfig for
  the local adapt path, buildHostedTenantConfig for the tenant source), tenant.Open's
  Validate option and its two consumers (Registry.Update, the startup migration — the latter
  fail-open and only touches pre-#118 documents, which cannot carry the new key), and the hosted
  adapt write (adaptapply.go:139-146, stores through Update).
• The two new tests' premises: validateTenantDoc and buildHostedTenantConfig exist in main's
  package; buildTenantConfig accepting the key is the same function reloadLocal calls.
• The doc and test strings: done in dashboard.md matches ReasonDone; TestPingReasonWords'
  expected words match both cg.py copies.
• Did not run builds or tests (not approved; Go is not on this machine).