Extract inline logic from BackgroundManager into focused modules by code-yeongyu · Pull Request #2034 · code-yeongyu/oh-my-openagent

code-yeongyu · 2026-02-22T03:00:02Z

Background

BackgroundManager (manager.ts) had grown to 2036 LOC with all logic inline — error classification, fallback retry, process cleanup, compaction-aware message resolution, and notification building were all embedded directly in the class. The background-agent/ directory already contained 15+ extracted modules from a previous incomplete refactoring attempt, but none were wired into the manager.

This PR completes that extraction: manager.ts now delegates to focused modules while preserving identical behavior and public API.

Changes

Extracted modules (new files)

fallback-retry-handler.ts (125 LOC) — standalone tryFallbackRetry() with full retry/fallback chain logic
process-cleanup.ts (82 LOC) — registerManagerForCleanup()/unregisterManagerForCleanup() for process signal handling
compaction-aware-message-resolver.ts (57 LOC) — isCompactionAgent()/findNearestMessageExcludingCompaction()

Enhanced modules

error-classifier.ts (21 → 83 LOC) — added isRecord(), extractErrorName(), extractErrorMessage(), getSessionErrorMessage()

Wired existing modules

duration-formatter.ts and task-poller.ts — already existed but weren't imported by manager

Deleted

notification-builder.ts — duplicate of background-task-notification-template.ts

Manager.ts reduction

2036 → 1647 LOC (19% reduction, -389 lines net)
Removed static cleanup fields (cleanupManagers, cleanupRegistered, cleanupHandlers)
Method bodies replaced with delegation wrappers
Dead private helpers removed (getErrorText, extractErrorName, extractErrorMessage, isRecord, getSessionErrorMessage, registerProcessSignal)

New tests (104 tests across 4 files)

File	Tests	Coverage
`error-classifier.test.ts`	80	All 6 exported functions with edge cases
`fallback-retry-handler.test.ts`	19	Retry logic, fallback chain, concurrency, session abort
`process-cleanup.test.ts`	7	Signal registration, multi-manager shutdown, unregister
`compaction-aware-message-resolver.test.ts`	13	Compaction detection, message resolution with temp dirs

Testing

bun test src/features/background-agent/   # 265 pass, 0 fail (was 161)
bun run typecheck                          # clean

All 161 existing manager.test.ts tests pass unchanged — no behavior changes
All 104 new tests pass both in isolation and in the full suite

Review Notes

No public API changes — index.ts barrel exports remain identical (BackgroundManager, SubagentSessionCreatedEvent, OnSubagentSessionCreated)
No private field/method renames — tests access privates via unsafe casts ((manager as unknown as { tasks: ... })), all preserved
process-cleanup.ts exports _resetForTesting() to handle module-level singleton state pollution across test files (when manager.test.ts runs before process-cleanup.test.ts)
Several existing extracted modules (spawner.ts, state.ts, result-handler.ts, session-output-validator.ts) remain unwired — they diverged too much from manager's inline logic. Future PR scope.

Summary by cubic

Extracted inline logic from BackgroundManager into focused modules while keeping behavior and the public API unchanged. Manager size reduced by 19%, and new tests cover the extracted logic.

Refactors
- Added modules: fallback-retry-handler, process-cleanup, compaction-aware-message-resolver; expanded error-classifier with name/message extractors and session error parsing.
- Wired existing duration-formatter and task-poller; manager now delegates duration formatting and stale task pruning/interrupts.
- Removed duplicate notification-builder (use background-task-notification-template).
- No API changes; all existing tests still pass. Added 104 new unit tests for the extracted modules.

^{Written for commit 8d66d56. Summary will update on new commits.}

… focused modules Extract 5 concerns from BackgroundManager into dedicated modules: - error-classifier.ts: enhance with extractErrorName, extractErrorMessage, getSessionErrorMessage, isRecord - fallback-retry-handler.ts: standalone tryFallbackRetry with full retry logic - process-cleanup.ts: registerManagerForCleanup/unregisterManagerForCleanup - compaction-aware-message-resolver.ts: isCompactionAgent/findNearestMessageExcludingCompaction - Delete notification-builder.ts (duplicate of background-task-notification-template.ts) Manager.ts method bodies now delegate to extracted modules. Wire duration-formatter.ts and task-poller.ts (existing but unused). manager.ts: 2036 -> 1647 LOC (19% reduction). All 161 existing tests pass unchanged.

Add 104 new tests across 4 test files: - error-classifier.test.ts (80 tests): isRecord, isAbortedSessionError, getErrorText, extractErrorName, extractErrorMessage, getSessionErrorMessage - fallback-retry-handler.test.ts (19 tests): retry logic, fallback chain, concurrency release, session abort, queue management - process-cleanup.test.ts (7 tests): signal registration, multi-manager shutdown, cleanup on unregister - compaction-aware-message-resolver.test.ts (13 tests): compaction agent detection, message resolution with temp dirs (pre-existing, verified) Total background-agent tests: 161 -> 265 (104 new, 0 regressions)

cubic-dev-ai

6 issues found across 10 files

Confidence score: 3/5

Some risk here due to a guaranteed 6-second hang on Ctrl-C in src/features/background-agent/process-cleanup.ts when the exit timeout isn’t .unref()’d, which can stall shutdown behavior.
Tests in src/features/background-agent/process-cleanup.test.ts call the SIGINT listener and trigger process.exit(), mutating global state and making cleanup tests unsafe or flaky across all three instances.
Additional test gaps in src/features/background-agent/compaction-aware-message-resolver.test.ts mean invalid JSON and skip logic aren’t actually exercised, so regressions could slip through.
Pay close attention to src/features/background-agent/process-cleanup.ts, src/features/background-agent/process-cleanup.test.ts, src/features/background-agent/compaction-aware-message-resolver.test.ts, src/features/background-agent/fallback-retry-handler.ts - shutdown hang, unsafe exit calls in tests, and retry task data loss.

Prompt for AI agents (all 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="src/features/background-agent/compaction-aware-message-resolver.test.ts">

<violation number="1" location="src/features/background-agent/compaction-aware-message-resolver.test.ts:99">
P2: This test does not actually verify the skipping logic. Because `002.json` is a valid match and evaluated first (due to reverse sorting), the compaction message (`001.json`) is never processed.</violation>

<violation number="2" location="src/features/background-agent/compaction-aware-message-resolver.test.ts:158">
P2: This test passes without evaluating the invalid JSON. Due to reverse sorting, `002.json` (valid) is evaluated and returned first, meaning `001.json` (invalid) is skipped entirely. Swap the filenames to fix.</violation>
</file>

<file name="src/features/background-agent/process-cleanup.test.ts">

<violation number="1" location="src/features/background-agent/process-cleanup.test.ts:67">
P1: Invoking the `SIGINT` listener triggers `process.exit()` and mutates the global exit code. Use the `exit` listener to test cleanup safely. (Note: Apply this fix to all 3 tests in this file where this pattern is used).</violation>
</file>

<file name="src/features/background-agent/compaction-aware-message-resolver.ts">

<violation number="1" location="src/features/background-agent/compaction-aware-message-resolver.ts:29">
P2: Optimize file reading by merging dual loops into a single pass to avoid redundant disk I/O.</violation>
</file>

<file name="src/features/background-agent/fallback-retry-handler.ts">

<violation number="1" location="src/features/background-agent/fallback-retry-handler.ts:119">
P2: The `retryInput` mapping omits the `isUnstableAgent` property from the `task`, causing fallback retries to lose their unstable agent status.</violation>
</file>

<file name="src/features/background-agent/process-cleanup.ts">

<violation number="1" location="src/features/background-agent/process-cleanup.ts:14">
P1: Missing `.unref()` on exit timeout causes a guaranteed 6-second hang on Ctrl-C.</violation>
</file>

_{Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.}

cubic-dev-ai · 2026-02-22T03:08:19Z

src/features/background-agent/process-cleanup.test.ts

+
+      registerManagerForCleanup(manager)
+
+      const [, listener] = processOnCalls[0]


P1: Invoking the SIGINT listener triggers process.exit() and mutates the global exit code. Use the exit listener to test cleanup safely. (Note: Apply this fix to all 3 tests in this file where this pattern is used).

Prompt for AI agents

Check if this issue is valid — if so, understand the root cause and fix it. At src/features/background-agent/process-cleanup.test.ts, line 67: <comment>Invoking the `SIGINT` listener triggers `process.exit()` and mutates the global exit code. Use the `exit` listener to test cleanup safely. (Note: Apply this fix to all 3 tests in this file where this pattern is used).</comment> <file context> @@ -0,0 +1,156 @@ + + registerManagerForCleanup(manager) + + const [, listener] = processOnCalls[0] + listener() + </file context>

cubic-dev-ai · 2026-02-22T03:08:20Z

src/features/background-agent/process-cleanup.ts

+    handler()
+    if (exitAfter) {
+      process.exitCode = 0
+      setTimeout(() => process.exit(), 6000)


P1: Missing .unref() on exit timeout causes a guaranteed 6-second hang on Ctrl-C.

Prompt for AI agents

Check if this issue is valid — if so, understand the root cause and fix it. At src/features/background-agent/process-cleanup.ts, line 14: <comment>Missing `.unref()` on exit timeout causes a guaranteed 6-second hang on Ctrl-C.</comment> <file context> @@ -0,0 +1,81 @@ + handler() + if (exitAfter) { + process.exitCode = 0 + setTimeout(() => process.exit(), 6000) + } + } </file context>

cubic-dev-ai · 2026-02-22T03:08:20Z

src/features/background-agent/compaction-aware-message-resolver.test.ts

+        agent: "oracle",
+        model: { providerID: "google", modelID: "gemini-2-flash" },
+      }
+      writeFileSync(join(tempDir, "001.json"), invalidJson)


P2: This test passes without evaluating the invalid JSON. Due to reverse sorting, 002.json (valid) is evaluated and returned first, meaning 001.json (invalid) is skipped entirely. Swap the filenames to fix.

Prompt for AI agents

Check if this issue is valid — if so, understand the root cause and fix it. At src/features/background-agent/compaction-aware-message-resolver.test.ts, line 158: <comment>This test passes without evaluating the invalid JSON. Due to reverse sorting, `002.json` (valid) is evaluated and returned first, meaning `001.json` (invalid) is skipped entirely. Swap the filenames to fix.</comment> <file context> @@ -0,0 +1,190 @@ + agent: "oracle", + model: { providerID: "google", modelID: "gemini-2-flash" }, + } + writeFileSync(join(tempDir, "001.json"), invalidJson) + writeFileSync(join(tempDir, "002.json"), JSON.stringify(validMessage)) + </file context>

cubic-dev-ai · 2026-02-22T03:08:20Z

src/features/background-agent/compaction-aware-message-resolver.test.ts

+        agent: "sisyphus",
+        model: { providerID: "anthropic", modelID: "claude-opus-4-6" },
+      }
+      writeFileSync(join(tempDir, "001.json"), JSON.stringify(compactionMessage))


P2: This test does not actually verify the skipping logic. Because 002.json is a valid match and evaluated first (due to reverse sorting), the compaction message (001.json) is never processed.

Prompt for AI agents

Check if this issue is valid — if so, understand the root cause and fix it. At src/features/background-agent/compaction-aware-message-resolver.test.ts, line 99: <comment>This test does not actually verify the skipping logic. Because `002.json` is a valid match and evaluated first (due to reverse sorting), the compaction message (`001.json`) is never processed.</comment> <file context> @@ -0,0 +1,190 @@ + agent: "sisyphus", + model: { providerID: "anthropic", modelID: "claude-opus-4-6" }, + } + writeFileSync(join(tempDir, "001.json"), JSON.stringify(compactionMessage)) + writeFileSync(join(tempDir, "002.json"), JSON.stringify(validMessage)) + </file context>

cubic-dev-ai · 2026-02-22T03:08:20Z

src/features/background-agent/compaction-aware-message-resolver.ts

+      .sort()
+      .reverse()
+
+    for (const file of files) {


P2: Optimize file reading by merging dual loops into a single pass to avoid redundant disk I/O.

Prompt for AI agents

Check if this issue is valid — if so, understand the root cause and fix it. At src/features/background-agent/compaction-aware-message-resolver.ts, line 29: <comment>Optimize file reading by merging dual loops into a single pass to avoid redundant disk I/O.</comment> <file context> @@ -0,0 +1,57 @@ + .sort() + .reverse() + + for (const file of files) { + try { + const content = readFileSync(join(messageDir, file), "utf-8") </file context>

cubic-dev-ai · 2026-02-22T03:08:20Z

src/features/background-agent/fallback-retry-handler.ts

+    parentTools: task.parentTools,
+    model: task.model,
+    fallbackChain: task.fallbackChain,
+    category: task.category,


P2: The retryInput mapping omits the isUnstableAgent property from the task, causing fallback retries to lose their unstable agent status.

Prompt for AI agents

Check if this issue is valid — if so, understand the root cause and fix it. At src/features/background-agent/fallback-retry-handler.ts, line 119: <comment>The `retryInput` mapping omits the `isUnstableAgent` property from the `task`, causing fallback retries to lose their unstable agent status.</comment> <file context> @@ -0,0 +1,125 @@ + parentTools: task.parentTools, + model: task.model, + fallbackChain: task.fallbackChain, + category: task.category, + } + queue.push({ task, input: retryInput }) </file context>

code-yeongyu added 2 commits February 22, 2026 11:58

cubic-dev-ai bot reviewed Feb 22, 2026

View reviewed changes

code-yeongyu merged commit ed43cd4 into dev Feb 22, 2026
8 checks passed

code-yeongyu deleted the refactor/background-manager-extraction branch February 22, 2026 03:09

code-yeongyu mentioned this pull request Feb 22, 2026

fix: address Oracle + Cubic review feedback for background-agent refactoring #2035

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Extract inline logic from BackgroundManager into focused modules#2034

Extract inline logic from BackgroundManager into focused modules#2034
code-yeongyu merged 2 commits intodevfrom
refactor/background-manager-extraction

code-yeongyu commented Feb 22, 2026 •

edited by cubic-dev-ai bot

Loading

Uh oh!

cubic-dev-ai bot left a comment

Uh oh!

cubic-dev-ai bot Feb 22, 2026 •

edited

Loading

Uh oh!

cubic-dev-ai bot Feb 22, 2026 •

edited

Loading

Uh oh!

cubic-dev-ai bot Feb 22, 2026 •

edited

Loading

Uh oh!

cubic-dev-ai bot Feb 22, 2026 •

edited

Loading

Uh oh!

cubic-dev-ai bot Feb 22, 2026 •

edited

Loading

Uh oh!

cubic-dev-ai bot Feb 22, 2026 •

edited

Loading

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant


		registerManagerForCleanup(manager)

		const [, listener] = processOnCalls[0]

Conversation

code-yeongyu commented Feb 22, 2026 • edited by cubic-dev-ai bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Background

Changes

Extracted modules (new files)

Enhanced modules

Wired existing modules

Deleted

Manager.ts reduction

New tests (104 tests across 4 files)

Testing

Review Notes

Summary by cubic

Uh oh!

cubic-dev-ai bot left a comment

Choose a reason for hiding this comment

Uh oh!

cubic-dev-ai bot Feb 22, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cubic-dev-ai bot Feb 22, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cubic-dev-ai bot Feb 22, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cubic-dev-ai bot Feb 22, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cubic-dev-ai bot Feb 22, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cubic-dev-ai bot Feb 22, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

code-yeongyu commented Feb 22, 2026 •

edited by cubic-dev-ai bot

Loading

cubic-dev-ai bot Feb 22, 2026 •

edited

Loading

cubic-dev-ai bot Feb 22, 2026 •

edited

Loading

cubic-dev-ai bot Feb 22, 2026 •

edited

Loading

cubic-dev-ai bot Feb 22, 2026 •

edited

Loading

cubic-dev-ai bot Feb 22, 2026 •

edited

Loading

cubic-dev-ai bot Feb 22, 2026 •

edited

Loading