fix(deploy): 封存設計資產建置來源 - #356
Conversation
以 SHA-256 manifest、跨 adapter lock 與 claim/swap rollback 保護正式部署及 Docker build 的設計資產。 補齊真實 git clean、來源存取錯誤、雙重失敗與 PowerShell/Node rollback regression。 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
📝 WalkthroughWalkthroughAdds manifest-based design asset synchronization for Node.js and PowerShell, including locking, path safety, atomic publication, rollback, Docker integration, rebuild coordination, and extensive failure-path tests. ChangesDesign asset synchronization
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
Suggested reviewers: Sequence Diagram(s)sequenceDiagram
participant DeployScript as deploy.ps1
participant AssetSync as Sync-DeploymentDesignAssets
participant DesignAssets as public/design-assets
participant Docker as docker compose
DeployScript->>AssetSync: synchronize and validate design assets
AssetSync->>DesignAssets: publish manifest and PNG inventory
DesignAssets-->>AssetSync: return validated mode and count
AssetSync-->>DeployScript: report synchronization result
DeployScript->>Docker: build images after successful staging
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 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 |
![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 (3)
scripts/tests/test-edge-console-deploy-contract.ps1 (1)
65-79: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert synchronization ordering, not only textual presence.
These checks still pass if
Sync-DeploymentDesignAssetsis moved afterdocker compose upordocker compose build. Compare invocation offsets—or inspect the PowerShell AST—to enforce the pre-build/pre-start contract.🤖 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-edge-console-deploy-contract.ps1` around lines 65 - 79, Strengthen the assertions in the test block that validates $startWebPlane, $startRuntimeManager, and $deploy so they verify Sync-DeploymentDesignAssets occurs before the relevant docker compose build/up invocation, not merely that both strings exist. Compare the matched command offsets or inspect the PowerShell AST, while preserving the existing import and compose configuration checks.scripts/start-runtime-manager-docker.ps1 (1)
13-13: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRoute the newly added operational events through
StructLog.psm1.
scripts/start-runtime-manager-docker.ps1#L13-L13: replace the synchronizationWrite-Hostevent.scripts/start-web-plane-docker.ps1#L106-L106: replace the synchronizationWrite-Hostevent.scripts/lib/rebuild-test-deploy.ps1#L737-L853: structure the new cleanup, retry, and restoration diagnostics.As per coding guidelines, “Use
scripts/lib/StructLog.psm1for structured logging output; do not replace with bareWrite-Hostcalls.” <coding_guidelines>🤖 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/start-runtime-manager-docker.ps1` at line 13, Replace the synchronization Write-Host events in scripts/start-runtime-manager-docker.ps1 lines 13-13 and scripts/start-web-plane-docker.ps1 lines 106-106 with the appropriate structured logging calls from StructLog.psm1. Update the cleanup, retry, and restoration diagnostics in scripts/lib/rebuild-test-deploy.ps1 lines 737-853 to use StructLog.psm1 as well, preserving their existing messages and event details.Source: Coding guidelines
web-viewer-sample/scripts/sync-design-assets.mjs (1)
109-127: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMove cleanup error propagation out of
finally.Line 124 triggers Biome’s
noUnsafeFinally. Capture the action result/error, release the lock afterward, then propagate the primary or cleanup error with the same precedence.Proposed refactor
function withDesignAssetLock(action) { const lock = enterDesignAssetLock(); let primaryError; + let result; try { - return action(); + result = action(); } catch (error) { primaryError = error; - throw error; - } finally { - try { - lock.release(); - } catch (error) { - if (primaryError) { - console.error(`[sync-design-assets] lock cleanup failed after primary error: ${error instanceof Error ? error.message : String(error)}`); - } else { - throw error; - } + } + + let cleanupError; + try { + lock.release(); + } catch (error) { + if (primaryError) { + console.error(`[sync-design-assets] lock cleanup failed after primary error: ${error instanceof Error ? error.message : String(error)}`); + } else { + cleanupError = error; } } + + if (primaryError) throw primaryError; + if (cleanupError) throw cleanupError; + return result; }🤖 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 `@web-viewer-sample/scripts/sync-design-assets.mjs` around lines 109 - 127, Refactor withDesignAssetLock so the action result or primary error is captured before cleanup, then call lock.release() outside the finally block. Preserve error precedence: rethrow the action error when both operations fail, otherwise propagate the cleanup error, and return the action result on success without throwing from finally.Source: Linters/SAST tools
🤖 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/design-assets.ps1`:
- Around line 107-113: Update the path boundary and lock-path comparisons in the
affected validation logic to select StringComparison.OrdinalIgnoreCase on
Windows and StringComparison.Ordinal on other platforms. Apply this
platform-aware comparison to each relevant StartsWith and Equals call,
preserving the existing boundary-check behavior.
---
Nitpick comments:
In `@scripts/start-runtime-manager-docker.ps1`:
- Line 13: Replace the synchronization Write-Host events in
scripts/start-runtime-manager-docker.ps1 lines 13-13 and
scripts/start-web-plane-docker.ps1 lines 106-106 with the appropriate structured
logging calls from StructLog.psm1. Update the cleanup, retry, and restoration
diagnostics in scripts/lib/rebuild-test-deploy.ps1 lines 737-853 to use
StructLog.psm1 as well, preserving their existing messages and event details.
In `@scripts/tests/test-edge-console-deploy-contract.ps1`:
- Around line 65-79: Strengthen the assertions in the test block that validates
$startWebPlane, $startRuntimeManager, and $deploy so they verify
Sync-DeploymentDesignAssets occurs before the relevant docker compose build/up
invocation, not merely that both strings exist. Compare the matched command
offsets or inspect the PowerShell AST, while preserving the existing import and
compose configuration checks.
In `@web-viewer-sample/scripts/sync-design-assets.mjs`:
- Around line 109-127: Refactor withDesignAssetLock so the action result or
primary error is captured before cleanup, then call lock.release() outside the
finally block. Preserve error precedence: rethrow the action error when both
operations fail, otherwise propagate the cleanup error, and return the action
result on success without throwing from finally.
🪄 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: 60a8bb6a-9b3a-4d18-8af3-97e0e4256cbb
📒 Files selected for processing (12)
.dockerignoreinfra/docker/web-viewer-sample.Dockerfilescripts/deploy.ps1scripts/lib/design-assets.ps1scripts/lib/rebuild-test-deploy.ps1scripts/start-runtime-manager-docker.ps1scripts/start-web-plane-docker.ps1scripts/tests/test-edge-console-deploy-contract.ps1scripts/tests/test-rebuild-test-deploy.ps1web-viewer-sample/.gitignoreweb-viewer-sample/scripts/sync-design-assets.mjsweb-viewer-sample/scripts/test-sync-design-assets-publication.mjs
| $fullPath = [System.IO.Path]::GetFullPath($Path) | ||
| $boundary = [System.IO.Path]::GetFullPath($BoundaryRoot).TrimEnd([char[]]@('\', '/')) | ||
| $boundaryPrefix = $boundary + [System.IO.Path]::DirectorySeparatorChar | ||
| if ( | ||
| $fullPath -ine $boundary -and | ||
| -not $fullPath.StartsWith($boundaryPrefix, [System.StringComparison]::OrdinalIgnoreCase) | ||
| ) { |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
rg -n -C2 'OrdinalIgnoreCase|StartsWith\(|\.Equals\(' scripts/lib/design-assets.ps1
rg -n -C2 'case-sensitive|BoundaryRoot|escaped repository root' scripts/tests \
--glob '*.ps1'Repository: monkey1sai/AI-BIM-governance
Length of output: 10729
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- scripts/lib/design-assets.ps1 (relevant ranges) ---'
sed -n '1,240p' scripts/lib/design-assets.ps1
echo
echo '--- scripts/lib/design-assets.ps1 (later range) ---'
sed -n '240,560p' scripts/lib/design-assets.ps1
echo
echo '--- call sites / tests mentioning lock or boundary root ---'
rg -n -C2 'Enter-DeploymentDesignAssetLock|Remove-ReplaceableDeploymentDesignAssetDirectory|Assert-DeploymentDesignAssetsPrestaged|Assert-NoDeploymentDesignAssetBackupResidue|BoundaryRoot' scripts/tests scripts/lib/design-assets.ps1Repository: monkey1sai/AI-BIM-governance
Length of output: 47919
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
p = Path('scripts/lib/design-assets.ps1')
text = p.read_text()
for marker in ['OrdinalIgnoreCase', 'Ordinal', '$IsWindows', 'BoundaryRoot', 'caller-owned']:
print(f'=== {marker} ===')
for i, line in enumerate(text.splitlines(), 1):
if marker in line:
print(f'{i}: {line}')
PYRepository: monkey1sai/AI-BIM-governance
Length of output: 5840
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
import os
# Check the platform semantics that matter for the review claim.
samples = [
('/work/repo', '/work/Repo'),
('/tmp/foo', '/tmp/Foo'),
]
for base, candidate in samples:
print(base, candidate, 'casefold-equal', base.casefold() == candidate.casefold(), 'exact-equal', base == candidate)
PYRepository: monkey1sai/AI-BIM-governance
Length of output: 282
Use platform-aware path comparisons. OrdinalIgnoreCase on the boundary and lock-path checks can treat /work/Repo/... as inside /work/repo on case-sensitive systems. Use OrdinalIgnoreCase on Windows and Ordinal elsewhere for the affected StartsWith/Equals 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/design-assets.ps1` around lines 107 - 113, Update the path
boundary and lock-path comparisons in the affected validation logic to select
StringComparison.OrdinalIgnoreCase on Windows and StringComparison.Ordinal on
other platforms. Apply this platform-aware comparison to each relevant
StartsWith and Equals call, preserving the existing boundary-check behavior.
Source: Coding guidelines
There was a problem hiding this comment.
Pull request overview
This PR seals the design-asset build source so that official deploys and Docker image builds no longer depend on shipping repo-root docs/plans/ into image contexts. It replaces the previous idempotent copy in sync-design-assets.mjs with a hardened transaction: when source dirs are absent, only a SHA-256 .deployment-manifest.json (schema design-assets/v1) sealed under web-viewer-sample/public/design-assets/ is accepted. The same behavior is mirrored in a new PowerShell library so the deploy/rebuild/start adapters coordinate through one cross-adapter .design-assets-sync.lock with fail-closed claim/swap/rollback recovery.
Changes:
- Rewrite the Node prebuild sync to add reparse-point guards, an exclusive lock, atomic staged publish with backup/rollback, and manifest validation; add a Node fault-injection test for publication rollback.
- Add
scripts/lib/design-assets.ps1(PowerShell mirror) and wire it intodeploy.ps1,rebuild-test-deploy.ps1, and the two docker-start adapters, centralizing thegit cleanexclusions for design-asset transaction paths. - Have
web-viewer-sample.Dockerfilevalidate the prestaged manifest during build; exclude staging/backup/lock/test artifacts via.dockerignoreand.gitignore; extend the PowerShell test suites.
Reviewed changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
web-viewer-sample/scripts/sync-design-assets.mjs |
Rewrites sync into a locked, manifest-sealed atomic publish with rollback and reparse-point guards. |
web-viewer-sample/scripts/test-sync-design-assets-publication.mjs |
New deterministic Node fault-injection tests for stage/validation/rollback/cleanup failure paths. |
scripts/lib/design-assets.ps1 |
New PowerShell mirror of the sealing/lock/publish/validate logic used by all deploy adapters. |
scripts/lib/rebuild-test-deploy.ps1 |
Wraps existing-checkout rebuild in one caller-owned asset lock; centralizes clean args; stages assets before docs removal. |
scripts/deploy.ps1 |
Sources the shared lib and stages/validates design assets before the docker build (Phase 2 fail-closed). |
scripts/start-web-plane-docker.ps1 / scripts/start-runtime-manager-docker.ps1 |
Import the lib and stage design assets before compose up. |
infra/docker/web-viewer-sample.Dockerfile |
Runs sync-design-assets.mjs to validate the prestaged manifest as USER node. |
.dockerignore / web-viewer-sample/.gitignore |
Exclude transient stage/backup/lock directories, the lock file, and the publication test. |
scripts/tests/test-rebuild-test-deploy.ps1 / scripts/tests/test-edge-console-deploy-contract.ps1 |
Extend coverage for lock contention, rollback, source-probe, clean, and contract assertions. |
The change is internally consistent and well-tested, but it modifies the canonical production deploy path (D:\Users\deploy\AI-bim-geo) with destructive git clean -fdx and cross-process filesystem transactions, adds a large security-sensitive PowerShell library that the repo's own tooling (GitNexus) cannot index (disclosed in the PR), and introduces a new hard build dependency (image builds now fail closed without a prestaged manifest). These factors place it beyond safe automated approval.
💡 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: 2636edd696
ℹ️ 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".
| RUN npm install | ||
|
|
||
| COPY --chown=node:node web-viewer-sample/ /workspace/web-viewer-sample/ | ||
| RUN node scripts/sync-design-assets.mjs |
There was a problem hiding this comment.
When the viewer image is built directly from a clean checkout, e.g. docker compose -f compose.runtime-manager.yml build viewer or CI invoking this Dockerfile without first running the PowerShell deploy adapter, this RUN executes in an image that only copied web-viewer-sample/. The root docs/plans/... sources are not present in the image, and web-viewer-sample/public/design-assets/ is gitignored so it is absent in a clean clone; the sync script therefore falls back to validating a prestaged .deployment-manifest.json and aborts the Docker build. Please either stage the assets inside the Dockerfile without shipping the docs in the final image, or make this build path invoke the same staging step before validation.
Useful? React with 👍 / 👎.
Summary
AI Coding Governance
Deploy Path Verification
Validation
node --check: PASSscripts/tests/test-edge-console-deploy-contract.ps1: PASSscripts/tests/test-rebuild-test-deploy.ps1: PASSnpm run test:session-firstandnpm run build: PASSKnown Risks
D:\Users\deploy\AI-bim-georebuild remains a post-merge gate.Start-HostNativeKitManagerdiagnostics after its parent test reports PASS/exit 0.Summary by CodeRabbit
New Features
Bug Fixes
Tests