NVIDIA / NVIDIA/NemoClaw

refactor(fs): centralize private-file transaction primitives

Open
#9,668 1 comment 0 reactions 1 assignee Claimed by @rsliter View on GitHub
area: onboarding area: security needs: unblock
Dominant language
TypeScript
Stars
22.5k
Forks
3.1k
Avg merge
1d 1h
Merged PRs (30d)
715

Description

**THIS IS NOT READY - it cannot be picked up until the work in #9323 is complete and then should only be picked up by the assignee.**

## Problem

Four onboarding persistence stores repeat security-sensitive filesystem code:

- `src/lib/onboard/managed-bootstrap/docker-journal.ts`
- `src/lib/onboard/managed-bootstrap/podman-bootstrap-journal.ts`
- `src/lib/onboard/runtime-provider/persisted-engine-lifecycle.ts`
- `src/lib/onboard/runtime-provider/host-local-create-journal.ts`

The Docker and Podman journals contain the same stable-read and directory-fsync implementation. The runtime-provider stores repeat the same core with current-user ownership and interrupted-publication recovery requirements.

These copies independently enforce `O_NOFOLLOW`, bounded reads, link and mode checks, metadata stability, temporary-file publication, file fsync, directory fsync, and cleanup. A change to one copy can leave another store with a weaker contract.

`src/lib/adapters/fs/regular-file.ts` is the existing shared filesystem boundary, but its current contract does not own durable publication or the complete stable-read checks required by these stores.

## Scope

- Add one focused private-file transaction owner under `src/lib/adapters/fs/`.
- Migrate the four named stores to that owner.
- Preserve each store's existing size limit, current-user ownership requirement, exclusive or replacement publication behavior, recovery behavior, and domain error.
- Keep schemas, serialization, paths, transition rules, and recovery decisions in their current domain modules.
- Add contract tests at the shared filesystem boundary. Keep consumer tests for domain behavior.

## Security invariants

- Fail closed when `O_NOFOLLOW` is unavailable.
- Return the existing missing-file result only for `ENOENT`.
- Reject symbolic links, non-regular files, invalid link counts, shared permissions, and wrong ownership where the current consumer checks ownership.
- Reject empty, oversized, short, overflowing, or metadata-changing reads.
- Fsync file contents before publication and fsync the parent directory after visibility or cleanup changes.
- Exclusive publication must not replace an existing target.
- Preserve the primary operation failure when descriptor or temporary-file cleanup also fails.
- Do not leak descriptors or temporary files after a failed operation.

## Delivery slices

1. Move the exact Docker and Podman journal primitives to the shared filesystem boundary.
2. Migrate the runtime-provider stores while preserving their current-user ownership and interrupted-publication recovery contracts.

## Acceptance criteria

- [ ] One shared implementation owns stable metadata comparison, bounded stable reads, directory fsync, and temporary-file publication for the four consumers.
- [ ] Existing canonical private files load without changing serialized bytes or lifecycle behavior.
- [ ] Missing files retain each store's current result.
- [ ] Symlink, permission, link-count, ownership, size, short-read, overflow, and metadata-race cases fail closed.
- [ ] Exclusive and replacement publication retain their current conflict and recovery behavior.
- [ ] File and directory fsync ordering has deterministic test evidence.
- [ ] Operation failures remain authoritative when cleanup also fails.
- [ ] The four focused consumer suites and the shared filesystem contract tests pass.
- [ ] CLI type-checking and repository checks pass.
- [ ] No user-visible command, configuration, state schema, or documentation behavior changes.

## Related work and boundaries

- PR #9323 currently changes `persisted-engine-lifecycle.ts` outside these helpers. Re-read its latest changes before migrating that consumer.
- Similar secure-file code in credentials, inference receipts, SSH bindings, and experimental flows is out of scope. Adopt the shared boundary there only after a separate contract comparison.
- Do not merge the four stores or create a generic persistence framework.
- No live E2E test is required because deterministic source tests own this filesystem contract.

Contributor guide

Open the contributing guide

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.