feat(workhub): inspect existing session conversations - #5886
Conversation
hqhq1025
left a comment
There was a problem hiding this comment.
Reviewed the current head. WorkHubInspect is available only to the WorkHub coordination Session and rechecks the current candidate scope before returning data. It reads durable user/assistant text through a bounded, signed cursor tied to the target and view, while keeping live root-Turn status distinct from transcript content and artifact completion. I found no substantiated P0–P3 issue in the changed path. Node 24 clean npm ci and build:test, 70 focused Runtime Host tests (including the production composition path), diff-check, and a static merge against current main pass. Current-head hosted test and label checks pass. I did not run an external-model journey or manual Desktop smoke test; the Host’s legacy transcript reader can perform its existing compatibility conversion when reading an old Session.\n\n> Automated review notice: This comment was posted by an automated review agent operated by hqhq1025. It is not an independent human review and does not replace one.
Astro-Han
left a comment
There was a problem hiding this comment.
Reviewed at 6419a6f3 with a focus on the trust boundary. It holds: the tool rejects non-coordinator callers and is only wired into the coordinator profile, which has no Task/Agent tool to delegate it; eligibility is re-checked before and after every page (cursors don't bypass it); archived, self, subagent-child and side sessions are excluded with a 32-session cap; cursors are HMAC-signed with a per-Host-process key, compared in constant time, and bound to session + view + fixed transcript watermark; pagination always progresses; output is bounded (~16k chars) and framed as untrusted, matching the existing result tool. The PR's tests are thorough.
No P0–P2. One P3 inline: the inspection tool's session list isn't the same set tasks candidates uses.
Not verified: whether reading execution status inside the target's admission lock is safe beyond the code comment and existing test; any provider-level cap on tool-result size.
Automated review notice: This comment was posted by an automated review agent. It is not an independent human review and does not replace one.
| requestDrain: context.requestDrain, | ||
| }); | ||
| workHubInspection = createWorkHubInspectionTool({ | ||
| listSessions: () => stores.sessionStore.listHeaders(), |
There was a problem hiding this comment.
P3: tasks candidates gets its list from manager.listSessions() → SessionStore.list(), which drops ledger-v0 imports still being prepared and conversationCopy.state === 'preparing' fork/revision copies (packages/storage/src/session-store.ts:607-609). listHeaders() doesn't filter them, and the eligibility check doesn't either, contrary to the "same current bounded discovery scope" in docs/workhub-domain-language.md.
Effects (narrow window, same user's data, so consistency rather than a leak): hidden sessions can occupy slots in the top 32, so a session tasks candidates just returned can come back unavailable while a 33rd the model never saw becomes readable; a half-built copy can be read; a still-importing session passes eligibility and then fails with the raw "Imported Session history is still being prepared" error instead of unavailable.
Suggest requireSessionManager(manager).listSessions() here (or sharing the action gate's candidate function), plus a test with a preparing copy / importing session.
|
Thanks @chihumyum — a careful, well-bounded inspection slice: coordinator-only access, per-page eligibility and signed snapshot cursors made the trust boundary easy to verify. The small session-list consistency note in review can go in a follow-up. Merging now. |
Astro-Han
left a comment
There was a problem hiding this comment.
Approved at @Astro-Han's explicit request: two independent automated reviews of this head found no blocking issues (one minor follow-up noted), and CI is green.
Summary
WorkHub can discover existing Sessions but cannot read their conversation unless it previously delegated work to them. Add the Host-native
WorkHubInspecttool to read recent user/assistant messages or the latest assistant reply within the current same-Host candidate scope.Reads preserve source identities and timestamps, use bounded output with signed snapshot cursors, and report the latest root Turn status separately from task/artifact completion. Inspection does not admit messages or Turns or change target configuration. Only committed conversation text is returned; streaming chunks, thinking and tool output are excluded.
Fixes #5874. This is the independent inspection slice discussed in #5745.
Verification
npm run lint,npm run format:check,npm run build,npm run typecheck.apps/desktopandpackages/ui; strict-base renderer architecture, Astryx surface inventory, Windows test inventory andnpm run check:release.No external-model or manual Desktop smoke test was run; the production integration uses a deterministic local provider.
Checklist
Does this PR entail a change in behavior?