fix: verify provider no-credential behavior - #1447
Merged
Merged
Conversation
|
Warning Review limit reached
Next review available in: 47 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 Plus Run ID: 📒 Files selected for processing (7)
📝 WalkthroughWalkthrough공통 no-credential conformance suite와 공개 타입이 추가되었습니다. 여러 provider 테스트가 누락 credential 진단, 네트워크 호출 0회, secret redaction을 검증하며 certification evidence와 관련 문서가 갱신되었습니다. ChangesProvider no-credential conformance
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant ProviderTest
participant ConformanceSuite
participant Provider
participant CertificationCheck
ProviderTest->>ConformanceSuite: 누락 credential 시나리오 등록
ConformanceSuite->>Provider: 환경 변수 제거 후 진단 실행
Provider-->>ConformanceSuite: diagnostic 및 networkAttempts 반환
ConformanceSuite-->>ProviderTest: 안정성, 호출 0회, redaction 검증
CertificationCheck->>ConformanceSuite: noCredentialSmoke evidence 검사
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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-24T06:42:58.351Z · Commit: 314aa65 |
kang-heewon
force-pushed
the
issue-1260-provider-no-credential
branch
from
July 22, 2026 22:24
2f27e8c to
d07b20f
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/telemetry-sdk-node/src/tests/ProviderNoCredentialConformance.spec.ts`:
- Around line 23-52: Ensure the fetch spy created in the run callback is
restored on every exit path, including when TelemetryRuntime.getInstance().init
throws a non-Problem error. Refactor the try/catch flow around TelemetryRuntime
initialization to use try/finally, preserving the existing Problem result and
rethrow behavior while guaranteeing fetchSpy.mockRestore() before returning or
propagating.
🪄 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: 61b01153-4ca0-430f-ac75-9110a141bd4f
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (28)
.changeset/provider-no-credential-conformance.mdREADME.mddocs/package-catalog.jsondocs/package-docs-report.mdpackages/auth-clerk/src/libs/ClerkAuthDiagnosticsProvider.tspackages/auth-clerk/src/tests/ProviderNoCredentialConformance.spec.tspackages/billing-polar/src/tests/ProviderNoCredentialConformance.spec.tspackages/docs/src/content/docs/api/testing/src/functions/createProviderNoCredentialConformanceSuite.mdpackages/docs/src/content/docs/api/testing/src/type-aliases/ProviderNoCredentialConformanceCase.mdpackages/docs/src/content/docs/api/testing/src/type-aliases/ProviderNoCredentialConformanceOptions.mdpackages/docs/src/content/docs/api/testing/src/type-aliases/ProviderNoCredentialConformanceSuite.mdpackages/docs/src/content/docs/api/testing/src/type-aliases/ProviderNoCredentialDiagnostic.mdpackages/docs/src/content/docs/api/testing/src/type-aliases/ProviderNoCredentialProbeResult.mdpackages/docs/src/content/docs/api/testing/src/type-aliases/ProviderNoCredentialScenario.mdpackages/docs/src/content/docs/en/reference/extension-matrix.mdpackages/storage-r2/src/tests/ProviderNoCredentialConformance.spec.tspackages/tasks-qstash/src/tests/ProviderNoCredentialConformance.spec.tspackages/tasks-qstash/tsconfig.jsonpackages/tasks-qstash/vitest.config.tspackages/telemetry-sdk-node/package.jsonpackages/telemetry-sdk-node/src/tests/ProviderNoCredentialConformance.spec.tspackages/testing/src/index.tspackages/testing/src/libs/provider-no-credential-conformance.tspackages/testing/src/tests/ConformanceContract.spec.tspackages/testing/src/tests/ProviderNoCredentialConformance.spec.tspublic-api-surface.snapshot.jsonscripts/provider-certification-check.mtsscripts/tests/provider-certification-check.spec.ts
Member
Author
|
@coderabbitai resolve |
✅ Action performedComments resolved and changes approved. |
Member
Author
|
Current validate failure is an independent base-wide dependency audit regression tracked in #1451. Exact |
kang-heewon
force-pushed
the
issue-1260-provider-no-credential
branch
from
July 24, 2026 05:10
63d1180 to
3a685ae
Compare
There was a problem hiding this comment.
Actionable comments posted: 8
🤖 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 `@docs/package-catalog.json`:
- Around line 1128-1165: Update the knownGaps entry for `@croco/auth-clerk` to
contain the actual live-smoke evidence reason instead of the field name
"liveSmoke". Add the same descriptive reason to the corresponding
knownGapAllowances mapping, following the existing wording pattern used by other
package entries.
In `@packages/billing-polar/src/tests/ProviderNoCredentialConformance.spec.ts`:
- Around line 1-3: Reorder the imports in
ProviderNoCredentialConformance.spec.ts to follow external packages first, then
internal `@croco/`* packages, then relative imports: place vitest before
`@croco/testing`, while keeping PolarBillingDiagnosticsProvider after them.
In `@packages/storage-r2/src/tests/ProviderNoCredentialConformance.spec.ts`:
- Around line 1-3: Reorder the imports in
ProviderNoCredentialConformance.spec.ts so the vitest import precedes the
`@croco/testing` import, matching the established ordering pattern in the related
specs while leaving the provider import unchanged.
In `@packages/tasks-qstash/src/tests/ProviderNoCredentialConformance.spec.ts`:
- Around line 1-3: Update the import order in
ProviderNoCredentialConformance.spec.ts so the external vitest import appears
before the internal `@croco/problems-core` and `@croco/testing` imports, matching
the ordering used by the related spec files.
- Around line 5-11: Move the mockPublishJSON initialization into a vi.hoisted
callback and have the `@upstash/qstash` vi.mock factory reuse that hoisted mock,
ensuring the mock exists before QStashTaskRunner and the mock factory are
evaluated.
In
`@packages/telemetry-sdk-node/src/tests/ProviderNoCredentialConformance.spec.ts`:
- Line 25: Update the fetch mock in ProviderNoCredentialConformance tests to
reject with the project’s appropriate Problem subclass instead of a generic
Error, while preserving the existing “network” failure scenario.
In `@packages/testing/src/tests/ProviderNoCredentialConformance.spec.ts`:
- Around line 45-55: Update the test around createSuite and the suite.cases loop
to save the original FIXTURE_API_TOKEN value before overriding it, then restore
that exact value in a finally block. Ensure restoration occurs whether any
testCase.run() succeeds or throws, deleting the variable only when it was
originally absent.
- Around line 68-89: Update the test around
createProviderNoCredentialConformanceSuite so the synchronous validation
exception is asserted at suite creation time with a synchronous toThrow matcher.
Remove the cases[0].run() and async rejects assertion while preserving the
expected “at least one secret sample” message.
🪄 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 Plus
Run ID: 882a36af-d217-4b05-8ffd-e84ce8fd50c7
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (28)
.changeset/provider-no-credential-conformance.mdREADME.mddocs/package-catalog.jsondocs/package-docs-report.mdpackages/auth-clerk/src/libs/ClerkAuthDiagnosticsProvider.tspackages/auth-clerk/src/tests/ProviderNoCredentialConformance.spec.tspackages/billing-polar/src/tests/ProviderNoCredentialConformance.spec.tspackages/docs/src/content/docs/api/testing/src/functions/createProviderNoCredentialConformanceSuite.mdpackages/docs/src/content/docs/api/testing/src/type-aliases/ProviderNoCredentialConformanceCase.mdpackages/docs/src/content/docs/api/testing/src/type-aliases/ProviderNoCredentialConformanceOptions.mdpackages/docs/src/content/docs/api/testing/src/type-aliases/ProviderNoCredentialConformanceSuite.mdpackages/docs/src/content/docs/api/testing/src/type-aliases/ProviderNoCredentialDiagnostic.mdpackages/docs/src/content/docs/api/testing/src/type-aliases/ProviderNoCredentialProbeResult.mdpackages/docs/src/content/docs/api/testing/src/type-aliases/ProviderNoCredentialScenario.mdpackages/docs/src/content/docs/en/reference/extension-matrix.mdpackages/storage-r2/src/tests/ProviderNoCredentialConformance.spec.tspackages/tasks-qstash/src/tests/ProviderNoCredentialConformance.spec.tspackages/tasks-qstash/tsconfig.jsonpackages/tasks-qstash/vitest.config.tspackages/telemetry-sdk-node/package.jsonpackages/telemetry-sdk-node/src/tests/ProviderNoCredentialConformance.spec.tspackages/testing/src/index.tspackages/testing/src/libs/provider-no-credential-conformance.tspackages/testing/src/tests/ConformanceContract.spec.tspackages/testing/src/tests/ProviderNoCredentialConformance.spec.tspublic-api-surface.snapshot.jsonscripts/provider-certification-check.mtsscripts/tests/provider-certification-check.spec.ts
kang-heewon
force-pushed
the
issue-1260-provider-no-credential
branch
from
July 24, 2026 06:28
3a685ae to
77f2a60
Compare
Member
Author
|
@coderabbitai resolve |
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (2)
packages/billing-polar/src/tests/ProviderNoCredentialConformance.spec.ts (1)
1-3: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win네 개의 no-credential conformance spec 파일 모두 import 순서 가이드라인을 위반합니다. "Organize imports in order: external packages, internal
@croco/* packages, relative paths" 규칙에 따라vitest(external)가@croco/testing/@croco/problems-core(internal)보다 먼저 와야 하는데, 네 파일 모두 반대 순서입니다. billing-polar/storage-r2/tasks-qstash는 과거 리뷰에서 이미 지적되었으나 미해결 상태이며, telemetry-sdk-node는 동일 패턴의 신규 인스턴스입니다.
packages/billing-polar/src/tests/ProviderNoCredentialConformance.spec.ts#L1-L3:vitestimport를@croco/testingimport보다 앞으로 이동.packages/storage-r2/src/tests/ProviderNoCredentialConformance.spec.ts#L1-L3:vitestimport를@croco/testingimport보다 앞으로 이동.packages/tasks-qstash/src/tests/ProviderNoCredentialConformance.spec.ts#L1-L3:vitestimport를@croco/problems-core/@croco/testingimport보다 앞으로 이동.packages/telemetry-sdk-node/src/tests/ProviderNoCredentialConformance.spec.ts#L1-L4:vitestimport를@croco/problems-core/@croco/testingimport보다 앞으로 이동.🤖 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/billing-polar/src/tests/ProviderNoCredentialConformance.spec.ts` around lines 1 - 3, Reorder imports in the four ProviderNoCredentialConformance.spec.ts files: packages/billing-polar/src/tests/ProviderNoCredentialConformance.spec.ts lines 1-3, packages/storage-r2/src/tests/ProviderNoCredentialConformance.spec.ts lines 1-3, packages/tasks-qstash/src/tests/ProviderNoCredentialConformance.spec.ts lines 1-3, and packages/telemetry-sdk-node/src/tests/ProviderNoCredentialConformance.spec.ts lines 1-4. Place the external vitest import before the internal `@croco/testing` or `@croco/problems-core` imports, followed by the relative provider import, without changing functionality.Source: Coding guidelines
docs/package-catalog.json (1)
1164-1164: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
knownGaps에 필드명 대신 실제 미충족 사유를 기록하세요.
["liveSmoke"]는 어떤 증거가 부족한지 설명하지 못하고 생성 보고서에도 그대로 노출됩니다. 두 record를 서술형 사유로 통일해 주세요.수정 예시
- "knownGaps": ["liveSmoke"] + "knownGaps": ["Recorded real Clerk liveSmoke evidence is required before certified state."] ... - "knownGaps": ["liveSmoke"] + "knownGaps": ["Recorded real QStash liveSmoke evidence is required before certified state."]Also applies to: 1203-1203
🤖 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-catalog.json` at line 1164, docs/package-catalog.json의 두 record에서 knownGaps 값으로 사용한 "liveSmoke" 필드명을 실제로 충족하지 못한 검증 사유를 설명하는 서술형 문자열로 교체하세요. 두 record 모두 동일한 형식과 의미의 구체적인 사유를 사용하고, 생성 보고서에 그대로 노출되어도 부족한 증거가 명확히 드러나도록 하세요.
🤖 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/auth-clerk/src/tests/ProviderNoCredentialConformance.spec.ts`:
- Around line 1-3: Reorder the imports in
ProviderNoCredentialConformance.spec.ts so the external vitest import appears
before the `@croco/testing` import, while leaving the ClerkAuthDiagnosticsProvider
internal import after both external imports.
In `@packages/tasks-qstash/src/tests/ProviderNoCredentialConformance.spec.ts`:
- Line 50: Replace the generic TypeError failure guard in
packages/tasks-qstash/src/tests/ProviderNoCredentialConformance.spec.ts at line
50 with assert.ok(false, ...) or an existing Problem subclass. Apply the same
change to
packages/telemetry-sdk-node/src/tests/ProviderNoCredentialConformance.spec.ts at
line 51, preserving the existing failure messages and test behavior.
- Around line 5-11: Defensively wrap mockPublishJSON with vi.hoisted() before
the vi.mock factory, then use the hoisted mock in the Client class field
initializer. Keep the existing publishJSON behavior unchanged while ensuring the
mock remains safe if Client is instantiated during module initialization.
In `@packages/testing/src/libs/provider-no-credential-conformance.ts`:
- Around line 102-114: Update serializeDiagnostic, used by the secret-redaction
conformance checks, so serialization includes secret-bearing non-enumerable
properties such as those on Error instances instead of relying solely on
JSON.stringify. Preserve the existing diagnostic output while ensuring both
redaction assertions detect secrets nested in diagnostic.details.
- Around line 205-207: Update assertNonEmpty to validate that value is a string
before calling trim, ensuring non-string inputs fail through assert.ok with the
provided message and produce an AssertionError rather than a raw TypeError.
---
Duplicate comments:
In `@docs/package-catalog.json`:
- Line 1164: docs/package-catalog.json의 두 record에서 knownGaps 값으로 사용한 "liveSmoke"
필드명을 실제로 충족하지 못한 검증 사유를 설명하는 서술형 문자열로 교체하세요. 두 record 모두 동일한 형식과 의미의 구체적인 사유를
사용하고, 생성 보고서에 그대로 노출되어도 부족한 증거가 명확히 드러나도록 하세요.
In `@packages/billing-polar/src/tests/ProviderNoCredentialConformance.spec.ts`:
- Around line 1-3: Reorder imports in the four
ProviderNoCredentialConformance.spec.ts files:
packages/billing-polar/src/tests/ProviderNoCredentialConformance.spec.ts lines
1-3, packages/storage-r2/src/tests/ProviderNoCredentialConformance.spec.ts lines
1-3, packages/tasks-qstash/src/tests/ProviderNoCredentialConformance.spec.ts
lines 1-3, and
packages/telemetry-sdk-node/src/tests/ProviderNoCredentialConformance.spec.ts
lines 1-4. Place the external vitest import before the internal `@croco/testing`
or `@croco/problems-core` imports, followed by the relative provider import,
without changing functionality.
🪄 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 Plus
Run ID: 43df4364-5e7c-4aef-bd65-7b0331490c38
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (28)
.changeset/provider-no-credential-conformance.mdREADME.mddocs/package-catalog.jsondocs/package-docs-report.mdpackages/auth-clerk/src/libs/ClerkAuthDiagnosticsProvider.tspackages/auth-clerk/src/tests/ProviderNoCredentialConformance.spec.tspackages/billing-polar/src/tests/ProviderNoCredentialConformance.spec.tspackages/docs/src/content/docs/api/testing/src/functions/createProviderNoCredentialConformanceSuite.mdpackages/docs/src/content/docs/api/testing/src/type-aliases/ProviderNoCredentialConformanceCase.mdpackages/docs/src/content/docs/api/testing/src/type-aliases/ProviderNoCredentialConformanceOptions.mdpackages/docs/src/content/docs/api/testing/src/type-aliases/ProviderNoCredentialConformanceSuite.mdpackages/docs/src/content/docs/api/testing/src/type-aliases/ProviderNoCredentialDiagnostic.mdpackages/docs/src/content/docs/api/testing/src/type-aliases/ProviderNoCredentialProbeResult.mdpackages/docs/src/content/docs/api/testing/src/type-aliases/ProviderNoCredentialScenario.mdpackages/docs/src/content/docs/en/reference/extension-matrix.mdpackages/storage-r2/src/tests/ProviderNoCredentialConformance.spec.tspackages/tasks-qstash/src/tests/ProviderNoCredentialConformance.spec.tspackages/tasks-qstash/tsconfig.jsonpackages/tasks-qstash/vitest.config.tspackages/telemetry-sdk-node/package.jsonpackages/telemetry-sdk-node/src/tests/ProviderNoCredentialConformance.spec.tspackages/testing/src/index.tspackages/testing/src/libs/provider-no-credential-conformance.tspackages/testing/src/tests/ConformanceContract.spec.tspackages/testing/src/tests/ProviderNoCredentialConformance.spec.tspublic-api-surface.snapshot.jsonscripts/provider-certification-check.mtsscripts/tests/provider-certification-check.spec.ts
✅ Action performedComments resolved and changes approved. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Outcome
Provider packages can now prove no-credential behavior through one public
@croco/testingconformance contract. The suite clears declared environment variables, requires stable actionable diagnostics, rejects any live network/API attempt, and fails closed unless at least one secret sample is proven absent from diagnostics.Auth Clerk, Polar billing, R2 storage, QStash tasks, and the Node telemetry SDK publish package-scoped evidence through the provider certification catalog. Certification records now require a concrete no-credential test artifact even before a provider reaches certified maturity.
This closes a security and release-evidence gap: missing provider credentials are verified without contacting live services or exposing configured values.
Verification
@croco/testing— 88/88@croco/auth-clerk— 95/95@croco/billing-polar— 62/62@croco/storage-r2— 37/37@croco/tasks-qstash— 16/16@croco/telemetry-sdk-node— 114/114secretSamplesa required non-empty tupleThe clean-worktree full test reached 208/224 tasks before the local Node 26 install failed to build
better-sqlite3@11.10.0; its 27 metering tests then failed solely because the native binding was absent/incompatible. No affected package uses that native module, and the exact affected suites above passed on the same head. GitHub CI remains the authoritative full-suite result.Review gates
Residual risk
Live provider calls remain intentionally env-gated and are not executed without credentials. The suite proves the default no-credential path and prevents network entry; it does not replace separately reviewed real-credential live-smoke evidence.
Fixes #1260
Summary by CodeRabbit