feat(desktop): reference bounded Session snapshots from Composer - #4446
feat(desktop): reference bounded Session snapshots from Composer#4446testikun wants to merge 27 commits into
Conversation
Add a read-only same-Host Session picker to the Composer and carry bounded transcript snapshots as provenance-preserving QuoteRefs. Keep snapshot reads redacted, bounded, reconnectable, and safe across owner changes. Generated-by: OpenAI Codex
|
Implementation note / 实现说明 This PR keeps the scope intentionally narrow: selecting The snapshot is built from the Runtime Host transcript tail and keeps only non-empty user/assistant text. Same-Host filtering happens before selection; the IPC boundary rejects archived Sessions; omitted older transcript pages are preserved as The Composer keeps Session references separate from serialized draft text and renders them with a distinct book icon. Existing file references and Skill mentions keep their previous behavior. 中文摘要: 本 PR 按 issue 的窄范围实现:在 Composer 的 |
Generated-by: OpenAI Codex
This reverts commit 5025030. Generated-by: OpenAI Codex
Generated-by: OpenAI Codex
Generated-by: OpenAI Codex
Generated-by: OpenAI Codex
Generated-by: OpenAI Codex
d81736a to
ada4863
Compare
…ion-reference # Conflicts: # apps/desktop/src/renderer/composition/desktop-feature-services.tsx
|
No description provided. |
…ion-reference # Conflicts: # apps/desktop/renderer-architecture.json # apps/desktop/src/renderer/app-shell.tsx # docs/astryx-surface-file-inventory.md
…ion-reference # Conflicts: # apps/desktop/renderer-architecture.json # apps/desktop/src/renderer/composition/desktop-feature-services.tsx # docs/astryx-surface-file-inventory.md
…ion-reference # Conflicts: # apps/desktop/renderer-architecture.json # apps/desktop/src/renderer/features/conversation/index.ts # patches/@astryxdesign+core+0.5.2.patch
me2seeks
left a comment
There was a problem hiding this comment.
Review
Method: traced the full path renderer → preload → ScopedIpcMain → main-process IPC → Runtime Host client → server subscription; built @maka/core at this head and ran the new tests locally (session-reference, events: 9/9 pass; the new isQuoteRef assertions fail on main, so they pin the change). Two suspected issues were checked and cleared before listing anything: the extra scope argument in preload is consumed and validated by ScopedIpcMain (not a parameter-order bug), and Session names are capped at 80 code points (SESSION_NAME_MAX_CODE_POINTS), so Session: <name> cannot overflow the 200-char quote-label limit at the send boundary.
Findings
P1 — Branch conflicts with main and carries an unrelated change. packages/cli/src/__tests__/runtime-host-local-target-activation.test.ts bumps settlementTimeoutMs 50 → 500 (commit 96de6d6), which is unrelated to Session references. main already fixed the same test differently in #4469, and the two edits textually conflict. On rebase, drop this hunk and take main's version; regenerate docs/astryx-surface-file-inventory.md to clear the second conflict. Please keep the PR scoped to the feature.
P2 — The e2e verification case is not committed. Verification describes one real-Electron Playwright test covering @, trailing @ , name selection, the MessagesSquare icon, and the staged chip, but no spec under apps/desktop/e2e/ is in this diff. Recent practice (#3981) commits exactly this class of interaction regression as an e2e spec. Either commit the spec or state why unit/integration coverage is sufficient here.
P2 — useComposerQuotes' mutable bucket needs a guard rail. use-composer-quotes.ts:34 mutates a ref during render and publish() shallow-copies the map while sharing bucket array references. The live array is what makes same-tick send observe a freshly picked snapshot (the "send waits for the snapshot" test pins this), but the array's identity now survives across mutations: any future useMemo/useEffect keyed on pendingQuotes silently goes stale. I verified no current consumer does this. Cheapest fix now: document on the returned pendingQuotes that its identity must not be used as a memo/effect dependency; alternatively schedule a follow-up to replace closure capture with an explicit getPendingQuotes(draftKey) accessor in the send path.
P3 — Truncation can split a surrogate pair. session-reference.ts:119 (text.slice(0, contentBudget)) and the join slice at :130 cut at UTF-16 code-unit boundaries; a message containing emoji at the budget edge yields a lone surrogate in model context. Backing off to a surrogate boundary after slicing is enough.
P3 — The vendored useTriggerMenu hunk should be tracked upstream. The @-across-whitespace behavior rides the @astryxdesign/core patch (patches/README.md documents the removal condition — good). The old Composer comment already said "the fix belongs upstream"; please file the upstream issue/PR so the hunk can eventually be deleted.
Production code that can be deleted
None identified. composer-mentions.tsx and use-app-shell-composer-quotes.ts shrink to thin compatibility entries; the moved logic lands in the existing feature-services pattern rather than a parallel path.
Low-quality tests to delete or replace
None identified. The new tests assert behavior, not implementation: redaction of tool/system records, truncation accounting including the role-prefix edge, quote-attribute escaping against tag injection (<research>), subscription close semantics, and the all-or-nothing provenance validation at both the codec and the IPC boundary.
Review-relevant risks
QuoteRef is part of the event log and wire contract; this PR extends it with optional fields (old payloads still validate against the widened shape). The @ menu now also stays open across whitespace for file mentions, a user-visible behavior change beyond Session references — stated in the Summary and consistent with the AND-of-substring matcher. Per CONTRIBUTING.md, material user-visible and contract changes require independent human review; this automated review is not an approval.
Conclusion
- The solution is optimal for the actual problem: it reuses the QuoteRef transport and the bounded-tail subscription instead of creating a parallel channel, and the three budget layers (Core 12k/32k, IPC 32k, renderer 32k) agree.
- Deletable production code: none identified.
- Deletable/replaceable tests: none identified.
- Deeper refactor: not required; the
features/conversationextraction follows the established services pattern. - Merge readiness: not yet — the P1 conflict/unrelated hunk must be resolved and the two P2 items answered or planned.
- Residual risks: the
@-menu behavior change for file mentions is the main product call; cross-version quote-field skew is bounded by theshared !== truepicker filter and co-versioned desktop/Host releases.
…ion-reference # Conflicts: # docs/astryx-surface-file-inventory.md # packages/cli/src/__tests__/runtime-host-local-target-activation.test.ts
|
Thanks for the review. Addressed in
The Local verification: targeted Electron E2E passed, Core snapshot/events tests passed, and Desktop/UI/Core typechecks passed. |
…ion-reference # Conflicts: # docs/astryx-surface-file-inventory.md
hqhq1025
left a comment
There was a problem hiding this comment.
Review
Reviewed exact head 6c8d9ef905e05e77072d07af4e63c1131bfb23e7.
This change adds same-Host, non-shared Session discovery to the Composer @ picker, reads a bounded read-only transcript snapshot through Runtime Host IPC, stores source/timestamp/truncation provenance in QuoteRef, and folds that snapshot into the active model context. It also updates the Astryx trigger patch so @ search can span spaces and adds the corresponding unit, IPC, and Electron coverage.
Finding
One P1 trust-boundary issue is attached inline: retained user and assistant messages bypass the repository's canonical secret redaction, so a selected Session can forward credentials verbatim into the active Session's model request. This directly violates the accepted scope for credential filtering.
Verification
- Clean
npm ciat the exact head, including all dependency patches build:testand full typecheck passed- Core: 777 passed; Runtime: 3,155 passed / 13 skipped; UI: 333 passed
- Desktop: 1,998 passed / 8 cancelled / 0 failed; the cancellations are the existing MCP OAuth timer cases
- Focused Session-reference, wire, IPC, Composer, and model-context suites: 63/63 passed
- Real Electron Session-reference E2E: 3/3 repeated runs passed after the clean install
- Clean synthetic merge onto current
main9d4002b38239d1f07cec57293b2633e506575139: build, full typecheck, 67 focused tests, renderer architecture 71/71 plus ledger, Biome, ASF headers, and diff checks passed - The exact-head secret probe showed
Authorization: Bearer sk-live-secret-token-valueunchanged in bothcreateSessionSnapshot()andformatTextWithInlineRefs()output, whileredactSecrets()returnsAuthorization: Bearer [redacted]
The hosted audit and windows_recovery checks are green. The required hosted test check is red in apps/desktop/e2e/code-scroll.spec.ts (an untouched file), so I am not treating the check set as green. I did not run native macOS/Windows UI coverage or a live third-party provider call; the model-facing formatter itself was exercised directly.
Conclusion: not merge-ready until the credential-redaction finding is fixed and re-reviewed on the resulting head.
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.
…ion-reference # Conflicts: # apps/desktop/renderer-architecture.json # docs/astryx-surface-file-inventory.md
Generated-by: OpenAI Codex
018a577 to
660b48e
Compare
Summary
Fixes #4309
Add a read-only Session reference to the Composer
@menu.MessagesSquareconversation icon and show the Session name only.@keeps the complete list open; whitespace-separated input searches Session names only./commands keep their previous behavior.The snapshot projection contains only non-empty user and assistant text. Tool calls, tool results, system records, and permission records stay out of the projection. The default snapshot budget is 12,000 characters (hard maximum 32,000), and omitted older transcript pages remain visible through the
truncatedprovenance flag.Verification
npm --workspace @maka/ui run typechecknpm --workspace @maka/ui run buildnode scripts/apply-dependency-patches.mjsapplies all dependency patches successfullynpm --workspace @maka/core run typecheck@, trailing@, name selection, theMessagesSquareicon, and the staged Session chipReal-window screenshots from the current PR build:
AI use
Tool(s) and scope: OpenAI Codex assisted with issue analysis, implementation, regression tests, and local verification. The human contributor remains responsible for review, accuracy, and submission.
Checklist
Does this PR entail a change in behavior?