feat: [ENG-2167] cross-platform path handling and richer VC error con… by ncnthien · Pull Request #491 · campfirein/byterover-cli

ncnthien · 2026-04-21T08:38:09Z

…text

Project name and display path now recognise backslash-separated Windows paths alongside macOS and Linux home directories, each with a unit-tested suite covering absolute paths, trailing separators, UNC shares, and degenerate inputs. The shared formatError helper accepts an optional ErrorContext so overrides can produce tailored copy — ERR_VC_USER_NOT_CONFIGURED now embeds the project path and tells the user exactly where to run brv vc config. Changes panel threads the selected project through to the three commit-related error sites.

Summary

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

Type of change

Scope (select all touched areas)

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:

…text Project name and display path now recognise backslash-separated Windows paths alongside macOS and Linux home directories, each with a unit-tested suite covering absolute paths, trailing separators, UNC shares, and degenerate inputs. The shared formatError helper accepts an optional ErrorContext so overrides can produce tailored copy — ERR_VC_USER_NOT_CONFIGURED now embeds the project path and tells the user exactly where to run brv vc config. Changes panel threads the selected project through to the three commit-related error sites.

github-actions · 2026-04-21T08:38:27Z

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

PR Review — feat: [ENG-2167] cross-platform path handling and richer VC error context

Score: 4.5 / 5

Summary

A well-scoped, clean PR that improves two utility functions (displayPath, getProjectName) for cross-platform path correctness and enriches the VC commit error flow with contextual guidance. The implementation is correct, the test coverage is thorough, and all changes are backward-compatible.

What's good

display-path.ts: Replacing the imperative startsWith chain with a single compiled regex is both more concise and easier to extend. The regex correctly handles macOS, Linux, and Windows (forward- and backslash variants), and the group-capture approach naturally preserves the caller's separator style in the ~\... output.
project-name.ts: Properly handles the trailing-separator and mixed-separator cases that the original split('/') missed. The UNC share test is a nice edge-case addition.
error-messages.ts: OverrideValue = ((ctx: ErrorContext) => string) | string is an elegant extension — the existing string overrides continue to work unchanged, and the !== undefined guard is a genuine improvement over the old truthiness check.
changes-panel.tsx: projectPath context is correctly scoped to the three commit-related error sites only. Push/pull/abort-merge don't receive it, which is precise (only commits can surface ERR_VC_USER_NOT_CONFIGURED).
TDD compliance: New tests were written for all changed logic, covering degenerate inputs (empty string, root path, UNC shares, dotfile names, unicode) that could easily be missed.

Issues

Severity	Location	Issue
nit	`project-name.ts:4`	`leaf && leaf.length > 0` — redundant length check; `leaf \|\| projectPath` is equivalent and idiomatic
nit	`display-path.ts:4`	Mixed escaping in regex: `/` unescaped inside `[/\\]`, escaped as `\/` outside — pick one style
nit	`error-messages.test.ts:45`	`formatError(error, undefined, {...})` — passing `undefined` explicitly looks accidental; use a real fallback string
suggestion	`display-path.test.ts:53`	Add a test for UNC paths (`\\server\share\folder`) to document that they return unchanged from `displayPath` — the regex correctly rejects them, but there's no test proving this
question	`changes-panel.tsx:38`	`runAction` intentionally omits `context` (stage/unstage can't trigger `ERR_VC_USER_NOT_CONFIGURED`); a short inline comment would prevent a future contributor from adding it "for consistency"

No issues found in

Security: No injection vectors; regex is compiled at module load (no new RegExp(userInput))
TypeScript strictness: ErrorContext uses optional projectPath? correctly; no any, no ! assertions added
Backward compatibility: formatError signature change is additive; all existing 1- and 2-arg callers are unaffected

Gather context and understand the request
Read changed files and diff
Review correctness and logic
Review test coverage and TDD compliance
Review code quality and standards
Post inline comments
Post final review summary

Drop the redundant length check in getProjectName in favour of a straight falsy-fallback, swap undefined fallback arguments in the error-messages tests for a named string so intent is obvious, and add a UNC-share test to pin displayPath's "unchanged when no drive letter" contract.

github-actions · 2026-04-21T08:47:53Z

Claude finished @ncnthien's task in 2m 13s —— View job

PR Review — feat: [ENG-2167] cross-platform path handling and richer VC error context

Score: 4.7 / 5

What this update addressed

The 23b8f5b nit-fixing commit cleanly resolved three items from the previous review pass:

✅ Resolved	Location	Change
Redundant length check	`project-name.ts:4`	`leaf && leaf.length > 0 ? leaf : projectPath` → `leaf \|\| projectPath`
`undefined` fallback in tests	`error-messages.test.ts:43,60`	Replaced implicit `undefined` args with the named string `'fallback copy'`
Missing UNC coverage	`display-path.test.ts:61`	Added `\\\\server\\share\\folder` → unchanged test

Still open

Severity	Location	Issue
nit	`display-path.ts:4`	Mixed escaping style in the regex — see inline comment
suggestion	`changes-panel.tsx:38`	`runAction` deliberately omits `ErrorContext`; a short inline comment (`// context intentionally omitted: only commit paths need ERR_VC_USER_NOT_CONFIGURED context`) would prevent a future contributor from adding it "for consistency" (couldn't post inline — that line isn't in the diff)

No new issues

Correctness: All three error-pass call sites (doCommit, continueMerge, handleStageAllAndCommit) correctly thread {projectPath: selectedProject}. Push/pull/abort remain correctly context-free.
getProjectName degenerate cases: '/', '\\', '///', and '' all fall back to the original string as expected — verified against the implementation.
Security: No regex constructed from user input; no injection vectors introduced.
TypeScript: No new any, no ! assertions, optional projectPath? is used correctly throughout.
Backward compatibility: formatError remains fully backward compatible — all 1-arg and 2-arg callers in the existing codebase are unaffected.
TDD compliance: Tests were written alongside implementation; all new paths (context-aware override, UNC, degenerate inputs) have explicit assertions.

Summary

A solid, focused PR. The nit-fixing commit shows responsiveness to review feedback. The two remaining open items are both cosmetic/preventive — neither blocks merge.

Gather context and understand the request
Read changed files and diff
Review correctness and logic
Review test coverage and TDD compliance
Review code quality and standards
Post inline comments
Post final review summary

github-actions · 2026-04-21T08:53:21Z

Claude finished @ncnthien's task in 2m 18s —— View job

PR Review — feat: [ENG-2167] cross-platform path handling and richer VC error context

Score: 4.8 / 5

What this sync addressed

The latest push is a merge-base update (d46eb67, merging proj/local-web-ui into feat/ENG-2167). No new functional changes were introduced; all 7 changed files remain the same as the previous reviewed commit. Reviewing the complete diff against origin/proj/local-web-ui confirms no regressions.

Closing the regex escaping discussion

The previous two review passes flagged mixed escaping in display-path.ts:4 as a nit. The author replied that changing it would violate the project's lint rules — this is correct. The escaping is context-dependent, not inconsistent:

Outside character classes: / terminates a regex literal and must be escaped as \/ (e.g. \/Users\/, \/home\/).
Inside character classes [/\\]: / does not need escaping, and no-useless-escape rightly prevents [\/\\].

The current pattern is correct as written. The prior nit was based on a misunderstanding of where no-useless-escape applies. No change needed.

Correctness check on `selectedProject` as `ErrorContext`

selectedProject is typed as string (never null) in TransportState, defaulting to ''. When passed as {projectPath: selectedProject}, an empty string is falsy, so the ERR_VC_USER_NOT_CONFIGURED override correctly falls back to the generic copy rather than embedding an empty path. This matches the CLAUDE.md convention of avoiding null and is correct behavior.

Remaining open item

Severity	Location	Issue
suggestion	`changes-panel.tsx:34–40`	`runAction` deliberately omits `ErrorContext` because stage/unstage operations cannot raise `ERR_VC_USER_NOT_CONFIGURED`. A one-line comment would prevent a future contributor from adding context "for consistency". Cannot post this as an inline comment since the line isn't in the diff.

No new issues in this sync

Correctness: All three commit-path error sites (doCommit, continueMerge, handleStageAllAndCommit) correctly thread {projectPath: selectedProject}. Push/pull/abort-merge remain context-free as intended.
Type safety: selectedProject: string is always a string; the falsy-check on projectPath in the override handles the initial '' value correctly.
Security: Regex is compiled once at module load; no user-controlled pattern construction.
Backward compatibility: formatError signature change is fully additive; all existing callers unaffected.
TDD compliance: All new logic paths have explicit test assertions. Degenerate inputs ('', '/', '\\', '///'), UNC shares, and context-aware overrides are each independently verified.

Summary

A clean, focused PR with thorough test coverage and correct cross-platform handling. All blocking items from previous reviews are resolved. The one remaining item is a cosmetic documentation suggestion. The regex escaping is correct and lint-compliant — the previous nit was invalid.

Gather context and understand the request
Read changed files and diff
Review correctness and logic
Review test coverage and TDD compliance
Review code quality and standards
Post inline comments
Post final review summary

ncnthien requested review from bao-byterover and leehpham as code owners April 21, 2026 08:38