feat: consolidate AI review workflows + fix architecture generation#118
Conversation
Merge ai-review.yml, claude-review-phase1/2/3.yml into a single ai-code-review.yml with 3 jobs: - preflight: gate (bot-actor, draft, size, label checks) - review: comment-only review + conditional PAL MCP consensus - autofix: opt-in via ai-patch label, creates draft PR Key changes: - Default behavior: comment-only review (was 4 concurrent reviews) - Consensus: opt-in via ai-consensus label or high-stakes files - Autofix: opt-in via ai-patch label, blocked for forks/protected files - All actions SHA-pinned, tool syntax normalized to colon form - Cost monitor updated to reference consolidated workflow - Recursive trigger safety: GITHUB_TOKEN only, never PAT Saves $4.50-13.50 per PR by eliminating duplicate reviews. Addresses: DreamServer PR #683 review items #1, #2, #3 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Tests were checking for deleted phase2/phase3 workflow files. Updated to test the new ai-code-review.yml consolidated workflow: - Renamed fixtures: phase2_config/phase3_config → ai_code_review_config - Replaced TestPhase2Workflow and TestPhase3Workflow with TestAICodeReviewWorkflow validating the 3-job consolidated structure - Tests now validate: preflight gate, review+consensus, autofix, protected file checks, GITHUB_TOKEN usage, draft PR creation All 12 tests ported to new structure + assertions preserved. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Changed test to look for 'gate' step ID (actual name in workflow) instead of 'high-stakes' string. The high-stakes file detection is part of the 'Evaluate gates' step with id='gate'. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
The old script did a full overwrite of ARCHITECTURE.md using generic GitNexus query calls, producing a skeletal 46-line file that lost the Mermaid diagram, functional area descriptions, execution flow traces, and testing architecture. The new script uses marker-based updates (<!-- auto:overview --> markers) to only replace the data-driven Codebase Overview table while preserving all hand-curated content. Also fixes file counting bugs (Rust excluded target/, workflows count only .github/workflows/*.yml) and adds multi-repo detection support. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
📝 WalkthroughWalkthroughThe architecture generation script moved from full-file rewrites to marker-based partial updates, swapped GitNexus query usage for Cypher-based stats with repo resolution, tightened dependency and validation checks, changed file-count exclusions and overview rendering, and updated the workflow to ignore changes to both Changes
Sequence Diagram(s)sequenceDiagram
participant Script as "generate-architecture.sh"
participant FS as "Filesystem (ARCHITECTURE.md)"
participant GN as "GitNexus (npx gitnexus)"
participant Git as "git (diff/commit/PR)"
Script->>FS: read ARCHITECTURE.md
alt markers missing
FS-->>Script: error -> exit
else markers present
Script->>GN: run `gitnexus list` (resolve repo)
GN-->>Script: repo list
Script->>GN: run `gitnexus cypher` (stats)
GN-->>Script: cypher results (rows)
Script->>FS: replace content between markers (awk)
Script->>FS: update "Last updated" (sed)
Script->>Git: git diff ARCHITECTURE.md, AGENTS.md
alt changes detected
Script->>Git: commit & push / create PR
else
Git-->>Script: no changes
end
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
Reviewer's GuideConsolidates AI code review GitHub workflows into a single configurable pipeline and rewrites architecture generation to perform safe, marker-based partial updates of ARCHITECTURE.md with corrected metrics and file counting logic. File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
PAL MCP Consensus Not AvailableSecurity-sensitive files were detected, but PAL MCP multi-model consensus is not configured. To enable consensus: Configure Claude Code Review results are still available above. |
Claude Code Review (via AWS Bedrock)OverviewPR consolidates architecture generation from full-file regeneration to marker-based partial updates, preserving hand-curated content. Main changes: rewrite of Critical IssuesNone blocking - code is functional and represents a design improvement. High Priority
Medium Priority
Positive Observations✅ Excellent Architecture Pattern - Marker-based partial updates is the right approach for preserving hand-curated content ✅ Good Error Messages - Clear ✅ Proper Shell Best Practices - Uses ✅ Clear Intent - Comments explain the "why" not just the "what" ✅ Improved File Counting - Better exclusions (venv, node_modules, target) vs previous version ✅ Dependency Validation - Checks for required commands upfront ✅ Number Formatting - Adds commas to large numbers for readability ✅ Comprehensive Output Validation - Checks line count, heading, and expected sections Suggestions for Follow-up
Review Summary
Recommendation: Approve with suggestions. The architectural improvement (marker-based updates) is significant and well-executed. Address high-priority items in a follow-up PR to improve robustness. Generated by Claude Code Review (AWS Bedrock - Sonnet 4.5) |
GitNexus Impact Analysis⚪ NONE Overall Risk Level
Per-File Impact
Affected ProcessesAffected ModulesDetailed Impact by FileGenerated by GitNexus impact analysis |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 3 issues, and left some high level feedback:
- The
format_numberhelper currently uses a simplesedexpression that will only insert a single comma on some inputs; consider either relying on raw counts (no formatting) or switching to a more robust formatter to avoid misleading output for larger values. - The
sed -iinvocation ingenerate-architecture.shis GNU-specific and will fail on macOS/BSD without a backup suffix; if you expect this script to be run locally as well as in CI, consider using a portable pattern likesed -i.bak ... && rm *.bakor routing throughperl/awkinstead.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The `format_number` helper currently uses a simple `sed` expression that will only insert a single comma on some inputs; consider either relying on raw counts (no formatting) or switching to a more robust formatter to avoid misleading output for larger values.
- The `sed -i` invocation in `generate-architecture.sh` is GNU-specific and will fail on macOS/BSD without a backup suffix; if you expect this script to be run locally as well as in CI, consider using a portable pattern like `sed -i.bak ... && rm *.bak` or routing through `perl`/`awk` instead.
## Individual Comments
### Comment 1
<location path=".github/scripts/generate-architecture.sh" line_range="38" />
<code_context>
+ local repo_list
+ repo_list=$(npx gitnexus list 2>&1 || true)
+ local count
+ count=$(echo "$repo_list" | grep -oP 'Indexed Repositories \(\K[0-9]+' 2>/dev/null || echo "1")
+ if [[ "$count" -le 1 ]]; then
+ # Single repo — no --repo flag needed
</code_context>
<issue_to_address>
**issue (bug_risk):** Using `grep -P` may break on environments where `grep` lacks PCRE support (e.g., macOS default `grep`).
Because `-P` isn’t supported by BSD `grep`, this will return an empty match on those systems and cause `count` to default to `1`, potentially skipping `--repo` even when multiple repos are indexed. Please use a more portable parsing approach (e.g., `sed`/`awk`, `grep -E` with an adjusted pattern, or have `gitnexus list` output a machine-readable format consumable by `jq`).
</issue_to_address>
### Comment 2
<location path=".github/scripts/generate-architecture.sh" line_range="90-91" />
<code_context>
-## Codebase Overview
+# Format numbers with commas
+format_number() {
+ echo "$1" | sed ':a;s/\B[0-9]\{3\}\>$/,&/;ta'
+}
</code_context>
<issue_to_address>
**issue (bug_risk):** Number formatting function is brittle and likely not doing full thousands-grouping as intended.
This sed pattern only inserts a single comma at the end and relies on non‑portable `\B`/`\>` regex extensions, so values like `1234567` may not become `1,234,567`, and behavior can vary between sed implementations. Consider a POSIX‑compatible sed/awk approach for repeated grouping, or drop the formatting and keep raw counts to avoid cross‑platform issues.
</issue_to_address>
### Comment 3
<location path=".github/scripts/generate-architecture.sh" line_range="167" />
<code_context>
+
+# ── Update timestamp ─────────────────────────────────────────────────
+GENERATED_DATE=$(date -u '+%Y-%m-%d %H:%M UTC')
+sed -i "s|^> Last updated:.*|> Last updated: ${GENERATED_DATE}|" "$ARCH_FILE"
+
+# ── Validate output ──────────────────────────────────────────────────
</code_context>
<issue_to_address>
**issue (bug_risk):** In-place `sed -i` usage is not portable between GNU sed and BSD/macOS sed.
On macOS/BSD, `sed -i` requires a backup suffix (e.g. `-i ''`), while GNU `sed` rejects an empty string, so this command will fail on at least one platform. To keep the script portable, either branch on the detected platform when calling `sed`, or avoid `-i` by writing to a temporary file and moving it back (as you do in `replace_marker_content`).
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| local repo_list | ||
| repo_list=$(npx gitnexus list 2>&1 || true) | ||
| local count | ||
| count=$(echo "$repo_list" | grep -oP 'Indexed Repositories \(\K[0-9]+' 2>/dev/null || echo "1") |
There was a problem hiding this comment.
issue (bug_risk): Using grep -P may break on environments where grep lacks PCRE support (e.g., macOS default grep).
Because -P isn’t supported by BSD grep, this will return an empty match on those systems and cause count to default to 1, potentially skipping --repo even when multiple repos are indexed. Please use a more portable parsing approach (e.g., sed/awk, grep -E with an adjusted pattern, or have gitnexus list output a machine-readable format consumable by jq).
| format_number() { | ||
| echo "$1" | sed ':a;s/\B[0-9]\{3\}\>$/,&/;ta' |
There was a problem hiding this comment.
issue (bug_risk): Number formatting function is brittle and likely not doing full thousands-grouping as intended.
This sed pattern only inserts a single comma at the end and relies on non‑portable \B/\> regex extensions, so values like 1234567 may not become 1,234,567, and behavior can vary between sed implementations. Consider a POSIX‑compatible sed/awk approach for repeated grouping, or drop the formatting and keep raw counts to avoid cross‑platform issues.
|
|
||
| # ── Update timestamp ───────────────────────────────────────────────── | ||
| GENERATED_DATE=$(date -u '+%Y-%m-%d %H:%M UTC') | ||
| sed -i "s|^> Last updated:.*|> Last updated: ${GENERATED_DATE}|" "$ARCH_FILE" |
There was a problem hiding this comment.
issue (bug_risk): In-place sed -i usage is not portable between GNU sed and BSD/macOS sed.
On macOS/BSD, sed -i requires a backup suffix (e.g. -i ''), while GNU sed rejects an empty string, so this command will fail on at least one platform. To keep the script portable, either branch on the detected platform when calling sed, or avoid -i by writing to a temporary file and moving it back (as you do in replace_marker_content).
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/scripts/generate-architecture.sh:
- Around line 71-87: The current cypher_query() call can silently produce empty
output and the subsequent assignments to SYMBOL_COUNT, EDGE_COUNT, and
PROCESS_COUNT coerce failures to "0"; change this so failures are fatal: have
cypher_query() return a non-zero exit code on error (preserve and echo the error
text), remove the "|| echo '0'" fallback, and after each jq parse (for
SYMBOL_COUNT, EDGE_COUNT, PROCESS_COUNT) validate the result is non-empty and
numeric (e.g., regex test or grep -E '^[0-9]+$'); if any parse fails, write a
clear error via >&2 with the cypher_query output and exit 1 so the workflow
fails rather than producing bogus "0" metrics. Ensure you reference the
cypher_query function and the SYMBOL_COUNT/EDGE_COUNT/PROCESS_COUNT assignments
when making the changes.
In `@ARCHITECTURE.md`:
- Around line 8-18: Update the hand-curated CI/CD sections in ARCHITECTURE.md to
match the auto overview: change both occurrences of "23 workflows" (the "CI/CD
(23 workflows)" heading and "23 GitHub Actions workflows") to "20 workflows",
remove or revise the obsolete "3-phase review" wording and any references to the
removed claude-review-phase1/2/3.yml files so the document no longer lists those
three workflows, and ensure the surrounding descriptive text (the paragraph
referencing the review phases) reflects the current single/updated review
process; search for the exact strings "CI/CD (23 workflows)", "23 GitHub Actions
workflows", "3-phase", and "claude-review-phase1/2/3.yml" to locate and edit the
lines.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 2cc311ec-dbfe-49e9-a28e-1ed2ba54eb99
📒 Files selected for processing (3)
.github/scripts/generate-architecture.sh.github/workflows/update-architecture.ymlARCHITECTURE.md
| cypher_query() { | ||
| local query="$1" | ||
| local result | ||
| # shellcheck disable=SC2086 | ||
| if ! result=$(npx gitnexus cypher $REPO_FLAG "$query" 2>&1); then | ||
| echo "::warning::Cypher query failed: $result" >&2 | ||
| echo "" | ||
| return | ||
| fi | ||
| echo "" | ||
| echo "$result" | ||
| } | ||
|
|
||
| # ── Generate ARCHITECTURE.md ────────────────────────────────────────── | ||
| GENERATED_DATE=$(date -u '+%Y-%m-%d %H:%M UTC') | ||
|
|
||
| cat > ARCHITECTURE.md <<EOF | ||
| # SuperClaude Architecture | ||
| echo "Querying GitNexus knowledge graph..." >&2 | ||
|
|
||
| > Auto-generated by GitNexus knowledge graph analysis. | ||
| > Last updated: ${GENERATED_DATE} | ||
| SYMBOL_COUNT=$(cypher_query 'MATCH (n) RETURN count(n) as c' | jq -r '.markdown' | tail -1 | tr -d '| ' || echo "0") | ||
| EDGE_COUNT=$(cypher_query 'MATCH ()-[r]->() RETURN count(r) as c' | jq -r '.markdown' | tail -1 | tr -d '| ' || echo "0") | ||
| PROCESS_COUNT=$(cypher_query 'MATCH (p:Process) RETURN count(p) as c' | jq -r '.markdown' | tail -1 | tr -d '| ' || echo "0") |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
parsed=$(
if ! out=$(npx gitnexus cypher 'THIS IS NOT CYPHER' 2>&1); then
printf '%s' ""
else
printf '%s' "$out"
fi | jq -r '.markdown' | tail -1 | tr -d '| ' || echo "0"
)
printf 'Parsed count after a forced cypher failure: %s\n' "$parsed"
test "$parsed" = "0"Repository: Tony363/SuperClaude
Length of output: 149
🏁 Script executed:
cat -n .github/scripts/generate-architecture.sh | head -100Repository: Tony363/SuperClaude
Length of output: 3875
🏁 Script executed:
sed -n '88,150p' .github/scripts/generate-architecture.shRepository: Tony363/SuperClaude
Length of output: 1964
🏁 Script executed:
sed -n '150,200p' .github/scripts/generate-architecture.shRepository: Tony363/SuperClaude
Length of output: 1262
Fail the workflow when GitNexus stats cannot be parsed.
If gitnexus cypher fails or its output shape changes, cypher_query() returns an empty string and lines 85-87 coerce the parse failure to 0. The script then keeps going and publishes a plausible-looking ARCHITECTURE.md with bogus counts (all metrics become "0") instead of failing fast. Downstream validation only checks file structure (line count, headings, sections), not metric correctness.
🛠️ Suggested fix
+extract_count() {
+ jq -er '.markdown' | tail -1 | tr -d '| ' | grep -E '^[0-9]+$'
+}
+
cypher_query() {
local query="$1"
local result
# shellcheck disable=SC2086
- if ! result=$(npx gitnexus cypher $REPO_FLAG "$query" 2>&1); then
- echo "::warning::Cypher query failed: $result" >&2
- echo ""
- return
- fi
+ result=$(npx gitnexus cypher $REPO_FLAG "$query" 2>&1) || {
+ echo "::error::Cypher query failed: $result" >&2
+ exit 1
+ }
echo "$result"
}
-SYMBOL_COUNT=$(cypher_query 'MATCH (n) RETURN count(n) as c' | jq -r '.markdown' | tail -1 | tr -d '| ' || echo "0")
-EDGE_COUNT=$(cypher_query 'MATCH ()-[r]->() RETURN count(r) as c' | jq -r '.markdown' | tail -1 | tr -d '| ' || echo "0")
-PROCESS_COUNT=$(cypher_query 'MATCH (p:Process) RETURN count(p) as c' | jq -r '.markdown' | tail -1 | tr -d '| ' || echo "0")
+SYMBOL_COUNT=$(cypher_query 'MATCH (n) RETURN count(n) as c' | extract_count)
+EDGE_COUNT=$(cypher_query 'MATCH ()-[r]->() RETURN count(r) as c' | extract_count)
+PROCESS_COUNT=$(cypher_query 'MATCH (p:Process) RETURN count(p) as c' | extract_count)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| cypher_query() { | |
| local query="$1" | |
| local result | |
| # shellcheck disable=SC2086 | |
| if ! result=$(npx gitnexus cypher $REPO_FLAG "$query" 2>&1); then | |
| echo "::warning::Cypher query failed: $result" >&2 | |
| echo "" | |
| return | |
| fi | |
| echo "" | |
| echo "$result" | |
| } | |
| # ── Generate ARCHITECTURE.md ────────────────────────────────────────── | |
| GENERATED_DATE=$(date -u '+%Y-%m-%d %H:%M UTC') | |
| cat > ARCHITECTURE.md <<EOF | |
| # SuperClaude Architecture | |
| echo "Querying GitNexus knowledge graph..." >&2 | |
| > Auto-generated by GitNexus knowledge graph analysis. | |
| > Last updated: ${GENERATED_DATE} | |
| SYMBOL_COUNT=$(cypher_query 'MATCH (n) RETURN count(n) as c' | jq -r '.markdown' | tail -1 | tr -d '| ' || echo "0") | |
| EDGE_COUNT=$(cypher_query 'MATCH ()-[r]->() RETURN count(r) as c' | jq -r '.markdown' | tail -1 | tr -d '| ' || echo "0") | |
| PROCESS_COUNT=$(cypher_query 'MATCH (p:Process) RETURN count(p) as c' | jq -r '.markdown' | tail -1 | tr -d '| ' || echo "0") | |
| extract_count() { | |
| jq -er '.markdown' | tail -1 | tr -d '| ' | grep -E '^[0-9]+$' | |
| } | |
| cypher_query() { | |
| local query="$1" | |
| local result | |
| # shellcheck disable=SC2086 | |
| result=$(npx gitnexus cypher $REPO_FLAG "$query" 2>&1) || { | |
| echo "::error::Cypher query failed: $result" >&2 | |
| exit 1 | |
| } | |
| echo "$result" | |
| } | |
| echo "Querying GitNexus knowledge graph..." >&2 | |
| SYMBOL_COUNT=$(cypher_query 'MATCH (n) RETURN count(n) as c' | extract_count) | |
| EDGE_COUNT=$(cypher_query 'MATCH ()-[r]->() RETURN count(r) as c' | extract_count) | |
| PROCESS_COUNT=$(cypher_query 'MATCH (p:Process) RETURN count(p) as c' | extract_count) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.github/scripts/generate-architecture.sh around lines 71 - 87, The current
cypher_query() call can silently produce empty output and the subsequent
assignments to SYMBOL_COUNT, EDGE_COUNT, and PROCESS_COUNT coerce failures to
"0"; change this so failures are fatal: have cypher_query() return a non-zero
exit code on error (preserve and echo the error text), remove the "|| echo '0'"
fallback, and after each jq parse (for SYMBOL_COUNT, EDGE_COUNT, PROCESS_COUNT)
validate the result is non-empty and numeric (e.g., regex test or grep -E
'^[0-9]+$'); if any parse fails, write a clear error via >&2 with the
cypher_query output and exit 1 so the workflow fails rather than producing bogus
"0" metrics. Ensure you reference the cypher_query function and the
SYMBOL_COUNT/EDGE_COUNT/PROCESS_COUNT assignments when making the changes.
| <!-- auto:overview --> | ||
| | Metric | Count | | ||
| |--------|-------| | ||
| | Total symbols | 8,994 | | ||
| | Relationships | 21,155 | | ||
| | Execution flows | 300 | | ||
| | Python files | 217 | | ||
| | Rust files | 88 | | ||
| | GitHub workflows | 23 | | ||
| | GitHub workflows | 20 | | ||
| | Documentation files | 270 | | ||
| <!-- /auto:overview --> |
There was a problem hiding this comment.
Manually update the preserved CI/CD sections to match this new count.
The overview now says 20 workflows, but the hand-curated sections later in this file still say CI/CD (23 workflows) on Line 100 and 23 GitHub Actions workflows on Line 265. Line 101 still describes a 3-phase review, and Line 269 still lists the removed claude-review-phase1/2/3.yml files. As-is, the architecture map contradicts itself.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@ARCHITECTURE.md` around lines 8 - 18, Update the hand-curated CI/CD sections
in ARCHITECTURE.md to match the auto overview: change both occurrences of "23
workflows" (the "CI/CD (23 workflows)" heading and "23 GitHub Actions
workflows") to "20 workflows", remove or revise the obsolete "3-phase review"
wording and any references to the removed claude-review-phase1/2/3.yml files so
the document no longer lists those three workflows, and ensure the surrounding
descriptive text (the paragraph referencing the review phases) reflects the
current single/updated review process; search for the exact strings "CI/CD (23
workflows)", "23 GitHub Actions workflows", "3-phase", and
"claude-review-phase1/2/3.yml" to locate and edit the lines.
Tests now validate the new marker-based partial update behavior: - Mock npx handles gitnexus cypher/list commands (not query) - Seed ARCHITECTURE.md with markers provided to each test - Assertions check curated content preservation, not full generation - New tests for missing file and missing marker error paths Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
PAL MCP Consensus Not AvailableSecurity-sensitive files were detected, but PAL MCP multi-model consensus is not configured. To enable consensus: Configure Claude Code Review results are still available above. |
Claude Code Review (via AWS Bedrock)OverviewThis PR consolidates AI review workflows and rewrites the architecture generation script to use a marker-based partial update approach. The change shifts from full document regeneration to targeted updates of data-driven sections, preserving hand-curated content. Includes comprehensive test updates and ARCHITECTURE.md marker additions. Key Changes:
Critical IssuesNone found. The changes are functional and well-tested. High Priority1. File Corruption Risk - Missing Atomic Update (
|
| Category | Rating | Notes |
|---|---|---|
| Security | ⭐⭐⭐/5 | File corruption risks without atomic updates or rollback. No validation of marker pairing. |
| Code Quality | ⭐⭐⭐⭐/5 | Clean, well-organized code with minor issues (hardcoded values, embedded awk scripts). |
| Architecture | ⭐⭐⭐⭐⭐/5 | Excellent design. Marker-based partial updates are a major improvement. |
| Testing | ⭐⭐⭐⭐/5 | Strong coverage with comprehensive rewrite. Some edge cases missed (malformed markers, multi-repo). |
Overall Assessment: This is a high-quality PR that significantly improves the architecture generation system. The marker-based approach is architecturally sound and well-tested. The main concerns are around file corruption risks that should be addressed before merge. Recommend addressing the High Priority items, particularly atomic file updates and marker validation.
Generated by Claude Code Review (AWS Bedrock) - Sonnet 4.5
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
tests/github_scripts/test_generate_architecture_sh.py (2)
344-427: Mark subprocess execution suite as integration/slow.These tests run shell subprocesses and filesystem setup repeatedly; they should be tagged for selective CI execution.
+@pytest.mark.integration class TestExecution:As per coding guidelines: "Mark slower test journeys with
@pytest.mark.sloworintegrationperpyproject.tomlconfiguration".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/github_scripts/test_generate_architecture_sh.py` around lines 344 - 427, Add the integration/slow pytest marker to the subprocess-heavy tests by importing pytest and annotating the affected test functions (test_updates_architecture_md, test_updates_file_counts, test_preserves_curated_content, test_includes_footer, test_updates_timestamp, test_fails_without_architecture_md, test_fails_without_markers) with `@pytest.mark.slow` or `@pytest.mark.integration` as appropriate (or apply a module-level pytestmark = [pytest.mark.slow] to mark them all), so CI can selectively run these longer subprocess/filesystem tests.
355-363: Assert cypher-derived counts, not only table labels.Line 356 says this validates cypher-backed data, but the test only checks header strings. A parsing regression could still pass.
Proposed tightening
def test_updates_file_counts(self, run_generate): """Updated file includes file count table with data from cypher queries.""" result, content = run_generate() assert result.returncode == 0 assert "Python files" in content assert "Rust files" in content - assert "Total symbols" in content - assert "Relationships" in content + assert "| Total symbols | 42 |" in content + assert "| Relationships | 42 |" in content + assert "| Execution flows | 42 |" in content🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/github_scripts/test_generate_architecture_sh.py` around lines 355 - 363, The test_updates_file_counts test currently only checks for header labels but should assert the cypher-derived numeric values are present; update the test (in function test_updates_file_counts using the run_generate fixture) to parse the generated content and assert numeric counts for each header (e.g., match a digit or specific expected numbers for "Python files", "Rust files", "Total symbols", and "Relationships") rather than just the labels—use a regex or HTML/table cell lookup to locate the cell next to each header and assert it contains a non-empty numeric string (or the known expected integer) to prevent regressions that remove/alter the cypher values.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/github_scripts/test_generate_architecture_sh.py`:
- Around line 408-427: Add a new test in
tests/github_scripts/test_generate_architecture_sh.py (e.g.,
test_fails_with_missing_closing_marker) that mirrors test_fails_without_markers
but writes an ARCHITECTURE.md containing the opening marker "<!-- auto:overview
-->" without the closing "<!-- /auto:overview -->"; run the script via
subprocess with the same env/cwd setup, assert result.returncode != 0 and that
result.stderr contains an indication of the missing closing marker (e.g., the
string "/auto:overview" or "closing marker") to validate the guard for a missing
closing marker in the script.
---
Nitpick comments:
In `@tests/github_scripts/test_generate_architecture_sh.py`:
- Around line 344-427: Add the integration/slow pytest marker to the
subprocess-heavy tests by importing pytest and annotating the affected test
functions (test_updates_architecture_md, test_updates_file_counts,
test_preserves_curated_content, test_includes_footer, test_updates_timestamp,
test_fails_without_architecture_md, test_fails_without_markers) with
`@pytest.mark.slow` or `@pytest.mark.integration` as appropriate (or apply a
module-level pytestmark = [pytest.mark.slow] to mark them all), so CI can
selectively run these longer subprocess/filesystem tests.
- Around line 355-363: The test_updates_file_counts test currently only checks
for header labels but should assert the cypher-derived numeric values are
present; update the test (in function test_updates_file_counts using the
run_generate fixture) to parse the generated content and assert numeric counts
for each header (e.g., match a digit or specific expected numbers for "Python
files", "Rust files", "Total symbols", and "Relationships") rather than just the
labels—use a regex or HTML/table cell lookup to locate the cell next to each
header and assert it contains a non-empty numeric string (or the known expected
integer) to prevent regressions that remove/alter the cypher values.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 6fc51174-c029-4604-adcb-e659b43a6243
📒 Files selected for processing (1)
tests/github_scripts/test_generate_architecture_sh.py
| def test_fails_without_markers(self, mock_bin_dir, tmp_path): | ||
| """Script fails with error if ARCHITECTURE.md has no markers.""" | ||
| work_dir = tmp_path / "no_markers" | ||
| work_dir.mkdir() | ||
| (work_dir / "ARCHITECTURE.md").write_text("# Architecture\n\nNo markers here.\n") | ||
|
|
||
| env = os.environ.copy() | ||
| env["PATH"] = f"{mock_bin_dir}:{env['PATH']}" | ||
| env["HOME"] = str(tmp_path) | ||
|
|
||
| result = subprocess.run( | ||
| ["bash", str(SCRIPT_PATH)], | ||
| capture_output=True, | ||
| text=True, | ||
| env=env, | ||
| cwd=str(work_dir), | ||
| timeout=30, | ||
| ) | ||
| assert result.returncode != 0 | ||
| assert "marker" in result.stderr.lower() |
There was a problem hiding this comment.
Add a dedicated test for missing closing marker.
Current coverage checks “no markers,” but the script has a separate guard for a missing <!-- /auto:overview -->. That path should be validated explicitly.
Suggested additional test
+ def test_fails_without_closing_marker(self, mock_bin_dir, tmp_path):
+ """Script fails if ARCHITECTURE.md is missing the closing overview marker."""
+ work_dir = tmp_path / "missing_closer"
+ work_dir.mkdir()
+ (work_dir / "ARCHITECTURE.md").write_text(
+ "# Architecture\n\n<!-- auto:overview -->\n| Metric | Count |\n"
+ )
+
+ env = os.environ.copy()
+ env["PATH"] = f"{mock_bin_dir}:{env['PATH']}"
+ env["HOME"] = str(tmp_path)
+
+ result = subprocess.run(
+ ["bash", str(SCRIPT_PATH)],
+ capture_output=True,
+ text=True,
+ env=env,
+ cwd=str(work_dir),
+ timeout=30,
+ )
+ assert result.returncode != 0
+ assert "closing marker" in result.stderr.lower() or "/auto:overview" in result.stderr🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/github_scripts/test_generate_architecture_sh.py` around lines 408 -
427, Add a new test in tests/github_scripts/test_generate_architecture_sh.py
(e.g., test_fails_with_missing_closing_marker) that mirrors
test_fails_without_markers but writes an ARCHITECTURE.md containing the opening
marker "<!-- auto:overview -->" without the closing "<!-- /auto:overview -->";
run the script via subprocess with the same env/cwd setup, assert
result.returncode != 0 and that result.stderr contains an indication of the
missing closing marker (e.g., the string "/auto:overview" or "closing marker")
to validate the guard for a missing closing marker in the script.
1 participant
Summary
ai-review.yml,claude-review-phase1/2/3.ymlinto a singleai-code-review.ymlwith preflight, review, and autofix jobsgenerate-architecture.sh: Replaces the full-overwrite approach (which produced skeletal 46-line output, as seen in PR docs: update ARCHITECTURE.md #110) with marker-based partial updates that preserve hand-curated content (Mermaid diagrams, functional area descriptions, execution flow traces)target/, workflow count only matches.github/workflows/*.ymlTest plan
tests/integration/test_workflow_configs.py)generate-architecture.shruns cleanly, outputs 372 lines with all sections preservedai-code-review.ymlworkflow triggers correctly on PR eventsupdate-architectureworkflow produces correct output on next main push🤖 Generated with Claude Code
Summary by Sourcery
Update architecture documentation generation to use marker-based partial updates while tightening validation and file metrics.
Enhancements:
Documentation:
Summary by CodeRabbit