feat:[ENG-2075] add ByteRover context tree as write fallback

[cuongdo-byterover]

• Change selectWriteTarget() to return null instead of throwing when no writable providers are available
• Add optional ICurateService to SwarmCoordinator constructor
• Add fallbackToByterover() that routes content to the curation pipeline (snippets) when no external provider can accept the write
• Add fallback?: boolean to SwarmStoreResult type
• Update CLI curate output to show fallback message
• 5 new tests: fallback with/without curateService, fallback on curate failure, no fallback for explicit --provider, normal write without fallback flag

Summary

• Problem:
• Why it matters:
• What changed:
• What did NOT change (scope boundary):

Type of change

• Bug fix
• New feature
• Refactor (no behavior change)
• Documentation
• Test
• Chore (build, dependencies, CI)

Scope (select all touched areas)

• TUI / REPL
• Agent / Tools
• LLM Providers
• Server / Daemon
• Shared (constants, types, transport events)
• CLI Commands (oclif)
• Hub / Connectors
• Cloud Sync
• CI/CD / Infra

Linked issues

• Closes #
• Related #

Root cause (bug fixes only, otherwise write N/A)

• Root cause:
• Why this was not caught earlier:

Test plan

• Coverage added:
  • Unit test
  • Integration test
  • Manual verification only
• Test file(s):
• Key scenario(s) covered:

User-visible changes

List user-visible changes (including defaults, config, or CLI output).
If none, write None.

Evidence

Attach at least one:

• Failing test/log before + passing after
• Trace/log snippets
• Screenshot/recording

Checklist

• Tests added or updated and passing (npm test)
• Lint passes (npm run lint)
• Type check passes (npm run typecheck)
• Build succeeds (npm run build)
• Commits follow Conventional Commits format
• Documentation updated (if applicable)
• No breaking changes (or clearly documented above)
• Branch is up to date with main

Risks and mitigations

List real risks for this PR. If none, write None.

• Risk:
  • Mitigation:

[github-actions]

Claude finished @cuongdo-byterover's task in 2m 24s —— View job

---

PR Review: feat:[ENG-2075] add ByteRover context tree as write fallback

Score: 3.5 / 5 — Solid feature implementation with clean dependency injection and good test coverage. A few semantic inconsistencies and one silent-failure path need attention before merging.

---

• Gather context: read changed files and git diff
• Review interface changes (i-swarm-coordinator.ts)
• Review core implementation (swarm-coordinator.ts)
• Review write router changes (swarm-write-router.ts)
• Review CLI output changes (curate.ts)
• Review test coverage (swarm-coordinator.test.ts, swarm-write-router.test.ts)
• Post final review with score

---

Issues

suggestion (correctness): Stale @throws JSDoc in swarm-write-router.ts

The JSDoc on selectWriteTarget still documents @throws Error if no writable+healthy provider is available (line 38), but the function now returns null. This is a direct documentation lie that will mislead future callers.

-  * @throws Error if no writable+healthy provider is available
+  * @returns null if no writable+healthy provider is available

Fix this →

---

issue (semantic inconsistency): fallback: false in no-curateService path vs undefined elsewhere

In fallbackToByterover() when !this.curateService, the error response explicitly sets fallback: false. But in all other failure paths (provider not found, write error, etc.), fallback is absent (undefined). The SwarmStoreResult interface documents this field as "True when content was routed to context tree as fallback" — setting it to false leaks the internal branching logic without a meaningful contract.

A caller doing if (result.fallback) treats both false and undefined the same way, but one who does if ('fallback' in result) gets surprising results. More importantly, fallback: false makes it look like "we tried and it failed," while undefined reads as "not applicable."

Suggestion: omit the field when no fallback was attempted, matching the established pattern:

  return {
    error: 'No writable providers available and curate service not configured...',
-   fallback: false,
    id: '',
    latencyMs: Date.now() - start,
    provider: '',
    success: false,
  }

Fix this →

---

issue (correctness): Silent failure when applied array is empty

swarm-coordinator.ts lines 467–474:

const firstApplied = result.applied[0]
return {
  fallback: true,
  id: firstApplied?.path ?? 'context-tree',
  latencyMs: Date.now() - start,
  provider: 'byterover',
  success: firstApplied?.status === 'success',  // false when undefined
}

If curateService.curate() resolves successfully but returns applied: [] (valid per the CurateResult contract), the result is {success: false, fallback: true, id: 'context-tree', error: undefined}. The caller gets a failure with no error message and a misleading id. There's no test for this path.

Suggestion: detect the empty case explicitly:

const firstApplied = result.applied[0]
if (!firstApplied) {
  return {
    error: 'Curate service returned no applied operations',
    fallback: true,
    id: '',
    latencyMs: Date.now() - start,
    provider: 'byterover',
    success: false,
  }
}
return {
  fallback: true,
  id: firstApplied.path,
  latencyMs: Date.now() - start,
  provider: 'byterover',
  success: firstApplied.status === 'success',
}

Fix this →

---

suggestion (minor): CurateService always instantiated in CLI even for explicit-provider requests

src/oclif/commands/swarm/curate.ts line 57 unconditionally instantiates new CurateService(workingDirectory) before calling coordinator.store(). When flags.provider is explicitly set, the fallback path is never reached. If CurateService construction has initialization side-effects or allocates resources, this wastes them. Low-risk if the constructor is cheap, but worth considering.

const curateService = flags.provider ? undefined : new CurateService(workingDirectory)
const coordinator = new SwarmCoordinator(providers, config, curateService)

---

nitpick: Missing test for applied: [] from curate service

The 5 new tests cover the main paths well. The gap is when curate() resolves but applied is empty — currently a silent failure (see issue above). A test like:

it('fallback returns failure when curateService returns empty applied', async () => {
  const mockCurate = { curate: sinon.stub().resolves({ applied: [], summary: {...} }), detectDomains: sinon.stub() }
  const coordinator = new SwarmCoordinator([obsidian], config, mockCurate)
  const result = await coordinator.store({content: 'test'})
  expect(result.success).to.be.false
  expect(result.error).to.include('no applied operations')
})

---

What's good

• Clean DI pattern: Optional curateService in the constructor is idiomatic and testable. No service locator or global state.
• Fallback boundary is correct: The fallback only triggers in auto-route mode. Explicit --provider calls never hit it, which is the right semantic.
• Error message quality: The "no curate service configured" message includes a actionable hint (brv curate).
• selectWriteTarget returning null instead of throwing: The right call — throwing from a routing helper that has a well-defined "no match" case was a design smell.
• Test for "no fallback for explicit --provider": This is exactly the kind of negative test that catches accidental fallback-always bugs.

[github-actions]

Claude finished @cuongdo-byterover's task in 2m 59s —— View job

---

PR Review: feat:[ENG-2075] add ByteRover context tree as write fallback

Score: 3 / 5 — The approach pivot from CurateService injection to a brv curate subprocess resolves the previous review's applied[] and fallback: false issues, but introduces new concerns around test isolation (a CLAUDE.md violation), stdout parsing, and argument length limits. Three issues below need attention before merging.

---

• Gather context: read changed files and git diff
• Review interface changes (i-swarm-coordinator.ts)
• Review core implementation (swarm-coordinator.ts)
• Review write router changes (swarm-write-router.ts)
• Review CLI output changes (curate.ts)
• Review test coverage (swarm-coordinator.test.ts, swarm-write-router.test.ts)
• Post final review with score

---

Issues

issue (correctness): id captures full human-readable stdout, not a task/log ID

swarm-coordinator.ts line 460:

id: result.stdout.trim(),

brv curate --detach in text mode outputs "✓ Context queued for processing. (Log: <logId>)" to stdout. That full sentence is stored as the id field, which then gets rendered in the CLI output (curate.ts line 67) as:

Stored to byterover (fallback — no external providers available) as ✓ Context queued for processing. (Log: abc-123)

The id field is documented as "ID assigned by the target provider" — it should be the log ID or task ID, not the whole message. Add --format json so the output is machine-parseable, then extract data.logId or data.taskId:

execFile('brv', ['curate', '--detach', '--format', 'json', request.content], ...)
// ...
const parsed = JSON.parse(result.stdout)
return {
  fallback: true,
  id: parsed.data?.logId ?? parsed.data?.taskId ?? '',
  latencyMs: Date.now() - start,
  provider: 'byterover',
  success: parsed.success === true,
}

Fix this →

---

issue (CLAUDE.md violation): Unit test does not mock execFile — calls real subprocess

swarm-coordinator.test.ts lines 392–398 include an explicit comment acknowledging the gap:

// The fallback calls `brv curate --detach` via execFile.
// We can't easily mock execFile here, so we verify the result shape
// when the fallback is triggered (it will fail because brv daemon
// may not be running in test, but the fallback flag should be set).

CLAUDE.md is unambiguous: "Unit tests must run fast and run completely in memory. Proper stubbing and mocking must be implemented." A test that spawns a real brv process is not a unit test — it's depending on the environment, making CI non-deterministic.

execFile is easily mockable with sinon by stubbing the node:child_process module:

import * as childProcess from 'node:child_process'
// ...
let execFileStub: sinon.SinonStub

beforeEach(() => {
  execFileStub = sinon.stub(childProcess, 'execFile')
})
afterEach(() => { execFileStub.restore() })

it('falls back to brv curate when no writable providers', async () => {
  execFileStub.callsFake((_cmd, _args, _opts, cb) => {
    cb(null, '{"success":true,"data":{"logId":"log-abc","taskId":"task-xyz","status":"queued"}}', '')
  })
  // ...
  expect(result.fallback).to.be.true
  expect(result.id).to.equal('log-abc')
})

Fix this →

---

suggestion (robustness): Large content will exceed OS ARG_MAX limits

request.content (the full knowledge payload) is passed as a single command-line argument:

execFile('brv', ['curate', '--detach', request.content], ...)

Linux's ARG_MAX is typically 2 MB total for all env vars + args. Agent content can easily exceed this for large documents, causing an E2BIG (Argument list too long) error. The brv curate command supports --files for file-based input but not for raw content from stdin.

The safest approach is to write content to a temp file and pass the path via --files:

import {writeFile, mkdtemp, rm} from 'node:fs/promises'
import {tmpdir} from 'node:os'
import {join} from 'node:path'

const dir = await mkdtemp(join(tmpdir(), 'brv-fallback-'))
const tmpFile = join(dir, 'content.txt')
try {
  await writeFile(tmpFile, request.content, 'utf8')
  execFile('brv', ['curate', '--detach', '--format', 'json', '--files', tmpFile, 'Fallback content'], ...)
} finally {
  await rm(dir, {recursive: true, force: true})
}

---

nitpick: PR description still references the removed CurateService approach

The most recent commit (fix:[ENG-2075] use brv curate subprocess for fallback instead of CurateService) changed the implementation, but the PR description still says "Add optional ICurateService to SwarmCoordinator constructor" and "5 new tests: fallback with/without curateService, fallback on curate failure, no fallback for explicit --provider, normal write without fallback flag". These no longer match the code. Not blocking, but worth updating so reviewers aren't misled.

---

What's good

• Previous review issues resolved: The stale @throws JSDoc is fixed, the fallback: false semantic inconsistency is gone, and the applied[] empty array silent failure no longer exists — all three because the approach was replaced with a cleaner subprocess call.
• selectWriteTarget returning null: Already present from the previous commit; this PR correctly adapts the coordinator to handle the null return without throwing.
• Fallback boundary preserved: The if (request.provider) guard on line 364 still prevents explicit-provider requests from hitting the fallback. The negative test ("does not fallback for explicit --provider target") correctly covers this.
• Timeout on subprocess: 30-second execFile timeout is appropriate and prevents indefinite hangs.
• Error surfacing from stderr: reject(new Error(stderr?.trim() || error.message)) correctly prefers stderr over the exit code message, which is where CLI tools put meaningful errors.

[github-actions]

Claude finished @cuongdo-byterover's task in 2m 21s —— View job

---

PR Review: feat:[ENG-2075] add ByteRover context tree as write fallback

Score: 3.5 / 5 — The pivot to CurateFallbackFn injection cleanly resolves all three issues from the previous review (CLAUDE.md test violation, stale JSDoc, applied[] silent failure). Two new issues introduced: a likely lint failure from interleaved imports, and silent error-message loss when brv curate returns success: false with an error body. The ARG_MAX risk from the prior review is still unaddressed.

---

• Read changed files and git diff
• Review interface changes (i-swarm-coordinator.ts)
• Review core implementation (swarm-coordinator.ts)
• Review write router changes (swarm-write-router.ts)
• Review CLI output changes (curate.ts)
• Review test coverage (swarm-coordinator.test.ts, swarm-write-router.test.ts)
• Post final review with score

---

Issues

issue (lint failure): Imports split by a function declaration in swarm-coordinator.ts

execBrvCurate is declared between the first import (line 1) and the rest of the imports (lines 29–44). ESLint's import/first rule requires all import statements to appear before any other code. This will likely fail lint and is the most actionable fix in this PR.

Move all imports to the top, then place execBrvCurate after:

// --- top of file ---
import {execFile} from 'node:child_process'
import type {QueryRequest} from '../../core/domain/swarm/types.js'
import type {IMemoryProvider} from '../../core/interfaces/i-memory-provider.js'
import type {
  ISwarmCoordinator,
  ProviderInfo,
  ProviderQueryMeta,
  SwarmQueryResult,
  SwarmStoreRequest,
  SwarmStoreResult,
  SwarmSummary,
} from '../../core/interfaces/i-swarm-coordinator.js'
import type {SwarmConfig} from './config/swarm-config-schema.js'
import {SwarmGraph} from './swarm-graph.js'
// ... rest of imports ...

export function execBrvCurate(...) { ... }

Fix this →

---

issue (correctness): execBrvCurate return type drops the error message on success: false

The resolved type is {data?: {logId?: string; taskId?: string}; success?: boolean}. When brv curate --format json returns {"success": false, "error": "task queue full"}, the error field is not in the type and gets silently dropped. fallbackToByterover then returns {success: false, fallback: true, error: undefined} — the caller sees a failure with no explanation.

Add error?: string to the return type and thread it through:

export function execBrvCurate(content: string): Promise<{
  data?: {logId?: string; taskId?: string}
  error?: string
  success?: boolean
}> { ... }

// in fallbackToByterover, success path:
return {
  error: parsed.success === true ? undefined : (parsed.error ?? 'brv curate returned success: false'),
  fallback: true,
  id: parsed.data?.logId ?? parsed.data?.taskId ?? '',
  latencyMs: Date.now() - start,
  provider: 'byterover',
  success: parsed.success === true,
}

Fix this →

---

suggestion (robustness): Large content will exceed OS ARG_MAX limits (carried from previous review, still unaddressed)

content is passed as a positional CLI argument: execFile('brv', ['curate', '--detach', '--format', 'json', content], ...). Linux's ARG_MAX is typically 2 MB for all args + env. Agent knowledge payloads can exceed this, producing an E2BIG error. Write to a temp file and pass the path instead:

import {mkdtemp, rm, writeFile} from 'node:fs/promises'
import {tmpdir} from 'node:os'
import {join} from 'node:path'

const dir = await mkdtemp(join(tmpdir(), 'brv-fallback-'))
const tmpFile = join(dir, 'content.txt')
try {
  await writeFile(tmpFile, content, 'utf8')
  // pass --files tmpFile with a short title arg
  execFile('brv', ['curate', '--detach', '--format', 'json', '--files', tmpFile, 'fallback'], ...)
} finally {
  await rm(dir, {recursive: true, force: true})
}

---

nitpick: execBrvCurate exported but only needed internally

Tests inject the mock via the CurateFallbackFn constructor parameter — they never import execBrvCurate directly. Exporting it unnecessarily widens the public API surface. The export keyword can be removed unless there's a consumer outside this module.

---

nitpick: CLI output has trailing "as " when id is empty

curate.ts line 67: Stored to ${result.provider} (fallback — no external providers available) as ${result.id} — when id is '' (e.g. brv curate --format json returns {"success": true} with no data), output is:

Stored to byterover (fallback — no external providers available) as 

Condition it on result.id being non-empty, or omit "as" when empty:

const idPart = result.id ? ` as ${result.id}` : ''
this.log(`Stored to ${result.provider} (fallback — no external providers available)${idPart}`)

---

What's good

• CLAUDE.md violation resolved: Tests now inject mockCurate via constructor — no real subprocess, runs in memory. Clean.
• CurateFallbackFn injection pattern: Right call. The default execBrvCurate is cleanly separated from the class, and the constructor parameter makes the seam obvious for tests and future callers.
• --format json added: id field now correctly carries logId/taskId (not the whole human-readable string from the previous review).
• All previous review issues fixed: Stale @throws JSDoc gone, fallback: false inconsistency gone, applied[] empty-array silent failure gone — all resolved by replacing the CurateService approach.
• Fallback boundary correct: request.provider guard on line 417 preserves the explicit-provider contract; the negative test covers it.
• swarm-write-router.ts test updates: Non-null assertion target!.id throughout is the correct adaptation to the IMemoryProvider | null return type.

[github-actions]

Claude finished @cuongdo-byterover's task in 2m 16s —— View job

---

PR Review: feat:[ENG-2075] add ByteRover context tree as write fallback

Score: 4 / 5 — All three hard issues and both nitpicks from the previous review are resolved. Two minor new items introduced: a as type assertion that violates the CLAUDE.md TypeScript guideline, and a missing test for the success: false + error-message propagation path. The ARG_MAX suggestion remains unaddressed.

---

• Gather context: read changed files and git diff
• Review interface changes (i-swarm-coordinator.ts)
• Review core implementation (swarm-coordinator.ts)
• Review write router changes (swarm-write-router.ts)
• Review CLI output changes (curate.ts)
• Review test coverage (swarm-coordinator.test.ts, swarm-write-router.test.ts)
• Post final review with score

---

Issues

issue (CLAUDE.md violation): as type assertion on error object in execBrvCurate

swarm-coordinator.ts line 34:

const err = error as {message: string; stderr?: string}

CLAUDE.md says: "Avoid as Type assertions — use type guards or proper typing instead." execFile errors are standard Error objects with optional extra fields; narrow with instanceof and in:

} catch (error) {
  const stderr =
    error instanceof Error && 'stderr' in error && typeof (error as {stderr: unknown}).stderr === 'string'
      ? (error as {stderr: string}).stderr.trim()
      : undefined
  throw new Error(stderr || (error instanceof Error ? error.message : String(error)))
}

Fix this →

---

nitpick: Missing test for success: false with error message from curate mock

The four fallback tests cover: success path (success: true), throw path, no-fallback for explicit provider, and normal write. The gap is when the mock resolves with {success: false, error: 'some error'} — this exercises the parsed.error ?? 'brv curate returned success: false' branch in fallbackToByterover and is the only way to confirm error propagation on a non-throwing failure:

it('fallback propagates error message when brv curate returns success: false', async () => {
  const mockCurate = sinon.stub().resolves({
    error: 'queue full',
    success: false,
  })
  const obsidian = createMockProvider('obsidian', 'obsidian', [])
  const config = createMinimalConfig()
  const coordinator = new SwarmCoordinator([obsidian], config, mockCurate)

  const result = await coordinator.store({content: 'test'})
  expect(result.success).to.be.false
  expect(result.fallback).to.be.true
  expect(result.error).to.equal('queue full')
})

---

suggestion (robustness): Large content will exceed OS ARG_MAX limits (unaddressed since 2nd review)

content is still passed as a positional CLI argument (execFile('brv', ['curate', '--detach', '--format', 'json', content], ...)). Linux ARG_MAX is ~2 MB total. Large agent knowledge payloads will produce an E2BIG error with no meaningful message to the caller. Writing to a temp file and passing --files is the safe path. This is a suggestion, not a blocker.

---

What's good

• All previous hard issues resolved:
  • Import ordering is clean — node:child_process / node:util imports come first, then all other imports, then BrvCurateResult type and execBrvCurate. The import/first lint risk from the prior review is gone.
  • BrvCurateResult now includes error?: string. Error messages from {success: false, error: "..."} responses are threaded through to SwarmStoreResult.error.
  • execBrvCurate is no longer exported — correctly private to this module.
• Both previous nitpicks resolved: CLI output uses idPart conditional to avoid trailing "as " when id is empty. execBrvCurate is not exported.
• promisify over manual callback wrapper: Cleaner than the manual callback wrapper from the prior iteration.
• BrvCurateResult exported: Correct to export since CurateFallbackFn (a constructor parameter type) references it — callers who provide a custom fallback need the return shape.
• Test injection pattern is clean: mockCurate stub injected via constructor, no subprocess spawned, full in-memory test. CLAUDE.md compliance restored.
• swarm-write-router.ts tests: Non-null assertions target!.id consistently applied after the null-return change.

[danhdoan]

lgtm