Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -567,12 +567,16 @@ def _ifc_artifact(event: Mapping[str, Any]) -> dict[str, Any]:
url = raw.get("url") or raw.get("file_url") or raw.get("signed_upload_reference")
if not url:
raise ValueError("ifc_artifact must include url, file_url, or signed_upload_reference.")
local_path = raw.get("local_path")
host_local_path = raw.get("host_local_path")
return {
"artifact_id": artifact_id,
"format": "ifc",
"filename": str(raw.get("filename") or "model.ifc"),
"url": url,
"checksum_sha256": raw.get("checksum_sha256"),
"local_path": str(local_path) if local_path else None,
"host_local_path": str(host_local_path) if host_local_path else None,
}


Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -55,6 +55,7 @@ def __init__(
config_path: Path | None = None,
timeout_seconds: int = 600,
work_dir: Path | None = None,
storage_root: Path | None = None,
) -> None:
self.repo_root = Path(repo_root)
self.powershell_exe = powershell_exe
Expand All @@ -64,6 +65,14 @@ def __init__(
self.timeout_seconds = int(timeout_seconds)
self.work_dir = Path(work_dir) if work_dir else self.repo_root
self.ps1_path = self.repo_root / "scripts" / "convert-ifc-to-usdc.ps1"
# streaming-server-prefer-local-ifc-path: shared volume sandbox base for
# dispatch payload host_local_path / local_path. Defaults to env STORAGE_ROOT
# (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 +74 to +75

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.

Comment on lines +70 to +75
Comment on lines +74 to +75

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

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 👍 / 👎.


# -- preflight -----------------------------------------------------------

Expand Down Expand Up @@ -162,6 +171,15 @@ def _resolve_local_ifc(self, ifc_ready_event: Mapping[str, Any]) -> Path:
raise ConversionAuthorityError(
"invalid_ifc_input", "ifc_ready event is missing ifc_artifact."
)
# streaming-server-prefer-local-ifc-path: 優先用 coordinator 寫到 shared volume
# 的 IFC,避免重複 HTTP fetch。host_local_path 是 host-native streaming-server
# 直接讀的 host fs path;local_path 在共享 fs 場景下與其同值,作為 fallback。
# 兩者都必須落在 storage_root 之內(防 path traversal)。
local = self._try_local_path(artifact.get("host_local_path"))
if local is None:
local = self._try_local_path(artifact.get("local_path"))
if local is not None:
return local
url = artifact.get("url") or artifact.get("file_url") or artifact.get(
"signed_upload_reference"
)
Expand All @@ -177,6 +195,38 @@ def _resolve_local_ifc(self, ifc_ready_event: Mapping[str, Any]) -> Path:
)
return local

def _try_local_path(self, candidate: Any) -> Path | None:
"""Resolve a dispatch-payload local path inside storage_root sandbox.

Returns the resolved Path if the file exists and lies inside
``self.storage_root``; returns None when the candidate is missing or
the file does not yet exist (soft fallback). Raises
``ConversionAuthorityError("invalid_ifc_input")`` when the resolved
path escapes ``storage_root`` (security hard-fail).
"""
if not candidate:
return None
raw = str(candidate).strip()
if not raw:
return None
path = Path(raw)
base = self.storage_root
resolved = (
path.resolve()
if path.is_absolute()
else (base / path).resolve()
)
Comment on lines +214 to +218

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 👍 / 👎.

try:
resolved.relative_to(base)
except ValueError as exc:
raise ConversionAuthorityError(
"invalid_ifc_input",
f"local IFC path is outside storage_root: {raw}",
) from exc
if not resolved.is_file():
return None
return resolved

def _url_to_local_path(self, url: str) -> Path | None:
parsed = urlparse(url)
scheme = (parsed.scheme or "").lower()
Expand Down
38 changes: 38 additions & 0 deletions bim-streaming-server/tests/test_conversion_authority_api.py
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@
from conversion_authority import ( # noqa: E402
ConversionAuthorityError,
ConversionAuthoritySettings,
_ifc_artifact,
create_conversion_api_app,
)

Expand Down Expand Up @@ -379,3 +380,40 @@ def test_coordinator_internal_request_failed_yields_failed_and_skipped_callback(
result = client.get(f"/api/conversions/{conversion_job_id}/result").json()
assert result["status"] == "failed"
assert result["authority"] == "bim-streaming-server"


# --- streaming-server-prefer-local-ifc-path:_ifc_artifact propagate local paths ----


def test_ifc_artifact_propagates_local_paths_when_present():
event = {
"ifc_artifact": {
"artifact_id": "artifact_local_propagate_001",
"format": "ifc",
"filename": "source.ifc",
"url": "edge-local://fixtures/source.ifc",
"local_path": "/workspace/storage/ifc-cache/job_x/source.ifc",
"host_local_path": "C:/host/storage/ifc-cache/job_x/source.ifc",
}
}

artifact = _ifc_artifact(event)

assert artifact["local_path"] == "/workspace/storage/ifc-cache/job_x/source.ifc"
assert artifact["host_local_path"] == "C:/host/storage/ifc-cache/job_x/source.ifc"


def test_ifc_artifact_local_paths_default_to_none_when_absent():
event = {
"ifc_artifact": {
"artifact_id": "artifact_local_propagate_002",
"format": "ifc",
"filename": "source.ifc",
"url": "edge-local://fixtures/source.ifc",
}
}

artifact = _ifc_artifact(event)

assert artifact["local_path"] is None
assert artifact["host_local_path"] is None
129 changes: 129 additions & 0 deletions bim-streaming-server/tests/test_host_native_conversion_service.py
Original file line number Diff line number Diff line change
Expand Up @@ -333,3 +333,132 @@ def test_adapter_from_env_explicit_powershell_wins(tmp_path: Path, monkeypatch):
)

assert adapter.powershell_exe == "powershell.exe"


# --- streaming-server-prefer-local-ifc-path:dispatch payload local path resolution -----


def _adapter_with_storage(tmp_path: Path, *, storage_root: Path | None = None) -> Ifc2UsdcPowershellConverterAdapter:
return Ifc2UsdcPowershellConverterAdapter(
repo_root=tmp_path,
storage_root=storage_root,
)


def test_adapter_resolve_prefers_host_local_path_inside_storage_root(tmp_path: Path):
storage = tmp_path / "storage"
ifc = storage / "ifc-cache" / "job_a" / "source.ifc"
ifc.parent.mkdir(parents=True)
ifc.write_bytes(b"IFC")
adapter = _adapter_with_storage(tmp_path, storage_root=storage)
event = ifc_ready_payload(
ifc_artifact={
"artifact_id": "artifact_local_001",
"format": "ifc",
"filename": "source.ifc",
"url": "http://127.0.0.1:9000/should-not-be-fetched.ifc",
"host_local_path": str(ifc),
}
)

resolved = adapter._resolve_local_ifc(event)

assert resolved == ifc.resolve()


def test_adapter_resolve_falls_back_to_local_path_when_host_local_path_missing(tmp_path: Path):
storage = tmp_path / "storage"
ifc = storage / "ifc-cache" / "job_b" / "source.ifc"
ifc.parent.mkdir(parents=True)
ifc.write_bytes(b"IFC")
adapter = _adapter_with_storage(tmp_path, storage_root=storage)
event = ifc_ready_payload(
ifc_artifact={
"artifact_id": "artifact_local_002",
"format": "ifc",
"filename": "source.ifc",
"url": "http://127.0.0.1:9000/should-not-be-fetched.ifc",
"local_path": str(ifc),
}
)

resolved = adapter._resolve_local_ifc(event)

assert resolved == ifc.resolve()


def test_adapter_resolve_falls_back_to_url_when_local_paths_unreadable(tmp_path: Path):
"""host_local_path/local_path 在 storage_root 內但檔案還沒寫好 → soft fallback url。"""
storage = tmp_path / "storage"
storage.mkdir()
# url 走 edge-local:// 解析(既有 _url_to_local_path 路徑)
fixture_root = tmp_path / "fixtures"
fixture_root.mkdir()
ifc = fixture_root / "fallback.ifc"
ifc.write_bytes(b"IFC")
adapter = Ifc2UsdcPowershellConverterAdapter(
repo_root=tmp_path,
storage_root=storage,
work_dir=tmp_path,
)
missing_local = storage / "ifc-cache" / "job_c" / "source.ifc" # 不存在
event = ifc_ready_payload(
ifc_artifact={
"artifact_id": "artifact_local_003",
"format": "ifc",
"filename": "fallback.ifc",
"url": "edge-local://fixtures/fallback.ifc",
"host_local_path": str(missing_local),
}
)

resolved = adapter._resolve_local_ifc(event)

assert resolved == ifc.resolve()


def test_adapter_resolve_rejects_local_path_outside_storage_root(tmp_path: Path):
storage = tmp_path / "storage"
storage.mkdir()
outside = tmp_path / "outside" / "secret.ifc"
outside.parent.mkdir()
outside.write_bytes(b"OUT")
adapter = _adapter_with_storage(tmp_path, storage_root=storage)
event = ifc_ready_payload(
ifc_artifact={
"artifact_id": "artifact_local_004",
"format": "ifc",
"filename": "secret.ifc",
"url": "edge-local://fixtures/demo-model.ifc",
"host_local_path": str(outside),
}
)

try:
adapter._resolve_local_ifc(event)
raised = None
except ConversionAuthorityError as exc:
raised = exc

assert raised is not None
assert raised.code == "invalid_ifc_input"
assert "outside storage_root" in raised.message


def test_adapter_resolve_existing_edge_local_url_still_works(tmp_path: Path):
"""Regression guard:legacy edge-local:// url-only payload(無 local_path/host_local_path)仍能解析。"""
storage = tmp_path / "storage"
storage.mkdir()
ifc = tmp_path / "fixtures" / "demo-model.ifc"
ifc.parent.mkdir()
ifc.write_bytes(b"IFC")
adapter = Ifc2UsdcPowershellConverterAdapter(
repo_root=tmp_path,
storage_root=storage,
work_dir=tmp_path,
)
event = ifc_ready_payload() # url=edge-local://fixtures/demo-model.ifc,無 local paths

resolved = adapter._resolve_local_ifc(event)

assert resolved == ifc.resolve()
Original file line number Diff line number Diff line change
@@ -0,0 +1,39 @@
# Acceptance — streaming-server-prefer-local-ifc-path

## L1 — Unit / pytest

- `cd bim-streaming-server && python -m pytest tests/test_conversion_authority_api.py -q` **PASS**(含新增 5 case)
- `cd bim-streaming-server && python -m pytest tests -q` **PASS**(整 suite regression)
- `cd bim-review-coordinator && npm run verify` **PASS**(11 files / 168 tests,不動 coordinator code)
- `python -m pytest tests -p no:cacheprovider` **PASS**(root contracts / fakes)

## L2 — OpenSpec validate

- `npx openspec validate streaming-server-prefer-local-ifc-path --strict` **valid**
- `npx openspec validate --specs --strict` **all passed**(本 change archive 之後仍綠)

## L3 — GitNexus

- Pre-impact:`_ifc_artifact` / `_resolve_local_ifc` / `_url_to_local_path` 的 upstream impact **LOW / MEDIUM**(內部 utility,d=1 callers 在同 module);任一 HIGH/CRITICAL 先回報
- Post-change `detect_changes`:staged file set = `conversion_authority.py` + `ifc2usdc_powershell_adapter.py` + `tests/test_conversion_authority_api.py` + openspec change folder(無額外飄逸 file)

## L4 — 真實 runtime end-to-end

- `docker rm -f` 既有 coordinator + viewer container
- `docker compose -p ai-bim-web-plane-host-kit ... up -d --build coordinator viewer`(用最新 main code)
- streaming-server host-native 重啟讀新 code
- Python urllib 等效 Postman ① ②(用 collection 預設值或使用者真實 URL):
- ① POST 預期 `HTTP 202 / dispatched / download_status:downloaded`(PR #94/#95 + 本 change 累積行為)
- ② Poll 預期 `conversion_status` 從 `queued` **離開 queued**(用 fallback-friendly fixture 至少能 fail with clear error;真實 URL 可達則 ready + viewer_url 出現)
- `docker exec coordinator ls /workspace/storage/ifc-cache/<jobId>/source.ifc` 確認 IFC bytes 真實落地(真實 URL 場景)
- streaming-server log 顯示 path resolution 結果走 host_local_path 分支

## L5 — UI(optional / nice-to-have)

- 若 conversion 真正 ready + viewer_url 出現:在 browser 開 `http://127.0.0.1:8004/ui/open?session=<lwv_id>` → 302 redirect 到 `127.0.0.1:5173/?session=<id>`,viewer 全螢幕 stream

## Stop conditions

任一 L1-L4 不 pass:stop,回報給人類。

不要為了 acceptance 而 hack(例如改 fixture 來 pass test、跳 sandboxing 來讓 path 通)。
Loading