fix: gate user-peer prefix on hosts.opencode.removeUserPrefix#24
Merged
Conversation
added 2 commits
June 15, 2026 15:58
The user peer was derived as `user:${peerName}` (normalised to
`user-<peerName>`), whereas the claude-honcho and hermes-honcho plugins
use the bare `peerName`. In a shared workspace this split the user's
memory into a separate `user-<peerName>` collection, so OpenCode never
saw or contributed to what the other tools learned. Drop the prefix so
the user peer matches the sibling plugins.
Removing the user: prefix means peerName and aiPeer can now normalise to the same peer id (e.g. both "opencode"), silently merging user and agent memory into one peer. Fail fast on that collision instead.
WalkthroughAdds a ChangesremoveUserPrefix Feature
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 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)
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)
src/tui.ts (1)
269-285:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winHonor an explicitly provided
partial.peerIdScheme.
saveSettings({ peerIdScheme: "prefixed" })is ignored becausenextnever copies the partial value; fresh setup then always writes"bare", and existing configs always preserve the old value.Suggested fix
const next: GlobalSettings & Record<string, unknown> = { ...sharedRaw, baseUrl: typeof partial.baseUrl === "string" ? partial.baseUrl : typeof current.baseUrl === "string" ? current.baseUrl : DEFAULT_BASE_URL, peerName: nextPeerName, hosts: currentHosts, } + if (partial.peerIdScheme === "prefixed" || partial.peerIdScheme === "bare") { + next.peerIdScheme = partial.peerIdScheme + } // First-time setup (no prior shared config): adopt the bare `<peerName>` peer // ID. When a config already exists, `...sharedRaw` preserves its scheme (or // its absence, which the runtime treats as the legacy "prefixed" format), so // upgrading users keep their established peer and context. - if (!existingSharedConfig && typeof next.peerIdScheme !== "string") { + if (!existingSharedConfig && next.peerIdScheme === undefined) { next.peerIdScheme = "bare" }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/tui.ts` around lines 269 - 285, The next object construction does not include peerIdScheme from the partial parameter, so explicitly provided values like saveSettings({ peerIdScheme: "prefixed" }) are ignored. Add peerIdScheme to the next object initialization with a similar conditional pattern to baseUrl: check if partial.peerIdScheme is a string and use it, otherwise fall back to the existing current.peerIdScheme value or apply the first-time setup default logic. This ensures that explicit peerIdScheme values in partial are honored during settings updates.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/index.ts`:
- Around line 628-635: The deriveUserPeerId function currently silently defaults
to the "prefixed" scheme for any peerIdScheme value that is not explicitly
"bare", which means invalid or malformed values like "bare " (with whitespace)
or "legacy" will route memory to the wrong peer without warning. Add explicit
validation in the deriveUserPeerId function to check that peerIdScheme is either
"bare" or "prefixed", and throw an error or log a warning for any other value
instead of silently falling through to the default behavior. This ensures that
configuration mistakes are caught and reported rather than causing silent data
misrouting.
---
Outside diff comments:
In `@src/tui.ts`:
- Around line 269-285: The next object construction does not include
peerIdScheme from the partial parameter, so explicitly provided values like
saveSettings({ peerIdScheme: "prefixed" }) are ignored. Add peerIdScheme to the
next object initialization with a similar conditional pattern to baseUrl: check
if partial.peerIdScheme is a string and use it, otherwise fall back to the
existing current.peerIdScheme value or apply the first-time setup default logic.
This ensures that explicit peerIdScheme values in partial are honored during
settings updates.
🪄 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: 59e4fca2-acc9-4ac7-8153-7b3a69e689fb
📒 Files selected for processing (8)
README.mdsrc/index.tssrc/tui.tstests/context-injection.test.jstests/honcho-setup.test.jstests/peer-collision.test.jstests/peer-topology.test.jstests/user-peer-id.test.js
Comment on lines
+628
to
+635
| const deriveUserPeerId = (settings: Pick<HonchoSettings, "peerName" | "peerIdScheme">) => { | ||
| const name = settings.peerName || currentUserName() | ||
| // "bare" matches the sibling claude-honcho/hermes-honcho plugins and is the | ||
| // format for installs created on/after this version. "prefixed" (the default) | ||
| // preserves the historical `user-<name>` peer ID so upgrading users keep the | ||
| // memory already accumulated under it. | ||
| return settings.peerIdScheme === "bare" ? normalizeId(name) : normalizeId(`user:${name}`) | ||
| } |
There was a problem hiding this comment.
Reject invalid peerIdScheme values instead of silently using legacy peers.
Config JSON is merged into HonchoSettings without enum validation on load, so a malformed value like "bare " or "legacy" currently falls through to "prefixed" and routes memory to the wrong peer without warning.
Suggested fix
+const parsePeerIdScheme = (value: unknown): PeerIdScheme => {
+ if (value === undefined) {
+ return "prefixed"
+ }
+ if (value === "prefixed" || value === "bare") {
+ return value
+ }
+ throw new Error(`Unsupported peerIdScheme value '${String(value)}'`)
+}
+
-const deriveUserPeerId = (settings: Pick<HonchoSettings, "peerName" | "peerIdScheme">) => {
+const deriveUserPeerId = (settings: Pick<HonchoSettings, "peerName"> & { peerIdScheme?: unknown }) => {
const name = settings.peerName || currentUserName()
+ const peerIdScheme = parsePeerIdScheme(settings.peerIdScheme)
// "bare" matches the sibling claude-honcho/hermes-honcho plugins and is the
// format for installs created on/after this version. "prefixed" (the default)
// preserves the historical `user-<name>` peer ID so upgrading users keep the
// memory already accumulated under it.
- return settings.peerIdScheme === "bare" ? normalizeId(name) : normalizeId(`user:${name}`)
+ return peerIdScheme === "bare" ? normalizeId(name) : normalizeId(`user:${name}`)
}📝 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.
Suggested change
| const deriveUserPeerId = (settings: Pick<HonchoSettings, "peerName" | "peerIdScheme">) => { | |
| const name = settings.peerName || currentUserName() | |
| // "bare" matches the sibling claude-honcho/hermes-honcho plugins and is the | |
| // format for installs created on/after this version. "prefixed" (the default) | |
| // preserves the historical `user-<name>` peer ID so upgrading users keep the | |
| // memory already accumulated under it. | |
| return settings.peerIdScheme === "bare" ? normalizeId(name) : normalizeId(`user:${name}`) | |
| } | |
| const parsePeerIdScheme = (value: unknown): PeerIdScheme => { | |
| if (value === undefined) { | |
| return "prefixed" | |
| } | |
| if (value === "prefixed" || value === "bare") { | |
| return value | |
| } | |
| throw new Error(`Unsupported peerIdScheme value '${String(value)}'`) | |
| } | |
| const deriveUserPeerId = (settings: Pick<HonchoSettings, "peerName"> & { peerIdScheme?: unknown }) => { | |
| const name = settings.peerName || currentUserName() | |
| const peerIdScheme = parsePeerIdScheme(settings.peerIdScheme) | |
| // "bare" matches the sibling claude-honcho/hermes-honcho plugins and is the | |
| // format for installs created on/after this version. "prefixed" (the default) | |
| // preserves the historical `user-<name>` peer ID so upgrading users keep the | |
| // memory already accumulated under it. | |
| return peerIdScheme === "bare" ? normalizeId(name) : normalizeId(`user:${name}`) | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/index.ts` around lines 628 - 635, The deriveUserPeerId function currently
silently defaults to the "prefixed" scheme for any peerIdScheme value that is
not explicitly "bare", which means invalid or malformed values like "bare "
(with whitespace) or "legacy" will route memory to the wrong peer without
warning. Add explicit validation in the deriveUserPeerId function to check that
peerIdScheme is either "bare" or "prefixed", and throw an error or log a warning
for any other value instead of silently falling through to the default behavior.
This ensures that configuration mistakes are caught and reported rather than
causing silent data misrouting.
PR #23 dropped the user-peer prefix unconditionally, which re-points every existing user at an empty `<peerName>` peer and orphans their accumulated memory. Gate it on a boolean under the opencode host block instead: hosts.opencode.removeUserPrefix: <boolean> - new installs are stamped `true` at config creation (runtime + TUI) -> bare `<peerName>`, matching the sibling claude-honcho / hermes-honcho plugins - existing/upgraded installs default `false` -> keep `user-<peerName>` and its memory; the config file is never mutated on upgrade - it's a normal validated host setting, so anyone can opt in via the file, /honcho:set-config, or the TUI Builds on @rrei's #23: keeps the bare derivation and the user/agent peer collision guard; generalizes deriveUserPeerId to honor the flag. Co-authored-by: Rui Rei <rui.rei@enline-transmission.com> Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
adavyas
force-pushed
the
fix/peer-id-scheme-legacy-safe
branch
from
6524562 to
3db6d5c
Compare
adavyas
changed the title
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/tui.ts (1)
142-151:⚠️ Potential issue | 🟠 Major | ⚡ Quick winBoolean field editing can persist
removeUserPrefixas a string when the field is initially missing.At Line 147 and Line 155, boolean handling depends on
typeof currentValue === "boolean". For upgrade configs wherehosts.opencode.removeUserPrefixis absent,currentValueisundefined, so Line 165 stores raw text ("true"/"false") instead of a boolean. That breaks the config contract and can misroute peer derivation downstream.Proposed fix
+const BOOLEAN_FIELD_PATHS = new Set([ + "hosts.opencode.removeUserPrefix", +]) + const sharedConfigPresetOptions = (fieldPath: string, currentValue: unknown) => { const presetKey = fieldPath.split(".").at(-1)?.toLowerCase() || fieldPath.toLowerCase() if (SHARED_CONFIG_PRESETS[presetKey]) { return [...SHARED_CONFIG_PRESETS[presetKey]] } - if (typeof currentValue === "boolean") { + if (typeof currentValue === "boolean" || BOOLEAN_FIELD_PATHS.has(fieldPath)) { return ["true", "false"] } return [] } -const parseSharedConfigValue = (currentValue: unknown, rawValue: string) => { +const parseSharedConfigValue = (fieldPath: string, currentValue: unknown, rawValue: string) => { const trimmed = rawValue.trim() - if (typeof currentValue === "boolean") { - return trimmed.toLowerCase() === "true" + if (typeof currentValue === "boolean" || BOOLEAN_FIELD_PATHS.has(fieldPath)) { + const normalized = trimmed.toLowerCase() + if (normalized === "true") return true + if (normalized === "false") return false + throw new Error(`Expected 'true' or 'false' for ${fieldPath}, received '${rawValue}'.`) } if (typeof currentValue === "number") { const parsed = Number(trimmed) if (!Number.isFinite(parsed)) { throw new Error(`Expected a number for this field, received '${rawValue}'.`) @@ const persistValue = async (rawValue: string) => { try { const nextConfig = structuredClone(config) - const nextValue = parseSharedConfigValue(currentValue, rawValue) + const nextValue = parseSharedConfigValue(fieldPath, currentValue, rawValue) setNestedValue(nextConfig, fieldPath, nextValue) const configPath = await writeSharedConfig(nextConfig)Also applies to: 153-166, 416-421
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/tui.ts` around lines 142 - 151, The sharedConfigPresetOptions function returns boolean string options ("true", "false") only when currentValue is already a boolean type, but fails to handle the case where currentValue is undefined for missing config fields. This causes raw text strings to be persisted instead of proper booleans, breaking the config contract. Modify the boolean check in sharedConfigPresetOptions to also return the boolean options when currentValue is undefined or missing, treating such fields as potentially boolean fields. Additionally, ensure that the same fix is applied at the other affected locations where boolean field values are being set or processed, so that undefined values for boolean fields are consistently handled and converted to actual booleans rather than string representations.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@src/tui.ts`:
- Around line 142-151: The sharedConfigPresetOptions function returns boolean
string options ("true", "false") only when currentValue is already a boolean
type, but fails to handle the case where currentValue is undefined for missing
config fields. This causes raw text strings to be persisted instead of proper
booleans, breaking the config contract. Modify the boolean check in
sharedConfigPresetOptions to also return the boolean options when currentValue
is undefined or missing, treating such fields as potentially boolean fields.
Additionally, ensure that the same fix is applied at the other affected
locations where boolean field values are being set or processed, so that
undefined values for boolean fields are consistently handled and converted to
actual booleans rather than string representations.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 229b7d35-4c36-4b4c-8887-15eecec60e6d
📒 Files selected for processing (6)
src/index.tssrc/tui.tstests/context-injection.test.jstests/honcho-setup.test.jstests/tui-behavior.test.jstests/user-peer-id.test.js
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The TUI value parser keyed "is this boolean?" off the runtime type of the current value. For upgrade configs where hosts.opencode.removeUserPrefix is absent, currentValue is undefined, so editing it persisted the raw string "true"/"false" instead of a boolean — and since "false" is a truthy non-empty string, deriveUserPeerId would route to the bare peer, the opposite of intended. Decide boolean-ness from the field path (BOOLEAN_FIELD_KEYS) so both the true/false options and the coercion are correct even when the field is missing. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Two real hardening fixes from code review (cosmetic findings dropped): - applyRawLayer coerces BOOLEAN_KEYS at the merge chokepoint, so a config string "false" is no longer truthy. The README invites hand-editing, and a stray string "false" would otherwise drop the user- prefix and orphan a user's memory. - deriveRuntimeHandle auto-resolves a bare/agent peer-id collision (peerName == aiPeer) by falling back to the prefixed form instead of throwing on the hot path, which runs in unguarded hooks. assertDistinct stays as a last resort. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/tui.ts (1)
277-287:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPreserve string-encoded
removeUserPrefixsemantics during setup saves.
saveSettingsonly preservesremoveUserPrefixwhen the existing value is already a boolean. If legacy/bad data contains"true"(string), a later setup write silently resets it tofalse, reverting peer derivation to prefixed IDs and splitting memory again.Suggested patch
+const coerceBoolean = (value: unknown): boolean | undefined => { + if (typeof value === "boolean") return value + if (typeof value === "string") { + const normalized = value.trim().toLowerCase() + if (normalized === "true") return true + if (normalized === "false") return false + } + return undefined +} + const saveSettings = async (partial: Partial<GlobalSettings>) => { @@ - const existingRemoveUserPrefix = current.hosts?.opencode?.removeUserPrefix + const existingRemoveUserPrefix = coerceBoolean(current.hosts?.opencode?.removeUserPrefix) @@ removeUserPrefix: partialHost?.removeUserPrefix ?? - (typeof existingRemoveUserPrefix === "boolean" ? existingRemoveUserPrefix : !existingSharedConfig), + (existingRemoveUserPrefix ?? !existingSharedConfig), }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/tui.ts` around lines 277 - 287, The removeUserPrefix preservation logic in the opencode host configuration only recognizes actual boolean values, causing string-encoded booleans like "true" to be silently lost and reset to false on subsequent saves. Update the condition that checks typeof existingRemoveUserPrefix === "boolean" to also handle string-encoded boolean values such as "true" and "false" strings, preserving their semantic meaning instead of falling back to !existingSharedConfig. This ensures legacy data with string-encoded booleans maintains its configuration intent across setup saves.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@src/tui.ts`:
- Around line 277-287: The removeUserPrefix preservation logic in the opencode
host configuration only recognizes actual boolean values, causing string-encoded
booleans like "true" to be silently lost and reset to false on subsequent saves.
Update the condition that checks typeof existingRemoveUserPrefix === "boolean"
to also handle string-encoded boolean values such as "true" and "false" strings,
preserving their semantic meaning instead of falling back to
!existingSharedConfig. This ensures legacy data with string-encoded booleans
maintains its configuration intent across setup saves.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 0abd4adc-4b64-4568-8957-450e535a83d8
📒 Files selected for processing (5)
README.mdsrc/index.tssrc/tui.tstests/honcho-setup.test.jstests/tui-behavior.test.js
✅ Files skipped from review due to trivial changes (1)
- README.md
🚧 Files skipped from review as they are similar to previous changes (1)
- src/index.ts
removeUserPrefix is now managed purely through the shared config file (and the runtime new-install stamp / honcho_set_config), not the native TUI editor. Revert src/tui.ts and tests/tui-behavior.test.js to their pre-feature state. The TUI's saveSettings still preserves the key on existing configs via the currentOpenCodeHost spread, so configs that set it aren't disturbed when other settings are edited. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This was referenced Jun 17, 2026
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.
Closes #22. Builds on and supersedes #23 by @rrei.
Problem
The user peer was derived as
user:${peerName}(normalized touser-<peerName>), whereas the sibling claude-honcho / hermes-honcho plugins use the barepeerName. In a shared workspace this splits the user's memory into a separateuser-<peerName>collection.#23 fixed this by dropping the prefix unconditionally — which matches the sibling plugins, but silently re-points every existing user at a brand-new, empty peer, orphaning the memory they've already accumulated under
user-<peerName>.Approach
Gate the prefix on a boolean under the opencode host block:
{ "hosts": { "opencode": { "removeUserPrefix": true } } }removeUserPrefix: trueat config creation (runtime + TUI) → bare<peerName>, matching the sibling plugins.false→ keepuser-<peerName>and all of its memory. The config file is never mutated on upgrade.HOST_SETTING_FIELDS/BOOLEAN_KEYS/SETTING_FIELD_PATHS/ TUI editable fields), so anyone can opt in via the file,/honcho:set-config removeUserPrefix true, or the TUI toggle./honcho:statusreports the effective value.The default direction is fail-safe: anything ambiguous resolves to keeping the existing
user-<name>peer, so no upgrade ever orphans memory.Credit
Both of @rrei's commits from #23 are cherry-picked here verbatim (authorship preserved), including:
deriveUserPeerIdderivation (generalized here to honor the flag)assertDistinctUserAndAgentPeers) and its testThe commit on top adds the
removeUserPrefixplumbing, the fresh-install stamp, and tests.Testing
tsc --noEmitclean.bun test— 61 passing, including:deriveUserPeerIdboth branches, fresh install →true/alice(persisted), and upgrade →false/user-alicewith the existing config left unmodified.Summary by CodeRabbit
New Features
hosts.opencode.removeUserPrefixto control how user peer IDs are derived.Bug Fixes
Documentation
removeUserPrefix.Tests