Skip to content

feat(keepalive): choose the ping count from what the thread waits for - #867

Merged
amiddavid merged 7 commits into
mainfrom
feat/keepalive-wait-policy
Nov 2, 2026
Merged

amiddavid merged 7 commits into
mainfrom
feat/keepalive-wait-policy

Conversation

@amiddavid

Copy link
Copy Markdown
Collaborator

Behavior change

This changes how much the proxy spends on pings for every tenant. A human developer must approve it before merge.

One switch turns 1, 3, 4 and 5 off: cache.keepalive_wait_plan: false. Change 2 is a default; set keepalive_min_prefix_tokens to bring back 80000.

  1. Ping count follows the wait.
    • Who: every tenant with keep-alive on (account, strategy).
    • Old: keepalive_max_pings (2) for every wait.
    • New: own tool = until the result comes, at most 8. Main end_turn + background = K+3. Main end_turn + person = K+1. Teammate, or subagent end_turn/SubagentHandback = K. done = 0.
    • Off: keepalive_wait_plan: false. Counts are keepalive_tool_wait_max_pings, keepalive_background_extra_pings, keepalive_human_extra_pings.
  2. Default minimum prefix 80000 -> 20000.
    • Who: every account that does not set keepalive_min_prefix_tokens. Small subagents now get pings.
    • Not changed: a value a user or a strategy set. The off switch does NOT bring back 80000.
  3. Strategy time windows apply only to a wait for a person.
    • Who: tenants with strategies that have windows.
    • Old: outside every window, the account config applied (often no pings).
    • New: for a non-person wait, the highest-priority active strategy that targets the tenant applies at any hour. No targeting strategy: the account config, as before. A person wait: unchanged.
    • Off: keepalive_wait_plan: false.
  4. No ping before the first tool call.
    • Who: threads whose responses never called a tool (side calls, plain chat).
    • Old: pinged if the prefix was big enough. New: 0 pings.
    • Off: keepalive_wait_plan: false.
  5. No sweep question during a tool wait.
    • Who: tenants with extract_llm_sweep.
    • Old: the question could replace a ping, or take the extra slot, in any span. New: not while the thread waits for its own tool. It is still asked on end_turn spans.
    • Off: keepalive_wait_plan: false.

Summary

Closes #858. The ping count is chosen when a turn ends, from the wait cause. Time windows gate only a wait for a person.

What changed

  • keepalive/waitplan.go (new): the plan. Policy.WaitPings, WindowsApply, IsSubagentThread, Keeper.WaitingFor.
  • keepalive/keeper.go: Record resolves the wait cause and sets MaxPings. New per-thread flag "a tool call was seen". pingable() refuses a thread with none. extraReadyLocked skips the sweep on a tool wait.
  • keepalive/substitute.go: no sweep question as a ping substitute on a tool wait.
  • keepalive/types.go: Policy.WaitPlan, ToolWaitMaxPings, BackgroundExtraPings, HumanExtraPings.
  • proxy/keepalive.go, proxy/keepalivestrategy.go: the strategy is resolved with the wait cause. New bestStrategy(..., ignoreWindows) uses the same priority rule. canPing also ignores windows (the cause is not known yet; a non-person wait is pinged at any hour).
  • proxy/keepaliveoverride.go: a session override keeps its exact count. An active learned campaign also keeps its count.
  • config/config.go, cmd/context-guru-proxy/main.go: four new keys. Default prefix 20000. The plan fields ride on a parked policy too, because a strategy can switch keep-alive on for it.
  • dash/keepalive.go + goldens: the replay gate follows the new default.
  • Docs: docs/how-to/cache-keepalive.md, docs/data-dependence.md.

Design choices

  • Windows (item 3): the strategy is resolved at record time, and record already has the response. It classifies the cause there and passes ignoreWindows. No new state.
  • Sweep gate (item 4): the entry stores its wait cause. On own_tool it gates both paths: the substitute for a planned ping, and the extra slot. The "never shorten the cache strategy" rule is untouched.
  • Dashboard (item 6): no code. Plans() reads the entry's MaxPings, so pings_left, next_ping_ms and held_until_ms show the new plan. The plugin reads those fields only. A no-tool thread shows no plan.
  • Scope: the plan changes the count only on the 5-minute Anthropic tier. A 1-hour thread, OpenAI, a session override and a campaign keep their own bounded schedule.

How it was tested

On the shared box, with -race:

  • go vet ./...: clean.
  • ./internal/waitcause ./keepalive ./components/... ./config ./tenant ./cmd/... ./context-guru-plugin: pass.
  • ./proxy/...: pass. ./dash/...: pass after the two replay goldens moved (see above).
  • New tests: one per table row (budget, 8 cap, result stops pings, base is the floor, 1-hour thread unchanged); no tool call yet; off switch gives 2 pings and pings a no-tool thread; windows (tool wait outside the window is pinged, person wait outside the window is not, subagent end_turn, priority and target); sweep (no question on own_tool, one on end_turn, no extra slot on own_tool); config YAML absent and present; defaults mirror config.
  • Mutation checks, each restored: removing the own_tool budget fails 6 tests. Removing the window bypass fails 2. Removing the sweep gate fails 2.

Hold and validation bounds

  • 8 pings fits. At 240-280 s the hold is (8+1) x 280 s = 42 min, under the 1 h maxOverrideHold.
  • The plan result is at most max(K, 11). 11 is maxOverridePings. At X < 300 s the longest hold is 60 min. Nothing holds a credential without a limit.
  • maxOverridePings and maxOverrideIdle are unchanged. The extra counts are validated at 0-11 in config.
  • A strategy's own MaxPings is the K the plan starts from. K+3 on a strategy with K=11 stays at 11.

Data dependence

Metadata only. Stop reason, tool names, open-task counts, thread-id shape (a:), token counts, time of day, and one bool per thread ("a tool call was seen").

  • Off / metadata only / everything: the same at every level. docs/data-dependence.md is updated. No "Decided by" row changes.

Open questions

🤖 Generated with Claude Code

Pick the ping count when a turn ends, from the wait cause (#858):
a tool wait is pinged until the result comes (at most 8), background
work gets K+3, a person K+1, a teammate or a main agent K, and a thread
with no tool call yet gets 0. Manager-strategy time windows apply only
to a wait for a person. The sweep question is not asked during a tool
wait. The default keepalive_min_prefix_tokens drops from 80000 to 20000.
One switch, keepalive_wait_plan: false, restores the fixed count, the
windows for every wait, the sweep questions and the no-tool-call pings.

Metadata only: stop reason, tool names, open-task counts, thread id
shape, token counts and time of day.

Assisted-By: Claude Opus 5.5 <noreply@anthropic.com>
Signed-off-by: DAVID AMID <DAVIDA@il.ibm.com>
Assisted-By: Claude Opus 5.5 <noreply@anthropic.com>
Signed-off-by: DAVID AMID <DAVIDA@il.ibm.com>
Assisted-By: Claude Opus 5.5 <noreply@anthropic.com>
Signed-off-by: DAVID AMID <DAVIDA@il.ibm.com>
@amiddavid

Copy link
Copy Markdown
Collaborator Author

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

The owner said, in words: "the behavorial changes are known" and "and approved". This covers every behavior change in #858:

  • Ping budget by wait cause:
    • a tool wait is pinged until the result comes back, at most 8 pings;
    • background work gets base + 3;
    • a person gets base + 1;
    • a teammate, and a subagent that waits for its main agent, get base.
  • No ping before a thread's first tool call.
  • Default keepalive_min_prefix_tokens: 80000 → 20000. Values set explicitly do not change.
  • Time windows apply only to a wait for a person. Every other wait is pinged at any hour.
  • No sweep question while a thread waits for its own tool.

How to turn it off: cache.keepalive_wait_plan: false restores today's behavior, except the 20k default.

This comment records the approval on the PR so that the integrator can read it. The release rule still asks the integrator to get the owner's yes in its own session.

@amiddavid amiddavid left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 9d31e0e0fefa blocking

Verdict: blocking

Findings

  1. proxy/keepalive.go:419 — canPing ignores strategy windows for every wait, so conversation content is kept for threads that never get a ping

    • What fails: account keep-alive off, wait plan on (the default when absent), an active strategy with a time window targets the tenant, and a response ends in a wait for a person outside that window. canPing resolves the strategy with the windows ignored and returns true. ExtractSweep.registerPing then keeps a full conversation copy — content, in memory. At record time the wait cause is "human", so WindowsApply says the windows count, the strategy does not match, the account policy is off, and the keeper holds nothing. The copy lives up to maxSweepPingAge = 30 minutes for a thread that never gets a ping. That breaks registerPing's own rule (components/offload/extract_sweep_ping.go:257-259: "a thread that will never get a ping must not pay them"). The same over-admission hits a thread with no tool call yet: noTool refuses it at record time, but canPing cannot see toolSeen.
    • Proof: proxy/modes.go:71-73 calls canPing on the request path, before the response is known. proxy/keepalive.go:419 passes tn.Cache.WaitPlan as ignoreWindows, so bestStrategy(..., true) matches a window-closed strategy and pol.On() is true. On the record path, proxy/keepalive.go:374-379 computes the cause, WindowsApply("human", false) returns true, so windows are respected, no strategy matches, and the account policy (off) refuses the entry.
    • Fix: in canPing, keep the windows (the behavior before #858). For an unknown wait, the pessimistic answer is the safe one: a tool wait outside the window then registers no copy and gets a plain ping. That loses only the sweep substitution, never correctness.
  2. keepalive/waitplan.go:97 and keepalive/waitplan.go:114 — the spliced-sniff-window branches have no test

    • What fails: WaitingFor's special case (no tool names plus a tool stop reason reads as "own tool") and calledATool's incomplete-list branch are new behavior. They decide the ping count (8 vs base+1) and the no-tool gate. No test covers them.
    • Proof: a grep over keepalive/*_test.go and proxy/*_test.go finds no test that calls WaitingFor, toolStop or calledATool, and no test that sets ToolUseIncomplete together with the plan. The existing ToolUseIncomplete tests (keepalive/terminal_test.go, proxy/terminal_usage_test.go) cover the #857 terminal rule only.
    • Fix: add two rows to TestWaitPlanPingBudgetPerWaitCause: (a) no names, stop reason "tool_use" → 8 pings; (b) ToolUseIncomplete true, no names → the thread counts as having called a tool, so it is pinged as before #858.

Non-blocking

  • docs/how-to/kv-cache-keepalive-budget.md:33 still shows keepalive_min_prefix_tokens: 80000 as the example. The shipped default is now 20000 (docs/how-to/cache-keepalive.md:63). A reader copies the old floor.
  • proxy/keepalivestrategy.go:70-74: the comment says the ping half and resolveHeadTTL "can never disagree about which strategy won". With ignoreWindows they now can: outside a window, a tool wait gets the strategy's ping policy while the head-TTL write tier falls back to the account's. Update the comment, and confirm the split is wanted.
  • keepalive/keeper.go:895: the comment says the dashboard reads the wait cause. I found no dashboard reader; EntryView carries no wait field. Only the sweep gate reads it.
  • keepalive/waitplan.go:97: the special case fires only when the classifier said "human". A tool stop with no names and open background tasks reads "background" (base+3) instead of "own tool" (8). Both plans ping more than the base and ignore the windows, so the error is small.

What I checked

  • The full diff b4dfbfe5..HEAD, all 22 files, and the code around each change: keeper.go (Record, pingable, Plans, Forget, Shutdown, extraReadyLocked), substitute.go (fireSweepSubstitute), waitplan.go, types.go (ForOneHour keeps WaitPlan), proxy/keepalive.go (record, canPing), proxy/keepalivestrategy.go (bestStrategy, applyStrategyWindows), proxy/keepaliveoverride.go, cmd/context-guru-proxy/main.go (cachePolicy, parked block), config (Resolved, validate, the 11 cap), dash (kaGateMinPrefix, the golden endpoints), and internal/waitcause (the classifier).
  • The hold bound claims: maxOverridePings = 11 matches MaxKeepAliveWaitPings; (11+1) x 299 s stays under 60 minutes; WaitPings never goes above the base.
  • toolSeen is bounded and reset exactly like turns (MaxTurnKeys reset, Forget, Shutdown), and is written under k.mu.
  • The golden changes follow from the fixture: the fixture's prefixes run 20000..~61k, so the 80k gate admitted no span (pings 0) and the 20k gate admits all of them. The ka_recommend dollar change follows from the higher ping cost.
  • The campaign and override paths set WaitPlan = false, so their counts stay their own.
  • Fail-open: the plan is pure arithmetic over metadata; it cannot panic or block a request. The sweep gate falls back to the ordinary ping.
  • I did not read the PR description (round 1 rule). The default changes — the 20000 floor and the plan on when the key is absent — must be named at the top of the PR per RELEASING.md.
  • No live tests were run (not approved).

@amiddavid amiddavid left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 9d31e0e0fefa blocking

Verdict: blocking

Findings

  1. dash/keepalive.go:1283 — the replay and the recommendation still model a fixed ping count, so after this PR they report a policy nobody runs
    • What fails: a manager opens the keep-alive panel on a tenant whose threads are mostly tool waits. The calculator's current rung (K=2) shows 2 pings per span and 860 s of coverage. The shipped policy now sends up to 8 pings (about 39 min of coverage) on a tool wait, K+3 on a background wait, and 0 on a thread with no tool call yet. The pings, ping cost, net, and the recommendation's lo_usd / hi_usd (bootstrapped at recMaxPings = 2, dash/keepalive.go:1736) are wrong for the policy the user actually runs. The ka-rec-policy panel copy still calls (240 s, K=2) "the shipped default".
    • Proof: the request path now chooses the count per wait (keepalive/keeper.go:908-909 pol.MaxPings = pol.WaitPings(...), and keeper.go:331 noTool refuses a thread with no tool call). The dash side still charges one fixed K per span: pingSpan.pings returns maxPings or PingsPerSpan(gap, idle, maxPings) (dash/keepalive.go:1283-1288), and pingSpans selects spans by turn >= 1 AND prefix >= ? only (dash/keepalive.go:1312-1326) — no wait cause, no tool-call filter. This is the same drift class the PR's own test names: "A drift here is a calculator modelling a policy nobody runs, which is the whole of F5" (dash/keepalivetab_test.go:870-872). The PR moved the gate to 20k but left the count model at fixed K. The data to model the plan is already in the requests table (waiting_for, wait_tool, open_background, open_teammate).
    • Fix: extend pingSpans to read the row's wait cause and tool history, and apply the same plan (Policy.WaitPings, the no-tool rule) per span; or state on the panel that the ladder and the range model the plan-off policy, and drop the current rung and the "shipped default" wording while the plan is on by default.

Non-blocking

  • Stale comments now say the floor is 80k where it is 20k: dash/keepalive.go:553 ("the shipped floor is now 80k"), :573 ("the shipped floor is now 80,000"), :1309-1311 ("the shipped floor is now 80k ... the panel's copy names 80k"), and dash/keepalivetab_test.go:825, :830 ("the 80k floor"). The PR changed the const but not these.
  • canPing ignores time windows for every wait when the account has the plan on, including a wait for a person (proxy/keepalive.go:419). A person wait outside a window then passes canPing but is not held at record time, so components keep sweep candidates for threads that never ping. The PR names this in its open questions. Bounded by the store TTL.
  • ignoreWindows is decided from the ACCOUNT's WaitPlan before the strategy, override and campaign layers run (proxy/keepalive.go:374-380). A window-closed strategy can therefore match on a non-person wait and displace an active campaign (applied != "" skips the campaign block, proxy/keepalive.go:446-450), or contribute its MaxUSDPerPing / PredictorID under a session override. Bounded, and consistent with "the strategy applies at any hour", but the campaign displacement is not named in the PR.

What I checked

  • Read CLAUDE.md, docs/design.md, issue #858, the PR body, and the full diff of all three commits.
  • Traced the plan end to end: config keys and validation (config/config.go), cachePolicy for parked and enabled blocks (cmd/context-guru-proxy/main.go), Policy.WaitPings / WindowsApply / WaitingFor / calledATool (keepalive/waitplan.go), Record and pingable (keepalive/keeper.go), the sweep gates (keepalive/substitute.go, extraReadyLocked), the strategy resolution with ignored windows (proxy/keepalivestrategy.go, bestStrategy, Matches vs Active && Covers), the override and campaign exclusions, and canPing.
  • Checked the classifier inputs against internal/waitcause: the askHumanTools and handbackTools lists, the terminal-tool path, and the OpenTasks wiring (h.waits.OpenCounts, thread-safe, fail-open).
  • Checked the bounds: MaxWaitPings = 11 = maxOverridePings; the hold math in the docs ((8+1) x 280 s = 42 min, (11+1) x 299 s < 60 min) is right; 1-hour threads and OpenAI are excluded from the count change; ForOneHour keeps its cap.
  • Checked state bounding: toolSeen is reset, kept and deleted exactly where turns and tier1h are (Record, MaxTurnKeys reset, Shutdown, Forget).
  • Checked the tests: every row of the issue's table has a test, plus the 8-cap, the off switch, the 1-hour thread, the sweep gates, the config YAML, and the defaults-mirror test. Test helpers referenced by the new files exist.
  • Checked the dash golden changes (gate 20k, pings 0 to 281/433/585/737) against pingSpans and the fixtures; the small-prefix fixture (1000 tokens) stays under the new gate.
  • Compliance: the PR names the behavior changes at the top with the off switch, and the data-dependence row is updated. The fail-open rules and the "keep-alive never shortens the cache strategy" rule are untouched: the plan only raises or lowers a ping count, never the hold limit, and the sweep suppression removes a substitute, so the plain ping still goes out.

- canPing keeps the strategy windows and refuses a thread with no tool
  call yet, so no sweep copy is kept for a thread that is never pinged.
- A tool stop with no names always gets the tool budget, also with open
  background tasks. Tests cover the spliced-window rows.
- A strategy matched only because its window was ignored never displaces
  an active campaign.
- The dash calculator and recommendation model a fixed count. They now
  say so (plan: fixed_k), and no rung is marked current. Follow-up:
  model the plan per span.
- Fix stale 80k comments and the 80000 doc example. Update the
  bestStrategyFor comment.

Assisted-By: Claude Opus 5.5 <noreply@anthropic.com>
Signed-off-by: DAVID AMID <DAVIDA@il.ibm.com>
Assisted-By: Claude Opus 5.5 <noreply@anthropic.com>
Signed-off-by: DAVID AMID <DAVIDA@il.ibm.com>
@amiddavid

Copy link
Copy Markdown
Collaborator Author

Round 1 reply. New head: 7c0d4843. Fixes are in 84957948 and 7c0d4843.

Reviewer 1

  1. canPing ignores windows: fixed (84957948).
    • canPing now keeps the windows, as before keepalive: choose the ping count from what the thread waits for (measured on two sources) #858. A tool wait outside a window registers no sweep copy and still gets its plain ping. It loses only the sweep substitute there.
    • canPing also refuses a thread with no tool call yet. toolSeen is one map read under the keeper lock, so it is cheap. New Keeper.ToolSeen. canPing now takes the thread id.
    • Test: TestCanPingKeepsWindowsAndRefusesAThreadWithNoToolCall covers a closed window, no tool call yet, a tool call seen, and the plan off.
    • Mutation: using the ignore-windows resolve fails it. Removing the ToolSeen check fails it. (The first version of the test did not catch the window mutation, because the no-tool guard masked it. Fixed in 7c0d4843, then re-checked.)
  2. Missing rows: fixed (84957948).
    • Rows added to TestWaitPlanPingBudgetPerWaitCause: tool stop with no names = 8, and tool stop with no names plus open background tasks = 8.
    • New TestWaitPlanIncompleteNamesCountAsAToolCall: an incomplete name list counts as a tool call and the thread is pinged.
    • Mutations: disabling the tool-stop branch fails the budget test. Dropping incomplete from calledATool fails the new test.
  3. Non-blocking, all fixed:
    • kv-cache-keepalive-budget.md example is now 20000.
    • bestStrategyFor comment now says the ping half and resolveHeadTTL can differ for a non-person wait. The split stays: the head-TTL write tier is a cost decision taken at write time, and a tool wait does not change it.
    • keeper.go comment no longer says the dashboard reads the wait cause.
    • A tool stop with no names now gets the tool budget whatever the classifier said. It excludes only done and retry.

Reviewer 2

  1. Dashboard models a fixed K: fallback chosen (84957948).
    • Why not the preferred fix: the plan needs the wait cause and tool history for each span. The recommendation bootstrap also needs the same for each expiry's previous request. It also needs a per-span WaitPings and a changed coverage test. That is too large for this PR, and I cannot verify it against real data here.
    • What changed:
      • The ladder and the recommendation are labelled as the fixed-K (plan-off) model. The calc response carries plan: "fixed_k".
      • No rung is marked current. The (current) text in the UI can no longer appear.
      • The UI copy says every rung is a fixed count and the live policy picks its count per wait.
      • The ka-rec-policy text no longer says "shipped default". The "PINGS" note says "a fixed-count policy".
    • Golden diff: only ka_calc.json, + "plan": "fixed_k" and - "current": true. ka_recommend and the other goldens are unchanged in this round.
    • Follow-up issue: I will open it only when you say so. It will cover modelling the plan per span in pingSpans, the calculator and the bootstrap.
  2. Stale 80k comments: fixed. dash/keepalive.go (3 places) and dash/keepalivetab_test.go (2 places).
  3. canPing and windows: fixed, see R1-1.
  4. Campaign displacement: fixed (84957948). A strategy that matches only because its window was ignored no longer takes the place of an active campaign. With a campaign and a closed window, the campaign wins, as before keepalive: choose the ping count from what the thread waits for (measured on two sources) #858. Test: TestClosedWindowStrategyDoesNotDisplaceACampaign. Mutation: removing the campaign check fails it. A session override still layers on top as before.

Other notes

  • My git pull merged origin/main into the branch (1dd56fe6). No force-push. All runs below include it.
  • Tests on the shared box with -race, at 7c0d4843:
    • go vet ./...: clean.
    • ./internal/waitcause ./keepalive ./components/... ./config ./tenant ./cmd/... ./context-guru-plugin: pass.
    • ./proxy/...: pass.
    • ./dash/..., the full package: pass.

@amiddavid amiddavid left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 7c0d4843ed0d approve

Verdict: approve

Findings

None. Both round-1 findings are fixed, and the fix commits introduce no new defect I can show.

Earlier findings

Mine (round 1):

  1. canPing ignored strategy windows (finding 1): fixed in 84957948. canPing now resolves the policy with the windows respected (resolvePolicy, proxy/keepalive.go:425), which is the pessimistic answer I asked for. It also refuses a thread with no tool call yet, through the new Keeper.ToolSeen (keepalive/waitplan.go:126-133), so no conversation copy is kept for a thread the plan will not ping. Traces:
    • person wait, window shut, account off: no strategy matches, pol.On() false, canPing false — no copy;
    • no tool call yet: pol.WaitPlan && !ToolSeen returns false — no copy;
    • tool wait, window shut: canPing false, record-time ignores the window, the plain pings still go out — only the sweep substitute is lost;
    • plan off or a campaign policy (WaitPlan = false): no ToolSeen check, and record-time guard is false too, so the two agree.
      Test TestCanPingKeepsWindowsAndRefusesAThreadWithNoToolCall covers all four, and 7c0d4843 removed the masking that hid the window mutation. The only caller (proxy/modes.go:72) passes the thread id.
  2. Missing spliced-window tests (finding 2): fixed in 84957948. Two rows in TestWaitPlanPingBudgetPerWaitCause (tool stop with no names → 8, also with open background tasks → 8) and TestWaitPlanIncompleteNamesCountAsAToolCall (an incomplete name list counts as a tool call). The special case in WaitingFor now returns "own tool" for any tool stop with no names, unless the classifier said "done" or "retry" — that also fixes my round-1 non-blocking note about open background tasks.

My non-blocking notes: all fixed (the budget-doc example is 20000, the bestStrategyFor comment names the split and says it is wanted, the keeper comment no longer names the dashboard).

Reviewer 2 (round 1), checked myself:

  1. Dash models a fixed K: fixed by the fallback the finding itself offered. The calc response carries plan: "fixed_k", no rung is marked current (Current: false, dash/keepalive.go:1471-1472), the UI copy says the live policy picks its count per wait, and the "shipped default" wording is gone (dash/ui/app.js). Tests pin both (TestCalculatorGoldenAgainstTheProductionBands). The golden diff is exactly + "plan" and - "current". The per-span modelling is a declared follow-up.
  2. Stale 80k comments: fixed — dash/keepalive.go (three places) and dash/keepalivetab_test.go (two places).
  3. canPing and windows: fixed, same as my finding 1.
  4. Campaign displacement: fixed in 84957948. A strategy that matches only because its window was ignored no longer displaces an active campaign (proxy/keepalive.go:449-455: the ignore pass runs only when no campaign is active). TestClosedWindowStrategyDoesNotDisplaceACampaign is not vacuous: campaignStrategyID("") returns "campaign:", which is neither "s" nor "".

Non-blocking

  • The recommendation's JSON (/api/keepalive/recommend) carries no plan field; only the calc does. The label for the recommendation lives in UI copy. A programmatic reader of that endpoint still sees no hint that the range models the plan-off policy.
  • proxy/keepalive.go:449-455: for a non-person wait, the windows-respected pass runs first and any in-window match wins, even over a higher-priority strategy whose window is shut (the ignore pass runs only when the respected pass found nothing). The docs say "the highest-priority active strategy that targets the tenant applies at any hour". The code is the more conservative choice, but the two disagree in that corner. Name the choice in the comment, or run the ignore pass first.

What I checked

  • Last review: round 1, 9d31e0e, 2026-11-02T20:48:00Z.
  • New commits read: 4 — 1dd56fe6 (merge origin/main), c66c392f (chore(plugin): bump the plugins for v0.6.5, plugin.json version strings only), 84957948 (fix(keepalive): address round-1 review of the wait plan), 7c0d4843 (test(proxy): make the canPing window test independent of the tool guard).
  • Read in full: 2 reviews, 2 comments, 0 inline comments (from .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 (20:48:11Z): read in full, checked each finding myself (see above).
    • The owner's approval comment (20:29:00Z): read. It records the owner's yes for every behavior change in #858 and the off switch, which closes my round-1 note about the RELEASING.md rule.
    • The author's round-1 reply (21:24:15Z): read. I verified each claim in the code, not only the reply: the new canPing body, Keeper.ToolSeen (lock, keying by Key.id(), same key Record writes), the campaign-displacement guard in resolvePolicyWindows, the broadened tool-stop branch in WaitingFor (with done/retry unreachable-but-safe exclusions), the new and changed tests, and the dash labelling. The reply's test-run claims (go vet, -race, all packages pass at 7c0d4843) are the author's; I did not run them.
  • The merge brought only plugin.json version bumps; nothing in this PR reads them.
  • Fail-open and the hold bounds are unchanged by the fix commits; canPing is a gate on keeping a copy, so its new refusals cannot drop a ping the keeper would send — they only drop the sweep substitute, which falls back to the plain ping.
  • No live tests were run (not approved).

@amiddavid amiddavid left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 7c0d4843ed0d approve

Verdict: approve

Findings

None. Every round-1 finding is fixed, and the fix commits introduce no new defect I can find.

Status of the round-1 findings

My finding 1 (dash models a fixed K): fixed, by the fallback option. The author did not model the plan per span, and gave the reason: it needs the wait cause and tool history per span and per expiry's previous request, too large to verify here. The reason is right, and the fallback was one of the two fixes I proposed. I checked each piece:

  • KeepAliveCalc carries plan: "fixed_k" (dash/keepalive.go), and the golden shows it.
  • No rung is marked current (Current: false), and the test now fails if any rung is.
  • The UI copy says "every rung is a fixed count" and "no rung is marked as the current one". The ka-rec-policy text no longer says "shipped default"; it names the plan-off setting. "Shipped default" is gone from the keep-alive copy, and the (current) marker can no longer render (the only .current reads left in app.js are the browser list and the password field).
  • The labelling is self-consistent: with the plan off, the fixed-K replay IS the policy it names, no-tool threads included.

My non-blocking items: all fixed. The stale 80k comments (three in dash/keepalive.go, two in the test) now say 20k. canPing keeps the windows and refuses a no-tool thread. A window-ignored strategy no longer displaces an active campaign.

Reviewer 1's findings: fixed. I verified both myself.

  • canPing now resolves the policy with the windows respected, then refuses a thread with no tool call yet through the new Keeper.ToolSeen (one map read under the keeper lock, keyed like Record's). The guard is off when the plan is off, and off for an override session, whose count is exact — both match the record path. Only extract_sweep_ping gates on CanKeepAlive; the summarizer never did, so the loss outside a window is the sweep substitute only, as the reply says. The test covers a closed window, no tool call yet, a tool call seen, and the plan off; 7c0d4843 fixed the masking the author found in their own first version.
  • The spliced-window rows are tested: a tool stop with no names gets 8, also with open background tasks (the fix-up in WaitingFor now fires for any tool stop, not only when the classifier said "human"), and an incomplete name list counts as a tool call. The done / retry exclusions are unreachable with an empty name list, which is harmless.

The campaign fix is correct. resolvePolicyWindows first resolves with the windows respected; it retries with them ignored only when nothing matched, the caller asked to ignore them, and no campaign is active. So: a window-matching strategy still wins (old precedence), a campaign beats a window-closed strategy (pre-#858 behavior), and without a campaign the strategy applies at any hour (the feature). A shadow campaign does not block the retry. The override still layers on top.

Non-blocking

  • KeepAliveCalc's last parameter is now ignored (_ int), while the API handler still reads k from the query and the UI still sends it. The K control still drives the behaviour and live panels, so it is not dead — but the calc endpoint accepts a value it no longer uses. Cosmetic.
  • Please open the follow-up issue for modelling the plan per span (pingSpans, the calculator, the bootstrap). You offered to open it on my word: consider this the word.

What I checked

  • Last review: round 1, 9d31e0e0fefacd59544377b636c08deee1c30b17, 2026-11-02T20:48:11Z.
  • New commits read: 3 — 1dd56fe6 (merge of main; only the two plugin.json version bumps from c66c392f), 84957948 (the fixes), 7c0d4843 (the canPing test amendment, 3 lines).
  • Read in full: 2 reviews, 2 comments, 0 inline comments (from the three saved files). The owner's behavior-change approval comment (2026-11-02T20:29Z) I read now for the first time; it covers every default change this PR makes, with the off switch.
  • New items since my last review: the author's round-1 reply comment (read in full, each claim checked against the code), and the three commits above (full diff 9d31e0e0..HEAD, plus each changed file around its change).
  • Checked the new canPing against its caller (proxy/modes.go now passes the thread id) and against the record path: the no-tool guard agrees with Record's noTool for the account, strategy, override and campaign paths.
  • Checked the widened tool-stop fix-up in WaitingFor against the classifier (internal/waitcause): with no names, done and retry cannot occur, so the guard changes nothing; the new classification (own tool) is the right reading of a tool stop.
  • Checked the dash changes end to end: Plan field, Current: false, the golden diff (+ "plan", - "current"), and every changed UI string in context.
  • Checked that only extract_sweep_ping consumes CanKeepAlive, so the canPing change cannot drop the summary substitute.
  • No live tests were run (not approved). The author reports go vet and the full test packages green at 7c0d4843 on the shared box with -race.

@amiddavid

Copy link
Copy Markdown
Collaborator Author

Follow-up for the per-span plan model: #868. The calculator and the recommendation keep the fixed-K (plan off) label until #868 lands.

Brings in #861 (tooldefer). Clean merge, no hand edits.

Assisted-By: Claude Opus 5.5 <noreply@anthropic.com>
@amiddavid

Copy link
Copy Markdown
Collaborator Author

Integrator check of the head move (RELEASING.md, "When the head moves after an approval").

  • The approvals name 7c0d4843. The head b4496aad is a merge of 7c0d4843 and origin/main 99b7c921 (feat(tooldefer): defer unused tools in a new opt-in ultrahigh preset (#485) #861).
  • git show --remerge-diff b4496aad is empty, so the merge has no hand edits.
  • The PR's own change is the same at both heads: git patch-id of merge-base..7c0d4843 equals that of 99b7c921..b4496aad.
  • 4 files changed on both sides (cmd/context-guru-proxy/main.go, config/config.go, config/config_test.go, docs/data-dependence.md). Git merged them with no conflict, and the patch check above shows the result is the approved change.
  • CI is green at b4496aad.

So only the base moved, and both approvals still count.

@amiddavid
amiddavid merged commit 38e4693 into main Nov 2, 2026
7 checks passed
@github-project-automation github-project-automation Bot moved this from New/ToDo to Done in Rossoctl Issue Prioritization Nov 2, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

keepalive: choose the ping count from what the thread waits for (measured on two sources)

1 participant