Skip to content
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
84 changes: 84 additions & 0 deletions .claude/scripts/check-file-moves.sh
Original file line number Diff line number Diff line change
@@ -0,0 +1,84 @@
#!/usr/bin/env bash
# PostToolUse hook on Bash. When the command is `git mv` or `git rm`, finds the source
# path(s) and greps the repo for refs to them. Prints findings as additionalContext so
# the model sees the worklist in the same session the move happened.
#
# Purpose: catch doc/comment/Dockerfile drift in the same session a file gets moved or
# deleted — same compounding loop as check-claude-md-refs.sh but for renames/deletions
# instead of CLAUDE.md edits.
#
# Why git mv / git rm specifically (not plain mv / rm): plain mv/rm is used constantly
# for temp files, build artifacts, and ephemeral work. Restricting to git-tracked
# operations dramatically cuts false-positive noise. The cost is missing the case where
# someone uses plain `mv` on a tracked file — that's the gap CodeRabbit's
# path_instruction for file rename/delete picks up at review time.
#
# See CLAUDE.md "File-move discipline" for the canonical rule.

set -uo pipefail

REPO_ROOT="/Users/joshuadell/NovaCraft"

command=$(jq -r '.tool_input.command // ""' 2>/dev/null)

# Only fire on git mv / git rm. Plain mv/rm is too noisy.
case "$command" in
*"git mv "*) ;;
*"git rm "*) ;;
*) exit 0 ;;
esac

old_paths=()

# git mv <src> <dst> — capture the FIRST positional arg (the source).
if [[ "$command" =~ git[[:space:]]+mv[[:space:]]+([^[:space:]]+) ]]; then
src="${BASH_REMATCH[1]}"
case "$src" in
-*) ;; # skip if it looked like a flag
*) old_paths+=("$src") ;;
esac
fi

# git rm [flags] <path>... — capture all non-flag tokens after `git rm`.
if [[ "$command" =~ git[[:space:]]+rm[[:space:]] ]]; then
args=$(echo "$command" | sed -E 's/^.*git[[:space:]]+rm[[:space:]]+//' | tr -s ' ')
for token in $args; do
case "$token" in
-*) continue ;;
"") continue ;;
*) old_paths+=("$token") ;;
esac
done
fi

# Nothing to do.
if [ ${#old_paths[@]} -eq 0 ]; then
exit 0
fi

# For each old path, grep for refs and collect findings. --fixed-strings so paths
# containing dots / slashes match literally instead of as regex.
findings=""
for old_path in "${old_paths[@]}"; do
case "$old_path" in
""|"*"|"."|"./"|".."|"./*") continue ;;
esac

matches=$(grep -rln --fixed-strings "$old_path" \
--include='*.md' --include='*.cs' --include='*.props' --include='*.csproj' \
--include='*.yml' --include='*.yaml' --include='*.sh' \
--include='Dockerfile*' \
"$REPO_ROOT" 2>/dev/null \
| head -30 \
|| true)
Comment on lines +67 to +73

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Exclude the documented allowlist file from drift matches.

Line 67 currently scans all markdown files, which will also flag .claude/audits/INDEX.md; that file is explicitly allowlisted and should be skipped to avoid false positives.

Suggested patch
-    matches=$(grep -rln --fixed-strings "$old_path" \
+    matches=$(grep -rln --fixed-strings "$old_path" \
         --include='*.md' --include='*.cs' --include='*.props' --include='*.csproj' \
         --include='*.yml' --include='*.yaml' --include='*.sh' \
         --include='Dockerfile*' \
         "$REPO_ROOT" 2>/dev/null \
+        | grep -v "^${REPO_ROOT}/.claude/audits/INDEX.md$" \
         | head -30 \
         || true)

As per coding guidelines, “Allowlist: .claude/audits/INDEX.md … do NOT flag links from INDEX.md as drift.”

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In @.claude/scripts/check-file-moves.sh around lines 67 - 73, The grep that
builds the matches variable is currently scanning all markdown files and will
pick up the allowlisted `.claude/audits/INDEX.md`; update the matches assignment
in check-file-moves.sh to exclude that specific file when searching for
"$old_path" under "$REPO_ROOT" (e.g., add an --exclude or filter out
`.claude/audits/INDEX.md` from the grep results) so links from INDEX.md are not
flagged as drift.

if [ -n "$matches" ]; then
findings+=$(printf "Refs to '%s' still present in:\n%s\n\n" "$old_path" "$matches")
fi
done

if [ -z "$findings" ]; then
exit 0
fi

msg=$(printf 'File-move/delete detected. Refs to the OLD path may need updating before commit:\n\n%s\nUpdate the paraphrases in the same PR. See CLAUDE.md "File-move discipline".' "$findings")
jq -n --arg m "$msg" '{hookSpecificOutput: {hookEventName: "PostToolUse", additionalContext: $m}}'
9 changes: 9 additions & 0 deletions .claude/settings.json
Original file line number Diff line number Diff line change
Expand Up @@ -61,6 +61,15 @@
"command": "/Users/joshuadell/NovaCraft/.claude/scripts/check-claude-md-refs.sh"
}
]
},
{
"matcher": "Bash",
"hooks": [
{
"type": "command",
"command": "/Users/joshuadell/NovaCraft/.claude/scripts/check-file-moves.sh"
}
Comment on lines +65 to +71

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

git mv path is configured in hook logic but not allowed in permissions.

The new hook handles both git mv and git rm, but only Bash(git rm *) is permitted. Add Bash(git mv *) so the rename branch can actually execute in tool-driven sessions.

Suggested patch
       "Bash(git checkout *)",
       "Bash(git pull *)",
+      "Bash(git mv *)",
       "Bash(git rm *)",
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In @.claude/settings.json around lines 65 - 71, The hook configuration adds a
Bash hook script (matcher "Bash" with command
"/Users/joshuadell/NovaCraft/.claude/scripts/check-file-moves.sh") that handles
both git mv and git rm, but the permissions list only allows Bash(git rm *);
update the permissions to also include Bash(git mv *) so tool-driven sessions
can perform renames—locate the permissions array in the same
.claude/settings.json where Bash(git rm *) is declared and add an entry for
Bash(git mv *).

]
}
]
}
Expand Down
75 changes: 75 additions & 0 deletions .coderabbit.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -447,6 +447,81 @@ reviews:
in bash run blocks, missing `permissions:` blocks, missing
`concurrency:` groups, secrets in plaintext, hardcoded tokens.

- path: "NextAurora.AppHost/AppHost.cs"
instructions: |
TOPOLOGY DOC PAIRING (CLAUDE.md "Doc-and-diagram discipline"). AppHost.cs
is the canonical source of the system topology — service count, transport
wiring, DB choices, external deps. When this file changes in a PR (new
service registration, new `WithReference`, new `AddAzure*`, new
`WaitFor`, removed resource), the topology depiction artifacts MUST be
updated in the same PR, OR the PR description names a tracking issue
for the deferred update.

Required pairing:
- `docs/architecture.md` (the prose system map + communication patterns)
- `docs/nextaurora-architecture.svg` (rendered diagram github.com inlines)
- `docs/nextaurora-architecture.excalidraw` (editable source)

Reviewers look at the .svg on the PR page to understand "what does this
system look like now?" — if the .svg doesn't show the new service or
transport, every subsequent PR review reasons against a stale picture.
Same for architecture.md: it's the doc people grep when onboarding or
debugging a comms issue. Flag this as a finding when AppHost.cs changes
without paired updates. Acceptable PR-description waivers: "topology
change is internal/transient (smoke-test scaffold) and will be reverted
in PR #N" or "docs/diagram updated in tracking issue #N, this PR ships
the code only because [reason]."

- path: "NextAurora.ServiceDefaults/Extensions.cs"
instructions: |
SERVICE-REQUEST-FLOW DOC PAIRING (CLAUDE.md "Doc-and-diagram discipline").
Extensions.cs configures the middleware pipeline ordering (auth →
correlation-id → authorization), JWT validation, and the
AddDefaultAuthentication / MapDefaultEndpoints contracts every service
composes with. When this changes (middleware order, JWT params, default
endpoint behavior), the service-request-flow depiction artifacts MUST
be updated in the same PR or named in a deferred issue:

Required pairing:
- `docs/architecture.md` Observability section (middleware-order rules)
- `docs/service-request-flow.svg` + `docs/service-request-flow.excalidraw`

Same reasoning as the AppHost.cs rule above — reviewers reason against
the .svg picture of the request lifecycle. Flag missing pairs.

- path: "**"
instructions: |
FILE-MOVE DISCIPLINE (CLAUDE.md "File-move discipline"). When this PR's
diff includes a file deletion or rename, grep the repo for refs to the
OLD path and flag any matches as findings. Look in: every `*.md`
(README, docs/, .claude/, .github/), inline comments in `*.cs`
(especially comments ending with literal `See CLAUDE.md.`), `Dockerfile*`
COPY/ADD instructions, `*.csproj` ProjectReference Include="...",
`.github/workflows/*.yml` `run:` blocks, `.coderabbit.yaml`
path_instructions, `.claude/scripts/*.sh` literal-path strings,
and `.claude/commands/*.md` slash-command bodies.

Failing to update these is exactly the drift class that produced the
broken-Dockerfile + broken-CLAUDE.md-link + stale-demo-doc fallout from
the simplicity refactor (PR #31 collapsed CatalogService to a single
project; refs to the pre-collapse 4-project layout sat silent in
`Dockerfile.catalog`, `docs/demo-deployment*.md`, and
`docs/performance-and-data-correctness.md` for months — only caught
when the doc-currency sweep happened in PR #112). Even a single missed
ref is a finding.

Mechanical enforcement already covers part of this: the
`.github/workflows/ci.yml` "Broken-link audit" step blocks the merge
when relative markdown links to local source files don't resolve.
That catches the broken-link case but not the "ref still points at a
file that happens to exist for a different reason" case — which is
where this CodeRabbit pass earns its keep.

Allowlist: `.claude/audits/INDEX.md` legitimately links to gitignored
per-article files (copyright reason — see
`.claude/commands/article-audit.md` step 5 "Copyright note"). The CI
guard excludes it; do NOT flag links from INDEX.md as drift.

# Chat: allow @coderabbitai mentions to ask follow-up questions on a PR.
chat:
auto_reply: true
Expand Down
93 changes: 93 additions & 0 deletions .github/workflows/ci.yml
Original file line number Diff line number Diff line change
Expand Up @@ -109,6 +109,24 @@ jobs:
exit 1
fi

# CLAUDE.md size budget — enforces the "Keep this file lean" discipline from
# CLAUDE.md "Continuous Rule Encoding" surface 1. Every byte of CLAUDE.md is loaded
# into every Claude Code session AND is cognitive overhead for human readers. Soft
# warning at 400 lines, hard fail at 500. When approaching either, audit each rule
# and move detail to the relevant paired doc in docs/ or skill in .claude/skills/;
# CLAUDE.md keeps headline + one-paragraph summary + link.
- name: CLAUDE.md size budget
run: |
lines=$(wc -l < CLAUDE.md)
echo "CLAUDE.md is $lines lines."
if [ "$lines" -gt 500 ]; then
echo "::error file=CLAUDE.md::CLAUDE.md exceeds 500-line hard limit (current: $lines). Move detail to docs/ or .claude/skills/ and leave a headline + link. See CLAUDE.md 'Continuous Rule Encoding' surface 1."
exit 1
fi
if [ "$lines" -gt 400 ]; then
echo "::warning file=CLAUDE.md::CLAUDE.md is approaching the size budget ($lines lines; soft limit 400, hard limit 500). Consider moving detail out."
fi

Comment on lines +119 to +129

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Add strict shell mode to the new CI audit run blocks.

The newly added multiline bash steps do not set set -euo pipefail. Please add it at the top of each run: | block so audit failures are surfaced reliably.

Suggested patch pattern
       - name: CLAUDE.md size budget
         run: |
+          set -euo pipefail
           lines=$(wc -l < CLAUDE.md)
           echo "CLAUDE.md is $lines lines."
           ...

       - name: Broken-COPY audit — Dockerfile source paths
         run: |
+          set -euo pipefail
           fail=0
           while IFS= read -r dockerfile; do
           ...

       - name: Diagram-pair audit — every .excalidraw needs a sibling .svg
         run: |
+          set -euo pipefail
           fail=0
           while IFS= read -r excalidraw; do
           ...

As per coding guidelines for .github/workflows/*.yml, “DO flag … missing set -euo pipefail in bash run blocks.”

Also applies to: 178-206, 222-239

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In @.github/workflows/ci.yml around lines 119 - 129, Add strict shell mode to
the Bash run blocks that perform the CLAUDE.md line-count audits by inserting
set -euo pipefail at the very top of each run: | block (specifically the block
that computes lines=$(wc -l < CLAUDE.md) and the other audit blocks referenced
around 178-206 and 222-239); this ensures the line-count checks and early exits
fail loudly and the workflow job fails on errors, undefined variables, or
pipeline errors.

# Broken-link audit: relative markdown links to local source files (.cs/.csproj/.props/
# .yml/.svg/.excalidraw/.md) that don't resolve. Catches drift from file renames and
# the kind of fallout the simplicity refactor produced — CLAUDE.md citations pointing at
Expand Down Expand Up @@ -144,6 +162,81 @@ jobs:
# step 5 "Copyright note" — the contract is "INDEX ships, per-article files don't."
# On a contributor's machine the links resolve; on the CI runner they don't, by design.

# Broken-COPY audit: Dockerfile* COPY/ADD lines that reference source paths which
# don't exist in the build context (the repo root). This is the gap the markdown
# audit above can't close — `Dockerfile.catalog` rotted silently for months after
# PR #31 (VSA collapse) because the broken paths only failed at deploy time, not
# at build/test time. The Fly demo at https://catalog-api-demo.fly.dev kept running
# pre-collapse code; any redeploy would have failed on the first COPY of a
# nonexistent csproj. Same shape as the markdown audit: parse, check existence, fail.
# Handles the common COPY forms: `COPY src dst`, `COPY src1 src2 dst` (multi-src),
# and `COPY --from=stage src dst` (cross-stage; skips the src check since src is in
# another build stage, not the repo). Skips wildcards (`COPY src/*.json .`) since
# those resolve at build time, not against the repo tree. See CLAUDE.md "File-move
# discipline" for the canonical rule.
- name: Broken-COPY audit — Dockerfile source paths
run: |
fail=0
while IFS= read -r dockerfile; do
# Read each COPY/ADD line, strip the destination (last token), check each source.
while IFS= read -r line; do
# Skip `COPY --from=...` cross-stage copies (src is from a build stage, not the repo).
case "$line" in *"--from="*) continue ;; esac
# Strip the instruction keyword and any flags.
args=$(echo "$line" | sed -E 's/^[[:space:]]*(COPY|ADD)[[:space:]]+//; s/--[a-zA-Z0-9=._/-]+[[:space:]]+//g')
# Last token is the destination; everything before is source(s).
srcs=$(echo "$args" | awk 'NF>1 {for(i=1;i<NF;i++) print $i}')
for src in $srcs; do
# Skip wildcards (resolved at build time, not against the repo tree).
case "$src" in *"*"*|*"?"*|*"["*) continue ;; esac
# Skip absolute paths (rare; not a build-context drift case).
case "$src" in /*) continue ;; esac
# Trim any trailing slash for the existence check.
target="${src%/}"
if [ ! -e "$target" ]; then
echo "::error file=$dockerfile::COPY/ADD source does not exist → $src"
fail=1
fi
done
done < <(grep -iE '^[[:space:]]*(COPY|ADD)[[:space:]]+' "$dockerfile" || true)
done < <(find . -type f \( -name 'Dockerfile' -o -name 'Dockerfile.*' \) \
-not -path './bin/*' -not -path './obj/*' \
-not -path '*/node_modules/*' -not -path '*/.git/*')
exit "$fail"

# Diagram-pair audit: every diagram in docs/ MUST exist as both an .excalidraw
# source and a .svg export. Reviewers (humans + CodeRabbit) look at the .svg to
# understand the system; the .excalidraw is the authoritative source that
# contributors edit. If one exists without the other, the diagram is broken — a
# .svg with no .excalidraw can't be edited at the source level, and a .excalidraw
# with no .svg means reviewers see the stale state on github.com (rendered SVGs
# are inline-viewable; .excalidraw files aren't). Catches the case "I added a
# new diagram source but forgot to render the SVG" and the inverse.
#
# This DOES NOT verify the .svg matches what would be regenerated from the
# .excalidraw source — that needs the Playwright/excalidraw rendering pipeline
# (see .claude/scripts/rebuild-diagrams.sh). That heavier check belongs in its
# own workflow gated on .excalidraw changes; the pair-existence check is the
# cheap mechanical floor. See CLAUDE.md "Doc-and-diagram discipline".
- name: Diagram-pair audit — every .excalidraw needs a sibling .svg
run: |
fail=0
while IFS= read -r excalidraw; do
svg="${excalidraw%.excalidraw}.svg"
if [ ! -f "$svg" ]; then
echo "::error file=$excalidraw::diagram source without rendered .svg → expected $svg"
fail=1
fi
done < <(find docs -maxdepth 2 -type f -name '*.excalidraw' 2>/dev/null || true)
while IFS= read -r svg; do
excalidraw="${svg%.svg}.excalidraw"
if [ ! -f "$excalidraw" ]; then
echo "::error file=$svg::rendered .svg without .excalidraw source → expected $excalidraw"
fail=1
fi
done < <(find docs -maxdepth 2 -type f -name '*.svg' 2>/dev/null || true)
exit "$fail"

# Testcontainers-based integration tests, in their own job: they need Docker (the
# ubuntu-latest runner ships it at the standard /var/run/docker.sock, so Testcontainers
# auto-detects — no DOCKER_HOST override, unlike macOS Docker Desktop locally). Kept
Expand Down
Loading
Loading