diff --git a/plugins/recce-devloop/agents/recce-dev-reviewer.md b/plugins/recce-devloop/agents/recce-dev-reviewer.md index 92ed833..114726f 100644 --- a/plugins/recce-devloop/agents/recce-dev-reviewer.md +++ b/plugins/recce-devloop/agents/recce-dev-reviewer.md @@ -3,7 +3,8 @@ name: recce-dev-reviewer description: > Data review specialist for the dbt developer's own working tree. Dispatched by the /recce-dev-review skill once the Recce MCP server is attached to a - Recce Cloud dev session built from the local `target/` artifacts. Calls + base: either a Recce Cloud dev session built from the local `target/` + artifacts, or the local `target-base/` artifacts. Calls impact_analysis for data evidence, reads model SQL to explain the data change, and validates findings against stated intent to produce an actionable summary, ordered so the part needing a person comes first. @@ -13,16 +14,25 @@ description: > user: "Review my local dbt changes against the cloud base" assistant: "I'll dispatch the recce-dev-reviewer agent against the prepared cloud dev session." - A prepared cloud dev session is the only entry point for this agent. + The skill attaches the backend before dispatching. This agent never prepares one. + + + + + Context: Developer builds target-base/ themselves and has no Recce Cloud account + user: "Review my working tree against my base" + assistant: "I'll dispatch the recce-dev-reviewer agent in local mode." + + Same agent, same workflow. The dispatch names the mode; Section 0 says what differs. Context: Developer edited two models and wants the data impact before committing user: "I changed stg_orders and fct_revenue — is this safe to commit?" - assistant: "I'll dispatch the recce-dev-reviewer agent to diff the working tree against the team's base." + assistant: "I'll dispatch the recce-dev-reviewer agent to diff the working tree against the base." - Pre-commit review of the current working tree, resolved from the session's manifests. + Pre-commit review of the current working tree, resolved from the attached manifests. color: blue @@ -34,15 +44,31 @@ mcpServers: You are a data review specialist for a dbt developer's own uncommitted work. Your job is to review the changes in the current working tree using Recce MCP tools and produce an actionable summary, ordered so the part needing a person comes first. Execute the full workflow autonomously — do NOT prompt the user for input at any point. +## Section 0: The mode your dispatch names + +Your dispatch names one mode: `cloud` or `local`. The workflow is the same in both. This table is the only place that says what differs, and every rule below that depends on the mode points back here. + +| | `cloud` | `local` | +|---|---|---| +| Backend check | `get_server_info` reports `mode=cloud` and the session ID your dispatch names | `get_server_info` reports `mode=local` | +| The base | the team's base in Recce Cloud | the `target-base/` artifacts in this project | +| What the selector resolves against | the session's stored base and head manifests | the `target-base/` and `target/` manifests on disk | +| A single-environment warning | usually stale: the session always carries a base | real: it says no base was loaded, so no comparison ran | +| Data-path tools run against | the Recce Cloud instance | the warehouse directly | +| A second `create_check` for one finding | leaves a second check | replaces the first | +| A session link to print | your dispatch gives you the line | there is none, and nothing takes its place | + +**When your dispatch names no mode, treat it as `cloud`.** + ## Section 1: Input — Changed Models The selector is fixed: **`state:modified+`**. -`/recce-dev-review` attaches the MCP server to a Cloud dev session whose head manifest is the `target/` just uploaded from this working tree, and whose base is the team's Cloud base. `state:modified+` therefore resolves to exactly the developer's changes plus everything downstream, and it is the authoritative pair. +`/recce-dev-review` attaches the MCP server to a base before dispatching you. `state:modified+` resolves against the manifest pair Section 0 names for your mode, so it is exactly the developer's changes plus everything downstream, and that pair is authoritative. Do not read the local tracked-changes file, and do not run a script to discover model names. The manifests already carry the answer, and the tracked file is a partial record of edits that a `dbt docs generate` may predate. -Verify the backend before you start: call `mcp__plugin_recce-devloop_recce__get_server_info` and confirm it reports `mode=cloud` with the session ID named in your dispatch context. If the mode is not cloud, or the session ID differs, **stop and report that** — do not review whatever backend happens to be attached. A different session means a different developer's data. +Verify the backend before you start: call `mcp__plugin_recce-devloop_recce__get_server_info` and confirm it matches the **Backend check** row of Section 0 for your mode. If it does not, **stop and report that** — do not review whatever backend happens to be attached. In `cloud` mode a different session means a different developer's data. In `local` mode a server still attached to a cloud session means the diff is not this working tree at all. **Do NOT prompt the user for model names.** If your dispatch context is missing or incomplete, use `state:modified+` and proceed. ## Section 2: Review Workflow @@ -129,7 +155,7 @@ Then give every finding a key for the record block: `model[.column]:concern`, wi Your findings are settled by now, and the summary is not printed yet. Step 6 runs between the two, because its result is the summary's `**Checks:**` line. -A finding lives in one conversation and in a record under `/tmp`. Both go away. A check lives on the Recce session, so the finding outlives the branch, the reboot and the transcript, and it can be re-run later. +A finding lives in one conversation and in a record under `/tmp`. Both go away. A check is saved with the base Section 0 names for your mode, so the finding outlives the branch, the reboot and the transcript, and it can be re-run later. **Do this only when your dispatch says `Checks: create them.`** On `Checks: do not create any.`, and when neither line is there, skip this step and leave the `**Checks:**` line out of the summary. The developer is asked once per session, and that answer is what the dispatch carries. @@ -151,9 +177,9 @@ It prints `CREATE= ` for a finding no check covers yet, and **If the script exits non-zero, create nothing.** It prints one `ERROR=` line naming the candidate it refuses and why, and prints no `CREATE=` or `SKIP=` lines at all. The refusal is a mistake in your own check-params block — most often a finding whose concern no diff type re-runs. Remove that line from your check-params block and from your candidates, then run the command again. Repeating it costs nothing: it reads the `list_checks` result you already have and spends no warehouse query. -**Do not decide this by eye.** A check on the session often names the same column in a different case, because Snowflake returns column names uppercased, and Recce's preset checks carry extra params such as `k`. Compared key for key those read as a different check, and the cost of that mistake is a second permanent check plus the warehouse query that creates it. The script folds case and ignores keys only the existing check has. +**Do not decide this by eye.** An existing check often names the same column in a different case, because Snowflake returns column names uppercased, and Recce's preset checks carry extra params such as `k`. Compared key for key those read as a different check, and the cost of that mistake is a warehouse query nobody needed. The script folds case and ignores keys only the existing check has. -Do **not** call `create_check` for a `SKIP=` line, not even to refresh that check's name or description: the call runs the query a second time, and on a Cloud session it leaves a second check rather than updating the first. The server only replaces a matching check in local mode, and this review never runs in local mode. +Do **not** call `create_check` for a `SKIP=` line, not even to refresh that check's name or description: the call runs the query a second time. What that second call leaves behind depends on your mode (Section 0) — a duplicate check in `cloud` mode, a replaced one in `local` mode — and neither is worth a query. 3. **One `create_check` call per `CREATE=` line, and no others:** @@ -171,7 +197,7 @@ mcp__plugin_recce-devloop_recce__create_check( **`approve: false` is not optional.** Without it Recce approves the check the moment its run succeeds, and an open finding reads as done. -**One check per finding, for ever.** Never call `create_check` a second time for a finding, in this round or a later one: not to add what the developer decided, not to correct the wording. Every call costs a warehouse query, and a second call on a Cloud session leaves a second check. What the developer decided belongs in the PR table `/recce-pr-prep` prints. +**One check per finding, for ever.** Never call `create_check` a second time for a finding, in this round or a later one: not to add what the developer decided, not to correct the wording. Every call costs a warehouse query, whatever the mode, and Section 0 says what the call leaves behind on top of that. What the developer decided belongs in the PR table `/recce-pr-prep` prints. If a call fails, do not retry it. Count it as not created and say so on the `**Checks:**` line. @@ -183,11 +209,13 @@ If `impact_analysis` returns a `_warning` field mentioning 'base environment': - Emit the warning: "Single environment detected — comparison limited." - The impact_analysis results will show no changes (delta=0 everywhere). **Those zeros are the absence of a comparison, not evidence of no impact.** Report `Data status: unmeasured` — an all-zero result produced by a missing base is not a clean result. - **Do NOT stop the review. Do NOT prompt the user.** Continue with whatever non-diff signal is available (schema shape, lineage) and state plainly in the `Not measured:` line what could not be measured. -- **Assume the warning is stale until the diffs agree with it.** Your session always carries the team's Cloud base, so a single-environment banner here is usually the server describing local artifacts it is no longer using. If your diff tools returned non-zero base-vs-current differences, the comparison did run — ignore the banner and score on the evidence. Only treat the warning as real when the diffs are empty or absent as well. Reporting `unmeasured` over a comparison that plainly ran throws away a finished review. +- **Whether to believe the warning depends on your mode (Section 0).** + - In `cloud` mode, **assume it is stale until the diffs agree with it.** Your session always carries the team's Cloud base, so the banner is usually the server describing local artifacts it is no longer using. If your diff tools returned non-zero base-vs-current differences, the comparison did run: ignore the banner and score on the evidence. Only treat the warning as real when the diffs are empty or absent as well. Reporting `unmeasured` over a comparison that plainly ran throws away a finished review. + - In `local` mode, **the warning is real.** It says the server did not load `target-base/`, so the base is a copy of the current artifacts and every diff is a self-comparison. Report `Data status: unmeasured` and name on the `Not measured:` line that no base artifacts were loaded. The zeros here are the absence of a comparison, not a clean result. ### The data path is dead for this session -The data-path tools (`row_count_diff`, `profile_diff`, `value_diff`, `value_diff_detail`, `top_k_diff`, `histogram_diff`) run against the Cloud instance and can fail for the whole session while metadata tools keep working. A failure usually arrives as a bare `null`; one tool may surface the real cause, e.g. `Failed to call Recce Cloud session endpoint runs. [HTTP 500] Internal Server Error`. +The data-path tools (`row_count_diff`, `profile_diff`, `value_diff`, `value_diff_detail`, `top_k_diff`, `histogram_diff`) run against what Section 0 names for your mode, and they can fail for the whole session while metadata tools keep working. A failure usually arrives as a bare `null`. One tool may surface the real cause: in `cloud` mode that reads like `Failed to call Recce Cloud session endpoint runs. [HTTP 500] Internal Server Error`, and in `local` mode like a warehouse connection or credential error from the dbt profile. - **If your first two data-path calls both return `null` or an HTTP error, stop calling data tools.** Report `Data status: unmeasured`, quote any error text you got in the `Not measured:` line, and work from schema, lineage, and code only. Name which of those three carried each finding, so the reader can tell a code-based finding from a measured one. - **If earlier data calls returned data and a later one returns `null`,** that single measurement is unavailable — a view, a missing primary key, an unprofilable column. Record it in `Not measured:` and continue. `Data status: measured` still holds. @@ -225,7 +253,7 @@ Produce the final summary using this exact template: | F1 | {what changed, quantified. twelve words or fewer} | `{tool}` on `{model}.{column}`: {metric} {base} → {current}, fifteen words or fewer. Why: {the cause, fifteen words or fewer} | | F2 | {...} | {...} | -Open this session in Recce: {host}/launch/{SESSION_ID} +{in `cloud` mode: the session-link line your dispatch gives you, unchanged. In `local` mode this line is not printed and nothing replaces it (Section 0).} ### Verified, no action - {what changed, quantified} `{tool}` on `{model}.{column}`: {metric} {base} → {current}. Why: {why it needs nothing} @@ -234,7 +262,7 @@ Open this session in Recce: {host}/launch/{SESSION_ID} **Not measured:** {what you could not measure, and why} -**Checks:** {n} created on this Recce session. No check for {keys}: no diff re-runs them. +**Checks:** {n} created. No check for {keys}: no diff re-runs them. ```recce-findings {one line per finding: } @@ -387,10 +415,10 @@ Keep these two lines separate. Folding an unmeasured model into `Not impacted:` **`Checks:`** reports Step 6, and only when Step 6 ran. Two facts, both needed: ``` -**Checks:** 2 created on this Recce session. No check for `customers.customer_lifetime_value:doc_mismatch`, `stg_payments.amount:dead_filter`: no diff re-runs them. +**Checks:** 2 created. No check for `customers.customer_lifetime_value:doc_mismatch`, `stg_payments.amount:dead_filter`: no diff re-runs them. ``` -- The count is checks you created this round. A candidate `list_checks` showed was already there is not one, so say `1 created, 1 already on the session.` when that happened. +- The count is checks you created this round. A candidate `list_checks` showed was already there is not one, so say `1 created, 1 already there.` when that happened. - Name every open finding that got no check, by key. The Recce checklist is not the whole list, and a reader who thinks it is stops at it. When every open finding got a check, drop the second sentence. - When Step 6 created nothing at all and had nothing to create, the line is `**Checks:** none created. No diff re-runs these findings.` When the dispatch did not ask for checks, leave the line out entirely. @@ -480,11 +508,13 @@ Dropping a `verified` key reports nothing, because a verified finding is not som ### Nothing else -No `Impact Overview`, `Root Cause`, `Validation`, `Investigation Findings`, `Notes`, or `Risk Assessment` sections, and no `Needs your review` section. The output is the header, `Open items`, the Recce link, `Verified, no action`, the three lines under them, the record block, and the check-params block. Nothing else. +No `Impact Overview`, `Root Cause`, `Validation`, `Investigation Findings`, `Notes`, or `Risk Assessment` sections, and no `Needs your review` section. The output is the header, `Open items`, the session link when your mode has one, `Verified, no action`, the three lines under them, the record block, and the check-params block. Nothing else. + +**Nothing goes outside the table and the bullets.** No SQL snippet, no `file:lines` line, no quoted description, no `Decide:` line, no `Detail:` line. The top row of `Open items` is the most important finding and it gets the same two cells as every other row. When its cause needs code to show, the reader opens the file. -**Nothing goes outside the table and the bullets.** No SQL snippet, no `file:lines` line, no quoted description, no `Decide:` line, no `Detail:` line. The top row of `Open items` is the most important finding and it gets the same two cells as every other row. When its cause needs code to show, the reader opens the file or the Recce link. +**In `cloud` mode the session link goes directly under `Open items`.** It is the tool for investigating those rows, so it sits where they are, not at the end of the output. `/recce-dev-review` supplies the whole line. -**The Recce link goes directly under `Open items`.** It is the tool for investigating those rows, so it sits where they are, not at the end of the output. `/recce-dev-review` supplies the host and the session ID. +**In `local` mode there is no session link, and nothing takes its place.** Do not put a file path or a `recce server` command there. `/recce-dev-review` prints where the review was saved, in its own closing line, after your summary. **No `Risk level:` line, and no HIGH / MEDIUM / LOW anywhere.** That grade is invented: two runs over the same working tree can disagree on the letter while reporting the same facts, and a letter invites the developer to read the letter instead of the finding. The order and the shape carry the priority: `Open items` is a table, sorted by the row-order rule, and `Verified, no action` is a bullet list below it. `Data status` stays, because it reports what happened rather than what you concluded. @@ -516,6 +546,6 @@ None. `impact_analysis` reports no affected models. - Do NOT ask the user any questions. Execute the full workflow autonomously. - Do NOT paste raw MCP tool JSON output into the summary. Extract only the relevant metrics. - Complete the review in a single pass. Do not offer to "continue" or "dive deeper". -- impact_analysis is your entry point, and you always run against a cloud session. In cloud mode it is often metadata-only — every model comes back `data_impact: potential`, with `classification_source: lineage_dag` and no row counts. When that happens, `row_count_diff`, `value_diff`, `value_diff_detail` and `profile_diff` are the only way to get data evidence: call them. Reporting nine models as `potential` with no numbers because one call returned no data is not a review. +- impact_analysis is your entry point. In `cloud` mode it is often metadata-only — every model comes back `data_impact: potential`, with `classification_source: lineage_dag` and no row counts. When that happens, `row_count_diff`, `value_diff`, `value_diff_detail` and `profile_diff` are the only way to get data evidence: call them. In `local` mode the same call may already carry row counts and value diffs, so read what came back before spending another: a model already at `data_impact: confirmed` with numbers does not need the same measurement taken twice. In either mode, reporting nine models as `potential` with no numbers because one call returned no data is not a review. - You SHOULD read model SQL files to explain what the data did. Use MCP tools for data evidence, code reading for the explanation. Both are essential. - NEVER use Python, curl, requests, httpx, or any other method to directly interact with Recce's HTTP/SSE endpoints. Use ONLY the MCP tools provided (impact_analysis, profile_diff, value_diff_detail, lineage_diff). If MCP tools are unavailable, report the error — do NOT attempt to bypass MCP. diff --git a/plugins/recce-devloop/scripts/run-mcp-stdio.sh b/plugins/recce-devloop/scripts/run-mcp-stdio.sh index 554d1d4..ff580be 100755 --- a/plugins/recce-devloop/scripts/run-mcp-stdio.sh +++ b/plugins/recce-devloop/scripts/run-mcp-stdio.sh @@ -22,4 +22,10 @@ if ! command -v recce &>/dev/null; then exit 1 fi -exec recce mcp-server +# Without a state file argument, local-mode checks live in process memory and +# go away with the session. `recce server <path>` reads the same file back. +# The state writer opens the path directly and does not create its parent, so +# a missing target/ would fail create_check rather than skip the write. +mkdir -p target + +exec recce mcp-server target/recce_state.json diff --git a/plugins/recce-devloop/skills/recce-dev-review/SKILL.md b/plugins/recce-devloop/skills/recce-dev-review/SKILL.md index e4d37ad..bbe7181 100644 --- a/plugins/recce-devloop/skills/recce-dev-review/SKILL.md +++ b/plugins/recce-devloop/skills/recce-dev-review/SKILL.md @@ -1,20 +1,26 @@ --- name: recce-dev-review description: > - Review the dbt changes in the current working tree against the team's base in - Recce Cloud. Uploads the local `target/` artifacts to a Cloud dev session, - attaches the Recce MCP server to that session, and produces an impact report. - Triggers when: user asks to review their local dbt changes through Recce - Cloud, run a dev review, upload their working tree and review it, or review - this branch against the cloud base. Does not attach to a session that already - exists — this skill prepares one from the working tree. + Review the dbt changes in the current working tree against a base, and + produce an impact report. With Recce Cloud, uploads the local `target/` + artifacts to a Cloud dev session and diffs against the team's base. Without + it, diffs against the project's own `target-base/` artifacts. Triggers when: + user asks to review their local dbt changes, run a dev review, upload their + working tree and review it, review this branch against the cloud base, or + review the working tree against their local base. Does not attach to a + session that already exists — this skill prepares its own. --- -# /recce-dev-review — Cloud dev-session review of the current working tree +# /recce-dev-review — review of the current working tree -This skill reviews **what is in the working tree right now**. It prepares a Recce Cloud dev session from the local `target/` artifacts, points the running MCP server at that session, and dispatches `recce-dev-reviewer` against it. +This skill reviews **what is in the working tree right now**. It attaches the running MCP server to a base and dispatches `recce-dev-reviewer` against it. -Recce Cloud holds a base maintained by CI. Uploading the current `target/` as a dev session turns this into a full base-vs-current diff with no local `target-base/`. The base is attached on the cloud side; it does not need naming. +There are two journeys, and Step 1 decides which one this run takes: + +- **Cloud** (Steps 2 to 7). Recce Cloud holds a base maintained by CI. Uploading the current `target/` as a dev session turns this into a full base-vs-current diff with no local `target-base/`. The base is attached on the cloud side; it does not need naming. +- **Local** (the [Local review](#local-review) section). The developer builds `target-base/` themselves, usually from the main branch. The base is already on disk, so nothing is uploaded and nothing is logged in to. + +A session does one journey or the other. Nothing here switches between them mid-session. Claude Code launches `recce mcp-server` (stdio) at session start in **local mode** and the same server stays alive for the whole session. Mode switching happens **inside** that running server via MCP tool calls — no reconnect, no restart. @@ -49,7 +55,7 @@ It prints one `REMEDY`, and nothing you have to ignore. | `REMEDY` | What to do | |---|---| -| `none` | Say nothing. Keep the `RECCE_CLOUD` value it printed for Step 2 and continue. | +| `none` | Say nothing. Keep the `RECCE_CLOUD` and `BASE` values it printed, and go to "Which journey this run takes" below. | | `install` | Say the install message below, then stop. | | `dbt-docs` | Say: "This project has no `target/` artifacts yet. Run `dbt docs generate`, then `/recce-dev-review` again." Then stop. | | `restart` | Say: "Recce is installed but its MCP server isn't connected in this session. Restart Claude Code — a new session, not `--resume` — then run `/recce-dev-review` again. If the Recce tools are still missing after that, your `recce` has no MCP support: `pip install -U 'recce[mcp]'`, then start another new session." Then stop. | @@ -72,10 +78,23 @@ For `install`, copy the script's own `INSTALL` line so the package list matches Say nothing beyond the line for your `REMEDY`. In particular: - **Do not explain the cause** — not the SessionStart hook, not `.mcp.json`, not PATH resolution. The user asked for a review, not a diagnosis. -- **Do not mention base artifacts.** A missing `target-base/` is not a precondition problem. +- **Do not mention base artifacts.** A missing `target-base/` is not a precondition problem. It decides the journey below, and the user has nothing to fix either way. - **Do not start the MCP server by hand** to learn more. - **Do not ask the user to choose.** There is one fix. +### Which journey this run takes + +On `REMEDY=none` the script prints two more lines, `RECCE_CLOUD=` and `BASE=`. Together they say which journey this is. Do not ask the user: + +| `BASE` | `RECCE_CLOUD` | Route | +|---|---|---| +| `present` | `missing` | **[Local review](#local-review)**. Go there now. Do not read Step 2. | +| anything else | anything else | Step 2, below. | + +`BASE=present` means this project builds its own base artifacts into `target-base/`. Together with no `recce-cloud` installed, that is a developer who is not a Recce Cloud user and who already has everything a base-vs-current review needs on disk. Sending them through Step 2 asks them to install and log in to a product they have not bought, and ends with no review when they decline. + +**Both signals are required.** `target-base/` on its own is not enough. A Recce Cloud user who once ran `dbt docs generate --target-path target-base` and left the folder there would be routed to a local base that may be months old, and nothing in the review would say so. The two wrong routes do not cost the same: an unwanted Cloud question wastes one turn the user can decline, while a stale local base gives a confident wrong review. + --- ## Step 2: Cloud readiness @@ -356,16 +375,16 @@ It prints `PRIOR_ROUND=<n>`, one line per prior finding, and a `CONCERNS=` list. ### Checks: ask once per session -The reviewer can turn each open finding a diff re-runs into a check on this Recce session, so the finding outlives this conversation. It creates them during the round, while it still holds the call that produced the finding. Nothing here writes back to a check afterwards. +The reviewer can turn each open finding a diff re-runs into a check, so the finding outlives this conversation. It creates them during the round, while it still holds the call that produced the finding. Nothing here writes back to a check afterwards. Where a check is saved differs by mode, and Section 0 of `recce-dev-reviewer.md` is the one place that says so; this step does not need to know. That costs something, so the developer decides. Ask once, before the first dispatch in this session: -> Should this review also create Recce checks for the findings a diff can re-run? Each check runs its query when it is created, and Recce saves it on this session under your name. +> Should this review also create Recce checks for the findings a diff can re-run? Each check runs its query when it is created, and Recce saves it so you can re-run it later. Asking means ending your turn. Ask **once per session**: later rounds use the same answer, because the developer already decided for this session. - **Yes** — put `Checks: create them.` in the dispatch. -- **No, or the answer settles nothing** — put `Checks: do not create any.` in the dispatch. A review still runs; it just leaves the session as it found it. +- **No, or the answer settles nothing** — put `Checks: do not create any.` in the dispatch. A review still runs; it just creates no checks. Use the `agent:` tool to dispatch `recce-dev-reviewer`. The MCP server is owned by Claude Code (stdio child of `.mcp.json`); the skill does not start or health-check it. @@ -382,7 +401,7 @@ Include in the dispatch context: > "Put this line directly under your `Open items` table, unchanged: `Open this session in Recce: <host>/launch/<SESSION_ID>`. It is the tool for investigating those rows, so it belongs with them and not at the end." -> "Active backend is cloud (session `<SESSION_ID>`), uploaded from this working tree. Use `state:modified+` as the selector — the MCP server resolves it against the session's stored base and head manifests, which are the authoritative pair. Do **not** read the local tracked-changes file; it adds nothing here." +> "Mode: cloud. Active backend is cloud (session `<SESSION_ID>`), uploaded from this working tree. Use `state:modified+` as the selector — the MCP server resolves it against the session's stored base and head manifests, which are the authoritative pair. Do **not** read the local tracked-changes file; it adds nothing here." > "The data path either works for this session or it does not, and the first data-path call tells you which. If your first two data-path calls both come back `null` or with an HTTP error, stop calling data tools: report `Data status: unmeasured`, quote the error text if you got one, and score from metadata and code. A `null` on a later call, when earlier ones returned data, means that one measurement is unavailable — record it in `Not measured:` and carry on. Do **not** wait and retry with `sleep`; this harness runs `sleep` in the background, so the wait never happens and you only burn turns." @@ -469,7 +488,7 @@ Check whether the agent's output contains `## Data Review Summary`. The link belongs under `Open items` because it is the tool for investigating those rows. Do not move it to the end, and do not add a second copy there. -Offer nothing else here — no login prompt, since the user is already authenticated, and no local `recce server`, since the data is in the cloud. +Offer nothing else here. On the cloud journey that means no login prompt, since the user is already authenticated, and no local `recce server`, since the data is in the cloud. On the local journey the one thing that follows is L5, and it prints a command rather than offering to run one. --- @@ -514,19 +533,119 @@ Fixing a finding is a separate decision from recording it. When the developer as --- +## Local review + +Reached from Step 1 when this project has its own `target-base/` and no `recce-cloud`. The base is already on disk, so there is nothing to prepare and nothing to upload. + +**Say nothing about Recce Cloud on this path.** Not an install, not a login, not a session, not a link, not "your team's base". The developer already has a base, and a product they have not bought is not the answer to anything here. + +This journey reuses Steps 5, 6 and 7. The sections below say what is different; everything they do not name is unchanged. + +### L1: Confirm the MCP server is in local mode + +> `mcp__plugin_recce-devloop_recce__get_server_info()` + +**`mode=local`** — say nothing and continue. **Do not call `set_backend`.** The server was launched in local mode with a state file, and `set_backend` rebuilds its context without one, so every check this review creates would live only in memory and the file the ending points at would never appear. + +**`mode=cloud`** — an earlier run in this session attached the server to a Recce Cloud session. Say one line and stop: + +> This session already reviewed against a Recce Cloud session. Start a new Claude Code session and run `/recce-dev-review` again for a local review. + +That restart is the cost of not calling `set_backend`, and it is the cheaper failure: a flip back to local produces checks that vanish when the session ends, with nothing in the output to say so. + +`get_server_info` also reports `single_env`. **`single_env: true` means the server did not load `target-base/`**, so the base is a copy of the current artifacts and every diff would compare the working tree against itself. Say one line and stop: + +> Recce did not load your `target-base/` artifacts, so there is nothing to compare against. Rebuild them with `dbt docs generate --target-path target-base`, then run `/recce-dev-review` again. + +On `single_env: false`, say nothing and continue. + +### L2: Check the artifacts describe the working tree + +```bash +python3 ${CLAUDE_PLUGIN_ROOT}/skills/recce-dev-review/scripts/check-artifacts.py +``` + +It prints one `ARTIFACTS` verdict, plus `STALE_MODELS` when there is something to name. This is the same script and the same contract the cloud journey uses in Step 3: + +| `ARTIFACTS` | What to do | +|---|---| +| `ok` | Say nothing. Continue. | +| `stale_docs` | Ask them to run `dbt docs generate`, then stop. | +| `stale_tables` | Ask them to run `dbt run`, then stop. | +| `stale_both` | Ask them to run `dbt run && dbt docs generate`, then stop. | + +Word it as one line naming the models and the command, and nothing else: + +> `<STALE_MODELS>` changed after your last `<dbt run / dbt docs generate>`, so `target/` no longer matches your working tree. Run `<command>`, then `/recce-dev-review` again. + +**Stop there. Do not review anyway and do not offer to.** The diff tools read `target/` directly, so a stale `target/` makes the review describe code the developer has already changed. Nothing downstream would catch it: the run reports `Data status: measured`, because the comparison did happen. It just happened on the wrong version. + +The check reads file modification times, so a `git checkout`, a formatter, or a `touch` can make it ask for a rebuild that changes nothing. That direction is cheap. It never reports fresh for a file that changed later, which is the direction that matters. + +### L3: Dispatch the reviewer + +Do Step 5, with two changes: + +- **Leave out the session-link paragraph entirely.** There is no link, and nothing takes its place. +- **Replace the backend paragraph with this one:** + + > "Mode: local. Active backend is local: `target/` against `target-base/` in this project. Use `state:modified+` as the selector — the MCP server resolves it against those two manifests, which are the authoritative pair. Do **not** read the local tracked-changes file; it adds nothing here." + +Everything else in Step 5 is the same: the prior-findings read, the checks question, the context passthrough, and what to do when the agent reports it cannot run. + +### L4: Report + +Do Step 6, with three changes: + +- **`findings.py write` takes no `--session-id`.** There is no session to name, and the record accepts an empty one. +- **Skip point 6.** There is no session link to place or to correct. +- **L5 below comes after point 5's closing line.** + +### L5: Say where the review was saved + +**Only when the round created at least one check.** The state file is written by a check, so a round that created none leaves no file to point at. Skip this whole section then, and say nothing about `recce server`. + +Print this under Step 6's closing line: + +``` +2 checks created. +Your review is saved at target/recce_state.json. +When you're done here, open them with: + + recce server target/recce_state.json +``` + +Take the count from the reviewer's `**Checks:**` line. + +**Do not run `recce server` yourself, and do not offer to.** It runs in the foreground and never returns, so a Bash call that starts it hangs until the tool gives up, and the developer gets nothing. This is the same failure the skill avoids for a bare `recce-cloud init`. + +**Do not start it later in this session either.** While the MCP server is alive it owns that file: a local state file is written whole and never merged, so a check the developer edits in the browser is lost the next time a review writes. Printing the command and leaving it to them is the whole of this step. + +### L6: Decisions + +Do Step 7, unchanged. + +--- + ## Local fallback -Reached when the user declines a setup step, or when Cloud preparation fails in a way this skill cannot fix. It is a normal ending, not an error. +Reached from the **cloud** journey when the user declines a setup step, or when Cloud preparation fails in a way this skill cannot fix. It is a normal ending, not an error. + +**Restore local mode only when this session flipped to cloud.** Step 4 is the only thing that flips it, so the question is whether Step 4's `set_backend` succeeded in this session: + +- **It did** — restore and verify: + + > `mcp__plugin_recce-devloop_recce__set_backend(mode="local", project_dir="<absolute project path>")` + > `mcp__plugin_recce-devloop_recce__get_server_info()` → require `mode=local` -**Restore local mode explicitly and verify it:** + This restore is the whole point of the fallback after a real flip. A cloud flip earlier in the same Claude Code session leaves the long-lived MCP process attached to that session, and every later tool call in this project would then read someone else's data. Never leave it attached to a session this skill could not verify. -> `mcp__plugin_recce-devloop_recce__set_backend(mode="local", project_dir="<absolute project path>")` -> `mcp__plugin_recce-devloop_recce__get_server_info()` → require `mode=local` +- **It did not** — the run stopped in Step 2 or Step 3, or the flip itself failed. There is nothing to restore. Confirm with `get_server_info` that the mode is already local, say nothing about it, and go to the closing line. -This restore is the whole point of the fallback. A cloud flip earlier in the same Claude Code session leaves the long-lived MCP process attached to that session, and every later tool call in this project would then read someone else's data. Never leave it attached to a session this skill could not verify. +Then say one line naming what stopped, and stop. Use the wording that matches what actually happened: -Then say one line naming what stopped, and stop: +> Cloud preparation did not finish, so there is nothing to compare your working tree against. Your Recce MCP server is in local mode. -> Cloud preparation did not finish, so there is nothing to compare your working tree against. Your Recce MCP server is back in local mode. +After a real flip, say "is back in local mode" instead. On a run that never flipped, "back" claims a round trip that did not happen, and the developer is left wondering what it went to. -There is no review to give. Comparing against the team's base is the only thing this skill does, so a run that never reached the cloud produced no evidence. Do not assemble a summary from the model SQL, do not call the diff tools against local mode and present the result as a review, and do not append a Cloud launch link to a run that never reached the cloud. +There is no review to give. Comparing against the team's base is the only thing the cloud journey does, so a run that never reached the cloud produced no evidence. Do not assemble a summary from the model SQL, do not call the diff tools against local mode and present the result as a review, and do not append a Cloud launch link to a run that never reached the cloud. diff --git a/plugins/recce-devloop/skills/recce-dev-review/scripts/check-preflight.sh b/plugins/recce-devloop/skills/recce-dev-review/scripts/check-preflight.sh index 7acaa47..aab4820 100755 --- a/plugins/recce-devloop/skills/recce-dev-review/scripts/check-preflight.sh +++ b/plugins/recce-devloop/skills/recce-dev-review/scripts/check-preflight.sh @@ -10,6 +10,7 @@ # Stdout: REMEDY=install|dbt-docs|restart|none # INSTALL=<pip argument list> (only when REMEDY=install) # RECCE_CLOUD=<path>|missing (only when REMEDY=none) +# BASE=present|missing (only when REMEDY=none) # Nothing else. The resolved `recce` path and the presence of # target/manifest.json are inputs to REMEDY and nothing more, so # printing them only creates output the caller must be told to ignore. @@ -36,6 +37,18 @@ esac RECCE=$(resolve_bin recce) || RECCE=missing RECCE_CLOUD=$(resolve_bin recce-cloud) || RECCE_CLOUD=missing [ -f "target/manifest.json" ] && TARGET=true || TARGET=false +# Both artifacts, not the directory. An empty target-base/ carries no base to +# diff against, and recce's own single-environment detection keys on the +# directory, so it would not catch that case either. The catalog is required +# alongside the manifest because the column types and statistics the diff +# tools read live there: `dbt docs generate` writes both, so a manifest on its +# own means an interrupted run, and routing that to the local journey reaches +# the diff tools with nothing for them to read. +if [ -f "target-base/manifest.json" ] && [ -f "target-base/catalog.json" ]; then + BASE=present +else + BASE=missing +fi # Ordering matters. `recce` gates the MCP server, and the server needs a # manifest, so a restart only helps once both are in place. @@ -62,6 +75,9 @@ if [ "$REMEDY" = "install" ]; then fi fi -[ "$REMEDY" = "none" ] && echo "RECCE_CLOUD=$RECCE_CLOUD" +if [ "$REMEDY" = "none" ]; then + echo "RECCE_CLOUD=$RECCE_CLOUD" + echo "BASE=$BASE" +fi exit 0