feat(architecture): continuous architecture learning ledger and quality grade (Phase 5) - #479
Conversation
…ty grade (Phase 5) Closes the openspec change's final phase: recurring improve-codebase-architecture findings are classified into six named erosion patterns in architecture/learning-ledger.json (C1 refactored, nine open backlog items honestly recorded), promotions are machine-verified against real invariant gates (the three ratchets are the historical promotions of structural-erosion-classes), and scripts/dev/report_architecture_quality.py publishes a deterministic graded snapshot (canonical: grade B - all gates green, 16 attributed debt entries) that never repairs anything. Task ledger reaches 26/26. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015QTVFY89rS2xRwRB2TpFP6
…mmit Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015QTVFY89rS2xRwRB2TpFP6
|
Warning Review limit reached
Next review available in: 38 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughAdds a machine-readable architecture learning ledger, JSON Schema validation, deterministic quality aggregation, a reporting CLI, verification-manifest wiring, tests, and Phase 5 completion records. ChangesArchitecture learning and quality reporting
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant CLI
participant QualityReport
participant LearningLedger
participant ArchitectureBaselines
CLI->>QualityReport: request architecture quality report
QualityReport->>LearningLedger: validate ledger and findings
QualityReport->>ArchitectureBaselines: count baseline debt
QualityReport-->>CLI: return grade, gates, debt, and issues
CLI-->>CLI: render output and set exit status
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 2📝 Generate docstrings 💡
🛠️ Fix failing CI checks 💡
🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (3)
scripts/lib/architecture_learning.py (1)
325-331: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueTwo small cleanups.
Move
import jsonat line 328 to the module imports. The deferred import gives no benefit here, and every other import is at module scope.At line 382, Ruff reports RUF005. Use unpacking instead of list concatenation.
♻️ Proposed cleanup
- debt = tuple(debt_counts + [("learning_open_findings", ledger.open_count)]) + debt = (*debt_counts, ("learning_open_findings", ledger.open_count))Also applies to: 382-382
🤖 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 `@scripts/lib/architecture_learning.py` around lines 325 - 331, Move the json import from _count_baseline_debt to the module-level imports, then update the list construction at the referenced RUF005 site to use iterable unpacking instead of list concatenation. Preserve the existing ordering and resulting values.Source: Linters/SAST tools
tests/test_architecture_learning.py (2)
224-245: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider covering the
--outputpath.The CLI test exercises stdout only.
_emitalso creates parent directories and writes withnewline=""to keep byte-identical output across platforms. That determinism claim is stated inarchitecture/README.mdbut is not tested. Add one case that passes--outputintotmp_pathand asserts the file content equals the stdout rendering.🤖 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 `@tests/test_architecture_learning.py` around lines 224 - 245, Extend test_report_script_passes_on_canonical_repository to invoke the report CLI with --output targeting a file under tmp_path, then assert the generated file content exactly matches completed.stdout. Preserve the existing JSON assertions and ensure the test covers parent-directory creation and byte-identical output.
159-164: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winAdd a regression test for the empty-invariants path.
test_promotion_must_point_at_a_declared_invariantcovers a wrongpromoted_tovalue while the contract still declares invariants. It does not cover a contract whoseinvariantsarray is empty. In that casecheck_learning_ledgercurrently skips the promotion check entirely, as flagged onscripts/lib/architecture_learning.pylines 162-200. Add a case that emptiesinvariantsin the temporary contract and asserts a failing status. Write the test together with the fix so the fail-open path stays closed.🤖 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 `@tests/test_architecture_learning.py` around lines 159 - 164, Extend test_promotion_must_point_at_a_declared_invariant to empty the temporary contract’s invariants array and assert check_learning_ledger returns a failed status, while preserving the existing unknown-invariant assertion for non-empty invariants. Update the promotion validation in check_learning_ledger so promotions are still checked when invariants is empty and cannot silently pass.
🤖 Prompt for all review comments with 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.
Inline comments:
In `@openspec/changes/introduce-executable-architecture-contracts/tasks.md`:
- Around line 414-420: 更新驗證紀錄中的 pytest 指令，將 `python -m pytest tests -q -p
no:cacheprovider` 改為使用 `./.venv/Scripts/python.exe -m pytest tests -p
no:cacheprovider`；若原紀錄確實使用系統 Python，先以指定的 `.venv` 直譯器重新執行測試並更新通過結果。
In `@openspec/lifecycle-ledger.json`:
- Around line 1384-1395: Update the lifecycle-ledger row’s subject_commit to
584314158cd87342cc5808d3f9bf090abfbc6919 so it matches the cited tasks.md
snapshot. Ensure this ledger correction precedes any NOW projection change, and
preserve status as active while the entry remains pending archive.
In `@scripts/dev/report_architecture_quality.py`:
- Around line 36-41: Update the --strict argument help text in the argument
parser to state its behavior directly, without a question-and-answer format.
Clarify that ledger warnings cause a failing result and that this matches the
sibling checkers, while preserving the existing semantics.
In `@scripts/lib/architecture_learning.py`:
- Around line 334-354: Update _count_baseline_debt so missing or non-list
service_edges, cycles, or violations baseline keys are treated as unreadable
rather than converted to zero. Validate the confirmed key names and container
types for both baseline documents, and append the same appropriate
baseline-unreadable issue when validation fails, while preserving the existing
counts for valid arrays and file-read/JSON parse failures.
- Around line 162-200: Update the promotion validation around invariant_ids and
the promoted_to check so a loaded contract with no valid invariant IDs also
fails closed: every promoted_to reference must be rejected when invariant_ids is
empty. Preserve the existing unverifiable issue for an unloaded contract, and
adjust the unknown-invariant condition and message or issue code as needed to
distinguish the empty-invariant case.
---
Nitpick comments:
In `@scripts/lib/architecture_learning.py`:
- Around line 325-331: Move the json import from _count_baseline_debt to the
module-level imports, then update the list construction at the referenced RUF005
site to use iterable unpacking instead of list concatenation. Preserve the
existing ordering and resulting values.
In `@tests/test_architecture_learning.py`:
- Around line 224-245: Extend test_report_script_passes_on_canonical_repository
to invoke the report CLI with --output targeting a file under tmp_path, then
assert the generated file content exactly matches completed.stdout. Preserve the
existing JSON assertions and ensure the test covers parent-directory creation
and byte-identical output.
- Around line 159-164: Extend test_promotion_must_point_at_a_declared_invariant
to empty the temporary contract’s invariants array and assert
check_learning_ledger returns a failed status, while preserving the existing
unknown-invariant assertion for non-empty invariants. Update the promotion
validation in check_learning_ledger so promotions are still checked when
invariants is empty and cannot silently pass.
🪄 Autofix
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 Plus
Run ID: bebf874a-7c50-4773-9447-95e6dc0c6303
📒 Files selected for processing (11)
architecture/README.mdarchitecture/deltas/introduce-executable-architecture-contracts.jsonarchitecture/learning-ledger.jsonarchitecture/learning-ledger.schema.jsonopenspec/changes/introduce-executable-architecture-contracts/design.mdopenspec/changes/introduce-executable-architecture-contracts/tasks.mdopenspec/lifecycle-ledger.jsonscripts/dev/report_architecture_quality.pyscripts/lib/architecture_learning.pyscripts/verification-manifest.jsontests/test_architecture_learning.py
There was a problem hiding this comment.
Pull request overview
This PR delivers Phase 5 (the final phase) of the introduce-executable-architecture-contracts OpenSpec change, bringing its task ledger to 26/26. It adds a machine-readable "learning ledger" that classifies recurring $improve-codebase-architecture findings into named erosion patterns, records pattern→gate promotions verifiably, and adds a read-only quality-grade report that aggregates the four architecture gates (observed-graph, layer-boundary, lifecycle, learning-ledger) plus attributed debt into a deterministic A/B/C snapshot. It fits into the existing governance/architecture-contract tooling under architecture/ and scripts/lib+scripts/dev, and is purely additive (no product API/runtime behavior change).
Changes:
- Adds
architecture/learning-ledger.json(+ Draft-07 schema) classifying the ten 2026-07-30 findings into six erosion patterns and recording three prior promotions; C1 isrefactored, the other nine are honestopenbacklog. - Adds
scripts/lib/architecture_learning.py(fail-closed ledger validation + quality aggregation) andscripts/dev/report_architecture_quality.py(read-only graded snapshot; canonical grade B, 16 attributed debt entries), with 14 new pinned/fail-closed tests. - Updates governance bookkeeping: OpenSpec
tasks.md/design.md/lifecycle-ledger.json(26/26), the architecture delta,architecture/README.md, andscripts/verification-manifest.json(routes both new scripts intoroot-contracts).
I verified the implementation against the surrounding code and data: the debt counts (2 undeclared edges + 3 grandfathered cycles + 2 layer violations + 9 open findings = 16) match the baseline files; all three promotions point at real invariants declared in architecture-contract.json; the schema exercises only Draft-07 features supported by the repo's validate_schema_instance; every pattern class is referenced; and tasks.md contains exactly 26 checked / 0 unchecked boxes consistent with the ledger's 26/26. No objective defects were found.
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
scripts/lib/architecture_learning.py |
Core: ledger reference-integrity validation (fail-closed) + quality report aggregation and grading. |
scripts/dev/report_architecture_quality.py |
Read-only CLI publishing gate statuses, attributed debt, and grade; strict/exit semantics match sibling checkers. |
tests/test_architecture_learning.py |
14 tests: canonical pass, pinned literals, 8 fail-closed counterexamples, quality aggregation, subprocess run. |
architecture/learning-ledger.json |
Six erosion patterns, three promotions, ten findings (C1 refactored; C2–F5 open). |
architecture/learning-ledger.schema.json |
Draft-07 schema constraining patterns/promotions/findings/resolution. |
architecture/deltas/introduce-executable-architecture-contracts.json |
Records the ledger as an additive public-contract change. |
architecture/README.md |
Documents the new files and the not-yet-claimed scope. |
scripts/verification-manifest.json |
Adds the two new scripts to the root-contracts path class and target globs. |
openspec/lifecycle-ledger.json |
Updates task_ledger to 26/26, last_verified, current_slice, and subject_commit. |
openspec/changes/.../tasks.md |
Marks 5.1–5.3 done with a delivery record. |
openspec/changes/.../design.md |
Marks Phase 5 complete in the phasing summary. |
💡 Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.
…malformed baselines are not zero debt CodeRabbit round on #479: a loaded contract whose invariant list is empty or fully malformed now still rejects every promotion reference (the truthiness guard skipped verification exactly when the contract lost its invariants); a baseline that is not the expected object shape surfaces as baseline_unreadable instead of silently counting zero debt; and the --strict help text states its semantics directly. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015QTVFY89rS2xRwRB2TpFP6
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 02287edc34
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
… strings fail the schema Second connector round on #479: a promotion pointing at a declared but non-active invariant (planned/delegated enforcement) now fails as invariant_not_active - the ledger cannot claim protection canonical verification does not run - and the ledger schema's nonEmptyString requires a non-whitespace character. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015QTVFY89rS2xRwRB2TpFP6
…rs unique; promoted findings need the record Third connector round on #479: --strict now fails on any constituent gate's warnings (a warning that fails export_observed --strict can no longer pass the aggregate), duplicate promotion pairs fail validation, and a finding flipped to promoted without a matching promoted_patterns entry fails as promotion_unrecorded - the grade cannot improve on an unrecorded promise. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015QTVFY89rS2xRwRB2TpFP6
monkey1sai-blip
left a comment
monkey1sai-blip
left a comment
There was a problem hiding this comment.
Approved by monkey1sai-blip (the reviewer account pinned by the repo's merge governance).
Submitted through scripts/blip_review.py — a scripted approval carrying the operator's authority, pinned to head 016ba74341f214166364e116dc999267e44a7716. This is the mechanism the GitHub App cannot satisfy: an App's approving review does not count toward required_approving_review_count.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 016ba74341
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…480) All 26 tasks across five phases delivered and merged (#459-era proposal through #479). The change moves to openspec/changes/archive/2026-08-10-introduce-executable-architecture-contracts, its ten requirements land in openspec/specs/executable-architecture-contracts, and the ledger row is archived with subject_commit rebound to the #479 squash (d0125cc, permanently reachable main history - the #477 convention). Claude-Session: https://claude.ai/code/session_015QTVFY89rS2xRwRB2TpFP6 Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
What
Phase 5 — the final phase of
introduce-executable-architecture-contracts(tasks 5.1–5.3); the task ledger reaches 26/26.architecture/learning-ledger.json(+ Draft-07 schema) classifies the ten findings of the 2026-07-30$improve-codebase-architectureround (C1–C5 / F1–F5) into six named erosion patterns (duplicate-capability-implementations,god-file-accretion,unregistered-literal-fanout,test-fixture-monolith,deep-module-consolidationas the positive template,structural-erosion-classesat repo level). C1 is recordedrefactored(PR refactor(coordinator): 深化 MinIO Watch Surface 並以 pollNow 消除觀測競態（C1 PR1/2） #442/refactor(coordinator): MinIO Watch Surface PR2 — browse/SSE/trigger 併入、minioClient 退場（C1 PR2/2） #444); the other nine are honestlyopenbacklog, described as observations of that round rather than claims about current line numbers.promoted_patternsrecords pattern→gate promotions machine-verifiably:structural-erosion-classeshas been promoted three times already (ARCH-GRAPH-001 / ARCH-LAYER-001 / ARCH-LIFECYCLE-001 — the Phase 2–4 ratchets).scripts/lib/architecture_learning.pyfails closed when a promotion points at an invariant the architecture contract does not declare, plus pattern reference integrity, duplicates, status↔resolution-kind agreement, and unused-pattern warnings. Closing a C2–F5 item later flips its status with a reference, and the pinned literals force that edit into the review diff.scripts/dev/report_architecture_quality.pyaggregates all four gates plus the attributed debt inventory into one deterministic snapshot: A = all green and zero debt, B = all green with attributed debt, C = any gate failing. Canonical today: grade B (four gates green; 2 undeclared edges + 3 grandfathered cycles + 2 layer violations + 9 open findings = 16 attributed debt entries). The tool is read-only — "without auto-merging repairs" is the contract. Honest limit: the report is a current snapshot; a shallow CI clone cannot reconstruct a time series, so "trend" is expressed as the debt inventory.Behavior contract changed = yesrefers to the governance machine-contract surface: the PR addsarchitecture/learning-ledger.json(+ schema), declared additively in the delta'spublic_contract_changes. No product API, event contract, or runtime behavior changes.Validation (Windows governed worktree, Python 3.12.7 / pytest 8.2.2)
python -m pytest tests -q -p no:cacheprovider— 478 passed, 9 skipped (+14 fromtests/test_architecture_learning.py: canonical, pins, 8 fail-closed counterexamples, quality aggregation)python scripts/dev/report_architecture_quality.py --repo-root . --strict— grade B, exit 0, four gates passed, 16 attributed debt entriesnpx openspec validate introduce-executable-architecture-contracts --strict— validnode scripts/tests/test-openspec-machine-truth.mjs— 24/24 (after the 26/26 ledger update and subject bind)node scripts/tests/test-verification-plan.mjs— 23/23 (new paths dispatch root-contracts)git diff --check— cleanPost-merge follow-up (same convention as #477): rebind the row's
subject_committo this PR's squash SHA.Known Risks
🤖 Generated with Claude Code
https://claude.ai/code/session_015QTVFY89rS2xRwRB2TpFP6
Summary by CodeRabbit