feat(desktop): reference bounded Session snapshots from Composer - #4446
feat(desktop): reference bounded Session snapshots from Composer#4446testikun wants to merge 22 commits into
Conversation
d81736a to
ada4863
Compare
|
No description provided. |
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.
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.
018a577 to
660b48e
Compare
f09b325 to
d4bbb4b
Compare
b054be5 to
7c5f2b2
Compare
|
Follow-up: the implementation findings are resolved in the current head (unrelated CLI hunk removed, Electron coverage committed, pendingQuotes identity contract documented, surrogate-safe truncation added, and canonical secret redaction applied to retained user/assistant text). The remaining maintenance item is now tracked upstream as facebook/astryx#6110: useTriggerMenu needs a trigger-specific whitespace policy so @ can support multi-word names and trailing-space browsing without changing / command grammar. Focused Core Session-reference tests pass 7/7 on the current head. @Astro-Han could you please re-review with that upstream tracking link in place? |
Astro-Han
left a comment
There was a problem hiding this comment.
Thanks for keeping the feature read-only and using the Host snapshot and existing QuoteRef channel. The current head is not yet connected end to end: three P1 findings are inline, covering production service composition, the send/quote commit boundary, and wire compatibility. I independently rebuilt/reran the focused Electron journey and confirmed startup fails with ConversationServicesProvider is missing; old screenshots cannot establish acceptance of this head.
[P2 · normal user path] The approved snapshot contract also needs to be reconciled across capture and display. #4309 explicitly settled on resolving the snapshot when the message is sent. This implementation captures on selection; if the source continues running before Send, the submitted snapshot is already older. Neither the staged token nor the sent QuoteRef chip exposes the persisted sourceCapturedAt/sourceTruncated fields, so the user cannot distinguish that older/bounded excerpt from a current complete reference. Please settle the snapshot at the existing send boundary and expose capture/truncation provenance through the existing chip/preview. If selection-time freezing is intentional, agree that change explicitly and make the frozen time visible. This is one cross-layer contract, not a request for another state owner.
After those fixes, refresh the architecture ledger and Astryx inventory and rerun the real app journey. The current inventory generator reports the two new conversation files missing from the generated Markdown inventory. The multiword @ path also still invokes file search, unlike the stated Session-only whitespace scope; please reconcile that scope while validating the actual menu, rather than relying on the source-regex test. No new component framework is needed: the existing Astryx primitives are suitable.
中文
只读Host快照与既有QuoteRef通道的方向正确,但生产链尚未接通。三条P1分别是缺少生产Provider、发送未等待quote读取、严格wire扩展未提升兼容版本。主审独立复跑真实Electron后确认启动即报ConversationServicesProvider缺失。
另有跨层P2:已批准方案要求发送时读取,目前却在选择时冻结,且staged/sent chip不显示已有capture time与truncated字段;来源继续运行或内容被截断时,用户无法判断本条引用实际包含什么时点、是否完整。建议在既有发送权威处完成快照,并用现有chip/preview展示来源信息;若要保留选择时冻结,先明确设计变更并展示冻结时间。
修复后再更新architecture ledger/Astryx清单并重跑真实App。含空格的查询仍调用文件搜索,也应与声明的Session-only范围对齐。当前原语选型本身无需另造组件体系。
AI-assisted review by three fresh reviewers and the coordinating Codex agent. Startup was independently reproduced; the later send/compatibility findings are source-traced and require production-path regressions after startup is repaired.
Astro-Han
left a comment
There was a problem hiding this comment.
Thanks. The independent design/simplification pass on c95e5f426 supports the narrow Host snapshot → existing QuoteRef direction, but the current production composition and send boundary remain incomplete (the three existing P1 threads still apply).
Please converge on one invariant: the send owner resolves the selected source once, then commits that complete MessageContent; failure must not submit a message without its selected reference. The accepted proposal, confirmed by the maintainer, specifies send-time capture. Selection-time capture silently changes that behavior when the source keeps generating; staged/sent previews also need to expose capture/truncation rather than retaining those facts only for the model. This is the existing P2 design gap, not a request for a second snapshot lifecycle.
There is concrete removable scope: use the metadata from the authoritative openSession snapshot instead of a preceding getSession; stop transporting items, estimatedTokens and maxChars when the sole production converter only consumes reference/text/truncated; remove newly introduced internal compatibility aliases once their static callers move. The broader Composer service migration needs either complete production wiring or a narrower use of existing seams, not a half-connected provider plus test-only wiring.
Replace the immediate-send helper test that manually calls waitForPending() with a real send-entry regression. A test supplying the step missing from production cannot prove this contract. The diff size itself is not the finding; incomplete composition and duplicate representations are.
中文
应统一为一个发送契约:现有 send owner 在发送时读取选定来源一次,构造完整 MessageContent 后再提交;读取失败不能发出缺引用消息。发送时捕获已由 issue 定案,当前选择时捕获会在来源继续输出时偏离承诺,UI 也缺捕获时间/截断展示。三个既有 P1 仍成立。可删前置重复 getSession、无生产消费者的快照字段和内部兼容别名;Composer 服务搬迁要完整接线或收缩范围。用真实发送入口回归替代手动补 wait 的 helper 自证,不要再叠快照生命周期。
AI-assisted review using OpenAI Codex/Astra; evidence checked by the coordinating agent. This is not an independent human review.
c95e5f4 to
e7be99d
Compare
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
Generated-by: OpenAI Codex
Generated-by: OpenAI Codex
Generated-by: OpenAI Codex
Generated-by: OpenAI Codex
0de9539 to
dfac3c1
Compare
99e526d to
e8fe049
Compare
hqhq1025
left a comment
There was a problem hiding this comment.
Reviewed exact head e8fe0491a7c7170be0b3a05a00b567b977bed3c7.
This revision now resolves selected Session references at the send boundary, wires the production conversation services, advances the strict QuoteRef compatibility epoch, redacts retained user/assistant text before cross-session projection, and displays capture/truncation provenance.
One P2 concurrency finding is attached inline. While the snapshot read is in flight, the pending Session controls remain interactive, but the operation commits the array captured before the await. A removed reference can therefore still be sent to the model, while a newly selected reference can be silently discarded when the old batch completes.
Verification completed on this exact head: clean install; build:test; full Core, UI, Runtime, and Desktop test suites; focused Session-reference/wire/IPC tests (26/26); the real Electron Session-reference E2E (1/1); full typecheck, lint, format, ASF headers, and diff checks; and a clean merge tree against current main 8d5c4612c46b19270f00fe7aea33c39dff23dbe5. The full Runtime Host suite had its single unchanged managed-Bash sandbox failure on this runner (1,818 passed / 12 skipped / 1 failed). Hosted test, package, and two installed-CLI jobs were still in progress at publication; the remaining hosted checks were green.
Conclusion: not merge-ready until the pending-reference transaction is made consistent and covered with deferred-read add/remove regressions.
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.
| } | ||
| if (!options.addQuote) return false; | ||
| for (const snapshot of snapshots) options.addQuote(sessionSnapshotToQuote(snapshot)); | ||
| pendingReferencesRef.current = []; |
There was a problem hiding this comment.
P2 — Keep pending Session mutations consistent with the in-flight send. waitForPending() captures the current array before awaiting the snapshot reads, but this unconditional clear commits that stale batch afterward. The Composer disables only Send during this wait; the input and each pending Session token remain interactive. In a compiled-hook reproduction, selecting A, sending, removing A before readSnapshot(A) resolved, and then resolving it returned true and appended A to the outgoing quote bucket. Selecting B during the same wait displayed [A, B], but only A was read and this line cleared B. Thus an explicitly removed cross-session excerpt can still reach the model, or a newly selected reference can be silently lost. Freeze those controls while the send owns the batch, or reconcile add/remove mutations against the operation generation, and cover both deferred-read cases.
There was a problem hiding this comment.
Thanks for the exact reproduction. I confirmed this is still unresolved at the current head: waitForPending() captures selected, while removePendingReference() does not invalidate the operation; after deferred reads, lines 157-158 clear the live pending list and append the stale snapshots. I am leaving this thread open. Should the intended fix freeze pending controls for the send, or reconcile add/remove mutations by operation generation?
hqhq1025
left a comment
There was a problem hiding this comment.
Supplemental review on exact head e8fe0491a7c7170be0b3a05a00b567b977bed3c7. The retained transcript text is now redacted, but one P1 cross-provider disclosure remains in the Session-name provenance path, attached inline. The previously reported pending-reference concurrency P2 also remains.
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.
| export function sessionSnapshotToQuote(snapshot: SessionSnapshot): QuoteRef { | ||
| return { | ||
| text: snapshot.text, | ||
| label: `Session: ${snapshot.reference.sessionName}`, |
There was a problem hiding this comment.
[P1] Redact the Session name before placing it in the model-visible quote label. The body is now passed through redactSecrets(), but snapshot.reference.sessionName is copied here unchanged and Runtime serializes q.label into <quoted_excerpt label=...>. This is reachable without a manual rename: when title generation fails, fallbackSessionTitle() uses the first non-empty line of the user's message as the Session name. On this exact build, a first message of sk-live-secret-token-value produced snapshot text User: [redacted] but model input containing label="Session: sk-live-secret-token-value". A source Session using one provider can therefore disclose a credential-shaped title to the target provider despite the new body redaction. Apply the same redaction at this projection boundary and add a regression that runs the fallback title through sessionSnapshotToQuote() and formatTextWithInlineRefs().
There was a problem hiding this comment.
Thanks, confirmed this remains unresolved at the current head. createSessionSnapshot() redacts message bodies, but sessionSnapshotToQuote() still copies sessionName into both the model-visible label and source metadata, so a fallback title can disclose the original first line. I am leaving this open. Should redaction apply at this projection boundary to the label and source name, with the fallback-title regression you described?
hqhq1025
left a comment
There was a problem hiding this comment.
Additional exact-head findings from the contract and dependency-patch pass. These are separate from the previously published pending-reference race and Session-name disclosure.
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.
| typeof record.sourceSessionName === 'string' && | ||
| record.sourceSessionName.length > 0 && | ||
| record.sourceSessionName.length <= QUOTE_REF_SESSION_NAME_MAX_LENGTH && | ||
| typeof record.sourceCapturedAt === 'number' && |
There was a problem hiding this comment.
[P2] Bound sourceCapturedAt to the ECMAScript Date range before accepting this persisted/wire payload. Both this validator and the Desktop send guard accept any finite non-negative number, including Number.MAX_VALUE, but QuoteRefChip immediately calls new Date(value).toISOString(). The exact built modules report isQuoteRef(...) === true and then throw RangeError: Invalid time value; because the app-level ErrorBoundary wraps the shell, one accepted QuoteRef can replace the conversation UI with the error surface. Reject values outside 0..8.64e15 (or validate new Date(value).getTime()) and cover the same boundary in Core, IPC, and provenance rendering tests.
There was a problem hiding this comment.
Confirmed this is still unresolved at the current head. isQuoteRef() accepts any finite non-negative sourceCapturedAt, while quoteProvenanceSummary() still calls new Date(value).toISOString(); Number.MAX_VALUE is therefore accepted and throws RangeError. I am leaving this open. Should the Date-range check be enforced in Core and mirrored at the IPC/Desktop boundary as proposed?
| lastMessagePreview: source.lastMessagePreview, | ||
| } satisfies SessionReferenceSession; | ||
| if (!pendingReferencesRef.current.some((reference) => reference.id === selected.id)) { | ||
| const next = [...pendingReferencesRef.current, selected]; |
There was a problem hiding this comment.
[P2] Enforce the send contract's quote limit while staging Session references. This path only deduplicates and appends, while the picker can expose 25 Sessions and the main-process guard rejects more than 16 total quotes. On this exact build, normalizeSessionSendCommand accepts 16 and throws Invalid send quotes for 17. A user can therefore select and await 17 valid snapshots only to have the real sessions:send boundary reject the message; existing non-Session quotes reduce the available count further. Cap against the current quote bucket before starting reads and expose a disabled/actionable state, then add a 16/17 boundary regression through the send entry.
There was a problem hiding this comment.
Confirmed this is still unresolved at the current head. The Session-reference staging path appends snapshots without checking the existing quote bucket, while the send guard rejects more than 16 total quotes. I am leaving this open. Should the picker cap the combined Session and existing quote count and expose a disabled/actionable state at the boundary?
| + // 'balanced' the 100px layer started 90px inside the opaque composer, so | ||
| + // the ramp was invisible wherever the composer painted and the transcript | ||
| + // stayed crisp right up to the dock's top edge. | ||
| + height: '100%', |
There was a problem hiding this comment.
[P2] Keep this dock-fade geometry hunk when regenerating the Astryx patch. patches/README.md still documents that the fixed per-density heights leave 134px of transcript crisp under the composer and explicitly says to delete the hunk only when Astryx ships the behavior. The dependency is still 0.5.2, and a clean install with this new patch restores height: 80/100/120 with no negative z-index in node_modules/@astryxdesign/core/src/Chat/ChatLayout.tsx. Thus this unrelated patch regeneration reintroduces the visible #3446 regression on every ChatLayout surface. Regenerate the patch from a tree that retains height: '100%', zIndex: -1, and the removal of the density-specific heights.
There was a problem hiding this comment.
Confirmed this remains unresolved at the current head. The 0.5.2 patch contains the newer ChatLayout changes but no retained height: 100% and zIndex: -1 geometry hunk, while patches/README.md still requires that hunk for #3446. I am leaving this open. Should the patch be regenerated from a tree that retains that geometry fix?
hqhq1025
left a comment
There was a problem hiding this comment.
The required hosted test check has now completed red on this exact head. The failure is attributable to the Composer mentions service extraction and removes four existing Storybook interaction checks, as detailed inline.
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.
e8fe049 to
a2e3ffe
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
Current head
13f6f897bc67a2871eccc0e7b9d71b6b9c174fc0is rebased ontoapache/mainata6fd57ba8. The real App journey below was captured from the equivalent pre-rebase feature tree4386b6dcf; the subsequent rebases introduced no feature-file conflicts:npm run buildnpm --workspace @maka/desktop run typecheckgit diff --check 19ac204ab...HEAD@picker, staged its snapshot, sent the follow-up, and verified that the sent message retained the Session reference.Real App screenshots
These screenshots were captured from the real Electron feature verification run described above. They are direct screenshots of the locally built app, not mockups or generated images.
The
@picker displays the real source Session alongside matching workspace files:Selecting the Session stages the provenance-preserving Composer chip:
After send, the message retains the Session reference and completes through the fake E2E backend:
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?