Skip to content

歸檔 canonical batch 並新增 enumeration 優化切片 - #35

Merged
monkey1sai merged 3 commits into
mainfrom
codex/openspec/optimize-worker-source-entity-enumeration
May 13, 2026
Merged

monkey1sai merged 3 commits into
mainfrom
codex/openspec/optimize-worker-source-entity-enumeration

Conversation

@monkey1sai

@monkey1sai monkey1sai commented May 12, 2026 •

Copy link
Copy Markdown
Owner

變更摘要

本 PR 將 worker-canonical-storage-batch-baseline 依使用者指示先歸檔,並建立新的 OpenSpec change optimize-worker-source-entity-enumeration,專門處理 canonical 89MB IFC fixture 在 source_entity_enumeration 階段 timeout 的 blocker。

修改原因

worker-canonical-storage-batch-baseline 已補入 batch timeout/status semantics、phase timing、worker UI handoff 與相關 evidence requirement,但 canonical --limit 1 --timeout-seconds 600 仍卡在第一個 89MB fixture 的 source_entity_enumeration,full 13-file batch 與 visual preview 仍未通過。為避免已歸檔規格被誤認為 runtime passed,本 PR 將 blocked 狀態寫入 roadmap,並把下一步收斂成更小的 enumeration optimization change。

主要變更

  • 將 openspec/changes/worker-canonical-storage-batch-baseline/ 移至 openspec/changes/archive/2026-05-12-worker-canonical-storage-batch-baseline/。
  • 更新 openspec/specs/worker-artifact-pipeline/spec.md,納入 canonical storage batch status、timeout diagnostics、single-fixture gate、phase timing 與 baseline lock 條件。
  • 更新 openspec/specs/runtime-verification-evidence/spec.md,明確區分 conversion success、visual preview blocker、canonical batch passed / blocked / timed_out / partial 狀態。
  • 更新 openspec/specs/worker-demo-upload-convert-ui/spec.md,補入 worker UI lineage / quality view 與 review viewer handoff 邊界。
  • 新增 openspec/changes/optimize-worker-source-entity-enumeration/,包含 proposal、design、delta specs 與 26 項 tasks。
  • 同步更新 docs/plans/AI-BIM-governance-saas-roadmap-2026-05.md 與同名 HTML 檢視版,標示 canonical batch archive 仍 runtime blocked,下一個 active change 改為 enumeration optimization。

驗證方式

  • openspec validate optimize-worker-source-entity-enumeration --strict:通過。
  • openspec validate --all --strict:12 passed / 0 failed。
  • git diff --cached --check:通過,無 whitespace error。
  • gitnexus analyze --embeddings --skills --skip-agents-md:完成,目前 worktree index 顯示 up-to-date。
  • gitnexus status:目前 worktree index up-to-date。
  • gitnexus detect-changes --scope staged:未完成;GitNexus resolver 仍未在 detect-changes repo 選單中刷新出本次 c161 worktree,即使 gitnexus list 與 gitnexus status 已可看到/確認該 index。

尚未執行 canonical runtime conversion、browser visual preview、full 13-file batch 或 Kit/GPU/WebRTC 驗證。

風險與影響

  • 影響範圍為 OpenSpec artifacts、現行 specs 與 roadmap 文件;未修改 production runtime code、API handler、資料庫 migration、環境變數、部署流程、排程或 webhook。
  • worker-canonical-storage-batch-baseline 已歸檔,但 runtime evidence 仍 blocked at source_entity_enumeration;不得將此 PR 解讀為 canonical batch passed。
  • 新 change 會把後續實作焦點放在 _worker source entity enumeration,後續若更動 converter symbol,仍需依 AGENTS 執行 GitNexus impact analysis。
  • Roadmap HTML 是 Markdown 的衍生檢視,source of truth 仍為同名 .md。

回滾方式

若需撤回,revert 本 PR 即可還原:

  • openspec/specs/ 的 canonical batch requirement 更新。
  • openspec/changes/archive/2026-05-12-worker-canonical-storage-batch-baseline/ 的歸檔移動。
  • openspec/changes/optimize-worker-source-entity-enumeration/ 新 change。
  • roadmap Markdown 與 HTML 對齊內容。

後續建議

  • 在 optimize-worker-source-entity-enumeration 實作前,先針對 _worker/app/converters.py 相關 symbols 做 GitNexus impact analysis。
  • 優先 profile _source_entities(model) / all-entity enumeration 的耗時與 last-known operation。
  • 維持 all-IFC-entity denominator,不得改成 geometry-only、IfcProduct-only、GUID-only 或 renderable-only。
  • 只有 canonical single fixture、visual preview、full 13-file batch、USDC openability、lineage API 與 locked coverage 全部通過後,才可設定 minimum_coverage_locked=true。

Summary by CodeRabbit

  • New Features

    • Per-fixture timeout with timed-out reporting and batch timeout summary
    • Detailed per-phase timing/progress (including "not_run" for dry-runs) and minimum-coverage locking semantics
    • Worker UI exposes review handoff with USDC review link
  • Bug Fixes / Improvements

    • Centralized status classification across conversion/quality/coverage/lineage
  • CLI

    • Added --timeout-seconds option for batch verification
  • Tests

    • Expanded tests for timeouts, dry-run, subset, failed, and locked-pass scenarios
  • Documentation

    • Roadmap, verification records, and specs updated for phase timing, timeout handling, and enumeration optimization

Review Change Stack

Copilot AI review requested due to automatic review settings May 12, 2026 11:36
@coderabbitai

coderabbitai Bot commented May 12, 2026 •

Copy link
Copy Markdown
Contributor

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 0d37a24b-f62e-4ceb-839b-f0e847a9361e

📥 Commits

Reviewing files that changed from the base of the PR and between b7d5e25 and dec9718.

📒 Files selected for processing (2)
  • docs/plans/AI-BIM-governance-saas-roadmap-2026-05.html
  • docs/plans/AI-BIM-governance-saas-roadmap-2026-05.md
🚧 Files skipped from review as they are similar to previous changes (1)
  • docs/plans/AI-BIM-governance-saas-roadmap-2026-05.md

📝 Walkthrough

Walkthrough

This PR adds per-fixture timeouts (multiprocessing) and timed_out reporting to batch verification, records converter per-phase timings/progress, exposes a review-viewer handoff in the UI, archives canonical batch evidence, and introduces an OpenSpec change to optimize source entity enumeration.

Changes

Timeout-aware batch verification with phase tracking

Layer / File(s) Summary
Converter phase tracking foundation
_worker/app/converters.py, _worker/app/store.py, _worker/tests/test_worker_converters.py, _worker/tests/test_worker_store.py
Adds CONVERSION_PHASES, per-phase phase_timings and progress persistence; store records artifact_publish timing and passes converter_job with phase_progress_path into adapter.convert(); tests assert phase timing entries.
Batch fixture processing and status logic
_worker/app/batch_verification.py
Extracts _run_single_fixture to run intake, conversion (with mapping), merge phase_timings, lineage lookup, and normalize result; _fixture_status centralizes status logic; _review_viewer_handoff builds handoff payload.
Timeout mechanism with multiprocessing and progress utilities
_worker/app/batch_verification.py
Implements _run_single_fixture_with_timeout and _run_single_fixture_process using multiprocessing + queue, drains progress/result/error messages, enforces per-fixture timeout_seconds, builds timed_out records using last-known phase progress, and provides queue/progress helpers.
Phase timing templates and batch aggregation
_worker/app/batch_verification.py
Adds phase timing templates for pending/not_run/timeout, timing/merge utilities, propagates timeout_seconds and timed_out_count in batch summary, and computes minimum_coverage_locked only when overall status is passed.
CLI, tests, and UI handoff
_worker/scripts/verify_storage_batch.py, _worker/tests/*, _worker/app/ui.py
Adds --timeout-seconds CLI arg (disabled in dry-run) and treats timed_out as failure; extends tests with fake converters for failing/locked/slow cases and adds UI handoff fields + configureReviewHandoff(); adds UI test confirming handoff exposure without USDC rendering.

Archived evidence and roadmap alignment

Layer / File(s) Summary
Canonical batch verification evidence and archive
docs/verification/2026-05-12-worker-canonical-storage-batch-baseline.md, openspec/changes/archive/2026-05-12-worker-canonical-storage-batch-baseline/tasks.md
Adds verification record showing 13 canonical IFC fixtures, timeout/phase diagnostics (timeout at source_entity_enumeration), canonical dry-run/real-run evidence, decision to keep minimum_coverage_locked=false, and archived task checklist updates.
Source entity enumeration optimization proposal and design
openspec/changes/optimize-worker-source-entity-enumeration/*
New OpenSpec change with metadata, proposal, design, specs, and tasks to profile and optimize source_entity_enumeration, requiring additive diagnostics, denominator preservation, and canonical single-fixture rerun verification.
Roadmap and spec updates
docs/plans/AI-BIM-governance-saas-roadmap-2026-05.md, openspec/specs/*
Mark canonical batch archived/blocked, require --limit 1 gate before full batch, require per-fixture phase timing with unavailable/not-reached labels, add batch status semantics, and specify review handoff prerequisites for visual preview integration.

Sequence Diagram(s)

sequenceDiagram
  participant CLI
  participant run_storage_batch_verification
  participant _run_single_fixture_with_timeout
  participant Process
  participant _run_single_fixture
  participant Converter
  participant job_phase_progress_path
  CLI->>run_storage_batch_verification: invoke (timeout_seconds)
  run_storage_batch_verification->>_run_single_fixture_with_timeout: start fixture
  _run_single_fixture_with_timeout->>Process: spawn (_run_single_fixture_process)
  Process->>_run_single_fixture: execute intake + conversion
  _run_single_fixture->>Converter: adapter.convert(converter_job)
  Converter->>job_phase_progress_path: write phase progress JSON
  _run_single_fixture->>_run_single_fixture_with_timeout: publish result via queue
  _run_single_fixture_with_timeout->>run_storage_batch_verification: return result (passed/failed/timed_out)
Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~60 minutes

Possibly related PRs

  • monkey1sai/AI-BIM-governance#29: Related batch verification work and status taxonomy that this PR extends with multiprocessing timeouts and timed_out outcomes.
  • monkey1sai/AI-BIM-governance#24: Earlier converter/store phase-timings work that this PR builds upon by persisting phase progress and embedding phase_timings in results.

Poem

🐰 I hopped through phases, timers in paw,
Queues whispered progress, the child process saw,
Timeouts caught, handoffs wrapped neat,
Evidence archived, roadmap set to beat—
Next, I’ll nibble at enumeration’s maw.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately describes the primary changes: archiving the canonical batch change and introducing a new optimization change focused on source entity enumeration.
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.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch codex/openspec/optimize-worker-source-entity-enumeration

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 and usage tips.

@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: 4

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (6)
openspec/changes/optimize-worker-source-entity-enumeration/specs/worker-artifact-pipeline/spec.md (1)

1-32: ⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Consider translating spec content to Traditional Chinese.

The coding guidelines state: "All OpenSpec artifacts must use Traditional Chinese (繁體中文); preserve original text for API paths, schema fields, CLI flags, status enums, logs/errors, external product names, and OpenSpec parser required headers."

While technical identifiers (source_entity_enumeration, ifc_entity_key, etc.) are correctly preserved in English, the descriptive requirement text and scenario prose should use Traditional Chinese for consistency with other OpenSpec artifacts in this repository. As per coding guidelines, OpenSpec artifacts should use Traditional Chinese (繁體中文).

🤖 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
`@openspec/changes/optimize-worker-source-entity-enumeration/specs/worker-artifact-pipeline/spec.md`
around lines 1 - 32, The spec's descriptive and scenario prose must be
translated from English into Traditional Chinese while preserving all technical
identifiers and API/schema/CLI tokens (e.g., source_entity_enumeration,
ifc_entity_key, ifc_entity_id, ifc_class, ifc_guid, name, coverage_denominator,
source_ifc_entity_count, minimum_coverage_locked) and keeping parser-required
headers and any exact flags/enums/log text unchanged; update the body text of
each Requirement and Scenario to 繁體中文, ensuring diagnostics/field names remain
in English where specified and that the resulting document remains semantically
identical.
openspec/changes/optimize-worker-source-entity-enumeration/README.md (1)

1-4: ⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Consider translating README content to Traditional Chinese.

As per coding guidelines, "All OpenSpec artifacts must use Traditional Chinese (繁體中文); preserve original text for API paths, schema fields, CLI flags, status enums, logs/errors, external product names, and OpenSpec parser required headers."

This README should use Traditional Chinese for the descriptive content while preserving technical identifiers like _worker, IfcOpenShell, source_entity_enumeration in English.

🤖 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 `@openspec/changes/optimize-worker-source-entity-enumeration/README.md` around
lines 1 - 4, The README currently uses English for descriptive content but must
be translated to Traditional Chinese (繁體中文) per guidelines while preserving
technical identifiers; update the README.md text (but not the identifiers
`_worker`, `IfcOpenShell`, `source_entity_enumeration`, "canonical 89MB IFC",
API/CLI/schema names, logs/errors) by replacing the English description with a
clear Traditional Chinese translation that retains those technical terms
verbatim.
_worker/app/converters.py (1)

160-167: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Attach GitNexus impact/detect-changes evidence for modified converter symbols before merge.

This PR modifies IfcOpenShellUsdConverter.convert and related flow, but the PR notes indicate detect-changes was not refreshed. Please include the final GitNexus impact + detect-changes output for these symbols in this PR.

As per coding guidelines, "**/*.{ts,tsx,js,jsx,py}: Before modifying a function, class, or method, run GitNexus impact analysis... MUST run gitnexus_detect_changes() before committing."

🤖 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 `@_worker/app/converters.py` around lines 160 - 167, The PR changed the
converter flow (notably IfcOpenShellUsdConverter.convert and its related
converter symbols) but did not include the required GitNexus impact and
detect-changes evidence; run gitnexus_detect_changes() locally (or via your CI
step) targeting the modified symbols (IfcOpenShellUsdConverter.convert and any
helper methods/classes you modified), capture the full GitNexus impact +
detect-changes output, and attach those results to this PR (update the PR
description or add a file) so the change log and impact analysis are recorded
before merge.
openspec/changes/optimize-worker-source-entity-enumeration/specs/runtime-verification-evidence/spec.md (1)

1-26: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

OpenSpec delta spec 內容語言需改為繁體中文。

目前新增規格主體為英文,請轉為繁體中文撰寫;僅保留規範允許維持原文的 API/欄位/CLI/status 等 token。

As per coding guidelines, "openspec/**/*.md: All OpenSpec artifacts must use Traditional Chinese (繁體中文); preserve original text for API paths, schema fields, CLI flags, status enums, logs/errors, external product names, and OpenSpec parser required headers."

🤖 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
`@openspec/changes/optimize-worker-source-entity-enumeration/specs/runtime-verification-evidence/spec.md`
around lines 1 - 26, 將此新增的 OpenSpec 規格主體(包括「Requirement: Source entity
enumeration optimization evidence」段落及其所有情境描述)從英文完整翻譯為繁體中文,僅保留原文的
API/欄位/CLI/status 等 token 不翻譯,例如保留 source_entity_enumeration、canonical fixture
identity、conversion_job_id、artifact_group_id、USDC、mapping artifact
ID、minimum_coverage_locked、_worker、timed_out、blocked 及 CLI 範例 `--limit 1
--timeout-seconds 600` 等文字;請在翻譯中保留原有段落結構、標題層級和專有符號(如
`Requirement:`、`Scenario:`)不變,並確保所有描述性文字改為繁體中文以符合 openspec/ 檔案語言規範。
_worker/app/ui.py (1)

434-459: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Reset review handoff state to avoid stale links across runs.

Line 507 only updates links in the usdc_url path. If a previous conversion succeeded, a later failed/partial run can keep old nextStep/reviewPreview targets enabled.

Suggested patch
     async function startConversion() {
       if (!selected) return;
       clearTimeout(pollTimer);
+      nextStep.href = "http://127.0.0.1:8004";
+      nextStep.setAttribute("aria-disabled", "true");
+      reviewPreview.href = "http://127.0.0.1:8004";
+      reviewPreview.setAttribute("aria-disabled", "true");
+      usdcUrlEl.textContent = "—";
+      reviewHandoffEl.textContent = "—";
       setStatus(jobStatus, "warn", "建立 job");
       try {
@@
     function configureReviewHandoff(result, artifactGroupId) {
@@
       if (result.usdc_url) {
         nextStep.href = target;
         nextStep.setAttribute("aria-disabled", "false");
         reviewPreview.href = target;
         reviewPreview.setAttribute("aria-disabled", "false");
+      } else {
+        nextStep.href = "http://127.0.0.1:8004";
+        nextStep.setAttribute("aria-disabled", "true");
+        reviewPreview.href = "http://127.0.0.1:8004";
+        reviewPreview.setAttribute("aria-disabled", "true");
       }
     }

Also applies to: 493-513

🤖 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 `@_worker/app/ui.py` around lines 434 - 459, Reset the previous run's
handoff/review links and IDs at the start (and on error) of a new conversion so
stale nextStep/reviewPreview targets are not left enabled; specifically, in
startConversion() clear or disable the review link elements and reset jobIdEl
and artifactGroupEl to placeholders before the fetch, and also ensure the catch
block clears/disables those same review/nextStep UI targets when a conversion
fails; reference the startConversion, jobIdEl, artifactGroupEl, and
pollConversion call sites to locate where to clear/reset the review handoff
state.
openspec/changes/optimize-worker-source-entity-enumeration/design.md (1)

1-51: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

請將此 OpenSpec 設計文件改為繁體中文(保留規定英文 token)

這份新增 design.md 目前大部分為英文敘述,與 OpenSpec 文件語言規範不一致,後續維運與審核可追溯性會受影響。

As per coding guidelines, "All OpenSpec artifacts must use Traditional Chinese (繁體中文); preserve original text for API paths, schema fields, CLI flags, status enums, logs/errors, external product names, and OpenSpec parser required headers."

🤖 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 `@openspec/changes/optimize-worker-source-entity-enumeration/design.md` around
lines 1 - 51, 將此 OpenSpec 設計文件從英文翻譯為繁體中文(保留規定英文 token 不變): 翻譯所有段落標題與敘述(例如
"Context", "Goals / Non-Goals", "Decisions", 各條列內容與 "Risks / Trade-offs"
等)與細節為繁體中文,但嚴格保留並不翻譯 API paths, schema fields, CLI flags, status enums,
logs/errors, external product names, 以及 OpenSpec parser required
headers(保持其原始英文字串不變);同時確保術語一致且原意不變、維持原有段落結構與所有英文 token
的精確大小寫,並在完成後執行快速對照檢查以確認沒有意外改動到保留的英文標記(例如保留 "source_entity_enumeration",
"_worker", "--limit 1", "minimum_coverage_locked=true" 等字串)。
🤖 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 `@_worker/app/batch_verification.py`:
- Around line 141-154: The timeout cleanup currently calls process.terminate()
and process.join(5) but doesn't hard-kill if the child ignores SIGTERM; add a
fallback that checks process.is_alive() after process.join(5) and, if still
alive, call process.kill() (SIGKILL) and then process.join() again (with a short
timeout) to ensure the subprocess is reaped before building the _timeout_record
and calling _mark_job_timed_out; keep this logic inside the same timeout branch
where process.terminate() is invoked and handle any exceptions from kill/join so
the function still returns the record.

In `@_worker/app/converters.py`:
- Around line 135-155: The _record_phase_progress function currently writes
diagnostics via _write_json and will raise if the progress_path is invalid or
unwritable; make this persistence best-effort by wrapping the call to
_write_json (using the progress_path variable) in a try/except that catches
OSError/IOError and Exception from serialization, and suppresses the exception
(optionally logging the error) so that failures to write phase progress do not
propagate and fail the conversion job; ensure the function still returns None on
write failure and does not re-raise.

In `@_worker/scripts/verify_storage_batch.py`:
- Around line 20-33: Validate that the --timeout-seconds value is strictly
positive before calling run_storage_batch_verification: update the
parser.add_argument for "--timeout-seconds" or add a post-parse check that
inspects args.timeout_seconds and raises/prints an error and exits if the value
is <= 0; ensure the check runs before invoking
run_storage_batch_verification(Settings.from_env(), ...) so that invalid inputs
are rejected at parse time with a clear message.

In `@openspec/changes/optimize-worker-source-entity-enumeration/proposal.md`:
- Around line 1-31: 將整個 proposal.md 內容改寫為繁體中文(包含段落說明與敘述),但保留所有不能翻譯的原文 token:如
`_worker`、`worker-artifact-pipeline`、`runtime-verification-evidence`、`_worker/app/converters.py`、`_worker/app/batch_verification.py`、`_worker/tests/*`、`scripts/verify_storage_batch.py`、CLI
flag、schema 欄位、API 路徑、狀態 enum、logs/errors、外部產品名稱及 OpenSpec 必要的 header 不變;確保標題(例如
"Why", "What Changes", "Capabilities", "Impact")或其翻譯仍符合 OpenSpec parser
要求並保留任何必需的原文標記與示例,以免破壞機器解析或識別欄位。

---

Outside diff comments:
In `@_worker/app/converters.py`:
- Around line 160-167: The PR changed the converter flow (notably
IfcOpenShellUsdConverter.convert and its related converter symbols) but did not
include the required GitNexus impact and detect-changes evidence; run
gitnexus_detect_changes() locally (or via your CI step) targeting the modified
symbols (IfcOpenShellUsdConverter.convert and any helper methods/classes you
modified), capture the full GitNexus impact + detect-changes output, and attach
those results to this PR (update the PR description or add a file) so the change
log and impact analysis are recorded before merge.

In `@_worker/app/ui.py`:
- Around line 434-459: Reset the previous run's handoff/review links and IDs at
the start (and on error) of a new conversion so stale nextStep/reviewPreview
targets are not left enabled; specifically, in startConversion() clear or
disable the review link elements and reset jobIdEl and artifactGroupEl to
placeholders before the fetch, and also ensure the catch block clears/disables
those same review/nextStep UI targets when a conversion fails; reference the
startConversion, jobIdEl, artifactGroupEl, and pollConversion call sites to
locate where to clear/reset the review handoff state.

In `@openspec/changes/optimize-worker-source-entity-enumeration/design.md`:
- Around line 1-51: 將此 OpenSpec 設計文件從英文翻譯為繁體中文(保留規定英文 token 不變): 翻譯所有段落標題與敘述(例如
"Context", "Goals / Non-Goals", "Decisions", 各條列內容與 "Risks / Trade-offs"
等)與細節為繁體中文,但嚴格保留並不翻譯 API paths, schema fields, CLI flags, status enums,
logs/errors, external product names, 以及 OpenSpec parser required
headers(保持其原始英文字串不變);同時確保術語一致且原意不變、維持原有段落結構與所有英文 token
的精確大小寫,並在完成後執行快速對照檢查以確認沒有意外改動到保留的英文標記(例如保留 "source_entity_enumeration",
"_worker", "--limit 1", "minimum_coverage_locked=true" 等字串)。

In `@openspec/changes/optimize-worker-source-entity-enumeration/README.md`:
- Around line 1-4: The README currently uses English for descriptive content but
must be translated to Traditional Chinese (繁體中文) per guidelines while preserving
technical identifiers; update the README.md text (but not the identifiers
`_worker`, `IfcOpenShell`, `source_entity_enumeration`, "canonical 89MB IFC",
API/CLI/schema names, logs/errors) by replacing the English description with a
clear Traditional Chinese translation that retains those technical terms
verbatim.

In
`@openspec/changes/optimize-worker-source-entity-enumeration/specs/runtime-verification-evidence/spec.md`:
- Around line 1-26: 將此新增的 OpenSpec 規格主體(包括「Requirement: Source entity
enumeration optimization evidence」段落及其所有情境描述)從英文完整翻譯為繁體中文,僅保留原文的
API/欄位/CLI/status 等 token 不翻譯,例如保留 source_entity_enumeration、canonical fixture
identity、conversion_job_id、artifact_group_id、USDC、mapping artifact
ID、minimum_coverage_locked、_worker、timed_out、blocked 及 CLI 範例 `--limit 1
--timeout-seconds 600` 等文字;請在翻譯中保留原有段落結構、標題層級和專有符號(如
`Requirement:`、`Scenario:`)不變,並確保所有描述性文字改為繁體中文以符合 openspec/ 檔案語言規範。

In
`@openspec/changes/optimize-worker-source-entity-enumeration/specs/worker-artifact-pipeline/spec.md`:
- Around line 1-32: The spec's descriptive and scenario prose must be translated
from English into Traditional Chinese while preserving all technical identifiers
and API/schema/CLI tokens (e.g., source_entity_enumeration, ifc_entity_key,
ifc_entity_id, ifc_class, ifc_guid, name, coverage_denominator,
source_ifc_entity_count, minimum_coverage_locked) and keeping parser-required
headers and any exact flags/enums/log text unchanged; update the body text of
each Requirement and Scenario to 繁體中文, ensuring diagnostics/field names remain
in English where specified and that the resulting document remains semantically
identical.
🪄 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: b4fd1315-2509-461b-a564-5aba80e6c0a7

📥 Commits

Reviewing files that changed from the base of the PR and between c37f5bb and b7d5e25.

📒 Files selected for processing (29)
  • _worker/app/batch_verification.py
  • _worker/app/converters.py
  • _worker/app/store.py
  • _worker/app/ui.py
  • _worker/scripts/verify_storage_batch.py
  • _worker/tests/test_worker_api.py
  • _worker/tests/test_worker_batch_verification.py
  • _worker/tests/test_worker_converters.py
  • _worker/tests/test_worker_store.py
  • docs/plans/AI-BIM-governance-saas-roadmap-2026-05.html
  • docs/plans/AI-BIM-governance-saas-roadmap-2026-05.md
  • docs/verification/2026-05-12-worker-canonical-storage-batch-baseline.md
  • openspec/changes/archive/2026-05-12-worker-canonical-storage-batch-baseline/.openspec.yaml
  • openspec/changes/archive/2026-05-12-worker-canonical-storage-batch-baseline/design.md
  • openspec/changes/archive/2026-05-12-worker-canonical-storage-batch-baseline/proposal.md
  • openspec/changes/archive/2026-05-12-worker-canonical-storage-batch-baseline/specs/runtime-verification-evidence/spec.md
  • openspec/changes/archive/2026-05-12-worker-canonical-storage-batch-baseline/specs/worker-artifact-pipeline/spec.md
  • openspec/changes/archive/2026-05-12-worker-canonical-storage-batch-baseline/specs/worker-demo-upload-convert-ui/spec.md
  • openspec/changes/archive/2026-05-12-worker-canonical-storage-batch-baseline/tasks.md
  • openspec/changes/optimize-worker-source-entity-enumeration/.openspec.yaml
  • openspec/changes/optimize-worker-source-entity-enumeration/README.md
  • openspec/changes/optimize-worker-source-entity-enumeration/design.md
  • openspec/changes/optimize-worker-source-entity-enumeration/proposal.md
  • openspec/changes/optimize-worker-source-entity-enumeration/specs/runtime-verification-evidence/spec.md
  • openspec/changes/optimize-worker-source-entity-enumeration/specs/worker-artifact-pipeline/spec.md
  • openspec/changes/optimize-worker-source-entity-enumeration/tasks.md
  • openspec/specs/runtime-verification-evidence/spec.md
  • openspec/specs/worker-artifact-pipeline/spec.md
  • openspec/specs/worker-demo-upload-convert-ui/spec.md

Comment on lines +141 to +154
process.terminate()
process.join(5)
elapsed = perf_counter() - started
partial = _last_progress(messages)
phase_progress = _read_phase_progress(settings, partial)
record = _timeout_record(
source_item,
timeout_seconds=timeout_seconds,
elapsed_seconds=elapsed,
partial=partial,
phase_progress=phase_progress,
)
_mark_job_timed_out(settings, record)
return record

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.

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Verify timeout cleanup path around terminate/join lacks hard-kill fallback.
rg -n -C4 'process\.terminate\(\)|process\.join\(5\)|process\.is_alive\(\)' _worker/app/batch_verification.py

Repository: monkey1sai/AI-BIM-governance

Length of output: 498


🏁 Script executed:

#!/bin/bash
# Get complete context of the function containing the timeout cleanup
sed -n '130,160p' _worker/app/batch_verification.py

Repository: monkey1sai/AI-BIM-governance

Length of output: 1028


🏁 Script executed:

#!/bin/bash
# Check if there are any tests for the timeout path
find . -name "*.py" -path "*/test*" -o -name "*test*.py" | head -20

Repository: monkey1sai/AI-BIM-governance

Length of output: 1745


🌐 Web query:

Python multiprocessing Process.kill() method documentation and usage

💡 Result:

In Python’s multiprocessing, multiprocessing.Process.kill immediately terminates the underlying child process by sending SIGKILL on POSIX (and an equivalent hard-kill on non-POSIX via the platform’s termination API). It is documented as “Same as terminate but using the SIGKILL signal on POSIX.” [1][2] Signature / where it lives - Method: multiprocessing.Process.kill(self) - Also present in the CPython implementation as Process.kill, which delegates to the underlying _popen.kill. [3] How to use it (typical pattern) 1) Create and start a multiprocessing.Process. 2) When you need to stop it urgently, call p.kill. [1][2] 3) Usually follow with p.join so the parent waits for the process to fully exit (and to avoid leaving a zombie on Unix). (join is the documented way to wait for the child process to terminate.) [4][3] Important behavioral notes / warnings - kill is “terminate with SIGKILL” (hard kill). [1][2] - If you kill a process while it is trying to use a multiprocessing.Queue, the queue data may become corrupted, potentially causing exceptions in other processes that later use the queue. [1][2] Relationship to terminate and join - terminate: a “graceful-ish” termination request compared to kill; on POSIX it uses SIGTERM and on Windows uses TerminateProcess. [5] - kill: like terminate but escalates to SIGKILL on POSIX (hard kill). [1][2] - join: does not kill; it blocks the calling thread until the process terminates (or a timeout occurs). [4] Example import multiprocessing as mp import time def worker: while True: time.sleep(1) if name == "main": p = mp.Process(target=worker) p.start time.sleep(2) p.kill # hard kill (SIGKILL on POSIX) p.join # wait for it to exit

Citations:


🏁 Script executed:

#!/bin/bash
# Check the timeout test coverage in test_worker_batch_verification.py
cat ./_worker/tests/test_worker_batch_verification.py | grep -n -A 20 "timeout\|is_alive\|terminate"

Repository: monkey1sai/AI-BIM-governance

Length of output: 2693


🏁 Script executed:

#!/bin/bash
# Check if there are other process management patterns in the codebase
rg -n 'process\.(terminate|kill|is_alive)' _worker/ --type py

Repository: monkey1sai/AI-BIM-governance

Length of output: 203


Add hard-kill fallback after timeout terminate to prevent orphan processes

The timeout cleanup path calls process.terminate() then process.join(5), but lacks a fallback if the process doesn't respond to SIGTERM. If the subprocess ignores the graceful termination signal, it remains alive after the 5-second timeout, leaving orphaned processes that accumulate across batch runs and compromise stability.

Add an is_alive() check after the join timeout with a kill() fallback:

Proposed fix
    if process.is_alive():
        process.terminate()
        process.join(5)
+       if process.is_alive():
+           process.kill()
+           process.join(5)
        elapsed = perf_counter() - started

The queue is already drained before this block, so the hard kill is safe from queue corruption. This follows Python multiprocessing best practices: graceful termination (SIGTERM) → timeout → hard kill (SIGKILL) → await cleanup.

🤖 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 `@_worker/app/batch_verification.py` around lines 141 - 154, The timeout
cleanup currently calls process.terminate() and process.join(5) but doesn't
hard-kill if the child ignores SIGTERM; add a fallback that checks
process.is_alive() after process.join(5) and, if still alive, call
process.kill() (SIGKILL) and then process.join() again (with a short timeout) to
ensure the subprocess is reaped before building the _timeout_record and calling
_mark_job_timed_out; keep this logic inside the same timeout branch where
process.terminate() is invoked and handle any exceptions from kill/join so the
function still returns the record.

Comment thread _worker/app/converters.py
Comment on lines +135 to +155
def _record_phase_progress(
job: Mapping[str, Any],
phase: str,
phase_timings: dict[str, dict[str, Any]],
*,
status: str = "running",
) -> None:
progress_path = job.get("phase_progress_path")
if not progress_path:
return
_write_json(
Path(str(progress_path)),
{
"conversion_job_id": job.get("conversion_job_id"),
"source_artifact_id": job.get("source_artifact_id"),
"current_phase": phase,
"status": status,
"phase_timings": phase_timings,
},
)

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.

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Make phase-progress persistence best-effort to avoid false conversion failures.

If phase_progress_path is invalid/unwritable, Line 145 currently raises and fails the whole conversion. Diagnostics I/O should not become a hard conversion gate.

Suggested patch
 def _record_phase_progress(
@@
 ) -> None:
@@
-    _write_json(
-        Path(str(progress_path)),
-        {
-            "conversion_job_id": job.get("conversion_job_id"),
-            "source_artifact_id": job.get("source_artifact_id"),
-            "current_phase": phase,
-            "status": status,
-            "phase_timings": phase_timings,
-        },
-    )
+    try:
+        _write_json(
+            Path(str(progress_path)),
+            {
+                "conversion_job_id": job.get("conversion_job_id"),
+                "source_artifact_id": job.get("source_artifact_id"),
+                "current_phase": phase,
+                "status": status,
+                "phase_timings": phase_timings,
+            },
+        )
+    except Exception:
+        # best-effort diagnostics; must not fail conversion path
+        return
🤖 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 `@_worker/app/converters.py` around lines 135 - 155, The _record_phase_progress
function currently writes diagnostics via _write_json and will raise if the
progress_path is invalid or unwritable; make this persistence best-effort by
wrapping the call to _write_json (using the progress_path variable) in a
try/except that catches OSError/IOError and Exception from serialization, and
suppresses the exception (optionally logging the error) so that failures to
write phase progress do not propagate and fail the conversion job; ensure the
function still returns None on write failure and does not re-raise.

Comment on lines +20 to +33
parser.add_argument(
"--timeout-seconds",
type=float,
default=600.0,
help="Per-fixture timeout for real conversion runs.",
)
args = parser.parse_args()

payload = run_storage_batch_verification(Settings.from_env(), limit=args.limit, dry_run=args.dry_run)
payload = run_storage_batch_verification(
Settings.from_env(),
limit=args.limit,
dry_run=args.dry_run,
timeout_seconds=None if args.dry_run else args.timeout_seconds,
)

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.

⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Validate --timeout-seconds is strictly positive.

Line 20 currently accepts 0/negative values, which can lead to immediate timeout behavior and misleading diagnostics. Reject invalid values at parse time.

Suggested patch
     args = parser.parse_args()
+    if not args.dry_run and args.timeout_seconds <= 0:
+        parser.error("--timeout-seconds must be > 0 for real conversion runs.")
 
     payload = run_storage_batch_verification(
         Settings.from_env(),
🤖 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 `@_worker/scripts/verify_storage_batch.py` around lines 20 - 33, Validate that
the --timeout-seconds value is strictly positive before calling
run_storage_batch_verification: update the parser.add_argument for
"--timeout-seconds" or add a post-parse check that inspects args.timeout_seconds
and raises/prints an error and exits if the value is <= 0; ensure the check runs
before invoking run_storage_batch_verification(Settings.from_env(), ...) so that
invalid inputs are rejected at parse time with a clear message.

Comment on lines +1 to +31
## Why

Canonical `storage/*.ifc` batch verification currently times out on the first 89MB fixture after `ifc_open`, with the last known phase stuck at `source_entity_enumeration`. This blocks single-fixture evidence, visual preview, and the full 13-file baseline lock even though timeout diagnostics are now recorded.

## What Changes

- Optimize `_worker` IFC source entity enumeration so canonical fixtures can progress past the current bottleneck without weakening all-IFC-entity coverage semantics.
- Add measurable before/after timing evidence for `source_entity_enumeration`, including entity counts, elapsed duration, and timeout/budget diagnostics.
- Preserve existing artifact intake, conversion job, lineage, mapping, and review viewer handoff contracts.
- Keep `minimum_coverage_locked=false` unless the full canonical batch still satisfies the archived baseline requirements.
- Do not introduce viewer, coordinator, Kit runtime, WebRTC, GPU, auth, session lifecycle, or production batch-job responsibilities into `_worker`.

## Capabilities

### New Capabilities

- None.

### Modified Capabilities

- `worker-artifact-pipeline`: require `_worker` source entity enumeration to be profiled and optimized for canonical IFC fixtures while preserving all-entity coverage and stable IFC-to-USD traceability.
- `runtime-verification-evidence`: require optimization evidence to record before/after source entity enumeration timing and the canonical single-fixture rerun result.

## Impact

- Owner: `_worker`.
- Likely code paths: `_worker/app/converters.py`, `_worker/app/batch_verification.py`, and focused `_worker/tests/*`.
- Data structures: conversion quality metrics and batch verification evidence may gain source entity enumeration diagnostics; existing fields must remain backward-compatible.
- CLI: `scripts/verify_storage_batch.py` may be used for validation; no required CLI breaking change is expected.
- Dependencies: no new production dependency unless a measured standard-library or existing IfcOpenShell/USD path cannot solve the bottleneck.
- Runtime boundary: visual preview remains outside `_worker` and must continue through `bim-review-coordinator`, `web-viewer-sample`, and `bim-streaming-server` after conversion succeeds.

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.

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

OpenSpec proposal內容語言需改為繁體中文。

目前新檔案主體為英文,與 OpenSpec markdown 語言規範不一致;請改為繁體中文,僅保留規範允許的 API/欄位/CLI 等原文 token。

As per coding guidelines, "openspec/**/*.md: All OpenSpec artifacts must use Traditional Chinese (繁體中文); preserve original text for API paths, schema fields, CLI flags, status enums, logs/errors, external product names, and OpenSpec parser required headers."

🤖 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 `@openspec/changes/optimize-worker-source-entity-enumeration/proposal.md`
around lines 1 - 31, 將整個 proposal.md 內容改寫為繁體中文(包含段落說明與敘述),但保留所有不能翻譯的原文 token:如
`_worker`、`worker-artifact-pipeline`、`runtime-verification-evidence`、`_worker/app/converters.py`、`_worker/app/batch_verification.py`、`_worker/tests/*`、`scripts/verify_storage_batch.py`、CLI
flag、schema 欄位、API 路徑、狀態 enum、logs/errors、外部產品名稱及 OpenSpec 必要的 header 不變;確保標題(例如
"Why", "What Changes", "Capabilities", "Impact")或其翻譯仍符合 OpenSpec parser
要求並保留任何必需的原文標記與示例,以免破壞機器解析或識別欄位。

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 archives the previously active canonical storage batch baseline OpenSpec change (while preserving its “runtime still blocked” status), folds the accepted requirements into the current specs, and introduces a new focused OpenSpec change to optimize _worker’s source_entity_enumeration phase (the current timeout blocker on the first 89MB canonical IFC fixture). In parallel, it updates _worker’s batch verification helper and UI handoff to capture richer timeout/phase diagnostics and enable review-viewer handoff without rendering USDC inside _worker.

Changes:

  • Archive worker-canonical-storage-batch-baseline, sync its key requirements into current OpenSpec specs, and add a detailed verification report + roadmap alignment indicating the canonical run remains blocked at source_entity_enumeration.
  • Add a new OpenSpec change optimize-worker-source-entity-enumeration with proposal/design/spec deltas + task plan focused on profiling/optimizing enumeration without weakening all-entity denominator semantics.
  • Enhance _worker batch verification with per-fixture timeout classification (via multiprocessing), phase progress persistence, additional phase timings (including artifact_publish), and UI review-viewer handoff parameters.

Reviewed changes

Copilot reviewed 22 out of 29 changed files in this pull request and generated 4 comments.

Show a summary per file
File Description
openspec/specs/worker-demo-upload-convert-ui/spec.md Updates worker demo UI boundary + lineage/quality + review viewer handoff requirements.
openspec/specs/worker-artifact-pipeline/spec.md Tightens canonical storage batch verification semantics (statuses, timings, single-fixture gate, handoff).
openspec/specs/runtime-verification-evidence/spec.md Clarifies evidence tiers and canonical gates (single fixture, visual preview, batch status classification).
openspec/changes/optimize-worker-source-entity-enumeration/tasks.md New task breakdown for profiling/optimizing enumeration + evidence and tests.
openspec/changes/optimize-worker-source-entity-enumeration/specs/worker-artifact-pipeline/spec.md Delta requirements for enumeration optimization + additive diagnostics.
openspec/changes/optimize-worker-source-entity-enumeration/specs/runtime-verification-evidence/spec.md Delta requirements for before/after timing evidence and baseline-unlocked semantics.
openspec/changes/optimize-worker-source-entity-enumeration/README.md Change README describing the optimization intent.
openspec/changes/optimize-worker-source-entity-enumeration/proposal.md Motivation/scope/impact statement for the optimization slice.
openspec/changes/optimize-worker-source-entity-enumeration/design.md Design decisions (profile-first, minimal identity scan, additive progress, staged validation).
openspec/changes/optimize-worker-source-entity-enumeration/.openspec.yaml New change metadata.
openspec/changes/archive/2026-05-12-worker-canonical-storage-batch-baseline/tasks.md Archived change tasks updated to reflect completion/blocked status narrative.
openspec/changes/archive/2026-05-12-worker-canonical-storage-batch-baseline/specs/worker-demo-upload-convert-ui/spec.md Archived delta spec snapshot for the UI handoff boundary.
openspec/changes/archive/2026-05-12-worker-canonical-storage-batch-baseline/specs/worker-artifact-pipeline/spec.md Archived delta spec snapshot for batch semantics/timings/gates.
openspec/changes/archive/2026-05-12-worker-canonical-storage-batch-baseline/specs/runtime-verification-evidence/spec.md Archived delta spec snapshot for evidence acceptance semantics.
openspec/changes/archive/2026-05-12-worker-canonical-storage-batch-baseline/proposal.md Archived proposal snapshot.
openspec/changes/archive/2026-05-12-worker-canonical-storage-batch-baseline/design.md Archived design snapshot.
openspec/changes/archive/2026-05-12-worker-canonical-storage-batch-baseline/.openspec.yaml Archived change metadata.
docs/verification/2026-05-12-worker-canonical-storage-batch-baseline.md New verification report documenting timeout evidence and blockers.
docs/plans/AI-BIM-governance-saas-roadmap-2026-05.md Roadmap updated to show archive + blocked status and point to enumeration optimization as next active slice.
_worker/tests/test_worker_store.py Asserts artifact_publish phase timing is recorded in stored quality metrics.
_worker/tests/test_worker_converters.py Adds assertions for converter phase_timings completion fields.
_worker/tests/test_worker_batch_verification.py Adds coverage for dry-run/subset/timeout/failed/passed semantics + handoff payload.
_worker/tests/test_worker_api.py Validates UI exposes review handoff without USD rendering strings.
_worker/scripts/verify_storage_batch.py Adds --timeout-seconds and exits non-zero on timed out runs.
_worker/app/ui.py Adds UI fields/actions for USDC URL + review viewer handoff link.
_worker/app/store.py Adds phase progress path injection + artifact_publish timing injection.
_worker/app/converters.py Adds conversion phase timings + on-disk phase progress writes for timeout diagnostics.
_worker/app/batch_verification.py Adds multiprocessing timeout wrapper, richer per-fixture records, phase timing merging, and baseline lock semantics.

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

Comment thread _worker/app/ui.py
Comment on lines +305 to 312
<dt>USDC</dt><dd id="usdcUrl">—</dd>
<dt>handoff</dt><dd id="reviewHandoff">—</dd>
</dl>
<div class="demo-actions">
<a id="nextStep" class="demo-btn" href="http://127.0.0.1:8004" aria-disabled="true">前往建立會議</a>
<a id="reviewPreview" class="demo-btn demo-btn--secondary" href="http://127.0.0.1:8004" aria-disabled="true">開啟 USDC Review</a>
</div>
</aside>
Comment on lines +130 to +154
ctx = mp.get_context("spawn")
queue = ctx.Queue()
process = ctx.Process(
target=_run_single_fixture_process,
args=(settings, source_id, converter, queue),
)
started = perf_counter()
process.start()
process.join(timeout_seconds)
messages = _drain_queue(queue)
if process.is_alive():
process.terminate()
process.join(5)
elapsed = perf_counter() - started
partial = _last_progress(messages)
phase_progress = _read_phase_progress(settings, partial)
record = _timeout_record(
source_item,
timeout_seconds=timeout_seconds,
elapsed_seconds=elapsed,
partial=partial,
phase_progress=phase_progress,
)
_mark_job_timed_out(settings, record)
return record
Comment on lines +463 to +485
def _timeout_phase_timings(
last_phase: str,
progress_timings: Any = None,
*,
intake_completed: bool = False,
) -> dict[str, dict[str, Any]]:
timings = _merge_phase_timings(_pending_phase_timings(), progress_timings)
if intake_completed:
timings["source_read"] = _completed_unmeasured_timing("completed_before_timeout")
timings["artifact_intake"] = _completed_unmeasured_timing("completed_before_timeout")
if last_phase in timings:
timings[last_phase] = {
"status": "timed_out",
"duration_seconds": None,
"diagnostic": "fixture_timed_out_during_phase",
}
else:
timings["conversion_total"] = {
"status": "timed_out",
"duration_seconds": None,
"diagnostic": f"fixture_timed_out_after_{last_phase}",
}
return timings
- **THEN** evidence 記錄 `minimum_coverage_locked=true`、`minimum_coverage_ratio=1.0`、`coverage_denominator=source_ifc_entity_count`、per-fixture metrics,並將 batch status 設為 `passed`

#### Scenario: Storage fixture batch is incomplete
#### Scenario: Storage fixture batch incomplete

@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: b7d5e25c7e

ℹ️ 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 _worker/app/ui.py
params.set("mapping_url", result.mapping_url || "");
const derived = result.derived_artifact_ids || {};
if (derived.element_mapping) params.set("mapping_artifact_id", derived.element_mapping);
const target = `http://127.0.0.1:8004?${params.toString()}`;

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 Route the review handoff to a page that consumes it

The new handoff action builds http://127.0.0.1:8004?..., but the coordinator only serves its console on /ui and /dev-console (checked bim-review-coordinator/src/app.ts), and its dev-console code does not read these query params when creating a session. In the worker UI success path, clicking “開啟 USDC Review” therefore either lands on the coordinator 404/root or drops the converted usdc_url/mapping_url, so the promised handoff does not open the just-converted model. Point this to a real route that imports these params or post an artifact_bindings payload.

Useful? React with 👍 / 👎.

started = perf_counter()
phase_timings = _pending_phase_timings()
store = WorkerStore(settings, converter=converter)
source_path, resolved_item = resolve_dev_ifc_source(settings.dev_storage_root, source_id)

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 stale fixture failures inside the batch result

In non-timeout runs, resolving the selected source now happens before _run_single_fixture enters its per-fixture try block. If an IFC is modified or removed between list_dev_ifc_sources() and processing, resolve_dev_ifc_source() raises ValueError("Unknown or stale dev IFC source.") and aborts the whole batch instead of recording that fixture as failed and continuing as the previous implementation did. Move this resolution into the existing try so transient storage races produce a normal failed fixture record.

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: dec971861a

ℹ️ 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 +262 to +264
result = completed.get("result") or {}
quality = result.get("quality_metrics") or {}
phase_timings = _merge_phase_timings(phase_timings, quality.get("phase_timings"))

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 Preserve phase progress for failed conversions

When a converter fails without hitting the timeout path, complete_conversion_job() returns a failed result with no quality_metrics, so this merge never picks up the .phase.json progress that the converter wrote. In that scenario the batch record only shows source_read/artifact_intake/conversion_total, losing the ifc_open or source_entity_enumeration phase that actually failed; this contradicts the new phase-timing diagnostics contract and makes non-timeout canonical blockers opaque. Read the phase progress file on failed results as well, not only in _timeout_record.

Useful? React with 👍 / 👎.

Comment on lines 76 to 84
for source_item in selected_sources:
started = perf_counter()
record: dict[str, Any] = {
"filename": source_item["filename"],
"relative_path": source_item["relative_path"],
"size_bytes": source_item["size_bytes"],
}
try:
source_path, resolved_item = resolve_dev_ifc_source(settings.dev_storage_root, source_item["source_id"])
source_artifact = store.create_source_artifact(
ArtifactIntakeRequest(
tenant_id="tenant_batch_verification",
project_id="project_batch_verification",
model_version_id="version_batch_verification",
source_system="dev_storage",
uploaded_by="batch_verification",
filename=resolved_item["filename"],
source_format="ifc",
content_base64=base64.b64encode(source_path.read_bytes()).decode("ascii"),
)
)
job = store.create_conversion_job(
source_artifact["source_artifact_id"],
{"target_format": "usdc", "generate_mapping": True},
if timeout_seconds is not None and timeout_seconds > 0:
record = _run_single_fixture_with_timeout(
settings,
source_item["source_id"],
source_item,
converter=converter,
timeout_seconds=timeout_seconds,
)

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 Gate full batch runs after the single-fixture blocker

With the CLI default --limit unset, selected_sources contains the full fixture set, and after one fixture times out this loop continues launching the remaining conversions. That bypasses the new current spec in openspec/specs/worker-artifact-pipeline/spec.md:261, which says the helper should try the full 13-file batch only after the canonical --limit 1 run passes or records a deterministic blocker; in the current 600s timeout case this can turn the known first-fixture blocker into many hours of unnecessary conversions and artifacts.

Useful? React with 👍 / 👎.

@monkey1sai

Copy link
Copy Markdown
Owner Author

Code review 結論

未發現需要阻擋合併的風險。

已檢查:

  • PR diff 與變更檔案範圍
  • PR mergeability:MERGEABLE
  • CodeRabbit check:pass / Review skipped
  • openspec validate --all --strict
  • _worker focused tests: ests/test_worker_converters.py tests/test_worker_batch_verification.py tests/test_worker_store.py
  • _worker touched Python compile
  • git diff --check origin/main...HEAD

注意:完整 _worker API tests 在目前全域 FastAPI/Starlette 相容性下無法完成 collection,已用 focused tests 覆蓋本次主要 runtime 修改面。GitHub 不允許同帳號 approve 自己的 PR,因此此留言作為本次 code review 記錄。

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