fix(mcp): recent_runs sees rotated .log.N siblings - #22
Conversation
Operator-facing symptom: in /var/log/plane-conductor/ logrotate truncates the live `<ts>-<ws>-<nick>-<short>.log` to 0 bytes and parks the real content as `<ts>-<ws>-<nick>-<short>.log.1` (and .log.2, ...). recent_runs globbed only `*.log`, so an audit of «what did our agents actually do last week» returned a wall of empty stubs and the real history sat invisible. Confirmed against a production log dir: 350 empty `.log` stubs vs 20 non-empty `.log.N` files — exactly the spread that made the recent coinex audit impossible to perform through the MCP tool and forced raw shell ls of the directory. Two changes: - `_LOG_NAME_RE` now accepts an optional `.N` suffix on the `.log` extension, so the parser doesn't drop rotated siblings. - recent_runs globs `*.log*`, groups the matches by (timestamp, workspace, nickname, issue_short), and keeps only the variant with the largest size_bytes per group — the human asking «what did this agent run output» wants the file holding the actual stdout, not the zero-byte stub. Sort is now stable on timestamp (was on filename, which worked while `.log` was the only extension but ordered `.log` ahead of `.log.1` lexicographically — wrong if we'd ever returned both). Added tests/test_mcp_server_recent_runs.py: rotated-pair dedup, multi-rotation biggest-wins, distinct-runs separation, filters + limit, non-matching siblings (`.log.bak`, README.md) stay ignored. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
Warning Rate limit exceeded
You’ve run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (3)
WalkthroughUpdated ChangesLogrotate-aware recent_runs deduplication
🎯 2 (Simple) | ⏱️ ~12 minutes 🚥 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 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 |
Single line-wrap diff flagged by `ruff format --check` in CI on PR #22. No behavioural change. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/plane_conductor/mcp_server.py (1)
175-176:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winValidate
limitto prevent negative-value behavior.Negative
limitcurrently yields unexpected subsets instead of an explicit “no results / invalid input” behavior. Guardlimit <= 0before slicing.Proposed fix
def recent_runs( workspace: str | None = None, nickname: str | None = None, issue_prefix: str | None = None, limit: int = 20, ) -> list[dict[str, Any]]: @@ ld = _log_dir() if not ld.exists(): return [] + if limit <= 0: + return [] @@ - return out[:limit] + return out[:limit]Also applies to: 225-225
🤖 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 `@src/plane_conductor/mcp_server.py` around lines 175 - 176, The function that declares "limit: int = 20) -> list[dict[str, Any]]" needs to explicitly guard against non-positive limits: check if limit <= 0 at the start of the function and return an empty list (or raise a ValueError if you prefer) before any slicing occurs so negative values cannot produce unexpected subsets; apply the same guard to the other function/block around the second occurrence (the other place that accepts "limit" near line 225) so both use the same early-return/validation for the limit parameter.
🤖 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.
Outside diff comments:
In `@src/plane_conductor/mcp_server.py`:
- Around line 175-176: The function that declares "limit: int = 20) ->
list[dict[str, Any]]" needs to explicitly guard against non-positive limits:
check if limit <= 0 at the start of the function and return an empty list (or
raise a ValueError if you prefer) before any slicing occurs so negative values
cannot produce unexpected subsets; apply the same guard to the other
function/block around the second occurrence (the other place that accepts
"limit" near line 225) so both use the same early-return/validation for the
limit parameter.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: a80313ec-4df7-4487-93d3-79c9eee6e6ec
📒 Files selected for processing (2)
src/plane_conductor/mcp_server.pytests/test_mcp_server_recent_runs.py
The file has been generated by uv but never tracked. plane-conductor is a deployable webhook orchestrator (not a redistributable library), so pinning the resolved dependency tree gives the same reproducibility win every other service in the modern Python toolchain expects: CI installs the exact same versions and hashes as the developer's machine, supply-chain swaps are caught by hash verification, and a fresh `uv sync` on a contributor's laptop doesn't drift away from production. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Two related fixes surfaced by PR #22: 1. CodeRabbit flagged `recent_runs(limit=<=0)` returning a tail-slice instead of an explicit empty list — added the early-return guard and pinned the behaviour with two tests. 2. The pre-commit mypy hook didn't list fastapi / typer / uvicorn / mcp in its additional_dependencies, so any contributor with hooks installed got a wall of pre-existing «Untyped decorator», «Cannot find module fastapi» errors that don't appear in CI (CI installs `.[dev]` which pulls them in). That gap was why the F5 ruff-format failure earlier slipped through to CI — pre-commit was effectively unusable locally. Hook now matches CI; `pre-commit run --all-files` passes clean. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Summary
mcp__plane-conductor__recent_runsglobbed only*.log, so once logrotatetruncated the live
<ts>-<ws>-<nick>-<short>.logto 0 bytes and parked thecontent as
<ts>-<ws>-<nick>-<short>.log.1, the MCP returned a wall ofzero-byte stubs and the real history sat invisible. Hit during a coinex audit
this week — production log dir had 350 empty stubs vs 20 non-empty
.log.Nfiles, and I had to fall back to raw
lsto find anything.Two changes in
mcp_server.py:_LOG_NAME_REaccepts an optional.Nsuffix on.log.recent_runsglobs*.log*, groups matches by(timestamp, workspace, nickname, issue_short), keeps only the variant with the largestsize_bytesper group, and sorts on timestamp (was on filename — fine whileonly
.logwas returned, wrong once.logand.log.1coexistlexicographically).
Added a small targeted test file (
tests/test_mcp_server_recent_runs.py) — 5cases covering rotated-pair dedup, multi-rotation max-wins, distinct-runs
separation, filters + limit, and non-matching siblings (
.log.bak,README.md)correctly ignored.
Test plan
205 passed, 7 skipped(e2e gated)./var/log/plane-conductor/dir after merge — callrecent_runs(workspace='coinex')and confirm the response carries the.log.1files with non-zerosize_bytesinstead of the empty.logstubs.🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
Tests