fix(FN-5994): prevent verification hangs

[gsxdsm]

Summary

• rewrite package test -- --run file filters in fn_run_verification to direct Vitest execution so package scripts do not expand into broad suites first
• kill the full verification process group on hard timeout so child shells/tests cannot outlive the tool
• suppress duplicate loop recovery while compact-and-resume is pending and bound compaction to 60s before falling through to kill/requeue
• fix the authoritative workflow cutover internal ID so reliability tests exercise the injected workflow definition

Verification

• pnpm --filter @fusion/engine exec vitest run --project engine-default src/__tests__/run-verification-command.test.ts src/__tests__/stuck-task-detector.test.ts src/__tests__/executor-step-session.test.ts --silent=passed-only --reporter=dot
• pnpm --filter @fusion/engine exec vitest run --project engine-reliability src/__tests__/reliability-interactions/workflow-interpreter-cutover.test.ts --silent=passed-only --reporter=dot
• pnpm --filter @fusion/engine typecheck
• pnpm --filter @fusion/core typecheck
• pnpm --filter @fusion/engine test:core
• timeout 360s pnpm --filter @fusion/engine test
• pnpm exec eslint packages/core/src/agent-prompts.ts packages/engine/src/executor.ts packages/engine/src/workflow-authoritative-driver.ts packages/engine/src/run-verification-tool.ts packages/engine/src/stuck-task-detector.ts
• git diff --check

Summary by CodeRabbit

• New Features
  • Normalize package-scoped test commands into direct per-file test runs with sensible defaults and surface rewrite warnings.
  • Ensure pre-install is prepended to the effective command when needed.
• Bug Fixes
  • Add timeout protection for loop-compaction recovery and fallback to kill/requeue on timeout.
  • Safer process termination with SIGKILL escalation if needed.
  • Improve stuck-task recovery state handling to avoid duplicate recovery attempts.
• Tests
  • Added coverage for loop-recovery timeout, command normalization, timeouts, and suppression behavior.

[coderabbitai]

📝 Walkthrough

Walkthrough

This PR normalizes package-scoped test commands to direct Vitest invocations, hardens verification process termination with escalation timers and process-group signaling, caps loop-compaction recovery with a timeout and session validation, and prevents duplicate stuck-task recovery callbacks while pending.

Changes

Task execution resilience and verification improvements

Layer / File(s)	Summary
Verification command normalization and guidance packages/core/src/agent-prompts.ts, packages/engine/src/run-verification-tool.ts, packages/engine/src/__tests__/run-verification-command.test.ts, packages/engine/src/executor.ts	normalizeVerificationCommand detects package-scoped pnpm test patterns and rewrites them into pnpm --filter <pkg> exec vitest run ... with injected default --silent=passed-only and --reporter=dot. Executor and agent prompts now guide users to use direct Vitest invocations for file-scoped package tests.
Process termination safety with escalation packages/engine/src/run-verification-tool.ts, packages/engine/src/__tests__/run-verification-command.test.ts	killVerificationProcess helper safely terminates child processes, preferring process-group signals on non-Windows platforms. Spawning sets detached for group signaling, and hard timeouts escalate from SIGTERM to SIGKILL via a tracked killTimer. Cleanup clears the optional killTimer on both normal close and spawn errors. Timeout tests validate behavior.
Tool factory integration of normalization packages/engine/src/run-verification-tool.ts	The effective command is normalized before bootstrap detection, and auto-prepended pnpm install --prefer-offline is applied to the normalized effective command (not the original) when node_modules/.modules.yaml is missing.
Loop recovery timeout and session validation packages/engine/src/executor.ts, packages/engine/src/__tests__/executor-step-session.test.ts	handleLoopDetected now runs context compaction under Promise.race with LOOP_COMPACTION_TIMEOUT_MS, aborts the session on timeout, validates the activeSessions entry still matches the compacted session, logs on timeout/mismatch, and falls back to kill/requeue. Tests cover the timeout path and logging.
Stuck task recovery callback state management packages/engine/src/stuck-task-detector.ts, packages/engine/src/__tests__/stuck-task-detector.test.ts	Classification handling aborts kill/retry when classifyStuckReason returns null. For loop-detected tasks, recoveryInProgress is set before awaiting the onLoopDetected callback to prevent re-entry during its execution; the flag is cleared only if the callback returns false or throws. Tests verify suppression of duplicate callback invocations while recovery is pending.
Authoritative workflow identifier update packages/engine/src/workflow-authoritative-driver.ts	AUTHORITATIVE_WORKFLOW_ID constant updated from "builtin:workflow-interpreter-authoritative" to "internal:workflow-interpreter-authoritative".

Sequence Diagram(s)

No sequence diagram generated: the PR contains multiple independent subsystems (command normalization, process management, loop recovery state tracking) with no single linear flow that benefits from a unified sequence diagram.

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

Possibly related PRs

• Runfusion/Fusion#63: Both PRs modify packages/engine/src/executor.ts's executor system-prompt generation; PR #63 refactors prompt assembly while this PR updates verification guidance within the embedded prompt.

Poem

🐰 Through timeouts and signals, I quietly hop,
Rewriting test routes so failures don't drop,
I guard the loops, prevent repeated calls,
Escalate gently when a child process stalls,
A little rabbit's patch — steady and spry.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 22.22% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title 'fix(FN-5994): prevent verification hangs' directly summarizes the main objective of the changeset—addressing verification hangs through command normalization, timeout handling, and loop recovery improvements.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

📝 Generate docstrings

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch feature/fix-hang

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[greptile-apps]

Greptile Summary

This PR addresses verification hangs and loop-recovery race conditions in the Fusion engine by rewriting pnpm test -- --run commands to direct Vitest invocations, killing entire process groups on hard timeout, bounding in-process context compaction to 60 s, and fixing a race where recoveryInProgress was set too late to suppress duplicate loop-recovery calls.

• Command normalization (run-verification-tool.ts): normalizeVerificationCommand rewrites pnpm --filter <pkg> test -- --run <files> to pnpm --filter <pkg> exec vitest run <files> so package test scripts don't expand into broad quality suites before the file filter is applied.
• Compaction timeout (executor.ts): handleLoopDetected races compactSessionContext against a 60 s timer; on timeout it calls session.abort() and returns false.
• recoveryInProgress race fix (stuck-task-detector.ts): the flag is now set to true before await onLoopDetected(event), closing the duplicate-recovery window.

Confidence Score: 5/5

Safe to merge — all three concurrency and timeout fixes are well-scoped with correct reset paths, the new tests cover the timer-race and process-group kill logic, and the workflow ID rename is already matched by the reliability test update.

The three main changes — moving recoveryInProgress assignment before the async await, bounding compaction with a 60 s race plus abort() call, and switching to process-group kill with detached: true — are each isolated and straightforward. The command normalizer is purely additive with comprehensive unit tests. No silent state corruption, no missing cleanup path, and no auth or data-integrity boundary is touched.

No files require special attention.

Important Files Changed

Filename	Overview
packages/engine/src/run-verification-tool.ts	Adds command normalizer to rewrite pnpm test -- --run to direct vitest invocation, and fixes hard-timeout to kill the full process group via -pid on POSIX; killTimer cleanup added to both close paths.
packages/engine/src/executor.ts	Adds LOOP_COMPACTION_TIMEOUT_MS (60 s) and wraps compactSessionContext in Promise.race; on timeout calls session.abort() and returns false; adds post-compaction session-identity guard.
packages/engine/src/stuck-task-detector.ts	Moves recoveryInProgress=true before the async onLoopDetected await, and resets it to false in both the unhandled and error branches, closing the duplicate-recovery race.
packages/engine/src/workflow-authoritative-driver.ts	Renames AUTHORITATIVE_WORKFLOW_ID prefix from builtin: to internal: so reliability tests exercise the injected workflow definition.

_{Reviews (2): Last reviewed commit: "fix(FN-5994): address verification revie..." | Re-trigger Greptile}

[coderabbitai]

🧹 Nitpick comments (1)

packages/engine/src/__tests__/run-verification-command.test.ts (1)

27-28: Rework workspaceRoot to avoid relying on process.cwd()

workspaceRoot = join(process.cwd(), "../..") is correct for the normal pnpm flow because pnpm --filter @fusion/engine ... runs the package scripts with that package as the working directory, so ../.. resolves to the repo root. The assumption breaks if vitest is invoked directly from another cwd (e.g., repo root) with a different invocation/config.

const workspaceRoot = join(process.cwd(), "../..");

Prefer deriving the root from the test file location (e.g., fileURLToPath(new URL("../../..", import.meta.url)) / path.resolve) to make the test robust to invocation cwd.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@packages/engine/src/__tests__/run-verification-command.test.ts` around lines
27 - 28, The test's workspaceRoot variable (const workspaceRoot =
join(process.cwd(), "../..")) depends on the current working directory; change
it to compute the repo root from the test file location instead (use
fileURLToPath(new URL(..., import.meta.url)) or path.resolve against
import.meta.url) so the value is stable regardless of how Vitest is invoked;
update the workspaceRoot assignment in run-verification-command.test.ts (and
remove reliance on process.cwd()) and keep using the existing workspaceRoot
symbol everywhere it is referenced.

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Nitpick comments:
In `@packages/engine/src/__tests__/run-verification-command.test.ts`:
- Around line 27-28: The test's workspaceRoot variable (const workspaceRoot =
join(process.cwd(), "../..")) depends on the current working directory; change
it to compute the repo root from the test file location instead (use
fileURLToPath(new URL(..., import.meta.url)) or path.resolve against
import.meta.url) so the value is stable regardless of how Vitest is invoked;
update the workspaceRoot assignment in run-verification-command.test.ts (and
remove reliance on process.cwd()) and keep using the existing workspaceRoot
symbol everywhere it is referenced.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: 014b0b84-ac44-49dd-b1b8-b1a327d8c2b7

📥 Commits

Reviewing files that changed from the base of the PR and between a7c8bef and 0c8674c.

📒 Files selected for processing (8)

• packages/core/src/agent-prompts.ts
• packages/engine/src/__tests__/executor-step-session.test.ts
• packages/engine/src/__tests__/run-verification-command.test.ts
• packages/engine/src/__tests__/stuck-task-detector.test.ts
• packages/engine/src/executor.ts
• packages/engine/src/run-verification-tool.ts
• packages/engine/src/stuck-task-detector.ts
• packages/engine/src/workflow-authoritative-driver.ts

👮 Files not reviewed due to content moderation or server errors (1)

• packages/engine/src/executor.ts

[gsxdsm]

workspaceRoot = join(process.cwd(), "../..") depends on the current working directory

Addressed: the normalization test now derives the workspace root from import.meta.url via fileURLToPath(new URL("../../../../", import.meta.url)), so it is stable regardless of the Vitest invocation cwd.

shellSplit silently treats unterminated quotes as closed; the @runfusion/fusion fast-path bypasses package.json verification; compaction timeout leaves background work

Addressed: shellSplit now bails out on unterminated quotes, CLI package resolution now verifies package.json like other packages, package directory lookup is cached, pnpm global flags are preserved, and loop-compaction timeout now aborts the active session before falling through to kill/requeue.

[coderabbitai]

🧹 Nitpick comments (1)

packages/engine/src/__tests__/run-verification-command.test.ts (1)

29-84: 💤 Low value

Consider adding test coverage for package-not-found case.

The normalization logic guards against non-existent packages by checking findWorkspacePackageDir and returning the command unchanged if null. Adding a test with a non-existent package name (e.g., @fusion/does-not-exist) would verify this guard works correctly.

🧪 Suggested test case

it("leaves command unchanged when package directory cannot be found", () => {
  const command = "pnpm --filter `@fusion/does-not-exist` test -- --run packages/nonexistent/test.ts";
  expect(normalizeVerificationCommand(command, workspaceRoot)).toEqual({ command, warnings: [] });
});

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@packages/engine/src/__tests__/run-verification-command.test.ts` around lines
29 - 84, Add a test to
packages/engine/src/__tests__/run-verification-command.test.ts that exercises
the guard in normalizeVerificationCommand when findWorkspacePackageDir cannot
locate a package: call normalizeVerificationCommand with a command like "pnpm
--filter `@fusion/does-not-exist` test -- --run packages/nonexistent/test.ts"
(using the existing workspaceRoot) and assert the result equals { command,
warnings: [] } to ensure the command is left unchanged and no warnings are
emitted.

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Nitpick comments:
In `@packages/engine/src/__tests__/run-verification-command.test.ts`:
- Around line 29-84: Add a test to
packages/engine/src/__tests__/run-verification-command.test.ts that exercises
the guard in normalizeVerificationCommand when findWorkspacePackageDir cannot
locate a package: call normalizeVerificationCommand with a command like "pnpm
--filter `@fusion/does-not-exist` test -- --run packages/nonexistent/test.ts"
(using the existing workspaceRoot) and assert the result equals { command,
warnings: [] } to ensure the command is left unchanged and no warnings are
emitted.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: 394e88a3-7eb9-4a09-9352-de6e22da73be

📥 Commits

Reviewing files that changed from the base of the PR and between 0c8674c and c0b6f21.

📒 Files selected for processing (4)

• packages/engine/src/__tests__/executor-step-session.test.ts
• packages/engine/src/__tests__/run-verification-command.test.ts
• packages/engine/src/executor.ts
• packages/engine/src/run-verification-tool.ts

🚧 Files skipped from review as they are similar to previous changes (3)

• packages/engine/src/executor.ts
• packages/engine/src/tests/executor-step-session.test.ts
• packages/engine/src/run-verification-tool.ts