Repository navigation
[AI-2326] Gate the work-items nudge on the tenant's plan - #976
Conversation
On the Free plan every /api/work-items route 403s work_items_not_in_plan, but the SessionStart nudge still told the agent to call declare_work_item each session, so the only visible kcap interaction was a bounced tool call that reads as a payment nag. The plan arrives the way the server's own version already does: a passive X-Kcap-Plan response header captured on every authenticated lane into a per-server file under the config root. That is what makes one answer serve all nine harnesses — eight of them discard the session-start response body, but they all send on a lane the handler is registered against, so no per-harness plumbing is needed. Fail-open throughout: an absent header, an unreachable server or a stale answer allows the nudge, so this can only ever remove one that today fires regardless. A denial that stops being re-heard expires, so a rolled-back server cannot suppress a paying tenant's nudges permanently.
PR Summary by QodoGate work-items nudges on cached tenant plan entitlements
AI Description
Diagram
High-Level Assessment
Files changed (20)
|
check-linear-ids.sh forbids them outright under src/, and suppression is for synthetic test data only — so the comments carry the behaviour instead of the ticket reference.
Code Review by Qodo
1. Tenants inherit another tenant's plan
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
The in-process write memo suppressed an identical rewrite unconditionally, so a long-lived process — the daemon, an MCP server — that kept observing the same denial never refreshed the record's timestamp. After seven days its own dedupe aged out the entry it was confirming, and the nudge came back. Bound the suppression by a refresh interval well under the staleness horizon, so repeated identical answers keep the record fresh while the hot path still touches disk at most once an hour per distinct value.
| ["seen_at"] = at, | ||
| }; | ||
|
|
||
| var tempPath = $"{path}.tmp"; |
There was a problem hiding this comment.
The cache is shared by independent kcap processes, but every writer for one server uses the same .tmp path. During concurrent hook responses, process A can write a denial, process B can overwrite that temp file with an allowance, and A can move the bytes from B into the cache while memoizing the denial; the move by B then fails and is swallowed. A will skip subsequent identical denying headers for up to RefreshAfter, leaving the cache allowing a Free tenant and re-enabling the nudge this change is meant to suppress. Please use a unique temp file per write and make deduplication account for changes to the shared file by other processes; a concurrent-writer regression case would pin this.
There was a problem hiding this comment.
Confirmed and fixed in 09064ec — both halves, thank you.
The shared temp path is gone: each write publishes through {path}.{pid}.{guid}.tmp, so a move can only ever place its own bytes and the loser of a race can no longer have its file published by the winner.
The memo hole was the more interesting half, and you're right that the unique temp name alone doesn't close it — the memo records what we wrote, which stops being what the file holds the moment a peer can overwrite it. Suppression now re-reads and skips only when the cache still carries our answer, so a correction can't be suppressed against a file that disagrees. That's a small read on the hot path in place of a write, which seemed the right trade versus dropping deduplication entirely and writing on every authenticated response in the daemon.
Two regression cases: APeerOverwritingTheFile_IsCorrectedRatherThanSuppressed (fails against the previous commit) and ConcurrentWriters_LeaveAWholeFileAndNoTempFiles.
Worth flagging that ServerVersionStore has the same shared-.tmp pattern this was modelled on. The consequence there is milder — a wrong version string in an update notice, not a re-enabled nudge — so I've left it alone rather than widen this PR, but it's the same race and I'm happy to follow up separately if you'd rather it went too.
realtonyyoung
left a comment
realtonyyoung
left a comment
There was a problem hiding this comment.
One actionable concurrency issue in the shared plan-entitlement cache; see the inline comment. Static review only; I did not build or run tests.
Two halves of one hole, both reachable when hooks fire concurrently on a machine. Every writer for a server used the same temp path, so two processes wrote the same file and whoever moved first published whichever copy happened to land there last — possibly the other process's. The loser's move then failed against a file already gone and was swallowed, and the publisher went on to memo a value the cache did not hold. Each write now publishes through a temp name unique to it, so a move can only ever place its own bytes. The memo records what we wrote, which is not what the file holds once a peer can overwrite it. Suppression now re-reads and only skips when the cache still carries our answer — a small read on the hot path in place of a write. Without it a process could suppress its own correction for up to an hour while the cache allowed a Free tenant, re-enabling the nudge this exists to suppress. Covers both with regression cases: a peer overwriting the file is corrected rather than suppressed, and concurrent writers leave a whole file and no temp files behind.
|
NO FINDINGS — re-reviewed the concurrent cache-write fix at 09064ec. Unique temp files prevent a peer from publishing another writer’s bytes, and the memo now verifies the shared file before suppressing a correction. Static review only; no build or tests run. |
realtonyyoung
left a comment
realtonyyoung
left a comment
There was a problem hiding this comment.
The concurrency fix addresses my earlier finding; no remaining actionable findings in the current diff. Static review only; no build or tests run.
What
On the Free plan every
/api/work-itemsroute 403swork_items_not_in_plan, but the SessionStart work-items nudge still told the agent to calldeclare_work_itemevery session. The bounced tool call was, for a Free user, the only visible kcap interaction — it reads as a payment nag rather than a product.This gates the nudge on the tenant's plan.
How
The plan arrives the way the server's own version already does — a passive response header captured into a per-server file under the config root:
X-Kcap-Plan: work_items=0,projects=1— comma-separatedkey=flag,0denies.PlanEntitlementCaptureHandlersits besideServerVersionCaptureHandleron the Default/Hook/Memory lanes (not Anonymous — same reasoning as the version capture: those requests go to a server the caller has not adopted).PlanEntitlementStoremirrorsServerVersionStore: one file per normalized server URL, atomic write, in-process dedupe.WorkItemsNudgeEmitter.Resolvetakes the cached answer and returns null when Work Items are denied.Why a header and not the session-start response body: eight of the nine harnesses discard that body — they post through
AgentHookPosterand only read the status. But they all send on a lane the capture handler is registered against, so a header needs no per-harness plumbing and noSessionStartAckfield. One answer, shared by every harness on the machine.The nudge still takes no round trip and no lease of its own, so it stays composable after the lease-gated memory/guidelines lane exactly as before — a cache read preserves that invariant where a fetch would not.
Failure posture
Fail-open in every direction, so this can only remove a nudge that today fires regardless:
The staleness horizon is only reachable when a server stops sending the header — every authenticated response refreshes it otherwise. Without it, a rollback would suppress a paying tenant's nudges permanently with no way back.
Cost of the cache being one response behind: at most one stale nudge across a plan change. Claude reads it after the session-start response has landed, so it is fresh in-session; the other eight write stdout before awaiting their POST (deliberately — see
KiroHookCommand), so they read the previous request's answer.Not in this PR
X-Kcap-PlanfromITenantPlan. Follows inkcap-serverwith the submodule pin bump. Until it merges this is inert: no header, no behaviour change.projectsandanalytics, which have the same shape one layer quieter (registered on Free, refused on Free) but nothing nudging an agent toward them. The header carries the whole entitlement set so they cost nothing later.Test
PlanEntitlementsTests(parse/render, caps, fail-open),PlanEntitlementStoreTests(round-trip, normalization, staleness, corruption),PlanEntitlementCaptureHandlerTests(capture, absent header, upgrade clears a denial), plus the plan-gate cases inWorkItemsNudgeEmitterTests. 24 + 12 green locally.Linear: AI-2326 — customer report from the moosegoose tenant, independently reproduced in dogfooding.