Skip to content

coordinator auto-poll streaming conversion:dispatch 後自動 ingest viewer_url - #98

Merged
monkey1sai merged 1 commit into
mainfrom
codex/openspec/coordinator-auto-poll-streaming-conversion
May 22, 2026
Merged

monkey1sai merged 1 commit into
mainfrom
codex/openspec/coordinator-auto-poll-streaming-conversion

Conversation

@monkey1sai

@monkey1sai monkey1sai commented May 22, 2026 •

Copy link
Copy Markdown
Owner

Summary

完整 OpenSpec change(coordinator-auto-poll-streaming-conversion)。補實既有 Coordinator ingests host-native conversion result into callback outbox requirement 已寫但未實作的「polling / internal result loop」gap。

fast-mvp loop archive 三段(remove-conflict-review-from-fast-mvp / fast-ifc-link-demo-loop / streaming-server-prefer-local-ifc-path)+ hotfix 三段(PR #94 / #95 / #96)收尾後驗證,coordinator 仍要外部手動 POST /api/internal/conversions/<id>/ingest 才能拿到 viewer_url。本 PR 加 in-process auto-poller 完成自動化,merge 後 fast-mvp happy path 真正 zero-touch。

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

Coordinator src

  • streamingConversionClient.ts:isTerminalConversionResult helper + pollConversionResult(jobId, options): PollerHandle(setTimeout chain、cancel()、max attempts → poll_timeout fake failed result)
  • config.ts:conversionPollEnabled / conversionPollIntervalSeconds(5)/ conversionPollMaxAttempts(60)+ parseBooleanEnv
  • app.ts:pollerRegistry、refactor manual ingest 邏輯成共用 helper、dispatch 後 schedule、manual ingest 開頭 cancel、dispose()

Coordinator tests

  • unit_kitpool.test.ts:fixture 加 3 個 config field(test 預設 false 防 timer 干擾 isolation)
  • auto-poll-conversion.test.ts(新):5 case cover happy path / failed / duplicate dispatch budget / manual cancel auto / disabled config

OpenSpec change

  • proposal / design / tasks / acceptance / conversion-webhook-lifecycle MODIFIED requirement + 3 新 Scenario(dispatch auto-schedules poller / manual de-dup / poll timeout failed-equivalent)

GitNexus blast radius = LOW

symbol risk d=1
fetchConversionResult LOW createCoordinatorApp
createCoordinatorApp LOW 無 upstream(main entry)

Verification

Level Result
L1 coordinator npm run verify 12 files / 173 tests passed(168 既有 + 5 新)
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 + Postman ① + 等(不手動 POST ingest)→ ② Poll 5-90 秒內 viewer_url 自動出現

Design highlights

詳見 openspec/changes/coordinator-auto-poll-streaming-conversion/design.md。重點:

Backward compatibility

  • POST /api/external/ifc-ready response 不變(202 dispatched,新增背景 schedule poller)
  • POST /api/internal/conversions/:id/ingest response 不變(同樣 callback / session payload,新增 cancel auto poller 動作)
  • 既有 168 tests 不破(unit_kitpool fixture 加 conversionPollEnabled: false 防 timer 干擾)

Spec / Capability impact

conversion-webhook-lifecycle MODIFIED Coordinator ingests host-native conversion result into callback outbox requirement,加 SHALL auto-poll 子條款 + 3 新 Scenario:

  • Dispatch auto-schedules a poller that drives ingestion to terminal
  • Auto-poll de-duplicates with manual ingest endpoint
  • Poll timeout yields a failed-equivalent terminal state

Test plan

  • L1-L3 全綠
  • L2 spec validate
  • CI green
  • Reviewer approve
  • merge
  • (post-merge)docker compose recreate coordinator + 重跑 Postman 看 viewer_url 自動出現(無 manual ingest)
  • sync + archive PR

🤖 Generated with Claude Code

Summary by CodeRabbit

  • New Features

    • Automatic polling for streaming conversion results until completion or timeout.
    • New configuration options to control polling behavior: enable/disable polling, adjust polling interval, and set maximum polling attempts.
  • Tests

    • Added comprehensive test coverage for auto-polling behavior, including success/failure scenarios, timeout handling, and idempotency checks.
  • Documentation

    • Updated specifications and added design documentation for the streaming conversion auto-polling workflow.

Review Change Stack

…_url(coordinator-auto-poll-streaming-conversion)

完整 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>
Copilot AI review requested due to automatic review settings May 22, 2026 05:19
@coderabbitai

coderabbitai Bot commented May 22, 2026 •

Copy link
Copy Markdown
Contributor
📝 Walkthrough

Walkthrough

This PR implements coordinator-side auto-polling for streaming IFC→USDC conversion results. After a successful dispatch, the coordinator automatically polls bim-streaming-server at a configurable interval until a terminal state is reached, then ingests the result into the callback outbox. Polling is controlled by environment variables, de-duplicated against manual ingest, and cleaned up on shutdown.

Changes

Coordinator Auto-Poll Streaming Conversion

Layer / File(s) Summary
Polling configuration and environment helpers
bim-review-coordinator/src/config.ts
CoordinatorConfig gains conversionPollEnabled, conversionPollIntervalSeconds, and conversionPollMaxAttempts fields. New parseBooleanEnv helper parses string env values into booleans; loadConfig populates polling settings from CONVERSION_POLL_* environment variables.
Terminal detection and polling client method
bim-review-coordinator/src/services/streamingConversionClient.ts
Exported isTerminalConversionResult classifies StreamingConversionResult as terminal/failed/ready. New PollerHandle and PollConversionResultOptions types. Implemented StreamingConversionClient.pollConversionResult using chained setTimeout ticks, invoking onTerminal callback when terminal or on max-attempt timeout with synthetic poll_timeout reason.
App-level poller registry and ingest refactor
bim-review-coordinator/src/app.ts (lines 17–22, 272–275, 778–843)
Updated imports for terminal-detection and polling types. Introduced module-scope pollerRegistry (Map<string, PollerHandle>) to track active pollers. Extracted ingestStreamingConversionResult helper that fetches/validates terminal results and converts them into existing conversion-report shape for outbox ingestion. Added schedulePollerForConversion to orchestrate polling with registry cleanup on completion.
Auto-poll trigger on successful dispatch
bim-review-coordinator/src/app.ts (lines 589–593)
After streaming conversion dispatch succeeds, conditionally schedules in-process poller if polling is enabled and conversion_job_id not already in registry, avoiding duplicate polling on idempotent replay.
Manual ingest cancellation and shutdown cleanup
bim-review-coordinator/src/app.ts (lines 244–246, 970–991, 1108–1117)
CoordinatorApp interface extends with dispose() method. Manual ingest endpoint cancels any active auto-poller for the same conversionJobId before ingesting to prevent double callbacks. Implemented dispose() iterates all registered pollers, cancels them, and clears registry during app shutdown/teardown.
Auto-poll integration test suite
bim-review-coordinator/tests/auto-poll-conversion.test.ts
Vitest suite with HTTP stub server modeling streaming-server responses. Tests cover: successful poll-to-ready with viewer URL and session data; failed polling without viewer URL; idempotent dispatch preventing poll duplication; manual ingest canceling auto-poll to avoid double ingest; and disabled polling leaving conversion queued with no stub poll calls.
Unit test fixture configuration
bim-review-coordinator/tests/unit_kitpool.test.ts (lines 62–66)
Updated defaultConfig test fixture with conversionPollEnabled: false plus interval and max-attempts settings to isolate auto-polling from unit test execution.
Design, specification, and acceptance documentation
openspec/changes/coordinator-auto-poll-streaming-conversion/*
Design document specifies auto-poll mechanism, terminal semantics (status/model_status alignment), timeout handling, idempotency/cleanup, backward compatibility, observability, and failure modes (502/network retries, 404 terminal, ingest errors). Proposal frames the problem (conversion_status stuck queued) and solution scope. Spec updates conversion-webhook-lifecycle with auto-polling requirement and scenarios. Acceptance plan covers L1–L4 verification (unit, OpenSpec strict, GitNexus, end-to-end) with explicit stop conditions and anti-hack rules. Tasks checklist establishes end-to-end implementation and verification workflow.

Sequence Diagram

sequenceDiagram
  participant Client
  participant Coordinator
  participant StreamingServer as bim-streaming-server
  participant Outbox

  Client->>Coordinator: POST /api/external/ifc-ready
  Coordinator->>StreamingServer: POST /api/conversions/ifc-to-usdc (dispatch)
  StreamingServer-->>Coordinator: 202 {conversion_job_id}
  Coordinator->>Coordinator: Schedule auto-poller (setTimeout chain)
  
  loop Poll until terminal (max 60 attempts @ 5s)
    Coordinator->>StreamingServer: GET /api/conversions/{id}/result
    alt Not ready
      StreamingServer-->>Coordinator: {status: "queued"}
      Coordinator->>Coordinator: Schedule next tick
    else Ready
      StreamingServer-->>Coordinator: {status: "ready", model_status: "ready"}
      Coordinator->>Coordinator: ingestStreamingConversionResult
      Coordinator->>Outbox: Enqueue conversion_result_ready callback
      Coordinator->>Coordinator: Cancel poller, clear registry
    else Failed
      StreamingServer-->>Coordinator: {status: "failed"}
      Coordinator->>Outbox: Enqueue conversion_failed callback
      Coordinator->>Coordinator: Cancel poller, clear registry
    end
  end
  
  alt Manual ingest before poller completes
    Client->>Coordinator: POST /api/internal/conversions/{id}/ingest
    Coordinator->>Coordinator: Cancel & delete active poller
    Coordinator->>Outbox: Enqueue callback once
  end
Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~22 minutes

Possibly related PRs

  • monkey1sai/AI-BIM-governance#73: Introduced earlier app.ts ingestion-helper infrastructure and StreamingConversionClient.fetchConversionResult that this PR builds auto-polling logic on top of.

Poem

🐰 Poll the streaming server, tick by tick,
Until the model's ready, conversion clicks—
No double-ingesting, we clean up with care,
A poller registry keeps duplicates rare!
When manual ingest arrives, we cancel the chain,
And shutdown disposes, no pollers remain. ✨

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 10.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 change: auto-polling of streaming conversion results after dispatch to automatically ingest viewer_url, which is the core objective of this PR.
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/coordinator-auto-poll-streaming-conversion

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

🧹 Nitpick comments (5)
bim-review-coordinator/tests/auto-poll-conversion.test.ts (1)

29-40: ⚡ Quick win

Clean up per-test temporary directories created in makeApp.

Line 130 creates a new temp root per test, but teardown never removes it. Over repeated local/CI runs this can accumulate stale data under tmp.

♻️ Proposed fix
 let active: CoordinatorApp | null = null;
 let activeStub: http.Server | null = null;
+const tempRoots: string[] = [];

 afterEach(async () => {
   if (active) active.dispose();
@@
   if (active) {
     active.io.close();
     await new Promise<void>((resolve) => active?.server.close(() => resolve()));
     active = null;
   }
+  for (const root of tempRoots.splice(0)) {
+    fs.rmSync(root, { recursive: true, force: true });
+  }
 });
@@
 function makeApp(streamingBase: string, overrides: Partial<CoordinatorConfig> = {}): CoordinatorApp {
   const root = fs.mkdtempSync(path.join(os.tmpdir(), "bim-coord-auto-poll-test-"));
+  tempRoots.push(root);
   active = createCoordinatorApp({

Also applies to: 129-147

🤖 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-review-coordinator/tests/auto-poll-conversion.test.ts` around lines 29 -
40, The test teardown currently disposes network stubs (active, activeStub) but
forgets to remove the per-test temporary root created by makeApp, causing stale
tmp accumulation; update the afterEach in auto-poll-conversion.test.ts to
capture the temp root returned by makeApp (or access the tempRoot property on
the app instance) and remove it during teardown using fs.promises.rm or
fs.rmSync with recursive/force; ensure you reference the same identifier used
when creating the app (e.g., tempRoot or app.tempRoot) and call await
fs.promises.rm(tempRoot, { recursive: true, force: true }) (or equivalent)
before nulling the variable so every test cleans up its temp directory.
openspec/changes/coordinator-auto-poll-streaming-conversion/design.md (2)

7-13: ⚡ Quick win

Add language identifier to fenced code block.

The code block lacks a language identifier. Consider adding ```text to improve markdown parsing and rendering consistency.

📝 Proposed fix
-```
+```text
 [POST /api/external/ifc-ready]
   → coordinator dispatch streaming-server `POST /api/conversions/ifc-to-usdc`
🤖 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/coordinator-auto-poll-streaming-conversion/design.md` around
lines 7 - 13, The fenced code block that begins with "[POST
/api/external/ifc-ready]" in design.md lacks a language identifier; update the
opening triple-backtick to include a language (e.g., replace ``` with ```text)
so the block becomes ```text and preserves formatting/rendering consistently for
the sequence showing coordinator → streaming-server → conversion_job_id/status
flow.

27-72: ⚡ Quick win

Add language identifier to fenced code block.

This large component layout block should have a language identifier for better markdown rendering. Consider ```text or ```typescript for the TypeScript portions.

📝 Proposed fix
-```
+```text
 streamingConversionClient:
   - fetchConversionResult(id)                       (既有,不改)
🤖 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/coordinator-auto-poll-streaming-conversion/design.md` around
lines 27 - 72, The fenced code block starting with "streamingConversionClient:"
is missing a language identifier; update that code fence in design.md to include
a language (e.g., ```text or ```typescript) so markdown renders it correctly —
locate the block beginning with streamingConversionClient and the closing ```
and change the opening fence from ``` to something like ```text (or
```typescript if you prefer) to apply the identifier.
openspec/changes/coordinator-auto-poll-streaming-conversion/tasks.md (2)

71-71: 💤 Low value

Clarify test count reference.

Line 71 mentions "11 files + 新 case 全綠" while acceptance.md line 5 specifies "168 既有 + ~6 新 case 全綠". The "11 files" likely refers to test file count, not test case count, but this could be clarified for consistency with the acceptance plan.

💡 Suggested clarification
-- [ ] 7.1 `cd bim-review-coordinator && npm run verify`(11 files + 新 case 全綠)
+- [ ] 7.1 `cd bim-review-coordinator && npm run verify`(168 既有 + ~6 新 case 全綠)

This aligns with the more specific count in acceptance.md.

🤖 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/coordinator-auto-poll-streaming-conversion/tasks.md` at line
71, Update the checklist entry for 7.1 (`cd bim-review-coordinator && npm run
verify`) to clarify that "11 files" refers to test files rather than test cases
and align the wording with acceptance.md's test-case counts (e.g., change "11
files + 新 case 全綠" to "11 test files (covers ~6 new test cases) — all green" or
similar); edit the line in tasks.md (the `- [ ] 7.1` checklist item) so the
terminology matches acceptance.md's "168 既有 + ~6 新 case 全綠" for consistency.

84-84: ⚡ Quick win

Environment-specific PID reference.

Line 84 references "PID 7488" which is specific to a particular runtime environment and will not be applicable to other environments. Consider rephrasing to indicate this is an example.

📝 Suggested clarification
-- [ ] 9.2 streaming-server 仍跑(PID 7488,STORAGE_ROOT absolute)
+- [ ] 9.2 streaming-server 仍跑(例如 PID 7488,STORAGE_ROOT absolute)

This makes it clear the PID is just an example from a specific test run.

🤖 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/coordinator-auto-poll-streaming-conversion/tasks.md` at line
84, Line 84 currently embeds an environment-specific PID ("PID 7488") in the
checklist item "9.3 跑 Postman ① POST /api/external/ifc-ready"; remove the
hardcoded PID and rephrase to indicate it is an example (e.g., "示例 PID(例如 7488)"
or "PID shown is from a specific run and will vary by environment") so readers
know it's not a required value; update the text in tasks.md for that checklist
entry accordingly.
🤖 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-review-coordinator/src/config.ts`:
- Around line 229-230: The config currently assigns
conversionPollIntervalSeconds and conversionPollMaxAttempts from numberFromEnv
which may return 0 or negative values; clamp both assignments to a minimum of 1
so polling cannot be instant or disabled by misconfigured envs (e.g. replace the
raw calls with Math.max(1, numberFromEnv("CONVERSION_POLL_INTERVAL_SECONDS", 5))
and Math.max(1, numberFromEnv("CONVERSION_POLL_MAX_ATTEMPTS", 60)) or implement
equivalent min logic inside numberFromEnv), referring to the
conversionPollIntervalSeconds and conversionPollMaxAttempts variables and the
numberFromEnv helper.

In `@openspec/changes/coordinator-auto-poll-streaming-conversion/design.md`:
- Around line 95-100: The design needs poll_timeout to be normalized to failed
everywhere: update the onTerminal handling so when onTerminal({ status:
"poll_timeout", conversion_job_id }) is triggered it calls
ingestStreamingConversionResult with a failed-equivalent report; ensure
externalIfcReadyStore.recordConversionOutcome(conversionStatus: "ready" |
"failed") is invoked with "failed" (not a new enum) for that case; make the
callback outbox enqueue event "conversion_failed" with payload.status="failed"
and payload.reason="poll_timeout" (so report.status remains failed and reason
carries the timeout), and confirm API surface code only exposes
conversion_status as "ready" or "failed" (no poll_timeout).

In
`@openspec/changes/coordinator-auto-poll-streaming-conversion/specs/conversion-webhook-lifecycle/spec.md`:
- Around line 30-49: Add a new test in
bim-review-coordinator/tests/auto-poll-conversion.test.ts that exercises the
"Poll timeout yields a failed-equivalent terminal state" scenario: simulate
dispatching a conversion via the coordinator (POST /api/external/ifc-ready) and
stub GET /api/conversions/<id>/result to return non-terminal responses for
conversionPollMaxAttempts (or CONVERSION_POLL_MAX_ATTEMPTS) cycles, assert the
auto-poller de-registers and the coordinator marks the conversion terminally
failed with reason "poll_timeout", and also assert the manual ingest endpoint
(/api/internal/conversions/<conversion_job_id>/ingest) MAY still be accepted
later (no duplicate callback/ingest produced); ensure assertions mirror existing
tests' patterns for ready→auto-ingest and failed→failed ingest to keep
idempotency and de-registration checks consistent with current helpers.

---

Nitpick comments:
In `@bim-review-coordinator/tests/auto-poll-conversion.test.ts`:
- Around line 29-40: The test teardown currently disposes network stubs (active,
activeStub) but forgets to remove the per-test temporary root created by
makeApp, causing stale tmp accumulation; update the afterEach in
auto-poll-conversion.test.ts to capture the temp root returned by makeApp (or
access the tempRoot property on the app instance) and remove it during teardown
using fs.promises.rm or fs.rmSync with recursive/force; ensure you reference the
same identifier used when creating the app (e.g., tempRoot or app.tempRoot) and
call await fs.promises.rm(tempRoot, { recursive: true, force: true }) (or
equivalent) before nulling the variable so every test cleans up its temp
directory.

In `@openspec/changes/coordinator-auto-poll-streaming-conversion/design.md`:
- Around line 7-13: The fenced code block that begins with "[POST
/api/external/ifc-ready]" in design.md lacks a language identifier; update the
opening triple-backtick to include a language (e.g., replace ``` with ```text)
so the block becomes ```text and preserves formatting/rendering consistently for
the sequence showing coordinator → streaming-server → conversion_job_id/status
flow.
- Around line 27-72: The fenced code block starting with
"streamingConversionClient:" is missing a language identifier; update that code
fence in design.md to include a language (e.g., ```text or ```typescript) so
markdown renders it correctly — locate the block beginning with
streamingConversionClient and the closing ``` and change the opening fence from
``` to something like ```text (or ```typescript if you prefer) to apply the
identifier.

In `@openspec/changes/coordinator-auto-poll-streaming-conversion/tasks.md`:
- Line 71: Update the checklist entry for 7.1 (`cd bim-review-coordinator && npm
run verify`) to clarify that "11 files" refers to test files rather than test
cases and align the wording with acceptance.md's test-case counts (e.g., change
"11 files + 新 case 全綠" to "11 test files (covers ~6 new test cases) — all green"
or similar); edit the line in tasks.md (the `- [ ] 7.1` checklist item) so the
terminology matches acceptance.md's "168 既有 + ~6 新 case 全綠" for consistency.
- Line 84: Line 84 currently embeds an environment-specific PID ("PID 7488") in
the checklist item "9.3 跑 Postman ① POST /api/external/ifc-ready"; remove the
hardcoded PID and rephrase to indicate it is an example (e.g., "示例 PID(例如 7488)"
or "PID shown is from a specific run and will vary by environment") so readers
know it's not a required value; update the text in tasks.md for that checklist
entry accordingly.
🪄 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: 32a4163c-e5c2-4261-9824-e28cdf493ac7

📥 Commits

Reviewing files that changed from the base of the PR and between 29fbe3e and f59ccb3.

📒 Files selected for processing (10)
  • bim-review-coordinator/src/app.ts
  • bim-review-coordinator/src/config.ts
  • bim-review-coordinator/src/services/streamingConversionClient.ts
  • bim-review-coordinator/tests/auto-poll-conversion.test.ts
  • bim-review-coordinator/tests/unit_kitpool.test.ts
  • openspec/changes/coordinator-auto-poll-streaming-conversion/acceptance.md
  • openspec/changes/coordinator-auto-poll-streaming-conversion/design.md
  • openspec/changes/coordinator-auto-poll-streaming-conversion/proposal.md
  • openspec/changes/coordinator-auto-poll-streaming-conversion/specs/conversion-webhook-lifecycle/spec.md
  • openspec/changes/coordinator-auto-poll-streaming-conversion/tasks.md

Comment on lines +229 to +230
conversionPollIntervalSeconds: numberFromEnv("CONVERSION_POLL_INTERVAL_SECONDS", 5),
conversionPollMaxAttempts: numberFromEnv("CONVERSION_POLL_MAX_ATTEMPTS", 60),

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

Guard polling interval/attempts against non-positive env values

At Line 229 and Line 230, numberFromEnv allows 0 or negative values. That can cause immediate/tight polling behavior or instant timeout semantics under misconfiguration. Please clamp both to a minimum of 1.

💡 Suggested patch
-    conversionPollIntervalSeconds: numberFromEnv("CONVERSION_POLL_INTERVAL_SECONDS", 5),
-    conversionPollMaxAttempts: numberFromEnv("CONVERSION_POLL_MAX_ATTEMPTS", 60),
+    conversionPollIntervalSeconds: Math.max(1, numberFromEnv("CONVERSION_POLL_INTERVAL_SECONDS", 5)),
+    conversionPollMaxAttempts: Math.max(1, numberFromEnv("CONVERSION_POLL_MAX_ATTEMPTS", 60)),
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
conversionPollIntervalSeconds: numberFromEnv("CONVERSION_POLL_INTERVAL_SECONDS", 5),
conversionPollMaxAttempts: numberFromEnv("CONVERSION_POLL_MAX_ATTEMPTS", 60),
conversionPollIntervalSeconds: Math.max(1, numberFromEnv("CONVERSION_POLL_INTERVAL_SECONDS", 5)),
conversionPollMaxAttempts: Math.max(1, numberFromEnv("CONVERSION_POLL_MAX_ATTEMPTS", 60)),
🤖 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-review-coordinator/src/config.ts` around lines 229 - 230, The config
currently assigns conversionPollIntervalSeconds and conversionPollMaxAttempts
from numberFromEnv which may return 0 or negative values; clamp both assignments
to a minimum of 1 so polling cannot be instant or disabled by misconfigured envs
(e.g. replace the raw calls with Math.max(1,
numberFromEnv("CONVERSION_POLL_INTERVAL_SECONDS", 5)) and Math.max(1,
numberFromEnv("CONVERSION_POLL_MAX_ATTEMPTS", 60)) or implement equivalent min
logic inside numberFromEnv), referring to the conversionPollIntervalSeconds and
conversionPollMaxAttempts variables and the numberFromEnv helper.

Comment on lines +95 to +100
`attempts >= maxAttempts` 觸發 `onTerminal({ status: "poll_timeout", conversion_job_id })`:

- ingestStreamingConversionResult 收到後視為 failed-equivalent
- externalIfcReadyStore 標 conversion_status `poll_timeout`(新 enum 值或映射 failed)
- callback outbox enqueue `conversion_failed` with reason="poll_timeout"
- 不自動重啟 poller(留給後續 ops decision;手動 endpoint 仍可救)

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

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Description: Check for poll_timeout status handling in stores and schemas

# Search for conversion_status enum definitions
rg -nP -C5 'conversion_status|ConversionStatus' \
  bim-review-coordinator/src/ \
  -g '!*.test.{ts,js}'

# Search for poll_timeout references
rg -nP -C5 'poll_timeout' \
  bim-review-coordinator/src/

# Search for callback outbox event type mapping
rg -nP -C8 'conversion_failed|enqueue' \
  bim-review-coordinator/src/ \
  -g '!*.test.{ts,js}'

Repository: monkey1sai/AI-BIM-governance

Length of output: 50384


🏁 Script executed:

#!/bin/bash
set -euo pipefail

# Focused search for poll_timeout wiring
rg -n --hidden --no-ignore -C3 'poll_timeout' bim-review-coordinator/src/app.ts bim-review-coordinator/src/services bim-review-coordinator/src/public/dev-console.html || true

# Confirm what externalIfcReadyStore allows for conversion_status
rg -n 'recordConversionOutcome|conversionStatus:|conversion_status' bim-review-coordinator/src/services/externalIfcReadyStore.ts bim-review-coordinator/src/types.ts bim-review-coordinator/src/app.ts || true

# Confirm callbackOutbox event mapping for failures (including reason)
rg -n 'enqueue\(|conversion_failed|reason:' bim-review-coordinator/src/app.ts bim-review-coordinator/src/services/callbackOutbox.ts || true

# Show the exact onTerminal fake timeout payload mapping
rg -n -C3 'fakeTimeoutResult|onTerminal\(fakeTimeoutResult\)|model_status: "failed"|reason: "poll_timeout"' bim-review-coordinator/src/services/streamingConversionClient.ts || true

# Show what API returns for conversions/:id and external/ifc-ready/:jobId
rg -n -C3 '/api/dev/conversions|/api/internal/conversion-result|/api/external/ifc-ready|conversions/:id/ingest|conversion_status' bim-review-coordinator/src/app.ts || true

Repository: monkey1sai/AI-BIM-governance

Length of output: 16254


🏁 Script executed:

#!/bin/bash
set -euo pipefail

# Print relevant sections (small ranges) to see reason->callback payload mapping
sed -n '840,930p' bim-review-coordinator/src/app.ts | nl -ba | head -n 120
echo "----"
sed -n '1,220p' bim-review-coordinator/src/services/callbackOutbox.ts | nl -ba | sed -n '1,220p' | head -n 220
echo "----"
sed -n '150,220p' bim-review-coordinator/src/services/externalIfcReadyStore.ts | nl -ba | head -n 120
echo "----"
sed -n '200,260p' bim-review-coordinator/src/services/streamingConversionClient.ts | nl -ba | head -n 120

Repository: monkey1sai/AI-BIM-governance

Length of output: 115


🏁 Script executed:

#!/bin/bash
set -euo pipefail

echo "---- app.ts (callback outbox enqueue payload area)"
sed -n '830,930p' bim-review-coordinator/src/app.ts | cat -n

echo "---- callbackOutbox.ts (enqueue + payload shape)"
sed -n '1,240p' bim-review-coordinator/src/services/callbackOutbox.ts | cat -n

echo "---- externalIfcReadyStore.ts (recordConversionOutcome contract)"
sed -n '140,220p' bim-review-coordinator/src/services/externalIfcReadyStore.ts | cat -n

echo "---- streamingConversionClient.ts (fake poll_timeout terminal result)"
sed -n '200,280p' bim-review-coordinator/src/services/streamingConversionClient.ts | cat -n

Repository: monkey1sai/AI-BIM-governance

Length of output: 20829


🏁 Script executed:

#!/bin/bash
set -euo pipefail

# Show the ingestStreamingConversionResult failure handling and report construction
sed -n '760,840p' bim-review-coordinator/src/app.ts | cat -n

# Show normalizeConversionReportStatus + conversionResultReportSchema (closest small ranges)
rg -n 'normalizeConversionReportStatus|conversionResultReportSchema' bim-review-coordinator/src/app.ts
sed -n '520,640p' bim-review-coordinator/src/app.ts | cat -n

Repository: monkey1sai/AI-BIM-governance

Length of output: 11111


Align design “poll_timeout” status with implementation (mapped to failed + callback reason).

  • externalIfcReadyStore only records conversion_status as "ready" | "failed" (recordConversionOutcome(conversionStatus: "ready" | "failed")); the auto-poll poll_timeout terminal is normalized to status: "failed", so conversion_status becomes "failed" (not a distinct poll_timeout value).
  • The callback outbox enqueues event: "conversion_failed" when report.status is failed, and carries payload.status: "failed" plus payload.reason: "poll_timeout" (fake timeout sets reason: "poll_timeout", then ingest forwards it to reason).
  • API responses surface conversion_status as "ready"/"failed" (no poll_timeout status is exposed via the API); poll_timeout appears only in the cloud callback payload reason.
🤖 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/coordinator-auto-poll-streaming-conversion/design.md` around
lines 95 - 100, The design needs poll_timeout to be normalized to failed
everywhere: update the onTerminal handling so when onTerminal({ status:
"poll_timeout", conversion_job_id }) is triggered it calls
ingestStreamingConversionResult with a failed-equivalent report; ensure
externalIfcReadyStore.recordConversionOutcome(conversionStatus: "ready" |
"failed") is invoked with "failed" (not a new enum) for that case; make the
callback outbox enqueue event "conversion_failed" with payload.status="failed"
and payload.reason="poll_timeout" (so report.status remains failed and reason
carries the timeout), and confirm API surface code only exposes
conversion_status as "ready" or "failed" (no poll_timeout).

Comment on lines +30 to +49
#### Scenario: Dispatch auto-schedules a poller that drives ingestion to terminal

- **WHEN** coordinator returns 202 from `POST /api/external/ifc-ready` with a dispatched `conversion_job_id` and `CONVERSION_POLL_ENABLED` is unset or `true`
- **THEN** coordinator schedules an in-process polling task keyed by that `conversion_job_id`
- **AND** the polling task fetches `GET /api/conversions/<id>/result` every `CONVERSION_POLL_INTERVAL_SECONDS`
- **AND** when the result reaches a terminal state the polling task triggers the same ingestion path as the manual endpoint(callback outbox + local session handoff per existing requirements)
- **AND** the polling task de-registers itself after terminal ingestion or after `CONVERSION_POLL_MAX_ATTEMPTS` poll-timeout

#### Scenario: Auto-poll de-duplicates with manual ingest endpoint

- **WHEN** a poller is active for `conversion_job_id` and the operator(or other internal caller)POSTs `/api/internal/conversions/<conversion_job_id>/ingest` with a valid internal token
- **THEN** coordinator cancels and de-registers the auto-poller for that `conversion_job_id`
- **AND** the manual ingest path runs exactly once
- **AND** no duplicate `conversion_result_ready` / `conversion_failed` callback is enqueued for the same `conversion_job_id`

#### Scenario: Poll timeout yields a failed-equivalent terminal state

- **WHEN** the auto-poller reaches `CONVERSION_POLL_MAX_ATTEMPTS` without observing a terminal result from `bim-streaming-server`
- **THEN** coordinator treats the local conversion job as terminally failed with reason `poll_timeout`
- **AND** the manual ingest endpoint MAY still be invoked later to re-ingest if the streaming-server eventually reaches terminal

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

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Description: Check that new test file covers all three scenarios

# Search for auto-poll test cases
rg -nP -C3 'test|it|describe' \
  bim-review-coordinator/tests/auto-poll-conversion.test.ts

# Look for specific test coverage:
# - dispatch auto-schedules poller (ready path)
# - manual ingest cancels auto-poller
# - max attempts / poll timeout

rg -nP 'dispatch.*poll|auto.*poll.*start|schedule.*poll' \
  bim-review-coordinator/tests/auto-poll-conversion.test.ts

rg -nP 'manual.*cancel|ingest.*cancel' \
  bim-review-coordinator/tests/auto-poll-conversion.test.ts

rg -nP 'timeout|max.*attempt' \
  bim-review-coordinator/tests/auto-poll-conversion.test.ts

Repository: monkey1sai/AI-BIM-governance

Length of output: 11073


🏁 Script executed:

#!/bin/bash
set -euo pipefail

# 1) Search for any explicit poll-timeout / max-attempts test coverage
rg -nP 'timeout|poll_timeout|maxAttempts|max attempts|conversionPollMaxAttempts|failed-equivalent' \
  bim-review-coordinator/tests/auto-poll-conversion.test.ts || true

# 2) Quick check if there's any additional auto-poll test file that might cover timeout
fd -e 'ts' -a bim-review-coordinator/tests -t f . | true
fd -a -e 'test.ts' -e 'spec.ts' bim-review-coordinator/tests || true

# 3) Find tasks.md and show section 5 context
fd -e 'md' -a . | rg -n 'tasks\.md$' || true
TASKS_FILE=$(fd -e 'md' -a . | rg -m1 'tasks\.md$' || true)
echo "TASKS_FILE=$TASKS_FILE"
if [ -n "${TASKS_FILE:-}" ]; then
  # Print around "section 5" (best-effort: look for "5." or "## 5" patterns)
  rg -nP '(^|\n)#{1,6}\s*5\b|(^|\n)\b5\.\s|\bSection\s*5\b' "$TASKS_FILE" -n || true
  # Show the first matching header section
  rg -nP '(^|\n)#{1,6}\s*5\b|(^|\n)\b5\.\s|\bSection\s*5\b' "$TASKS_FILE" -n | head -n 5
  # Show ~120 lines around the first match
  LINE=$(rg -nP '(^|\n)#{1,6}\s*5\b|(^|\n)\b5\.\s|\bSection\s*5\b' "$TASKS_FILE" -n | head -n 1 | sed -E 's/^(.*):([0-9]+):.*$/\2/')
  if [ -n "${LINE:-}" ]; then
    start=$((LINE-40))
    end=$((LINE+120))
    sed -n "${start},${end}p" "$TASKS_FILE"
  fi
fi

Repository: monkey1sai/AI-BIM-governance

Length of output: 927


🏁 Script executed:

#!/bin/bash
set -euo pipefail

# A) Locate the relevant test file(s)
fd -t f 'auto-poll' bim-review-coordinator/tests -a || true
fd -t f 'auto.*poll.*conversion' bim-review-coordinator/tests -a || true
fd -t f 'conversion.*poll' bim-review-coordinator/tests -a || true

# B) Search across all tests for poll-timeout / max-attempts related assertions
rg -nP 'poll_timeout|poll time(out)?|CONVERSION_POLL_MAX_ATTEMPTS|conversionPollMaxAttempts|max.*attempt|failed-equivalent|timeout' \
  bim-review-coordinator/tests || true

# C) Locate tasks.md and print the section 5 area (best effort)
TASKS_MD=$(fd -t f -e md --full-path 'tasks\.md$' . | head -n 1 || true)
echo "TASKS_MD=$TASKS_MD"
if [ -n "${TASKS_MD:-}" ]; then
  rg -nP '(^|\n)#{1,6}\s*5\b|(^|\n)\b5\.\s|Section\s*5' "$TASKS_MD" -n || true
  # print around first match
  MATCH_LINE=$(rg -nP '(^|\n)#{1,6}\s*5\b|(^|\n)\b5\.\s|Section\s*5' "$TASKS_MD" -n | head -n 1 | sed -E 's/^(.*):([0-9]+):.*$/\2/')
  if [ -n "${MATCH_LINE:-}" ]; then
    start=$((MATCH_LINE-60))
    end=$((MATCH_LINE+220))
    sed -n "${start},${end}p" "$TASKS_MD"
  else
    # fallback: show any "tasks" headings
    head -n 220 "$TASKS_MD"
  fi
fi

Repository: monkey1sai/AI-BIM-governance

Length of output: 1565


🏁 Script executed:

#!/bin/bash
set -euo pipefail

TEST_FILE="bim-review-coordinator/tests/auto-poll-conversion.test.ts"
echo "== $TEST_FILE =="
wc -l "$TEST_FILE"
echo "--- tail ---"
tail -n 120 "$TEST_FILE"
echo "--- lines 250-380 (if exists) ---"
sed -n '240,380p' "$TEST_FILE" || true

TASKS_MD="./openspec/changes/archive/2026-05-07-add-dev-ifc-source-selection-flow/tasks.md"
echo "== $TASKS_MD =="
wc -l "$TASKS_MD"
# show section 5 content (from the header line to next header)
# first find line number of the header
SECTION_LINE=$(rg -nP '^##\s*5\.' "$TASKS_MD" | head -n 1 | cut -d: -f1 || true)
echo "SECTION_LINE=$SECTION_LINE"
if [ -n "${SECTION_LINE:-}" ]; then
  # print from section line to next '## ' header
  awk -v start="$SECTION_LINE" 'NR>=start{print} NR>start && /^##\s*[0-9]+(\.|$)/{exit}' "$TASKS_MD"
fi

Repository: monkey1sai/AI-BIM-governance

Length of output: 8990


🏁 Script executed:

#!/bin/bash
set -euo pipefail

# Search across all coordinator tests for explicit timeout/max-attempts coverage
rg -nP 'poll_timeout|poll time(out)?|CONVERSION_POLL_MAX_ATTEMPTS|conversionPollMaxAttempts|max.*attempt' \
  bim-review-coordinator/tests -S

Repository: monkey1sai/AI-BIM-governance

Length of output: 379


🏁 Script executed:

#!/bin/bash
set -euo pipefail
TEST_FILE="bim-review-coordinator/tests/auto-poll-conversion.test.ts"
echo "== Head =="
sed -n '1,240p' "$TEST_FILE"
echo "== Search within file =="
rg -nP 'it\(|timeout|poll_timeout|maxAttempts|CONVERSION_POLL_MAX_ATTEMPTS|max attempts|failed-equivalent' "$TEST_FILE"

Repository: monkey1sai/AI-BIM-governance

Length of output: 9690


Add test coverage for the poll-timeout (max-attempts) scenario

  • bim-review-coordinator/tests/auto-poll-conversion.test.ts covers: ready → auto-ingest (incl. viewer_url/callback), failed → failed ingest (no viewer_url), idempotent duplicate dispatch (no double polling), and manual ingest canceling the auto-poller (no double ingest).
  • The spec scenario “Poll timeout yields a failed-equivalent terminal state” is not actually tested: there’s only a comment mentioning poll_timeout, but no test forces the auto-poller to hit conversionPollMaxAttempts without reaching a terminal result.
  • tasks.md “## 5.” in the located file is unrelated to coordinator auto-poll conversion test coverage, so it doesn’t support the mapping.
🤖 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/coordinator-auto-poll-streaming-conversion/specs/conversion-webhook-lifecycle/spec.md`
around lines 30 - 49, Add a new test in
bim-review-coordinator/tests/auto-poll-conversion.test.ts that exercises the
"Poll timeout yields a failed-equivalent terminal state" scenario: simulate
dispatching a conversion via the coordinator (POST /api/external/ifc-ready) and
stub GET /api/conversions/<id>/result to return non-terminal responses for
conversionPollMaxAttempts (or CONVERSION_POLL_MAX_ATTEMPTS) cycles, assert the
auto-poller de-registers and the coordinator marks the conversion terminally
failed with reason "poll_timeout", and also assert the manual ingest endpoint
(/api/internal/conversions/<conversion_job_id>/ingest) MAY still be accepted
later (no duplicate callback/ingest produced); ensure assertions mirror existing
tests' patterns for ready→auto-ingest and failed→failed ingest to keep
idempotency and de-registration checks consistent with current helpers.

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 completes the coordinator-side “internal result loop” gap by adding an in-process auto-poller that starts after a successful streaming conversion dispatch and automatically ingests terminal conversion results to produce viewer_url without requiring a manual /api/internal/conversions/:id/ingest call.

Changes:

  • Add pollConversionResult + shared terminal-detection helper in StreamingConversionClient.
  • Add polling config knobs (CONVERSION_POLL_ENABLED, interval, max attempts) and integrate an auto-poller registry + shared ingest helper into createCoordinatorApp.
  • Add Vitest coverage for auto-poll happy/failed paths, dedupe behavior, and manual-ingest cancellation; plus OpenSpec change scaffold/spec delta for the new behavior.

Reviewed changes

Copilot reviewed 10 out of 10 changed files in this pull request and generated 5 comments.

Show a summary per file
File Description
openspec/changes/coordinator-auto-poll-streaming-conversion/tasks.md OpenSpec task breakdown for implementing coordinator auto-poll + ingest loop
openspec/changes/coordinator-auto-poll-streaming-conversion/specs/conversion-webhook-lifecycle/spec.md Spec delta documenting auto-poll scheduling, dedupe, and timeout behavior
openspec/changes/coordinator-auto-poll-streaming-conversion/proposal.md Motivation and scope constraints for choosing polling over push callbacks
openspec/changes/coordinator-auto-poll-streaming-conversion/design.md Design details for setTimeout polling chain, registry, terminal detection
openspec/changes/coordinator-auto-poll-streaming-conversion/acceptance.md Acceptance criteria for tests/spec validation and runtime expectations
bim-review-coordinator/tests/unit_kitpool.test.ts Extend test config fixture with new polling config fields
bim-review-coordinator/tests/auto-poll-conversion.test.ts New unit tests validating dispatch-triggered polling and manual cancel behavior
bim-review-coordinator/src/services/streamingConversionClient.ts Add terminal helper + poller implementation with timeout handling
bim-review-coordinator/src/config.ts Add polling config fields + boolean env parsing
bim-review-coordinator/src/app.ts Add poller registry, refactor ingest logic, schedule poller after dispatch, cancel on manual ingest, add dispose()

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

Comment on lines +228 to +240
if (attempts >= options.maxAttempts) {
const fakeTimeoutResult: StreamingConversionResult = {
conversion_job_id: conversionJobId,
status: "failed",
ready: false,
model_status: "failed",
usdc_ref: null,
element_mapping_ref: null,
manifest_ref: null,
reason: "poll_timeout",
raw: { reason: "poll_timeout", attempts },
};
await options.onTerminal(fakeTimeoutResult);
const result = options.result ?? (await streamingConversionClient.fetchConversionResult(conversionJobId));
const correlationId = result.correlation_id;
if (!correlationId) {
return { ok: false, status: 422, detail: "streaming conversion result has no correlation_id" };
Comment on lines +830 to +834
await ingestStreamingConversionResult(conversionJobId, { result, source: "auto-poll" });
} catch (err) {
console.warn("auto-poll ingest failed", { conversionJobId, err: err instanceof Error ? err.message : String(err) });
} finally {
pollerRegistry.delete(conversionJobId);
// - dispatch 後 in-process polling 自動 ingest ready / failed
// - poller 重複 dispatch 不雙起
// - manual ingest endpoint 觸發 cancel auto poller(no double ingest)
// - max attempts 達到 → poll_timeout 走 failed-equivalent ingest
Comment on lines +970 to 976
// coordinator-auto-poll-streaming-conversion §4.4:manual endpoint 觸發前 cancel
// 對應 auto-poller,避免後續 onTerminal 觸發第二次 ingest(double callback)。
const existing = pollerRegistry.get(conversionJobId);
if (existing) {
existing.cancel();
pollerRegistry.delete(conversionJobId);
}

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

ℹ️ 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 +229 to +233
const fakeTimeoutResult: StreamingConversionResult = {
conversion_job_id: conversionJobId,
status: "failed",
ready: false,
model_status: "failed",

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 Include correlation ID in poll-timeout terminal payload

When maxAttempts is reached, the synthesized timeout result omits correlation_id, but ingestStreamingConversionResult requires that field and returns 422 without ingesting. In that timeout path, the poller then de-registers and stops, so the job never transitions to failed, no conversion_failed callback is enqueued, and viewer_url/terminal state can remain stuck indefinitely unless someone manually recovers it.

Useful? React with 👍 / 👎.

Comment on lines +972 to 976
const existing = pollerRegistry.get(conversionJobId);
if (existing) {
existing.cancel();
pollerRegistry.delete(conversionJobId);
}

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 auto-poller when manual ingest sees non-terminal result

The manual ingest endpoint now cancels and removes the active poller before checking whether the conversion result is terminal. If an internal caller invokes /api/internal/conversions/:id/ingest while the job is still queued/running (the endpoint returns 409), auto-polling has already been disabled and is not re-registered, so the job can remain permanently stuck in non-terminal state unless another manual ingest is triggered later.

Useful? React with 👍 / 👎.

// coordinator-auto-poll-streaming-conversion:default 啟用 polling;test fixture
// 應在 loadConfig overrides 內傳 conversionPollEnabled: false。
conversionPollEnabled: parseBooleanEnv("CONVERSION_POLL_ENABLED", true),
conversionPollIntervalSeconds: numberFromEnv("CONVERSION_POLL_INTERVAL_SECONDS", 5),

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 Parse poll interval as float to avoid zero-delay hot loops

The new polling interval env is parsed via numberFromEnv, which uses integer parsing; values like CONVERSION_POLL_INTERVAL_SECONDS=0.5 are truncated to 0. That makes setTimeout(..., 0) in the poll chain and can generate very high-frequency polling load against the conversion API. This is especially easy to hit because the test suite uses sub-second intervals (e.g. 0.05) through overrides, so operators may reasonably try fractional seconds in env too.

Useful? React with 👍 / 👎.

@monkey1sai
monkey1sai merged commit 2103bf7 into main May 22, 2026
5 checks passed
@monkey1sai
monkey1sai deleted the codex/openspec/coordinator-auto-poll-streaming-conversion branch May 22, 2026 05:28
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