Repository navigation
fix(native-chat): enforce each pending send's own boundary in glue matching (STA-4477) #14935
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
9fef6a6
ee94f1b
e57a335
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,111 @@ | ||
| // Locally-recorded slash commands (e.g. `/clear`). They dispatch into the | ||
| // agent's TUI rather than the conversation, so native chat renders its own | ||
| // marker line and applies their transcript boundaries — kept out of the | ||
| // optimistic-send pruning in native-chat-pending.ts, which is a separate rule. | ||
|
|
||
| import type { NativeChatMessage } from '../../../../shared/native-chat-types' | ||
| import { setBoundedScopeCacheEntry } from './native-chat-composer-scope-cache' | ||
|
|
||
| /** A locally-recorded slash command (e.g. `/clear`). Slash commands dispatch to | ||
| * the agent's TUI and are not chat turns, so we surface a small system line as | ||
| * feedback that the command ran rather than echoing a user bubble. */ | ||
| export type NativeChatCommandMarker = { | ||
| id: string | ||
| /** The command as typed, e.g. `/clear`. */ | ||
| command: string | ||
| sentAt: number | ||
| } | ||
|
|
||
| export type NativeChatCommandMarkerScope = { | ||
| paneKey: string | ||
| agent: string | ||
| sessionId: string | null | ||
| } | ||
|
|
||
| const COMMAND_MARKER_LIMIT = 8 | ||
| const commandMarkerCache = new Map<string, NativeChatCommandMarker[]>() | ||
| let commandMarkerCounter = 0 | ||
|
|
||
| function commandMarkerScopeKey(scope: NativeChatCommandMarkerScope): string { | ||
| return `${scope.paneKey}\0${scope.agent}\0${scope.sessionId ?? ''}` | ||
| } | ||
|
|
||
| export function readCommandMarkerCache( | ||
| scope: NativeChatCommandMarkerScope | ||
| ): NativeChatCommandMarker[] { | ||
| return [...(commandMarkerCache.get(commandMarkerScopeKey(scope)) ?? [])] | ||
| } | ||
|
|
||
| export function appendCommandMarkerCache( | ||
| scope: NativeChatCommandMarkerScope, | ||
| command: string, | ||
| sentAt = Date.now() | ||
| ): NativeChatCommandMarker[] { | ||
| commandMarkerCounter += 1 | ||
| const key = commandMarkerScopeKey(scope) | ||
| // Why: native/TUI view switches remount the chat surface, but slash commands | ||
| // are not transcript turns, so their local feedback needs a pane-scoped cache. | ||
| const next = [ | ||
| ...(commandMarkerCache.get(key) ?? []), | ||
| { id: `${sentAt}-${commandMarkerCounter}`, command, sentAt } | ||
| ].slice(-COMMAND_MARKER_LIMIT) | ||
| // Why: the per-key array is capped at 8, but the KEY (paneKey\0agent\0sessionId, | ||
| // sessionId changes on every /clear) is ephemeral and was never evicted, so it | ||
| // grew one entry per (pane, session) for the renderer's whole life. LRU-bound | ||
| // the key count (mirrors the #7566 draft/attachment caches in this folder). | ||
| setBoundedScopeCacheEntry(commandMarkerCache, key, next) | ||
| return [...next] | ||
| } | ||
|
|
||
| export function clearCommandMarkerCacheForTests(): void { | ||
| commandMarkerCache.clear() | ||
| commandMarkerCounter = 0 | ||
| } | ||
|
|
||
| function isClearCommand(command: string): boolean { | ||
| return command.trim().toLowerCase().split(/\s+/)[0] === '/clear' | ||
| } | ||
|
|
||
| function latestClearSentAt(markers: readonly NativeChatCommandMarker[]): number | null { | ||
| let latest: number | null = null | ||
| for (const marker of markers) { | ||
| if (isClearCommand(marker.command) && (latest === null || marker.sentAt > latest)) { | ||
| latest = marker.sentAt | ||
| } | ||
| } | ||
| return latest | ||
| } | ||
|
|
||
| export function applyCommandMarkerBoundaries( | ||
| messages: readonly NativeChatMessage[], | ||
| markers: readonly NativeChatCommandMarker[] | ||
| ): NativeChatMessage[] { | ||
| const clearSentAt = latestClearSentAt(markers) | ||
| if (clearSentAt === null) { | ||
| return messages as NativeChatMessage[] | ||
| } | ||
| // Why: `/clear` mutates the TUI/transcript asynchronously. Hide the current | ||
| // transcript immediately so native chat reflects the command before the agent | ||
| // writes a replacement session or truncates the file. | ||
| return messages.filter((message) => message.timestamp !== null && message.timestamp > clearSentAt) | ||
| } | ||
|
|
||
| /** Render command markers as compact `system` messages. The `system` role draws | ||
| * as a muted aside (not a user bubble); the text avoids the harness noise | ||
| * prefixes so stripNoiseMessages keeps it. */ | ||
| export function commandMarkersAsMessages( | ||
| markers: readonly NativeChatCommandMarker[] | ||
| ): NativeChatMessage[] { | ||
| return markers.map((marker) => ({ | ||
| id: `command:${marker.id}`, | ||
| role: 'system' as const, | ||
| blocks: [{ type: 'text' as const, text: `Ran ${marker.command}` }], | ||
| timestamp: marker.sentAt, | ||
| source: 'scrape' as const | ||
| })) | ||
| } | ||
|
|
||
| /** True when a message id was minted for a slash-command marker. */ | ||
| export function isCommandMarkerId(id: string): boolean { | ||
| return id.startsWith('command:') | ||
| } | ||
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -79,17 +79,21 @@ export function advancedNativeChatUserContentCounts( | |||||||||||||||||||
| return advanced | ||||||||||||||||||||
| } | ||||||||||||||||||||
|
|
||||||||||||||||||||
| /** User texts that already have a later non-user turn (ready to prune echoes). */ | ||||||||||||||||||||
| export function advancedNativeChatUserTexts( | ||||||||||||||||||||
| /** A transcript user turn, kept identifiable so a caller can decide which | ||||||||||||||||||||
| * pending sends it is allowed to represent. */ | ||||||||||||||||||||
| export type NativeChatUserRow = { id: string; text: string } | ||||||||||||||||||||
|
|
||||||||||||||||||||
| /** User rows that already have a later non-user turn (ready to prune echoes). */ | ||||||||||||||||||||
| export function advancedNativeChatUserRows( | ||||||||||||||||||||
| messages: readonly NativeChatMessage[] | ||||||||||||||||||||
| ): readonly string[] { | ||||||||||||||||||||
| const advanced: string[] = [] | ||||||||||||||||||||
| const waiting: string[] = [] | ||||||||||||||||||||
| ): readonly NativeChatUserRow[] { | ||||||||||||||||||||
| const advanced: NativeChatUserRow[] = [] | ||||||||||||||||||||
| const waiting: NativeChatUserRow[] = [] | ||||||||||||||||||||
| for (const message of messages) { | ||||||||||||||||||||
| if (message.role === 'user') { | ||||||||||||||||||||
| const text = normalizedNativeChatUserMessageText(message) | ||||||||||||||||||||
| if (text) { | ||||||||||||||||||||
| waiting.push(text) | ||||||||||||||||||||
| waiting.push({ id: message.id, text }) | ||||||||||||||||||||
| } | ||||||||||||||||||||
| continue | ||||||||||||||||||||
| } | ||||||||||||||||||||
|
|
@@ -99,18 +103,18 @@ export function advancedNativeChatUserTexts( | |||||||||||||||||||
| return advanced | ||||||||||||||||||||
| } | ||||||||||||||||||||
|
|
||||||||||||||||||||
| /** All user texts (for hiding optimistic echoes once the turn exists). */ | ||||||||||||||||||||
| export function matchingNativeChatUserTexts( | ||||||||||||||||||||
| /** All user rows (for hiding optimistic echoes once the turn exists). */ | ||||||||||||||||||||
| export function matchingNativeChatUserRows( | ||||||||||||||||||||
| messages: readonly NativeChatMessage[] | ||||||||||||||||||||
| ): readonly string[] { | ||||||||||||||||||||
| const texts: string[] = [] | ||||||||||||||||||||
| ): readonly NativeChatUserRow[] { | ||||||||||||||||||||
| const rows: NativeChatUserRow[] = [] | ||||||||||||||||||||
| for (const message of messages) { | ||||||||||||||||||||
| const text = normalizedNativeChatUserMessageText(message) | ||||||||||||||||||||
| if (text) { | ||||||||||||||||||||
| texts.push(text) | ||||||||||||||||||||
| rows.push({ id: message.id, text }) | ||||||||||||||||||||
| } | ||||||||||||||||||||
| } | ||||||||||||||||||||
| return texts | ||||||||||||||||||||
| return rows | ||||||||||||||||||||
| } | ||||||||||||||||||||
|
|
||||||||||||||||||||
| /** | ||||||||||||||||||||
|
|
@@ -149,32 +153,49 @@ export function countLeadingPendingTextsGluedToUserText( | |||||||||||||||||||
| return 0 | ||||||||||||||||||||
| } | ||||||||||||||||||||
|
|
||||||||||||||||||||
| /** A transcript row a glue match may consume, carrying the send boundaries it | ||||||||||||||||||||
| * satisfies — this matcher has no clock of its own. */ | ||||||||||||||||||||
| export type NativeChatGluedUserRow = { | ||||||||||||||||||||
| text: string | ||||||||||||||||||||
| /** Indices into `pending` this row landed after. A row that already existed | ||||||||||||||||||||
| * when a send was issued can never be that send's echo. */ | ||||||||||||||||||||
| representablePendingIndices: ReadonlySet<number> | ||||||||||||||||||||
| } | ||||||||||||||||||||
|
|
||||||||||||||||||||
| /** | ||||||||||||||||||||
| * Mark pending entries represented only by multi-send glue (2+ consecutive | ||||||||||||||||||||
| * optimistic texts concatenated into one transcript user row). Exact single | ||||||||||||||||||||
| * matches stay in the content-key/occurrence path so repeated prompts and | ||||||||||||||||||||
| * send boundaries keep their existing semantics. | ||||||||||||||||||||
| * | ||||||||||||||||||||
| * `userTexts` must already be filtered to rows after the oldest entry's send | ||||||||||||||||||||
| * boundary — this matcher has no clock of its own. | ||||||||||||||||||||
| */ | ||||||||||||||||||||
| export function selectPendingIndicesRepresentedByUserTexts( | ||||||||||||||||||||
| export function selectPendingIndicesRepresentedByUserRows( | ||||||||||||||||||||
| pending: readonly NativeChatPendingOccurrence[], | ||||||||||||||||||||
| userTexts: readonly string[] | ||||||||||||||||||||
| rows: readonly NativeChatGluedUserRow[] | ||||||||||||||||||||
| ): Set<number> { | ||||||||||||||||||||
| const represented = new Set<number>() | ||||||||||||||||||||
| if (pending.length < 2 || userTexts.length === 0) { | ||||||||||||||||||||
| if (pending.length < 2 || rows.length === 0) { | ||||||||||||||||||||
| return represented | ||||||||||||||||||||
| } | ||||||||||||||||||||
| const remaining = pending.map((entry, index) => ({ | ||||||||||||||||||||
| index, | ||||||||||||||||||||
| text: normalizeNativeChatPendingText(entry.text) | ||||||||||||||||||||
| })) | ||||||||||||||||||||
| for (const userText of userTexts) { | ||||||||||||||||||||
| const open = remaining.filter((entry) => !represented.has(entry.index) && entry.text.length > 0) | ||||||||||||||||||||
| for (const row of rows) { | ||||||||||||||||||||
| const open: typeof remaining = [] | ||||||||||||||||||||
| for (const entry of remaining) { | ||||||||||||||||||||
| if (represented.has(entry.index) || entry.text.length === 0) { | ||||||||||||||||||||
| continue | ||||||||||||||||||||
| } | ||||||||||||||||||||
|
Comment on lines
+186
to
+188
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win Stop the glue run at an unresolved empty-text send.
Keep skipping already represented entries. Break when an unresolved entry has empty normalized text. Proposed fix- if (represented.has(entry.index) || entry.text.length === 0) {
+ if (represented.has(entry.index)) {
continue
}
+ if (entry.text.length === 0) {
+ break
+ }📝 Committable suggestion
Suggested change
|
||||||||||||||||||||
| // Glue consumes a leading run, so a send this row predates ends the run | ||||||||||||||||||||
| // rather than being skipped over — adjacency is what makes it glue. | ||||||||||||||||||||
| if (!row.representablePendingIndices.has(entry.index)) { | ||||||||||||||||||||
| break | ||||||||||||||||||||
| } | ||||||||||||||||||||
| open.push(entry) | ||||||||||||||||||||
| } | ||||||||||||||||||||
| const gluedCount = countLeadingPendingTextsGluedToUserText( | ||||||||||||||||||||
| open.map((entry) => entry.text), | ||||||||||||||||||||
| userText | ||||||||||||||||||||
| row.text | ||||||||||||||||||||
| ) | ||||||||||||||||||||
| // Why: gluedCount === 1 is an exact match — leave it to occurrence counting. | ||||||||||||||||||||
| if (gluedCount < 2) { | ||||||||||||||||||||
|
|
||||||||||||||||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Shorten the cache-history comment.
Lines 52-55 include historical detail that the code does not need. Keep one brief comment that states why scope keys need bounding.
Proposed change
As per coding guidelines: “Comments must be concise, non-obvious, and brief—prefer one line; do not explain obvious behavior or walk through code.”
📝 Committable suggestion
Source: Coding guidelines