Skip to content

fix(backend): address CodeRabbit review comments for KB permission resolver#25

Merged
cocowh merged 1 commit intoweagent/kb-permission-extensionfrom
weagent/fix-kb-permission-coderabbit-review
Apr 23, 2026
Merged

fix(backend): address CodeRabbit review comments for KB permission resolver#25
cocowh merged 1 commit intoweagent/kb-permission-extensionfrom
weagent/fix-kb-permission-coderabbit-review

Conversation

@cocowh
Copy link
Copy Markdown
Owner

@cocowh cocowh commented Apr 23, 2026

修改内容

针对 PR wecode-ai#1025 中 CodeRabbit 的 5 条 review 意见逐一修复。

修复详情

1. kb_permissions.pywrap() 返回 None 时静默降级,难以诊断配置错误

# Before: 任何 falsy 值都静默丢弃
if result:
    logger.info("KB permission resolver extension loaded")
    return result

# After: 明确区分 None(误配置)与正常 falsy 值
if result is None:
    logger.warning(
        "kb_permissions extension wrap() returned None; "
        f"using default resolver ({settings.SERVICE_EXTENSION})"
    )
else:
    logger.info("KB permission resolver extension loaded")
    return result

2. knowledge_service.py — extension 运行时异常会中断整个 KB 列表查询

# Before: extension 报错直接上抛,ALL scope 的 KB 列表全挂
ext_kb_ids = kb_permission_resolver.get_accessible_kb_ids(db, user_id)

# After: 降级为空列表,核心列表路径不受扩展包影响
try:
    ext_kb_ids = kb_permission_resolver.get_accessible_kb_ids(db, user_id)
except Exception as e:
    logger.warning(f"kb_permissions extension get_accessible_kb_ids failed: {e}; ...")
    ext_kb_ids = []

3. test_kb_permission_resolver.py — tautological 测试(mock 了函数再断言 mock 返回值)

原测试 patch 了 _create_resolver 然后直接调用 mock,永远通过,完全没有覆盖真实逻辑。

# Before: 自己 mock 自己,无意义
with patch.object(kb_permissions, "_create_resolver") as mock_create:
    mock_create.return_value = DefaultKbPermissionResolver()
    result = mock_create()  # 调用的是 mock 本身!

# After: 用 monkeypatch 设置 SERVICE_EXTENSION="" 再调用真实函数
monkeypatch.setattr(settings, "SERVICE_EXTENSION", "")
result = _create_resolver()

4. test_kb_permission_resolver.py — 死赋值被 side_effect 立即覆盖

# Before: return_value 设置后立即被 side_effect 覆盖,对行为无影响
mock_query.return_value.filter.return_value.first.return_value = mock_kb  # 死赋值
mock_query.return_value.filter.return_value.first.side_effect = [mock_kb, None]

# After: 只保留有效的 side_effect
mock_query.return_value.filter.return_value.first.side_effect = [mock_kb, None]

5. test_kb_permission_resolver.py — 未使用变量 role 触发 Ruff RUF059

# Before
has_access, role, is_creator = service.get_user_kb_permission(...)

# After
has_access, _role, is_creator = service.get_user_kb_permission(...)

Test Plan

  • uv run pytest tests/services/readers/test_kb_permission_resolver.py — 10 passed
  • uv run black --check — 全量通过
  • uv run isort --check — 全量通过

…solver

- kb_permissions.py: change `if result:` to explicit `if result is None`
  check and add warning log when extension wrap() returns None, so
  misconfigured extensions are surfaced instead of silently discarded
- knowledge_service.py: guard get_accessible_kb_ids() call in try/except
  so extension runtime errors degrade gracefully to ext_kb_ids=[] rather
  than breaking the entire KB listing path
- test_kb_permission_resolver.py:
  - rewrite tautological test to call the real _create_resolver() with
    monkeypatched SERVICE_EXTENSION="" rather than patching the function
    itself (test was asserting on its own mock, not the real code)
  - remove dead return_value assignment that was immediately overridden by
    side_effect (Ruff/mock semantics: side_effect takes precedence)
  - rename unused variable `role` to `_role` to signal intent (Ruff RUF059)
  - fix stale docstring references kbPermissionResolver -> kb_permission_resolver
@cocowh cocowh merged commit dadae3b into weagent/kb-permission-extension Apr 23, 2026
@cocowh cocowh deleted the weagent/fix-kb-permission-coderabbit-review branch April 27, 2026 03:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant