ci(deps): validate OpenShell candidate runtime#6738
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:
📝 WalkthroughWalkthroughAdds candidate compatibility resolution and verification tooling, deterministic lane planning, artifact materialization, evidence finalization, a GitHub Actions workflow, and tests covering provenance, security boundaries, runtime observations, and complete evidence aggregation. ChangesCandidate Compatibility
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant GitHubActions
participant CandidateCLI
participant NemoClawCheckout
participant CandidateRuntime
GitHubActions->>CandidateCLI: resolve candidate and generate plan
CandidateCLI-->>GitHubActions: receipt and lane matrix
GitHubActions->>NemoClawCheckout: checkout resolved NemoClaw SHA
GitHubActions->>CandidateRuntime: materialize candidate and run lane
CandidateRuntime-->>GitHubActions: observed output and lane result
GitHubActions->>CandidateCLI: finalize lane evidence
CandidateCLI-->>GitHubActions: auditable compatibility evidence
Suggested labels: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
HOYALIM
force-pushed
the
codex/issue-6691-candidate-compat
branch
from
4fe9f8c to
05fedce
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tools/candidate-compat.mts (1)
552-564: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winValidate the initial artifact URL host before issuing the request.
downloadusesredirect: "follow"and only checks the host of the finalresponse.url. The initial request toartifactValue.urlis issued before any host check, so a tampered receipt URL would still trigger an outbound fetch to an arbitrary host. The receipt is produced by the trustedresolvejob, so this is defense-in-depth rather than an active exploit, but re-asserting the host on the initial URL is cheap and closes the gap.🛡️ Proposed defense-in-depth check
async function download(artifactValue: Artifact, directory: string): Promise<string> { + const requestUrl = new URL(artifactValue.url); + if (requestUrl.protocol !== "https:" || !DOWNLOAD_HOSTS.has(requestUrl.hostname)) { + throw new Error(`candidate artifact is not on an approved download host: ${artifactValue.url}`); + } const response = await fetch(artifactValue.url, { redirect: "follow" }); if (!response.ok) throw new Error(`candidate download failed (${response.status})`);🤖 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 `@tools/candidate-compat.mts` around lines 552 - 564, Update download to parse and validate artifactValue.url against the approved HTTPS protocol and DOWNLOAD_HOSTS before calling fetch, rejecting unapproved initial hosts while preserving the existing final response URL validation.
🤖 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.
Nitpick comments:
In `@tools/candidate-compat.mts`:
- Around line 552-564: Update download to parse and validate artifactValue.url
against the approved HTTPS protocol and DOWNLOAD_HOSTS before calling fetch,
rejecting unapproved initial hosts while preserving the existing final response
URL validation.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 6b2028c0-76d0-4737-9143-32cb81fc4ff1
📒 Files selected for processing (4)
.github/workflows/candidate-compatibility.yamlci/source-shape-test-budget.jsontest/candidate-compat.test.tstools/candidate-compat.mts
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
Signed-off-by: Ho Lim <subhoya@gmail.com>
HOYALIM
force-pushed
the
codex/issue-6691-candidate-compat
branch
from
05fedce to
ae774d4
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (2)
tools/candidate-compat.mts (1)
111-121: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valueUse code-point ordering in
stableJsonresolutionIdand the equality checks built from this helper should not depend on locale/ICU collation.♻️ Use deterministic code-point ordering
- .sort(([left], [right]) => left.localeCompare(right)) + .sort(([left], [right]) => (left < right ? -1 : left > right ? 1 : 0))🤖 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 `@tools/candidate-compat.mts` around lines 111 - 121, Update stableJson’s object-key sort to use deterministic code-point ordering rather than localeCompare. Preserve the existing filtering, serialization, and recursive traversal behavior while ensuring generated strings are independent of locale or ICU collation..github/workflows/candidate-compatibility.yaml (1)
162-175: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winPass
resolution_idviaenv:instead of interpolating into the shell body.Line 172 expands
${{ needs.resolve.outputs.resolution_id }}directly into therunscript, unlike every other step here which passes values throughenv:(e.g.LANE). The value is validated to^[a-f0-9]{64}$at line 85 so actual injection risk is nil, but keeping it as an env reference silences the scanner and honors the untrusted-as-data convention.🔒 Proposed change to pass the value as data
env: LANE: ${{ matrix.lane }} + RESOLUTION_ID: ${{ needs.resolve.outputs.resolution_id }} shell: bash run: | set -euo pipefail node --experimental-strip-types controller/tools/candidate-compat.mts materialize \ --receipt candidate-input/candidate-receipt.json \ - --resolution-id "${{ needs.resolve.outputs.resolution_id }}" \ + --resolution-id "$RESOLUTION_ID" \ --directory "${RUNNER_TEMP}/candidate-runtime" \ --output "candidate-observed-${LANE}.json" \ --github-env "$GITHUB_ENV" 2>&1 | tee "candidate-materialize-${LANE}.log"As per path instructions: "pass untrusted values as data rather than interpolating them into shell programs."
🤖 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 @.github/workflows/candidate-compatibility.yaml around lines 162 - 175, Update the “Materialize and verify candidate runtime” step to add needs.resolve.outputs.resolution_id to its env mapping, then reference that environment variable in the candidate-compat materialize command instead of interpolating the GitHub expression directly in the shell script. Preserve the existing resolution ID value and command behavior.Sources: Path instructions, 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.
Nitpick comments:
In @.github/workflows/candidate-compatibility.yaml:
- Around line 162-175: Update the “Materialize and verify candidate runtime”
step to add needs.resolve.outputs.resolution_id to its env mapping, then
reference that environment variable in the candidate-compat materialize command
instead of interpolating the GitHub expression directly in the shell script.
Preserve the existing resolution ID value and command behavior.
In `@tools/candidate-compat.mts`:
- Around line 111-121: Update stableJson’s object-key sort to use deterministic
code-point ordering rather than localeCompare. Preserve the existing filtering,
serialization, and recursive traversal behavior while ensuring generated strings
are independent of locale or ICU collation.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: d87636d0-b189-4b6a-8730-8c380b6a5681
📒 Files selected for processing (4)
.github/workflows/candidate-compatibility.yamlci/source-shape-test-budget.jsontest/candidate-compat.test.tstools/candidate-compat.mts
🚧 Files skipped from review as they are similar to previous changes (2)
- ci/source-shape-test-budget.json
- test/candidate-compat.test.ts
wscurran
added
area: ci
|
✨ Thanks for the CI infrastructure work, @HOYALIM. Adding a maintainer-dispatched compatibility controller for candidate versions will improve our validation coverage. Ready for maintainer review. Related open issues: |
cjagwani
left a comment
cjagwani
left a comment
There was a problem hiding this comment.
Reviewed exact head ae774d4. Both manually dispatched Advisors analyzed this head and recommend merge_as_is, but the direct supply-chain review found three blockers their broad checks miss:
-
High — .github/workflows/candidate-compatibility.yaml:162-248 and tools/candidate-compat.mts:744-812,857-877 can mark a lane successful after standalone materialization plus ordinary tests without proving the lane consumed the candidate. NEMOCLAW_CANDIDATE_* has no repository consumer; the recorded version is a pre-lane probe. Require trusted, receipt-bound evidence that each selected install/build/runtime path actually invoked the candidate, and fail if that proof is absent.
-
Medium — tools/candidate-compat.mts:347-356,530-536,748-787 admits npm hermes-agent as an official Hermes artifact using only version equality. The package's own registry metadata identifies it as an unofficial bridge from wyrtensi/hermes-agent-npm, while the receipt labels NousResearch. A digest authenticates those bytes, not official provenance. Remove it or replace it with an artifact verifiably controlled by the official upstream.
-
Medium — tools/candidate-compat.mts:249-269,432-440,789-798 materializes OpenClaw with npm install from only a verified top-level tarball. The transitive closure is neither policy-checked nor bound into resolutionId/evidence; current caret dependencies make identical resolution IDs capable of executing different bytes. Resolve and bind a complete lock/integrity closure in the trusted resolve job, install from it, and preserve it in evidence.
Terra's process-boundary test warning is therefore blocking, not cosmetic. Add a fixture-based materialize CLI regression that validates observed JSON, PATH/candidate env output, actual candidate invocation, and fail-closed version mismatch.
PR Review Advisor — InformationalAdvisor assessment: Informational / low confidence Model lanes
Nemotron output stays in workflow artifacts and does not change the assessment above. E2E guidanceAdvisory only. E2E / PR Gate selects and runs jobs independently. Recommended E2E: 1 warning · 0 suggestionsWarningsWarnings do not block.
|
Signed-off-by: Ho Lim <subhoya@gmail.com>
HOYALIM
changed the title
|
Addressed the requested supply-chain redesign in verified commit |
cv
left a comment
cv
left a comment
There was a problem hiding this comment.
[P1] .github/workflows/candidate-compatibility.yaml:177-216 and tools/candidate-compat.mts:655-689 — the deterministic installer lane still does not prove the tests exercised the candidate. The workflow invokes each wrapper with --version before running Vitest, and verifyCandidateInvocations accepts exactly those three probes as sufficient for an installer success. Nothing requires an installer test to invoke a candidate binary during the tested install/build path, so a suite that uses only fixtures or the committed stable pin can still emit successful candidate evidence. This leaves the original process-boundary blocker unresolved and does not meet #6691, which requires normal deterministic coverage to run against the candidate environment and every candidate-aware job to report the runtime it actually exercised. Require receipt-bound invocation from the installer behavior under test (not a pre-lane probe), or select a deterministic test that performs and verifies a real candidate install/runtime path; add a negative regression where only the three preflight --version calls occur and the lane is rejected.
Summary
openshell-gateway-auth-contractlive test against the materialized candidate and require proof that the candidate gateway was started, not merely version-probedThe earlier Hermes and OpenClaw paths were removed: Hermes included an artifact not controlled by NousResearch, while OpenClaw's top-level tarball did not bind its transitive dependency closure. Those components need separate provenance-complete slices before they can be added safely.
Validation
npm run typecheck:clinpx vitest run test/candidate-compat.test.ts test/e2e/support/openshell-gateway-auth-source-contract-helpers.test.ts test/e2e/support/openshell-gateway-auth-contract-workflow-boundary.test.ts(27 passed)npm run source-shape:check(0 unbudgeted cases)npm run check:diff81895e0967a17b6d063ecaf4482aab7a4ee2b9a0; bound all three official release digests to resolution55cffa81b9833cf7adbd0099cc7740f1a42038cdb3b4e3cc51b5b4ab4e45dda3Local Docker runtime evidence could not be produced because the local Docker daemon was unavailable. The new Ubuntu
livejob executes that exact gateway-auth contract and fails closed if runtime invocation proof is absent.Refs #6691
Signed-off-by: Ho Lim subhoya@gmail.com