chore: promote dev trash reset fix to homolog#702
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ 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 |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a centralized session identity resolution mechanism via resolveKhalSessionId to handle canonical KHAL session IDs across dispatching and session cleaning. It also updates the AgnoClient to return structured deletion results and includes comprehensive unit tests. The review feedback highlights opportunities to simplify the session cleaning logic by removing a redundant payload check and to improve consistency in resolveKhalSessionId by normalizing the personId handling at the start of the function.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| rawPayload: options.rawPayload, | ||
| }); | ||
| const { sessionId, legacySessionId } = identity; | ||
| const hasKhalContext = !!identity.canonicalSessionId || !!identity.environment || !!options.rawPayload?.khalSessionId; |
There was a problem hiding this comment.
The check !!options.rawPayload?.khalSessionId is redundant. If options.rawPayload contains a KHAL session ID, resolveKhalSessionId will extract it and set identity.canonicalSessionId (making !!identity.canonicalSessionId true) and set identity.source to 'explicit-khal'. Since identity.source is 'explicit-khal', the condition identity.source === 'legacy' on line 122 will be false, and the if block will not be entered. Thus, !!options.rawPayload?.khalSessionId has no effect on the logic and can be safely removed.
| const hasKhalContext = !!identity.canonicalSessionId || !!identity.environment || !!options.rawPayload?.khalSessionId; | |
| const hasKhalContext = !!identity.canonicalSessionId || !!identity.environment; |
| export function resolveKhalSessionId(input: KhalSessionIdentityInput): ResolvedAgentSessionIdentity { | ||
| const sessionStrategy = input.sessionStrategy ?? 'per_chat'; | ||
| const legacySessionId = computeLegacySessionId(sessionStrategy, input.from, input.chatId, input.threadId); | ||
| const explicitKhalSessionId = extractKhalSessionIdFromRawPayload(input.rawPayload); | ||
| if (explicitKhalSessionId) { | ||
| return { | ||
| sessionId: explicitKhalSessionId, | ||
| legacySessionId, | ||
| sessionStrategy, | ||
| canonicalSessionId: explicitKhalSessionId, | ||
| personId: input.personId ?? undefined, | ||
| environment: resolveKhalEnvironment(input.rawPayload, input.environment), | ||
| channelSegment: channelToKhalSessionSegment(input.channel), | ||
| source: 'explicit-khal', | ||
| }; | ||
| } | ||
|
|
||
| const environment = resolveKhalEnvironment(input.rawPayload, input.environment); | ||
| const channelSegment = channelToKhalSessionSegment(input.channel); | ||
| const personId = input.personId?.trim(); | ||
|
|
||
| if (input.providerSchema === 'agno' && environment && channelSegment && personId) { | ||
| const canonicalSessionId = buildCanonicalKhalSessionId({ | ||
| environment, | ||
| instanceId: input.instanceId, | ||
| channelSegment, | ||
| personId, | ||
| }); | ||
| return { | ||
| sessionId: canonicalSessionId, | ||
| legacySessionId, | ||
| sessionStrategy, | ||
| canonicalSessionId, | ||
| personId, | ||
| environment, | ||
| channelSegment, | ||
| source: 'canonical-khal', | ||
| }; | ||
| } | ||
|
|
||
| return { | ||
| sessionId: legacySessionId, | ||
| legacySessionId, | ||
| sessionStrategy, | ||
| personId: personId ?? undefined, | ||
| environment, | ||
| channelSegment, | ||
| source: 'legacy', | ||
| }; | ||
| } |
There was a problem hiding this comment.
The handling of personId is inconsistent and duplicated. It is trimmed in some places, left untrimmed in others, and empty strings are not consistently normalized to undefined (e.g., personId ?? undefined on line 154 keeps "" as ""). Defining personId once at the top of the function as input.personId?.trim() || undefined simplifies the logic, ensures consistency, and allows using shorthand property names.
export function resolveKhalSessionId(input: KhalSessionIdentityInput): ResolvedAgentSessionIdentity {
const sessionStrategy = input.sessionStrategy ?? 'per_chat';
const legacySessionId = computeLegacySessionId(sessionStrategy, input.from, input.chatId, input.threadId);
const personId = input.personId?.trim() || undefined;
const explicitKhalSessionId = extractKhalSessionIdFromRawPayload(input.rawPayload);
if (explicitKhalSessionId) {
return {
sessionId: explicitKhalSessionId,
legacySessionId,
sessionStrategy,
canonicalSessionId: explicitKhalSessionId,
personId,
environment: resolveKhalEnvironment(input.rawPayload, input.environment),
channelSegment: channelToKhalSessionSegment(input.channel),
source: 'explicit-khal',
};
}
const environment = resolveKhalEnvironment(input.rawPayload, input.environment);
const channelSegment = channelToKhalSessionSegment(input.channel);
if (input.providerSchema === 'agno' && environment && channelSegment && personId) {
const canonicalSessionId = buildCanonicalKhalSessionId({
environment,
instanceId: input.instanceId,
channelSegment,
personId,
});
return {
sessionId: canonicalSessionId,
legacySessionId,
sessionStrategy,
canonicalSessionId,
personId,
environment,
channelSegment,
source: 'canonical-khal',
};
}
return {
sessionId: legacySessionId,
legacySessionId,
sessionStrategy,
personId,
environment,
channelSegment,
source: 'legacy',
};
}
namastex888
force-pushed
the
promote/dev-to-homolog-trash-reset-20260603
branch
from
a34c155 to
1cf51a3
Compare
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a34c1551a6
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| readHeader(rawPayload?.headers, 'x-khal-environment') ?? | ||
| process.env.KHAL_SESSION_ENV?.trim() ?? | ||
| process.env.KHAL_ENV?.trim() ?? | ||
| process.env.OMNI_ENV?.trim() |
There was a problem hiding this comment.
When OMNI_ENV is set in a normal deployment, every Agno dispatch that has a resolved personId now satisfies environment in resolveKhalSessionId and is moved from the configured legacy strategy (for example per_chat using chatId) to khal:<env>:omni:<instance>:<channel>:<person>. That silently forks session history for any existing non-KHAL Agno provider, and if personId cannot be resolved the session cleaner also fails closed instead of deleting the legacy Agno session. Please only use KHAL-specific signals here (explicit/header/config), not the generic app environment.
Useful? React with 👍 / 👎.
2 participants
Summary
devintohomologafter the canonical KHAL trash reset/session identity fix landed green on dev.2.260603.2package/plugin metadata.Validation
bun install --frozen-lockfilebunx knipbunx biome check packages/api/src/services/agent-session-identity.ts packages/api/src/services/__tests__/agent-session-identity.test.ts packages/api/src/plugins/session-cleaner.ts packages/api/src/plugins/__tests__/session-cleaner.test.ts(cd packages/api && bun test src/services/__tests__/agent-session-identity.test.ts src/plugins/__tests__/session-cleaner.test.ts)(cd packages/api && bun run typecheck)HML follow-up gate
After merge, deploy/verify HML reset and memory purge against homolog runtime. Do not claim HML GO until live HML evidence proves canonical KHAL session cleanup.