fix(core): route conductor/tasks runtime chatter through logger (#649)#687
Conversation
conductor.py had 84 bare print() calls (Supervisor/GlobalFix/dependency-analysis chatter) and tasks.py had 2 on the LLM-fallback path. Because conductor.py is imported by the FastAPI server, these leaked to server stdout — violating CLAUDE.md headless-core rules (#1, #3). - conductor.py: all 84 print() -> logger.{info,warning,error,debug} by intent; decorative leading newlines/spaces stripped; ===/--- separator-only prints dropped. - tasks.py: LLM-fallback prints -> logger.warning. - Add tests/core/test_conductor_logging.py: AST guard asserting zero print() call nodes in conductor.py/tasks.py/runtime.py, plus a caplog behavioral check. - Update 3 conductor tests that asserted on captured stdout to assert on caplog (same intent, new channel — the very behavior this issue introduces). Left as-is (verified): runtime.py:1020/1091 are print() inside docstring Example blocks (never executed); events.py uses Rich console.print for CLI-intended output. CLI UX is preserved via events.py milestone output (print_event=True) and the CLI's own Rich batch summary. Closes #649.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughAll bare Changesprint() → logger Migration in Conductor and Tasks
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related issues
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Code Review — fix(core): route conductor/tasks runtime chatter through logger (#649)Summary: Clean, well-scoped architectural fix. Replacing 84 bare What's done well
Minor concerns1. f-string formatting anti-pattern (minor perf) # Current (string always formatted, even when log level is disabled)
logger.info(f"Execution plan: {plan.num_groups} groups, {plan.total_tasks} tasks")
# Preferred (lazy evaluation via % args)
logger.info("Execution plan: %d groups, %d tasks", plan.num_groups, plan.total_tasks)Since 2. Emoji in log messages logger.warning("⊘ Still blocked")
logger.warning("⚠️ Batch marked PARTIAL due to failed batch-level gates")Emoji in log output can confuse log aggregators, break grep patterns, and cause encoding issues in some environments. Since the original 3. # conductor.py (CycleDetectedError catch block)
logger.warning(f"Error: {e}")
logger.warning("Falling back to serial execution")These two could be combined and made more specific: logger.warning("CycleDetectedError during dependency analysis, falling back to serial: %s", e)Saves a log entry and makes the error type discoverable without reading the next line. 4. The diff shows Acceptance criteria check
Verdict: Approve with nits. The architectural fix is correct and complete. The f-string and emoji points are style nits that can be addressed in a follow-up logging style pass. Ready to merge. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
codeframe/core/conductor.py (1)
226-226: Consider using parameterized logging withexc_info=Truefor exception paths to preserve tracebacks and follow logging best practices.Logger f-strings are present at multiple locations (lines 226, 374-375, 404-405, 2176). While the current Ruff configuration does not include explicit logging rules, parameterized logging is a best practice for lazy evaluation and proper exception context preservation.
For exception paths specifically (lines 226 and 2176), adding
exc_info=Trueis recommended:Exception paths that would benefit from exc_info
- logger.warning(f"[Supervisor] Classification failed: {e}") + logger.warning("[Supervisor] Classification failed: %s", e, exc_info=True) - logger.error(f"Error executing task: {e}") + logger.error("Error executing task: %s", e, exc_info=True)For non-exception contexts (lines 374-375, 404-405), parameterized logging improves consistency without requiring
exc_info:- logger.info(f"[GlobalFix] Agent taking ownership: {fix_description[:60]}...") + logger.info("[GlobalFix] Agent taking ownership: %s...", fix_description[:60])🤖 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 `@codeframe/core/conductor.py` at line 226, Replace f-string logging with parameterized logging in the conductor.py file. For exception handling paths where an exception variable is available (such as the `logger.warning(f"[Supervisor] Classification failed: {e}")` pattern), use parameterized logging by passing the exception as a parameter and including `exc_info=True` to preserve the full traceback for debugging. For non-exception logging contexts in the file, use parameterized logging without exc_info for consistency and to enable proper lazy evaluation of the logger. Change from logger.method(f"message: {variable}") to logger.method("message: %s", variable) or logger.method("message: %s", variable, exc_info=True) for exception paths.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.
Inline comments:
In `@codeframe/core/tasks.py`:
- Around line 812-813: The logger.warning() calls are using f-string
interpolation which violates Ruff's LOG001 logging rule. Convert both
logger.warning() calls that currently use f-string format (such as the one with
"LLM generation failed ({e})") to use parameterized logging format instead,
passing the error variable as a separate argument to the logger method rather
than interpolating it into the string. This matches the parameterized logging
pattern already used elsewhere in the file.
In `@tests/core/test_conductor.py`:
- Around line 3-6: The test module test_conductor.py is missing the module-level
pytest marker required by the coding guidelines. Add the statement pytestmark =
pytest.mark.v2 after the import statements at the top of the file to ensure
these tests are included when running the v2 pytest marker filter with uv run
pytest -m v2.
- Around line 180-182: The assertion on line 181 is weakened by the `or
batch.status == BatchStatus.COMPLETED` condition, which allows the test to pass
even when "Execution plan:" is missing from the logs. Remove the `or
batch.status == BatchStatus.COMPLETED` clause from the assertion so it strictly
checks for the presence of "Execution plan:" in caplog.text. Keep line 182
separate to verify that the batch status is COMPLETED, ensuring the regression
test actually validates the logger-channel behavior as intended.
---
Nitpick comments:
In `@codeframe/core/conductor.py`:
- Line 226: Replace f-string logging with parameterized logging in the
conductor.py file. For exception handling paths where an exception variable is
available (such as the `logger.warning(f"[Supervisor] Classification failed:
{e}")` pattern), use parameterized logging by passing the exception as a
parameter and including `exc_info=True` to preserve the full traceback for
debugging. For non-exception logging contexts in the file, use parameterized
logging without exc_info for consistency and to enable proper lazy evaluation of
the logger. Change from logger.method(f"message: {variable}") to
logger.method("message: %s", variable) or logger.method("message: %s", variable,
exc_info=True) for exception paths.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: b98f02d4-717b-4d11-9a88-6186ba1d01d7
📒 Files selected for processing (4)
codeframe/core/conductor.pycodeframe/core/tasks.pytests/core/test_conductor.pytests/core/test_conductor_logging.py
- Add module-level `pytestmark = pytest.mark.v2` to test_conductor.py per CLAUDE.md convention (the file is also auto-marked by tests/core/conftest.py). - Tighten test_start_batch_strategy_parallel_works: assert the "Execution plan:" line is logged unconditionally (drop the weak `or batch.status == COMPLETED` escape that let the log-channel assertion pass vacuously).
|
Thanks @coderabbitai — triaged all three: 1. 2. 3. |
|
Tip For best results, initiate chat on the files or code changes.
🐇 ✨ ✏️ Learnings added
|
…view)
Replace the vague two-line `Error: {e}` + `Falling back to serial execution`
with a single specific message naming the dependency cycle, in both auto and
parallel strategy fallback paths.
|
Thanks @claude — applied finding #3 in the latest commit: the |
|
Claude finished @frankbria's task in 45s —— View job Review Update
Verified commit 8330c96. The # "auto" strategy path (line 717-719)
except CycleDetectedError as e:
logger.warning(f"Dependency cycle detected, falling back to serial execution: {e}")
_execute_serial(workspace, batch, on_event)
# "parallel" strategy path (line 727-729)
except CycleDetectedError as e:
logger.warning(f"Dependency cycle detected, falling back to serial execution: {e}")
_execute_serial(workspace, batch, on_event)Both catches now emit a single descriptive message that identifies the error type, the action taken, and the cause — down from the two-line bare Triage dispositions confirmed sound:
PR is in good shape. All three review items accounted for, 2641 tests passing, ruff clean. |
Follow-up Review — commits 2 & 3Good follow-through on the feedback. Two specific items addressed since my first review: Addressed
Still open (both are style nits, not blockers)
Neither item blocks merge. The architectural fix is correct, the test guard is solid, and the substantive feedback from both reviews has been incorporated. Verdict: Approved. |
Follow-up review (commits 2–3)The author addressed both CodeRabbit feedback and my prior review in the subsequent commits — no new concerns. Commit 2 ( Commit 3 ( logger.warning(f"Dependency cycle detected, falling back to serial execution: {e}")Remaining open items from prior review (both non-blocking):
Overall: The PR is correct, well-scoped, and tests are solid. The architecture violation is fixed, the AST guard prevents regressions, and the follow-up commits improved test quality and log message clarity. Ready to merge. |
1 participant
Summary
Fixes #649.
core/conductor.pyhad 84 bareprint()calls (Supervisor / GlobalFix / dependency-analysis chatter) andcore/tasks.pyhad 2 on the LLM-fallback path. Becauseconductor.pyis imported by the FastAPI server (ui/routers/batches_v2.py,tasks_v2.py), these leaked directly to server stdout — violating CLAUDE.md Architecture Rules #1 (core must be headless) and #3 (no UI/progress concepts in core).Changes
conductor.py: all 84print()→logger.{info,warning,error,debug}, mapped by intent:✗ Failed,Error executing task, validation failed) →error/warningPARTIAL, falling back to serial, timeouts, still blocked/failed) →warninginfodebug\n/spaces stripped;===/---separator-only prints dropped.tasks.py: LLM-fallbackprint()(lines 812, 818) →logger.warning.tests/core/test_conductor_logging.py: an AST-based guard asserting zeroprint()call nodes inconductor.py/tasks.py/runtime.py, plus acaplogbehavioral check on the supervisor failure path.caploginstead — same assertion intent, new channel (the exact behavior this issue introduces).Verified deviations from the issue's literal line list
runtime.py:1020,1091— theseprint()calls are inside docstringExample:blocks, not executable code. They never run or leak to stdout (AST confirms 0 real call nodes). Converting them would be incorrect, so they're left as-is.core/events.py— uses Richconsole.printfor CLI-intended output; left as-is per the issue.No CLI UX regression
cf work batch runkeeps its own Rich summary, andevents.pystill prints batch/task milestones (emit_for_workspace(..., print_event=True)calls are untouched). Only raw per-task chatter moves tologger, where it's properly captured in server logs.Acceptance criteria
print()inconductor.py(and the namedtasks.pylines) on server-reachable paths.logger.Testing
uv run pytest tests/core tests/integration tests/ui→ 2641 passed, 2 skipped.uv run ruff check→ clean.Known limitations
logger; the CLI shows milestones viaevents.py+ its own summary). This is the intended headless-core behavior.Summary by CodeRabbit
print()calls exist in server-reachable core modules and added a regression test for supervisor classification warning logging.