test: cover access-policy RBAC deny-path and circuit-breaker probe guard - #1194
Conversation
Close the three untested branches on two security/resilience primitives to 100% line coverage: - access_policy.evaluate_access: the final `rbac_denied` deny-path, reached when a non-admin owns (or is delegated) a resource but their role and group both fail the permit set. Ownership is necessary, not sufficient. - access_policy._equivalent_roles: the fallback that maps an unrecognised role to only itself. - CircuitBreaker.call: the half-open concurrency guard that rejects a second probe while the lone half-open probe is still in flight, so a provider that may still be down is not hit twice at once. Tests only; no production behavior changes. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01T47gHdkeM8H2Mpu4VwZT3c
|
Warning Review limit reached
Next review available in: 6 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThe changes expand tests for access-policy role handling, RBAC denial, and circuit-breaker half-open probe concurrency. ChangesAccess policy test coverage
Circuit breaker test coverage
Estimated code review effort: 2 (Simple) | ~10 minutes 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
|
Closing after Loop drain: permanently blocked — branch was updated onto develop for mergeability, which cleared prior APPROVED robot evidence; re-review (CodeRabbit/OpenCode) and/or central gate jobs (metadata-only gate evaluation, coverage-evidence) remained pending/stuck without a re-runnable workflow handle. Not force-merging (merge-gate policy). Re-open a focused PR when robot capacity is available. Related product security fixes that reimplemented cleanly remain on branch goal/carddav-path-traversal-decode (#1206) for relaunch. |
|
PR governance metadata gate update for PR governance metadata gate is ready; all current-head requirements passed. |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@backend/tests/test_access_policy.py`:
- Around line 320-344: Update
test_owner_without_role_or_group_permission_is_rbac_denied to match its
coverage: either add a separate focused case where a non-owner is included in
delegated_user_ids and still receives rbac_denied, or remove the
delegated-access wording from the docstring. Keep the existing owner scenario
focused on ownership.
🪄 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 Plus
Run ID: 1c3ecc81-1f87-4e19-be43-330363ede07b
📒 Files selected for processing (2)
backend/tests/test_access_policy.pybackend/tests/test_circuit_breaker.py
seonghobae
left a comment
There was a problem hiding this comment.
Maintenance revalidation completed for current head 2241f1d79ef0a11a55a567454a8258b67efee121: required GitHub Actions workflows are green, the CodeRabbit finding is resolved, and the added tests are scoped to existing security/resilience behavior. Triggering the central review scheduler to refresh the required opencode-review evidence.
seonghobae
left a comment
There was a problem hiding this comment.
Current head 2241f1d79ef0a11a55a567454a8258b67efee121 remains immutable, all repository-local required workflows are successful, and the review finding is addressed. Re-triggering the central review/merge scheduler for current-head opencode-review evidence.
- 브랜치를 `develop`의 최신 커밋(a67a2b3)으로 하드 리셋(hard reset)하여 실수로 누락되거나 되돌려진 타 PR(#1194 등) 변경 사항을 원복 - `CHANGELOG.md`, `backend/api/tools.py`, `backend/tests/test_tools_api.py` 단 3개의 파일에 대해서만 의도된 유틸리티 도구 변경사항(uuid_generator, hash_generator) 재적용 - Docstring 추가, integer 파라미터 타입 변경, 회귀(regression) 테스트, 랜덤 멀티캐스트 노드 프라이버시 설정 등 모든 검증 요구사항 유지 - 에러 코드 포맷팅 보존 및 `api.tools` 테스트 라인 커버리지 100% 검증 완료
Description
Closes three untested branches on two backend security/resilience primitives, bringing both modules to 100% line coverage. Tests only — no production behavior changes.
services/access_policy.py:130— the finalrbac_denieddeny-path inevaluate_access, reached when a non-admin owns (or is delegated) a resource but their role and group both fail the permit set. This pins the invariant that ownership is necessary but not sufficient: a stray refactor that returnedallowedafter the ownership gate would silently widen access, and this test now catches it.services/access_policy.py:77— the_equivalent_rolesfallback that maps an unrecognised role to only itself (no accidental alias expansion for unknown roles).services/circuit_breaker.py:72— the half-open concurrency guard that rejects a second probe while the lone half-open probe is still in flight, so a provider that may still be down is not hit twice at once. Driven with anasyncio.Eventso the in-flight slot is genuinely claimed before the second call arrives.Type of change
Checklist:
Verification
🤖 Generated with Claude Code
Generated by Claude Code
Summary by CodeRabbit