fix: enforce architecture catalog group consistency - #1200
Conversation
|
Warning Review limit reached
Next review available in: 44 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. 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: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (6)
📝 WalkthroughWalkthroughcroco.arch.json의 패키지 그룹 구성을 재분류하고 카탈로그 그룹 오버라이드를 추가했으며, architecture-policy-check.mts에 공개 워크스페이스 패키지와 package-catalog.json 간 그룹 일관성을 검증하는 로직과 출력/종료코드 변경을 도입했다. 이를 검증하는 신규 Vitest 통합 테스트 스위트도 추가되었다. Changes패키지 카탈로그-정책 그룹 일관성 검증
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant CLI as architecture-policy-check.mts
participant Manifest as croco.arch.json
participant Catalog as docs/package-catalog.json
participant Packages as packages/*
CLI->>Manifest: resolveManifestPath 후 매니페스트 로드
CLI->>Packages: readPublicWorkspacePackages 호출
CLI->>Catalog: readPackageCatalogGroups 호출
CLI->>Manifest: packageCatalogGroupOverrides 파싱
CLI->>CLI: findMatchingArchitecturePackageGroups로 그룹 비교
CLI-->>CLI: violations 누적 및 status 결정
CLI-->>CLI: JSON/텍스트 출력 및 종료 코드 반환
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 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 |
📊 Benchmark Results✅ All benchmarks passed
Updated: 2026-07-03T23:50:51.509Z · Commit: 8586d0e |
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
croco.arch.json (1)
89-108: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
forbiddenImports의to.packages목록이integrations그룹과 어긋남.
packageGroups.integrations.packages(89-108)에@croco/telemetry-api가 추가되었지만, framework→provider 임포트를 막는forbiddenImports규칙의to.packages(157-176)에는 반영되지 않았습니다. 두 목록이 수동으로 동기화되는 구조이기 때문에, framework 패키지가@croco/telemetry-api를 임포트해도 이 규칙으로는 차단되지 않아 아키텍처 경계 정책에 구멍이 생깁니다.🛠️ 제안 수정
"`@croco/storage-r2`", + "`@croco/telemetry-api`", "`@croco/telemetry-sdk-node`", "`@croco/transports-`*",Also applies to: 157-176
🤖 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 `@croco.arch.json` around lines 89 - 108, The forbiddenImports rule is missing the newly added `@croco/telemetry-api` entry, so framework-to-provider imports are not fully blocked. Update the forbiddenImports configuration’s to.packages list to stay in sync with packageGroups.integrations.packages, using the same package identifier(s) already present there, so the architecture boundary enforced by this rule includes `@croco/telemetry-api`.
🤖 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 `@croco.arch.json`:
- Line 105: Remove `@croco/telemetry-api` from the integrations group in the arch
config, since it is a shared telemetry API package rather than an integration
module. Update the relevant grouping entry in croco.arch.json so only actual
integration packages remain, and keep the API surface that provides Trace,
withSpan, recordError, getTracer, and the browser correlation bridge out of that
group.
In `@scripts/architecture-policy-check.mts`:
- Around line 3-68: The manifest is being read twice because
`checkPackageCatalogGroupConsistency` re-parses the same file with a separate
raw JSON path just to access `packageCatalogGroupOverrides`. Add
`packageCatalogGroupOverrides` to the `ArchitecturePolicyManifest` schema used
by `readArchitecturePolicyManifest`, then update
`checkPackageCatalogGroupConsistency` to consume the parsed manifest directly
instead of introducing `readJsonFile<RawManifest>`, keeping the logic centered
around `checkArchitecturePolicy`, `formatArchitecturePolicyDiagnostic`, and
`readArchitecturePolicyManifest`.
- Around line 320-334: `readPackageCatalogGroups` is reading
`docs/package-catalog.json` without a file-existence guard, so a missing catalog
crashes the check instead of recording a violation. Update
`readPackageCatalogGroups` to mirror the `readPublicWorkspacePackages` pattern
by checking for the catalog file before calling `readJsonFile`, and if it is
absent push a `PackageCatalogGroupViolation` with an actionable
message/recovery/evidence and return an empty map.
- Around line 178-188: The package catalog consistency check is hardcoded to
read only the default workspace set and ignores the manifest’s packageRoots.
Update checkPackageCatalogGroupConsistency to derive the workspace package list
from the manifest configuration and pass the configured roots into
readPublicWorkspacePackages so apps/libs-style additions are included. Use the
existing symbols checkPackageCatalogGroupConsistency,
readPublicWorkspacePackages, and ArchitecturePolicyManifest to wire the
manifest-driven roots through this path.
---
Outside diff comments:
In `@croco.arch.json`:
- Around line 89-108: The forbiddenImports rule is missing the newly added
`@croco/telemetry-api` entry, so framework-to-provider imports are not fully
blocked. Update the forbiddenImports configuration’s to.packages list to stay in
sync with packageGroups.integrations.packages, using the same package
identifier(s) already present there, so the architecture boundary enforced by
this rule includes `@croco/telemetry-api`.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: fde6f63b-e5e3-4d14-8846-97951a4e4a41
📒 Files selected for processing (3)
croco.arch.jsonscripts/architecture-policy-check.mtsscripts/tests/architecture-policy-check.spec.ts
Fixes #1147.
Summary
architecture-policy:check.packageCatalogGroupOverridesfor intentional catalog/policy group differences.croco.arch.jsonpackage groups so current public packages classify exactly once, with explicit overrides for@croco/events-txand@croco/tx-drizzle.Verification
corepack pnpm vitest run scripts/tests/architecture-policy-check.spec.tscorepack pnpm architecture-policy:checkcorepack pnpm checkcorepack pnpm testcorepack pnpm typecheckgit diff --checkReview / QA
APPROVE, 0 findings.Summary by CodeRabbit
Bug Fixes
New Features