fix(streaming): contain the Kit process tree on converter timeout (#489 PR-A) - #509
Merged
Merged
Conversation
#489 SEC-001: `_run_powershell_conversion` used `subprocess.run(timeout=...)`, whose timeout only `kill()`s the direct PowerShell child. Kit grandchildren kept processing untrusted input while the `with` block unwound and released the HOOPS entrypoint pin. - Replace `subprocess.run` with `_ContainedProcess`, which spawns the converter inside an OS-level containment boundary: on Windows a Job Object with JOB_OBJECT_LIMIT_KILL_ON_JOB_CLOSE (launched CREATE_SUSPENDED | CREATE_BREAKAWAY_FROM_JOB, assigned, then resumed via a Toolhelp thread walk); on POSIX its own session/process group. - On timeout the whole tree is terminated and then POLLED until every descendant is observed gone, and that poll runs INSIDE the `_hold_windows_hoops_entrypoint_for_execution` block so the pin outlives the tree. Only then may the function raise. - When containment cannot be proven within the deadline the adapter fails closed with a distinct `converter_containment_failed` reason (with surviving PIDs) instead of a plain `converter_timeout`. - Tempfile stdout/stderr redirection is preserved; pipes are still never used (the pipe-hang invariant test now asserts it at the `Popen` seam and checks the ps1-timeout + 30s buffer is still what the wait is given). #489 L1-COR-004 (same-family adjacent hole): `convert-ifc-to-usdc.ps1` only did `Stop-Process -Id $process.Id -Force` on its own `-TimeoutSeconds` expiry. It now uses `Stop-ConverterProcessTree`, a breadth-first child walk (same idiom as `Stop-HostNativeService`) that kills leaves first and then waits until every collected PID is observed gone, reporting surviving PIDs in the timeout message. Tests: new real-process test spawns a child + grandchild, records both PIDs and sleeps past `timeout_seconds + buffer`; asserts `converter_timeout`, both PIDs gone, and a bounded wall clock. Plus ordering test (pin acquired -> tree contained -> pin released) and an unprovable-containment fail-closed test. Verified: .venv pytest test_host_native_conversion_service.py 104 passed, 6 skipped (baseline 101 passed, 6 skipped); bim-streaming-server ps1 test suite passed; PowerShell AST parse check passed; behavioural probe of Stop-ConverterProcessTree terminated a real 2-deep tree with zero survivors. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Contributor
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThe converter now runs in cross-platform containment boundaries. Timeout handling terminates and verifies the full process tree, preserves logs, keeps the HOOPS entrypoint pinned during cleanup, and reports distinct containment failures. ChangesConverter process containment
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant ConversionService
participant ContainedProcess
participant Converter
participant ProcessTree
ConversionService->>ContainedProcess: launch converter
ContainedProcess->>Converter: run within containment boundary
ConversionService->>ContainedProcess: wait with timeout
ContainedProcess->>ProcessTree: terminate descendants on timeout
ProcessTree-->>ContainedProcess: return surviving PIDs
ContainedProcess-->>ConversionService: return result or containment error
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
bim-streaming-server/scripts/convert-ifc-to-usdc.ps1 (1)
470-482: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winBound the post-timeout
WaitForExitwhen containment fails.
Stop-ConverterProcessTreecan return surviving PIDs, and the root Kit PID is part of the collected tree. If Kit survives, thefinallyblock at line 486 calls$process.WaitForExit()with no timeout. The script then blocks forever and never reports the containment failure that lines 504-509 were added to surface. Pass a bounded timeout there, and keep the drain best-effort.🛠️ Proposed fix (line 486, outside the selected range)
- try { $process.WaitForExit() } catch { } + # Bounded: after a failed containment the child may still be alive, and an + # unbounded wait would hide the surviving-PID timeout report. + try { [void]$process.WaitForExit(5000) } catch { }🤖 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/scripts/convert-ifc-to-usdc.ps1` around lines 470 - 482, Update the post-timeout drain in the finally block associated with Stop-ConverterProcessTree to call $process.WaitForExit with a bounded timeout instead of the parameterless overload. Keep this drain best-effort so execution proceeds to the existing surviving-PID reporting and timeout handling when the Kit process remains alive.
🧹 Nitpick comments (4)
bim-streaming-server/tests/test_host_native_conversion_service.py (1)
3616-3618: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAssert against the adapter constant, not the literal 30.
The literal duplicates
_OUTER_TIMEOUT_BUFFER_SECONDS. If the buffer changes, this assertion fails for a reason that is unrelated to the redirection contract it verifies.♻️ Proposed refactor
- assert captured["proc"].wait_timeouts == [adapter.timeout_seconds + 30] + assert captured["proc"].wait_timeouts == [ + adapter.timeout_seconds + adapter._OUTER_TIMEOUT_BUFFER_SECONDS + ]🤖 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/tests/test_host_native_conversion_service.py` around lines 3616 - 3618, Update the wait-timeout assertion for captured["proc"] to use the adapter’s _OUTER_TIMEOUT_BUFFER_SECONDS constant instead of the literal 30, while preserving the expected adapter.timeout_seconds plus buffer contract.bim-streaming-server/scripts/convert-ifc-to-usdc.ps1 (2)
335-360: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winRe-walk the tree after termination to catch late descendants.
Stop-ConverterProcessTreeenumerates the tree once, then terminates from leaf to root. The root Kit process stays alive until the last iteration, so it can spawn new children after enumeration. The poll loop only inspects$ids, so a late descendant is neither killed nor reported. Consider re-collecting children after the first kill pass and repeating termination until the walk returns no new PIDs.♻️ Proposed refactor
- for ($i = $ids.Count - 1; $i -ge 0; $i--) { - Stop-Process -Id ([int]$ids[$i]) -Force -ErrorAction SilentlyContinue - } + for ($pass = 0; $pass -lt 3; $pass++) { + for ($i = $ids.Count - 1; $i -ge 0; $i--) { + Stop-Process -Id ([int]$ids[$i]) -Force -ErrorAction SilentlyContinue + } + # Descendants spawned between enumeration and termination are only + # visible on a second walk. + $rescan = @() + foreach ($known in $ids) { + $rescan += @(Get-ConverterChildProcessId -ParentProcessId $known) + } + $fresh = @($rescan | Where-Object { $ids -notcontains $_ }) + if ($fresh.Count -eq 0) { break } + $ids += $fresh + }🤖 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/scripts/convert-ifc-to-usdc.ps1` around lines 335 - 360, Update Stop-ConverterProcessTree to re-walk the process tree after each termination pass, collecting any newly spawned descendant PIDs and terminating them leaf-first. Repeat collection and termination until no new PIDs are found, then retain the existing wait/reporting behavior so late descendants are also killed or returned as survivors.
308-333: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winFail closed on unsupported process-enumeration platforms
Keep the check compatible with Windows PowerShell 5.1 (
powershell.exe) instead of referencing$IsWindowsdirectly. When/procis unavailable, fail the containment check instead of treating an empty child list as confirmed termination.🤖 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/scripts/convert-ifc-to-usdc.ps1` around lines 308 - 333, The Get-ConverterChildProcessId process-enumeration path must fail closed when running on an unsupported platform without /proc, rather than returning an empty list that implies successful containment. Keep platform detection compatible with Windows PowerShell 5.1 by using the existing $env:OS check and add an explicit unsupported-platform failure when /proc is unavailable, while preserving the Windows CIM and Linux /proc behavior.bim-streaming-server/source/extensions/ezplus.bim_review_stream.messaging/ezplus/bim_review_stream/messaging/ifc2usdc_powershell_adapter.py (1)
376-378: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueStatic analysis flags on
_popenare false positives.Ruff S603 and the ast-grep
subprocess-from-requestrule report untrusted command execution here.cmdoriginates from_build_converter_command, which assembles resolved repository paths and validated arguments, andshell=Falseis always set at the call sites. No request-controlled string reaches the argument vector. Consider a# noqa: S603with a short justification so the finding does not recur on every scan.🤖 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 376 - 378, The static-analysis suppression belongs on the subprocess invocation in _popen, not its callers. Add a narrowly scoped # noqa: S603 with a brief justification that cmd is built from resolved repository paths and validated arguments and that callers always use shell=False; preserve the existing Popen behavior.Source: Linters/SAST tools
🤖 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 465-473: Update the POSIX branch of close() to terminate the
converter’s process group when releasing containment, while preserving the
existing Windows job-handle cleanup and idempotent _closed guard. Ensure normal
as well as timeout exits cannot leave Kit grandchildren running.
- Around line 539-565: Update _terminate_tree_posix so deadline failure
populates self._survivors with the currently live PIDs in the process group,
discovered by enumerating /proc, rather than defaulting to the direct child pid.
Preserve the empty-survivor success paths and ensure
converter_containment_failed receives the actual surviving group members.
In `@bim-streaming-server/tests/test_host_native_conversion_service.py`:
- Around line 3704-3755: Add test execution for
test_host_native_conversion_service.py to the canonical Linux and Windows CI
test jobs, ensuring both workflows invoke this test file through their existing
test commands. Do not modify the test’s finally cleanup or process-tree
assertions.
---
Outside diff comments:
In `@bim-streaming-server/scripts/convert-ifc-to-usdc.ps1`:
- Around line 470-482: Update the post-timeout drain in the finally block
associated with Stop-ConverterProcessTree to call $process.WaitForExit with a
bounded timeout instead of the parameterless overload. Keep this drain
best-effort so execution proceeds to the existing surviving-PID reporting and
timeout handling when the Kit process remains alive.
---
Nitpick comments:
In `@bim-streaming-server/scripts/convert-ifc-to-usdc.ps1`:
- Around line 335-360: Update Stop-ConverterProcessTree to re-walk the process
tree after each termination pass, collecting any newly spawned descendant PIDs
and terminating them leaf-first. Repeat collection and termination until no new
PIDs are found, then retain the existing wait/reporting behavior so late
descendants are also killed or returned as survivors.
- Around line 308-333: The Get-ConverterChildProcessId process-enumeration path
must fail closed when running on an unsupported platform without /proc, rather
than returning an empty list that implies successful containment. Keep platform
detection compatible with Windows PowerShell 5.1 by using the existing $env:OS
check and add an explicit unsupported-platform failure when /proc is
unavailable, while preserving the Windows CIM and Linux /proc behavior.
In
`@bim-streaming-server/source/extensions/ezplus.bim_review_stream.messaging/ezplus/bim_review_stream/messaging/ifc2usdc_powershell_adapter.py`:
- Around line 376-378: The static-analysis suppression belongs on the subprocess
invocation in _popen, not its callers. Add a narrowly scoped # noqa: S603 with a
brief justification that cmd is built from resolved repository paths and
validated arguments and that callers always use shell=False; preserve the
existing Popen behavior.
In `@bim-streaming-server/tests/test_host_native_conversion_service.py`:
- Around line 3616-3618: Update the wait-timeout assertion for captured["proc"]
to use the adapter’s _OUTER_TIMEOUT_BUFFER_SECONDS constant instead of the
literal 30, while preserving the expected adapter.timeout_seconds plus buffer
contract.
🪄 Autofix
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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: de1f838b-2e23-4a27-b27e-9caa2550e0dd
📒 Files selected for processing (3)
bim-streaming-server/scripts/convert-ifc-to-usdc.ps1bim-streaming-server/source/extensions/ezplus.bim_review_stream.messaging/ezplus/bim_review_stream/messaging/ifc2usdc_powershell_adapter.pybim-streaming-server/tests/test_host_native_conversion_service.py
Contributor
There was a problem hiding this comment.
Pull request overview
This PR implements the "PR-A" half of issue #489, hardening the IFC→USDC converter so that a timeout contains the entire Kit process tree, not just the direct PowerShell child. Previously, subprocess.run(timeout=...) only killed the immediate child, allowing Kit grandchildren to keep processing untrusted input while the HOOPS pin was released — a containment gap flagged as SEC-001 (high) and L1-COR-004 (medium).
Changes:
- Python (SEC-001): Replaces
subprocess.runin_run_powershell_conversionwith a new_ContainedProcessabstraction — Windows Job Objects (JOB_OBJECT_LIMIT_KILL_ON_JOB_CLOSE,CREATE_SUSPENDEDrace protection, Toolhelp thread resume) and POSIXstart_new_session+killpg. On timeout it kills the tree and polls until every descendant is observed gone before leaving the HOOPS pin block; when it cannot prove the tree is empty it raises a distinct fail-closed codeconverter_containment_failed(with surviving PIDs) rather than a plainconverter_timeout. - PowerShell (L1-COR-004): Replaces
Stop-Process -IdwithStop-ConverterProcessTree— a BFS child walk (Win32_Process//proc), leaf-first kills, and a bounded wait until every collected PID is confirmed gone, naming survivors in the timeout message. - Tests: Adapts existing
subprocess.rundoubles to asubprocess.Popenseam (_FakeContainedPopen/_patch_popen) and adds real process-tree integration, pin-ordering, and fail-closed regression tests.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
bim-streaming-server/source/.../ifc2usdc_powershell_adapter.py |
Adds _Win32ContainmentApi + _ContainedProcess; refactors _run_powershell_conversion into _build_converter_command / _spawn_contained_converter with tree-containment and the new fail-closed error code. |
bim-streaming-server/scripts/convert-ifc-to-usdc.ps1 |
Adds Get-ConverterChildProcessId + Stop-ConverterProcessTree; the timeout path now kills the whole tree, waits for confirmation, and reports surviving PIDs. |
bim-streaming-server/tests/test_host_native_conversion_service.py |
Moves converter doubles from the subprocess.run seam to subprocess.Popen; adds tree-kill, pin-ordering, and unprovable-containment tests. |
💡 Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 19cec832db
ℹ️ 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".
![codex-tri-adversarial-bot[bot]](https://avatars.githubusercontent.com/u/311287868?s=60&v=4){kind=small}
There was a problem hiding this comment.
Codex Tri-Adversarial Bot
Automated tri-adversarial ship-gate (L0 terra triage / L1 tier-routed lens fanout / L2 refute-by-default / L3 sol apex — Codex models).
Mapped event: COMMENT
Codex Tri-Adversarial ship-gate — PR #509
- Repo head:
fix/489a-converter-tree-containment@19cec83 - Base:
main@970dc34 - Files changed: 3
- Engine: four-model tri-adversarial gate on Codex — L0 triage
gpt-5.6-terra/low; L1 lens finders routedgpt-5.6-terra/low →gpt-5.6-luna/medium →gpt-5.5/xhigh (security floorgpt-5.5); L2 refute-by-defaultgpt-5.5/xhigh, top-tier findings refuted bygpt-5.6-sol/xhigh (every refutation cross-model); L3 apexgpt-5.6-sol/max. 誠實聲明：層級與 Claude 三層 gate 同構（terra≈haiku、luna≈sonnet、gpt-5.5≈opus、sol≈fable），但模型池是 Codex 的，非 Anthropic 的。
Verdict
SHIP
- 阻擋門檻 severity:
critical, high - mapped GitHub event:
COMMENT - ℹ️ 判定為 SHIP，但刻意不送 APPROVE：GitHub App 的 approving review 不計入
required_approving_review_count（2026-07-31 實測）。本報告是證據，approving 那一票請由真人帳號投。
Difficulty & routing
- overall:
critical(source: terra-triage) - lens tiers: correctness→
gpt-5.5, security→gpt-5.5, simplification→gpt-5.5, test-gap→gpt-5.5
Layer stats
- L1: raw=7 deduped=7 finder_failures=0
- L2: confirmed=5 refuted=2 unverified=0
- L3 final: 5
Findings (final, after apex)
[medium] PowerShell timeout can still block on unbounded final WaitForExit
- id:
L1-COR-001lens:correctnessfile:bim-streaming-server/scripts/convert-ifc-to-usdc.ps1 - provenance: finder=
gpt-5.5refuter=gpt-5.6-sol(cross-model-guard) L2=confirmed - evidence: The bounded timeout branch calls
Stop-ConverterProcessTree, but the unchangedfinallypath subsequently invokes parameterless$process.WaitForExit()unconditionally. - why:
WaitForExit()has no deadline. A surviving root process can block it directly, while an untracked descendant retaining inherited redirected-output handles can delay asynchronous stream completion indefinitely. The script may therefore never reach its timeout error. - proposed fix: On timeout, replace the final wait/drain with a bounded operation and cancel or close asynchronous readers when the deadline expires; do not invoke parameterless
WaitForExit()after containment has already reported survivors.
[medium] Windows launch can resume a converter after Job assignment failed
- id:
L1-COR-002lens:correctnessfile:bim-streaming-server/source/extensions/ezplus.bim_review_stream.messaging/ezplus/bim_review_stream/messaging/ifc2usdc_powershell_adapter.py - provenance: finder=
gpt-5.5refuter=gpt-5.6-sol(cross-model-guard) L2=confirmed - evidence:
assigned = api.assign_process(...)is followed byapi.resume_process_threads(pid)before thenot assignedbranch closes the Job handle and setsjob = None. - why: The child is still suspended when assignment failure is known, so resuming it discards the last race-free failure point. The later process-tree snapshot is not equivalent containment: the running converter can create children during enumeration, and descendants can become undiscoverable after their parent exits.
- proposed fix: If assignment fails, terminate and reap the still-suspended child, close the Job handle, and raise a launch/containment error without resuming any thread.
[medium] PowerShell process-tree killer is not directly covered by the added tests
- id:
L1-TG-001lens:test-gapfile:bim-streaming-server/scripts/convert-ifc-to-usdc.ps1 - provenance: finder=
gpt-5.5refuter=gpt-5.6-sol(cross-model-guard) L2=confirmed - evidence: The real-process regression test replaces
_build_converter_commandwith a Python fixture, so it never loadsconvert-ifc-to-usdc.ps1; the other new timeout tests use fakePopenor mockedterminate_treebehavior. - why: No added automated test exercises
Get-ConverterChildProcessId,Stop-ConverterProcessTree, its platform-specific discovery, leaf-first termination, bounded survivor polling, or timeout-message construction. A defect in this independently implemented containment path can therefore pass the visible suite. - proposed fix: Add a PowerShell-level regression test that launches a child and grandchild, invokes the actual timeout/tree-kill path, and asserts bounded completion, both processes gone, and accurate survivor reporting when termination is forced to fail.
[medium] Job-object containment launch fallback lacks adversarial tests
- id:
L1-TG-002lens:test-gapfile:bim-streaming-server/source/extensions/ezplus.bim_review_stream.messaging/ezplus/bim_review_stream/messaging/ifc2usdc_powershell_adapter.py - provenance: finder=
gpt-5.5refuter=gpt-5.6-sol(cross-model-guard) L2=confirmed - evidence: The diff adds branches for breakaway-denied retry, failed Job assignment, and zero resumed threads, but the new tests exercise only the host's normal path or mock timeout-time containment.
- why: These launch transitions determine whether the converter ever executes inside a provable boundary and whether suspended children and handles are cleaned up. The assignment defect survived because no deterministic fault-injection test covers these branches.
- proposed fix: Use fake
_Win32ContainmentApiandPopenobjects to verify breakaway retry flags, assignment-failure termination before resume, resume-failure termination/reaping, handle closure, andconverter_unavailablemapping.
[low] POSIX process-group liveness can mistake zombies for surviving work
- id:
L1-COR-003lens:correctnessfile:bim-streaming-server/source/extensions/ezplus.bim_review_stream.messaging/ezplus/bim_review_stream/messaging/ifc2usdc_powershell_adapter.py - provenance: finder=
gpt-5.5refuter=gpt-5.6-sol(cross-model-guard) L2=confirmed - evidence:
_posix_group_alive()treats every successfulos.killpg(pgid, 0)as live, while_reap_bounded()can reap only the direct child and the deadline path reports(pid,)rather than identifying the remaining group member. - why: A zombie descendant can keep the process group discoverable even though it cannot execute input. With a delayed or non-reaping PID 1/subreaper, this produces a false
converter_containment_failedand can report the already-reaped root PID. Severity is reduced to low because the behavior remains fail-closed and does not permit continued execution. - proposed fix: On Linux, inspect process-group members and treat the group as non-executing only when every remaining member is demonstrably in zombie state; retain conservative behavior on platforms where state cannot be established and report actual members when possible.
Killed (did not survive L2/L3)
L1-SIM-001[low] Converter child-walk logic is duplicated instead of using the existing platform helper — The finding’s own proposed alternative—document why the converter intentionally keeps a private copy—is already satisfied by the added comment: it identifies the existing helper and explains that importing it would couple the standalone converter to deploy-lib. The finder also admits it did not inspL1-SIM-002[low] Tests keep a compatibility wrapper that now just delegates to the new fake Popen helper — The wrapper remains an intent-specific test helper: it exposes only returncode/stdout/stderr, hides_patch_popen’s timeout/on_start/capture mechanics, and lets existing result-oriented tests remain unchanged during a security-focused seam migration. The diff does not establish that its remaining c
Summary
All five survivors remain actionable; L1-COR-003 is downgraded to low because it is a fail-closed false positive rather than a containment escape. The primary fixes are to keep the PowerShell timeout path bounded and to abort a suspended Windows launch when Job assignment fails. Add direct PowerShell and Win32 fault-injection regression coverage alongside those fixes.
Agent calls
- 13/13 ok, engine wall-clock 627.7s
VERDICT
SHIP
VERDICT: SHIP
Owner
Author
|
補充 runtime 佐證 → gate 報告的 L1-COR-001（medium，PowerShell final WaitForExit 無界阻塞）： 本 session 對真實 89.4MB IFC 的轉檔實測恰好重現了這個機制——兩次在多 agent 高負載並行下，USDC 於 ~2 分鐘完整產出後 wrapper 逾 10 分鐘不返回（被殺）；單獨重跑（run3）與 origin/main baseline 皆 34–35s 乾淨退出。與 finding 的機制吻合：無參 注意這段 |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: daaf3bf15c
ℹ️ 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".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d6f791b350
ℹ️ 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".
…SIX all-exit-path group cleanup (#489 review)
…489) PR #509 review round 2. Each fix is scoped to the containment boundary the change already promises; the review findings that are pre-existing or not closable from inside this PR are answered on the threads instead. - Windows launch is now fail-closed: when the Job Object cannot be created or AssignProcessToJobObject fails, the still-suspended child is terminated and the launch raises (-> converter_unavailable) instead of silently degrading to a racy PID-tree snapshot with no containment boundary. - POSIX close() terminates the process group. POSIX has no kill-on-close equivalent and terminate_tree() only runs for TimeoutExpired, so a converter that exited early (crash, external kill, non-zero exit) could leave Kit grandchildren running while the HOOPS pin was released. - POSIX liveness ignores defunct members. killpg(pgid, 0) also succeeds for a group whose members are all zombies; on hosts where PID 1 does not reap orphans that burned the whole containment deadline and reported a false converter_containment_failed. - converter_containment_failed now lists the live group members discovered from /proc instead of the direct child PID, which is usually the process that has already exited. - The POSIX grace period is granted to the whole group: the code used to SIGKILL as soon as the direct child was reaped, truncating descendant signal handlers. - Timeout and containment failures carry the Kit log diagnostics (kit_stdout_log / kit_stderr_log + output tail) that streaming-ifc-usdc-conversion-authority requires for any post-start failure; all three post-start failure paths now share one helper. - convert-ifc-to-usdc.ps1 re-discovers descendants through termination instead of trusting a single BFS snapshot, and no longer counts defunct PIDs as survivors. The reparenting case remains out of reach from inside the script; the authoritative boundary is the caller's Job Object / process group, and that limit is now documented rather than implied away. - The _ContainedProcess docstring states the real boundary strength per platform: the Windows Job Object is non-escapable, the POSIX process group is best-effort and a descendant that calls setsid() leaves it. Validation: - .venv pytest bim-streaming-server/tests/test_host_native_conversion_service.py -> 114 passed, 6 skipped (baseline 104/6; 10 new tests, all red before the fix) - pwsh bim-streaming-server/scripts/tests/test-convert-ifc-to-usdc.ps1 -> passed (new: AST-lifted containment helpers, live 2-deep tree, late-descendant rescan; the rescan case was verified red against the pre-fix function, which returned survivors=[] with the late child still alive) - Windows host-native Kit re-run at this head: real 89.4MB IFC (storage/demo_lib_2026.ifc) -> EXIT=0, 39s, 28,462,810 bytes, byte-identical to the recorded baseline Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Propagate inner containment failures with the dedicated code
When Kit reaches the PowerShell timeout and Stop-ConverterProcessTree reports surviving PIDs or fails to reach a fixed point, the script throws and exits during the outer adapter's intentional 30-second buffer, so timed_out remains false and this branch always raises converter_failed. The newly introduced converter_containment_failed code is therefore used only when PowerShell itself exceeds the outer timeout, while the usual inner containment-failure path is misclassified and downstream job/API diagnostics cannot distinguish a potentially live process tree from an ordinary conversion error.
ℹ️ 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".
…e overlapping containment hunks) Two agents worked the same PR #509 review threads. The origin implementation is kept verbatim for every overlapping hunk (fail-closed Windows launch, POSIX close() sweep, /proc-based group membership, ps1 discover->kill fixed point); the local-only deltas are re-applied in the following commit. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Adds the review findings that the concurrent origin commit did not cover. The overlapping hunks (fail-closed Windows launch, POSIX close() sweep, /proc-based group membership, ps1 discover->kill fixed point) are origin's implementation, kept verbatim. - POSIX grace period is granted to the whole GROUP. _reap_bounded returns the moment PowerShell is reaped, so the old code escalated to SIGKILL within milliseconds and a Kit/HOOPS descendant still running its SIGTERM handler was killed mid-cleanup; the configured grace was never actually granted. Reap the direct child, then poll the group until the grace deadline. - Timeout and containment failures now carry the Kit log diagnostics. streaming-ifc-usdc-conversion-authority requires kit_stdout_log / kit_stderr_log plus a tail summary for any failure after the subprocess starts, and names timeout explicitly; both timeout raises had neither, so conversion_authority._fail_job could only surface an error with no diagnostic path. All three post-start failure paths now share _failure_diagnostics(). The ps1 prints ##CONV_META## before Kit starts, so the paths survive the kill. - convert-ifc-to-usdc.ps1 no longer counts defunct PIDs as containment survivors. Get-Process keeps returning a zombie, so on hosts whose PID 1 does not promptly reap orphans the wait burned the full deadline and then reported a containment FAILURE for a tree that was already dead. Test-ConverterProcessAlive reads /proc/<pid>/stat and excludes state Z; the state field is taken after the last ')' because comm may contain spaces and parens. - _ContainedProcess documents the real per-platform boundary strength: the Windows Job Object is non-escapable (no JOB_OBJECT_LIMIT_BREAKAWAY_OK), the POSIX process group is best-effort and a descendant that calls setsid() leaves it. Closing that residual needs a cgroup and is out of scope here; the fix is to stop implying a guarantee POSIX does not give. - Module-level _POSIX_SIGTERM/_POSIX_SIGKILL so the POSIX branches stay unit testable from a Windows host instead of only being verifiable on Linux. - convert-ifc-to-usdc.ps1 gets its first executable coverage: the containment helpers are lifted out through the PowerShell AST and driven against a real 2-deep process tree plus a late-appearing descendant. Validation: - .venv pytest bim-streaming-server/tests/test_host_native_conversion_service.py -> 113 passed, 7 skipped (baseline 104/6; the 7th skip is origin's SIGKILL-requires-POSIX test, not a Windows regression) - pwsh bim-streaming-server/scripts/tests/test-convert-ifc-to-usdc.ps1 -> passed (the late-descendant case was verified red against the pre-review function, which returned survivors=[] with the late child still alive) - Windows host-native Kit re-run at this head: real 89.4MB IFC (storage/demo_lib_2026.ifc) -> EXIT=0, 44s, 28,462,810 bytes, byte-identical to the recorded baseline Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
This was referenced Aug 12, 2026
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 86a52a5baa
ℹ️ 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".
…onest wrapper claims (#489 review r2)
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 397fbe327a
ℹ️ 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".
… test probe (#489) PR #509 review round 2. Verdicts: adapter:599 fixed, test:3687 fixed, ps1:427 refuted with executable evidence. - _windows_live_pids returns None when QueryInformationJobObject fails, and _terminate_tree_windows can no longer satisfy its success condition from a PPID walk. The Job Object is the authoritative membership record; ancestry is a different, weaker question, and the root may already be gone with its descendants reparented, so an empty walk was being promoted into a containment proof for a job whose members were never observed. A single failed query may be transient, so the poll continues to the deadline and only then fails closed; the walk is still used, but purely to name suspect PIDs in the error, never to prove emptiness. - _pid_is_alive in the tests excludes defunct processes. os.kill(pid, 0) succeeds for a corpse, while the production containment deliberately treats zombies as terminated, so on a host whose PID 1 does not promptly reap orphans the probe would have failed test_converter_timeout_kills_the_whole_process_tree_before_raising while containment had in fact succeeded. Parsed independently of the adapter. - ps1:427 (orphan forked between enumeration and the reverse-order kill) does NOT apply to Windows. Verified on a real host: Windows does not reparent orphans, Win32_Process.ParentProcessId still resolves to the killed parent, and the loop re-walks from every KNOWN pid, so the orphan is rediscovered and terminated before a fixed point is declared. Locked in by a new 3-level (root -> mid -> leaf) test where the mid process forks late and is then killed; a root-only re-walk was measured against the same fixture and reported FixedPointReached=True with the orphan still alive, so the test discriminates. Linux does reparent, which destroys that link, so Test-OrphanRediscoverySupported now gates the "terminated and confirmed gone" wording and the POSIX message states that containment is not proven by the script there (the caller's process-group boundary is authoritative; cgroup work tracked in #517). Validation: - .venv pytest bim-streaming-server/tests/test_host_native_conversion_service.py -> 118 passed, 7 skipped (previous round 113/7) - pwsh bim-streaming-server/scripts/tests/test-convert-ifc-to-usdc.ps1 -> passed - Windows host-native Kit re-run at this head: real 89.4MB IFC -> EXIT=0, 44s, 28,462,810 bytes, byte-identical to the recorded baseline Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…claim Third concurrent collision on this branch. origin's implementation is kept for every overlapping hunk (immediate fail-closed on unreadable job membership plus the containment_detail reason code, and the zombie-aware test liveness probe); this merge only re-adds the local-only work and corrects one factual claim. origin's Stop-ConverterProcessTree comment and its timeout message stated that a PPID walk loses an orphan whose parent died "to init/PID 1 on Linux, to nothing on Windows". That is measured to be false on Windows: an exited process keeps being named by its children's Win32_Process.ParentProcessId, so querying children of an already-killed KNOWN pid still returns the orphan, which is exactly why the loop re-walks from every known member. Probe on a real host: ORPHAN_STILL_DISCOVERABLE_FROM_DEAD_PARENT=True. The comment and the Windows timeout wording now say what was measured; the Linux half of origin's claim is correct and is kept, gated behind Test-OrphanRediscoverySupported. Re-added on top of origin's version: - scripts/tests/test-convert-ifc-to-usdc.ps1 case 2: a 3-level root -> mid -> leaf fixture where the leaf is forked late and the mid process is killed, asserting the leaf is terminated. The same fixture driven by a root-only re-walk was measured reporting FixedPointReached=True with the leaf still alive, so the case discriminates rather than passing vacuously. - test_windows_containment_failure_names_the_live_job_members: readable and non-empty membership, the third case alongside origin's unreadable and readable-empty ones. - test_liveness_probe_helper_treats_a_posix_zombie_as_dead: locks in origin's _pid_is_alive zombie fix, which had no test. Validation: pytest 118 passed / 7 skipped; ps1 harness passed. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
monkey1sai
added a commit
that referenced
this pull request
Aug 12, 2026
…spend one budget Second Codex tri-adversarial round on PR #513 returned NO-SHIP on three more findings against Stop-HostNativeProcessTreeAndWait. All three are real; none was refuted. L1-COR-001 (high): the exited-parent sweep added in 110c657 is only sound where the OS keeps the creator PID on an orphan. Windows does - measured here, a killed parent's PID still resolves its orphaned child and its conhost through Win32_Process.ParentProcessId, and the PID cannot be recycled while this call holds the exited process handle. Linux does not: the kernel re-parents orphans to init or the nearest subreaper, so both discovery passes return empty, and an empty pass was being read as containment. That platform fact now lives in Test-OrphanRediscoverySupported alongside the other platform primitives, and it is the same conclusion the converter containment work reached in #509, which had to own a job object / process group established at launch rather than trust a PPID link. Where rediscovery is unsupported and the parent had already exited on entry, the helper now fails closed and says so: containment is not provable via PPID on this platform, and the authoritative boundary is the caller. A new -KnownDescendantProcessIds parameter is the escape hatch - a descendant record captured before the parent could exit replaces the link discovery lost, and the helper then contains and proves that set normally. L1-SEC-001 (high): the loop broke cleanly only when survivors AND newly discovered descendants were both zero, but the deadline branch broke on time alone and the post-loop check only tested survivors. A deadline pass that discovered a descendant and successfully stopped it therefore left zero survivors and returned success, with no fixed point ever reached. Success now requires a recorded clean pass; reaching the deadline without one throws. L1-COR-002 (medium): TimeoutMs was not one budget. The stopwatch started after discovery and the parent wait still received the full allowance. It now starts before discovery, the parent wait receives only the remainder, and no further containment pass begins on an exhausted budget. Verified against the previous implementation to confirm these are real, not theoretical: the churn scenario returned SUCCESS after discovering twelve descendants, and the slow-discovery scenario spent 2786ms against an advertised 1500ms bound. Both now fail closed and stay inside the budget. Four new behavioural cases: production defaults against an already-exited parent asserting whichever branch this platform is on (no injected lookup), the POSIX fail-closed path driven from a Windows run via the injected capability gate, the pre-exit record proving containment on that same simulated platform, and one case each for the clean-pass requirement and the end-to-end budget. The gated deployment static test's three pinned literals are unchanged; the parent wait keeps `$Process.WaitForExit($TimeoutMs)` honest by living in a nested helper whose own TimeoutMs parameter IS the remaining allowance. Refs #489. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 145381ab21
ℹ️ 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".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b483e87798
ℹ️ 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".
monkey1sai-blip
approved these changes
Aug 12, 2026
monkey1sai-blip
left a comment
monkey1sai-blip
left a comment
Collaborator
There was a problem hiding this comment.
Approved by monkey1sai-blip (the reviewer account pinned by the repo's merge governance).
Submitted through scripts/blip_review.py — a scripted approval carrying the operator's authority, pinned to head 841277a7c6e8f4319e4f32e764e5b2ab8b8654b0. This is the mechanism the GitHub App cannot satisfy: an App's approving review does not count toward required_approving_review_count.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 841277a7c6
ℹ️ 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".
This was referenced Aug 12, 2026
monkey1sai
added a commit
that referenced
this pull request
Aug 12, 2026
…489-B) (#513) * fix(deploy): resolve Kit control URL after the .env missing-key merge The Phase 1 audit sealed $resolvedKitControlUrl and the Kit Manager runtime signature before the Phase 2 ".env / .env.example missing-key merge" ran. That merge can both append a missing KIT_CONTROL_URL and repoint $resolvedEnvFile from the .example to the real env file, so a run that repaired the file still started the host-native Kit Manager with the stale pre-merge value and then persisted a signature describing the repaired state — a blocked runtime-control state that reads as configured on the next run. Pure relocation, not duplication: both assignments move down to immediately after the env merge and volume fix. Nothing between the old and new positions reads either variable, so the exactly-once AST guarantee asserted by test-deploy-governance-static.ps1 is preserved. $resolvedAllowedStageHosts is deliberately left where it is. test-deploy-governance-static.ps1 gains index-ordering assertions that pin the resolve, the signature build and the child launch after the merge marker. Known delta: -DryRun exits before Phase 2, so it no longer validates KIT_CONTROL_URL. The value it used to validate was the pre-merge one read from whichever file the audit resolved, which is the stale read this change removes. Refs #490 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * fix(verify): assert Kit control URL locality in the Deployment profile $expectedKitControlUrl was read verbatim from kit-manager-api.params.json and only string-compared against the /health payload's kit_control_url. The signature checks cover port and revision equality, which prove checkout identity and nothing about URL policy, and the local-address assertion was applied to host_native_bind_host and the conversion health host but never to this URL. A REMOTE origin agreed between a drifted signature and a drifted service therefore passed silently. Assert-DeploymentKitControlUrlIsLocal now runs beside the two existing asserts inside the same -not $PlanOnly guard. It deliberately re-implements the rule from Resolve-HostNativeKitControlUrl rather than importing it — the verifier is an independent layer, exactly as Assert-DeploymentHostNativeBindIsLocal re-implements Test-HostNativeLocalAddress. Empty stays allowed as the honest unconfigured state, localhost is accepted because the launcher canonicalises it through, and non-literal hosts are refused without DNS resolution so the verifier cannot be rebound. test-verify-all.ps1 gains a locality accept/reject matrix, subprocess rejection cases for a remote and a credentialed control URL, acceptance runs for 127.0.0.1 and localhost, an AST assertion that the verifier never calls the launcher resolver, and a paired case proving that a matching-but-remote health payload satisfies the identity comparison on its own. Refs #491 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * fix(launcher): contain descendants in Stop-HostNativeProcessTreeAndWait Kill($true), WaitForExit and HasExited all describe the SAME Process object. Microsoft documents that they can report completion while descendants are still running, and the old catch additionally swallowed the tree-kill exception whenever the parent had already exited — precisely the case where orphaned grandchildren survive. The CAD hardener and the Kit Manager import probe use this helper to prove a timed-out tree is gone before releasing the trust boundary they hold, so the postcondition was false where it mattered most. Second defect in the same helper: Process.Kill([bool]) does not exist on .NET Framework, so under Windows PowerShell 5.1 the tree kill ALWAYS threw into that catch and the helper silently degraded to parent-only termination. The helper now snapshots the descendant PID set through Get-PlatformChildProcessIds before terminating (afterwards the parent/child links are gone and orphans are re-parented), guards the tree kill behind an overload probe with the enumerated-PID fallback Stop-HostNativeService already uses, and waits for the parent AND every snapshotted descendant inside one bounded budget. Anything still alive throws instead of reporting success. Descendant liveness is judged on process identity, not the bare PID, so a recycled PID reads as "our descendant is gone" rather than failing a caller closed on an unrelated process. test-host-native-launcher.ps1 gains a dynamic hung-fixture case proving both recorded PIDs exited inside the bounded window, a negative case with an injected unkillable descendant proving the helper fails closed, and source-shape assertions pinning the overload guard. This commit also registers the bundle's single open ledger entry `mechanism-hardening-2`, covering the three verification-mechanism paths this pull request changes (#490 deploy.ps1, #491 verify-all.ps1, #489 host-native-launcher.ps1). One entry, one canonical Linux rebuild at fixpoint, instead of three serialised debts for one hardening round. Refs #489 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * fix(launcher): prove containment for exited parents and legacy fallbacks The PR #513 Codex tri-adversarial ship-gate returned NO-SHIP on four findings against Stop-HostNativeProcessTreeAndWait, and four PR review threads named the same defects. L1-COR-001 (high): `if ($Process.HasExited) { return }` ran before the descendant snapshot, so a parent that lost the race between the caller's liveness decision and this helper took the entire sweep with it while its descendants kept running. Neither OS cascades termination, so an exited parent now excuses the parent kill/wait only - the snapshot, the termination and the survivor proof still run. L1-COR-002: both fallback loops stopped snapshotted descendants by bare PID. A PID recycled between enumeration and the stop meant terminating an unrelated host process while the survivor poll still read "our descendant is gone". Every stop is now identity-revalidated immediately before it fires, and a changed incarnation is treated as already gone. L1-SEC-002: one fixed snapshot is not containment - a snapshotted process can spawn another child before it dies, and that child was in neither the stop list nor the success check. Containment is now a bounded fixed point that re-enumerates, terminates and verifies until a pass finds nothing new and nothing alive, and fails closed at the deadline. Re-enumeration expands only from roots that are still the incarnation we recorded; the parent stays a root because this call holds its Process handle. L1-TG-003: the tree-kill capability decision is injectable, so the .NET Framework / Windows PowerShell 5.1 fallback is driven as behaviour from a PowerShell 7 run instead of by source-string order alone. Six behavioural cases cover it: an already-exited parent with a real orphaned descendant, the same shape failing closed, an identity-gated stop on a recycled PID, a forced no-Kill(bool) fallback over a real three-level chain (deepest-first, parent terminated, every PID proven gone), a post-snapshot spawn, and the fallback's own fail-closed path. Two more findings from the same review round: - `deploy.ps1 -DryRun` stopped adjudicating KIT_CONTROL_URL when the authoritative resolution moved after the Phase 2 missing-key merge. That merge only appends an absent key with a default and can never repair an existing value, so an unusable authority is now a Phase 1 hard fail, reported without echoing the URL, with the authoritative post-merge resolution left where it is. - scripts/tests/test-host-native-launcher.ps1 ran in no workflow, so these regressions gated nothing. It now runs in the required rebuild-test-deploy job, PowerShell 7 only: the dynamic fixtures use ProcessStartInfo.ArgumentList, which .NET Framework does not have. Refs #489. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * docs(evidence): bind mechanism-hardening-2 bootstrap evidence to its reviewed head The PR #513 review threads found this bundle's evidence unbindable: it recorded only the baseline commit, so no reader could tie the PASS results to the code under review, and the PASS lines carried bare command ids with no invocation or command-map reference. The record now splits into the two rounds that produced it, pins the reviewed head commit `110c657` with the clean-worktree result observed at it, names the immutable command map that resolves the ids, and writes the resolved invocation inline for the three ids that map does not carry. That third point also corrects a partial disclosure: the earlier note covered `test-deploy-dryrun` and `test-deploy-env-fallback` but omitted `test-stop-all-single-pid`, which is in the same position. All three have executable sources under scripts/tests/ and pass; none is a verification-contract command id. Promoting them into the contract would enlarge this entry's fixpoint obligation, so that decision is left to the ledger owner rather than taken inside a review-fix round. Refs #489. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * fix(launcher): gate orphan sweeps by platform, require a clean pass, spend one budget Second Codex tri-adversarial round on PR #513 returned NO-SHIP on three more findings against Stop-HostNativeProcessTreeAndWait. All three are real; none was refuted. L1-COR-001 (high): the exited-parent sweep added in 110c657 is only sound where the OS keeps the creator PID on an orphan. Windows does - measured here, a killed parent's PID still resolves its orphaned child and its conhost through Win32_Process.ParentProcessId, and the PID cannot be recycled while this call holds the exited process handle. Linux does not: the kernel re-parents orphans to init or the nearest subreaper, so both discovery passes return empty, and an empty pass was being read as containment. That platform fact now lives in Test-OrphanRediscoverySupported alongside the other platform primitives, and it is the same conclusion the converter containment work reached in #509, which had to own a job object / process group established at launch rather than trust a PPID link. Where rediscovery is unsupported and the parent had already exited on entry, the helper now fails closed and says so: containment is not provable via PPID on this platform, and the authoritative boundary is the caller. A new -KnownDescendantProcessIds parameter is the escape hatch - a descendant record captured before the parent could exit replaces the link discovery lost, and the helper then contains and proves that set normally. L1-SEC-001 (high): the loop broke cleanly only when survivors AND newly discovered descendants were both zero, but the deadline branch broke on time alone and the post-loop check only tested survivors. A deadline pass that discovered a descendant and successfully stopped it therefore left zero survivors and returned success, with no fixed point ever reached. Success now requires a recorded clean pass; reaching the deadline without one throws. L1-COR-002 (medium): TimeoutMs was not one budget. The stopwatch started after discovery and the parent wait still received the full allowance. It now starts before discovery, the parent wait receives only the remainder, and no further containment pass begins on an exhausted budget. Verified against the previous implementation to confirm these are real, not theoretical: the churn scenario returned SUCCESS after discovering twelve descendants, and the slow-discovery scenario spent 2786ms against an advertised 1500ms bound. Both now fail closed and stay inside the budget. Four new behavioural cases: production defaults against an already-exited parent asserting whichever branch this platform is on (no injected lookup), the POSIX fail-closed path driven from a Windows run via the injected capability gate, the pre-exit record proving containment on that same simulated platform, and one case each for the clean-pass requirement and the end-to-end budget. The gated deployment static test's three pinned literals are unchanged; the parent wait keeps `$Process.WaitForExit($TimeoutMs)` honest by living in a nested helper whose own TimeoutMs parameter IS the remaining allowance. Refs #489. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * docs(evidence): record the round 3 containment fixes and their measured basis Binds the second ship-gate round to its reviewed head d634530 with the clean-worktree result observed at it, and records what was actually measured rather than argued: - Windows orphan rediscovery is SUPPORTED. A killed fixture parent's PID still resolved its orphaned python child and its conhost through Win32_Process.ParentProcessId, and the production-default sweep contained and proved both. That is the platform fact Test-OrphanRediscoverySupported encodes, and the reason the POSIX branch has to fail closed instead. - Both behavioural regressions were reproduced against the previous implementation before fixing: the clean-pass scenario returned SUCCESS after discovering twelve descendants, and the slow-discovery scenario spent 2786 ms against an advertised 1500 ms bound. The POSIX branch of the platform gate is proven on this host through the injected capability decision only; no canonical Linux session was opened, so it has never run on a real re-parenting kernel. That is recorded as an explicit NOT_RUN rather than folded into the PASS list. Refs #489. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * fix(launcher): narrow the process-tree helper's claim to what it can prove Three ship-gate rounds all landed HIGH findings on the same thing: the helper was documented and used as if it delivered inescapable containment, while every mechanism it has rides the OS parent/child link, which is advisory. Patching the next escape window would have invited a fourth. This narrows the claim instead. The contract is now stated in the function itself: a bounded best-effort SWEEP with a fail-closed provability report. It enumerates what it can reach, terminates deepest-first with identity revalidation, re-enumerates to a fixed point, and throws whenever it cannot prove the set it knows about is gone. It is explicitly NOT an escape-proof boundary - only an OS boundary established at LAUNCH (Windows Job Object, POSIX process group / cgroup) can be that, which is #517 and the Start-HostNativeService follow-up. A caller gets "this sweep proved what it could see, or it threw", never "nothing survived". Removed -KnownDescendantProcessIds. It was the escape hatch that let the helper keep claiming provable containment on a re-parenting platform, neither production caller passed it, and its existence blurred exactly the line this round is drawing. The POSIX already-exited-parent path stays a fail-closed throw, and the message now states the narrowed contract and names the launch- time boundary and #517 rather than offering a parameter as the answer. The gate's remaining HIGH - a descendant spawned between enumeration and the stop that follows it - is REFUTED by measurement, not argument. A real three-level fixture whose grandchild is hidden from the snapshot (running the whole time, so it stands in for one spawned a moment after enumeration), with the tree-kill capability forced off so .NET cannot do the containment for us: the helper stopped the snapshot, killed the parent last, and pass 1 re-walked every snapshot member it had just killed, rediscovered the grandchild through its dead parent's link, stopped it, and pass 2 came back clean. All three PIDs gone, no throw. That is now a regression test. The lookup sequence it asserts on is the mechanism: [P, D, P, D, conhost, G, P]. The residual that survives that refutation is documented rather than papered over, because closing it would make things worse: a descendant discovered AND stopped inside one pass drops out of the expansion roots, so a child it spawned in that sub-window is not rediscovered. Expanding from dead PIDs every pass would trade this fail-open gap for a fail-dangerous one - a recycled PID would contribute an unrelated process's children to the kill set. Also: the orphan-rediscovery test no longer asks Test-OrphanRediscoverySupported which branch to expect, which was the implementation grading its own homework. It reads the raw OS record instead - Win32_Process.ParentProcessId on Windows, /proc/<pid>/stat field 4 on Linux - and asserts the helper agrees with it. The gated deployment static test's three pinned literals are unchanged. Refs #489, #517. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * docs(evidence): record round 4, the HIGH-2 refutation and the residual it leaves Binds the narrowed-claim round to its reviewed head 3044e78 with the clean worktree observed at it, and keeps the three kinds of statement apart instead of flattening them into one PASS list: - MEASURED: a descendant hidden from the snapshot but running throughout was rediscovered, stopped and proven gone by the re-enumerating fixed point, with the tree kill forced off so .NET could not do the work. That refutes the gate's remaining HIGH and is now a regression case. - ANALYSIS-NOT-MEASURED: the sub-window that survives it - a descendant discovered and stopped inside one pass leaves the expansion roots, so a child it spawned in that window is not rediscovered. Left open deliberately, because expanding from dead PIDs would trade a fail-open gap for a fail-dangerous one. - CONTRACT: the helper is a bounded best-effort sweep with a fail-closed provability report, not an escape-proof boundary, and -KnownDescendantProcessIds is gone. The POSIX leg of the platform gate is still NOT_RUN on a real re-parenting kernel; it remains proven only through the injected capability decision. Refs #489, #517. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * fix(env): 補上 Kit 控制網址樣板佔位 * chore(governance): declare platform-adapter.ps1 in the mechanism-hardening-2 entry Round-3 added Test-OrphanRediscoverySupported to scripts/lib/platform/platform-adapter.ps1, which is a classified verification-mechanism path; the bundle's ledger entry must declare every mechanism path its diff touches. test-self-referential-bootstrap all green. Refs #489 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * fix(deploy): 保留明示環境檔並封閉子程序列舉失敗 --------- Co-authored-by: Claude Fable 5 <noreply@anthropic.com> Co-authored-by: monkey1sai-blip <monkey1sai@icloud.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d930d8840e
ℹ️ 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".
monkey1sai-blip
approved these changes
Aug 12, 2026
monkey1sai-blip
approved these changes
Aug 12, 2026
monkey1sai-blip
left a comment
monkey1sai-blip
left a comment
Collaborator
There was a problem hiding this comment.
@claude 最後確認 + opus 5 max 交叉對抗驗證
|
Claude finished @monkey1sai-blip's task in 5m 16s —— View job Claude finished @monkey1sai-blip's taskTodo
Scope of this passThis is a review/confirmation request ("最後確認 + opus 5 max 交叉對抗驗證"), so I read the current code at HEAD ( I re-verified the core containment path end-to-end (
Two findings from the codex pass on
|
This was referenced Aug 12, 2026
Closed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
_run_powershell_conversion的subprocess.run換成_ContainedProcess——Windows 用 Job Object（CreateJobObjectW＋JOB_OBJECT_LIMIT_KILL_ON_JOB_CLOSE＋AssignProcessToJobObject，CREATE_SUSPENDED|CREATE_BREAKAWAY_FROM_JOB啟動後 Toolhelp32 thread walk resume；ERROR_ACCESS_DENIED時退回 suspended-only）；POSIX 用start_new_session＋killpgSIGTERM→SIGKILL。timeout 時先殺樹、在_hold_windows_hoops_entrypoint_for_executionpin 區塊內 poll 到所有 descendant 消失才 raise；無法證明清空時 raise 獨立 fail-closed 錯誤碼converter_containment_failed（附殘存 PIDs），不偽裝成單純 timeout。tempfile 重導向保留（pipe 掛死不變式有專屬測試）。convert-ifc-to-usdc.ps1的Stop-Process -Id換成Stop-ConverterProcessTree——BFS child walk（Win32_Process／/procstat）、葉先殺、等到每個收集的 PID 觀察消失，殘存者具名進 timeout 訊息。subprocess.run(timeout=4)對同 fixture 留下活孫代（PID 44240 存活實錄）——證明新測試是真回歸守門。bim-streaming-server/**不在 mechanism regex；host-native-launcher 半部＝[gate #484] SEC-001+L1-COR-004: outer timeout 未含 Kit process tree、terminator 只證 parent — 需 Job Object containment #489-B，另隨 mechanism bundle）。AI Coding Governance
Machine values:
Change lane=F/B/G/S;Behavior contract changed=yes/no;Requirement source=issue/docs/plans/superpowers spec/existing contract/not applicable._run_powershell_conversion唯一 production caller＝同檔convert；Invoke-KitConversion唯一 caller＝同 ps1；新 symbol 無外部 caller）Frontend Verification
User-facing changes must pass two independent producers: real frontend/runtime operability evidence and the pinned
docs/plans/design-system-reference.manifest.jsonfidelity gate. Scope is derived from changed paths plus the base/head manifest union; the PR body cannot select an easier screen.mixedandpartial_reference_missingpermit honest partial work but requireFull completion claimed = no. Semantic evidence is produced only by thedesign-semantic-visualCI Playwright job, never supplied as PR input;PR Metadata Contractvalidates the live PR metadata, while normal protected CI checks determine mergeability.Deploy Path Verification
Required for runtime / Docker / Kit / viewer / ports / env / conversion-service changes.
Windows On-Demand Verification
Required when changed paths can alter Windows platform behavior. The tier is machine-derived from changed paths (
scripts/lib/windows-verification-scope.ps1); the highest match wins and the PR body cannot select an easier one. Docs and tests-only changes owe nothing.convert-ifc-to-usdc.ps1對真實 89.4MB IFC（storage/demo_lib_2026.ifc）在 Windows host-native Kit（RTX 4060 Ti、deploy 區 kit.exe＋omni.services.convert.cad-508.0.1）實跑：EXIT=0、39s、產出 28,462,810 bytesdemo_lib_2026.usdc，與 origin/main baseline 腳本同輸入實跑（EXIT=0、35s）byte 數一致；另 pytest 127 passed／7 skipped（三輪 review 修復後 floor 104→127），含真 process-tree fixture 與 ps1 AST 抽出Stop-ConverterProcessTree對 live 2-deep tree 實測 survivors=[]；r3 追加（head145381ab2143e2549a0f6eb9d0f2a21192052972）：本輪 diff 只動兩處——(a)convert-ifc-to-usdc.ps1的平台偵測改走Test-ConverterHostIsWindows（Get-Variable -ErrorAction SilentlyContinue探測，取代會在 Windows PowerShell 5.1 ＋Set-StrictMode -Version Latest下丟例外的裸$IsWindows解參考；Windows 分支的選擇結果不變），(b) POSIX 直接子行程的 reap 時序（os.waitid(..., WNOWAIT)觀察退出、sweep 之後才 reap；Windows Job Object 路徑一行未動）。轉檔語義因此不變，上方真 IFC→USDC 實跑證據仍成立；於此 head 另在同一台 Windows host 實跑：pwsh scripts/deploy.ps1 -DryRun→ EXIT=0（hybrid 模式、TCP/UDP 埠位全 FREE、storage root ALIGNED）；pytest bim-streaming-server/tests/test_host_native_conversion_service.py -q→ 130 passed／8 skipped（floor 127→130，新增 3 個 POSIX pgid 保留契約測試）；pwsh bim-streaming-server/scripts/tests/test-convert-ifc-to-usdc.ps1→ passed（新增Set-StrictMode -Version Latest下的平台探測覆蓋，加上對整份腳本 AST 掃描、斷言不存在任何裸$IsWindows解參考的回歸守門）；Invoke-ScriptAnalyzer -Severity Error對兩支 ps1 皆 0 findings；python -m py_compileadapter＋測試檔皆 OK。kit_gpu tier 的真 GPU 轉檔未於本輪重跑（此 worktree 無 kit.exe），誠實記錄為沿用前一 head 的實跑證據＋上述 diff 範圍論證；r4 追加（headb483e87798903efbed52d0028e1d8b1e70fd0a87）：本輪 diff 只動convert-ifc-to-usdc.ps1的Stop-ConverterProcessTree與其測試——多輪 discover→kill 迴圈在每次Stop-Process前重新驗證 (Id, StartTime) 行程身分，PID 已死或被回收即從後續回合、re-walk 種子與存活輪詢中剔除（修 P2：累積的 PID 集合每回合重殺，被回收的 PID 會讓 containment 殺到樹外的無關服務）；身分在任一側讀不到時退回既有的存在性檢查，因此不會少殺原本殺得到的行程。於此 head 在同一台 Windows host 實跑：pwsh bim-streaming-server/scripts/tests/test-convert-ifc-to-usdc.ps1→ passed（新增 stale-PID 身分回歸案例，並以 mutation 反證：把重驗改回無條件Stop-Process即 fail）；Invoke-ScriptAnalyzer -Severity Error對兩支 ps1 皆 0 findings；pwsh scripts/deploy.ps1 -DryRun→ EXIT=0（hybrid 模式、TCP/UDP 埠位全 FREE、storage root ALIGNED）。kit_gpu tier 的真 GPU 轉檔未於本輪重跑（此 worktree 無 kit.exe），轉檔成功路徑語義未變；r4 第二輪追加（head841277a7c6e8f4319e4f32e764e5b2ab8b8654b0）：reviewer 對上一個 commit 追加的第二個 P2——身分只在 kill 階段驗證仍嫌太晚，因為 re-walk 先跑：跨回合留存的 PID 若在本回合 walk 之前被回收，列舉它的子行程會把「無關行程的子代」以全新且自洽的身分收進 kill 集合，而反序 kill 會在辨識出被回收的 parent 之前先殺掉它們。修法：BFS 在展開任何 已知 PID 的子清單之前先重驗身分，recycled直接跳過不展開；gone仍照常展開（Windows 的 orphan 重新發現正是靠已死 parent 的 PID，case 2 回歸測試依賴此行為，且gone代表沒有任何行程持有該 PID，不可能經它觸及無關行程）。於此 head 實跑：pwsh bim-streaming-server/scripts/tests/test-convert-ifc-to-usdc.ps1→ passed（新增 case 5：以 shadowStop-Process記錄「被送出訊號的 PID」，斷言經由被回收 PID 觸及的無關子行程從未被列舉、未被送訊號、也未列入 survivors；mutation 反證：移除 walk 階段的重驗即 fail）；Invoke-ScriptAnalyzer -Severity Error兩支 ps1 皆 0 findings；pwsh scripts/deploy.ps1 -DryRun→ EXIT=0Self-Referential Bootstrap
Required when the PR changes the verification mechanism itself (deploy path / evidence harness / gate script). Rule:
docs/agents/self-referential-bootstrap.md. Open ledger debt inscripts/self-referential-bootstrap-ledger.jsonblocks further mechanism PRs until fixpoint closure.Validation
.venv\Scripts\python.exe -m pytest bim-streaming-server/tests/test_host_native_conversion_service.py -q→ 104 passed, 6 skipped（baseline 101/6；3 輪重跑穩定）。test_converter_subprocess_uses_file_redirection_not_pipes與 TimeoutExpired 測試保綠、原斷言全保留。test_stage_loading_stage_composition.py（缺 pytest-asyncio，未動的 main 上同樣重現 7 failed）。bim-streaming-server/scripts/tests/test-convert-ifc-to-usdc.ps1→ passed；PowerShell 5.1 AST parse OK。Known Risks
R3-2 行為變更（merge 前值得刻意一看）：containment 證明現在跑於所有離開路徑（含成功），無法證明邊界時 returncode 0 會轉
converter_containment_failed——唯一可能讓既有成功轉檔轉紅的變更；已以真行程＋真 Job Object 驗證成功路徑不受拖累。512-PID job 清單上限現為硬失敗邊界（正常個位數行程；踩到即異常訊號）。
POSIX 分支（killpg／/proc walk）本機為 win32 未實跑，code-review only；canonical-linux 驗證段補真跑。
新錯誤碼
converter_containment_failed經result.error.code上浮；grep 全 repo 無列舉 converter codes 的 contract，下游若有 switch-on-code 消費者需留意。Windows 若 real child 無法 resume（suspended），fail-closed 殺掉並回
converter_unavailable——本機實測 resume 正常，其他 Windows 組態未測。觀察備註：兩次在多 agent 高負載並行下的轉檔 wrapper 曾逾 10 分鐘未返回（被殺；USDC 已完整產出）；單獨重跑（run3）與 baseline 皆 34–35s 乾淨退出，root cause 未定，先誠實記錄供觀察，不影響本 diff 的 timeout 語義（該路徑有 bounded-deadline 測試鎖定）。
🤖 Generated with Claude Code