From afde2efcf6467b9cc4ff0db62a603d28579c2f2c Mon Sep 17 00:00:00 2001 From: Magal Date: Thu, 21 May 2026 11:33:31 -0300 Subject: [PATCH] feat(story-6.2): implement code reviewer deep audit with independent get_impact and find_dead_code queries The code-review skills (bmad-code-review, gds-code-review) now independently query Memtrace during reviews. Step 1 runs get_impact on modified symbols and find_dead_code on modified files via memtrace-adapter.mjs with --check-freshness --summarize. Step 2 Acceptance Auditor cross-references structural data against the diff, raising decision_needed findings for unhandled downstream dependencies and patch findings for dead code and high blast radius. FR12: Code Review Agent queries get_impact on PRs for downstream dependency audit FR13: Code Review Agent executes find_dead_code to block orphaned code 9 files modified across source templates and installed copies. --- .../skills/bmad-code-review/customize.toml | 1 + .../steps/step-01-gather-context.md | 40 ++++++++++++++++++- .../bmad-code-review/steps/step-02-review.md | 26 ++++++++++++ .agents/skills/gds-code-review/customize.toml | 1 + .../steps/step-01-gather-context.md | 40 ++++++++++++++++++- .../gds-code-review/steps/step-02-review.md | 26 ++++++++++++ .../bmad-code-review/customize.toml | 1 + .../steps/step-01-gather-context.md | 40 ++++++++++++++++++- .../bmad-code-review/steps/step-02-review.md | 31 +++++++++++++- 9 files changed, 201 insertions(+), 5 deletions(-) diff --git a/.agents/skills/bmad-code-review/customize.toml b/.agents/skills/bmad-code-review/customize.toml index 26ba792f9..eadbbd868 100644 --- a/.agents/skills/bmad-code-review/customize.toml +++ b/.agents/skills/bmad-code-review/customize.toml @@ -32,6 +32,7 @@ activation_steps_append = [] persistent_facts = [ "file:{project-root}/**/project-context.md", + "Memtrace structural deep audit is available for independent code review verification. Use the adapter at _bmad/scripts/memtrace/memtrace-adapter.mjs with --query get_impact --target --check-freshness --summarize to independently verify the blast radius of modified symbols. Use --query find_dead_code --target --check-freshness for dead code detection in modified modules. Run these during Step 1 (gather context) after constructing the diff. All queries MUST use sequential for...of with await — NEVER Promise.all. Index freshness check via list_indexed_repositories is mandatory before trusting graph output. Structural audit is advisory/supplemental — NEVER block the review on Memtrace availability. Timeout (MEMTRACE_MCP_ERROR_TIMEOUT) or stale index ([FRESHNESS] in STDERR) means skip and continue.", ] # Scalar: executed when the workflow reaches its final step, diff --git a/.agents/skills/bmad-code-review/steps/step-01-gather-context.md b/.agents/skills/bmad-code-review/steps/step-01-gather-context.md index 22b9fbd3d..ec152358f 100644 --- a/.agents/skills/bmad-code-review/steps/step-01-gather-context.md +++ b/.agents/skills/bmad-code-review/steps/step-01-gather-context.md @@ -3,6 +3,8 @@ diff_output: '' # set at runtime spec_file: '' # set at runtime (path or empty) review_mode: '' # set at runtime: "full" or "no-spec" story_key: '' # set at runtime when discovered from sprint status +memtrace_blast_radius: '' # set at runtime: structured blast radius data or "unavailable"/"partial" +memtrace_dead_code: '' # set at runtime: structured dead code data or "unavailable"/"partial" --- # Step 1: Gather Context @@ -75,9 +77,45 @@ story_key: '' # set at runtime when discovered from sprint status - If the user opts to chunk: agree on the first group, narrow `{diff_output}` accordingly, and list the remaining groups for the user to note for follow-up runs. - If the user declines: proceed as-is with the full diff. +7. **Run structural deep audit queries (Memtrace).** If the repository is indexed by Memtrace, independently verify the diff's structural impact. This step is DIAGNOSTIC — the review continues regardless of availability. + + **Check Availability:** + - Use `list_indexed_repositories` to confirm the project repo is indexed + - Check the `last_indexed_at` value — if older than 30 minutes, flag as stale and skip graph queries + - If not indexed or stale, set `{memtrace_blast_radius}` = `"unavailable"` and `{memtrace_dead_code}` = `"unavailable"`, then skip to CHECKPOINT + + **Extract modified symbols from the diff:** + - Parse `{diff_output}` to identify modified functions, methods, classes, and exported variables + - Extract symbol names from changed lines (lines starting with `+` or `-` in function/class/method declarations) + - De-duplicate and limit to at most 15 symbols (prioritize: exported > internal, functions > variables) + - For each symbol, note its containing file path + - If no modified symbols are extracted (e.g., only config files, comments, or whitespace changes), skip both blast radius and dead code queries — set `{memtrace_blast_radius}` = `"empty"` and `{memtrace_dead_code}` = `"empty"` + + **Run blast radius audit (`get_impact`):** + - For each extracted symbol, call the adapter: + `node _bmad/scripts/memtrace/memtrace-adapter.mjs --target --query get_impact --check-freshness --summarize` + - Process STRICTLY SEQUENTIALLY using `for...of` with `await` — NEVER `Promise.all` + - On exit 0: collect `summarized.critical_dependents`, `summarized.module_impact`, `summarized.total_affected` + - On exit 1 with `[FRESHNESS]` in STDERR: note "Index stale — skipping blast radius for " and continue + - On exit 1 with `MEMTRACE_MCP_ERROR_TIMEOUT`: note "MCP timeout — skipping blast radius for " and continue + - Set `{memtrace_blast_radius}` to the structured results (or `"partial"` if some queries failed, or `"unavailable"` if ALL queries failed) + + **Run dead code audit (`find_dead_code`):** + - For each UNIQUE modified file (not per-symbol), call the adapter: + `node _bmad/scripts/memtrace/memtrace-adapter.mjs --target --query find_dead_code --check-freshness` + - Process STRICTLY SEQUENTIALLY using `for...of` with `await` + - On exit 0: collect the list of dead code symbols in that file + - On failure: note the failure and continue with remaining files + - Set `{memtrace_dead_code}` to the structured results (or `"partial"` if some queries failed, or `"unavailable"` if ALL queries failed) + + **Graceful Degradation:** + - If `list_indexed_repositories` returns empty or the project repo is not indexed: skip ALL queries, set both variables to `"unavailable"` + - If any individual query times out or fails: skip that query only, mark results as `"partial"`, continue with remaining symbols/files + - NEVER block or halt the review on Memtrace availability — the structural audit is supplemental intelligence + ### CHECKPOINT -Present a summary before proceeding: diff stats (files changed, lines added/removed), `{review_mode}`, and loaded spec/context docs (if any). HALT and wait for user confirmation to proceed. +Present a summary before proceeding: diff stats (files changed, lines added/removed), `{review_mode}`, loaded spec/context docs (if any), and structural audit status (if `{memtrace_blast_radius}` is not empty/unavailable: if `"partial"`, note "structural audit partially complete — {partial_count}/{total_count} symbol queries failed"; if fully available, include "blast radius queried for {count} symbols, dead code checked in {count} files"). HALT and wait for user confirmation to proceed. ## NEXT diff --git a/.agents/skills/bmad-code-review/steps/step-02-review.md b/.agents/skills/bmad-code-review/steps/step-02-review.md index 4bf1c59b9..92e392cff 100644 --- a/.agents/skills/bmad-code-review/steps/step-02-review.md +++ b/.agents/skills/bmad-code-review/steps/step-02-review.md @@ -50,6 +50,32 @@ failed_layers: '' # set at runtime: comma-separated list of layers that failed o > - **If the story involves dead-code removal (find_dead_code, dead-code in tasks) BUT no "Dead Code Pitfall Validation Report" exists in the spec file** → raise a `patch` finding: "Missing Dead Code Pitfall Validation Report — story involved dead-code removal but no pitfall validation was performed via validate-dead-code.mjs." > - **If neither dead-code removal nor a pitfall validation report exists**: skip this gate (story does not involve dead-code). > + > **Structural Deep Audit — Memtrace Verification:** + > + > If `{memtrace_blast_radius}` and `{memtrace_dead_code}` are available (not `"unavailable"` or empty), you have access to independently-computed structural data. Use this to verify the diff's claims against the actual codebase graph. + > + > **Blast Radius Verification:** + > - Cross-reference each modified symbol in the diff against `{memtrace_blast_radius}` results. + > - If a symbol has `critical_dependents` (any depth) that are NOT modified in the diff → raise a `decision_needed` finding: **"Unhandled downstream dependency: `` depends on modified `` — diff does not include test or mitigation."** + > - Evidence: list the specific dependent name(s) and the blast radius data + > - If the blast radius `total_affected` for any symbol exceeds 20 → raise a `patch` finding: **"High blast radius: `` affects `` dependents (depth ``) — consider narrower refactor scope or expanded test coverage."** + > - If the diff or commit message claims "no downstream impact" but blast radius shows dependents → raise a `decision_needed` finding: **"Downstream impact claim falsified: diff claims no impact but blast radius shows `` affected symbols at depth 1+."** + > - If a blast radius query returned `"partial"` for some symbols: note which symbols were not verified and proceed with available data only. + > + > **Dead Code Audit:** + > - Check whether any `{memtrace_dead_code}` findings overlap with lines ADDED in modified files (new code in `+` lines). + > - If a modified file introduces a NEW function/method/class that also appears in the dead code results → raise a `patch` finding: **"New dead code introduced: `` in `` — added but has zero callers in the codebase graph."** + > - If a modified file (`+` or `-` lines) contains EXISTING dead code symbols that were NOT removed → raise a `patch` finding: **"Pre-existing dead code unaddressed: `` in `` — appears in dead code results but was not cleaned up in this change."** + > - Do NOT flag dead code in files that the diff did not touch. + > + > **If `{memtrace_blast_radius}` or `{memtrace_dead_code}` is `"unavailable"` or empty:** + > - Note in the review output: "Structural deep audit unavailable — Memtrace not indexed or queries failed. Proceeding with text-based review only." + > - DO NOT raise any structural-audit-specific findings (no blast radius or dead code flags). + > + > **If `{memtrace_blast_radius}` or `{memtrace_dead_code}` is `"partial"`:** + > - Apply the blast radius rules ONLY if `{memtrace_blast_radius}` has complete data; apply dead code rules ONLY if `{memtrace_dead_code}` has complete data. + > - Note which symbols were not verified due to query failures. + > > Output findings as a Markdown list. Each finding: one-line title, which quality gate rule it violates, and evidence from the diff/story file. 3. **Subagent failure handling**: If any subagent fails, times out, or returns empty results, append the layer name to `{failed_layers}` (comma-separated) and proceed with findings from the remaining layers. diff --git a/.agents/skills/gds-code-review/customize.toml b/.agents/skills/gds-code-review/customize.toml index 6aba24dcd..78a4dadb9 100644 --- a/.agents/skills/gds-code-review/customize.toml +++ b/.agents/skills/gds-code-review/customize.toml @@ -32,6 +32,7 @@ activation_steps_append = [] persistent_facts = [ "file:{project-root}/**/project-context.md", + "Memtrace structural deep audit is available for independent code review verification. Use the adapter at _bmad/scripts/memtrace/memtrace-adapter.mjs with --query get_impact --target --check-freshness --summarize to independently verify the blast radius of modified symbols. Use --query find_dead_code --target --check-freshness for dead code detection in modified modules. Run these during Step 1 (gather context) after constructing the diff. All queries MUST use sequential for...of with await — NEVER Promise.all. Index freshness check via list_indexed_repositories is mandatory before trusting graph output. Structural audit is advisory/supplemental — NEVER block the review on Memtrace availability. Timeout (MEMTRACE_MCP_ERROR_TIMEOUT) or stale index ([FRESHNESS] in STDERR) means skip and continue.", ] # Scalar: executed when the workflow reaches Step 4 (Present and Act), diff --git a/.agents/skills/gds-code-review/steps/step-01-gather-context.md b/.agents/skills/gds-code-review/steps/step-01-gather-context.md index 8294a8c24..ed499546f 100644 --- a/.agents/skills/gds-code-review/steps/step-01-gather-context.md +++ b/.agents/skills/gds-code-review/steps/step-01-gather-context.md @@ -3,6 +3,8 @@ diff_output: '' # set at runtime spec_file: '' # set at runtime (path or empty) review_mode: '' # set at runtime: "full" or "no-spec" story_key: '' # set at runtime when discovered from sprint status +memtrace_blast_radius: '' # set at runtime: structured blast radius data or "unavailable"/"partial" +memtrace_dead_code: '' # set at runtime: structured dead code data or "unavailable"/"partial" --- # Step 1: Gather Context @@ -75,9 +77,45 @@ story_key: '' # set at runtime when discovered from sprint status - If the user opts to chunk: agree on the first group, narrow `{diff_output}` accordingly, and list the remaining groups for the user to note for follow-up runs. - If the user declines: proceed as-is with the full diff. +7. **Run structural deep audit queries (Memtrace).** If the repository is indexed by Memtrace, independently verify the diff's structural impact. This step is DIAGNOSTIC — the review continues regardless of availability. + + **Check Availability:** + - Use `list_indexed_repositories` to confirm the project repo is indexed + - Check the `last_indexed_at` value — if older than 30 minutes, flag as stale and skip graph queries + - If not indexed or stale, set `{memtrace_blast_radius}` = `"unavailable"` and `{memtrace_dead_code}` = `"unavailable"`, then skip to CHECKPOINT + + **Extract modified symbols from the diff:** + - Parse `{diff_output}` to identify modified functions, methods, classes, and exported variables + - Extract symbol names from changed lines (lines starting with `+` or `-` in function/class/method declarations) + - De-duplicate and limit to at most 15 symbols (prioritize: exported > internal, functions > variables) + - For each symbol, note its containing file path + - If no modified symbols are extracted (e.g., only config files, comments, or whitespace changes), skip both blast radius and dead code queries — set `{memtrace_blast_radius}` = `"empty"` and `{memtrace_dead_code}` = `"empty"` + + **Run blast radius audit (`get_impact`):** + - For each extracted symbol, call the adapter: + `node _bmad/scripts/memtrace/memtrace-adapter.mjs --target --query get_impact --check-freshness --summarize` + - Process STRICTLY SEQUENTIALLY using `for...of` with `await` — NEVER `Promise.all` + - On exit 0: collect `summarized.critical_dependents`, `summarized.module_impact`, `summarized.total_affected` + - On exit 1 with `[FRESHNESS]` in STDERR: note "Index stale — skipping blast radius for " and continue + - On exit 1 with `MEMTRACE_MCP_ERROR_TIMEOUT`: note "MCP timeout — skipping blast radius for " and continue + - Set `{memtrace_blast_radius}` to the structured results (or `"partial"` if some queries failed, or `"unavailable"` if ALL queries failed) + + **Run dead code audit (`find_dead_code`):** + - For each UNIQUE modified file (not per-symbol), call the adapter: + `node _bmad/scripts/memtrace/memtrace-adapter.mjs --target --query find_dead_code --check-freshness` + - Process STRICTLY SEQUENTIALLY using `for...of` with `await` + - On exit 0: collect the list of dead code symbols in that file + - On failure: note the failure and continue with remaining files + - Set `{memtrace_dead_code}` to the structured results (or `"partial"` if some queries failed, or `"unavailable"` if ALL queries failed) + + **Graceful Degradation:** + - If `list_indexed_repositories` returns empty or the project repo is not indexed: skip ALL queries, set both variables to `"unavailable"` + - If any individual query times out or fails: skip that query only, mark results as `"partial"`, continue with remaining symbols/files + - NEVER block or halt the review on Memtrace availability — the structural audit is supplemental intelligence + ### CHECKPOINT -Present a summary before proceeding: diff stats (files changed, lines added/removed), `{review_mode}`, and loaded spec/context docs (if any). HALT and wait for user confirmation to proceed. +Present a summary before proceeding: diff stats (files changed, lines added/removed), `{review_mode}`, loaded spec/context docs (if any), and structural audit status (if `{memtrace_blast_radius}` is not empty/unavailable: if `"partial"`, note "structural audit partially complete — {partial_count}/{total_count} symbol queries failed"; if fully available, include "blast radius queried for {count} symbols, dead code checked in {count} files"). HALT and wait for user confirmation to proceed. ## NEXT diff --git a/.agents/skills/gds-code-review/steps/step-02-review.md b/.agents/skills/gds-code-review/steps/step-02-review.md index 8aafdaa06..d68ec5991 100644 --- a/.agents/skills/gds-code-review/steps/step-02-review.md +++ b/.agents/skills/gds-code-review/steps/step-02-review.md @@ -50,6 +50,32 @@ failed_layers: '' # set at runtime: comma-separated list of layers that failed o > - **If the story involves dead-code removal (find_dead_code, dead-code in tasks) BUT no "Dead Code Pitfall Validation Report" exists in the spec file** → raise a `patch` finding: "Missing Dead Code Pitfall Validation Report — story involved dead-code removal but no pitfall validation was performed via validate-dead-code.mjs." > - **If neither dead-code removal nor a pitfall validation report exists**: skip this gate (story does not involve dead-code). > + > **Structural Deep Audit — Memtrace Verification:** + > + > If `{memtrace_blast_radius}` and `{memtrace_dead_code}` are available (not `"unavailable"` or empty), you have access to independently-computed structural data. Use this to verify the diff's claims against the actual codebase graph. + > + > **Blast Radius Verification:** + > - Cross-reference each modified symbol in the diff against `{memtrace_blast_radius}` results. + > - If a symbol has `critical_dependents` (any depth) that are NOT modified in the diff → raise a `decision_needed` finding: **"Unhandled downstream dependency: `` depends on modified `` — diff does not include test or mitigation."** + > - Evidence: list the specific dependent name(s) and the blast radius data + > - If the blast radius `total_affected` for any symbol exceeds 20 → raise a `patch` finding: **"High blast radius: `` affects `` dependents (depth ``) — consider narrower refactor scope or expanded test coverage."** + > - If the diff or commit message claims "no downstream impact" but blast radius shows dependents → raise a `decision_needed` finding: **"Downstream impact claim falsified: diff claims no impact but blast radius shows `` affected symbols at depth 1+."** + > - If a blast radius query returned `"partial"` for some symbols: note which symbols were not verified and proceed with available data only. + > + > **Dead Code Audit:** + > - Check whether any `{memtrace_dead_code}` findings overlap with lines ADDED in modified files (new code in `+` lines). + > - If a modified file introduces a NEW function/method/class that also appears in the dead code results → raise a `patch` finding: **"New dead code introduced: `` in `` — added but has zero callers in the codebase graph."** + > - If a modified file (`+` or `-` lines) contains EXISTING dead code symbols that were NOT removed → raise a `patch` finding: **"Pre-existing dead code unaddressed: `` in `` — appears in dead code results but was not cleaned up in this change."** + > - Do NOT flag dead code in files that the diff did not touch. + > + > **If `{memtrace_blast_radius}` or `{memtrace_dead_code}` is `"unavailable"` or empty:** + > - Note in the review output: "Structural deep audit unavailable — Memtrace not indexed or queries failed. Proceeding with text-based review only." + > - DO NOT raise any structural-audit-specific findings (no blast radius or dead code flags). + > + > **If `{memtrace_blast_radius}` or `{memtrace_dead_code}` is `"partial"`:** + > - Apply the blast radius rules ONLY if `{memtrace_blast_radius}` has complete data; apply dead code rules ONLY if `{memtrace_dead_code}` has complete data. + > - Note which symbols were not verified due to query failures. + > > Output findings as a Markdown list. Each finding: one-line title, which quality gate rule it violates, and evidence from the diff/story file. 3. **Subagent failure handling**: If any subagent fails, times out, or returns empty results, append the layer name to `{failed_layers}` (comma-separated) and proceed with findings from the remaining layers. diff --git a/src/bmm-skills/4-implementation/bmad-code-review/customize.toml b/src/bmm-skills/4-implementation/bmad-code-review/customize.toml index 26ba792f9..eadbbd868 100644 --- a/src/bmm-skills/4-implementation/bmad-code-review/customize.toml +++ b/src/bmm-skills/4-implementation/bmad-code-review/customize.toml @@ -32,6 +32,7 @@ activation_steps_append = [] persistent_facts = [ "file:{project-root}/**/project-context.md", + "Memtrace structural deep audit is available for independent code review verification. Use the adapter at _bmad/scripts/memtrace/memtrace-adapter.mjs with --query get_impact --target --check-freshness --summarize to independently verify the blast radius of modified symbols. Use --query find_dead_code --target --check-freshness for dead code detection in modified modules. Run these during Step 1 (gather context) after constructing the diff. All queries MUST use sequential for...of with await — NEVER Promise.all. Index freshness check via list_indexed_repositories is mandatory before trusting graph output. Structural audit is advisory/supplemental — NEVER block the review on Memtrace availability. Timeout (MEMTRACE_MCP_ERROR_TIMEOUT) or stale index ([FRESHNESS] in STDERR) means skip and continue.", ] # Scalar: executed when the workflow reaches its final step, diff --git a/src/bmm-skills/4-implementation/bmad-code-review/steps/step-01-gather-context.md b/src/bmm-skills/4-implementation/bmad-code-review/steps/step-01-gather-context.md index 22b9fbd3d..ec152358f 100644 --- a/src/bmm-skills/4-implementation/bmad-code-review/steps/step-01-gather-context.md +++ b/src/bmm-skills/4-implementation/bmad-code-review/steps/step-01-gather-context.md @@ -3,6 +3,8 @@ diff_output: '' # set at runtime spec_file: '' # set at runtime (path or empty) review_mode: '' # set at runtime: "full" or "no-spec" story_key: '' # set at runtime when discovered from sprint status +memtrace_blast_radius: '' # set at runtime: structured blast radius data or "unavailable"/"partial" +memtrace_dead_code: '' # set at runtime: structured dead code data or "unavailable"/"partial" --- # Step 1: Gather Context @@ -75,9 +77,45 @@ story_key: '' # set at runtime when discovered from sprint status - If the user opts to chunk: agree on the first group, narrow `{diff_output}` accordingly, and list the remaining groups for the user to note for follow-up runs. - If the user declines: proceed as-is with the full diff. +7. **Run structural deep audit queries (Memtrace).** If the repository is indexed by Memtrace, independently verify the diff's structural impact. This step is DIAGNOSTIC — the review continues regardless of availability. + + **Check Availability:** + - Use `list_indexed_repositories` to confirm the project repo is indexed + - Check the `last_indexed_at` value — if older than 30 minutes, flag as stale and skip graph queries + - If not indexed or stale, set `{memtrace_blast_radius}` = `"unavailable"` and `{memtrace_dead_code}` = `"unavailable"`, then skip to CHECKPOINT + + **Extract modified symbols from the diff:** + - Parse `{diff_output}` to identify modified functions, methods, classes, and exported variables + - Extract symbol names from changed lines (lines starting with `+` or `-` in function/class/method declarations) + - De-duplicate and limit to at most 15 symbols (prioritize: exported > internal, functions > variables) + - For each symbol, note its containing file path + - If no modified symbols are extracted (e.g., only config files, comments, or whitespace changes), skip both blast radius and dead code queries — set `{memtrace_blast_radius}` = `"empty"` and `{memtrace_dead_code}` = `"empty"` + + **Run blast radius audit (`get_impact`):** + - For each extracted symbol, call the adapter: + `node _bmad/scripts/memtrace/memtrace-adapter.mjs --target --query get_impact --check-freshness --summarize` + - Process STRICTLY SEQUENTIALLY using `for...of` with `await` — NEVER `Promise.all` + - On exit 0: collect `summarized.critical_dependents`, `summarized.module_impact`, `summarized.total_affected` + - On exit 1 with `[FRESHNESS]` in STDERR: note "Index stale — skipping blast radius for " and continue + - On exit 1 with `MEMTRACE_MCP_ERROR_TIMEOUT`: note "MCP timeout — skipping blast radius for " and continue + - Set `{memtrace_blast_radius}` to the structured results (or `"partial"` if some queries failed, or `"unavailable"` if ALL queries failed) + + **Run dead code audit (`find_dead_code`):** + - For each UNIQUE modified file (not per-symbol), call the adapter: + `node _bmad/scripts/memtrace/memtrace-adapter.mjs --target --query find_dead_code --check-freshness` + - Process STRICTLY SEQUENTIALLY using `for...of` with `await` + - On exit 0: collect the list of dead code symbols in that file + - On failure: note the failure and continue with remaining files + - Set `{memtrace_dead_code}` to the structured results (or `"partial"` if some queries failed, or `"unavailable"` if ALL queries failed) + + **Graceful Degradation:** + - If `list_indexed_repositories` returns empty or the project repo is not indexed: skip ALL queries, set both variables to `"unavailable"` + - If any individual query times out or fails: skip that query only, mark results as `"partial"`, continue with remaining symbols/files + - NEVER block or halt the review on Memtrace availability — the structural audit is supplemental intelligence + ### CHECKPOINT -Present a summary before proceeding: diff stats (files changed, lines added/removed), `{review_mode}`, and loaded spec/context docs (if any). HALT and wait for user confirmation to proceed. +Present a summary before proceeding: diff stats (files changed, lines added/removed), `{review_mode}`, loaded spec/context docs (if any), and structural audit status (if `{memtrace_blast_radius}` is not empty/unavailable: if `"partial"`, note "structural audit partially complete — {partial_count}/{total_count} symbol queries failed"; if fully available, include "blast radius queried for {count} symbols, dead code checked in {count} files"). HALT and wait for user confirmation to proceed. ## NEXT diff --git a/src/bmm-skills/4-implementation/bmad-code-review/steps/step-02-review.md b/src/bmm-skills/4-implementation/bmad-code-review/steps/step-02-review.md index bbc1f9a82..12bd1f82b 100644 --- a/src/bmm-skills/4-implementation/bmad-code-review/steps/step-02-review.md +++ b/src/bmm-skills/4-implementation/bmad-code-review/steps/step-02-review.md @@ -22,8 +22,35 @@ failed_layers: '' # set at runtime: comma-separated list of layers that failed o - **Edge Case Hunter** — receives `{diff_output}` and read access to the project. Invoke via the `bmad-review-edge-case-hunter` skill. - - **Acceptance Auditor** (only if `{review_mode}` = `"full"`) — receives `{diff_output}`, the content of the file at `{spec_file}`, and any loaded context docs. Its prompt: - > You are an Acceptance Auditor. Review this diff against the spec and context docs. Check for: violations of acceptance criteria, deviations from spec intent, missing implementation of specified behavior, contradictions between spec constraints and actual code. Output findings as a Markdown list. Each finding: one-line title, which AC/constraint it violates, and evidence from the diff. + - **Acceptance Auditor** (only if `{review_mode}` = `"full"`) — receives `{diff_output}`, the content of the file at `{spec_file}`, and any loaded context docs. Its prompt: + > You are an Acceptance Auditor. Review this diff against the spec and context docs. Check for: violations of acceptance criteria, deviations from spec intent, missing implementation of specified behavior, contradictions between spec constraints and actual code. + > + > **Structural Deep Audit — Memtrace Verification:** + > + > If `{memtrace_blast_radius}` and `{memtrace_dead_code}` are available (not `"unavailable"` or empty), you have access to independently-computed structural data. Use this to verify the diff's claims against the actual codebase graph. + > + > **Blast Radius Verification:** + > - Cross-reference each modified symbol in the diff against `{memtrace_blast_radius}` results. + > - If a symbol has `critical_dependents` (any depth) that are NOT modified in the diff → raise a `decision_needed` finding: **"Unhandled downstream dependency: `` depends on modified `` — diff does not include test or mitigation."** + > - Evidence: list the specific dependent name(s) and the blast radius data + > - If the blast radius `total_affected` for any symbol exceeds 20 → raise a `patch` finding: **"High blast radius: `` affects `` dependents (depth ``) — consider narrower refactor scope or expanded test coverage."** + > - If the diff or commit message claims "no downstream impact" but blast radius shows dependents → raise a `decision_needed` finding: **"Downstream impact claim falsified: diff claims no impact but blast radius shows `` affected symbols at depth 1+."** + > - If a blast radius query returned `"partial"` for some symbols: note which symbols were not verified and proceed with available data only. + > + > **Dead Code Audit:** + > - Check whether any `{memtrace_dead_code}` findings overlap with lines ADDED in modified files (new code in `+` lines). + > - If a modified file introduces a NEW function/method/class that also appears in the dead code results → raise a `patch` finding: **"New dead code introduced: `` in `` — added but has zero callers in the codebase graph."** + > - If a modified file (`+` or `-` lines) contains EXISTING dead code symbols that were NOT removed → raise a `patch` finding: **"Pre-existing dead code unaddressed: `` in `` — appears in dead code results but was not cleaned up in this change."** + > - Do NOT flag dead code in files that the diff did not touch. + > + > **If `{memtrace_blast_radius}` or `{memtrace_dead_code}` is `"unavailable"` or empty:** + > - Note in the review output: "Structural deep audit unavailable — Memtrace not indexed or queries failed. Proceeding with text-based review only." + > - DO NOT raise any structural-audit-specific findings (no blast radius or dead code flags). + > + > **If `{memtrace_blast_radius}` or `{memtrace_dead_code}` is `"partial"`:** + > - Apply the blast radius rules ONLY if `{memtrace_blast_radius}` has complete data; apply dead code rules ONLY if `{memtrace_dead_code}` has complete data. + > + > Output findings as a Markdown list. Each finding: one-line title, which AC/constraint it violates, and evidence from the diff. 3. **Subagent failure handling**: If any subagent fails, times out, or returns empty results, append the layer name to `{failed_layers}` (comma-separated) and proceed with findings from the remaining layers.