Skip to content

streaming-server prefer local IFC path:接 host_local_path/local_path 走 shared volume - #96

Merged
monkey1sai merged 1 commit into
mainfrom
codex/openspec/streaming-server-prefer-local-ifc-path
May 22, 2026
Merged

monkey1sai merged 1 commit into
mainfrom
codex/openspec/streaming-server-prefer-local-ifc-path

Conversation

@monkey1sai

@monkey1sai monkey1sai commented May 22, 2026 •

Copy link
Copy Markdown
Owner

Summary

完整 OpenSpec change(streaming-server-prefer-local-ifc-path)。補實 fast-ifc-link-demo-loop (PR #92, archived 2026-05-21) spec 已宣告但 streaming-server 端未實作的 consumer 行為:streaming-server SHALL prefer host_local_path / local_path over url。

PR #94 / #95 修好 coordinator 端的 ENOTDIR + compose shared volume mount;2026-05-22 重跑 Postman 驗證 IFC 已正確落到 shared volume,但 streaming-server 仍走 ifc_artifact.url HTTP fetch,conversion_status 卡 queued 不前進。本 PR 讓 streaming-server 真實消費 dispatch payload 的 local path 欄位。

What changed(9 files / +593 lines)

Streaming-server source(2 files / +54)

  • conversion_authority._ifc_artifact:return dict propagate local_path / host_local_path
  • ifc2usdc_powershell_adapter.Ifc2UsdcPowershellConverterAdapter:
    • __init__ 加 storage_root: Path | None(resolve env STORAGE_ROOT 或 cwd)
    • _resolve_local_ifc 新解析順序:host_local_path → local_path → 既有 url fallback
    • 新 helper _try_local_path:storage_root sandbox(path traversal hard-fail)+ 不存在 soft fallback

Streaming-server tests(2 files / +167)

  • adapter _resolve_local_ifc 5 case:host 優先 / local 後備 / url soft-fallback / outside 拒絕 / regression edge-local://
  • _ifc_artifact 2 case:propagate / default None

OpenSpec change scaffold(5 files / +372)

  • proposal / design / tasks / acceptance / conversion-webhook-lifecycle spec delta(## ADDED Requirements + 3 Scenarios)

GitNexus blast radius = LOW

symbol risk d=1
_ifc_artifact LOW StreamingConversionStore.create_conversion_job(同 module)
_resolve_local_ifc LOW Ifc2UsdcPowershellConverterAdapter.convert(同 class)
_url_to_local_path LOW _resolve_local_ifc(同 class)

Verification

Level Result
L1 streaming-server pytest 31 passed(原 23 + 新 8)
L1 coordinator npm run verify 168 passed(regression OK)
L1 root pytest tests 9 passed
L2 openspec validate streaming-server-prefer-local-ifc-path --strict valid
L2 openspec validate --specs --strict 26 passed / 0 failed
L3 GitNexus pre-impact LOW
L4 真實 runtime 跳過 — 需重啟既有 host-native streaming-server(:49101);merge 後重啟 + 重跑 Postman ① ② 驗證 conversion_status → ready + viewer_url 出現

Design highlights

詳見 openspec/changes/streaming-server-prefer-local-ifc-path/design.md。重點:

  • host_local_path 優先 over local_path:streaming-server 為 host-native(per AGENTS.md §3.5),看 host fs;host_local_path 直接可用,local_path 在 host-native 場景通常同值,當 fallback
  • storage_root sandboxing:防止 coordinator(或偽造 event)寫 host_local_path: "C:\Windows\System32\..." 讓 streaming-server 餵任意檔給 Kit
  • soft vs hard fail:
    • 路徑在 storage_root 外 → hard raise invalid_ifc_input(security)
    • 路徑在 storage_root 內但 file 不存在 → soft fallback 下一順位(race condition 友善)
  • 不引入 HTTP fetch:streaming-server 端 HTTP download 引入 retry / timeout / cert 等複雜度,超出本 change scope;fast-mvp 場景 coordinator 已負責 HTTP download

Backward compatibility

  • legacy file:// / edge-local:// url-only payload → 沒 local_path → fallback 既有 _url_to_local_path → 不變(test regression case 已 cover)
  • coordinator 不帶 local_path / host_local_path(test fake、舊 caller)→ fallback 既有 url → 不變
  • 既有 31 streaming-server tests + 168 coordinator tests + 9 root tests 全綠

Spec / Capability impact

conversion-webhook-lifecycle capability:## ADDED Requirements 加 1 個 requirement Streaming-server consumes shared-volume local IFC path before url fetch(3 Scenarios)。archive 時 sync 進 main spec body。

注意(follow-up)

archive 本 PR 時,順手補 main spec body 缺少的 fast-ifc-link-demo-loop 那個 Coordinator dispatch payload carries local path references requirement entry(PR #93 archive sync 只加了 implementation status note 沒搬 requirement body),讓主 spec 完整反映兩個 change 的累積宣告。

Test plan

  • L1 全綠
  • L2 spec validate 全綠
  • L3 GitNexus impact LOW
  • CI green
  • Reviewer approve
  • merge
  • (post-merge)重啟 streaming-server + 重跑 Postman → 確認 viewer_url 終於出現
  • sync + archive PR

🤖 Generated with Claude Code

Summary by CodeRabbit

Release Notes

  • New Features

    • IFC conversion now prioritizes local filesystem paths when available, falling back to URL resolution only when needed. This improves performance by eliminating unnecessary network transfers while maintaining backward compatibility.
  • Tests

    • Added comprehensive test coverage for local path resolution behavior, priority ordering, fallback logic, and path security enforcement.

Review Change Stack

… shared volume (streaming-server-prefer-local-ifc-path)

完整 OpenSpec change(scaffold + apply 同 commit;模仿 fast-ifc-link-demo-loop PR #92 pattern)。

## Why

fast-ifc-link-demo-loop(PR #92,archived 2026-05-21)宣告 coordinator → streaming-server
dispatch payload 加 `local_path` / `host_local_path`,並寫進 spec delta:**streaming-server
SHALL prefer host_local_path when present...**。但 streaming-server 端**完全沒實作**這個
consumer 行為(`conversion_authority._ifc_artifact` 只認 url,`ifc2usdc_powershell_adapter._url_to_local_path`
HTTP scheme 直接 fail)。

PR #94/#95 修好 coordinator 端(host-native default + compose env);2026-05-22 重跑 Postman
證實 IFC 已正確落到 shared volume,但 `conversion_status` 卡 queued、`viewer_url: null`。
本 change 補完 streaming-server consumer drift,讓 fast-mvp happy path 真實跑通。

## What changed

### Streaming-server source(2 files / +54 lines)

- `conversion_authority._ifc_artifact`:return dict propagate `local_path` / `host_local_path`
  (lineage / debug visibility)
- `ifc2usdc_powershell_adapter.Ifc2UsdcPowershellConverterAdapter`:
  - `__init__` 加 `storage_root: Path | None`,resolve env `STORAGE_ROOT` 或 cwd
  - `_resolve_local_ifc` 新解析順序:`host_local_path` → `local_path` → 既有 url 解析
  - 新 helper `_try_local_path`:storage_root sandbox(path traversal hard-fail)+ 不存在 soft fallback

### Streaming-server tests(2 files / +167 lines)

- `test_host_native_conversion_service.py`:加 5 個 adapter `_resolve_local_ifc` test
  - host_local_path 在 storage_root 內 + 存在 → 直接用,跳過 url
  - local_path 為 fallback(host_local_path 不在時)
  - 兩者 path 在 storage_root 內但檔案不存在 → soft fallback url(edge-local://)
  - host_local_path 在 storage_root 外 → raise `invalid_ifc_input`
  - regression:既有 edge-local:// url-only payload 仍能解析
- `test_conversion_authority_api.py`:加 2 個 `_ifc_artifact` propagation test
  - local_path / host_local_path 存在時 propagate 進 return dict
  - 不存在時 default None

### OpenSpec change(5 files / +372 lines)

`openspec/changes/streaming-server-prefer-local-ifc-path/`:
- `proposal.md`:why / what / non-goals / capability impact
- `design.md`:resolution order / sandboxing / backward compat / failure semantics
- `tasks.md`:12 phase tasks(0 setup → 11 archive)
- `acceptance.md`:L1-L5 acceptance
- `specs/conversion-webhook-lifecycle/spec.md`:`## ADDED Requirements`
  `Streaming-server consumes shared-volume local IFC path before url fetch` + 3 Scenarios

## Blast radius

GitNexus pre-impact:`_ifc_artifact` / `_resolve_local_ifc` / `_url_to_local_path` 全 **LOW**
(direct d=1 callers 在同 module/class,affected processes = `create_conversion` + `convert`,
都是預期內)

## Verification(L1-L3)

| Level | Result |
|---|---|
| L1 `cd bim-streaming-server && pytest tests -q` | **31 passed**(原 23 + 新 8 cases) |
| L1 `cd bim-review-coordinator && npm run verify` | 11 files / **168 tests passed**(regression OK,coordinator side 不動) |
| L1 root `pytest tests -p no:cacheprovider` | **9 passed**(contracts / fakes regression OK) |
| L2 `openspec validate streaming-server-prefer-local-ifc-path --strict` | **valid** |
| L2 `openspec validate --specs --strict` | 26 passed / 0 failed |
| L3 GitNexus impact (pre-change) | LOW for `_ifc_artifact` / `_resolve_local_ifc` / `_url_to_local_path` |
| L4 真實 runtime end-to-end | **跳過** — 需重啟既有 host-native streaming-server(:49101),建議 merge 後重啟並重跑 Postman 驗證 conversion_status → ready + viewer_url 出現 |

## Predecessor / Follow-up

✓ Predecessor:`fast-ifc-link-demo-loop`(PR #92, archived PR #93) + hotfix PR #94(coordinator host-native fallback)+ PR #95(compose env)
✗ 仍 open follow-up(本 change 不做):
- streaming-server docker deployment 的 dual-fs 議題(scope = host-native fast MVP only)
- `main spec body` 缺少 fast-ifc-link-demo-loop 的 `Coordinator dispatch payload carries local path references` requirement entry(archive PR #93 只加了 implementation status note;archive 本 change 時可順手補)

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Copilot AI review requested due to automatic review settings May 22, 2026 03:56
@coderabbitai

coderabbitai Bot commented May 22, 2026 •

Copy link
Copy Markdown
Contributor
📝 Walkthrough

Walkthrough

This pull request implements local IFC path preference in the streaming-server conversion workflow. The server now attempts to resolve IFC sources from dispatch payload's local_path and host_local_path fields before falling back to URL-based retrieval, enforcing path sandboxing via storage_root to prevent directory traversal escapes.

Changes

Streaming-Server Local IFC Path Preference

Layer / File(s) Summary
Artifact path propagation
bim-streaming-server/source/extensions/ezplus.bim_review_stream.messaging/.../conversion_authority.py, bim-streaming-server/tests/test_conversion_authority_api.py
_ifc_artifact helper now reads local_path and host_local_path fields from incoming ifc_ready event and returns them in the artifact dict, enabling downstream path resolution logic to access local filesystem metadata.
Local IFC resolution with storage_root sandbox
bim-streaming-server/source/extensions/ezplus.bim_review_stream.messaging/.../ifc2usdc_powershell_adapter.py
Ifc2UsdcPowershellConverterAdapter accepts optional storage_root parameter (deriving from STORAGE_ROOT env or current directory if not provided). _resolve_local_ifc() prefers host_local_path, falls back to local_path, each validated via new _try_local_path() helper that hard-fails on paths escaping storage_root and soft-falls back on missing files. Retains URL resolution fallback when local paths are unavailable.
Resolution behavior and security tests
bim-streaming-server/tests/test_host_native_conversion_service.py
Five new test cases cover _resolve_local_ifc priority (host_local_path > local_path), soft fallback to URL resolution when local paths are missing/unreadable, hard rejection of paths outside storage_root with ConversionAuthorityError("invalid_ifc_input"), and regression guard ensuring legacy edge-local:// URL payloads still resolve correctly.
Specification, design, and acceptance documentation
openspec/changes/streaming-server-prefer-local-ifc-path/*
OpenSpec artifacts define design rationale, resolution semantics, path sandboxing failure modes, formal spec requirement, acceptance verification steps, and implementation tasks. Spec delta adds mandatory requirement for local-path-before-url resolution with STORAGE_ROOT confinement and three scenario blocks covering preference, fallback, and sandbox-escape rejection.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Poem

🐰 A rabbit hops down local paths with glee,
Checking storage_root boundaries carefully!
No escapes allowed—security won first place,
While URLs wait as backup for the race. 🏃‍♂️✨

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 20.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 references the main feature (prefer local IFC path via host_local_path/local_path on shared volume), matching the core change across 9 files (+593 lines) that implements this priority-based resolution behavior.
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 docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch codex/openspec/streaming-server-prefer-local-ifc-path

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

🧹 Nitpick comments (3)
openspec/changes/streaming-server-prefer-local-ifc-path/design.md (2)

30-30: ⚡ Quick win

Remove unused work_dir reference in pseudo-code.

Line 30 assigns work_dir = self.work_dir but this variable is never used in the resolution logic that follows. Per section 6 (line 94-100), work_dir sandboxing applies only to url-derived paths via _anchor, not to the _try_local path shown here.

🧹 Proposed fix
 def _resolve_local_ifc(event):
     artifact = event["ifc_artifact"]
     storage_root = self.storage_root  # absolute, from env STORAGE_ROOT or cwd
-    work_dir = self.work_dir
🤖 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/streaming-server-prefer-local-ifc-path/design.md` at line
30, Remove the unused local binding work_dir = self.work_dir from the
pseudo-code: locate the assignment to work_dir in the resolution routine and
delete it so the function no longer creates an unused variable; ensure the logic
that handles `_try_local` remains unchanged and that `_anchor`/URL-derived
sandboxing still references self.work_dir where needed (e.g., in `_anchor`
handling), leaving functions/methods named `_try_local` and `_anchor` intact.

26-70: ⚡ Quick win

Add language specifier to fenced code block.

The pseudo-code block lacks a language identifier, triggering a markdown linter warning. Consider adding python or text to improve tooling compliance.

📝 Proposed fix
-```
+```python
 def _resolve_local_ifc(event):

As per coding guidelines, static analysis hints should be addressed when valid.

🤖 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/streaming-server-prefer-local-ifc-path/design.md` around
lines 26 - 70, The fenced code block in design.md lacks a language specifier
causing linter warnings; update the opening fence to include a language (e.g.,
change ``` to ```python) so the pseudocode for _resolve_local_ifc and _try_local
is marked as Python and linting/static analysis tools recognize it.
openspec/changes/streaming-server-prefer-local-ifc-path/tasks.md (1)

27-31: 💤 Low value

Consider clarifying the storage_root initialization logic.

Line 31's nested expression Path(os.environ.get("STORAGE_ROOT") or Path.cwd()) works correctly but is slightly awkward because Path() may receive either a string or a Path object depending on whether the environment variable is set.

While functionally correct, a clearer alternative would separate the environment check from the Path construction:

self.storage_root = (
    storage_root or 
    (Path(os.environ["STORAGE_ROOT"]) if "STORAGE_ROOT" in os.environ else Path.cwd())
).resolve()

This is optional—the current version is valid Python and will work as intended.

🤖 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/streaming-server-prefer-local-ifc-path/tasks.md` around
lines 27 - 31, The storage_root initialization in
Ifc2UsdcPowerShellAdapter.__init__ is ambiguous because Path(...) may be given a
str or Path; change the logic to first check for a provided storage_root, then
check os.environ for "STORAGE_ROOT", and only then construct a Path from the
determined string/Path before calling .resolve(); update the assignment to
self.storage_root to reflect this clearer three-step flow so the code
consistently constructs a Path from the chosen source.
🤖 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
`@bim-streaming-server/source/extensions/ezplus.bim_review_stream.messaging/ezplus/bim_review_stream/messaging/ifc2usdc_powershell_adapter.py`:
- Around line 74-75: The adapter currently reads STORAGE_ROOT directly from
os.environ which prevents adapter_from_env(env=...) from controlling it; update
the storage root resolution in ifc2usdc_powershell_adapter.py so that it first
checks the injected env parameter (the env dict passed into adapter_from_env),
then falls back to os.environ, and finally to Path.cwd(); set self.storage_root
accordingly (replacing the current env_root lookup and Path.cwd() fallback) so
adapter_from_env(env=...) can override STORAGE_ROOT when provided.

---

Nitpick comments:
In `@openspec/changes/streaming-server-prefer-local-ifc-path/design.md`:
- Line 30: Remove the unused local binding work_dir = self.work_dir from the
pseudo-code: locate the assignment to work_dir in the resolution routine and
delete it so the function no longer creates an unused variable; ensure the logic
that handles `_try_local` remains unchanged and that `_anchor`/URL-derived
sandboxing still references self.work_dir where needed (e.g., in `_anchor`
handling), leaving functions/methods named `_try_local` and `_anchor` intact.
- Around line 26-70: The fenced code block in design.md lacks a language
specifier causing linter warnings; update the opening fence to include a
language (e.g., change ``` to ```python) so the pseudocode for
_resolve_local_ifc and _try_local is marked as Python and linting/static
analysis tools recognize it.

In `@openspec/changes/streaming-server-prefer-local-ifc-path/tasks.md`:
- Around line 27-31: The storage_root initialization in
Ifc2UsdcPowerShellAdapter.__init__ is ambiguous because Path(...) may be given a
str or Path; change the logic to first check for a provided storage_root, then
check os.environ for "STORAGE_ROOT", and only then construct a Path from the
determined string/Path before calling .resolve(); update the assignment to
self.storage_root to reflect this clearer three-step flow so the code
consistently constructs a Path from the chosen source.
🪄 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: 78763a22-cb94-4c09-bfab-772c01b06f65

📥 Commits

Reviewing files that changed from the base of the PR and between 12e883d and c0567af.

📒 Files selected for processing (9)
  • bim-streaming-server/source/extensions/ezplus.bim_review_stream.messaging/ezplus/bim_review_stream/messaging/conversion_authority.py
  • bim-streaming-server/source/extensions/ezplus.bim_review_stream.messaging/ezplus/bim_review_stream/messaging/ifc2usdc_powershell_adapter.py
  • bim-streaming-server/tests/test_conversion_authority_api.py
  • bim-streaming-server/tests/test_host_native_conversion_service.py
  • openspec/changes/streaming-server-prefer-local-ifc-path/acceptance.md
  • openspec/changes/streaming-server-prefer-local-ifc-path/design.md
  • openspec/changes/streaming-server-prefer-local-ifc-path/proposal.md
  • openspec/changes/streaming-server-prefer-local-ifc-path/specs/conversion-webhook-lifecycle/spec.md
  • openspec/changes/streaming-server-prefer-local-ifc-path/tasks.md

Comment on lines +74 to +75
env_root = os.environ.get("STORAGE_ROOT")
self.storage_root = (Path(env_root) if env_root else Path.cwd()).resolve()

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

Honor injected env for STORAGE_ROOT when building the adapter.

Line 74 reads process env directly, so adapter_from_env(env=...) cannot control STORAGE_ROOT. This can silently pick cwd as sandbox root and misclassify valid shared-volume paths.

Proposed fix
 def adapter_from_env(repo_root: Path, env: Mapping[str, str] | None = None):
@@
     return Ifc2UsdcPowershellConverterAdapter(
         repo_root=Path(repo_root),
         powershell_exe=src.get("STREAMING_CONVERSION_POWERSHELL_EXE") or _default_powershell_exe(),
         kit_exe_path=_path("STREAMING_CONVERSION_KIT_EXE"),
         hoops_main_path=_path("STREAMING_CONVERSION_HOOPS_MAIN"),
         config_path=_path("STREAMING_CONVERSION_CONFIG_PATH"),
         timeout_seconds=timeout,
         work_dir=_path("STREAMING_CONVERSION_WORK_DIR"),
+        storage_root=_path("STORAGE_ROOT"),
     )
🤖 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
`@bim-streaming-server/source/extensions/ezplus.bim_review_stream.messaging/ezplus/bim_review_stream/messaging/ifc2usdc_powershell_adapter.py`
around lines 74 - 75, The adapter currently reads STORAGE_ROOT directly from
os.environ which prevents adapter_from_env(env=...) from controlling it; update
the storage root resolution in ifc2usdc_powershell_adapter.py so that it first
checks the injected env parameter (the env dict passed into adapter_from_env),
then falls back to os.environ, and finally to Path.cwd(); set self.storage_root
accordingly (replacing the current env_root lookup and Path.cwd() fallback) so
adapter_from_env(env=...) can override STORAGE_ROOT when provided.

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 implements the streaming-server consumer behavior promised by the earlier fast-mvp loop work: prefer ifc_artifact.host_local_path / ifc_artifact.local_path from the dispatch payload (shared volume) before falling back to legacy URL-based resolution, with path sandboxing to mitigate traversal.

Changes:

  • Propagate local_path / host_local_path through conversion_authority._ifc_artifact.
  • Add storage_root + sandboxed local-path resolution (host_local_path → local_path → legacy URL parsing) in Ifc2UsdcPowershellConverterAdapter.
  • Add pytest coverage for local-path propagation and resolution order, plus OpenSpec change scaffold & spec delta.

Reviewed changes

Copilot reviewed 9 out of 9 changed files in this pull request and generated 2 comments.

Show a summary per file
File Description
bim-streaming-server/source/extensions/ezplus.bim_review_stream.messaging/ezplus/bim_review_stream/messaging/conversion_authority.py _ifc_artifact now carries through local_path/host_local_path fields.
bim-streaming-server/source/extensions/ezplus.bim_review_stream.messaging/ezplus/bim_review_stream/messaging/ifc2usdc_powershell_adapter.py Adds storage_root and resolves local IFC paths with sandboxing and fallback behavior.
bim-streaming-server/tests/test_host_native_conversion_service.py New unit tests for adapter resolution order, fallback, and sandbox rejection.
bim-streaming-server/tests/test_conversion_authority_api.py Tests that _ifc_artifact propagates local-path fields and defaults them to None.
openspec/changes/streaming-server-prefer-local-ifc-path/proposal.md Documents motivation/scope for implementing local-path preference in streaming-server.
openspec/changes/streaming-server-prefer-local-ifc-path/design.md Details resolution order and sandboxing rationale.
openspec/changes/streaming-server-prefer-local-ifc-path/tasks.md Execution checklist for implementing/verifying the change.
openspec/changes/streaming-server-prefer-local-ifc-path/acceptance.md Acceptance criteria for tests, spec validation, and runtime verification.
openspec/changes/streaming-server-prefer-local-ifc-path/specs/conversion-webhook-lifecycle/spec.md Spec delta: adds requirement/scenarios for preferring shared-volume local paths.

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

Comment on lines +70 to +75
# (compose 對齊),or cwd 作為 host-native fallback。
if storage_root is not None:
self.storage_root = Path(storage_root).resolve()
else:
env_root = os.environ.get("STORAGE_ROOT")
self.storage_root = (Path(env_root) if env_root else Path.cwd()).resolve()
Comment on lines +17 to +40
- [ ] 1.4 `gitnexus_impact({target:"Ifc2UsdcPowerShellAdapter", direction:"upstream"})`
- [ ] 1.5 任一回 HIGH/CRITICAL → stop,回報後等使用者裁定

## 2. conversion_authority._ifc_artifact propagate local paths

- [ ] 2.1 `bim-streaming-server/.../conversion_authority.py` `_ifc_artifact`:
- return dict 加 `local_path: str | None = raw.get("local_path")`
- return dict 加 `host_local_path: str | None = raw.get("host_local_path")`
- 不驗證(留給 adapter `_resolve_local_ifc` sandbox)

## 3. Ifc2UsdcPowerShellAdapter storage_root config

- [ ] 3.1 `bim-streaming-server/.../ifc2usdc_powershell_adapter.py` `Ifc2UsdcPowerShellAdapter.__init__`:
- 加 `storage_root: Path | None = None` 參數
- 內部 resolve:`self.storage_root = (storage_root or Path(os.environ.get("STORAGE_ROOT") or Path.cwd())).resolve()`

## 4. Adapter _resolve_local_ifc new resolution order

- [ ] 4.1 加 helper `_try_local(self, candidate: str | None) -> Path | None`:
- None / empty → None
- relative → join `self.storage_root`
- absolute → resolve
- 必須 `relative_to(self.storage_root)`,否則 raise `ConversionAuthorityError("invalid_ifc_input", "local IFC path is outside storage_root: ...")`
- `is_file()` 才回傳;否則 None(soft fallback)
@monkey1sai
monkey1sai merged commit 3c7dbd5 into main May 22, 2026
5 checks passed
@monkey1sai
monkey1sai deleted the codex/openspec/streaming-server-prefer-local-ifc-path branch May 22, 2026 04:02

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

ℹ️ 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 +74 to +75
env_root = os.environ.get("STORAGE_ROOT")
self.storage_root = (Path(env_root) if env_root else Path.cwd()).resolve()

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 Anchor default storage_root to repo root, not process cwd

When storage_root is omitted, the new default Path.cwd() makes path validation depend on how the service was launched. The documented host-native launcher (scripts/start-host-native-conversion-service.ps1) Push-Locations into the messaging module directory, so self.storage_root becomes that module path instead of <repo>/storage. In that common setup, coordinator-dispatched absolute host_local_path values under <repo>/storage/ifc-cache/... are treated as outside storage_root and _try_local_path raises invalid_ifc_input before URL fallback, causing conversion jobs to fail unless STORAGE_ROOT is manually set.

Useful? React with 👍 / 👎.

Comment on lines +74 to +75
env_root = os.environ.get("STORAGE_ROOT")
self.storage_root = (Path(env_root) if env_root else Path.cwd()).resolve()

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 Respect injected env when resolving storage_root

adapter_from_env(env=...) is intended to build configuration from the provided mapping, but the new storage_root path is not passed through and __init__ reads os.environ directly instead. As a result, callers/tests that provide {"STORAGE_ROOT": ...} via the env argument still get a cwd-based sandbox, which can incorrectly reject valid host_local_path inputs as outside storage_root and makes behavior differ between injected-config runs and real process env runs.

Useful? React with 👍 / 👎.

Comment on lines +214 to +218
resolved = (
path.resolve()
if path.is_absolute()
else (base / path).resolve()
)

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 Normalize local-path resolve failures to invalid_ifc_input

The new local-path branch calls Path.resolve() on untrusted host_local_path/local_path values without handling resolver errors, so filesystem issues (e.g., symlink loops on Python 3.12, permission errors on intermediate directories) escape as generic exceptions instead of the intended ConversionAuthorityError("invalid_ifc_input", ...). That changes failure classification for the same bad input class, which can break downstream retry/debug logic keyed on invalid_ifc_input and makes behavior inconsistent with the explicit validation path.

Useful? React with 👍 / 👎.

monkey1sai added a commit that referenced this pull request May 22, 2026
…ecs + roadmap (#97)

PR #96(implementation,squash `3c7dbd5`)merged 後的 OpenSpec sync/archive cleanup。

- `git mv openspec/changes/streaming-server-prefer-local-ifc-path/` → `openspec/changes/archive/2026-05-22-streaming-server-prefer-local-ifc-path/`
- `openspec/specs/conversion-webhook-lifecycle/spec.md` 加 2 個 ADD requirement:
  1. `Coordinator dispatch payload carries local path references`(backfill `fast-ifc-link-demo-loop` archive PR #93 缺漏的 requirement body,3 Scenarios)
  2. `Streaming-server consumes shared-volume local IFC path before url fetch`(本 change 新 ADD,3 Scenarios)
  + 對應的 implementation status note(2026-05-21 與 2026-05-22 兩段)
- `docs/plans/AI-BIM-governance-saas-roadmap-2026-05.md` 加 archive 摘要(2026-05-22 fast-mvp loop final landing,含 hotfix bundle PR #94 / #95 / #96)

`openspec validate --specs --strict` = **26 passed / 0 failed**

## fast-mvp loop 收尾完成

兩段 OpenSpec change archive(`remove-conflict-review-from-fast-mvp` + `fast-ifc-link-demo-loop`)+ 三段 hotfix(PR #94 coordinator default、PR #95 compose env、PR #96 streaming-server consumer),把外部 IFC Worker → coordinator → streaming-server → viewer 連結這條 happy path 從 spec 到 code 完整對齊。

L4 真實 happy path 需 user 設 `.env` `RUNTIME_STORAGE_ROOT=<host absolute path>`、recreate coordinator container、重啟 host-native streaming-server 帶 `STORAGE_ROOT=<host absolute path>`,本 archive 不包含此環境設定步驟(留給 user runbook)。

Co-authored-by: Claude Opus 4.7 <noreply@anthropic.com>
monkey1sai added a commit that referenced this pull request May 22, 2026
…_url(coordinator-auto-poll-streaming-conversion) (#98)

完整 OpenSpec change(scaffold + apply 同 commit)。

## Why

fast-mvp loop 三段 archive 收尾後驗證,`POST /api/external/ifc-ready` → coordinator dispatch → streaming-server 真實轉檔 succeeded(40s)— **但 coordinator 端 `conversion_status` 永遠停在 `queued`**:既有 spec `Coordinator ingests host-native conversion result into callback outbox` 已寫「SHALL ingest through polling, an internal result loop, or an equivalent internal callback」,但 polling / internal result loop 都未實作。`fetchConversionResult` 只在 manual `POST /api/internal/conversions/<id>/ingest` 端點被呼叫;沒有自動 caller,viewer_url 永不出現。

## What changed(10 files / +968 / -50)

### Coordinator src(3 files)
- `services/streamingConversionClient.ts`:加 module-level `isTerminalConversionResult` helper(terminal 判定抽出來避免雙處硬編)+ class method `pollConversionResult(jobId, options): PollerHandle`:setTimeout chain(避免 setInterval overlap)、cancel()、max attempts → poll_timeout 走 fake failed result 餵 onTerminal
- `config.ts`:加 `conversionPollEnabled` / `conversionPollIntervalSeconds`(default 5)/ `conversionPollMaxAttempts`(default 60 = 5 min)+ `parseBooleanEnv` helper
- `app.ts`:
  - `CoordinatorApp` interface 加 `dispose: () => void`
  - 加 module-scope `pollerRegistry: Map<conversion_job_id, PollerHandle>`
  - refactor 既有 manual ingest handler 內邏輯抽成共用 helper `ingestStreamingConversionResult(jobId, { result?, source })`(manual + auto-poll 共用)
  - 加 `schedulePollerForConversion(jobId)`:closure wrapper for `streamingConversionClient.pollConversionResult` + onTerminal=ingest helper + registry cleanup
  - dispatch path:`markDispatched` 之後若 `config.conversionPollEnabled && !pollerRegistry.has(id)` → `schedulePollerForConversion(id)`
  - manual ingest handler 開頭 cancel + delete registry(避免雙 ingest)
  - return 加 `dispose`:遍歷 registry cancel 所有 timer

### Coordinator tests(2 files)
- `unit_kitpool.test.ts`:fixture 加 3 個新 config field(default `false` 避免 unit test 啟 timer)
- `auto-poll-conversion.test.ts`(新):5 case
  - dispatch → poller queued/queued/ready 序列 → 自動 ingest 出 viewer_url
  - dispatch → poller queued/failed → 自動 ingest 為 failed,viewer_url 不出現
  - 重複 idempotent dispatch 不雙起 poller(stub /result 呼叫次數有 budget)
  - manual ingest 觸發 cancel auto poller(stub /result 只被打 1 次)
  - `conversionPollEnabled: false` fixture 不啟 poller

### OpenSpec change(5 files)
- proposal / design / tasks / acceptance / `conversion-webhook-lifecycle` MODIFIED requirement + 3 新 Scenario

## GitNexus blast radius = LOW

| symbol | risk | d=1 |
|---|---|---|
| `fetchConversionResult` | LOW | `createCoordinatorApp`(同 module) |
| `createCoordinatorApp` | LOW | 無 upstream(main entry) |
| `loadConfig` | LOW | (PR #94 同樣 path,LOW) |

## Verification

| Level | Result |
|---|---|
| L1 coordinator `npm run verify` | 12 files / **173 tests passed**(168 既有 + 5 新 auto-poll) |
| L1 streaming-server `pytest tests -q` | **31 passed**(不動,regression OK) |
| L1 root `pytest tests` | **9 passed** |
| L2 `openspec validate coordinator-auto-poll-streaming-conversion --strict` | **valid** |
| L2 `openspec validate --specs --strict` | **26 passed / 0 failed** |
| L3 GitNexus pre-impact | LOW for `fetchConversionResult` / `createCoordinatorApp` |
| **L4 真實 runtime** | **跳過** — 留 merge 後 docker compose recreate coordinator(讀新 code)+ 跑 Postman ① + 等(不手動 POST ingest)→ ② Poll 預期 5-90 秒內 `viewer_url` 自動出現 |

## Predecessor / Follow-up

✓ Predecessor:`streaming-server-prefer-local-ifc-path`(PR #96 / archive PR #97)+ PR #94 / PR #95 hotfix bundle
本 change 是 fast-mvp loop 自動化的最後一片拼圖,merge 後外部 caller 不再需要任何手動 trigger 就能拿到 viewer_url。

不解 / 排除:
- 不持久化 poller state(coordinator restart in-memory timers lost;手動 endpoint 仍可救)
- 不解 cloud callback outbox retry / dead-letter(另一條既有 capability,本 change 不動)
- 不引入 streaming-server push callback(本 change 走 coordinator pull)
- 不引入第三方 scheduler library

Co-authored-by: Claude Opus 4.7 <noreply@anthropic.com>
monkey1sai added a commit that referenced this pull request May 22, 2026
…步 specs + roadmap (#99)

PR #98(implementation,squash `2103bf7`)merged 後的 OpenSpec sync/archive cleanup。

- `git mv openspec/changes/coordinator-auto-poll-streaming-conversion/` → `openspec/changes/archive/2026-05-22-coordinator-auto-poll-streaming-conversion/`
- `openspec/specs/conversion-webhook-lifecycle/spec.md` MODIFIED `Coordinator ingests host-native conversion result into callback outbox` requirement:加 SHALL auto-poll 子條款(env config 三項可調)+ 3 新 Scenario(dispatch auto-schedules / manual de-dup / poll timeout failed-equivalent)+ implementation status note
- `docs/plans/AI-BIM-governance-saas-roadmap-2026-05.md` 加 archive 摘要(2026-05-22 fast-mvp loop zero-touch automation 完成)

`openspec validate --specs --strict` = **26 passed / 0 failed**

## fast-mvp loop zero-touch 收尾完成

4 段 OpenSpec change archive(`remove-conflict-review-from-fast-mvp` + `fast-ifc-link-demo-loop` + `streaming-server-prefer-local-ifc-path` + `coordinator-auto-poll-streaming-conversion`)+ 4 段 hotfix bundle(PR #94 / #95 / #96 / #98)整體把外部 IFC Worker → coordinator → streaming-server → viewer 連結這條 happy path 從 spec 到 code、從 manual 到 zero-touch 完整對齊並驗證。

L4 真實 runtime 第一次 zero-touch end-to-end 跑通(2026-05-22):dispatch → 40 秒內 viewer_url 自動出現,無 manual ingest trigger。

Co-authored-by: Claude Opus 4.7 <noreply@anthropic.com>
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