Skip to content

Commit cb018f0

Browse files
jyaunchesjyaunches
authored
fix(docs): mount reviewer inputs before startup (#9365)
<!-- markdownlint-disable MD041 --> ## Summary The post-merge documentation reviewer now mounts its prepared checkout and model configuration read-only when OpenShell creates the sandbox. The hard Landlock policy now finds both required paths before it starts the first container process. ## Changes - Enable bind mounts on the workflow's loopback-only OpenShell gateway. - Mount the reviewer checkout and configuration read-only at container creation, with no reviewer uploads. - Use explicit Git metadata and worktree paths so the sandbox user can read the runner-owned checkout. - Keep the author sandbox on its existing three-upload path with no driver configuration. - Test the gateway capability, exact reviewer mounts, author path, cross-UID Git arguments, and config file modes. The escaped defect came from the runner fake copying inputs without checking the real OpenShell startup transport. Public documentation is unchanged because this repairs the documented workflow instead of changing its contract. ## Type of Change - [x] 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 - [x] Tests added or updated for changed behavior - [ ] Existing tests cover changed behavior — justification: - [ ] Tests not applicable — justification: - [x] Sensitive paths changed (security, policy, credentials, preflight, onboarding, inference, runner, sandbox, or messaging) - [x] Sensitive-path review completed or maintainer-approved waiver recorded — reviewer/approval link/justification: Independent Codex security review of `1545062c0` found no blocker. The production rerun remains the Docker and Landlock enforcement proof. - [ ] Non-success, skipped, or missing CI check accepted by maintainer — check name, approval link, and follow-up issue: ## DGX Station Hardware Evidence - [ ] Tested on DGX Station - Tested commit: - Station profile/scenario: - Result: - Supporting evidence: ## Verification - [x] 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 - [x] Targeted behavior tests pass for the current change set, or tests are marked not applicable above — `npm exec -- vitest run --project integration test/post-merge-docs.test.ts` passed 27 tests; targeted strict TypeScript, Oxlint, Oxfmt, source-shape, and diff checks passed. - [ ] Applicable broad gate passed — `npm test` for broad runtime/test-harness changes; `npm run check` for repo-wide validation/coverage changes — command/result: - [x] Quality Gates section completed with required justifications or waivers - [x] No secrets, API keys, or credentials committed - [ ] `npm run docs` builds without warnings (doc changes only) - [ ] Doc pages follow the [style guide](https://github.com/NVIDIA/NemoClaw/blob/main/docs/CONTRIBUTING.md) (doc changes only) - [ ] New doc pages include SPDX header and frontmatter (new pages only) The pre-commit and commit-message hooks passed. The pre-push `tsc-cli` hook was skipped after current `main` at `183a9c876` reproduced unrelated errors in `src/lib/onboard/machine/handlers/sandbox-messaging.ts`, its test, and `src/lib/state/portable-uninstall-retirement.test.ts`. All other applicable pre-push hooks passed. --- Signed-off-by: Julie Yaunches <jyaunches@nvidia.com> <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit - **New Features** - Documentation review workflows now access repository and configuration files through read-only mounts. - Review runs use explicit repository settings for more reliable Git operations. - Configuration files receive appropriate permissions during reviews. - Authoring workflows continue to use uploaded content without unnecessary Git configuration. - **Bug Fixes** - Improved isolation by preventing Git environment settings from leaking into authoring runs. - Added support for bind mounts in OpenShell-based documentation workflows. <!-- end of auto-generated comment: release notes by coderabbit.ai --> Signed-off-by: Julie Yaunches <jyaunches@nvidia.com>
1 parent 183a9c8 commit cb018f0

2 files changed

Lines changed: 135 additions & 22 deletions

File tree

‎test/post-merge-docs.test.ts‎

Lines changed: 66 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -11,7 +11,7 @@ import YAML from "yaml";
1111

1212
import { validatePostMergeDocsWorkflowBoundary } from "../tools/post-merge-docs/contract.mts";
1313
import { publishDocumentation, type Request } from "../tools/post-merge-docs/publish.mts";
14-
import { executePostMergeDocs } from "../tools/post-merge-docs/run.mts";
14+
import { configurePostMergeDocs, executePostMergeDocs } from "../tools/post-merge-docs/run.mts";
1515
import type { OpenShellTools } from "../tools/openshell-agent/runtime.mts";
1616

1717
const directories: string[] = [];
@@ -182,6 +182,7 @@ function runnerFixture(phase: "author" | "review") {
182182
GITHUB_REPOSITORY: repository,
183183
GITHUB_SHA: mainSha,
184184
HOME: root,
185+
OPENSHELL_GATEWAY_ENDPOINT: "http://127.0.0.1:8080",
185186
PI_IMAGE: "image",
186187
POST_MERGE_DOCS_ARTIFACT_DIR: path.join(root, "artifact"),
187188
POST_MERGE_DOCS_CANDIDATE_DIR: candidate,
@@ -190,6 +191,7 @@ function runnerFixture(phase: "author" | "review") {
190191
POST_MERGE_DOCS_WORKDIR: path.join(root, "work"),
191192
RANGE_START_SHA: mainSha,
192193
RANGE_START_TAG: "v1.0.0",
194+
RUNNER_TEMP: path.join(root, "runner-temp"),
193195
SANDBOX_NAME: `docs-${phase}`,
194196
TRUSTED_CHECKOUT: source,
195197
},
@@ -203,13 +205,19 @@ function runnerTools(
203205
const { env, root } = input;
204206
const sandbox = path.join(root, "sandbox");
205207
const output = path.join(root, "work/output");
206-
const state = { deleted: false };
208+
const state = {
209+
agentArgs: [] as readonly string[],
210+
createArgs: [] as readonly string[],
211+
deleted: false,
212+
};
207213
const handlers: Record<string, (args: readonly string[]) => unknown> = {
208-
create: () => {
214+
create: (args) => {
215+
state.createArgs = args;
209216
fs.cpSync(path.join(root, "work/repo"), sandbox, { recursive: true });
210217
expect(git(sandbox, ["rev-parse", "HEAD"])).toBe(env.GITHUB_SHA);
211218
},
212-
agent: () => {
219+
agent: (args) => {
220+
state.agentArgs = args;
213221
const agents = {
214222
author: () => fs.writeFileSync(path.join(sandbox, "docs/guide.mdx"), "authored\n"),
215223
review: () =>
@@ -329,18 +337,71 @@ describe("post-merge documentation publisher", () => {
329337
});
330338

331339
describe("post-merge documentation runner", () => {
340+
it("enables bind mounts before creating a reviewer sandbox", async () => {
341+
const input = runnerFixture("review");
342+
const responses = new Map([["which", "/trusted/bin/openshell-sandbox"]]);
343+
const tools: OpenShellTools = {
344+
run: vi.fn((command) => responses.get(command) ?? ""),
345+
start: vi.fn(),
346+
wait: async () => undefined,
347+
};
348+
await configurePostMergeDocs(input.env, tools);
349+
const config = fs.readFileSync(
350+
path.join(input.root, "runner-temp/openshell-gateway/gateway.toml"),
351+
"utf8",
352+
);
353+
expect(config).toContain("enable_bind_mounts = true");
354+
});
355+
332356
it("authors from the triggering SHA without exposing host credentials", () => {
333357
const input = runnerFixture("author");
334358
const { state, tools } = runnerTools(input);
335359
executePostMergeDocs(input.env, tools);
336360
expect(fs.readFileSync(path.join(input.root, "artifact/docs.patch"), "utf8")).toContain(
337361
"+authored",
338362
);
363+
expect(state.createArgs.filter((argument) => argument === "--upload")).toHaveLength(3);
364+
expect(state.createArgs).not.toContain("--driver-config-json");
365+
expect(state.agentArgs.join("\n")).not.toContain("GIT_DIR=");
339366
expect(state.deleted).toBe(true);
340367
});
341368
it("records the exact independent approval", () => {
342369
const input = runnerFixture("review");
343-
executePostMergeDocs(input.env, runnerTools(input).tools);
370+
const { state, tools } = runnerTools(input);
371+
executePostMergeDocs(input.env, tools);
372+
const driverConfigIndex = state.createArgs.indexOf("--driver-config-json");
373+
expect(JSON.parse(state.createArgs[driverConfigIndex + 1] as string)).toEqual({
374+
docker: {
375+
mounts: [
376+
{
377+
read_only: true,
378+
source: path.join(input.root, "work/repo"),
379+
target: "/sandbox/repo",
380+
type: "bind",
381+
},
382+
{
383+
read_only: true,
384+
source: path.join(input.root, "config"),
385+
target: "/sandbox/config",
386+
type: "bind",
387+
},
388+
],
389+
},
390+
});
391+
expect(state.createArgs).not.toContain("--upload");
392+
expect(state.createArgs.slice(-6)).toEqual([
393+
"--",
394+
"/usr/bin/git",
395+
"--git-dir=/sandbox/repo/.git",
396+
"--work-tree=/sandbox/repo",
397+
"status",
398+
"--short",
399+
]);
400+
expect(state.agentArgs).toEqual(
401+
expect.arrayContaining(["GIT_DIR=/sandbox/repo/.git", "GIT_WORK_TREE=/sandbox/repo"]),
402+
);
403+
expect(fs.statSync(path.join(input.root, "config")).mode & 0o777).toBe(0o755);
404+
expect(fs.statSync(path.join(input.root, "config/task.txt")).mode & 0o777).toBe(0o444);
344405
expect(
345406
JSON.parse(fs.readFileSync(path.join(input.root, "artifact/review.json"), "utf8")),
346407
).toEqual({

‎tools/post-merge-docs/run.mts‎

Lines changed: 69 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -164,8 +164,15 @@ function prepare(env: NodeJS.ProcessEnv): void {
164164
const config = required(env.POST_MERGE_DOCS_CONFIG_DIR, "POST_MERGE_DOCS_CONFIG_DIR");
165165
fs.mkdirSync(output, { mode: 0o700 });
166166
reset(config);
167-
write(path.join(config, "models.json"), resolverModelConfiguration());
168-
write(path.join(config, "task.txt"), `${prompt(env, current)}\n`);
167+
const models = path.join(config, "models.json");
168+
const task = path.join(config, "task.txt");
169+
write(models, resolverModelConfiguration());
170+
write(task, `${prompt(env, current)}\n`);
171+
if (current === "review") {
172+
fs.chmodSync(config, 0o755);
173+
fs.chmodSync(models, 0o444);
174+
fs.chmodSync(task, 0o444);
175+
}
169176
}
170177

171178
function agentCommand(current: Phase): string[] {
@@ -188,36 +195,69 @@ function agentCommand(current: Phase): string[] {
188195
function create(env: NodeJS.ProcessEnv, tools: OpenShellTools): void {
189196
const current = phase(env);
190197
const work = required(env.POST_MERGE_DOCS_WORKDIR, "POST_MERGE_DOCS_WORKDIR");
198+
const config = required(env.POST_MERGE_DOCS_CONFIG_DIR, "POST_MERGE_DOCS_CONFIG_DIR");
199+
const review = current === "review";
191200
const policy =
192201
current === "author"
193202
? "pr-merge-conflict-fixer/policy.yaml"
194203
: "post-merge-docs/review-policy.yaml";
195204
createOpenShellSandbox(
196205
env,
197206
{
198-
command: ["/usr/bin/git", "-C", "/sandbox/repo", "status", "--short"],
207+
command: review
208+
? [
209+
"/usr/bin/git",
210+
"--git-dir=/sandbox/repo/.git",
211+
"--work-tree=/sandbox/repo",
212+
"status",
213+
"--short",
214+
]
215+
: ["/usr/bin/git", "-C", "/sandbox/repo", "status", "--short"],
199216
image: required(env.PI_IMAGE, "PI_IMAGE"),
200217
name: required(env.SANDBOX_NAME, "SANDBOX_NAME"),
201218
policyPath: path.join(required(env.TRUSTED_CHECKOUT, "TRUSTED_CHECKOUT"), "tools", policy),
202-
uploads: [
203-
{ destination: "/sandbox", source: path.join(work, "repo") },
204-
{
205-
destination: "/sandbox",
206-
source: required(env.POST_MERGE_DOCS_CONFIG_DIR, "POST_MERGE_DOCS_CONFIG_DIR"),
207-
},
208-
{ destination: "/sandbox", source: path.join(work, "output") },
209-
],
219+
driverConfig: review
220+
? {
221+
docker: {
222+
mounts: [
223+
{
224+
read_only: true,
225+
source: path.join(work, "repo"),
226+
target: "/sandbox/repo",
227+
type: "bind",
228+
},
229+
{
230+
read_only: true,
231+
source: config,
232+
target: "/sandbox/config",
233+
type: "bind",
234+
},
235+
],
236+
},
237+
}
238+
: undefined,
239+
uploads: review
240+
? []
241+
: [
242+
{ destination: "/sandbox", source: path.join(work, "repo") },
243+
{ destination: "/sandbox", source: config },
244+
{ destination: "/sandbox", source: path.join(work, "output") },
245+
],
210246
},
211247
tools,
212248
);
213249
}
214250

215251
function run(env: NodeJS.ProcessEnv, tools: OpenShellTools): void {
252+
const current = phase(env);
216253
execOpenShellSandbox(
217254
env,
218255
{
219-
command: agentCommand(phase(env)),
256+
command: agentCommand(current),
220257
environment: {
258+
...(current === "review"
259+
? { GIT_DIR: "/sandbox/repo/.git", GIT_WORK_TREE: "/sandbox/repo" }
260+
: {}),
221261
HOME: "/sandbox/output",
222262
PI_CODING_AGENT_DIR: "/sandbox/config",
223263
PI_OFFLINE: "1",
@@ -311,14 +351,26 @@ export function executePostMergeDocs(
311351
}
312352
}
313353

354+
export function configurePostMergeDocs(
355+
env: NodeJS.ProcessEnv,
356+
tools: OpenShellTools = defaultOpenShellTools,
357+
): Promise<void> {
358+
return configureOpenShellInference(
359+
env,
360+
{
361+
enableBindMounts: true,
362+
gatewayId: "post-merge-docs",
363+
modelId: RESOLVER_MODEL_ID,
364+
providerName: "docs",
365+
},
366+
tools,
367+
);
368+
}
369+
314370
async function main(): Promise<void> {
315371
switch (required(process.argv[2], "command")) {
316372
case "configure":
317-
await configureOpenShellInference(process.env, {
318-
gatewayId: "post-merge-docs",
319-
modelId: RESOLVER_MODEL_ID,
320-
providerName: "docs",
321-
});
373+
await configurePostMergeDocs(process.env);
322374
return;
323375
case "execute":
324376
executePostMergeDocs(process.env);

0 commit comments

Comments
 (0)