fix(rebuild): settle expired Shields before success - #8713
Conversation
Signed-off-by: Apurv Kumaria <akumaria@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. |
📝 WalkthroughWalkthroughThe pull request updates backup Shields relocking for expired deferred timers. It also adds redacted Hermes runtime diagnostics and gateway identity capture to the rebuild E2E flow, while removing brittle generated-script assertions. ChangesShields relocking
Hermes rebuild diagnostics
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Code Coverage OverviewLanguages: TypeScript TypeScript / code-coverage/pluginThe overall coverage in commit d5d0788 in the TypeScript / code-coverage/cliThe overall coverage in commit d5d0788 in the Show a code coverage summary of the most impacted files.
Updated |
Signed-off-by: Apurv Kumaria <akumaria@nvidia.com>
PR Review Advisor — No blocking findings reportedAdvisor assessment: No blocking advisor findings reported Model lanes
1 terminology difference 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. 4 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. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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 `@test/e2e/live/mcp-bridge-hermes-http.ts`:
- Around line 47-61: Update buildHermesMcpRuntimeDiagnosticsScript and its
FAILURE_BODY_EMITTER flow to avoid emitting raw service logs, process output,
command lines, or other secret-bearing content. Capture only explicitly
allowlisted, bounded diagnostic fields, and ensure every emitted value is fully
redacted before artifact capture rather than relying solely on API_SERVER_KEY
replacement or the existing four caller-supplied redactions.
🪄 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: 6572d05e-565a-4410-9119-4fbffdaa8409
📒 Files selected for processing (5)
src/lib/actions/sandbox/backup-shields-window.tstest/e2e/live/mcp-bridge-hermes-http.tstest/e2e/live/mcp-bridge.test.tstest/e2e/support/mcp-bridge-hermes-http.test.tstest/rebuild-shields-window.test.ts
💤 Files with no reviewable changes (1)
- test/e2e/support/mcp-bridge-hermes-http.test.ts
| export function buildHermesMcpRuntimeDiagnosticsScript(): string { | ||
| return [ | ||
| "set -eu", | ||
| "set -a", | ||
| "[ ! -f /sandbox/.hermes/.env ] || . /sandbox/.hermes/.env", | ||
| "set +a", | ||
| "{", | ||
| 'for log in /tmp/nemoclaw-start.log /tmp/gateway.log; do printf \'== %s ==\\n\' "$log"; tail -n 100 "$log" 2>&1 || true; done', | ||
| "printf '%s\\n' '== permissions =='", | ||
| "stat -c '%a %U:%G %n' /sandbox /sandbox/.hermes /sandbox/.hermes/logs 2>&1 || true", | ||
| "printf '%s\\n' '== managed supervisor =='", | ||
| "cat /run/nemoclaw/gateway-control/status 2>&1 || true", | ||
| "printf '%s\\n' '== gateway identity =='", | ||
| "cat /sandbox/.hermes/runtime/gateway.pid 2>&1 || true", | ||
| `} | /usr/bin/python3 -I -S -c ${shellQuote(FAILURE_BODY_EMITTER)} /dev/stdin`, |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Complete redaction before emitting runtime diagnostics.
Lines 54-61 include raw service log output. The local emitter replaces only API_SERVER_KEY. The caller also supplies only four named redaction values. Other credentials, tokens, request bodies, or command lines in these logs can reach the artifact unredacted.
Emit allowlisted diagnostic fields, or redact all secret-bearing values before artifact capture. As per path instructions, capture only bounded, redacted artifacts and do not emit credentials, tokens, request bodies, command lines, or child-process output to logs.
🤖 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/e2e/live/mcp-bridge-hermes-http.ts` around lines 47 - 61, Update
buildHermesMcpRuntimeDiagnosticsScript and its FAILURE_BODY_EMITTER flow to
avoid emitting raw service logs, process output, command lines, or other
secret-bearing content. Capture only explicitly allowlisted, bounded diagnostic
fields, and ensure every emitted value is fully redacted before artifact capture
rather than relying solely on API_SERVER_KEY replacement or the existing four
caller-supplied redactions.
Source: Path instructions
2 participants
Summary
A long Hermes rebuild can outlive an inherited Shields auto-restore timer. NemoClaw now settles an expired deferred timer through the existing fenced Shields-up path before reporting rebuild success, which prevents lockdown from racing the first post-rebuild tool call.
The live E2E lane now records bounded, redacted HTTP, runtime, fixture, Shields, and cleanup evidence for this boundary.
Related Issue
Fixes #8697
Changes
Confirmed evidence
31364512126, Hermes job93380168425, completed rebuild in 845,292 ms.Permission denied: '/sandbox/.hermes/logs'.Root cause and correction
The failure was not stale MCP configuration or credential rotation. The overdue Shields timer made
/sandbox/.hermesread-only immediately after rebuild released its lifecycle lock. Rebuild now settles that timer with the existing fenced Shields-up state transition before it reports success.Remaining validation
mcp-bridge (hermes)lane on the exact PR commit.MCP_AUTH_REWRITE_OK::nemoclaw-authenticated-mcp-proof.tools/callwith the rotated provider credential.Type of Change
Quality Gates
Documentation Writer Review
no-docs-neededdocs/is unchanged.DGX 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:cliandnpx prek run --from-ref origin/main --to-ref HEADpassed.npm testfor broad runtime/test-harness changes;npm run checkfor repo-wide validation/coverage changes — command/result: Not applicable. The diff changes one lifecycle decision and focused live evidence.npm run docsbuilds without warnings (doc changes only)Signed-off-by: Apurv Kumaria akumaria@nvidia.com
Summary by CodeRabbit
Bug Fixes
Tests