Skip to content

fix: support canonical and client session detail identities - #1382

Merged
ding113 merged 5 commits into
devfrom
fix/session-detail-identity-ordering
Aug 2, 2026
Merged

fix: support canonical and client session detail identities#1382
ding113 merged 5 commits into
devfrom
fix/session-detail-identity-ordering

Conversation

@ding113

@ding113 ding113 commented Aug 2, 2026

Copy link
Copy Markdown
Owner

Summary

  • Support both canonical Prefix session identity and physical/client session ID when opening session detail and message views.
  • Keep canonical identity as the route path while preserving sourceSessionId, requestSequence, and requestId selectors for precise request navigation.
  • Sort session request lists newest-first by default and add displaySequence fallback logic for Prefix affinity timelines.
  • Show canonical/client session IDs in session detail and log detail UI, and add missing i18n/error mappings.

TDD Coverage

  • Repository seam: dual identity lookup, request ordering, displaySequence fallback, and selector preservation.
  • Action/API seam: getSessionDetails/getSessionRequests/hasSessionMessages support both IDs and requestId forwarding.
  • Frontend seam: session detail header, request list ordering/sequence display, log detail link rendering, and i18n error messages.
  • Locale/schema seam: five dashboard locale files include the new keys and generated OpenAPI types are current.

Verification

  • bunx vitest run tests/unit/repository/message-session-request-query.test.ts tests/unit/actions/active-sessions-requests.test.ts tests/unit/actions/active-sessions-detail-snapshots.test.ts tests/unit/actions/active-sessions-special-settings.test.ts tests/unit/actions/active-sessions-termination.test.ts tests/unit/api/v1/api-client-actions.test.ts tests/unit/frontend/api-error-i18n.test.ts src/app/[locale]/dashboard/logs/_components/error-details-dialog.test.tsx src/app/[locale]/dashboard/sessions/[sessionId]/messages/_components/session-messages-client-actions.test.tsx src/app/[locale]/dashboard/sessions/[sessionId]/messages/_components/request-list-sidebar.test.tsx
  • bun run lint
  • bun run lint:fix
  • bun run typecheck
  • bun scripts/generate-v1-types.ts --check
  • bun run build
  • bun run test
  • git diff --check

Notes: build emits existing Edge Runtime warnings for instrumentation and Node-only modules, but exits successfully.

Greptile Summary

The PR updates session-detail navigation to preserve canonical and physical identities while consistently carrying request selectors, and replaces per-physical-session Prefix labels with timeline-wide numbering.

  • Scopes session detail and request repository lookups by the authenticated owner.
  • Preserves canonical route identity alongside client session ID, request ID, and request sequence.
  • Uses chronological row_number() values for Prefix-affinity timeline labels.
  • Updates dashboard rendering, API contracts, generated types, tests, localization, and error mappings.

Confidence Score: 5/5

The PR appears safe to merge.

No blocking failure remains; current owner-scoped predicates prevent the reported detail-query identity collision, and Prefix timelines now use a single chronological row number rather than independent physical-session sequences.

Important Files Changed

Filename Overview
src/repository/message.ts Adds owner-scoped canonical/physical identity predicates and timeline-wide Prefix display sequencing; both previously reported defects are addressed.
src/actions/active-sessions.ts Propagates authenticated ownership and stable request selectors through session detail, request-list, and navigation flows.
src/lib/session-request-locator.ts Resolves canonical and physical request identities while retaining request-specific selectors.
src/app/[locale]/dashboard/sessions/[sessionId]/messages/_components/request-list-sidebar.tsx Displays timeline-wide sequence labels while keeping navigation anchored to request ID, physical session ID, and request sequence.
src/app/api/v1/resources/sessions/handlers.ts Forwards the expanded session selectors through the management API contract.

Sequence Diagram

sequenceDiagram
  participant UI as Session Detail UI
  participant Action as Session Actions
  participant Repo as Message Repository
  participant DB as PostgreSQL
  UI->>Action: canonical ID + request selectors
  Action->>Repo: resolve locator with owner ID
  Repo->>DB: owner-scoped canonical/physical lookup
  DB-->>Repo: request ID, physical ID, canonical ID
  Repo-->>Action: request locator
  Action->>Repo: load owner-scoped timeline
  Repo->>DB: chronological Prefix query
  DB-->>Repo: rows with timeline-wide displaySequence
  Repo-->>UI: details and newest-first request list
Loading

Reviews (6): Last reviewed commit: "fix(message): match legacy null-identity..." | Re-trigger Greptile

Context used (3)

@gemini-code-assist

Copy link
Copy Markdown
Contributor

Caution

The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased.

@coderabbitai

coderabbitai Bot commented Aug 2, 2026

Copy link
Copy Markdown

Review Change Stack

Note

Reviews paused

It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

  • @coderabbitai resume to resume automatic reviews.
  • @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

  • ▶️ Resume reviews
  • 🔍 Trigger review
📝 Walkthrough

Walkthrough

本次变更统一 canonical Session ID、物理 Session ID、request ID、owner scope 和 key ownership。会话详情、请求列表、日志对话框、API、缓存及代理持久化流程已同步更新。测试覆盖请求定位、归属校验、终止和多语言文案。

Changes

Canonical Session 流程

Layer / File(s) Summary
Canonical identity 与请求归属
src/repository/message.ts, src/actions/active-sessions.ts, src/lib/session-request-locator.ts
会话查询按 canonical identity 和 owner 过滤。请求定位、审计查询、详情读取和来源链查询改用 request ID、key ID 与用户归属。
Redis 请求状态与代理写入
src/lib/session-manager.ts, src/app/v1/_lib/proxy/*
请求序号使用 Lua 原子递增。Redis 保存请求 owner key ID 和 TTL。响应、快照、响应头及上游元数据写入时刷新请求归属。
缓存、API 与请求列表
src/lib/cache/session-cache.ts, src/lib/api/v1/schemas/sessions.ts, src/app/api/v1/resources/sessions/*, src/app/[locale]/dashboard/sessions/[sessionId]/messages/_components/*
会话详情缓存增加 owner 隔离和物理别名管理。消息存在性接口支持 requestId。请求列表默认按降序返回,并显示 displaySequence
日志详情与会话标识显示
src/app/[locale]/dashboard/logs/_components/*
日志记录向详情对话框传递 request ID。消息检查和会话链接使用 request ID。详情页面显示 canonical Session ID,并在值不同时显示 client Session ID。
测试与本地化文案
tests/**/*, messages/{en,ja,ru,zh-CN,zh-TW}/dashboard.json
测试覆盖请求定位、请求归属、owner scope、分页排序、来源链、终止、缓存清理、错误映射和五种语言字段。新增 Session ID、请求标题、未知模型和详情加载失败文案。

Estimated code review effort: 5 (Critical) | ~90 minutes

Possibly related PRs

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 35.29% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Title check ✅ Passed 标题准确概括了本次变更的核心内容,即支持规范会话标识和客户端会话标识。
Description check ✅ Passed 描述与变更内容相关,涵盖双重会话标识、请求定位、排序、界面文案和测试验证。
✨ Finishing Touches 💡 1
📝 Generate docstrings 💡
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix/session-detail-identity-ordering

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

Comment thread src/repository/message.ts Outdated
options?: { limit?: number; offset?: number; order?: "asc" | "desc" }
): Promise<Awaited<ReturnType<typeof findRequestsBySessionId>>> {
const { limit = 20, offset = 0, order = "asc" } = options || {};
const { limit = 20, offset = 0, order = "desc" } = options || {};

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 security Cross-owner identity collision

If a physical session ID equals another user's canonical Prefix identity, messageSessionLookup matches both sets of rows after the one-time ownership check, causing request lists, navigation, and stored message details to disclose the other user's session data.

How this was verified: The authorized canonical value reaches repository queries whose new predicate also matches physical session IDs, and those queries contain no user filter.

Knowledge Base Used:

Prompt To Fix With AI
This is a comment left during a code review.
Path: src/repository/message.ts
Line: 2173

Comment:
**Cross-owner identity collision**

If a physical session ID equals another user's canonical Prefix identity, `messageSessionLookup` matches both sets of rows after the one-time ownership check, causing request lists, navigation, and stored message details to disclose the other user's session data.

**How this was verified:** The authorized canonical value reaches repository queries whose new predicate also matches physical session IDs, and those queries contain no user filter.

**Knowledge Base Used:**
- [Database Schema & Repository Layer](https://app.greptile.com/ygxz/-/custom-context/knowledge-base/ding113/claude-code-hub/-/docs/database-schema.md)
- [Redis Caching and Session Tracking](https://app.greptile.com/ygxz/-/custom-context/knowledge-base/ding113/claude-code-hub/-/docs/redis-caching-and-sessions.md)

---

For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.

Comment thread src/repository/message.ts
Comment on lines 2188 to +2201
sequence: messageRequest.requestSequence,
displaySequence: sql<number>`CASE
WHEN ${messageRequest.sessionIdentityKind} = 'prefix_affinity'
AND NOT bool_or(COALESCE(${messageRequest.requestSequence}, 1) <> 1) OVER ()
THEN COALESCE(
NULLIF(jsonb_array_length(${messageRequest.affinityFingerprintChain}), 0),
row_number() OVER (ORDER BY ${messageRequest.createdAt} ASC, ${messageRequest.id} ASC)::int
)
ELSE COALESCE(
${messageRequest.requestSequence},
NULLIF(jsonb_array_length(${messageRequest.affinityFingerprintChain}), 0),
row_number() OVER (ORDER BY ${messageRequest.createdAt} ASC, ${messageRequest.id} ASC)::int
)
END`,

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Duplicate Prefix timeline numbering

When a Prefix-affinity timeline contains multiple physical sessions and any session has a request sequence above one, the window-wide branch makes displaySequence reuse each physical session's independent sequence, causing duplicate and non-monotonic labels such as #1, #1, #2, #2.

Knowledge Base Used:

Prompt To Fix With AI
This is a comment left during a code review.
Path: src/repository/message.ts
Line: 2188-2201

Comment:
**Duplicate Prefix timeline numbering**

When a Prefix-affinity timeline contains multiple physical sessions and any session has a request sequence above one, the window-wide branch makes `displaySequence` reuse each physical session's independent sequence, causing duplicate and non-monotonic labels such as `#1, #1, #2, #2`.

**Knowledge Base Used:**
- [Dashboard UI (Next.js App Router)](https://app.greptile.com/ygxz/-/custom-context/knowledge-base/ding113/claude-code-hub/-/docs/dashboard-ui.md)
- [Redis Caching and Session Tracking](https://app.greptile.com/ygxz/-/custom-context/knowledge-base/ding113/claude-code-hub/-/docs/redis-caching-and-sessions.md)

---

For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 5

🧹 Nitpick comments (1)
src/app/[locale]/dashboard/sessions/[sessionId]/messages/_components/request-list-sidebar.test.tsx (1)

8-8: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

使用 @/ 导入路径。

Line 8 使用了相对导入。将 ./request-list-sidebar 替换为从 src/ 开始的 @/app/[locale]/dashboard/sessions/[sessionId]/messages/_components/request-list-sidebar

As per coding guidelines, **/*.{ts,tsx,js,jsx} 必须使用 @/ 映射到 ./src/ 的导入路径。

🤖 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/app/`[locale]/dashboard/sessions/[sessionId]/messages/_components/request-list-sidebar.test.tsx
at line 8, Update the RequestListSidebar import in the test to use the `@/` alias
mapped to src, targeting the full
app/[locale]/dashboard/sessions/[sessionId]/messages/_components/request-list-sidebar
path instead of the relative ./request-list-sidebar path.

Source: Coding guidelines

🤖 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/actions/active-sessions.ts`:
- Around line 1413-1423: 更新终止 Session 的缓存清理逻辑,不能仅依赖
aggregateMultipleSessionStats 返回的 requestedSessionIds,因为它只包含本次输入的别名;应从已解析的
Session source 获取该 canonical Session 的全部物理 alias,或复用 canonical ID 到缓存键的反向索引,并在
sessionDetailCacheIds 中加入所有关联缓存键后统一调用 clearSessionDetailsCache。

In `@src/app/`[locale]/dashboard/logs/_components/error-details-dialog/index.tsx:
- Around line 140-167: Update the message-check flow around hasSessionMessages
to reset hasMessages to false when starting a new check and keep it false
whenever result.ok is false. In the else branch for a closed dialog or missing
sessionId, increment messageCheckRequestIdRef.current before clearing state so
pending requests cannot update it later.

In
`@src/app/`[locale]/dashboard/sessions/[sessionId]/messages/_components/request-list-sidebar.tsx:
- Around line 132-134: Update the collapsed-view title near the request list
item rendering to use the existing next-intl translation function instead of
hardcoded “#”, separator text, and “Unknown”. Add the requestList.itemTitle and
requestList.unknownModel keys for zh-CN, zh-TW, en, ja, and ru, then generate
the title through t(...) while preserving the display sequence and model
fallback.

In `@src/repository/message.ts`:
- Around line 2131-2134: 修复 src/repository/message.ts 第2131-2134行的查询逻辑:不要仅用
displaySequence 为 requestSequence IS NULL 的记录生成展示序号;应提供 SessionManager
可稳定使用的真实定位方式,或直接过滤掉这些不可导航记录。将相同规则应用于第2189-2201行的 Prefix affinity 查询,确保返回的每条请求都能被
findSessionRequestLocator 通过 sequence/requestId 成功定位。

In `@tests/unit/frontend/api-error-i18n.test.ts`:
- Around line 107-114: Extend the locale loop in the test “defines Session
detail identity and error labels in every locale” to also assert that
dashboard.sessions.details.error is truthy, alongside the existing
sessions.status.error assertion, ensuring every locale defines the
session-detail loading error label.

---

Nitpick comments:
In
`@src/app/`[locale]/dashboard/sessions/[sessionId]/messages/_components/request-list-sidebar.test.tsx:
- Line 8: Update the RequestListSidebar import in the test to use the `@/` alias
mapped to src, targeting the full
app/[locale]/dashboard/sessions/[sessionId]/messages/_components/request-list-sidebar
path instead of the relative ./request-list-sidebar path.
🪄 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 Plus

Run ID: e34945d9-471f-4334-9c53-a3b251605b27

📥 Commits

Reviewing files that changed from the base of the PR and between 5dd805e and ef7aaf1.

📒 Files selected for processing (33)
  • messages/en/dashboard.json
  • messages/ja/dashboard.json
  • messages/ru/dashboard.json
  • messages/zh-CN/dashboard.json
  • messages/zh-TW/dashboard.json
  • src/actions/active-sessions.ts
  • src/app/[locale]/dashboard/logs/_components/error-details-dialog.test.tsx
  • src/app/[locale]/dashboard/logs/_components/error-details-dialog/components/MetadataTab.tsx
  • src/app/[locale]/dashboard/logs/_components/error-details-dialog/components/SummaryTab.tsx
  • src/app/[locale]/dashboard/logs/_components/error-details-dialog/index.tsx
  • src/app/[locale]/dashboard/logs/_components/error-details-dialog/types.ts
  • src/app/[locale]/dashboard/logs/_components/usage-logs-table.tsx
  • src/app/[locale]/dashboard/logs/_components/virtualized-logs-table.tsx
  • src/app/[locale]/dashboard/sessions/[sessionId]/messages/_components/request-list-sidebar.test.tsx
  • src/app/[locale]/dashboard/sessions/[sessionId]/messages/_components/request-list-sidebar.tsx
  • src/app/[locale]/dashboard/sessions/[sessionId]/messages/_components/session-messages-client-actions.test.tsx
  • src/app/[locale]/dashboard/sessions/[sessionId]/messages/_components/session-messages-client.tsx
  • src/app/api/v1/resources/sessions/handlers.ts
  • src/app/api/v1/resources/sessions/router.ts
  • src/lib/api-client/v1/actions/active-sessions.ts
  • src/lib/api-client/v1/errors.ts
  • src/lib/api-client/v1/openapi-types.gen.ts
  • src/lib/api/v1/schemas/sessions.ts
  • src/lib/cache/session-cache.ts
  • src/repository/message.ts
  • tests/api/v1/sessions/sessions.test.ts
  • tests/unit/actions/active-sessions-detail-snapshots.test.ts
  • tests/unit/actions/active-sessions-requests.test.ts
  • tests/unit/actions/active-sessions-special-settings.test.ts
  • tests/unit/actions/active-sessions-termination.test.ts
  • tests/unit/api/v1/api-client-actions.test.ts
  • tests/unit/frontend/api-error-i18n.test.ts
  • tests/unit/repository/message-session-request-query.test.ts

Comment thread src/actions/active-sessions.ts
Comment thread src/repository/message.ts
Comment on lines +107 to +114
test("defines Session detail identity and error labels in every locale", () => {
for (const dashboard of [enDashboard, zhCNDashboard, zhTWDashboard, jaDashboard, ruDashboard]) {
expect(dashboard.sessions.status.error).toBeTruthy();
expect(dashboard.sessions.details.canonicalSessionId).toBeTruthy();
expect(dashboard.sessions.details.clientSessionId).toBeTruthy();
expect(dashboard.logs.details.metadata.canonicalSessionId).toBeTruthy();
expect(dashboard.logs.details.metadata.clientSessionId).toBeTruthy();
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win

补充会话详情错误键的五语言断言。

此测试只验证 dashboard.sessions.status.error。该键不能覆盖本次新增的会话详情加载失败文案。请同时断言 dashboard.sessions.details.error,以防任一 locale 缺少该键。

建议修改
       expect(dashboard.sessions.status.error).toBeTruthy();
+      expect(dashboard.sessions.details.error).toBeTruthy();
       expect(dashboard.sessions.details.canonicalSessionId).toBeTruthy();
📝 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
test("defines Session detail identity and error labels in every locale", () => {
for (const dashboard of [enDashboard, zhCNDashboard, zhTWDashboard, jaDashboard, ruDashboard]) {
expect(dashboard.sessions.status.error).toBeTruthy();
expect(dashboard.sessions.details.canonicalSessionId).toBeTruthy();
expect(dashboard.sessions.details.clientSessionId).toBeTruthy();
expect(dashboard.logs.details.metadata.canonicalSessionId).toBeTruthy();
expect(dashboard.logs.details.metadata.clientSessionId).toBeTruthy();
}
test("defines Session detail identity and error labels in every locale", () => {
for (const dashboard of [enDashboard, zhCNDashboard, zhTWDashboard, jaDashboard, ruDashboard]) {
expect(dashboard.sessions.status.error).toBeTruthy();
expect(dashboard.sessions.details.error).toBeTruthy();
expect(dashboard.sessions.details.canonicalSessionId).toBeTruthy();
expect(dashboard.sessions.details.clientSessionId).toBeTruthy();
expect(dashboard.logs.details.metadata.canonicalSessionId).toBeTruthy();
expect(dashboard.logs.details.metadata.clientSessionId).toBeTruthy();
}
🤖 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 `@tests/unit/frontend/api-error-i18n.test.ts` around lines 107 - 114, Extend
the locale loop in the test “defines Session detail identity and error labels in
every locale” to also assert that dashboard.sessions.details.error is truthy,
alongside the existing sessions.status.error assertion, ensuring every locale
defines the session-detail loading error label.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: ef7aaf1582

ℹ️ 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".

Comment thread src/repository/message.ts Outdated
Comment on lines +2192 to +2194
THEN COALESCE(
NULLIF(jsonb_array_length(${messageRequest.affinityFingerprintChain}), 0),
row_number() OVER (ORDER BY ${messageRequest.createdAt} ASC, ${messageRequest.id} ASC)::int

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Avoid deriving sequence from the capped fingerprint chain

For prefix-affinity conversations longer than the configured fingerprint window, this returns the same displaySequence for every later request. computeFingerprintChain truncates its tail to DEFAULT_AFFINITY_WINDOW (8 by default) in src/app/v1/_lib/proxy/affinity/fingerprint.ts, so jsonb_array_length(affinityFingerprintChain) saturates at 8 instead of continuing to represent timeline position; the sidebar consequently shows multiple distinct requests as #8. Use an uncapped ordering value such as the chronological row number when producing this display sequence.

Useful? React with 👍 / 👎.

@github-actions

github-actions Bot commented Aug 2, 2026

Copy link
Copy Markdown
Contributor

🧪 测试结果

测试类型 状态
代码质量
单元测试
集成测试
API 测试

总体结果: ✅ 所有测试通过

@github-actions github-actions Bot added the size/XL Extra Large PR (> 1000 lines) label Aug 2, 2026
Comment thread src/repository/message.ts Outdated
const { limit = 20, offset = 0, order = "desc" } = options || {};
const where = and(
eq(messageSessionIdentity, identity),
messageSessionLookup(identity),

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[CRITICAL] [SECURITY-VULNERABILITY] Canonical reads can mix physical rows across owners

Why this is a problem: The action authorizes sessionStats first and then passes the canonical sessionStats.sessionId into this query. The new messageSessionLookup predicate also matches any raw message_request.session_id. Because client-provided physical IDs are retained in that column, a physical ID from one user can equal another user’s pfx:... canonical identity; the count/list, locator, and adjacency queries then combine both owners’ rows after authorization. That can expose the other user’s request metadata and stored messages, and the same alias predicate is also used by the new identity-resolution path used for termination.

Suggested fix:

const canonicalSessionLookup = eq(messageSessionIdentity, identity);
const where = and(
  canonicalSessionLookup,
  isNotNull(messageRequest.sessionId),
  eq(messageRequest.isReplay, false),
  isNull(messageRequest.deletedAt)
);

Canonicalize a physical ID during the ownership check, then use an exact canonical-identity predicate for all subsequent reads and termination operations. Add a regression test where another user’s raw session_id equals the authorized user’s canonical identity.,

Comment thread src/repository/message.ts Outdated
WHEN ${messageRequest.sessionIdentityKind} = 'prefix_affinity'
AND NOT bool_or(COALESCE(${messageRequest.requestSequence}, 1) <> 1) OVER ()
THEN COALESCE(
NULLIF(jsonb_array_length(${messageRequest.affinityFingerprintChain}), 0),

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[MEDIUM] [LOGIC-BUG] Prefix display sequences are not unique or unbounded

Why this is a problem: When any row in a prefix-affinity timeline has requestSequence > 1, the ELSE branch returns each physical session’s independent requestSequence, so multiple physical sources can render duplicate labels such as #1, #1, #2, #2. When all sequences are 1, jsonb_array_length(affinityFingerprintChain) is capped by the configured affinity window (8 by default), so longer timelines reuse the same display number. The new sidebar therefore shows misleading sequence labels even though the request ID may still identify the row.

Suggested fix:

displaySequence: sql<number>`CASE
  WHEN ${messageRequest.sessionIdentityKind} = prefix_affinity
    THEN row_number() OVER (
      ORDER BY ${messageRequest.createdAt} ASC, ${messageRequest.id} ASC
    )::int
  ELSE COALESCE(
    ${messageRequest.requestSequence},
    row_number() OVER (
      ORDER BY ${messageRequest.createdAt} ASC, ${messageRequest.id} ASC
    )::int
  )
END`,
```,

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review Summary

This PR makes a broad, coordinated change to session identity resolution across the repository, actions, API, UI, and tests. The main risks are concentrated in the new repository lookups: one change can break tenant isolation, and another can make prefix-affinity timelines display misleading request numbers.

PR Size: XL

  • Lines changed: 535
  • Files changed: 33
  • Split suggestions: Separate the repository/action identity canonicalization work from the dashboard/i18n/test updates, and isolate the request-list ordering/display-sequence change into its own PR.

Issues Found

Category Critical High Medium Low
Logic/Bugs 0 0 1 0
Security 1 0 0 0
Error Handling 0 0 0 0
Types 0 0 0 0
Comments/Docs 0 0 0 0
Tests 0 0 0 0
Simplification 0 0 0 0

Critical Issues (Must Fix)

  • src/repository/message.ts:2175 — The widened messageSessionLookup(identity) predicate can join an authorized canonical identity with another user’s raw physical session_id, which creates a cross-owner data exposure path for request lists, adjacency, and stored-message lookups.

High Priority Issues (Should Fix)

  • src/repository/message.ts:2193displaySequence is derived from per-physical-session requestSequence or the capped fingerprint-chain length, so prefix-affinity timelines can render duplicate or saturated labels like #1, #1, #2, #2 or repeated #8 after the affinity window is exceeded.

Review Coverage

  • Logic and correctness
  • Security (OWASP Top 10)
  • Error handling
  • Type safety
  • Documentation accuracy
  • Test coverage
  • Code clarity

Automated review by Codex AI

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • Reviewed PR #1382, applied the size/XL label, and submitted the summary review on GitHub.
  • Posted 2 inline comments on new diff lines:
    • src/repository/message.ts:2175 — critical cross-owner data exposure risk from widening canonical lookups to also match raw physical session_id values.
    • src/repository/message.ts:2193 — prefix-affinity displaySequence can become duplicate/capped, producing misleading request numbering in the sidebar.
  • Kept the report scoped to modified lines and validated both findings against the surrounding call paths (active-sessions → repository lookups → UI navigation).
  • I also included XL split suggestions in the summary: separate identity canonicalization from the dashboard/i18n/test churn, and split request-order/display-sequence behavior into its own PR.
  • I could not run the repo’s bun/bunx validations in this runner because those binaries are not installed here.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 3

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (2)
src/repository/message.ts (1)

1822-1846: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win

canonicalOwnerCondition 增加空值防御。

如果所有 userInfoMap.get(...)?.userId 都是 undefinedor(...) 返回 undefined。第 1846 行的 and(undefined, LEDGER_BILLING_CONDITION) 随后退化为没有任何 Session 过滤的全表聚合。同样的条件被第 1863、1892、1916 行复用,会触发四次全表扫描。

当前 canonicalSessionIds 的每一项都来自填充 userInfoMap 的同一批行,所以该情形不会发生。但这是隐式不变量。建议显式收集有效条件并在为空时提前返回。

🛡️ 建议的防御性写法
-  const canonicalOwnerCondition = or(
-    ...canonicalSessionIds.map((canonicalSessionId) => {
-      const owner = userInfoMap.get(canonicalSessionId)?.userId;
-      return owner === undefined
-        ? undefined
-        : ledgerCanonicalSessionLookup(canonicalSessionId, owner);
-    })
-  );
+  const canonicalOwnerConditions = canonicalSessionIds.flatMap((canonicalSessionId) => {
+    const owner = userInfoMap.get(canonicalSessionId)?.userId;
+    return owner === undefined ? [] : [ledgerCanonicalSessionLookup(canonicalSessionId, owner)];
+  });
+  if (canonicalOwnerConditions.length === 0) {
+    return [];
+  }
+  const canonicalOwnerCondition = or(...canonicalOwnerConditions);
🤖 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/repository/message.ts` around lines 1822 - 1846, Update the logic around
canonicalOwnerCondition to explicitly collect only defined owner lookup
conditions and return early when none exist, before the stats query. Ensure the
existing condition is reused for the later queries while preserving the current
behavior when valid conditions are available.
src/lib/session-manager.ts (1)

379-403: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick win

检查 pipeline 结果中的单命令错误。

redis.pipeline().exec() 的 promise 只在系统错误时 reject,单个 Redis 命令失败会走结果数组;当前丢弃 exec() 返回值后,setex(ownerKey, ...) 失败会静默忽略。ownerKey 缺失会让 isSessionRequestOwnedByKey 返回 false,导致后续获取 session messages/response 的接口显示为未存储或已过期。需解构结果数组并记录命令级错误。

🤖 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/lib/session-manager.ts` around lines 379 - 403, Update the pipeline
handling in the session request sequence method around redis.pipeline().exec()
to inspect the returned per-command results, rather than discarding them. Detect
and log command-level errors from operations such as setex(ownerKey, ...), while
preserving the existing fallback path for rejected exec() promises and returning
the sequence when the pipeline completes.
🧹 Nitpick comments (8)
src/repository/message.ts (3)

2207-2210: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

移除不可达的 COALESCE 回退分支。

两处查询的 WHERE 条件都包含 isNotNull(messageRequest.requestSequence)。因此 COALESCE(requestSequence, row_number() ...) 的第二个参数永远不会被使用。该窗口函数仍会被规划器计算,属于多余开销与误导性代码。

直接使用 requestSequence 即可。

Also applies to: 2282-2285

🤖 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/repository/message.ts` around lines 2207 - 2210, Update the
displaySequence expressions in both affected queries to use
messageRequest.requestSequence directly, removing the unreachable COALESCE and
row_number fallback while preserving the existing WHERE conditions.

1532-1547: 🚀 Performance & Scalability | 🔵 Trivial

确认相关子查询的索引支持。

该相关子查询对外层每一行执行一次。过滤条件为 session_iduser_idkeydeleted_atis_replay,排序为 created_at DESC, id DESC。schema 中只有 idx_message_request_session_idsession_id 单列,带 deleted_at IS NULL 部分条件)。当单个 identity 关联大量请求时,该查询会退化。

建议评估新增复合索引,例如 (session_id, user_id, key, created_at DESC, id DESC) 并带 deleted_at IS NULL 部分条件;或改写为一次性的 DISTINCT ON 预聚合,避免逐行相关子查询。

🤖 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/repository/message.ts` around lines 1532 - 1547, 为相关子查询补充索引支持:在
message_request 的 schema 中新增覆盖 session_id、user_id、key 以及 created_at DESC、id DESC
的复合部分索引,并将 deleted_at IS NULL 与 is_replay = false 纳入索引条件;确保它匹配
messageSessionIdentity 查询中的过滤和排序,保留现有子查询逻辑。

54-60: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

统一两个 canonical lookup 的 owner 参数契约。

ledgerCanonicalSessionLookup 要求 ownerUserId: numbermessageCanonicalSessionLookup 接受 ownerUserId?: number。两个函数命名相似,但归属约束的强度不同。调用方容易在 message 侧遗漏 owner 参数而得到无租户约束的查询。

如果 message 侧确实需要支持管理员的无约束读取,请在函数上补充注释说明该语义。如果不需要,请把参数改为必填。

Also applies to: 77-86

🤖 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/repository/message.ts` around lines 54 - 60, 统一
ledgerCanonicalSessionLookup 与 messageCanonicalSessionLookup 的 ownerUserId 契约:将
messageCanonicalSessionLookup 的 ownerUserId 改为必填,并更新所有调用方传入所属用户
ID,确保查询始终包含租户约束;仅在确需管理员无约束读取时,保留可选参数并为该语义补充注释。
tests/unit/lib/session-request-locator.test.ts (1)

88-127: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

补充单次查询路径的 owner 透传用例。

该用例传入 requestId = 107,因此触发两次 findSessionRequestLocator 调用。当选择器全部为空时,函数只执行第一次查询并直接返回 identityLocator。该路径的 ownerUserId 透传目前没有断言覆盖。

建议增加一个用例:只传 identityownerUserId,断言 findSessionRequestLocator 恰好被调用一次,且第三个参数为该 owner。

💚 建议新增的用例
+  it("forwards the owner scope when no selector is provided", async () => {
+    findSessionRequestLocatorMock.mockResolvedValueOnce({
+      requestId: 108,
+      sourceSessionId: "physical-latest",
+      requestSequence: 8,
+      keyId: 23,
+      identityKind: "session_id",
+      scopeTag: null,
+      fingerprint: null,
+    });
+    const { resolveSessionRequestLocator } = await import("`@/lib/session-request-locator`");
+
+    await resolveSessionRequestLocator("sess-1", undefined, undefined, undefined, 23);
+
+    expect(findSessionRequestLocatorMock).toHaveBeenCalledTimes(1);
+    expect(findSessionRequestLocatorMock).toHaveBeenCalledWith("sess-1", {}, 23);
+  });
🤖 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 `@tests/unit/lib/session-request-locator.test.ts` around lines 88 - 127, 在
resolveSessionRequestLocator 的测试中补充单次查询路径:仅传入 identity 和 ownerUserId,不传选择器,断言
findSessionRequestLocatorMock 恰好调用一次,并验证第三个参数透传该 ownerUserId。
src/lib/session-manager.ts (1)

357-366: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

补充 keyId 的文档说明。

JSDoc 只列出了 @param sessionId。请补充 @param keyId,并说明该方法会写入 session:{sessionId}:req:{sequence}:owner 归属键。该副作用是 isSessionRequestOwnedByKey 的前置条件,调用方需要知道。

同时请说明 Redis 不可用时走 fallback 序号且不写入归属键,此时后续归属校验会失败。

🤖 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/lib/session-manager.ts` around lines 357 - 366, 补充
`getNextRequestSequence` 的 JSDoc:为 `keyId` 增加 `@param` 说明,并明确方法会写入
`session:{sessionId}:req:{sequence}:owner` 归属键;同时说明 Redis 不可用时使用 fallback
序号且不会写入归属键,后续 `isSessionRequestOwnedByKey` 校验将失败。
src/actions/active-sessions.ts (1)

954-961: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

简化恒定为真的分支。

findSessionRequestLocatorrequestSequence == null 时返回 null,因此 locatorResult.locator.requestSequence 的类型是 numbereffectiveSequence == null 恒为假,三元表达式的 { prevRequest: null, nextRequest: null } 分支不可达。第 1124 行的 effectiveSequence ?? null 同样多余。

直接调用 findAdjacentSessionRequests 即可。

🤖 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/actions/active-sessions.ts` around lines 954 - 961, 在相关会话请求处理逻辑中移除对
effectiveSequence == null 的不可达分支,直接调用
findAdjacentSessionRequests(canonicalSessionId, effectiveRequestId,
sessionStats.userId);同时删除第 1124 行对 effectiveSequence 的多余 ?? null,直接使用
effectiveSequence。
src/lib/session-request-locator.ts (1)

13-21: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

考虑把选择器参数改为对象。

resolveSessionRequestLocator 现在有 5 个位置参数,且后 3 个都可选。调用方需要写出占位实参,例如 resolveSessionRequestLocator(id, requestSequence, sourceSessionId, undefined, ownerUserId)。这种写法容易在后续新增参数时错位,并且 undefined 占位掩盖了调用意图。

findSessionRequestLocator 已经使用 selector 对象。建议在此处保持一致。

♻️ 建议的签名
 export async function resolveSessionRequestLocator(
   identity: string,
-  requestSequence?: number,
-  sourceSessionId?: string,
-  requestId?: number,
-  ownerUserId?: number
+  options: {
+    requestSequence?: number;
+    sourceSessionId?: string;
+    requestId?: number;
+    ownerUserId?: number;
+  } = {}
 ): Promise<SessionRequestLocatorResult> {
+  const { requestSequence, sourceSessionId, requestId, ownerUserId } = options;
🤖 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/lib/session-request-locator.ts` around lines 13 - 21, 将
resolveSessionRequestLocator 的选择器参数改为对象参数,统一采用与 findSessionRequestLocator 相同的
selector 风格,避免多个可选位置参数及 undefined 占位。同步更新该函数内部的属性读取和所有调用方,确保
requestSequence、sourceSessionId、requestId、ownerUserId 按字段名传递且现有行为不变。
src/lib/cache/session-cache.ts (1)

102-104: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

为缓存键提供配对的解析函数。

第 163-165 行用 alias.indexOf(":") 手工反解 sessionDetailsCacheKey 生成的键。该逻辑依赖「userId 不含冒号」这一隐式约定,并且与 sessionDetailsCacheKey 分散在两处。如果键格式改变,索引回收会静默失效,sessionDetailsOwnersCache 会持续累积条目。

建议增加 parseSessionDetailsCacheKey(key)sessionDetailsCacheKey 配对,并在解析失败时跳过该项。

♻️ 建议的重构
 function sessionDetailsCacheKey(sessionId: string, userId: number): string {
   return `${userId}:${sessionId}`;
 }
+
+function parseSessionDetailsCacheKey(key: string): { userId: number; sessionId: string } | null {
+  const separatorIndex = key.indexOf(":");
+  if (separatorIndex < 0) return null;
+  const userId = Number(key.slice(0, separatorIndex));
+  if (!Number.isInteger(userId)) return null;
+  return { userId, sessionId: key.slice(separatorIndex + 1) };
+}
     for (const alias of aliases) {
       sessionDetailsCache.delete(alias);
-      const separatorIndex = alias.indexOf(":");
-      if (separatorIndex >= 0) {
-        const identity = alias.slice(separatorIndex + 1);
-        const owners = sessionDetailsOwnersCache.get(identity);
+      const parsed = parseSessionDetailsCacheKey(alias);
+      if (parsed) {
+        const owners = sessionDetailsOwnersCache.get(parsed.sessionId);
         if (owners) {

Also applies to: 163-165

🤖 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/lib/cache/session-cache.ts` around lines 102 - 104, 为
sessionDetailsCacheKey 增加配对的 parseSessionDetailsCacheKey 函数,集中处理缓存键的生成与解析,并让第
163-165 行的 sessionDetailsOwnersCache 回收逻辑改用该解析函数。解析失败时跳过当前条目,避免依赖手工查找冒号导致索引回收失效。
🤖 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/actions/active-sessions.ts`:
- Around line 985-996: Update getSessionDetails to store the result of
isSessionRequestOwnedByKey as redisArtifactsOwned instead of returning an error
when it is false. Guard all SessionManager Redis-side reads in the Promise.all
block with this flag, using null placeholders when ownership is unavailable,
while always retaining findMessageRequestAuditById and the database-backed
sessionStats, navigation, specialSettings, and status data.

In `@src/app/`[locale]/dashboard/logs/_components/error-details-dialog.test.tsx:
- Line 499: Remove the duplicate pendingCheck declaration in the test’s
surrounding scope, keeping exactly one const Promise.withResolvers declaration
so the file compiles without same-scope redeclaration errors.

In `@src/repository/message.ts`:
- Around line 39-52: 修正 ledgerSessionLookupForOwner() 及
aggregateMultipleSessionStats() 的物理 Session 查询,避免在 ownerUserId 未提供时仅按非保留
identity 的 session_id 跨用户合并账单统计。将物理 ID 先规范化为 canonical identity 后查询,或在物理 ID
分支强制附加当前用户约束,并确保费用、Token、供应商和模型统计与用户信息属于同一用户。

---

Outside diff comments:
In `@src/lib/session-manager.ts`:
- Around line 379-403: Update the pipeline handling in the session request
sequence method around redis.pipeline().exec() to inspect the returned
per-command results, rather than discarding them. Detect and log command-level
errors from operations such as setex(ownerKey, ...), while preserving the
existing fallback path for rejected exec() promises and returning the sequence
when the pipeline completes.

In `@src/repository/message.ts`:
- Around line 1822-1846: Update the logic around canonicalOwnerCondition to
explicitly collect only defined owner lookup conditions and return early when
none exist, before the stats query. Ensure the existing condition is reused for
the later queries while preserving the current behavior when valid conditions
are available.

---

Nitpick comments:
In `@src/actions/active-sessions.ts`:
- Around line 954-961: 在相关会话请求处理逻辑中移除对 effectiveSequence == null 的不可达分支,直接调用
findAdjacentSessionRequests(canonicalSessionId, effectiveRequestId,
sessionStats.userId);同时删除第 1124 行对 effectiveSequence 的多余 ?? null,直接使用
effectiveSequence。

In `@src/lib/cache/session-cache.ts`:
- Around line 102-104: 为 sessionDetailsCacheKey 增加配对的
parseSessionDetailsCacheKey 函数,集中处理缓存键的生成与解析,并让第 163-165 行的
sessionDetailsOwnersCache 回收逻辑改用该解析函数。解析失败时跳过当前条目,避免依赖手工查找冒号导致索引回收失效。

In `@src/lib/session-manager.ts`:
- Around line 357-366: 补充 `getNextRequestSequence` 的 JSDoc:为 `keyId` 增加 `@param`
说明,并明确方法会写入 `session:{sessionId}:req:{sequence}:owner` 归属键;同时说明 Redis 不可用时使用
fallback 序号且不会写入归属键,后续 `isSessionRequestOwnedByKey` 校验将失败。

In `@src/lib/session-request-locator.ts`:
- Around line 13-21: 将 resolveSessionRequestLocator 的选择器参数改为对象参数,统一采用与
findSessionRequestLocator 相同的 selector 风格,避免多个可选位置参数及 undefined
占位。同步更新该函数内部的属性读取和所有调用方,确保 requestSequence、sourceSessionId、requestId、ownerUserId
按字段名传递且现有行为不变。

In `@src/repository/message.ts`:
- Around line 2207-2210: Update the displaySequence expressions in both affected
queries to use messageRequest.requestSequence directly, removing the unreachable
COALESCE and row_number fallback while preserving the existing WHERE conditions.
- Around line 1532-1547: 为相关子查询补充索引支持:在 message_request 的 schema 中新增覆盖
session_id、user_id、key 以及 created_at DESC、id DESC 的复合部分索引,并将 deleted_at IS NULL
与 is_replay = false 纳入索引条件;确保它匹配 messageSessionIdentity 查询中的过滤和排序,保留现有子查询逻辑。
- Around line 54-60: 统一 ledgerCanonicalSessionLookup 与
messageCanonicalSessionLookup 的 ownerUserId 契约:将 messageCanonicalSessionLookup 的
ownerUserId 改为必填,并更新所有调用方传入所属用户 ID,确保查询始终包含租户约束;仅在确需管理员无约束读取时,保留可选参数并为该语义补充注释。

In `@tests/unit/lib/session-request-locator.test.ts`:
- Around line 88-127: 在 resolveSessionRequestLocator 的测试中补充单次查询路径:仅传入 identity 和
ownerUserId,不传选择器,断言 findSessionRequestLocatorMock 恰好调用一次,并验证第三个参数透传该
ownerUserId。
🪄 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 Plus

Run ID: c4ab2a87-115d-439b-8010-c1e41ec90e90

📥 Commits

Reviewing files that changed from the base of the PR and between ef7aaf1 and 7cb7311.

📒 Files selected for processing (32)
  • messages/en/dashboard.json
  • messages/ja/dashboard.json
  • messages/ru/dashboard.json
  • messages/zh-CN/dashboard.json
  • messages/zh-TW/dashboard.json
  • src/actions/active-sessions.ts
  • src/actions/session-origin-chain.ts
  • src/actions/session-response.ts
  • src/app/[locale]/dashboard/logs/_components/error-details-dialog.test.tsx
  • src/app/[locale]/dashboard/logs/_components/error-details-dialog/index.tsx
  • src/app/[locale]/dashboard/sessions/[sessionId]/messages/_components/request-list-sidebar.test.tsx
  • src/app/[locale]/dashboard/sessions/[sessionId]/messages/_components/request-list-sidebar.tsx
  • src/app/v1/_lib/proxy/session-guard.ts
  • src/lib/cache/session-cache.ts
  • src/lib/session-manager-detail-snapshots.test.ts
  • src/lib/session-manager.ts
  • src/lib/session-request-locator.ts
  • src/repository/message.ts
  • tests/unit/actions/active-sessions-detail-snapshots.test.ts
  • tests/unit/actions/active-sessions-requests.test.ts
  • tests/unit/actions/active-sessions-special-settings.test.ts
  • tests/unit/actions/active-sessions-termination.test.ts
  • tests/unit/actions/session-origin-chain-integration.test.ts
  • tests/unit/actions/session-origin-chain.test.ts
  • tests/unit/actions/session-response.test.ts
  • tests/unit/frontend/api-error-i18n.test.ts
  • tests/unit/lib/cache/session-cache.test.ts
  • tests/unit/lib/session-request-locator.test.ts
  • tests/unit/repository/message-aggregate-multiple-session-stats.test.ts
  • tests/unit/repository/message-aggregate-session-stats.test.ts
  • tests/unit/repository/message-public-readback.test.ts
  • tests/unit/repository/message-session-request-query.test.ts
🚧 Files skipped from review as they are similar to previous changes (7)
  • tests/unit/frontend/api-error-i18n.test.ts
  • messages/zh-TW/dashboard.json
  • messages/zh-CN/dashboard.json
  • messages/ru/dashboard.json
  • messages/en/dashboard.json
  • src/app/[locale]/dashboard/sessions/[sessionId]/messages/_components/request-list-sidebar.tsx
  • src/app/[locale]/dashboard/logs/_components/error-details-dialog/index.tsx

Comment thread src/actions/active-sessions.ts Outdated

test("clears stale Session detail availability while checking a new request", async () => {
hasSessionMessagesMock.mockResolvedValueOnce({ ok: true, data: true });
const pendingCheck = Promise.withResolvers<{ ok: false; error: string }>();

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎯 Functional Correctness | 🔴 Critical | ⚡ Quick win

删除重复的 pendingCheck 声明。

提供的片段在 Line 499 重复声明 const pendingCheck。如果最终文件包含两条声明,TypeScript 会报告同一作用域中的重复声明错误,测试文件无法编译。请保留一条声明。

🤖 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/app/`[locale]/dashboard/logs/_components/error-details-dialog.test.tsx at
line 499, Remove the duplicate pendingCheck declaration in the test’s
surrounding scope, keeping exactly one const Promise.withResolvers declaration
so the file compiles without same-scope redeclaration errors.

Comment thread src/repository/message.ts
@github-actions

github-actions Bot commented Aug 2, 2026

Copy link
Copy Markdown
Contributor

🧪 测试结果

测试类型 状态
代码质量
单元测试
集成测试
API 测试

总体结果: ✅ 所有测试通过

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 7cb73111b2

ℹ️ 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".

Comment thread src/actions/active-sessions.ts Outdated
Comment on lines 97 to 98
if (terminated && isReservedSessionIdentity(identity)) {
await SessionTracker.terminateObservedSession(identity);

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Clear observed state for ordinary session identities

When a non-prefix session uses an ordinary client ID and has Provider history, this condition skips terminateObservedSession. The preceding call passes expectedProviderIds, so SessionManager.terminateSession takes its provider-scoped path and returns before deleting session:{id}:info; SessionTracker.getObservedActiveSessions therefore continues returning the supposedly terminated session until its TTL expires. Run the observed-state cleanup after every successful non-prefix termination, not only for pfx:/sid: identities.

Useful? React with 👍 / 👎.

Comment thread src/actions/active-sessions.ts Outdated
Comment on lines +939 to +940
requestId,
sessionStats.userId

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Anchor admin request lookups to the requested row

When two users reuse the same ordinary client session ID, an admin opening a specific log request can no longer reach it. The unscoped loadCanonicalSessionStats call selects the most recent matching owner's row, and this call then constrains the exact requestId lookup to that arbitrarily selected sessionStats.userId; a request belonging to the other user returns SESSION_REQUEST_SOURCE_MISMATCH even though admins may inspect both users. Resolve the owner from the requested row, or avoid applying the arbitrary stats owner to admin request-ID lookups.

Useful? React with 👍 / 👎.

Comment thread src/actions/active-sessions.ts Outdated
Comment on lines +992 to +995
return {
ok: false,
error: "Session 请求详情已过期",
};

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge Return an i18n error code for expired request details

When the new request-owner marker is absent or expired, this literal Chinese error is returned without an errorCode, and SessionMessagesClient consequently renders result.error directly for every locale. English, Japanese, and Russian dashboards therefore display untranslated Chinese text; return a business error code mapped through the existing locale files instead.

AGENTS.md reference: AGENTS.md:L14-L14

Useful? React with 👍 / 👎.

Comment thread src/lib/session-manager.ts Outdated
Comment on lines +382 to +385
const ownerKey = `session:${sessionId}:req:${sequence}:owner`;
const pipeline = redis.pipeline();
pipeline.expire(key, SessionManager.SESSION_TTL);
pipeline.setex(ownerKey, SessionManager.SESSION_TTL, String(keyId));

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Refresh the owner marker when storing late response artifacts

For a request that runs longer than SESSION_TTL (300 seconds by default), this owner marker expires before the response handler finishes. The response and response snapshots are then stored at completion with a fresh TTL, but every detail/response read first calls isSessionRequestOwnedByKey and rejects the request because the marker is gone, making the newly stored artifacts unreachable. Refresh the per-request owner marker whenever late artifacts are written, or otherwise keep it alive through request completion.

Useful? React with 👍 / 👎.

@ding113
ding113 force-pushed the fix/session-detail-identity-ordering branch from 7cb7311 to 9a522ff Compare August 2, 2026 11:38

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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/actions/active-sessions.ts (1)

68-101: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick win

将非 affinity 分支的并行终止改为容错执行。

第 86-95 行对 physicalSources 使用 Promise.all 并行终止。如果其中一次 SessionManager.terminateSession 调用抛出异常,Promise.all 会立即 reject,即使其他物理来源已经成功终止,整个 terminateResolvedSessionIdentity 调用也会抛出异常。

调用方 terminateActiveSessionterminateActiveSessionsBatch 会把这种异常当作彻底失败处理,也不会执行第 97-99 行的观测状态清理逻辑,即便部分物理 Session 已经终止成功。

改用 Promise.allSettled 收集每个物理来源的终止结果,任一失败不影响其他来源的结果判定。

🔧 建议的修复
-    const physicalSources = await listPhysicalSessionSourcesForIdentity(identity, ownerUserId);
-    const outcomes = await Promise.all(
-      physicalSources.map((source) =>
-        SessionManager.terminateSession(
-          source.sessionId,
-          source.providerIds.length > 0 ? source.providerIds : undefined,
-          source.keyId
-        )
-      )
-    );
-    const terminated = outcomes.some(Boolean);
+    const physicalSources = await listPhysicalSessionSourcesForIdentity(identity, ownerUserId);
+    const outcomes = await Promise.allSettled(
+      physicalSources.map((source) =>
+        SessionManager.terminateSession(
+          source.sessionId,
+          source.providerIds.length > 0 ? source.providerIds : undefined,
+          source.keyId
+        )
+      )
+    );
+    const terminated = outcomes.some(
+      (outcome) => outcome.status === "fulfilled" && outcome.value
+    );
🤖 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/actions/active-sessions.ts` around lines 68 - 101, Update the
non-affinity branch of terminateResolvedSessionIdentity to use
Promise.allSettled for physicalSources termination, so one
SessionManager.terminateSession failure does not abort other attempts or prevent
cleanup. Derive terminated from fulfilled results whose values are true,
preserving the existing SessionTracker.terminateObservedSession behavior for
reserved identities.
♻️ Duplicate comments (1)
src/actions/active-sessions.ts (1)

1500-1509: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift

终止流程的详情缓存清理未主动发现全部物理 alias。 单个终止与批量终止都只清理"本次调用已知的 ID 集合"(输入 ID、canonical ID,或同批次中命中同一 canonical 的其他输入 ID),没有主动查询该 canonical Session 的全部物理来源。共同根因:应复用文件中已导入的 listPhysicalSessionSourcesForIdentity(或等价的 canonical→缓存键反向索引)来发现全部物理 alias,而不是仅依赖调用方传入的 ID 集合。

  • src/actions/active-sessions.ts#L1500-L1509:在清理 sessionDetailCacheIds 前,对每个已终止的 canonical ID 调用 listPhysicalSessionSourcesForIdentity(canonicalId, ownerUserId) 补充全部物理 alias,而不仅采集 sessionsData 中的 requestedSessionIds
  • src/actions/active-sessions.ts#L1306-L1315:同样在清理 sessionId/canonicalSessionId 前,调用 listPhysicalSessionSourcesForIdentity(canonicalSessionId, sessionStats.userId) 获取全部物理 alias 并逐个清理。
🤖 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/actions/active-sessions.ts` around lines 1500 - 1509, 终止流程仅清理调用方已知的
Session ID,未发现 canonical Session 的全部物理 alias。更新
src/actions/active-sessions.ts#L1500-L1509,在清理 sessionDetailCacheIds 前针对每个已终止
canonical ID 调用 listPhysicalSessionSourcesForIdentity(canonicalId,
ownerUserId),将返回的 alias 加入并逐个清理;同时更新
src/actions/active-sessions.ts#L1306-L1315,在清理 sessionId/canonicalSessionId 前调用
listPhysicalSessionSourcesForIdentity(canonicalSessionId, sessionStats.userId)
补充并清理所有物理 alias。
🤖 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/lib/cache/session-cache.ts`:
- Around line 77-79: 补齐 setSessionDetailsCache 相关测试 fixture,使其满足
sessionDetailsCache 类型要求。为每个 fixture 添加 sessionIdentityKind 和
sessionFingerprint,并根据对应 sessionId 使用一致的身份类型与指纹,确保 TypeScript 检查通过。

---

Outside diff comments:
In `@src/actions/active-sessions.ts`:
- Around line 68-101: Update the non-affinity branch of
terminateResolvedSessionIdentity to use Promise.allSettled for physicalSources
termination, so one SessionManager.terminateSession failure does not abort other
attempts or prevent cleanup. Derive terminated from fulfilled results whose
values are true, preserving the existing SessionTracker.terminateObservedSession
behavior for reserved identities.

---

Duplicate comments:
In `@src/actions/active-sessions.ts`:
- Around line 1500-1509: 终止流程仅清理调用方已知的 Session ID,未发现 canonical Session 的全部物理
alias。更新 src/actions/active-sessions.ts#L1500-L1509,在清理 sessionDetailCacheIds
前针对每个已终止 canonical ID 调用 listPhysicalSessionSourcesForIdentity(canonicalId,
ownerUserId),将返回的 alias 加入并逐个清理;同时更新
src/actions/active-sessions.ts#L1306-L1315,在清理 sessionId/canonicalSessionId 前调用
listPhysicalSessionSourcesForIdentity(canonicalSessionId, sessionStats.userId)
补充并清理所有物理 alias。
🪄 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 Plus

Run ID: 966aeb5e-e3ad-455c-8605-d6fc61e1f2f4

📥 Commits

Reviewing files that changed from the base of the PR and between 7cb7311 and 9a522ff.

📒 Files selected for processing (47)
  • messages/en/dashboard.json
  • messages/ja/dashboard.json
  • messages/ru/dashboard.json
  • messages/zh-CN/dashboard.json
  • messages/zh-TW/dashboard.json
  • src/actions/active-sessions.ts
  • src/actions/session-origin-chain.ts
  • src/actions/session-response.ts
  • src/app/[locale]/dashboard/logs/_components/error-details-dialog.test.tsx
  • src/app/[locale]/dashboard/logs/_components/error-details-dialog/components/MetadataTab.tsx
  • src/app/[locale]/dashboard/logs/_components/error-details-dialog/components/SummaryTab.tsx
  • src/app/[locale]/dashboard/logs/_components/error-details-dialog/index.tsx
  • src/app/[locale]/dashboard/logs/_components/error-details-dialog/types.ts
  • src/app/[locale]/dashboard/logs/_components/usage-logs-table.tsx
  • src/app/[locale]/dashboard/logs/_components/virtualized-logs-table.tsx
  • src/app/[locale]/dashboard/sessions/[sessionId]/messages/_components/request-list-sidebar.test.tsx
  • src/app/[locale]/dashboard/sessions/[sessionId]/messages/_components/request-list-sidebar.tsx
  • src/app/[locale]/dashboard/sessions/[sessionId]/messages/_components/session-messages-client-actions.test.tsx
  • src/app/[locale]/dashboard/sessions/[sessionId]/messages/_components/session-messages-client.tsx
  • src/app/api/v1/resources/sessions/handlers.ts
  • src/app/api/v1/resources/sessions/router.ts
  • src/app/v1/_lib/proxy/session-guard.ts
  • src/lib/api-client/v1/actions/active-sessions.ts
  • src/lib/api-client/v1/errors.ts
  • src/lib/api-client/v1/openapi-types.gen.ts
  • src/lib/api/v1/schemas/sessions.ts
  • src/lib/cache/session-cache.ts
  • src/lib/session-manager-detail-snapshots.test.ts
  • src/lib/session-manager.ts
  • src/lib/session-request-locator.ts
  • src/repository/message.ts
  • tests/api/v1/sessions/sessions.test.ts
  • tests/unit/actions/active-sessions-detail-snapshots.test.ts
  • tests/unit/actions/active-sessions-requests.test.ts
  • tests/unit/actions/active-sessions-special-settings.test.ts
  • tests/unit/actions/active-sessions-termination.test.ts
  • tests/unit/actions/session-origin-chain-integration.test.ts
  • tests/unit/actions/session-origin-chain.test.ts
  • tests/unit/actions/session-response.test.ts
  • tests/unit/api/v1/api-client-actions.test.ts
  • tests/unit/frontend/api-error-i18n.test.ts
  • tests/unit/lib/cache/session-cache.test.ts
  • tests/unit/lib/session-request-locator.test.ts
  • tests/unit/repository/message-aggregate-multiple-session-stats.test.ts
  • tests/unit/repository/message-aggregate-session-stats.test.ts
  • tests/unit/repository/message-public-readback.test.ts
  • tests/unit/repository/message-session-request-query.test.ts
🚧 Files skipped from review as they are similar to previous changes (45)
  • tests/unit/repository/message-public-readback.test.ts
  • src/app/[locale]/dashboard/logs/_components/usage-logs-table.tsx
  • src/lib/api-client/v1/errors.ts
  • src/lib/api-client/v1/openapi-types.gen.ts
  • tests/unit/repository/message-aggregate-session-stats.test.ts
  • src/app/api/v1/resources/sessions/router.ts
  • tests/unit/actions/session-origin-chain-integration.test.ts
  • src/lib/api-client/v1/actions/active-sessions.ts
  • src/actions/session-origin-chain.ts
  • src/app/v1/_lib/proxy/session-guard.ts
  • tests/unit/actions/active-sessions-requests.test.ts
  • tests/unit/actions/session-origin-chain.test.ts
  • tests/unit/frontend/api-error-i18n.test.ts
  • src/app/[locale]/dashboard/sessions/[sessionId]/messages/_components/session-messages-client.tsx
  • messages/ja/dashboard.json
  • src/app/[locale]/dashboard/sessions/[sessionId]/messages/_components/request-list-sidebar.test.tsx
  • tests/unit/lib/cache/session-cache.test.ts
  • messages/zh-CN/dashboard.json
  • tests/api/v1/sessions/sessions.test.ts
  • messages/en/dashboard.json
  • src/lib/api/v1/schemas/sessions.ts
  • src/actions/session-response.ts
  • src/app/api/v1/resources/sessions/handlers.ts
  • src/app/[locale]/dashboard/logs/_components/virtualized-logs-table.tsx
  • src/app/[locale]/dashboard/logs/_components/error-details-dialog/index.tsx
  • src/lib/session-request-locator.ts
  • tests/unit/actions/session-response.test.ts
  • messages/ru/dashboard.json
  • src/app/[locale]/dashboard/logs/_components/error-details-dialog/types.ts
  • src/app/[locale]/dashboard/sessions/[sessionId]/messages/_components/session-messages-client-actions.test.tsx
  • src/lib/session-manager-detail-snapshots.test.ts
  • tests/unit/lib/session-request-locator.test.ts
  • tests/unit/repository/message-aggregate-multiple-session-stats.test.ts
  • messages/zh-TW/dashboard.json
  • tests/unit/api/v1/api-client-actions.test.ts
  • src/app/[locale]/dashboard/logs/_components/error-details-dialog/components/SummaryTab.tsx
  • src/app/[locale]/dashboard/logs/_components/error-details-dialog.test.tsx
  • tests/unit/actions/active-sessions-special-settings.test.ts
  • src/lib/session-manager.ts
  • tests/unit/actions/active-sessions-termination.test.ts
  • tests/unit/repository/message-session-request-query.test.ts
  • tests/unit/actions/active-sessions-detail-snapshots.test.ts
  • src/app/[locale]/dashboard/sessions/[sessionId]/messages/_components/request-list-sidebar.tsx
  • src/repository/message.ts
  • src/app/[locale]/dashboard/logs/_components/error-details-dialog/components/MetadataTab.tsx

Comment thread src/lib/cache/session-cache.ts
Anchors session detail lookups, origin chain queries, and termination
cleanup to the immutable key owner of each request rather than the
session-level canonical identity. This prevents cross-owner data
leakage when multiple API keys share a physical session ID.

- resolveSessionRequestLocator now resolves directly by requestId
  when provided, returning canonicalSessionId, keyId, and userId so
  callers can scope all downstream reads to the request owner
- findSessionOriginChain queries by requestId and keyId, selecting the
  nearest initial-selection provider chain within the same key epoch
  instead of the first request in the session
- getNextRequestSequence uses an atomic Lua script to PERSIST the
  sequence key and write the owner marker in a single round trip
- storeSessionResponse, storeSessionResponseHeaders,
  storeSessionUpstreamResponseMeta, and phase snapshot methods now
  accept and refresh the keyId owner marker, keeping late-arriving
  response artifacts readable
- getSessionDetails and hasSessionMessages resolve the locator before
  loading stats when a requestId is supplied, then use the locator's
  userId for all subsequent queries; Redis-backed artifacts are skipped
  (returning null) when the owner marker is missing instead of failing
  closed, while database-backed audit data is still returned
- terminateResolvedSessionIdentity now always calls
  terminateObservedSession and returns sourceSessionIds so the caller
  can invalidate detail caches for every physical alias
- terminateSession fails closed when the owner key lookup throws with
  an expected keyId, preventing unauthorized cleanup
- Scoped getSessionMessages and getSessionResponse no longer fall
  through to legacy unscoped keys when the per-request key is absent
@github-actions

github-actions Bot commented Aug 2, 2026

Copy link
Copy Markdown
Contributor

🧪 测试结果

测试类型 状态
代码质量
单元测试
集成测试
API 测试

总体结果: ✅ 所有测试通过

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: e8956e85d2

ℹ️ 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".

const rawSequence = await redis.eval(
`
local sequence = redis.call('INCR', KEYS[1])
redis.call('PERSIST', KEYS[1])

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge Restore bounded lifetime for session sequence counters

For workloads that continually create short-lived or generated session IDs, PERSIST removes the sequence counter's previous TTL, leaving one permanent Redis key for every session. The normal artifact expiry and terminateSession cleanup do not delete session:${sessionId}:seq, so Redis memory grows with lifetime session cardinality and can eventually exhaust the instance. Preserve collision protection with a bounded expiry or explicitly remove these counters when their durable lifetime is no longer required.

Useful? React with 👍 / 👎.

Comment on lines +383 to +386
local sequence = redis.call('INCR', KEYS[1])
redis.call('PERSIST', KEYS[1])
local ownerKey = ARGV[1] .. sequence .. ':owner'
redis.call('SETEX', ownerKey, ARGV[2], ARGV[3])

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge Keep sequence and owner updates in one Redis Cluster slot

On Redis Cluster deployments, this script accesses session:<id>:seq and the dynamically constructed session:<id>:req:<sequence>:owner key without a shared hash tag, so the two keys normally occupy different slots and EVAL fails with CROSSSLOT—the same cluster constraint documented in docs/streaming-discovery.md lines 51-56. The catch path returns a fallback sequence without creating the owner marker, causing every subsequent isSessionRequestOwnedByKey check to fail and making request details unavailable. Store both values in a same-slot layout or a single Redis key.

Useful? React with 👍 / 👎.

Comment thread src/repository/message.ts
Comment on lines +79 to +81
const canonicalCondition = isReservedSessionIdentity(identity)
? eq(messageRequest.sessionIdentity, identity)
: eq(messageSessionIdentity, identity);

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Preserve legacy rows with reserved-looking physical IDs

For pre-0116 requests whose client-supplied physical session ID starts with pfx: or sid:, session_identity is null because drizzle/0116_gigantic_zombie.sql adds the column without backfilling message_request. The new reserved-identity branch therefore ignores the valid session_id fallback, even though aggregateMultipleSessionStats initially resolves that same row through session_id; detail, request-list, navigation, and termination lookups subsequently report the session missing. Backfill these rows to an unambiguous canonical identity or retain an owner-scoped legacy fallback.

Useful? React with 👍 / 👎.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
src/lib/session-manager.ts (1)

366-397: 🚀 Performance & Scalability | 🟠 Major | ⚡ Quick win

session:{id}:seq 键改为永久 PERSIST,但没有任何清理路径会删除它。

getNextRequestSequence 现在通过 Lua 脚本对序号 key 执行 PERSIST,彻底移除其 TTL。terminateSession(约第 3170-3183 行)会删除 infolast_seenconcurrent_countmessagesresponse 等 key,但不会删除 session:{sessionId}:seq

由于 sessionId 基于时间戳加随机数生成,每个物理会话都会在 Redis 中留下一个永不过期、且没有任何路径会主动删除的 key。长期运行会导致该类 key 在 Redis 中无限累积。

如果 Redis 使用 volatile-* 淘汰策略,没有 TTL 的 key 永远不会被淘汰,只能靠内存耗尽触发 OOM;如果使用 allkeys-* 淘汰策略,该 key 有可能被提前淘汰,导致序号重置,与本次改动希望防止的"序号复用碰撞"目标相悖。

请在 terminateSession 的清理流程中一并删除 session:{sessionId}:seq,或者改用一个足够长但有限的 TTL,而不是无条件 PERSIST

♻️ 建议的修复方向
       // 2. 删除所有 Session 相关的 key
       const pipeline = redis.pipeline();

       // Binding mirrors are mutated only by the tenant-authorized helpers above.
       pipeline.del(`session:${sessionId}:info`);
       pipeline.del(`session:${sessionId}:last_seen`);
       pipeline.del(`session:${sessionId}:concurrent_count`);
+      pipeline.del(`session:${sessionId}:seq`);
🤖 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/lib/session-manager.ts` around lines 366 - 397, Update the session
cleanup flow in terminateSession to delete the corresponding
session:${sessionId}:seq key along with the other session keys, preserving the
current PERSIST behavior in getNextRequestSequence while ensuring terminated
sessions do not leave permanent sequence keys in Redis.
🧹 Nitpick comments (2)
tests/unit/actions/active-sessions-detail-snapshots.test.ts (1)

266-309: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

建议补充 Redis owner 校验参数断言。

此测试验证了数据库侧统计与审计查询使用 locator 返回的 owner userId(2),而不是管理员自己的用户 ID。但测试未验证 isSessionRequestOwnedByKeyMock 是否使用 locator 返回的 sourceSessionId("shared-client-session")、requestSequence(4)和 keyId(22)。

本 PR 的目标之一是统一 Redis 支持的请求归属校验到不可变的请求 owner。补充此断言可以确认管理员按 request-id 查询时,Redis 归属校验也正确使用了所选行的 owner,而不是当前登录用户的上下文。

♻️ 建议补充的断言
     expect(aggregateMultipleSessionStatsMock).toHaveBeenCalledWith(["sid:owner-two"], 2);
     expect(findMessageRequestAuditByIdMock).toHaveBeenCalledWith(202, 2);
+    expect(isSessionRequestOwnedByKeyMock).toHaveBeenCalledWith(
+      "shared-client-session",
+      4,
+      22
+    );
   });
🤖 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 `@tests/unit/actions/active-sessions-detail-snapshots.test.ts` around lines 266
- 309, 在测试“anchors an admin request-id lookup to the selected row owner”中补充对
isSessionRequestOwnedByKeyMock 的调用断言,验证其使用 locator 返回的
sourceSessionId("shared-client-session")、requestSequence(4)和
keyId(22)作为归属校验参数,并保持现有 owner userId 的数据库统计与审计断言不变。
src/app/v1/_lib/proxy/response-handler.ts (1)

92-95: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

request owner key 解析逻辑在两个文件中重复,建议提炼为共享工具函数。 response-handler.ts 中的 getSessionRequestOwnerKeyIdforwarder.ts 中两处内联表达式实现完全相同的逻辑,根因是该解析规则未被提炼为跨文件共享的工具函数。

  • src/app/v1/_lib/proxy/response-handler.ts#L92-L95: 将 getSessionRequestOwnerKeyId 移到共享的 proxy 工具模块并导出。
  • src/app/v1/_lib/proxy/forwarder.ts#L764-L765: 改为调用共享的 getSessionRequestOwnerKeyId,替换内联表达式。
  • src/app/v1/_lib/proxy/forwarder.ts#L8332-L8340: 同样改为调用共享的 getSessionRequestOwnerKeyId
🤖 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/app/v1/_lib/proxy/response-handler.ts` around lines 92 - 95, 提取
response-handler.ts 中的 getSessionRequestOwnerKeyId 到共享 proxy 工具模块并导出,保留其现有回退顺序;在
src/app/v1/_lib/proxy/forwarder.ts:764-765 和
src/app/v1/_lib/proxy/forwarder.ts:8332-8340 分别改为调用该共享函数,移除重复的内联解析表达式。
🤖 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/lib/session-manager.ts`:
- Around line 366-397: Update the session cleanup flow in terminateSession to
delete the corresponding session:${sessionId}:seq key along with the other
session keys, preserving the current PERSIST behavior in getNextRequestSequence
while ensuring terminated sessions do not leave permanent sequence keys in
Redis.

---

Nitpick comments:
In `@src/app/v1/_lib/proxy/response-handler.ts`:
- Around line 92-95: 提取 response-handler.ts 中的 getSessionRequestOwnerKeyId 到共享
proxy 工具模块并导出,保留其现有回退顺序;在 src/app/v1/_lib/proxy/forwarder.ts:764-765 和
src/app/v1/_lib/proxy/forwarder.ts:8332-8340 分别改为调用该共享函数,移除重复的内联解析表达式。

In `@tests/unit/actions/active-sessions-detail-snapshots.test.ts`:
- Around line 266-309: 在测试“anchors an admin request-id lookup to the selected
row owner”中补充对 isSessionRequestOwnedByKeyMock 的调用断言,验证其使用 locator 返回的
sourceSessionId("shared-client-session")、requestSequence(4)和
keyId(22)作为归属校验参数,并保持现有 owner userId 的数据库统计与审计断言不变。

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: f55e378d-89f1-4123-8f77-b5c18ec3548a

📥 Commits

Reviewing files that changed from the base of the PR and between 9a522ff and e8956e8.

📒 Files selected for processing (21)
  • src/actions/active-sessions.ts
  • src/actions/session-origin-chain.ts
  • src/app/v1/_lib/proxy/forwarder.ts
  • src/app/v1/_lib/proxy/response-handler.ts
  • src/app/v1/_lib/proxy/warmup-guard.ts
  • src/lib/session-manager-detail-snapshots.test.ts
  • src/lib/session-manager.ts
  • src/lib/session-request-locator.ts
  • src/repository/message.ts
  • tests/unit/actions/active-sessions-detail-snapshots.test.ts
  • tests/unit/actions/active-sessions-special-settings.test.ts
  • tests/unit/actions/active-sessions-termination.test.ts
  • tests/unit/actions/session-origin-chain-integration.test.ts
  • tests/unit/actions/session-origin-chain.test.ts
  • tests/unit/lib/session-manager-terminate-session.test.ts
  • tests/unit/lib/session-request-locator.test.ts
  • tests/unit/proxy/response-handler-lease-decrement.test.ts
  • tests/unit/proxy/warmup-guard.test.ts
  • tests/unit/repository/message-origin-chain.test.ts
  • tests/unit/repository/message-session-readback.test.ts
  • tests/unit/repository/message-session-request-query.test.ts
🚧 Files skipped from review as they are similar to previous changes (7)
  • tests/unit/actions/session-origin-chain-integration.test.ts
  • src/actions/session-origin-chain.ts
  • tests/unit/actions/session-origin-chain.test.ts
  • tests/unit/lib/session-request-locator.test.ts
  • tests/unit/actions/active-sessions-special-settings.test.ts
  • tests/unit/repository/message-session-request-query.test.ts
  • tests/unit/actions/active-sessions-termination.test.ts

Add sessionIdentityKind and sessionFingerprint to each
setSessionDetailsCache fixture so the cached session details
satisfy the canonical identity contract enforced by the cache
layer.
@github-actions

github-actions Bot commented Aug 2, 2026

Copy link
Copy Markdown
Contributor

🧪 测试结果

测试类型 状态
代码质量
单元测试
集成测试
API 测试

总体结果: ✅ 所有测试通过

…served lookups

Reserved session identities with an ownerUserId now also match
message_request rows whose session_identity is null but whose
session_id equals the queried identity. This supports legacy
physical session ids that use reserved prefixes.

The fallback is only added when ownerUserId is present so that
unscoped canonical lookups remain narrow and do not pick up
unrelated null-identity rows.
@github-actions

github-actions Bot commented Aug 2, 2026

Copy link
Copy Markdown
Contributor

🧪 测试结果

测试类型 状态
代码质量
单元测试
集成测试
API 测试

总体结果: ✅ 所有测试通过

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: a7fecc9537

ℹ️ 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".

Comment thread src/repository/message.ts
Comment on lines +2331 to +2336
displaySequence: sql<number>`CASE
WHEN ${messageRequest.sessionIdentityKind} = 'prefix_affinity'
THEN row_number() OVER (
ORDER BY ${messageRequest.createdAt} ASC, ${messageRequest.id} ASC
)::int
ELSE COALESCE(

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Number the entire prefix timeline consistently

When the owner-scoped reserved-identity fallback combines modern prefix_affinity rows with legacy rows whose identity kind is null or session_id, this CASE assigns global row numbers only to the modern rows while displaying each legacy row's physical requestSequence. That can produce duplicate or non-monotonic labels in one sidebar timeline. Fresh evidence beyond the earlier sequence report is this new mixed-kind CASE; determine numbering from the resolved timeline identity rather than each row's kind.

Useful? React with 👍 / 👎.

Comment thread src/repository/message.ts
Comment on lines +55 to +60
function ledgerCanonicalSessionLookup(identity: string, ownerUserId: number) {
const canonicalCondition = isReservedSessionIdentity(identity)
? eq(usageLedger.sessionIdentity, identity)
: eq(ledgerSessionIdentity, identity);

return and(canonicalCondition, eq(usageLedger.userId, ownerUserId));

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Include legacy reserved sessions in ledger aggregation

For pre-0116 rows whose physical session ID starts with pfx: or sid:, the current message lookup can now resolve the row via its null-identity fallback, but this ledger predicate requires usage_ledger.session_identity to equal the reserved ID. Those legacy ledger rows have a null identity, so the session opens successfully while all billing statistics, providers, models, and timestamps are reported as zero or empty despite existing usage. Preserve the owner-scoped session_id fallback when aggregating a legacy-resolved reserved session.

Useful? React with 👍 / 👎.

Comment on lines +1469 to +1472
const sessionsData = await aggregateMultipleSessionStats(
uniqueSessionIds,
isAdmin ? undefined : currentUserId
);

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Preserve unauthorized batch-termination classification

For a non-admin batch containing another user's session ID, passing currentUserId into the aggregation removes that session before summarizeTerminateSessionsBatch performs its ownership classification. The request is consequently reported in missingSessionIds and missingCount, while unauthorizedSessionIds and unauthorizedCount can never contain it, contradicting the API/UI contract and the helper's explicit unauthorized-versus-missing behavior. Retrieve enough unscoped metadata to classify the request, while continuing to owner-scope the actual termination.

Useful? React with 👍 / 👎.

@ding113
ding113 merged commit f9a894d into dev Aug 2, 2026
14 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size/XL Extra Large PR (> 1000 lines)

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

1 participant