From 7b46e1f73c481dfa9e8f39ad3f133bf08a2d4774 Mon Sep 17 00:00:00 2001 From: Turtle Date: Wed, 22 Jul 2026 16:49:50 +0800 Subject: [PATCH] fix: address PR #504 review warnings - workspace-context: a transiently unavailable but still-effective candidate keeps its cached trimmed digest in the directory's dedup slot, so an identical later sibling is not emitted as a duplicate set until the next successful reconciliation - app-boot: --resume rejects a following token that is itself resume syntax instead of accepting it as a session id - tui: the queued-steering badge tracks per-entry sources and a drain removes one matching entry, so loop-authored steering (no agent/queued) cannot consume a pending user message's slot --- .../context/workspace-context/src/state.ts | 12 +++++- .../tests/workspace-context.spec.ts | 41 +++++++++++++++++++ packages/ui/app-boot/src/index.ts | 4 +- packages/ui/app-boot/tests/app-boot.spec.ts | 5 +++ packages/ui/tui/src/index.ts | 41 +++++++++++-------- packages/ui/tui/tests/tui.spec.ts | 17 +++++++- 6 files changed, 101 insertions(+), 19 deletions(-) diff --git a/packages/context/workspace-context/src/state.ts b/packages/context/workspace-context/src/state.ts index b2f1fa88ef..66b70f639d 100644 --- a/packages/context/workspace-context/src/state.ts +++ b/packages/context/workspace-context/src/state.ts @@ -448,7 +448,17 @@ export async function reconcileInstructionContext( const { directory } = decodeScopeKey(scope) const previous = effective.get(scope) const probe = await probeScopeInstruction(scope, projectRoot, resolved, fileSystem, options.signal) - if (probe.kind === 'unavailable') continue + if (probe.kind === 'unavailable') { + // Last-good-state: the candidate stays effective, so its cached trimmed + // digest must keep occupying the directory's dedup slot — otherwise an + // identical later sibling would be emitted as a duplicate `set` until the + // next successful reconciliation removed it again. + const cached = versions.get(scope) + if (cached !== undefined && previous !== undefined && previous.action !== 'remove') { + registerKeptTrimmed(directory, cached.trimmedDigest) + } + continue + } if (probe.kind === 'absent') { if (previous === undefined || previous.action === 'remove') versions.delete(scope) else pushRemoval(scope, previous.path) diff --git a/packages/context/workspace-context/tests/workspace-context.spec.ts b/packages/context/workspace-context/tests/workspace-context.spec.ts index 1565b50db5..c26be1a58e 100644 --- a/packages/context/workspace-context/tests/workspace-context.spec.ts +++ b/packages/context/workspace-context/tests/workspace-context.spec.ts @@ -2201,6 +2201,47 @@ describe('dynamic nested workspace context injection', () => { } }) + it('keeps deduplicating against a loaded candidate whose probe transiently fails', async () => { + const root = await tempRepo() + const home = await tempRepo() + const ctx = new Context() + try { + await ctx.plugin(SystemPrompt) + await ctx.plugin(ToolRegistry) + await ctx.plugin(RecordingFileSystem) + const fs = ctx.fs as RecordingFileSystem + fs.entries.set(join(root, '.git'), { type: 'directory' }) + fs.entries.set(join(root, 'pkg/AGENTS.md'), { type: 'file', content: 'nested rule' }) + fs.entries.set(join(root, 'pkg/file.txt'), { type: 'file', content: 'hello' }) + await ctx.plugin(ToolFs) + await ctx.plugin(workspaceContext, { dshHome: home, maxBytes: 65536 }) + const agent = stubAgent(root) + + const first = await ctx.tools.execute({ + signal: testToolSignal, + callId: CallId('read-before-transient-probe-failure'), name: 'read', arguments: { file_path: join('pkg', 'file.txt') }, agent, + }) + appendAdditionalContexts(agent, first) + expect(first.additionalContexts).toBeDefined() + + // The loaded candidate's probe fails while an identical sibling appears: + // the cached candidate stays effective (last good state), so the sibling + // must still deduplicate against it rather than land as a duplicate set. + fs.throwOnStat.add(join(root, 'pkg/AGENTS.md')) + fs.entries.set(join(root, 'pkg/CLAUDE.md'), { type: 'file', content: 'nested rule' }) + const duringFailure = await ctx.tools.execute({ + signal: testToolSignal, + callId: CallId('read-during-transient-probe-failure'), name: 'read', arguments: { file_path: join('pkg', 'file.txt') }, agent, + }) + + expect(duringFailure.additionalContexts).toBeUndefined() + } finally { + await ctx.fiber.dispose() + await rm(root, { recursive: true, force: true }) + await rm(home, { recursive: true, force: true }) + } + }) + it('removes a previously rendered sibling once its content becomes a duplicate of an earlier candidate', async () => { const root = await tempRepo() const home = await tempRepo() diff --git a/packages/ui/app-boot/src/index.ts b/packages/ui/app-boot/src/index.ts index c150b4a1c1..2fd4ba4c05 100644 --- a/packages/ui/app-boot/src/index.ts +++ b/packages/ui/app-boot/src/index.ts @@ -66,7 +66,9 @@ export function parseResumeArg( if (arg === RESUME_FLAG || inlineValue) { if (resumeSessionId !== undefined) throw new Error(`${RESUME_FLAG} may be given only once`) const value = inlineValue ? arg.slice(RESUME_FLAG.length + 1) : argv[i + 1] - if (value === undefined || value === '') { + // A following token that is itself resume syntax (`--resume --resume x`) + // is a missing id, not a session literally named `--resume…`. + if (value === undefined || value === '' || value === RESUME_FLAG || value.startsWith(`${RESUME_FLAG}=`)) { throw new Error(`${RESUME_FLAG} requires a session id (e.g. ${RESUME_FLAG} )`) } resumeSessionId = value diff --git a/packages/ui/app-boot/tests/app-boot.spec.ts b/packages/ui/app-boot/tests/app-boot.spec.ts index fef17b5b07..76e5238db7 100644 --- a/packages/ui/app-boot/tests/app-boot.spec.ts +++ b/packages/ui/app-boot/tests/app-boot.spec.ts @@ -48,6 +48,11 @@ describe('parseResumeArg', () => { expect(() => parseResumeArg(['--resume='])).toThrow('--resume requires a session id') expect(() => parseResumeArg(['--resume', 'a', '--resume', 'b'])).toThrow('--resume may be given only once') }) + + it('rejects resume syntax used as the flag value instead of resuming a session named like the flag', () => { + expect(() => parseResumeArg(['--resume', '--resume', 'sess'])).toThrow('--resume requires a session id') + expect(() => parseResumeArg(['--resume', '--resume=sess'])).toThrow('--resume requires a session id') + }) }) describe('loadEnv', () => { diff --git a/packages/ui/tui/src/index.ts b/packages/ui/tui/src/index.ts index 77857c0d71..99e0ec2727 100644 --- a/packages/ui/tui/src/index.ts +++ b/packages/ui/tui/src/index.ts @@ -1307,12 +1307,15 @@ export function createTuiChat( let toolsExpanded = false let streaming: StreamingAssistantComponent | undefined let runningStatus: RunningStatus | undefined - // Steering messages the user queued during the running turn that the loop has - // not yet drained, shown as a badge on the status line. Reset on the - // running→idle status transition, which also absorbs a cancellation that - // clears the queue without logging drains; the status line exists only while - // running, so idle carries no badge to keep current. - let pendingSteering = 0 + // Steering messages queued during the running turn (`agent/queued`) that the + // loop has not yet drained, shown as a badge on the status line. Each entry is + // the queued message's serialized source: a drain (`steering/message`) removes + // one MATCHING entry, so loop-authored steering — continuation reasons enter + // the inbox without an `agent/queued` event — cannot consume a pending user + // message's slot. Cleared on leaving `running`, which also absorbs a + // cancellation that discards the queue without logging drains; the status + // line exists only while running, so idle carries no badge to keep current. + const pendingSteering: string[] = [] let disposed = false let shuttingDown: Promise | undefined // Optional: skills mount conditionally, so read the global service store @@ -1509,7 +1512,9 @@ export function createTuiChat( // controller's phase and the current steering count. const renderStatus = (running: RunningStatus): void => { const at = now() - running.loader.setMessage(formatTurnStatus(running.phase, at - running.phaseStartedAt, at - running.stepStartedAt, pendingSteering)) + running.loader.setMessage( + formatTurnStatus(running.phase, at - running.phaseStartedAt, at - running.stepStartedAt, pendingSteering.length), + ) } // Move to a derived phase, resetting the phase timer on a genuine change and @@ -1536,7 +1541,7 @@ export function createTuiChat( const phase = prior?.phase ?? 'waiting' const phaseStartedAt = prior?.phaseStartedAt ?? at const stepStartedAt = prior?.stepStartedAt ?? at - const message = formatTurnStatus(phase, at - phaseStartedAt, at - stepStartedAt, pendingSteering) + const message = formatTurnStatus(phase, at - phaseStartedAt, at - stepStartedAt, pendingSteering.length) const loader = new Loader(ui, text => palette.accent(text), text => palette.muted(text), message) statusContainer.addChild(loader) const running: RunningStatus = { @@ -2240,12 +2245,16 @@ export function createTuiChat( if (session !== agent.session) return recordEventUsage(tokens, event) advanceTurnPhase(event) - if (event.type === 'steering/message' && pendingSteering > 0) { - // A queued steering message reached the model as it drained; drop it from - // the badge. Clamped because loop-authored steering (e.g. continuation - // reasons) also logs here without a matching user-queued increment. - pendingSteering -= 1 - refreshStatus() + if (event.type === 'steering/message') { + // A queued steering message reached the model as it drained; drop its + // entry from the badge. Matching by source keeps loop-authored steering + // (e.g. continuation reasons), which logs here without a matching + // `agent/queued` increment, from consuming a pending user slot. + const drained = pendingSteering.indexOf(JSON.stringify(event.data.source)) + if (drained >= 0) { + pendingSteering.splice(drained, 1) + refreshStatus() + } } if ('surfaceOp' in event && typeof event.surfaceOp === 'object') { rebuildTranscript(false) @@ -2256,7 +2265,7 @@ export function createTuiChat( }) const disposeQueued = ctx.on('agent/queued', (subject, _content, info) => { if (subject !== agent || !info.steering) return - pendingSteering += 1 + pendingSteering.push(JSON.stringify(info.source)) refreshStatus() }) const disposeStatus = ctx.on('agent/status', (subject, status) => { @@ -2264,7 +2273,7 @@ export function createTuiChat( // Leaving 'running' ends the turn's status line; clear any badge so the // next running turn starts from zero (and a cancellation, which discards // the queue without logging drains, cannot strand a stale count). - if (status !== 'running') pendingSteering = 0 + if (status !== 'running') pendingSteering.length = 0 setStatus(status) }) const disposeError = ctx.on('agent/error', (subject, turn, step, error) => { diff --git a/packages/ui/tui/tests/tui.spec.ts b/packages/ui/tui/tests/tui.spec.ts index 10be7f99bf..a718d08eeb 100644 --- a/packages/ui/tui/tests/tui.spec.ts +++ b/packages/ui/tui/tests/tui.spec.ts @@ -584,13 +584,28 @@ describe('pi-tui chat lifecycle and transcript', () => { expect(result.terminal.output).toContain('— Enter sends steering, Esc cancels') expect(result.terminal.output).not.toContain('queued') - // A loop-authored steering drain past zero clamps rather than underflowing. + // A drain with no matching queued entry is ignored rather than underflowing. result.terminal.output = '' drainSteering('continuation') queueSteering('after') await tick() expect(result.terminal.output).toContain('1 queued') + // A loop-authored steering event (plugin source, no matching agent/queued) + // cannot consume a pending user slot, even when it drains first. + result.terminal.output = '' + result.session.append('steering/message', { + turn: 1, + content: [{ type: 'text', text: 'continue: goal not reached' }], + source: { kind: 'plugin', plugin: 'hooks' }, + }, { surfaceOp: 'append' }) + await tick() + expect(result.terminal.output).toContain('1 queued') + result.terminal.output = '' + drainSteering('after') + await tick() + expect(result.terminal.output).not.toContain('queued') + // The turn ending resets the badge, so the next running turn starts clean. result.agent.status = 'idle' result.ctx.emit('agent/status', result.agent, 'idle')