fix(shields): wait for Hermes inference convergence - #8530
Conversation
Signed-off-by: Prekshi Vyas <prekshiv@nvidia.com>
|
Auto-sync is disabled for draft pull requests in this repository. Workflows must be run manually. Contributors can view more details about this message here. |
|
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:
📝 WalkthroughWalkthroughShields shutdown now verifies Hermes inference-route convergence after configuration unlock. The probe retries unhealthy responses with bounded delays. Failed convergence triggers rollback and route-specific recovery guidance. ChangesHermes route convergence
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant ShieldsDown
participant RelockReconfirm
participant ConvergenceHelper
participant HermesRoute
ShieldsDown->>RelockReconfirm: unlock Hermes configuration
RelockReconfirm->>ConvergenceHelper: wait for route convergence
ConvergenceHelper->>HermesRoute: run inference-route probe
HermesRoute-->>ConvergenceHelper: health and HTTP status
alt Route converges
ConvergenceHelper-->>RelockReconfirm: healthy result
else Route remains unavailable
ConvergenceHelper-->>RelockReconfirm: failed result with attempts
RelockReconfirm-->>ShieldsDown: raise transition error
end
Possibly related PRs
Suggested labels: 🚥 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 3a31307 in the TypeScript / code-coverage/cliThe overall coverage in commit 3a31307 in the Show a code coverage summary of the most impacted files.
Updated |
Signed-off-by: Prekshi Vyas <prekshiv@nvidia.com>
Signed-off-by: Prekshi Vyas <prekshiv@nvidia.com>
…erence-convergence Signed-off-by: Prekshi Vyas <prekshiv@nvidia.com>
Maintainer security review — PASS
No security blocker found. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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 `@src/lib/shields/index.ts`:
- Around line 3857-3862: Update the Hermes inference convergence-failure path
around the rollback and shared catch so HTTP 503 or unavailable-route failures
select a distinct recovery message advising retry after route recovery, rather
than the generic file-ownership guidance. Preserve the existing status and
attempts details, and route other failures through the current recovery-message
branch.
In `@src/lib/shields/inference-convergence.test.ts`:
- Around line 66-79: Update the test around
waitForHermesInferenceRouteConvergence to use status: 0 with the malformed
“attacker preamble” output, proving the parser rejects untrusted output even
when the command succeeds. Preserve a separate status: 1 case only if needed to
cover unavailable or unhealthy probe execution.
In `@src/lib/shields/inference-convergence.ts`:
- Around line 53-54: Update the option normalization near maxAttempts and
retryDelayMs to detect non-finite values and fall back to DEFAULT_MAX_ATTEMPTS
or DEFAULT_RETRY_DELAY_MS before applying the existing integer and non-negative
constraints. Ensure maxAttempts cannot cause unbounded probes, retryDelayMs
cannot pass Infinity to sleep, and add tests covering NaN and Infinity for both
options.
🪄 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: 91b00fd1-9c7c-4921-a066-66ebf7f5fc28
📒 Files selected for processing (5)
src/lib/shields/index.tssrc/lib/shields/inference-convergence.test.tssrc/lib/shields/inference-convergence.tssrc/lib/shields/legacy-hermes-compat.test.tssrc/lib/shields/relock-reconfirm.ts
PR Review Advisor — No blocking findings reportedAdvisor assessment: No blocking advisor findings reported Model lanes
2 terminology differences from the second opinionAdvisory only. These are normalized differences from the primary terminology receipt.
2 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. 3 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: Prekshi Vyas <prekshiv@nvidia.com>
Signed-off-by: Prekshi Vyas <prekshiv@nvidia.com>
|
Follow-up on advisor warning PRA-1: fixed in f6c63f2. The caller-level failed-convergence test now uses a stateful sealed-Hermes fixture and asserts all four recovery invariants: command failure, persisted |
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 `@src/lib/shields/legacy-hermes-compat.test.ts`:
- Around line 182-185: Update the begin-shields-transition mock handler around
isGuardAction to require the exact production --shields-mode argument format,
reject a missing mode or any value other than "locked" or "mutable" before
changing pendingMode, and preserve the existing state update only for valid
modes.
🪄 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: 86b6adfb-0c89-4a1c-957b-55b95318ce09
📒 Files selected for processing (1)
src/lib/shields/legacy-hermes-compat.test.ts
Signed-off-by: Prekshi Vyas <prekshiv@nvidia.com>
Signed-off-by: Prekshi Vyas <prekshiv@nvidia.com>
…erence-convergence Signed-off-by: Prekshi Vyas <prekshiv@nvidia.com>
|
Advisor suggestion PRA-1 evaluated on the current branch and intentionally not applied: importing |
…erence-convergence Signed-off-by: Prekshi Vyas <prekshiv@nvidia.com>
cjagwani
left a comment
cjagwani
left a comment
There was a problem hiding this comment.
The new convergence probe is wired to the wrong execution boundary. buildSandboxInferenceRouteProbeArgs() returns OpenShell subcommand argv beginning with sandbox, but waitForHermesInferenceRouteConvergence() passes that array to the generic runner.run(), which executes argv[0] directly. In production this attempts to spawn an executable named sandbox rather than the resolved OpenShell CLI, so all Hermes probes fail and shields down rolls back even when the route is healthy. Please execute through the OpenShell adapter (or otherwise prepend the resolved OpenShell binary through an injectable boundary) and add a regression that exercises/asserts that executable boundary instead of teaching the mock that sandbox is a valid executable. Keep the existing bounded retries and rollback assertions.
Signed-off-by: Prekshi Vyas <prekshiv@nvidia.com>
…erence-convergence Signed-off-by: Prekshi Vyas <prekshiv@nvidia.com>
Signed-off-by: Prekshi Vyas <prekshiv@nvidia.com>
…erence-convergence Signed-off-by: Prekshi Vyas <prekshiv@nvidia.com>
|
Addressed the change request and the still-current advisor blocker on head
Please re-review when convenient. |
Signed-off-by: Prekshi Vyas <prekshiv@nvidia.com>
|
Advisor suggestion PRA-1 is also addressed on head |
Commit 3a89844 now runs the convergence probe through the resolved OpenShell binary. The regression asserts the full OpenShell executable and sandbox exec argv.
…erence-convergence Signed-off-by: Prekshi Vyas <prekshiv@nvidia.com>
|
Current-main refresh completed.
Re-requesting review on the current head. |
cv
left a comment
cv
left a comment
There was a problem hiding this comment.
Run the missing required checks workflow and obtain a current automated review for a10ab759; the available advisor result covers ec2b6f93. This is security-sensitive Shields rollback and convergence code, so approval requires head-bound CI and review evidence. The bounded route probe, strict immediate MCP assertion, and rollback-on-timeout design are otherwise aligned with the reported failure.
|
Update: this evidence was superseded while it was being posted. Reviewer cv merged new main 2f520b3 into the branch, producing head 31ea118. The linked checks and advisor run below are valid for the prior head a10ab75 only; I am revalidating the refreshed head and will post new exact-head evidence after its automatic workflows finish. PRA-1 assessment remains: the existing live regression already exercises this boundary. test/e2e/live/mcp-bridge-hermes-lifecycle.ts runs the production shields-down transition, whose new path probes https://inference.local/v1/models with the OpenShell-managed CA through the resolved OpenShell binary. test/e2e/live/mcp-bridge.test.ts then immediately performs the real Hermes MCP call through SandboxClient.execShell, whose process boundary is OpenShell sandbox exec. The two cited source jobs failed at that immediate assertion with HTTP 503, demonstrating that this live contract reaches the requested policy and route boundary. A second mocked test would not add live-boundary coverage. No E2E workflow was manually dispatched. |
Signed-off-by: Prekshi Vyas <prekshiv@nvidia.com>
…erence-convergence Signed-off-by: Prekshi Vyas <prekshiv@nvidia.com>
|
PRA-1 is fixed in signed commit 229814c. The focused policy test now mocks resolveOpenshell to /opt/nvidia/bin/openshell and asserts the exact argv for buildPolicySetCommand, buildPolicyGetCommand, and buildPolicyGetFullCommand. The integration suite passes 75/75. Current head: e3cb9e0 No E2E workflow was manually dispatched. Please re-review this current head when convenient. |
cjagwani
left a comment
cjagwani
left a comment
There was a problem hiding this comment.
Independent E2E-drain re-review PASS on revised head: the convergence probe now resolves and prepends the OpenShell executable before passing argv to the generic runner; a default-boundary regression asserts that exact executable path; retries and sleeps remain bounded; only HTTP 2xx is accepted as usable; unusable responses roll the Shields transaction back without recording a successful audit event; focused tests cover 503 convergence, exhausted budgets, hostile output, non-2xx responses, route-specific recovery guidance, and rollback. Required checks, DCO, commit lint, and installer hash are green. Merge remains subject to exact-head verification, actionable-feedback closure, branch currency, and all protections.
Signed-off-by: Prekshi Vyas <prekshiv@nvidia.com>
|
Advisor blocker PRA-1 is fixed in signed commit 3a31307. The dedicated hermes-shields-config live E2E now calls https://inference.local/v1/models immediately after each successful Shields-down return and before status, posture, restart, or other agent work. Each strict curl uses a 3-second connect timeout and 10-second total timeout with no retry or delay, then validates that the configured Hermes test model is present. The target contract and completion receipt record this post-transition assertion. Verification:
Please re-review head 3a31307. |
|
Current-head merge-gate evidence for 3a31307:
The earlier evidence-only change request is now satisfied on the current head. Please clear it and re-approve when convenient; merge remains gated on a current approval and currency with main. |
3 participants
Summary
Main E2E evidence
mcp-bridge (hermes)): the local Hermes gateway returned HTTP 503 220 ms aftershields downreported successmcp-bridge (hermes)): the same assertion returned HTTP 503 in 100 msBoth failing policy transitions took about 11 seconds and reported the new policy loaded; the passing transition took 3.5 seconds. This change makes the command prove
inference.local/v1/modelsis usable before returning.Verification
npx vitest run src/lib/shields/inference-convergence.test.ts src/lib/shields/legacy-hermes-compat.test.ts src/lib/shields/policy-transition.test.ts(48 tests)npm run typechecknpm run build:clinpx biome checkon changed filesgit diff --checkSigned-off-by: Prekshi Vyas prekshiv@nvidia.com
Summary by CodeRabbit
New Features
Bug Fixes
Tests