Skip to content

fix: keep named sessions visible in scoped inventory - #2164

Merged
thymikee merged 1 commit into
mainfrom
fix/session-list-explicit-sessions
Aug 31, 2026
Merged

thymikee merged 1 commit into
mainfrom
fix/session-list-explicit-sessions

Conversation

@thymikee

@thymikee thymikee commented Aug 31, 2026 •

Copy link
Copy Markdown
Member

Summary

Keep explicitly named sessions visible in agent-device session list while preserving cwd and tenant isolation.

Session creation now persists explicit provenance (cwd, tenant, named-local, or global-default), and inventory filtering uses that stored fact instead of reconstructing ownership from the session address. This keeps a valid local name such as tenant-a:qa local, prevents tenant inventories from seeing it, and prevents a global unscoped default from leaking into cwd-scoped inventory.

This fixes the reported contradiction where --session qa-cart-integrity remained bound to an Android recording session while session list --json returned an empty list. Externally terminating the emulator still does not silently discard the recording session: record stop retains its recovery opportunity and close remains the explicit fallback.

Also clarify selector conflicts by saying the session is bound to the device, rather than saying the command itself is bound to the session.

Validation

The regressions were observed red before the production change: local inventory omitted tenant-a:qa and leaked global default, while tenant A inventory also leaked the local colon-named session. Tests now cover both directions, cross-worktree filtering, and provenance stored through real tenant and cwd open routes.

At exact head 8394fd7c1f, 39 focused routing, inventory, open, and session-runner tests pass. pnpm check:affected --run passed format, lint, typecheck, layering, fallow, and build; its related-test lane passed 1,423 tests and hit two 5-second provider-scenario timeouts. The recording scenario passed in isolation, and the remaining scripted iOS Settings timeout reproduced unchanged at pre-review head c1ee1e2e33.

Exact-head GitHub CI is green, including integration, coverage, repository guards, CodeQL, package checks, and Android/iOS/Linux/macOS smoke. The iOS workflow exercised its simulator smoke paths; its physical-device step was skipped by workflow policy.

No separate live device run was needed because the changed behavior is deterministic daemon inventory filtering and provenance; recording recovery mechanics are unchanged.

17 files changed, all within the session command family and mirrored tests; scope did not grow beyond that family.

@github-actions

github-actions Bot commented Aug 31, 2026 •

Copy link
Copy Markdown

Size Report

Metric Base Current Diff
JS raw 2.53 MB 2.53 MB -7.2 kB
JS gzip 851.0 kB 847.6 kB -3.4 kB
npm tarball 976.8 kB 973.9 kB -2.9 kB
npm unpacked 3.37 MB 3.37 MB -7.6 kB

npm unpacked components

Component Base Current Diff
JS / dist source 2.69 MB 2.68 MB -7.5 kB
Apple runner source/project 581.2 kB 581.1 kB -65 B
macOS helper source 54.8 kB 54.8 kB 0 B
Android helper artifacts 0 B 0 B 0 B
Other package files 45.6 kB 45.6 kB 0 B

Startup median (7 runs, lower is better):

Scenario Base Current Diff
CLI --version 28.9 ms 28.6 ms -0.3 ms
CLI --help 82.8 ms 83.0 ms +0.2 ms

Top changed chunks:

Chunk Raw diff Gzip diff
dist/src/sdk-batch-runner.js -684 B -273 B
dist/src/session2.js -708 B -256 B
dist/src/registry.js -379 B -147 B
dist/src/cli-help.js -230 B -79 B
dist/src/interaction.js -175 B -79 B

Top changed packed files

Packed file Base Current Diff
dist/src/device-session.js 10.8 kB 0 B -10.8 kB
dist/src/src.js 8.5 kB 19.2 kB +10.7 kB
dist/src/session-allocation.js 2.0 kB 0 B -2.0 kB
dist/src/session-routing.js 0 B 1.6 kB +1.6 kB
dist/src/app-catalog.js 1.6 kB 0 B -1.6 kB
dist/src/screen-recording-resource-recovery.js 19.8 kB 18.8 kB -1.1 kB
dist/src/session2.js 217.0 kB 216.3 kB -708 B
dist/src/app-log-runtime2.js 18.6 kB 17.9 kB -685 B
dist/src/sdk-batch-runner.js 82.1 kB 81.4 kB -684 B
dist/src/registry.js 157.4 kB 157.1 kB -379 B

@thymikee

Copy link
Copy Markdown
Member Author

Reviewed exact head f40d3827049fa61d266a2c7c376239ba0301f8db. P1: the filter widens visibility to every session with no sessionScope, not only explicitly named local sessions. Tenant isolation intentionally prefixes the stored session address but does not assign a cwd scope; a local cwd-scoped session list can therefore expose another tenant’s session address, state/log paths, and device identity. Keep tenant ownership at the SessionRef/address inventory seam and apply it before cwd visibility; absence of sessionScope is not proof of a globally visible local session. Add a regression proving a tenant-owned unscoped session stays hidden, alongside the desired result containing the caller’s current implicit session plus the local explicitly named session while excluding other cwd sessions. Exact-head CI is green; no device proof is needed for this deterministic inventory path, but the isolation finding blocks readiness.

@thymikee
thymikee force-pushed the fix/session-list-explicit-sessions branch from f40d382 to c1ee1e2 Compare August 31, 2026 11:13
@thymikee

Copy link
Copy Markdown
Member Author

Addressed in c1ee1e2e33. The finding was valid: sessionScope === undefined conflated globally named local sessions with tenant-owned sessions.

The inventory now resolves tenant ownership from the canonical SessionRef.address namespace first, then applies cwd visibility only to local sessions. A local request cannot enumerate tenant-owned unscoped sessions, while a tenant request can enumerate only its own namespace.

Regression coverage uses one mixed inventory: the caller's implicit cwd session and a local explicitly named session remain visible; another cwd and a lease-less tenant session stay hidden. A second test proves tenant A cannot see tenant B or global local sessions. The mixed regression was observed failing before the production fix because tenant-a:remote-recording leaked into the response.

Validation: pnpm check:affected --run (200 related test files, 1,198 tests; format, lint, typecheck, layering, fallow, build, and daemon wire compatibility all passed). No device proof is needed for this deterministic inventory path.

@thymikee

Copy link
Copy Markdown
Member Author

Two actionable findings still block readiness at c1ee1e2e33b30f6ff5db7bb4e6fd69b25d5372e0:

  1. Tenant ownership is reconstructed from any unscoped address prefix before :. Local explicit session names permit : (they are sanitized for storage, not rejected), so a normal local --session tenant-a:qa is misclassified as tenant-owned: tenant A can see its device/artifact paths while local inventory hides it. Persist/use explicit tenant provenance instead of parsing an overloaded user-controlled address, with regressions for both exposure directions.

  2. sessionMatchesScope() still treats every unscoped session as matching every cwd scope. That makes a global unscoped default session (created without meta.cwd) visible to unrelated worktrees while trying to expose explicitly named local sessions. The ownership model must distinguish global unscoped default from an explicitly named local session; add the cross-worktree default regression.

The new tenant-prefix test fixes the prior direct case but does not cover either ambiguous ownership shape. No ready-for-human label until both boundaries are explicit and tested on a new head.

@thymikee
thymikee force-pushed the fix/session-list-explicit-sessions branch from c1ee1e2 to 8394fd7 Compare August 31, 2026 12:11
@thymikee

Copy link
Copy Markdown
Member Author

Addressed both findings at 8394fd7c1f by replacing inferred ownership with stored session provenance.

  • Every production session creation path now records one explicit scope: cwd, tenant, named-local, or global-default. Inventory filtering uses only that field and fails closed for unstamped state; it no longer parses tenant ownership from the address.
  • A local explicit session named tenant-a:qa remains visible to local inventory and is excluded from tenant A inventory. A real tenant A session remains visible only to tenant A.
  • CWD inventory includes same-CWD implicit sessions and explicitly named local sessions, but excludes both other-CWD sessions and an unscoped global default.
  • Router-level tests pin provenance on real tenant and CWD open paths, while record, replay-open, and session-creating snapshot paths use the same resolver.

Planted-red evidence: before the production change, the local inventory regression omitted local tenant-a:qa and leaked global default; tenant A inventory also leaked local tenant-a:qa.

Exact-head validation: 39 focused tests pass across the six routing/inventory/open suites. pnpm check:affected --run passed format, lint, typecheck, layering, fallow, and build; its related-test lane passed 1,423 tests and hit two unrelated 5s provider-scenario timeouts. The record scenario passed in isolation, and the remaining iOS Settings timeout reproduces unchanged at pre-review head c1ee1e2e33, so exact-head CI remains the authority for that lane. I have not added ready-for-human pending CI.

@thymikee thymikee added the ready-for-human Valid work that needs human implementation, judgment, or maintainer merge label Aug 31, 2026
@thymikee

Copy link
Copy Markdown
Member Author

Follow-up: all checks on 8394fd7c1f are now green, including Integration Tests, Coverage, Repo Guards, and Android/iOS/Linux/macOS smoke. I updated the PR validation and added ready-for-human.

@thymikee
thymikee merged commit 3982cc8 into main Aug 31, 2026
18 checks passed
@thymikee
thymikee deleted the fix/session-list-explicit-sessions branch August 31, 2026 12:30
@github-actions

Copy link
Copy Markdown
PR Preview Action v1.8.1
Preview removed because the pull request was closed.
2026-08-31 12:30 UTC

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.

1 participant