Skip to content

feat: add Sapling (sl) VCS backend support#291

Open
zzl0 wants to merge 1 commit into
modem-dev:mainfrom
zzl0:main
Open

feat: add Sapling (sl) VCS backend support#291
zzl0 wants to merge 1 commit into
modem-dev:mainfrom
zzl0:main

Conversation

@zzl0
Copy link
Copy Markdown

@zzl0 zzl0 commented May 11, 2026

Sapling joins git and jj as a supported VCS mode. Set vcs = "sl" in config to use hunk diff and hunk show with Sapling revsets.

Manual Tests

Step 1: Create a Sapling repo:

sl clone https://github.com/modem-dev/hunk.git

Step 2: Run hunk show

bun run ~/ws/hunk/src/main.tsx -- show

Step 3: Run hun diff

# change a few files
bun run ~/ws/hunk/src/main.tsx -- diff

Then ran agents to add comments

@benvinegar
Copy link
Copy Markdown
Member

@zzl0 Hey, would you mind rebasing? Will need some attention. Between this and other VCS-related PRs, I thought it prudent to do #313 first. Feedback welcome.

@zzl0
Copy link
Copy Markdown
Author

zzl0 commented May 14, 2026

@benvinegar thanks. Yeah, I will update and rebase it. Currently, it's still in draft status and not ready for review yet.

@zzl0 zzl0 force-pushed the main branch 2 times, most recently from 4990b44 to 86be867 Compare May 15, 2026 05:10
@zzl0 zzl0 marked this pull request as ready for review May 15, 2026 05:10
@greptile-apps
Copy link
Copy Markdown
Contributor

greptile-apps Bot commented May 15, 2026

Greptile Summary

This PR adds Sapling (sl) as a third supported VCS backend alongside Git and Jujutsu. Users can opt in via vcs = "sl" in config to use hunk diff and hunk show with Sapling revsets.

  • src/core/sl.ts + src/core/vcs/sl.ts: New command runner and adapter implementing working-tree diff, revision-show, untracked-file synthesis, and watch-signature computation for Sapling; staged and stash-show operations are explicitly rejected with user-friendly errors.
  • src/core/loaders.ts: buildUntrackedDiffFile gets an sl-specific branch that reads file content directly (with a binary heuristic guard) and synthesizes a two-file patch, since Sapling has no equivalent of git diff --no-index.
  • CI workflows: Both ci.yml and pr-ci.yml gain an Install Sapling step that downloads a pinned release tarball and symlinks the sl binary into PATH for integration tests.

Confidence Score: 3/5

The Sapling adapter is functionally complete and well-structured, but the error-classification logic in sl.ts can route non-revset failures through the wrong user-facing message, and the CI tarball installation runs as root without integrity verification.

The "not found" fragment in isInvalidRevsetMessage will silently reclassify errors like file not found or remote not found as invalid-revset failures, showing users a misleading could not resolve Sapling revset message when the real problem is something else. This affects the core error-reporting path for every Sapling command that fails for a non-revset reason.

src/core/sl.ts (error classification logic) and both CI workflow files (tarball integrity)

Security Review

  • CI supply-chain risk (.github/workflows/ci.yml, .github/workflows/pr-ci.yml): The Sapling tarball is downloaded with curl -fsSL \u2026 | sudo tar -xJ and extracted with root privileges without any SHA-256 or checksum verification. A compromised or swapped release asset would silently inject binaries into the runner's PATH.

Important Files Changed

Filename Overview
src/core/sl.ts New Sapling command runner — contains an overly-broad "not found" fragment in isInvalidRevsetMessage that can misclassify non-revset errors
src/core/vcs/sl.ts New VCS adapter for Sapling — detection logic, loadReview, and watchSignature are structurally sound; watchSignature spawns 3 sl processes per tick
src/core/loaders.ts Adds sl-specific untracked-file handling in buildUntrackedDiffFile; binary check and UTF-8 read pattern are consistent with the existing approach
.github/workflows/ci.yml Adds Sapling install step using curl
src/core/types.ts VcsMode union extended with "sl" — minimal, correct change
src/core/vcs/index.ts slAdapter registered between jjAdapter and gitAdapter; ordering is intentional and matches the updated test

Sequence Diagram

sequenceDiagram
    participant User
    participant HunkCLI
    participant slAdapter
    participant sl_ts as sl.ts helpers
    participant Sapling as sl binary

    User->>HunkCLI: hunk diff [range]
    HunkCLI->>slAdapter: loadReview(working-tree-diff)
    slAdapter->>sl_ts: resolveSlRepoRoot()
    sl_ts->>Sapling: sl root
    Sapling-->>sl_ts: /path/to/repo
    sl_ts-->>slAdapter: repoRoot

    slAdapter->>sl_ts: runSlText(buildSlDiffArgs)
    sl_ts->>Sapling: sl diff --git [-r range]
    Sapling-->>sl_ts: patch text
    sl_ts-->>slAdapter: patchText

    slAdapter->>sl_ts: listSlUntrackedFiles()
    sl_ts->>Sapling: sl status --unknown --print0
    Sapling-->>sl_ts: null-delimited paths
    sl_ts-->>slAdapter: untrackedFiles[]

    slAdapter-->>HunkCLI: VcsPatchResult
    HunkCLI->>HunkCLI: buildUntrackedDiffFile (sl branch)
    Note over HunkCLI: isProbablyBinaryFile? skip else readFileSync + createTwoFilesPatch
    HunkCLI-->>User: rendered diff
Loading
Prompt To Fix All With AI
Fix the following 3 code review issues. Work through them one at a time, proposing concise fixes.

---

### Issue 1 of 3
src/core/sl.ts:87-96
The `"not found"` fragment is far too broad. Any Sapling error that contains those two words — e.g., `"file 'foo.txt' not found"`, `"remote repository not found"`, or a network timeout message — will be misclassified as an invalid-revset error and produce a misleading `` `hunk diff X` could not resolve Sapling revset `X` `` message when the real cause is something else entirely. The other fragments here are specific enough; `"not found"` alone needs a more anchored form such as `"revision not found"` or should be removed.

```suggestion
function isInvalidRevsetMessage(stderr: string) {
  return [
    "unknown revision",
    "ambiguous identifier",
    "can't find revision",
    "is not a valid revision",
    "revision not found",
    "parse error",
  ].some((fragment) => stderr.toLowerCase().includes(fragment.toLowerCase()));
}
```

### Issue 2 of 3
.github/workflows/ci.yml:38-44
**No integrity check on the downloaded Sapling tarball**

The tarball is fetched with `curl -fsSL … | sudo tar -xJ` and extracted with root privileges, but there is no SHA-256 or checksum verification. If the GitHub release asset is replaced or the download is tampered with in transit, malicious binaries would be silently extracted to `/opt/sapling` and symlinked into `PATH`. The same step is duplicated in `pr-ci.yml`. Consider adding a `sha256sum --check` step after download and before extraction.

### Issue 3 of 3
src/core/vcs/sl.ts:107-117
**Three `sl` subprocess calls per watch tick**

For `working-tree-diff`, `watchSignature` runs `sl diff``sl root``sl status` in sequence on every polling interval. The `jj` adapter only runs one command per tick. While not a correctness bug, this triple-spawn pattern could make watch mode noticeably sluggish in repositories with many untracked files or on slower machines. Consider caching `repoRoot` across ticks or combining the status check into the diff output if Sapling supports it.

Reviews (1): Last reviewed commit: "feat: add Sapling (sl) VCS backend suppo..." | Re-trigger Greptile

Comment thread src/core/sl.ts
Comment thread .github/workflows/ci.yml
Comment thread src/core/vcs/sl.ts
Comment on lines +107 to +117
throw createSlUnsupportedStashShowError();
}
},

watchSignature(operation, { cwd }) {
switch (operation.kind) {
case "working-tree-diff": {
const input = operation.input;
const trackedPatch = runSlText({ input, args: buildSlDiffArgs(input), cwd });
const repoRoot = resolveSlRepoRoot(input, { cwd });
const untrackedSignatures = listSlUntrackedFiles(input, { cwd, repoRoot }).map(
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Three sl subprocess calls per watch tick

For working-tree-diff, watchSignature runs sl diffsl rootsl status in sequence on every polling interval. The jj adapter only runs one command per tick. While not a correctness bug, this triple-spawn pattern could make watch mode noticeably sluggish in repositories with many untracked files or on slower machines. Consider caching repoRoot across ticks or combining the status check into the diff output if Sapling supports it.

Prompt To Fix With AI
This is a comment left during a code review.
Path: src/core/vcs/sl.ts
Line: 107-117

Comment:
**Three `sl` subprocess calls per watch tick**

For `working-tree-diff`, `watchSignature` runs `sl diff``sl root``sl status` in sequence on every polling interval. The `jj` adapter only runs one command per tick. While not a correctness bug, this triple-spawn pattern could make watch mode noticeably sluggish in repositories with many untracked files or on slower machines. Consider caching `repoRoot` across ticks or combining the status check into the diff output if Sapling supports it.

How can I resolve this? If you propose a fix, please make it concise.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The current behavior matches git side. sl root should be fast, let's keep it as-is for now.

@zzl0 zzl0 force-pushed the main branch 2 times, most recently from 0f94b66 to f2d52a7 Compare May 15, 2026 06:08
Sapling joins git and jj as a supported VCS mode. Set `vcs = "sl"` in
config to use `hunk diff` and `hunk show` with Sapling revsets. Repos
using `.sl` or `.hg` directories are auto-detected for repo-local
config. CI workflows install Sapling for test coverage.
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.

2 participants