修復測試部署清理失敗時的 .env 還原 - #243
Conversation
📝 WalkthroughWalkthrough
ChangesEnv Restore on Clean Failure
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
scripts/tests/test-rebuild-test-deploy.ps1 (1)
251-253: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAlso assert the clean failure exit code to harden this regression test.
The test checks message text, but not the expected non-zero code from the mocked
clean -fdxfailure. Assertingexit code 42here would better lock the error contract.Proposed test tweak
Assert-True (-not [string]::IsNullOrWhiteSpace($cleanFailureMessage)) 'clean failure is surfaced' + Assert-True ($cleanFailureMessage -match 'exit code 42') 'clean failure includes exit code' Assert-True ($cleanFailureMessage -match 'locked governance log') 'clean failure includes command output' Assert-True (-not $cleanFailureDeployWasCalled) 'clean failure stops before deploy'🤖 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/tests/test-rebuild-test-deploy.ps1` around lines 251 - 253, The test validates the clean failure message and deployment behavior but does not assert the expected exit code from the mocked clean failure. Add an Assert-True statement after the existing assertions to verify that the clean failure exit code equals 42, ensuring the error contract is properly locked. Store and check a variable representing the exit code from the clean command failure (similar to how $cleanFailureMessage and $cleanFailureDeployWasCalled are currently validated) to complete the regression test coverage.
🤖 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/lib/rebuild-test-deploy.ps1`:
- Line 272: Replace the bare Write-Host call in the deployment restoration
logging at line 272 with a structured logging call from
scripts/lib/StructLog.psm1. Instead of using Write-Host directly, use the
appropriate structured logging function from the StructLog module to log the
message about restored deployment environment files after failed cleanup. This
ensures consistent output formatting and enables downstream parsing as per the
scripts coding guidelines.
---
Nitpick comments:
In `@scripts/tests/test-rebuild-test-deploy.ps1`:
- Around line 251-253: The test validates the clean failure message and
deployment behavior but does not assert the expected exit code from the mocked
clean failure. Add an Assert-True statement after the existing assertions to
verify that the clean failure exit code equals 42, ensuring the error contract
is properly locked. Store and check a variable representing the exit code from
the clean command failure (similar to how $cleanFailureMessage and
$cleanFailureDeployWasCalled are currently validated) to complete the regression
test coverage.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 8876b86c-1a5a-4e46-94f7-903b7cbeb37a
📒 Files selected for processing (2)
scripts/lib/rebuild-test-deploy.ps1scripts/tests/test-rebuild-test-deploy.ps1
| try { | ||
| $restoredAfterFailure = @(Restore-TestDeployEnvSnapshot -DeploymentPath $deployRoot -Snapshot $envSnapshot) | ||
| if ($restoredAfterFailure.Count -gt 0) { | ||
| Write-Host "[rebuild-test-deploy] restored deployment env files after failed cleanup count=$($restoredAfterFailure.Count): $($restoredAfterFailure -join ', ')" |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Use structured logging instead of bare Write-Host in the new failure-restore path.
Line 272 introduces a new bare Write-Host; this should go through the structured logger used by scripts for consistent output and downstream parsing.
As per coding guidelines: "scripts/**/*.ps1: Use scripts/lib/StructLog.psm1 for structured logging output; do not replace with bare Write-Host calls".
🤖 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/rebuild-test-deploy.ps1` at line 272, Replace the bare Write-Host
call in the deployment restoration logging at line 272 with a structured logging
call from scripts/lib/StructLog.psm1. Instead of using Write-Host directly, use
the appropriate structured logging function from the StructLog module to log the
message about restored deployment environment files after failed cleanup. This
ensures consistent output formatting and enables downstream parsing as per the
scripts coding guidelines.
Source: Coding guidelines
There was a problem hiding this comment.
Pull request overview
This PR fixes a secret-loss bug in the test-deploy rebuild helper: previously, if git clean -fdx failed after deleting deployment .env files (e.g., due to a locked governance log), the preserved .env snapshot was never restored. The change wraps the fetch/reset/clean/restore sequence in a try/catch so that the env snapshot is restored even on failure, before the error is re-thrown to abort the deploy. This protects deployment-only secrets (notably MinIO credentials in .env.web-plane.host-kit) that exist only in the deployment checkout.
Changes:
- Wrapped the git fetch/reset/clean and env-restore steps in
try/catchinInvoke-TestDeployRebuild, restoring the env snapshot on failure and surfacing a combined error if the restore itself fails. - Added a regression test simulating a failed
git clean -fdx(after the host-kit env was deleted) that asserts the MinIO credentials are restored, the failure is surfaced, and deploy is not invoked.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
scripts/lib/rebuild-test-deploy.ps1 |
Adds try/catch around fetch/reset/clean/restore so preserved env files are restored on cleanup failure, then re-throws to abort before deploy. |
scripts/tests/test-rebuild-test-deploy.ps1 |
Adds a regression test verifying env (MinIO credential) restoration after a simulated failed git clean -fdx, and that deploy is not reached. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![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: 98cd595181
ℹ️ 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".
| try { | ||
| $restoredAfterFailure = @(Restore-TestDeployEnvSnapshot -DeploymentPath $deployRoot -Snapshot $envSnapshot) | ||
| if ($restoredAfterFailure.Count -gt 0) { | ||
| Write-Host "[rebuild-test-deploy] restored deployment env files after failed cleanup count=$($restoredAfterFailure.Count): $($restoredAfterFailure -join ', ')" |
There was a problem hiding this comment.
When git clean -fdx fails after deleting preserved env files, this new recovery message is emitted only via naked Write-Host, even though scripts/AGENTS.md explicitly requires scripts to use scripts/lib/StructLog.psm1 and not bare Write-Host. That makes this failure-recovery event invisible to the structured-log pipeline used for deploy diagnostics; please emit it through the structured logger instead.
Useful? React with 👍 / 👎.
| } catch { | ||
| $cleanupError = $_ | ||
| try { | ||
| $restoredAfterFailure = @(Restore-TestDeployEnvSnapshot -DeploymentPath $deployRoot -Snapshot $envSnapshot) |
There was a problem hiding this comment.
In a partial cleanup failure where one preserved env file remains locked or unwritable while another has already been deleted, this single restore call aborts on the first WriteAllBytes error inside Restore-TestDeployEnvSnapshot. Because snapshots are restored in .env, coordinator .env, then host-kit order, a locked root .env prevents later deleted credentials such as .env.web-plane.host-kit from being restored, leaving the deployment checkout without the private MinIO settings this recovery path is meant to preserve.
Useful? React with 👍 / 👎.
| $restoredEnvFiles = @(Restore-TestDeployEnvSnapshot -DeploymentPath $deployRoot -Snapshot $envSnapshot) | ||
| $restoredEnvFiles = @() | ||
| try { | ||
| Invoke-TestDeployGitCommand -Tool 'git' -Arguments @('fetch', 'origin', $mainRefSpec) -WorkingDirectory $deployRoot -CommandRunner $CommandRunner | Out-Null |
There was a problem hiding this comment.
When the explicit git fetch fails because of network or auth, no reset or clean has run yet, but placing it inside this cleanup-recovery try means the catch still rewrites preserved .env snapshots. That changes the documented fail-fast fetch path and can even mask the real fetch blocker if a preserved env file is locked or unwritable; only failures after a destructive reset/clean step should trigger snapshot restoration.
Useful? React with 👍 / 👎.
摘要
rebuild-test-deploy在git clean -fdx失敗時沒有還原.envsnapshot 的問題。git clean已刪除.env.web-plane.host-kit後因 locked log 失敗，確認 MinIO credential 檔會被還原。驗證
powershell -NoProfile -ExecutionPolicy Bypass -File scripts\tests\test-rebuild-test-deploy.ps1git diff --check -- scripts/lib/rebuild-test-deploy.ps1 scripts/tests/test-rebuild-test-deploy.ps1gitnexus detect_changes(scope=staged)：2 files, low risk, no indexed symbols affectedDeploy Path Verification
D:\Users\deploy\AI-bim-geoscripts\dev\rebuild-test-deploy.ps1 -Build.env,bim-review-coordinator\.env,.env.web-plane.host-kitrestored before deploy on normal path;.env.web-plane.host-kitrestored after simulated failed clean200, viewer200, governance200, conversion200/api/external/minio-watch/statusreturnedenabled=true,last_error=null,poll_count=18,seen_count=3已知限制
.env。triggered_total=0表示 watcher 尚未看到新的*/model.ifc物件；既有物件已作為 baseline。Summary by CodeRabbit
Bug Fixes
Tests