test(state): move MCP lifecycle-lock classifier properties to the source suite - #8436
Merged
prekshivyas merged 4 commits intoAug 6, 2026
Merged
Conversation
…rce suite The classifier properties lived in test/helpers/mcp-lifecycle-lock-properties.ts, which imported Vitest and registered a describe block. test/mcp-lifecycle-lock.test.ts pulled them in by side-effect import, so src/lib/state/mcp-lifecycle-lock-identity.ts did not own the coverage of its own classifier. Move the four unique properties into the source-local suite and delete the helper. - 'keeps foreign-host and foreign-namespace contenders active' is not moved. Its host arm is already covered by 'never probes or reaps an owner from a different host' and its namespace arm by the PID-namespace property, both with broader arbitraries. Its distinct nowMs and grace values are inert: classifyMcpLifecycleLock reads nowMs, mtimeMs and corruptLockGraceMs only on the no-owner or wrong-sandbox branch (mcp-lifecycle-lock-identity.ts:209-213). - 'keeps ownership when a cached start mismatch disappears on refresh' is replaced by the moved 'reaps a live PID only when a fresh identity read confirms the mismatch', which covers the same match case, adds the uncovered unavailable case, and asserts the full read sequence instead of one call. The moved properties keep 250 runs, the recorded seed, and the 15s timeout. No production code changes. Refs: NVIDIA#8356 Signed-off-by: harjoth <harjoth.khara@gmail.com>
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (3)
💤 Files with no reviewable changes (2)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThe PR expands seeded lifecycle-lock property tests for owner liveness, fresh process identity, corrupt or wrong-sandbox locks, grace-period boundaries, and shared test timeouts. It also removes the former property-test helper. ChangesLifecycle lock identity tests
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
harjothkhara
marked this pull request as ready for review
August 6, 2026 04:51
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
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/state/mcp-lifecycle-lock-identity.test.ts`:
- Around line 384-410: Update the property-based test around
classifyMcpLifecycleLock to derive the tested age values from the generated
graceMs, explicitly covering ageMs equal to graceMs and at least one
boundary-adjacent case. Replace the independent ageMs duration generator while
preserving the existing wrong-sandbox-owner setup and expected stale-versus-wait
assertion.
🪄 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: 157bc657-b981-45cd-bdde-ae321677a669
📒 Files selected for processing (3)
src/lib/state/mcp-lifecycle-lock-identity.test.tstest/helpers/mcp-lifecycle-lock-properties.tstest/mcp-lifecycle-lock.test.ts
💤 Files with no reviewable changes (2)
- test/mcp-lifecycle-lock.test.ts
- test/helpers/mcp-lifecycle-lock-properties.ts
Contributor
PR Review Advisor — No blocking findings reportedAdvisor assessment: No blocking advisor findings reported Model lanes
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: None This automated review informs maintainers. Warnings and suggestions do not require a response. A maintainer decides whether to merge. |
harjothkhara
added a commit
to harjothkhara/NemoClaw
that referenced
this pull request
Aug 6, 2026
The moved property is named for the grace boundary but drew ageMs and graceMs as two independent durations, so ageMs === graceMs was almost never sampled and the >= boundary went untested. Relaxing the classifier from >= to > at mcp-lifecycle-lock-identity.ts:210 still passed 15/15 before this change. Test ages graceMs - 1, graceMs, and graceMs + 1 alongside the random age, which keeps the broad coverage the random draw gave. The >= mutation now fails. Reported by CodeRabbit on NVIDIA#8436. Refs: NVIDIA#8356 Signed-off-by: harjoth <harjoth.khara@gmail.com>
The lifecycle-lock grace property moved in 7e586eb is named for the grace boundary but drew ageMs and graceMs as two independent durations, so ageMs === graceMs was almost never sampled and the >= boundary went untested. Relaxing the classifier from >= to > at mcp-lifecycle-lock-identity.ts:210 still passed 15/15 before this change. Test ages graceMs - 1, graceMs, and graceMs + 1 alongside the random age, which keeps the broad coverage the random draw gave. The >= mutation now fails. Reported by CodeRabbit on NVIDIA#8436. Refs: NVIDIA#8356 Signed-off-by: harjoth <harjoth.khara@gmail.com>
harjothkhara
force-pushed
the
oss-find/nemoclaw-2026-08-06b
branch
from
August 6, 2026 05:00
1f13b35 to
4333809
Compare
Contributor
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
Collaborator
|
Gate classification for the current branch revision:
The remaining merge gate is independent approval. |
Signed-off-by: Apurv Kumaria <akumaria@nvidia.com>
Contributor
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
Collaborator
|
Maintainer refresh completed for the current branch revision (4484b01).
Fresh repository checks are now running. Independent approval remains required before merge. |
prekshivyas
approved these changes
Aug 6, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
The MCP lifecycle-lock classifier properties lived in
test/helpers/mcp-lifecycle-lock-properties.ts, which imported Vitest and registered adescribeblock.test/mcp-lifecycle-lock.test.tspulled them in with a side-effect import, sosrc/lib/state/mcp-lifecycle-lock-identity.tsdid not own the tests for its own classifier. This moves the unique properties into the source-local suite and deletes the helper. No production code changes.Related Issue
Refs: #8356
Changes
src/lib/state/mcp-lifecycle-lock-identity.test.ts: the grace-boundary property, the live-owner age/grace property, the main/reaper token-parity property, and the fresh-read property.owner,observation,probes,boundaryPidArbitrary, andprocessIdentityArbitrary, which are equal or broader than the helper's. AdddurationArbitrary, andSEEDED_PROPERTY_PARAMETERSto keep 250 runs and the recorded seed0x5876c0de.test/helpers/mcp-lifecycle-lock-properties.tsand its side-effect import.PROPERTY_IO_TIMEOUT_MStoPROPERTY_TIMEOUT_MS. The same 15s value now covers both describe blocks, so theIOqualifier no longer distinguishes a real case.Two judgment calls a reviewer should check:
keeps foreign-host and foreign-namespace contenders active without local probingis not moved. Its host arm is already covered bynever probes or reaps an owner from a different hostand its namespace arm by the PID-namespace property, both of which vary the local identity itself instead of holding it fixed, so they are strictly broader. Its only other difference isnowMs = MAX_SAFE_INTEGERandgraceMs = 1, and those are inert here:classifyMcpLifecycleLockreadsnowMs,mtimeMs, andcorruptLockGraceMsonly on the no-owner or wrong-sandbox branch (src/lib/state/mcp-lifecycle-lock-identity.ts:209-213), which a present, correct-sandbox owner never reaches.keeps ownership when a cached start mismatch disappears on refreshis deleted. The movedreaps a live PID only when a fresh identity read confirms the mismatchcovers the same match case, adds theunavailablefresh-read case that nothing covered before, and asserts the whole read sequence instead of a single call. Keeping both would leave an equivalent property duplicated, which the issue asks to remove.Type of Change
Quality Gates
docs/manage-sandboxes/manage-mcp-servers.mdx,docs/reference/troubleshoot-mcp-servers.mdx) document timeouts and recovery, none of which move. No documentation references the deleted helper.Documentation Writer Review
no-docs-neededdocs/for pages covering the MCP lifecycle lock and confirmed each documents operator-visible behavior (timeouts, retry guidance, crash recovery) that this change does not touch, and found no reference totest/helpers/mcp-lifecycle-lock-properties.tsor to the root suite owning classifier properties anywhere indocs/,CLAUDE.md,AGENTS.md,CONTRIBUTING.md,test/e2e/README.md,vitest.config.ts,test/helpers/vitest-watch-triggers.ts,ci/, orscripts/.no-docs-needed. The PR-relative diff remains limited to the same three test files and does not change user-facing behavior.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 testfor broad runtime/test-harness changes;npm run checkfor repo-wide validation/coverage changes — command/result:npm run docsbuilds without warnings (doc changes only)The issue's validation block
61 tests before, 59 after: 4 properties moved, 1 dropped as already covered, 1 pre-existing property removed as subsumed.
Also clean:
npm run typecheck:cli,npm run test:titles:check,npm run test-size:check,npx biome check.Proof that the coverage actually moved
A consolidation can silently drop coverage while every suite stays green, so I mutated the classifier and checked which lane notices. Before this change the
clilane — the source-local owner — was blind to both mutations:clibeforeintegrationbeforecliafteridentity.ts:212"wait"→"stale"identity.ts:239drop therefreshedIdentity !== nullguardEach failure is the expected property: the grace-boundary one for the first mutation, the fresh-read one for the second. The classifier was restored to its committed state afterwards; this PR contains no production change.
Follow-up commit: the boundary property did not test its boundary
CodeRabbit caught a real gap, and it predates this PR — the property came over from the helper as-is. It is named for the grace boundary but drew
ageMsandgraceMsas two independentfc.integer({min:1,max:1_000_000})values, soageMs === graceMswas essentially never sampled. Relaxing the classifier from>=to>atmcp-lifecycle-lock-identity.ts:210passed 15/15.It now tests
graceMs - 1,graceMs,graceMs + 1, and the original randomageMs, so the broad random coverage is kept rather than replaced. The same>=mutation now fails.So one of the four properties is strengthened rather than relocated unchanged; the other three are verbatim.
Honest limits
src/lib/state/fail on my machine —state-file-key-merge-behavior.test.tsandstate-file-key-merge-file-safety.test.ts, 16 tests. They fail identically with my three paths restored fromorigin/main, so they are pre-existing here and untouched by this change.test/mcp-lifecycle-lock.test.tsI saw one transient failure that did not reproduce in the eight runs after it. That file spawns real child processes and binds sockets, so it is timing-sensitive on a loaded machine. I could not attribute it to a specific test and it is not related to the moved properties, which are pure in-process property tests.Signed-off-by: harjoth harjoth.khara@gmail.com
Summary by CodeRabbit