feat: add before_identity_resolve plugin hook#50875
Open
khshah6 wants to merge 4 commits intoopenclaw:mainfrom
Open
feat: add before_identity_resolve plugin hook#50875khshah6 wants to merge 4 commits intoopenclaw:mainfrom
khshah6 wants to merge 4 commits intoopenclaw:mainfrom
Conversation
Contributor
Greptile SummaryThis PR adds a Issues found:
Confidence Score: 3/5
Prompt To Fix All With AIThis is a comment left during a code review.
Path: src/routing/session-key.ts
Line: 118-121
Comment:
**Agent-ID normalization silently mangles email-format canonical peer IDs**
`normalizeResolvedPeerId` delegates to `normalizeAgentId`, which replaces every character outside `[a-z0-9_-]` with `-`. This is correct for agent identifiers that must be shell/path-safe, but canonical peer IDs returned by LDAP, SSO, or identity-API plugins are commonly email addresses or other richly-formatted strings.
Concrete collision scenario:
- Plugin A returns `john.doe@acme.com` → normalized to `john-doe-acme-com`
- Plugin B (different user, different domain) returns `john-doe@acme-com` → also `john-doe-acme-com`
These two distinct identities silently share the same session key.
The PR description explicitly lists "LDAP, SSO, identity APIs" as sources, yet those sources regularly return values like `john.doe@example.com`, `cn=John Doe,dc=example,dc=com`, or `john+tag@company.io` that all collapse through this transform.
The function should either:
- Reject non-alphanumeric IDs and require plugins to emit pre-sanitized slugs (with a clear doc contract and an error/warning path), or
- Use a dedicated, lossless encoding (e.g., `encodeURIComponent`-style or hex-encode the value) so no two distinct inputs can produce the same output.
```typescript
// option: encode special chars instead of collapsing them
function normalizeResolvedPeerId(value: string | undefined | null): string | null {
const trimmed = (value ?? "").trim();
if (!trimmed) return null;
// Replace only characters unsafe in session key paths, preserving uniqueness
const encoded = trimmed.toLowerCase().replace(/[^a-z0-9_@.+-]/g, "-");
return encoded || null;
}
```
How can I resolve this? If you propose a fix, please make it concise.
---
This is a comment left during a code review.
Path: src/routing/session-key.ts
Line: 118-121
Comment:
**Canonical peer ID `"main"` is silently dropped**
`normalizeResolvedPeerId` returns `null` whenever `normalizeAgentId` returns `DEFAULT_AGENT_ID` (`"main"`). Because `normalizeAgentId` lowercases the input, any plugin returning the string `"main"` or `"MAIN"` as a canonical identity will have it quietly discarded, and routing will fall back to `identityLinks` with no error or log message.
While `"main"` as a canonical user identity is uncommon, this sentinel check introduces an implicit reserved word that isn't visible to plugin authors. The condition should test for an empty-after-cleaning string rather than the `DEFAULT_AGENT_ID` constant:
```suggestion
function normalizeResolvedPeerId(value: string | undefined | null): string | null {
const trimmed = (value ?? "").trim();
if (!trimmed) return null;
const normalized = normalizeAgentId(trimmed);
// Only reject if the sanitizer returned the fallback because the input was empty/all-invalid.
// Compare against the trimmed input, not the constant, so "main" is preserved.
return normalized === DEFAULT_AGENT_ID && trimmed.toLowerCase() !== DEFAULT_AGENT_ID
? null
: normalized;
}
```
How can I resolve this? If you propose a fix, please make it concise.Last reviewed commit: "feature: before_iden..." |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1a9a651567
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
…edPeerId normalizeResolvedPeerId previously delegated to normalizeAgentId, which strips characters outside [a-z0-9_-]. This silently mangled email addresses and other richly-formatted identifiers returned by identity plugins (e.g., john.doe@acme.com → john-doe-acme-com), causing potential session key collisions between distinct users. Now applies trim + lowercase only, matching how identityLinks canonical keys are handled. Also fixes "main" being silently discarded as a valid canonical peer ID. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1121ac784b
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
20 tasks
Contributor
|
Codex review: found issues before merge. Summary Reproducibility: yes. Current main does not contain the requested hook/helper/resolved-peer surface, and the PR blockers are source-reproducible by comparing the diff with current Next step before merge Security Review findings
Review detailsBest possible solution: Land the feature only after a maintainer-approved hook contract lives in Do we have a high-confidence way to reproduce the issue? Yes. Current main does not contain the requested hook/helper/resolved-peer surface, and the PR blockers are source-reproducible by comparing the diff with current Is this the best way to solve the issue? No. The additive hook direction is plausible, but this branch is not the best current solution until it rebases onto the active hook contract, handles first-valid results and secondary routes, and documents the DM isolation trust model. Full review comments:
Overall correctness: patch is incorrect Security concerns:
What I checked:
Likely related people:
Remaining risk / open question:
Codex review notes: model gpt-5.5, reasoning high; reviewed against ea75cd897182. |
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
session.identityLinksconfig map. Can't integrate with external identity services or resolve dynamically.before_identity_resolveplugin hook that lets plugins resolve channel-specific peer IDs (phone numbers, Slack user IDs) to canonical identities before session key construction. Integrated into Slack (DMs + slash commands), MS Teams, and WhatsApp. AddedresolveInboundPeerIdentityhelper in plugin-sdk for channel use. Updated docs.resolveAgentRoutestays synchronous. Group/channel peers are not affected. Slack system event routing (context.ts) is deferred — sync function with ~15 sync callers.Change Type (select all)
Scope (select all touched areas)
Linked Issue/PR
User-visible / Behavior Changes
before_identity_resolveavailable forapi.on()registrationsession.identityLinksconfigidentityLinkscontinues to work as fallback when no plugin is registered or plugin returns undefinedSecurity Impact (required)
NoNoNo(plugins may make network calls, but that's plugin-authored code)NoNoRepro + Verification
Environment
Steps
before_identity_resolvehook in a plugin that maps phone numbers to canonical IDssession.dmScope: "per-peer"with existingidentityLinksExpected
agent:main:direct:cann-kanchi)identityLinks(which would produceagent:main:direct:kanchi)Actual
agent:main:direct:cann-kanchi(plugin-resolved) instead ofagent:main:direct:kanchi(identityLinks fallback)Evidence
Test results:
pnpm check: pass (0 warnings, 0 errors)pnpm test:contracts: 125 passpnpm test:extension slack: 463 passpnpm test:extension msteams: 229 passpnpm test:extension whatsapp: 271 passHuman Verification (required)
pnpm checkcleanidentityLinks; email-format IDs preserved verbatim;"main"as canonical ID preserved (not discarded)Review Conversations
Addressed P1s (normalization mangling + "main" dropped) in follow-up commit. Acknowledged P2s (hook short-circuiting, Slack system events) as follow-ups with rationale.
Compatibility / Migration
YesNoNoresolveAgentRouteremains synchronous — 38 callers + 12 type references unchangedresolvedPeerIdis an optional additive parameter — existing callers unaffectedidentityLinksas beforeFailure Recovery (if this breaks)
api.on("before_identity_resolve", ...)registration, or disable the plugin viaplugins.entries.<id>.enabled: falseidentityLinkscontinues to work as fallbackidentityLinks(would create new sessions instead of continuing existing ones)Risks and Mitigations
identityLinkscanonical keys, causing session splitting for existing usersidentityLinkskeys should use the same canonical IDs. Deploy plugin alongsideidentityLinksfirst, verify session keys match, then removeidentityLinks.resolveInboundPeerIdentitywraps hook in try/catch (fail-open). Plugin authors should useAbortSignal.timeout()and local caching. Docs include caching example.🤖 Generated with Claude Code
AI-assisted: Yes — design, implementation, and testing done with Claude Code. We understand what the code does.