Skip to content

refactor(workhub): clarify renderer decision seams - #4822

Closed
ARE404 wants to merge 2 commits into
apache:mainfrom
ARE404:refactor/workhub-decision-boundaries
Closed

refactor(workhub): clarify renderer decision seams#4822
ARE404 wants to merge 2 commits into
apache:mainfrom
ARE404:refactor/workhub-decision-boundaries

Conversation

@ARE404

@ARE404 ARE404 commented Sep 5, 2026

Copy link
Copy Markdown
Contributor

Summary

  • move the renderer-side WorkHub interface out of workhub-controller.ts and into the application contract layer
  • make the Session and Coordination desktop adapters, controller, and surface depend on that contract instead of the controller implementation
  • replace the combined workhub-request-intent.ts re-export with separate renderer contracts for Action Intent and Session Resolver
  • update the renderer architecture ledger to record the new inward dependency direction

This is a structure-only refactor. It does not change routing policy, proposal shapes, Runtime Host admission, persistence, or WorkHub behavior.

Resulting seams

  • Action Intent: application/contracts/workhub-action-intent.ts over the shared Core module
  • Session Resolver: application/contracts/workhub-session-resolver.ts over the shared Core module
  • Action Policy: workhub-route-policy.ts
  • Application interface: application/contracts/workhub.ts
  • Controller implementation: workhub-controller.ts
  • Desktop adapters: workhub-session-port.ts and workhub-coordination-port.ts
  • Action Proposal / Gate protocol: unchanged in Runtime Host

The controller implementation is no longer the source of the interfaces its adapters implement. The Runtime Host remains the only admission authority for WorkHub effects.

Validation

  • 209 focused WorkHub tests passed across Desktop, Core, Runtime Host, and Storage
  • npm --workspace @maka/desktop run build:main
  • renderer TypeScript check: tsc -p tsconfig.renderer.json --noEmit
  • npm run lint
  • base-aware npm run check:renderer-architecture -- --base <main-sha>
  • npm run check:asf-headers
  • git diff --check

Design context: #4819

@github-actions github-actions Bot added the effort/L Under 1000 readable lines label Sep 5, 2026

@Astro-Han Astro-Han left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for laying this out; the move itself is clean (I diffed the export sets on both sides: nothing lost, nothing duplicated, no cycle). My suggestion is to close this one and fold the one real fix into #4713, because as a standalone change it does not fix a defect and it raises the entropy it says it lowers.

The checker on main already accepts the current direction: check-renderer-architecture.mjs:1575 puts every src/renderer/workhub-*.ts in the legacy zone, and the only rule on legacy sources is :2196, which forbids importing non-public application paths. legacy → legacy is legal, there is no cycle, and the two tests that import the controller also import createWorkHubController from it, so they were never forced to. The ledger delta (−3 counted paths) comes from isPublicApplicationPath filtering application/contracts/** out of the comparison, not from a rule; the 282 lines moved into contracts/workhub.ts are in a zone the ledger does not measure.

What it costs: modules 2 → 4, +69 lines (36 of them license headers), the same concept count. application/contracts/ on main holds four thin Core adapters (one is a single export type); this file is 282 lines of renderer-owned interfaces plus a runtime class, a different kind of thing under the same directory name. The workhub-request-intent.ts split has one production consumer, workhub-route-policy.ts:20-29, which now imports both halves; no consumer depends on less. And the move dropped four rule comments, including "Implementations must not persist a second writable copy of it" on routingEvidence, and left the WorkHubStopClarificationReason paragraph orphaned on WorkHubStopRouteDecision (workhub-route-policy.ts:64-77).

The one real fix in here is three lines: workhub-coordination-port.ts:42 re-exports WorkHubCoordinationFailure, giving the class two import paths, and this PR deletes that. #4713 already touches that file and workhub-surface.tsx; please fold the deletion into it. If #4819's stages should live in code, the move that pays for itself is taking the controller, adapters and surface out of legacy into their zones together, not relocating the types alone.

Since #4713 adds variants to the same three types this PR relocates, closing this also settles the order: #4713 rebases onto #4699 and nothing else. Your call, so I am leaving it open rather than closing it myself.

AI-assisted review: drafted with Maka; I verified the checker rules, the export sets and the ledger mechanics myself.

@ARE404

ARE404 commented Sep 5, 2026

Copy link
Copy Markdown
Contributor Author

Agreed. The ablation result is that the relocation does not earn the extra modules or lines: it changes the ledger accounting without establishing a new enforced boundary, and the intent split does not reduce any production consumer's dependency surface.\n\nI moved the concrete fix—removing the duplicate WorkHubCoordinationFailure export path—into #4713 as commit 4e3a96b, with its consumers importing from the defining module. The targeted desktop build, 91 WorkHub tests, and lint all pass locally; #4713 CI is running now. Closing this PR. Thanks for the precise review.

@ARE404 ARE404 closed this Sep 5, 2026
@ARE404

ARE404 commented Sep 5, 2026

Copy link
Copy Markdown
Contributor Author

Follow-up: #4713 is now green at final head 4477263. Its architecture ratchet keeps the surface → coordination-port dependency, so the port now exposes only an isWorkHubCoordinationFailure classifier rather than a second import path for the class. The class itself remains imported directly from its defining controller by tests and internal consumers.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

effort/L Under 1000 readable lines

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants