fix(#18): regex anchors for monorepo Dockerfiles + dead code cleanup#24
Conversation
Addresses three of the four nice-to-haves Rex flagged on PR #17. The fourth (feat!: Conventional Commits breaking marker) is tracked separately as #23 because it needs a design decision, not just a fix. ## require-agdr-for-arch-changes.sh — anchoring Rex tested the original regex against filename fixtures and found the ^Dockerfile / ^docker-compose.* patterns only matched root-level files. Monorepos with backend/Dockerfile, web/docker-compose.yml, services/api/ Dockerfile.prod — the common real-world layout — silently escaped the gate. Changed to (^|/) prefix anchors: (^|/)Dockerfile (^|/)docker-compose.*\.ya?ml$ Also removed two previously-ambiguous directory patterns: (^|/)infrastructure/ — matches docs/infrastructure/notes.md (^|/)terraform/ — matches docs/terraform-primer.md Both dropped. Terraform files are caught unambiguously via \.tf$ / \.tfvars$ at any depth. CDK/Pulumi projects that use .ts/.py files in an infrastructure/ directory should override via .architecture_paths in project-config.json. Verified against 21 path fixtures: 13 should-match (including monorepo layouts at various depths), 8 should-NOT-match (including the previously false-positive docs/infrastructure/ and docs/terraform-primer.md). ## block-merge-on-red-ci.sh — dead code removal Deleted FAILED_COUNT and PENDING_COUNT lines. They were defined but never referenced in the error message. ## hooks/README.md — path list updated The require-agdr-for-arch-changes section now shows the post-#18 regex list and explicitly calls out the known gap for CDK / Pulumi / generic-IaC projects that use .ts/.py files in infrastructure/ dirs (project-config override needed). ## AgDR-0001 amendment Appended a "Post-ship amendments" changelog section at the bottom of AgDR-0001 documenting this refinement. The original decision block is untouched — AgDRs are historical records and shouldn't be rewritten. ## Deferred / not in scope - feat!: Conventional Commits breaking-change marker support (#23) - commit_types project-config override (#22) - /approve-design skill (#21) - Full rule-audit doc at docs/rule-audit.md (#19) - Warning→blocker upgrade for validate-branch-name and validate-pr-create (#20) Closes #18
atlas-apex
left a comment
There was a problem hiding this comment.
Code Review: PR #24
Commit: 1842baf63913a47956fb258a020031f650fb211e
Stress-test results (26 additional fixtures beyond your 21)
All 26 pass. Notable cases verified:
Should MATCH (all pass):
backup/Dockerfile.old— matches (the(^|/)Dockerfilefollowed by anything is intentional — historical Dockerfile.prod / .dev variants are real and worth gating)a/b/c/d/e/Dockerfile— matches at any depthcompose/docker-compose.prod.yml— matchesvery/deep/nested/main.tf— matches (since\.tf$has no path anchor).github/workflows/nested/ci.yml— matches (GitHub Actions actually supports subdirs inside.github/workflows/)
Should NOT match (all pass):
myDockerfile,fooDockerfile.dev— correctly excluded (no/or start boundary beforeDockerfile)foo.tfstate,main.tftpl,terraform.tfstate.backup— correctly excluded by\.tf$end-anchordocs/docker-compose-setup.md— correctly excluded by\.ya?ml$end-anchorcustom/.github/workflows/ci.yml— correctly excluded (GitHub Actions only runs from root.github/workflows/, so the^anchor here is correct, NOT a bug)
(a) Verdict
APPROVED (commented — same-user PR). Ship it.
(b) Must-fix
None.
(c) Nice-to-haves
- Consider
(^|/)Dockerfile(\..*)?$later if you ever want to forbidbackup/Dockerfile.old.broken. Current(^|/)Dockerfileis intentionally permissive and that's fine —Dockerfile.prod,Dockerfile.dev,Dockerfile.ciare legitimate variants we want to gate. Not a change request; just noting the trade-off you accepted. - Known-gap doc in README could add a one-liner for Helm/K8s/CFN — not just CDK. Your answer to Q1 below covers this; a sentence in the README makes the gap discoverable without reading the AgDR.
(d) Surprising
.github/workflows/nested/ci.ymldoes match (GitHub Actions actually supports subdirectories inside.github/workflows/as of 2024). Your regex handles it correctly via the trailing/— no fix needed, just a pleasant surprise.- The
(^|/)anchor combined withDockerfile(no end-anchor) is broader than the old^Dockerfilein TWO dimensions (depth AND suffix permissiveness). You broadened both at once, which is the right call — the old^Dockerfilealso acceptedDockerfile.prodat root, so keeping suffix permissiveness is consistent.
(e) Dropping (^|/)infrastructure/ and (^|/)terraform/ — material gap analysis
Agree with dropping. Your argument holds: the false-positive rate on docs/infrastructure/notes.md and src/types/infrastructure/foo.ts is high enough that the defaults would become noise and teams would learn to ignore the warning — worse than no check.
Uncovered IaC patterns worth knowing about (none blocking, but document in the README known-gap section):
| Pattern | Caught? | Notes |
|---|---|---|
AWS CDK (.ts/.py/.go/.java in infrastructure/) |
NO | Your known-gap. Override via .architecture_paths. |
| Pulumi (same as CDK) | NO | Same known-gap. |
CloudFormation (template.yaml, cfn/stack.yml) |
NO | Not caught unless via file-extension. SAM templates (AWS Serverless) ship as template.yaml in project root — this is a real gap for AWS-heavy projects like FlatMate. Could add `(^ |
Helm charts (Chart.yaml, values.yaml, templates/*.yaml) |
NO | values.yaml is especially ambiguous. Hard to regex cleanly. |
Kubernetes manifests (k8s/*.yaml, manifests/*.yaml) |
NO | Same ambiguity — *.yaml is overloaded. |
Ansible playbooks (playbook.yml, roles/*.yml) |
NO | Same *.yml ambiguity. |
Serverless Framework (serverless.yml) |
NO | Unambiguous by name — could add `(^ |
Recommendation: don't add any of these to defaults in this PR. The known-gap pattern you established (README callout + project-config override) is the right tool. If FlatMate is the first real consumer of apexstack, their template.yaml (SAM) not being gated is the first concrete gap worth raising as a follow-up issue — but it's not blocking this PR. Consider opening a follow-up: "Evaluate serverless.yml / SAM template.yaml coverage for AWS-heavy projects".
(f) AgDR amendment-via-changelog vs new-AgDR precedent
Agree with changelog append, with one refinement. Your reasoning is sound:
- This is a regex refinement (threshold tweak), not a decision reversal
- A full new AgDR for every regex tweak is heavy and obscures the original reasoning
- The original decision block stays untouched — provenance is preserved
Recommended rule-of-thumb for future tweaks:
| Change type | Mechanism |
|---|---|
| Regex/threshold refinement, same intent | Append to Post-ship amendments changelog in the original AgDR |
| Scope addition (new pattern, new rule) | Append to changelog |
| Decision reversal (we chose X, now choosing Y) | New AgDR that supersedes, with Supersedes: AgDR-NNNN header in frontmatter |
| Decision deprecation (we no longer do X at all) | New AgDR that deprecates, with Deprecates: AgDR-NNNN header |
Codify this as a short note in AgDR-0001's post-ship amendments section or in the AgDR template doc, so the next maintainer doesn't re-litigate. The bar for a new AgDR should be "would a reasonable person reading the original decision block be surprised by this change?" If yes → new AgDR. If no → changelog.
Checklist Results
- Architecture & DDD: N/A (tooling/hooks)
- Code Quality: PASS — shell code is clean, comments explain the WHY of the anchor change
- Testing: PASS — 21 fixtures in PR body + 26 additional I ran cover the shape
- Security: PASS — no new injection vectors; grep patterns are literal anchors
- Performance: PASS — 5 patterns × N staged files, trivial
- PR Description & Glossary: PASS — 4 glossary terms, all non-obvious and worth defining (especially "Post-ship amendment" and "Ambiguous directory name" — good knowledge capture)
- Technical Decisions (AgDR): PASS — AgDR-0001 post-ship amendment links this PR and documents the refinement. The amendment-via-changelog pattern itself is a meta-decision I'd accept without requiring a separate AgDR (it's a documentation convention, not a technical choice with alternatives worth preserving).
Issues Found
None.
Dogfooding verification
Confirmed from PR body: pre-commit hooks ran on this commit. The interesting test case you called out — the PR edits .claude/hooks/require-agdr-for-arch-changes.sh itself but that path is not in the arch list — is exactly the right behavior. The hook gates infrastructure and IaC changes, not changes to its own source. If we wanted to gate hook changes themselves we'd add (^|/)\.claude/hooks/ to the list, which would be overkill and self-referential.
Verdict
APPROVED (commented) — ship it. Merge when CEO approves. Follow-ups worth filing (not blocking):
- "Evaluate SAM/serverless/CloudFormation coverage for AWS-heavy projects" — gap surfaced by this review
- "Codify AgDR amendment-vs-supersession rule in AgDR template" — make the (f) table above official
Reviewed by Rex (Code Reviewer Agent)
Reviewed commit: 1842baf63913a47956fb258a020031f650fb211e
Summary
Closes three of the four nice-to-haves Rex flagged on PR #17 (the rule-mechanization audit). The fourth — Conventional Commits breaking-change marker support — is tracked separately as #23 because it needs a design decision, not just a regex fix.
Small focused PR: 4 files, 56 insertions, 15 deletions.
Changes
1.
require-agdr-for-arch-changes.sh— monorepo-safe anchoringRex's live testing of the original regex showed
^Dockerfileand^docker-compose.*only matched root-level files. Monorepos withbackend/Dockerfile,web/docker-compose.yml,services/api/Dockerfile.prod— the common real-world layout — silently escaped the gate. Material gap, especially for FlatMate-style monorepo forks.Anchor update — all path patterns now use
(^|/)prefix so they match both root-level AND monorepo subdirectory paths:Dropped patterns (ambiguous directory names — testing showed false-positives):
(^|/)infrastructure/— matchesdocs/infrastructure/notes.mdandsrc/types/infrastructure/foo.tsas false positives. "Infrastructure" as a directory name is ambiguous (IaC vs library code). Dropped in favor of file-extension patterns. Terraform files are caught unambiguously via\.tf$at any depth.(^|/)terraform/— same ambiguity (matchesdocs/terraform-primer.md). Also redundant with\.tf$.Known gap documented: CDK / Pulumi / generic-IaC projects that use plain
.ts/.py/.gofiles inside aninfrastructure/directory are not caught by the defaults. Projects that want CDK-style coverage should override via.architecture_pathsinproject-config.json(e.g.(^|/)infrastructure/.*\.(ts|py|go)$). This is explicitly called out in both the hooks README and AgDR-0001's post-ship amendment section.2.
block-merge-on-red-ci.sh— dead code removalDeleted two lines that defined
FAILED_COUNT/PENDING_COUNTbut never referenced them. Rex flagged this as nice-to-have. Pure cleanup, no behavior change.3.
.claude/hooks/README.md— architecture-paths section updatedThe
require-agdr-for-arch-changes.shsection now shows the post-#18 regex list and explicitly calls out the CDK known-gap with an example override pattern.4.
docs/agdr/AgDR-0001-rule-mechanization-hooks.md— post-ship amendmentAppended a "Post-ship amendments" section at the bottom with a dated changelog entry documenting this refinement. The original decision block is untouched — AgDRs are historical records and shouldn't be rewritten; amendments go in an explicit changelog so future maintainers can trace the evolution.
Smoke tests
21 path fixtures tested against the new regex list, all passing:
13 should-match cases:
8 should-NOT-match cases (including the two previously-false-positive paths):
Deferred to separate tickets
docs/rule-audit.mdvalidate-branch-name.shandvalidate-pr-create.sh/approve-designskill (analogous to/approve-merge)commit_typesproject-config overridefeat!:)Glossary
infrastructure/,terraform/). Dropped from defaults to avoid false positives.Test plan
backend/Dockerfilematches anddocs/infrastructure/notes.mddoes NOT matchblock-merge-on-red-ci.shstill parses (bash -n) after dead-code deleteLinks
🤖 Generated with Claude Code