refactor(desktop): extract goals as a feature slice

[chihumyum]

Summary

• Move Goal read state, subscriptions, controls, dialog ownership, and indicator assembly behind a renderer feature boundary.
• Isolate Desktop bridge access behind a narrow GoalServices adapter and provider; keep AppShell limited to Session context plus host, commands, and selectors.
• Add fake services and focused lifecycle, adapter, model, and architecture-boundary coverage.

Refs #3439

Verification

• npm run lint
• npm run format:check
• npm run build
• npm run typecheck
• npm --workspace @maka/desktop run test:dist — 1198 passed
• npx knip --workspace apps/desktop
• npx knip --workspace packages/ui
• npm --workspace @maka/desktop run build-storybook
• npm --workspace @maka/desktop run smoke:storybook — 166 stories passed
• npx playwright test --config e2e/playwright.config.ts from apps/desktop — 50 passed, 1 skipped

No screenshots are included because this changes no Goal copy, CSS, layout, IPC contract, or user-visible behavior.

Review focus

• Session-switch fencing for stale Goal reads.
• Goal-change event filtering and subscription disposal.
• Dialog ownership of the Session captured when it opens.
• Per-Session pause/resume re-entry protection.

AI use

Select exactly one:

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

Tool(s) and scope: Codex assisted with the feature extraction, implementation, tests, and verification.

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 — described under Summary above
• No

[Astro-Han]

Reviewed at head b29e3c4e. One blocking item, and it is a generated file rather than the code — the extraction itself came through clean.

[P1] The Astryx surface inventory wasn't regenerated, and that's what's failing test.

test is FAILURE on this exact head (job 97083453961), and I reproduced it locally: npm run astryx:surface-inventory exits 1 on this branch, consistently. Three moved files are on disk but absent from docs/astryx-surface-file-inventory.paths — apps/desktop/src/renderer/features/goals/ui/goal-dialog.tsx, plus services-context.tsx and goal-host.tsx in the same directory — while the pre-move apps/desktop/src/renderer/goal-dialog.tsx is still listed. docs/astryx-surface-file-inventory.md is likewise out of sync with the generator's output.

Fix is mechanical: run npm run astryx:surface-inventory:write and commit both regenerated files. Nothing about the runtime behaviour is at fault here — the gate is doing its job, the branch just moved surfaces without re-running it.

---

On the refactor itself: no P0–P3.

For a pure extraction the question is only ever whether behaviour changed under cover of moving things, so that is where I spent the time. use-goal-controller.ts ends up holding the active goal, session-switch clearing, the late-read sequence and disposed guards, the goal-change subscription, per-session control dedupe, and the dialog's session snapshot — and it stays a projection of Host events rather than becoming a second authority, which is the failure mode this kind of slice usually introduces. create-goal-services.ts is the single Desktop bridge adapter, and AppShell only reaches Goals through GoalHost and the controller's public API.

The deletion side checks out too, which is the part that makes an extraction actually reduce entropy rather than add a layer: use-session-goal.ts is gone, the old dialog's direct window.maka dependency is gone, and AppShell no longer carries goal control state. A repo-wide search finds no useSessionGoal and no surviving production entry to the old dialog path.

Verified: Goals focused tests 12/12; controller, model, adapter, and feature-boundary suites all pass; @maka/desktop builds and typechecks across renderer, preload, main, and Storybook; Biome clean on changed files. The full desktop main suite ran 1190 pass / 0 fail with 8 cancelled — the cancellations sit in pre-existing MCP OAuth pending tests and are not attributable to this branch, so I'm reporting it that way rather than calling the suite green.

Regenerate the inventory and this is ready for another look.

[chihumyum]

Fixed in f9554a9af by regenerating both checked-in Astryx surface inventory files.

Local verification:

• npm run astryx:surface-inventory:write
• npm run astryx:surface-inventory — 203 files, 1 exclusion
• git diff --check

The new CI run is currently action_required with no jobs, so it needs maintainer approval before GitHub will execute the workflow.

[Astro-Han]

Two independent lines on f9554a9af129f6bede8b7975c0a4bb9a2a0cc3be, from different models, neither able to see the other's conclusion. Both came back with nothing at P0–P3, and — more importantly for a refactor — both answered the question that actually matters here.

A refactor's failure mode isn't "didn't fix it", it's "changed behaviour in transit", and in a diff a relocation and a rewrite look identical. So both lines were asked to split -263 into what genuinely disappeared versus what is just move noise.

What disappeared (~150 lines):

• use-session-goal.ts is gone as a file, and its authority — goal state, the change subscription, session-switch semantics — moved wholesale to the controller rather than being copied.
• The indicator construction inlined in app-shell.tsx (~55 lines), plus runGoalControl, pendingGoalControlSessionIdsRef and goalDialogSessionId, all leave the shell. AppShell keeps two lines of wiring.

That is one ownership leaving one place and arriving in another, not a copy. Move noise (~110 lines) is the dialog relocating into features/goals/ui/ and isLiveGoal moving to model/live-goal.ts.

The controller being larger than the hook it replaces is explained rather than hand-waved: it absorbed the behaviour that used to live inline in the shell, so the two are not comparable line-for-line.

Semantic parity was checked point by point rather than asserted: subscription filtering (including the broadcast undefined case), session-switch ordering, the sequence-number race guard, control re-entrancy, dialog keying against the wrong session, and the GoalState type resolving to the same underlying type through a different path. One behavioural difference was found and it is an improvement: the indicator moves from an IIFE rebuilding a closure every render to a memo keyed on the active goal, so the reference is now stable.

No second implementation survives. We verified this at the gate independently: window.maka.goal has zero hits in production code on this head, use-session-goal.ts no longer exists in the tree, and the only remaining references to the old surface are stories pointing at the deliberate testing export.

One thing worth recording without grading it: the adapter's subscribeChanges collapses events down to sessionId | undefined, discarding metadata beyond the reason. That is the right boundary for what goals need today, but if goals ever need another session reason — a removal, say — the adapter is where that has to be reopened. Noting it as a known edge of the seam, not as a defect.

CI is terminal green on this head (test: completed / success), which is the head this approval is bound to.

Approving.

[chihumyum]

Rebased onto current upstream/main (729839ed8) and resolved the GoalDialog conflict in cfce73ee9.

The resolution preserves the newly merged reconnect reconciliation behavior: Goal arm outcomes still fence stale results, lock reconciled states, and prevent duplicate submission, while the bridge call remains behind the Goals service port. The reconciliation model and its tests now live inside the Goals feature boundary.

Verified after the final rebase:

• npm run lint
• npm run format:check
• npm run astryx:surface-inventory
• npm run build
• npm run typecheck
• npm --workspace @maka/desktop run test:dist — 1255 passed
• npx knip --workspace apps/desktop

GitHub now reports the PR as mergeable. The new fork CI run is action_required and needs maintainer approval before jobs start.

[chihumyum]

Rechecked against the latest upstream/main (047567c4b). Main had advanced by five commits and the PR had become DIRTY solely because the generated Astryx inventory conflicted again.

Rebased and regenerated the inventory in 0ed2c53c4. Verification:

• npm run astryx:surface-inventory — 204 files, 1 exclusion
• git diff --check upstream/main...HEAD
• merge-tree probe against current main — clean

GitHub no longer reports a merge conflict. The current head workflow is run 32630272181, which is action_required with no jobs and needs maintainer approval to execute.

[jackwener]

Incremental technical review of exact head 0ed2c53c4bddef7c97434ad111541ea7111c33d8, executed by the Kabi review agent on behalf of @WAWQAQ under the standing Kabi review/publisher authorization.

GO — no live P0–P3. APPROVE; no merge action.

This review covers the rebase-only delta that incorporated the newly merged Goal arm reconciliation contract. Comparing the pre-extraction implementation at current base 047567c4b with the moved feature code shows semantic parity: only armed closes the dialog; matching/different/null reconciled results and reconciliation_unavailable all lock the form, show the authoritative outcome, and prevent duplicate submission. interpretGoalArmOutcome is unchanged apart from its relative import.

The bridge authority remains singular: main.tsx creates one Desktop Goal service adapter, its bridge.goal.arm result is returned through the service and controller as Promise<GoalArmOutcome>, and GoalHost injects that exact function into GoalDialog. The Goals feature contains no window.maka.goal access and the old direct production path is absent.

Verification: focused Goals suites 18/18; Desktop main 1273/1274 on the first full run, with the sole unrelated shell-env timeout fixture passing 12/12 when rerun alone; Desktop preload/main/renderer/Storybook typechecks passed. Three discussion endpoints were reconciled after provisional sealing: two old-head reviews, no inline comments, and three author status comments; no live P0–P2.

Fresh action gate: OPEN, non-draft, MERGEABLE/CLEAN; exact-head hosted test is completed/success. This Agent-executed approval does not claim to satisfy any separate unresolved human-review wording.

[chihumyum]

Summary

• Move Goal read state, subscriptions, controls, dialog ownership, and indicator assembly behind a renderer feature boundary.
• Isolate Desktop bridge access behind a narrow GoalServices adapter and provider; keep AppShell limited to Session context plus host, commands, and selectors.
• Add fake services and focused lifecycle, adapter, model, and architecture-boundary coverage.

Refs #3439

Verification

• npm run lint
• npm run format:check
• npm run build
• npm run typecheck
• npm --workspace @maka/desktop run test:dist — 1198 passed
• npx knip --workspace apps/desktop
• npx knip --workspace packages/ui
• npm --workspace @maka/desktop run build-storybook
• npm --workspace @maka/desktop run smoke:storybook — 166 stories passed
• npx playwright test --config e2e/playwright.config.ts from apps/desktop — 50 passed, 1 skipped

No screenshots are included because this changes no Goal copy, CSS, layout, IPC contract, or user-visible behavior.

Review focus

• Session-switch fencing for stale Goal reads.
• Goal-change event filtering and subscription disposal.
• Dialog ownership of the Session captured when it opens.
• Per-Session pause/resume re-entry protection.

AI use

Select exactly one:

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

Tool(s) and scope: Codex assisted with the feature extraction, implementation, tests, and verification.

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 — described under Summary above
• No

[Astro-Han]

Reviewed at head b29e3c4e. One blocking item, and it is a generated file rather than the code — the extraction itself came through clean.

[P1] The Astryx surface inventory wasn't regenerated, and that's what's failing test.

test is FAILURE on this exact head (job 97083453961), and I reproduced it locally: npm run astryx:surface-inventory exits 1 on this branch, consistently. Three moved files are on disk but absent from docs/astryx-surface-file-inventory.paths — apps/desktop/src/renderer/features/goals/ui/goal-dialog.tsx, plus services-context.tsx and goal-host.tsx in the same directory — while the pre-move apps/desktop/src/renderer/goal-dialog.tsx is still listed. docs/astryx-surface-file-inventory.md is likewise out of sync with the generator's output.

Fix is mechanical: run npm run astryx:surface-inventory:write and commit both regenerated files. Nothing about the runtime behaviour is at fault here — the gate is doing its job, the branch just moved surfaces without re-running it.

---

On the refactor itself: no P0–P3.

For a pure extraction the question is only ever whether behaviour changed under cover of moving things, so that is where I spent the time. use-goal-controller.ts ends up holding the active goal, session-switch clearing, the late-read sequence and disposed guards, the goal-change subscription, per-session control dedupe, and the dialog's session snapshot — and it stays a projection of Host events rather than becoming a second authority, which is the failure mode this kind of slice usually introduces. create-goal-services.ts is the single Desktop bridge adapter, and AppShell only reaches Goals through GoalHost and the controller's public API.

The deletion side checks out too, which is the part that makes an extraction actually reduce entropy rather than add a layer: use-session-goal.ts is gone, the old dialog's direct window.maka dependency is gone, and AppShell no longer carries goal control state. A repo-wide search finds no useSessionGoal and no surviving production entry to the old dialog path.

Verified: Goals focused tests 12/12; controller, model, adapter, and feature-boundary suites all pass; @maka/desktop builds and typechecks across renderer, preload, main, and Storybook; Biome clean on changed files. The full desktop main suite ran 1190 pass / 0 fail with 8 cancelled — the cancellations sit in pre-existing MCP OAuth pending tests and are not attributable to this branch, so I'm reporting it that way rather than calling the suite green.

Regenerate the inventory and this is ready for another look.

[chihumyum]

Fixed in f9554a9af by regenerating both checked-in Astryx surface inventory files.

Local verification:

• npm run astryx:surface-inventory:write
• npm run astryx:surface-inventory — 203 files, 1 exclusion
• git diff --check

The new CI run is currently action_required with no jobs, so it needs maintainer approval before GitHub will execute the workflow.

[Astro-Han]

Two independent lines on f9554a9af129f6bede8b7975c0a4bb9a2a0cc3be, from different models, neither able to see the other's conclusion. Both came back with nothing at P0–P3, and — more importantly for a refactor — both answered the question that actually matters here.

A refactor's failure mode isn't "didn't fix it", it's "changed behaviour in transit", and in a diff a relocation and a rewrite look identical. So both lines were asked to split -263 into what genuinely disappeared versus what is just move noise.

What disappeared (~150 lines):

• use-session-goal.ts is gone as a file, and its authority — goal state, the change subscription, session-switch semantics — moved wholesale to the controller rather than being copied.
• The indicator construction inlined in app-shell.tsx (~55 lines), plus runGoalControl, pendingGoalControlSessionIdsRef and goalDialogSessionId, all leave the shell. AppShell keeps two lines of wiring.

That is one ownership leaving one place and arriving in another, not a copy. Move noise (~110 lines) is the dialog relocating into features/goals/ui/ and isLiveGoal moving to model/live-goal.ts.

The controller being larger than the hook it replaces is explained rather than hand-waved: it absorbed the behaviour that used to live inline in the shell, so the two are not comparable line-for-line.

Semantic parity was checked point by point rather than asserted: subscription filtering (including the broadcast undefined case), session-switch ordering, the sequence-number race guard, control re-entrancy, dialog keying against the wrong session, and the GoalState type resolving to the same underlying type through a different path. One behavioural difference was found and it is an improvement: the indicator moves from an IIFE rebuilding a closure every render to a memo keyed on the active goal, so the reference is now stable.

No second implementation survives. We verified this at the gate independently: window.maka.goal has zero hits in production code on this head, use-session-goal.ts no longer exists in the tree, and the only remaining references to the old surface are stories pointing at the deliberate testing export.

One thing worth recording without grading it: the adapter's subscribeChanges collapses events down to sessionId | undefined, discarding metadata beyond the reason. That is the right boundary for what goals need today, but if goals ever need another session reason — a removal, say — the adapter is where that has to be reopened. Noting it as a known edge of the seam, not as a defect.

CI is terminal green on this head (test: completed / success), which is the head this approval is bound to.

Approving.

[chihumyum]

Rebased onto current upstream/main (729839ed8) and resolved the GoalDialog conflict in cfce73ee9.

The resolution preserves the newly merged reconnect reconciliation behavior: Goal arm outcomes still fence stale results, lock reconciled states, and prevent duplicate submission, while the bridge call remains behind the Goals service port. The reconciliation model and its tests now live inside the Goals feature boundary.

Verified after the final rebase:

• npm run lint
• npm run format:check
• npm run astryx:surface-inventory
• npm run build
• npm run typecheck
• npm --workspace @maka/desktop run test:dist — 1255 passed
• npx knip --workspace apps/desktop

GitHub now reports the PR as mergeable. The new fork CI run is action_required and needs maintainer approval before jobs start.

[chihumyum]

Rechecked against the latest upstream/main (047567c4b). Main had advanced by five commits and the PR had become DIRTY solely because the generated Astryx inventory conflicted again.

Rebased and regenerated the inventory in 0ed2c53c4. Verification:

• npm run astryx:surface-inventory — 204 files, 1 exclusion
• git diff --check upstream/main...HEAD
• merge-tree probe against current main — clean

GitHub no longer reports a merge conflict. The current head workflow is run 32630272181, which is action_required with no jobs and needs maintainer approval to execute.

[jackwener]

Incremental technical review of exact head 0ed2c53c4bddef7c97434ad111541ea7111c33d8, executed by the Kabi review agent on behalf of @WAWQAQ under the standing Kabi review/publisher authorization.

GO — no live P0–P3. APPROVE; no merge action.

This review covers the rebase-only delta that incorporated the newly merged Goal arm reconciliation contract. Comparing the pre-extraction implementation at current base 047567c4b with the moved feature code shows semantic parity: only armed closes the dialog; matching/different/null reconciled results and reconciliation_unavailable all lock the form, show the authoritative outcome, and prevent duplicate submission. interpretGoalArmOutcome is unchanged apart from its relative import.

The bridge authority remains singular: main.tsx creates one Desktop Goal service adapter, its bridge.goal.arm result is returned through the service and controller as Promise<GoalArmOutcome>, and GoalHost injects that exact function into GoalDialog. The Goals feature contains no window.maka.goal access and the old direct production path is absent.

Verification: focused Goals suites 18/18; Desktop main 1273/1274 on the first full run, with the sole unrelated shell-env timeout fixture passing 12/12 when rerun alone; Desktop preload/main/renderer/Storybook typechecks passed. Three discussion endpoints were reconciled after provisional sealing: two old-head reviews, no inline comments, and three author status comments; no live P0–P2.

Fresh action gate: OPEN, non-draft, MERGEABLE/CLEAN; exact-head hosted test is completed/success. This Agent-executed approval does not claim to satisfy any separate unresolved human-review wording.

[chihumyum]

Summary

• Move Goal read state, subscriptions, controls, dialog ownership, and indicator assembly behind a renderer feature boundary.
• Isolate Desktop bridge access behind a narrow GoalServices adapter and provider; keep AppShell limited to Session context plus host, commands, and selectors.
• Add fake services and focused lifecycle, adapter, model, and architecture-boundary coverage.

Refs #3439

Verification

• npm run lint
• npm run format:check
• npm run build
• npm run typecheck
• npm --workspace @maka/desktop run test:dist — 1198 passed
• npx knip --workspace apps/desktop
• npx knip --workspace packages/ui
• npm --workspace @maka/desktop run build-storybook
• npm --workspace @maka/desktop run smoke:storybook — 166 stories passed
• npx playwright test --config e2e/playwright.config.ts from apps/desktop — 50 passed, 1 skipped

No screenshots are included because this changes no Goal copy, CSS, layout, IPC contract, or user-visible behavior.

Review focus

• Session-switch fencing for stale Goal reads.
• Goal-change event filtering and subscription disposal.
• Dialog ownership of the Session captured when it opens.
• Per-Session pause/resume re-entry protection.

AI use

Select exactly one:

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

Tool(s) and scope: Codex assisted with the feature extraction, implementation, tests, and verification.

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 — described under Summary above
• No

[Astro-Han]

Reviewed at head b29e3c4e. One blocking item, and it is a generated file rather than the code — the extraction itself came through clean.

[P1] The Astryx surface inventory wasn't regenerated, and that's what's failing test.

test is FAILURE on this exact head (job 97083453961), and I reproduced it locally: npm run astryx:surface-inventory exits 1 on this branch, consistently. Three moved files are on disk but absent from docs/astryx-surface-file-inventory.paths — apps/desktop/src/renderer/features/goals/ui/goal-dialog.tsx, plus services-context.tsx and goal-host.tsx in the same directory — while the pre-move apps/desktop/src/renderer/goal-dialog.tsx is still listed. docs/astryx-surface-file-inventory.md is likewise out of sync with the generator's output.

Fix is mechanical: run npm run astryx:surface-inventory:write and commit both regenerated files. Nothing about the runtime behaviour is at fault here — the gate is doing its job, the branch just moved surfaces without re-running it.

---

On the refactor itself: no P0–P3.

For a pure extraction the question is only ever whether behaviour changed under cover of moving things, so that is where I spent the time. use-goal-controller.ts ends up holding the active goal, session-switch clearing, the late-read sequence and disposed guards, the goal-change subscription, per-session control dedupe, and the dialog's session snapshot — and it stays a projection of Host events rather than becoming a second authority, which is the failure mode this kind of slice usually introduces. create-goal-services.ts is the single Desktop bridge adapter, and AppShell only reaches Goals through GoalHost and the controller's public API.

The deletion side checks out too, which is the part that makes an extraction actually reduce entropy rather than add a layer: use-session-goal.ts is gone, the old dialog's direct window.maka dependency is gone, and AppShell no longer carries goal control state. A repo-wide search finds no useSessionGoal and no surviving production entry to the old dialog path.

Verified: Goals focused tests 12/12; controller, model, adapter, and feature-boundary suites all pass; @maka/desktop builds and typechecks across renderer, preload, main, and Storybook; Biome clean on changed files. The full desktop main suite ran 1190 pass / 0 fail with 8 cancelled — the cancellations sit in pre-existing MCP OAuth pending tests and are not attributable to this branch, so I'm reporting it that way rather than calling the suite green.

Regenerate the inventory and this is ready for another look.

[chihumyum]

Fixed in f9554a9af by regenerating both checked-in Astryx surface inventory files.

Local verification:

• npm run astryx:surface-inventory:write
• npm run astryx:surface-inventory — 203 files, 1 exclusion
• git diff --check

The new CI run is currently action_required with no jobs, so it needs maintainer approval before GitHub will execute the workflow.

[Astro-Han]

Two independent lines on f9554a9af129f6bede8b7975c0a4bb9a2a0cc3be, from different models, neither able to see the other's conclusion. Both came back with nothing at P0–P3, and — more importantly for a refactor — both answered the question that actually matters here.

A refactor's failure mode isn't "didn't fix it", it's "changed behaviour in transit", and in a diff a relocation and a rewrite look identical. So both lines were asked to split -263 into what genuinely disappeared versus what is just move noise.

What disappeared (~150 lines):

• use-session-goal.ts is gone as a file, and its authority — goal state, the change subscription, session-switch semantics — moved wholesale to the controller rather than being copied.
• The indicator construction inlined in app-shell.tsx (~55 lines), plus runGoalControl, pendingGoalControlSessionIdsRef and goalDialogSessionId, all leave the shell. AppShell keeps two lines of wiring.

That is one ownership leaving one place and arriving in another, not a copy. Move noise (~110 lines) is the dialog relocating into features/goals/ui/ and isLiveGoal moving to model/live-goal.ts.

The controller being larger than the hook it replaces is explained rather than hand-waved: it absorbed the behaviour that used to live inline in the shell, so the two are not comparable line-for-line.

Semantic parity was checked point by point rather than asserted: subscription filtering (including the broadcast undefined case), session-switch ordering, the sequence-number race guard, control re-entrancy, dialog keying against the wrong session, and the GoalState type resolving to the same underlying type through a different path. One behavioural difference was found and it is an improvement: the indicator moves from an IIFE rebuilding a closure every render to a memo keyed on the active goal, so the reference is now stable.

No second implementation survives. We verified this at the gate independently: window.maka.goal has zero hits in production code on this head, use-session-goal.ts no longer exists in the tree, and the only remaining references to the old surface are stories pointing at the deliberate testing export.

One thing worth recording without grading it: the adapter's subscribeChanges collapses events down to sessionId | undefined, discarding metadata beyond the reason. That is the right boundary for what goals need today, but if goals ever need another session reason — a removal, say — the adapter is where that has to be reopened. Noting it as a known edge of the seam, not as a defect.

CI is terminal green on this head (test: completed / success), which is the head this approval is bound to.

Approving.

[chihumyum]

Rebased onto current upstream/main (729839ed8) and resolved the GoalDialog conflict in cfce73ee9.

The resolution preserves the newly merged reconnect reconciliation behavior: Goal arm outcomes still fence stale results, lock reconciled states, and prevent duplicate submission, while the bridge call remains behind the Goals service port. The reconciliation model and its tests now live inside the Goals feature boundary.

Verified after the final rebase:

• npm run lint
• npm run format:check
• npm run astryx:surface-inventory
• npm run build
• npm run typecheck
• npm --workspace @maka/desktop run test:dist — 1255 passed
• npx knip --workspace apps/desktop

GitHub now reports the PR as mergeable. The new fork CI run is action_required and needs maintainer approval before jobs start.

[chihumyum]

Rechecked against the latest upstream/main (047567c4b). Main had advanced by five commits and the PR had become DIRTY solely because the generated Astryx inventory conflicted again.

Rebased and regenerated the inventory in 0ed2c53c4. Verification:

• npm run astryx:surface-inventory — 204 files, 1 exclusion
• git diff --check upstream/main...HEAD
• merge-tree probe against current main — clean

GitHub no longer reports a merge conflict. The current head workflow is run 32630272181, which is action_required with no jobs and needs maintainer approval to execute.

[jackwener]

Incremental technical review of exact head 0ed2c53c4bddef7c97434ad111541ea7111c33d8, executed by the Kabi review agent on behalf of @WAWQAQ under the standing Kabi review/publisher authorization.

GO — no live P0–P3. APPROVE; no merge action.

This review covers the rebase-only delta that incorporated the newly merged Goal arm reconciliation contract. Comparing the pre-extraction implementation at current base 047567c4b with the moved feature code shows semantic parity: only armed closes the dialog; matching/different/null reconciled results and reconciliation_unavailable all lock the form, show the authoritative outcome, and prevent duplicate submission. interpretGoalArmOutcome is unchanged apart from its relative import.

The bridge authority remains singular: main.tsx creates one Desktop Goal service adapter, its bridge.goal.arm result is returned through the service and controller as Promise<GoalArmOutcome>, and GoalHost injects that exact function into GoalDialog. The Goals feature contains no window.maka.goal access and the old direct production path is absent.

Verification: focused Goals suites 18/18; Desktop main 1273/1274 on the first full run, with the sole unrelated shell-env timeout fixture passing 12/12 when rerun alone; Desktop preload/main/renderer/Storybook typechecks passed. Three discussion endpoints were reconciled after provisional sealing: two old-head reviews, no inline comments, and three author status comments; no live P0–P2.

Fresh action gate: OPEN, non-draft, MERGEABLE/CLEAN; exact-head hosted test is completed/success. This Agent-executed approval does not claim to satisfy any separate unresolved human-review wording.

[chihumyum]

Summary

• Move Goal read state, subscriptions, controls, dialog ownership, and indicator assembly behind a renderer feature boundary.
• Isolate Desktop bridge access behind a narrow GoalServices adapter and provider; keep AppShell limited to Session context plus host, commands, and selectors.
• Add fake services and focused lifecycle, adapter, model, and architecture-boundary coverage.

Refs #3439

Verification

• npm run lint
• npm run format:check
• npm run build
• npm run typecheck
• npm --workspace @maka/desktop run test:dist — 1198 passed
• npx knip --workspace apps/desktop
• npx knip --workspace packages/ui
• npm --workspace @maka/desktop run build-storybook
• npm --workspace @maka/desktop run smoke:storybook — 166 stories passed
• npx playwright test --config e2e/playwright.config.ts from apps/desktop — 50 passed, 1 skipped

No screenshots are included because this changes no Goal copy, CSS, layout, IPC contract, or user-visible behavior.

Review focus

• Session-switch fencing for stale Goal reads.
• Goal-change event filtering and subscription disposal.
• Dialog ownership of the Session captured when it opens.
• Per-Session pause/resume re-entry protection.

AI use

Select exactly one:

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

Tool(s) and scope: Codex assisted with the feature extraction, implementation, tests, and verification.

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 — described under Summary above
• No

[Astro-Han]

Reviewed at head b29e3c4e. One blocking item, and it is a generated file rather than the code — the extraction itself came through clean.

[P1] The Astryx surface inventory wasn't regenerated, and that's what's failing test.

test is FAILURE on this exact head (job 97083453961), and I reproduced it locally: npm run astryx:surface-inventory exits 1 on this branch, consistently. Three moved files are on disk but absent from docs/astryx-surface-file-inventory.paths — apps/desktop/src/renderer/features/goals/ui/goal-dialog.tsx, plus services-context.tsx and goal-host.tsx in the same directory — while the pre-move apps/desktop/src/renderer/goal-dialog.tsx is still listed. docs/astryx-surface-file-inventory.md is likewise out of sync with the generator's output.

Fix is mechanical: run npm run astryx:surface-inventory:write and commit both regenerated files. Nothing about the runtime behaviour is at fault here — the gate is doing its job, the branch just moved surfaces without re-running it.

---

On the refactor itself: no P0–P3.

For a pure extraction the question is only ever whether behaviour changed under cover of moving things, so that is where I spent the time. use-goal-controller.ts ends up holding the active goal, session-switch clearing, the late-read sequence and disposed guards, the goal-change subscription, per-session control dedupe, and the dialog's session snapshot — and it stays a projection of Host events rather than becoming a second authority, which is the failure mode this kind of slice usually introduces. create-goal-services.ts is the single Desktop bridge adapter, and AppShell only reaches Goals through GoalHost and the controller's public API.

The deletion side checks out too, which is the part that makes an extraction actually reduce entropy rather than add a layer: use-session-goal.ts is gone, the old dialog's direct window.maka dependency is gone, and AppShell no longer carries goal control state. A repo-wide search finds no useSessionGoal and no surviving production entry to the old dialog path.

Verified: Goals focused tests 12/12; controller, model, adapter, and feature-boundary suites all pass; @maka/desktop builds and typechecks across renderer, preload, main, and Storybook; Biome clean on changed files. The full desktop main suite ran 1190 pass / 0 fail with 8 cancelled — the cancellations sit in pre-existing MCP OAuth pending tests and are not attributable to this branch, so I'm reporting it that way rather than calling the suite green.

Regenerate the inventory and this is ready for another look.

[chihumyum]

Fixed in f9554a9af by regenerating both checked-in Astryx surface inventory files.

Local verification:

• npm run astryx:surface-inventory:write
• npm run astryx:surface-inventory — 203 files, 1 exclusion
• git diff --check

The new CI run is currently action_required with no jobs, so it needs maintainer approval before GitHub will execute the workflow.

[Astro-Han]

Two independent lines on f9554a9af129f6bede8b7975c0a4bb9a2a0cc3be, from different models, neither able to see the other's conclusion. Both came back with nothing at P0–P3, and — more importantly for a refactor — both answered the question that actually matters here.

A refactor's failure mode isn't "didn't fix it", it's "changed behaviour in transit", and in a diff a relocation and a rewrite look identical. So both lines were asked to split -263 into what genuinely disappeared versus what is just move noise.

What disappeared (~150 lines):

• use-session-goal.ts is gone as a file, and its authority — goal state, the change subscription, session-switch semantics — moved wholesale to the controller rather than being copied.
• The indicator construction inlined in app-shell.tsx (~55 lines), plus runGoalControl, pendingGoalControlSessionIdsRef and goalDialogSessionId, all leave the shell. AppShell keeps two lines of wiring.

That is one ownership leaving one place and arriving in another, not a copy. Move noise (~110 lines) is the dialog relocating into features/goals/ui/ and isLiveGoal moving to model/live-goal.ts.

The controller being larger than the hook it replaces is explained rather than hand-waved: it absorbed the behaviour that used to live inline in the shell, so the two are not comparable line-for-line.

Semantic parity was checked point by point rather than asserted: subscription filtering (including the broadcast undefined case), session-switch ordering, the sequence-number race guard, control re-entrancy, dialog keying against the wrong session, and the GoalState type resolving to the same underlying type through a different path. One behavioural difference was found and it is an improvement: the indicator moves from an IIFE rebuilding a closure every render to a memo keyed on the active goal, so the reference is now stable.

No second implementation survives. We verified this at the gate independently: window.maka.goal has zero hits in production code on this head, use-session-goal.ts no longer exists in the tree, and the only remaining references to the old surface are stories pointing at the deliberate testing export.

One thing worth recording without grading it: the adapter's subscribeChanges collapses events down to sessionId | undefined, discarding metadata beyond the reason. That is the right boundary for what goals need today, but if goals ever need another session reason — a removal, say — the adapter is where that has to be reopened. Noting it as a known edge of the seam, not as a defect.

CI is terminal green on this head (test: completed / success), which is the head this approval is bound to.

Approving.

[chihumyum]

Rebased onto current upstream/main (729839ed8) and resolved the GoalDialog conflict in cfce73ee9.

The resolution preserves the newly merged reconnect reconciliation behavior: Goal arm outcomes still fence stale results, lock reconciled states, and prevent duplicate submission, while the bridge call remains behind the Goals service port. The reconciliation model and its tests now live inside the Goals feature boundary.

Verified after the final rebase:

• npm run lint
• npm run format:check
• npm run astryx:surface-inventory
• npm run build
• npm run typecheck
• npm --workspace @maka/desktop run test:dist — 1255 passed
• npx knip --workspace apps/desktop

GitHub now reports the PR as mergeable. The new fork CI run is action_required and needs maintainer approval before jobs start.

[chihumyum]

Rechecked against the latest upstream/main (047567c4b). Main had advanced by five commits and the PR had become DIRTY solely because the generated Astryx inventory conflicted again.

Rebased and regenerated the inventory in 0ed2c53c4. Verification:

• npm run astryx:surface-inventory — 204 files, 1 exclusion
• git diff --check upstream/main...HEAD
• merge-tree probe against current main — clean

GitHub no longer reports a merge conflict. The current head workflow is run 32630272181, which is action_required with no jobs and needs maintainer approval to execute.

[jackwener]

Incremental technical review of exact head 0ed2c53c4bddef7c97434ad111541ea7111c33d8, executed by the Kabi review agent on behalf of @WAWQAQ under the standing Kabi review/publisher authorization.

GO — no live P0–P3. APPROVE; no merge action.

This review covers the rebase-only delta that incorporated the newly merged Goal arm reconciliation contract. Comparing the pre-extraction implementation at current base 047567c4b with the moved feature code shows semantic parity: only armed closes the dialog; matching/different/null reconciled results and reconciliation_unavailable all lock the form, show the authoritative outcome, and prevent duplicate submission. interpretGoalArmOutcome is unchanged apart from its relative import.

The bridge authority remains singular: main.tsx creates one Desktop Goal service adapter, its bridge.goal.arm result is returned through the service and controller as Promise<GoalArmOutcome>, and GoalHost injects that exact function into GoalDialog. The Goals feature contains no window.maka.goal access and the old direct production path is absent.

Verification: focused Goals suites 18/18; Desktop main 1273/1274 on the first full run, with the sole unrelated shell-env timeout fixture passing 12/12 when rerun alone; Desktop preload/main/renderer/Storybook typechecks passed. Three discussion endpoints were reconciled after provisional sealing: two old-head reviews, no inline comments, and three author status comments; no live P0–P2.

Fresh action gate: OPEN, non-draft, MERGEABLE/CLEAN; exact-head hosted test is completed/success. This Agent-executed approval does not claim to satisfy any separate unresolved human-review wording.

[chihumyum]

Summary

• Move Goal read state, subscriptions, controls, dialog ownership, and indicator assembly behind a renderer feature boundary.
• Isolate Desktop bridge access behind a narrow GoalServices adapter and provider; keep AppShell limited to Session context plus host, commands, and selectors.
• Add fake services and focused lifecycle, adapter, model, and architecture-boundary coverage.

Refs #3439

Verification

• npm run lint
• npm run format:check
• npm run build
• npm run typecheck
• npm --workspace @maka/desktop run test:dist — 1198 passed
• npx knip --workspace apps/desktop
• npx knip --workspace packages/ui
• npm --workspace @maka/desktop run build-storybook
• npm --workspace @maka/desktop run smoke:storybook — 166 stories passed
• npx playwright test --config e2e/playwright.config.ts from apps/desktop — 50 passed, 1 skipped

No screenshots are included because this changes no Goal copy, CSS, layout, IPC contract, or user-visible behavior.

Review focus

• Session-switch fencing for stale Goal reads.
• Goal-change event filtering and subscription disposal.
• Dialog ownership of the Session captured when it opens.
• Per-Session pause/resume re-entry protection.

AI use

Select exactly one:

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

Tool(s) and scope: Codex assisted with the feature extraction, implementation, tests, and verification.

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 — described under Summary above
• No

[Astro-Han]

Reviewed at head b29e3c4e. One blocking item, and it is a generated file rather than the code — the extraction itself came through clean.

[P1] The Astryx surface inventory wasn't regenerated, and that's what's failing test.

test is FAILURE on this exact head (job 97083453961), and I reproduced it locally: npm run astryx:surface-inventory exits 1 on this branch, consistently. Three moved files are on disk but absent from docs/astryx-surface-file-inventory.paths — apps/desktop/src/renderer/features/goals/ui/goal-dialog.tsx, plus services-context.tsx and goal-host.tsx in the same directory — while the pre-move apps/desktop/src/renderer/goal-dialog.tsx is still listed. docs/astryx-surface-file-inventory.md is likewise out of sync with the generator's output.

Fix is mechanical: run npm run astryx:surface-inventory:write and commit both regenerated files. Nothing about the runtime behaviour is at fault here — the gate is doing its job, the branch just moved surfaces without re-running it.

---

On the refactor itself: no P0–P3.

For a pure extraction the question is only ever whether behaviour changed under cover of moving things, so that is where I spent the time. use-goal-controller.ts ends up holding the active goal, session-switch clearing, the late-read sequence and disposed guards, the goal-change subscription, per-session control dedupe, and the dialog's session snapshot — and it stays a projection of Host events rather than becoming a second authority, which is the failure mode this kind of slice usually introduces. create-goal-services.ts is the single Desktop bridge adapter, and AppShell only reaches Goals through GoalHost and the controller's public API.

The deletion side checks out too, which is the part that makes an extraction actually reduce entropy rather than add a layer: use-session-goal.ts is gone, the old dialog's direct window.maka dependency is gone, and AppShell no longer carries goal control state. A repo-wide search finds no useSessionGoal and no surviving production entry to the old dialog path.

Verified: Goals focused tests 12/12; controller, model, adapter, and feature-boundary suites all pass; @maka/desktop builds and typechecks across renderer, preload, main, and Storybook; Biome clean on changed files. The full desktop main suite ran 1190 pass / 0 fail with 8 cancelled — the cancellations sit in pre-existing MCP OAuth pending tests and are not attributable to this branch, so I'm reporting it that way rather than calling the suite green.

Regenerate the inventory and this is ready for another look.

[chihumyum]

Fixed in f9554a9af by regenerating both checked-in Astryx surface inventory files.

Local verification:

• npm run astryx:surface-inventory:write
• npm run astryx:surface-inventory — 203 files, 1 exclusion
• git diff --check

The new CI run is currently action_required with no jobs, so it needs maintainer approval before GitHub will execute the workflow.

[Astro-Han]

Two independent lines on f9554a9af129f6bede8b7975c0a4bb9a2a0cc3be, from different models, neither able to see the other's conclusion. Both came back with nothing at P0–P3, and — more importantly for a refactor — both answered the question that actually matters here.

A refactor's failure mode isn't "didn't fix it", it's "changed behaviour in transit", and in a diff a relocation and a rewrite look identical. So both lines were asked to split -263 into what genuinely disappeared versus what is just move noise.

What disappeared (~150 lines):

• use-session-goal.ts is gone as a file, and its authority — goal state, the change subscription, session-switch semantics — moved wholesale to the controller rather than being copied.
• The indicator construction inlined in app-shell.tsx (~55 lines), plus runGoalControl, pendingGoalControlSessionIdsRef and goalDialogSessionId, all leave the shell. AppShell keeps two lines of wiring.

That is one ownership leaving one place and arriving in another, not a copy. Move noise (~110 lines) is the dialog relocating into features/goals/ui/ and isLiveGoal moving to model/live-goal.ts.

The controller being larger than the hook it replaces is explained rather than hand-waved: it absorbed the behaviour that used to live inline in the shell, so the two are not comparable line-for-line.

Semantic parity was checked point by point rather than asserted: subscription filtering (including the broadcast undefined case), session-switch ordering, the sequence-number race guard, control re-entrancy, dialog keying against the wrong session, and the GoalState type resolving to the same underlying type through a different path. One behavioural difference was found and it is an improvement: the indicator moves from an IIFE rebuilding a closure every render to a memo keyed on the active goal, so the reference is now stable.

No second implementation survives. We verified this at the gate independently: window.maka.goal has zero hits in production code on this head, use-session-goal.ts no longer exists in the tree, and the only remaining references to the old surface are stories pointing at the deliberate testing export.

One thing worth recording without grading it: the adapter's subscribeChanges collapses events down to sessionId | undefined, discarding metadata beyond the reason. That is the right boundary for what goals need today, but if goals ever need another session reason — a removal, say — the adapter is where that has to be reopened. Noting it as a known edge of the seam, not as a defect.

CI is terminal green on this head (test: completed / success), which is the head this approval is bound to.

Approving.

[chihumyum]

Rebased onto current upstream/main (729839ed8) and resolved the GoalDialog conflict in cfce73ee9.

The resolution preserves the newly merged reconnect reconciliation behavior: Goal arm outcomes still fence stale results, lock reconciled states, and prevent duplicate submission, while the bridge call remains behind the Goals service port. The reconciliation model and its tests now live inside the Goals feature boundary.

Verified after the final rebase:

• npm run lint
• npm run format:check
• npm run astryx:surface-inventory
• npm run build
• npm run typecheck
• npm --workspace @maka/desktop run test:dist — 1255 passed
• npx knip --workspace apps/desktop

GitHub now reports the PR as mergeable. The new fork CI run is action_required and needs maintainer approval before jobs start.

[chihumyum]

Rechecked against the latest upstream/main (047567c4b). Main had advanced by five commits and the PR had become DIRTY solely because the generated Astryx inventory conflicted again.

Rebased and regenerated the inventory in 0ed2c53c4. Verification:

• npm run astryx:surface-inventory — 204 files, 1 exclusion
• git diff --check upstream/main...HEAD
• merge-tree probe against current main — clean

GitHub no longer reports a merge conflict. The current head workflow is run 32630272181, which is action_required with no jobs and needs maintainer approval to execute.

[jackwener]

Incremental technical review of exact head 0ed2c53c4bddef7c97434ad111541ea7111c33d8, executed by the Kabi review agent on behalf of @WAWQAQ under the standing Kabi review/publisher authorization.

GO — no live P0–P3. APPROVE; no merge action.

This review covers the rebase-only delta that incorporated the newly merged Goal arm reconciliation contract. Comparing the pre-extraction implementation at current base 047567c4b with the moved feature code shows semantic parity: only armed closes the dialog; matching/different/null reconciled results and reconciliation_unavailable all lock the form, show the authoritative outcome, and prevent duplicate submission. interpretGoalArmOutcome is unchanged apart from its relative import.

The bridge authority remains singular: main.tsx creates one Desktop Goal service adapter, its bridge.goal.arm result is returned through the service and controller as Promise<GoalArmOutcome>, and GoalHost injects that exact function into GoalDialog. The Goals feature contains no window.maka.goal access and the old direct production path is absent.

Verification: focused Goals suites 18/18; Desktop main 1273/1274 on the first full run, with the sole unrelated shell-env timeout fixture passing 12/12 when rerun alone; Desktop preload/main/renderer/Storybook typechecks passed. Three discussion endpoints were reconciled after provisional sealing: two old-head reviews, no inline comments, and three author status comments; no live P0–P2.

Fresh action gate: OPEN, non-draft, MERGEABLE/CLEAN; exact-head hosted test is completed/success. This Agent-executed approval does not claim to satisfy any separate unresolved human-review wording.

[chihumyum]

Summary

• Move Goal read state, subscriptions, controls, dialog ownership, and indicator assembly behind a renderer feature boundary.
• Isolate Desktop bridge access behind a narrow GoalServices adapter and provider; keep AppShell limited to Session context plus host, commands, and selectors.
• Add fake services and focused lifecycle, adapter, model, and architecture-boundary coverage.

Refs #3439

Verification

• npm run lint
• npm run format:check
• npm run build
• npm run typecheck
• npm --workspace @maka/desktop run test:dist — 1198 passed
• npx knip --workspace apps/desktop
• npx knip --workspace packages/ui
• npm --workspace @maka/desktop run build-storybook
• npm --workspace @maka/desktop run smoke:storybook — 166 stories passed
• npx playwright test --config e2e/playwright.config.ts from apps/desktop — 50 passed, 1 skipped

No screenshots are included because this changes no Goal copy, CSS, layout, IPC contract, or user-visible behavior.

Review focus

• Session-switch fencing for stale Goal reads.
• Goal-change event filtering and subscription disposal.
• Dialog ownership of the Session captured when it opens.
• Per-Session pause/resume re-entry protection.

AI use

Select exactly one:

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

Tool(s) and scope: Codex assisted with the feature extraction, implementation, tests, and verification.

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 — described under Summary above
• No

[Astro-Han]

Reviewed at head b29e3c4e. One blocking item, and it is a generated file rather than the code — the extraction itself came through clean.

[P1] The Astryx surface inventory wasn't regenerated, and that's what's failing test.

test is FAILURE on this exact head (job 97083453961), and I reproduced it locally: npm run astryx:surface-inventory exits 1 on this branch, consistently. Three moved files are on disk but absent from docs/astryx-surface-file-inventory.paths — apps/desktop/src/renderer/features/goals/ui/goal-dialog.tsx, plus services-context.tsx and goal-host.tsx in the same directory — while the pre-move apps/desktop/src/renderer/goal-dialog.tsx is still listed. docs/astryx-surface-file-inventory.md is likewise out of sync with the generator's output.

Fix is mechanical: run npm run astryx:surface-inventory:write and commit both regenerated files. Nothing about the runtime behaviour is at fault here — the gate is doing its job, the branch just moved surfaces without re-running it.

---

On the refactor itself: no P0–P3.

For a pure extraction the question is only ever whether behaviour changed under cover of moving things, so that is where I spent the time. use-goal-controller.ts ends up holding the active goal, session-switch clearing, the late-read sequence and disposed guards, the goal-change subscription, per-session control dedupe, and the dialog's session snapshot — and it stays a projection of Host events rather than becoming a second authority, which is the failure mode this kind of slice usually introduces. create-goal-services.ts is the single Desktop bridge adapter, and AppShell only reaches Goals through GoalHost and the controller's public API.

The deletion side checks out too, which is the part that makes an extraction actually reduce entropy rather than add a layer: use-session-goal.ts is gone, the old dialog's direct window.maka dependency is gone, and AppShell no longer carries goal control state. A repo-wide search finds no useSessionGoal and no surviving production entry to the old dialog path.

Verified: Goals focused tests 12/12; controller, model, adapter, and feature-boundary suites all pass; @maka/desktop builds and typechecks across renderer, preload, main, and Storybook; Biome clean on changed files. The full desktop main suite ran 1190 pass / 0 fail with 8 cancelled — the cancellations sit in pre-existing MCP OAuth pending tests and are not attributable to this branch, so I'm reporting it that way rather than calling the suite green.

Regenerate the inventory and this is ready for another look.

[chihumyum]

Fixed in f9554a9af by regenerating both checked-in Astryx surface inventory files.

Local verification:

• npm run astryx:surface-inventory:write
• npm run astryx:surface-inventory — 203 files, 1 exclusion
• git diff --check

The new CI run is currently action_required with no jobs, so it needs maintainer approval before GitHub will execute the workflow.

[Astro-Han]

Two independent lines on f9554a9af129f6bede8b7975c0a4bb9a2a0cc3be, from different models, neither able to see the other's conclusion. Both came back with nothing at P0–P3, and — more importantly for a refactor — both answered the question that actually matters here.

A refactor's failure mode isn't "didn't fix it", it's "changed behaviour in transit", and in a diff a relocation and a rewrite look identical. So both lines were asked to split -263 into what genuinely disappeared versus what is just move noise.

What disappeared (~150 lines):

• use-session-goal.ts is gone as a file, and its authority — goal state, the change subscription, session-switch semantics — moved wholesale to the controller rather than being copied.
• The indicator construction inlined in app-shell.tsx (~55 lines), plus runGoalControl, pendingGoalControlSessionIdsRef and goalDialogSessionId, all leave the shell. AppShell keeps two lines of wiring.

That is one ownership leaving one place and arriving in another, not a copy. Move noise (~110 lines) is the dialog relocating into features/goals/ui/ and isLiveGoal moving to model/live-goal.ts.

The controller being larger than the hook it replaces is explained rather than hand-waved: it absorbed the behaviour that used to live inline in the shell, so the two are not comparable line-for-line.

Semantic parity was checked point by point rather than asserted: subscription filtering (including the broadcast undefined case), session-switch ordering, the sequence-number race guard, control re-entrancy, dialog keying against the wrong session, and the GoalState type resolving to the same underlying type through a different path. One behavioural difference was found and it is an improvement: the indicator moves from an IIFE rebuilding a closure every render to a memo keyed on the active goal, so the reference is now stable.

No second implementation survives. We verified this at the gate independently: window.maka.goal has zero hits in production code on this head, use-session-goal.ts no longer exists in the tree, and the only remaining references to the old surface are stories pointing at the deliberate testing export.

One thing worth recording without grading it: the adapter's subscribeChanges collapses events down to sessionId | undefined, discarding metadata beyond the reason. That is the right boundary for what goals need today, but if goals ever need another session reason — a removal, say — the adapter is where that has to be reopened. Noting it as a known edge of the seam, not as a defect.

CI is terminal green on this head (test: completed / success), which is the head this approval is bound to.

Approving.

[chihumyum]

Rebased onto current upstream/main (729839ed8) and resolved the GoalDialog conflict in cfce73ee9.

The resolution preserves the newly merged reconnect reconciliation behavior: Goal arm outcomes still fence stale results, lock reconciled states, and prevent duplicate submission, while the bridge call remains behind the Goals service port. The reconciliation model and its tests now live inside the Goals feature boundary.

Verified after the final rebase:

• npm run lint
• npm run format:check
• npm run astryx:surface-inventory
• npm run build
• npm run typecheck
• npm --workspace @maka/desktop run test:dist — 1255 passed
• npx knip --workspace apps/desktop

GitHub now reports the PR as mergeable. The new fork CI run is action_required and needs maintainer approval before jobs start.

[chihumyum]

Rechecked against the latest upstream/main (047567c4b). Main had advanced by five commits and the PR had become DIRTY solely because the generated Astryx inventory conflicted again.

Rebased and regenerated the inventory in 0ed2c53c4. Verification:

• npm run astryx:surface-inventory — 204 files, 1 exclusion
• git diff --check upstream/main...HEAD
• merge-tree probe against current main — clean

GitHub no longer reports a merge conflict. The current head workflow is run 32630272181, which is action_required with no jobs and needs maintainer approval to execute.

[jackwener]

Incremental technical review of exact head 0ed2c53c4bddef7c97434ad111541ea7111c33d8, executed by the Kabi review agent on behalf of @WAWQAQ under the standing Kabi review/publisher authorization.

GO — no live P0–P3. APPROVE; no merge action.

This review covers the rebase-only delta that incorporated the newly merged Goal arm reconciliation contract. Comparing the pre-extraction implementation at current base 047567c4b with the moved feature code shows semantic parity: only armed closes the dialog; matching/different/null reconciled results and reconciliation_unavailable all lock the form, show the authoritative outcome, and prevent duplicate submission. interpretGoalArmOutcome is unchanged apart from its relative import.

The bridge authority remains singular: main.tsx creates one Desktop Goal service adapter, its bridge.goal.arm result is returned through the service and controller as Promise<GoalArmOutcome>, and GoalHost injects that exact function into GoalDialog. The Goals feature contains no window.maka.goal access and the old direct production path is absent.

Verification: focused Goals suites 18/18; Desktop main 1273/1274 on the first full run, with the sole unrelated shell-env timeout fixture passing 12/12 when rerun alone; Desktop preload/main/renderer/Storybook typechecks passed. Three discussion endpoints were reconciled after provisional sealing: two old-head reviews, no inline comments, and three author status comments; no live P0–P2.

Fresh action gate: OPEN, non-draft, MERGEABLE/CLEAN; exact-head hosted test is completed/success. This Agent-executed approval does not claim to satisfy any separate unresolved human-review wording.

[chihumyum]

Summary

• Move Goal read state, subscriptions, controls, dialog ownership, and indicator assembly behind a renderer feature boundary.
• Isolate Desktop bridge access behind a narrow GoalServices adapter and provider; keep AppShell limited to Session context plus host, commands, and selectors.
• Add fake services and focused lifecycle, adapter, model, and architecture-boundary coverage.

Refs #3439

Verification

• npm run lint
• npm run format:check
• npm run build
• npm run typecheck
• npm --workspace @maka/desktop run test:dist — 1198 passed
• npx knip --workspace apps/desktop
• npx knip --workspace packages/ui
• npm --workspace @maka/desktop run build-storybook
• npm --workspace @maka/desktop run smoke:storybook — 166 stories passed
• npx playwright test --config e2e/playwright.config.ts from apps/desktop — 50 passed, 1 skipped

No screenshots are included because this changes no Goal copy, CSS, layout, IPC contract, or user-visible behavior.

Review focus

• Session-switch fencing for stale Goal reads.
• Goal-change event filtering and subscription disposal.
• Dialog ownership of the Session captured when it opens.
• Per-Session pause/resume re-entry protection.

AI use

Select exactly one:

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

Tool(s) and scope: Codex assisted with the feature extraction, implementation, tests, and verification.

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 — described under Summary above
• No

[Astro-Han]

Reviewed at head b29e3c4e. One blocking item, and it is a generated file rather than the code — the extraction itself came through clean.

[P1] The Astryx surface inventory wasn't regenerated, and that's what's failing test.

test is FAILURE on this exact head (job 97083453961), and I reproduced it locally: npm run astryx:surface-inventory exits 1 on this branch, consistently. Three moved files are on disk but absent from docs/astryx-surface-file-inventory.paths — apps/desktop/src/renderer/features/goals/ui/goal-dialog.tsx, plus services-context.tsx and goal-host.tsx in the same directory — while the pre-move apps/desktop/src/renderer/goal-dialog.tsx is still listed. docs/astryx-surface-file-inventory.md is likewise out of sync with the generator's output.

Fix is mechanical: run npm run astryx:surface-inventory:write and commit both regenerated files. Nothing about the runtime behaviour is at fault here — the gate is doing its job, the branch just moved surfaces without re-running it.

---

On the refactor itself: no P0–P3.

For a pure extraction the question is only ever whether behaviour changed under cover of moving things, so that is where I spent the time. use-goal-controller.ts ends up holding the active goal, session-switch clearing, the late-read sequence and disposed guards, the goal-change subscription, per-session control dedupe, and the dialog's session snapshot — and it stays a projection of Host events rather than becoming a second authority, which is the failure mode this kind of slice usually introduces. create-goal-services.ts is the single Desktop bridge adapter, and AppShell only reaches Goals through GoalHost and the controller's public API.

The deletion side checks out too, which is the part that makes an extraction actually reduce entropy rather than add a layer: use-session-goal.ts is gone, the old dialog's direct window.maka dependency is gone, and AppShell no longer carries goal control state. A repo-wide search finds no useSessionGoal and no surviving production entry to the old dialog path.

Verified: Goals focused tests 12/12; controller, model, adapter, and feature-boundary suites all pass; @maka/desktop builds and typechecks across renderer, preload, main, and Storybook; Biome clean on changed files. The full desktop main suite ran 1190 pass / 0 fail with 8 cancelled — the cancellations sit in pre-existing MCP OAuth pending tests and are not attributable to this branch, so I'm reporting it that way rather than calling the suite green.

Regenerate the inventory and this is ready for another look.

[chihumyum]

Fixed in f9554a9af by regenerating both checked-in Astryx surface inventory files.

Local verification:

• npm run astryx:surface-inventory:write
• npm run astryx:surface-inventory — 203 files, 1 exclusion
• git diff --check

The new CI run is currently action_required with no jobs, so it needs maintainer approval before GitHub will execute the workflow.

[Astro-Han]

Two independent lines on f9554a9af129f6bede8b7975c0a4bb9a2a0cc3be, from different models, neither able to see the other's conclusion. Both came back with nothing at P0–P3, and — more importantly for a refactor — both answered the question that actually matters here.

A refactor's failure mode isn't "didn't fix it", it's "changed behaviour in transit", and in a diff a relocation and a rewrite look identical. So both lines were asked to split -263 into what genuinely disappeared versus what is just move noise.

What disappeared (~150 lines):

• use-session-goal.ts is gone as a file, and its authority — goal state, the change subscription, session-switch semantics — moved wholesale to the controller rather than being copied.
• The indicator construction inlined in app-shell.tsx (~55 lines), plus runGoalControl, pendingGoalControlSessionIdsRef and goalDialogSessionId, all leave the shell. AppShell keeps two lines of wiring.

That is one ownership leaving one place and arriving in another, not a copy. Move noise (~110 lines) is the dialog relocating into features/goals/ui/ and isLiveGoal moving to model/live-goal.ts.

The controller being larger than the hook it replaces is explained rather than hand-waved: it absorbed the behaviour that used to live inline in the shell, so the two are not comparable line-for-line.

Semantic parity was checked point by point rather than asserted: subscription filtering (including the broadcast undefined case), session-switch ordering, the sequence-number race guard, control re-entrancy, dialog keying against the wrong session, and the GoalState type resolving to the same underlying type through a different path. One behavioural difference was found and it is an improvement: the indicator moves from an IIFE rebuilding a closure every render to a memo keyed on the active goal, so the reference is now stable.

No second implementation survives. We verified this at the gate independently: window.maka.goal has zero hits in production code on this head, use-session-goal.ts no longer exists in the tree, and the only remaining references to the old surface are stories pointing at the deliberate testing export.

One thing worth recording without grading it: the adapter's subscribeChanges collapses events down to sessionId | undefined, discarding metadata beyond the reason. That is the right boundary for what goals need today, but if goals ever need another session reason — a removal, say — the adapter is where that has to be reopened. Noting it as a known edge of the seam, not as a defect.

CI is terminal green on this head (test: completed / success), which is the head this approval is bound to.

Approving.

[chihumyum]

Rebased onto current upstream/main (729839ed8) and resolved the GoalDialog conflict in cfce73ee9.

The resolution preserves the newly merged reconnect reconciliation behavior: Goal arm outcomes still fence stale results, lock reconciled states, and prevent duplicate submission, while the bridge call remains behind the Goals service port. The reconciliation model and its tests now live inside the Goals feature boundary.

Verified after the final rebase:

• npm run lint
• npm run format:check
• npm run astryx:surface-inventory
• npm run build
• npm run typecheck
• npm --workspace @maka/desktop run test:dist — 1255 passed
• npx knip --workspace apps/desktop

GitHub now reports the PR as mergeable. The new fork CI run is action_required and needs maintainer approval before jobs start.

[chihumyum]

Rechecked against the latest upstream/main (047567c4b). Main had advanced by five commits and the PR had become DIRTY solely because the generated Astryx inventory conflicted again.

Rebased and regenerated the inventory in 0ed2c53c4. Verification:

• npm run astryx:surface-inventory — 204 files, 1 exclusion
• git diff --check upstream/main...HEAD
• merge-tree probe against current main — clean

GitHub no longer reports a merge conflict. The current head workflow is run 32630272181, which is action_required with no jobs and needs maintainer approval to execute.

[jackwener]

Incremental technical review of exact head 0ed2c53c4bddef7c97434ad111541ea7111c33d8, executed by the Kabi review agent on behalf of @WAWQAQ under the standing Kabi review/publisher authorization.

GO — no live P0–P3. APPROVE; no merge action.

This review covers the rebase-only delta that incorporated the newly merged Goal arm reconciliation contract. Comparing the pre-extraction implementation at current base 047567c4b with the moved feature code shows semantic parity: only armed closes the dialog; matching/different/null reconciled results and reconciliation_unavailable all lock the form, show the authoritative outcome, and prevent duplicate submission. interpretGoalArmOutcome is unchanged apart from its relative import.

The bridge authority remains singular: main.tsx creates one Desktop Goal service adapter, its bridge.goal.arm result is returned through the service and controller as Promise<GoalArmOutcome>, and GoalHost injects that exact function into GoalDialog. The Goals feature contains no window.maka.goal access and the old direct production path is absent.

Verification: focused Goals suites 18/18; Desktop main 1273/1274 on the first full run, with the sole unrelated shell-env timeout fixture passing 12/12 when rerun alone; Desktop preload/main/renderer/Storybook typechecks passed. Three discussion endpoints were reconciled after provisional sealing: two old-head reviews, no inline comments, and three author status comments; no live P0–P2.

Fresh action gate: OPEN, non-draft, MERGEABLE/CLEAN; exact-head hosted test is completed/success. This Agent-executed approval does not claim to satisfy any separate unresolved human-review wording.

[chihumyum]

Summary

• Move Goal read state, subscriptions, controls, dialog ownership, and indicator assembly behind a renderer feature boundary.
• Isolate Desktop bridge access behind a narrow GoalServices adapter and provider; keep AppShell limited to Session context plus host, commands, and selectors.
• Add fake services and focused lifecycle, adapter, model, and architecture-boundary coverage.

Refs #3439

Verification

• npm run lint
• npm run format:check
• npm run build
• npm run typecheck
• npm --workspace @maka/desktop run test:dist — 1198 passed
• npx knip --workspace apps/desktop
• npx knip --workspace packages/ui
• npm --workspace @maka/desktop run build-storybook
• npm --workspace @maka/desktop run smoke:storybook — 166 stories passed
• npx playwright test --config e2e/playwright.config.ts from apps/desktop — 50 passed, 1 skipped

No screenshots are included because this changes no Goal copy, CSS, layout, IPC contract, or user-visible behavior.

Review focus

• Session-switch fencing for stale Goal reads.
• Goal-change event filtering and subscription disposal.
• Dialog ownership of the Session captured when it opens.
• Per-Session pause/resume re-entry protection.

AI use

Select exactly one:

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

Tool(s) and scope: Codex assisted with the feature extraction, implementation, tests, and verification.

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 — described under Summary above
• No

[Astro-Han]

Reviewed at head b29e3c4e. One blocking item, and it is a generated file rather than the code — the extraction itself came through clean.

[P1] The Astryx surface inventory wasn't regenerated, and that's what's failing test.

test is FAILURE on this exact head (job 97083453961), and I reproduced it locally: npm run astryx:surface-inventory exits 1 on this branch, consistently. Three moved files are on disk but absent from docs/astryx-surface-file-inventory.paths — apps/desktop/src/renderer/features/goals/ui/goal-dialog.tsx, plus services-context.tsx and goal-host.tsx in the same directory — while the pre-move apps/desktop/src/renderer/goal-dialog.tsx is still listed. docs/astryx-surface-file-inventory.md is likewise out of sync with the generator's output.

Fix is mechanical: run npm run astryx:surface-inventory:write and commit both regenerated files. Nothing about the runtime behaviour is at fault here — the gate is doing its job, the branch just moved surfaces without re-running it.

---

On the refactor itself: no P0–P3.

For a pure extraction the question is only ever whether behaviour changed under cover of moving things, so that is where I spent the time. use-goal-controller.ts ends up holding the active goal, session-switch clearing, the late-read sequence and disposed guards, the goal-change subscription, per-session control dedupe, and the dialog's session snapshot — and it stays a projection of Host events rather than becoming a second authority, which is the failure mode this kind of slice usually introduces. create-goal-services.ts is the single Desktop bridge adapter, and AppShell only reaches Goals through GoalHost and the controller's public API.

The deletion side checks out too, which is the part that makes an extraction actually reduce entropy rather than add a layer: use-session-goal.ts is gone, the old dialog's direct window.maka dependency is gone, and AppShell no longer carries goal control state. A repo-wide search finds no useSessionGoal and no surviving production entry to the old dialog path.

Verified: Goals focused tests 12/12; controller, model, adapter, and feature-boundary suites all pass; @maka/desktop builds and typechecks across renderer, preload, main, and Storybook; Biome clean on changed files. The full desktop main suite ran 1190 pass / 0 fail with 8 cancelled — the cancellations sit in pre-existing MCP OAuth pending tests and are not attributable to this branch, so I'm reporting it that way rather than calling the suite green.

Regenerate the inventory and this is ready for another look.

[chihumyum]

Fixed in f9554a9af by regenerating both checked-in Astryx surface inventory files.

Local verification:

• npm run astryx:surface-inventory:write
• npm run astryx:surface-inventory — 203 files, 1 exclusion
• git diff --check

The new CI run is currently action_required with no jobs, so it needs maintainer approval before GitHub will execute the workflow.

[Astro-Han]

Two independent lines on f9554a9af129f6bede8b7975c0a4bb9a2a0cc3be, from different models, neither able to see the other's conclusion. Both came back with nothing at P0–P3, and — more importantly for a refactor — both answered the question that actually matters here.

A refactor's failure mode isn't "didn't fix it", it's "changed behaviour in transit", and in a diff a relocation and a rewrite look identical. So both lines were asked to split -263 into what genuinely disappeared versus what is just move noise.

What disappeared (~150 lines):

• use-session-goal.ts is gone as a file, and its authority — goal state, the change subscription, session-switch semantics — moved wholesale to the controller rather than being copied.
• The indicator construction inlined in app-shell.tsx (~55 lines), plus runGoalControl, pendingGoalControlSessionIdsRef and goalDialogSessionId, all leave the shell. AppShell keeps two lines of wiring.

That is one ownership leaving one place and arriving in another, not a copy. Move noise (~110 lines) is the dialog relocating into features/goals/ui/ and isLiveGoal moving to model/live-goal.ts.

The controller being larger than the hook it replaces is explained rather than hand-waved: it absorbed the behaviour that used to live inline in the shell, so the two are not comparable line-for-line.

Semantic parity was checked point by point rather than asserted: subscription filtering (including the broadcast undefined case), session-switch ordering, the sequence-number race guard, control re-entrancy, dialog keying against the wrong session, and the GoalState type resolving to the same underlying type through a different path. One behavioural difference was found and it is an improvement: the indicator moves from an IIFE rebuilding a closure every render to a memo keyed on the active goal, so the reference is now stable.

No second implementation survives. We verified this at the gate independently: window.maka.goal has zero hits in production code on this head, use-session-goal.ts no longer exists in the tree, and the only remaining references to the old surface are stories pointing at the deliberate testing export.

One thing worth recording without grading it: the adapter's subscribeChanges collapses events down to sessionId | undefined, discarding metadata beyond the reason. That is the right boundary for what goals need today, but if goals ever need another session reason — a removal, say — the adapter is where that has to be reopened. Noting it as a known edge of the seam, not as a defect.

CI is terminal green on this head (test: completed / success), which is the head this approval is bound to.

Approving.

[chihumyum]

Rebased onto current upstream/main (729839ed8) and resolved the GoalDialog conflict in cfce73ee9.

The resolution preserves the newly merged reconnect reconciliation behavior: Goal arm outcomes still fence stale results, lock reconciled states, and prevent duplicate submission, while the bridge call remains behind the Goals service port. The reconciliation model and its tests now live inside the Goals feature boundary.

Verified after the final rebase:

• npm run lint
• npm run format:check
• npm run astryx:surface-inventory
• npm run build
• npm run typecheck
• npm --workspace @maka/desktop run test:dist — 1255 passed
• npx knip --workspace apps/desktop

GitHub now reports the PR as mergeable. The new fork CI run is action_required and needs maintainer approval before jobs start.

[chihumyum]

Rechecked against the latest upstream/main (047567c4b). Main had advanced by five commits and the PR had become DIRTY solely because the generated Astryx inventory conflicted again.

Rebased and regenerated the inventory in 0ed2c53c4. Verification:

• npm run astryx:surface-inventory — 204 files, 1 exclusion
• git diff --check upstream/main...HEAD
• merge-tree probe against current main — clean

GitHub no longer reports a merge conflict. The current head workflow is run 32630272181, which is action_required with no jobs and needs maintainer approval to execute.

[jackwener]

Incremental technical review of exact head 0ed2c53c4bddef7c97434ad111541ea7111c33d8, executed by the Kabi review agent on behalf of @WAWQAQ under the standing Kabi review/publisher authorization.

GO — no live P0–P3. APPROVE; no merge action.

This review covers the rebase-only delta that incorporated the newly merged Goal arm reconciliation contract. Comparing the pre-extraction implementation at current base 047567c4b with the moved feature code shows semantic parity: only armed closes the dialog; matching/different/null reconciled results and reconciliation_unavailable all lock the form, show the authoritative outcome, and prevent duplicate submission. interpretGoalArmOutcome is unchanged apart from its relative import.

The bridge authority remains singular: main.tsx creates one Desktop Goal service adapter, its bridge.goal.arm result is returned through the service and controller as Promise<GoalArmOutcome>, and GoalHost injects that exact function into GoalDialog. The Goals feature contains no window.maka.goal access and the old direct production path is absent.

Verification: focused Goals suites 18/18; Desktop main 1273/1274 on the first full run, with the sole unrelated shell-env timeout fixture passing 12/12 when rerun alone; Desktop preload/main/renderer/Storybook typechecks passed. Three discussion endpoints were reconciled after provisional sealing: two old-head reviews, no inline comments, and three author status comments; no live P0–P2.

Fresh action gate: OPEN, non-draft, MERGEABLE/CLEAN; exact-head hosted test is completed/success. This Agent-executed approval does not claim to satisfy any separate unresolved human-review wording.