fix: preserve Goal continuation through compaction recovery - #640
Open
madgegja wants to merge 11 commits into
Open
fix: preserve Goal continuation through compaction recovery#640madgegja wants to merge 11 commits into
madgegja wants to merge 11 commits into
Conversation
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
17 issues found across 86 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="packages/coding-agent/test/suite/app-server-thread-handlers-archive.test.ts">
<violation number="1" location="packages/coding-agent/test/suite/app-server-thread-handlers-archive.test.ts:218">
P3: This replaces a check that the unarchive timestamp bump persists into the cold thread listing with a `notLoaded` status assertion that is already redundant in this test (the earlier `unarchivedThread.status` and `getLoadedThread` throw already cover it). The dropped `updatedAt >= unarchivedUpdatedAt` assertion was the only verification that the bumped mtime from `touchThreadSession` is actually reflected in a subsequent default `thread/list`, which is directly in the 'no stale state after unarchive' invariant space this PR is hardening. Consider restoring the persisted-timestamp check so the cold listing's `updatedAt` (not just status) is still guarded.</violation>
</file>
<file name="packages/coding-agent/src/core/extensions/builtin/compaction/log.ts">
<violation number="1" location="packages/coding-agent/src/core/extensions/builtin/compaction/log.ts:23">
P3: The new `todoItemCount` log key is allowlisted and typed but never emitted anywhere in production — only tests pass it. Either the deterministic-fallback log site in `index.ts` should also emit it (it tracks preserved todo state per the PR), or drop the unused key to avoid dead surface in the sanitized-keys allowlist.</violation>
</file>
<file name="packages/agent/src/types.ts">
<violation number="1" location="packages/agent/src/types.ts:148">
P3: Direct prompts are also subject to this callback, not just queued messages; the current comment understates the API scope and can lead callers to assume `runAgentLoop` prompts cannot be rejected. Describing both prompt and queued-message admission would make the contract accurate.</violation>
</file>
<file name="packages/coding-agent/src/core/extensions/types.ts">
<violation number="1" location="packages/coding-agent/src/core/extensions/types.ts:1864">
P2: The new `sendMessage()` return type is not available from the package's public entrypoint: consumers can use `ExtensionAPI` but cannot import `MessageDelivery` from `@code-yeongyu/senpi`. Re-export `MessageDelivery` from `src/index.ts` alongside the other extension API types so the new public contract is usable by typed extensions.</violation>
</file>
<file name="packages/coding-agent/test/footer-data-provider.test.ts">
<violation number="1" location="packages/coding-agent/test/footer-data-provider.test.ts:52">
P3: The hardening only mocks fs.watch via watchWithErrorHandler; watchFile (node:fs polling, interval 250ms on tables.list and 1000ms on HEAD) is still real and calls scheduleRefresh on its own. That makes assertions like execFile toHaveBeenCalledTimes(1) race against the polling interval, so the tests are not actually hermetic and can still flake on fast hosts. Consider checking stdout from provider onBranchChange/spawnSync side effects and also neutralizing watchFile (vi.spyOn(fs,'watchFile')) so only the emitted watcher events drive refresh.</violation>
</file>
<file name="packages/coding-agent/test/suite/regressions/automatic-compaction-timeout-recovery.test.ts">
<violation number="1" location="packages/coding-agent/test/suite/regressions/automatic-compaction-timeout-recovery.test.ts:47">
P3: This test's determinism depends on real-time host timing: the hanging summary never resolves and the deterministic-fallback path is forced by a watchdog with `maxDurationMs: 5` (ms), while `awaitSignal` aborts after a 2s wall-clock timeout. On a slow or heavily loaded CI worker, a delayed event loop can let the watchdog fire before the summary request is actually in flight (or delay signal dispatch past the 2s timeout), changing which branch is exercised and producing intermittent failures. Prefer a fake/controllable clock or a larger, explicitly-scaled budget tied to a settable timer so the overflow-timeout path is triggerable deterministically rather than by racing real time.</violation>
</file>
<file name="packages/coding-agent/test/mcp/oauth-race.test.ts">
<violation number="1" location="packages/coding-agent/test/mcp/oauth-race.test.ts:189">
P3: The new assertion `log.requests.length > preGate` is trivially satisfied: `preGate` is sampled before `seedNearExpiryToken`, and that seeding already performs a logged `/token` exchange, so the count grows even if the workers emit no traffic. This also replaces the removed `postRaceFailureKinds`/`postRaceOk` checks that actually demonstrated the family-invalidation disaster, so the control case no longer verifies the post-race failure it claims to lock against. Consider sampling the request baseline after seeding (next to `before = tokenHits`) so the assertion reflects worker-generated traffic, and keep an explicit post-race failure check.</violation>
</file>
<file name="packages/coding-agent/src/core/compaction/stream-watchdog.ts">
<violation number="1" location="packages/coding-agent/src/core/compaction/stream-watchdog.ts:177">
P2: Caller cancellation can leave compaction waiting for the final result for the remainder of the max-duration budget when `getResult()` has not settled after iteration ends. Including `callerAbortPromise` in this race and returning the normal cancellation outcome when it wins would preserve the documented signal behavior.</violation>
</file>
<file name="packages/coding-agent/src/core/extensions/builtin/goal/lifecycle-helpers.ts">
<violation number="1" location="packages/coding-agent/src/core/extensions/builtin/goal/lifecycle-helpers.ts:155">
P2: A concurrent goal update is rejected by the expectation check, but this branch returns the pre-admission snapshot anyway. Callers store that result as the current goal, so continuation scheduling can proceed from stale active state; re-read and return the persisted goal (or propagate a stale outcome) when the guarded mutation returns `null`.</violation>
</file>
<file name="packages/coding-agent/src/core/extensions/builtin/goal/store.ts">
<violation number="1" location="packages/coding-agent/src/core/extensions/builtin/goal/store.ts:44">
P2: An accepted input or continuation guard that races `clearGoal` can reject with `GoalNotFoundError` instead of being ignored as stale. The expected/CAS overload should treat a missing current goal as a mismatch and return `null`, while preserving the exception for legacy calls without `expected`.</violation>
<violation number="2" location="packages/coding-agent/src/core/extensions/builtin/goal/store.ts:79">
P2: A failed replacement can leave a completed goal in history even though `createGoal` did not create the new goal. Retrying after a sidecar/filesystem failure can append the same completed goal again, so history no longer reflects successful replacements.</violation>
<violation number="3" location="packages/coding-agent/src/core/extensions/builtin/goal/store.ts:221">
P2: The full-objective sidecar is persisted before the goal file, and the pairing check in readObjectiveForPrompt only validates that the sidecar re-truncates to the same display string. If the process crashes between the sidecar write and the goal write, a replacement goal's full objective can remain paired with the still-persisted old goal whenever the new objective shares the same truncation prefix, re-injecting stale objective text into the old goal's continuation prompt. Consider writing the sidecar and goal together (e.g. reference a matching goal id inside the sidecar or write to a single atomic file) so the pairing survives interruption rather than relying on the shared truncation prefix.</violation>
</file>
<file name="packages/coding-agent/test/suite/goal-monitor-continuation.test.ts">
<violation number="1" location="packages/coding-agent/test/suite/goal-monitor-continuation.test.ts:216">
P3: The second continuation in this test runs through the immediate admission path because no terminal monitor is ever activated (`terminal_monitor_state` is never emitted and timers are never advanced), so it does not actually cover the monitor-delayed continuation the title and the PR's compaction-recovery scenario reference. Consider emitting `terminal_monitor_state` with `activeCount: 1` and advancing `GOAL_MONITOR_CONTINUATION_DELAY_MS` for the agent_end phase, and note that the hardcoded `repeat(180)` only reaches the sidecar because it exceeds `MAX_OBJECTIVE_LENGTH` (4000) — a longer bound would silently stop testing compaction.</violation>
</file>
<file name="packages/coding-agent/test/suite/goal-monitor-stall.test.ts">
<violation number="1" location="packages/coding-agent/test/suite/goal-monitor-stall.test.ts:201">
P2: The assertion expect(harness.sent).toHaveLength(2) is inconsistent with the continuation flow in this test. Unlike the user-prompt reset test (where noteUserPrompt disarms the timer), the monitor settle here only resets the toolless streak; it does not prevent the third continuation from being scheduled and delivered. afterAgentEnd with an active monitor calls #schedule, then advanceTimersByTimeAsync(240_000) fires #continueIfEligible → admitAndQueueGoalContinuation → sendMessage, so harness.sent should be 3 (matching the count the test's original runMonitorContinuationCycle observed). As written the assertion either fails or under-verifies the guarded scenario; it should expect length 3.</violation>
</file>
<file name="packages/agent/src/agent-loop.ts">
<violation number="1" location="packages/agent/src/agent-loop.ts:126">
P3: When a run's entire initial prompt batch is filtered out by `shouldDeliverMessage`, this path emits `turn_start` and then jumps straight to `agent_end` without the matching `turn_end`. Every other runLoop exit emits `turn_end` before `agent_end`, so listeners that pair turn lifecycle events (turn accounting / loop consumers) will see an unbalanced `turn_start`. Consider emitting `turn_end` with empty results before `agent_end` on this early-return path, or skip emitting `agent_start`/`turn_start` for an empty, fully-suppressed batch.</violation>
</file>
<file name="packages/coding-agent/src/core/extensions/builtin/compaction/index.ts">
<violation number="1" location="packages/coding-agent/src/core/extensions/builtin/compaction/index.ts:456">
P2: Required automatic compaction can now fail instead of recovering if the Goal store is unreadable. `capturePendingMetadata` became async and reads the Goal file via `readGoal`, but it is awaited before the `try` that runs the summarizer and builds the deterministic fallback. `readGoal` throws on a malformed goal file or a storage read error (only a missing file returns `null`), so those errors escape the handler and abort compaction entirely — the exact opposite of this PR's goal of making recovery deterministic and robust. Consider deferring the Goal read into the existing recovery path or guarding it so a Goal-store failure degrades to the prior `resolveInheritedTaskIntent`/`resolveDroppedUserIntent` fallback rather than failing compaction.</violation>
<violation number="2" location="packages/coding-agent/src/core/extensions/builtin/compaction/index.ts:601">
P3: The new `deterministic_fallback_applied` diagnostic always reports `hasTaskIntent: false`, so it carries no signal about whether the recovery checkpoint preserved user/Goal intent. The fallback's `details` deliberately no longer contains `taskIntent` (the prior test asserts it is absent), yet this log line still probes `details.taskIntent`, which is therefore permanently undefined. Either log the intent-preservation decision from the source the handler actually has (e.g. the `recoveryMetadata.taskIntent ?? resolveInheritedTaskIntent(...)` value used to build the fallback) or drop the field so the log doesn't imply it measured something it never reads.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
|
|
||
| type HandlerFn = (...args: unknown[]) => Promise<unknown>; | ||
|
|
||
| export interface MessageDelivery { |
There was a problem hiding this comment.
P2: The new sendMessage() return type is not available from the package's public entrypoint: consumers can use ExtensionAPI but cannot import MessageDelivery from @code-yeongyu/senpi. Re-export MessageDelivery from src/index.ts alongside the other extension API types so the new public contract is usable by typed extensions.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/coding-agent/src/core/extensions/types.ts, line 1864:
<comment>The new `sendMessage()` return type is not available from the package's public entrypoint: consumers can use `ExtensionAPI` but cannot import `MessageDelivery` from `@code-yeongyu/senpi`. Re-export `MessageDelivery` from `src/index.ts` alongside the other extension API types so the new public contract is usable by typed extensions.</comment>
<file context>
@@ -1861,10 +1861,17 @@ export interface ExtensionShortcut {
type HandlerFn = (...args: unknown[]) => Promise<unknown>;
+export interface MessageDelivery {
+ readonly id: string;
+ cancel(): boolean;
</file context>
Comment on lines
+177
to
+186
| const finalResultContenders: Array<Promise<Result | typeof BUDGET_TRIP>> = [ | ||
| Promise.resolve(getResult(resolvedStream)), | ||
| ]; | ||
| if (budgetPromise) finalResultContenders.push(budgetPromise); | ||
| const finalResult = await Promise.race(finalResultContenders); | ||
| if (finalResult === BUDGET_TRIP) { | ||
| abort(); | ||
| throw new StreamDurationBudgetError(budgetMs); | ||
| } | ||
| return finalResult; |
There was a problem hiding this comment.
P2: Caller cancellation can leave compaction waiting for the final result for the remainder of the max-duration budget when getResult() has not settled after iteration ends. Including callerAbortPromise in this race and returning the normal cancellation outcome when it wins would preserve the documented signal behavior.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/coding-agent/src/core/compaction/stream-watchdog.ts, line 177:
<comment>Caller cancellation can leave compaction waiting for the final result for the remainder of the max-duration budget when `getResult()` has not settled after iteration ends. Including `callerAbortPromise` in this race and returning the normal cancellation outcome when it wins would preserve the documented signal behavior.</comment>
<file context>
@@ -114,38 +127,64 @@ export async function consumeStreamWithIdleTimeout<T>(
+ }
+ if (result.done) {
+ if (!getResult) return undefined;
+ const finalResultContenders: Array<Promise<Result | typeof BUDGET_TRIP>> = [
+ Promise.resolve(getResult(resolvedStream)),
+ ];
</file context>
Suggested change
| const finalResultContenders: Array<Promise<Result | typeof BUDGET_TRIP>> = [ | |
| Promise.resolve(getResult(resolvedStream)), | |
| ]; | |
| if (budgetPromise) finalResultContenders.push(budgetPromise); | |
| const finalResult = await Promise.race(finalResultContenders); | |
| if (finalResult === BUDGET_TRIP) { | |
| abort(); | |
| throw new StreamDurationBudgetError(budgetMs); | |
| } | |
| return finalResult; | |
| const finalResultContenders: Array<Promise<Result | typeof BUDGET_TRIP | typeof CALLER_ABORTED>> = [ | |
| Promise.resolve(getResult(resolvedStream)), | |
| ]; | |
| if (budgetPromise) finalResultContenders.push(budgetPromise); | |
| if (callerAbortPromise) finalResultContenders.push(callerAbortPromise); | |
| const finalResult = await Promise.race(finalResultContenders); | |
| if (finalResult === CALLER_ABORTED) return undefined; | |
| if (finalResult === BUDGET_TRIP) { | |
| abort(); | |
| throw new StreamDurationBudgetError(budgetMs); | |
| } | |
| return finalResult; |
| ); | ||
| if (recordedGoal === null) { | ||
| reservation.cancel(); | ||
| return { goal, admitted: false }; |
There was a problem hiding this comment.
P2: A concurrent goal update is rejected by the expectation check, but this branch returns the pre-admission snapshot anyway. Callers store that result as the current goal, so continuation scheduling can proceed from stale active state; re-read and return the persisted goal (or propagate a stale outcome) when the guarded mutation returns null.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/coding-agent/src/core/extensions/builtin/goal/lifecycle-helpers.ts, line 155:
<comment>A concurrent goal update is rejected by the expectation check, but this branch returns the pre-admission snapshot anyway. Callers store that result as the current goal, so continuation scheduling can proceed from stale active state; re-read and return the persisted goal (or propagate a stale outcome) when the guarded mutation returns `null`.</comment>
<file context>
@@ -55,21 +131,59 @@ export async function admitAndQueueGoalContinuation(
+ );
+ if (recordedGoal === null) {
+ reservation.cancel();
+ return { goal, admitted: false };
+ }
+ reservation.onCancelled(() => {
</file context>
| if (current !== null && current.status !== "complete") { | ||
| throw new GoalAlreadyExistsError("cannot create a new goal because this thread already has a goal"); | ||
| } | ||
| if (current?.status === "complete") await archiveGoalUnlocked(ref, current); |
There was a problem hiding this comment.
P2: A failed replacement can leave a completed goal in history even though createGoal did not create the new goal. Retrying after a sidecar/filesystem failure can append the same completed goal again, so history no longer reflects successful replacements.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/coding-agent/src/core/extensions/builtin/goal/store.ts, line 79:
<comment>A failed replacement can leave a completed goal in history even though `createGoal` did not create the new goal. Retrying after a sidecar/filesystem failure can append the same completed goal again, so history no longer reflects successful replacements.</comment>
<file context>
@@ -29,54 +36,105 @@ export function objectiveFullTextFilePath(ref: GoalStoreRef): string {
+ if (current !== null && current.status !== "complete") {
+ throw new GoalAlreadyExistsError("cannot create a new goal because this thread already has a goal");
+ }
+ if (current?.status === "complete") await archiveGoalUnlocked(ref, current);
+ const now = nowSeconds();
+ const goal: Goal = {
</file context>
|
|
||
| export async function readObjectiveForPrompt(ref: GoalStoreRef, goal: Pick<Goal, "id" | "objective">): Promise<string> { | ||
| return withHardenedGoalStoreMutation(ref, async () => { | ||
| const current = await readGoalFile(ref); |
There was a problem hiding this comment.
P2: An accepted input or continuation guard that races clearGoal can reject with GoalNotFoundError instead of being ignored as stale. The expected/CAS overload should treat a missing current goal as a mismatch and return null, while preserving the exception for legacy calls without expected.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/coding-agent/src/core/extensions/builtin/goal/store.ts, line 44:
<comment>An accepted input or continuation guard that races `clearGoal` can reject with `GoalNotFoundError` instead of being ignored as stale. The expected/CAS overload should treat a missing current goal as a mismatch and return `null`, while preserving the exception for legacy calls without `expected`.</comment>
<file context>
@@ -29,54 +36,105 @@ export function objectiveFullTextFilePath(ref: GoalStoreRef): string {
+
+export async function readObjectiveForPrompt(ref: GoalStoreRef, goal: Pick<Goal, "id" | "objective">): Promise<string> {
+ return withHardenedGoalStoreMutation(ref, async () => {
+ const current = await readGoalFile(ref);
+ if (current?.id !== goal.id || current.objective !== goal.objective) return goal.objective;
+ await hardenGoalAuxiliaryFilePermissions(ref);
</file context>
| }); | ||
|
|
||
| it("recovers Goal, todo, and queued input through the real overflow route", async () => { | ||
| const watchdog = { idleTimeoutMs: 1_000, maxDurationMs: 5 }; |
There was a problem hiding this comment.
P3: This test's determinism depends on real-time host timing: the hanging summary never resolves and the deterministic-fallback path is forced by a watchdog with maxDurationMs: 5 (ms), while awaitSignal aborts after a 2s wall-clock timeout. On a slow or heavily loaded CI worker, a delayed event loop can let the watchdog fire before the summary request is actually in flight (or delay signal dispatch past the 2s timeout), changing which branch is exercised and producing intermittent failures. Prefer a fake/controllable clock or a larger, explicitly-scaled budget tied to a settable timer so the overflow-timeout path is triggerable deterministically rather than by racing real time.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/coding-agent/test/suite/regressions/automatic-compaction-timeout-recovery.test.ts, line 47:
<comment>This test's determinism depends on real-time host timing: the hanging summary never resolves and the deterministic-fallback path is forced by a watchdog with `maxDurationMs: 5` (ms), while `awaitSignal` aborts after a 2s wall-clock timeout. On a slow or heavily loaded CI worker, a delayed event loop can let the watchdog fire before the summary request is actually in flight (or delay signal dispatch past the 2s timeout), changing which branch is exercised and producing intermittent failures. Prefer a fake/controllable clock or a larger, explicitly-scaled budget tied to a settable timer so the overflow-timeout path is triggerable deterministically rather than by racing real time.</comment>
<file context>
@@ -0,0 +1,171 @@
+ });
+
+ it("recovers Goal, todo, and queued input through the real overflow route", async () => {
+ const watchdog = { idleTimeoutMs: 1_000, maxDurationMs: 5 };
+ const inflate: AgentTool = {
+ name: "inflate",
</file context>
| raceArtifacts.push(artifact); | ||
|
|
||
| expect(log.tokenHits - before).toBeGreaterThanOrEqual(2); | ||
| expect(log.requests.length).toBeGreaterThan(preGate); |
There was a problem hiding this comment.
P3: The new assertion log.requests.length > preGate is trivially satisfied: preGate is sampled before seedNearExpiryToken, and that seeding already performs a logged /token exchange, so the count grows even if the workers emit no traffic. This also replaces the removed postRaceFailureKinds/postRaceOk checks that actually demonstrated the family-invalidation disaster, so the control case no longer verifies the post-race failure it claims to lock against. Consider sampling the request baseline after seeding (next to before = tokenHits) so the assertion reflects worker-generated traffic, and keep an explicit post-race failure check.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/coding-agent/test/mcp/oauth-race.test.ts, line 189:
<comment>The new assertion `log.requests.length > preGate` is trivially satisfied: `preGate` is sampled before `seedNearExpiryToken`, and that seeding already performs a logged `/token` exchange, so the count grows even if the workers emit no traffic. This also replaces the removed `postRaceFailureKinds`/`postRaceOk` checks that actually demonstrated the family-invalidation disaster, so the control case no longer verifies the post-race failure it claims to lock against. Consider sampling the request baseline after seeding (next to `before = tokenHits`) so the assertion reflects worker-generated traffic, and keep an explicit post-race failure check.</comment>
<file context>
@@ -180,16 +180,14 @@ describe("cross-process refresh race", () => {
raceArtifacts.push(artifact);
expect(log.tokenHits - before).toBeGreaterThanOrEqual(2);
+ expect(log.requests.length).toBeGreaterThan(preGate);
expect(log.familyInvalidated).toBe(true);
expect(results.some((result) => result.ok === false && result.kind === "invalid_grant")).toBe(true);
</file context>
| expect(sent[0]?.delivery.state).toBe("cancelled"); | ||
| }); | ||
|
|
||
| it("reinjects the full objective tail for startup and monitor continuations", async () => { |
There was a problem hiding this comment.
P3: The second continuation in this test runs through the immediate admission path because no terminal monitor is ever activated (terminal_monitor_state is never emitted and timers are never advanced), so it does not actually cover the monitor-delayed continuation the title and the PR's compaction-recovery scenario reference. Consider emitting terminal_monitor_state with activeCount: 1 and advancing GOAL_MONITOR_CONTINUATION_DELAY_MS for the agent_end phase, and note that the hardcoded repeat(180) only reaches the sidecar because it exceeds MAX_OBJECTIVE_LENGTH (4000) — a longer bound would silently stop testing compaction.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/coding-agent/test/suite/goal-monitor-continuation.test.ts, line 216:
<comment>The second continuation in this test runs through the immediate admission path because no terminal monitor is ever activated (`terminal_monitor_state` is never emitted and timers are never advanced), so it does not actually cover the monitor-delayed continuation the title and the PR's compaction-recovery scenario reference. Consider emitting `terminal_monitor_state` with `activeCount: 1` and advancing `GOAL_MONITOR_CONTINUATION_DELAY_MS` for the agent_end phase, and note that the hardcoded `repeat(180)` only reaches the sidecar because it exceeds `MAX_OBJECTIVE_LENGTH` (4000) — a longer bound would silently stop testing compaction.</comment>
<file context>
@@ -179,6 +184,58 @@ describe("goal continuation while a monitor is active", () => {
+ expect(sent[0]?.delivery.state).toBe("cancelled");
+ });
+
+ it("reinjects the full objective tail for startup and monitor continuations", async () => {
+ const notices: string[] = [];
+ const { handlers, sent } = createGoalHarness();
</file context>
| newMessages.push(prompt); | ||
| } | ||
| if (newMessages.length === 0) { | ||
| await emit({ type: "agent_end", messages: newMessages }); |
There was a problem hiding this comment.
P3: When a run's entire initial prompt batch is filtered out by shouldDeliverMessage, this path emits turn_start and then jumps straight to agent_end without the matching turn_end. Every other runLoop exit emits turn_end before agent_end, so listeners that pair turn lifecycle events (turn accounting / loop consumers) will see an unbalanced turn_start. Consider emitting turn_end with empty results before agent_end on this early-return path, or skip emitting agent_start/turn_start for an empty, fully-suppressed batch.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/agent/src/agent-loop.ts, line 126:
<comment>When a run's entire initial prompt batch is filtered out by `shouldDeliverMessage`, this path emits `turn_start` and then jumps straight to `agent_end` without the matching `turn_end`. Every other runLoop exit emits `turn_end` before `agent_end`, so listeners that pair turn lifecycle events (turn accounting / loop consumers) will see an unbalanced `turn_start`. Consider emitting `turn_end` with empty results before `agent_end` on this early-return path, or skip emitting `agent_start`/`turn_start` for an empty, fully-suppressed batch.</comment>
<file context>
@@ -107,17 +107,24 @@ export async function runAgentLoop(
+ newMessages.push(prompt);
+ }
+ if (newMessages.length === 0) {
+ await emit({ type: "agent_end", messages: newMessages });
+ return newMessages;
}
</file context>
| tokensBefore: compactEvent.compactionEntry.tokensBefore, | ||
| retainedEntryCount: keptEntries.length, | ||
| summaryBytes: Buffer.byteLength(compactEvent.compactionEntry.summary), | ||
| hasTaskIntent: details.taskIntent !== undefined, |
There was a problem hiding this comment.
P3: The new deterministic_fallback_applied diagnostic always reports hasTaskIntent: false, so it carries no signal about whether the recovery checkpoint preserved user/Goal intent. The fallback's details deliberately no longer contains taskIntent (the prior test asserts it is absent), yet this log line still probes details.taskIntent, which is therefore permanently undefined. Either log the intent-preservation decision from the source the handler actually has (e.g. the recoveryMetadata.taskIntent ?? resolveInheritedTaskIntent(...) value used to build the fallback) or drop the field so the log doesn't imply it measured something it never reads.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/coding-agent/src/core/extensions/builtin/compaction/index.ts, line 601:
<comment>The new `deterministic_fallback_applied` diagnostic always reports `hasTaskIntent: false`, so it carries no signal about whether the recovery checkpoint preserved user/Goal intent. The fallback's `details` deliberately no longer contains `taskIntent` (the prior test asserts it is absent), yet this log line still probes `details.taskIntent`, which is therefore permanently undefined. Either log the intent-preservation decision from the source the handler actually has (e.g. the `recoveryMetadata.taskIntent ?? resolveInheritedTaskIntent(...)` value used to build the fallback) or drop the field so the log doesn't imply it measured something it never reads.</comment>
<file context>
@@ -539,8 +571,36 @@ export default function compactionExtension(
+ tokensBefore: compactEvent.compactionEntry.tokensBefore,
+ retainedEntryCount: keptEntries.length,
+ summaryBytes: Buffer.byteLength(compactEvent.compactionEntry.summary),
+ hasTaskIntent: details.taskIntent !== undefined,
+ });
+ }
</file context>
Author
|
The branch is now conflict-resolved and mergeable at Maintainer action requested: approve workflow run Local merge-tree gates already completed: |
Author
|
Final conflict-resolved head verification (
The remaining |
Author
|
@code-yeongyu Maintainer action needed: please approve fork workflow run https://github.com/code-yeongyu/senpi/actions/runs/30701437955. The PR is mergeable; GitGuardian/Cubic and the exact local CI command set are green. The submitting account cannot request review or approve the run because it has read-only repository permission. |
Author
|
@code-yeongyu Follow-up: upstream CI run https://github.com/code-yeongyu/senpi/actions/runs/30701437955 is still |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Verification
npm testexit 0fs.watchexhaustion: 60/60 passednpm run checkgateSecurity and isolation
taskIntentfrom persisted detailsCommit structure
The branch is split into 10 independently checked commits covering Agent admission, delivery receipts, Goal invariants, RPC forwarding, compaction recovery, export privacy, fixture hardening, runner invalidation, and documentation.
Summary by cubic
Preserves Goal continuation across compaction and restarts by serializing continuation admission and restoring full private objectives for prompts. Adds deterministic compaction timeout recovery that carries recent user intent and current todo state, and makes extension-delivered messages cancellable while filtering queued input at admission.
Bug Fixes
New Features
pi.sendMessage()returns a delivery receipt withonStarted()andcancel(); cancellation removes only that exact pending delivery (identical siblings stay).steer()andfollowUp()accept images/options, emit the same input/disposition lifecycle as queued prompts, and propagate input source (defaults tointeractive, RPC usesrpc).shouldDeliverMessageto admit/skip queued messages at delivery time; filtered messages don’t enter loop context.Written for commit f124e56. Summary will update on new commits.