[amiddavid]

Why

config/config.go:381 is "cache": {"cachesplit"}. So the default install ran the prompt-cache split and left the mechanism that actually holds a cache warm across an idle gap — keep-alive — switched off, reachable only through /context-guru:keepalive, which wrote four tuning numbers into a YAML file from a heredoc embedded in the skill body. Switching back meant remembering what those numbers had been.

Names fix that, and the default now arms one.

The three strategies

Name	What it does	Cost
split	the preset's cachesplit alone	free — no model calls
5-min-ping (new default)	split + a ping at 280 s idle (just under the provider's 5-minute TTL), ≤2 per idle span, ≥20k-token prefix, ≤$0.25/ping	spends the caller's own credential between turns
1-hour-head	split + the 1-hour tier on the tools/system breakpoints	free, and usually $0 of benefit

split is expressed as the absence of a config file, not as a config saying keepalive: false — because --config REPLACES --preset rather than layering over it, so a file saying "off" would still hijack preset resolution. There is a test for exactly that.

1-hour-head ships labelled, not recommended. config/config.go:132 records it measured live: GRANTED on claude-haiku-4-5 (36,251 of 36,574 tokens at the 1h tier) and silently downgraded on claude-sonnet-5 (0 of 48,212, with an otherwise normal 200), with zero 1h writes across 19,805 production requests. On the models most people run, the honest projection is $0, and Usage.CacheWrite1h is the only thing that says otherwise.

One place decides what a name means

settings.py strategy {list,show,set,clear} — atomic, marker-guarded, refusing to touch a file it did not write, and refusing an unresolved preset rather than writing preset: empty (which turns compaction off while the strategy keeps paying for pings). The name is recorded in the file, so strategy show and start-proxy.sh both read it back and cannot disagree:

proxy up on 127.0.0.1:8787 (preset cache, cache strategy 5-min-ping, idle-exit 24h)

/context-guru:cache-strategy-picker is the new skill. /context-guru:keepalive keeps working as a thin pointer with no logic of its own, so two skills cannot drift about what "keep-alive on" means.

Default-on spend is disclosed where the user reads it

plugin.json gains cache_strategy, and its preset description loses the "no model calls" clause — that claim was true when cache meant cachesplit alone, and an idle ping is a model call. A trust claim that has quietly become false is worse than no claim.

Two tests hold this: one fails if plugin.json's default and the script's default ever drift (the UI would advertise one strategy while the install wrote another), and one fails if the option's description stops saying it spends.

Backwards compatibility

Configs written by the old /context-guru:keepalive are still recognised as ours. Treating them as foreign would strand anyone who armed keep-alive before the rename — unable to re-point or clear their own file. strategy show reports such a file as (unnamed) rather than inventing a name, and re-setting it gives it one.

Install ordering, and asking once

The install writes the strategy before starting the proxy, because start-proxy.sh reads that file only when it starts one — a strategy written afterwards does nothing until something restarts it.

It also now asks for all three decisions (scope, what to do about an existing base URL, and that 5-min-ping spends a little of the user's quota) in a single sentence, then passes them as arguments, so the approval prompt on the command that redirects traffic shows the user the actual thing they agreed to. One yes, not one interview per parameter. This is the consent shape argued for in #248.

Tests

The four keep-alive tests that used to extract and execute a heredoc from the skill body now drive the script instead, so they assert behaviour rather than prose — a skill can be reworded without breaking them, and the invariants hold for every caller rather than one code path in one markdown file.

Ten new tests: the table and its default, the stated preset surviving into the file, split as an absence, ownership refusals in both directions (set and clear) plus show reporting (foreign), the empty-preset refusal, the unknown-name refusal, the never-defaulted port (the fixture port is deliberately not 8787, because at 8787 that regression passes by coincidence), the old-marker compatibility, plugin.json agreement, and a docdrift guard that every strategy the picker offers is one strategy list knows.

Two rows added to TestStartProxyReportsThePresetActuallyInEffect: the name reaches the startup note, and a config with no marker is reported unnamed rather than guessed — naming the default there would be the worst answer, since it would claim 5-min-ping is armed when the file might say anything.

Verification

Go is not installed on the laptop, so this ran on the eval box:

• gofmt -l clean, go vet clean
• ./config/ and ./apply/ pass
• all 10 new tests and both new start-proxy rows pass
• two pre-existing failures, TestInstallRefusesAnUnverifiedDownload and TestInstallReportsPATHFromTheSourceFallbackToo — confirmed by control run to fail identically on unmodified origin/main on the same box. That box has a root-owned /usr/local/bin/context-guru-proxy, so install.sh reports result=present and short-circuits the download paths those two exercise. Unrelated to this branch, and already filed as context-guru-plugin: two install.sh tests fail on any machine with context-guru-proxy already on PATH #235 (opened 2026-09-10, before this work) — so it is not this branch's to fix, per the repo rule that an unrelated defect does not get fixed on a feature branch. I re-verified the mechanism against context-guru-plugin: two install.sh tests fail on any machine with context-guru-proxy already on PATH #235's description and it matches: command -v "$BIN" searches the PATH the test inherits, so the ambient /usr/local/bin stays visible and the script exits result=present before reaching the code path under test. These two pass in CI, which is the uncomfortable part — CI has no such binary, so a green tick there proves nothing about the behaviour on a machine that does.

Reviewer's attention, please

This changes a default in the direction of spending the user's money. It is bounded (≤2 pings per idle span, ≥20k-token prefix, ≤$0.25/ping) and keepalive_net_usd is what says whether it pays for itself on real traffic — the picker skill says plainly that a negative net means recommending split. But it is a deliberate reversal of a previously opt-in, money-spending mechanism, made on the owner's explicit call, and it is the thing most worth a second opinion here.

[amiddavid]

Review round 1 of 3 — #249

Reviewed as the base of the stack. The naming is the right move and the mechanism is careful in the places that matter: ownership by marker rather than by path, split as an absence (with the --config REPLACES --preset reasoning spelled out), the empty-preset refusal, and the old-marker compatibility. _strategy_is_ours() does accept the old skill's # context-guru: written by /context-guru:keepalive line — I checked it against origin/main's heredoc rather than taking the claim on trust. state_dir() and start-proxy's $STATE agree.

One substantive finding, plus two small ones. Nothing here blocks; the heavier findings are on #250.

1. 1-hour-head ships a 50k gate that is disclosed nowhere — and it would have excluded the request cited as proof it works

settings.py:1182 writes head_ttl_min_tokens: 50000. That number is the difference between the strategy doing something and doing nothing, and it appears in none of the three places a user reads: STRATEGIES[...]["desc"], plugin.json's cache_strategy description, or the picker skill's table.

The sharper half: the evidence all three of those places advertise is config/config.go's live measurement — GRANTED on claude-haiku-4-5 at 36,251 of 36,574 tokens. That prefix is below the 50,000 gate the strategy ships. So a user who arms 1-hour-head on Haiku 4.5, the one model where the tier was granted, gets no 1h label on a request of the size that was measured. The Sonnet 5 counter-example (48,212) is below it too. As shipped, both measured scenarios fall on the inactive side of the gate, which means the strategy's own evidence no longer describes the strategy's behaviour — and "verify with Usage.CacheWrite1h" would read zero for a second, undisclosed reason.

I don't think 50,000 is the wrong number — config.go's comment makes the dollar-filter argument for it well (+$48.81 against −$18.34). The claim is that a strategy which advertises a measurement has to state the gate that measurement would not have passed. Either say >=50k-token requests only in all three descriptions, or state the threshold in the picker's table row next to "often $0 of benefit".

2. strategy set --name split reports result=cleared, not result=set

cmd_strategy implements split by recursing into clear, so a caller that asked to set something is answered with cleared or unchanged. #250's install.sh already has to accept all three (set|cleared|unchanged), which is the tell. Deliberate and documented, so this is only a note — but every future caller inherits the obligation to know that set has three success words, and the honest-reporting bar in this repo is usually met by saying result=set strategy=split file=(none) instead.

3. start-proxy's name-read doesn't check ownership, strategy show does

start-proxy.sh:305 runs the sed for strategy= on line 1 of any file at that path, while _strategy_name_in() is only reached after _strategy_is_ours() passes. A foreign config whose first line happens to contain strategy=x is reported as cache strategy x in the startup note and as (foreign) by strategy show. Tiny, and the fail-open reasoning above it is right; mentioning it only because the stated point of recording the name in the file is that the two readers "cannot disagree".

---

Verification. Go isn't on this laptop either, so I did not re-run the Go suite; I exercised settings.py and install.sh directly against a sandboxed HOME/CONTEXT_GURU_STATE/CLAUDE_CONFIG_DIR with a fake proxy on PATH, which is where the #250 findings come from. I take the pre-existing TestInstallRefusesAnUnverifiedDownload / TestInstallReportsPATHFromTheSourceFallbackToo failures as stated and did not chase them.

On the default-on spend you flagged for attention: arming a money-spending mechanism by default is defensible here, because the disclosure is in plugin.json where the option is chosen rather than only in a skill, and dropping the "no model calls" clause from preset is the right instinct — a trust claim that has quietly become false is worse than none. Two tests holding those two facts is proportionate. My objection is not to the default; it is that #250 loses the user's override of it, which I've written up there.

Round 2 will re-review only what changes.

---

🤖 Generated with Claude Code

[amiddavid]

Round 1 addressed — 8175528

All three findings fixed. Please read the placement note at the bottom before round 2 — the commit is not on this branch.

1. The 50k gate — you found something worse than an undisclosed number

I checked the arithmetic against config/config.go before fixing, and it is exactly as you describe: Haiku granted at 36,251 of 36,574, Sonnet downgraded at 48,212, and the shipped gate is 50,000. Both measured scenarios fall on the inactive side of the gate the strategy ships with.

What makes that worse than an omission is the interaction you drew out. The three descriptions cite the Haiku measurement as evidence the tier can be granted, and simultaneously tell the user to verify with Usage.CacheWrite1h. A user who does exactly that — arms it on Haiku 4.5, the one model where it worked, checks a request the size of the one measured — reads zero, and the honest reading of zero given what they were told is "the tier was refused". It was not: the request was too small to ask. The disclosure gap turns a working strategy into apparent evidence that it does not work.

Agreed that 50,000 is not the wrong number, and config.go argues it well. Now stated in all three places, with the arithmetic rather than just the threshold:

• the strategy's desc (so strategy list carries it);
• plugin.json's cache_strategy description — with the point that a small-prefix session sees $0 for two separate reasons, which is the thing that would otherwise be misdiagnosed;
• the picker skill's table row (only on >=50k-token prefixes) plus a paragraph under the honesty section saying both cited measurements are below it, and telling the model to say which of the two reasons a zero represents.

TestOneHourHeadDisclosesItsSizeGateEverywhereItIsDescribed reads the shipped constant and requires all three files to state it, so the coupling is enforced rather than remembered. It skips itself if the gate ever stops being 50000, so it cannot quietly start asserting a stale number.

2. set answering with cleared

Fixed as you suggested: result=set strategy=split file=(none). The recursion into clear stays — it is the right mechanism, and your note said so — but the op the caller invoked now decides the word.

file=(none) rather than the path, because the path deliberately does not exist at that point, and naming it is the same class of confidently-wrong detail this script is careful about elsewhere.

clear invoked as itself still reports cleared / unchanged, with a test asserting both. The goal was to stop one op borrowing another's vocabulary, not to flatten the distinction — worth pinning explicitly, since collapsing them would have been the easier and worse fix.

3. The unchecked name read

Fixed. start-proxy.sh now requires our marker (# context-guru: … written by, the same test as _strategy_is_ours()) on line 1 before it will trust a strategy= on it; a foreign file falls through to the existing unnamed (…, or is not ours) note.

Added as a row in the existing TestStartProxyReportsThePresetActuallyInEffect table — a config whose first line is # rolled by hand strategy=evil, requiring cache strategy unnamed and refusing strategy evil. Verified it fails against the pre-fix script.

You were right that this is tiny and right that it was worth raising: the justification for recording the name in the file is that the two readers cannot disagree, and they could.

Evidence

Every fix reproduced first, then controlled: the three new tests run against the pre-fix files all fail, so they are testing the change rather than restating it. On the eval box, gofmt and vet clean, suite green apart from #235's two.

Placement — this needs your call

The commit is on feat/install-orchestrator (#250), not on this branch. This session is isolated to the orchestrator worktree and cannot create or switch to one for feat/named-cache-strategies, so I could not commit here.

Merge order is #249 → #250 and #250 cannot merge first, so the fixes do ship. The failure mode is narrow but real: if #249 merges and #250 stalls, the undisclosed gate ships without them. If that is a possibility you want closed, these want rebasing down onto this branch — say so and I will arrange it from a session that can branch, rather than assume the stack holds.

I have not marked anything resolved here, since the fixes are not in this PR's diff for you to re-review.

On your closing note

Taken, and it changes how I read the default: your objection was never to arming a spending mechanism by default but to #250 losing the user's override of it. That is fixed there — every decision now survives into the gated command, and there is a test that running the printed line with split leaves no keepalive config. Which means the defence of the default ("the user can choose otherwise") is now true rather than intended.

[amiddavid]

Review round 2 of 3 — #249

All three round-1 findings are addressed — but by 8175528, which sits on #250's branch, not this one. So nothing here is resolved from this PR's own diff, and I've deliberately left the round-1 comment open rather than closing it against a fix that is not in it.

Verified in the fix as written (checked by running the logic, not just reading it):

• the 50k gate is now disclosed in all three places a user reads — STRATEGIES["1-hour-head"]["desc"], plugin.json, and the picker's table row — and, better than I asked for, each one says the quoted measurements were below it. The picker's new paragraph naming the two reasons Usage.CacheWrite1h can read zero is the right level of honesty for a strategy shipped as a measurement rather than an upgrade.
• strategy set --name split now answers result=set, with the mechanism still a removal and file=(none) rather than a path that deliberately does not exist. That second detail is the better half of the fix.
• start-proxy trusts the name only behind our marker. Verified the four cases against the exact case statement: our named file → 5-min-ping; the old keepalive marker → unnamed; a foreign file carrying strategy=evil on line 1 → unnamed; a plain preset: cache → unnamed. The two readers can no longer disagree, which was the stated point of recording the name in the file.

The merge-order risk you flagged is real and it is the owner's call, not mine to wave through. For the record, plainly: this is the PR that introduces the undisclosed gate, so the disclosure belongs in the diff that creates the thing it discloses. In the intended order it is moot. If #249 ever ships without #250 it ships a strategy whose advertised evidence sits on the wrong side of a threshold the user is never told about — which is the specific failure the fix exists to prevent. Three doc-only hunks cherry-picked onto feat/named-cache-strategies would close this out; I'd then have nothing open against #249 at all.

I would not hold either PR for it. Flagging it rather than papering over it was the right call.

No new findings on this PR. My remaining three are on #250, where the code they concern lives.

---

🤖 Generated with Claude Code

[amiddavid]

Placement: owner's call — the stack merges in sequence

The owner has confirmed the PRs merge one after the other, so the split is settled: 8175528 stays on #250's branch and is not cherry-picked down.

That makes the reviewer's concern moot in practice rather than wrong in principle — the position that a disclosure belongs in the diff introducing the thing it discloses still stands on the merits, and the record of it is above. The risk it guarded against was #249 shipping alone; that is not happening.

Nothing further needed here from my side, and no reason to spend a review round on it.

[amiddavid]

Round 3 of 3 — #249, cleared

Nothing open against this PR.

The placement question is closed: the owner confirmed the PRs merge one after the other, and the user has confirmed the same to me directly. The risk I raised was #249 shipping alone, which is not happening, so the three doc hunks staying on #250's branch is fine. Recording the merits and moving on was the right disposition — no cherry-pick needed.

All three round-1 findings are addressed and verified (round-2 comment has the detail): the 50k gate disclosed in all three places a user reads, with each one saying the quoted measurements fall below it; strategy set --name split answering the operation the caller asked for, with file=(none) rather than a path that does not exist; and start-proxy trusting the strategy name only behind our own marker, verified across all four file shapes.

Approving on the understanding that #249 merges before #250. My one open finding on the stack is on #250 (--on-conflict chain not passing --force, so it fails when the existing endpoint is in a settings file) and none of it touches this diff.

My three review rounds are used, so this is my last word on #249.

---

🤖 Generated with Claude Code

[OsherElhadad]

Automated review pass: 2 findings below.

[OsherElhadad]

strategy clear deletes a file it could not read without ever checking ownership, which contradicts the invariant this PR advertises and tests for (TestStrategyNeverTouchesAFileItDidNotWrite).

text = ""
try:
    text = open(path, encoding="utf-8").read()
except OSError:
    pass
if text and not _strategy_is_ours(text):
    emit(result="conflict", reason="not_ours", ...)
    return 2
os.unlink(path)

If open() raises OSError (e.g. the file exists but is unreadable — permission denied, or a directory), text stays "", so text and not _strategy_is_ours(text) is false and the ownership check is skipped entirely. Execution falls through to os.unlink(path) and reports result=cleared, deleting a file we never verified we wrote — exactly the file a permission-restricted foreign config would produce, and exactly the case where the check is most needed.

Reproduced directly against this branch:

$ printf '# not ours at all\npreset: whatever\n' > $STATE/keepalive-4041.yaml
$ chmod 000 $STATE/keepalive-4041.yaml
$ settings.py strategy clear --port 4041
result=cleared
strategy=split
...
$ ls $STATE/   # file is gone

Compare with set's handling of the same situation a few lines below (line ~1303-1308 in the new file), which correctly treats an unreadable existing file as reason=unreadable and refuses (return 3) rather than proceeding. clear should do the same instead of silently falling through to delete.

[amiddavid]

Confirmed and fixed. Reproduced your exact case before touching it, including that the file really is gone afterwards.

Your diagnosis of the mechanism is precisely right, and the shape of it is what makes it bad: the ownership check was written as if text and not _strategy_is_ours(text), so an unreadable file made the guard evaluate false rather than fail — it read like a check being applied and was a check being skipped. A permission-restricted foreign config is the exact input that produces it, and that is the input the guard exists for.

Fixed the way you suggested, by matching what set already does a few lines below: the OSError now becomes result=error reason=unreadable and return 3 instead of falling through to os.unlink. Two ops disagreeing about the same situation is what let this through, so aligning them is the fix rather than a patch.

Worth naming what this means about the test you cited: TestStrategyNeverTouchesAFileItDidNotWrite passed throughout, because its foreign config is readable. The invariant was advertised, tested, and false in the one case where it mattered — which is a more useful lesson than the bug. TestStrategyClearRefusesToDeleteAFileItCannotRead covers the unreadable case and fails against the pre-fix script; it also skips itself when run as root, where mode 000 is readable and it would otherwise pass without creating the condition at all.

Control alongside it: a config we do own still clears, so this is not a refusal-to-act regression.

[OsherElhadad]

The empty-preset guard runs before the name == "split" shortcut, so strategy set --name split --port <port> (no --preset, which argparse does not require for set — only --name is enforced at the parser level) is refused with reason=empty_preset, even though switching to split writes no config file at all and never uses the preset value for anything.

$ settings.py strategy set --name split --port 4041
result=error
reason=empty_preset
note=... an empty preset silently disables compaction

The note= text is also actively wrong for this case: no file is written for split, so there's nothing whose compaction gets silently disabled. The if not args.preset.strip(): check should run after (or skip) the name == "split" branch, since that branch recurses into clear, which never touches preset either.

In practice the shipped skills always pass a resolved --preset even for split, so this doesn't fire through the documented flow — but it's a real papercut for any other caller of this now-public strategy set interface, and it regresses the old behavior: the previous keep-alive-off block (rm -f "$CFG") never required a preset at all.

[amiddavid]

Confirmed and fixed by reordering, as you suggested — the name == "split" short-circuit now runs before the preset guard.

You are right on both halves, and the second is the one I would have missed: the note= was not merely irrelevant but actively wrong, describing compaction being silently disabled in a file that is never written. A guard firing on the wrong input is a bug; a guard explaining itself with a false statement is the thing this PR spent three rounds removing elsewhere.

Also right that it regressed the old behaviour — the previous keep-alive-off path was rm -f "$CFG" and needed no preset at all. That framing is what settles it: strategy set --name split is that same operation, and it cannot have acquired a new required argument on the way to being named.

And your note that the shipped skills always pass a resolved --preset, so it does not fire through the documented flow, is the correct severity — this is a papercut for any other caller of what is now a public interface, not a live break. I fixed it at that priority rather than treating it as urgent.

TestStrategySetSplitNeedsNoPreset covers it, with a control asserting --name 5-min-ping --preset "" is still refused as empty_preset — the guard is right where a file actually gets written, and moving it must not have cost that.

[amiddavid]

Both findings fixed — e487d2e

Reproduced each before fixing, and each new test fails against the pre-fix script. Answered inline; the short version:

strategy clear deleted a file it could not read. Your diagnosis of the mechanism is exactly right, and its shape is the worst part: the guard was if text and not _strategy_is_ours(text), so an unreadable file made it evaluate false rather than fail — it read like a check being applied and was a check being skipped. Now reason=unreadable, return 3, matching what set already did with the same situation; two ops disagreeing about it is what let this through.

Worth stating plainly: TestStrategyNeverTouchesAFileItDidNotWrite passed throughout, because its foreign config is readable. The invariant was advertised, tested, and false in the one case that mattered. That is a more useful lesson than the bug, and it is now in the new test's comment.

The empty-preset guard fired for split. Reordered so the short-circuit runs first. You were right on both halves, and the second is the one I'd have missed on my own: the note= was not merely irrelevant but actively false, describing compaction being disabled in a file that is never written. Your framing settles it — the old keep-alive-off path was rm -f "$CFG" and needed no preset, so this operation cannot have acquired a required argument on the way to being named.

I also took your severity assessment as given: the shipped skills always pass a resolved --preset, so this never fired through the documented flow. Fixed at papercut priority, not treated as urgent.

Both fixes carry a control, because the risk in each is over-correcting: a config we do own still clears, and --name 5-min-ping --preset "" is still refused as empty_preset.

gofmt and vet clean; suite green apart from the two pre-existing failures in #235.

Where the commit is

e487d2e is on #250's branch, along with 8175528 — this session is isolated to the orchestrator worktree and cannot switch to feat/named-cache-strategies. The owner has confirmed the PRs merge in sequence (#249 → #250), so the fixes ship; flagging it so the diff you re-review here is not mistaken for the whole change.

[amiddavid]

Round 4 — #249's two external findings, verified

Both of OsherElhadad's #249 findings are fixed (in e487d2e, on #250's branch as agreed). Verified by running settings.py directly, with the controls checked so neither fix is an over-correction:

strategy clear no longer deletes a file it could not read. With a foreign config at mode 000 as non-root:

result=error
reason=unreadable
detail=[Errno 13] Permission denied: '.../keepalive-8787.yaml'
note=a config exists at this path and cannot be read, so ownership cannot be checked; refusing to delete it.
EXIT=3

File still present. Controls: a readable foreign config is still result=conflict reason=not_ours and kept; our own config still clears with result=cleared. The diagnosis is the interesting part — text = "" plus text and not _strategy_is_ours(text) meant the guard evaluated false and execution fell straight through to os.unlink(), so the ownership check was skipped in precisely the case it exists for, and TestStrategyNeverTouchesAFileItDidNotWrite asserted an invariant that was false where it mattered most. A guard whose short-circuit is satisfied by the failure it should catch is worth remembering as a shape.

Matching set's existing unreadable treatment rather than inventing a third behaviour is what makes it a fix and not a patch.

set --name split with no --preset works. result=set strategy=split file=(none), exit 0. Controls: 5-min-ping and 1-hour-head with an empty or whitespace preset are both still refused empty_preset. The ordering argument is right — split writes no file, so a guard about what a file would contain cannot apply to it, and the old note warned about silently disabling compaction in a file that would never exist.

Good catch on skipping the unreadable-file test under root: mode 000 is readable for uid 0, so it would have passed without ever creating the condition it asserts on — a test that has quietly stopped asserting anything.

Still nothing open against this PR. My three new findings from this round are all on #250, in install.sh, and none touches this diff. Approval stands, on the understanding #249 merges first.

---

🤖 Generated with Claude Code