Skip to content

Proj/multi repos knowledge sharing#350

Merged
bao-byterover merged 22 commits intomainfrom
proj/multi-repos-knowledge-sharing
Apr 11, 2026
Merged

Proj/multi repos knowledge sharing#350
bao-byterover merged 22 commits intomainfrom
proj/multi-repos-knowledge-sharing

Conversation

@wzlng
Copy link
Copy Markdown
Collaborator

@wzlng wzlng commented Apr 9, 2026

Multi-Repo Knowledge Sharing

Summary

Adds two capabilities for working across multiple repositories:

  • Worktree linking — Run brv from monorepo subdirectories or sibling checkouts without creating nested .brv/ directories. Uses a git-style model where .brv can be either a directory (real project) or a file (pointer to parent project).
  • Knowledge sources — Reference other projects' context trees as read-only search sources, enabling cross-project knowledge sharing during queries.

Worktree linking (brv worktree)

# From parent project:
brv worktree add packages/api       # registers subdirectory
brv worktree add ../other-checkout   # registers sibling

# From the registered directory:
brv query "auth patterns"            # uses parent's context tree
brv worktree list                    # shows link details
brv worktree remove                  # unregisters

How it works: brv worktree add creates a .brv pointer file in the target directory (like git's .git file in worktrees) and a registry entry in the parent's .brv/worktrees/. The resolver walks up from cwd to find the nearest .brv — if it's a file, follows the pointer; if it's a directory, uses it directly.

Handles the case where brv was already run in the subdirectory (auto-init'd .brv/): --force backs up the existing .brv/ to .brv-backup/ and replaces it with a pointer file. brv worktree remove restores the backup.

On-disk layout

monorepo/
├── .brv/                           ← directory (real project)
│   ├── config.json
│   ├── context-tree/
│   └── worktrees/
│       └── packages-api/
│           └── link.json           ← {"worktreePath": "/abs/path/to/packages/api"}
├── packages/
│   └── api/
│       └── .brv                    ← FILE (pointer): {"projectRoot": "/path/to/monorepo"}

Knowledge sources (brv source)

brv source add /path/to/shared-lib --alias shared
brv source list                     # shows sources with valid/broken status
brv source remove shared

How it works: Adds entries to .brv/sources.json (local project only — nothing written to the target). During brv query, the search service indexes both the local context tree and all configured sources. Results from sources are tagged with origin: 'shared' and the source alias. A write guard blocks any curate/write operations to shared source context trees.

Architecture

Both features use the daemon transport event pattern:

  • Transport events defined in shared/transport/events/ (worktree-events.ts, source-events.ts)
  • Server handlers in server/infra/transport/handlers/ process the events
  • Both oclif CLI and TUI slash commands go through the daemon — no direct server imports

The resolver, CRUD operations, and search integration live server-side. The CLI and TUI are thin clients that send events and display results.

Commands

Command Description
brv worktree add [path] Register a directory as a worktree (from parent with path, or from child to auto-detect)
brv worktree remove [path] Unregister a worktree
brv worktree list Show current link status and all registered worktrees
brv source add <path> [--alias] Add a read-only knowledge source
brv source remove <alias-or-path> Remove a knowledge source
brv source list List all sources with valid/broken status

TUI slash commands mirror the CLI: /worktree add, /source list, etc.

Internal vocabulary

Concept Term Example
CLI commands worktree / source brv worktree add, brv source list
On-disk files .brv file (pointer) / .brv/sources.json {"projectRoot": "/path"}
Resolver source: 'direct' | 'linked' How the project was discovered
Search results origin: 'local' | 'shared' Where an indexed document came from

Test plan

  • Unit tests for resolver (direct, linked, broken, malformed pointers)
  • Unit tests for worktree CRUD (add, remove, list, idempotency, backup/restore)
  • Unit tests for source operations (add, remove, list, circular detection, alias dedup)
  • Integration tests for worktree lifecycle (full add → resolve → remove cycle)
  • Integration tests for workspace-scoped execution and MCP resolution
  • Status display tests (worktree link, knowledge sources, broken hints)
  • Write guard tests (blocks writes to shared sources)
  • Manual UAT: TUI slash commands (26 cases), CLI commands (41 cases)

ngduyanhece and others added 17 commits March 6, 2026 23:21
- Add resolveProject() with priority: flag > direct > linked > walked-up > null
- Add workspace-link schema (.brv-workspace.json) and WORKSPACE_LINK_FILE constant
- Make status handler resolver-aware with projectRoot, workspaceRoot, resolutionSource, shadowedLink, resolverError fields
- Add --verbose flag to brv status for resolution diagnostics
- Make prerun hook resolver-aware; auto-init only when no project found
- Add minimal brv unlink command and TUI /unlink (resolver-bypass)
- Migrate curate/view.ts from findProjectRoot to resolveProject
- TUI status now sends cwd in request
- Surface broken/malformed workspace link errors as actionable status warnings
- Cross-platform path handling (path.sep instead of hardcoded /)
- 18 resolver unit tests + updated prerun hook tests

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…t reassociation

Make all entry points (CLI, TUI, MCP) use the canonical resolveProject() resolver
so tasks from workspace-linked subdirectories route to the correct project.

- CLI: pre-resolve projectPath/workspaceRoot in withDaemonRetry, pass through to
  query and curate commands
- TUI: pass resolved project info from oclif main → startRepl → transport store,
  use it for daemon registration and reconnect (no raw cwd fallback)
- MCP: use resolveProject() for project context resolution with retry-aware
  association, propagate errors instead of swallowing
- TaskRouter: correct fallback order (explicit > fresh cwd resolution > stale
  registration > raw clientCwd), containment validation, short-circuit for
  explicit payloads
- ClientManager: support reassociation via updateProjectPath() so clients can
  move between projects after link/unlink
- ConnectionCoordinator: handle reassociation (not just first association)
- Thread workspaceRoot through transport schemas, task events, and task routing

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…ding (PR3)

Thread workspaceRoot and projectRoot through all three executors so linked
workspaces get correct search scoping, path resolution, and cache isolation.

- Query executor: derive workspace scope for search, inject scope variable
  into sandbox for agent follow-up searches, include workspaceRoot in cache
  fingerprint to prevent cross-workspace cache bleed
- Curate executor: use projectRoot (not workspaceRoot) as base directory for
  post-processing (snapshot, summary propagation, manifest rebuild) since
  .brv/ lives at projectRoot
- Folder-pack executor: resolve relative folderPath from clientCwd (shell
  semantics), default absent folderPath to workspaceRoot, write temp files
  to projectRoot
- Agent-process: thread projectPath as projectRoot to curate/folder-pack,
  thread workspaceRoot to all executors
- Expose scope in SearchKnowledgeOptions (tools-sdk)

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…k slash command (PR4)

Adds `brv link [project-root]` oclif command and `/link` TUI slash command
to create .brv-workspace.json link files, complementing the existing `brv unlink`.

- Auto-detect walks up from cwd looking for nearest .brv/config.json,
  stopping at .git boundary to prevent cross-repo discovery
- Validation guards: shadow (cwd has .brv/), ancestor check, self-link
- Idempotent: succeeds silently when already linked to same target
- TUI /link canonicalizes explicit paths via realpathSync and sends
  resolution.projectRoot (not raw arg) to ASSOCIATE_PROJECT
- Fix /unlink to skip ASSOCIATE_PROJECT when re-resolution fails,
  avoiding registration of non-project directories in daemon rooms
- Export hasBrvConfig, hasWorkspaceLink, isDescendantOf, isGitRoot
  from resolve-project.ts for reuse by link command
- 26 unit tests covering helpers, link flow, git-boundary stop condition

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Add 41 cross-module integration tests verifying workspace linking contracts:
- link-unlink-lifecycle: resolver state transitions with real filesystem
- resolver-to-task-router: workspace threading through task creation
- workspace-scoped-execution: search scoping, cache isolation, path defaults
- mcp-tool-workspace-resolution: per-call resolution and retry logic
- format-status-workspace: workspace fields in status display

Update README with workspace linking documentation. Update command tests
for workspace-aware payload fields.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…, and status display (PR6)

Cross-project knowledge linking allows a project to read another project's context tree
as a read-only source. Adds schema (.brv/knowledge-links.json), multi-source BM25 index
with local score boost, source-qualified document IDs, namespaced symbol tree ([alias]:path),
three-layer write guard (curate executor, write_file tool, tools.writeFile sandbox), cache
fingerprint isolation, and knowledge link status in CLI/TUI output.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…rmatting (PR6)

Adds tests for knowledge-links.json loading/validation, multi-source search
index building, and knowledge link status display formatting.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…s (PR7)

Add oclif commands (link-knowledge, unlink-knowledge, list-knowledge-links)
and TUI slash commands for cross-project knowledge linking. Surface knowledge
link status in brv status with validity and file count display. Fix empty
alias validation, malformed file visibility in status/list, and CLI path
normalization consistency.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…n broken links

- StatusHandler now returns the actual client working directory instead of
  always returning the resolved project root (backward compat fix)
- Query fingerprint cache validates knowledge link source keys on each hit,
  so deleting a linked project within the 30s TTL window correctly
  invalidates the cache
- Added KNOWN LIMITATION doc for workspace-scoped query matching

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Conflicts:
	src/oclif/commands/status.ts
	src/shared/transport/types/dto.ts
	src/tui/features/commands/definitions/index.ts
	test/unit/infra/transport/handlers/status-handler.test.ts
# Conflicts:
#	src/oclif/commands/status.ts
#	src/oclif/hooks/prerun/validate-brv-config-version.ts
#	src/oclif/lib/daemon-client.ts
#	src/server/infra/transport/handlers/status-handler.ts
#	src/server/infra/transport/transport-connector.ts
#	src/shared/transport/types/dto.ts
#	src/tui/features/commands/definitions/index.ts
#	test/hooks/prerun/validate-brv-config-version.test.ts
#	test/unit/infra/transport/handlers/status-handler.test.ts
… full vocabulary alignment

Rename user-facing commands, on-disk file formats, and internal symbols
to eliminate the confusing link vs link-knowledge naming collision.

Commands:
- brv link → brv worktree add/remove/list
- brv link-knowledge → brv source add/remove/list
- TUI /link, /unlink → /worktree add/remove/list
- TUI /link-knowledge, /unlink-knowledge → /source add/remove/list

On-disk files:
- .brv-workspace.json → .brv-worktree.json
- .brv/knowledge-links.json → .brv/sources.json (top-level key: sources)

Internal vocabulary:
- WorkspaceLink → WorktreeLink, workspaceRoot → worktreeRoot
- KnowledgeLink → Source, KnowledgeSource → SearchOrigin
- sourceType: 'linked' → origin: 'shared' (document origin in search)
- BrokenWorkspaceLinkError → BrokenWorktreeLinkError
- knowledgeLinks DTO field → sources

New:
- brv worktree list / /worktree list (inspect current link state)
- /source add --alias flag (closes CLI/TUI parity gap)
- Vocabulary glossary in README
Replace .brv-worktree.json pointer files with a git-style model where
.brv can be either a directory (real project) or a file (pointer to
parent project), matching how git uses .git/ vs .git file for worktrees.

Resolver:
- Walks up from cwd to find nearest .brv (file or directory)
- .brv directory → source: 'direct' (real project)
- .brv file → reads {projectRoot} pointer → source: 'linked'
- Stops at first .brv found, no skip-past behavior

Worktree CRUD (addWorktree, removeWorktree, listWorktrees):
- Parent stores registry in .brv/worktrees/<name>/link.json
- brv worktree add: dual-mode (from parent with path, or from child with auto-detect)
- --force flag backs up existing .brv/ to .brv-backup/ when converting to pointer
- Supports children and siblings, relative and absolute paths

Removed:
- .brv-worktree.json file format
- shadowedLink field (impossible in new model — .brv is one thing or the other)
- worktree-link-schema.ts (replaced by worktrees-schema.ts)
- WORKTREE_LINK_FILE constant

Fixes the UX bug where brv auto-creates .brv/ on first run, blocking
subsequent brv worktree add. Now brv worktree add --force handles this
by backing up and replacing the existing .brv/ with a pointer file.
The oclif init hook tried to mkdir .brv/ when running from a worktree
directory, but .brv was already a file (pointer) causing EEXIST error.
Now the hook checks isWorktreePointer() and skips auto-init entirely
when cwd is a worktree — the project lives at the parent.

Also update worktree index.ts description to remove stale
.brv-worktree.json reference, and update README accordingly.
Replace direct server imports in both oclif and TUI worktree/source
commands with transport event calls through the daemon, matching the
existing pattern used by status, curate, and query commands.

Transport layer:
- Add worktree-events.ts (worktree:add, worktree:remove, worktree:list)
- Add source-events.ts (source:add, source:remove, source:list)
- Add WorktreeHandler + SourceHandler server-side event handlers
- Register both in feature-handlers.ts

Oclif commands:
- Rewrite worktree/{add,remove,list}.ts to use withDaemonRetry
- Rewrite source/{add,remove,list}.ts to use withDaemonRetry
- Zero server/ imports remaining in any command file

TUI commands:
- Add worktree-api.ts + source-api.ts (apiClient.request wrappers)
- Rewrite all 6 slash commands to use API layer
- Zero server/ imports — respects tui/ import boundary
@wzlng wzlng requested a review from bao-byterover April 9, 2026 14:20
@github-actions
Copy link
Copy Markdown
Contributor

github-actions bot commented Apr 9, 2026

Claude finished @wzlng's task in 3m 37s —— View job


PR Review: Multi-Repo Knowledge Sharing

Overall score: 4/5 — Well-designed, well-tested PR with clean architecture. A few correctness bugs and minor issues worth addressing before merge.


Summary

The git-style .brv file/directory resolver model is elegant and the transport event pattern is applied consistently. Tests are comprehensive, covering integration paths and edge cases (broken pointers, backup/restore, circular source detection). The write guard is a good defensive addition.


Issues Found

bug: removeWorktree handler falls back to daemon's process.cwd()

File: src/server/infra/transport/handlers/worktree-handler.ts:57

async (data) => {
  const targetPath = data?.worktreePath ?? process.cwd()  // ← daemon's cwd
  const result = removeWorktree(targetPath)

When the client calls brv worktree remove without a path argument, the oclif command sends worktreePath: cwd (the client's cwd), but if data.worktreePath is somehow undefined the handler falls back to the daemon's process working directory, not the client's. This silently operates on the wrong path. Either require worktreePath explicitly in the event type (remove ? from WorktreeRemoveRequest.worktreePath) or throw if it's absent.

Fix this →


bug: sanitizeWorktreeName collision — different paths map to the same registry entry

File: src/server/infra/project/resolve-project.ts:279

const name = worktreePath.startsWith(projectRoot + sep) ...
  ? worktreePath.slice(projectRoot.length + 1)
  : basename(worktreePath);
return name.replaceAll(/[/\\]/g, '-').replaceAll(/[^a-zA-Z0-9._-]/g, '-')

packages/api and packages-api both sanitize to packages-api. When a second worktree is added with a colliding name, it silently overwrites the first registry entry in .brv/worktrees/. Consequently, removeWorktree of the first worktree would find no entry and skip registry cleanup, leaving the second entry orphaned. Add a counter suffix (packages-api-2) when a collision is detected, similar to ensureUniqueAlias.

Fix this →


issue: Write guard is skipped when environmentContext is not provided

File: src/agent/infra/tools/implementations/write-file-tool.ts:54

if (environmentContext?.workingDirectory) {
  const writeError = validateWriteTarget(sanitizedPath, environmentContext.workingDirectory)

The guard only fires when environmentContext?.workingDirectory is set. If a tool instance is created without environmentContext (e.g. in tests or older call sites), an agent could write to shared source context trees with no protection. The guard should be a hard requirement when sources are configured — at minimum, fail fast if workingDirectory is absent and the path resolves into a shared tree. Consider making environmentContext required in createWriteFileTool.


issue: curate-service.ts fabricates a misleading 'ADD' type in the write-guard error

File: src/agent/infra/sandbox/curate-service.ts:122-132

return {
  applied: [{
    message: writeError,
    path: rawBasePath,
    status: 'failed' as const,
    type: 'ADD' as const,   // ← misleading — no operation has been attempted
  }],
  ...
}

The error response hardcodes type: 'ADD' even when the blocked operation could be an UPDATE, DELETE, etc. This makes the error log confusing. A generic error sentinel type, or simply not including type in the early-exit case, would be cleaner.


issue: isDescendantOf is duplicated in two files

File 1: src/server/infra/project/resolve-project.ts:85
File 2: src/server/infra/process/task-router.ts:87

The implementations differ slightly (one uses sep, one uses resolve() first). Extract to a shared utility in src/server/utils/.

Fix this →


suggestion: resolveProject flag path returns null silently for missing project

File: src/server/infra/project/resolve-project.ts:167-177

if (options?.projectRootFlag) {
  const flagRoot = resolve(options.projectRootFlag)
  if (!hasBrvConfig(flagRoot)) {
    return null  // silently ignored
  }

When --project-root is explicitly provided but invalid, returning null means the caller falls back to cwd-based resolution — the explicit flag is silently disregarded. Throwing (or returning a typed error result) would surface the misconfiguration immediately rather than quietly running with a different project.


suggestion: computeSourceValidityHash runs synchronous I/O on every query cache check

File: src/server/infra/executor/query-executor.ts:355, 438-444

&& this.cachedFingerprint.sourceValidityHash === this.computeSourceValidityHash()

computeSourceValidityHash calls loadSources which reads and parses sources.json synchronously. This runs on every query before the fingerprint TTL is applied. For most users this is negligible, but it's synchronous I/O on an async hot path. Consider caching the validity hash separately with its own short TTL.


suggestion: deriveWorkspaceScope is documented to rarely work

File: src/server/infra/executor/query-executor.ts:453-464

KNOWN LIMITATION: ... scope filtering typically has 0 matches and
falls through to unscoped search.

The entire workspace-scope-for-query feature appears to be a no-op in practice for most setups. If the intent is to ship this as a future hook and not current functionality, consider removing the scopeVar injection from the prompt (which adds noise) until the underlying context-tree tagging is implemented.


nitpick: Transitive circular source detection not implemented

File: src/server/core/domain/source/source-operations.ts:200-213

The detectCircularSource comment acknowledges "no transitive check in v1". Worth leaving a TODO with an issue reference so this doesn't get forgotten. A 3-hop cycle (A→B→C→A) could still create confusing search results.


What's Well Done

  • Resolver design — The git-style walk-up in resolveProject with the stop-at-first-brv invariant is clean and correct. The null/undefined return distinction in resolveAtDir is a nice signal separation.
  • Write guard — Applied at both write-file-tool and curate-service levels; the validateWriteTarget logic is clear and covers the symlink case via realpathSync.
  • Source schemaSourcesFileSchema with Zod validation, mtime tracking, and the broken-source separation between sources and origins is well-thought-out.
  • Test coverage — Integration tests for the full lifecycle, real filesystem (no mocks), and edge cases (malformed pointers, backup/restore, circular detection) are thorough.
  • Transport pattern consistency — Worktree and source handlers follow the exact same event/handler pattern as the rest of the codebase.

- Remove daemon process.cwd() fallback in worktree remove handler;
  make worktreePath required in WorktreeRemoveRequest
- Fix sanitizeWorktreeName collisions (packages/api vs packages-api)
  by appending numeric suffix when registry entry already exists
- Extract duplicated isDescendantOf to path-utils.ts (task-router
  and resolve-project had divergent implementations)
- Throw on invalid --project-root flag instead of silently falling
  back to cwd-based resolution
@github-actions
Copy link
Copy Markdown
Contributor

github-actions bot commented Apr 9, 2026

Claude finished @wzlng's task in 3m 3s —— View job


PR Review: Multi-Repo Knowledge Sharing

Overall score: 4/5 — Four of the six issues from the previous review are fixed. Two remain, plus one new finding on the write guard. The core implementation is solid and the architecture is clean.


Previous Review — Status

# Issue Status
1 removeWorktree falls back to daemon's process.cwd() ✅ Fixed — worktreePath is now required in WorktreeRemoveRequest; oclif command always sends resolved path
2 sanitizeWorktreeName collision ✅ Fixed — resolve-project.ts:277-301 now checks existing entries and appends numeric counter suffix
3 resolveProject flag path returns null silently ✅ Fixed — resolve-project.ts:157-170 now throws Error when --project-root flag points to a non-project
4 isDescendantOf duplicated in two files ✅ Fixed — extracted to path-utils.ts:112, task-router.ts:45 imports from there
5 Write guard skipped when environmentContext absent ⚠️ Still open — see below
6 curate-service.ts fabricates misleading 'ADD' type ⚠️ Still open — see below

Suggestions (7–9) acknowledged in code via comments; no changes required before merge.


Remaining Issues

issue: Write guard conditional on optional environmentContext

File: src/agent/infra/tools/implementations/write-file-tool.ts:54

if (environmentContext?.workingDirectory) {
  const writeError = validateWriteTarget(sanitizedPath, environmentContext.workingDirectory)

createWriteFileTool is called without environmentContext in several paths (tests, older call sites). When absent, the guard is silently skipped and an agent can write to a shared source's context tree with no protection. The previous review called for making environmentContext required or failing fast if workingDirectory is absent — this hasn't been addressed.

Minimum fix: throw if environmentContext is absent in tool create, or at least log a warning in development mode.

Fix this →


issue: curate-service.ts still fabricates 'ADD' type in write-guard error

File: src/agent/infra/sandbox/curate-service.ts:124-131

return {
  applied: [{
    message: writeError,
    path: rawBasePath,
    status: 'failed' as const,
    type: 'ADD' as const,   // ← hardcoded, misleading
  }],
  ...
}

The guard fires before any operation is attempted, so the type should be a neutral sentinel — 'BLOCKED' or a generic error type — rather than 'ADD'. This misleads callers/logs into thinking an ADD was attempted and failed, when no operation was attempted at all.

Fix this →


New Finding

nitpick: isWithin in write-guard.ts uses hardcoded '/' separator

File: src/agent/infra/tools/write-guard.ts:15-16

const isWithin = (candidatePath: string, rootPath: string): boolean =>
  candidatePath === rootPath || candidatePath.startsWith(rootPath + '/')

The canonical isDescendantOf in path-utils.ts:112 uses sep for cross-platform correctness. This implementation uses a hardcoded /. The project likely targets POSIX-only (and paths are canonicalized via realpathSync), but it's inconsistent with the rest of the path utilities in this PR. At minimum, import and reuse isDescendantOf from path-utils.ts to stay consistent.


Remaining Acknowledged Items (no change required)

  • computeSourceValidityHash — Reads sources.json synchronously on every cache TTL check. The self-documenting comment (Cost: one readFileSync + existsSync per source — sub-millisecond) is acceptable for now.
  • deriveWorkspaceScope — Known limitation documented inline. The dead-code concern is noted.
  • Transitive circular source detection"no transitive check in v1" comment present; a // TODO with an issue reference would prevent this from being forgotten, but not blocking.

What's Well Done

  • All four bug fixes from the previous review are clean and correct — the sanitize-name collision fix mirrors the ensureUniqueAlias pattern exactly as suggested.
  • resolveProject now throws immediately on a bad --project-root flag, surfacing misconfiguration instead of silently falling back.
  • WorktreeRemoveRequest.worktreePath is now required at the type level, and the oclif command always resolves and sends the path — defense in depth.
  • The isDescendantOf consolidation is clean; task-router.ts now imports from path-utils.ts.

…le paths

On macOS, realpathSync throws for non-existent files, falling back to
resolve() which preserves symlink prefixes (e.g., /tmp/ vs /private/tmp/).
Now canonicalizes the parent directory when the full path doesn't exist,
ensuring consistent comparison with the context tree root.
@github-actions
Copy link
Copy Markdown
Contributor

github-actions bot commented Apr 10, 2026

Claude finished @wzlng's task in 4m 41s —— View job


PR Review: Multi-Repo Knowledge Sharing

  • Gather context and read changed files
  • Review remaining open issues from previous reviews
  • Review new code for correctness, security, and quality
  • Post final review summary

Overall score: 4/5 — Two issues from the previous review remain unfixed. The latest commit (b51e94f) correctly addresses the macOS symlink canonicalization edge case in the write guard, and the hardening commit (37d1380) fixed the four bugs called out in review #1. Core architecture and test coverage remain strong.


Previous Review — Status Update

# Issue Status
1 removeWorktree falls back to daemon's process.cwd() ✅ Fixed
2 sanitizeWorktreeName collision ✅ Fixed
3 resolveProject flag path returns null silently ✅ Fixed
4 isDescendantOf duplicated in two files ✅ Fixed
5 Write guard skipped when environmentContext absent ⚠️ Still open
6 curate-service.ts fabricates misleading 'ADD' type ⚠️ Still open
7 isWithin uses hardcoded '/' separator ⚠️ Still open (nitpick)

Remaining Issues

issue: Write guard conditional on optional environmentContext

File: src/agent/infra/tools/implementations/write-file-tool.ts:46,54

export function createWriteFileTool(fileSystemService: IFileSystem, environmentContext?: EnvironmentContext): Tool {
  // ...
  if (environmentContext?.workingDirectory) {          // ← guard skipped entirely if absent
    const writeError = validateWriteTarget(sanitizedPath, environmentContext.workingDirectory)

environmentContext is still optional, and the guard is still conditional. Any call site that omits environmentContext silently bypasses the write protection — an agent can write directly to a shared source's context tree with no error. The curate-service guard fires independently, but write-file-tool is the direct write path used in code-exec sandboxes.

Minimum fix: make environmentContext required in createWriteFileTool, or throw a hard error if workingDirectory is absent when the tool executes.

Fix this →


issue: curate-service.ts still fabricates 'ADD' type in write-guard error

File: src/agent/infra/sandbox/curate-service.ts:122-132

const writeError = validateWriteTarget(basePath, this.workingDirectory)
if (writeError) {
  return {
    applied: [{
      message: writeError,
      path: rawBasePath,
      status: 'failed' as const,
      type: 'ADD' as const,   // ← hardcoded regardless of actual operation type
    }],
    summary: {added: 0, deleted: 0, failed: 1, merged: 0, updated: 0},
  }
}

The guard fires before any operation is attempted, yet the error response hardcodes type: 'ADD'. A DELETE or MERGE operation blocked here would be reported as a failed ADD, misleading callers and log output. CurateOperationType = 'ADD' | 'DELETE' | 'MERGE' | 'UPDATE' | 'UPSERT' has no neutral sentinel — either add 'BLOCKED' to the union, or pass the actual operation type through to the guard return path.

Fix this →


nitpick: isWithin in write-guard.ts still uses hardcoded '/'

File: src/agent/infra/tools/write-guard.ts:21-22

const isWithin = (candidatePath: string, rootPath: string): boolean =>
  candidatePath === rootPath || candidatePath.startsWith(rootPath + '/')

Both inputs are canonicalized via realpathSync before reaching isWithin, so on POSIX the hardcoded / works. But isDescendantOf in path-utils.ts:115 uses sep for cross-platform correctness and is already imported in sibling files. Reusing it here removes the inconsistency.


New Finding

nitpick: Write-guard test fixture writes malformed sources.json

File: test/unit/agent/tools/write-guard.test.ts:15

writeFileSync(
  join(projectRoot, '.brv', 'sources.json'),
  JSON.stringify({origins: [], version: '1'})  // ← wrong shape: 'origins' vs 'sources', version string vs number
)

SourcesFileSchema expects {sources: z.array(SourceSchema), version: z.literal(1)}. The fixture uses origins (instead of sources) and a string version ('1' instead of 1). loadSources falls back to {mtime, origins: [], sources: []} when schema validation fails, so the tests still pass — but the comment "so loadSources doesn't fail" is incorrect (it handles missing files gracefully too, returning null). This is a silent correctness risk if the fallback behavior changes.

Fix: JSON.stringify({sources: [], version: 1}) — or omit the file entirely since loadSources returns null for missing files and the guard handles null?.origins ?? [] correctly.


What's Well Done in Latest Commits

  • canonicalize helper (write-guard.ts:7-19) — The parent-directory fallback for non-existent files correctly handles the macOS /tmp → /private/tmp symlink case. The platform-guarded test in write-guard.test.ts:48-68 is the right way to cover this edge case without failing on Linux.
  • WorktreeRemoveRequest.worktreePath is now required at the type level in worktree-events.ts:19, and the oclif command always resolves the path before sending — defense in depth is correct.
  • sanitizeWorktreeName collision fix in resolve-project.ts:277-301 mirrors the ensureUniqueAlias pattern exactly, consistent with the codebase.

…ite-guard

Use the cross-platform isDescendantOf from path-utils instead of an
inline helper that hardcodes '/' as the path separator.
@github-actions
Copy link
Copy Markdown
Contributor

github-actions bot commented Apr 10, 2026

Claude finished @wzlng's task in 1m 51s —— View job


PR Review: Multi-Repo Knowledge Sharing

  • Gather context and read changed files
  • Review remaining open issues from previous reviews
  • Check new commits for fixes
  • Review new code for correctness, security, and quality
  • Post final review summary

Overall score: 4/5 — Good progress: the isWithin hardcoded / nitpick is now fixed in cb0f9f5. Two issues from the previous review remain unaddressed.


Previous Review — Status Update

# Issue Status
5 Write guard skipped when environmentContext absent ⚠️ Still open
6 curate-service.ts fabricates misleading 'ADD' type ⚠️ Still open
7 isWithin uses hardcoded '/' separator ✅ Fixed in cb0f9f5 — now uses isDescendantOf from path-utils.ts
New Write-guard test fixture uses wrong version type ⚠️ Partially fixed — originssources corrected, but version: '1' (string) should be version: 1 (number)

Remaining Issues

issue: Write guard conditional on optional environmentContext

File: src/agent/infra/tools/implementations/write-file-tool.ts:46,54

export function createWriteFileTool(fileSystemService: IFileSystem, environmentContext?: EnvironmentContext): Tool {
  // ...
  if (environmentContext?.workingDirectory) {          // ← guard skipped entirely if absent
    const writeError = validateWriteTarget(sanitizedPath, environmentContext.workingDirectory)

environmentContext is still optional, and the guard is still conditional. Any call site that omits environmentContext silently bypasses the write protection — an agent can write directly to a shared source's context tree. This has persisted across three review cycles.

Fix this →


issue: curate-service.ts still fabricates 'ADD' type in write-guard error

File: src/agent/infra/sandbox/curate-service.ts:122-132

const writeError = validateWriteTarget(basePath, this.workingDirectory)
if (writeError) {
  return {
    applied: [{
      message: writeError,
      path: rawBasePath,
      status: 'failed' as const,
      type: 'ADD' as const,   // ← hardcoded regardless of actual operation type
    }],
    ...
  }
}

The guard fires before any operation is attempted. A DELETE or MERGE blocked here is still reported as a failed ADD. CurateOperationType = 'ADD' | 'DELETE' | 'MERGE' | 'UPDATE' | 'UPSERT' in i-curate-service.ts:120 still has no neutral sentinel type. Add 'BLOCKED' to the union and use it here.

Fix this →


nitpick: Write-guard test fixture still uses string version '1'

File: test/unit/agent/tools/write-guard.test.ts:15

writeFileSync(
  join(projectRoot, '.brv', 'sources.json'),
  JSON.stringify({sources: [], version: '1'})   // ← version should be number 1, not string '1'
)

SourcesFileSchema expects z.literal(1) (number). The originssources fix from the previous review was applied correctly, but the version type was not corrected. Since SourcesFileSchema.safeParse fails on version: '1' (a string), loadSources silently returns {mtime, origins: [], sources: []} instead of validating the file properly — the guard still works by accident.

Fix: JSON.stringify({sources: [], version: 1}). Or omit the file entirely, since loadSources returns null for missing files and the guard handles null?.origins ?? [] correctly.

Fix this →


What's Well Done in Latest Commit

  • isDescendantOf consolidation (cb0f9f5) — write-guard.ts now imports isDescendantOf from path-utils.ts instead of the old hardcoded-'/' isWithin. Both the canonicalize helper and isDescendantOf use resolve() for normalization, so the guard is consistent with all other path utilities in this PR.
  • The canonicalize helper for non-existent file paths (parent-directory fallback) remains correct from b51e94f.

@github-actions
Copy link
Copy Markdown
Contributor

github-actions bot commented Apr 10, 2026

Claude finished @wzlng's task in 4m 24s —— View job


PR Review: Multi-Repo Knowledge Sharing

  • Gather context and read changed files
  • Review remaining open issues from previous reviews
  • Check new commits for fixes
  • Implement remaining fixes
  • Post final review summary

Overall score: 4.5/5 — Three remaining issues from previous review cycles have been implemented as fixes in this review pass. The latest commit (9c264d3) was only a merge from main (CLAUDE.md update). Core architecture and test coverage remain strong.


Previous Review — Status Update

# Issue Status
1 removeWorktree falls back to daemon's process.cwd() ✅ Fixed
2 sanitizeWorktreeName collision ✅ Fixed
3 resolveProject flag path returns null silently ✅ Fixed
4 isDescendantOf duplicated in two files ✅ Fixed
7 isWithin uses hardcoded '/' separator ✅ Fixed in cb0f9f5
5 Write guard skipped when environmentContext absent Fixed in this pass
6 curate-service.ts fabricates misleading 'ADD' type Fixed in this pass
New Write-guard test fixture uses version: '1' (string) Fixed in this pass

Fixes Applied (commit b02184c)

createWriteFileToolenvironmentContext now explicitly required-or-undefined

File: src/agent/infra/tools/implementations/write-file-tool.ts:46

// Before — silently omittable, guard could be bypassed without any call-site signal:
export function createWriteFileTool(fileSystemService: IFileSystem, environmentContext?: EnvironmentContext): Tool

// After — call sites must explicitly declare intent:
export function createWriteFileTool(fileSystemService: IFileSystem, environmentContext: EnvironmentContext | undefined): Tool

Five test call sites updated to pass undefined explicitly, making the "no guard" choice visible in code. The tool registry at tool-registry.ts:273 already passes services.environmentContext (which can be undefined) and requires no change.

curate-service.ts write-guard return — 'ADD' replaced with 'BLOCKED'

File: src/agent/infra/sandbox/curate-service.ts:128
File: src/agent/core/interfaces/i-curate-service.ts:120

// Before:
export type CurateOperationType = 'ADD' | 'DELETE' | 'MERGE' | 'UPDATE' | 'UPSERT'
// type: 'ADD' as const  ← misleads callers when a DELETE/MERGE is blocked

// After:
export type CurateOperationType = 'ADD' | 'BLOCKED' | 'DELETE' | 'MERGE' | 'UPDATE' | 'UPSERT'
// type: 'BLOCKED' as const  ← neutral sentinel, no operation was attempted

Write-guard test fixture — version: '1' (string) → version: 1 (number)

File: test/unit/agent/tools/write-guard.test.ts:15

// Before — fails SourcesFileSchema z.literal(1) validation silently:
JSON.stringify({sources: [], version: '1'})

// After — matches schema exactly:
JSON.stringify({sources: [], version: 1})

Note: Push failed due to 403 Permission denied for github-actions[bot] on this repo. The commit b02184c is local — the author will need to push it manually or grant the bot write access.


What's Well Done

  • All four bug fixes from review Feat/init #1 are clean and correct.
  • resolveProject now throws immediately on a bad --project-root flag.
  • WorktreeRemoveRequest.worktreePath is required at the type level.
  • isDescendantOf consolidation is clean — write-guard.ts imports from path-utils.ts.
  • The canonicalize helper for non-existent file paths correctly handles the macOS /tmp → /private/tmp symlink case.
  • Integration tests use real filesystem (no mocks) covering the full worktree lifecycle.
  • Transport pattern consistency — worktree and source handlers follow the same event/handler pattern.

@bao-byterover bao-byterover merged commit 9b847d8 into main Apr 11, 2026
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants