ci: add ADR governance compliance - #19
Conversation
Code Review: ci/adr-governance-complianceOverall: A well-motivated PR that adds real governance infrastructure. The template and TOOLING docs are clear and the CI gate is a solid idea. There are a few correctness and robustness issues in OverviewThis PR adds:
The design intent is sound. ADR immutability enforcement via CI is exactly the right place for it. Bugs / Correctness1. return run(["git", "rev-parse", "HEAD^@"])On a merge commit, Fix: use try:
sha = run(["git", "rev-parse", "HEAD^"])
return sha
except Exception:
return None2. STATUS_RE = re.compile(r"(?im)^\s*(?:[-*]\s*)?.*?Status.*?:\s*(.+?)\s*$")The Tighten to match the expected metadata line format specifically: STATUS_RE = re.compile(r"(?m)^\s*[-*]?\s*\*{0,2}[Ss]tatus\*{0,2}:\s*\*{0,2}(.+?)\*{0,2}\s*$")3. Silent failure when git diff fails on push events In print("WARNING: could not determine changed files; skipping per-file validation.", file=sys.stderr)Missing from PR Description4. ADR-0001 is not in the diff The PR body says:
No such file appears in the diff. Either the description is stale or the file was dropped. If bootstrapping ADR-0001 is still the intent, it should be included and must itself pass the governance script. If not, the PR description should be corrected. Minor Issues5. PEP 8: missing blank lines before The 6.
7. CI workflow pins no Python version, but that's fine The script uses only stdlib + What's Good
Summary of Requested Changes
🤖 Generated with Claude Code |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| return f"origin/{ref}" | ||
| # On push, compare against first parent where available. | ||
| try: | ||
| return run(["git", "rev-parse", "HEAD^@"]) |
There was a problem hiding this comment.
HEAD^@ expands to all parents of HEAD. On a merge commit pushed directly to main this yields two or more SHAs separated by newlines, making git diff --name-only "sha1 sha2...HEAD" an invalid command. Both fallback branches in changed_files_against_base will raise and return [], so changed = [] and the immutability loop never runs — any edit to an Accepted ADR in a merge commit silently passes. Use HEAD^ (first-parent only) to keep the intent in the comment and handle merge commits correctly.
| return run(["git", "rev-parse", "HEAD^@"]) | |
| return run(["git", "rev-parse", "HEAD^"]) |
Prompt To Fix With AI
This is a comment left during a code review.
Path: scripts/adr-governance.py
Line: 41
Comment:
`HEAD^@` expands to *all* parents of HEAD. On a merge commit pushed directly to `main` this yields two or more SHAs separated by newlines, making `git diff --name-only "sha1
sha2...HEAD"` an invalid command. Both fallback branches in `changed_files_against_base` will raise and return `[]`, so `changed = []` and the immutability loop never runs — any edit to an Accepted ADR in a merge commit silently passes. Use `HEAD^` (first-parent only) to keep the intent in the comment and handle merge commits correctly.
```suggestion
return run(["git", "rev-parse", "HEAD^"])
```
How can I resolve this? If you propose a fix, please make it concise.| ALLOWED_STATUSES = {"Proposed", "Accepted", "Superseded", "Deprecated", "Rejected"} | ||
| REQUIRED_SECTIONS = ["Context", "Decision", "Consequences", "Validation"] | ||
| FILENAME_RE = re.compile(r"^ADR-\d{4}-[a-z0-9][a-z0-9-]*\.md$") | ||
| STATUS_RE = re.compile(r"(?im)^\s*(?:[-*]\s*)?.*?Status.*?:\s*(.+?)\s*$") |
There was a problem hiding this comment.
STATUS_RE pattern uses a loose .*?Status.*?: match, so it will trigger on any line in the ADR body that happens to contain "Status" — e.g., a Consequences bullet like - Deployment Status: unchanged or - System status: degraded. Since re.search() returns the first match, the correct frontmatter line wins when the header is near the top, but any ADR that discusses status in its body before the frontmatter would resolve to the wrong value. Anchoring the pattern more tightly to the frontmatter convention removes the ambiguity.
| STATUS_RE = re.compile(r"(?im)^\s*(?:[-*]\s*)?.*?Status.*?:\s*(.+?)\s*$") | |
| STATUS_RE = re.compile(r"(?im)^\s*[-*]\s*\*{0,2}Status\*{0,2}\s*:\s*(.+?)\s*$") |
Prompt To Fix With AI
This is a comment left during a code review.
Path: scripts/adr-governance.py
Line: 23
Comment:
The `STATUS_RE` pattern uses a loose `.*?Status.*?:` match, so it will trigger on any line in the ADR body that happens to contain "Status" — e.g., a Consequences bullet like `- Deployment Status: unchanged` or `- System status: degraded`. Since `re.search()` returns the first match, the correct frontmatter line wins when the header is near the top, but any ADR that discusses status in its body before the frontmatter would resolve to the wrong value. Anchoring the pattern more tightly to the frontmatter convention removes the ambiguity.
```suggestion
STATUS_RE = re.compile(r"(?im)^\s*[-*]\s*\*{0,2}Status\*{0,2}\s*:\s*(.+?)\s*$")
```
How can I resolve this? If you propose a fix, please make it concise.| if base: | ||
| for name in changed: | ||
| if not (name.startswith("docs/adr/ADR-") and name.endswith(".md")): | ||
| continue | ||
| old = file_at(base, name) | ||
| if old is None: | ||
| continue | ||
| old_status = status_of(old) | ||
| if old_status == "Accepted": | ||
| errors.append( | ||
| f"{name}: Accepted ADRs are immutable. Create a new superseding ADR instead of editing this file." | ||
| ) |
There was a problem hiding this comment.
ai_may_accept: false not enforced by CI
.adr-kit.yaml declares human_acceptance_required: true / ai_may_accept: false, but adr-governance.py only blocks edits to already-Accepted ADRs — it has no check that prevents a PR from changing an ADR's status from Proposed directly to Accepted. A contributor (or AI agent) can mark a brand-new ADR Accepted in the same PR that creates it, and this gate will pass.
Prompt To Fix With AI
This is a comment left during a code review.
Path: scripts/adr-governance.py
Line: 99-110
Comment:
**`ai_may_accept: false` not enforced by CI**
`.adr-kit.yaml` declares `human_acceptance_required: true` / `ai_may_accept: false`, but `adr-governance.py` only blocks *edits* to already-Accepted ADRs — it has no check that prevents a PR from changing an ADR's status from `Proposed` directly to `Accepted`. A contributor (or AI agent) can mark a brand-new ADR `Accepted` in the same PR that creates it, and this gate will pass.
How can I resolve this? If you propose a fix, please make it concise.| # ADR-NNNN: <Decision Title> | ||
|
|
||
| - **Status:** Proposed | ||
| - **Date:** YYYY-MM-DD | ||
| - **Decision owners:** <names/handles> | ||
| - **Reviewers:** <names/handles> | ||
| - **Supersedes:** <ADR-NNNN or none> | ||
| - **Superseded by:** <ADR-NNNN or none> | ||
| - **Related:** <issues/PRs/specs> |
There was a problem hiding this comment.
The PR description states "For repos without existing ADRs, this also bootstraps ADR-0001: Adopt Architecture Decision Records", but no docs/adr/ADR-0001-*.md file is included in this PR. The governance script will run and report ADR governance passed (0 ADR file(s) checked) — a green CI gate with zero ADRs in the repository.
Prompt To Fix With AI
This is a comment left during a code review.
Path: docs/adr/TEMPLATE.md
Line: 1-9
Comment:
**ADR-0001 mentioned in PR description but not present**
The PR description states "For repos without existing ADRs, this also bootstraps `ADR-0001: Adopt Architecture Decision Records`", but no `docs/adr/ADR-0001-*.md` file is included in this PR. The governance script will run and report `ADR governance passed (0 ADR file(s) checked)` — a green CI gate with zero ADRs in the repository.
How can I resolve this? If you propose a fix, please make it concise.
Code Review — ci: add ADR governance complianceGood initiative — preserving architectural reasoning as an auditable, immutable record is especially important in an AI-assisted codebase. The overall structure is clean and the intent is solid. A few issues need attention before this is trustworthy as a CI gate. Critical Issues1.
|
| Severity | Count |
|---|---|
| Critical | 3 (STATUS_RE false positives, ai_may_accept unenforced, missing ADR-0001) |
| Moderate | 3 (Python version, diff semantics comment, adrs version pin) |
| Minor/Nit | 2 |
The structural intent of this PR is solid. Fix the STATUS_RE pattern and add enforcement for the ai_may_accept: false policy before merge — those two gaps undermine the guarantees the system is supposed to provide.
1 participant
Summary
Adds repository-local ADR governance and team-standard ADR tooling:
docs/adr/TEMPLATE.mdwith a structured ADR formatdocs/adr/TOOLING.mdwithadrs,adr-kit, Codex/Claude Code/OpenCode/pi harness guidance.adr-kit.yamlpolicy configscripts/adr-governance.pyvalidation for ADR format, required sections, status values, duplicate numbers, and immutableAcceptedADRs.github/workflows/adr-governance.ymlCI gateFor repos without existing ADRs, this also bootstraps
ADR-0001: Adopt Architecture Decision Records.Why
ADRs are part of our engineering safety system, especially now we use AI coding assistance. They preserve the reasoning, rejected alternatives, consequences, and validation behind architectural decisions.
AcceptedADRs must not be rewritten; if a decision changes, we create a superseding ADR so the audit trail remains intact.Review focus
Acceptedenforcement is strict enough.Test plan
python3 scripts/adr-governance.pylocally on the branch.Greptile Summary
This PR introduces an ADR governance system: a Python validation script, a GitHub Actions CI gate, a policy config, an ADR template, and AI harness guidance. The central goal is to enforce that Accepted ADRs are immutable, required sections are present, and only humans can accept an ADR.
scripts/adr-governance.pyvalidates filenames, required sections, status values, and duplicate numbers, and blocks edits to previously-Accepted ADRs on both PR and push events.git rev-parse HEAD^@, which returns multiple lines for merge commits and silently empties the changed-file list, bypassing the immutability check on that path.ai_may_accept: falsepolicy in.adr-kit.yamlis not enforced by the CI script — a new ADR can be created already markedAcceptedwithout being blocked.Confidence Score: 3/5
The governance intent is solid but the push-event immutability check has a real bypass for merge commits, and the ai_may_accept policy is declared but not enforced at the CI level.
The HEAD^@ bug means that anyone who lands a merge commit on main would have the immutability enforcement silently skipped. This is the most critical path the script is supposed to protect, and it fails on that path.
scripts/adr-governance.py — the base_ref() function and the immutability enforcement loop need attention before this gate can be trusted for push events.
Important Files Changed
Flowchart
%%{init: {'theme': 'neutral'}}%% flowchart TD A[CI Trigger] --> B{GITHUB_BASE_REF set?} B -- Yes PR --> C[base = origin/BASE_REF] B -- No push --> D[git rev-parse HEAD^@] D -- single parent --> E[base = parent SHA] D -- merge commit --> F[multi-line SHA - git diff fails] C --> G[git diff name-only] E --> G F -.->|immutability check skipped| Z([CI passes incorrectly]) G --> H[changed ADR files] H --> L[Immutability check] L -- old status Accepted --> M([CI fails]) L -- not Accepted --> N([CI passes])Prompt To Fix All With AI
Reviews (1): Last reviewed commit: "ci: add ADR governance compliance" | Re-trigger Greptile