fix(shields): anchor lock inspection to descriptor

[deepujain]

Summary

Anchor shields transition-lock inspection to a single no-follow file descriptor. This removes the pathname check-before-open shape tracked by CodeQL while preserving fail-closed handling when the canonical lock path is replaced.

Related Issue

Refs #6263

Changes

• Open the lock with O_NOFOLLOW before making file-type or ownership decisions.
• Validate the opened descriptor, then confirm the canonical path still names the same inode before parsing the owner record.
• Move the replacement-race regression test to the post-open boundary.

Type of Change

• Code change (feature, bug fix, or refactor)
• Code change with doc updates
• Doc only (prose changes, no code sample modifications)
• Doc only (includes code sample changes)

Quality Gates

• Tests added or updated for changed behavior
• Existing tests cover changed behavior — justification:
• Tests not applicable — justification:
• Docs updated for user-facing behavior changes
• Docs not applicable — justification: Internal lock inspection ordering changed without altering the CLI or operator workflow.
• Sensitive paths changed (security, policy, credentials, preflight, onboarding, inference, runner, sandbox, or messaging)
• Sensitive-path review completed or maintainer-approved waiver recorded — reviewer/approval link/justification: Reviewed descriptor lifetime, symlink handling, inode replacement, stale-owner takeover, and close paths against the focused transition-lock suite.
• Non-success, skipped, or missing CI check accepted by maintainer — check name, approval link, and follow-up issue:

Documentation Writer Review

• Documentation writer subagent reviewed the completed changes
• Result: no-docs-needed
• Evidence: src/lib/shields/transition-lock.ts and src/lib/shields/transition-lock.test.ts; the refreshed PR diff is byte-identical to the reviewed security fix and has no user-visible behavior, CLI, or documentation impact.
• Agent: Codex Desktop

DGX Station Hardware Evidence

• Tested on DGX Station
• Tested commit:
• Station profile/scenario:
• Result:
• Supporting evidence:

Verification

• PR description includes a Signed-off-by: line and every commit appears as Verified in GitHub
• Normal pre-commit, commit-msg, and pre-push hooks passed, or npm run validate:pr passed after refreshing origin/main when hooks were skipped or unavailable — maintainer refresh evidence: npm run validate:pr passed at 414f9739b.
• Targeted behavior tests pass for the current change set, or tests are marked not applicable above — command/result or justification: npm exec -- vitest run --project cli src/lib/shields/transition-lock.test.ts (47 passed)
• Applicable broad gate passed — npm test for broad runtime/test-harness changes; npm run check for repo-wide validation/coverage changes — command/result: npm run build:cli, npm run typecheck:cli, npm run lint, and npm run check:diff
• Quality Gates section completed with required justifications or waivers
• No secrets, API keys, or credentials committed
• npm run docs builds without warnings (doc changes only)
• Doc pages follow the style guide (doc changes only)
• New doc pages include SPDX header and frontmatter (new pages only)

---

Signed-off-by: Deepak Jain deepujain@gmail.com

Summary by CodeRabbit

• Bug Fixes
  • Improved transition-lock handling when the lock file is replaced or modified during inspection.
  • Prevented stale or mismatched lock information from being reported during rapid file changes.
  • Preserved safe handling for missing, symbolic-link, or invalid lock files.
  • Added coverage for lock identity changes occurring during inspection.

[copy-pr-bot]

This pull request requires additional validation before any workflows can run on NVIDIA's runners.

Pull request vetters can view their responsibilities here.

Contributors can view more details about this message here.

[coderabbitai]

Note

Reviews paused

It 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 reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

• @coderabbitai resume to resume automatic reviews.
• @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

• ▶️ Resume reviews
• 🔍 Trigger review

📝 Walkthrough

Walkthrough

readExistingLock now opens the lock descriptor before path validation, handles post-open ENOENT, and closes the descriptor. The regression test simulates canonical identity replacement during fstatSync.

Changes

Transition lock identity handling

Layer / File(s)	Summary
Descriptor-anchored lock validation src/lib/shields/transition-lock.ts	readExistingLock opens the lock with O_NOFOLLOW before lstatSync, preserves file checks, and handles post-open ENOENT by closing the descriptor and returning null.
Post-open identity swap regression test src/lib/shields/transition-lock.test.ts	The test replaces the canonical lock during fstatSync, restores the spy in finally, and verifies the return value and file contents.

Estimated code review effort: 3 (Moderate) | ~20 minutes

Suggested labels: area: sandbox

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly summarizes the main change: anchoring lock inspection to a file descriptor.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests

---

_{Comment @coderabbitai help to get the list of available commands.}

[coderabbitai]

🧹 Nitpick comments (1)

src/lib/shields/transition-lock.test.ts (1)

300-310: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Ensure the fstatSync spy is explicitly restored.

The changed code installs a global filesystem spy. If the omitted test tail does not restore it in a finally block, a failed assertion can leak the mock into later tests. As per coding guidelines, deterministic tests must restore spies explicitly.

🤖 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 `@src/lib/shields/transition-lock.test.ts` around lines 300 - 310, Explicitly
restore the fs.fstatSync spy created in the test, using a finally block that
runs even when assertions or filesystem operations fail. Keep the existing swap
behavior unchanged and ensure cleanup occurs before the test exits.

Source: Coding guidelines

🤖 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.

Nitpick comments:
In `@src/lib/shields/transition-lock.test.ts`:
- Around line 300-310: Explicitly restore the fs.fstatSync spy created in the
test, using a finally block that runs even when assertions or filesystem
operations fail. Keep the existing swap behavior unchanged and ensure cleanup
occurs before the test exits.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Enterprise

Run ID: 26dcba79-0298-44f4-b4e0-487dbe8f5df5

📥 Commits

Reviewing files that changed from the base of the PR and between 4dcb89e and 7287a25.

📒 Files selected for processing (2)

• src/lib/shields/transition-lock.test.ts
• src/lib/shields/transition-lock.ts

[deepujain]

Added explicit try/finally restoration for the fstatSync spy in efd3c5a. The focused suite still passes 44/44.

[github-actions]

PR Review Advisor — Informational

Advisor assessment: Informational / low confidence
Next action: No advisor follow-up needed.
Findings: 0 blockers · 0 warnings · 0 suggestions
Status: Same-session synthesis validation failed; the advisor result is incomplete.

Model lanes

• GPT-5.6 Terra (primary): Completed · low confidence · 0 blockers · 0 warnings · 0 suggestions
• Nemotron 3 Ultra (second opinion): Completed · high confidence · 0 blockers · 0 warnings · 0 suggestions
• Model comparison: normalized findings match; normalized terminology decisions differ; normalized E2E selections differ; severity counts match.

1 terminology difference from the second opinion

Advisory only. These are normalized differences from the primary terminology receipt.

• anchored to one no-follow descriptor at src/lib/shields/transition-lock.ts:244: selected only by the second-opinion lane as justified.

3 additional E2E selections from the second opinion

Advisory only. The primary lane did not select these E2E jobs or targets.

• shields-config: The completed second-opinion lane identified E2E coverage that the primary lane omitted.
• sandbox-operations: The completed second-opinion lane identified E2E coverage that the primary lane omitted.
• sandbox-survival: The completed second-opinion lane identified E2E coverage that the primary lane omitted.

Second-opinion terminology and E2E selections are advisory. They do not change the primary assessment or E2E / PR Gate.

1 semantic terminology decision

Terminology decisions are advisory. They affect the assessment only when a separate finding identifies concrete semantic impact.

• established — no-follow descriptor at src/lib/shields/transition-lock.ts:244: Retain the established term.

E2E guidance

Advisory only. E2E / PR Gate selects and runs jobs independently.

Recommended E2E: cloud-inference, security-posture

Workflow run details

This automated review informs maintainers. Warnings and suggestions do not require a response. A maintainer decides whether to merge.

[deepujain]

Rebased onto current main. Focused tests and npm run validate:pr pass; all commits remain verified.

[deepujain]

Rebased onto current main. The focused transition-lock suite and PR validation pass.

[wscurran]

✨ Thanks for the PR. This fixes a TOCTOU race in lock inspection by anchoring checks to an O_NOFOLLOW file descriptor and validating the opened descriptor. Maintainers will review the descriptor validation logic and the regression test for the replacement-race boundary.

---

Related open issues:

• #6263 chore(security): track remaining CodeQL findings

---

Related open issues:

• #6263 chore(security): track remaining CodeQL findings

[apurvvkumaria]

Reviewed exact head bd598b5. Opening with O_NOFOLLOW before path inspection anchors validation to one descriptor, and the subsequent lstat plus inode comparison safely rejects a replaced canonical path while all exceptional paths close the descriptor. The focused race test covers the post-open swap and restores its filesystem spy. I found no blocking correctness, security, compatibility, or regression issue.

[deepujain]

Rebased on current main. The 47 transition-lock tests and full PR validation pass, and both commits are verified. @coderabbitai review

[coderabbitai]

@deepujain I will review the rebased changes and the transition-lock validation logic.

✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

[deepujain]

Synced with latest main. All 47 transition-lock tests and PR validation pass; CI is rerunning.

[deepujain]

Synced with current main while preserving the approved security changes and maintainer merges. All commits are verified; CI is rerunning.

[deepujain]

Synced with today's main while preserving the approved security history. All commits remain verified; advisors and E2E are still running.

[coderabbitai]

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.

[deepujain]

Synced with current main while preserving the reviewed security history. All commits remain verified, approval and CodeRabbit are intact, and fresh advisor and E2E checks are running.