fix(state): make lifecycle lock timeouts side-effect free - #7859
Conversation
Signed-off-by: Ho Lim <subhoya@gmail.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:
📝 WalkthroughWalkthroughMCP lifecycle lock acquisition now uses an injectable monotonic clock and fixed deadline guards across observation, stale-lock reclamation, publication, reaper admission, and critical-section entry. Reclaimed generations are restored when a post-claim deadline check fails. Tests cover these boundaries. ChangesMCP lock timeout enforcement
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Caller
participant acquireMcpLifecycleLock
participant LockStorage
participant CriticalSection
Caller->>acquireMcpLifecycleLock: Request lock with timeout
acquireMcpLifecycleLock->>LockStorage: Observe or reclaim lock generation
LockStorage-->>acquireMcpLifecycleLock: Return state or restore generation
acquireMcpLifecycleLock->>acquireMcpLifecycleLock: Assert monotonic deadline
acquireMcpLifecycleLock->>CriticalSection: Enter protected operation before deadline
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
PR Review Advisor — No blocking findings reportedAdvisor assessment: No blocking advisor findings reported Model lanes
Second-opinion terminology and E2E selections are advisory. They do not change the primary assessment or E2E / PR Gate. 2 semantic terminology decisionsTerminology decisions are advisory. They affect the assessment only when a separate finding identifies concrete semantic impact.
E2E guidanceAdvisory only. E2E / PR Gate selects and runs jobs independently. Recommended E2E: This automated review informs maintainers. Warnings and suggestions do not require a response. A maintainer decides whether to merge. |
Signed-off-by: Ho Lim <subhoya@gmail.com>
wscurran
added
area: architecture
|
✨ Thanks for the PR. This fixes the lifecycle lock timeout regression by making it a fail-closed mutation boundary with deterministic tests. Maintainers will review the lock generation preservation and critical-section entry logic. Related open issues: |
|
@cv @NVIDIA/nemoclaw-maintainer Could you please help with the protected E2E disposition for exact head 19f17ff?
I cannot authorize or repair that trusted-run path from the fork. I will keep this head unchanged so checks are not reset again. Please either authorize/retrigger the repository-supported E2E path for this exact head, identify any remaining code or evidence blocker I can address, or mark the PR superseded if its outcome is no longer wanted. |
apurvvkumaria
left a comment
apurvvkumaria
left a comment
There was a problem hiding this comment.
Blocking finding confirmed at exact head 19f17ff: the change promises side-effect-free lifecycle-lock timeouts, but stale main-lock and reaper reclamation only check the deadline before awaiting reclaimStaleMcpLifecycleLockGeneration. If the asynchronous rename or deletion crosses the deadline, a pre-existing lock generation is still removed after the timeout boundary. That deterministically violates the stated fail-closed purpose and can admit a conflicting lifecycle mutation. Please make reclamation deadline-aware across the mutation boundary: re-check after the atomic claim and restore the claimed generation without overwriting any replacement when the deadline has expired, then reject with the timeout. Add deterministic main-lock and reaper tests that advance the injected monotonic clock during reclamation and prove the original generation remains. The existing advisor finding documents the same location, so I am not duplicating an inline comment.
Signed-off-by: Ho Lim <subhoya@gmail.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: 1
🧹 Nitpick comments (1)
test/mcp-lifecycle-lock.test.ts (1)
498-498: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAppend
(#7858)to both test titles. The enclosingdescribetitle does not provide the required issue suffix.🤖 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/mcp-lifecycle-lock.test.ts` at line 498, Append "(`#7858`)" to the title of the test beginning with "restores a stale main lock when reclamation crosses the deadline", and update the other test title in the same test file similarly so both titles include the required issue suffix.Sources: Coding guidelines, Learnings
🤖 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/state/mcp-lifecycle-lock-storage.ts`:
- Around line 100-109: Update restoreClaimedMcpLifecycleLockGeneration to
restore quarantined directories as well as regular files, replacing the
hard-link-only approach with a path-type-compatible restoration strategy and
preserving cleanup/error handling. Ensure the canonical lock path is restored
when deadline assertion fails, and add a timeout test covering a corrupt
directory.
---
Nitpick comments:
In `@test/mcp-lifecycle-lock.test.ts`:
- Line 498: Append "(`#7858`)" to the title of the test beginning with "restores a
stale main lock when reclamation crosses the deadline", and update the other
test title in the same test file similarly so both titles include the required
issue suffix.
🪄 Autofix (Beta)
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: 96c2168c-1811-49bf-a748-286d2404d5f4
📒 Files selected for processing (3)
src/lib/state/mcp-lifecycle-lock-acquisition.tssrc/lib/state/mcp-lifecycle-lock-storage.tstest/mcp-lifecycle-lock.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/lib/state/mcp-lifecycle-lock-acquisition.ts
apurvvkumaria
left a comment
apurvvkumaria
left a comment
There was a problem hiding this comment.
Re-reviewed the current revision after the requested deadline fix. Both stale-main-lock and stale-reaper reclamation now perform a post-claim deadline assertion; if the deadline was crossed, the claimed generation is restored to its canonical path before the timeout propagates. Regression tests force the clock past the deadline immediately after rename and confirm the callback is not entered and the original owner token is restored. The exact-head advisor reports no blocking findings. The remaining growth-guardrail failure concerns two if statements in the new test setup and is nonblocking test-structure cleanup. The prior blocking correctness finding is resolved, and I found no new blocking defect.
Signed-off-by: Ho Lim <subhoya@gmail.com>
apurvvkumaria
left a comment
apurvvkumaria
left a comment
There was a problem hiding this comment.
Re-reviewed the latest revision. It preserves the resolved post-claim deadline behavior and now marks directory observations non-reclaimable, so a corrupt directory at either lock path is waited on and reported by timeout instead of being renamed into a form the hard-link restoration protocol cannot restore. Other file, symlink, and parsed-owner generations retain the existing reclaim behavior. The new regression confirms the directory remains intact; the small test-structure rewrite also satisfies the conditional guardrail without weakening assertions. No new blocking correctness, security, compatibility, or regression defect found.
Signed-off-by: Prekshi Vyas <prekshiv@nvidia.com>
apurvvkumaria
left a comment
apurvvkumaria
left a comment
There was a problem hiding this comment.
Approve — re-reviewed exact head e495e8d. The only change since the covered merge head adds issue references to five existing test titles; it does not alter product logic, fixtures, assertions, or the resolved post-claim deadline and directory-reclamation behavior. No new blocking correctness, security, compatibility, test, or regression defect was introduced.
Signed-off-by: Ho Lim <subhoya@gmail.com>
apurvvkumaria
left a comment
apurvvkumaria
left a comment
There was a problem hiding this comment.
Approve — re-reviewed exact head ce22702 after its mainline merge. The sole PR-specific conflict resolution correctly awaits stale-generation reclamation inside the try block, so the finally clause cannot release the reaper generation while async claim, deadline validation, or restoration is still in flight. This preserves the previously reviewed post-claim deadline and fail-closed restoration guarantees. Both exact-head advisor lanes and the growth guardrail pass. No blocking correctness, security, compatibility, or regression defect found.
|
Exact-head infrastructure disposition for ce22702: both advisor lanes and the visible required checks are green, but E2E / PR Gate Coordination has remained in progress with title Waiting for PR CI since 02:16 UTC. The controller waited nearly six hours and then timed out at 08:14 UTC without a selected E2E verdict. This is a stuck coordination state rather than a reported product-test failure. Please use the supported coordination retrigger/recovery path for this exact head; I am not creating a branch commit or treating the timeout as passing evidence. |
Signed-off-by: Charan Jagwani <cjagwani@nvidia.com>
Signed-off-by: Charan Jagwani <cjagwani@nvidia.com>
cjagwani
left a comment
cjagwani
left a comment
There was a problem hiding this comment.
Security review — exact head d12c4523980d7958fdacf72a9397c502b059f6ee against base 7f429e3588d545398411dd9caae92938eaac1853: PASS with no findings.
- Secrets and credentials — PASS. No secret, credential, logging, or environment behavior changes.
- Input validation and injection resistance — PASS. Lock metadata remains schema-validated and identity-bound. Timeout and deadline checks use the existing bounded monotonic-time inputs and do not introduce command or path interpolation.
- Authentication and authorization — PASS. Ownership remains tied to the exact lifecycle-lock generation and process identity; no caller gains authority over another operation's lock.
- Dependencies and supply chain — PASS. No dependency, lockfile, image, or external source change.
- Error handling and information exposure — PASS. Timeout paths restore a claimed stale generation only when doing so cannot overwrite a replacement; ambiguous or corrupt state remains fail-closed and no sensitive detail is emitted.
- Cryptography and data protection — PASS / not applicable. No cryptographic behavior changes. Atomic hard-link and directory-replacement rules continue to protect persisted lock state.
- Configuration and infrastructure — PASS. No policy, privilege, port, network, header, or supported configuration change.
- Security testing — PASS. Exact focused suites pass 88 with 2 skipped across acquisition, identity, lifecycle, and messaging-compatible endpoint coverage, including deadline crossings, replacement generations, corrupt state, cleanup ownership, and deterministic clock behavior.
- System security — PASS. The change prevents post-timeout side effects and cannot remove or overwrite a replacement generation. The new base's host-local vLLM serving infrastructure is unrelated to lifecycle-lock ownership, release, and cleanup boundaries.
The signed merge refresh preserves the effective eight-file patch byte-for-byte (raw SHA-256 c7e34040c138eac19ba8f81f15ddb3c606fe4a16b98c62e8b052399fec35aa9d; stable patch ID 21a1c4b9d143b22040e61119067a954864b71659). Fresh CI, protected E2E, and independent post-push approval remain mandatory; no prior-base result is accepted.
Signed-off-by: Charan Jagwani <cjagwani@nvidia.com>
cjagwani
left a comment
cjagwani
left a comment
There was a problem hiding this comment.
Security review — exact head 6998bfb5d694d0f0139dac69d941114143510c7a against base 6f2cab8aa67bf75337efa7d55330a3bea61c27ee: PASS with no findings.
- Secrets and credentials — PASS. No secret, credential, logging, or environment behavior changes.
- Input validation and injection resistance — PASS. Lock metadata remains schema-validated and identity-bound. Timeout and deadline checks use the existing bounded monotonic-time inputs and do not introduce command or path interpolation.
- Authentication and authorization — PASS. Ownership remains tied to the exact lifecycle-lock generation and process identity; no caller gains authority over another operation's lock.
- Dependencies and supply chain — PASS. No dependency, lockfile, image, or external source change.
- Error handling and information exposure — PASS. Timeout paths restore a claimed stale generation only when doing so cannot overwrite a replacement; ambiguous or corrupt state remains fail-closed and no sensitive detail is emitted.
- Cryptography and data protection — PASS / not applicable. No cryptographic behavior changes. Atomic hard-link and directory-replacement rules continue to protect persisted lock state.
- Configuration and infrastructure — PASS. No policy, privilege, port, network, header, or supported configuration change.
- Security testing — PASS. Exact focused suites pass 88 with 2 skipped across acquisition, identity, lifecycle, and messaging-compatible endpoint coverage, including deadline crossings, replacement generations, corrupt state, cleanup ownership, and deterministic clock behavior.
- System security — PASS. The change prevents post-timeout side effects and cannot remove or overwrite a replacement generation. The new base's host-local vLLM serving infrastructure and Hermes health-streak reset are unrelated to lifecycle-lock ownership, release, and cleanup boundaries.
The signed merge refresh preserves the effective eight-file patch byte-for-byte (raw SHA-256 c7e34040c138eac19ba8f81f15ddb3c606fe4a16b98c62e8b052399fec35aa9d; stable patch ID 21a1c4b9d143b22040e61119067a954864b71659). Fresh CI, protected E2E, and independent post-push approval remain mandatory; no prior-base result is accepted.
|
Refreshed onto current
Fresh ordinary CI is running. Protected E2E remains unapproved until the exact risk plan is available and verified. An independent approval of this post-push head is still required; no prior-base result or review is being carried forward. |
cjagwani
left a comment
cjagwani
left a comment
There was a problem hiding this comment.
Security review — exact head a14f5667122cc3f22770759b55a5d45a66d74e0e against base 287e4747215dd2407441e3add7a4f4cb46ad13c6: PASS with no findings.
- Secrets and credentials — PASS. No secret, credential, logging, or environment behavior changes.
- Input validation and injection resistance — PASS. Lock metadata remains schema-validated and identity-bound. Timeout and deadline checks use existing bounded monotonic-time inputs and introduce no command or path interpolation.
- Authentication and authorization — PASS. Ownership remains tied to the exact lifecycle-lock generation and process identity; no caller gains authority over another operation's lock.
- Dependencies and supply chain — PASS. No dependency, lockfile, image, or external source change.
- Error handling and information exposure — PASS. Timeout paths restore a claimed stale generation only when that cannot overwrite a replacement; ambiguous or corrupt state remains fail-closed and no sensitive detail is emitted.
- Cryptography and data protection — PASS / not applicable. No cryptographic behavior changes. Atomic hard-link and directory-replacement rules continue to protect persisted lock state.
- Configuration and infrastructure — PASS. No policy, privilege, port, network, header, or supported configuration change.
- Security testing — PASS. The exact focused suites previously passed 88 with 2 platform skips across acquisition, identity, lifecycle, and messaging-compatible endpoint coverage, including deadline crossings, replacement generations, corrupt state, cleanup ownership, and deterministic clock behavior. Fresh GitHub Actions remains authoritative for this exact head.
- System security — PASS. The change prevents post-timeout side effects and cannot remove or overwrite a replacement generation.
The signed merge refresh had no conflict resolution and preserves the effective eight-file patch byte-for-byte (raw SHA-256 c7e34040c138eac19ba8f81f15ddb3c606fe4a16b98c62e8b052399fec35aa9d; stable patch ID 21a1c4b9d143b22040e61119067a954864b71659). Base PR #8370 changes internal shared contributor/reviewer guidance and Advisor tooling only; it does not alter lifecycle-lock ownership, release, or cleanup boundaries. Fresh CI, protected E2E, documentation receipt, and independent post-push approval remain mandatory; no prior-head or prior-base result is accepted.
cjagwani
left a comment
cjagwani
left a comment
There was a problem hiding this comment.
Security review — exact head 95967eadee3fa8a13b6fc87376ada97d7b4b354b against base c176af5f08595ed748b4eeac8988c554e180183e: PASS with no findings.
- Secrets and credentials — PASS. No secret, credential, logging, or environment behavior changes.
- Input validation and injection resistance — PASS. Lock metadata remains schema-validated and identity-bound. Timeout and deadline checks use existing bounded monotonic-time inputs and introduce no command or path interpolation.
- Authentication and authorization — PASS. Ownership remains tied to the exact lifecycle-lock generation and process identity; no caller gains authority over another operation's lock.
- Dependencies and supply chain — PASS. No dependency, lockfile, image, or external source change.
- Error handling and information exposure — PASS. Timeout paths restore a claimed stale generation only when that cannot overwrite a replacement; ambiguous or corrupt state remains fail-closed and no sensitive detail is emitted.
- Cryptography and data protection — PASS / not applicable. No cryptographic behavior changes. Atomic hard-link and directory-replacement rules continue to protect persisted lock state.
- Configuration and infrastructure — PASS. No policy, privilege, port, network, header, or supported configuration change.
- Security testing — PASS. The exact focused suites previously passed 88 with 2 platform skips across acquisition, identity, lifecycle, and messaging-compatible endpoint coverage. Fresh GitHub Actions remains authoritative for this head.
- System security — PASS. The change prevents post-timeout side effects and cannot remove or overwrite a replacement generation.
The signed merge was conflict-free and preserves the effective eight-file patch byte-for-byte (raw SHA-256 c7e34040c138eac19ba8f81f15ddb3c606fe4a16b98c62e8b052399fec35aa9d; stable patch ID 21a1c4b9d143b22040e61119067a954864b71659). Base PR #8371 extracts the existing vLLM Hugging Face download operation behind its preserved API; it does not alter lifecycle-lock ownership, release, persistence, or cleanup. Fresh CI, protected E2E, documentation receipt, and independent post-push approval remain mandatory.
Signed-off-by: Charan Jagwani <cjagwani@nvidia.com>
cjagwani
left a comment
cjagwani
left a comment
There was a problem hiding this comment.
Verdict
Security review — exact head cd31fa8dd2584db6dcfa24246949c26839f58c5d against base 3b208d79e5d3bda4183704145ee5c28d79876ae1: PASS with no findings. The effective patch is safe from the reviewed security perspective, subject to every repository merge gate.
Findings Table
No findings.
Detailed Analysis
-
Secrets and Credentials — PASS. No secret, credential, logging, or environment behavior changes.
-
Input Validation and Data Sanitization — PASS. Lock metadata remains schema-validated and identity-bound. Timeout and deadline checks use existing bounded monotonic-time inputs and introduce no command or path interpolation.
-
Authentication and Authorization — PASS. Ownership remains tied to the exact lifecycle-lock generation and process identity; no caller gains authority over another operation's lock.
-
Dependencies and Third-Party Libraries — PASS. No dependency, lockfile, image, or external source change.
-
Error Handling and Logging — PASS. Timeout paths restore a claimed stale generation only when that cannot overwrite a replacement; ambiguous or corrupt state remains fail-closed and no sensitive detail is emitted.
-
Cryptography and Data Protection — PASS / not applicable. No cryptographic behavior changes. Atomic hard-link and directory-replacement rules continue to protect persisted lock state.
-
Configuration and Security Headers — PASS. No policy, privilege, port, network, header, or supported configuration change.
-
Security Testing — PASS. The exact focused suites previously passed 88 with 2 platform skips across acquisition, identity, lifecycle, and messaging-compatible endpoint coverage. Fresh GitHub Actions remains authoritative for this head.
-
System Security — PASS. The change prevents post-timeout side effects and cannot remove or overwrite a replacement generation.
Files Reviewed
src/lib/state/mcp-lifecycle-lock-acquisition.test.tssrc/lib/state/mcp-lifecycle-lock-acquisition.tssrc/lib/state/mcp-lifecycle-lock-identity.test.tssrc/lib/state/mcp-lifecycle-lock-identity.tssrc/lib/state/mcp-lifecycle-lock-storage.tstest/e2e/support/messaging-compatible-endpoint-helpers.test.tstest/helpers/mcp-lifecycle-lock-properties.tstest/mcp-lifecycle-lock.test.ts
Provenance and Required Gates
The signed refresh merge is conflict-free and preserves the effective 8-file patch byte-for-byte (raw SHA-256 c7e34040c138eac19ba8f81f15ddb3c606fe4a16b98c62e8b052399fec35aa9d; stable patch ID 21a1c4b9d143b22040e61119067a954864b71659). Base PR #8372 changes the canonical security rubric and PR Review Advisor machinery only; it does not alter any effective PR file or runtime trust boundary. This review independently applies the new rubric.
Fresh CI, protected E2E, documentation receipt, unresolved-review resolution, and independent exact-head approval remain mandatory. This security review waives none of those gates.
Signed-off-by: Charan Jagwani <cjagwani@nvidia.com>
cjagwani
left a comment
cjagwani
left a comment
There was a problem hiding this comment.
Verdict
Security review — exact head 29303579efe8ef32cc4e91f4fdd75de753fc87e1 against base c6713c9dd17a9682654713ccb6829b8714f1b307: PASS with no findings. The effective patch is safe from the reviewed security perspective, subject to every repository merge gate.
Findings Table
No findings.
Detailed Analysis
-
Secrets and Credentials — PASS. No secret, credential, logging, or environment behavior changes.
-
Input Validation and Data Sanitization — PASS. Lock metadata remains schema-validated and identity-bound. Timeout and deadline checks use existing bounded monotonic-time inputs and introduce no command or path interpolation.
-
Authentication and Authorization — PASS. Ownership remains tied to the exact lifecycle-lock generation and process identity; no caller gains authority over another operation's lock.
-
Dependencies and Third-Party Libraries — PASS. No dependency, lockfile, image, or external source change.
-
Error Handling and Logging — PASS. Timeout paths restore a claimed stale generation only when that cannot overwrite a replacement; ambiguous or corrupt state remains fail-closed and no sensitive detail is emitted.
-
Cryptography and Data Protection — PASS / not applicable. No cryptographic behavior changes. Atomic hard-link and directory-replacement rules continue to protect persisted lock state.
-
Configuration and Security Headers — PASS. No policy, privilege, port, network, header, or supported configuration change.
-
Security Testing — PASS. The exact focused suites previously passed 88 with 2 platform skips across acquisition, identity, lifecycle, and messaging-compatible endpoint coverage. Fresh GitHub Actions remains authoritative for this head.
-
System Security — PASS. The change prevents post-timeout side effects and cannot remove or overwrite a replacement generation.
Files Reviewed
src/lib/state/mcp-lifecycle-lock-acquisition.test.tssrc/lib/state/mcp-lifecycle-lock-acquisition.tssrc/lib/state/mcp-lifecycle-lock-identity.test.tssrc/lib/state/mcp-lifecycle-lock-identity.tssrc/lib/state/mcp-lifecycle-lock-storage.tstest/e2e/support/messaging-compatible-endpoint-helpers.test.tstest/helpers/mcp-lifecycle-lock-properties.tstest/mcp-lifecycle-lock.test.ts
Provenance and Required Gates
The signed refresh merge is conflict-free and preserves the effective 8-file patch byte-for-byte (raw SHA-256 c7e34040c138eac19ba8f81f15ddb3c606fe4a16b98c62e8b052399fec35aa9d; stable patch ID 21a1c4b9d143b22040e61119067a954864b71659). Base PR #8383 adds only the v0.0.103 release-note page and does not alter any effective PR file or reviewed runtime trust boundary. Base PR #8372 changes the canonical security rubric and PR Review Advisor machinery only; it does not alter any effective PR file or runtime trust boundary. This review independently applies the new rubric.
Fresh CI, protected E2E, documentation receipt, unresolved-review resolution, and independent exact-head approval remain mandatory. This security review waives none of those gates.
Signed-off-by: Charan Jagwani <cjagwani@nvidia.com>
cjagwani
left a comment
cjagwani
left a comment
There was a problem hiding this comment.
Verdict
Security revalidation — exact head c7645de85ece551492eabb1e79f21036dbd1355e against current base 1608281462923ff8282151c7be1a109ee262b4e9: PASS with no findings, subject to every repository merge gate.
Findings Table
No findings.
Detailed Analysis
-
Secrets and Credentials — PASS. The effective PR patch is unchanged from the prior exact review. The base refresh adds no overlapping secret, credential, environment, or sensitive-logging path.
-
Input Validation and Data Sanitization — PASS. The reviewed validation and command/path construction are unchanged. The new base delta does not alter an effective PR file.
-
Authentication and Authorization — PASS. No identity, ownership, permission, or authorization boundary changed in the PR patch or its integration with the refreshed base.
-
Dependencies and Third-Party Libraries — PASS. The PR's reviewed dependency and artifact identities are unchanged. Base PR #8387 adds verified llama.cpp GGUF acquisition in eight disjoint files and does not change this PR's dependency surface.
-
Error Handling and Logging — PASS. The reviewed fail-closed behavior and diagnostics are unchanged; the base refresh introduces no overlapping error or logging path.
-
Cryptography and Data Protection — PASS. The PR's reviewed cryptographic and protected-data behavior is unchanged. No new cross-patch data flow exists.
-
Configuration and Security Headers — PASS. The effective PR patch preserves its reviewed configuration, network, privilege, capability, port, and header boundaries.
-
Security Testing — PASS. Raw patch SHA-256 remains
c7e34040c138eac19ba8f81f15ddb3c606fe4a16b98c62e8b052399fec35aa9dand stable patch ID remains21a1c4b9d143b22040e61119067a954864b71659. Documentation writer revalidation passed at this exact head with no docs change needed. Prior targeted test evidence remains applicable; fresh exact-head GitHub Actions is authoritative. -
System Security — PASS. The signed refresh is conflict-free, preserves the reviewed patch byte-for-byte, and introduces no new runtime trust-boundary interaction with the disjoint base delta.
Provenance and Required Gates
This exact-head revalidation supplements the full prior nine-category review at #7859 (review). It applies the current rubric from .agents tree 7320c9e331ddbcd9ff83e0fce9387a004e3bd1aa.
Fresh required CI, protected E2E, the exact documentation receipt, unresolved-review resolution, GitHub commit verification, and an independent exact-head approval remain mandatory. This review waives none of those gates.
Signed-off-by: Carlos Villela <cvillela@nvidia.com>
Signed-off-by: Carlos Villela <cvillela@nvidia.com>
cjagwani
left a comment
cjagwani
left a comment
There was a problem hiding this comment.
Verdict
Security review — exact head d9f3229d5b12133f8ded9b60feb63c8af13f7ce0 against current base b148bc13ee53b7a1f612d07e42275ec732e71877: PASS with no findings, subject to every repository merge gate.
Findings Table
No findings.
Detailed Analysis
-
Secrets and Credentials — PASS. The effective PR patch is unchanged and adds no secret, credential source, environment exposure, or sensitive logging. The refreshed base introduces no overlapping credential path.
-
Input Validation and Data Sanitization — PASS. Reviewed validation, command construction, and path handling remain unchanged. The portable-profile base delta has no exact file overlap with this PR.
-
Authentication and Authorization — PASS. No identity, ownership, permission, or authorization boundary is widened by the PR patch or its integration with the refreshed base.
-
Dependencies and Third-Party Libraries — PASS. The PR's reviewed dependency and artifact identities are unchanged. Base PRs #8333 and #8376 add disjoint E2E artifact restoration and portable experimental onboarding behavior.
-
Error Handling and Logging — PASS. Reviewed fail-closed behavior and diagnostics remain unchanged; the signed integrations add no overlapping error or logging path.
-
Cryptography and Data Protection — PASS. Reviewed cryptographic and protected-data behavior remains unchanged, with no new cross-patch data flow.
-
Configuration and Security Headers — PASS. The effective PR patch preserves its reviewed configuration, network, privilege, capability, port, and header boundaries. The hidden portable profile does not bypass this PR's authority checks.
-
Security Testing — PASS. Raw patch SHA-256 remains
c7e34040c138eac19ba8f81f15ddb3c606fe4a16b98c62e8b052399fec35aa9dand stable patch ID remains21a1c4b9d143b22040e61119067a954864b71659. The prior exact targeted lifecycle-lock evidence remains applicable. Exact-head documentation writer revalidation passed; fresh GitHub Actions is authoritative. -
System Security — PASS. Both signed refreshes are conflict-free, preserve the effective patch byte-for-byte, and introduce no new runtime trust-boundary interaction. For #7853 specifically, the portable profile supplies onboarding defaults but neither stages messaging credentials nor bypasses registry-driven credential-drift validation.
Provenance and Required Gates
The exact head preserves contributor history through signed merge commits and applies the current rubric from .agents tree 7320c9e331ddbcd9ff83e0fce9387a004e3bd1aa. This supplements the full prior nine-category review at #7859 (review).
Fresh required CI, protected E2E, the exact documentation receipt, unresolved-review resolution, GitHub commit verification, and an independent exact-head approval remain mandatory. This review waives none of those gates.
Signed-off-by: Carlos Villela <cvillela@nvidia.com>
Signed-off-by: Carlos Villela <cvillela@nvidia.com>
Signed-off-by: Carlos Villela <cvillela@nvidia.com>
Signed-off-by: Carlos Villela <cvillela@nvidia.com>
Signed-off-by: Carlos Villela <cvillela@nvidia.com>
Summary
Make the Model Context Protocol (MCP) lifecycle-lock timeout a fail-closed mutation boundary. After the monotonic acquisition deadline passes, NemoClaw preserves existing lock generations and does not enter the critical section.
Related Issue
Fixes #7858
Changes
Type of Change
Quality Gates
4f8e3b8c6287491471e6bc50a6d43f9e1dea76efagainst base SHAdb31c286129e878c3356eed49f76ab259561e47e. The detailed receipt appears below.Documentation Writer Review
no-docs-needed4f8e3b8c6against base SHAdb31c2861. The change enforces the documented MCP lifecycle-lock timeout contract, and the named test helper preserves the deadline-handoff behavior without changing a user-facing API, command, configuration, or recovery procedure.DGX Station Hardware Evidence
scripts/prepare-dgx-station-host.shdoes not change.Verification
Signed-off-by:line and every commit appears asVerifiedin GitHub — all 59 commits appear asVerified.pre-commit,commit-msg, andpre-pushhooks passed, ornpm run validate:prpassed after refreshingorigin/mainwhen hooks were skipped or unavailable — normal hooks passed, including repository checks, source-shape and test-size budgets, gitleaks, commit lint, and the CLI pre-push type check.npm run docsbuilds without warnings (doc changes only)Sensitive-Path Security Review
Verdict: PASS. The review found no security findings for current PR commit
4f8e3b8c6287491471e6bc50a6d43f9e1dea76efagainst base SHAdb31c286129e878c3356eed49f76ab259561e47e.Files reviewed:
src/lib/state/mcp-lifecycle-lock-acquisition.test.ts,src/lib/state/mcp-lifecycle-lock-acquisition.ts,src/lib/state/mcp-lifecycle-lock-identity.test.ts,src/lib/state/mcp-lifecycle-lock-identity.ts,src/lib/state/mcp-lifecycle-lock-storage.ts,test/e2e/support/messaging-compatible-endpoint-helpers.test.ts,test/helpers/mcp-lifecycle-lock-deadline-clock.ts,test/helpers/mcp-lifecycle-lock-properties.ts, andtest/mcp-lifecycle-lock.test.ts.GitHub Actions
GitHub Actions is authoritative for current PR commit
4f8e3b8c6287491471e6bc50a6d43f9e1dea76efagainst base SHAdb31c286129e878c3356eed49f76ab259561e47e. Required checks are running, GitHub reports merge stateBLOCKED, and no check waiver is recorded.Signed-off-by: Ho Lim subhoya@gmail.com