feat(apps): keep a builtin app's cached data across leaving its page

[chenmingwei23]

Part of the app view-state and cache-retention work. Three PRs, one prerequisite:

• feat(apps): identity seam, view-state store, and cache retention #8403 identity seam (base: main) -- prerequisite for both below
• feat(apps): restore where a builtin app page left off #8407 view-state store (base: feat/builtin-app-identity-seam)
• feat(apps): keep a builtin app's cached data across leaving its page #8404 cache retention (base: feat/builtin-app-identity-seam)

#8407 and #8404 are siblings, not a chain: either may merge first once #8403 is in.
Related: #8412 (stale bundle ceiling on main, merged), #8394 (external app cache
isolation, separate track), #8401 (convert the remaining AWS Control key sites,
after #8407).

Stacked on the app-identity branch (feat/builtin-app-identity-seam) -- review this against that branch, not main. This change depends on the PROPERTY that a builtin page carries a host-minted app identity and that a host-owned namespace is granted only for a builtin origin; it does not depend on which change in the stack ships that code. It is independent of the view-state work and touches none of the same files.

1. What is the problem?

Leave a builtin app page and the dashboard unmounts it. React Router drops the routed element, so the component's state resets, and react-query's default gcTime of 5 minutes then collects the data the page was showing. Come back after a short absence and the page rebuilds from nothing: loading placeholders first, then a fresh round of requests for data that has not changed.

This is platform-wide rather than a defect in any one app. Of the 19 builtin apps that issue queries, exactly one is exempt, and only because it solved the problem in app-local code no other app can reuse: apps/issue-radar/IssueRadarPage.tsx:37 calls setQueryDefaults(['issue-radar'], { gcTime: CACHE_RETENTION_MS }) at module scope. The other 18 have no mechanism to reach for.

There is a second, quieter problem underneath it. An app page cannot say which keys in the shared query cache are its own. Without that, there is nothing for retention -- or for anything else scoped to an app -- to attach to.

2. Why this issue matters to the user

An app that repaints from scratch on every visit cannot be a place you return to. It pushes you toward one long-lived session and charges you for navigating, which is the opposite of what a dashboard made of many small apps is for. The cost lands on the backend as well: unchanged data is re-fetched once per visit.

AWS Control is the reported case. Its accounts, drive listing and cost figures all sit in the cache under ['aws-control', ...], so a six-minute detour into chat and back means three skeletons and three round trips for a bill that has not moved.

Visual evidence (isolated pod, port 8009)

Captured from an isolated pod running this branch's built bundle (appQuery-onDNC_pg.js confirmed in the served dist/assets). The pod has no AWS accounts, so the page renders its empty state -- which still exercises the react-query cache path (the accounts/drive/costs queries fire and return empty arrays; cache retention keeps those results across unmount).

1. AWS Control loaded -- the app's Accounts page after initial navigation:

2. Navigated away to chat -- left the app, which unmounts the page component:

3. Returned to AWS Control -- navigated back; the page painted immediately from cached query data with no loading skeletons:

The absence of skeletons on return (frame 3 is identical to frame 1) is the shipped behaviour. On the base branch without this change, react-query's 5-minute gcTime would have collected the query results during the absence, and the page would re-fetch from nothing. The unit-test A/B (faked clock, 6 minutes, with and without identity) reproduces and pins the difference; these frames confirm the SPA renders in the pod with the feature code active.

3. How our fix solves it

Chaining from the symptom to the root cause:

• Symptom: skeletons and a re-fetch on returning to an app.
• Because: the app's cache entries were garbage-collected while the page was unmounted.
• Because: the shipped default is react-query's 5 minutes, and nothing raises it per app.
• Because: raising it per app requires knowing which keys belong to which app, and a builtin page had no app identity in the React tree to scope anything to.

So the fix has two halves, both standing on that page-level identity.

Retention, at the page seam. BuiltinAppRoute is the one place the host knows "this route belongs to app X" while the page's first query has not yet mounted. A sibling ahead of its Suspense boundary, <AppCacheRetention />, reads the appId through useTrustedAppId() and registers gcTime for the [appId] key prefix. react-query matches query defaults by prefix, so that single registration covers every key the app already writes by hand.

This is the first thing worth stating plainly rather than leaving for a reviewer: retention needs no change to any app. AWS Control's ['aws-control', ...] keys -- all 35 of them before this change converted two -- are already under the prefix, so the reported symptom is fixed by host code alone. useAppQuery buys something different -- the greppable distinction between "my key" and "a key I share with the host" -- and not the fix itself. Which keeps the design's framing honest at two levels: gcTime is an accelerator, not the mechanism, and the mechanism for landing you back where you were is the view-state record in #8407.

The registration happens in a render body, never in an effect, and that is not a style preference. An effect version passes a cold-load test and still fails a user. On a cold load the page module is still being fetched, so Suspense holds the child back until after the parent has committed and its effects have run -- the ordering bug is invisible. On a repeat visit the module is already resolved, React renders parent and child in one pass, and an effect is a render too late. That is precisely the visit this design exists for. Both cases are pinned; moving the call into a useEffect reds only the second.

Namespacing, at the call site. useAppQuery(['costs', id], { queryFn }) takes the appId from context and prefixes the key, so an app cannot forge its own namespace. Code that genuinely wants the host's cache stays on plain useQuery.

The second thing worth stating up front: the prefix is exactly the appId, not ['app', appId, ...]. That is the only shape under which two requirements hold at once -- the host authors the prefix, and no existing app query key is renamed. useAppQuery(['drive', account]) resolves to ['aws-control', 'drive', account], byte-identical to what the app writes today, so every existing invalidateQueries keeps matching. What changes is who authors the prefix, not what it is. One limit on that, stated rather than left to a reader: the hook makes the prefix host-authored for the code that calls it -- it does not stop a query elsewhere in the tree from typing another app's prefix by hand. What stops that is the ownership ratchet in section 4. The hook is how an app opts in; the ratchet is the tree-wide invariant. That matters because five prefixes are shared between an app and the host deliberately -- artifact, awsConsent, apps, pull-request-source, and workflow-definitions / workflow-runs. Renaming the last would split a deliberately shared cache in two and desynchronize the workflow cards in chat (pages/chat/ActivityViewer.tsx:843, pages/chat/WorkflowRunCard.tsx:159) from the list in the app.

The third thing, and the earlier version of this section had it wrong in a way that undersold the change. I had written that retention is a no-op for workflows, command-bar and code-review-sage, and that this set was "almost exactly" the shared-prefix table. Measured properly -- counting, per registered app, how many of its query keys sit under its own appId versus another prefix:

app                     own  other   other prefixes
issue-radar              43      5   atp, issue-radar-pipeline
aws-control              29      1   artifact
ops-mission-control      27      0
code-review-sage         20      5   code-review-sage-draft, pull-request-source, sage
pptx-maker               16      0
mochi                    14      0
meetings                 10      0
file-explorer             9      0
spec-builder              9      0
personal-shopper          7      0
papyrus                   6      0
md-notebook               3      0
auto-improvement          0     14   auto-improvement-branches, auto-improvement-config, ...
auto-research             0     15   research-campaign, research-campaigns, ...
workflows                 0      7   workflow-definitions, workflow-runs, ...

So 12 apps and 193 keys are covered with no change to any app -- not just AWS Control -- each on the first visit to its page. command-bar and code-review-sage were in my no-op list and should not have been: they hold 2 and 20 own-namespace keys respectively and get retention for them; only their genuinely shared prefixes (apps, pull-request-source) are left out.

The real no-op set is three apps, and only one of them for the reason I gave. workflows is the designed case: its keys are workflow-definitions / workflow-runs, read by the chat cards, so the host's cache stays the host's decision. auto-improvement and auto-research are a different thing worth naming -- their keys are ['auto-improvement-config'] and ['research-campaign'], single hyphenated strings rather than ['auto-improvement', 'config']. react-query matches defaults element-wise, so those miss [appId] by a naming shape, not by any deliberate sharing. They would be covered by writing the key as an array, which is what useAppQuery produces by construction. That is a better argument for the namespacing half than the one I had been making.

The silent failure this is uniquely exposed to, and how it is closed. Everything above depends on the page's identity being the one the host minted. Two builtin pages mount an api-layer provider inside their own subtree: apps/spec-builder/SpecBuilderPage.tsx:131 and apps/ops-mission-control/IncidentChat.tsx:77. Had a nested provider shadowed the host identity with its own origin: 'external' default, those pages' queries would fall back to un-prefixed keys while retention stayed registered on [appId] -- the app's data landing outside the very namespace being retained. There would be no error, no skeleton fix, and nothing in a diff to grep for: the feature would simply not happen on two pages, and the bug report would read "the cache fix does not work sometimes".

The identity layer closes it by publishing only when there is none in context, so a nested provider cannot revoke a host-minted namespace. Two tests hold it from this side -- one per provider shape -- and the mutation result is what shows they measure the real property rather than passing by construction: deleting the if (existing) return scoped early return reds only the AppApiProvider case and correctly leaves the AppScopedApiProvider case green, because the scoped layer publishes no identity to shadow with.

APP_CACHE_RETENTION_MS is 30 minutes, and deliberately issue-radar's own number rather than a second one. A test reads it back from that app's source, so the two cannot drift apart silently.

One call site converted, and deliberately a mixed one. UsagePane's costs query in ConsoleView.tsx now uses useAppQuery(['costs', id]), while the refetchGated invalidation three lines below it keeps writing ['aws-control', 'costs', id] by hand. They still address one cache entry, which demonstrates the byte-identity claim in the product rather than only in a test -- and it is what lets the rest of the app convert one call site at a time. The remaining hand-written sites -- 33 occurrences of the ['aws-control' literal, one of them a comment -- are #8401. #8401 is what waits on #8407, not this PR: #8407 edits DrivePage.tsx and AwsControlPage.tsx, and a concurrent rewrite of those two files would conflict invisibly. Nothing in this change depends on #8407.

Also worth knowing: with zero conversions, useAppQuery has no importer and rolldown tree-shakes it out of the production bundle entirely. The one conversion is what puts it in the shipped code (dist/assets/appQuery-*.js), so the mechanism is verifiable in a build rather than only in a test run.

4. What tests we did

34 new tests across three files, plus two added to the app's own suite.

Pure rules (test/appCacheRetention.test.ts): the retention plan is refused for a host page and an external app; the resolved key is byte-identical to the literal it replaces; a key that already carries the appId collapses instead of double-prefixing, and warns once per app; applyCacheRetention has no dead null-branch, proven by a restored guard reding the case rather than by a type directive; and the 30-minute value, asserted directly and against the 5-minute default it exists to beat (there is deliberately no second literal to reconcile it with any more).

Mounted behaviour (test/appQuery.test.tsx): the key lands under the app namespace; it degrades to a plain query with no identity; an external app is refused the namespace; the namespace survives either api-layer provider nested inside a builtin page (the two call sites named in section 3); useAppQueryKey() builds the same key useAppQuery used and an invalidation through it refetches; retention is in place before the page's first render on a cold load and on a repeat visit; one registration per client and app; nothing registered on a host page or for an external app; retention at [appId] does not reach a prefix that is not the appId; and BuiltinAppRoute actually mounts the seam.

The consequence, not just the registration. All of the above proves setQueryDefaults was called with the right value at the right moment. It does not prove what a user feels, so two more tests do, as an A/B of one flow -- load, unmount, wait, look -- on a faked clock, in structurally identical trees differing only in whether an identity is published:

• with identity, the entry is still there after six minutes and gone after thirty (retained, and bounded rather than Infinity)
• without identity, it is gone at six minutes -- the reported symptom, reproduced in a test rather than argued about

The control is what makes that evidence: granting retention to a null appId reds it, so it is measuring the real property. Six minutes of wall time costs nothing on a fake clock, which is why this is in the suite rather than deferred to a manual walk.

The invariant [appId] retention rests on is now a test, not an audit. Registering gcTime for an app's whole prefix is only safe because no query outside that app claims it -- otherwise a host query would inherit an app's retention the moment the app's route rendered. That held when this landed, established by hand, which is a fact about one afternoon. test/appPrefixOwnership.test.ts makes it a property of the tree: it derives ownership from the registry itself (which already pairs each appId with the module that owns it, so a new app is covered the moment it is registered) and scans every non-test source file for a foreign claim. 0 violations across 22 registered apps. The detector is also fed a PLANTED violation, because a scanner that finds nothing and a scanner that looks at nothing are indistinguishable from a green test -- disabling its match reds that case and leaves the real scan green.

The two ordering cases deserve a note, because the first version of them was quietly vacuous. A first-render assertion under a React.lazy child cannot tell a render-body registration from an effect: Suspense holds the child back until the parent has committed, so the effect has already run. Only a warm second mount reds. The first version got its warmth from the cold test having run before it in the same file, which meant that run alone -- a -t filter, a reordering, a shard split -- it passed against the effect version. It also asserted no-suspension by checking the fallback was absent from the DOM, which is true after the page arrives whether or not the tree ever suspended.

Both are fixed. The cold case builds its own deferred lazy so it is cold regardless of order; the warm case does both mounts itself, with a fresh client on the second so retention has to be registered again. Suspension is recorded by the fallback component as it renders rather than read off the DOM afterwards, so each case asserts positively which path it was on: the cold one suspended once, the warm one not at all. Verified by re-running the mutation in three scopes -- with applyCacheRetention moved into a useEffect, the warm case now reds alone as well as in file order, while the cold case alone stays green.

Product-level (apps/aws-control/ConsoleView.test.tsx): the host-built costs key and the hand-written invalidation beside it address a single cache entry, with only one costs entry in the cache; and the reverse, where a useAppQueryKey()-built invalidation reaches a hand-written query key and makes the pane refetch. The second went in vacuous -- it invalidated the literal rather than the builder's output, so the prefix mutation did not red it -- and was rewritten to route through the builder. The pane's 12 existing mounts now run inside AppIdentityProvider, so they exercise the namespaced path a user is on rather than the degrade path.

Sixteen mutation checks, each killing a named test. Notably: replacing the render-body registration with a useEffect fails the repeat-visit test -- run alone as well as in file order -- while the cold-load test stays green, which is the asymmetry this design warned about; changing the prefix shape to ['app', appId, ...] fails both AWS Control byte-identity tests, in each direction; reading useAppIdentity().appId instead of useTrustedAppId() fails the external-app refusal; and removing the provider's no-shadow early return fails only the nested-AppApiProvider case, since the scoped layer publishes no identity to shadow with. Two of the newest confirm the clock A/B is not vacuous: setting the retention to react-query's own 5 minutes reds the retained case, and granting retention to a null appId reds the CONTROL. tsc -b and eslint clean. 171 tests green across the affected files, including the identity seam's own suites, the provider-split suite, and the chat protocol boundary pin.

Pod evidence: the pod is running this code, not a stale bundle

Verified in an isolated pod on a pinned port, never the running gateway. Recorded verbatim, because "the pod served an older bundle" is the specific failure a screenshot would otherwise be guarding against, and a grep of the served asset answers it directly.

Positive -- strings from this diff, fetched from the pod over HTTP:

GET /assets/appQuery-onDNC_pg.js
  "already starts with the appId"                 -> 1 match   (resolveAppQueryKey's warning)
  "may not use a host-owned state namespace"      -> 1 match   (the builtin gate it reads)
  18e5                                            -> 1 match   (APP_CACHE_RETENTION_MS, 30 min)

GET /assets/AwsControlPage-DCc0tUSK.js
  ...from"./appQuery-onDNC_pg.js"                 <- the app chunk imports the host module
  i=D([`costs`,t],{queryFn:()=>Z.costs(t)         <- host-built key, query direction
  count of ([`costs`,t]                           -> 1 match
  count of ([`drive`,t]                           -> 1 match   (host-built key, invalidation
                                                                direction, via useAppQueryKey)

Negative -- what this change removes, absent from the served bundle. Two cases, both measured:

ConsoleView.tsx        ['aws-control', 'costs'   at 9f9b0c64a -> 2   (query + invalidation)
ConsoleView.tsx        ['aws-control', 'costs'   at this commit -> 1  (invalidation only)
GET /assets/AwsControlPage-DCc0tUSK.js  [`aws-control`,`costs`  -> 1  (matches: the query moved)

GET /assets/IssueRadarPage-CjA_n9iC.js  setQueryDefaults          -> 0
GET /assets/IssueRadarPage-CjA_n9iC.js  18e5                      -> 0

The first pair shows the costs query moved to the host-built key while its invalidation did not, in shipped output. The second pair is issue-radar's app-local retention being genuinely gone rather than commented out: no registration, and not even the 30-minute literal, which now exists in exactly one place in the shipped bundle.

One detail worth reading, because it looks like the build did not run: deleting CACHE_RETENTION_MS from issue-radar left every chunk hash unchanged. That is the correct outcome and is itself evidence -- the constant had no production reader left, so it was already being tree-shaken out, and removing it changed no emitted bytes.

Note what is grepped and what is not: every pattern above is a string literal, an object key, or a chunk filename, because those are what survive minification. A grep for a function name is unreliable in both directions, measured across this same dist:

useTrustedAppId        -> 0 files    identifier, mangled away
useAppCacheRetention   -> 0 files    identifier, mangled away
resolveAppQueryKey     -> 0 files    identifier, mangled away
AppCacheRetention      -> 0 files    identifier, mangled away
useAppQuery            -> 1 file     NOT the identifier -- it is inside the
                                     warning text this change introduces

Zero would not have meant absent, and the one non-zero result is not the function either. Hence literals.

The pod's gateway also boots healthy on this change, and website/dist and the served src/kiro_crew/static/dist carry the same appQuery-BTmvLjqw.js, so the served copy is this build and not a leftover.

Outstanding: full-window screenshots, and the real-clock walk

Two items are outstanding on the record rather than dropped, and neither is a waiver.

Full-window screenshots are not attached yet. The agent cannot mint a dashboard credential for the pod: kirocrew pod token is blocked precisely because it prints a credential, and the pod's own credential is withheld here because the guard cannot prove which process holds the port without lsof/netstat on this host. Reading the pod's secret file to mint a token is the same door with a different handle, so it was not done. No cropped or component-level image is offered as a substitute either.

The end-to-end walk on a real clock is also outstanding: open AWS Control, descend a folder, leave, wait past five minutes, return, and see it repaint with no placeholder. That needs a logged-in dashboard, which is the same missing credential, so it belongs in the same batch as the captures. The mechanism itself is covered without it -- the faked-clock A/B above reproduces the symptom and its absence, and the byte-identity test pins the key -- but the walk is what confirms it in a browser rather than in jsdom, and it is not being claimed as done.

Both get attached once a credential is pasted in or the frames are taken from a logged-in dashboard.

5. Any other suggestions on the work

Two advisory review findings were acted on rather than argued with, and one was declined with reasons.

Acted on -- issue-radar's duplicate registration is deleted, and so is its constant. The registration was worse than redundant, which the first version of this PR body got wrong. setQueryDefaults is a Map keyed by the hashed key, so the host's ['issue-radar'] registration and the app's own wrote the SAME entry and the last writer won -- and which one was last depended on module evaluation order (the host registers while rendering the route; the app's module-scope line runs when its lazy chunk evaluates, after). Identical values hid it; any future divergence would have been decided silently by chunk timing.

Removing the registration then left CACHE_RETENTION_MS with no production reader at all -- a constant existing only to be reconciled against another constant by a test. So it is deleted too, its rationale is folded into APP_CACHE_RETENTION_MS where the number now actually lives, issueRadarPolling.test.tsx reads the platform constant, and the source-reading drift test is gone with the drift it was watching for. One constant beats two literals kept in step by a test.

Acted on -- useAppQueryKey no longer ships without a consumer. refetchGated in ConsoleView.tsx now builds its drive invalidation through it. That also strengthens the demonstration instead of diluting it, because the pane is now mixed in BOTH directions: costs is queried by the host and invalidated by a hand-written literal, drive is queried by a hand-written literal and invalidated by the host. Either mismatch would break a grant's ability to refresh what it changed. Changing the prefix shape to ['app', appId, ...] reds both cases.

Acted on -- useAppCacheRetention returns void. It was handing back the plan that nothing read: its one caller renders nothing from it, and resolveCacheRetention already exposes the same value purely, so the return was a second way to obtain something no caller wanted.

Acted on, ahead of the suggestion -- the safety claim is a test now. Registering gcTime for an app's whole prefix is only safe because no query outside that app claims it; that was established by hand during this work, which is a fact about one afternoon rather than a property of the tree. Since this PR body asserts the claim, it should be the PR that pins it: test/appPrefixOwnership.test.ts derives ownership from the registry (already the appId-to-module map, so a new app is covered on registration) and scans every non-test source file for a foreign claim -- 0 violations across 22 apps -- with the detector additionally run against a planted violation so a green scan is not the same as a scan that looked at nothing. Suggested as a follow-up to #8401; done here because the assertion is made here.

Answered -- whether the namespacing hooks belong in this PR at all. Retention alone fixes the reported symptom, so useAppQuery and useAppQueryKey could in principle be split out. They are here because the approved delivery plan puts them here: its PR-3 row reads "Cache retention: useAppQuery auto-prefixing plus per-app gcTime; AWS Control as first consumer". The two halves also answer one question together rather than separately -- retention needs a namespace to attach to, and without a host-authored way to say "this key is mine" the next app has no way to opt in except by naming its keys after itself and hoping. Each hook has exactly one production consumer, both in this PR, both exercised in opposite directions by the same pane.

Declined -- registering all app prefixes in a module-scope loop over the registry. The suggestion is to drop the render-time seam and instead call setQueryDefaults([appId], ...) for every registry entry at startup, deleting the component, the per-client memo, the render-body ordering constraint, and the cold/warm test apparatus. It is a real simplification and the reasoning is sound; two things argue against it, and one argument for keeping the current shape turned out NOT to hold:

• What does not hold: the objection that startup-wide registration would apply app retention to host queries sitting under an app-shaped prefix. Checked across every appId in the registry -- no host query does. The only apparent hit, pages/DevFleetPage.tsx, is the dev-fleet app's own page, filed outside apps/.
• getQueryDefaults iterates every registered default on every query's option resolution, which is every query on every render across the whole dashboard. That is 23 prefix comparisons instead of one or two, permanently, for prefixes belonging to apps the user never opened. Small, but not the "semantics are identical" the suggestion assumes.
• The ordering property would stop being enforced and stop being tested. Right now it is guaranteed by React rendering a parent before its children and pinned by a test that reds when the call moves into an effect. At module scope it becomes an invisible property of the import graph -- true today because the registry is statically imported, with nothing failing if that ever changes.

Left as it is, deliberately, and recorded here so the option is on file rather than lost.

Acted on -- useAppCacheRetention is module-private. It was exported with no consumer outside its own file; the only caller is the AppCacheRetention component beside it. app-sdk/appQuery.ts now exports exactly three things: the component, useAppQueryKey, and useAppQuery.

Held -- folding resolveCacheRetention / applyCacheRetention / CacheRetentionPlan into the hook. The consumer counts are accurate, and inlining would collapse them to about three lines. Two reasons it stays split, the second being the substantive one. The approved design names it directly -- "Keep resolveCacheRetention and identity resolution as pure functions, following the shape of apps/overlaySlots.ts" -- with the stated reason that a rule deciding where a user's cached data lands should be verifiable without mounting a tree. And resolveAppQueryKey has to stay in that module either way (the review agrees: two consumers, real logic), so folding the other two removes no file and no import -- it moves the retention rule out of the pure module that survives regardless and into the React one, which is a relocation rather than a subtraction. If the maintainer wants the smaller surface anyway, it is a contained change and I will make it.

Held -- the already-prefixed warning in resolveAppQueryKey. Correct that no current call site can trip it, because there is one converted site and it is hand-verified. That is precisely what it is for: the 34 conversions in #8401 are where a careless one silently forks a query off the namespace its own invalidations match, and the symptom then appears as stale data after a write, far from the line that caused it. A warning is cheap now and expensive to add after the sweep.

Answered, and the first version of this was wrong -- why not just raise the global default. Setting gcTime beside the existing staleTime: Infinity in api/queryClient.ts is one line and would remove the symptom for every app at once, including the three this change leaves as no-ops. This PR body previously rejected that by asserting "chat and sessions hold large result sets on the same client", and First Principles is right that nothing here measured it. So that claim is withdrawn: the memory differential of holding host keys 25 extra minutes is not measured, and this PR does not claim it is.

What the choice actually rests on does not need a measurement. api/queryClient.ts defaults reach chat, sessions, and every host surface -- code this change does not touch, does not test, and whose owners did not ask for a retention change. A per-app registration changes only the app whose page is open, is one legible line per app, and is revertible per app; a global default is one line to write and a dashboard-wide behaviour change to reason about. That asymmetry, not a memory number, is why the approved design put the global raise out of scope.

It is worth saying plainly that this makes the global raise the cheaper change, not a worse one. If it is what the platform wants, it should be measured and taken on its own -- with chat and sessions in the blast radius considered by the people who own them -- rather than arriving as a side effect of an app-scoped cache fix.

Held, with the reason stated a third time -- deferring the namespacing hooks to #8401. Raised in each review round and answered the same way: the approved delivery plan's PR-3 row specifies both halves together, and retention needs a namespace to attach to or the next app can only opt in by naming its keys after itself and hoping. First Principles marks it concerns-not-block on the same grounds. If the maintainer prefers the split, it is a mechanical extraction of two hooks and one call site and I will do it -- but it is a plan change, not a defect.

Held, though my first reason for holding was wrong -- the injectable client on AppCacheRetention. It has no production consumer; BuiltinAppRoute mounts it bare. I first defended it on test isolation, and the review correctly answered that the retention tests already mount their own QueryClientProvider, so useQueryClient() would return the per-test client and isolation would be unaffected. That was right and my reason was not.

The actual blocker is measured rather than argued. AppCacheRetention is mounted by BuiltinAppRoute, and that route's own suite renders it with no QueryClientProvider at all, so switching to useQueryClient() turns those renders into "No QueryClient set": 3 tests in the stacked change below this one, plus 1 of mine. Verified by making the switch and running both suites. Fixing it means editing a test file that belongs to another change in the stack while that change is review-ready, which is not this PR's to touch. Once it lands, the switch is a two-line change and I will make it.

• No useAppInfiniteQuery. Infinite queries, setQueryData and invalidateQueries all take a key from useAppQueryKey() and stay on the plain react-query hook, so one resolver serves every cache API. Adding a wrapper later is additive; two of AWS Control's sites are infinite queries and will use the key builder.
• A pre-existing trust gap, filed separately rather than folded in: an external app can reach the host's QueryClient today, because app-sdk/shared-modules.ts:40 registers the live react-query instance on window.__kirocrew_modules while the scoped-API layer scopes only REST paths. That is why useAppQuery is off the app-sdk barrel and imported by path -- but the barrel is not what is leaking.

[github-actions]

UX Review (Fable 5) — ✅ PASS

UX-level review of badab7b73b2426b6d110bfb32f0cfe34f2fb5298 — updated in place on each push. A BLOCK verdict blocks PR readiness; PASS/CONCERNS are advisory.

UX-Verdict: PASS

Invisible-by-design change — returning to an app now repaints from cache instead of skeletons; no new user-facing surface, strings, or controls.

Suggestions

• The evidence screenshots don't exercise the change: 03-returned-no-skeletons.png is byte-identical to 01-aws-control-loaded.png but shows "0 accounts · 0 keys" (an empty state that renders instantly regardless), and a seconds-long chat detour sits inside the pre-existing 5-minute gcTime anyway — re-capture with populated account data after a >5-minute absence to actually demonstrate the retention.

[UX-REVIEWED] badab7b

[github-actions]

Design Review (Fable 5) — ✅ PASS

Design-level review of badab7b73b2426b6d110bfb32f0cfe34f2fb5298 — updated in place on each push. A BLOCK verdict blocks PR readiness; PASS/CONCERNS are advisory.

All hunks map to the stated purpose, alternatives were explicitly weighed (global gcTime raise, module-scope registry loop), keys stay byte-identical so there's no cache migration and the change is reversible per app, the namespace grant sits behind the builtin-origin gate, and the last-writer-wins hazard with issue-radar's own registration is removed rather than left latent. The remaining candidate findings (regex-based ownership ratchet robustness, render-body side effect) are line-level or already pinned by tests, and the committed screenshots follow an existing repo convention.

Design-Verdict: PASS

Right seam, byte-identical keys, gated namespace, per-app reversibility — the alternatives were weighed on the record and the residual gaps are named, tested, or tracked.

[DESIGN-REVIEWED] badab7b

[github-actions]

Opus 4.8 Review — ✅ no blocking findings

Reviewed badab7b73b2426b6d110bfb32f0cfe34f2fb5298 — this comment is updated in place on each push.

Review details

No findings.

[OPUS-REVIEWED] badab7b

Verdict parsed from the review's SHA-scoped output markers for commit badab7b73b2426b6d110bfb32f0cfe34f2fb5298.

False positive or not applicable? A repository writer can comment:
/ai-review override fable badab7b73b2426b6d110bfb32f0cfe34f2fb5298: <one-sentence reason>

[github-actions]

First Principles Review (Fable 5) — 🟡 CONCERNS

Premise-level review of badab7b73b2426b6d110bfb32f0cfe34f2fb5298 — why this exists and whether the shipped surface is the smallest honest version. Updated in place on each push. A BLOCK verdict blocks PR readiness; PASS/CONCERNS are advisory.

All checks done — the review contract, intent, patch, and repo evidence are gathered; consumer counts verified by grep. Final review:

First-Principles-Verdict: CONCERNS

The retention seam alone is the fix; the useAppQuery namespacing half rides along with exactly one converted call site, its value deferred to #8401.

What this change ships

Intent: returning to a builtin app should repaint from cached data instead of skeletons — a FIX (reported on AWS Control), shipped with a declared ADDITION.

1. Returning to any builtin app within 30 min repaints instantly, no skeletons — justified, A/B-pinned
2. All 19 builtin apps' data now held 30 min in memory after leaving (default was 5) — justified, number inherited from issue-radar production
3. Issue Radar's own retention constant and module-scope registration deleted — justified subtraction (last-writer-wins hazard named)
4. New useAppQuery hook (host-authored key prefix) — rides along, declared; one consumer
5. New useAppQueryKey builder for non-hook cache APIs — rides along, declared; one consumer
6. AWS Control: 2 of ~35 key sites converted — declared partial, deferred to Convert AWS Control's remaining query sites to the host-namespaced key #8401
7. Double-prefixed keys collapsed with a once-per-app console warning — declared
8. New tree-wide ratchet: no host query may claim an app's key prefix — justified (makes [appId] retention safe)
9. Three PR screenshots under temp-screenshots/ — repo convention (801 existing files), not a finding

Watch

• The description itself concedes the split: "retention needs no change to any app" and useAppQuery is "not the fix itself." Grepped useAppQuery|useAppQueryKey outside tests: one consumer each (ConsoleView.tsx:266,272). The namespacing half is a seam whose payoff is entirely in Convert AWS Control's remaining query sites to the host-namespaced key #8401; if that stalls, this PR shipped a 154-line module for one query. Declared and coherent, so not a block — but a human should confirm Convert AWS Control's remaining query sites to the host-namespaced key #8401 is real before this merges.
• The zero option for item 4 was available: ship retention + the ownership ratchet only, since the prefix is already the appId by hand. The description's counter (greppable ownership, forge-proofing) is an argument from future conversions, not from a defect anyone hit.

Subtractions

• Fold resolveCacheRetention + CacheRetentionPlan + applyCacheRetention (apps/appCacheRetention.ts:282,262,351) into their single caller useAppCacheRetention (app-sdk/appQuery.ts:104-117) — one non-test consumer each (grepped); the whole retention rule is one line, client.setQueryDefaults([appId], { gcTime: APP_CACHE_RETENTION_MS }), and resolveAppQueryKey alone justifies the pure module.

[FIRST-PRINCIPLES-REVIEWED] badab7b

[github-actions]

GPT 5.6 Review — ✅ no blocking findings

GPT 5.6 completed its review of badab7b73b2426b6d110bfb32f0cfe34f2fb5298 and found no blocking issues.

This comment is updated in place on each push.

Review details

No findings.
[GPT-REVIEWED] badab7b

False positive or not applicable? A repository writer can comment:
/ai-review override gpt badab7b73b2426b6d110bfb32f0cfe34f2fb5298: <one-sentence reason>

[chenmingwei23]

Both First Principles concerns are fixed in 3a4ab1674 rather than argued with.

issue-radar's duplicate registration is deleted, and it was worse than redundant -- the PR body had this wrong. setQueryDefaults is a Map keyed by the hashed key, so the host's ['issue-radar'] write and the app's own wrote the SAME entry and the last writer won, with "last" decided by module evaluation order: the host registers while rendering the route, the app's module-scope line runs when its lazy chunk evaluates, after. Identical values hid it. Any future change to either number would have been settled silently by chunk timing. Its queryClient and CACHE_RETENTION_MS imports went with it; lib/format.ts still exports the constant, which is what this change reads its own number back from. The app's chunk now ships zero setQueryDefaults calls.

useAppQueryKey has a real consumer: refetchGated in ConsoleView.tsx builds its drive invalidation through it. That strengthens the demonstration rather than diluting it, because the pane is now mixed in both directions -- costs queried by the host and invalidated by a hand-written literal, drive queried by a literal and invalidated by the host. Either mismatch would stop a consent grant refreshing what it changed. Changing the prefix shape to ['app', appId, ...] now reds both cases; the reverse-direction test went in vacuous (it invalidated the literal instead of the builder's output, so the mutation did not red it) and was rewritten to route through the builder.

[chenmingwei23]

On the module-scope loop over BUILTIN_COMPONENT_REGISTRY: a real simplification, and one argument I expected to make against it does not hold. Declining it, with the reasoning on the record.

What does not hold: I assumed startup-wide registration would leak app retention onto host queries sitting under an app-shaped prefix. Checked every appId in the registry -- no host query does. The only apparent hit, pages/DevFleetPage.tsx, is the dev-fleet app's own page, filed outside apps/.

Two things that do:

1. getQueryDefaults iterates every registered default on every query's option resolution -- which is every query on every render across the whole dashboard, not just app pages. That makes it 23 prefix comparisons instead of one or two, permanently, for prefixes belonging to apps the user never opened. Small, but "semantics are identical" is not quite the whole story.
2. The ordering property would stop being enforced and stop being tested. Today it is guaranteed by React rendering a parent before its children, and pinned by a test that reds when the call moves into an effect -- verified again this round, in isolation as well as in file order. At module scope it becomes an invisible property of the import graph: true right now because the registry is statically imported, with nothing failing if that ever changes.

The test apparatus reads as the most mis-fixable part of the diff, and I think it is the opposite: it is what stops the mechanism being quietly moved into an effect later. Its first version WAS vacuous -- order-dependent warmth plus a fallback assertion that held whether or not the tree suspended -- and it has been rewritten so each case establishes its own state and asserts positively which path it took.

Recorded in section 5 so the option is on file rather than lost.

[chenmingwei23]

Both subtractions considered in 1b6dd1feb; one taken, one held with the reasoning stated.

Taken -- useAppCacheRetention is module-private now. It was exported with no consumer outside its own file; the only caller is the AppCacheRetention component beside it. app-sdk/appQuery.ts exports exactly three things now: the component, useAppQueryKey, and useAppQuery.

Held -- folding resolveCacheRetention / applyCacheRetention / CacheRetentionPlan into the hook. The counts are right and inlining would collapse them to roughly three lines, so this is a judgement call rather than a disagreement about facts. Two reasons, and the second is the one that decides it for me.

The approved design names this directly: "Keep resolveCacheRetention and identity resolution as pure functions, following the shape of apps/overlaySlots.ts", on the grounds that a rule deciding where a user's cached data lands should be verifiable without mounting a tree.

More concretely: resolveAppQueryKey stays in that module either way -- you agree, two consumers and real logic -- so folding the other two deletes no file and no import. It moves the retention rule out of a pure module that survives regardless and into the React one. That is a relocation, not a subtraction, and it costs the ability to check the rule against a recording fake. If the maintainer prefers the smaller surface anyway, it is contained and I will make it.

Held -- the already-prefixed warning in resolveAppQueryKey. Correct that no current call site can trip it: there is one converted site and it is hand-verified. That is what it is for. The 34 conversions in #8401 are where a careless one silently forks a query off the namespace its own invalidations match, and the symptom then surfaces as stale data after a write, far from the line that caused it. Cheap now, expensive to add after the sweep.

On the contingency point -- if #8401 stalls, the namespacing half is permanent surface with one consumer. Accepted as stated. It is the honest risk of the staging, and the reason the conversion is a filed issue with a named scope rather than a vague intention.

For the record on process: still one commit, amended rather than added, per the two-commit gate.

[chenmingwei23]

Checking these two claims corrected an error of mine that had been in the body since the first push, in this PR's favour.

"The apps it exists to onboard are barred from adopting it" is not right, and neither was my no-op list. I had written that retention is a no-op for workflows, command-bar and code-review-sage. Measured per registered app -- own-appId keys versus other prefixes:

issue-radar          43 own / 5 other      code-review-sage  20 / 5
aws-control          29 / 1               pptx-maker        16 / 0
ops-mission-control  27 / 0               mochi             14 / 0
meetings             10 / 0               file-explorer      9 / 0
spec-builder          9 / 0               personal-shopper   7 / 0
papyrus               6 / 0               md-notebook        3 / 0
auto-improvement      0 / 14              auto-research      0 / 15
workflows             0 / 7

So 12 apps and 193 keys are covered with no change to any app, not just AWS Control. command-bar (2 own keys) and code-review-sage (20) were in my no-op list and should not have been -- they get retention for their own namespace, and only their genuinely shared prefixes are left out.

The real no-op set is three apps, and only workflows for the reason I gave: its keys are read by the chat cards, so the host's cache stays the host's decision. auto-improvement and auto-research are something else worth naming -- their keys are ['auto-improvement-config'] and ['research-campaign'], single hyphenated strings rather than ['auto-improvement', 'config']. Defaults match element-wise, so those miss [appId] by key SHAPE, not by any deliberate sharing, and writing the key as an array covers them -- which is what useAppQuery produces by construction. That is a better argument for the namespacing half than the one I had been making.

Accepted -- the ratchet, not the hook, is what stops an in-tree query claiming another app's namespace. Correct, and the body now says so: the hook makes the prefix host-authored for the code that calls it, and the ownership ratchet is the tree-wide invariant. My "cannot forge" phrasing overstated the hook's reach.

Held -- the client prop, and my earlier reason for holding it was wrong. You are right that the retention tests already mount their own QueryClientProvider, so useQueryClient() would give per-test isolation; my test-isolation defence does not stand. The actual blocker is measured: AppCacheRetention is mounted by BuiltinAppRoute, whose own suite renders it with no provider, so the switch produces "No QueryClient set" in 3 tests of the stacked commit below this one plus 1 of mine. Verified by making the change and running both suites. That file is not this PR's to edit while the change below is review-ready; once it lands the switch is two lines and I will make it. The decomposition subtraction is linked to the same trigger.