fix: harden test-deployment rebuild and verification profiles - #402
Conversation
📝 WalkthroughWalkthroughThe PR adds Windows PowerShell child-environment handling to test deployment execution and introduces Developer and Deployment profiles with plan-only support for PowerShell and POSIX aggregate verification scripts. ChangesVerification and deployment tooling
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant verify-all.ps1
participant rebuild-test-deploy.ps1
participant RepositoryArtifacts
participant HTTPHealthChecks
verify-all.ps1->>rebuild-test-deploy.ps1: Load pruning contract
verify-all.ps1->>RepositoryArtifacts: Validate preserved production files
verify-all.ps1->>HTTPHealthChecks: Run required health checks
HTTPHealthChecks-->>verify-all.ps1: Return status
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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: 2
🧹 Nitpick comments (2)
scripts/verify-all.ps1 (2)
7-7: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
$Profileshadows the PowerShell automatic variable$PROFILE.
$Profileis a built-in automatic variable (path to the current PowerShell profile). Reusing it as a parameter name works here since the script never reads the profile path, but it's an easy-to-miss gotcha for future maintainers. Consider$VerifyProfileto avoid the collision.🤖 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/verify-all.ps1` at line 7, Rename the script parameter $Profile to $VerifyProfile in the parameter declaration and update every reference to it within the script, preserving the existing ValidateSet values and default.
154-162: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valuePlan output uses bare
Write-Host.The new
[PLAN]/[EXECUTE]/[OMIT]output is emitted with bareWrite-Host. Coding guidelines require structured logging output inscripts/**/*.ps1to go throughscripts/lib/StructLog.psm1rather than bareWrite-Host. Since the rest of this file already predates that convention, consider routing at least the new plan output through StructLog for consistency.As per coding guidelines: "Use
scripts/lib/StructLog.psm1for structured logging output; do not replace with bareWrite-Hostcalls".🤖 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/verify-all.ps1` around lines 154 - 162, Route the new plan-reporting messages in the $publishInventory block through the structured logging helpers from scripts/lib/StructLog.psm1 instead of bare Write-Host calls. Update the [PLAN], [EXECUTE], and [OMIT] output while preserving their current message content and warning severity for omitted targets.Source: Coding guidelines
🤖 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/tests/test-verify-all.ps1`:
- Around line 76-78: Replace the token-matching assertions in the POSIX verifier
checks with an actual invocation of scripts/verify-all.sh using --profile
Deployment --plan-only against the pruned fixture. Capture and validate its
parsed Deployment inventory against the PowerShell plan output, including
omitted targets, and assert that missing artifacts produce the expected failure.
In `@scripts/verify-all.sh`:
- Around line 45-62: Update the profile dispatch in verify-all.sh so Deployment
never falls through into the Developer target-building and execution block when
PLAN_ONLY is not set. Gate that block on PROFILE=Developer or implement the
corresponding Deployment checks, and reject StreamingOnly, TsOnly, and PyOnly
under Deployment to match the behavior of verify-all.ps1.
---
Nitpick comments:
In `@scripts/verify-all.ps1`:
- Line 7: Rename the script parameter $Profile to $VerifyProfile in the
parameter declaration and update every reference to it within the script,
preserving the existing ValidateSet values and default.
- Around line 154-162: Route the new plan-reporting messages in the
$publishInventory block through the structured logging helpers from
scripts/lib/StructLog.psm1 instead of bare Write-Host calls. Update the [PLAN],
[EXECUTE], and [OMIT] output while preserving their current message content and
warning severity for omitted targets.
🪄 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 Plus
Run ID: bf33bffa-087d-41cd-9787-f01314ae4a74
📒 Files selected for processing (5)
scripts/lib/rebuild-test-deploy.ps1scripts/tests/test-rebuild-test-deploy.ps1scripts/tests/test-verify-all.ps1scripts/verify-all.ps1scripts/verify-all.sh
| $verifyShell = Get-Content -LiteralPath (Join-Path $repoRoot 'scripts\verify-all.sh') -Raw | ||
| Assert-True ($verifyShell -match '--profile') 'POSIX verifier mirror accepts an explicit deployment profile' | ||
| Assert-True ($verifyShell -match '--plan-only') 'POSIX verifier mirror publishes the same profile inventory without executing it' |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== locate files =="
git ls-files | rg '(^|/)test-verify-all\.ps1$|verify-all\.sh$|verify-all\.ps1$|AGENTS\.md$' || true
echo "== script sizes =="
for f in scripts/tests/test-verify-all.ps1 scripts/verify-all.sh scripts/verify-all.ps1 scripts/lib/StructLog.psm1; do
[ -f "$f" ] && wc -l "$f"
done
echo "== inspect test file around assertions =="
if [ -f scripts/tests/test-verify-all.ps1 ]; then
cat -n scripts/tests/test-verify-all.ps1 | sed -n '1,140p'
fi
echo "== inspect shell verifier outline/content =="
if [ -f scripts/verify-all.sh ]; then
wc -l scripts/verify-all.sh
cat -n scripts/verify-all.sh | sed -n '1,260p'
fi
echo "== inspect powershell verifier outline/content around relevant sections =="
if [ -f scripts/verify-all.ps1 ]; then
wc -l scripts/verify-all.ps1
cat -n scripts/verify-all.ps1 | sed -n '1,260p'
fi
echo "== search for test-verify-all usages and fixtures =="
rg -n "test-verify-all|verify-all|--profile|--plan-only|deployment profile|profile inventory|Plan output|missing|artifact" scripts AGENTS.md .github 2>/dev/null || trueRepository: monkey1sai/AI-BIM-governance
Length of output: 50385
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== git status/stat =="
git status --short
git diff --stat
echo "== shell syntax parse if available =="
bash -n scripts/verify-all.sh 2>&1 || true
pwsh -Command "& {if (Test-Path scripts/verify-all.ps1) { $f='scripts/verify-all.ps1'; $tokens=$errors=$null; [System.Management.Automation.Parser]::ParseInput((Get-Content -Raw $f), [ref]$tokens, [ref]$errors); $errors?.Write(); $tokens } else { 'verify-all.ps1 missing' } }" 2>&1 || true
echo "== deterministic flag-token semantic probe =="
python3 - <<'PY'
from pathlib import Path
import re
p=Path('scripts/tests/test-verify-all.ps1')
if p.exists():
text=p.read_text()
print("has profile token assertion:", bool(re.search(r'--profile', text)))
print("has plan-only token assertion:", bool(re.search(r'--plan-only', text)))
print("calls verify shell exec:", bool(re.search(r'Invoke-Expression|Start-Process|powershell|pwsh|verify-all', text)))
print("contains parity output assertions:", bool(re.search(r'parity|output|assert.*profile|profile inventory', text, re.I)))
PYRepository: monkey1sai/AI-BIM-governance
Length of output: 261
Exercise the Bash profile contract instead of scanning tokens.
These assertions pass if the flags only occur as strings; they do not verify Bash parsing, the Deployment inventory generated from the pruned fixture, omitted targets, or missing-artifact failure. Run scripts/verify-all.sh with --profile Deployment --plan-only and assert parity with the PowerShell plan output.
🤖 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-verify-all.ps1` around lines 76 - 78, Replace the
token-matching assertions in the POSIX verifier checks with an actual invocation
of scripts/verify-all.sh using --profile Deployment --plan-only against the
pruned fixture. Capture and validate its parsed Deployment inventory against the
PowerShell plan output, including omitted targets, and assert that missing
artifacts produce the expected failure.
Source: Coding guidelines
There was a problem hiding this comment.
Pull request overview
This PR hardens the workspace's test-deployment verification path per issue #400. It normalizes the Windows PowerShell child PSModulePath used by the canonical rebuild (so Get-FileHash resolves under a PowerShell 7-first parent), and splits the aggregate verifier into explicit Developer and Deployment profiles so a pruned deployment checkout is checked against the artifacts/runtime it actually preserves rather than authoring-only inputs. Regression tests cover both the new profiles and the child-environment contract.
Changes:
- Add a normalized Windows PowerShell child environment (
Get-TestDeployWindowsPowerShellChildEnvironment) and a pruning-contract accessor (Get-TestDeployPruningContract) inscripts/lib/rebuild-test-deploy.ps1, threaded into both the injected and real deploy launch paths. - Add
Developer/Deploymentprofiles,-PlanOnly, required-artifact + health-endpoint checks, and an omission inventory toscripts/verify-all.ps1, with a partial mirror inscripts/verify-all.sh. - Add regression coverage in
scripts/tests/test-verify-all.ps1and extendscripts/tests/test-rebuild-test-deploy.ps1for the child-environment contract and closure scope capture.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| scripts/verify-all.ps1 | Adds Developer/Deployment profiles, plan-only inventory, required-artifact and health-endpoint targets, and Required/Action target semantics. |
| scripts/verify-all.sh | POSIX mirror of --profile/--plan-only; only emits a Deployment plan and diverges from the .ps1 for non-plan-only and Developer plan-only runs. |
| scripts/lib/rebuild-test-deploy.ps1 | Adds pruning-contract accessor and Windows PowerShell child PSModulePath normalization; passes normalized env to injected and cmd.exe launch paths. |
| scripts/tests/test-verify-all.ps1 | New fixture asserting Developer full contract, Deployment omission inventory, and missing-artifact failure (not yet wired into CI). |
| scripts/tests/test-rebuild-test-deploy.ps1 | Captures helper functions for closure scope, adds -Environment to injected runners, and validates the normalized child module path. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| case "$PROFILE" in | ||
| Developer) ;; | ||
| Deployment) | ||
| echo "[PLAN] profile=deployment" | ||
| echo "[EXECUTE] deployment required artifacts" | ||
| echo "[EXECUTE] coordinator health" | ||
| echo "[EXECUTE] governance health" | ||
| echo "[EXECUTE] conversion health" | ||
| echo "[EXECUTE] kit manager health" | ||
| echo "[EXECUTE] viewer endpoint" | ||
| echo "[OMIT] tests (contracts+fakes)" | ||
| echo "[OMIT] bim-review-coordinator (full verify)" | ||
| echo "[OMIT] web-viewer-sample (full verify)" | ||
| echo "[OMIT] bim-streaming-server stage-loading contract" | ||
| if [ "$PLAN_ONLY" -eq 1 ]; then exit 0; fi | ||
| ;; | ||
| *) echo "unknown profile: $PROFILE" >&2; exit 2 ;; | ||
| esac |
| # scripts/tests/test-verify-all.ps1 | ||
| # Verifies the canonical aggregate verifier's developer and pruned-deployment profiles. |
| [switch] $PyOnly, | ||
| [switch] $ContinueOnError | ||
| [switch] $ContinueOnError, | ||
| [ValidateSet('Developer', 'Deployment')][string] $Profile = 'Developer', |
![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: 527507f3e9
ℹ️ 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".
| Deployment) | ||
| echo "[PLAN] profile=deployment" | ||
| echo "[EXECUTE] deployment required artifacts" | ||
| echo "[EXECUTE] coordinator health" | ||
| echo "[EXECUTE] governance health" | ||
| echo "[EXECUTE] conversion health" | ||
| echo "[EXECUTE] kit manager health" | ||
| echo "[EXECUTE] viewer endpoint" | ||
| echo "[OMIT] tests (contracts+fakes)" | ||
| echo "[OMIT] bim-review-coordinator (full verify)" | ||
| echo "[OMIT] web-viewer-sample (full verify)" | ||
| echo "[OMIT] bim-streaming-server stage-loading contract" | ||
| if [ "$PLAN_ONLY" -eq 1 ]; then exit 0; fi | ||
| ;; |
There was a problem hiding this comment.
When scripts/verify-all.sh --profile Deployment is run without --plan-only, this branch only prints the deployment inventory and then falls through to the normal Developer target construction below, so pruned deployment checkouts run/skip authoring suites instead of checking required artifacts and live health endpoints; it also accepts filters that the PowerShell verifier rejects. Please make the .sh Deployment profile execute the same deployment targets or delegate to the PowerShell verifier so the mirror does not report the wrong result.
AGENTS.md reference: scripts/AGENTS.md:L34-L34
Useful? React with 👍 / 👎.
| $response = Invoke-WebRequest @requestParameters | ||
| if ($response.StatusCode -ne 200) { | ||
| throw "unexpected HTTP status $($response.StatusCode)" | ||
| } |
There was a problem hiding this comment.
For Deployment profile health checks, this helper treats any HTTP 200 as success, but the conversion authority intentionally returns HTTP 200 even when converter preflight is unavailable with status="degraded" and ifc_to_usdc_conversion=false (see bim-streaming-server/tests/test_host_native_conversion_service.py:2518-2544). In that scenario conversion health is marked [OK] although IFC→USDC conversion is unusable, so parse the JSON status/claims for endpoints that expose them before passing the deployment verifier.
Useful? React with 👍 / 👎.
| fi | ||
|
|
||
| case "$PROFILE" in | ||
| Developer) ;; |
There was a problem hiding this comment.
When scripts/verify-all.sh --plan-only is used with the default Developer profile, this branch does nothing and the script proceeds to construct and execute the real pytest/npm targets instead of printing the inventory and exiting like verify-all.ps1 -PlanOnly. That turns a dry plan request into a full verification run in dependency-missing or pruned environments; please emit the Developer plan and exit before target execution.
AGENTS.md reference: scripts/AGENTS.md:L34-L34
Useful? React with 👍 / 👎.
| $Targets += New-DeploymentHealthTarget -Name 'coordinator health' -Uri 'http://127.0.0.1:8004/health' | ||
| $Targets += New-DeploymentHealthTarget -Name 'governance health' -Uri 'http://127.0.0.1:49102/health' | ||
| $Targets += New-DeploymentHealthTarget -Name 'conversion health' -Uri 'http://127.0.0.1:49101/health' | ||
| $Targets += New-DeploymentHealthTarget -Name 'kit manager health' -Uri 'http://127.0.0.1:8010/health' | ||
| $Targets += New-DeploymentHealthTarget -Name 'viewer endpoint' -Uri 'http://127.0.0.1:5173/' |
There was a problem hiding this comment.
When deploy.ps1 is run with supported non-default topology such as COORDINATOR_PORT, VIEWER_PORT, or -GovernancePort, the Deployment verifier still probes the hard-coded defaults here. In that environment it can either fail a healthy deployment or pass against stale services left on the default ports, so these URIs need to be derived from the same deploy/env settings used to launch the stack.
Useful? React with 👍 / 👎.
| $pruningContract = Get-TestDeployPruningContract | ||
| $requiredArtifacts = @('scripts\deploy.ps1') + @($pruningContract.PreservedProductionFiles) | ||
| Test-DeploymentRequiredArtifacts -Root $RepoRoot -RequiredRelativePaths $requiredArtifacts |
There was a problem hiding this comment.
This loads the pruning contract but only uses PreservedProductionFiles, so RootToolingDirNames is never checked. If the cleanup regresses and leaves .codex, .claude, openspec, patches, or other authoring/tooling roots in the deployment checkout, -Profile Deployment can still pass as long as the required files and health endpoints exist; add an absence/allowlist check for the pruned roots so a clean deployment result actually proves the contract.
AGENTS.md reference: AGENTS.md:L48-L48
Useful? React with 👍 / 👎.
monkey1sai
changed the title
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 64881de068
ℹ️ 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: powershell.exe -NoProfile -NonInteractive -ExecutionPolicy Bypass -File scripts/tests/test-rebuild-test-deploy.ps1 | ||
|
|
||
| - name: Run aggregate verifier profile tests | ||
| if: needs.changes.outputs.rebuild_test_deploy == 'true' |
There was a problem hiding this comment.
With this condition, the new aggregate verifier tests only run when rebuild_test_deploy is true, but the classifier pattern above does not include scripts/verify-all.ps1 or scripts/verify-all.sh—only deploy/rebuild helpers and test files. A PR that changes the verifier itself will therefore get only static analysis and skip the regression tests added here; gate this step on a classifier that also matches the verifier scripts.
Useful? React with 👍 / 👎.
| . (Join-Path $PSScriptRoot 'lib\rebuild-test-deploy.ps1') | ||
| $pruningContract = Get-TestDeployPruningContract | ||
| $requiredArtifacts = @('scripts\deploy.ps1') + @($pruningContract.PreservedProductionFiles) | ||
| Test-DeploymentRequiredArtifacts -Root $RepoRoot -RequiredRelativePaths $requiredArtifacts |
There was a problem hiding this comment.
When the Deployment profile is pointed at an incomplete checkout, this eager artifact check throws while the target list is still being constructed, so -ContinueOnError cannot collect the remaining health failures and -PlanOnly cannot print the intended inventory. The same artifact validation is already registered as the first target action below, where failures are summarized and honor the normal control flow, so remove this pre-loop execution.
Useful? React with 👍 / 👎.
Scope
This PR delivers the canonical rebuild and deployment-profile verification slices of #400. The authority-owned mounted A4 capability and post-merge canonical deployment evidence remain follow-up acceptance gates.
Summary
Change Classification
AI Coding Governance
Deploy Path Verification
./scripts/deploy.ps1 -DryRun./scripts/verify-all.ps1 -Profile Deployment -PlanOnlyVerification
scripts/tests/test-verify-all.ps1git diff --checkbash scripts/verify-all.sh --profile Deployment --plan-onlytest-rebuild-test-deploy.ps1remains blocked by an existing injected callback scope error (Assert-Trueunavailable inside the test callback).origin/mainmerge and authority-owned secrets/capability inputs.Frontend Verification
scripts/tests/test-verify-all.ps1Part of #400