🧪 [테스트 커버리지 개선: get_encryption_keyring] - #1290
Conversation
|
👋 Jules, reporting for duty! I'm here to lend a hand with this pull request. When you start a review, I'll add a 👀 emoji to each comment to let you know I've read it. I'll focus on feedback directed at me and will do my best to stay out of conversations between you and other bots or reviewers to keep the noise down. I'll push a commit with your requested changes shortly after. Please note there might be a delay between these steps, but rest assured I'm on the job! For more direct control, you can switch me to Reactive Mode. When this mode is on, I will only act on comments where you specifically mention me with New to Jules? Learn more at jules.google/docs. For security, I will only act on instructions from the user who triggered this task. |
|
Important Review available on request
Reviews should be triggered manually for repositories with fewer than 10 stars. Select Trigger review above or comment ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📝 WalkthroughWalkthroughThe pull request adds a Trivy vulnerability ignore entry and tests for ChangesSecurity ignore update
Encryption keyring tests
Estimated code review effort: 2 (Simple) | ~10 minutes 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
backend/tests/test_db_models.py (2)
37-44: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winCover empty and whitespace-only keys.
backend/core/runtime_secrets.py:144-176rejects bothNoneand values whereactive_key_value.strip()is empty. This test covers onlyNone. ParametrizeENCRYPTION_KEYwithNone,SecretStr(""), andSecretStr(" ").As per coding guidelines, backend tests must include focused contract tests for changed behavior.
🤖 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 `@backend/tests/test_db_models.py` around lines 37 - 44, Expand test_get_encryption_keyring_missing_key to be parametrized over None, SecretStr(""), and SecretStr(" ") for settings.ENCRYPTION_KEY, while keeping ENCRYPTION_PREVIOUS_KEYS set to None. Preserve the existing RuntimeError assertion and message match for every invalid key representation.Source: Coding guidelines
16-18: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winAssert the configured key material.
These tests verify IDs and counts, but not that
keyandkey2become the active and previous encryption keys. A regression that swaps or ignores key values can still pass. Use the existing keyring API or exposed key objects to verify both generated keys.backend/db/models.py:56-68passes both configured values into the keyring builder.As per coding guidelines, backend tests must include focused contract tests for changed behavior.
Also applies to: 30-34
🤖 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 `@backend/tests/test_db_models.py` around lines 16 - 18, Extend the focused tests around get_encryption_keyring to assert that the generated active and previous key objects contain the configured key and key2 material, using the existing EncryptionKeyRing API or exposed key objects. Preserve the existing ID and count assertions while covering both key assignments so swaps or ignored values fail.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.
Nitpick comments:
In `@backend/tests/test_db_models.py`:
- Around line 37-44: Expand test_get_encryption_keyring_missing_key to be
parametrized over None, SecretStr(""), and SecretStr(" ") for
settings.ENCRYPTION_KEY, while keeping ENCRYPTION_PREVIOUS_KEYS set to None.
Preserve the existing RuntimeError assertion and message match for every invalid
key representation.
- Around line 16-18: Extend the focused tests around get_encryption_keyring to
assert that the generated active and previous key objects contain the configured
key and key2 material, using the existing EncryptionKeyRing API or exposed key
objects. Preserve the existing ID and count assertions while covering both key
assignments so swaps or ignored values fail.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 3ede37ca-b7ac-42f6-b4e8-b24c40dd1348
📒 Files selected for processing (2)
.trivyignorebackend/tests/test_db_models.py
![opencode-agent[bot]](https://avatars.githubusercontent.com/in/1549082?s=60&v=4){kind=small}
There was a problem hiding this comment.
Pull request overview
OpenCode cannot approve yet because required coverage evidence did not pass.
Review outcome
1. HIGH .github/workflows/opencode-review.yml:1 - Coverage evidence did not prove required test/docstring evidence
-
Problem: The required coverage-evidence job result was
failure, so OpenCode cannot establish approval sufficiency for this head. -
Root cause: Automated approval is only valid when the same-head coverage-evidence job proves supported repository test suites passed and configured docstring gates passed or were advisory, or reports not applicable because no supported source files or package manifests exist. Missing, failed, skipped, unavailable, or unsupported-tooling test evidence is a blocker.
-
Fix: Install or configure the repository test/docstring evidence tooling when source files or package manifests exist, rerun the current-head coverage-evidence job, and approve only after it reports
successwith required evidence or explicit no-source not-applicable evidence. -
Regression test: Keep the approval branch checking
needs.coverage-evidence.result == successbefore posting APPROVE, and publish REQUEST_CHANGES when coverage-evidence blocker states such as cancelled, skipped, failed, unsupported-tooling, or below-100 evidence are present. -
Result: REQUEST_CHANGES
-
Reason: coverage-evidence result was
failure, so required test/docstring evidence was not proven for current head4e91c2ab5082440458e787b8af08f869a2aa4a0b. -
Head SHA:
4e91c2ab5082440458e787b8af08f869a2aa4a0b -
Workflow run: 31351926847
-
Workflow attempt: 1
Coverage evidence
Coverage evidence job did not run or did not publish coverage evidence.
Changed-File Evidence Map
flowchart LR
PR["PR changed files"] --> Evidence["OpenCode bounded evidence"]
Evidence --> S1["Changed file: .trivyignore"]
S1 --> I1["repository behavior"]
I1 --> R1["Review risk: Changed file: .trivyignore"]
R1 --> V1["required checks"]
Evidence --> S2["Backend: test_db_models.py"]
S2 --> I2["API and service runtime"]
I2 --> R2["Review risk: Backend: test_db_models.py"]
R2 --> V2["backend tests"]
OpenCode Review Overview
Pull request overviewOpenCode cannot approve yet because required coverage evidence did not pass. Review outcome1. HIGH .github/workflows/opencode-review.yml:1 - Coverage evidence did not prove required test/docstring evidence
Coverage evidenceCoverage evidence job did not run or did not publish coverage evidence. Changed-File Evidence Mapflowchart LR
PR["PR changed files"] --> Evidence["OpenCode bounded evidence"]
Evidence --> S1["Changed file: .trivyignore"]
S1 --> I1["repository behavior"]
I1 --> R1["Review risk: Changed file: .trivyignore"]
R1 --> V1["required checks"]
Evidence --> S2["Backend: test_db_models.py"]
S2 --> I2["API and service runtime"]
I2 --> R2["Review risk: Backend: test_db_models.py"]
R2 --> V2["backend tests"]
|
|
PR governance metadata gate is not ready for
|
1 participant
Superseded
This focused test lane is closed without merge because its complete useful diff has been preserved on the current-base canonical encryption-key regression PR #1287.
The
db.models.get_encryption_keyringcoverage from current headfe9d0bd09ec3db1c263a72d7f29f813e1d2578cb—active-key construction, previous-key preservation, real Fernet decryptability, and fail-closed missing-key behavior—is now present on #1287 asbackend/tests/test_db_models.py.#1287 also consolidates adjacent runtime keyring and key-identifier boundary coverage, reducing parallel micro-PRs on one encryption-key contract. #1287 must independently pass exact-head CI, coverage, security, review, and protected-branch governance before merge.