feat(deploy): harden canonical Linux test deployment - #484
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe PR adds canonical Linux deployment skills, trusted CAD entrypoint validation, host-native Kit Manager control handling, target-aware deployment verification, proxy-free runtime requests, and self-referential bootstrap evidence. ChangesLinux deployment security and verification
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant Operator
participant Deployment as scripts/deploy.ps1
participant Hardener as harden-cad-extension-cache.py
participant Adapter as Conversion adapter
participant Verifier as scripts/verify-all.ps1
participant KitGateway as KitRuntimeGateway
Operator->>Deployment: Start canonical Linux deployment
Deployment->>Hardener: Harden CAD extension cache
Hardener->>Adapter: Validate and harden HOOPS entrypoint
Adapter-->>Deployment: Return hardening status
Deployment->>Verifier: Run target-aware verification
Verifier->>KitGateway: Check runtime control status
KitGateway-->>Verifier: Return proxy-free runtime result
Verifier-->>Operator: Report deployment and health results
Possibly related issues
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 2📝 Generate docstrings 💡
🛠️ Fix failing CI checks 💡
🧪 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 |
There was a problem hiding this comment.
Pull request overview
This PR hardens the canonical Linux test-deployment path for the AI-BIM-governance workspace. It strengthens the trust boundary around NVIDIA's CAD converter entrypoint (hoops_main.py), makes deployment health verification target-aware and privacy-redacted with strict service-identity checks, pins the host-native Kit Manager's runtime identity, and adds a Codex operator skill for the owner-inventory SSH deployment workflow.
Changes:
- Adds pinned CAD entrypoint validation (exact package/size/SHA-256, symlink-escape rejection, owner-private permission checks) plus an atomic 0400-inode hardener, invoked from
deploy.ps1on the Linux conversion path and independently re-validated at runtime preflight/execution. - Makes
verify-all.ps1deployment health checks resolve targets via the registry, redact private host-native bind addresses, and assert typed JSON service identities; gives the host-native Kit Manager an explicit child-only environment restored infinally. - Adds the
deploy-linux-test-environmentCodex skill (manifest/gitignore/gitattributes entries) and broad test coverage.
Reviewed changes
Copilot reviewed 14 out of 16 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
ifc2usdc_powershell_adapter.py |
Pinned CAD entrypoint discovery/validation, atomic permission hardening, and post-preflight identity re-checks |
bim-streaming-server/scripts/harden-cad-extension-cache.py |
New CLI wrapper that hardens the entrypoint and emits a redacted result schema |
bim-streaming-server/config/trusted-cad-entrypoints.json |
New per-platform trusted package/digest manifest |
bim-streaming-server/source/apps/ezplus.bim_ifc_usd_converter.kit |
Trailing-newline-only change |
scripts/deploy.ps1 |
Runs the CAD hardener on the Linux conversion path after Kit build |
scripts/verify-all.ps1 |
Target-aware, redacted, redirect/proxy-safe health checks with typed service identity |
scripts/lib/host-native-launcher.ps1 |
Explicit Kit Manager child env with parent restoration |
bim-streaming-server/tests/test_host_native_conversion_service.py |
Tests for pinning, escapes, ambiguity, swap-after-preflight, and atomic hardening |
scripts/tests/test-verify-all.ps1, test-host-native-launcher.ps1, test-deploy-governance-static.ps1 |
Verifier/launcher/static assertions for the new behavior |
agent-skills-manifest.json, .gitignore, .gitattributes, .codex/skills/deploy-linux-test-environment/* |
New Codex deployment skill and its registration |
💡 Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (4)
bim-streaming-server/source/extensions/ezplus.bim_review_stream.messaging/ezplus/bim_review_stream/messaging/ifc2usdc_powershell_adapter.py (1)
538-548: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low valueBound the read by the expected size before loading the file into memory.
source_statis already available at Line 532. The loop reads the whole file intotrusted_bytesand only then compares the length toexpected_size. If the inode holds a much larger file, the process allocates all of it before rejecting it. Checksource_stat.st_sizeagainstexpected_sizefirst, and cap the read.♻️ Proposed refactor
+ if source_stat.st_size != expected_size: + raise ConversionAuthorityError( + "converter_unavailable", + "Pinned CAD extension entrypoint size does not match the tracked manifest.", + ) trusted_bytes = bytearray() while True: chunk = os.read(source_descriptor, 1024 * 1024) if not chunk: break trusted_bytes.extend(chunk) + if len(trusted_bytes) > expected_size: + break if len(trusted_bytes) != expected_size or hashlib.sha256(trusted_bytes).hexdigest() != expected_sha256:🤖 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 538 - 548, Update the trusted-byte loading logic near source_stat and the os.read loop: reject the source when source_stat.st_size differs from expected_size before allocating or reading its contents, then cap each read so the total trusted_bytes cannot exceed expected_size. Preserve the existing SHA-256 validation and ConversionAuthorityError behavior for invalid content.scripts/verify-all.ps1 (1)
109-118: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRename
$matchesto avoid the automatic variable.
$Matchesis a PowerShell automatic variable that the-matchand-notmatchoperators overwrite. The current loop body does not run-matchbetween the assignment on Line 109 and the read on Line 115, so the code works today. A later edit that adds a regex comparison inside the loop would silently corrupt the result. Use a local name.♻️ Proposed change
- $matches = if ($expectedValue -is [bool]) { + $propertyMatches = if ($expectedValue -is [bool]) { $actualValue -is [bool] -and $actualValue -eq $expectedValue } else { [string]$actualValue -ceq [string]$expectedValue } - if (-not $matches) { + if (-not $propertyMatches) { throw "response identity property '$propertyName' did not match the expected value" }Note:
scripts/tests/test-verify-all.ps1Line 171 asserts the boolean comparison text only, so this rename does not break that assertion.🤖 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 `@scripts/verify-all.ps1` around lines 109 - 118, Rename the local `$matches` variable in the response identity comparison block to a non-reserved local name, and update its subsequent negated check accordingly. Keep the boolean and string comparison logic unchanged.bim-streaming-server/tests/test_host_native_conversion_service.py (1)
682-686: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAlign the path comparison with the discovery return contract.
Line 485 compares
resolved_hoops_main.resolve()withhoops_main.resolve(). Line 684 compares the unresolved return value withhoops_main.resolve().harden_default_hoops_main_permissionsreturns the value of_default_hoops_main(), and the fixture reaches the leaf through a directory symlink inrelease_root. If discovery returns the symlinked path, this assertion fails. Use the same normalization in both tests.♻️ Proposed change
- assert hardened == hoops_main.resolve() - assert (hardened.stat().st_dev, hardened.stat().st_ino) != original_identity + assert hardened.resolve() == hoops_main.resolve() + assert (hardened.stat().st_dev, hardened.stat().st_ino) != original_identity🤖 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 682 - 686, Update the assertion for harden_default_hoops_main_permissions to compare hardened.resolve() with hoops_main.resolve(), matching the normalization used by the other discovery test while preserving the identity and permission assertions.scripts/tests/test-verify-all.ps1 (1)
159-159: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAnchor the private-address leak assertion.
192\.0\.2\.1also matches inside192.0.2.10, which is the fixturepublic_hoston Line 117. The current plan output does not printpublic_host, so the assertion passes. A future change that prints the public host would fail this assertion for the wrong reason. Add a boundary.♻️ Proposed change
- Assert-True ($deploymentPlan.Output -notmatch '192\.0\.2\.1') 'deployment profile never publishes the private host-native bind address' + Assert-True ($deploymentPlan.Output -notmatch '192\.0\.2\.1(?!\d)') 'deployment profile never publishes the private host-native bind address'🤖 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 `@scripts/tests/test-verify-all.ps1` at line 159, Update the private-address assertion in the deployment-plan test to match 192.0.2.1 only as a complete host value, using an appropriate boundary so it cannot match the public_host fixture 192.0.2.10.
🤖 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/scripts/harden-cad-extension-cache.py`:
- Around line 30-50: Ensure every execution path in the script emits the
`cad-extension-cache-hardening/v1` JSON contract, including import,
initialization, and unexpected failures. Add a shared status-emission helper and
place the import and `Ifc2UsdcPowershellConverterAdapter` setup within broad
exception handling, using a distinct `reason_kind` for unexpected errors while
preserving `ConversionAuthorityError.code`. Use the same helper for the success
branch so both success and failure output remain consistently structured.
In
`@bim-streaming-server/source/extensions/ezplus.bim_review_stream.messaging/ezplus/bim_review_stream/messaging/ifc2usdc_powershell_adapter.py`:
- Around line 197-215: Update _group_is_private_to_process to cache each
group_id result at instance or module scope, and return the cached value on
repeated checks. Treat an empty pwd.getpwall() result as not private before
evaluating group membership, while preserving the existing Windows and
lookup-error behavior.
In `@scripts/deploy.ps1`:
- Around line 1225-1236: Validate that the path returned by
Resolve-PlatformVenvPython exists and is executable before invoking the hardener
in the Phase 2 flow. If validation fails, log the failure and enter the existing
CAD hardening failure path with a nonzero exit result, preventing the success
tag from being emitted; only invoke the command and read $LASTEXITCODE after
validation succeeds.
In `@scripts/tests/test-host-native-launcher.ps1`:
- Around line 255-268: Update the Test 21 stubs around Resolve-HostNativePython
and Start-HostNativeKitManager so the test no longer depends on a real python
executable or installed fastapi/uvicorn packages. Route resolution through the
existing platform adapter or mock/extract the import probe, while preserving the
test’s focus on captured environment wiring.
In `@scripts/tests/test-verify-all.ps1`:
- Around line 104-107: Guard the default Deployment plan assertions in the test
block around Invoke-VerificationPlan with a Windows-only condition, so they run
only when the current platform resolves the Windows target; alternatively,
invoke the plan with an explicit loopback fixture target. Ensure Linux runs do
not fail or skip the subsequent Linux fixture assertions.
---
Nitpick comments:
In
`@bim-streaming-server/source/extensions/ezplus.bim_review_stream.messaging/ezplus/bim_review_stream/messaging/ifc2usdc_powershell_adapter.py`:
- Around line 538-548: Update the trusted-byte loading logic near source_stat
and the os.read loop: reject the source when source_stat.st_size differs from
expected_size before allocating or reading its contents, then cap each read so
the total trusted_bytes cannot exceed expected_size. Preserve the existing
SHA-256 validation and ConversionAuthorityError behavior for invalid content.
In `@bim-streaming-server/tests/test_host_native_conversion_service.py`:
- Around line 682-686: Update the assertion for
harden_default_hoops_main_permissions to compare hardened.resolve() with
hoops_main.resolve(), matching the normalization used by the other discovery
test while preserving the identity and permission assertions.
In `@scripts/tests/test-verify-all.ps1`:
- Line 159: Update the private-address assertion in the deployment-plan test to
match 192.0.2.1 only as a complete host value, using an appropriate boundary so
it cannot match the public_host fixture 192.0.2.10.
In `@scripts/verify-all.ps1`:
- Around line 109-118: Rename the local `$matches` variable in the response
identity comparison block to a non-reserved local name, and update its
subsequent negated check accordingly. Keep the boolean and string comparison
logic unchanged.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 1db1ab5a-4556-44dc-a2f8-5348a140c119
📒 Files selected for processing (16)
.codex/skills/deploy-linux-test-environment/SKILL.md.codex/skills/deploy-linux-test-environment/agents/openai.yaml.gitattributes.gitignoreagent-skills-manifest.jsonbim-streaming-server/config/trusted-cad-entrypoints.jsonbim-streaming-server/scripts/harden-cad-extension-cache.pybim-streaming-server/source/apps/ezplus.bim_ifc_usd_converter.kitbim-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.pyscripts/deploy.ps1scripts/lib/host-native-launcher.ps1scripts/tests/test-deploy-governance-static.ps1scripts/tests/test-host-native-launcher.ps1scripts/tests/test-verify-all.ps1scripts/verify-all.ps1
![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: 36ed788e77
ℹ️ 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: 2c6c96392f
ℹ️ 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: 9225480416
ℹ️ 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: aeb02d8828
ℹ️ 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.
Actionable comments posted: 9
🤖 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 @.claude/skills/deploy-linux-test-environment/agents/openai.yaml:
- Around line 2-4: Update the default_prompt in the deploy skill metadata to be
action-neutral and invoke $deploy-linux-test-environment for read-only preflight
or status inspection by default. Do not instruct rebuilding, cleanup, service
restarts, verification after mutation, or tag creation unless the user
explicitly requests a rebuild.
In @.claude/skills/deploy-linux-test-environment/SKILL.md:
- Around line 16-25: Update the deployment workflow instructions to require a
pre-deploy verification gate before any remote checkout reset, rebuild, or other
mutation: run affected type checks, lint, and unit/integration checks first,
fail closed on any failure, and explicitly report each skipped or unrun check
with its reason. Anchor this requirement to the “Load current truth” workflow
and preserve the existing verification-source ordering.
- Around line 29-31: Update the deployment reconstruction flow to create a fresh
sibling worktree from the captured, freshly fetched origin/main SHA, then verify
that worktree’s HEAD matches the SHA and its status is clean before invoking
rebuild-test-deploy.ps1. Run the wrapper from this isolated worktree instead of
the current branch worktree, while preserving the existing handle and
input-integrity checks.
- Line 82: Update the final report guidance in the deployment skill to redact
private paths: show a command template with placeholders instead of expanded
inventoryPath, identityFile, or deploy_root values, and report snapshot/log
artifacts using repository-relative paths. Keep raw commands and absolute paths
only in protected local evidence, while preserving the requirement to include
the rebuild command and artifact locations.
In
`@docs/evidence/linux-test-deploy-verifier-hardening/self-referential-bootstrap/README.md`:
- Line 1: Add explicit document metadata to the README heading, declaring
“Document nature: working note,” and add equivalent machine-readable metadata at
the start of verification.txt without disrupting its existing evidence format.
Ensure both documents clearly remain evidence rather than authoritative runtime
or API specifications. Apply changes to
docs/evidence/linux-test-deploy-verifier-hardening/self-referential-bootstrap/README.md
(lines 1-1) and verification.txt (lines 1-1).
- Around line 14-16: Update README.md lines 14-16 to document git fetch origin
--prune, sibling worktree creation from origin/main, matching git rev-parse HEAD
and origin/main values, an empty git status --porcelain before rebuild, the
rebuild-test-deploy.ps1 -Build command, and the owner-controlled private
canonical-linux inventory; do not present reset/clean of a dirty checkout as
isolated-baseline evidence. Update verification.txt line 18 to include the exact
canonical rebuild command, target, inventory source, and source commit, or
explicitly mark canonical-linux-rebuild as bootstrap evidence pending fixpoint.
In
`@docs/evidence/linux-test-deploy-verifier-hardening/self-referential-bootstrap/verification.txt`:
- Around line 5-17: Update the self-referential bootstrap verification report to
explain the single skipped test and explicitly record whether the affected
Python and PowerShell type-check and lint checks ran. For every check that did
not run, include its reason; otherwise record its result, preserving the
existing check results.
In `@scripts/self-referential-bootstrap-ledger.json`:
- Around line 109-131: Update the ledger entry’s verification_mechanism_paths to
include every changed runtime and test implementation file underlying the listed
command_ids, including harden-cad-extension-cache.py and
test_host_native_conversion_service.py. If any paths belong to a separate scope,
move them to a distinct ledger entry with its own fixpoint obligation.
In `@scripts/tests/test-self-referential-bootstrap.ps1`:
- Around line 400-401: Update commandPathById and the self-referential bootstrap
assertion to preserve canonical deployment invocation details, not just source
paths. Ensure the command runner generates the rebuild command with -Build and
inventory/target metadata, and invokes scripts/verify-all.ps1 with the
Deployment profile plus TargetId and InventoryPath; alternatively store this
invocation metadata alongside each mapped command and assert it.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: c719d7f4-c921-475d-845d-e5e7b12f0269
📒 Files selected for processing (9)
.claude/skills/deploy-linux-test-environment/SKILL.md.claude/skills/deploy-linux-test-environment/agents/openai.yaml.gitattributes.gitignoreagent-skills-manifest.jsondocs/evidence/linux-test-deploy-verifier-hardening/self-referential-bootstrap/README.mddocs/evidence/linux-test-deploy-verifier-hardening/self-referential-bootstrap/verification.txtscripts/self-referential-bootstrap-ledger.jsonscripts/tests/test-self-referential-bootstrap.ps1
🚧 Files skipped from review as they are similar to previous changes (2)
- .gitattributes
- agent-skills-manifest.json
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ce30c055af
ℹ️ 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.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (6)
.claude/skills/deploy-linux-test-environment/SKILL.md (4)
117-117: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winRequire explicit approval for the ACL mutation.
Creating the destination with a protected ACL changes ACL state. The approval described here covers file creation and removal, but it does not explicitly cover the ACL change. Require approval that names creation, ACL assignment, and cleanup. Otherwise, mark the workflow
HELD.As per coding guidelines, ACL changes require explicit approval and the workflow must not automatically alter ACLs.
🤖 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 @.claude/skills/deploy-linux-test-environment/SKILL.md at line 117, Update the destination-creation workflow guidance to require explicit approval covering file creation, protected ACL assignment, and temporary-copy cleanup before proceeding. If approval does not explicitly include the ACL mutation, mark the workflow HELD and do not automatically alter ACLs.Source: Coding guidelines
162-162: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winRequire the operability gate before claiming full-system E2E.
The requirement lists semantic E2E, first frame, USD stage, DataChannel acknowledgement, and design-fidelity evidence. It does not require the operability gate. HTTP 200 checks do not prove Review Room or Edge Console operation.
Require both design-fidelity and operability evidence. If scope or either gate is unknown, report
Full-system E2E claimed: no.As per coding guidelines, user-facing completion requires both the design-fidelity gate and the operability gate, and unknown scope must fail closed.
🤖 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 @.claude/skills/deploy-linux-test-environment/SKILL.md at line 162, Update the full-system E2E claim requirement in the deployment guidance to require both design-fidelity evidence and the operability gate, including proof that Review Room and Edge Console operate beyond HTTP 200 checks. If the scope or either gate is unknown, require reporting “Full-system E2E claimed: no.”Source: Coding guidelines
150-156: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winSynchronize the conversion-health instruction with the verifier.
.claude/skills/deploy-linux-test-environment/SKILL.mdsays the deployment profile keeps conversion checks on loopback at line 21, then line 27 requires conversion health through the public route.scripts/verify-all.ps1usesconversionHealthHostfrombim-streaming-conversion-service.params.jsonand asserts it is local, so the documented public-route expectation conflicts with the verifier’s intended loopback binding. State one route explicitly, or add a second requirement if both loopback and public checks are required.🤖 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 @.claude/skills/deploy-linux-test-environment/SKILL.md around lines 150 - 156, Update the conversion health verification instruction near the coordinator and viewer checks to match the verifier’s loopback behavior: require HTTP 200 from conversion at :49101 through the local/loopback route, not the public route. Keep the existing coordinator and viewer role/port/result requirements unchanged.Source: Coding guidelines
100-100: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winRequest
READ_CONTROLfor the security descriptor query.
GetFileSecurityFromHandlefrom this same handle requires the handle to have access to the security descriptor, whichFILE_READ_ATTRIBUTESdoes not provide. RequestREAD_CONTROL | FILE_READ_ATTRIBUTES, or query owner/DACL from a separate security-descriptor handle or path.Proposed access-mask correction
-CreateFileW(FILE_READ_ATTRIBUTES, FILE_SHARE_READ | FILE_SHARE_WRITE, OPEN_EXISTING, FILE_FLAG_BACKUP_SEMANTICS | FILE_FLAG_OPEN_REPARSE_POINT) +CreateFileW(READ_CONTROL | FILE_READ_ATTRIBUTES, FILE_SHARE_READ | FILE_SHARE_WRITE, OPEN_EXISTING, FILE_FLAG_BACKUP_SEMANTICS | FILE_FLAG_OPEN_REPARSE_POINT)🤖 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 @.claude/skills/deploy-linux-test-environment/SKILL.md at line 100, Update the selected private-root handle opened by the validation procedure to request READ_CONTROL together with FILE_READ_ATTRIBUTES, preserving the existing sharing, creation, and flag settings so GetFileSecurityFromHandle can query the owner and DACL from that handle.bim-streaming-server/tests/test_host_native_conversion_service.py (1)
791-794: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winAdd a POSIX-only marker to the atomic hardening test.
Line 794 hardcodes the
linux-x86_64release directory. On Windows,_default_release_rootreturnswindows-x86_64, so_discover_default_hoops_mainfinds no candidate andharden_default_hoops_main_permissionsraisesconverter_unavailable. Line 804 also relies on POSIX mode bits.The neighbouring hardener test at line 816 carries
@pytest.mark.skipif(os.name == "nt", ...). Apply the same marker here.💚 Proposed fix
+@pytest.mark.skipif(os.name == "nt", reason="POSIX descriptor hardening contract") def test_hardener_atomically_replaces_pinned_entrypoint_with_private_inode( tmp_path: Path, monkeypatch ): release_root = tmp_path / "_build" / "linux-x86_64" / "release"🤖 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 791 - 794, Add the same `@pytest.mark.skipif`(os.name == "nt", ...) decorator used by the neighbouring hardener test to test_hardener_atomically_replaces_pinned_entrypoint_with_private_inode, preserving the test’s POSIX-only behavior and existing implementation.bim-streaming-server/source/extensions/ezplus.bim_review_stream.messaging/ezplus/bim_review_stream/messaging/ifc2usdc_powershell_adapter.py (1)
709-717: 🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy liftAn explicitly configured
hoops_main_pathbypasses the pinned-package trust boundary.When
self.hoops_main_pathis set, line 717 assigns it directly toeffective_hoops. The code then only resolves the path and records its identity at lines 727-733. It never calls_trusted_cad_entrypointor_verify_cad_entrypoint_digestfor that path. The pinned package name, size, and SHA-256 are not checked.The docstring at lines 510-511 states that runtime discovery never accepts an unpinned digest. That guarantee holds only for the discovery path.
scripts/deploy.ps1lines 1301-1306 rejectSTREAMING_CONVERSION_HOOPS_MAINon the canonical Linux path, so the deployed configuration is covered. Any other construction of the adapter withhoops_main_pathstill runs an unverified entrypoint.Apply the same digest verification to the configured path, or document that the configured path is an unpinned developer-only escape hatch.
🤖 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 709 - 717, Update the configured-path branch in the adapter initialization flow so `self.hoops_main_path` is validated through the same pinned-package trust checks as `_default_hoops_main`, including `_trusted_cad_entrypoint` and `_verify_cad_entrypoint_digest`; retain the existing `ConversionAuthorityError` handling and failure state, and only assign a configured path to `effective_hoops` after verification succeeds.
🧹 Nitpick comments (3)
bim-streaming-server/scripts/harden-cad-extension-cache.py (1)
60-60: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMark the intentional broad exception handler for Ruff.
Line 60 is required to convert unexpected failures into the JSON status contract. Ruff reports
BLE001here. Add a scoped# noqa: BLE001with a rationale, or configure this handler explicitly. Do not narrow the handler without preserving the fallback contract.Proposed fix
- except Exception as exc: + # Preserve the machine-readable failure contract for unexpected errors. + except Exception as exc: # noqa: BLE001🤖 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/harden-cad-extension-cache.py` at line 60, Add a scoped Ruff suppression for BLE001 on the broad exception handler `except Exception as exc`, including a brief rationale that it preserves the JSON status fallback contract; keep the broad handler and its existing fallback behavior unchanged.Source: Linters/SAST tools
scripts/lib/host-native-launcher.ps1 (1)
543-546: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winApply the same import-probe fix to
Start-HostNativeGovernance.Lines 543-546 replace the stale
$LASTEXITCODEcheck with an injectable process probe that reads a realExitCode.Start-HostNativeGovernanceat lines 470-474 still uses& $pythonExe -c "..." *> $nullfollowed by$LASTEXITCODE.
deploy.ps1sets$ErrorActionPreference = 'Continue'. If the interpreter cannot be launched there,$LASTEXITCODEkeeps its previous value. A prior successful native command then makes the governance import check pass, and the service dies at import instead. The failure surfaces 30 seconds later as a Phase 4a health timeout.Reuse the new
ImportProbeFnpattern for governance so both launchers share one probe contract.🤖 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 `@scripts/lib/host-native-launcher.ps1` around lines 543 - 546, Update Start-HostNativeGovernance to use the injectable ImportProbeFn and its returned real ExitCode, replacing the direct python invocation and $LASTEXITCODE check. Preserve the existing governance import validation and failure behavior, matching the probe contract already used by the surrounding launcher.scripts/deploy.ps1 (1)
566-591: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winPreserve the hardener failure reason and reconsider the stderr rule.
Two points about this result-interpretation block:
- Lines 566-570 discard the caught exception. The caller at line 1331 then logs only
CAD extension cache permission hardening failed. On a remote canonical deploy the operator loses the actual cause. Capture$_.Exception.Messageand return it so the failure tag can include it.- Line 578 requires
[string]::IsNullOrWhiteSpace($stderr). Any benign warning that Python writes to stderr, for example aDeprecationWarningfrom an imported module, invalidates a successful hardening and aborts the deploy with exit 2. Decide whether stderr silence is a required part of the contract, or gate only on the exit code and the single JSON status line.🤖 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 `@scripts/deploy.ps1` around lines 566 - 591, Update the hardener result-interpretation block to preserve the caught exception message from the catch handler and return or propagate it so the caller’s failure log includes the actual cause. Reconsider the [string]::IsNullOrWhiteSpace($stderr) requirement in the $exitCode/$lines validation, retaining it only if stderr silence is an explicit contract; otherwise validate successful execution using the zero exit code and single valid JSON status line while tolerating benign warnings.
🤖 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 @.claude/skills/deploy-linux-test-environment/SKILL.md:
- Around line 39-55: Update the deployment setup block to capture the caller’s
exact cwd, current branch, and porcelain worktree status before git fetch or
Set-Location, alongside the existing sourceRepoRoot. Validate each provenance
command and mark deployment HELD on failure, then retain these captured values
for inclusion in the final report rather than reporting only the isolated
worktree state.
- Around line 52-54: Update the isolated deployment worktree validation around
the git rev-parse and git status commands to run them separately and capture
each command’s output and exit status. Check $LASTEXITCODE after both commands,
and throw the existing deployment-held error if either command fails; only
compare the resolved HEAD and clean-status output after both checks succeed.
In `@scripts/deploy.ps1`:
- Around line 556-573: Bound child-process waits in both helpers: in
scripts/deploy.ps1 lines 556-573, update Invoke-CadExtensionCacheHardener to
accept a timeout, use the timeout-based WaitForExit call, and kill the process
tree when it expires; in scripts/lib/host-native-launcher.ps1 lines 512-520,
apply the same bounded wait to the default ImportProbeFn’s $importProcess and
return a non-zero exit code on timeout.
- Around line 1310-1317: Update the hardener interpreter readiness check around
$hardenerPythonReady to verify that [System.IO.File]::GetUnixFileMode is
available before invoking it, preserving the existing executable-bit check when
supported and avoiding a false “missing or not executable” failure on older
PowerShell/.NET runtimes.
In `@scripts/tests/test-rebuild-test-deploy.ps1`:
- Around line 271-273: Update the Start-HostNativeKitManager test double and its
host-native launch scenario to retain the received KitControlUrl and assert it
matches the expected canonical control URL, ensuring empty or incorrect caller
values fail the test.
In `@services/kit-manager-api/app/kit_gateway.py`:
- Line 12: Update the opener initialization in the gateway class to install a
no-op HTTP redirect handler alongside ProxyHandler({}), preventing redirects
from the Kit control authority from being followed. Extend the existing redirect
test to verify the gateway rejects or does not follow a redirect and therefore
never contacts the redirected authority.
---
Outside diff comments:
In @.claude/skills/deploy-linux-test-environment/SKILL.md:
- Line 117: Update the destination-creation workflow guidance to require
explicit approval covering file creation, protected ACL assignment, and
temporary-copy cleanup before proceeding. If approval does not explicitly
include the ACL mutation, mark the workflow HELD and do not automatically alter
ACLs.
- Line 162: Update the full-system E2E claim requirement in the deployment
guidance to require both design-fidelity evidence and the operability gate,
including proof that Review Room and Edge Console operate beyond HTTP 200
checks. If the scope or either gate is unknown, require reporting “Full-system
E2E claimed: no.”
- Around line 150-156: Update the conversion health verification instruction
near the coordinator and viewer checks to match the verifier’s loopback
behavior: require HTTP 200 from conversion at :49101 through the local/loopback
route, not the public route. Keep the existing coordinator and viewer
role/port/result requirements unchanged.
- Line 100: Update the selected private-root handle opened by the validation
procedure to request READ_CONTROL together with FILE_READ_ATTRIBUTES, preserving
the existing sharing, creation, and flag settings so GetFileSecurityFromHandle
can query the owner and DACL from that handle.
In
`@bim-streaming-server/source/extensions/ezplus.bim_review_stream.messaging/ezplus/bim_review_stream/messaging/ifc2usdc_powershell_adapter.py`:
- Around line 709-717: Update the configured-path branch in the adapter
initialization flow so `self.hoops_main_path` is validated through the same
pinned-package trust checks as `_default_hoops_main`, including
`_trusted_cad_entrypoint` and `_verify_cad_entrypoint_digest`; retain the
existing `ConversionAuthorityError` handling and failure state, and only assign
a configured path to `effective_hoops` after verification succeeds.
In `@bim-streaming-server/tests/test_host_native_conversion_service.py`:
- Around line 791-794: Add the same `@pytest.mark.skipif`(os.name == "nt", ...)
decorator used by the neighbouring hardener test to
test_hardener_atomically_replaces_pinned_entrypoint_with_private_inode,
preserving the test’s POSIX-only behavior and existing implementation.
---
Nitpick comments:
In `@bim-streaming-server/scripts/harden-cad-extension-cache.py`:
- Line 60: Add a scoped Ruff suppression for BLE001 on the broad exception
handler `except Exception as exc`, including a brief rationale that it preserves
the JSON status fallback contract; keep the broad handler and its existing
fallback behavior unchanged.
In `@scripts/deploy.ps1`:
- Around line 566-591: Update the hardener result-interpretation block to
preserve the caught exception message from the catch handler and return or
propagate it so the caller’s failure log includes the actual cause. Reconsider
the [string]::IsNullOrWhiteSpace($stderr) requirement in the $exitCode/$lines
validation, retaining it only if stderr silence is an explicit contract;
otherwise validate successful execution using the zero exit code and single
valid JSON status line while tolerating benign warnings.
In `@scripts/lib/host-native-launcher.ps1`:
- Around line 543-546: Update Start-HostNativeGovernance to use the injectable
ImportProbeFn and its returned real ExitCode, replacing the direct python
invocation and $LASTEXITCODE check. Preserve the existing governance import
validation and failure behavior, matching the probe contract already used by the
surrounding launcher.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: f7767b3a-c368-49ba-9d68-9b024dcf9233
📒 Files selected for processing (21)
.claude/skills/deploy-linux-test-environment/SKILL.md.claude/skills/deploy-linux-test-environment/agents/openai.yaml.codex/skills/deploy-linux-test-environment/SKILL.md.codex/skills/deploy-linux-test-environment/agents/openai.yamlagent-skills-manifest.jsonbim-streaming-server/scripts/harden-cad-extension-cache.pybim-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.pydocs/evidence/linux-test-deploy-verifier-hardening/self-referential-bootstrap/README.mddocs/evidence/linux-test-deploy-verifier-hardening/self-referential-bootstrap/verification.txtscripts/deploy.ps1scripts/lib/host-native-launcher.ps1scripts/self-referential-bootstrap-ledger.jsonscripts/tests/test-deploy-governance-static.ps1scripts/tests/test-host-native-launcher.ps1scripts/tests/test-rebuild-test-deploy.ps1scripts/tests/test-self-referential-bootstrap.ps1scripts/tests/test-verify-all.ps1scripts/verify-all.ps1services/kit-manager-api/app/kit_gateway.pyservices/kit-manager-api/tests/test_kit_service_runtime_status.py
🚧 Files skipped from review as they are similar to previous changes (6)
- agent-skills-manifest.json
- scripts/self-referential-bootstrap-ledger.json
- docs/evidence/linux-test-deploy-verifier-hardening/self-referential-bootstrap/verification.txt
- docs/evidence/linux-test-deploy-verifier-hardening/self-referential-bootstrap/README.md
- .codex/skills/deploy-linux-test-environment/agents/openai.yaml
- scripts/verify-all.ps1
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9df4b9f768
ℹ️ 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: a40fabe69f
ℹ️ 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.
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/source/extensions/ezplus.bim_review_stream.messaging/ezplus/bim_review_stream/messaging/ifc2usdc_powershell_adapter.py (1)
716-733: 🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy liftValidate configured HOOPS paths against the same trust policy.
Line 717 accepts
self.hoops_main_pathwithout package, trusted-root, owner, permission, size, or SHA-256 validation. Lines 727-733 only record the identity of that untrusted file. A writable configured file can therefore pass preflight and reach-HoopsMainPathat Line 1017.Apply the manifest and owner-private validation to configured paths before calculating the execution identity. Add a test that an existing arbitrary configured file fails before PowerShell starts.
🤖 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 716 - 733, Update the configured-path branch around effective_hoops validation to apply the same manifest, trusted-root, owner-private permissions, size, and SHA-256 checks used for packaged HOOPS files before calling _hoops_file_identity or assigning validated_hoops_main. Ensure an existing arbitrary self.hoops_main_path is rejected during preflight and cannot reach the -HoopsMainPath execution path, and add a test verifying PowerShell is not started.
🤖 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.
Outside diff comments:
In
`@bim-streaming-server/source/extensions/ezplus.bim_review_stream.messaging/ezplus/bim_review_stream/messaging/ifc2usdc_powershell_adapter.py`:
- Around line 716-733: Update the configured-path branch around effective_hoops
validation to apply the same manifest, trusted-root, owner-private permissions,
size, and SHA-256 checks used for packaged HOOPS files before calling
_hoops_file_identity or assigning validated_hoops_main. Ensure an existing
arbitrary self.hoops_main_path is rejected during preflight and cannot reach the
-HoopsMainPath execution path, and add a test verifying PowerShell is not
started.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 4905932f-f44e-4cdf-b3f8-2779acfc5efb
📒 Files selected for processing (5)
bim-streaming-server/source/extensions/ezplus.bim_review_stream.messaging/ezplus/bim_review_stream/messaging/ifc2usdc_powershell_adapter.pyscripts/tests/test-deploy-governance-static.ps1scripts/tests/test-host-native-launcher.ps1scripts/tests/test-self-referential-bootstrap.ps1scripts/tests/test-verify-all.ps1
🚧 Files skipped from review as they are similar to previous changes (3)
- scripts/tests/test-self-referential-bootstrap.ps1
- scripts/tests/test-verify-all.ps1
- scripts/tests/test-deploy-governance-static.ps1
Closes the outstanding PR #484 review threads: - validate Windows owner/DACL per path component before trusting the CAD extension cache instead of returning unconditional success on nt - keep the pinned entrypoint inode when it is already 0400, so an idempotent redeploy cannot interrupt an in-flight conversion - detect NTFS junctions through the reparse attribute on Python 3.11, where Path.is_junction does not exist - reject redirects from the Kit control authority in KitRuntimeGateway - bound the CAD hardener and Kit Manager import-probe child waits, killing the process tree and failing closed on timeout - drop the GetUnixFileMode probe, which is .NET 7+/PowerShell 7.3+ only and aborted every conversion-enabled deploy on PowerShell 7.0-7.2 - require a revision in each deployment runtime signature and reject a runtime from another checkout - mirror --target-id and --inventory-path in scripts/verify-all.sh - assert the forwarded canonical Kit control URL in the rebuild harness - give the deploy-linux-test-environment skill caller provenance capture, exit-code-checked git probes, a finally-based worktree/branch closeout, a report template that mirrors the actual invocation, and an isolated-worktree canonical env destination
…y-verifier-hardening debt entry
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
scripts/tests/test-verify-all.ps1 (1)
333-360: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winOn Windows the execution matrix ignores the canonical-Linux fixture it just validated.
Lines 340-355 build and validate
$executionInventoryPathforcanonical-linux. Lines 357-360 then attach-InventoryPathonly when-not $IsWindows. On a Windows runner,Invoke-VerificationExecutionruns with no target arguments and resolves the current-platform target instead. The runtime-signature rejection matrix at Lines 434-441 therefore exercises thelocal-windowspath, and the validated canonical-Linux fixture is never used.The canonical operator workstation is Windows, so this is the path that runs in practice. Either pass
-TargetId canonical-linux -InventoryPath $executionInventoryPathon both platforms, or state in a comment why the Windows run must use the platform default and assert which target the execution resolved.🤖 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 `@scripts/tests/test-verify-all.ps1` around lines 333 - 360, The execution matrix validates the canonical-linux fixture but omits it on Windows, causing verification to use the platform-default target. Update the `$executionArguments` construction and its `Invoke-VerificationExecution` caller to pass `-TargetId 'canonical-linux' -InventoryPath $executionInventoryPath` on both platforms, preserving any required non-Windows arguments.
🧹 Nitpick comments (4)
.codex/skills/deploy-linux-test-environment/SKILL.md (1)
182-232: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDocument the
git branch -dprecondition for the cleanup step.Line 218 uses
git branch -d. Git refuses this delete when the branch is not merged into the currentHEADor its upstream. The isolated branch points atorigin/main. If the caller worktree is checked out at a revision that does not containorigin/main, the delete fails and the closeout reportsHELDeven though nothing changed. The fail-closed outcome is correct, but the operator needs to know this cause. Add one sentence that names this condition, so the operator does not treat it as tampering evidence.🤖 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 @.codex/skills/deploy-linux-test-environment/SKILL.md around lines 182 - 232, Add a sentence in the “Close the isolated worktree” cleanup documentation explaining that `git branch -d` can fail when the caller’s current HEAD or upstream does not contain the isolated branch’s `origin/main` commit; identify this as a non-tampering cause of the resulting HELD status.services/kit-manager-api/app/kit_gateway.py (1)
40-43: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueThe
failed_http_{status}branch is unreachable through the opener.
OpenerDirector.openinstallsHTTPDefaultErrorHandler, which raisesHTTPErrorfor any status at or above 400.HTTPErroris a subclass ofURLError, so control reaches Line 44 and the gateway returnsblocked_runtime_control_unavailable. Lines 41-42 never run. A Kit control that answers with 403 or 500 is therefore reported as unreachable rather than as an explicit rejection.The behavior matches the previous
urlopencall, so this is not a regression. Consider catchingHTTPErrorbeforeURLErrorto keep the distinct status, or remove the dead branch.♻️ Proposed change
try: with self._opener.open(request, timeout=self.timeout_seconds) as response: - if response.status >= 400: - return f"failed_http_{response.status}" return "sent" + except HTTPError as exc: + return f"failed_http_{exc.code}" except URLError: return "blocked_runtime_control_unavailable"Import
HTTPErrorfromurllib.erroralongsideURLError. Note that the redirect rejection surfaces asHTTPErrorwith code 302, so update the redirect test expectation if you apply this change.🤖 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 `@services/kit-manager-api/app/kit_gateway.py` around lines 40 - 43, Update the request handling around KitGateway’s opener call to catch urllib.error.HTTPError before URLError and return failed_http_{status} using the error’s status code, preserving distinct handling for HTTP 4xx/5xx responses. Also account for redirect rejections surfaced as HTTPError code 302 by updating the corresponding redirect test expectation.bim-streaming-server/source/extensions/ezplus.bim_review_stream.messaging/ezplus/bim_review_stream/messaging/ifc2usdc_powershell_adapter.py (1)
1022-1038: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueChain the fallback anchor error to its cause.
Line 1035 raises a new
ConversionAuthorityErrorinside anexcept ConversionAuthorityErrorblock withoutfrom. The original anchor failure is then hidden in the traceback. Ruff reports this asB904. Bind the caught error and chain it.♻️ Proposed change
try: anchor = self._trusted_directory_anchor(candidate) - except ConversionAuthorityError: + except ConversionAuthorityError as anchor_error: if self.hoops_main_path is None: raise try: anchor = candidate.parent.resolve(strict=True) except OSError as exc: raise ConversionAuthorityError( "converter_unavailable", "Configured HOOPS entrypoint parent could not be resolved safely.", ) from exc if not self._path_components_are_owner_private(anchor, anchor): raise ConversionAuthorityError( "converter_unavailable", "Configured HOOPS entrypoint parent is not owner-private.", - ) + ) from anchor_error🤖 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 1022 - 1038, Update the inner fallback handler in the anchor resolution flow to bind the outer ConversionAuthorityError and chain the new ConversionAuthorityError raised for an unsafe HOOPS parent to that caught cause using explicit exception chaining, satisfying B904 while preserving existing behavior.Source: Linters/SAST tools
services/kit-manager-api/tests/test_kit_service_runtime_status.py (1)
139-184: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueStart both servers inside the
tryblock, and silence the RuffA002hit.Two small points:
- Lines 153-155 start the destination server before the
tryat Line 171. IfThreadingHTTPServer(...)at Line 168 raises, the destination server and its thread are never closed. Move the destination server setup inside thetry, or manage both servers withcontextlib.ExitStack.- Ruff reports
A002at Lines 150 and 165 becauseformatshadows a builtin. The name comes from theBaseHTTPRequestHandler.log_messagesignature, so keep it and add# noqa: A002.The redirect assertions themselves are correct. The rejected redirect surfaces as
HTTPError, whichURLErrorhandling maps toblocked_runtime_control_unavailable, and the destination counter proves the second authority was never contacted.🤖 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 `@services/kit-manager-api/tests/test_kit_service_runtime_status.py` around lines 139 - 184, Update test_configured_gateway_rejects_redirects_without_contacting_the_destination to create both HTTP servers and their threads inside the try block, ensuring cleanup runs if either setup fails; retain the existing finally cleanup for successfully initialized resources. Keep the required BaseHTTPRequestHandler.log_message parameter name format and add the targeted noqa A002 suppression to both handler methods.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 `@scripts/verify-all.sh`:
- Around line 104-109: Add scripts/verify-all.sh to the
linux-test-deploy-verifier-hardening entry in
scripts/self-referential-bootstrap-ledger.json, alongside
scripts/verify-all.ps1. Rerun the governance check to confirm the bootstrap
ledger covers this POSIX adapter.
---
Outside diff comments:
In `@scripts/tests/test-verify-all.ps1`:
- Around line 333-360: The execution matrix validates the canonical-linux
fixture but omits it on Windows, causing verification to use the
platform-default target. Update the `$executionArguments` construction and its
`Invoke-VerificationExecution` caller to pass `-TargetId 'canonical-linux'
-InventoryPath $executionInventoryPath` on both platforms, preserving any
required non-Windows arguments.
---
Nitpick comments:
In @.codex/skills/deploy-linux-test-environment/SKILL.md:
- Around line 182-232: Add a sentence in the “Close the isolated worktree”
cleanup documentation explaining that `git branch -d` can fail when the caller’s
current HEAD or upstream does not contain the isolated branch’s `origin/main`
commit; identify this as a non-tampering cause of the resulting HELD status.
In
`@bim-streaming-server/source/extensions/ezplus.bim_review_stream.messaging/ezplus/bim_review_stream/messaging/ifc2usdc_powershell_adapter.py`:
- Around line 1022-1038: Update the inner fallback handler in the anchor
resolution flow to bind the outer ConversionAuthorityError and chain the new
ConversionAuthorityError raised for an unsafe HOOPS parent to that caught cause
using explicit exception chaining, satisfying B904 while preserving existing
behavior.
In `@services/kit-manager-api/app/kit_gateway.py`:
- Around line 40-43: Update the request handling around KitGateway’s opener call
to catch urllib.error.HTTPError before URLError and return failed_http_{status}
using the error’s status code, preserving distinct handling for HTTP 4xx/5xx
responses. Also account for redirect rejections surfaced as HTTPError code 302
by updating the corresponding redirect test expectation.
In `@services/kit-manager-api/tests/test_kit_service_runtime_status.py`:
- Around line 139-184: Update
test_configured_gateway_rejects_redirects_without_contacting_the_destination to
create both HTTP servers and their threads inside the try block, ensuring
cleanup runs if either setup fails; retain the existing finally cleanup for
successfully initialized resources. Keep the required
BaseHTTPRequestHandler.log_message parameter name format and add the targeted
noqa A002 suppression to both handler methods.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 066586b3-926a-4835-a869-5f221b30f74d
📒 Files selected for processing (16)
.claude/skills/deploy-linux-test-environment/SKILL.md.codex/skills/deploy-linux-test-environment/SKILL.mdagent-skills-manifest.jsonbim-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.pydocs/evidence/linux-test-deploy-verifier-hardening/self-referential-bootstrap/verification.txtscripts/deploy.ps1scripts/lib/host-native-launcher.ps1scripts/tests/test-deploy-governance-static.ps1scripts/tests/test-host-native-launcher.ps1scripts/tests/test-rebuild-test-deploy.ps1scripts/tests/test-verify-all.ps1scripts/verify-all.ps1scripts/verify-all.shservices/kit-manager-api/app/kit_gateway.pyservices/kit-manager-api/tests/test_kit_service_runtime_status.py
🚧 Files skipped from review as they are similar to previous changes (9)
- agent-skills-manifest.json
- scripts/tests/test-host-native-launcher.ps1
- docs/evidence/linux-test-deploy-verifier-hardening/self-referential-bootstrap/verification.txt
- scripts/deploy.ps1
- scripts/tests/test-deploy-governance-static.ps1
- .claude/skills/deploy-linux-test-environment/SKILL.md
- scripts/lib/host-native-launcher.ps1
- scripts/verify-all.ps1
- bim-streaming-server/tests/test_host_native_conversion_service.py
…er static sandbox
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8da4c4284e
ℹ️ 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: 85d518b5a6
ℹ️ 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".
Scheduled overnight run — evidence and hand-off (not a human review)This comment is machine-generated by the Codex tri-adversarial ship gateRun on head Verdict: SHIP — 0 critical, 0 high. L1 raw 13 / deduped 13, L2 confirmed 6 / refuted 2 / unverified 0, L3 final 6. Surviving findings, all
Refuted and dropped at L2/L3: L1-COR-001 is already closed by Reviewer threadsAll 15 previously unresolved threads are resolved, each with the specific fix recorded on the thread. Local validation at head
|
…port-test target id Two review follow-ups on this branch: - capture_output piped the converter's stdout/stderr into PowerShell and its Kit grandchild; on timeout, run() kills only the direct child and its cleanup communicate() waits forever on the pipe the grandchild still holds. The adapter now redirects into temp files (nothing to wait on after the kill), decodes them for the existing CONV_META parsing, and gives the outer timeout a 30s buffer so the ps1's own -TimeoutSeconds cleans the Kit tree first. Pinned by a no-pipes contract test and a converter_timeout mapping test. - The transport unit tests no longer invoke helpers with the canonical-linux target id: the canonical rebuild's regular form takes no -TargetId at all (the wrapper defaults to the registry canonical target), so unit fixtures use a neutral id and the canonical id appears only where the registry declares it. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015QTVFY89rS2xRwRB2TpF P6
…he canonical Linux host deploy-main-to-linux-test rides on deploy-linux-test-environment (the single source of truth for every deployment guardrail) and records the 2026-08-11 live-verified shortest path: isolated origin/main worktree, canonical env staging, the regular no-TargetId invocation, tag/health verification, and the pitfall table (pwsh 7 vs 5.1 native-stderr, missing staging HELD, worktree-add stderr, no explicit canonical id). Claude side is canonical; the Codex mirror, gitignore whitelists, and manifest digest are synced via sync-agent-skills (Check valid, 31 skills). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015QTVFY89rS2xRwRB2TpFP6
…or form The canonical rebuild's regular form takes no -TargetId: the wrapper resolves the registry canonical_target, and the explicit selector exists only for on-demand targets. The bootstrap test's command specs and the recorded baseline evidence previously smuggled '-TargetId canonical-linux' into what claimed to be the canonical form. - test-self-referential-bootstrap: command specs use the regular form and a new assertion pins canonical-linux-rebuild to stay selector-free. - evidence: baseline rerun with the branch wrapper (head 1b764e2) in the regular form; deployed source a93c5a3, tag deploy-20260811-639220225263578177-002, deploy_exit=0. - conversion suite count updated (96 passed) for the pipe-hang fix cases. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015QTVFY89rS2xRwRB2TpFP6
…ation root The owner-private walk only covered the validation root downward. On Linux an ancestor of the release root or extension cache root that another account can write would let that account rename the validated tree and substitute its own content before PowerShell reopens the entrypoint by pathname. Walk from each validation root up to the filesystem root and require every ancestor to be a directory owned by root or the service account, not other-writable and not group-writable to a shared group, with the sticky bit exempting shared directories where foreign entries cannot be renamed. Five platform-independent unit cases pin the contract (root-owned chain, other-writable ancestor, sticky world-writable ancestor, foreign-owned ancestor, group-writable ancestor without a private group). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015QTVFY89rS2xRwRB2TpFP6
Every mirrored skill tree carries a text eol=lf attribute so the agent-skill-tree digest is byte-stable across platforms; the new skill was missing its pair. On the windows-latest governance runners git's default autocrlf checkout turned SKILL.md into CRLF, so the source-side digest no longer matched the manifest and the sync preflight failed closed (integrity mismatch in non-remediable location [claude]). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015QTVFY89rS2xRwRB2TpFP6
monkey1sai-blip
left a comment
monkey1sai-blip
left a comment
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 6cf4fb8ba57f27a63e0a0a444fefc3b7eb927feb. This is the mechanism the GitHub App cannot satisfy: an App's approving review does not count toward required_approving_review_count.
…mmand (#487) * fix(deploy): stop Start-Process from shredding the Linux Kit build command The bash launch path passed the whole build command as one -c string with embedded quotes. Start-Process joins its ArgumentList into a single Arguments string and re-tokenizes it, so bash actually received only the repo.sh path as the command: repo.sh printed its usage with no arguments and exited 0, the build argument and the log redirect were silently dropped, and deploy.ps1 took the fake exit 0 as a successful build until the artifact recheck failed with a far less diagnosable message. This was latent since the Linux migration (#467) — every earlier rebuild found the deployment checkout unchanged and skipped the build phase — and first fired on the post-#484 fixpoint rebuild, which reset the checkout and cleaned _build. Write the launch command into a wrapper script instead, so the command line carries exactly one plain path argument that no platform's argument re-quoting can damage. Also fail closed when the build process exits 0 without ever creating its log file: that combination means the launch line was shredded and nothing ran. Verified on the canonical Linux host (isolated probe): repo.sh received exactly 'build', the redirect created the log, exit 0. The full canonical rebuild through this path lands with the ledger fixpoint after merge. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015QTVFY89rS2xRwRB2TpFP6 * fix(deploy): feed the build wrapper via stdin and scope the log to this launch Review round: feed the wrapper script to bash on stdin so the command line carries no argument at all — a deploy_root with spaces has nothing left to shred. Remove any stale kit-repo-build.log before launching so the exit-0-must-have-a-log guard proves this build created it, not an earlier one. Mark the test fixture repo.sh executable on POSIX hosts where exec would otherwise fail with EACCES, and run the dynamic bash test inside a directory with spaces. Verified again on the canonical Linux host: exit 0, args seen=[build], log created, from a 'deploy root with spaces' directory. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015QTVFY89rS2xRwRB2TpFP6 * fix(deploy): escape shell metacharacters in the wrapper's embedded paths The registry accepts deploy_root values containing $ and backtick; embedding those raw inside the wrapper's double-quoted sh strings lets the shell perform parameter/command substitution on the path, so exec or the log redirect targets a different location. Escape the four double-quote-special characters when composing the wrapper, and run Test 15c from a directory carrying spaces, $, and a backtick (fails with exit 127 without the escaping). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018Gn3PJQ96Krb3adAErpXGu * test(deploy): make launcher regressions portable on Windows * fix(deploy): fail closed on stale build logs --------- Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
…e timeout path (#502) Closes #493 (TG-02, gate #484 finding). Every existing dynamic Start-HostNativeKitManager case injected a succeeding fake -ImportProbeFn, so the DEFAULT probe's WaitForExit/Stop-HostNativeProcessTreeAndWait/-1 propagation was only proven by source-regex assertions, never actually driven. Add a dynamic case to Test 21 that puts a fastapi.py shim on PYTHONPATH which spawns a child and sleeps, forcing the hardcoded `-c 'import fastapi, uvicorn'` probe to hang. Calls Start-HostNativeKitManager with no -ImportProbeFn override and a 1s -ImportProbeTimeoutSec, asserting: the documented "...cannot import fastapi and uvicorn" throw, a bounded (<10s) elapsed time, that Start-HostNativeService is never reached, and that both the shim's parent and child PIDs have exited. Verified the assertions actually bite: temporarily raising -ImportProbeTimeoutSec past the 10s bound fails the elapsed-time assertion, and temporarily stubbing out Stop-HostNativeProcessTreeAndWait fails the surviving-PID assertion. Both reverted before commit. Test-only change; scripts/lib/host-native-launcher.ps1 is untouched. Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
…with its rebuild-backed fixpoint (#499) * fix(governance): close the linux-test-deploy-verifier-hardening debt with its rebuild-backed fixpoint Rerun the entry's ordered 14-command verification contract after #487 merged: local suites 1-11 all exit 0, canonical Linux rebuild exit 0 with the repaired stdin-fed build launch proven on the canonical host (deploy tag deploy-20260811-639220482065640754-003), the CAD hardener idempotently exit 0, and the remote Deployment-profile verify all green. Two group-writable directory drifts the #484 trust-root ancestry validation correctly refused are recorded in the summary with their in-run chmod remediation. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018Gn3PJQ96Krb3adAErpXGu * docs(evidence): attest the strict contract-order fixpoint sweep Rerun the full 14-command contract in the opening contract's exact order (1-11 local at the deployed source commit c88dca6, then harden 12, rebuild 13 with deploy tag deploy-20260811-639220494716638402-004, verify 14) after review flagged the first sweep's 13-before-12 chronology; every command exit 0 in a single pass. The first sweep and the in-run permission findings remain recorded as context. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018Gn3PJQ96Krb3adAErpXGu * docs(evidence): declare an allowed document nature and add run timestamps working note replaces the non-vocabulary 'evidence' nature per docs/AGENTS.md, and the attested-run section now carries UTC time anchors proving the 12-before-13 execution order. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018Gn3PJQ96Krb3adAErpXGu * docs(evidence): rerun command 14 with the pinned -InventoryPath invocation The immutable command map pins canonical-linux-deployment-verify as verify-all.ps1 -Profile Deployment -InventoryPath <owner-private-inventory>; the sweep had substituted the environment-variable inventory form. Rerun the pinned invocation against the same unchanged -004 deployment (exit 0, all six checks Passed, 2026-08-12T02:00:16Z) and make it the attested record. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018Gn3PJQ96Krb3adAErpXGu * fix(governance): attest the fixpoint with a pinned-form 12-14 remediation rerun Review P1 on this PR proved command 13's recorded invocation carried an extra -IdentityFile beyond the immutable command map's pinned form. The owner moved the deploy key into default ssh resolution (batch-mode preflight DEFAULT_IDENTITY_OK), then commands 12-14 were rerun in contract order, all pinned form, single pass: - 12 harden-cad on the remote deploy_root: exact schema line, exit 0 - 13 rebuild from fresh origin/main (970dc34, isolated worktree), NO -IdentityFile / -TargetId: deploy exit 0, tag deploy-20260812-639221007059362180-001 pushed - 14 verify-all -Profile Deployment -InventoryPath on the NEW deployment: six checks Passed, none Failed, exit 0 summary.md keeps the 2026-08-11 invocation as a historical record and marks the 2026-08-12 rerun as the attested one; ledger fixpoint reverified_at rebound to 2026-08-12T03:07:00Z. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
Summary
deploy-linux-test-environmentCodex skill for the owner-inventory canonical Linux workflow0400inodeChange Classification
Deploy Path Verification
scripts/deploy.ps1, host-native launcher, verifier, and repo-local operator skillscripts/dev/rebuild-test-deploy.ps1 -Build -InventoryPath <owner-private inventory>from freshorigin/mainpwsh -NoProfile -NonInteractive -File scripts/verify-all.ps1 -Profile Deployment; branch bootstrap adapter harden/reverify was separately labeled self-referentialAI Coding Governance
Windows On-Demand Verification
git diff --checkclean; exact-head CI run: https://github.com/monkey1sai/AI-BIM-governance/actions/runs/31462624695 (in progress at body update; local exact-head gates passed)Self-Referential Bootstrap
Verification
python -m pytest bim-streaming-server/tests/test_host_native_conversion_service.py -q: 101 passed, 6 skipped on Windows at exact head6cf4fb8ba57f27a63e0a0a444fefc3b7eb927feb0400scripts/tests/test-host-native-launcher.ps1: passed, including exact child env and parent restorationpython -m pytest services/kit-manager-api/tests -q: 14 passed, including the hostile-proxy direct-socket regression and the new redirect-rejection regressionscripts/tests/test-verify-all.ps1: passedscripts/tests/test-deploy-governance-static.ps1: passedscripts/tests/test-deploy-target-registry.ps1: passedscripts/tests/test-rebuild-test-deploy.ps1: passed on the final staged revisiongit diff --check: passedDeployment evidence and limits
canonical-linux/linux_host_native/canonical_test_deploy/ SSHa93c5a34cfef7bb6f3fdd5d20c287d9c83c89ea1; build/restart exited 0 and created deploy tagdeploy-20260811-639220225263578177-0020400degradedbecause its old resolver cannot traverse the top-level cache link; this PR does not mislabel branch bootstrap as canonical post-change success.bim-deployroot ACL restoration was verified after the final private-input use: protected DACL with exactly owner, SYSTEM, and AdministratorsKnown follow-up
origin/main, rerun the same strict deployment verification, and close the bootstrap ledger fixpoint in a separate PRSummary by CodeRabbit
New Features
Bug Fixes
Tests