Skip to content

refactor(daemon): retire the session-state group dispatcher, routing commands straight to handlers - #3472

Merged
thymikee merged 1 commit into
mainfrom
crap/3424-session-state-removal
Oct 12, 2026
Merged

thymikee merged 1 commit into
mainfrom
crap/3424-session-state-removal

Conversation

@thymikee

@thymikee thymikee commented Oct 12, 2026 •

Copy link
Copy Markdown
Member

Summary

Closes #3424 (CRAP burn-down, umbrella #3414). Final layer on top of the merged stack (#3455–#3458), settling the design question from review: src/daemon/handlers/session-state.ts re-dispatched on req.command after the session.ts table already routed boot, shutdown and appstate, so it only forwarded params.

This layer deletes it. boot, shutdown and appstate map straight to handleBootCommand, handleShutdownCommand and handleAppStateCommand through per-command SessionCommandHandler entries in the typed SESSION_COMMAND_HANDLER_IMPLS record, so the group-level null return and the string re-dispatch disappear (the record's satisfies Record<DescriptorSessionRouteCommandName, …> check already guarantees coverage). The six importers of handleSessionStateCommands retarget at the owning handler (session-boot, session-shutdown, session-appstate) or the router: the group-membership test in session-state.test.ts becomes a mirrored session.test.ts driving handleSessionCommands; daemon-layer-manifest.json drops the deleted module. No behavior change; no production file imports the deleted module (all other session-state imports are the unrelated SessionState type module).

CRAP stays clean: check:coverage-crap --all reports no function >= 30.

Validation

  • pnpm check:affected --run on this head under the machine lock: passed; result posted in comments.
  • pnpm check:layering: pass; pnpm exec tsc --noEmit: clean.

Created by Apex.

@github-actions

github-actions Bot commented Oct 12, 2026 •

Copy link
Copy Markdown
Contributor

Size Report

Metric Base Current Diff
Installed (including dependencies) 5.17 MB 5.17 MB -2.2 kB
Package (unpacked) 5.17 MB 5.17 MB -2.2 kB
Package (download) 1.55 MB 1.55 MB -1.0 kB

Startup median (7 runs, lower is better):

Scenario Base Current Diff
CLI --version 27.3 ms 28.7 ms +1.5 ms
CLI --help 76.8 ms 77.0 ms +0.1 ms

@cubic-dev-ai cubic-dev-ai Bot 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.

1 issue found across 9 files

Prompt for AI agents (unresolved issues)

Check if these issues are valid — if so, understand the root cause of each and fix them. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. If appropriate, use sub-agents to investigate and fix each issue separately.


<file name="src/daemon/handlers/session.ts">

<violation number="1" location="src/daemon/handlers/session.ts:12">
P3: Deleting `session-state.ts` leaves dead path references behind: `packages/contracts/src/app-state.ts` and `packages/contracts/src/device.ts` cite `src/daemon/handlers/session-state.ts` as the source of `handleAppStateCommand` and the fixed success shape, and `docs/dependency-graph-findings.md` lists `handlers/session-state.ts` in its findings snapshot. Sweep those references (handleAppStateCommand now lives in `src/daemon/handlers/session-appstate.ts`) before landing the removal.</violation>
</file>

Reply with feedback, questions, or to request a fix.

View guided diff | Turn on auto-fix | Re-trigger cubic

Comment thread src/daemon/handlers/session.ts
@thymikee

Copy link
Copy Markdown
Member Author

Validation update: pnpm check:affected --run on this head (fc4e138) under the machine lock — all runnable checks passed (2026-10-12 10:29 CEST). check:layering and tsc --noEmit also clean.

Created by Apex.

@thymikee
thymikee force-pushed the crap/3424-appstate-admission branch from 2a8cb15 to 16db730 Compare October 12, 2026 08:36
@thymikee
thymikee force-pushed the crap/3424-session-state-removal branch from fc4e138 to babd286 Compare October 12, 2026 08:36
An error occurred while trying to automatically change base from crap/3424-appstate-admission to crap/3424-appstate-modules October 12, 2026 08:42
@thymikee
thymikee force-pushed the crap/3424-session-state-removal branch from babd286 to b07d9ee Compare October 12, 2026 08:45
@thymikee
thymikee changed the base branch from crap/3424-appstate-admission to main October 12, 2026 08:45
@thymikee

Copy link
Copy Markdown
Member Author

Retargeted to main after the stack merged (squash commits 4a1a9c0..7da2934). Rebased onto origin/main as a single commit b07d9ee; 9 files, 174 gross lines vs main. Body switched to Closes #3424 as the final G PR. check:affected --run on b07d9ee under the machine lock: all runnable checks passed, exit 0 (2026-10-12 10:50 CEST); standalone pnpm run lint, tsc --noEmit, and check:layering all clean.

Created by Apex.

@thymikee

Copy link
Copy Markdown
Member Author

Nudging CI: workflows did not trigger after the base retarget from crap/3424-appstate-admission to main (close/reopen to re-fire pull_request events on b07d9ee). No code change.

Created by Apex.

@thymikee thymikee closed this Oct 12, 2026
@thymikee thymikee reopened this Oct 12, 2026
@github-actions

github-actions Bot commented Oct 12, 2026 •

Copy link
Copy Markdown
Contributor
PR Preview Action v1.8.1
Preview removed because the pull request was closed.
2026-10-12 10:36 UTC

@thymikee

Copy link
Copy Markdown
Member Author

This PR is ready at b07d9ee. The group dispatcher is gone and commands now route straight to their handlers, with the same handlers behind them.

Not blocking, take or leave: the three new wrappers for boot, shutdown and appstate in https://github.com/callstack/agent-device/blob/b07d9ee/src/daemon/handlers/session.ts#L66 only copy fields into handlers that already accept the params, so could the record map handleBootCommand, handleShutdownCommand and handleAppStateCommand directly like the keyboard, replay and test entries do? Also, comments in packages/contracts/src/app-state.ts (line 6) and device.ts (lines 11 and 35) still point at the deleted session-state.ts, so they could name session-appstate.ts, session-boot.ts and session-shutdown.ts instead.

The Cubic thread on the stale comment paths still applies, as a low-priority note: #3472 (comment)

I did not run tsc, the unit suite or the layering and coverage checks. I took the PR body's word that they pass. I also did not read the new session.test.ts against the real harness for the not_a_command null path, and I did not read the check:affected comment the PR body points to.

CI is still pending. Smoke Tests is the only job not passing, and it is still running. The change is mechanical, but boot, shutdown and appstate do flow through the route a smoke job exercises, so please re-check once it finishes. Smoke Tests must finish green before merge.

@thymikee thymikee added the ready-for-human Valid work that needs human implementation, judgment, or maintainer merge label Oct 12, 2026
@thymikee
thymikee force-pushed the crap/3424-session-state-removal branch from b07d9ee to a8903b5 Compare October 12, 2026 09:36
@thymikee

Copy link
Copy Markdown
Member Author

Final validation on a8903b5 (post cubic-sweep amend): check:affected --run under the machine lock — all 6 runnable checks passed, exit 0 (11:39 CEST). CI on this head: all workflows success (CI, iOS, macOS, Android, Linux, Size, Test App Build Cache, Cleanup). Diff vs main: 12 files, 183 gross lines. All review threads resolved.

Created by Apex.

@thymikee
thymikee merged commit 482d91c into main Oct 12, 2026
19 checks passed
@thymikee
thymikee deleted the crap/3424-session-state-removal branch October 12, 2026 10:35
@thymikee

Copy link
Copy Markdown
Member Author

The PR is ready. The earlier review at b07d9ee was clean, and I found no new problems at a8903b5. The stale session-state.ts path references in packages/contracts are now fixed: app-state.ts points to session-appstate.ts, which is correct, and docs/dependency-graph-findings.md is updated too.

Not blocking: the BootCommandResult and ShutdownCommandResult doc comments in https://github.com/callstack/agent-device/blob/a8903b5/packages/contracts/src/device.ts#L11 now point to session-appstate.ts, but boot is handled in session-boot.ts and shutdown in session-shutdown.ts. Pointing each comment at its own module would fix it. You can take this or leave it.

The cubic-dev-ai thread about stale session-state.ts paths is fixed at this head, so you can resolve it: #3472 (comment)

All 19 checks pass and none fail. You report that check:affected passed on this head. I could not confirm that run, and I did not run the tests myself. I judged the regression coverage in session.test.ts from the diff only. Cubic has not reviewed this head. There are no conflicts. Nothing else must happen before merge.

thymikee added a commit that referenced this pull request Oct 13, 2026
… place (#3557)

* refactor(daemon): own the boot/shutdown device-status response in one place

- Point the BootCommandResult and ShutdownCommandResult doc comments in
  packages/contracts/src/device.ts at their owning handlers (session-boot.ts,
  session-shutdown.ts) instead of the retired session-appstate.ts.
- Replace the duplicated bootedDeviceResponse wire-shape builders in the boot
  and shutdown handlers with one deviceStatusReportResponse owner.
- Map the boot, shutdown, and appstate dispatch-table entries in session.ts
  straight to their handlers; the pass-through wrappers only copied fields the
  handlers already accept.

Follow-ups from #3455, #3472. No behavior change.

* chore(gates): give session-device-status-response.ts its daemon-sessions layer claim

* refactor(daemon): pin the device-status wire order and cite the shared builder

Review follow-up (#3557): the contracts doc comments now cite the shared
deviceStatusReportResponse the literals moved into, and the boot-shape test
pins Object.keys so a wire key-order change is a test failure.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ready-for-human Valid work that needs human implementation, judgment, or maintainer merge

Projects

None yet

Development

Successfully merging this pull request may close these issues.

CRAP: daemon handlers and server (needs splits)

1 participant