feat(vcs): add adapter abstraction#313
Conversation
Greptile SummaryIntroduces a VCS adapter abstraction with a priority-ordered registry (
Confidence Score: 4/5Safe to merge; the core adapter dispatch and capability-checking logic in the load path is solid, and all three adapters are well-tested. The load path correctly guards unsupported operations before ever reaching an adapter. The two noted gaps — findVcsRepoRootCandidate coupling adapter IDs to directory names, and watch.ts skipping the capability check before calling watchSignature — are not reachable through normal app flow but represent inconsistencies that could affect future adapters or tests. No data-loss or security concerns were found. src/core/vcs/index.ts (findVcsRepoRootCandidate detection logic) and src/core/watch.ts (missing capability guard in vcsPatchSignature) Important Files Changed
Prompt To Fix All With AIFix the following 3 code review issues. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 3
src/core/vcs/index.ts:33-47
`findVcsRepoRootCandidate` hard-codes the assumption that each adapter's filesystem marker is `.${adapter.id}` — this implicitly couples the adapter's `id` string to its directory name. Every adapter already exposes its own `detect()` method which knows the correct path to look for. If a future adapter uses an ID that doesn't map 1:1 to its directory, the function would silently fail to detect it as a repo root, causing config files in that root to be missed even though `detectVcs` correctly identifies the VCS.
```suggestion
export function findVcsRepoRootCandidate(cwd = process.cwd()) {
let current = resolve(cwd);
for (;;) {
if (vcsAdapters.some((adapter) => adapter.detect(current) !== null)) {
return current;
}
const parent = dirname(current);
if (parent === current) {
return undefined;
}
current = parent;
}
}
```
### Issue 2 of 3
src/core/watch.ts:24-31
`vcsPatchSignature` doesn't validate adapter capabilities before calling `watchSignature`, unlike `loadVcsChangeset` which explicitly checks `adapter.capabilities.reviewOperations` first. If `stash-show` with a jj or sl adapter somehow reaches `computeWatchSignature`, the adapter's `watchSignature` switch will hit its `stash-show` arm and throw a raw internal `Error` rather than a `HunkUserError`, bypassing user-friendly error handling upstream.
```suggestion
/** Build one exact patch signature for adapter-backed review inputs. */
function vcsPatchSignature(input: Extract<CliInput, { kind: "vcs" | "show" | "stash-show" }>) {
const adapter = getVcsAdapter(input.options.vcs ?? "git");
if (!adapter.watchSignature) {
throw new Error(`${adapter.name} does not support watch signatures.`);
}
const operation = operationFromInput(input);
if (!adapter.capabilities.reviewOperations.has(operation.kind)) {
throw createUnsupportedVcsOperationError(adapter, operation);
}
return adapter.watchSignature(operation, { cwd: process.cwd() });
}
```
### Issue 3 of 3
src/core/vcs/sl.ts:111-112
Both `sl.ts` (here and at `watchSignature` line 127) and `jj.ts` throw a raw `Error` for the `stash-show` arm, but the production load path throws a `HunkUserError` via `createUnsupportedVcsOperationError`. Any caller that directly invokes the adapter gets an internal error without the user-friendly hints. Consider using `HunkUserError` to keep error handling uniform across all adapter entry points.
Reviews (1): Last reviewed commit: "feat(vcs): add adapter abstraction" | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| export function findVcsRepoRootCandidate(cwd = process.cwd()) { | ||
| let current = resolve(cwd); | ||
|
|
||
| for (;;) { | ||
| if (vcsAdapters.some((adapter) => fs.existsSync(join(current, `.${adapter.id}`)))) { | ||
| return current; | ||
| } | ||
|
|
||
| const parent = dirname(current); | ||
| if (parent === current) { | ||
| return undefined; | ||
| } | ||
| current = parent; | ||
| } | ||
| } |
There was a problem hiding this comment.
findVcsRepoRootCandidate hard-codes the assumption that each adapter's filesystem marker is .${adapter.id} — this implicitly couples the adapter's id string to its directory name. Every adapter already exposes its own detect() method which knows the correct path to look for. If a future adapter uses an ID that doesn't map 1:1 to its directory, the function would silently fail to detect it as a repo root, causing config files in that root to be missed even though detectVcs correctly identifies the VCS.
| export function findVcsRepoRootCandidate(cwd = process.cwd()) { | |
| let current = resolve(cwd); | |
| for (;;) { | |
| if (vcsAdapters.some((adapter) => fs.existsSync(join(current, `.${adapter.id}`)))) { | |
| return current; | |
| } | |
| const parent = dirname(current); | |
| if (parent === current) { | |
| return undefined; | |
| } | |
| current = parent; | |
| } | |
| } | |
| export function findVcsRepoRootCandidate(cwd = process.cwd()) { | |
| let current = resolve(cwd); | |
| for (;;) { | |
| if (vcsAdapters.some((adapter) => adapter.detect(current) !== null)) { | |
| return current; | |
| } | |
| const parent = dirname(current); | |
| if (parent === current) { | |
| return undefined; | |
| } | |
| current = parent; | |
| } | |
| } |
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/core/vcs/index.ts
Line: 33-47
Comment:
`findVcsRepoRootCandidate` hard-codes the assumption that each adapter's filesystem marker is `.${adapter.id}` — this implicitly couples the adapter's `id` string to its directory name. Every adapter already exposes its own `detect()` method which knows the correct path to look for. If a future adapter uses an ID that doesn't map 1:1 to its directory, the function would silently fail to detect it as a repo root, causing config files in that root to be missed even though `detectVcs` correctly identifies the VCS.
```suggestion
export function findVcsRepoRootCandidate(cwd = process.cwd()) {
let current = resolve(cwd);
for (;;) {
if (vcsAdapters.some((adapter) => adapter.detect(current) !== null)) {
return current;
}
const parent = dirname(current);
if (parent === current) {
return undefined;
}
current = parent;
}
}
```
How can I resolve this? If you propose a fix, please make it concise.There was a problem hiding this comment.
Good catch. I changed findVcsRepoRootCandidate to use each adapter's detect() result and only return the current directory when the adapter reports that exact directory as the repo root, so repo-root config lookup no longer assumes .${adapter.id}. Added a registry test with a custom marker to cover this.
This comment was generated by Pi using GPT-5
| /** Build one exact patch signature for adapter-backed review inputs. */ | ||
| function vcsPatchSignature(input: Extract<CliInput, { kind: "vcs" | "show" | "stash-show" }>) { | ||
| const adapter = getVcsAdapter(input.options.vcs ?? "git"); | ||
| if (!adapter.watchSignature) { | ||
| throw new Error(`${adapter.name} does not support watch signatures.`); | ||
| } | ||
| return adapter.watchSignature(operationFromInput(input), { cwd: process.cwd() }); | ||
| } |
There was a problem hiding this comment.
vcsPatchSignature doesn't validate adapter capabilities before calling watchSignature, unlike loadVcsChangeset which explicitly checks adapter.capabilities.reviewOperations first. If stash-show with a jj or sl adapter somehow reaches computeWatchSignature, the adapter's watchSignature switch will hit its stash-show arm and throw a raw internal Error rather than a HunkUserError, bypassing user-friendly error handling upstream.
| /** Build one exact patch signature for adapter-backed review inputs. */ | |
| function vcsPatchSignature(input: Extract<CliInput, { kind: "vcs" | "show" | "stash-show" }>) { | |
| const adapter = getVcsAdapter(input.options.vcs ?? "git"); | |
| if (!adapter.watchSignature) { | |
| throw new Error(`${adapter.name} does not support watch signatures.`); | |
| } | |
| return adapter.watchSignature(operationFromInput(input), { cwd: process.cwd() }); | |
| } | |
| /** Build one exact patch signature for adapter-backed review inputs. */ | |
| function vcsPatchSignature(input: Extract<CliInput, { kind: "vcs" | "show" | "stash-show" }>) { | |
| const adapter = getVcsAdapter(input.options.vcs ?? "git"); | |
| if (!adapter.watchSignature) { | |
| throw new Error(`${adapter.name} does not support watch signatures.`); | |
| } | |
| const operation = operationFromInput(input); | |
| if (!adapter.capabilities.reviewOperations.has(operation.kind)) { | |
| throw createUnsupportedVcsOperationError(adapter, operation); | |
| } | |
| return adapter.watchSignature(operation, { cwd: process.cwd() }); | |
| } |
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/core/watch.ts
Line: 24-31
Comment:
`vcsPatchSignature` doesn't validate adapter capabilities before calling `watchSignature`, unlike `loadVcsChangeset` which explicitly checks `adapter.capabilities.reviewOperations` first. If `stash-show` with a jj or sl adapter somehow reaches `computeWatchSignature`, the adapter's `watchSignature` switch will hit its `stash-show` arm and throw a raw internal `Error` rather than a `HunkUserError`, bypassing user-friendly error handling upstream.
```suggestion
/** Build one exact patch signature for adapter-backed review inputs. */
function vcsPatchSignature(input: Extract<CliInput, { kind: "vcs" | "show" | "stash-show" }>) {
const adapter = getVcsAdapter(input.options.vcs ?? "git");
if (!adapter.watchSignature) {
throw new Error(`${adapter.name} does not support watch signatures.`);
}
const operation = operationFromInput(input);
if (!adapter.capabilities.reviewOperations.has(operation.kind)) {
throw createUnsupportedVcsOperationError(adapter, operation);
}
return adapter.watchSignature(operation, { cwd: process.cwd() });
}
```
How can I resolve this? If you propose a fix, please make it concise.There was a problem hiding this comment.
Fixed. vcsPatchSignature now builds the neutral operation once, checks adapter.capabilities.reviewOperations, and throws the same user-facing unsupported-operation error before calling watchSignature. Added watch coverage for stash-show under vcs = "jj".
This comment was generated by Pi using GPT-5
| case "stash-show": | ||
| throw new Error("Sapling adapter does not support stash-show."); |
There was a problem hiding this comment.
sl.ts (here and at watchSignature line 127) and jj.ts throw a raw Error for the stash-show arm, but the production load path throws a HunkUserError via createUnsupportedVcsOperationError. Any caller that directly invokes the adapter gets an internal error without the user-friendly hints. Consider using HunkUserError to keep error handling uniform across all adapter entry points.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/core/vcs/sl.ts
Line: 111-112
Comment:
Both `sl.ts` (here and at `watchSignature` line 127) and `jj.ts` throw a raw `Error` for the `stash-show` arm, but the production load path throws a `HunkUserError` via `createUnsupportedVcsOperationError`. Any caller that directly invokes the adapter gets an internal error without the user-friendly hints. Consider using `HunkUserError` to keep error handling uniform across all adapter entry points.
How can I resolve this? If you propose a fix, please make it concise.There was a problem hiding this comment.
The Sapling adapter was removed from this PR so we do not expose partial Sapling support here. I also updated the Jujutsu adapter's direct stash-show arms to throw the same user-facing Git-required error instead of a raw internal Error.
This comment was generated by Pi using GPT-5
benvinegar
force-pushed
the
feat/vcs-abstract
branch
2 times, most recently
from
42d58c5 to
43557c6
Compare
benvinegar
force-pushed
the
feat/vcs-abstract
branch
from
43557c6 to
7f10606
Compare
1 participant
Summary
Related PRs
sl) VCS backend support.Difference from #291
.sl/.hgdetection, Sapling command/error handling, pathspec behavior, and untracked-file support.src/core/vcs/sl.tsadapter and registered through this abstraction.Tests
bun run typecheckbun testbun test src/core/vcs/git.test.ts src/core/vcs/jj.test.ts src/core/vcs/index.test.ts src/core/config.test.ts src/core/loaders.test.ts src/core/watch.test.tsThis PR description was generated by Pi using GPT-5