Conversation
Replace imperative link commands with two declarative config files:
- .brv/workspaces.json (local) — workspace paths for cross-project search
- .brv/context-tree/dependencies.json (remote) — hub bundle tracking
New commands: brv workspace add/remove, brv hub install (all),
brv hub uninstall, /workspace, /hub install, /hub uninstall.
Agent: multi-source search indexing, write guard for linked projects,
score boosting for local results, source metadata in search results.
Infrastructure: git-like .brv/ walk-up resolution, bundles install to
context-tree/bundles/{id}/, status shows workspaces + dependencies,
all TUI/oclif operations go through daemon transport events.
- Use node:path relative() for cross-platform path containment check
in write-guard instead of hardcoded '/' separator
- Add --format json flag to hub uninstall for scripting consistency
- Warn on malformed workspaces.json/dependencies.json instead of
silent empty fallback
- Use resolveRequiredProjectPath in hub install-all and uninstall
handlers instead of falling back to daemon's process.cwd()
- Report skipped (already installed) bundles in install-all results
- Warn on unsupported glob patterns in workspaces.json
- Wire WorkspaceHandler into daemon feature-handlers
- Fix bundle install path to bundles/{id}/ subdirectory
- Fix dependencies.json to store entry id instead of name
- Move install-all logic from oclif to server via INSTALL_ALL event
- Add TUI /hub install and /hub uninstall commands
- Fix lint errors (import ordering, no-await-in-loop, scoping)
Feat/eng 1401
…ng' into proj/multi-repos-knowledge-sharing-develop # Conflicts: # src/server/infra/transport/handlers/status-handler.ts # src/shared/transport/types/dto.ts # src/tui/features/status/utils/format-status.ts
|
Claude finished @wzlng's task in 5m 14s —— View job Code Review
SummaryThis PR implements multi-repo knowledge sharing — a well-scoped feature that lets a ByteRover project link to other projects and include their knowledge in searches. The overall design is clean: declarative config ( Issues FoundBug (write-guard)
Exit codes on failure
Design concerns
Non-determinism
Pre-existing issues surfaced by this PR (follow-up candidates)
Test Coverage
What's Good
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
|
|
||
| const localContextTree = resolve(projectRoot, BRV_DIR, CONTEXT_TREE_DIR) | ||
|
|
||
| const canonicalTarget = tryRealpath(resolve(targetPath)) |
There was a problem hiding this comment.
issue: resolve(targetPath) resolves relative paths against process.cwd(), not projectRoot. If a caller ever passes a relative targetPath, the guard would silently compare against the wrong directory.
While current callers (write-file-tool, curate-service) do pass absolute paths, this is a fragile contract. Prefer making the intent explicit:
| const canonicalTarget = tryRealpath(resolve(targetPath)) | |
| const canonicalTarget = tryRealpath(resolve(projectRoot, targetPath)) |
| * Computes relative path from projectRoot to targetPath. | ||
| * Validates target is a brv project and prevents duplicates. | ||
| */ | ||
| export function addWorkspace(projectRoot: string, targetPath: string): OperationResult { |
There was a problem hiding this comment.
suggestion: addWorkspace validates that the target is a brv project, but doesn't verify that projectRoot itself is one. If called with a non-brv directory as projectRoot, writeWorkspacesFile will happily create .brv/workspaces.json without a corresponding .brv/config.json.
The WorkspaceHandler (transport layer) trusts resolveProjectPath to return a valid project path, which is fine. But for a standalone API this could be confusing. Consider adding a check:
const projectConfig = join(projectRoot, BRV_DIR, PROJECT_CONFIG_FILE)
if (!existsSync(projectConfig)) {
return {message: `Not a ByteRover project: ${projectRoot}`, success: false}
}
| const toInstall: string[] = [] | ||
| const skipped: string[] = [] | ||
| for (const id of entries) { | ||
| const bundleDir = join(projectPath, BRV_DIR, CONTEXT_TREE_DIR, BUNDLES_DIR, id) |
There was a problem hiding this comment.
nit: The already-installed check hard-codes the same bundle path logic that performInstall uses internally. If the install path ever changes (e.g., the bundle directory moves), this skip-check would silently report bundles as "already installed" when they're not, or would re-install when skipped says it shouldn't.
Consider extracting a shared helper (or expose a isInstalled(projectPath, entryId) method on hubInstallService) so there's a single source of truth for "is this bundle installed".
| }), | ||
| ) | ||
|
|
||
| this.log(result.message) |
There was a problem hiding this comment.
issue: On failure (result.success === false), the command logs the error message but exits with code 0. Callers/scripts that check the exit code won't detect the failure.
| this.log(result.message) | |
| if (result.success) { | |
| this.log(result.message) | |
| } else { | |
| this.error(result.message, {exit: 1}) | |
| } |
| async (client) => | ||
| client.requestWithAck<WorkspaceOperationResponse, WorkspaceRemoveRequest>( | ||
| WorkspaceEvents.REMOVE, | ||
| {path: args.path}, |
There was a problem hiding this comment.
issue: Same exit code problem as workspace add — failure exits with code 0. Apply the same fix: this.error(result.message, {exit: 1}) when !result.success.
|
|
||
| try { | ||
| const entries = readdirSync(parentDir, {withFileTypes: true}) | ||
| return entries |
There was a problem hiding this comment.
nit: readdirSync doesn't guarantee ordering — the order of resolved sources will be filesystem-dependent, making the index non-deterministic across platforms/filesystems.
| return entries | |
| return entries | |
| .filter((e) => e.isDirectory()) | |
| .sort((a, b) => a.name.localeCompare(b.name)) | |
| .map((e) => join(parentDir, e.name)) |
| (r) => `### ${r.title}\n**Source**: .brv/context-tree/${r.path}\n\n${r.excerpt}`, | ||
| ) | ||
| const sections = highConfidenceResults.map((r) => { | ||
| const source = r.sourceAlias ? `[${r.sourceAlias}]:${r.path}` : `.brv/context-tree/${r.path}` |
There was a problem hiding this comment.
nit: The source label looks good. However, tryDirectSearchResponse (line ~487, unchanged in this PR) still reads full document content via join(BRV_DIR, CONTEXT_TREE_DIR, result.path), which is the local context tree path and will silently fail for linked results (falling back to excerpt). This pre-exists the PR but is now more relevant — worth a follow-up fix using result.sourceContextTreeRoot ?? join(this.baseDirectory!, BRV_DIR, CONTEXT_TREE_DIR) as the base.
| const basePath = resolve(this.workingDirectory, rawBasePath) | ||
|
|
||
| // Write guard: block writes to linked context trees | ||
| const guardError = validateWriteTarget(basePath, this.workingDirectory) |
There was a problem hiding this comment.
nit: The write guard here blocks the entire batch of operations if the basePath itself is in a linked context tree. This is correct behavior, but the error message includes the guard string (e.g., "Cannot write to knowledge-linked project...") directly without any context about which operation triggered it. The user sees operations.length identical failure entries which might be confusing for a batch operation with mixed targets.
|
Claude finished @wzlng's task in 3m 27s —— View job Code Review — Score: 3/5
SummaryThis PR implements multi-repo knowledge sharing cleanly: declarative config ( Issues Still Unfixed from Previous ReviewBug — write-guard.ts:24 (inline) Bug — exit codes (add.ts, remove.ts) Nit — non-deterministic glob ordering (workspaces-resolver.ts:59) Suggestion — missing projectRoot validation (workspaces-operations.ts:17) New IssuesIssue — alias collision (workspaces-resolver.ts:96) Nit — Pre-existing — Nit — What's Good
|
|
|
||
| try { | ||
| const entries = readdirSync(parentDir, {withFileTypes: true}) | ||
| return entries |
There was a problem hiding this comment.
nit: readdirSync order is filesystem-dependent, so the resolved source list is non-deterministic across platforms (ext4 vs APFS vs HFS+). Flagged in the previous review and still unfixed.
| return entries | |
| return entries | |
| .filter((e) => e.isDirectory()) | |
| .sort((a, b) => a.name.localeCompare(b.name)) | |
| .map((e) => join(parentDir, e.name)) |
| seen.add(canonicalDir) | ||
|
|
||
| return { | ||
| alias: basename(canonicalDir), |
There was a problem hiding this comment.
issue: alias is set to basename(canonicalDir), which is just the directory name. If two linked projects sit at different paths but share the same directory name (e.g. ~/work/app/packages/utils and ~/other/packages/utils), they get the same alias. The [alias]:path namespace would then be ambiguous — search results from both projects would render with the same prefix.
Consider using a truncated hash of canonicalDir (you already have deriveSourceKey for this) or appending a numeric suffix on collision inside resolveWorkspaces.
| * Computes relative path from projectRoot to targetPath. | ||
| * Validates target is a brv project and prevents duplicates. | ||
| */ | ||
| export function addWorkspace(projectRoot: string, targetPath: string): OperationResult { |
There was a problem hiding this comment.
suggestion: addWorkspace validates the target is a brv project, but doesn't check that projectRoot itself is initialized. If called with an uninitialized directory as projectRoot, writeWorkspacesFile will throw an ENOENT at the writeFileSync call because .brv/ doesn't exist.
The WorkspaceHandler relies on resolveRequiredProjectPath to guarantee a valid project path, so this is safe in production. But for standalone API usage (and to match the defensive style of the target check), add:
const projectConfig = join(projectRoot, BRV_DIR, PROJECT_CONFIG_FILE)
if (!existsSync(projectConfig)) {
return {message: `Not a ByteRover project: ${projectRoot}`, success: false}
}Flagged in the previous review and still unaddressed.
| const sections = highConfidenceResults.map( | ||
| (r) => `### ${r.title}\n**Source**: .brv/context-tree/${r.path}\n\n${r.excerpt}`, | ||
| ) | ||
| const sections = highConfidenceResults.map((r) => { |
There was a problem hiding this comment.
nit: The source label for linked results is [alias]:path in the pre-fetched context, but tryDirectSearchResponse (further down, unchanged by this PR) still reads content via join(BRV_DIR, CONTEXT_TREE_DIR, result.path) — the local context tree path — for all results. For linked results this will silently fall back to result.excerpt. The attribution is now correct here; the content read in tryDirectSearchResponse is the remaining gap to fix in a follow-up.
|
|
||
| const localContextTree = resolve(projectRoot, BRV_DIR, CONTEXT_TREE_DIR) | ||
|
|
||
| const canonicalTarget = tryRealpath(resolve(targetPath)) |
There was a problem hiding this comment.
issue: resolve(targetPath) resolves relative paths against process.cwd(), not projectRoot. This was flagged in the previous review and is still unfixed.
Current callers always pass absolute paths so it doesn't fail today, but any future caller passing a relative path will silently compare against the wrong directory.
| const canonicalTarget = tryRealpath(resolve(targetPath)) | |
| const canonicalTarget = tryRealpath(resolve(projectRoot, targetPath)) |
| }), | ||
| ) | ||
|
|
||
| this.log(result.message) |
There was a problem hiding this comment.
issue: Failure exits with code 0 — this.log() is used regardless of result.success. Scripts and CI that rely on exit codes will silently miss errors. Flagged in the previous review and still unfixed.
| this.log(result.message) | |
| if (result.success) { | |
| this.log(result.message) | |
| } else { | |
| this.error(result.message, {exit: 1}) | |
| } |
| async (client) => | ||
| client.requestWithAck<WorkspaceOperationResponse, WorkspaceRemoveRequest>( | ||
| WorkspaceEvents.REMOVE, | ||
| {path: args.path}, |
There was a problem hiding this comment.
issue: Same exit-code problem as workspace add. Apply the same conditional fix.
| {path: args.path}, | |
| if (result.success) { | |
| this.log(result.message) | |
| } else { | |
| this.error(result.message, {exit: 1}) | |
| } |
|
|
||
| const localContextTree = resolve(projectRoot, BRV_DIR, CONTEXT_TREE_DIR) | ||
|
|
||
| const canonicalTarget = tryRealpath(resolve(targetPath)) |
There was a problem hiding this comment.
issue: resolve(targetPath) resolves relative targetPath against process.cwd(), not projectRoot. This was flagged in the previous review and remains unfixed.
Current callers (write-file-tool, curate-service) always pass absolute paths so it doesn't break today, but the contract is fragile: any future caller passing a relative path will silently compare against the wrong directory.
| const canonicalTarget = tryRealpath(resolve(targetPath)) | |
| const canonicalTarget = tryRealpath(resolve(projectRoot, targetPath)) |
| }), | ||
| ) | ||
|
|
||
| this.log(result.message) |
There was a problem hiding this comment.
issue: Failure is logged via this.log() — exit code stays 0 even when result.success === false. Scripts and CI pipelines that check exit codes will silently miss failures. Flagged in the previous review and still unaddressed.
| this.log(result.message) | |
| if (result.success) { | |
| this.log(result.message) | |
| } else { | |
| this.error(result.message, {exit: 1}) | |
| } |
| async (client) => | ||
| client.requestWithAck<WorkspaceOperationResponse, WorkspaceRemoveRequest>( | ||
| WorkspaceEvents.REMOVE, | ||
| {path: args.path}, |
There was a problem hiding this comment.
issue: Same exit code problem as workspace add. this.log(result.message) always exits 0. Apply the same conditional fix.
| {path: args.path}, | |
| if (result.success) { | |
| this.log(result.message) | |
| } else { | |
| this.error(result.message, {exit: 1}) | |
| } |
1 participant
No description provided.