test(inference): stage OpenClaw llama.cpp qualification - #8532
Conversation
Signed-off-by: Aaron Erickson <aerickson@nvidia.com>
|
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 declarative OpenClaw qualification for llama.cpp DGX Spark. It defines qualification contracts, bounded probes, managed-image execution, llama.cpp routing, workflow gating, receipt validation, and risk-plan activation. Changesllama.cpp OpenClaw qualification
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Workflow
participant QualificationPlan
participant DgxSparkRunner
participant OpenShellE2E
participant OpenClaw
participant LlamaCpp
Workflow->>QualificationPlan: export agent_qualification_execution
Workflow->>OpenShellE2E: install OpenShell when enabled
DgxSparkRunner->>OpenClaw: validate image and run probes
OpenClaw->>LlamaCpp: send synchronous and streaming requests
OpenShellE2E-->>DgxSparkRunner: return probe evidence and cleanup status
DgxSparkRunner-->>Workflow: record validated receipt
Possibly related issues
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Comment |
Code Coverage OverviewLanguages: TypeScript TypeScript / code-coverage/pluginThe overall coverage in commit 39b66e9 in the TypeScript / code-coverage/cliThe overall coverage in commit 39b66e9 in the Show a code coverage summary of the most impacted files.
Updated |
Signed-off-by: Aaron Erickson <aerickson@nvidia.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (5)
tools/advisors/risk-plan.mts (1)
100-101: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider deriving the qualification path from one canonical constant.
This path literal now exists in at least three places: here, in
scripts/checks/export-llama-cpp-image-config.mts(Lines 154-159 and 913-916), and intest/pr-risk-plan.test.tsLine 475. If the manifest is renamed and only the export script is updated, this rule silently stops selecting the protected DGX Spark lane while the file still drives qualification behavior. The failure is silent because no check compares the two literals.Export the path constant from the contract module and import it in both the advisor and the export script. Keep the test literal as an independent copy so it still detects an unintended path change.
As per path instructions: "Derive inventories and limits from a canonical source where possible; flag duplicated lists that can silently drift."
🤖 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/advisors/risk-plan.mts` around lines 100 - 101, Export the qualification path from the contract module that defines the canonical manifest contract, then replace the duplicated literal in the advisor constant LLAMA_CPP_DGX_SPARK_AGENT_QUALIFICATION and the export script’s corresponding references with that imported constant. Leave the independent literal in test/pr-risk-plan.test.ts unchanged so the test continues detecting unintended path changes.Source: Path instructions
test/pr-risk-plan.test.ts (1)
490-492: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a false-positive case for the new qualification path.
This assertion proves detection. It does not prove the rule stays narrow. A neighbouring file such as
managed-inference/qualifications/other.yamlormanaged-inference/recipes/llama-cpp.nemotron-3-nano-30b-a3b.spark-single.v1.yamlmust not selectllama-cpp-dgx-spark-qualification, because the rule uses exact equality rather than a prefix. Add one negative assertion to lock that boundary.As per path instructions: "Require focused tests for both detection and false-positive behavior."
🤖 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 `@test/pr-risk-plan.test.ts` around lines 490 - 492, Add a focused negative assertion alongside the existing riskPlanRequiredJobIds test, using a neighbouring non-matching qualification or recipe path, and verify it does not include "llama-cpp-dgx-spark-qualification". Preserve the existing positive detection assertion to cover both exact-match detection and prefix-based false-positive prevention.Source: Path instructions
test/managed-image-protected-runtime-contract.test.ts (1)
148-196: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd the
llama-cppplus--gpurejection case.The title claims llama.cpp runs "without direct sandbox GPU access". The three cases prove llama.cpp is accepted without
--gpu, vLLM is rejected without--gpu, and failure injection is rejected. None of them proves that llama.cpp with--gpufails. The parser guard atscripts/checks/run-managed-image-openshell-e2e.tsline 183 covers that rule and stays untested. The same guard also rejects llama.cpp without--model.Add a case for each branch so the guard cannot be removed without a test failure.
🤖 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 `@test/managed-image-protected-runtime-contract.test.ts` around lines 148 - 196, Extend the test in “allows only llama.cpp local inference without direct sandbox GPU access” to assert that llama-cpp with --gpu throws the expected rejection, and that llama-cpp without --model also throws. Keep the existing accepted llama-cpp case and unrelated rejection cases unchanged, using parseManagedImageOpenShellE2eInputs for both new assertions.scripts/checks/run-managed-image-openshell-e2e.ts (1)
921-923: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConfirm that skipping
assertProtectedLocalInferenceis intended.When a caller supplies
afterLocalInference, this runner no longer asserts protected local inference itself. The callback becomes the only inference proof. The current llama.cpp callback does probeinference.local, so the guarantee holds today. A future callback that does not probe inference would silently lose the assertion.Consider running
assertProtectedLocalInferencein both paths, or document why the callback replaces it.🤖 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/checks/run-managed-image-openshell-e2e.ts` around lines 921 - 923, Ensure protected local inference is asserted for every local-provider run, including when afterLocalInference is supplied, by updating the conditional around assertProtectedLocalInference in the runner. Preserve the callback execution while preventing it from being the sole inference guarantee.test/llama-cpp-dgx-spark-qualification-runner.test.ts (1)
390-410: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCover the revision and malformed-JSON rejections.
The test proves one of the five label checks in
validateOpenClawQualificationImageLabels. A wrongorg.opencontainers.image.revisionis the check that binds the image to the declarative source revision, and it stays untested. The malformed-JSON path that throws "labels are invalid" is also untested.Add a case that passes a different expected revision, and a case that passes a non-JSON string.
🤖 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 `@test/llama-cpp-dgx-spark-qualification-runner.test.ts` around lines 390 - 410, Extend the test for validateOpenClawQualificationImageLabels with a case using a different expected revision and asserting the declarative-source revision error, plus a case passing malformed JSON and asserting the “labels are invalid” error. Keep the existing valid-label and agent-identity assertions unchanged.
🤖 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/checks/llama-cpp-openclaw-agent-qualification.mts`:
- Around line 25-37: Redact child-process output before it reaches the thrown
error in requireSuccess. Replace the raw stdout/stderr handling in detail with
the audited managed-image redaction helper used by sibling probes, or limit
failures to the label and exit status; preserve the response-size check while
ensuring credentials, tokens, prompts, request data, and model output cannot
enter CI logs.
- Around line 13-16: Update the qualification evidence type and return value in
the plan-derived tool-call flow around agentToolCall to use the configured tool
name from config.tool.name instead of the literal "read"; ensure
parseLlamaCppDgxSparkQualificationReceipt continues receiving the same
configured name that the session probe validated.
In `@scripts/checks/run-llama-cpp-dgx-spark-qualification.mts`:
- Around line 1012-1016: Move the docker startup-log retrieval and
validateStartupLog call to immediately after waitForHealth, before the OpenClaw
qualification probes run. Store the validated result and reuse it at the current
qualification point instead of reading docker logs again, preserving the
existing maximumBytes and UTF-8 decoding.
In `@scripts/checks/run-managed-image-openshell-e2e.ts`:
- Around line 1081-1089: Update the gateway cleanup logic in the finally block
to inspect the result of stopping/removing the gateway, and append a descriptive
failure to cleanupErrors when removal cannot be verified. Only set
cleanup.gatewayRemoved to true after successful verification, while preserving
the existing cleanup receipt structure and qualification flow.
In `@test/llama-cpp-openclaw-agent-qualification.test.ts`:
- Around line 36-45: Remove the if statement in runSandbox that checks !output,
and replace it with an expression that still throws on unexpected qualification
invocations while preserving the existing output handling and returned status,
stdout, and stderr values.
In `@tools/e2e/llama-cpp-dgx-spark-qualification-workflow-boundary.mts`:
- Around line 331-353: Add the same explicit negative check used by the
`compile` and `qualify` validation paths to the `installOpenShell` step: reject
its `run` body when it contains `.candidate-llama-cpp/scripts`. Keep the
existing `requireFragments` assertions and gate validation unchanged.
---
Nitpick comments:
In `@scripts/checks/run-managed-image-openshell-e2e.ts`:
- Around line 921-923: Ensure protected local inference is asserted for every
local-provider run, including when afterLocalInference is supplied, by updating
the conditional around assertProtectedLocalInference in the runner. Preserve the
callback execution while preventing it from being the sole inference guarantee.
In `@test/llama-cpp-dgx-spark-qualification-runner.test.ts`:
- Around line 390-410: Extend the test for
validateOpenClawQualificationImageLabels with a case using a different expected
revision and asserting the declarative-source revision error, plus a case
passing malformed JSON and asserting the “labels are invalid” error. Keep the
existing valid-label and agent-identity assertions unchanged.
In `@test/managed-image-protected-runtime-contract.test.ts`:
- Around line 148-196: Extend the test in “allows only llama.cpp local inference
without direct sandbox GPU access” to assert that llama-cpp with --gpu throws
the expected rejection, and that llama-cpp without --model also throws. Keep the
existing accepted llama-cpp case and unrelated rejection cases unchanged, using
parseManagedImageOpenShellE2eInputs for both new assertions.
In `@test/pr-risk-plan.test.ts`:
- Around line 490-492: Add a focused negative assertion alongside the existing
riskPlanRequiredJobIds test, using a neighbouring non-matching qualification or
recipe path, and verify it does not include "llama-cpp-dgx-spark-qualification".
Preserve the existing positive detection assertion to cover both exact-match
detection and prefix-based false-positive prevention.
In `@tools/advisors/risk-plan.mts`:
- Around line 100-101: Export the qualification path from the contract module
that defines the canonical manifest contract, then replace the duplicated
literal in the advisor constant LLAMA_CPP_DGX_SPARK_AGENT_QUALIFICATION and the
export script’s corresponding references with that imported constant. Leave the
independent literal in test/pr-risk-plan.test.ts unchanged so the test continues
detecting unintended path changes.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: ec4d68e8-8558-4970-9e71-8fcadee41a89
📒 Files selected for processing (21)
.github/workflows/e2e.yamlmanaged-inference/qualifications/llama-cpp.openclaw.spark-single.v1.yamlscripts/checks/export-llama-cpp-dgx-spark-qualification-plan.mtsscripts/checks/export-llama-cpp-image-config.mtsscripts/checks/llama-cpp-dgx-spark-qualification-contract.mtsscripts/checks/llama-cpp-openclaw-agent-qualification.mtsscripts/checks/managed-image-protected-runtime-contract.tsscripts/checks/run-llama-cpp-dgx-spark-qualification.mtsscripts/checks/run-managed-image-openshell-e2e.tstest/e2e/live/llama-cpp-dgx-spark-qualification.test.tstest/e2e/support/e2e-cross-runtime-compatibility.test.tstest/e2e/support/llama-cpp-dgx-spark-qualification-workflow.test.tstest/llama-cpp-dgx-spark-qualification-contract.test.tstest/llama-cpp-dgx-spark-qualification-plan.test.tstest/llama-cpp-dgx-spark-qualification-runner.test.tstest/llama-cpp-image.test.tstest/llama-cpp-openclaw-agent-qualification.test.tstest/managed-image-protected-runtime-contract.test.tstest/pr-risk-plan.test.tstools/advisors/risk-plan.mtstools/e2e/llama-cpp-dgx-spark-qualification-workflow-boundary.mts
| return { | ||
| cleanup: { | ||
| gatewayRemoved: true, | ||
| networkRemoved: true, | ||
| sandboxRemoved: true, | ||
| stateRemoved: true, | ||
| }, | ||
| ...(probeEvidence === undefined ? {} : { probeEvidence }), | ||
| }; |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
gatewayRemoved: true is asserted, not proven.
The returned cleanup record is a literal. The caller copies it into the qualification receipt at scripts/checks/run-llama-cpp-dgx-spark-qualification.mts line 991, and the receipt parser accepts it as cleanup evidence. Three of the four fields have a matching verification in the finally block: the container, the network, and the state directory each push to cleanupErrors when removal cannot be proven. The gateway does not. Lines 963-966 stop the gateway process and run gateway remove without inspecting the result, so gatewayRemoved: true records an outcome that was never checked.
Verify gateway removal and push a cleanup error when it fails, so the receipt reports measured cleanup.
🤖 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/checks/run-managed-image-openshell-e2e.ts` around lines 1081 - 1089,
Update the gateway cleanup logic in the finally block to inspect the result of
stopping/removing the gateway, and append a descriptive failure to cleanupErrors
when removal cannot be verified. Only set cleanup.gatewayRemoved to true after
successful verification, while preserving the existing cleanup receipt structure
and qualification flow.
PR Review Advisor — No blocking findings reportedAdvisor assessment: No blocking advisor findings reported Model lanes
8 terminology differences from the second opinionAdvisory only. These are normalized differences from the primary terminology receipt.
3 additional E2E selections from the second opinionAdvisory only. The primary lane did not select these E2E jobs or targets.
Second-opinion terminology and E2E selections are advisory. Live E2E does not run automatically for pull requests. 2 semantic terminology decisionsTerminology decisions are advisory. They affect the assessment only when a separate finding identifies concrete semantic impact.
E2E guidanceAdvisory only. A maintainer can dispatch the default E2E suite against this exact revision. Recommended E2E: This automated review informs maintainers. Warnings and suggestions do not require a response. A maintainer decides whether to merge. |
Signed-off-by: Aaron Erickson <aerickson@nvidia.com>
Signed-off-by: Aaron Erickson <aerickson@nvidia.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@tools/advisors/risk-plan.mts`:
- Line 5: Update the contract imports and risk-selection logic around
llamaCppDgxSparkQualificationContract to destructure and use
LLAMA_CPP_DGX_SPARK_QUALIFICATION_ACTIVATION_PATH alongside the agent path,
replacing the local activation-path comparison. Add focused coverage for both
protected paths and one unrelated path, using the canonical contract paths
rather than duplicating them.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: ae048266-513c-48e2-9a35-776650b4a05a
📒 Files selected for processing (12)
scripts/checks/export-llama-cpp-image-config.mtsscripts/checks/llama-cpp-dgx-spark-qualification-contract.mtsscripts/checks/llama-cpp-openclaw-agent-qualification.mtsscripts/checks/run-llama-cpp-dgx-spark-qualification.mtsscripts/checks/run-managed-image-openshell-e2e.tstest/e2e/support/llama-cpp-dgx-spark-qualification-workflow.test.tstest/llama-cpp-dgx-spark-qualification-runner.test.tstest/llama-cpp-openclaw-agent-qualification.test.tstest/managed-image-protected-runtime-contract.test.tstest/pr-risk-plan.test.tstools/advisors/risk-plan.mtstools/e2e/llama-cpp-dgx-spark-qualification-workflow-boundary.mts
🚧 Files skipped from review as they are similar to previous changes (9)
- test/pr-risk-plan.test.ts
- tools/e2e/llama-cpp-dgx-spark-qualification-workflow-boundary.mts
- test/llama-cpp-dgx-spark-qualification-runner.test.ts
- test/llama-cpp-openclaw-agent-qualification.test.ts
- scripts/checks/export-llama-cpp-image-config.mts
- test/managed-image-protected-runtime-contract.test.ts
- scripts/checks/run-llama-cpp-dgx-spark-qualification.mts
- scripts/checks/llama-cpp-dgx-spark-qualification-contract.mts
- scripts/checks/llama-cpp-openclaw-agent-qualification.mts
Signed-off-by: Aaron Erickson <aerickson@nvidia.com>
Signed-off-by: Aaron Erickson <aerickson@nvidia.com>
1 participant
Summary
Stages the execution-disabled, YAML-authored qualification path for running OpenClaw against the existing llama.cpp/Nemotron single-DGX-Spark recipe through
inference.local. This lands the trusted compiler, protected runner wiring, and evidence contract without claiming support; the shipped recipe still declaresagents: [], and a later YAML-only activation can exercise the tuple after this code is onmain.Related Issue
Advances #8144.
Changes
AgentQualificationYAML that pins the OpenClaw image digest, Docker runtime, route, bounds, prompts, sessions, tool fixture, and required probes while keeping execution disabled.inference.local, execute synchronous/streaming and real agent/tool/multi-turn probes, and emit sanitized identity-bound evidence.Type of Change
Quality Gates
Documentation Writer Review
no-docs-neededDGX Station Hardware Evidence
Verification
Signed-off-by:line and every commit appears asVerifiedin GitHubpre-commit,commit-msg, andpre-pushhooks passed, ornpm run validate:prpassed after refreshingorigin/mainwhen hooks were skipped or unavailablenpm run typecheck -- --pretty false; 233 focused qualification, workflow, and risk tests.npm run docsbuilds without warnings (doc changes only)Signed-off-by: Aaron Erickson aerickson@nvidia.com
Summary by CodeRabbit
New Features
Bug Fixes
Tests