refactor(#277): replace LLM calls with deterministic code in pipeline operations#302
refactor(#277): replace LLM calls with deterministic code in pipeline operations#302xsovad06 wants to merge 3 commits into
Conversation
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
WalkthroughThis PR replaces LLM-driven logic in three pipeline operations with deterministic/no-op alternatives. PR body generation now uses a structured template with an optional issue-context excerpt instead of an LLM call. Memory extraction and reviewer memory capture become no-ops. Triage heuristics are rewritten with granular signal-based classification. Tests are updated accordingly. ChangesDeterministic Pipeline Operations
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant CreatePRStep
participant Git
participant BuildPRBody
CreatePRStep->>Git: git log / git diff --stat
Git-->>CreatePRStep: commit_log, diff_stat
CreatePRStep->>BuildPRBody: build body(commit_log, diff_stat, issue_body)
BuildPRBody-->>CreatePRStep: structured body with Context section
Related issues: Related PRs: None identified Suggested labels: refactor, core, tech-debt Suggested reviewers: None identified 🐰 No more prompts to LLMs we send, 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
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: 2
🤖 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 `@sova/core/steps/create_pr.py`:
- Around line 148-153: The create_pr flow adds a “## Context” section in
create_pr.py using issue_body from ctx.task.body, but it only checks truthiness
before stripping, so whitespace-only bodies still emit an empty header. Update
the logic around the issue_body/excerpt handling to strip first and only append
the “## Context” block when the trimmed body is non-empty, keeping the
truncate() call on the cleaned text.
In `@sova/roles/triage.py`:
- Around line 175-223: The early no-body return in triage.py causes
agent:human-only issues with empty descriptions to be misclassified as
needs_spec because label checks happen later. Move the human-only label handling
in TaskAssessment logic to run before the has_body guard, using the existing
task.labels / is_human_only path from heuristic_assess() as the reference
behavior. Then remove the now-redundant downstream label_set/is_human_only
computation so the label always wins regardless of body content.
🪄 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: ASSERTIVE
Plan: Pro Plus
Run ID: db747ca8-008a-4b76-a0d7-2c2c6935c56f
⛔ Files ignored due to path filters (4)
.claude/commands/ingest-review.mdis excluded by!.claude/**and included by none.claude/rules/architecture.mdis excluded by!.claude/**and included by nonedocs/error-handling-guidelines.mdis excluded by!docs/**and included by nonedocs/pipeline-determinism.htmlis excluded by!docs/**and included by none
📒 Files selected for processing (7)
sova/core/steps/create_pr.pysova/knowledge/extraction.pysova/roles/reviewer.pysova/roles/triage.pytests/test_core.pytests/test_extraction.pytests/test_roles.py
- Fix whitespace-only issue body producing empty Context section in PR body (create_pr.py) - Fix agent:human-only label being ignored when issue body is empty (triage.py) -- label check now runs before body check - Reduce cognitive complexity in _heuristic_assess by extracting _assess_body_content, _has_criteria_markers, _has_code_references helper methods (triage.py) - Silence unused-parameter warnings in no-op extract_memories by referencing retained params (extraction.py) - Add 14 tests covering all fixed code paths
xsovad06
left a comment
xsovad06
left a comment
There was a problem hiding this comment.
Code Review for PR #302
The most critical issue is incorrect async context manager syntax in .claude/commands/ingest-review.md (line 30: await get_session() should be get_session()), which will cause runtime errors. Second, the agent:human-only label check in triage.py is unreachable when issue body is empty due to early return ordering (severity 7 logic bug). Third, whitespace-only issue bodies produce dangling '## Context' headers in PR descriptions (severity 6). Both bugs have corresponding test gaps (severity 5 each). Overall, the determinism migration is architecturally sound, but execution has three bugs and two test coverage gaps that need fixing before merge.
7 findings (6 inline, 1 in summary)
| Sev | Category | File | Finding |
|---|---|---|---|
| 8/10 | bug | .claude/commands/ingest-review.md:30 |
Incorrect async context manager syntax: async with await get_session() as session: should be async with get_session() as session:. The await here is incorrect and will cause a runtime error when the command executes. get_session() returns an async context manager, not a coroutine. |
| 7/10 | bug | sova/roles/triage.py:176 |
agent:human-only label is ignored when issue body is empty. The early return at line 176 (if not has_body) fires before is_human_only is computed (line 214), so an issue labeled agent:human-only with no body returns needs_spec instead of human_only. This breaks the labeled requirement that human-only issues should skip autonomous processing regardless of body content. |
| 6/10 | bug | sova/core/steps/create_pr.py:151 |
Whitespace-only issue body produces an empty '## Context' section in PR body. At line 151, if issue_body: is True for ' ', but after issue_body.strip() at line 152, excerpt becomes ''. Lines 152-153 then append ['## Context', '', '', ''], leaving a dangling header with no content in the PR description. |
| 5/10 | testing | tests/test_roles.py:1823 |
Missing test case for agent:human-only label with empty body. The new test test_triage_human_only_label uses body='Something detailed enough.', which has content. The edge case where labels=['agent:human-only'] but body='' is not covered, so the bug at triage.py:176 (early return before label check) goes undetected. |
| 5/10 | testing | tests/test_core.py:2833 |
Missing test case for whitespace-only issue body. Test test_pr_body_omits_context_when_no_issue_body covers body='', but the CodeRabbit-identified bug (whitespace-only body like ' ' producing empty Context section) is not tested. This edge case is explicitly mentioned in the spec but has no test coverage. |
| 4/10 | design | sova/roles/triage.py:222 |
Inconsistent complexity estimation logic: _estimate_complexity() checks for keywords like 'migration', 'refactor', 'breaking change', 'epic' but these checks are case-sensitive after body.lower() is passed in. However, the function is called with the original body (line 222), not body_lower, so keyword matching will fail for issues with 'Migration' or 'EPIC' in uppercase. |
| 3/10 | design | sova/knowledge/extraction.py:67 |
No-op function logs at DEBUG level but caller contexts (ExtractMemoryStep, ReviewerRole) may expect INFO-level confirmation that extraction was skipped. The original implementation logged at INFO level when learnings were extracted or when none were found. Now it's silent except for DEBUG, which may make it harder to trace why no memories are being captured during debugging. |
Findings not on changed lines
1. [5/10] [testing] tests/test_core.py:2833
Missing test case for whitespace-only issue body. Test test_pr_body_omits_context_when_no_issue_body covers body='', but the CodeRabbit-identified bug (whitespace-only body like ' ' producing empty Context section) is not tested. This edge case is explicitly mentioned in the spec but has no test coverage.
Fix: Add a test case async def test_pr_body_omits_context_when_whitespace_only_body() that creates a Task with body=' ' (whitespace only) and asserts '## Context' not in body. This will fail with current code at create_pr.py:151, confirming the bug.
Assessment: BLOCK -- critical issues must be fixed before merge
Closes #277
xsovad06
dismissed
coderabbitai[bot]’s stale review
Findings addressed in latest push.
|
1 participant
Summary
Changes
PR Creation (
create_pr.py)_generate_pr_body()LLM call and prompt constantsMemory Extraction (
extract_memory.py,extraction.py)ExtractMemoryStepto no-op (logs skip, returns success)execute()Review Ingestion (
.claude/commands/ingest-review.md)TaskRun.handoff_jsonTriage (
triage.py)_heuristic_assess()with better pattern detection (acceptance criteria, code blocks, label hints)Review guidance
test_core.py)test_roles.pyhas new fixtures)Test plan
make check)Closes #277