fix: webui表达方式审核状态持久化到数据库#1631
Closed
hsd221 wants to merge 1 commit into
Closed
Conversation
Walkthrough将表达式评估状态的判断与持久化从外部审查状态函数切换为直接使用 ORM 字段：选择未评估项时用 Changes表达式评估状态管理重构
估算代码审查工作量🎯 2 (Simple) | ⏱️ ~12 分钟 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Review rate limit: 7/8 reviews remaining, refill in 7 minutes and 30 seconds.Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/learners/expression_auto_check_task.py (1)
84-108:⚠️ Potential issue | 🟠 Major | ⚡ Quick win不要在
check_expression_suitability()返回error时落库审核结果。这里会先把
checked/rejected/modified_by写回数据库，再处理error。一旦是临时的 LLM/网络异常，这条表达方式也会被永久标记为“已审核”，后续不会再进入待审核队列。src/learners/expression_learner.py:704-716已经采用了“有error就直接返回、不持久化最终状态”的处理方式，这里也应保持一致。建议修正
suitable, reason, error = await check_expression_suitability( expression.situation, expression.style, ) + if error: + logger.warning(f"表达方式评估时出现错误 [ID: {expression.id}]: {error}") + return False try: with get_db_session() as session: expr = session.exec(select(Expression).where(Expression.id == expression.id)).first() if expr: expr.checked = True expr.rejected = not suitable expr.modified_by = ModifiedBy.AI session.add(expr) @@ - if error: - logger.warning(f"表达方式评估时出现错误 [ID: {expression.id}]: {error}") - logger.debug(f"表达方式 [ID: {expression.id}] 评估完成: {status}, reason={reason}") return suitableAs per coding guidelines,
src/**/*.py: 请重点关注以下方面：- 异常处理是否覆盖了边界情况🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/learners/expression_auto_check_task.py` around lines 84 - 108, The DB write currently persists checked/rejected/modified_by even when check_expression_suitability returned an error; change the flow in the expression suitability handling so that if error is truthy you do not write the audit fields to the DB: call check_expression_suitability, then if error is present immediately log/warn and return (or skip persistence) instead of updating Expression via get_db_session(); only when error is None proceed to open get_db_session(), fetch Expression (select(Expression).where(Expression.id == expression.id)), set expr.checked, expr.rejected = not suitable, expr.modified_by = ModifiedBy.AI, session.add(expr), and then log the status. Ensure the behavior matches the pattern used in expression_learner.py (early-return on error).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@src/learners/expression_auto_check_task.py`:
- Around line 84-108: The DB write currently persists
checked/rejected/modified_by even when check_expression_suitability returned an
error; change the flow in the expression suitability handling so that if error
is truthy you do not write the audit fields to the DB: call
check_expression_suitability, then if error is present immediately log/warn and
return (or skip persistence) instead of updating Expression via
get_db_session(); only when error is None proceed to open get_db_session(),
fetch Expression (select(Expression).where(Expression.id == expression.id)), set
expr.checked, expr.rejected = not suitable, expr.modified_by = ModifiedBy.AI,
session.add(expr), and then log the status. Ensure the behavior matches the
pattern used in expression_learner.py (early-return on error).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 425ecbf1-5361-4fdd-8bc5-7bb333691fe1
📒 Files selected for processing (2)
src/learners/expression_auto_check_task.pysrc/webui/routers/expression.py
hsd221
force-pushed
the
fix/expression-review-db-persistence
branch
from
6af4dfc to
80aa2ad
Compare
check_expression_suitability 返回错误时 early-return，不写数据库， 对齐 expression_learner.py 的错误处理模式。
hsd221
force-pushed
the
fix/expression-review-db-persistence
branch
from
80aa2ad to
35caf96
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/learners/expression_auto_check_task.py (1)
56-56: ⚡ Quick win将“未审核”过滤下推到数据库查询层
这里先全量读取再做
expr.checked过滤，数据量上来后会放大内存和 I/O 开销。建议直接在 SQL 层过滤未审核记录，再做随机抽样。♻️ 建议修改
- with get_db_session(auto_commit=False) as session: - statement = select(Expression) - all_expressions = session.exec(statement).all() - - unevaluated_expressions = [expr for expr in all_expressions if not expr.checked] + with get_db_session(auto_commit=False) as session: + statement = select(Expression).where(Expression.checked.is_(False)) + unevaluated_expressions = session.exec(statement).all()Also applies to: 65-65
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/learners/expression_auto_check_task.py` at line 56, The comprehension unevaluated_expressions = [expr for expr in all_expressions if not expr.checked] pulls all records into memory and then filters; change the data fetch so the "unchecked" predicate is applied in the DB query (e.g., add WHERE checked = false / checked IS FALSE) and perform any random sampling/limit in the SQL (ORDER BY random()/RAND() + LIMIT) instead of sampling after retrieval; update both occurrences that reference all_expressions -> unevaluated_expressions (the line creating unevaluated_expressions and the similar occurrence around line 65) so the query that populates all_expressions (or replace it with a new query) returns only unchecked items and optionally uses DB-level random sampling.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/learners/expression_auto_check_task.py`:
- Around line 91-97: The current read-then-write in get_db_session() fetches
Expression and unconditionally sets checked/rejected, which can overwrite
concurrent human review; modify the update to only affect records still
unchecked by adding a checked.is_(False) condition (e.g., include
Expression.checked.is_(False) in the select/where or perform an UPDATE where
Expression.id == expression.id AND Expression.checked.is_(False)), and only set
modified_by = ModifiedBy.AI and session.add(expr) when the conditional query
returns a row so already-reviewed records are skipped.
---
Nitpick comments:
In `@src/learners/expression_auto_check_task.py`:
- Line 56: The comprehension unevaluated_expressions = [expr for expr in
all_expressions if not expr.checked] pulls all records into memory and then
filters; change the data fetch so the "unchecked" predicate is applied in the DB
query (e.g., add WHERE checked = false / checked IS FALSE) and perform any
random sampling/limit in the SQL (ORDER BY random()/RAND() + LIMIT) instead of
sampling after retrieval; update both occurrences that reference all_expressions
-> unevaluated_expressions (the line creating unevaluated_expressions and the
similar occurrence around line 65) so the query that populates all_expressions
(or replace it with a new query) returns only unchecked items and optionally
uses DB-level random sampling.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: cdf68766-efcc-4cb6-81d6-1a320782fa9c
📒 Files selected for processing (2)
src/learners/expression_auto_check_task.pysrc/webui/routers/expression.py
🚧 Files skipped from review as they are similar to previous changes (1)
- src/webui/routers/expression.py
Comment on lines
+91
to
+97
| with get_db_session() as session: | ||
| expr = session.exec(select(Expression).where(Expression.id == expression.id)).first() | ||
| if expr: | ||
| expr.checked = True | ||
| expr.rejected = not suitable | ||
| expr.modified_by = ModifiedBy.AI | ||
| session.add(expr) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's examine the file and understand the context
cat -n src/learners/expression_auto_check_task.py | head -120 | tail -50Repository: Mai-with-u/MaiBot
Length of output: 2060
🏁 Script executed:
# Look at the Expression model to understand the checked field
fd -t f "\.py$" | xargs grep -l "class Expression" | head -5Repository: Mai-with-u/MaiBot
Length of output: 241
🏁 Script executed:
# Find get_db_session implementation
rg "def get_db_session" -A 10Repository: Mai-with-u/MaiBot
Length of output: 3684
🏁 Script executed:
# Check Expression model definition
rg "class Expression" src/common/database/database_model.py -A 30Repository: Mai-with-u/MaiBot
Length of output: 1150
🏁 Script executed:
# Get full get_db_session implementation
rg "def get_db_session" src/common/database/database.py -A 30Repository: Mai-with-u/MaiBot
Length of output: 1631
🏁 Script executed:
# Check if there are other places where Expression is updated to see patterns
rg "Expression.*checked\s*=" src/ -B 2 -A 2Repository: Mai-with-u/MaiBot
Length of output: 43
🏁 Script executed:
# Search for other updates to Expression model
rg "session.add\(expr" src/ -B 5 -A 2Repository: Mai-with-u/MaiBot
Length of output: 2061
🏁 Script executed:
# Check if there are any locking mechanisms used elsewhere
rg "with_for_update\|lock\|Lock" src/common/database/ -iRepository: Mai-with-u/MaiBot
Length of output: 43
🏁 Script executed:
# Look for other places that might be updating Expression concurrently
rg "Expression" src/learners/ -B 2 -A 2 | head -100Repository: Mai-with-u/MaiBot
Length of output: 6817
🏁 Script executed:
# Check the broader context of the _evaluate_expression function
cat -n src/learners/expression_auto_check_task.py | sed -n '50,130p'Repository: Mai-with-u/MaiBot
Length of output: 3457
缺少并发保护，可能覆盖人工审核结果
这里查询后直接写入会在"查询后到写入前"窗口覆盖其他审核者（例如 WebUI 人工审核）的审核状态。建议在更新时增加 Expression.checked.is_(False) 条件，确保仅更新仍为未审核的记录，已被审核则跳过。
建议修改
with get_db_session() as session:
- expr = session.exec(select(Expression).where(Expression.id == expression.id)).first()
- if expr:
- expr.checked = True
- expr.rejected = not suitable
- expr.modified_by = ModifiedBy.AI
- session.add(expr)
+ expr = session.exec(
+ select(Expression).where(
+ Expression.id == expression.id,
+ Expression.checked.is_(False),
+ )
+ ).first()
+ if not expr:
+ logger.debug(f"表达方式 [ID: {expression.id}] 已被其他流程审核，跳过写入")
+ return False
+
+ expr.checked = True
+ expr.rejected = not suitable
+ expr.modified_by = ModifiedBy.AI
+ session.add(expr)📝 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
| with get_db_session() as session: | |
| expr = session.exec(select(Expression).where(Expression.id == expression.id)).first() | |
| if expr: | |
| expr.checked = True | |
| expr.rejected = not suitable | |
| expr.modified_by = ModifiedBy.AI | |
| session.add(expr) | |
| with get_db_session() as session: | |
| expr = session.exec( | |
| select(Expression).where( | |
| Expression.id == expression.id, | |
| Expression.checked.is_(False), | |
| ) | |
| ).first() | |
| if not expr: | |
| logger.debug(f"表达方式 [ID: {expression.id}] 已被其他流程审核，跳过写入") | |
| return False | |
| expr.checked = True | |
| expr.rejected = not suitable | |
| expr.modified_by = ModifiedBy.AI | |
| session.add(expr) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/learners/expression_auto_check_task.py` around lines 91 - 97, The current
read-then-write in get_db_session() fetches Expression and unconditionally sets
checked/rejected, which can overwrite concurrent human review; modify the update
to only affect records still unchecked by adding a checked.is_(False) condition
(e.g., include Expression.checked.is_(False) in the select/where or perform an
UPDATE where Expression.id == expression.id AND Expression.checked.is_(False)),
and only set modified_by = ModifiedBy.AI and session.add(expr) when the
conditional query returns a row so already-reviewed records are skipped.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
问题
表达方式自动审核（LLM 评估）结果未持久化，WebUI 始终显示"未审核"。
根因
expression_to_response将checked、rejected、modified_by硬编码为False/None_evaluate_expression仅写入内存缓存 (local_storage)，未写数据库修改
src/webui/routers/expression.py:expression_to_response直接读取expression.checked、expression.rejected、expression.modified_by数据库字段src/learners/expression_auto_check_task.py:checked,rejected,modified_by)expr.checked数据库字段expression_review_store导入测试
语法检查通过，无新增函数/方法，本地已测试修复。
Summary by CodeRabbit
发行说明
Bug Fixes