fix: keep publish-profile CLI tests deterministic - #1659
Merged
Conversation
📝 WalkthroughWalkthroughCLI 테스트 계약과 패키지 바이너리 검증 범위를 확장했습니다. 릴리스 증거 실행기는 stdout/stderr 로그 보존, 출력 버퍼 제한, 파일 쓰기 오류 처리, 인터럽트 종료 및 종료 코드 보고를 지원합니다. 관련 동작 검증 테스트도 추가했습니다. Changes검증 계약 및 패키지 범위
릴리스 증거 실행기
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant CLI
participant interruptActiveCommand
participant CommandRunner
participant runChecks
participant ArtifactDirectory
CLI->>interruptActiveCommand: 인터럽트 신호 전달
interruptActiveCommand->>CommandRunner: 활성 명령 종료 요청
runChecks->>CommandRunner: 명령 실행 및 출력 수집
CommandRunner->>ArtifactDirectory: stdout/stderr 로그 기록
runChecks->>ArtifactDirectory: 실패·타임아웃·중단 로그 보존
runChecks->>CLI: 보고서와 종료 코드 반환
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 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-08-01T12:40:55.716Z · Commit: 6cac67b |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@scripts/release-spine-evidence.mts`:
- Around line 477-478: Update openCommandOutput and the
child.stdout/child.stderr data handlers in defaultCommandRunner to catch
filesystem errors from mkdirSync, openSync, and writeSync, converting them into
the command’s handled failure path rather than uncaught exceptions. Ensure a
single runner() failure is contained by runReleaseSpineEvidence so remaining
checks and final report generation still execute, including during
interruption/finalization.
- Around line 668-700: Update persistFailedCommandOutput so each stdout.log and
stderr.log is overwritten with the current result output regardless of whether
the file already exists. Preserve the existing artifact-reference construction,
including fresh: true, only after replacing any stale log contents.
🪄 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: cbe1cad4-2796-4c0a-9a2d-fff86f597f32
📒 Files selected for processing (5)
.changeset/steady-cli-unit-tests.mdpackages/cli/package.jsonpackages/cli/src/tests/CliTestContract.spec.tsscripts/release-spine-evidence.mtsscripts/tests/release-spine-evidence.spec.ts
kang-heewon
force-pushed
the
fix/1652-cli-publish-tests
branch
from
July 31, 2026 16:24
883f753 to
85e004c
Compare
Member
Author
|
@coderabbitai resolve |
✅ Action performedComments resolved and changes approved. |
kang-heewon
force-pushed
the
fix/1652-cli-publish-tests
branch
2 times, most recently
from
July 31, 2026 17:21
546ad8d to
3f9138a
Compare
kang-heewon
force-pushed
the
fix/1652-cli-publish-tests
branch
from
July 31, 2026 18:26
3f9138a to
6beb2f0
Compare
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
scripts/release-spine-evidence.mts (1)
1446-1481: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
interruptSignal이null로 좁혀져 종료 코드 분기가 타입 오류가 됩니다.
interruptSignal은main본문에서 초기값null만 가집니다. 중첩 클로저interrupt는 후속 호출로서 변수 타입 변경을 가능하게 만드는 것처럼 보여, 그後の 참조가 재확정됩니다. 따라서interruptSignal === "SIGINT"비교가 TS2367을 발생시킬 수 있습니다.가변 홀더 객체를 사용하면 이 좁혀짐을 피할 수 있습니다.
🐛 홀더 객체 적용 제안
- let interruptSignal: NodeJS.Signals | null = null; - const interrupt = (signal: string) => { - interruptSignal = signal as NodeJS.Signals; - interruptActiveCommand(interruptSignal); + const interruptState: { signal: NodeJS.Signals | null } = { signal: null }; + const interrupt = (signal: string) => { + interruptState.signal = signal as NodeJS.Signals; + interruptActiveCommand(interruptState.signal); console.error(`release-spine-evidence: interrupted by ${signal}`); };- getInterruptSignal: () => interruptSignal, + getInterruptSignal: () => interruptState.signal,exit( - interruptSignal - ? interruptSignal === "SIGINT" + interruptState.signal + ? interruptState.signal === "SIGINT" ? 130 : 143 : report.status === "passed" ? 0 : 1, );🤖 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/release-spine-evidence.mts` around lines 1446 - 1481, Update the interrupt state used by main and the nested interrupt callback to store the signal in a mutable holder object rather than a directly captured interruptSignal variable, preventing TypeScript from narrowing it to null. Read the holder’s current signal when passing getInterruptSignal and when selecting the final exit code, preserving the existing SIGINT, SIGTERM, and report-status behavior.
🤖 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 `@scripts/release-spine-evidence.mts`:
- Around line 800-824: Mark fallback artifacts as truncated when fileComplete is
false so consumers can distinguish them from complete streamed logs. Update the
artifact result construction near wroteCurrentOutput to include a clear
truncation indicator in the label or copyError, while preserving normal metadata
for fileComplete === true and existing write-error reporting.
- Around line 786-799: Update the array passed to map in
persistFailedCommandOutput with an explicit tuple type so each entry preserves
string, string, string, and boolean-or-undefined positions; use that typed
collection for the existing destructuring and file-writing logic so join,
writeFileSync, and EvidenceArtifactReference assignments receive their expected
types.
- Around line 828-836: Update discardCommandOutput so cleanup failures from
removing stdout.log or stderr.log are absorbed instead of escaping to
runReleaseSpineEvidence; use a force-enabled removal operation or catch and
ignore unlinkSync errors while preserving cleanup attempts for both files.
In `@scripts/tests/release-spine-evidence.spec.ts`:
- Around line 753-768: Update the commandOutputWriter callback’s stdout
detection to use the chunk-independent payload distinction described by the
test, such as identifying stdout via its character content rather than requiring
the full “stdout diagnostics” phrase in one chunk. Keep the ENOSPC throw and
partial stdout artifact assertion unchanged.
In `@scripts/tests/verification-manifest.spec.ts`:
- Around line 289-293: In the filter callback of the verification-manifest test,
remove the duplicated type-predicate declarations and retain exactly one valid
`pkg is ...` predicate before the arrow body. Preserve the existing private,
bin, and name checks and the narrowed package type.
In `@scripts/verification-manifest.mts`:
- Around line 417-420: affectsPackageBins()의 경로 판정에
packages/openapi-spec/src/**, packages/migration-runner/src/**,
packages/rpc-codegen/src/**를 추가해 해당 변경이 packageBinBuildArguments에 필터를 포함하도록
수정하세요. 각 패키지 변경 시 build 및 package-bins-smoke 명령이 적용되는지 검증하는 계약 테스트를 추가하고, 기존 경로의
동작은 유지하세요.
---
Outside diff comments:
In `@scripts/release-spine-evidence.mts`:
- Around line 1446-1481: Update the interrupt state used by main and the nested
interrupt callback to store the signal in a mutable holder object rather than a
directly captured interruptSignal variable, preventing TypeScript from narrowing
it to null. Read the holder’s current signal when passing getInterruptSignal and
when selecting the final exit code, preserving the existing SIGINT, SIGTERM, and
report-status behavior.
🪄 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: 91cd8a57-7c76-4ead-8ef3-2b1e54539d39
📒 Files selected for processing (6)
.changeset/327f9106.mdpackages/cli/src/tests/CliTestContract.spec.tsscripts/release-spine-evidence.mtsscripts/tests/release-spine-evidence.spec.tsscripts/tests/verification-manifest.spec.tsscripts/verification-manifest.mts
Member
Author
|
Addressed the actionable review findings in fc4b24e.
Validation for fc4b24e: 53 focused tests passed; full release gates passed 43 files / 1,120 tests; Oxlint and repository Oxfmt checks passed; pre-push passed 234/234 test tasks and 233/233 typecheck tasks. Current-head GitHub CI is running. |
Member
Author
|
@coderabbitai resolve |
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 `@scripts/release-spine-evidence.mts`:
- Around line 835-850: Update discardCommandOutput so it does not attempt to
delete the second log after the first deletion fails, and preferably validate
that both stdout.log and stderr.log are removable before deleting either file.
Preserve complete command evidence when cleanup cannot be completed, including
the failure path that invokes persistFailedCommandOutput.
🪄 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: 033c940a-534e-47b4-9c16-c6098e44023f
📒 Files selected for processing (7)
scripts/package-bin-smoke.mtsscripts/package-manifest-contracts.mjsscripts/release-spine-evidence.mtsscripts/tests/package-manifest-contracts.spec.tsscripts/tests/release-spine-evidence.spec.tsscripts/tests/verification-manifest.spec.tsscripts/verification-manifest.mts
✅ 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
@croco/clitest task deterministic by preventing the shell from expanding its integration-test exclusion into unintended Vitest include paths.cli-e2epublish gate.Verification
NPM_CONFIG_PROVENANCE=true pnpm verify:publish -- --base origin/trunk --head HEAD --allow-pending-release-metadata— 40/47 passed, 7 not applicable, 0 failed.pnpm --filter @croco/cli test— 30 files and 274 tests passed; integration files were not selected.pnpm exec vitest run scripts/tests/release-spine-evidence.spec.ts scripts/tests/verification-manifest.spec.ts --config vitest.config.ts— 39 tests passed.git diff --checkpassed.Review gates
Correctness
cli-e2epasses independently.API, security, and release
@croco/cli, and failure logs use the existing access-controlled verification artifact upload.Maintainability
Residual risk
Failure artifacts contain the output emitted by repository verification commands. They are retained only for unsuccessful or interrupted checks and inherit the existing GitHub Actions artifact visibility.
Fixes #1652
Summary by CodeRabbit
개선 사항
검증