Review Summary

Excellent work on fixing the premature exit issue in non-interactive mode! The solution is elegant and addresses the root cause effectively. The implementation is solid with good error handling and state management.

✅ Strengths

1. Root Cause Analysis: Correctly identified that the 2-second countdown is too slow for opencode run which exits immediately on session.idle

2. Smart Timing: Hooking into message.updated BEFORE session.idle fires is the right approach - prevents the session from going idle in the first place

3. Robust Detection: detector.ts covers multiple cases:

   • CI environment variables (CI, GITHUB_ACTIONS)
   • OpenCode-specific variables (OPENCODE_RUN, OPENCODE_NON_INTERACTIVE)
   • TTY detection as fallback

4. Proper State Management:

   • preemptivelyInjectedSessions prevents duplicate injections
   • Cleared on user message and injection failure
   • Respects recovery/error states

5. BackgroundManager Integration: Checks for running background tasks, not just todos - comprehensive solution

6. Error Handling: Gracefully handles failures and cleans up state

🤔 Questions & Suggestions

1. Injected Message Role Assumption

The code assumes injected prompts (line 346-353) are processed as "user" messages, which would trigger the clear logic at line 294. Could you verify this behavior? If the injected messages have a different role, preemptivelyInjectedSessions might not get cleared, preventing re-injection on subsequent finishes.

Test scenario:

• Agent finishes with incomplete todos → preemptive injection
• Agent processes prompt but finishes again without completing todos
• Does second injection happen?

2. Type Change Verification

Line 283: finish type changed from boolean | undefined to string | undefined. This aligns with the logic at line 301, but could you confirm this matches the actual event payload structure?

3. TTY Detection Edge Case

Line 14 in detector.ts:

if (!process.stdout.isTTY) {
  return true
}

This correctly handles undefined (when stdout is not a TTY), but consider making it explicit for clarity:

if (process.stdout.isTTY === false || process.stdout.isTTY === undefined) {
  return true
}

4. Consider Adding Tests

Given the complexity of the state management, consider adding test cases for:

• Multiple preemptive injections in sequence
• Background task completion during injection
• Error recovery flows

📝 Minor Suggestions

Export Consistency

non-interactive-env/index.ts now exports detector.ts. Consider whether this should be the default export or if the structure matches other hook modules.

Logging Verbosity

The extensive logging is great for debugging! Consider whether all logs should remain at the default level or if some could be moved to a debug level once the feature stabilizes.

🎯 Testing Validation

The PR description mentions:

• ✅ bun run typecheck - Pass
• ✅ bun test - 31 pass, 0 fail

Recommendation: Add a real-world CI test by triggering a GitHub Actions workflow that:

1. Runs opencode run with background tasks
2. Verifies the process doesn't exit prematurely
3. Confirms all todos complete

Verdict

LGTM with minor questions ✅

The implementation is sound and solves a critical issue for CI/CD workflows. The questions above are mostly for verification and documentation purposes. Great work!

Closes #216 appropriately. Merge when ready!