mirror of
https://github.com/wu736139669/hapi.git
synced 2026-10-08 19:19:42 +00:00
fix(opencode): verify persisted compaction results (#1357)
This commit is contained in:
Junmo Kim
@@ -11,7 +11,7 @@ import type { OpencodeSession } from './session';
|
||||
import type { OpencodeMode, PermissionMode } from './types';
|
||||
import { RPC_METHODS } from '@hapi/protocol/rpcMethods';
|
||||
import { allocateFreePort, createOpencodeBackend } from './utils/opencodeBackend';
|
||||
import { fetchCompactionSummary, splitProviderModel, triggerOpencodeCompact } from './utils/opencodeCompactBridge';
|
||||
import { captureCompactionMarkerSnapshot, fetchCompactionResult, splitProviderModel, triggerOpencodeCompact } from './utils/opencodeCompactBridge';
|
||||
import { OpencodePermissionHandler } from './utils/permissionHandler';
|
||||
import { OPENCODE_NATIVE_TOOL_INSTRUCTION, PLAN_MODE_INSTRUCTION } from './utils/systemPrompt';
|
||||
import { resolveThoughtLevelEffort } from './thoughtLevelEffort';
|
||||
@@ -45,6 +45,8 @@ export type AbortStatusDecision = {
|
||||
shouldClearThinking: boolean;
|
||||
};
|
||||
|
||||
type CompactOperationPhase = 'idle' | 'snapshot' | 'summarize' | 'post-summarize' | 'verification';
|
||||
|
||||
/**
|
||||
* Pure decision logic for handleAbort()'s final step: which status message
|
||||
* to show, and whether `thinking` should be cleared. Extracted out of the
|
||||
@@ -102,31 +104,15 @@ class OpencodeRemoteLauncher extends RemoteLauncherBase {
|
||||
// deliberately unbounded (see triggerOpencodeCompact's doc comment) and
|
||||
// the launcher stays wedged until it eventually settles on its own.
|
||||
private compactAbortController: AbortController | null = null;
|
||||
// True from the moment handleAbort() observes a compact operation in
|
||||
// flight until the dequeue loop creates the next one. A 6th PR-review
|
||||
// round found that unconditionally aborting `compactAbortController` on
|
||||
// *plain* Stop (not just switch/exit) broke a core invariant this
|
||||
// feature's whole redesign (see the FIFO-queue comment on the dequeue
|
||||
// loop) depends on: compact and a prompt must never touch the same
|
||||
// OpenCode session at once. Aborting only unblocks the *client's* fetch
|
||||
// — `session/update` notifications are a separate channel from that
|
||||
// HTTP request's lifecycle (see AcpSdkBackend.suppressUpdatesDuring's
|
||||
// doc comment), so the agent can still be compacting server-side well
|
||||
// after the client gives up, and the quiet-drain there (bounded at
|
||||
// ~1.2s) is not a real guarantee that a multi-minute server-side
|
||||
// compaction has actually finished. If the dequeue loop moved on to a
|
||||
// prompt as soon as the client-side abort settled, that prompt could
|
||||
// run concurrently with a compaction still touching the same session.
|
||||
//
|
||||
// The fix: plain Stop only sets this flag (suppressing the eventual
|
||||
// result) and leaves `compactAbortController` alone, so
|
||||
// runCompactOperation()'s own awaits keep blocking the dequeue loop
|
||||
// until the *real* HTTP response arrives — i.e. until the server
|
||||
// actually finishes. Switch-to-local/exit still abort the controller for
|
||||
// real (see handleAbort's `leavingRemote` parameter) because cleanup()
|
||||
// is about to disconnect the whole ACP subprocess regardless, so there's
|
||||
// no session left to protect.
|
||||
// A plain Stop must keep waiting only while the summarize POST is
|
||||
// actually in flight. That POST can outlive a client-side abort while
|
||||
// continuing to mutate the shared OpenCode session, so advancing to a
|
||||
// prompt would violate FIFO. The pre-POST marker snapshot and post-POST
|
||||
// result verification are read-only GETs; Stop aborts those immediately.
|
||||
// `compactOperationPhase` makes that distinction explicit for
|
||||
// handleAbort(), while this flag suppresses every eventual compact result.
|
||||
private compactResultSuppressed = false;
|
||||
private compactOperationPhase: CompactOperationPhase = 'idle';
|
||||
private displayPermissionMode: PermissionMode | null = null;
|
||||
private instructionsSent = false;
|
||||
private currentBackendModel: string | null = null;
|
||||
@@ -340,6 +326,7 @@ class OpencodeRemoteLauncher extends RemoteLauncherBase {
|
||||
const compactAbortController = isCompactBatch ? new AbortController() : null;
|
||||
if (compactAbortController) {
|
||||
this.compactAbortController = compactAbortController;
|
||||
this.compactOperationPhase = 'idle';
|
||||
// Reset here (as early as the controller itself — see its
|
||||
// sibling field's doc comment for why that timing matters)
|
||||
// rather than inside runCompactOperation(), so a plain Stop
|
||||
@@ -520,6 +507,7 @@ class OpencodeRemoteLauncher extends RemoteLauncherBase {
|
||||
if (cancelledBeforeStart) {
|
||||
if (this.compactAbortController === compactAbortController) {
|
||||
this.compactAbortController = null;
|
||||
this.compactOperationPhase = 'idle';
|
||||
}
|
||||
// A 10th PR-review round found this skip path never
|
||||
// calls session.onThinkingChange(true) (that's the
|
||||
@@ -545,7 +533,11 @@ class OpencodeRemoteLauncher extends RemoteLauncherBase {
|
||||
if (compactLocalId) {
|
||||
session.client.emitMessagesConsumed([compactLocalId], { clearQueuedThinkingGrace: true });
|
||||
}
|
||||
session.onThinkingChange(false);
|
||||
// Plain Stop before summarize already emitted this
|
||||
// keepalive in handleAbort(); localId cancellation did not.
|
||||
if (!this.compactResultSuppressed) {
|
||||
session.onThinkingChange(false);
|
||||
}
|
||||
if (session.queue.size() === 0 && !this.shouldExit) {
|
||||
sendReady();
|
||||
}
|
||||
@@ -692,13 +684,11 @@ class OpencodeRemoteLauncher extends RemoteLauncherBase {
|
||||
* mechanism only ever observes a cancel for this item's *queue message*,
|
||||
* and by this point the item has already been dequeued. `isCancelled()`
|
||||
* below checks all three, so any kind of cancellation suppresses the
|
||||
* eventual result the same way — but only switch/exit (`leavingRemote`
|
||||
* in handleAbort()) actually aborts `compactAbortController.signal`; a
|
||||
* plain Stop sets `compactResultSuppressed` alone and deliberately
|
||||
* leaves the signal un-aborted, so this function's own awaits below keep
|
||||
* blocking the dequeue loop until the operation *really* finishes
|
||||
* server-side — see `compactResultSuppressed`'s field doc comment for
|
||||
* why that invariant matters.
|
||||
* eventual result the same way. A plain Stop aborts only the read-only
|
||||
* snapshot/verification GET phases; while the summarize POST itself is
|
||||
* in flight it leaves the signal alone and waits for real server-side
|
||||
* completion, preserving the shared-session FIFO invariant. Switch/exit
|
||||
* aborts every phase because teardown disconnects the session.
|
||||
*
|
||||
* `compactAbortController` is created by the caller (the dequeue loop),
|
||||
* not here, and passed in — deliberately, before the loop's model/effort
|
||||
@@ -747,57 +737,94 @@ class OpencodeRemoteLauncher extends RemoteLauncherBase {
|
||||
return;
|
||||
}
|
||||
|
||||
if (isCancelled()) {
|
||||
logger.debug('[opencode-remote] /compact skipped before marker snapshot: cancelled or aborted');
|
||||
return;
|
||||
}
|
||||
|
||||
// The pre-POST marker snapshot is a read-only GET. A plain Stop
|
||||
// aborts it because no summarize request has started yet.
|
||||
this.compactOperationPhase = 'snapshot';
|
||||
const markerSnapshot = await captureCompactionMarkerSnapshot({
|
||||
baseUrl,
|
||||
sessionId: acpSessionId,
|
||||
signal: compactAbortController.signal
|
||||
});
|
||||
if (isCancelled()) {
|
||||
logger.debug('[opencode-remote] /compact skipped after marker snapshot: cancelled or aborted');
|
||||
return;
|
||||
}
|
||||
|
||||
// Suppressed: OpenCode keeps streaming session/update notifications
|
||||
// (agent_thought_chunk etc.) over the ACP transport while this raw
|
||||
// HTTP call runs — with no prompt() turn in flight to own them, they
|
||||
// would otherwise leak into the previous turn's still-installed
|
||||
// onUpdate and render as a duplicate assistant message alongside the
|
||||
// explicit summary we show below (from fetchCompactionSummary).
|
||||
// explicit summary we show below (from fetchCompactionResult).
|
||||
// See AcpSdkBackend.suppressUpdatesDuring's doc comment.
|
||||
//
|
||||
// `signal` lets handleAbort() interrupt this specific call (see
|
||||
// compactAbortController's field doc comment) — triggerOpencodeCompact
|
||||
// otherwise has no deadline by design, since a real compaction can
|
||||
// legitimately take minutes.
|
||||
const result = await backend.suppressUpdatesDuring(() => triggerOpencodeCompact({
|
||||
baseUrl,
|
||||
sessionId: acpSessionId,
|
||||
providerId: split.providerId,
|
||||
modelId: split.modelId,
|
||||
signal: compactAbortController.signal
|
||||
}));
|
||||
if (!result.ok) {
|
||||
// The summarize POST can keep mutating the shared session after a
|
||||
// client abort, so a plain Stop deliberately waits only in this
|
||||
// phase. It has no deadline because real compaction can take minutes.
|
||||
this.compactOperationPhase = 'summarize';
|
||||
const requestResult = await backend.suppressUpdatesDuring(async () => {
|
||||
try {
|
||||
return await triggerOpencodeCompact({
|
||||
baseUrl,
|
||||
sessionId: acpSessionId,
|
||||
providerId: split.providerId,
|
||||
modelId: split.modelId,
|
||||
signal: compactAbortController.signal
|
||||
});
|
||||
} finally {
|
||||
// suppressUpdatesDuring may still quiet-drain after the
|
||||
// POST settles. It is no longer server-side compaction.
|
||||
this.compactOperationPhase = 'post-summarize';
|
||||
}
|
||||
});
|
||||
if (!requestResult.ok) {
|
||||
if (!isCancelled()) {
|
||||
session.sendSessionEvent({ type: 'message', message: `📦 Compaction failed: ${result.error}` });
|
||||
session.sendSessionEvent({ type: 'message', message: `📦 Compaction failed: ${requestResult.error}` });
|
||||
} else {
|
||||
logger.debug('[opencode-remote] /compact failure suppressed: cancelled or aborted before it resolved');
|
||||
}
|
||||
return;
|
||||
}
|
||||
if (isCancelled()) {
|
||||
logger.debug('[opencode-remote] /compact verification skipped: cancelled or aborted after summarize');
|
||||
return;
|
||||
}
|
||||
|
||||
// Best-effort: fetch the actual summary text OpenCode generated
|
||||
// before the final cancellation check, so a cancel landing anywhere
|
||||
// during this whole operation (REST call or summary lookup)
|
||||
// suppresses "Compaction completed" and the Reasoning block
|
||||
// together — this mirrors the pre-redesign behavior, where both were
|
||||
// produced by one combined async step checked once. `signal` is
|
||||
// required on this call (see OpencodeCompactCallOpts) for exactly
|
||||
// the reason a prior PR-review round flagged as missing here: the
|
||||
// POST above being interruptible isn't enough on its own if this
|
||||
// GET can still block Stop/switch-to-local for as long as it takes.
|
||||
const summary = await fetchCompactionSummary({ baseUrl, sessionId: acpSessionId, signal: compactAbortController.signal });
|
||||
// The persisted-result lookup is also read-only: once summarize
|
||||
// returned, a plain Stop can abort it without concurrent session work.
|
||||
this.compactOperationPhase = 'verification';
|
||||
const result = await fetchCompactionResult({
|
||||
baseUrl,
|
||||
sessionId: acpSessionId,
|
||||
markerIdsBefore: markerSnapshot?.markerIds ?? null,
|
||||
signal: compactAbortController.signal
|
||||
});
|
||||
|
||||
if (isCancelled()) {
|
||||
logger.debug('[opencode-remote] /compact result suppressed: cancelled or aborted before it resolved');
|
||||
return;
|
||||
}
|
||||
|
||||
session.sendSessionEvent({ type: 'message', message: '📦 Compaction completed' });
|
||||
if (summary.found) {
|
||||
const converted = convertAgentMessage({ type: 'reasoning', text: summary.text, id: randomUUID() });
|
||||
if (converted) {
|
||||
session.sendAgentMessage(converted);
|
||||
switch (result.status) {
|
||||
case 'success': {
|
||||
session.sendSessionEvent({ type: 'message', message: '📦 Compaction completed' });
|
||||
const converted = convertAgentMessage({ type: 'reasoning', text: result.text, id: randomUUID() });
|
||||
if (converted) {
|
||||
session.sendAgentMessage(converted);
|
||||
}
|
||||
return;
|
||||
}
|
||||
case 'failed':
|
||||
session.sendSessionEvent({ type: 'message', message: `📦 Compaction failed: ${result.reason}` });
|
||||
return;
|
||||
case 'unverified':
|
||||
session.sendSessionEvent({ type: 'message', message: '📦 Compaction result could not be verified.' });
|
||||
return;
|
||||
}
|
||||
} finally {
|
||||
// Defensive: only clear if this is still the controller we set —
|
||||
@@ -808,6 +835,7 @@ class OpencodeRemoteLauncher extends RemoteLauncherBase {
|
||||
// with another runCompactOperation call).
|
||||
if (this.compactAbortController === compactAbortController) {
|
||||
this.compactAbortController = null;
|
||||
this.compactOperationPhase = 'idle';
|
||||
}
|
||||
}
|
||||
}
|
||||
@@ -867,13 +895,11 @@ class OpencodeRemoteLauncher extends RemoteLauncherBase {
|
||||
* this feature's core invariant: compact and a prompt must never touch
|
||||
* the same OpenCode session concurrently (see
|
||||
* `compactResultSuppressed`'s field doc comment for the full
|
||||
* reasoning). Plain Stop now only suppresses the eventual result and
|
||||
* leaves the compact operation's REST call running for real — the
|
||||
* dequeue loop stays blocked on it until the server actually finishes,
|
||||
* exactly as it does for an un-aborted turn. Switch/exit still abort it
|
||||
* for real: `cleanup()` disconnects the whole ACP subprocess right
|
||||
* after, so there is no shared-session invariant left to protect and
|
||||
* responsiveness (fixed in an earlier round) matters more.
|
||||
* reasoning). Plain Stop aborts only read-only marker/result GETs. It
|
||||
* leaves an in-flight summarize POST running so the dequeue loop waits
|
||||
* for its real server-side completion; only that phase can still mutate
|
||||
* the shared session. Switch/exit aborts every phase because cleanup()
|
||||
* disconnects the ACP subprocess right after.
|
||||
*/
|
||||
private async handleAbort(leavingRemote = false): Promise<void> {
|
||||
// A hostile-review sweep found that a plain Stop during an in-flight
|
||||
@@ -891,7 +917,15 @@ class OpencodeRemoteLauncher extends RemoteLauncherBase {
|
||||
const compactAbortController = this.compactAbortController;
|
||||
if (compactAbortController) {
|
||||
this.compactResultSuppressed = true;
|
||||
if (leavingRemote) {
|
||||
// Only summarize can still mutate the shared OpenCode session.
|
||||
// Plain Stop aborts snapshot/verification reads, but deliberately
|
||||
// waits for an in-flight POST to complete server-side.
|
||||
if (
|
||||
leavingRemote
|
||||
|| this.compactOperationPhase === 'snapshot'
|
||||
|| this.compactOperationPhase === 'post-summarize'
|
||||
|| this.compactOperationPhase === 'verification'
|
||||
) {
|
||||
compactAbortController.abort();
|
||||
}
|
||||
}
|
||||
@@ -911,10 +945,11 @@ class OpencodeRemoteLauncher extends RemoteLauncherBase {
|
||||
// overlap. Without this, the (now-stale) plain-Stop continuation
|
||||
// could append its "still waiting" message after the switch's
|
||||
// "Turn aborted" already ran, showing the two in a confusing order.
|
||||
const activeCompactAbortController = this.compactAbortController;
|
||||
const decision = selectAbortStatusMessage({
|
||||
hasCompactInFlight: compactAbortController !== null,
|
||||
hasCompactInFlight: activeCompactAbortController !== null && this.compactOperationPhase === 'summarize',
|
||||
leavingRemote,
|
||||
compactAborted: compactAbortController?.signal.aborted ?? false
|
||||
compactAborted: activeCompactAbortController?.signal.aborted ?? false
|
||||
});
|
||||
if (decision.shouldClearThinking) {
|
||||
this.session.onThinkingChange(false);
|
||||
|
||||
Reference in New Issue
Block a user