Skip to content

Support worktree-based workflows via session-path targeting#118

Open
danielstarman wants to merge 1 commit intomodem-dev:mainfrom
danielstarman:feat/session-path-worktree
Open

Support worktree-based workflows via session-path targeting#118
danielstarman wants to merge 1 commit intomodem-dev:mainfrom
danielstarman:feat/session-path-worktree

Conversation

@danielstarman
Copy link

Following up on #95. I use a worktree-first workflow where agents work in isolated worktrees while I review from a separate terminal. Session matching by repoRoot breaks here because each worktree has a different path.

My first attempt used git rev-parse --git-common-dir to resolve all worktrees to a shared root. It worked for my setup, but session resolution gets messy when multiple sessions are open against the same repo, which is exactly the scenario worktrees enable.

So I went a different direction: target sessions by their working directory (--session-path) and provide a separate --source flag for reload to specify where the diff comes from. This keeps each selector unambiguous regardless of how many sessions are open.

What is here:

  • sessionPath as a new session selector (matches on the TUI cwd)
  • sourcePath on reload (load diff from a different directory than the session own)
  • --session-path and --source as distinct CLI flags (no overloading of --repo)
  • cwd threaded through the loader chain so reloads resolve files from the source directory
  • Tests across CLI parsing, daemon state, server forwarding, session commands, and loaders

Happy to adjust the approach if you have a different direction in mind.

@danielstarman danielstarman force-pushed the feat/session-path-worktree branch from 95c52e4 to c0d8ffe Compare March 25, 2026 00:57
@danielstarman danielstarman marked this pull request as ready for review March 25, 2026 01:01
@greptile-apps
Copy link

greptile-apps bot commented Mar 25, 2026

Greptile Summary

This PR adds first-class worktree support to Hunk's session-targeting model by introducing sessionPath (match a live session by its working directory) and sourcePath (load the diff from a different directory than the session's own). The change is well-motivated: in a worktree workflow each worktree has a distinct path, making the existing repoRoot-based selector ambiguous when multiple sessions are open against the same repository.

Key changes:

  • --session-path / --source CLI flags added to hunk session reload; resolveReloadSelector enforces all pairwise mutual-exclusion rules between sessionId, sessionPath, and repoRoot.
  • cwd threading through the loader chain (loadAppBootstraploadGitChangeset, loadShowChangeset, loadFileDiffChangeset, loadPatchChangeset, loadAgentContext) so that git commands and file reads resolve against the source directory, not process.cwd().
  • Session display output updated (hunk session list / get) to always show both path (cwd) and repo (repoRoot) fields.
  • Daemon state (resolveSessionTarget) extended with a sessionPath branch that matches session.cwd; all sendCommand helpers thread the new field into their selectors.
  • Full test coverage across CLI parsing, loader cwd override, daemon resolution, server forwarding, and session command normalization.

One issue was found: the fallback error message in daemonState.ts when multiple sessions are registered without a matching selector still says "specify sessionId or repoRoot", omitting the new sessionPath option — this would mislead users trying to use the new flag. The corresponding test assertion also checks the stale substring and should be updated alongside the fix.

Confidence Score: 4/5

  • Safe to merge after the stale error message in daemonState.ts is updated to include sessionPath.
  • The implementation is coherent end-to-end — the new selector propagates correctly through CLI → HTTP → WebSocket → TUI, and the cwd threading through all loader functions is consistent. Test coverage is solid across all layers. The single substantive issue (stale error message omitting sessionPath) is a UX bug rather than a functional one and does not affect the happy path.
  • src/mcp/daemonState.ts (stale error message at line 116–119) and test/mcp-daemon.test.ts (matching assertion at line 795).

Important Files Changed

Filename Overview
src/core/cli.ts Adds resolveReloadSelector with full conflict-detection for all three selector types (sessionId, sessionPath, repoRoot) plus sourcePath support; updates help text and parseSessionCommand reload branch accordingly.
src/mcp/daemonState.ts Adds sessionPath branch to resolveSessionTarget (matches on session.cwd) and threads sessionPath into all sendCommand selectors; the fallback multi-session error message still says "specify sessionId or repoRoot", omitting the new option.
src/core/loaders.ts Threads an explicit cwd parameter through all loader functions (loadGitChangeset, loadShowChangeset, loadStashShowChangeset, loadFileDiffChangeset, loadPatchChangeset, loadAppBootstrap) so git commands and file reads resolve relative paths against the source directory.
src/ui/App.tsx Passes sourcePath from reload options into both resolveConfiguredCliInput and loadAppBootstrap as cwd, correctly delegating git and file operations to the source directory during a cross-worktree reload.
src/session/commands.ts Adds sessionPath to sessionMatchesSelector and normalizeSessionSelector; updates list/get output to show both path and repo fields; renames normalizeRepoRoot to normalizeSessionSelector to cover the new field.
test/mcp-daemon.test.ts Adds tests for sessionPath-based session resolution; the fallback multi-session error assertion still checks for the old "sessionId or repoRoot" text, and the test description dropped "repo root" despite still covering it.

Sequence Diagram

sequenceDiagram
    participant CLI as CLI (hunk session reload)
    participant SC as session/commands.ts
    participant HTTP as HTTP Daemon API
    participant DS as daemonState.ts
    participant WS as WebSocket → TUI
    participant APP as App.tsx (reloadSession)
    participant LDR as loaders.ts (loadAppBootstrap)

    CLI->>CLI: resolveReloadSelector(sessionId?, sessionPath?, repoRoot?, source?)
    note right of CLI: produces { selector, sourcePath }
    CLI->>SC: runSessionCommand({ selector: {sessionPath}, sourcePath, nextInput })
    SC->>SC: normalizeSessionSelector (resolve paths)
    SC->>HTTP: POST /session-api { action:"reload", selector, sourcePath, nextInput }
    HTTP->>DS: sendReloadSession({ sessionPath, sourcePath, nextInput })
    DS->>DS: resolveSessionTarget → match session.cwd === sessionPath
    DS->>WS: send { command:"reload_session", input:{ sourcePath, nextInput } }
    WS->>APP: reloadIncomingSession({ sourcePath, nextInput })
    APP->>APP: resolveConfiguredCliInput(nextInput, { cwd: sourcePath })
    APP->>LDR: loadAppBootstrap(configuredInput, { cwd: sourcePath })
    LDR->>LDR: resolveGitRepoRoot / runGitText with cwd=sourcePath
    LDR-->>APP: AppBootstrap (changeset from source repo)
    APP-->>WS: ReloadedSessionResult
    WS-->>HTTP: command-result
    HTTP-->>SC: { result: ReloadedSessionResult }
    SC-->>CLI: formatted output
Loading

Comments Outside Diff (3)

  1. src/mcp/daemonState.ts, line 116-119 (link)

    P1 Stale error message omits sessionPath option

    The fallback error thrown when no selector is provided but multiple sessions exist still says "specify sessionId or repoRoot", omitting the newly added sessionPath option. A user who encounters this error while trying to work with the new --session-path flag will not know it's a valid disambiguation option.

  2. test/mcp-daemon.test.ts, line 795 (link)

    P2 Assertion matches stale error-message substring

    The toThrow check for the multi-session ambiguity error still looks for "specify sessionId or repoRoot". Once the error message in daemonState.ts is updated to include sessionPath, this assertion will need to be updated as well to remain meaningful.

  3. test/mcp-daemon.test.ts, line 779 (link)

    P2 Test description dropped "repo root" but still covers it

    The test name was updated from "session id, repo root, or sole-session fallback" to "session id, session path, or sole-session fallback", but the test still exercises repoRoot matching (line 793). The description could be:

    Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!

Reviews (1): Last reviewed commit: "feat: support worktree-based workflows v..." | Re-trigger Greptile

Add sessionPath as a new session selector that matches on the TUI cwd,
and sourcePath on reload to load diffs from a different directory than
the session own. This enables worktree-first workflows where agents
work in isolated worktrees while the reviewer runs hunk elsewhere.

- sessionPath selector threaded through daemon state, commands, and CLI
- --session-path and --source as distinct CLI flags on reload
- cwd parameter threaded through the loader chain for cross-directory reloads
- Tests across CLI parsing, daemon state, server, session commands, and loaders

Closes modem-dev#95
@danielstarman danielstarman force-pushed the feat/session-path-worktree branch from c0d8ffe to 7c5c4e7 Compare March 25, 2026 01:28
@benvinegar
Copy link
Member

@danielstarman Thanks for submitting this. I'm going to need time to review/parse it, but I'll get to it.

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