fix: use bare peerName for the user peer to match sibling plugins#23
fix: use bare peerName for the user peer to match sibling plugins#23rrei wants to merge 2 commits into
Conversation
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.
|
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 (2)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughDerives a normalized user peer ID from ChangesUser Peer ID Normalization
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 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 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
🤖 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 760-762: The code can produce identical IDs for userPeerId and
rootAgentPeerId after normalization (deriveUserPeerId and normalizeId), causing
sessionPeerConfigs keys to collide; update the logic that sets
userPeerId/rootAgentPeerId (or inside deriveUserPeerId) to detect equality after
normalization and disambiguate (for example by adding a stable prefix/suffix
like "user-" or "agent-" or falling back to a unique variant) so userPeerId !==
rootAgentPeerId, and ensure activeAgentPeerId still references the agent id
(rootAgentPeerId) while any downstream usage (e.g., sessionPeerConfigs) uses the
disambiguated ids.
🪄 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: 12b55323-eb58-4354-a239-147d28826bfb
📒 Files selected for processing (4)
src/index.tstests/context-injection.test.jstests/peer-topology.test.jstests/user-peer-id.test.js
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.
|
hey @rrei ! apologies for the late review on this PR. thank you for your work on this. in order for this to not break with the legacy plugin ecosystem we've pushed out for the opencode-honcho plugin, we'd like to execute something like peerNamingStrategy: "prefixed" | "bare": new installs get seeded with "bare" in ensureSharedGlobalSettings → new default, matches the sibling plugins. this is sort of an opt-in system for existing users while shifting new installations to the new peerName. |
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>
Hey @adavyas, thanks for the review! Good point on the backward compatibility. 👍 It didn't even cross my mind, sorry about that! 😅 |
* fix: use bare peerName for the user peer to match sibling plugins
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.
* fix: reject configs where the user and agent peers collide
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.
* feat: gate user-peer prefix on hosts.opencode.removeUserPrefix
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>
* docs: mention removeUserPrefix in the config example
* fix: coerce removeUserPrefix to boolean when editing an absent field
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.
* fix: coerce removeUserPrefix at read time and soften peer collision
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.
* fix: drop TUI surface for removeUserPrefix, keep it config-only
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: Rui Rei <rui.rei@enline-transmission.com>
|
yes! this issue has been addressed and code was salvaged for PR #24 . will update the opencode plugin shortly. thank you once again for your work and raising this issue. |
2 participants
Closes #22.
The user peer was derived as
user:${peerName}(normalised touser-<peerName>), whereas the claude-honcho and hermes-honcho plugins use the barepeerName. In a shared workspace this split the user's memory into a separateuser-<peerName>collection, so OpenCode never saw or contributed to what the other tools learned. This drops the prefix so the user peer matches the sibling plugins.Added a regression test for the derivation and updated two fixtures that encoded the old convention.
bun test(58 passing) andtsc --noEmitare green.Summary by CodeRabbit