Address code-review findings on pause-abort + task-chat fixes

- Reliability (P3): emit an `Auto-recovered:`-prefixed log on the
  benign-todo stale-failure clear path so NotificationService
  proactively cancels the pending failure timer (recoveredStatus path)
  instead of relying only on the fire-time re-check, which is
  race-contingent when failureNotificationDelayMs is near 0. Scoped to
  the actual-clear path so the common no-failure re-queue isn't
  mislabeled as a recovery.
- Project-standards (P3): add the required yyyy-MM-dd-hh:mm stamp to the
  new FNXC comments (AGENTS.md FNXC_LOG convention).
- Maintainability (P3): extract the scheduler "queued" waiting marker to
  a named SCHEDULER_WAITING_STATUS constant.
- Testing: pin the guard's skip on a clean todo row, assert the
  Auto-recovered log fires on the stale-failure path, and add a
  paused+unassigned in-progress idle case (paused early-return wins over
  the ephemeral active-session path).

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This commit is contained in:
gsxdsm
2026-06-20 20:07:07 -07:00
parent b990872827
commit 7b85fead54
5 changed files with 44 additions and 12 deletions

View File

@@ -2,4 +2,4 @@
"@runfusion/fusion": patch
---
Clear the stale `failed` status when a pause/resume abort is reclassified as a benign todo re-queue, so the task no longer surfaces as failed on the board and the deferred failure notification is suppressed. Previously a pause-abort parked `status:"failed"` on an earlier non-todo observation stayed dispatchable (the scheduler filters on column+paused, not status), re-entered the benign-todo branch, and was logged benign while the row stayed failed — firing a contradictory failure alert during global pause when self-healing recovery was suppressed.
Clear the stale `failed` status when a pause/resume abort is reclassified as a benign todo re-queue, so the task no longer surfaces as failed on the board and the deferred failure notification is suppressed. Previously a pause-abort parked `status:"failed"` on an earlier non-todo observation stayed dispatchable (the scheduler filters on column+paused, not status), re-entered the benign-todo branch, and was logged benign while the row stayed failed — firing a contradictory failure alert during global pause when self-healing recovery was suppressed. The clear path also emits an `Auto-recovered:`-prefixed log so the notification service proactively cancels the pending failure timer instead of relying only on the fire-time re-check.

View File

@@ -53,6 +53,11 @@ const STEERING_BLOCKED_STATUSES = new Set([
"needs-replan",
]);
const REVIEW_STEERABLE_STATUSES = new Set(["reviewing", "merging", "merging-fix", "fixing"]);
// The scheduler's waiting/blocked marker for a not-yet-dispatched task
// (self-healing.ts documents `status: "queued"` as the blocked marker). A queued
// in-progress row has no agent executing yet, so it stays assignment-gated rather
// than counting as an implied active session.
const SCHEDULER_WAITING_STATUS = "queued";
const BOTTOM_FOLLOW_THRESHOLD = 48;
const TOP_LOAD_THRESHOLD = 48;
@@ -185,7 +190,7 @@ function isActiveAgentSession(task: Task | TaskDetail, opts: { sessionLive?: boo
// has a reviewer/merger running. A null-status in-review row is awaiting
// human review, not actively worked, so it stays assignment-gated and idle.
const executionImpliesActiveAgent =
(task.column === "in-progress" && statusAllowsProgressSteering && task.status !== "queued")
(task.column === "in-progress" && statusAllowsProgressSteering && task.status !== SCHEDULER_WAITING_STATUS)
|| (task.column === "in-review" && task.status != null && REVIEW_STEERABLE_STATUSES.has(task.status));
return columnAllowsSteering
&& (hasAssignedAgent || executionImpliesActiveAgent);

View File

@@ -1976,6 +1976,9 @@ describe("TaskChatTab", () => {
["in-progress task without an assigned or checked-out agent", makeTask({ column: "in-progress", status: "queued", assignedAgentId: undefined, checkedOutBy: undefined })],
["paused in-progress task", makeTask({ column: "in-progress", status: "queued", paused: true })],
["user-paused in-progress task", makeTask({ column: "in-progress", status: "queued", userPaused: true })],
// Paused early-return must win over the ephemeral executionImpliesActiveAgent path:
// a paused/unassigned in-progress task in an otherwise-active status stays idle.
["paused unassigned in-progress task in an active status", makeTask({ column: "in-progress", status: "planning", paused: true, assignedAgentId: undefined, checkedOutBy: undefined })],
["paused in-review task", makeTask({ column: "in-review", status: "reviewing", paused: true })],
["user-paused in-review task", makeTask({ column: "in-review", status: "reviewing", userPaused: true })],
])("keeps the composer sendable with idle guidance for %s", (_label, task) => {

View File

@@ -82,15 +82,25 @@ describe("pause-abort benign requeue-to-todo (FN-6782)", () => {
expect((executor as any).pausedAborted.has(task.id)).toBe(false);
// FNXC:WorkflowLifecycle the leaked worktree slot must be released to avoid board-wide concurrency blockage.
expect((executor as any).activeWorktrees.has(task.id)).toBe(false);
// FNXC:WorkflowLifecycle 2026-06-20-19:58 a clean todo row (no stale status/error)
// must NOT trigger the reconciliation write — the `live.status != null ||
// live.error != null` guard skips it so the common benign re-queue stays a no-op.
const clearedClean = store.updateTask.mock.calls.some(
(call: unknown[]) => {
const patch = call[1] as { status?: unknown; error?: unknown } | undefined;
return patch?.status === null && patch?.error === null;
},
);
expect(clearedClean).toBe(false);
});
it("clears a stale failed status when reclassifying a todo pause-abort as benign (no lingering failure notification)", async () => {
// FNXC:WorkflowLifecycle a pause-abort parked status:"failed" on an earlier
// non-todo observation stays dispatchable (scheduler filters column+paused,
// not status) and re-enters this branch in todo. The benign reclassification
// must reconcile the row to status:null/error:null — otherwise the persisted
// failure survives, the board shows it failed, and the deferred failure
// notification fires despite the benign log.
// FNXC:WorkflowLifecycle 2026-06-20-19:58 a pause-abort parked status:"failed"
// on an earlier non-todo observation stays dispatchable (scheduler filters
// column+paused, not status) and re-enters this branch in todo. The benign
// reclassification must reconcile the row to status:null/error:null —
// otherwise the persisted failure survives, the board shows it failed, and
// the deferred failure notification fires despite the benign log.
const { store, task, executor } = makeHarness({
column: "todo",
status: "failed",
@@ -112,6 +122,11 @@ describe("pause-abort benign requeue-to-todo (FN-6782)", () => {
);
expect(reParkedFailed).toBe(false);
expect(logText(store)).toContain("benign, cleared for normal scheduling");
// FNXC:WorkflowLifecycle 2026-06-20-19:58 the clear path must emit an
// `Auto-recovered:`-prefixed log so NotificationService proactively cancels
// the pending failure timer (recoveredStatus path), not just suppress it at
// fire time. Prefix is the documented self-healing recovery contract.
expect(logText(store)).toContain("Auto-recovered: cleared stale pause-abort failure on todo re-queue");
});
it("STILL parks a non-todo (in-review) pause-abort as operator-action failed", async () => {

View File

@@ -6701,9 +6701,9 @@ export class TaskExecutor {
const todoBenign = `Workflow graph run ended during ${pauseProvenance} with task re-queued to todo — benign, cleared for normal scheduling`;
executorLog.log(`${task.id}: ${todoBenign}`);
await this.store.logEntry(task.id, todoBenign, undefined, this.getRunContextFor(task.id));
// FNXC:WorkflowLifecycle 2026-06-20: reconcile a stale persisted
// failure with the benign reclassification. A pause-abort parked
// `status:"failed"` on an earlier non-todo observation stays
// FNXC:WorkflowLifecycle 2026-06-20-19:58: reconcile a stale
// persisted failure with the benign reclassification. A pause-abort
// parked `status:"failed"` on an earlier non-todo observation stays
// dispatchable (scheduler.ts filters column+paused, NOT status) and
// re-enters this branch in `todo`; `recoverPausedAbortFailures` that
// would clear it is suppressed during global/engine pause
@@ -6711,9 +6711,18 @@ export class TaskExecutor {
// log: the board shows it failed AND the deferred failure
// notification fires (notification-service fire-time check sees
// status === "failed"). Clear status/error here so the row matches
// the log and the pending notification is suppressed at dispatch.
// the log, then emit an `Auto-recovered:`-prefixed entry so
// NotificationService.maybeSuppressTransientFailedNotification
// PROACTIVELY cancels the pending failure timer on the task:updated
// event (recoveredStatus path) — rather than relying only on the
// fire-time re-check, which is race-contingent when
// failureNotificationDelayMs is near 0. The prefix is the documented
// contract for self-healing recovery logs (see self-healing.ts /
// project-engine.ts). Scoped to the actual-clear path so the common
// no-failure benign re-queue is not mislabeled as a recovery.
if (live.status != null || live.error != null) {
await this.store.updateTask(task.id, { status: null, error: null }, this.getRunContextFor(task.id));
await this.store.logEntry(task.id, "Auto-recovered: cleared stale pause-abort failure on todo re-queue — failure notification suppressed", undefined, this.getRunContextFor(task.id));
}
await this.persistTokenUsage(task.id);
return;