refactor(sdlc): extract RDLC commons, reparent AuthorityCase#67
Open
ryanklee wants to merge 1 commit into
Open
refactor(sdlc): extract RDLC commons, reparent AuthorityCase#67ryanklee wants to merge 1 commit into
ryanklee wants to merge 1 commit into
Conversation
Extract risk_tier and evidence_ledger from sdlc to commons as per W2 spec. Leave temporary shims in sdlc/ for backward compatibility. Reparent AuthorityCase to BaseGovernanceCase to remove duplicate fields and YAML parsing logic. Repoint rdlc modules (research_case.py, research_ledger.py) to use commons directly. Tighten the import-graph lint to ensure rdlc never runtime-imports sdlc. All tests pass (147 passed, 2 skipped).
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
📝 WalkthroughWalkthrough
ChangesPromote shared primitives to commons and enforce clean import graph
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
rdlc/research_ledger.py (1)
16-21:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUpdate stale interim-import documentation after ledger migration.
The import now correctly targets
commons.evidence_ledger, but the module docstring still describes the ledger as living insdlc/pending extraction. Please align the text with the current architecture to avoid confusion.🤖 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 `@rdlc/research_ledger.py` around lines 16 - 21, The module docstring in rdlc/research_ledger.py still references the old location of the evidence ledger in sdlc/ or describes it as pending extraction, but the imports now correctly target commons.evidence_ledger. Update the module-level docstring at the top of the file to accurately reflect the current architecture and remove outdated references to the previous sdlc/ location or extraction status, ensuring the documentation aligns with the actual import structure that now pulls from commons.evidence_ledger.
🧹 Nitpick comments (2)
commons/evidence_ledger.py (1)
57-63: ⚡ Quick winStream ledger reads instead of loading the entire file into memory.
This path scales better for large JSONL ledgers and avoids unnecessary allocations.
Suggested change
- entries = [] - for line in path.read_text(encoding="utf-8").strip().splitlines(): + entries = [] + with path.open("r", encoding="utf-8") as f: + for line in f: if not line.strip(): continue entry = EvidenceEntry.model_validate_json(line) if case_id is None or entry.case_id == case_id: entries.append(entry)🤖 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 `@commons/evidence_ledger.py` around lines 57 - 63, The code currently loads the entire ledger file into memory with path.read_text(encoding="utf-8").strip().splitlines() before iterating through lines, which doesn't scale well for large JSONL files. Refactor this to stream the file line by line instead: replace the read_text().strip().splitlines() call with an open() context manager that reads the file with the specified encoding, then iterate directly through the file object to process each line. This avoids allocating memory for the entire file contents while maintaining the same filtering logic for case_id matching and entry validation with EvidenceEntry.model_validate_json().sdlc/authority_case.py (1)
54-54: 💤 Low valueConsider removing duplicate
model_config.
BaseGovernanceCasealready definesmodel_config = {"extra": "allow"}(seecommons/governance_case.pyline 45). Pydantic inherits model config from parent classes, so this redeclaration is redundant and could lead to drift if the base class config changes.♻️ Proposed fix
consent_contract_required: bool | None = None consent_contract_reason: str | None = None - - model_config = {"extra": "allow"} def parsed_stage(self) -> Stage:🤖 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 `@sdlc/authority_case.py` at line 54, The AuthorityCase class contains a redundant model_config definition that duplicates the one already inherited from its parent class BaseGovernanceCase. Remove the model_config = {"extra": "allow"} line from the AuthorityCase class in sdlc/authority_case.py, as Pydantic will automatically inherit this configuration from the parent class, and this inheritance approach prevents configuration drift if the base class config changes in the future.
🤖 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 `@rdlc/research_case.py`:
- Line 26: The module docstring at the top of the research_case.py file contains
an outdated note about a sanctioned interim `rdlc -> sdlc` dependency, but the
import statement now correctly imports `RiskTier` from `commons.risk_tier`
directly. Update or remove the dependency note in the module-level docstring to
reflect the actual import source and ensure the file-level documentation
contract matches the current code implementation.
---
Outside diff comments:
In `@rdlc/research_ledger.py`:
- Around line 16-21: The module docstring in rdlc/research_ledger.py still
references the old location of the evidence ledger in sdlc/ or describes it as
pending extraction, but the imports now correctly target
commons.evidence_ledger. Update the module-level docstring at the top of the
file to accurately reflect the current architecture and remove outdated
references to the previous sdlc/ location or extraction status, ensuring the
documentation aligns with the actual import structure that now pulls from
commons.evidence_ledger.
---
Nitpick comments:
In `@commons/evidence_ledger.py`:
- Around line 57-63: The code currently loads the entire ledger file into memory
with path.read_text(encoding="utf-8").strip().splitlines() before iterating
through lines, which doesn't scale well for large JSONL files. Refactor this to
stream the file line by line instead: replace the
read_text().strip().splitlines() call with an open() context manager that reads
the file with the specified encoding, then iterate directly through the file
object to process each line. This avoids allocating memory for the entire file
contents while maintaining the same filtering logic for case_id matching and
entry validation with EvidenceEntry.model_validate_json().
In `@sdlc/authority_case.py`:
- Line 54: The AuthorityCase class contains a redundant model_config definition
that duplicates the one already inherited from its parent class
BaseGovernanceCase. Remove the model_config = {"extra": "allow"} line from the
AuthorityCase class in sdlc/authority_case.py, as Pydantic will automatically
inherit this configuration from the parent class, and this inheritance approach
prevents configuration drift if the base class config changes in the future.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 5efb0419-711d-42a2-b96f-ea1094ab18c5
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (9)
commons/evidence_ledger.pycommons/risk_tier.pyrdlc/research_case.pyrdlc/research_ledger.pysdlc/authority_case.pysdlc/evidence_ledger.pysdlc/risk_tier.pytests/rdlc/test_import_graph.pytests/sdlc/test_authority_case.py
| from pydantic import Field | ||
|
|
||
| from commons.governance_case import BaseGovernanceCase | ||
| from commons.risk_tier import RiskTier |
There was a problem hiding this comment.
Synchronize the module docstring with the new import source.
RiskTier now comes from commons.risk_tier (Line 26), but the module docstring still states the sanctioned interim rdlc -> sdlc dependency. Please update/remove that note so the file-level contract matches the code.
🤖 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 `@rdlc/research_case.py` at line 26, The module docstring at the top of the
research_case.py file contains an outdated note about a sanctioned interim `rdlc
-> sdlc` dependency, but the import statement now correctly imports `RiskTier`
from `commons.risk_tier` directly. Update or remove the dependency note in the
module-level docstring to reflect the actual import source and ensure the
file-level documentation contract matches the current code implementation.
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.
Extracts
risk_tierandevidence_ledgerfromsdlctocommonsas per W2 spec. Leaves temporary shims insdlc/for backward compatibility. ReparentsAuthorityCasetoBaseGovernanceCaseto remove duplicate fields and YAML parsing logic. Repointsrdlcmodules (research_case.py,research_ledger.py) to usecommonsdirectly. Tightens the import-graph lint to ensurerdlcnever runtime-importssdlc. All tests pass (147 passed, 2 skipped).Summary by CodeRabbit
Release Notes
New Features
Refactor