Skip to content

fix(deploy): harden transactional test rebuild - #359

Merged
monkey1sai merged 8 commits into
mainfrom
codex/non-a4-branch-convergence
Jul 21, 2026
Merged

monkey1sai merged 8 commits into
mainfrom
codex/non-a4-branch-convergence

Conversation

@monkey1sai

@monkey1sai monkey1sai commented Jul 17, 2026 •

Copy link
Copy Markdown
Owner

Summary

  • 將非 A4 的 fix/rebuild-kit-generated-reparse 收斂成 transactional test-deploy rebuild:同 parent staging、path/reparse/lock guard、env snapshot/restore、失敗 stage 保留、previous checkout recovery path,並在 CI/PR review agent 同時跑 PowerShell 7 與 Windows PowerShell 5.1 專用 suite。
  • 修補交叉審查發現的 origin security gap:file:// origin 不再跳過 query/fragment credential guard;失敗診斷只保留 tool + git operation,不回顯後續 arguments。
  • 依 docs/plans 與現行 runtime 校正 minio-watch-key-structure:首輪未入 ledger 物件會自動進件、display hints 可落非權威 local shadow、P7 指向 canonical #/pipeline。
  • 修正 PR CI 發現的兩個 gate 問題:PowerShell source-reader fixture 改為 runner-independent deterministic seam,CI 以明確 -File scope 跑 PS7/PS5;functional config 只執行 contract 指定的 commit-bound conv-history producer,不再讓額外 spec 覆寫 tracked screenshots。
  • 修正 GitHub Windows runner 的 PowerShell 5.1 encoding 差異:Unicode fixture payload 仍保留,但 script source 改成 ASCII-safe code points,避免無 BOM UTF-8 被 CP1252 解碼成智慧引號而破壞 parser。
  • 關閉 PR inline review gaps:rooted local origin 以 ProcessStartInfo.ArgumentList/PS5 CommandLineToArgvW fallback 保留 exact argv,git clone -- 防 option injection;kit-manager-api 納入 cutover stop 集合;structured lifecycle log、non-Windows explicit unavailable warning、Hi-Fi runtime gate 與 head-SHA artifact upload 均有回歸測試。
  • 修正 GitHub Windows PowerShell 5.1 對無 BOM UTF-8 StructLog.psm1 的 parser failure:module 全檔 ASCII-safe,並新增 governance assertion,避免 em dash bytes 被 CP1252 解成 smart quote。
  • 合併最新 origin/main@d538894,採納 doc-first canon v2;需求裁決以 docs/plans v6 正本為準,code/tests 僅作 runtime 現況查證。
  • 明確排除 .worktrees/a4-semantic-search-model-qa/* 與相關 branch code;未 cherry-pick 任何 A4、dirty MinIO E2E、tasks-only checkbox 或會回退目前 main 的 stale branch。

Conflict review

Candidate Decision Evidence / reason
fix/rebuild-kit-generated-reparse absorbed, then security-amended 唯一有效非 A4 runtime delta;雙 PowerShell transaction suites passed
codex/fix/deploy-design-assets-build skipped 已被目前 main patch-equivalent/後續 PR 吸收
doc-first canon v2 (origin/main@d538894) merged cleanly docs/plans v6 正本與 archived governance spec 已進 branch;無 conflict、無 force-push
stale backup / superseded console branches skipped 直接吸收會回退目前 main;有用行為已存在
dirty minio-watch-key-structure worktree no cherry-pick; four-file hand-port only dirty E2E 鎖錯舊 IA;只保留符合 runtime 與 docs/plans 的 contract/comment 校正
rvt-ifc-usdc-lineage branch no commit to absorb branch 為 0 ahead;governed source-bundle intake、manifest revalidation、LineagePublicationOutbox runtime 仍是獨立 implementation gap
A4 worktree / branch excluded 使用者明確要求不吸收;沒有 branch/worktree code 進入本 PR

Change Classification

Item Result
Change lane S
Behavior contract changed yes
Requirement source docs/plans v6 — doc-first canonical product boundary;existing test-deploy contract 只提供治理/驗證程序

AI Coding Governance

Item Result
Linked issue User-authorized non-A4 branch convergence; no GitHub issue
Requirement source docs/plans v6 — docs/plans/docs-plans-README.md and canonical AI-BIM design document
CODEOWNERS / owner review repository owner review required by standard branch protection
GitNexus evidence detect_changes(scope=compare, base_ref=origin/main) = medium; 16 files, 3 affected MinIO watcher processes; no HIGH/CRITICAL
Browser E2E evidence not user-facing-complete in this PR; canonical #/pipeline real-MinIO P7 remains pending and is explicitly not claimed
Agent workflow changed? yes — PR review planner and CI route rebuild transaction changes to PS7 + PS5.1 file-scope suites; functional producer and Hi-Fi artifacts remain commit-bound; rollback is revert this PR (feature commits through c64a41c; 85722f1 only merges current main)
Required checks expected CI, Agent Governance, PR Review Agent, PowerShell static analysis, secret pattern scan

Frontend Verification

Machine-derived scope is gate_infrastructure_only, frontend_product=false, visual_required=false. No frontend behavior or design-fidelity completion is claimed. The MinIO contract records a future canonical #/pipeline P7 deployment E2E; this PR does not implement or substitute that evidence.

Deploy Path Verification

Item Result
Affects runtime / docker / Kit / viewer / ports / env? yes — test-deployment rebuild transaction and env preservation only; no service API or port change
Canonical deploy path updated? scripts/dev/rebuild-test-deploy.ps1 and scripts/lib/rebuild-test-deploy.ps1 updated; scripts/deploy.ps1 remains the canonical post-cutover build runner
New root script added? no
Deploy dry-run command .\scripts\deploy.ps1 -DryRun not run; dedicated transaction suite exercises dry-run and failure fixtures without touching the fixed deployment
Full deploy tested not run pre-merge because the canonical rebuild helper must fetch fresh origin/main; running it now would validate the old main, not this PR
Verify command pwsh -File .\scripts\tests\test-rebuild-test-deploy.ps1 and powershell.exe -File .\scripts\tests\test-rebuild-test-deploy.ps1
Frontend URL verified not applicable; no deployment or frontend completion claim
Evidence path console test results in PR validation and GitHub Actions

Validation

  • pwsh -NoProfile -NonInteractive -File .\scripts\tests\test-rebuild-test-deploy.ps1 — PASS.
  • powershell.exe -NoProfile -NonInteractive -ExecutionPolicy Bypass -File .\scripts\tests\test-rebuild-test-deploy.ps1 — PASS.
  • CP1252 decode + PowerShell parser simulation — RED with 47 parser errors before the ASCII-safe fixture fix, then PASS with 0 errors.
  • pwsh -NoProfile -NonInteractive -File .\scripts\tests\test-pr-review-agent.ps1 — PASS.
  • pwsh -NoProfile -NonInteractive -File .\scripts\tests\test-agent-governance-check.ps1 — PASS.
  • npm test -- tests/minio-watcher-loop.test.ts — PASS, 18/18.
  • bim-review-coordinator: npm run verify through the PR-bound review agent — PASS after lockfile-based npm ci.
  • PowerShell AST parser on all 6 changed .ps1 files — PASS, 0 parse errors.
  • npx --no-install openspec validate minio-watch-key-structure --strict — PASS against doc-first canon v2 main.
  • git diff --check origin/main — PASS.
  • pwsh -NoProfile -NonInteractive -ExecutionPolicy Bypass -File scripts/dev/check-pr-local-preflight.ps1 -PrNumber 359 — PASS against PR base d538894... / parser-fix head 8db52e7...; PR body evidence, PR-bound review agent and 13-screen/26-golden design reference gate passed. Review-agent status is disclosed warning/medium, not a failed gate.
  • pwsh -File .\scripts\tests\test-functional-runtime-result.ps1 — RED before the config fix, then PASS.
  • E2E_DISABLE_WEBSERVER=1 npx playwright test e2e/hifi-token-authority.spec.ts --config=playwright.config.ts — PASS, 4/4; true coordinator process teardown reaches port refusal and tracked PNGs were restored afterward.
  • npx playwright test --config=playwright.functional-runtime.config.ts with E2E_DISABLE_WEBSERVER=1 — PASS, 1/1 targeted conv-history test; no tracked artifact drift.
  • Nested Actions-equivalent invocations (pwsh outer shell launching PS7 / PS5.1 with -File) — PASS for both full rebuild transaction suites.
  • powershell.exe -NoProfile -NonInteractive -ExecutionPolicy Bypass -Command "Import-Module -Force .\scripts\lib\StructLog.psm1" — PASS after ASCII-safe parser fix; full Windows PowerShell 5.1 transaction suite and governance assertion also PASS.
  • verify-functional-runtime-result.ps1 -TargetCommit 8db52e7c1ac6a6d83d641ecc582b56c97f2eafef -AllowUntrackedArtifacts — PASS; evidence is bound to the pushed clean subject commit.
  • GitNexus detect_changes(scope=compare, base_ref=origin/main) — MEDIUM, 16 files / 3 affected MinIO tick processes, no HIGH/CRITICAL.
  • Two read-only reviewers: Terra xhigh APPROVE; Sol max 的「未綁 PR head」BLOCKER 已由 8db52e7 push + commit-bound local evidence 關閉。Sol 另列 partial service-stop recovery MEDIUM;coordinator 依 docs/plans 與現有 launcher authority 裁決為 residual risk,不臆造缺少 executable/args/env 的自動 restart callback。最終無未處理 BLOCKER/HIGH。

Known Risks

  • Fixed-path D:\Users\deploy\AI-bim-geo rebuild is intentionally deferred until this change is in fresh origin/main; pre-merge execution would not test the PR.
  • Local PSScriptAnalyzer is unavailable; the Windows CI powershell static analysis job remains the authoritative check.
  • An extra Windows PowerShell 5.1 invocation of test-pr-review-agent.ps1 hits the same pre-existing native-stderr behavior on both this branch and clean origin/main; the supported PS7 invocation passes and this PR does not broaden scope to that unrelated baseline gap.
  • Transaction hardening is incremental: not every future deployment mutation entrypoint automatically shares this lock/recovery protocol, and retained failed/previous paths still require operator lifecycle management.
  • Pre-cutover stop 若在部分 service 已停止後遇到 injected/unexpected stopper failure,現行流程會 abort 且不自動重啟先前已停服務。這是已知 residual MEDIUM;安全修正需要先建立具完整 executable/args/env 的 restart authority,不能只憑 service name 臆造。
  • Canonical #/pipeline real-MinIO browser E2E and full rvt-ifc-usdc-lineage runtime are not implemented here; full frontend, lineage, Docker, Kit, WebRTC, or full-system completion is not claimed.

Copilot AI review requested due to automatic review settings July 17, 2026 12:01
@coderabbitai

coderabbitai Bot commented Jul 17, 2026 •

Copy link
Copy Markdown
Contributor

Review Change Stack

📝 Walkthrough

Walkthrough

Changes

The PR hardens transactional rebuild deployment and its PowerShell validation, updates MinIO watcher baseline and contract documentation, and separates functional runtime selection from Hi-Fi token authority E2E execution and evidence collection.

Transactional rebuild safety

Layer / File(s) Summary
Rebuild validation and recovery flow
scripts/lib/rebuild-test-deploy.ps1, scripts/dev/rebuild-test-deploy.ps1, scripts/lib/StructLog.psm1
Rebuild paths, origins, native Git execution, staged cutovers, environment restoration, structured logging, and retained recovery paths receive stricter validation and reporting.
Rebuild transaction safety coverage
scripts/tests/test-rebuild-test-deploy.ps1
Tests cover reparse resolution, argument handling, credential rejection, diagnostic redaction, fail-closed snapshots, retained stages, cutover failures, and deterministic cleanup.
PowerShell and CI validation wiring
.github/workflows/ci.yml, scripts/lib/pr-review-agent.ps1, scripts/tests/test-agent-governance-check.ps1, scripts/tests/test-pr-review-agent.ps1
Changed-path classification schedules rebuild safety tests per available PowerShell host, handles unavailable platforms, and verifies CI execution under PowerShell 7 and Windows PowerShell 5.1.

MinIO watcher contract

Layer / File(s) Summary
Watcher behavior and intake metadata
openspec/changes/minio-watch-key-structure/..., bim-review-coordinator/src/services/minioWatcher.ts, bim-review-coordinator/tests/minio-watcher-loop.test.ts
The contract defines multi-layer key parsing, ledger-based startup behavior, parseable baseline counting, optional display hints, and updated browser E2E requirements.

Functional runtime validation

Layer / File(s) Summary
Runtime test and evidence wiring
web-viewer-sample/playwright.functional-runtime.config.ts, web-viewer-sample/e2e/hifi-token-authority.spec.ts, .github/workflows/ci.yml, scripts/tests/test-functional-runtime-result.ps1
Functional runtime selection is narrowed to conv-history.spec.ts; CI separately runs the Hi-Fi token authority test, uploads its evidence, and waits for coordinator ports to close during cleanup.

Estimated code review effort: 5 (Critical) | ~120 minutes

Sequence Diagram(s)

sequenceDiagram
  participant CI
  participant PRReviewAgent
  participant PowerShellHost
  participant TestRebuildTestDeploy
  participant RebuildFlow
  CI->>PRReviewAgent: detect rebuild/deploy path changes
  PRReviewAgent->>PowerShellHost: discover pwsh and powershell.exe
  PRReviewAgent->>TestRebuildTestDeploy: schedule one test per host
  PowerShellHost->>TestRebuildTestDeploy: execute safety suite
  TestRebuildTestDeploy->>RebuildFlow: validate paths, origins, staging, and cutover behavior
  RebuildFlow-->>TestRebuildTestDeploy: return results and retained recovery paths
Loading

Possibly related PRs

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately summarizes the main change: hardening the transactional test rebuild/deploy flow.
✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch codex/non-a4-branch-convergence

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR hardens the transactional test-deploy rebuild helper (scripts/lib/rebuild-test-deploy.ps1) into a safer, transactional cutover: it validates only the path components leading to the opaque live checkout (preserving Kit-generated reparse points below it), stages a same-parent sibling clone, moves the old live checkout aside as a retained "previous" checkout for recovery, restores env snapshots post-cutover, and never recursively deletes a failed stage. It also adds an origin-URL credential guard, redacts git-command diagnostics to tool + operation only, wires a dedicated PS7 + Windows PowerShell 5.1 rebuild-transaction test suite into CI and the PR-review planner, and applies documentation-only corrections to the minio-watch-key-structure OpenSpec change (first-round un-ledgered objects auto-intake, display hints as non-authoritative local shadow, canonical #/pipeline P7).

Changes:

  • Transactional rebuild hardening: component-only path safety, sibling-stage clone, retained previous/failed checkouts, post-cutover env restore, and a new PreviousPath result field surfaced by the dev entrypoint.
  • Security hardening: Assert-TestDeployOriginUrlSafe rejects inline/query/fragment credentials (including file:// and scp-like forms) without echoing canaries; git diagnostics keep only tool + operation + exit code + output.
  • Test/CI wiring: new rebuild_test_deploy CI classifier + PS7/PS5.1 suite runs, PR-review planner schedules the suite per available PowerShell host, plus PS5.1-compatible reparse-target resolution in tests; OpenSpec docs realigned to current runtime.

Reviewed changes

Copilot reviewed 11 out of 11 changed files in this pull request and generated no comments.

Show a summary per file
File Description
scripts/lib/rebuild-test-deploy.ps1 Core transactional rebuild: component-only path safety, origin credential guard, redacted git diagnostics, retained previous/failed stages, post-cutover env restore, PreviousPath output
scripts/dev/rebuild-test-deploy.ps1 Logs new previous_path from the rebuild result
scripts/lib/pr-review-agent.ps1 Adds Get-PrReviewPowerShellHosts and schedules the rebuild suite once per available PowerShell host
scripts/tests/test-rebuild-test-deploy.ps1 PS5.1 reparse-target shim + new credential/Kit-reparse/post-cutover/unsafe-stage transaction scenarios
scripts/tests/test-pr-review-agent.ps1 Test 8b asserts per-host rebuild suite scheduling
scripts/tests/test-agent-governance-check.ps1 Adds rebuild_test_deploy to output/gate arrays and asserts the PS7/PS5.1 CI steps
.github/workflows/ci.yml New rebuild_test_deploy classifier output + PS7/PS5.1 rebuild suite steps in powershell-static job
bim-review-coordinator/src/services/minioWatcher.ts Comment-only: display hints may persist as coordinator-local, non-authoritative metadata
openspec/changes/minio-watch-key-structure/* Doc corrections: ledger-based auto-intake, non-authoritative shadow hints, canonical #/pipeline P7, explicit lineage implementation gap

I reviewed the credential guard (all https/ssh/scp/file:// and query/fragment cases), the diagnostic redaction, the transactional cutover/recovery flow, env-restore reordering, CI/governance/PR-review consistency, and the spec/comment alignment against the coordinator's conversion-ledger persistence. I did not find a high-confidence, concrete defect. However, this is a Lane G, deployment-critical, security-sensitive change with a large volume of complex transactional filesystem logic and a documented pre-merge execution deferral (the fixed-path rebuild is intentionally not exercised until the change is in fresh origin/main), so it warrants human sign-off.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: d1bef90ad0

ℹ️ 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".

Comment on lines +534 to +536
if (-not $looksLikeUri) {
if ($isAbsoluteUri) {
throw 'current repo origin URL is invalid'

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Accept local Windows origin paths

When the caller repo was cloned from a local Windows mirror/path such as D:\Repos\AI-BIM-governance.git, [Uri]::TryCreate(..., Absolute) succeeds even though $looksLikeUri is false, so this branch throws before the staging clone. I checked git clone -h, which confirms local repositories are valid clone sources (--local ... clone from a local repository), and this helper cannot rebuild deployments in that supported setup despite no inline credentials. Treat absolute filesystem origins as safe local paths while still rejecting query/fragment/userinfo.

Useful? React with 👍 / 👎.

### Requirement: coordinator SHALL 以輪詢自動偵測 MinIO 新 IFC 並觸發既有 intake 鏈(O4 定案)

watcher SHALL 為 env opt-in(`MINIO_WATCH_ENABLED` 預設 false,未啟用時系統行為與無此功能完全一致)。啟用時 SHALL 以 `ListObjectsV2`(含 `IsTruncated`/`NextContinuationToken` 分頁)定時輪詢指定 bucket/prefix,僅對 key 以設定後綴(預設 `/model.ifc`)結尾者反應。首輪 SHALL 只登記 baseline 不觸發;後續輪的新 key 或同 key 新 etag SHALL 觸發對 loopback `POST /api/external/ifc-ready`(帶既有 webhook secret 與 presigned GET URL),由既有下載/sanitize/dispatch/callback 鏈處理。idempotency key SHALL 由 `bucket|key|etag` 確定性導出,重啟重掃同物件 SHALL 命中既有去重(idempotent_replay,不重複建 job)。輪詢間隔 SHALL 受下限保護且下限 SHALL 在 config overrides 合併後仍生效。單一物件的 intake 觸發失敗(presign / 網路 / 逾時 / HTTP error / 2xx 非 JSON)SHALL NOT 將該物件標記為已處理,watcher SHALL 於後續輪重試(漏抓自癒);key 層級不符(malformed)為確定性結果,SHALL 計數一次後跳過、不重試。
watcher SHALL 為 env opt-in(`MINIO_WATCH_ENABLED` 預設 false,未啟用時系統行為與無此功能完全一致)。啟用時 SHALL 以 `ListObjectsV2`(含 `IsTruncated`/`NextContinuationToken` 分頁)定時輪詢指定 bucket/prefix,僅對 key 以設定後綴(預設 `/model.ifc`)結尾者反應。每輪 SHALL 由 `bucket|key|etag` 確定性導出 idempotency key 並查持久 conversion ledger:ledger 無紀錄者(包含首輪既有未轉物件)SHALL 觸發對 loopback `POST /api/external/ifc-ready`(帶既有 webhook secret 與 presigned GET URL)並落帳;已有紀錄者 SHALL skip。`baseline_count` 只表示首輪 list 到的可解析規約檔數,不代表「首輪不觸發」。重啟重掃同 key 同 etag SHALL 算出同一 idempotency key、命中持久 ledger 而不重複建 job;新 key 或同 key 新 etag SHALL 產生新 idempotency key 並觸發。輪詢間隔 SHALL 受下限保護且下限 SHALL 在 config overrides 合併後仍生效。單一物件的 intake 觸發失敗(presign / 網路 / 逾時 / HTTP error / 2xx 非 JSON)SHALL NOT 落成功 ledger 紀錄,watcher SHALL 於後續輪重試(漏抓自癒);key 層級不符(malformed)為確定性結果,SHALL 計數一次後跳過、不重試。

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Repair the ledger on idempotent replay

This new contract says an unledgered object is triggered and then recorded in the persistent conversion ledger, but the current intake handler only calls conversionLedger.upsert(...) on the new-job path; if that upsert was swallowed by its catch, the next watcher restart posts the same idempotency key, hits findExisting(...), and returns from the replay branch before any ledger write. In that recovery case the object remains permanently absent from the ledger and is retried on every restart, so either the replay path needs to backfill the ledger or the spec should not promise ledger repair after triggering.

Useful? React with 👍 / 👎.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 3

🧹 Nitpick comments (1)
scripts/tests/test-rebuild-test-deploy.ps1 (1)

2846-2962: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick win

Assert process-environment restoration in both post-cutover modes.

These scenarios cover nonzero and thrown deploy outcomes but never verify that every variable changed by Push-TestDeployProcessEnv returns to its exact prior value, including unset values. Add post-invocation assertions to prevent process-wide state leakage between validations.

🤖 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 2846 - 2962, The
post-cutover scenarios do not verify restoration of process environment state
after deployment failures. Around the foreach cases invoking
Invoke-TestDeployRebuild, capture the exact pre-invocation values and
presence/absence of every variable managed by Push-TestDeployProcessEnv, then
add expectations for both nonzero and throw modes that each variable is restored
exactly, including previously unset variables.
🤖 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
`@openspec/changes/minio-watch-key-structure/specs/minio-watch-auto-intake/spec.md`:
- Around line 22-26: Update the baseline-count logic in minioWatcher.ts to count
only objects that successfully parse through deriveIntakeFromKey, excluding
malformed keys while preserving suffix filtering and ledger-based first-run
processing. Assign baseline_count from the parseable-object collection rather
than the raw objects.length.

In `@scripts/lib/pr-review-agent.ps1`:
- Around line 721-728: Gate the rebuild-test-deploy plans in
Get-PrReviewPowerShellHosts handling within
scripts/lib/pr-review-agent.ps1:721-728 on $IsWindows or the equivalent Win32NT
platform check, and report platform unavailability when appropriate instead of
scheduling them on non-Windows systems. Update the corresponding expectations
and host selection in scripts/tests/test-pr-review-agent.ps1:548-568 to apply
the same platform rule, preserving the existing Windows plan count and using
zero plans or hosts on other platforms.

In `@scripts/lib/rebuild-test-deploy.ps1`:
- Around line 853-855: Replace the bare Write-Host retained-checkout diagnostic
with the shared structured logger from scripts/lib/StructLog.psm1 in
scripts/lib/rebuild-test-deploy.ps1#L853-L855, preserving the recovery path
value. Apply the same logging change to the previous_path output in
scripts/dev/rebuild-test-deploy.ps1#L21-L23; both sites must use the structured
logging path and no longer call Write-Host.

---

Nitpick comments:
In `@scripts/tests/test-rebuild-test-deploy.ps1`:
- Around line 2846-2962: The post-cutover scenarios do not verify restoration of
process environment state after deployment failures. Around the foreach cases
invoking Invoke-TestDeployRebuild, capture the exact pre-invocation values and
presence/absence of every variable managed by Push-TestDeployProcessEnv, then
add expectations for both nonzero and throw modes that each variable is restored
exactly, including previously unset variables.
🪄 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: 55fe077c-49b5-4192-a040-fac8d1cab1ae

📥 Commits

Reviewing files that changed from the base of the PR and between 0d24fb6 and d1bef90.

📒 Files selected for processing (11)
  • .github/workflows/ci.yml
  • bim-review-coordinator/src/services/minioWatcher.ts
  • openspec/changes/minio-watch-key-structure/proposal.md
  • openspec/changes/minio-watch-key-structure/specs/minio-watch-auto-intake/spec.md
  • openspec/changes/minio-watch-key-structure/tasks.md
  • scripts/dev/rebuild-test-deploy.ps1
  • scripts/lib/pr-review-agent.ps1
  • scripts/lib/rebuild-test-deploy.ps1
  • scripts/tests/test-agent-governance-check.ps1
  • scripts/tests/test-pr-review-agent.ps1
  • scripts/tests/test-rebuild-test-deploy.ps1

Comment thread scripts/lib/pr-review-agent.ps1
Comment on lines +853 to +855
if (-not [string]::IsNullOrWhiteSpace($previousPath)) {
Write-Host "[rebuild-test-deploy] retained previous checkout for recovery: $previousPath"
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win

Use the shared structured logger for retained-checkout output.

Both sites introduce bare Write-Host diagnostics instead of the required logging module.

  • scripts/lib/rebuild-test-deploy.ps1#L853-L855: emit the retained recovery path through scripts/lib/StructLog.psm1.
  • scripts/dev/rebuild-test-deploy.ps1#L21-L23: emit the previous_path result through the same structured logging path.

As per coding guidelines, use scripts/lib/StructLog.psm1 for structured logging output and do not use bare Write-Host.

📍 Affects 2 files
  • scripts/lib/rebuild-test-deploy.ps1#L853-L855 (this comment)
  • scripts/dev/rebuild-test-deploy.ps1#L21-L23
🤖 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` around lines 853 - 855, Replace the bare
Write-Host retained-checkout diagnostic with the shared structured logger from
scripts/lib/StructLog.psm1 in scripts/lib/rebuild-test-deploy.ps1#L853-L855,
preserving the recovery path value. Apply the same logging change to the
previous_path output in scripts/dev/rebuild-test-deploy.ps1#L21-L23; both sites
must use the structured logging path and no longer call Write-Host.

Source: Coding guidelines

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 84ab5b5d3f

ℹ️ 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".

Comment thread scripts/lib/rebuild-test-deploy.ps1 Outdated
Comment on lines 811 to 813
Assert-TestDeployPathComponentsSafety -Path $deployZoneRunDir | Out-Null
$serviceStopFailures = New-Object 'System.Collections.Generic.List[string]'
foreach ($serviceName in @('bim-streaming-server', 'bim-streaming-conversion-service', 'governance-service')) {

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge Stop kit-manager before swapping the deployment

When the existing test deployment was started by scripts/deploy.ps1, Phase 4c-2 also launches kit-manager-api from services\kit-manager-api with its pidfile under the deploy root. This new transactional path stops only the three legacy services before renaming the live checkout, so a running kit-manager process can keep the old tree/port 8010 alive; on Windows that can block the directory move, or the subsequent deploy can health-check the stale process while the new checkout has no matching pidfile. Include kit-manager-api in this pre-cutover stop pass.

Useful? React with 👍 / 👎.


export default defineConfig(baseConfig, {
testMatch: ["conv-history.spec.ts", "hifi-token-authority.spec.ts"],
testMatch: ["conv-history.spec.ts"],

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Keep the hifi runtime slice in CI

For frontend changes, the functional-runtime-conv workflow invokes Playwright through this config (.github/workflows/ci.yml runs npx playwright test --config=playwright.functional-runtime.config.ts), and after this change it no longer includes hifi-token-authority.spec.ts; I checked repo-wide rg and there is no other workflow/config invoking that spec. This removes the only runtime coverage for the unified home token authority and legacy #/demo-control/#/kit reachability, so those regressions can merge unnoticed. If the conv evidence binding must stay single-producer, split the hifi slice into a separate config/job instead of dropping it.

Useful? React with 👍 / 👎.

@monkey1sai monkey1sai left a comment

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Codex PR review

Verdict

No remaining BLOCKER or HIGH-severity findings. This is a review comment rather than an approval because the reviewer is operating on the PR author's behalf and must not self-approve.

Findings

None.

Three-layer adversarial review

  1. Requirements authority: docs/plans/* was used as the sole target authority; runtime code/tests were used only to distinguish implemented behavior from explicit gaps. Terra requirements review: CLEAR.
  2. Security and failure semantics: file-origin query/fragment bypass and git-argument diagnostic disclosure were found, fixed, and regression-tested. Sol security review: CLEAR.
  3. Coordinator arbitration: A4 worktree/branch code remained excluded; stale/regressive branches and dirty E2E/tasks-only deltas were rejected. Final arbitration: no unresolved BLOCKER/HIGH.

Evidence

  • GitNexus compare: MEDIUM, 13 changed files, 3 affected MinIO watcher processes; no HIGH/CRITICAL result.
  • Latest local PR preflight: PASS for 143cfb37093b359148f42085f4a2e9937f4536ac.
  • PowerShell 7 and Windows PowerShell 5.1 rebuild transaction suites: PASS.
  • CP1252 decode/parser regression: RED with 47 errors before the ASCII-safe Unicode fixture fix, PASS afterward.
  • OpenSpec strict validation: 61 passed, 0 failed.
  • Commit-bound conv-history functional runtime: PASS, 1/1; verifier bound to 143cfb3.
  • GitHub Actions: all applicable checks PASS, including PowerShell static analysis, functional runtime, coordinator, viewer, contracts, governance, PR review agent, and secret scan.
  • Final diff hygiene: clean tracked worktree and git diff --check origin/main...HEAD PASS.

Residual risks / scope not claimed

  • The fixed deployment checkout rebuild is intentionally deferred until this PR is merged into fresh origin/main; running it pre-merge would validate the old main.
  • Canonical #/pipeline real-MinIO browser E2E and the rvt-ifc-usdc-lineage runtime remain explicit implementation gaps.
  • This PR does not claim full frontend, lineage, Docker, Kit, WebRTC, or full-system completion.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 143cfb3709

ℹ️ 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".

### Requirement: coordinator SHALL 以輪詢自動偵測 MinIO 新 IFC 並觸發既有 intake 鏈(O4 定案)

watcher SHALL 為 env opt-in(`MINIO_WATCH_ENABLED` 預設 false,未啟用時系統行為與無此功能完全一致)。啟用時 SHALL 以 `ListObjectsV2`(含 `IsTruncated`/`NextContinuationToken` 分頁)定時輪詢指定 bucket/prefix,僅對 key 以設定後綴(預設 `/model.ifc`)結尾者反應。首輪 SHALL 只登記 baseline 不觸發;後續輪的新 key 或同 key 新 etag SHALL 觸發對 loopback `POST /api/external/ifc-ready`(帶既有 webhook secret 與 presigned GET URL),由既有下載/sanitize/dispatch/callback 鏈處理。idempotency key SHALL 由 `bucket|key|etag` 確定性導出,重啟重掃同物件 SHALL 命中既有去重(idempotent_replay,不重複建 job)。輪詢間隔 SHALL 受下限保護且下限 SHALL 在 config overrides 合併後仍生效。單一物件的 intake 觸發失敗(presign / 網路 / 逾時 / HTTP error / 2xx 非 JSON)SHALL NOT 將該物件標記為已處理,watcher SHALL 於後續輪重試(漏抓自癒);key 層級不符(malformed)為確定性結果,SHALL 計數一次後跳過、不重試。
watcher SHALL 為 env opt-in(`MINIO_WATCH_ENABLED` 預設 false,未啟用時系統行為與無此功能完全一致)。啟用時 SHALL 以 `ListObjectsV2`(含 `IsTruncated`/`NextContinuationToken` 分頁)定時輪詢指定 bucket/prefix,僅對 key 以設定後綴(預設 `/model.ifc`)結尾者反應。每輪 SHALL 由 `bucket|key|etag` 確定性導出 idempotency key 並查持久 conversion ledger:ledger 無紀錄者(包含首輪既有未轉物件)SHALL 觸發對 loopback `POST /api/external/ifc-ready`(帶既有 webhook secret 與 presigned GET URL)並落帳;已有紀錄者 SHALL skip。`baseline_count` 只表示首輪 list 到的可解析規約檔數,不代表「首輪不觸發」。重啟重掃同 key 同 etag SHALL 算出同一 idempotency key、命中持久 ledger 而不重複建 job;新 key 或同 key 新 etag SHALL 產生新 idempotency key 並觸發。輪詢間隔 SHALL 受下限保護且下限 SHALL 在 config overrides 合併後仍生效。單一物件的 intake 觸發失敗(presign / 網路 / 逾時 / HTTP error / 2xx 非 JSON)SHALL NOT 落成功 ledger 紀錄,watcher SHALL 於後續輪重試(漏抓自癒);key 層級不符(malformed)為確定性結果,SHALL 計數一次後跳過、不重試。

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Align restart spec with ledger skip

When the watcher restarts after a successful intake whose idempotency key is already in the persistent ledger, this new requirement says the object is skipped via the ledger, and the implementation does exactly that before calling triggerIntake (minioWatcher.ts skips at the isLedgered(idkey) branch). The unchanged restart scenario below still requires the intake endpoint to return idempotent_replay, which can no longer happen for the normal ledgered restart path, so tests or reviewers following the spec will expect a replay POST that the runtime intentionally suppresses.

Useful? React with 👍 / 👎.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 85722f1ce0

ℹ️ 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".

Comment on lines +577 to +580
if ($isAbsoluteUri) {
throw 'current repo origin URL is invalid'
}
return $candidate

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Reject credential markers in relative origins

When the caller repo has a non-rooted origin such as repo.git?access_token=... or repo.git#..., $looksLikeUri, IsPathRooted, and the scp-like regex are all false, so this branch returns it as safe. I checked git clone -h (git clone [<options>] [--] <repo> [<dir>]) and reproduced that a missing relative <repo> is echoed in the fatal stderr; because Invoke-TestDeployGitCommand appends stderr to the thrown diagnostic, the token can still leak despite the new argument redaction. Apply the same ?/# guard before accepting relative origins.

Useful? React with 👍 / 👎.

Comment thread .github/workflows/ci.yml
working-directory: web-viewer-sample
env:
E2E_DISABLE_WEBSERVER: '1'
run: npx playwright test e2e/hifi-token-authority.spec.ts --config=playwright.config.ts

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Revalidate hifi screenshot drift after the split

When hifi-token-authority.spec.ts passes but changes its tracked screenshots under artifacts/e2e/hifi-token-authority/ (for example a token or route visual drift not covered by assertions), this standalone step runs after the only verify-functional-runtime-result.ps1 call; that validator is the piece that enforces git diff --quiet $TargetCommit --, so hifi artifact drift no longer fails CI. I checked the hifi PNGs are tracked with git ls-tree -r f7bbcf6 artifacts/e2e/hifi-token-authority; add a post-hifi clean-tree/binding check or run this slice before the existing validator.

Useful? React with 👍 / 👎.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants