Repository navigation
Conversation
AlexStocks
left a comment
AlexStocks
left a comment
There was a problem hiding this comment.
审查结论
可以合并。 只有 4 个文件、86 行新增，做三件事：每次模型迭代前重新校验 Session 可达性、把 legacy adoption helper 收窄到自己真正用的能力、给 ContextWindowForTurn 补契约注释与回归测试。前两件是安全加固，第三件是本 PR 最有价值的部分（把「快照互不影响」这条隐含约定变成内存/GORM 共用的 contract test）。
本次审查的基准
- 相对
ai的累计 diff：+7279 / −872，99 个文件 —— 这个数字有误导性：它包含了 #1592 + #1594 + #1596 三层，本 PR 实际只有 4 个文件。 - 相对上一层的增量：
+86 / −2，4 个文件（steps.go、server/component.go、store/store.go、store/test/contract.go） - 与当前
ai（97d2db47）三方合并：0 冲突
这个 PR 是堆叠系列里「累计 diff 最不可信」的一个：按 ai 的 diff 审会读 7279 行，其中 7193 行不属于它。建议后续这类堆叠 PR 都在描述里给出增量 compare 链接（作者在 #1608 的描述里确实给了 Qiao-yq/dubbo-admin/compare/6ada403...fix/ai-1539-context-window，这点做得对，只是 GitHub 的 Files changed 页仍然展示累计 diff）。
已核实的实现细节
访问复检确实落在每次迭代上。 ai/component/agent/react/steps.go:145-153 在 runIteration 里（不是外层循环）每次取 security.AccessContextFromContext(ctx)，失败直接返回错误，因此一条已经加载好的快照不会在 Session 失效后继续被用于下一次模型调用。这条正是 PR 描述里承诺的语义，实现位置正确。
能力收窄是实质性的。 ai/component/server/component.go:255 原来直接依赖 conversationstore.MaintenanceStore，现在改成局部接口 legacySessionAdopter（只要求 AdoptLegacySessions(context.Context, security.AccessContext, string, string) (int, error)）。这让 server 包不再需要导入整个 store 包，也让测试可以用更窄的 fake —— 属于「减少依赖面」的正确做法，不是换个名字。
契约测试覆盖了关键不变量。 ai/store/test/contract.go:729 新增的 bounded context window snapshots 覆盖了 5 件事：limit=0 只含活动 Turn、limit=N 取最近 N 个已完成 Turn、超配的 limit=100 退化到全量、已完成/不存在的 Turn 返回 ErrTurnNotFound、以及调用方修改返回的消息不会影响存储（:770 直接改 messages[len-1].Content[0].Text 后再读一次）。内存与 GORM 共用同一套断言，这是防止「一个后端返回深拷贝、另一个返回别名」这类问题最有效的手段。
建议（不阻塞）
P3 — 用 IsTurnEmpty 做可达性探针，意图不透明且丢掉了返回值。 steps.go:148：
if _, err := ra.messageStore.IsTurnEmpty(ctx, access, s.Session, turnID); err != nil {
return false, err
}这行的真实目的是「借一次 Store 调用验证 access 是否仍有效」（PR 描述写的是 Recheck Session access before each model iteration），但函数名承诺的是「Turn 是否为空」，返回值被 _ 丢掉。后果有两点：一是每次迭代多一次纯检查性往返；二是读者第一反应会以为这是个遗留 bug 而去「修复」它。建议在 Store 上加一个语义明确的 CheckTurnAccess(ctx, access, sessionID, turnID) error，或至少在这行上方写清「本调用的唯一目的是复检授权，返回值被刻意忽略」。
P3 — if state == nil 分支不可达。 steps.go:149 新增的 if state == nil { return false, fmt.Errorf(...) }，在 messageStore != nil 的前提下永远走不到：真正决定「state 必须非 nil」的地方是调用方 run() 里的 steps.go:87（if ra.messageStore != nil && state == nil { return s.Usage, fmt.Errorf("interaction context snapshot not found") }）。同样地，steps.go:186 的 if state != nil 也变成了恒真。三个位置对「state 能否为 nil」给出三种不同表态（:87 报错、:149 再报一次错、:186 仍假设可能为 nil），读者很难判断哪一个才是真正的约束。建议保留 :87 的检查作为唯一入口，删掉 :149，并把 :186 的 if state != nil 去掉。
合并前置
|
LGTM |
The quoted-value branch of the credential assignment rule stopped at an escaped quote, leaving the remaining credential suffix visible to the model. Match escape pairs inside double- and single-quoted values so the whole value is redacted. Add regressions for SanitizeText and the modelInputView boundary covering escaped quotes and backslashes.
|
@Qiao-yq please merge the ai branch and resolve the conflicts. |
…lifecycle # Conflicts: # ai/component/agent/react/react.go # ai/component/memory/component.go # ai/component/server/component.go # ai/component/server/config_test.go # ai/component/server/engine/handlers.go # ai/component/server/engine/models.go # ai/component/server/engine/router.go # ai/component/server/engine/session/session.go # ai/store/gorm/models.go # ai/store/gorm/store.go # ai/store/gorm/store_test.go # ai/store/memory/store.go # ai/store/memory/store_test.go # ai/store/store.go # ai/store/validation.go
…into fix/ai-1539-context-window
|
Please provide a description of this PR:
Ready for review. This PR is blocked by and must be merged after #1596.
Recheck Session access before each model iteration while retaining the interaction's stable context snapshot. This prevents an already-loaded snapshot from being used for another model call after the Session becomes inaccessible.
This is the prerequisite fix for the production authentication and usage/audit PR. It adds no JWT adapter or accounting records.
While #1596 is open, GitHub's comparison against
aialso includes its dependency changes. The changes on top of #1596 can be reviewed here.Validation from
ai/:go test ./... -timeout 120sgo test -race ./component/agent/react ./component/server/... ./store/... -timeout 180sgit diff --checkGORM tests use SQLite; this does not establish production MySQL/PostgreSQL acceptance.
Part of #1539. Blocked by #1596.
To help us figure out who should review this PR, please put an X in all the areas that this PR affects.
Please check any characteristics that apply to this pull request.