refactor(desktop): move Module Hub controller below AppShell

[chihumyum]

Summary

• add a hand-authored controllerOwners architecture policy that pins a controller implementation and symbol to one feature-owned JSX Provider
• make the policy monotonic against main: an owner contract cannot be removed or redirected, and its call count may only retire from 1 to 0
• move useModuleHubController from AppShellContent into ModuleHubProvider
• keep AppShell event intents on a stable command port while Scheduled Tasks, the Skills catalog revision, and the Module Hub Host subscribe at their actual readers
• remove the raw controller from the production feature entry and retain it only through the testing seam
• add deterministic render-scope, command-port cleanup, architecture provenance, and production Provider-to-Host coverage

This intentionally keeps the root guard and the first protected migration in one atomic PR: the migration never lands without its anti-regression contract, and the contract lands with a real registered owner.

Refs #4582 (supersedes #3439)

Performance evidence

I measured the ownership change in one Electron renderer instance with an isolated temporary profile. The run alternated legacy/scoped blocks in 7 paired rounds, with 30 awaited state updates per block. CDP CPU profiling used a 100 microsecond sampling interval; Performance metrics were sampled around each block.

Metric	Legacy owner / 30 updates	Scoped owner / 30 updates	Change	Per update
Busy JavaScript	235.323 ms	12.955 ms	-94.5%	7.844 ms -> 0.432 ms
ScriptDuration	230.511 ms	12.457 ms	-94.6%	7.684 ms -> 0.415 ms
TaskDuration	357.148 ms	98.100 ms	-72.5%	11.905 ms -> 3.270 ms

All 7/7 paired blocks favored the scoped owner.

This is a controlled scope benchmark: it isolates the same state update at the former AppShell owner versus the new Provider owner. It is not a claim that end-user interactions or the whole application are 94% faster; Module Hub updates are also less frequent than session switching.

The permanent, non-wall-clock regression test uses the real controller twice with identical fake services. A Scheduled Tasks update causes the legacy shell, unrelated subtree, and reader to render once; after scoping, the shell and unrelated subtree stay at zero while only the reader renders. It also verifies that a Skills refresh wakes only the Composer revision reader and that stale command-port cleanup cannot detach a newer controller target.

The temporary benchmark probes and driver were removed from the final diff.

Ownership guarantee

The previous debt ledger could ratchet hook/import/token counts, but it could not prove controller provenance or unique runtime ownership. The new contract additionally checks:

• the exact controller import binding and call site inside the registered Provider
• direct named Provider export and JSX-only use
• directory and /index public-entry imports
• aliases, namespace imports, re-exports, private/intermediary barrels, runtime loads, and same-name hooks from another module
• production/testing separation and the absence of the raw controller from the public feature entry
• base monotonicity and generator preservation, without auto-blessing newly observed ownership

This is a cooperative static architecture guard, not a hostile-code proof against mechanisms such as eval. Future controller migrations should register their owner in controllerOwners; direct new stateful hooks in AppShell remain blocked by the existing AppShell hook/debt ratchets.

Verification

• npm run check:renderer-architecture -- --base upstream/main — 70/70 checker fixtures and real checkout passed
• npm run check:app-shell-hooks — 41 hooks / 79 call sites, exact inventory passed
• npm run typecheck
• npm run rebuild
• npm --workspace @maka/desktop run test:dist — 1,849/1,849 passed
• npm --workspace @maka/desktop run build-storybook
• npm --workspace @maka/desktop run smoke:storybook — 243/243 stories passed
• npm run lint
• npm run format:check
• npm run astryx:surface-inventory:write
• npm run astryx:surface-inventory — 236 files, 1 exclusion
• git diff --check
• git merge-tree --write-tree upstream/main HEAD

AI use

Select exactly one:

• No generative tool made a substantive contribution
• Generative tooling made a substantive contribution

Tool(s) and scope: OpenAI Codex designed and implemented the ownership boundary and guardrail, added tests, built and ran the temporary performance harness, performed independent code audits, and prepared this PR. Both commits include Generated-by: OpenAI Codex trailers.

Checklist

• Tests cover the change and fail without it
• Lint, format, typecheck and the affected suites pass locally

Does this PR entail a change in behavior?

• Yes — Module Hub state no longer re-renders AppShell-wide consumers; there is no intended user-visible UI change
• No

[chihumyum]

@Astro-Han Hi, can you take a look? Thanks.

[Astro-Han]

Working on it! The new head also need a rebase.

[Astro-Han]

Reviewed exact head a09ebabd7f5076acddb621dc00b5908d70eeb6bc (+1715/-62, 18 files). MERGEABLE; plan passes, heavy still running at the time of writing. Splitting the diff: the guard (check-renderer-architecture.mjs + its fixtures + renderer-architecture.json) is about 1,070 added lines, the renderer change and its coverage about 640.

Problem

Demonstrated, and demonstrated the right way. useModuleHubController was called in AppShellContent's render body, so in React terms its scope was the whole tree. The paired Electron/CDP run and the deterministic render-count test say the same thing from two directions, and the description is careful to bound the wall-clock number rather than generalize it. The scenario the test pins — a Scheduled Tasks change rendering the shell and an unrelated subtree once, versus zero after scoping — is the scenario the problem statement describes.

It also lands where the existing gate said it should. scripts/check-app-shell-hooks.mjs prescribes this exact shape in its own header — the call site moves into a provider, React bails out of children, the entry disappears — and warns that cross-feature intent the shell issues must stay an explicit command rather than become an implicit subscription bought to lower a number. The command port is that explicit command, and the hook entry disappears rather than shrinking. This is the prescribed migration, not a new interpretation of it.

Solution

I traced the bail-out rather than taking it on trust, and it holds. ModuleHubProvider receives AppShell's frame as children, so on a Module Hub state change the three context Providers are rebuilt with the same children element reference and React skips that subtree; only the actual context consumers wake. Each reader reads exactly one context, so a Scheduled Tasks change does not wake the skill-revision boundary and vice versa. ModuleHubHost reads the host model at its own position instead of receiving it from the shell.

The command port's connect returning () => { if (target === next) target = null } is the right guard, and it is load-bearing: controller.commands is nested inside the controller's single useMemo, whose deps include input.openSession — a fresh arrow on every AppShellContent render — so the connect effect re-runs on every shell render. Cleanup-then-connect ordering makes that churn harmless, but it is churn that only that identity check keeps safe. Worth a comment at createModuleHubCommandPort saying so; the reason it is written that way is not obvious from the code.

Findings

P3 — two required props became optional-with-default and are now injected invisibly to the type checker. ComposerMentionsSurface.skillCatalogRevision goes from number to number? with a = 0 default at the reader, and scheduledTasks leaves SessionNavigationProvider's call site; both arrive through cloneElement. ReactElement<SkillCatalogRevisionTargetProps> constrains the child only to accept an optional prop, so the boundary would type-check against almost any element, and a future edit that drops or reorders the wrapper compiles cleanly and silently pins the composer's Skills projection at revision 0 — an installed Skill would not appear in @ until restart. Not reachable today: each provider has exactly one mount site, and module-hub-boundary.test.ts asserts the wrapper's presence by string match. But the guarantee moved from the compiler to a string-matching test, which is a downgrade in a PR whose subject is making ownership provable. Smallest fix: keep the props required and have the boundary render a typed wrapper, or export useModuleHubScheduledTasks / useModuleHubSkillCatalogRevision and let each reader call it — which removes cloneElement and both boundary components outright. The second is more invasive because it points composer-mentions.tsx at the feature's public entry; I would still prefer it, but the call is yours.

P3 — one invariant is now asserted by three mechanisms, and this PR adds to the weakest. That the controller is called once, inside ModuleHubProvider, through a direct import, and is absent from the public entry, is now checked by the new controllerOwners policy; by the existing exact-inventory hook gate, which already fails if useModuleHubController reappears in AppShellContent; and by three new exact-string assertions added here to module-hub-boundary.test.ts (const controller = useModuleHubController(input);, commandPort.connect(controller.commands), productionEntry.includes('useModuleHubController')). The first of those breaks on a parameter rename or a formatter change while proving strictly less than the checker does. If controllerOwners is the authority for provenance, those three assertions should not land with it.

P3 — a third public seam for one symbol. stories.ts re-exports two fakes from testing.ts and adds ModuleHubHostView, and the feature-boundary regex widens from index|testing to index|stories|testing to permit it. Exporting ModuleHubHostView from testing.ts gets the same story coverage with two seams instead of three and leaves the boundary rule untouched. If the intent is that stories must not reach the fakes-plus-controller surface, say so in stories.ts — as written the file reads as a convenience barrel.

On the guard's size

The atomicity argument is sound: a contract with no registered owner proves nothing, and an owner with no contract can be undone silently. I am not asking for a split. But I want to name what the 1,070 lines buy, because the registry has one entry and the description itself frames this as a cooperative guard rather than a proof against hostile code. Under that framing, the evasion classes divide: namespace imports, barrel re-exports, and private intermediaries are things a cooperative author does by habit, so covering them is real value; eval and runtime loads are not, and the checker's own framing concedes it. Registering owner two costs five lines of JSON, which is the leverage — I would just rather see the description claim that leverage than the evasion breadth.

Next step

This is ready. None of the three findings is a reason to hold the migration, and I would rather this land and unblock the next owner in #3439 than trade a demonstrated improvement for P3 polish. Take the first one if you touch the branch again — it is the only one that trades a compiler guarantee for a string match, inside a change whose whole subject is provability — and treat the other two as cleanup you can fold into the next controller migration, when the second registered owner will show whether the duplication is worth carrying.

The one thing still outstanding is mechanical: heavy was running when I finished, so merge on a green run of a09ebabd7. My evidence boundary: I read validateControllerOwners and the config schema closely and spot-checked the fixtures, but I did not audit all 70, and I did not re-run your Electron benchmark.

---

AI-assisted review: I used Maka to split and read the diff, to trace the context bail-out and the command-port lifecycle against the branch's own source, and to draft this comment. I verified the render-scope mechanism, the useMemo dependency that makes the connect effect re-run, the single mount site of each provider, and the overlap between the three checking mechanisms myself. I did not re-run your Electron benchmark and did not audit all 70 checker fixtures; I own everything above.

[chihumyum]

@Astro-Han Rebased onto current main b714a39 and resolved the generated renderer-architecture/Astryx inventory conflicts. The new exact head is e95a965.

Local validation passed: renderer architecture 80/80 against upstream/main, AppShell hook scope, Astryx inventory and fixtures, format, lint, full workspace typecheck, Desktop production build, and the Module Hub boundary/provider tests 6/6. Could you take another look when the new CI run settles? Thanks.

[chihumyum]

Follow-up to the review, pushed as a separate commit on top of the rebase onto main (c86da40d7). Exact head: b8152e2fd611a2559d6ba0876a0a94e0cbc15b55.

• P3-1 (cloneElement): both boundaries now take a typed render(value, children) prop instead of cloning their child. The Session rail site passes scheduledTasks to SessionNavigationProvider explicitly; the Composer site delegates to renderComposerMentionsProvider(surface) in composer-mentions.tsx, where skillCatalogRevision is a required prop again (the = 0 default is gone). Dropping either wire is now a compile error. children is the element AppShell already built and is forwarded untouched, so the bail-out you traced is unchanged; module-hub-provider.test.ts renders the boundaries the same way.
• P3-2 (three mechanisms): the two provider.includes(...) source-string assertions and the productionEntry.includes('useModuleHubController') check are gone from module-hub-boundary.test.ts; controllerOwners is the provenance authority. The boundary-presence string checks went with them, since the compiler now enforces that wiring.
• P3-3 (third seam): stories.ts is deleted. ModuleHubHostView could not move to testing.ts: the Desktop node suite imports testing.js, and module-hub-host pulls page modules that dist/main does not carry, so every module-hub test failed to load. It is exported from the public entry next to ModuleHubHost instead; the story takes the view from index and the fakes from testing, and the boundary regex is back to index|testing.
• The comment you asked for is on createModuleHubCommandPort, saying why the cleanup's identity check is load-bearing.

Two small changes keep app-shell.tsx under the token ratchet after the typed wiring (main 15,725; now 15,712): openSession is the shell's existing stable openSessionInChat callback rather than a fresh arrow, and the conditional spread for openSkillsFolder is a plain conditional prop. Behavior is unchanged. The arrow was one of the inputs that re-ran the connect effect on every shell render; appendComposerText still is, which is why the guard comment stays.

Local verification on this head with Node 24: desktop test:dist 1973/1973, typecheck (four projects), lint, format, check:renderer-architecture --base upstream/main, check:app-shell-hooks (41 hooks / 77 call sites), Astryx inventory, Knip, ASF headers, git diff --check — all green.

Automated update from Claude Code on behalf of the PR author.

[chihumyum]

Rebased again onto current main (ad18da42c, #4455) and regenerated the Astryx inventory, which was the only conflict; nothing else changed. Exact head: 0ddea0d3213b71d44e9a71c43b77411e5caf0811.

Same local verification on this head with Node 24: desktop test:dist, typecheck (four projects), lint, format, check:renderer-architecture --base upstream/main, check:app-shell-hooks (41 hooks / 77 call sites), Astryx inventory, Knip, ASF headers, git diff --check — all green.

Automated update from Claude Code on behalf of the PR author.

[chihumyum]

@Astro-Han could you take a look at the follow-up when you have a moment? This PR gates #4491 and #4498 (both register in controllerOwners once it lands), so it is the one to merge first.

Since your approval the branch gained one commit, 5efcc07a2 ("type the Module Hub reader boundaries"), which addresses the three P3s: typed render props replace cloneElement on both boundaries (readers' props are required again), the duplicated source-string provenance assertions are gone in favour of controllerOwners, and stories.ts is deleted with ModuleHubHostView exported from the public entry (details in my earlier comment above). Everything else is rebases: the branch now sits on main 68cda0b54 with the ledger and Astryx inventory regenerated. Exact head: 5efcc07a2c5f8066f3ea6b4649b24d466f28756f.

Local verification on this head with Node 24: desktop test:dist, typecheck (four projects), lint, format, check:renderer-architecture --base upstream/main, check:app-shell-hooks, Astryx inventory, Knip, ASF headers, git diff --check — all green.

Posted by Claude Code on behalf of the PR author.

[chihumyum]

Rebased once more onto main b9748a773 (#4576 landed right after the previous push; the ledger was the only conflict and was regenerated). Exact head: 646766361c70e73ebefa1a891073a8f7f9226a40; the full local verification is green on it. The follow-up commit to look at is unchanged in content.

Posted by Claude Code on behalf of the PR author.

[chihumyum]

The run on 646766361 failed only in Desktop e2e, on prompt-rail.spec.ts:370 ("manual transcript scrolling keeps exactly the visible prompt current"), with the rest of the job green. That spec exercises transcript scrolling, which this PR does not touch, and it was changed on main today by #4417. On this head the whole file passes locally three times in a row (--repeat-each 3, 33/33). Re-pushed with identical content to re-run the job; exact head f1eec1537a7b0a71a2be98b784ba7916f3e8d6d5.

Posted by Claude Code on behalf of the PR author.