fix: enforce static architecture policy contracts - #1059
Conversation
|
Warning Review limit reached
More reviews will be available in 45 minutes. Learn how PR review limits work. Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable the review add-on in the billing tab to keep reviews running โ you're only billed for reviews past your plan's rate limits ($0.25/file). โ How to resolve this issue?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 credits. ๐ฆ How do rate limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan refill rate. 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, the refill rate gradually slows as usage increases. The highest same-day bursts are limited more strictly. Please see our Fair Usage Limits Policy for further information. โน๏ธ Review infoโ๏ธ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: โ Files ignored due to path filters (1)
๐ Files selected for processing (33)
๐ Walkthrough๊ฐ์
๋ณ๊ฒฝ์ฌํญ์ ์ ์ํคํ ์ฒ ์ ์ฑ ์์ง ๋ฐ CLI ๊ฒ์ดํธ
์ํ์ค ๋ค์ด์ด๊ทธ๋จsequenceDiagram
participant Developer as ๊ฐ๋ฐ์ / CI
participant CLI as croco CLI
participant Engine as `@croco/architecture-policy`
participant Manifest as croco.arch.json
participant SourceFiles as ์์ค ํ์ผ
Developer->>CLI: croco architecture-policy check --manifest croco.arch.json
CLI->>Manifest: readArchitecturePolicyManifest(path)
Manifest-->>CLI: ArchitecturePolicyManifest
CLI->>Engine: checkArchitecturePolicy(options)
Engine->>SourceFiles: walkFiles + collectSourceImports
SourceFiles-->>Engine: ImportRecord[]
Engine->>Engine: forbiddenImports ๊ท์น ์คํ
Engine->>Engine: allowedGroupImports ๊ท์น ์คํ
Engine->>Engine: publicEntrypoints ๊ท์น ์คํ
Engine-->>CLI: ArchitecturePolicyReport(pass/fail, diagnostics[])
alt ํต๊ณผ
CLI->>Developer: "passed N imports across M packages" (exit 0)
else ์คํจ
CLI->>Developer: formatArchitecturePolicyDiagnostic ร N + "N errors" (exit 1)
end
์์ ๊ฒํ ์๊ฐ๐ฏ 4 (๋ณต์ก) | โฑ๏ธ ~60๋ถ ๊ด๋ จ ์ด์
๊ด๋ จ PR
๐ฅ Pre-merge checks | โ 4 | โ 1โ Failed checks (1 warning)
โ Passed checks (4 passed)
โ๏ธ Tip: You can configure your own custom pre-merge checks in the settings. โจ 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 |
There was a problem hiding this comment.
Actionable comments posted: 5
๐ค 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 `@packages/architecture-policy/src/index.ts`:
- Around line 151-158: The error handling in the manifest parser is throwing
generic Error instances instead of Problem subclasses, which violates the
repository's exception contract. Replace the two generic Error throws (one in
the isRecord validation check and one in the schemaVersion validation check)
with appropriate Problem subclasses that implement the RFC 7807 Problem format.
Each Problem subclass should include proper code and category properties to
enable consistent error handling upstream. Define or use existing Problem
subclasses that represent these specific validation failures.
- Around line 824-833: The matchesTargetMatcher function has two issues: first,
on line 828, matcher.paths is incorrectly compared against context.sourcePath
when it should be compared against context.targetPath since this function is
evaluating target matchers; second, on line 832, the function uses
checks.some(Boolean) which applies OR logic (returns true if any condition
matches), but it should use checks.every(Boolean) to apply AND logic so that all
present conditions in the matcher must be true for the overall match to succeed.
Fix both the wrong context property and the logic operator to ensure target
matchers evaluate correctly without over-matching.
In `@packages/architecture-policy/src/tests/ArchitecturePolicy.spec.ts`:
- Around line 278-285: The test for parseArchitecturePolicyManifest currently
only validates the error message string, which allows any Error type to pass the
test. Strengthen this test by replacing the simple message validation with a
check that validates the exception is a Problem subclass with the correct code
and category properties. Modify the expect().toThrow() call to validate not just
the error message but also confirm the thrown error is an instance of the
appropriate Problem class and verify its code and category property values match
the expected exception contract.
In `@packages/cli/src/commands/architecturePolicy.ts`:
- Around line 117-118: The issue is that getFlagValue for --root does not
properly consume its value argument, causing that value to be incorrectly picked
up as a positional argument by getFirstPosition in the manifest fallback on line
117. The --root value should not be treated as a positional argument. Ensure
that getFlagValue properly consumes both the flag and its associated value so
that getFirstPosition only returns actual positional arguments that were not
consumed by flag parsing. This will prevent `--root repo` from incorrectly
assigning repo to the manifest variable.
In `@scripts/architecture-policy-check.mts`:
- Around line 23-27: The argument validation for the --manifest flag (and
similarly for other flags around lines 33-37) only checks if a value exists, but
doesn't verify that the value isn't itself another flag starting with '--'. When
parsing the value from args[index + 1], add a check to ensure the value doesn't
start with '--' in addition to the existing null/undefined check, so that
invalid inputs like '--manifest --json' are properly rejected with a usage error
instead of silently accepting the flag as a path value.
๐ช 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: a57743f8-7fe9-4522-9fb2-97c14a1c2584
โ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
๐ Files selected for processing (27)
.changeset/static-architecture-policy.mdREADME.mdcroco.arch.jsondocs/architecture-policy.mddocs/package-catalog.jsondocs/package-docs-baseline.jsondocs/package-docs-report.mdpackage.jsonpackages/architecture-policy/README.mdpackages/architecture-policy/package.jsonpackages/architecture-policy/src/index.tspackages/architecture-policy/src/tests/ArchitecturePolicy.spec.tspackages/architecture-policy/tsconfig.jsonpackages/cli/package.jsonpackages/cli/src/commands/architecturePolicy.tspackages/cli/src/commands/root.tspackages/cli/src/index.tspackages/cli/src/tests/architecturePolicyCheck.spec.tspackages/cli/vitest.config.tspackages/create-croco-app/src/generator.tspackages/create-croco-app/src/tests/e2e-generation.spec.tspackages/create-croco-app/src/tests/templates-build.spec.tspackages/create-croco-app/templates/saas/package.json.hbspublic-api-surface.snapshot.jsonscripts/architecture-policy-check.mtsscripts/create-croco-app-generated-smoke.mtsscripts/package-quality-report.mts
8f287fa to
bf6a1bc
Compare
๐ Benchmark Resultsโ Some benchmarks failed Gate failures
Updated: 2026-06-21T12:42:01.166Z ยท Commit: b77fdd2 |
eaca863 to
6ca6757
Compare
6ca6757 to
0a1ebbd
Compare
There was a problem hiding this comment.
Actionable comments posted: 9
๐ค 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`:
- Around line 107-122: The `to.packages` list in the
framework-provider-package-boundary rule (lines 107-122) is incomplete and does
not include all the integration implementation packages defined earlier in the
file (lines 56-73). Review the integrations section to identify all
implementation packages and add the missing ones to the `to.packages` array,
including packages like `@croco/*-posthog`, `@croco/events-tx`,
`@croco/metrics-billing`, and `@croco/telemetry-sdk-node` that are currently
missing. Ensure the forbidden packages list comprehensively covers all
integration implementation packages so the framework group cannot directly
import them.
In `@docs/package-docs-baseline.json`:
- Line 9: The entries in the allowedMissingApiDocs array are not sorted in
alphabetical order. The entry "architecture-policy" appears before
"analytics-posthog", but alphabetically "analytics" should come before
"architecture". Reorder the array entries so that they are in alphabetical
order, ensuring "analytics-posthog" appears before "architecture-policy" in the
allowedMissingApiDocs array.
In `@package.json`:
- Line 7: The pnpm filter selector in the architecture-policy:check script uses
`@croco/architecture-policy^...` which excludes the target package itself from
the build, causing import failures when scripts/architecture-policy-check.mts
tries to use `@croco/architecture-policy`. Change the filter from
`@croco/architecture-policy^...` to `@croco/architecture-policy...` to include
the package itself in the build process.
In `@packages/architecture-policy/src/index.ts`:
- Around line 175-177: The parseArchitecturePolicyManifest function and the
similar code at line 476-477 directly call JSON.parse without error handling,
which throws generic SyntaxError instances that violate the coding guideline
requiring only Problem subclasses to be thrown. Wrap the JSON.parse calls in
try-catch blocks at both locations (lines 176-177 and 476-477), catch any errors
thrown by JSON.parse, and convert them to an appropriate Problem subclass before
throwing, ensuring the error information includes relevant code and category
properties for consistent handling by upper layers.
In `@packages/architecture-policy/src/tests/ArchitecturePolicy.spec.ts`:
- Around line 519-527: The captureError function violates the coding guideline
by throwing a generic Error instance at the end of the function. Instead of
using throw new Error for the failure case when the function does not throw an
exception as expected, replace it with a Vitest assertion API call such as
fail() which provides a proper way to fail the test without throwing a generic
Error class. This maintains the same intent while adhering to the rule that only
Problem subclasses should be thrown.
In `@packages/cli/src/commands/architecturePolicy.ts`:
- Around line 199-200: The resolvePath function uses path.startsWith("/") to
detect absolute paths, which only works for POSIX-style paths. This causes
Windows absolute paths like C:\repo or C:/repo to be treated as relative paths
and incorrectly resolved against the cwd. Replace the path.startsWith("/") check
with the path.isAbsolute() method, which correctly identifies absolute paths
across all operating systems (both POSIX and Windows).
In `@packages/create-croco-app/src/generator.ts`:
- Around line 249-256: The ignore patterns in the generator.ts file are missing
the `__tests__` directory convention used in many projects. Add the pattern
`src/**/__tests__/**` to the ignore arrays in both locations (the block starting
at line 249 and the block starting at line 349) alongside the existing patterns
for `src/**/tests/**`, `*.spec.ts`, and `*.test.ts`. This will ensure that
projects using the `__tests__` naming convention for test directories won't
trigger false positives in architecture-policy checks by treating test code as
production imports.
In `@packages/create-croco-app/src/tests/e2e-generation.spec.ts`:
- Line 784: The assertion in the e2e-generation.spec.ts file is checking for an
exact version string "^0.0.4" for the "`@croco/cli`" dev dependency, which will
cause test failures whenever the package version is updated. Instead of using
toBe() with a hardcoded version, change the assertion to validate that the
"`@croco/cli`" dependency exists and matches a semver pattern using a regular
expression like /^\^[0-9]+\.[0-9]+\.[0-9]+$/, or simply verify the dependency
key exists in the devDependencies object without validating the specific version
number.
In `@packages/create-croco-app/src/tests/templates-build.spec.ts`:
- Around line 426-427: The regex pattern for the `demo:smoke` task validation at
line 426-427 is incomplete and does not verify the final `ops:smoke` stage in
the chain. Extend the stringMatching regex pattern to include `ops:smoke` at the
end after `api-server demo:smoke` to ensure the complete task chain is validated
and prevent the test from passing when the `ops:smoke` step is missing from the
template.
๐ช 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: d4541166-3065-460f-9264-127af50ad9c7
๐ Files selected for processing (24)
.changeset/static-architecture-policy.mdREADME.mdcroco.arch.jsondocs/architecture-policy.mddocs/package-catalog.jsondocs/package-docs-baseline.jsondocs/package-docs-report.mddocs/problem-code-registry.jsonpackage.jsonpackages/architecture-policy/README.mdpackages/architecture-policy/package.jsonpackages/architecture-policy/src/index.tspackages/architecture-policy/src/tests/ArchitecturePolicy.spec.tspackages/architecture-policy/tsconfig.jsonpackages/cli/package.jsonpackages/cli/src/commands/architecturePolicy.tspackages/cli/src/commands/root.tspackages/cli/src/index.tspackages/cli/src/tests/architecturePolicyCheck.spec.tspackages/cli/vitest.config.tspackages/create-croco-app/src/generator.tspackages/create-croco-app/src/tests/e2e-generation.spec.tspackages/create-croco-app/src/tests/templates-build.spec.tspackages/create-croco-app/templates/saas/package.json.hbs
๐ค Files with no reviewable changes (1)
- docs/problem-code-registry.json
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 9
๐ค 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`:
- Around line 107-122: The `to.packages` list in the
framework-provider-package-boundary rule (lines 107-122) is incomplete and does
not include all the integration implementation packages defined earlier in the
file (lines 56-73). Review the integrations section to identify all
implementation packages and add the missing ones to the `to.packages` array,
including packages like `@croco/*-posthog`, `@croco/events-tx`,
`@croco/metrics-billing`, and `@croco/telemetry-sdk-node` that are currently
missing. Ensure the forbidden packages list comprehensively covers all
integration implementation packages so the framework group cannot directly
import them.
In `@docs/package-docs-baseline.json`:
- Line 9: The entries in the allowedMissingApiDocs array are not sorted in
alphabetical order. The entry "architecture-policy" appears before
"analytics-posthog", but alphabetically "analytics" should come before
"architecture". Reorder the array entries so that they are in alphabetical
order, ensuring "analytics-posthog" appears before "architecture-policy" in the
allowedMissingApiDocs array.
In `@package.json`:
- Line 7: The pnpm filter selector in the architecture-policy:check script uses
`@croco/architecture-policy^...` which excludes the target package itself from
the build, causing import failures when scripts/architecture-policy-check.mts
tries to use `@croco/architecture-policy`. Change the filter from
`@croco/architecture-policy^...` to `@croco/architecture-policy...` to include
the package itself in the build process.
In `@packages/architecture-policy/src/index.ts`:
- Around line 175-177: The parseArchitecturePolicyManifest function and the
similar code at line 476-477 directly call JSON.parse without error handling,
which throws generic SyntaxError instances that violate the coding guideline
requiring only Problem subclasses to be thrown. Wrap the JSON.parse calls in
try-catch blocks at both locations (lines 176-177 and 476-477), catch any errors
thrown by JSON.parse, and convert them to an appropriate Problem subclass before
throwing, ensuring the error information includes relevant code and category
properties for consistent handling by upper layers.
In `@packages/architecture-policy/src/tests/ArchitecturePolicy.spec.ts`:
- Around line 519-527: The captureError function violates the coding guideline
by throwing a generic Error instance at the end of the function. Instead of
using throw new Error for the failure case when the function does not throw an
exception as expected, replace it with a Vitest assertion API call such as
fail() which provides a proper way to fail the test without throwing a generic
Error class. This maintains the same intent while adhering to the rule that only
Problem subclasses should be thrown.
In `@packages/cli/src/commands/architecturePolicy.ts`:
- Around line 199-200: The resolvePath function uses path.startsWith("/") to
detect absolute paths, which only works for POSIX-style paths. This causes
Windows absolute paths like C:\repo or C:/repo to be treated as relative paths
and incorrectly resolved against the cwd. Replace the path.startsWith("/") check
with the path.isAbsolute() method, which correctly identifies absolute paths
across all operating systems (both POSIX and Windows).
In `@packages/create-croco-app/src/generator.ts`:
- Around line 249-256: The ignore patterns in the generator.ts file are missing
the `__tests__` directory convention used in many projects. Add the pattern
`src/**/__tests__/**` to the ignore arrays in both locations (the block starting
at line 249 and the block starting at line 349) alongside the existing patterns
for `src/**/tests/**`, `*.spec.ts`, and `*.test.ts`. This will ensure that
projects using the `__tests__` naming convention for test directories won't
trigger false positives in architecture-policy checks by treating test code as
production imports.
In `@packages/create-croco-app/src/tests/e2e-generation.spec.ts`:
- Line 784: The assertion in the e2e-generation.spec.ts file is checking for an
exact version string "^0.0.4" for the "`@croco/cli`" dev dependency, which will
cause test failures whenever the package version is updated. Instead of using
toBe() with a hardcoded version, change the assertion to validate that the
"`@croco/cli`" dependency exists and matches a semver pattern using a regular
expression like /^\^[0-9]+\.[0-9]+\.[0-9]+$/, or simply verify the dependency
key exists in the devDependencies object without validating the specific version
number.
In `@packages/create-croco-app/src/tests/templates-build.spec.ts`:
- Around line 426-427: The regex pattern for the `demo:smoke` task validation at
line 426-427 is incomplete and does not verify the final `ops:smoke` stage in
the chain. Extend the stringMatching regex pattern to include `ops:smoke` at the
end after `api-server demo:smoke` to ensure the complete task chain is validated
and prevent the test from passing when the `ops:smoke` step is missing from the
template.
๐ช 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: d4541166-3065-460f-9264-127af50ad9c7
๐ Files selected for processing (24)
.changeset/static-architecture-policy.mdREADME.mdcroco.arch.jsondocs/architecture-policy.mddocs/package-catalog.jsondocs/package-docs-baseline.jsondocs/package-docs-report.mddocs/problem-code-registry.jsonpackage.jsonpackages/architecture-policy/README.mdpackages/architecture-policy/package.jsonpackages/architecture-policy/src/index.tspackages/architecture-policy/src/tests/ArchitecturePolicy.spec.tspackages/architecture-policy/tsconfig.jsonpackages/cli/package.jsonpackages/cli/src/commands/architecturePolicy.tspackages/cli/src/commands/root.tspackages/cli/src/index.tspackages/cli/src/tests/architecturePolicyCheck.spec.tspackages/cli/vitest.config.tspackages/create-croco-app/src/generator.tspackages/create-croco-app/src/tests/e2e-generation.spec.tspackages/create-croco-app/src/tests/templates-build.spec.tspackages/create-croco-app/templates/saas/package.json.hbs
๐ค Files with no reviewable changes (1)
- docs/problem-code-registry.json
๐ Comments failed to post (9)
croco.arch.json (1)
107-122:
โ ๏ธ Potential issue | ๐ Major | โก Quick win
framework-provider-package-boundary๊ธ์ง ๋์์ด ๋ถ์์ ํฉ๋๋ค.Line 107-122์
to.packages๊ฐ ๊ฐ์ ํ์ผ Line 56-73์ integrations ๊ตฌํ ํจํค์ง์ ๋ถ์ผ์นํฉ๋๋ค. ํ์ฌ ์ํ๋ฉด framework ๊ทธ๋ฃน์ด ์ผ๋ถ ๊ตฌํ ํจํค์ง(@croco/*-posthog,@croco/events-tx,@croco/metrics-billing,@croco/telemetry-sdk-node๋ฑ)๋ฅผ ์ง์ importํด๋ ์ฐจ๋จ๋์ง ์์ต๋๋ค.์์ ์์
"to": { "packages": [ + "`@croco/`*-posthog", "`@croco/`*-drizzle", "`@croco/`*-qstash", "`@croco/`*-upstash", "`@croco/auth-better-auth`", "`@croco/auth-clerk`", "`@croco/billing-polar`", + "`@croco/events-tx`", "`@croco/llm-openai`", + "`@croco/metrics-billing`", "`@croco/notifications-resend`", "`@croco/search-meilisearch`", "`@croco/storage-cloudflare`", "`@croco/storage-cloudinary`", "`@croco/storage-r2`", + "`@croco/telemetry-sdk-node`", "`@croco/transports-`*", "`@croco/tx-drizzle`" ] },๐ Committable suggestion
โผ๏ธ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements."packages": [ "`@croco/`*-posthog", "`@croco/`*-drizzle", "`@croco/`*-qstash", "`@croco/`*-upstash", "`@croco/auth-better-auth`", "`@croco/auth-clerk`", "`@croco/billing-polar`", "`@croco/events-tx`", "`@croco/llm-openai`", "`@croco/metrics-billing`", "`@croco/notifications-resend`", "`@croco/search-meilisearch`", "`@croco/storage-cloudflare`", "`@croco/storage-cloudinary`", "`@croco/storage-r2`", "`@croco/telemetry-sdk-node`", "`@croco/transports-`*", "`@croco/tx-drizzle`" ]๐ค 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 107 - 122, The `to.packages` list in the framework-provider-package-boundary rule (lines 107-122) is incomplete and does not include all the integration implementation packages defined earlier in the file (lines 56-73). Review the integrations section to identify all implementation packages and add the missing ones to the `to.packages` array, including packages like `@croco/*-posthog`, `@croco/events-tx`, `@croco/metrics-billing`, and `@croco/telemetry-sdk-node` that are currently missing. Ensure the forbidden packages list comprehensively covers all integration implementation packages so the framework group cannot directly import them.docs/package-docs-baseline.json (1)
9-9:
โ ๏ธ Potential issue | ๐ก Minor | โก Quick win
allowedMissingApiDocs๋ฐฐ์ด์ ์ํ๋ฒณ ์์๊ฐ ์๋ชป๋์์ต๋๋ค.Line 9์
"architecture-policy"๋ line 10์"analytics-posthog"๋ณด๋ค ๋ค์ ์์ผ ํฉ๋๋ค. "analytics"๊ฐ "architecture"๋ณด๋ค ์ํ๋ฒณ ์์์ ์์ ๋๋ค.- "admin-react", - "architecture-policy", - "analytics-posthog", + "admin-react", + "analytics-posthog", + "architecture-policy",๐ Committable suggestion
โผ๏ธ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements."admin-react", "analytics-posthog", "architecture-policy",๐ค 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 `@docs/package-docs-baseline.json` at line 9, The entries in the allowedMissingApiDocs array are not sorted in alphabetical order. The entry "architecture-policy" appears before "analytics-posthog", but alphabetically "analytics" should come before "architecture". Reorder the array entries so that they are in alphabetical order, ensuring "analytics-posthog" appears before "architecture-policy" in the allowedMissingApiDocs array.package.json (1)
7-7:
โ ๏ธ Potential issue | ๐ Major๐งฉ Analysis chain
๐ Web query:
pnpm filter ๋ฌธ๋ฒ์์ "<package>^..."๋ ์ด๋ค ํจํค์ง ์งํฉ์ ์ ํํ๋์? ๋์ ํจํค์ง ์์ ๋ ํฌํจ๋๋์?๐ก Result:
pnpm์ filter ๋ฌธ๋ฒ์์ ^...๋ ๋์ ํจํค์ง์ ์์กด์ฑ(์ง์ ๋ฐ ๊ฐ์ ์์กด์ฑ ๋ชจ๋ ํฌํจ)๋ง์ ์ ํํฉ๋๋ค [1][2][3]. ์ง๋ฌธํ์ ๋ด์ฉ์ ๋ํ ํต์ฌ ๋ต๋ณ์ ๋ค์๊ณผ ๊ฐ์ต๋๋ค: ๋์ ํจํค์ง ์์ ์ ํฌํจ๋์ง ์์ต๋๋ค [1][3][4]. ์ด ๋ฌธ๋ฒ์ ํน์ ํจํค์ง์ ์์กด์ฑ๋ค์ ๋์์ผ๋ก ์์ (์: ํ ์คํธ ์คํ, ๋น๋ ๋ฑ)์ ์ํํ๊ณ ์ ํ ๋, ํด๋น ํจํค์ง ์์ฒด๋ ์ ์ธํ๊ณ ์ถ์ ๊ฒฝ์ฐ์ ์ฌ์ฉ๋ฉ๋๋ค [1][2][4]. ์ฐธ๊ณ ๋ก, ๋์ ํจํค์ง๊น์ง ํฌํจํ์ฌ ์์กด์ฑ์ ์ ํํ๋ ค๋ฉด ... ๋ฌธ๋ฒ์ ์ฌ์ฉํด์ผ ํฉ๋๋ค [1][5][3].
Citations:
- 1: https://pnpm.io/filtering
- 2: https://pnpm.io/next/filtering
- 3: https://github.com/pnpm/pnpm.io/blob/main/versioned_docs/version-10.x/filtering.md
- 4: pnpm/pnpm#2201
- 5: https://github.com/pnpm/spec/blob/master/workspace/filtering.md
pnpm --filterํํฐ ์ ํ์ ์์ ํ์
@croco/architecture-policy^...ํํฐ๋ ๋์ ํจํค์ง์ ์์กด์ฑ๋ง ์ ํํ๊ณ@croco/architecture-policy์์ฒด๋ ์ ์ธํฉ๋๋ค. ๋ฐ๋ผ์scripts/architecture-policy-check.mts์์@croco/architecture-policy๋ฅผ importํ ๋ ๋น๋ ์ฐ์ถ๋ฌผ์ด ์์ด ์คํจํฉ๋๋ค.
@croco/architecture-policy^...๋์@croco/architecture-policy...๋ก ์์ ํ์ฌ ๋์ ํจํค์ง๋ ํจ๊ป ๋น๋ํ์ธ์.๐ค 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 `@package.json` at line 7, The pnpm filter selector in the architecture-policy:check script uses `@croco/architecture-policy^...` which excludes the target package itself from the build, causing import failures when scripts/architecture-policy-check.mts tries to use `@croco/architecture-policy`. Change the filter from `@croco/architecture-policy^...` to `@croco/architecture-policy...` to include the package itself in the build process.packages/architecture-policy/src/index.ts (1)
175-177:
โ ๏ธ Potential issue | ๐ Major | โก Quick winJSON ํ์ฑ ์คํจ๋ฅผ
Problemํ์ ํ์ ์ผ๋ก ์ ๊ทํํ์ธ์.Line 176-177, Line 476-477์์
JSON.parse์SyntaxError๊ฐ ๊ทธ๋๋ก ์ ํ๋์ด ์์ธ ๊ณ์ฝ์ด ๊นจ์ง๋๋ค. ์๋ชป๋ JSON ์ ๋ ฅ ์ ์์ ๋ ์ด์ด๊ฐcode/category๊ธฐ๋ฐ์ผ๋ก ์ผ๊ด ์ฒ๋ฆฌํ์ง ๋ชปํฉ๋๋ค.์์ ์์
+export class ArchitecturePolicyManifestJsonParseProblem extends Problem { + constructor() { + super( + "architecture-policy/manifest-json-parse", + ProblemCategory.ValidationError, + "Architecture policy manifest must contain valid JSON.", + ); + } +} + +export class ArchitecturePolicyPackageJsonParseProblem extends Problem { + constructor(packageJsonPath: string) { + super( + "architecture-policy/package-json-parse", + ProblemCategory.ValidationError, + `Invalid package.json: ${packageJsonPath}.`, + ); + } +} + export function parseArchitecturePolicyManifest(content: string): ArchitecturePolicyManifest { - const parsed = JSON.parse(content) as unknown; + let parsed: unknown; + try { + parsed = JSON.parse(content) as unknown; + } catch { + throw new ArchitecturePolicyManifestJsonParseProblem(); + } @@ function readJsonFile(path: string): unknown { - return JSON.parse(readFileSync(path, "utf-8")) as unknown; + try { + return JSON.parse(readFileSync(path, "utf-8")) as unknown; + } catch { + throw new ArchitecturePolicyPackageJsonParseProblem(path); + } }As per coding guidelines,
Throw only Problem subclasses, never generic Error instances๊ท์น์ ๋ฐ๋ผ์ผ ํฉ๋๋ค.Also applies to: 476-477
๐ค 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 `@packages/architecture-policy/src/index.ts` around lines 175 - 177, The parseArchitecturePolicyManifest function and the similar code at line 476-477 directly call JSON.parse without error handling, which throws generic SyntaxError instances that violate the coding guideline requiring only Problem subclasses to be thrown. Wrap the JSON.parse calls in try-catch blocks at both locations (lines 176-177 and 476-477), catch any errors thrown by JSON.parse, and convert them to an appropriate Problem subclass before throwing, ensuring the error information includes relevant code and category properties for consistent handling by upper layers.Source: Coding guidelines
packages/architecture-policy/src/tests/ArchitecturePolicy.spec.ts (1)
519-527: ๐ ๏ธ Refactor suggestion | ๐ Major | โก Quick win
ํ ์คํธ ํฌํผ์ ์คํจ ๊ฒฝ๋ก์์ generic
Error์ฌ์ฉ์ ์ ๊ฑฐํ์ธ์.Line 526์
new Error(...)๋์ Vitest assertion API๋ฅผ ์ฌ์ฉํ๋ฉด ์์ธ ๊ณ์ฝ ์๋ฐ ์์ด ๋์ผํ ์๋๋ฅผ ์ ์งํ ์ ์์ต๋๋ค.์์ ์์
function captureError(fn: () => void): unknown { try { fn(); } catch (error) { return error; } - throw new Error("Expected function to throw."); + expect.unreachable("Expected function to throw."); }As per coding guidelines,
Throw only Problem subclasses, never generic Error instances๊ท์น์ ์ค์ํด์ผ ํฉ๋๋ค.๐ Committable suggestion
โผ๏ธ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.function captureError(fn: () => void): unknown { try { fn(); } catch (error) { return error; } expect.unreachable("Expected function to throw."); }๐ค 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 `@packages/architecture-policy/src/tests/ArchitecturePolicy.spec.ts` around lines 519 - 527, The captureError function violates the coding guideline by throwing a generic Error instance at the end of the function. Instead of using throw new Error for the failure case when the function does not throw an exception as expected, replace it with a Vitest assertion API call such as fail() which provides a proper way to fail the test without throwing a generic Error class. This maintains the same intent while adhering to the rule that only Problem subclasses should be thrown.Source: Coding guidelines
packages/cli/src/commands/architecturePolicy.ts (1)
199-200:
โ ๏ธ Potential issue | ๐ Major๐งฉ Analysis chain
๐ Script executed:
# Check if the file exists and examine the relevant lines head -n 220 packages/cli/src/commands/architecturePolicy.ts | tail -n 30Repository: croco-dev/framework
Length of output: 734
๐ Script executed:
# Check the imports at the top of the file head -n 30 packages/cli/src/commands/architecturePolicy.tsRepository: croco-dev/framework
Length of output: 1048
๐ Script executed:
# Get the entire function context around lines 199-200 sed -n '190,210p' packages/cli/src/commands/architecturePolicy.tsRepository: croco-dev/framework
Length of output: 304
๐ Script executed:
# Verify that isAbsolute is available from node:path module node -e "const path = require('path'); console.log(typeof path.isAbsolute);"Repository: croco-dev/framework
Length of output: 72
๐ Web query:
node:path isAbsolute cross-platform Windows absolute paths๐ก Result:
In Node.js, the path.isAbsolute(path) method determines if a path is absolute based on the operating system of the current process [1][2]. When running on Windows, it uses the Windows-specific rules for determining absolute paths [1][3]. Key behaviors on Windows include: 1. Rooted Paths: Paths that start with a drive letter and a colon followed by a separator (e.g., C:\foo or C:/foo) are considered absolute [1][4]. 2. UNC Paths: Paths starting with a double separator (e.g., \\server\share or //server/share) are considered absolute [1][4]. 3. Drive-Relative Paths: A point of common confusion is that paths starting with a single separator (e.g., \Windows or /Windows) are also considered absolute by Node.js [5][4]. While these are technically "drive-relative" (relative to the current drive's root), Node.js follows the Windows API convention which treats them as absolute because they contain a root component [5][6]. If you need to strictly verify whether a path is fully qualified (i.e., including both a drive letter/UNC root and a path), you cannot rely solely on path.isAbsolute [6]. Instead, you must manually check if the path starts with a drive letter (e.g., /^[a-zA-Z]:\/ or similar) or a UNC prefix [6]. The path.win32 object provides access to these Windows-specific path methods regardless of the host operating system, which is useful for cross-platform processing of Windows paths [2]. Note that path.isAbsolute is intended for checking the structure of a string and is not secure for mitigating path traversal vulnerabilities [1][2].
Citations:
- 1: https://nodejs.org/api/path.html
- 2: https://github.com/nodejs/node/blob/main/doc/api/path.md
- 3: https://nodejs.org/dist/latest/docs/api/path.html
- 4: nodejs/node@1c40079
- 5: nodejs/node#56766
- 6: nodejs/node#2656
Windows ํ๊ฒฝ์์ ์ ๋ ๊ฒฝ๋ก ํ๋ณ์ด ์ค์๋ํฉ๋๋ค.
Line 199์
path.startsWith("/")๋ POSIX ๊ฒฝ๋ก๋ง ์ ๋ ๊ฒฝ๋ก๋ก ์ธ์ํฉ๋๋ค. Windows์C:\repo๋๋C:/repo๊ฐ์ ๊ฒฝ๋ก๋ ์๋ ๊ฒฝ๋ก๋ก ์ฒ๋ฆฌ๋์ดcwd์ ์๋ชป ๊ฒฐํฉ๋ฉ๋๋ค.path.isAbsolute()๋ฅผ ์ฌ์ฉํ๋ฉด OS์ ๋ง๊ฒ ์ ๋ ๊ฒฝ๋ก๋ฅผ ์ฌ๋ฐ๋ฅด๊ฒ ํ๋ณํ ์ ์์ต๋๋ค.๐ง ์์ ์ ์
-import { resolve } from "node:path"; +import { isAbsolute, resolve } from "node:path"; @@ function resolvePath(path: string, cwd: string): string { - return path.startsWith("/") ? path : resolve(cwd, path); + return isAbsolute(path) ? path : resolve(cwd, path); }๐ Committable suggestion
โผ๏ธ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.import { isAbsolute, resolve } from "node:path"; function resolvePath(path: string, cwd: string): string { return isAbsolute(path) ? path : resolve(cwd, path); }๐ค 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 `@packages/cli/src/commands/architecturePolicy.ts` around lines 199 - 200, The resolvePath function uses path.startsWith("/") to detect absolute paths, which only works for POSIX-style paths. This causes Windows absolute paths like C:\repo or C:/repo to be treated as relative paths and incorrectly resolved against the cwd. Replace the path.startsWith("/") check with the path.isAbsolute() method, which correctly identifies absolute paths across all operating systems (both POSIX and Windows).packages/create-croco-app/src/generator.ts (1)
249-256:
โ ๏ธ Potential issue | ๐ก Minor | โก Quick winํ ์คํธ ์ ์ธ glob์
__tests__๊ฐ ๋น ์ ธ ์์ด ์ ์ฑ ์คํ ๊ฐ๋ฅ์ฑ์ด ์์ต๋๋ค.Line 249-256๊ณผ Line 349-356์์
src/**/tests/**,*.spec.ts,*.test.ts๋ง ์ ์ธํ๊ณsrc/**/__tests__/**๋ฅผ ์ ์ธํ์ง ์์ต๋๋ค. ์์ฑ ํ๋ก์ ํธ๊ฐ__tests__๊ด๋ก๋ฅผ ์ฐ๋ฉด architecture-policy๊ฐ ํ ์คํธ ์ฝ๋๋ฅผ production import๋ก ๊ฒ์ฌํ ์ ์์ต๋๋ค.์์ ์์
ignore: [ + "apps/*/src/**/__tests__/**", "apps/*/src/**/tests/**", "apps/*/src/**/*.spec.ts", "apps/*/src/**/*.test.ts", + "libs/shared/*/src/**/__tests__/**", "libs/shared/*/src/**/tests/**", "libs/shared/*/src/**/*.spec.ts", "libs/shared/*/src/**/*.test.ts", ], ... ignoreImports: [ { paths: [ + "apps/*/src/**/__tests__/**", "apps/*/src/**/tests/**", "apps/*/src/**/*.spec.ts", "apps/*/src/**/*.test.ts", + "libs/shared/*/src/**/__tests__/**", "libs/shared/*/src/**/tests/**", "libs/shared/*/src/**/*.spec.ts", "libs/shared/*/src/**/*.test.ts", ],Also applies to: 349-356
๐ค 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 `@packages/create-croco-app/src/generator.ts` around lines 249 - 256, The ignore patterns in the generator.ts file are missing the `__tests__` directory convention used in many projects. Add the pattern `src/**/__tests__/**` to the ignore arrays in both locations (the block starting at line 249 and the block starting at line 349) alongside the existing patterns for `src/**/tests/**`, `*.spec.ts`, and `*.test.ts`. This will ensure that projects using the `__tests__` naming convention for test directories won't trigger false positives in architecture-policy checks by treating test code as production imports.packages/create-croco-app/src/tests/e2e-generation.spec.ts (1)
784-784:
โ ๏ธ Potential issue | ๐ก Minor | โก Quick winํจํค์ง ๋ฒ์ ๊ณ ์ assertion์ด E2E๋ฅผ ๋ถํ์ํ๊ฒ ๊นจ๋จ๋ฆด ์ ์์ต๋๋ค.
Line 784์
toBe('^0.0.4')๋ ๋ฒ์ ์ ๋ฐ์ดํธ๋ง๋ค ํ ์คํธ๋ฅผ ๊นจ๊ฒ ๋ฉ๋๋ค. ์์ฑ ๊ฒฐ๊ณผ์ ์๋๋ โCLI ์์กด์ฑ ์กด์ฌ/ํ์โ ๊ฒ์ฆ์ด๋ฏ๋ก semver ํจํด ๋๋ ์กด์ฌ์ฑ ๊ฒ์ฆ์ผ๋ก ์ํํ๋ ํธ์ด ์์ ํฉ๋๋ค.๐ค 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 `@packages/create-croco-app/src/tests/e2e-generation.spec.ts` at line 784, The assertion in the e2e-generation.spec.ts file is checking for an exact version string "^0.0.4" for the "`@croco/cli`" dev dependency, which will cause test failures whenever the package version is updated. Instead of using toBe() with a hardcoded version, change the assertion to validate that the "`@croco/cli`" dependency exists and matches a semver pattern using a regular expression like /^\^[0-9]+\.[0-9]+\.[0-9]+$/, or simply verify the dependency key exists in the devDependencies object without validating the specific version number.packages/create-croco-app/src/tests/templates-build.spec.ts (1)
426-427:
โ ๏ธ Potential issue | ๐ก Minor | โก Quick win
demo:smoke์ ๊ท์์ด ๋ง์ง๋งops:smoke๋จ๊ณ๋ฅผ ๋ณด์ฅํ์ง ์์ต๋๋ค.Line 426-427 ํจํด์
api-server demo:smoke๊น์ง๋ง ๊ฒ์ฆํด์, ํ ํ๋ฆฟ์์ops:smoke๊ฐ ๋น ์ ธ๋ ํต๊ณผํ ์ ์์ต๋๋ค. ์ฒด์ธ ์๊ฒฐ์ฑ์ ๊ฒ์ฆํ๋๋ก ํจํด ๋์ops:smoke๊น์ง ํฌํจํด ์ฃผ์ธ์.๐ค 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 `@packages/create-croco-app/src/tests/templates-build.spec.ts` around lines 426 - 427, The regex pattern for the `demo:smoke` task validation at line 426-427 is incomplete and does not verify the final `ops:smoke` stage in the chain. Extend the stringMatching regex pattern to include `ops:smoke` at the end after `api-server demo:smoke` to ensure the complete task chain is validated and prevent the test from passing when the `ops:smoke` step is missing from the template.
ddb49f1 to
8457dc3
Compare
|
@coderabbitai review |
โ Action performedReview finished.
|
There was a problem hiding this comment.
Actionable comments posted: 5
๐ค 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 `@packages/architecture-policy/package.json`:
- Around line 14-25: The top-level `main` and `types` fields in package.json
point to `./src/index.ts`, but since only the `dist/` directory is published to
npm (via the `files` array), external consumers cannot resolve these paths.
Update the top-level `main` field to `./dist/index.js` and the `types` field to
`./dist/index.d.ts` to match the values in `publishConfig`, ensuring the
published package can be correctly resolved by consumers.
In `@packages/architecture-policy/src/index.ts`:
- Around line 763-765: The condition on line 763-765 in the index.ts file checks
only if context.targetGroup is null to determine if a dependency is external,
but targetGroup can be null for both actual external packages and internal
packages that lack group mapping. This causes unclassified internal packages to
bypass the allowedGroupImports validation when allowExternal is true. Fix this
by adding an additional check to distinguish between true external dependencies
and unmapped internal packages. You need to verify that the dependency is
actually external (not just unclassified internal) before allowing it through
the allowExternal exception, ensuring that internal packages without group
mapping still go through the allowedGroupImports validation gate.
In `@packages/architecture-policy/src/tests/ArchitecturePolicy.spec.ts`:
- Around line 485-517: Add a regression test case to verify the allowExternal
behavior in the generatedAppManifest function. The test should validate that
when allowExternal is set to true in the generated-app-layer-edges rule,
external packages from allowPackages are correctly permitted, while internal
packages that are not mapped to allowed groups are properly rejected. This
ensures the allowExternal logic does not regress in future changes.
In `@packages/cli/src/commands/architecturePolicy.ts`:
- Around line 172-193: The getConsumedArgumentIndexes function uses indexOf to
find flag positions, which only returns the first occurrence of a flag. When
duplicate flags like --root or --manifest appear multiple times (e.g., --root a
--root b), only the first flag and its value are marked as consumed, causing the
second flag's value to be incorrectly treated as a positional argument. Replace
indexOf with lastIndexOf for the --manifest and --root flags in the first loop
to capture the last occurrence of each flag, ensuring all duplicate flag values
are properly marked as consumed and not confused with positional arguments.
In `@packages/create-croco-app/src/generator.ts`:
- Around line 249-258: The ignore patterns in the architecture policy check
configuration are excluding TypeScript test files (*.spec.ts, *.test.ts) but not
their TypeScript JSX equivalents (*.spec.tsx, *.test.tsx). Since the policy
check explicitly includes TSX files, TSX test files are unintentionally being
checked against the policy rules. Add four additional ignore patterns to the
ignore array in both locations: one for "*.spec.tsx" files in apps, one for
"*.test.tsx" files in apps, one for "*.spec.tsx" files in libs/shared, and one
for "*.test.tsx" files in libs/shared. This should be done in the ignore array
around line 249-258 and also in the similar ignore configuration around line
351-360.
๐ช 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: 8ef0adbd-9b0d-46f7-b3af-22aaf45ef1ff
โ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
๐ Files selected for processing (33)
.changeset/static-architecture-policy.mdREADME.mdcroco.arch.jsondocs/architecture-policy.mddocs/package-catalog.jsondocs/package-docs-baseline.jsondocs/package-docs-report.mddocs/problem-code-registry.jsonpackage.jsonpackages/architecture-policy/README.mdpackages/architecture-policy/package.jsonpackages/architecture-policy/src/index.tspackages/architecture-policy/src/tests/ArchitecturePolicy.spec.tspackages/architecture-policy/tsconfig.jsonpackages/cli/package.jsonpackages/cli/src/commands/architecturePolicy.tspackages/cli/src/commands/root.tspackages/cli/src/index.tspackages/cli/src/tests/architecturePolicyCheck.spec.tspackages/cli/vitest.config.tspackages/create-croco-app/src/generator.tspackages/create-croco-app/src/tests/e2e-generation.spec.tspackages/create-croco-app/src/tests/templates-build.spec.tspackages/create-croco-app/templates/saas/package.json.hbspackages/docs/src/content/docs/en/guides/getting-started.mdxpackages/docs/src/content/docs/en/index.mdxpackages/docs/src/content/docs/en/reference/problem-recovery-cookbook.mdpublic-api-surface.snapshot.jsonscripts/architecture-policy-check.mtsscripts/create-croco-app-generated-smoke.mtsscripts/package-entrypoint-smoke.mtsscripts/package-quality-report.mtsscripts/tests/package-entrypoint-smoke.spec.ts
8457dc3 to
6f86fb4
Compare
Summary
croco architecture-policy checkthrough @croco/cli and runs the repo gate inpnpm check.Verification
pnpm --filter @croco/architecture-policy testpnpm --filter @croco/architecture-policy typecheckpnpm architecture-policy:checkpnpm checkpnpm changeset-required:checkpnpm exec turbo run build --filter=@croco/architecture-policy --filter=@croco/cli --filter=create-croco-apppnpm create-croco-app:smokepnpm test,pnpm typecheckSummary by CodeRabbit
๋ฆด๋ฆฌ์ค ๋ ธํธ
croco architecture-policy check๋ช ๋ น ๋ฐ--json๋ฆฌํฌํธ ์ง์croco.arch.json์ด ํฌํจ๋๊ณ , ์์ฑ ๊ณผ์ ์ ๊ฒ ๋จ๊ณ์ ์๋ ์ถ๊ฐ