fix(shields): refuse a corrupt transition lock without a raw stack trace - #8122
Conversation
An unreadable owner record can never be recovered by waiting, but shields commands polled it for the full wait timeout and then let a plain Error reach the user. Refuse as soon as the record is observed and report it through the shields exit sentinel, so up, down, and read-only status all fail closed with one line plus the recovery hint. Signed-off-by: Tinson Lai <tinsonl@nvidia.com>
📝 WalkthroughWalkthroughThe Shields transition-lock flow now rejects stale malformed lock records immediately. It exposes structured recovery details, prints sanitized deferred-command errors, preserves the lock file, and adds synchronous, asynchronous, and CLI coverage. ChangesShields transition-lock handling
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 docstrings
🧪 Generate unit tests (beta)
Comment |
Code Coverage OverviewLanguages: TypeScript TypeScript / code-coverage/pluginThe overall coverage in commit a986513 in the TypeScript / code-coverage/cliThe overall coverage in commit a986513 in the Show a code coverage summary of the most impacted files.
Updated |
laitingsheng
added
NV QA
![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/cli/shields-transition-lock.test.ts`:
- Around line 31-69: Add a corrupt-transition-lock test alongside the existing
cases in the “shields commands with a corrupt transition lock” suite, invoking
the public `alpha shields up` command and verifying exit code 1, clean refusal
output via `expectCleanRefusal`, completion within `REFUSAL_BUDGET_MS`, and
unchanged malformed lock contents. Keep the setup consistent with the existing
`shields status` and `shields down` tests.
🪄 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: 23c0e00a-fe78-4cc1-b32c-80c3727c2f6c
📒 Files selected for processing (4)
src/lib/shields/index.tssrc/lib/shields/transition-lock.test.tssrc/lib/shields/transition-lock.tstest/cli/shields-transition-lock.test.ts
| describe("shields commands with a corrupt transition lock", () => { | ||
| it( | ||
| "refuses read-only status without a raw stack trace (#8108)", | ||
| testTimeoutOptions(30_000), | ||
| ({ testHome }) => { | ||
| const { home } = testHome; | ||
| writeSandboxRegistry(home); | ||
| const lockPath = writeCorruptTransitionLock(home, "alpha"); | ||
|
|
||
| const startedAt = Date.now(); | ||
| const status = runWithEnv("alpha shields status 2>&1", testHome.environment()); | ||
| const elapsedMs = Date.now() - startedAt; | ||
|
|
||
| expect(status.code).toBe(1); | ||
| expectCleanRefusal(status.out); | ||
| expect(elapsedMs).toBeLessThan(REFUSAL_BUDGET_MS); | ||
| expect(fs.readFileSync(lockPath, "utf8")).toBe(""); | ||
| }, | ||
| ); | ||
|
|
||
| it( | ||
| "refuses shields down without a raw stack trace (#8108)", | ||
| testTimeoutOptions(30_000), | ||
| ({ testHome }) => { | ||
| const { home } = testHome; | ||
| writeSandboxRegistry(home); | ||
| const lockPath = writeCorruptTransitionLock(home, "alpha"); | ||
|
|
||
| const startedAt = Date.now(); | ||
| const down = runWithEnv("alpha shields down --reason test 2>&1", testHome.environment()); | ||
| const elapsedMs = Date.now() - startedAt; | ||
|
|
||
| expect(down.code).toBe(1); | ||
| expectCleanRefusal(down.out); | ||
| expect(elapsedMs).toBeLessThan(REFUSAL_BUDGET_MS); | ||
| expect(fs.readFileSync(lockPath, "utf8")).toBe(""); | ||
| }, | ||
| ); | ||
| }); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Add corrupt-lock coverage for shields up.
This suite covers shields status and shields down only. Add a shields up case that verifies exit code 1, clean recovery output, prompt refusal, and preservation of the malformed lock file.
As per path instructions, tests must prove that public entrypoints reach the new path. The PR objective includes shields up, shields down, and shields status.
🧰 Tools
🪛 ast-grep (0.45.0)
[warning] 46-46: Filesystem path is not a string literal; a request-/variable-derived path can enable path traversal. Validate and normalize the path before use.
Context: fs.readFileSync(lockPath, "utf8")
Note: [CWE-22] Improper Limitation of a Pathname to a Restricted Directory ('Path Traversal').
(detect-non-literal-fs-filename-typescript)
[warning] 65-65: Filesystem path is not a string literal; a request-/variable-derived path can enable path traversal. Validate and normalize the path before use.
Context: fs.readFileSync(lockPath, "utf8")
Note: [CWE-22] Improper Limitation of a Pathname to a Restricted Directory ('Path Traversal').
(detect-non-literal-fs-filename-typescript)
🤖 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/cli/shields-transition-lock.test.ts` around lines 31 - 69, Add a
corrupt-transition-lock test alongside the existing cases in the “shields
commands with a corrupt transition lock” suite, invoking the public `alpha
shields up` command and verifying exit code 1, clean refusal output via
`expectCleanRefusal`, completion within `REFUSAL_BUDGET_MS`, and unchanged
malformed lock contents. Keep the setup consistent with the existing `shields
status` and `shields down` tests.
Source: Path instructions
PR Review Advisor — No blocking findings reportedAdvisor assessment: No blocking advisor findings reported Model lanes
4 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. They do not change the primary assessment or E2E / PR Gate. 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: Carlos Villela <cvillela@nvidia.com>
apurvvkumaria
left a comment
apurvvkumaria
left a comment
There was a problem hiding this comment.
Reviewed current head d1729d9. Stale malformed owner records fail fast while recent partial writes still wait; the corrupt lock remains untouched, and the CLI path exits cleanly after lock cleanup. Sync, async, and public CLI paths are covered, with required CI and E2E passing. I found no blocking defect.
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>
apurvvkumaria
left a comment
apurvvkumaria
left a comment
There was a problem hiding this comment.
Approve — reviewed exact head f070bbe. Old malformed transition locks now fail closed immediately through the typed CLI sentinel, preserve the lock, and render actionable recovery without a raw stack. The current head includes public-entrypoint coverage for status, up, and down, satisfying the still-open automated test thread. Unit coverage passes 47 of 47; the exact CI CLI failure is an unrelated managed-image registry fetch timeout. No blocking correctness, security, compatibility, or regression defect found.
4 participants
Summary
A shields transition lock whose owner record cannot be parsed is refused on purpose, but the refusal was reached only after the full 30-second wait timeout and then surfaced as a raw Node stack trace. Shields commands now refuse as soon as the unreadable record is observed, and report it through the existing shields exit sentinel, so
shields up,shields down, and the read-onlyshields statusall fail closed with a single line plus the recovery hint.Related Issue
Fixes #8108
Changes
src/lib/shields/transition-lock.ts: split the wait-reason text into a reason and a recovery hint so a caller can present them on separate lines. Every existing message is byte-identical; the error'smessageis still the reason followed by the hint.src/lib/shields/transition-lock.ts: addedShieldsTransitionLockUnavailableErrorfor a lock that cannot be taken, with a name-keyed guard. The guard matches on shape rather thaninstanceofbecausedistandsrccopies of the class can both be loaded, the same reasonisDeferredShieldsExitalready works that way.src/lib/shields/transition-lock.ts: an owner record that is unreadable and already past the malformed-stale threshold is refused immediately instead of polling until the wait timeout. That state is terminal: it cannot become recoverable by waiting, and no automatic recovery applies to it. A record younger than the threshold still waits, because a writer can finish it.src/lib/shields/index.ts:completeDeferredShieldsExitprints the reason and aRecovery:line, then routes throughfailShieldsCommand. BoththrowOnErrorsettings reach the same sentinel, soshields upandshields down(which passthrowOnError: true) andshields status(which does not) all exit 1 without a stack trace.src/lib/shields/transition-lock.test.tscovers the immediate refusal on the synchronous and asynchronous acquisition paths and the reason/hint split;test/cli/shields-transition-lock.test.tsruns publicshields status,shields up, andshields downcommands against a truncated lock file and asserts the exit code, both output lines, the absence of stack frames, preservation of the lock, and completion well inside the old timeout.Type of Change
Quality Gates
docs/security/tcb-boundary.mdxalready states that the transition lock rejects ambiguous owners, anddocs/manage-sandboxes/runtime-controls.mdxanddocs/manage-sandboxes/backup-restore.mdxdescribe only which operations take the lock. No page documents the previous wait duration or the stack trace.a986513e25472c8654c91c8f54d0c847522a3493against49ffca04d0ac3ec3a1715e2fa3455c76db15d803with no findings.Documentation Writer Review
no-docs-needed421952aedc74a60b88238f352644f9f1eeaceacbis identical to the independently reviewed tree. Existingdocs/security/tcb-boundary.mdx,docs/reference/host-files-and-state.mdx, anddocs/changelog/2026-07-15.mdxalready document fail-closed handling and manual recovery for malformed or ambiguous transition-lock owners. This change corrects refusal timing and error presentation without changing command grammar, configuration, supported procedures, or lifecycle behavior. Changed messages and test titles follow the writing rules. GitHub CI provides validation for this commit.DGX Station Hardware Evidence
scripts/prepare-dgx-station-host.sh.Verification
Signed-off-by:line and every commit appears asVerifiedin GitHub.pre-commit,commit-msg, andpre-pushhooks passed, ornpm run validate:prpassed after refreshingorigin/mainwhen hooks were skipped or unavailable — the normal pre-push checks passed for the identical reviewed tree, and GitHub commit lint passed for the contributor's current merge commit.checksgate passed for the current commit.npm run docsbuilds without warnings (doc changes only) — Not applicable; no documentation file changed.GitHub CI is authoritative.
No duplicate local test, documentation, build, or typecheck suite ran after the merge refresh.
Security Review
PASSa986513e25472c8654c91c8f54d0c847522a349349ffca04d0ac3ec3a1715e2fa3455c76db15d803PASSPASSPASSPASSPASSPASSPASSPASSPASSshields status,shields up, andshields down, asserting exit code 1, prompt refusal, bounded recovery output without a raw stack, and an unchanged malformed lock. Synchronous and asynchronous manager paths assert immediate refusal without sleeping.Signed-off-by: Tinson Lai tinsonl@nvidia.com
Summary by CodeRabbit
shields status,shields up, andshields downnow exit cleanly without stack traces when a malformed lock is detected.