feat: opt-in jujutsu (jj) backend for workspace isolation#38
Open
feat: opt-in jujutsu (jj) backend for workspace isolation#38
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughIntroduces a VCS backend abstraction with Git and Jujutsu (jj) implementations, auto-detection, per-session/spec Changes
Sequence Diagram(s)sequenceDiagram
participant CLI as CLI (`ensure_worktree`)
participant Detect as VCS Detector
participant Backend as VcsBackend (Git / Jj)
participant FS as Repository FS / VCS binary
CLI->>Detect: Resolve repo path [, optional vcs-kind]
Detect-->>CLI: VcsKind (arg or auto-detected)
CLI->>Backend: workspace_add(repo_path, dest, session_id, branch, tmp_dir)
Backend->>FS: run `git` or `jj` operations (fetch/default-branch/add)
FS-->>Backend: success / error
Backend-->>CLI: SpecEntry (or error)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Greptile SummaryThis PR introduces a However, two paths that create and restore workspaces at session-start were not updated for jj, leaving functional gaps:
The Confidence Score: 2/5
Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
SS[session-start] --> D{CLAUDE_WORKTREES set?}
D -->|yes| CFE[create_from_env]
D -->|no| CAS[create_auto_sandbox]
CFE -->|git::is_git_repo check| SKIP[⚠️ jj repos silently skipped]
CAS -->|git::is_git_repo check| SKIP
CFE -->|git repo| CSW[create_single_worktree Git]
CAS -->|git repo| CSW
PTU[PreToolUse / permissions.rs] --> VK{sess.vcs_kind}
VK -->|Jj / JjColocated| JJB[JjBackend safety checks]
VK -->|Git| GIT[gitcheck safety checks]
JJB --> WA{worktree_active?}
GIT --> WA
WA -->|false + repo op| WM[WORKTREE_MISSING msg\nwith vcs_kind encoded]
WM --> EW[ensure-worktree repo vcs-kind]
EW --> EWVCS{vcs_kind}
EWVCS -->|Jj/JjColocated| JJW[JjBackend::workspace_add\njj workspace add]
EWVCS -->|Git| GTW[worktree::ensure_for_repo\ngit worktree add]
SE[session-end] --> RW[remove_worktrees]
RW --> ENTRY{entry.vcs_kind}
ENTRY -->|Jj/JjColocated| JJR[JjBackend::workspace_remove\njj workspace forget]
ENTRY -->|Git| GTR[worktree::remove\ngit worktree remove]
RESUME[session-start resume] --> RST[restore_worktrees]
RST -->|git::is_valid_worktree| RSKIP[⚠️ jj workspaces\nalways skipped]
RST -->|git::is_git_repo| RSKIP
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
hooks/src/bin/session_end.rs (1)
74-99:⚠️ Potential issue | 🟠 MajorDirty JJ workspaces still get a Git cleanup command.
Line 96 always suggests
git ... worktree remove, but this loop now removesVcsKind::Jj/VcsKind::JjColocatedentries viaJjBackend.workspace_remove(...). For dirty JJ entries, the warning points users at a cleanup command that cannot remove the leftover workspace. Please branch the changelog/help text onentry.vcs_kindtoo.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@hooks/src/bin/session_end.rs` around lines 74 - 99, The changelog warning always suggests a Git worktree cleanup even for JJ workspaces; update the dirty-branch in the block after workspace removal (where entry.vcs_kind, VcsKind::Jj, VcsKind::JjColocated and JjBackend.workspace_remove are used) to choose the appropriate cleanup/help text based on entry.vcs_kind before calling append_to_changelog (and when formatting the message for sess.changelog_path); i.e., detect JJ vs Git and format the cleanup command and hint accordingly so JJ entries get the correct JJ-specific removal instruction instead of the Git worktree command.
🧹 Nitpick comments (5)
hooks/src/vcs/jj.rs (3)
155-159: Consider whether jj merge conflicts should affect "clean" status.While jj auto-snapshots prevent data loss, a workspace with unresolved merge conflicts might not be truly "clean" for all use cases. The current always-true implementation is valid for the documented purpose but worth noting if callers expect conflict detection.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@hooks/src/vcs/jj.rs` around lines 155 - 159, The current is_clean(&self, _path: &Path) unconditionally returns true; change it to detect unresolved merge conflicts and return false when present so callers that care about conflicts get accurate status. In practice, update is_clean to query the repository/working-copy conflict state (e.g., via the jj API or our existing workspace state helpers) for the given Path and return false if any unresolved conflicts exist, otherwise return true; keep the auto-snapshot rationale in comments and add/update tests and documentation to note that is_clean now considers merge conflicts.
79-79:_forceparameter is ignored.The
workspace_removemethod ignores theforceparameter and always performs the same forget+remove operation. If force semantics differ for jj (e.g., removing even with uncommitted changes), this should be documented or implemented. Otherwise, consider documenting why force is a no-op for jj.📝 Suggested documentation
- fn workspace_remove(&self, entry: &SpecEntry, _force: bool) -> (bool, Option<String>) { - // Run forget from repo root (workspace dir may be in a bad state). + fn workspace_remove(&self, entry: &SpecEntry, _force: bool) -> (bool, Option<String>) { + // jj workspace forget is always safe (jj auto-snapshots changes), so force + // has no special handling. Run from repo root since workspace may be corrupted.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@hooks/src/vcs/jj.rs` at line 79, The workspace_remove function currently ignores the _force parameter and always performs the same forget+remove sequence; update workspace_remove(&self, entry: &SpecEntry, _force: bool) to either implement force semantics for jj (e.g., when force is true, bypass checks and remove even with uncommitted changes) or explicitly document that force is a no-op for jj: modify the function body to branch on the force flag (or rename parameter and add a comment) and adjust behavior accordingly so callers understand whether uncommitted changes will be preserved/checked, referencing workspace_remove and SpecEntry to locate the change.
31-32: Immutable revision detection is limited torootandtrunk.The regex only catches
jj edit rootandjj edit trunk, but other immutable revisions (like@-pointing to immutable commits, or revision IDs of immutable commits) won't be blocked. This may be intentional to avoid false positives, but worth documenting the limitation.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@hooks/src/vcs/jj.rs` around lines 31 - 32, RE_JJ_EDIT_IMMUTABLE currently only matches "root" and "trunk", so it misses other immutable targets like "@-" or explicit revision IDs; update the detection to include those forms (e.g., include "@-" and a hex rev-id pattern such as [0-9a-f]{7,40}) or move this check out of the single regex into a small helper that parses the edit target and calls an is_immutable_revision check; modify the LazyLock/Regex named RE_JJ_EDIT_IMMUTABLE (or add a new helper function) accordingly, add a brief doc comment explaining supported patterns, and adjust/add tests to cover "@-" and hex id cases.hooks/src/session.rs (1)
76-80: Double file read inState::from_id.
spec_file_has_contentreads the file metadata, thenread_spec_filereads and parses the entire file again. Consider reading the spec file once and deriving bothworktree_activeandvcs_kindfrom the parsed entries.♻️ Suggested optimization
pub fn from_id(session_id: &str) -> Self { - let worktree_active = spec_file_has_content(&config::spec_file_path(session_id)); - let vcs_kind = match read_spec_file(&config::spec_file_path(session_id)) { - Ok(entries) if !entries.is_empty() => entries[0].vcs_kind, - _ => VcsKind::Git, + let spec_path = config::spec_file_path(session_id); + let (worktree_active, vcs_kind) = match read_spec_file(&spec_path) { + Ok(entries) if !entries.is_empty() => (true, entries[0].vcs_kind), + Ok(_) => (false, VcsKind::Git), + Err(_) => (false, VcsKind::Git), }; Self {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@hooks/src/session.rs` around lines 76 - 80, The code currently calls spec_file_has_content(...) and read_spec_file(...) twice; instead call read_spec_file(config::spec_file_path(session_id)) once inside State::from_id, match on the Result to get parsed entries and set worktree_active = !entries.is_empty() (or false on Err) and derive vcs_kind = entries[0].vcs_kind with a VcsKind::Git fallback when entries is empty; handle the Err case consistently (e.g., treat as no entries and use defaults) so you no longer need spec_file_has_content and avoid double file access.hooks/src/vcs/git.rs (1)
44-74: Duplicated branch logic forNoneand default branch cases.The code blocks for
None(lines 45-58) andSome(b) if b == default_branch(lines 60-74) are identical. Consider extracting to reduce duplication.♻️ Suggested simplification
let (actual_branch, args) = match branch { - None => { - // No branch specified: create ephemeral wt/<short-id>. - let br = format!("wt/{session_id}"); - let args = vec![ - "-C".into(), - repo_str.clone(), - "worktree".into(), - "add".into(), - "-b".into(), - br.clone(), - wt_path.to_string_lossy().to_string(), - format!("origin/{default_branch}"), - ]; - (br, args) - } - Some(b) if b == default_branch => { - // Default branch requested: redirect to ephemeral (don't lock default). + None | Some(b) if branch.is_none() || branch == Some(&default_branch) => { + // No branch or default branch requested: create ephemeral wt/<short-id>. let br = format!("wt/{session_id}"); let args = vec![ "-C".into(),Alternatively, a helper function could build the ephemeral branch args.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@hooks/src/vcs/git.rs` around lines 44 - 74, The match arm for None and Some(b) if b == default_branch duplicates creation of the ephemeral branch and args; extract the duplicated logic into a small helper (e.g., build_ephemeral_worktree_args or create_ephemeral_branch_args) that takes session_id, repo_str, wt_path, and default_branch and returns (br, args), then call that helper from both match arms instead of repeating the vec! construction and br formatting; update the bindings (actual_branch, args) to receive the helper's return value.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@hooks/src/bin/ensure_worktree.rs`:
- Around line 41-42: Currently the code eagerly defaults vcs_kind to
VcsKind::Git from args.get(2), which causes JJ repos to be misdetected; change
the logic so args.get(2) is parsed into an Option<VcsKind> (fail fast on an
explicit parse error), defer choosing the backend until after repo_path is
resolved, and when the arg is absent auto-detect the repo kind from repo_path
(e.g., via your repo-detection helper) and only then set vcs_kind; remove the
unconditional unwrap_or_default() and ensure explicit invalid values return an
error instead of silently falling back to Git.
In `@hooks/src/vcs/git.rs`:
- Around line 129-144: The workspace_remove implementation returns (false, err)
unconditionally on the force branch which breaks the success boolean contract;
update the force=true branch in workspace_remove to inspect git::run_git's
Result: if run_git succeeds return (true, None) (or match cleanup::remove's
success semantics), and on error return (false, Some(err_string)) so both
branches return (success, Option<String>) consistently; locate workspace_remove
and replace the current unconditional (false, err) with a match on
git::run_git(...) that maps Ok -> (true, None) and Err(e) -> (false, Some(e)) to
mirror cleanup::remove.
In `@hooks/src/vcs/jj.rs`:
- Around line 117-149: The workspace_list function currently looks for a "(at "
token which jj does not emit, causing paths to be thrown away; change the
parsing in workspace_list (and the closure that builds WorkspaceInfo) to split
each line on the first ':' (e.g., line.split_once(':')), trim the left side as
the workspace name and the right side as the path string, convert the trimmed
path into a PathBuf (or use PathBuf::new() if the path part is empty), and
return WorkspaceInfo { name, path: PathBuf::from(path_str) } instead of the
existing "(at " logic.
---
Outside diff comments:
In `@hooks/src/bin/session_end.rs`:
- Around line 74-99: The changelog warning always suggests a Git worktree
cleanup even for JJ workspaces; update the dirty-branch in the block after
workspace removal (where entry.vcs_kind, VcsKind::Jj, VcsKind::JjColocated and
JjBackend.workspace_remove are used) to choose the appropriate cleanup/help text
based on entry.vcs_kind before calling append_to_changelog (and when formatting
the message for sess.changelog_path); i.e., detect JJ vs Git and format the
cleanup command and hint accordingly so JJ entries get the correct JJ-specific
removal instruction instead of the Git worktree command.
---
Nitpick comments:
In `@hooks/src/session.rs`:
- Around line 76-80: The code currently calls spec_file_has_content(...) and
read_spec_file(...) twice; instead call
read_spec_file(config::spec_file_path(session_id)) once inside State::from_id,
match on the Result to get parsed entries and set worktree_active =
!entries.is_empty() (or false on Err) and derive vcs_kind = entries[0].vcs_kind
with a VcsKind::Git fallback when entries is empty; handle the Err case
consistently (e.g., treat as no entries and use defaults) so you no longer need
spec_file_has_content and avoid double file access.
In `@hooks/src/vcs/git.rs`:
- Around line 44-74: The match arm for None and Some(b) if b == default_branch
duplicates creation of the ephemeral branch and args; extract the duplicated
logic into a small helper (e.g., build_ephemeral_worktree_args or
create_ephemeral_branch_args) that takes session_id, repo_str, wt_path, and
default_branch and returns (br, args), then call that helper from both match
arms instead of repeating the vec! construction and br formatting; update the
bindings (actual_branch, args) to receive the helper's return value.
In `@hooks/src/vcs/jj.rs`:
- Around line 155-159: The current is_clean(&self, _path: &Path) unconditionally
returns true; change it to detect unresolved merge conflicts and return false
when present so callers that care about conflicts get accurate status. In
practice, update is_clean to query the repository/working-copy conflict state
(e.g., via the jj API or our existing workspace state helpers) for the given
Path and return false if any unresolved conflicts exist, otherwise return true;
keep the auto-snapshot rationale in comments and add/update tests and
documentation to note that is_clean now considers merge conflicts.
- Line 79: The workspace_remove function currently ignores the _force parameter
and always performs the same forget+remove sequence; update
workspace_remove(&self, entry: &SpecEntry, _force: bool) to either implement
force semantics for jj (e.g., when force is true, bypass checks and remove even
with uncommitted changes) or explicitly document that force is a no-op for jj:
modify the function body to branch on the force flag (or rename parameter and
add a comment) and adjust behavior accordingly so callers understand whether
uncommitted changes will be preserved/checked, referencing workspace_remove and
SpecEntry to locate the change.
- Around line 31-32: RE_JJ_EDIT_IMMUTABLE currently only matches "root" and
"trunk", so it misses other immutable targets like "@-" or explicit revision
IDs; update the detection to include those forms (e.g., include "@-" and a hex
rev-id pattern such as [0-9a-f]{7,40}) or move this check out of the single
regex into a small helper that parses the edit target and calls an
is_immutable_revision check; modify the LazyLock/Regex named
RE_JJ_EDIT_IMMUTABLE (or add a new helper function) accordingly, add a brief doc
comment explaining supported patterns, and adjust/add tests to cover "@-" and
hex id cases.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 9406556d-e4a6-4d6b-bc58-70007fffd74a
📒 Files selected for processing (13)
CLAUDE.mdhooks/src/bin/ensure_worktree.rshooks/src/bin/permissions.rshooks/src/bin/session_end.rshooks/src/gitcheck.rshooks/src/lib.rshooks/src/sandbox.rshooks/src/session.rshooks/src/vcs/git.rshooks/src/vcs/jj.rshooks/src/vcs/mod.rshooks/src/worktree/mod.rshooks/tests/jj_backend.rs
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (3)
hooks/src/vcs/jj.rs (2)
41-42:⚠️ Potential issue | 🟠 Major
is_repo_op()still treats anyjjsubstring as a repo command.
cmd.contains("jj")is too broad. In the no-worktree pathpermissions.rsturns a false positive here into a hard deny, so shell text likeecho jjor any argument containing/jj/can still be blocked even though nojjcommand is being run.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@hooks/src/vcs/jj.rs` around lines 41 - 42, The current is_repo_op() treats any "jj" substring as a repo command; instead parse the command string to identify the actual executable token and only treat it as a repo op if that token is literally "jj" or its basename is "jj" (e.g., "/usr/bin/jj"), and still exclude safe subcommands via RE_JJ_SAFE_SUBCOMMAND; update the function is_repo_op to split cmd by whitespace (or otherwise extract the first token/executable), get its basename (Path::new(...).file_name()) and check equality to "jj" (or exact match) rather than using cmd.contains("jj"), while keeping the existing RE_JJ_SAFE_SUBCOMMAND check.
133-165:⚠️ Potential issue | 🟠 MajorUse a machine-readable JJ interface for workspace paths.
The CLI already gives you
jj workspace list -Tfor custom rendering andjj workspace root --name <workspace>for the root path. Scraping a hard-coded"(at ...)"fragment from the default renderer is brittle, and when that fragment is absent this implementation collapses non-default workspaces toPathBuf::new(). (docs.jj-vcs.dev)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@hooks/src/vcs/jj.rs` around lines 133 - 165, In workspace_list, avoid brittle scraping of the human renderer: replace parsing of "jj workspace list" output in the workspace_list function by using jj's machine-readable interface — either call "jj workspace list -T" with a template that emits both name and root (e.g. include a {root} field) and parse that output, or after obtaining each workspace name call "jj workspace root --name <workspace>" to get the exact path; update the Command::new("jj") invocations accordingly and use the resulting root Paths when constructing WorkspaceInfo instead of falling back to PathBuf::new().hooks/src/bin/ensure_worktree.rs (1)
41-42:⚠️ Potential issue | 🟠 MajorReject invalid
vcs-kindvalues instead of silently auto-detecting.If the third argument is present but unparsable,
parse().ok()drops it and falls back to repo detection. That hides caller bugs and can route the request through the wrong backend.🛠️ Suggested fix
- let explicit_vcs_kind: Option<VcsKind> = args.get(2).and_then(|s| s.parse().ok()); + let explicit_vcs_kind = match args.get(2) { + Some(raw) => match raw.parse::<VcsKind>() { + Ok(kind) => Some(kind), + Err(_) => { + muzzle::log::error("ensure-worktree", &format!("invalid vcs-kind: {raw}")); + std::process::exit(1); + } + }, + None => None, + };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@hooks/src/bin/ensure_worktree.rs` around lines 41 - 42, The code currently swallows parse errors for the optional VCS kind by using args.get(2).and_then(|s| s.parse().ok()), causing implicit auto-detection on invalid input; change this so that if a third argument is present but fails to parse into VcsKind you return/exit with an error and a clear message instead of falling back. Specifically, replace the args.get(2).and_then(...).ok() pattern used to populate explicit_vcs_kind with logic that checks args.get(2): if Some(s) attempt s.parse::<VcsKind>() and propagate the parse error (or print an error and exit non-zero) so invalid values are rejected; keep the rest of the code that uses explicit_vcs_kind unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@hooks/src/bin/session_end.rs`:
- Around line 93-98: The JJ cleanup hint currently passes the full working-tree
path (entry.wt_path) as the workspace name; update the VcsKind::Jj |
VcsKind::JjColocated arm that builds cleanup_hint to extract the workspace name
via the path basename (like workspace_remove() does) and construct the command
using that basename and the -R <repo_root> flag (matching the git hint pattern),
so the hint becomes "jj workspace forget <basename> -R <repo_root> && rm -rf
<wt_path>" (use entry.wt_path for rm -rf and the basename for the jj workspace
name).
In `@hooks/src/session.rs`:
- Around line 850-853: Replace usage of std::env::temp_dir() when creating test
fixtures with config::state_dir() so tests exercise the XDG layout; update the
code that creates tmp (the variable named tmp, the create_dir_all call, and the
spec_path join for "roundtrip.env" and the analogous block around the second
occurrence) to build paths from config::state_dir(). Ensure you import or
reference config::state_dir() and keep the same subdirectory name
("muzzle-test-spec-5field") so only the base directory changes.
- Around line 53-54: The session currently stores a single VCS kind in
State.vcs_kind (set from entries[0].vcs_kind in State::from_id()) which breaks
multi-repo sessions and makes check_bash() construct a single backend; change
the design to track VCS kind per SpecEntry instead of copying entries[0] into
State.vcs_kind or persist a per-target mapping of repo->VcsKind, then update
State::from_id(), any serialization/deserialize paths, and check_bash() to
consult the per-entry/per-target VcsKind when building backends; ensure new
sessions with empty spec do not default to Git by avoiding any fallback to
entries[0].vcs_kind.
In `@hooks/src/vcs/git.rs`:
- Around line 54-56: workspace_add() currently ignores the caller-provided dest
and always uses config::worktree_path(repo_path, session_id) (wt_path) and then
derives repo from dest.file_name(), which records the session ID as the repo
name; change workspace_add() to honor the dest argument as the actual
destination path, derive repo from repo_path (e.g., repo_path.file_name() or
repo_str derived from repo_path) instead of from dest, and only use
config::worktree_path(repo_path, session_id) when dest is not provided or
explicitly meant to be a worktree; update uses in the function (variables
repo_str, wt_path, and the construction of SpecEntry) and the similar code block
around lines 113-123 to ensure SpecEntry.repo is set from the real repo name
derived from repo_path.
In `@hooks/src/vcs/jj.rs`:
- Around line 91-130: The function workspace_remove currently returns (true,
None) on successful JJ workspace forget and fs removal, which incorrectly
signals a "dirty" workspace left behind; update the final successful return in
workspace_remove (the tuple after the remove_dir_all success branch) to (false,
None) so it follows Git's (dirty, err) contract used by
session_end::remove_worktrees(); leave the existing error returns intact (they
already return false with an error message).
---
Duplicate comments:
In `@hooks/src/bin/ensure_worktree.rs`:
- Around line 41-42: The code currently swallows parse errors for the optional
VCS kind by using args.get(2).and_then(|s| s.parse().ok()), causing implicit
auto-detection on invalid input; change this so that if a third argument is
present but fails to parse into VcsKind you return/exit with an error and a
clear message instead of falling back. Specifically, replace the
args.get(2).and_then(...).ok() pattern used to populate explicit_vcs_kind with
logic that checks args.get(2): if Some(s) attempt s.parse::<VcsKind>() and
propagate the parse error (or print an error and exit non-zero) so invalid
values are rejected; keep the rest of the code that uses explicit_vcs_kind
unchanged.
In `@hooks/src/vcs/jj.rs`:
- Around line 41-42: The current is_repo_op() treats any "jj" substring as a
repo command; instead parse the command string to identify the actual executable
token and only treat it as a repo op if that token is literally "jj" or its
basename is "jj" (e.g., "/usr/bin/jj"), and still exclude safe subcommands via
RE_JJ_SAFE_SUBCOMMAND; update the function is_repo_op to split cmd by whitespace
(or otherwise extract the first token/executable), get its basename
(Path::new(...).file_name()) and check equality to "jj" (or exact match) rather
than using cmd.contains("jj"), while keeping the existing RE_JJ_SAFE_SUBCOMMAND
check.
- Around line 133-165: In workspace_list, avoid brittle scraping of the human
renderer: replace parsing of "jj workspace list" output in the workspace_list
function by using jj's machine-readable interface — either call "jj workspace
list -T" with a template that emits both name and root (e.g. include a {root}
field) and parse that output, or after obtaining each workspace name call "jj
workspace root --name <workspace>" to get the exact path; update the
Command::new("jj") invocations accordingly and use the resulting root Paths when
constructing WorkspaceInfo instead of falling back to PathBuf::new().
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 647c0361-1836-4d98-8248-40255f76b52b
📒 Files selected for processing (7)
hooks/src/bin/ensure_worktree.rshooks/src/bin/permissions.rshooks/src/bin/session_end.rshooks/src/lib.rshooks/src/session.rshooks/src/vcs/git.rshooks/src/vcs/jj.rs
Add VcsBackend trait abstraction with GitBackend (wrapping existing code) and JjBackend (new jj workspace support). Git remains the default; jj activates via auto-detection (.jj/ presence). - VcsBackend trait with 15 methods covering workspace lifecycle, safety checks, and workspace enforcement - GitBackend: pure delegation to existing worktree/ and gitcheck modules - JjBackend: jj workspace add/forget, 6 safety regex patterns, colocated mode with GIT_DIR awareness - VcsKind enum (Git/Jj/JjColocated) with auto-detection from .jj/.git - Session State and SpecEntry carry vcs_kind; 5-field spec file format with backward compat for 4-field - All 4 hook binaries route through detected VCS backend - 39 new tests (12 unit + 27 integration), zero regressions Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- Add vcs/mod.rs, vcs/git.rs, vcs/jj.rs to architecture tree - Fix unresolved `Display` doc link to `std::fmt::Display` Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- ensure_worktree: auto-detect VCS from repo path when arg omitted, resolve repo_path once before VCS routing - permissions: use JjBackend::is_repo_op() to avoid matching utility commands (jj version/help/config/init) as repo operations - jj.rs: add is_repo_op() pub method, fix workspace_list parsing to handle default workspace path, fix is_valid_workspace to accept .worktrees/ paths, document force no-op in workspace_remove - git.rs: deduplicate ephemeral branch args into helper function - session_end: VCS-aware dirty cleanup hints (jj workspace forget vs git worktree remove) - session.rs: eliminate double file read in State::from_id() - lib.rs: include VCS kind in ensure-worktree invocation hint
- jj.rs: workspace_remove returned (true, None) on success — inverted the dirty flag, causing every clean jj removal to be logged as dirty. Fixed to (false, None). - jj.rs: RE_JJ_PUSH_BOOKMARK now matches --bookmark=name form (= sign) in addition to --bookmark name (whitespace) - jj.rs: added test for --bookmark= form - git.rs: workspace_add derived repo name from dest (session ID) instead of repo_path; also now uses caller's dest path instead of recomputing - session_end: jj cleanup hint now uses workspace name (last path component) not full path, and runs from repo root via cd
There was a problem hiding this comment.
♻️ Duplicate comments (2)
hooks/src/vcs/git.rs (1)
128-143:⚠️ Potential issue | 🟡 MinorReturn value semantics still inconsistent on the force path.
The
force=truebranch returns(false, err)unconditionally. Per the(success, Option<error>)contract used bysession_end::remove_worktrees(), successful removal should return(true, None). The current code will incorrectly signal failure even whengit worktree remove --forcesucceeds.🐛 Proposed fix
fn workspace_remove(&self, entry: &SpecEntry, force: bool) -> (bool, Option<String>) { if force { - let err = git::run_git(&[ + let result = git::run_git(&[ "-C", &entry.repo_path, "worktree", "remove", "--force", &entry.wt_path, - ]) - .err(); - (false, err) + ]); + match result { + Ok(_) => (true, None), + Err(e) => (false, Some(e)), + } } else { cleanup::remove(entry) } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@hooks/src/vcs/git.rs` around lines 128 - 143, The force branch of workspace_remove currently returns (false, err) regardless of git::run_git's outcome; change it to inspect the Result from git::run_git - if Ok, return (true, None), otherwise return (false, Some(err)) so the function adheres to the (success, Option<error>) contract used by session_end::remove_worktrees(); update the force path in workspace_remove (working with SpecEntry, git::run_git) to capture the result, check result.is_ok() and return the appropriate tuple.hooks/src/vcs/jj.rs (1)
135-168:⚠️ Potential issue | 🟡 MinorFix
workspace_list()to parse actualjj workspace listoutput format.The code searches for
"(at "which doesn't exist in the standard output format. The actual format isname: /path/to/workspace (o: operation_id @ commit_id), where the path comes directly after the colon. Non-default workspaces currently return empty paths because the"(at "substring is never found.♻️ Suggested parsing fix
stdout .lines() .filter_map(|line| { - // Format: "name: <change-id> <commit-id> <description>" - // Non-default workspaces may include "(at <path>)" but this - // isn't guaranteed across jj versions. Derive paths from the - // repo root as a best-effort fallback. - let name = line.split(':').next()?.trim().to_string(); - let path = if let Some(start) = line.find("(at ") { - let rest = &line[start + 4..]; - PathBuf::from(rest.strip_suffix(')').unwrap_or(rest).trim()) - } else if name == "default" { - // The default workspace lives at the repo root. - repo_path.to_path_buf() - } else { - // Non-default workspaces: best-effort, path unknown. - PathBuf::new() - }; + // Format: "name: /path/to/workspace (o: operation_id @ commit_id)" + let (name, rest) = line.split_once(':')?; + let name = name.trim().to_string(); + let path_str = rest.trim(); + let path = if path_str.starts_with('/') { + PathBuf::from(path_str.split_whitespace().next().unwrap_or(path_str)) + } else if name == "default" { + repo_path.to_path_buf() + } else { + PathBuf::new() + }; Some(WorkspaceInfo { name, path }) }) .collect()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@hooks/src/vcs/jj.rs` around lines 135 - 168, The current workspace_list() in jj.rs incorrectly looks for "(at " which doesn't appear in jj output; update the parser in workspace_list (and the WorkspaceInfo construction) to take the text after the first ':' as the workspace location portion, trim it, and if it contains a ' (' (space then '(') treat the path as the substring from after ':' up to the ' ('; if that path is empty and name == "default" use repo_path.to_path_buf(), otherwise use PathBuf::new() as the unknown-path fallback. Ensure you still keep the extraction of name = line.split(':').next()?.trim().to_string() and robustly handle lines without parentheses or without a path.
🧹 Nitpick comments (1)
hooks/src/bin/ensure_worktree.rs (1)
41-42: Consider failing fast on invalid explicit VCS kind argument.Line 42 uses
.ok()to silently convert parse errors toNone, which falls back to auto-detection. This could mask typos (e.g.,ensure-worktree myrepo gtiwould auto-detect instead of erroring). Consider failing explicitly when a non-empty argument fails to parse.♻️ Optional: Fail fast on invalid explicit VCS kind
- let explicit_vcs_kind: Option<VcsKind> = args.get(2).and_then(|s| s.parse().ok()); + let explicit_vcs_kind: Option<VcsKind> = match args.get(2) { + Some(s) if !s.is_empty() => match s.parse() { + Ok(k) => Some(k), + Err(_) => { + muzzle::log::error( + "ensure-worktree", + &format!("invalid vcs-kind '{}'; expected git, jj, or jj-colocated", s), + ); + std::process::exit(1); + } + }, + _ => None, + };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@hooks/src/bin/ensure_worktree.rs` around lines 41 - 42, The current parsing of the second CLI argument silently ignores parse errors (args.get(2).and_then(|s| s.parse().ok())), which hides typos and falls back to auto-detection; change this so if args.get(2) is Some(s) and s is non-empty, attempt to parse into VcsKind and if parsing fails return an explicit error/exit with a clear message rather than converting to None—locate the explicit_vcs_kind binding in ensure_worktree.rs and replace the .ok() fallback with explicit handling that validates the provided string and fails fast on parse errors.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@hooks/src/vcs/git.rs`:
- Around line 128-143: The force branch of workspace_remove currently returns
(false, err) regardless of git::run_git's outcome; change it to inspect the
Result from git::run_git - if Ok, return (true, None), otherwise return (false,
Some(err)) so the function adheres to the (success, Option<error>) contract used
by session_end::remove_worktrees(); update the force path in workspace_remove
(working with SpecEntry, git::run_git) to capture the result, check
result.is_ok() and return the appropriate tuple.
In `@hooks/src/vcs/jj.rs`:
- Around line 135-168: The current workspace_list() in jj.rs incorrectly looks
for "(at " which doesn't appear in jj output; update the parser in
workspace_list (and the WorkspaceInfo construction) to take the text after the
first ':' as the workspace location portion, trim it, and if it contains a ' ('
(space then '(') treat the path as the substring from after ':' up to the ' (';
if that path is empty and name == "default" use repo_path.to_path_buf(),
otherwise use PathBuf::new() as the unknown-path fallback. Ensure you still keep
the extraction of name = line.split(':').next()?.trim().to_string() and robustly
handle lines without parentheses or without a path.
---
Nitpick comments:
In `@hooks/src/bin/ensure_worktree.rs`:
- Around line 41-42: The current parsing of the second CLI argument silently
ignores parse errors (args.get(2).and_then(|s| s.parse().ok())), which hides
typos and falls back to auto-detection; change this so if args.get(2) is Some(s)
and s is non-empty, attempt to parse into VcsKind and if parsing fails return an
explicit error/exit with a clear message rather than converting to None—locate
the explicit_vcs_kind binding in ensure_worktree.rs and replace the .ok()
fallback with explicit handling that validates the provided string and fails
fast on parse errors.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: de57327c-35e9-4492-88ef-f55112046e01
📒 Files selected for processing (13)
CLAUDE.mdhooks/src/bin/ensure_worktree.rshooks/src/bin/permissions.rshooks/src/bin/session_end.rshooks/src/gitcheck.rshooks/src/lib.rshooks/src/sandbox.rshooks/src/session.rshooks/src/vcs/git.rshooks/src/vcs/jj.rshooks/src/vcs/mod.rshooks/src/worktree/mod.rshooks/tests/jj_backend.rs
✅ Files skipped from review due to trivial changes (2)
- CLAUDE.md
- hooks/src/session.rs
🚧 Files skipped from review as they are similar to previous changes (6)
- hooks/src/worktree/mod.rs
- hooks/src/sandbox.rs
- hooks/src/bin/session_end.rs
- hooks/src/lib.rs
- hooks/src/bin/permissions.rs
- hooks/src/vcs/mod.rs
- default_branch: match "main:" not "main" to avoid prefix false
positives (e.g. "mainline" bookmark incorrectly matching "main")
- is_repo_op: use word-boundary regex matching instead of
cmd.contains("jj") to avoid false positives on unrelated commands
(e.g. "mkdir jj-workspace", "cat config.jjk")
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
VcsBackendtrait abstraction withGitBackend(wrapping existing code) andJjBackend(new jj workspace support).jj/presence)StateandSpecEntrycarryvcs_kind; 5-field spec file format with backward compat for 4-fieldWhat changed
New files (4):
hooks/src/vcs/mod.rs—VcsBackendtrait,VcsKindenum,detect()functionhooks/src/vcs/git.rs—GitBackendwrapping all existing git logic (pure delegation)hooks/src/vcs/jj.rs—JjBackendwith workspace ops + 6 safety regex patternshooks/tests/jj_backend.rs— 27 integration testsModified files (8): session.rs, sandbox.rs, lib.rs, gitcheck.rs, permissions.rs, ensure_worktree.rs, session_end.rs, worktree/mod.rs
jj backend features
jj workspace add/forgetfor session isolationjj git push, protected bookmark deletion.jj/+.git/): delegates git safety checks for raw git commandsWORKTREE_MISSINGmessage encodes VCS kind forensure-worktreerouting.gitdetection handles both files (worktree markers) and directoriesNo new dependencies
All jj operations use
Command::new("jj")— no new crate dependencies.Test plan
cargo checkcleancargo clippy -D warningscleancargo fmt --checkcleanCloses #37
🤖 Generated with Claude Code