Repository navigation
fix: A1 支援已下載 MinIO IFC 直接檢核 - #316
Conversation
📝 WalkthroughWalkthroughThis PR adds a coordinator-side resolver (resolveDownloadedJobForRuleRun) and a new /api/governance/rule-runs/for-ifc-ready/:jobId proxy endpoint for triggering governance rule-runs on downloaded IFC-ready jobs without a review session, plus corresponding web-viewer client method, UI routing, and tests. ChangesIFC-ready governance rule-run proxy
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant User
participant A1GovernanceWorkbenchPage
participant governanceClient
participant GovernanceProxy
participant GovernanceService
User->>A1GovernanceWorkbenchPage: Click Pick (no session match)
A1GovernanceWorkbenchPage->>A1GovernanceWorkbenchPage: dispatch PICK_FILE ifc-ready://jobId
User->>A1GovernanceWorkbenchPage: Click Run Rule Validation
A1GovernanceWorkbenchPage->>governanceClient: createRuleRunForIfcReady(ifcReadyJobId, body)
governanceClient->>GovernanceProxy: POST /api/governance/rule-runs/for-ifc-ready/:jobId
GovernanceProxy->>GovernanceProxy: resolveDownloadedJobForRuleRun(jobId)
alt job downloaded and IFC path fresh
GovernanceProxy->>GovernanceService: POST /api/rule-runs (ifc_source_path, model_version_id)
GovernanceService-->>GovernanceProxy: rule_run_id, status
GovernanceProxy-->>governanceClient: 200 response
else missing or stale
GovernanceProxy-->>governanceClient: 404/409 with artifact_health
end
governanceClient-->>A1GovernanceWorkbenchPage: response
Possibly related PRs
🚥 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 |
PR Review Agent Summary
Blockers
Warnings
Validation Commands
Checks
Human Review Notes
|
![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: 63b55b22b1
ℹ️ 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".
| const { rule_run_id } = ifcReadyJobId | ||
| ? await governanceClient.createRuleRunForIfcReady(ifcReadyJobId, { ids_path: idsPath || undefined }) | ||
| : selectedSession | ||
| ? await governanceClient.createRuleRunForSession(selectedSession, { ids_path: idsPath || undefined }) |
There was a problem hiding this comment.
When a no-session MinIO job is locked (state.ifcPath is ifc-ready://...) and the operator later picks any review session from the panel, this new branch correctly dispatches the rule-run through for-ifc-ready but leaves selectedSession populated. The rest of doRun still uses selectedSession for mapping enrichment, and the deliverables section uses it for Review Room handoff, so failures from the no-session IFC can be enriched/opened against an unrelated session. Please gate or clear selectedSession for ifc-ready:// runs, or validate that the selected session matches the job before using it downstream.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Pull request overview
This PR closes a gap in the A1 governance workbench: when a MinIO source_ifc object has already been downloaded by the watcher but no review_session exists yet, A1 was previously blocked (the operator was told to go create a session via the IFC→USD schedule / Review Room). The change lets A1 run a CPU rule-run directly against the server-resolved local IFC path in that no-session state, while keeping the security boundary intact (the browser never sends a MinIO key or host path — it sends only the ifc_ready_job_id and the coordinator resolves the path server-side).
Changes:
- Adds coordinator route
POST /api/governance/rule-runs/for-ifc-ready/:jobId, backed by a sharedresolveDownloadedJobForRuleRunhelper (refactored out of the existing for-session resolver) and guarded byisSafeIfcReadyJobId.markSourceIfcUnavailablenow accepts anullsession. - A1 frontend locks
ifc-ready://<jobId>for downloaded/no-session jobs and calls the newcreateRuleRunForIfcReady; session-backed jobs keep the existingfor-sessionpath. Also fixes the reviewer-found P2: a lockedifc-ready://source is no longer silently rerouted through a manually selected session. - Adds coordinator, component, and Playwright E2E tests covering the no-session happy path, invalid job id (400), failed download (404), stale artifact (409), and the P2 non-rerouting guard.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
bim-review-coordinator/src/routes/governanceProxy.ts |
Adds the for-ifc-ready/:jobId route + shared forwardResolvedRuleRun; generalizes RuleRunSessionContext→RuleRunSourceContext (alias kept). |
bim-review-coordinator/src/app.ts |
Extracts resolveDownloadedJobForRuleRun, wires the ifc-ready resolver, and makes markSourceIfcUnavailable session-optional. |
web-viewer-sample/src/console/pages.tsx |
Routes no-session downloaded jobs through ifc-ready://, updates pick/run gating and resolution notes. |
web-viewer-sample/src/console/governanceClient.ts |
Adds createRuleRunForIfcReady client method. |
web-viewer-sample/src/console/A1ViewerEmbed.test.tsx |
Replaces the "stays blocked" test with ifc-ready-proxy + P2 non-rerouting coverage. |
web-viewer-sample/e2e/a1-minio-local-resolution.spec.ts |
Adds an E2E scenario for the no-session ifc-ready rule-run. |
bim-review-coordinator/tests/governance-rule-run-for-session.test.ts |
Adds coordinator tests for the new route (400/404/409/happy path). |
I reviewed the resolver refactor for behavioral parity on the for-session path (the added download_status === "downloaded" gate and reason-string change are covered by existing tests), verified the frontend staleness guard preserves the session semantics while adding the ifc_ready_job_id match, and confirmed the new route reuses isSafeIfcReadyJobId and the stable stale_session_artifact error contract. No blocking code defects were found.
This PR does, however, modify the coordinator's session→IFC resolution and proxy boundary — a security- and correctness-sensitive path — adds a new browser-visible route (the author correctly notes ifc_ready_job_id is not an auth token), and touches bim-review-coordinator/src/app.ts and src/routes/governanceProxy.ts, which are enumerated in the repo's documented backend-freeze list and carry the author's own high-risk impact analysis. These warrant human confirmation.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
PR Review Agent Summary
Blockers
Warnings
Validation Commands
Checks
Human Review Notes
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4f7e468069
ℹ️ 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".
| ? state.ifcPath.slice("ifc-ready://".length) | ||
| : ""; | ||
| const { rule_run_id } = ifcReadyJobId | ||
| ? await governanceClient.createRuleRunForIfcReady(ifcReadyJobId, { ids_path: idsPath || undefined }) |
There was a problem hiding this comment.
Fresh evidence in this revision is that the new ifcReadyJobId branch only protects the initial rule-run POST: after locking a no-session ifc-ready:// job, the operator can still manually select any review session, and the success path continues to use selectedSession for elementMappingForSession and Review Room handoff. In that scenario, failed GUIDs from the no-session IFC can be enriched or opened against an unrelated session even though the run itself correctly went through /for-ifc-ready/:jobId; please clear or ignore selectedSession for ifc-ready:// runs unless it is proven to belong to the job.
Useful? React with 👍 / 👎.
摘要
source_ifc已下載、但尚未建立review_session時被卡在轉檔排程/Review Room 的問題。POST /api/governance/rule-runs/for-ifc-ready/:jobId，由 server-side IFC-ready store 解析本機 IFC path，再轉送 governance-service；瀏覽器不送 MinIO key 或 host path。ifc-ready://<jobId>並呼叫createRuleRunForIfcReady；session-backed job 仍走既有/for-session/:sessionId。ifc-ready://後手動選 session，不得把 run 靜默改派到不相干的 review session。Formal Spec Evidence
風險與邊界
detect-changes --scope staged --repo AI-BIM-governance：high risk，7 files / 21 symbols / 6 affected flows；範圍符合 coordinator governance proxy + A1 console route 的預期。ifc_ready_job_id不是 auth token；若未來要放到 multi-tenant/public endpoint，需先加 coordinator user/tenant auth。驗證
cd bim-review-coordinator && npm run verify：53 files / 591 tests passed。cd web-viewer-sample && npm run verify：build passed；50 files / 558 tests passed；struct-log 10 tests passed。Vite chunk-size warning 與既有 React act / stream-controller 測試警告未造成失敗。cd web-viewer-sample && npm run test:session-first：passed。cd web-viewer-sample && npx playwright test e2e/a1-minio-local-resolution.spec.ts --reporter=listwith repo-local Vite server +E2E_DISABLE_WEBSERVER=1：2 passed。git diff --check、git diff --cached --check：passed。Frontend Verification
/#a1viaweb-viewer-sampleVite onhttp://127.0.0.1:5180/#a1a1-source-minio,a1-minio-select,a1-step-pick,a1-step-runsource_ifckey松風庵/root/main/u1/model.ifc; IFC-ready jobifcready_no_sessionwithdownload_status=downloaded,review_session_id=null,artifact_health.source_ifc_exists=truecoordinator ifc-ready proxy; run button posts/api/governance/rule-runs/for-ifc-ready/:jobId; scoreboarda1-rulerun-scoreboardbecomes visiblecd web-viewer-sample && E2E_DISABLE_WEBSERVER=1 npx playwright test e2e/a1-minio-local-resolution.spec.ts --reporter=listafter starting repo-local Vite on127.0.0.1:5180artifacts/e2e/a1-minio-local-resolution-no-session.png; also covers existing session path screenshotartifacts/e2e/a1-minio-local-resolution.png