feat: add per-repo session strategy#32
Conversation
Adds a per-repo session strategy that derives the session name from `basename(git rev-parse --show-toplevel)` instead of `basename(cwd)`. Stable across cwd movement within a repo. Useful for multi-tool setups where a separate client (e.g., a server-side memory writer) needs to resolve the same session name from a different cwd within the repo. Implementation matches the per-repo logic shipped in NousResearch/hermes-agent's Honcho client (Python) bit-for-bit: - basename of git toplevel, falling back to cwd basename on any failure - 5s git timeout - manual override map honored for both per-directory and per-repo - sanitization, sessionPeerPrefix, and override-keying behavior unchanged Uses Bun.spawnSync (no Node child_process import) for runtime idiom. Guards against missing stdout on exit-zero and falls through to cwd basename on any error. Plus a small UX fix in `mcp/server.ts`: register `per-repo` in the status-card label map so `/honcho:status` renders `mapping per repo` instead of the raw hyphenated strategy name. README updated with new strategy row.
WalkthroughA new Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
README.md (1)
216-216:⚠️ Potential issue | 🟡 MinorStale union in the config example: include
per-repo.The strategies table further down lists
per-repo, but the inline comment on the JSONC example here still enumerates only the three legacy values. Users copy-pasting this example as a reference for valid values will thinkper-repois unsupported.📝 Proposed fix
- "sessionStrategy": "per-directory", // "per-directory" | "git-branch" | "chat-instance" + "sessionStrategy": "per-directory", // "per-directory" | "per-repo" | "git-branch" | "chat-instance"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@README.md` at line 216, Update the inline comment for the "sessionStrategy" example to include the new "per-repo" option so the JSONC sample matches the strategies table; locate the "sessionStrategy" line in the README example and append "per-repo" to the comment enum (e.g., "// \"per-directory\" | \"git-branch\" | \"chat-instance\" | \"per-repo\"") so copy-paste users see the current valid values.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@plugins/honcho/src/config.ts`:
- Around line 571-600: The code uses Bun.spawnSync(..., { timeout }) in the
"per-repo" case, so add a minimum Bun engine requirement to package.json to
guarantee the timeout option exists (set "engines": { "bun":
"<minimum-tested-version-or-the-sep-2024-release-that-introduced-timeout>" } or
at least ">= 1.0.0" if you prefer conservative); also optionally deduplicate the
cwd-basename fallback inside the same "per-repo" case by extracting const
fallback = sanitizeForSessionName(basename(cwd)) and using it in the three
branches (references: Bun.spawnSync, sanitizeForSessionName, basename, the
"per-repo" case).
---
Outside diff comments:
In `@README.md`:
- Line 216: Update the inline comment for the "sessionStrategy" example to
include the new "per-repo" option so the JSONC sample matches the strategies
table; locate the "sessionStrategy" line in the README example and append
"per-repo" to the comment enum (e.g., "// \"per-directory\" | \"git-branch\" |
\"chat-instance\" | \"per-repo\"") so copy-paste users see the current valid
values.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: e33fbcb6-a3a7-4b07-bccc-42efde396494
📒 Files selected for processing (4)
CHANGELOG.mdREADME.mdplugins/honcho/src/config.tsplugins/honcho/src/mcp/server.ts
| case "per-repo": { | ||
| let repoBase: string; | ||
| try { | ||
| // git rev-parse --show-toplevel with 5s timeout. Falls back to cwd | ||
| // basename on any failure (non-git, timeout, non-zero exit, empty | ||
| // stdout, thrown error). Mirrors hermes-agent's per-repo derivation. | ||
| const result = Bun.spawnSync(["git", "rev-parse", "--show-toplevel"], { | ||
| cwd, | ||
| stdout: "pipe", | ||
| stderr: "ignore", | ||
| stdin: "ignore", | ||
| timeout: 5000, | ||
| }); | ||
| // SyncSubprocess.stdout is `Buffer | undefined`; guard against the | ||
| // exit-0-but-empty-stdout case so we never produce an empty session. | ||
| if (result.exitCode === 0 && result.stdout) { | ||
| const root = result.stdout.toString().trim(); | ||
| if (root) { | ||
| repoBase = sanitizeForSessionName(basename(root)); | ||
| } else { | ||
| repoBase = sanitizeForSessionName(basename(cwd)); | ||
| } | ||
| } else { | ||
| repoBase = sanitizeForSessionName(basename(cwd)); | ||
| } | ||
| } catch { | ||
| repoBase = sanitizeForSessionName(basename(cwd)); | ||
| } | ||
| return usePrefix ? `${peerPart}-${repoBase}` : repoBase; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find any declared Bun version floor (engines/bun, packageManager, README, CI).
fd -t f -e json -e md -e yml -e yaml --exec rg -nH -C2 -i 'bun.*[0-9]+\.[0-9]+|engines|packageManager' {} \;Repository: plastic-labs/claude-honcho
Length of output: 36709
🏁 Script executed:
cat plugins/honcho/package.jsonRepository: plastic-labs/claude-honcho
Length of output: 582
🌐 Web query:
Bun spawnSync timeout option version added changelog
💡 Result:
The timeout option for Bun.spawnSync (and Bun.spawn) was added in late September 2024. The feature request was opened in January 2023 (Issue #1788) and closed as completed on September 26, 2024, when the implementing pull request #14180 (for AbortSignal support, which was part of the same feature) was merged. Official Bun documentation now includes the timeout?: number option in SpawnSyncOptions, specifying it kills the process with killSignal (default SIGTERM) after the specified milliseconds. If timeout occurs, SyncSubprocess.exitedDueToTimeout is true.
Citations:
- 1: Add
signalandtimeoutoptions tospawnandspawnSyncoven-sh/bun#1788 - 2: https://bun.sh/reference/bun/Spawn/SpawnSyncOptions/timeout
- 3: https://bun.com/reference/bun/Spawn/SpawnSyncOptions
- 4: https://bun.com/reference/bun/spawnSync
- 5: https://bun.sh/docs/runtime/child-process
Verify the minimum Bun version requirement and add it to package.json.
The implementation is correct, but the plugin has no explicit Bun version constraint. The timeout option in Bun.spawnSync was added in September 2024 (not v1.2.6 as mentioned earlier). Without a declared minimum, users on earlier Bun versions will silently skip the timeout, and a hung git process could block session initialization indefinitely. Add "bun": ">= 1.0.0" (or more conservatively, the current tested Bun version) to the engines field in package.json to enforce this requirement.
♻️ Optional readability nit: deduplicate the fallback
The cwd-basename fallback is constructed in three branches. A small helper makes intent clearer and shrinks the case body:
case "per-repo": {
+ const cwdFallback = () => sanitizeForSessionName(basename(cwd));
let repoBase: string;
try {
// git rev-parse --show-toplevel with 5s timeout. Falls back to cwd
// basename on any failure (non-git, timeout, non-zero exit, empty
// stdout, thrown error). Mirrors hermes-agent's per-repo derivation.
const result = Bun.spawnSync(["git", "rev-parse", "--show-toplevel"], {
cwd,
stdout: "pipe",
stderr: "ignore",
stdin: "ignore",
timeout: 5000,
});
- // SyncSubprocess.stdout is `Buffer | undefined`; guard against the
- // exit-0-but-empty-stdout case so we never produce an empty session.
- if (result.exitCode === 0 && result.stdout) {
- const root = result.stdout.toString().trim();
- if (root) {
- repoBase = sanitizeForSessionName(basename(root));
- } else {
- repoBase = sanitizeForSessionName(basename(cwd));
- }
- } else {
- repoBase = sanitizeForSessionName(basename(cwd));
- }
+ const root = result.exitCode === 0 && result.stdout
+ ? result.stdout.toString().trim()
+ : "";
+ repoBase = root ? sanitizeForSessionName(basename(root)) : cwdFallback();
} catch {
- repoBase = sanitizeForSessionName(basename(cwd));
+ repoBase = cwdFallback();
}
return usePrefix ? `${peerPart}-${repoBase}` : repoBase;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| case "per-repo": { | |
| let repoBase: string; | |
| try { | |
| // git rev-parse --show-toplevel with 5s timeout. Falls back to cwd | |
| // basename on any failure (non-git, timeout, non-zero exit, empty | |
| // stdout, thrown error). Mirrors hermes-agent's per-repo derivation. | |
| const result = Bun.spawnSync(["git", "rev-parse", "--show-toplevel"], { | |
| cwd, | |
| stdout: "pipe", | |
| stderr: "ignore", | |
| stdin: "ignore", | |
| timeout: 5000, | |
| }); | |
| // SyncSubprocess.stdout is `Buffer | undefined`; guard against the | |
| // exit-0-but-empty-stdout case so we never produce an empty session. | |
| if (result.exitCode === 0 && result.stdout) { | |
| const root = result.stdout.toString().trim(); | |
| if (root) { | |
| repoBase = sanitizeForSessionName(basename(root)); | |
| } else { | |
| repoBase = sanitizeForSessionName(basename(cwd)); | |
| } | |
| } else { | |
| repoBase = sanitizeForSessionName(basename(cwd)); | |
| } | |
| } catch { | |
| repoBase = sanitizeForSessionName(basename(cwd)); | |
| } | |
| return usePrefix ? `${peerPart}-${repoBase}` : repoBase; | |
| } | |
| case "per-repo": { | |
| const cwdFallback = () => sanitizeForSessionName(basename(cwd)); | |
| let repoBase: string; | |
| try { | |
| // git rev-parse --show-toplevel with 5s timeout. Falls back to cwd | |
| // basename on any failure (non-git, timeout, non-zero exit, empty | |
| // stdout, thrown error). Mirrors hermes-agent's per-repo derivation. | |
| const result = Bun.spawnSync(["git", "rev-parse", "--show-toplevel"], { | |
| cwd, | |
| stdout: "pipe", | |
| stderr: "ignore", | |
| stdin: "ignore", | |
| timeout: 5000, | |
| }); | |
| const root = result.exitCode === 0 && result.stdout | |
| ? result.stdout.toString().trim() | |
| : ""; | |
| repoBase = root ? sanitizeForSessionName(basename(root)) : cwdFallback(); | |
| } catch { | |
| repoBase = cwdFallback(); | |
| } | |
| return usePrefix ? `${peerPart}-${repoBase}` : repoBase; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@plugins/honcho/src/config.ts` around lines 571 - 600, The code uses
Bun.spawnSync(..., { timeout }) in the "per-repo" case, so add a minimum Bun
engine requirement to package.json to guarantee the timeout option exists (set
"engines": { "bun":
"<minimum-tested-version-or-the-sep-2024-release-that-introduced-timeout>" } or
at least ">= 1.0.0" if you prefer conservative); also optionally deduplicate the
cwd-basename fallback inside the same "per-repo" case by extracting const
fallback = sanitizeForSessionName(basename(cwd)) and using it in the three
branches (references: Bun.spawnSync, sanitizeForSessionName, basename, the
"per-repo" case).
Brings the per-repo session strategy patch (PR plastic-labs#32 at upstream) into fork main so /plugin install honcho@honcho from sasha-id/claude-honcho includes the patch. Upstream PR is unaffected — PR diff is computed against plastic-labs/main, not fork main.
1 participant
Summary
Adds a
per-reposession strategy that derives the session name frombasename(git rev-parse --show-toplevel)instead ofbasename(cwd).Why
Multi-tool setups where claude-honcho and a server-side memory writer (e.g., a Python or other-language Honcho client) target the same workspace need a session-naming scheme that survives cwd variation within a single repo. Current strategies all derive from cwd or branch:
per-directoryresolves differently fromrepo/vsrepo/src/.git-branchfragments memory across branches.chat-instancedoesn't persist across restarts.This
per-repostrategy mirrors the existingper-repologic in Hermes (NousResearch/hermes-agent's Python Honcho client atplugins/memory/honcho/client.py:509-523, 578-582). Two clients pointing at the same workspace can now share a session name without per-cwd config translation.Implementation
Two source files:
plugins/honcho/src/config.ts— four small edits:SessionStrategytype extended with"per-repo".HonchoCLAUDEConfig.sessionStrategyupdated to list the new value.getSessionForPath) extended to fire for bothper-directoryandper-repo(matches Hermes behavior —client.py:548-551).case "per-repo"in the strategy switch:Bun.spawnSync(["git", "rev-parse", "--show-toplevel"], { cwd, timeout: 5000, ... })basename(root)thensanitizeForSessionName.sanitizeForSessionName(basename(cwd)).plugins/honcho/src/mcp/server.ts— one cosmetic edit:"per-repo": "per repo"in thestrategyLabelsmap so/honcho:statusand theget_configMCP tool rendermapping per reporather than the raw hyphenated strategy name.No new imports.
Bun.spawnSyncis the native runtime API.README.mdSession Strategies table gains aper-reporow.CHANGELOG.mdupdated under a new## [Unreleased]block per CONTRIBUTING.md.Note on session caching:
src/hooks/session-start.ts:121only auto-caches resolved session names forper-directory(and unset) strategies. Withper-repo, the resolved name is recomputed on every session start fromgit rev-parse --show-toplevel— stable across cwd movement within the repo, no cache needed. This matches the Python reference (Hermes also recomputes per session). Intentionally not extending the cache gate to per-repo.Behavior parity matrix
For cwd
~/projects/<group>/<project>/(project root, git toplevel coincides), withpeerName=alice,sessionPeerPrefix=false:per-directory<project><project>(unchanged)<project>per-repo<project><project>For cwd
~/projects/<group>/<project>/repo/(a subdir that is also its own git repo):per-directoryreporepo(unchanged)repoper-reporeporepoStrict parity confirmed.
Known divergences from Hermes (intentional, called out for reviewers)
Path(...).name); this patch applies the existingsanitizeForSessionName(toLowerCase+[^a-z0-9-_]→-). For ASCII-lowercase repo names the outputs are identical; mixed-case or unicode repo names will diverge. Closing the gap requires either dropping sanitization here (breaks URL/header safety) or adding it in Hermes. Out of scope for this PR.git rev-parse --show-toplevelfrom inside a worktree returns the worktree path, not the main repo path. Two worktrees of the same repo resolve to different session names. Hermes has identical behavior.cwd(not by resolved repo root). Override fires only when launching from the exact configured cwd. Hermes has identical behavior (client.py:548-551). A future "key by repo root" improvement is a separate, paired change in both codebases.Tests
Repo doesn't ship a test suite. Verification was by inspection of
~/.honcho/cache.jsonafter switching the strategy —sessionsmap shows the resolved name matches the parity matrix above.Checklist
bunx tsc --noEmit -p plugins/honcho/tsconfig.json[Unreleased]block added per CONTRIBUTING.mdSummary by CodeRabbit
per-reposession strategy that derives session names from the git repository root, with fallback to the current working directory if git resolution fails.