src/plugins/trackers/builtin/beads-bv/index.ts (1)

374-384: Consider caching epic children to avoid repeated bd list calls.

getEpicChildrenIds spawns a child process on every getNextTask invocation when an epic is active. If task selection is called frequently (e.g. polling), this could add latency. A short-lived cache (TTL or invalidated on completeTask/updateTaskStatus) would eliminate redundant subprocess calls.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/plugins/trackers/builtin/beads-bv/index.ts` around lines 374 - 384,
getNextTask calls getEpicChildrenIds on every invocation which spawns a
subprocess; add a short-lived cache to avoid repeated bd list calls by caching
results inside the tracker instance and invalidating it on mutations. Implement
a simple TTL-backed cache field (e.g. epicChildrenCache with {ids, expiresAt})
used by getEpicChildrenIds so it returns cached ids when not expired, and ensure
cache is cleared/updated in completeTask and updateTaskStatus (and whenever
getEpicId changes) so stale results aren't used; keep fallback to
super.getNextTask unchanged when id not in children.

src/plugins/trackers/builtin/beads-bv/index.ts (1)

389-399: Epic membership check spawns a child process on every getNextTask call.

getEpicChildrenIds runs bd list --json --parent ... each time getNextTask is invoked with an epicId or filter.parentId. If getNextTask is called frequently (e.g., in a loop or polling scenario), this could be a noticeable overhead. Consider caching the epic children list and invalidating it when tasks are completed or statuses change.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/plugins/trackers/builtin/beads-bv/index.ts` around lines 389 - 399,
getNextTask is calling getEpicChildrenIds every invocation which shells out via
bd list; add an in-memory cache to avoid spawning a child process on every call
by caching results keyed by parentId inside the tracker (e.g.,
this._epicChildrenCache) and update getEpicChildrenIds to read/write that cache;
invalidate or refresh the cache when tasks change (on task completion, status
updates, or after a short TTL) by adding an explicit
invalidateEpicChildren(parentId) and/or TTL-based refresh logic so getNextTask
reads from the cache and only invokes the bd list subprocess when the cache is
missing/stale.

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

src/plugins/trackers/builtin/beads-bv/index.ts (2)

360-367: All three status-filter fall-throughs now correctly delegate to super.getNextTask(filter).

Lines 360–367, 426–428, and 443–445 all return super.getNextTask(filter) instead of undefined, preserving filter semantics as previously requested.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/plugins/trackers/builtin/beads-bv/index.ts` around lines 360 - 367, The
status-filter fall-throughs in getNextTask incorrectly returned undefined
previously; update each branch that checks statusFilter (the checks around the
status array logic inside getNextTask in
src/plugins/trackers/builtin/beads-bv/index.ts) to delegate back to the parent
by returning super.getNextTask(filter) when the statusFilter excludes 'open' and
'in_progress', ensuring all three occurrences use super.getNextTask(filter) to
preserve filter semantics.

---

49-76: Discriminated union correctly addresses the prior type-safety concern.

BvRobotNextOutput as BvRobotNextTask | BvRobotNextEmpty and the narrowing guard 'message' in nextOutput at line 402 properly resolve the previously flagged issue with id being required on a response shape that omits it.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/plugins/trackers/builtin/beads-bv/index.ts` around lines 49 - 76, The
discriminated-union fix using BvRobotNextOutput = BvRobotNextTask |
BvRobotNextEmpty is correct; ensure any consumer uses the narrowing guard
('message' in nextOutput) before accessing task-only fields like id, and remove
the duplicate/extra review comment that was posted (the one repeating approval)
so the PR comments are not duplicated.

src/plugins/trackers/builtin/beads-bv/index.ts (2)

406-416: Consider caching getEpicChildrenIds results to avoid a redundant process spawn per call.

When an epic is configured, every getNextTask invocation spawns two child processes: bv --robot-next and bd list --json --parent <epicId>. Epic membership rarely changes between successive calls, so the second spawn is effectively redundant.

♻️ Suggested approach

+  private epicChildrenCache: Map<string, { ids: string[]; fetchedAt: number }> = new Map();
+  private readonly EPIC_CHILDREN_TTL_MS = 60_000; // 1 minute

   private async getEpicChildrenIds(epicId: string): Promise<string[]> {
+    const cached = this.epicChildrenCache.get(epicId);
+    if (cached && Date.now() - cached.fetchedAt < this.EPIC_CHILDREN_TTL_MS) {
+      return cached.ids;
+    }
+
     const { stdout, exitCode } = await execBd(
       ['list', '--json', '--parent', epicId, '--limit', '0'],
       this.getWorkingDir()
     );
     if (exitCode !== 0) { return []; }
     try {
       const beads = JSON.parse(stdout) as Array<{ id: string }>;
-      return beads.map((b) => b.id);
+      const ids = beads.map((b) => b.id);
+      this.epicChildrenCache.set(epicId, { ids, fetchedAt: Date.now() });
+      return ids;
     } catch {
       return [];
     }
   }

The cache can be invalidated in completeTask and updateTaskStatus where task state changes occur.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/plugins/trackers/builtin/beads-bv/index.ts` around lines 406 - 416,
getNextTask calls getEpicChildrenIds each time which spawns an extra process;
cache getEpicChildrenIds results (keyed by epicId) in the tracker instance to
reuse between getNextTask calls and avoid redundant bd list spawns. Implement a
simple in-memory cache (e.g. a Map<string, string[]> or { ids, timestamp }) and
consult it in getNextTask before calling getEpicChildrenIds; fall back to
calling getEpicChildrenIds when cache miss or expired. Invalidate or update the
cache inside completeTask and updateTaskStatus whenever tasks move between epics
or their parent changes so epic membership stays correct, and keep the existing
fallback to super.getNextTask when nextOutput.id is not in the cached epic
children.

---

751-753: Add a .catch() to the fire-and-forget refreshTriage() promise.

refreshTriage() is currently designed to always resolve, but without an explicit .catch() any future change that allows a rejection to escape would become an unhandled promise rejection. Starting with Node.js 15, unhandled rejections crash the application by default.

♻️ Suggested fix

-    this.triageRefreshInFlight = this.refreshTriage().finally(() => {
-      this.triageRefreshInFlight = null;
-    });
+    this.triageRefreshInFlight = this.refreshTriage()
+      .catch((err) => {
+        console.error('Unexpected error in background triage refresh:', err);
+      })
+      .finally(() => {
+        this.triageRefreshInFlight = null;
+      });

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/plugins/trackers/builtin/beads-bv/index.ts` around lines 751 - 753, The
fire-and-forget assignment to this.triageRefreshInFlight uses refreshTriage()
without a .catch(), which risks unhandled promise rejections if refreshTriage
ever throws; update the expression that sets this.triageRefreshInFlight (the
call to refreshTriage() followed by .finally(...)) to chain a .catch(err => { /*
log or handle error */ }) before the .finally so any rejection is handled (use
your logger e.g. this.logger.error or console.error and include the error) and
leave the existing .finally(() => { this.triageRefreshInFlight = null; })
intact.

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

src/plugins/trackers/builtin/beads-bv/index.ts (1)

160-227: execBv and execBd are near-identical — consider extracting a shared helper.

These two functions differ only in the spawned binary name ('bv' vs 'bd'). A single parameterised helper (e.g. execCommand(bin: string, args, cwd)) would eliminate the duplication.

♻️ Proposed refactor

-async function execBv(
-  args: string[],
-  cwd?: string
-): Promise<{ stdout: string; stderr: string; exitCode: number }> {
-  return new Promise((resolve) => {
-    const proc = spawn('bv', args, {
-      cwd,
-      env: { ...process.env },
-      stdio: ['ignore', 'pipe', 'pipe'],
-    });
-    ...
-  });
-}
-
-async function execBd(
-  args: string[],
-  cwd?: string
-): Promise<{ stdout: string; stderr: string; exitCode: number }> {
-  return new Promise((resolve) => {
-    const proc = spawn('bd', args, {
-      cwd,
-      env: { ...process.env },
-      stdio: ['ignore', 'pipe', 'pipe'],
-    });
-    ...
-  });
-}
+async function execCommand(
+  bin: string,
+  args: string[],
+  cwd?: string
+): Promise<{ stdout: string; stderr: string; exitCode: number }> {
+  return new Promise((resolve) => {
+    const proc = spawn(bin, args, {
+      cwd,
+      env: { ...process.env },
+      stdio: ['ignore', 'pipe', 'pipe'],
+    });
+
+    let stdout = '';
+    let stderr = '';
+
+    proc.stdout.on('data', (data: Buffer) => {
+      stdout += data.toString();
+    });
+
+    proc.stderr.on('data', (data: Buffer) => {
+      stderr += data.toString();
+    });
+
+    proc.on('close', (code) => {
+      resolve({ stdout, stderr, exitCode: code ?? 1 });
+    });
+
+    proc.on('error', (err) => {
+      stderr += err.message;
+      resolve({ stdout, stderr, exitCode: 1 });
+    });
+  });
+}
+
+const execBv = (args: string[], cwd?: string) => execCommand('bv', args, cwd);
+const execBd = (args: string[], cwd?: string) => execCommand('bd', args, cwd);

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/plugins/trackers/builtin/beads-bv/index.ts` around lines 160 - 227, Both
execBv and execBd duplicate the same spawn-and-collect logic; extract a single
parameterized helper (e.g. execCommand or execSpawn) that takes the binary name
(bin: string), args: string[], and optional cwd and returns Promise<{ stdout:
string; stderr: string; exitCode: number }>, then implement execBv and execBd as
thin wrappers that call this helper (execCommand('bv', args, cwd) and
execCommand('bd', args, cwd)). Ensure the helper reproduces the existing
behavior: spawn with env: { ...process.env } and stdio ['ignore','pipe','pipe'],
accumulates stdout/stderr from data events, resolves on 'close' with code ?? 1,
and handles 'error' by appending err.message to stderr and resolving with
exitCode 1.

src/plugins/trackers/builtin/beads-bv/index.test.ts (2)

211-243: No test for bv --robot-next returning a non-zero exit code.

The getNextTask method falls back to super.getNextTask(filter) when exitCode !== 0 (line 390 in index.ts). This path isn't exercised by the current test suite. Consider adding a test that queues a response with a non-zero exit code and verifies the fallback.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/plugins/trackers/builtin/beads-bv/index.test.ts` around lines 211 - 243,
Add a test that covers the branch where bv --robot-next returns a non-zero exit
code so getNextTask falls back to the base tracker: in the existing
describe('getNextTask with --robot-next') block create a test using
BeadsBvTrackerPlugin with bvAvailable=true and scheduleTriageRefresh stubbed,
call queueSpawnResponse with command 'bv' and a non-zero exitCode (e.g.
exitCode: 1) (and optional stdout/stderr), stub
BeadsTrackerPlugin.prototype.getNextTask to return a fallback TrackerTask,
invoke plugin.getNextTask() and assert it equals the fallback, then restore the
original BeadsTrackerPlugin.prototype.getNextTask.

---

234-242: Prototype mutation for BeadsTrackerPlugin.prototype.getNextTask is restored safely via finally.

This pattern is acceptable since tests within a describe block run sequentially in Bun. The try/finally ensures restoration even if the assertion throws. Repeated across multiple tests (lines 275-283 as well) — if more tests adopt this pattern, consider extracting a small helper to reduce boilerplate.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/plugins/trackers/builtin/beads-bv/index.test.ts` around lines 234 - 242,
Tests repeatedly mutate BeadsTrackerPlugin.prototype.getNextTask and restore it
in a try/finally; extract a small reusable helper (e.g., stubPrototypeMethod or
withTemporaryPrototypeStub) to encapsulate saving the original (from
BeadsTrackerPlugin.prototype.getNextTask), assigning the stubbed async
implementation (returning fallbackTask), running the test callback, and
restoring the original in a finally block so all tests can call the helper
instead of duplicating the try/finally logic.

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro