feat(cli): recce check-base + MCP startup freshness warning (M2)#1353
feat(cli): recce check-base + MCP startup freshness warning (M2)#1353
Conversation
…(M2) Adds `recce check-base` CLI subcommand with statuses FRESH / STALE_TIME / STALE_SHA / MISSING and --format json|text output. Exports check_base_freshness() as a module-level helper so mcp_server.py can call it at startup without duplicating logic (R8: cli.py-primary split). feat(mcp): emit stale-base warning at startup; expose base_status (M2, AC-3) Calls check_base_freshness() after load_context() in run_mcp_server() and prints a [Warning] line to stderr when status is STALE_TIME or STALE_SHA. Adds base_status field to get_server_info tool response so agents can programmatically detect stale state without parsing stderr. test: cover FRESH/STALE_TIME/STALE_SHA/MISSING + best-effort SHA absent (R9) 5 unit tests in tests/test_check_base.py; all pass. The absent-field test (test_sha_absent_no_raise) confirms DBT_GIT_SHA absence falls through to FRESH without raising. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> Signed-off-by: even-wei <evenwei@infuseai.io>
Codecov Report❌ Patch coverage is
... and 4 files with indirect coverage changes 🚀 New features to boost your workflow:
|
Updated Review — PR #1353 (round 3)Re-reviewed Verification on the pushed SHA: Resolution of round-2 finding
The chosen approach — extracting a shared helper rather than duplicating the inline join — is the right call. It eliminates the very class of drift that produced the round-2 finding (CLI fixed, MCP missed). Fresh pass on
|
| Pass | Result | Notes |
|---|---|---|
| A — Correctness & logic | PASS | Helper handles absolute / relative / None project_dir correctly. Smoke-tested: resolve_target_base_path(None, "target-base") == "target-base" (relative, resolved against CWD downstream — equivalent to pre-fix CLI behaviour). |
| C — Cross-reference | PASS | Import from recce.cli import check_base_freshness, resolve_target_base_path at mcp_server.py:2618; argument order matches helper signature; --target-base-path Click default ("target-base") makes the runtime None case impossible. |
| D — Error handling | PASS | Helper is pure (no I/O, cannot raise on documented input). MCP try/except wrapper unchanged; startup is still best-effort. |
| E — Test quality | PASS | 4 new tests cover relative / absolute / None / end-to-end-like paths. See NOTE 2. |
| F — Diff specifics | PASS (with NOTE) | Only two call sites of the inline join existed; both migrated. No stale callers. See NOTE 1. |
| G — Performance | N/A | Pure path operation. |
| H — Async/concurrency | PASS | Helper is sync and pure; safe in async context. The sync check_base_freshness file I/O inside async def run_mcp_server is pre-existing and gated to one-time startup. |
Notes
-
Diff is dominated by autoformatter cosmetic changes unrelated to the fix.
recce/cli.pyshows -445 lines andrecce/mcp_server.py-259 in the stat, but the actual MCP fix is ~30 lines (helper + call-site + import). The remainder is line-collapsing wheremain's style was reapplied after12fc92d0(merge frommain) preserved the branch's wrapped form. Behaviour-preserving but it inflates the review surface for a focused fix and complicates targeted reverts. Future fix-up commits should isolate format normalization into a separate commit so the actual fix can be reviewed without scrolling past hundreds of cosmetic changes. -
test_resolve_mcp_startup_finds_artifacts_under_project_dirdoes not actually exercise MCP startup. It calls the helper directly and runscheck_base_freshnesson the result. If a future change revertsmcp_server.pyto the inlinePath("./")join (i.e. drops the helper), this test would still pass — it would not catch the very regression its name advertises. Either rename totest_helper_resolves_under_project_dirto match what's verified, or upgrade the body to invokerun_mcp_serveragainst a fake context and assert onserver._base_status/ log output.
Verdict: GO
The functional fix is small, correct, well-tested, and addresses the round-2 finding cleanly. Both NOTEs are non-blocking — the first is a hygiene observation about commit shape, the second is a test-naming / test-strength suggestion that doesn't affect runtime behaviour.
wcchang1115
left a comment
There was a problem hiding this comment.
Claude Code Review: Critical issues found. See review comment for details.
Critical:
- base_status enum: standardize on all-lowercase ("fresh" / "stale_time" /
"stale_sha" / "missing" / "single_env" / "unknown"). Document the full
enum in get_server_info MCP tool description so LLM agents have a
stable contract.
- check-base: honor --project-dir (and DBT_PROJECT_DIR envvar) like every
other dbt-aware command. Resolves target-base-path relative to
project-dir unless absolute.
Warnings:
- MCP startup now also warns on `missing` (not just stale_*), with a
distinct rebuild-path message; `print(..., file=sys.stderr)` swapped
for `logger.warning` to match the rest of mcp_server.py.
- check-base exit codes split: 0 fresh, 1 missing, 2 stale_*. Documented
in the docstring so shell automation can branch without parsing JSON.
- Suggested-fix messages now interpolate target_base_path into the
--target-path flag (no more hardcoded "target-base" misleading users
on non-default paths).
- Add 6 CliRunner tests covering JSON schema, text rendering, exit-code
mapping per status, and --project-dir resolution.
Nitpicks:
- Drop unused target_path parameter from check_base_freshness().
- Bare `except Exception:` now logs at debug for diagnosability.
Refs: #1353
Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
Signed-off-by: even-wei <evenwei@infuseai.io>
|
@wcchang1115 Thanks for the thorough review — every finding was actionable and is now addressed in Critical
WarningsMCP startup asymmetry on Exit code conflation — Split into three classes: No Hardcoded Nitpicks
Verification on the pushed SHA: |
…ade-003-pre-pr-one-sentence Signed-off-by: even-wei <evenwei@infuseai.io> # Conflicts: # recce/cli.py
wcchang1115
left a comment
There was a problem hiding this comment.
Claude Code Review (updated): All prior critical and warning findings have been resolved in ef1fa71. Tests pass (11/11), lint clean. See updated review comment for evidence per finding.
wcchang1115
left a comment
There was a problem hiding this comment.
Claude Code Review (round 2): Fresh pass on the new code surfaced one real bug — MCP startup freshness check ignores --project-dir (parallel to the CLI bug, just at a different call site). See updated review comment for details and a drop-in fix.
Round-2 review finding: cli.py was fixed to join --project-dir onto a
relative --target-base-path, but the parallel call site in mcp_server.py
startup was not updated. So `recce mcp-server --project-dir /foo/bar`
looks for ./target-base/manifest.json relative to CWD instead of
/foo/bar/target-base/manifest.json — spurious "missing" warnings, or
worse, silently picking up a stale manifest from another project that
happens to live in CWD.
Extracted resolve_target_base_path() next to check_base_freshness in
cli.py so the join logic lives in exactly one place. CLI's check_base
and MCP's run_mcp_server startup both call the helper, and the
resolution can no longer drift across the two call sites.
- cli.py: new helper resolve_target_base_path(); check_base uses it
- mcp_server.py: startup freshness check uses the helper, joining
kwargs["project_dir"] with kwargs["target_base_path"]
- tests/test_check_base.py: 4 new tests
- test_resolve_relative_joins_with_project_dir
- test_resolve_absolute_bypasses_project_dir
- test_resolve_no_project_dir_uses_cwd
- test_resolve_mcp_startup_finds_artifacts_under_project_dir
(mirrors test_cli_project_dir_resolves against the helper)
Refs round-2 review:
#1353 (comment)
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Signed-off-by: even-wei <evenwei@infuseai.io>
|
@wcchang1115 Round-2 finding addressed in What changedExtracted def resolve_target_base_path(project_dir, target_base_path):
base = Path(target_base_path)
if base.is_absolute():
return str(base)
return str(Path(project_dir or "./") / base)
TestsMirrored
The MCP-startup test mirrors Verification
Format churn note
Cross-referenced "things that turned out fine"Confirmed your other-things-checked items are all still good after this change:
Asking for one more pass when convenient. |
wcchang1115
left a comment
There was a problem hiding this comment.
Round-3 review: round-2 finding resolved by b50c5b34. No BLOCKERs or ISSUEs on the fresh pass. Two non-blocking NOTEs in the review comment (diff bloated by unrelated autoformatter changes; the new "MCP startup" regression test doesn't actually exercise MCP startup). LGTM.
Summary
recce check-basesubcommand tocli.py; returns JSON withstatus(FRESH | STALE_TIME | STALE_SHA | MISSING),recommendation, and human-readablemessage.check_base_freshness()helper (48 h mtime threshold + optionalDBT_GIT_SHAmanifest comparison; STALE_SHA is best-effort — no raise when env var absent).check_base_freshness()afterload_context(); emits[Warning] Base artifacts are staleto stderr forSTALE_TIME/STALE_SHA(AC-3).base_statuson server object; exposed via_tool_get_server_info().tests/test_check_base.py: FRESH, STALE_TIME, STALE_SHA, MISSING, SHA-absent-no-raise — all pass.Test plan
uv run pytest tests/test_check_base.py -vCascade self-review evidence
Triplet review at every stage. Mechanical correctness verified:
recce check-baseClick command registered (grep-confirmed)check_base_freshness()helper called frommcp_server.pystartup afterload_context()(grep-confirmed)get_server_infoexposesbase_statusfieldspacedock-ops/docs/cascade/003-pre-pr-one-sentence/_trace/: design / build / verify / deliver all PASS first-tryHuman reviewer must check (cascade can't verify these)
metadata.env.DBT_GIT_SHAadapter consistencyDBT_GIT_SHAenv var is set? If adapter divergence exists, the STALE_SHA branch may fire inconsistently.recce.ymlordbt_project.yml-relative file, not just--freshness-threshold-hoursper invocation?recce check-baseships, the JSON output keys (status,recommendation,message,artifact_age_hours,base_sha,current_sha,threshold_hours) become a public contract that downstream agents parse. Forward-compat: if a future field is added, will agents tolerate it? Should keys be marked deprecated/removed via a versioning convention?base_statusenum stabilityReferences
Resolves recce-pre-pr-summary-in-one-sentence-claude-code-and-codex-2fd8dfb5866e (M2)
Paired PR: DataRecce/recce-claude-plugin#26 (M1, M4) — must merge after this one.
🤖 Generated with cascade workflow (triplet review at every stage: design / build / verify / deliver — all PASS).