ci(deps): add candidate compatibility workflow#6738
Conversation
📝 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 LaneRuntime
GitHubActions->>CandidateCLI: resolve candidate and generate plan
CandidateCLI-->>GitHubActions: receipt and lane matrix
GitHubActions->>NemoClawCheckout: checkout resolved NemoClaw SHA
GitHubActions->>LaneRuntime: materialize candidate and run lane
LaneRuntime-->>GitHubActions: observed output and lane result
GitHubActions->>CandidateCLI: finalize lane evidence
CandidateCLI-->>GitHubActions: auditable compatibility evidence
Possibly related issues
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
E2E Advisor RecommendationRequired E2E: None Full advisor summaryE2E Recommendation AdvisorBase: Required E2E
Optional E2E
New E2E recommendations
|
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.
Summary
Validation
npx vitest run test/candidate-compat.test.ts(9 passed)npm run checknpm run typecheck:clinpm run check:diffRefs #6691
Signed-off-by: Ho Lim subhoya@gmail.com
Summary by CodeRabbit