Conversation
…heck When deleting a branch that points to the same commit as HEAD, `isAncestor` incorrectly returned false because `isDescendent` uses a strict proper-ancestor test. A commit is trivially an ancestor of itself, so short-circuit to true when OIDs match.
fix: [ENG-1948] treat identical OIDs as ancestor in branch -d merge check
- Intercept --help/-h/help in TUI slash command processor to show TUI-style help instead of falling through to oclif Parser - Fix /vc commit -m stripping multi-word messages to first word by joining remaining argv with the parsed flag value - Apply same fix to oclif vc commit command (strict: false + argv join) - Rewrite server "brv vc" hints to "/vc" in TUI formatTransportError
fix:[ENG-1951] TUI help flow and vc commit message parsing
|
Claude finished @bao-byterover's task in 4m 49s —— View job PR Review — Score: 4/5
Overall AssessmentThis is a well-scoped, well-tested PR. The slug-based URL migration is cleanly designed with robust fallbacks, the Issues FoundMedium — Correctness
Low — Security
Low — Standards / Nitpicks
What's Well Done
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| teamId: data.teamId, | ||
| teamName: data.teamName, | ||
| url: buildCogitRemoteUrl(this.gitApiBaseUrl, data.teamId, data.spaceId), | ||
| url: buildCogitRemoteUrl(this.gitRemoteBaseUrl, data.teamName, data.spaceName), |
There was a problem hiding this comment.
issue (correctness): buildCogitRemoteUrl is called with data.teamName / data.spaceName, but the new URL format requires slugs, not display names. If a team has a display name like "Acme Corp" (with a space or special chars), the constructed URL will be invalid. This path is only reached when the TUI passes teamName/spaceName via the clone request, so in practice these values come from space.teamSlug / space.slug — but the parameter naming creates a semantic mismatch that could regress.
suggestion: Either rename the IVcCloneRequest fields to teamSlug/spaceSlug, or pass data.teamSlug/data.spaceSlug here and fall back to data.teamName/data.spaceName only when slugs are absent. Fix this →
| public async run(): Promise<void> { | ||
| this.log('The init command is not available. Use: brv vc init') | ||
| return | ||
|
|
There was a problem hiding this comment.
nitpick (code quality): Dead code is suppressed with /* eslint-disable no-unreachable */ at the file level, silencing the linter for the entire file. If the intent is to keep the old implementation for future reference, it would be safer to move it to a comment block or a separate archived file. The current approach is fragile — any genuinely unreachable code added later to this file won't be caught.
If the brv init command is being permanently retired (it's now hidden and redirects users), just delete the old body entirely.
| const {message} = flags | ||
| // Support unquoted multi-word messages: brv vc commit -m hello world | ||
| const extra = (argv as string[]).join(' ') | ||
| const message = flags.message |
There was a problem hiding this comment.
nitpick (standards): argv as string[] is a type assertion — the coding standards prohibit as Type casts in favour of type guards or proper typing. In oclif with strict: false, this.parse() already returns argv: string[] in the result type; you likely just need to destructure argv without the cast.
| const message = flags.message | |
| const extra = argv.join(' ') |
| * Format: {baseUrl}/{teamName}/{spaceName}.git | ||
| */ | ||
| export function buildCogitRemoteUrl(baseUrl: string, teamId: string, spaceId: string): string { | ||
| export function buildCogitRemoteUrl(baseUrl: string, teamName: string, spaceName: string): string { |
There was a problem hiding this comment.
nitpick (docs): JSDoc says {teamName}/{spaceName} but after this PR the segments are slugs, not display names. Update to {teamSlug}/{spaceSlug} to keep the contract clear.
| export function buildCogitRemoteUrl(baseUrl: string, teamName: string, spaceName: string): string { | |
| * Format: {baseUrl}/{teamSlug}/{spaceSlug}.git |
| * Parse a .git URL to extract team and space names. | ||
| * Expected path: /{teamName}/{spaceName}.git | ||
| */ | ||
| export function parseUserFacingUrl(url: string): null | {spaceName: string; teamName: string} { |
There was a problem hiding this comment.
nitpick (docs): Same issue — the property names in the return type {teamName, spaceName} are misleading because these values are parsed from URL path segments, which are slugs. The caller (vc-handler.ts) passes them straight to resolveTeamSpaceNames(parsed.teamName, parsed.spaceName) which then matches against t.slug. Consider renaming to {teamSlug, spaceSlug} to make the intent obvious and prevent future misuse.
| @@ -82,6 +88,8 @@ export function VcCloneFlow({onCancel, onComplete, url}: VcCloneFlowProps): Reac | |||
| }, [onComplete, step, cloneUrl, url]) | |||
There was a problem hiding this comment.
nitpick (correctness): cloneMutation is used inside this effect but is absent from the dependency array. React's exhaustive-deps rule would flag this. If cloneMutation reference changes (it's from useExecuteVcClone) between renders while step === 'cloning' and mutatedRef.current is false, the stale mutation object would be invoked. In practice this is unlikely because mutatedRef guards re-entry, but adding cloneMutation to the dep array (or using a ref for it) is the safer pattern.
| @@ -1366,7 +1356,7 @@ export class VcHandler { | |||
| private validateRemoteUrlDomain(url: string): void { | |||
| try { | |||
There was a problem hiding this comment.
suggestion (security): The domain validation only checks host — protocol is not validated. A URL using http://byterover.dev/team/space.git would pass this check and git credentials would be sent over plain HTTP. Consider adding a protocol check:
if (parsed.protocol !== 'https:') {
throw new VcError(
`Invalid remote URL. Use: ${this.gitRemoteBaseUrl}/<team>/<space>.git`,
VcErrorCode.INVALID_REMOTE_URL,
)
}
|
|
||
| return error.message.replace(/ for event '[^']+'(?: after \d+ms)?$/, '') | ||
| // Strip transport suffix and rewrite CLI-style hints (brv vc ...) to TUI slash commands (/vc ...) | ||
| return error.message.replace(/ for event '[^']+'(?: after \d+ms)?$/, '').replaceAll(/\bbrv\s+/g, '/') |
There was a problem hiding this comment.
nitpick (correctness): The regex .replaceAll(/\bbrv\s+/g, '/') rewrites all brv occurrences — including any that appear in user-provided data embedded in an error message (e.g. a remote URL path, a commit message, or a file path that contains the string brv ). This is low-risk in practice but worth noting for future error message changes where user content could be interpolated.
A tighter approach would be to only match the specific prefixes that the server layer is known to emit (brv vc → /vc , brv login → /login, etc.) rather than a blanket word-boundary replace.
cuongdo-byterover
left a comment
cuongdo-byterover
left a comment
There was a problem hiding this comment.
Let's bring this to next release
2 participants
Linked issues
Root cause (bug fixes only, otherwise write
N/A)isDescendentuses a strict proper-ancestor test, so a commit is not considered a descendant of itself. Deleting a branch pointing at the same commit as HEAD failed the merge check.strictmode rejected extra positional args after-m, socommit -m hello worldonly capturedhello. TUI error messages passed server-sidebrv vchints through without rewriting for the slash-command context.Test plan
test/unit/core/domain/entities/space.test.ts(new)test/unit/core/domain/entities/team.test.tstest/unit/infra/git/git-remote-url.test.tstest/unit/infra/space/http-space-service.test.tstest/unit/infra/transport/handlers/vc-handler.test.tstest/unit/tui/utils/error-messages.test.tstest/integration/infra/git/isomorphic-git-service.test.tsbuildCogitRemoteUrlproduces/{team}/{space}.git(no/git/prefix)parseGitPathUrl/stripCredentialsFromUrl/isUuidand their testsbrv vc→/vcrewriting in TUI error messagesUser-visible changes
https://host/git/{uuid}/{uuid}.gittohttps://host/{teamSlug}/{spaceSlug}.gitbrv initnow prints "The init command is not available. Use: brv vc init" and is hidden from helpbrv vc commit -m hello world(unquoted) now correctly captures "hello world" as the full message/vcinstead ofbrv vcdev-beta-app.byterover.devtodev-beta.byterover.devBRV_GIT_API_BASE_URLenv var removedEvidence
Attach at least one:
Checklist
npm test)npm run lint)npm run typecheck)npm run build)mainRisks and mitigations
.git/configwhich will no longer match the new URL format.remote set-urlflow or re-clone. Existing.brv/config.jsonslug fields fall back to name for backward compatibility.slugfields for all teams/spaces.namewhen absent (json.slug ?? json.name), so the system degrades gracefully.BRV_GIT_API_BASE_URLis a breaking env config change.BRV_GIT_REMOTE_BASE_URLonly..env.exampleupdated. Deployments must drop the old var.