[amiddavid]

Behavior change

This changes what the dashboard shows. It does not change what the proxy does.

• Who it affects: managers and users who read the keep-alive panel (calculator and recommendation).
• Old behavior: fixed-K figures, labelled "plan off". No row was marked current.
• New behavior: the first row is the live plan (the proxy's per-wait rule), marked current. The fixed-K rows stay as a comparison. The recommendation names its model and shows the fixed-K range beside it.
• How to turn it off: set keepalive_wait_plan: false on the account. The panel then models fixed K, as in feat(keepalive): choose the ping count from what the thread waits for #867, with the row of the account's K marked current.

Summary

• The calculator now replays the policy that runs.
• The dashboard asks the keeper's own rule for each count. It does not copy the rule.
• A new test runs the real keeper and the dashboard on the same cases and checks equal counts.

What changed

• keepalive/waitplan.go
  • New Policy.SpanPings(SpanFacts). It is the keeper's decision for a stored row: wait cause, no-tool rule, done, retry, OpenAI, 1-hour tier. It calls WaitPings.
  • The tool-stop rule is now one function (toolStopWait) used by both the keeper and the replay.
  • Why here: dash already imports keepalive's neighbours, and keepalive does not import dash. No new package was needed, and dash still does not import proxy.
• dash/keepalive.go
  • pingSpans and a new expiries read carry the stored facts of the request that opens each gap: stop_reason, waiting_for, thread_id, provider, cache_ttl, and whether the thread called a tool before.
  • Each read is one SQL statement. The tool history is one extra window (MAX(...) OVER by tenant, session, thread).
  • The calculator has a wait_plan rung (current) and the four fixed_k rungs. Each rung has a plan field. The response keeps a top-level plan for the account.
  • The recommendation has plan, and fixed_k (a range) when the plan is on. recMaxPings is still the base, and still means "plan off" for the fixed-K model.
  • A span with no ping count (no tool yet, done) gets no reach. A fixed-K expiry is counted as before.
• dash/keepaliveapi.go, dash/api.go, proxy/keepalive.go, cmd/context-guru-proxy/main.go
  • New hook SetWaitPolicy. The proxy answers with the account's keepalive_wait_plan and its three counts. Without a hook, the shipped plan is on.
• dash/ui/app.js: row names ("Live plan, base 2 (current)", "Fixed 2"), new copy, and the fixed-K range under the recommendation.
• docs/data-dependence.md, docs/how-to/dashboard-guide.md: updated.

Golden diffs

Only two files changed. Both are intended.

• ka_calc.json
  • plan is now wait_plan.
  • A new first row (current: true, plan: wait_plan). On this fixture it has the same counts as fixed K=2 (112 expiries, 433 pings): the fixture rows carry no wait cause.
  • Each fixed row gains "plan": "fixed_k".
• ka_recommend.json: new plan: wait_plan and fixed_k (the same range here, for the same reason).

I also regenerated the other goldens once. tools*.json and toolfilter.json changed only in float noise in the last digits, from the box. I did not commit those.

How it was tested

All on the shared box, with -race:

• go vet ./...: clean. gofmt -l: clean.
• ./keepalive/...: ok.
• ./dash/...: ok (562 s).
• ./proxy/... ./cmd/...: ok.

New tests (dash/keepaliveplan_test.go):

• TestDashboardReplayMatchesTheKeeper: 14 cases at base 2 and base 10. It records the real responses in a real Keeper, stores the rows the proxy would write, and compares ping counts.
• TestDashboardReplayKnownGapSplicedToolStop: pins the one known difference (see below).
• Calculator tests: plan rung is current; plan off gives fixed K only; recommendation states its model.
• TestPlanReplayStaysNearTheFixedKCost: relative cost bound.

Cost. On 20,000 rows, without -race: fixed-K reads 177 ms, plan reads 582 ms (about 3.3x). The routes are cached (cachedRoute). The existing TestOverviewStaysFastOnALargeWindow passes.

Mutation checks (both restored):

• The dashboard ignores the wait cause (fixed K): the drift test fails in 8 cases.
• The dashboard drops the no-tool rule: the drift test fails in 4 cases.

Data dependence

Metadata only. It reads columns the proxy already writes: stop_reason, waiting_for, thread_id, provider, cache_ttl, and request times. No text. Storage level:

• Off (no dashboard): nothing to show.
• Metadata only: works in full.
• Everything: same as metadata only.

docs/data-dependence.md has the dashboard row updated. The data dependence does not change in either direction.

Open questions

• Known gap (pinned in a test): a stored row cannot tell "a tool stop with no tool name" from "an ask-a-person tool". Both read human. The keeper plans 8 pings for the first, the replay plans base+1. This needs a spliced sniff window and is rare. Fix, if wanted: store the case in a new column. That is a schema change, so I did not do it here.
• Interleaved threads. The wait comes from the request that opens the gap, on its own thread. If a main agent and a subagent interleave, the keeper holds one entry per thread. The replay uses the last request before the gap. The issue's open question ("same row, or a join?") is answered: same row, no join, but this approximation holds.
• Old rows with no waiting_for and no stop_reason use the base count (nothing to replay from).
• The 1-hour tier uses the base count, as the keeper does.

Closes #868

🤖 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	fd354b768116	blocking

Verdict: blocking

Findings

1. keepalive/waitplan.go:168 — the plan replay charges the base count for 1-hour-tier spans; the live plan sends the 1-hour schedule

   • What fails: an account with the plan on, base K=2, X=240 s, and a thread on the 1-hour tier (cache_ttl = 'ephemeral_1h') that goes idle for 2 hours at session end. The proxy sends 1 ping at 59 minutes (ForOneHour: MaxPings1h, default 1; Idle1h, default 59 min). The calculator's "Live plan, base 2 (current)" rung counts 2 pings for that span, charged at the 240 s cadence. The PINGS column and the ping cost are wrong by 2x at the defaults. They are wrong in the other direction when an account tunes MaxPings1h above its base.
   • Proof: keepalive/keeper.go:891-896 swaps in pol.ForOneHour() for a 1-hour thread; keepalive/types.go:191-207 sets MaxPings = MaxPings1h (default 1, internal/modelinfo/cachefrac.go:27) and Idle = 59 min; docs/design.md:251 names the live schedule "1 ping at 59 minutes on the 1-hour tier". waitplan.go:168-170 returns p.MaxPings (the base) for s.OneHour, and dash/keepalive.go:1294 (planPings) charges that count at the X-second cadence. The PR body says "The 1-hour tier uses the base count, as the keeper does" — the keeper does not. TestDashboardReplayMatchesTheKeeper has no 1-hour case, so nothing pins this.
   • A second divergence in the same branch: OneHour reads the tier the request asked for (the stored cache_ttl), while the keeper reads the tier the provider billed (ResolveTier, keeper.go:267-275: CacheWrite1h >= CacheWrite, sticky per thread). A mixed-tier row (asked 1 h, billed partly 5 m) runs the per-wait plan in the proxy — a tool wait gets up to 8 — but gets the base count in the replay.
   • Fix: model the 1-hour tier with its own schedule (Idle1h / MaxPings1h, defaults 59 min / 1), or keep the simplification and state it in the panel copy and the code comment, and pin the gap in a test as the spliced-stop gap is pinned. Drop the "as the keeper does" claim either way.

2. keepalive/waitplan.go:165 — a stored "retry" wait gets 0 pings, but the keeper pings after a failed call

   • What fails: a non-2xx response that still reported usage with cache_read over the 20k floor, followed by a long idle gap. The proxy keeps the account policy on for that request (proxy/keepalive.go:349 — tracked gates only the campaign and the manual hold), the keeper tracks it (keeper.go:865 refuses only when usageOK and both cache counts are 0) and plans its base count. The keeper never classifies a wait as "retry" itself: its WaitingFor (waitplan.go:93) passes failed = false, so the wait reads unknown or human. The stored row says waiting_for = 'retry' (proxy/dashcapture.go:512,535), so the replay charges 0 pings for the span. The plan rung undercounts its ping cost — the flattering direction.
   • Proof: the trace above; the doc at waitplan.go:155 says "the keeper holds nothing for them", and the code does not do that. The case is rare (it needs a failed call with parsed usage and a long idle after it), so this is minor next to finding 1.
   • Fix: drop the WaitingRetry branch and let a stored "retry" read as its classifier fallback, or make the claim true in the keeper and pin that in a test. Fix the comment either way.

Non-blocking

• dash/ui/app.js:9580 — the plan rung and the fixed rung with the same K share one data-testid (ka-ladder-k2 appears twice). A selector now matches two rows.
• dash/ui/app.js kaRungCoverage hardcodes "a tool up to 8" and "a person base+1". Wrong copy for an account that tuned keepalive_tool_wait_max_pings or keepalive_human_extra_pings.
• The OpenAI cadence (28 minutes) and the 1-hour reach are still judged at the 5-minute policy, as before this PR. Only the COUNT is new. The fixed rungs carry the same simplification, so the comparison stays fair.
• proxy/keepalive.go:553 DashWaitPolicy falls back to plan-on (DefaultWaitPolicy) when the tenant lookup fails. The panel then marks the plan rung current for an account that may run fixed-K.
• The seen window counts a tool stop or a stored own_tool. The keeper also counts an incomplete name list (calledATool). Same family as the pinned spliced-stop gap.

What I checked

• The full diff of all 14 files, and the code around each change: keepalive/waitplan.go, keepalive/keeper.go (Record, pingable, ForOneHour, ResolveTier, Plans), keepalive/types.go, internal/waitcause (the classifier, retry and done), dash/keepalive.go (pingSpans, the new expiries, the calc and the recommendation), dash/keepaliveapi.go, dash/api.go, proxy/keepalive.go, proxy/dashcapture.go (what waiting_for stores), cmd/context-guru-proxy/main.go, dash/ui/app.js, both docs, both goldens, and the three test files.
• That the replay's SQL returns the same rows as the fixed-K paths (factsCTE vs addressableCTE vs the old pingSpans): same filter, same gate, pings excluded.
• That storedWait reconstructs the keeper's tool-stop rule from the stored cause, and that SpanPings matches the keeper for the 14 drift-test cases plus the OpenAI, old-row and no-tool paths.
• The PR body: behavior-change statement, data-dependence statement and docs/data-dependence.md row, golden diffs. Both are present and accurate for the columns read.
• No tests were run (live tests not approved); all claims above are from reading the code.

[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	fd354b768116	blocking

Verdict: blocking

Findings

1. proxy/keepalive.go:553 — DashWaitPolicy reads only the account's own config, so the calculator marks the per-wait plan "current" for accounts whose live policy has the plan off.
   • What fails: A tenant with keepalive_wait_plan on (the default) and an ACTIVE learned campaign runs a fixed count. The request path forces the plan off for it: pol.WaitPlan = false at proxy/keepalive.go:460, because the campaign owns the count. A manager strategy with HeadTTL1h has the same effect through a second door: its Idle band allows up to 3590 s (keepalive/types.go:188), and the keeper applies per-wait counts only below 300 s of Idle (keepalive/keeper.go:903). For such an account, /api/keepalive/calc answers plan: "wait_plan" and marks the plan rung current; /api/keepalive/recommend models the plan interval. The user reads "Live plan, base 2 (current)" while the proxy sends one fixed count for every wait.
   • Proof: cmd/context-guru-proxy/main.go:745 says the hook exists "so the calculator replays the policy that runs (#868)". The hook returns tn.Cache fields only (proxy/keepalive.go:567-569). It never consults the campaign runtime or bestStrategy, both of which the request path resolves in resolvePolicyWindows (proxy/keepalive.go:440-462). An armed per-session override also forces the plan off (proxy/keepaliveoverride.go:189); that one is per session, so an account-wide figure cannot carry it, but the campaign and the one-hour strategy are per account and knowable at read time.
   • Fix: Resolve the effective policy in DashWaitPolicy the way record does — the tenant's best strategy and active campaign — and report WaitPlan: false when either owns the count. Or state the limit on the panel: "your account setting, before campaigns and strategies".

Non-blocking

• The plan rung ignores the keeper's Idle < 300 s gate (keepalive/keeper.go:903). The UI clamps X to 60–290 (dash/ui/app.js:9502-9508), so only a direct API call with x >= 300 hits it. There the plan rung shows per-wait counts the live policy would not apply at that idle.
• dash/ui/app.js:9525 and the ladder hint hardcode "up to 8", "base+1", "base plus 3". An account that tuned ToolWaitMaxPings and the extras — the replay itself uses its values — reads wrong bounds on the page.
• Two ladder rows share data-testid="ka-ladder-k2" when the plan rung's base equals a fixed rung's K (both have max_pings: 2). No test uses that id today.
• dash/keepalive.go:1357 still says "this package does not read tenant configuration". Half stale now: the reads receive the tenant's plan settings through the host hook. Only the 20k floor gate stays config-blind, and the panel copy names 20k.
• The spliced-tool-stop gap (a tool stop stored as "human" replays base+1; the keeper reads 8) is stated in the code and pinned by TestDashboardReplayKnownGapSplicedToolStop. Good handling of a known limit.

What I checked

• The full diff (14 files) and the code around each change: the keeper's decision path (record, pingable, Plans, WaitPings, WaitingFor), the replay (factsCTE, pingSpansWithFacts, expiries, SpanPings, storedWait, covers), the calculator and recommendation arithmetic, the proxy hook and its wiring, the UI rendering, and both golden files.
• The new tests' math, re-derived by hand: planFixture's spans (5 pings plan vs 2 fixed on session a; 3 on the open person wait; 0 on session b) and expiries (E1 only the plan reaches, E2 only fixed K=2, both at K=3) match what the code produces. The drift test stores what the proxy stores (the classifier's answer, before the keeper's tool-stop override), so it compares the right two things.
• SQL: every predicate is parameterized; toolStopList is built from fixed keeper constants. Ping rows stay excluded through Filter.where. The facts window partitions by thread and orders by ts, id; the old addressable CTE orders by ts alone, so rows with one timestamp can order differently — rare, and only inside one session.
• Boundaries: no import cycle (keepalive does not import dash); no request-path behavior change (the waitplan.go refactor moves toolStopWait and toolStop unchanged); fail-open is untouched, the change is read-only.
• Data dependence: the replay reads stored metadata only (stop reason, wait cause, thread id, cache tier, provider). docs/data-dependence.md states this. No new content reads.
• Tests were not run: live tests are not approved for this review, and Go is not installed on this machine.

[amiddavid]

Round 1: replies

New head: 010dce8f. Every finding is below.

R1-1 (1-hour tier): fixed in 010dce8

• SpanPings now returns the count and the spacing. The 1-hour tier uses ForOneHour() (MaxPings1h at Idle1h).
• The tier is the billed tier. The query follows the ResolveTier rule: asked 1 hour, and cache_write_1h >= cache_write, sticky per thread. A turn that writes nothing inherits.
• Reach on the 1-hour tier is judged against a 1-hour lifetime.
• The "as the keeper does" claim is gone.
• New drift cases: 1-hour own tool, 1-hour person, 1-hour no tool yet, read-only turn inherits, asked 1 hour but billed 5 minutes, 1 hour then 5 minutes. They compare count and spacing.

R1-2 (stored retry): rejected, with a test

• A stored retry comes from a response that failed (non-2xx). The keeper's Record returns before it tracks such a response, so it holds no entry and plans no ping for it.
• I added the drift case "failed call after a tool call". The keeper and the replay both give 0.
• The comment in waitplan.go now says this. If you have a case where the keeper pings a stored retry, send it and I will add it as a case.

R2-1 (effective policy): fixed in 010dce8

• DashWaitPolicy reports WaitPlan: false for an active campaign (where the tenant's keep-alive is on) and for an active strategy with Idle of 300 s or more.
• A shadow campaign and a 5-minute strategy keep the plan on.
• The test is TestDashWaitPolicyReportsTheEffectivePlan.
• The panel copy says the figures show the account setting, not a per-session override.

Non-blocking, all fixed

• A failed tenant lookup now returns plan OFF (fixed K).
• At an idle of 300 s or more the calculator models fixed K (TestCalculatorAtThreeHundredSecondsModelsFixedK).
• The calculator response has plan_counts (tool-wait max, background extra, person extra, max). The panel copy reads them. Nothing is hard-coded. Golden ka_calc.json gains this field, and nothing else changed.
• The plan rung has data-testid="ka-ladder-plan".
• The stale comment about tenant configuration is rewritten.
• Incomplete name list (calledATool): the stored row does not have this fact. I added it to the pinned known-gap test and its comment. No new column.

Mutations (both restored)

• SpanPings uses the base count for the 1-hour tier: 6 drift cases fail.
• DashWaitPolicy ignores the campaign: TestDashWaitPolicyReportsTheEffectivePlan fails ("active campaign: plan still on").

Tests, with -race, on the box: go vet ./... clean, gofmt -l clean, ./keepalive/... ok, ./dash/... ok (580 s), ./proxy/... ok (470 s), ./cmd/... ok.

[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	010dce8f19b7	blocking

Verdict: blocking

What I checked

• Last review: round 1, sha fd354b76811611169d242d538383a63bfb4585b8, posted 2026-11-03T00:01:26Z.
• New commits since: 1 — 010dce8f "fix(dash): model the 1-hour tier, retry waits and the effective plan (#868)". Full diff read, plus the code around every change.
• Conversation read in full: 2 reviews, 1 comment, 0 inline comments (from the three saved files).
• New items since my round 1: the author's reply comment (2026-11-03T00:33:35Z) — every claim in it checked against the code; the new commit; reviewer 2's round-1 review — its finding checked against the fix.
• No tests were run (live tests not approved); all claims below are from reading the code.

Earlier findings

1. R1-1 (1-hour tier, base count): fixed.
   • SpanPings now returns the count and the spacing, and the 1-hour tier runs ForOneHour() — MaxPings1h at Idle1h, the live schedule.
   • The tier is the billed tier. The SQL (factsCTE, dash/keepalive.go:1321-1342) follows ResolveTier's rule: asked 1 hour, cache_write_1h >= cache_write, sticky per thread. A read-only row inherits. The wcode encoding is sound: ROW_NUMBER strictly increases per writing row, so MAX(wcode) picks the latest write; the parity bit carries the billed tier; a thread with no writes reads hour = 0.
   • 1-hour reach is judged at a 1-hour lifetime. The "as the keeper does" claim is gone. Six new drift cases pin count and spacing.
2. R1-2 (stored "retry"): rejected, and the rejection is right. I accept it.
   • Record returns before it tracks a non-2xx response (keeper.go:773-775). My round-1 trace missed that gate.
   • I also missed that any arriving request drops the live entry (arriveAt, proxy.go:1411, ArriveAt deletes k.live[id]). So the failed call's own arrival ends the previous entry's pings. Both sides give 0, and the new drift case "failed call after a tool call" pins it. The round-1 claim was wrong; the comment now matches the code.
3. Reviewer 2's R2-1 (effective policy): fixed for the main cases, but the fix has a divergence — see finding 1.
   • The plain account, the shadow campaign, the 1-hour strategy, the 5-minute strategy and the config-off case are all right, and TestDashWaitPolicyReportsTheEffectivePlan pins them. The lookup-failure fallback is now off, as I asked in round 1.

Findings

1. proxy/keepalive.go:581-588 — DashWaitPolicy returns plan-off for any ACTIVE campaign, but the request path lets a manager strategy displace the campaign and keep the plan on
   • What fails: an account with keepalive_wait_plan on, keep-alive on, an ACTIVE learned campaign, and a manager strategy with Idle under 300 s (say 240 s) that targets it. On the request path the strategy matches first (applyStrategyWindows, applied != ""), so the campaign branch is skipped — the code's own comment says "An ACTIVE campaign decides the ping count unless a manager strategy matched" (proxy/keepalive.go:456-458). The strategy sets Idle/MaxPings and leaves WaitPlan as the account set it (proxy/keepalivestrategy.go:146-158 never touches WaitPlan). Idle 240 s is under the 300 s gate, so the per-wait plan runs. The hook still returns off at :585-587, because its campaign branch tests only Mode == campaignModeActive. The calculator then reports plan: fixed_k, drops the plan rung, and the panel says the account setting switched the plan off — the opposite of what the proxy sends.
   • A second case, same branch: an ACTIVE campaign on an account whose models are unpriced. predictiveDecideInner returns pdecOK = false without a price where pRead > 0 && pWrite > pRead (proxy/predictive.go:360-365), so the campaign never owns the count on the request path and the plan runs. The hook says off.
   • Proof: the trace above. The doc comment at :550-552 says the hook reports off "whenever the request path would turn the plan off for the account" — the campaign+strategy case contradicts it. The new test has the active campaign and the 5-minute strategy as separate cases, never both at once.
   • Fix: check the strategy first. When a covering strategy with Idle under 300 s exists for the tenant, return the plan-on policy; return off for the campaign only when no strategy covers it. The pdecOK conditions are per request, so a read hook cannot copy them exactly — either price-check the account's recent models, or state that limit in the doc comment and the panel copy.

Non-blocking

• factsCTE's "asked" half reads cache_ttl, which ttlTier sets when any breakpoint asks 1 hour (apply/apply.go). ResolveTier requires every breakpoint to ask (keeper.go:267-275, the comment names the mixed-ask head-over-tail case). The two diverge only for a mixed ask whose writes all billed 1 hour — rare, but the comment on factsCTE says "as keepalive.ResolveTier decides it", which is not exact.
• PlanCounts.MaxPings reports keepalive.MaxWaitPings (11), but the real cap is max(base, 11) (waitplan.go:73). The UI does not render this field today, so nothing is wrong on the page — a trap for the next reader.
• Both fixed by the same commit and verified: the 300 s fixed-K model, plan_counts wired through the UI (assigned at app.js:9546, read at 9527), ka-ladder-plan testid, the rewritten stale comment, the calledATool gap pinned in the known-gap test.

Data dependence and behavior

Unchanged from round 1: metadata only, no behavior change on the request path. The new hook is a read path. The PR body's statements hold for the new commit.

[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	010dce8f19b7	blocking

Verdict: blocking

Findings

1. proxy/keepalive.go:581 — DashWaitPolicy lets an active campaign switch the plan off even when a manager strategy owns the policy. The request path resolves the strategy first.
   • What fails: an account with keepalive_wait_plan on, an ACTIVE learned campaign, and an active manager strategy with an Idle under 300 s (the ordinary 5-minute band). On the request path the strategy matches, so the campaign does not displace it: resolvePolicyWindows sets applied at proxy/keepalive.go:450 and skips the pol.WaitPlan = false at :458-461. The keeper runs the per-wait plan. DashWaitPolicy checks the campaign first and returns the fixed-K model at :585-587, before it reads the strategy at :589-594. The calculator then shows fixed-K rows only and marks the account's K current. The hint says "every row is a fixed count", while the proxy picks the count per wait.
   • Proof: the two orders, cited above. The doc comment at :550-558 says "an active campaign owns the count (resolvePolicyWindows)" — resolvePolicyWindows hands the campaign the count only when no strategy matched (:458 tests applied == ""). TestDashWaitPolicyReportsTheEffectivePlan covers each layer alone, never both together. The precedence is a supported state, not a freak: the comment at :456-457 names the strategy "the more explicit instruction".
   • Fix: mirror the request path. Resolve bestStrategy first. When one matches, let its Idle decide (300 s or more → off, else the account's plan) and skip the campaign. When none matches, let an active campaign switch the plan off. Add the coexistence case to the test.

Earlier findings, re-checked

Reviewer 1, finding 1 (the 1-hour tier ran the base count, at the asked tier): fixed.

• SpanPings returns the count and the spacing, and models ForOneHour (1 ping at 59 min by default).
• factsCTE computes the BILLED tier: the last write of the thread, sticky, with read-only turns inheriting. I checked the parity code against ResolveTier case by case (a write billed 1 h, a write billed 5 m, a read-only turn that asks 1 h, one that asks 5 m, a thread with no write). All match.
• planReaches judges a 1-hour-tier expiry against a 1-hour lifetime.
• Six new drift cases compare the count and the spacing. The "as the keeper does" claim is gone.

Reviewer 1, finding 2 (a stored "retry" got 0 pings): rejected by the author. I checked the rejection, and it holds.

• The host's Arrive retires the earlier entry when the failed request arrives (keeper.go:705-723).
• Record returns at keeper.go:773-775 for any status outside 2xx, before the tracking gates. The round-1 trace stopped at the no-entry gate (:865) and missed this one.
• So the keeper holds nothing after a failed call and plans no ping. The replay's 0 matches. The new drift case pins it, and the comment now states the rule.

My finding 1 (DashWaitPolicy read only the account config): fixed in the main cases, with one residual — finding 1 above.

• An active campaign, a 1-hour strategy, a failed lookup and the account switch are all modeled now, each with a test.

Non-blocking from round 1, both reviewers: all fixed.

• The 300 s gate: modeled in SpanPings and in KeepAliveCalc, with a test.
• The hardcoded copy: plan_counts reaches the page; the golden gains the field and nothing else.
• The duplicate ka-ladder-k2 testid: the plan rung is ka-ladder-plan.
• The stale comment about tenant configuration: rewritten.
• The incomplete name list: pinned as a second known gap, in the test and its comment.
• The OpenAI cadence still runs at the 5-minute policy, as before this PR. The fixed rungs carry the same simplification, so the comparison stays fair (reviewer 1's own note).

Non-blocking

• The stored cache_ttl means ANY breakpoint asked 1 hour (apply.go:1029-1034). ResolveTier requires EVERY breakpoint to ask it (apply.go:1082). A mixed ask whose writes all billed 1 hour reads OneHour in the replay, while the keeper runs the 5-minute plan for it. The comments at waitplan.go:137-139 and dash/keepalive.go:1316-1318 say "asked 1 hour on every breakpoint", which the stored column cannot tell. State the limit, or pin it as a known gap as the spliced-stop one is.
• The plan boolean is one figure for the account. The live path resolves a strategy per wait cause: windows gate only a person's wait (proxy/keepalive.go:374-379), so a 1-hour strategy with a closed window leaves a person's wait on the account plan at night. The doc comment names the session-override limit; this one has the same shape and no name.

What I checked

• Last review: round 1, fd354b76811611169d242d538383a63bfb4585b8, 2026-11-03T00:03:26Z.
• New commits read: 1 (010dce8f fix(dash): model the 1-hour tier, retry waits and the effective plan (#868)).
• Read in full: 2 reviews, 1 comment, 0 inline comments.
• New items since my last review: the commit above (full diff, plus the code around each change) and the author's reply comment at 2026-11-03T00:33:35Z (each claim checked against the code).

Then:

• The new commit's seven files, and the code around each change: waitplan.go (SpanPings, the retry comment), dash/keepalive.go (factsCTE, planPings, planReaches, the calculator and the recommendation, PlanCounts), proxy/keepalive.go (DashWaitPolicy), both test files, app.js, the golden.
• The factsCTE parity arithmetic by hand: wcode rises by 2 per row, so MAX(wcode) is the latest write's code; odd means that write billed 1 hour; a read-only row emits NULL, so it inherits. Checked against ResolveTier for every case above.
• The drift test's new model: Arrive between records retires the earlier entry, as the live host does. The 1-hour cases compare the spacing too.
• The rejection of reviewer 1's finding 2: Arrive and the status gate, cited above.
• DashWaitPolicy against resolvePolicyWindows, order by order — the source of finding 1.
• Nil-safety: predRT() returns nil when the keeper is nil; forTenant is nil-safe; the strategy read sits behind h.keeper != nil.
• SQL stays parameterized; ping rows stay excluded; keepalive imports no dash package. No request-path behavior changed: the commit touches the replay, the read hook and tests.
• Tests were not run: live tests are not approved for this review, and Go is not installed on this machine.

[amiddavid]

Round 2: reply

New head: ab1c0abc.

Blocking (strategy before campaign): fixed in ab1c0ab

• DashWaitPolicy now follows resolvePolicyWindows. It finds the covering strategy first, with bestStrategy as the request path does for a wait that is not for a person.
  • Idle of 300 s or more: plan off.
  • Idle under 300 s: the account's own setting, and the campaign is skipped.
• Only with no covering strategy does an active campaign turn the plan off.
• New cases in TestDashWaitPolicyReportsTheEffectivePlan: active campaign plus a 5-minute strategy gives plan on. Active campaign plus a 1-hour strategy gives plan off. The single-layer cases stay.
• Mutation: with the campaign check back first, the new case fails ("active campaign + 5-minute strategy: plan off, want on"). Restored.

Limits, now stated

• The DashWaitPolicy doc comment names three: the campaign's per-request price gate (pdecOK), the per-session override, and a 1-hour strategy whose window is closed.
• The comments at waitplan.go (SpanFacts.OneHour) and dash/keepalive.go (factsCTE) say the stored cache_ttl means ANY breakpoint asked 1 hour, where ResolveTier needs EVERY one.
• A third known-gap test, TestDashboardReplayKnownGapMixedBreakpointTier, pins it: the keeper stays on 5 minutes, the replay reads 1 hour.
• PlanCounts.MaxPings is now max(base, MaxWaitPings). No golden changed (the default base gives 11).

Tests (box, -race where run): go vet ./... clean, gofmt -l clean, ./keepalive/... ok, ./proxy/... ok (481 s), ./dash/... ok (578 s).

[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.317	offload-review skill, @amiddavid's session	resumed	2 of 2	3 of 3	ab1c0abc5ab6	approve

Verdict: approve

Findings

None. Every finding from rounds 1 and 2 is fixed or settled, and the new commit adds no defect I can show.

Earlier findings, re-checked

My round-2 finding 1, and reviewer 1's round-2 finding 1 (the campaign checked before the strategy): fixed.

• DashWaitPolicy now resolves the covering strategy first (proxy/keepalive.go:588-594). An Idle of 300 s or more turns the plan off; less keeps the account's setting and skips the campaign. Only with no covering strategy does an active campaign turn the plan off (:596-603). This mirrors resolvePolicyWindows (:448-461).
• The test gains both coexistence cases: an active campaign plus a 5-minute strategy gives plan on; plus a 1-hour strategy gives plan off. The author also reports a mutation check.

Reviewer 1's round-2 second case (an active campaign on an account with no prices): settled by a stated limit.

• The live path hands the campaign the count only on a request that passes its price gate (pdecOK). The hook cannot see a single request, so it reports plan off for such an account. The doc comment now names this limit (:561-563). Reviewer 1's own fix text allowed "state that limit in the doc comment". I accept it.

My round-2 non-blocking notes: both addressed.

• The mixed-ask gap: both comments now say the stored cache_ttl means ANY breakpoint asked 1 hour, where ResolveTier needs EVERY one. A third known-gap test pins it.
• The window-collapse limit: stated as the third doc bullet (:565-566).

Reviewer 1's round-2 non-blocking: both addressed.

• PlanCounts.MaxPings is now max(base, MaxWaitPings) (dash/keepalive.go:1624), which matches WaitPings' own cap min(n, max(base, MaxWaitPings)). The golden stays at 11 for the default base, so nothing changed on the wire.

Non-blocking

• One more member of the documented window family, not named: an ACTIVE campaign plus a covering strategy with an Idle under 300 s whose window is CLOSED. On the live path the windows-ignored strategy apply is skipped when a campaign is active (resolvePolicyWindows, :453), and the windows-respected apply fails on the closed window, so the campaign owns the count and the plan is off. The hook finds the strategy with windows ignored (:589) and reports the plan on. It diverges only while the window is closed. The doc bullet at :565-566 states the mirror image. One sentence there would close it, or a windows-respected lookup when a campaign is active.

What I checked

• Last review: round 2, 010dce8f19b7001b48a5843b78332147e8b4c2ee, 2026-11-03T00:45:42Z.
• New commits read: 1 (ab1c0abc fix(proxy): a covering strategy decides the dashboard plan before a campaign (#868)).
• Read in full: 4 reviews, 2 comments, 0 inline comments.
• New items since my last review: reviewer 1's round-2 review at 00:45:16Z (posted seconds before mine, so I had not read it when I wrote round 2 — its finding checked against the fix), the author's round-2 reply at 08:32:19Z (each claim checked against the code), and commit ab1c0abc (full diff, plus the code around each change).

Then:

• The commit's five files, and the code around each change: proxy/keepalive.go (DashWaitPolicy and its doc comment), proxy/dashwaitpolicy_test.go, keepalive/waitplan.go (SpanFacts.OneHour), dash/keepalive.go (factsCTE, PlanCounts), dash/keepaliveplan_test.go.
• The new precedence against resolvePolicyWindows, case by case: no layers; a strategy alone (Idle under and over 300 s); a campaign alone; a campaign plus a 5-minute strategy; a campaign plus a 1-hour strategy; a closed window plus a campaign (the non-blocking note above).
• The third known-gap test's math: the partial replace makes only the system breakpoint ask 1 hour, so the keeper reads 5 minutes (AllBreakpointsAskExtendedTTL fails) while the replay reads 1 hour (the stored cache_ttl, a fully billed 1-hour write). The assertions pin the spacing on both sides.
• Counts.MaxPings = max(k, MaxWaitPings) against WaitPings' cap, with k defaulted before the count is built.
• The doc comment's three limits against the code: the price gate (predictiveDecideInner returns not-ok without prices), the per-session override, the closed 1-hour window.
• No request-path behavior changed: the commit touches the read hook, comments and tests only. SQL, filters and boundaries are untouched by this commit.
• Tests were not run: live tests are not approved for this review, and Go is not installed on this machine. The author reports the package suites green on the box with -race.

[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.317	offload-review skill, @amiddavid's session	resumed	1 of 2	3 of 3	ab1c0abc5ab6	blocking

Verdict: blocking

What I checked

• Last review: round 2, sha 010dce8f19b7001b48a5843b78332147e8b4c2ee, posted 2026-11-03T00:45:16Z.
• New commits read: 1 — ab1c0abc "fix(proxy): a covering strategy decides the dashboard plan before a campaign (#868)". Full diff read, plus the code around every change.
• Read in full: 4 reviews, 2 comments, 0 inline comments (from the three files).
• New items since my round 2: the new commit (every hunk checked against the request path); the author's round-2 reply comment (2026-11-03T08:32:19Z — every claim checked against the code); reviewer 2's round-2 review (2026-11-03T00:45:42Z — same finding as my round 2, its proof checked).
• No tests were run (live tests not approved); all claims below are from reading the code.

Earlier findings

1. My round-2 finding (strategy must decide before the campaign): fixed for the unwindowed case.
   • DashWaitPolicy now follows resolvePolicyWindows in the same order: the covering strategy first, its Idle decides, the campaign only when no strategy covers (proxy/keepalive.go:588-603).
   • Two new coexistence cases pin it: campaign + 5-minute strategy gives plan on; campaign + 1-hour strategy gives plan off. Both hold under the new order.
   • The pdecOK price-gate limit is now stated in the doc comment, as I proposed.
   • One residual divergence remains, through the strategy time windows — finding 1 below.
2. R1-1 (1-hour tier): fixed in round 2, stays fixed. No change to that code in this commit.
3. R1-2 (stored "retry"): rejection accepted in round 2. Unchanged.
4. Reviewer 2's findings: agree, both closed. R2-1 and their round-2 finding are the same as mine; the fix handles them. Their two round-2 non-blocking notes are addressed: the ANY-vs-EVERY cache_ttl divergence now has accurate comments and a pinned test; the closed-window limit is now named as the doc comment's third limit.

Findings

1. proxy/keepalive.go:589-593 — the hook ignores strategy time windows; with an active campaign and a closed window, the request path turns the plan off but the hook reports it on
   • What fails: an account with the plan on, keep-alive on, an ACTIVE learned campaign, and a manager strategy with Idle under 300 s that carries time windows (the ordinary 60–290 s kind). During the strategy's closed hours, for every wait — person or not — the request path turns the plan off: the windowed match finds nothing (Matches requires InWindow, tenant/keepalivestrategy.go:297-302), the window-ignoring retry is skipped because the campaign is active (proxy/keepalive.go:453, its own comment: "with a campaign and a closed window, the campaign wins"), and the campaign then owns the count and sets WaitPlan = false (:458-461). The hook finds the strategy with windows ignored (:589, bestStrategy(..., true)), sees Idle under 300 s, and returns the plan on (:593). The panel marks the plan rung current and shows per-wait counts, while the proxy sends the campaign's fixed count for every wait during the closed hours. The hook's answer does not change with the hour, so the page shows the same wrong flag all day.
   • Proof: the trace above. The doc comment's third limit (:565-566) states the mirror direction only — a closed 1-hour window makes the hook say off where a person's wait stays on. The opposite direction (closed window + campaign → path off, hook on) is unstated, and it is the misleading direction: the panel promises a plan that does not run. Without a campaign the hook is right in every case (the retry then runs for non-person waits), so the campaign is a required part of the case — the same coexistence the commit exists to model.
   • Fix: when a campaign is active for the tenant, match the strategy with windows respected (bestStrategy(..., false)), as the request path does — a closed window then falls through to the campaign's off, and the flag is right at the moment the page is read. Or state this direction beside the third limit in the doc comment.

Non-blocking

• The mixed-tier known gap is handled well: the comments at waitplan.go (SpanFacts.OneHour) and dash/keepalive.go (factsCTE) now state the ANY-vs-EVERY rule exactly, and TestDashboardReplayKnownGapMixedBreakpointTier pins it. I checked the test's construction: partial replaces only the first of the two cache_control breakpoints in planBody, so the keeper sees a mixed ask (5-minute plan) while the stored cache_ttl and a fully 1-hour-billed write make the replay read 1-hour. Sound.
• PlanCounts.MaxPings is now max(base, MaxWaitPings), which matches the real cap (waitplan.go:73). The golden stays at 11 for base 2, consistent.
• The two new test cases in TestDashWaitPolicyReportsTheEffectivePlan match the new ordering; the single-layer cases still hold.

Data dependence and behavior

Unchanged from round 2: metadata only; the commit touches the read hook, comments and tests. No request-path behavior change.