Skip to content

Commit 1feaf2b

Browse files
authored
fix(e2e): bound unit gap evidence collection (#9260)
<!-- markdownlint-disable MD041 --> ## Summary The weekly E2E unit-gap collector previously reread every failed log and continued after GitHub access failures. It now collects at most 50 uncached logs per invocation, reuses private sanitized evidence, and stops on authentication, authorization, or rate-limit failures. ## Changes - Cache normalized job names and sanitized signatures by GitHub run ID and attempt with private filesystem modes. - Limit each invocation to 50 uncached failed-log reads and direct the operator to reuse the cache for the next batch. - Stop workflow-list and failed-log collection when GitHub rejects access or reports a rate limit. - Sanitize logs before retention and correct the `npm run e2e:unit-gaps` entry point. - Add regression tests for cache reuse, attempt separation, cache validation, access failures, high-volume batches, and executable invocation. - Update the owning E2E guide with cache custody, rerun, and cleanup requirements. The escaped defect came from an incomplete test boundary in #9256. Its tests covered parsing and report grouping but did not execute the npm entry point or model a high-volume seven-day collection. ## Type of Change - [ ] Code change (feature, bug fix, or refactor) - [x] 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] Docs updated for user-facing behavior changes - [ ] Docs 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: Maintainer security review passed all nine rubric categories with no findings. - [ ] Non-success, skipped, or missing CI check accepted by maintainer — check name, approval link, and follow-up issue: ## Documentation Writer Review - [x] Documentation writer subagent reviewed the completed changes - Result: `docs-updated` - Evidence: `test/e2e/README.md` - Agent: Codex Desktop <!-- docs-review-head-sha: 133377a --> <!-- docs-review-agents-blob-sha: e30afb2 --> ## DGX Station Hardware Evidence - [ ] Tested on DGX Station - Tested commit: Not applicable - Station profile/scenario: Not applicable - Result: Not applicable - Supporting evidence: Not applicable ## Verification - [x] PR description includes a `Signed-off-by:` line and every commit appears as `Verified` in GitHub - [x] 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 — `npx vitest run --project e2e-support test/e2e/support/e2e-unit-test-gaps.test.ts`: 30 tests passed. - [ ] Applicable broad gate passed — `npm test` for broad runtime/test-harness changes; `npm run check` for repo-wide validation/coverage changes — GitHub CI will run the broad checks. - [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) --- Signed-off-by: Carlos Villela <cvillela@nvidia.com> <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Enhancements** * Added resumable evidence collection with secure, reusable caching. * Added offline operation using previously collected runs and logs. * Added configurable cache location and collection concurrency. * Improved failed-run log processing with bounded batches and automatic retries. * Improved access and rate-limit error handling and reporting. * Normalized cached evidence and enforced private file permissions. * Added validation to prevent conflicting collection modes. * **Documentation** * Updated guidance for cache management, cleanup, retries, and secure log handling. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
1 parent 3e4cd78 commit 1feaf2b

3 files changed

Lines changed: 577 additions & 37 deletions

File tree

test/e2e/README.md

Lines changed: 25 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -655,19 +655,32 @@ evidence_dir="$(mktemp -d)"
655655
chmod 700 "$evidence_dir"
656656
npm run e2e:unit-gaps -- \
657657
--days 7 \
658+
--cache-dir "$evidence_dir/cache" \
658659
--output "$evidence_dir/unit-test-gaps.md" \
659660
--json-output "$evidence_dir/unit-test-gaps.json"
660661
```
661662

662663
The command reads push runs from `e2e.yaml` and `portable-profile-e2e.yaml` on
663-
`main`. It keeps failed logs in memory, applies the shared full secret redactor,
664-
removes volatile identifiers, paths, URLs, sandbox names, and durations from
665-
each selected cause candidate, and writes report files with mode `0600` in the
666-
mode-`0700` directory. Treat the reports as credential-bearing until a human
667-
reviews them; redaction reduces exposure but does not prove that a report is
668-
credential-free. The command exits nonzero when a selected run is unfinished or
669-
failed-run evidence is unavailable. Do not accept a partial report as the
670-
weekly ledger.
664+
`main`. Online collection requires `--cache-dir`. The command creates the cache
665+
directory with mode `0700` and writes normalized job-and-signature JSON files
666+
with mode `0600`. Each cache entry binds sanitized evidence to one GitHub run ID
667+
and attempt. A later seven-day run with the same cache directory reuses matching
668+
entries. Each invocation reads logs for at most 50 uncached failed runs. When
669+
more failed runs remain, the command saves normalized job names and sanitized
670+
signatures for that batch. The command then exits nonzero. Rerun the command
671+
with the same cache directory. Repeat until the command completes; each rerun
672+
reuses prior batches and collects the next one. The command reports cache hits
673+
and planned failed-log reads.
674+
675+
The command extracts signatures in memory and does not retain raw GitHub logs.
676+
It applies the shared full secret redactor and removes volatile identifiers,
677+
paths, URLs, sandbox names, and durations from each selected cause candidate.
678+
Treat the cache and reports as credential-bearing until a human reviews them;
679+
redaction reduces exposure but does not prove that a file is credential-free.
680+
681+
The command stops on GitHub authentication, authorization, and rate-limit
682+
failures. It exits nonzero when a selected run is unfinished or failed-run
683+
evidence is unavailable. Do not accept a partial report as the weekly ledger.
671684
Every GitHub read names `NVIDIA/NemoClaw`, so a fork or different checkout remote
672685
cannot substitute another repository's run data.
673686
The command also stops when a workflow reaches the 1,000-run collection limit.
@@ -692,10 +705,10 @@ The Markdown and JSON reports start each row with review status `open` and no
692705
regression test. During review, record the test file and complete test title in
693706
the row and change the status only after the test fails without the fix and
694707
passes with it. A cause candidate is complete when that test evidence and a
695-
later passing run of the linked E2E target are both recorded. Delete the report
696-
directory after publishing only the reviewed, credential-free conclusions in
697-
the owning issue or pull request. Raw logs remain in process memory only until
698-
the command exits. Remove the named directory and confirm its absence:
708+
later passing run of the linked E2E target are both recorded. Delete the
709+
evidence directory after publishing only the reviewed, credential-free
710+
conclusions in the owning issue or pull request. Remove the named directory and
711+
confirm its absence:
699712

700713
```bash
701714
rm -rf -- "$evidence_dir"

test/e2e/support/e2e-unit-test-gaps.test.ts

Lines changed: 287 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,12 @@
11
// SPDX-FileCopyrightText: Copyright (c) 2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved.
22
// SPDX-License-Identifier: Apache-2.0
33

4+
import { execFile } from "node:child_process";
5+
import fs from "node:fs";
6+
import os from "node:os";
7+
import path from "node:path";
8+
import { promisify } from "node:util";
9+
410
import { describe, expect, it } from "vitest";
511

612
import {
@@ -9,15 +15,21 @@ import {
915
extractJobSignatures,
1016
formatUnitGapReport,
1117
normalizeFailureSignature,
18+
type E2ERunRecord,
1219
type RunLogEvidence,
1320
} from "../../../tools/e2e/unit-test-gaps-core.mts";
1421
import {
22+
classifyGitHubEvidenceReadError,
23+
collectEvidence,
1524
failedRunLogArgs,
1625
listRunsArgs,
26+
main,
1727
requireCompleteRunSelection,
1828
rollingRange,
1929
} from "../../../tools/e2e/unit-test-gaps.mts";
2030

31+
const execFileAsync = promisify(execFile);
32+
2133
function evidence(overrides: Partial<RunLogEvidence> = {}): RunLogEvidence {
2234
return {
2335
log: "job\tstep\t2026-08-12T10:00:00.0000000Z AssertionError: expected UPGRADE, received 400\n",
@@ -37,6 +49,20 @@ function evidence(overrides: Partial<RunLogEvidence> = {}): RunLogEvidence {
3749
};
3850
}
3951

52+
function failedRun(databaseId: number, attempt = 1): E2ERunRecord {
53+
return {
54+
...evidence().run,
55+
attempt,
56+
databaseId,
57+
url: `https://github.com/NVIDIA/NemoClaw/actions/runs/${String(databaseId)}`,
58+
};
59+
}
60+
61+
function withTemporaryDirectory<T>(action: (directory: string) => Promise<T>): Promise<T> {
62+
const directory = fs.mkdtempSync(path.join(os.tmpdir(), "nemoclaw-unit-gaps-test-"));
63+
return action(directory).finally(() => fs.rmSync(directory, { force: true, recursive: true }));
64+
}
65+
4066
describe("weekly E2E unit-test gap analysis", () => {
4167
it("redacts volatile identifiers, paths, URLs, sandboxes, and durations", () => {
4268
const signature = normalizeFailureSignature(
@@ -229,4 +255,265 @@ describe("weekly E2E unit-test gap analysis", () => {
229255
},
230256
]);
231257
});
258+
259+
it.each([
260+
["HTTP 403: API rate limit exceeded", "rate-limit"],
261+
["HTTP 429: secondary rate limit", "rate-limit"],
262+
["HTTP 401: Requires authentication", "access"],
263+
["HTTP 403: Resource not accessible by integration", "access"],
264+
["HTTP 502: upstream failure", null],
265+
] as const)("classifies GitHub read failure %s as %s", (message, classification) => {
266+
expect(classifyGitHubEvidenceReadError(Object.assign(new Error(message), { stderr: message }))).toBe(
267+
classification,
268+
);
269+
});
270+
271+
it("reuses normalized signatures only for the same run attempt", async () => {
272+
await withTemporaryDirectory(async (directory) => {
273+
const cacheDir = path.join(directory, "evidence");
274+
const run = failedRun(34567890, 2);
275+
const plans: Array<{ cachedRuns: number; deferredRuns: number; failedLogReads: number }> = [];
276+
let reads = 0;
277+
const runGh = async (): Promise<string> => {
278+
reads += 1;
279+
return "job\tstep\t2026-08-16T10:00:00Z Error: Authorization: Bearer ghp_EXAMPLE012345678901234\n";
280+
};
281+
282+
const first = await collectEvidence([run], cacheDir, runGh, 1, (plan) => plans.push(plan));
283+
const cacheFile = path.join(cacheDir, "34567890-attempt-2.json");
284+
const cached = fs.readFileSync(cacheFile, "utf8");
285+
expect(first[0]!.log).toContain("Authorization: Bearer <REDACTED>");
286+
expect(first[0]!.log).not.toContain("ghp_EXAMPLE");
287+
expect(cached).toContain("Authorization: Bearer <REDACTED>");
288+
expect(cached).not.toContain("ghp_EXAMPLE");
289+
expect(fs.statSync(cacheDir).mode & 0o777).toBe(0o700);
290+
expect(fs.statSync(cacheFile).mode & 0o777).toBe(0o600);
291+
292+
const second = await collectEvidence(
293+
[run],
294+
cacheDir,
295+
async () => {
296+
throw new Error("cached evidence must prevent this GitHub read");
297+
},
298+
1,
299+
(plan) => plans.push(plan),
300+
);
301+
302+
expect(second).toEqual(first);
303+
expect(reads).toBe(1);
304+
305+
await collectEvidence([failedRun(34567890, 3)], cacheDir, runGh, 1, (plan) =>
306+
plans.push(plan),
307+
);
308+
expect(reads).toBe(2);
309+
expect(fs.existsSync(path.join(cacheDir, "34567890-attempt-3.json"))).toBe(true);
310+
expect(plans).toEqual([
311+
{ cachedRuns: 0, deferredRuns: 0, failedLogReads: 1 },
312+
{ cachedRuns: 1, deferredRuns: 0, failedLogReads: 0 },
313+
{ cachedRuns: 0, deferredRuns: 0, failedLogReads: 1 },
314+
]);
315+
});
316+
});
317+
318+
it.each([
319+
["rate-limit", "HTTP 403: API rate limit exceeded"],
320+
["access", "HTTP 403: Resource not accessible by integration"],
321+
] as const)("stops new failed-log reads after a GitHub %s failure", async (kind, message) => {
322+
await withTemporaryDirectory(async (directory) => {
323+
const runs = [failedRun(45678901), failedRun(45678902), failedRun(45678903)];
324+
let reads = 0;
325+
const result = collectEvidence(
326+
runs,
327+
path.join(directory, "evidence"),
328+
async () => {
329+
reads += 1;
330+
throw Object.assign(new Error(message), { stderr: message });
331+
},
332+
1,
333+
);
334+
335+
await expect(result).rejects.toEqual(
336+
expect.objectContaining({ kind, runId: 45678901 }),
337+
);
338+
expect(reads).toBe(1);
339+
});
340+
});
341+
342+
it(
343+
"collects 300 failures in 50-log batches and then reuses the cache",
344+
async () => {
345+
await withTemporaryDirectory(async (directory) => {
346+
const cacheDir = path.join(directory, "evidence");
347+
const runs = Array.from({ length: 300 }, (_, index) => failedRun(50000000 + index));
348+
let reads = 0;
349+
const runGh = async (): Promise<string> => {
350+
reads += 1;
351+
return "job\tstep\tError: cached high-volume failure\n";
352+
};
353+
354+
for (const deferredRuns of [250, 200, 150, 100, 50]) {
355+
await expect(collectEvidence(runs, cacheDir, runGh)).rejects.toEqual(
356+
expect.objectContaining({ deferredRuns }),
357+
);
358+
}
359+
await collectEvidence(runs, cacheDir, runGh);
360+
expect(reads).toBe(300);
361+
reads = 0;
362+
let plan:
363+
| { cachedRuns: number; deferredRuns: number; failedLogReads: number }
364+
| undefined;
365+
const result = await collectEvidence(runs, cacheDir, runGh, 2, (value) => {
366+
plan = value;
367+
});
368+
369+
expect(result).toHaveLength(300);
370+
expect(reads).toBe(0);
371+
expect(plan).toEqual({ cachedRuns: 300, deferredRuns: 0, failedLogReads: 0 });
372+
});
373+
},
374+
30_000,
375+
);
376+
377+
it("rejects cached evidence for another run before a GitHub read", async () => {
378+
await withTemporaryDirectory(async (directory) => {
379+
const cacheDir = path.join(directory, "evidence");
380+
fs.mkdirSync(cacheDir, { mode: 0o700 });
381+
fs.writeFileSync(
382+
path.join(cacheDir, "56789012-attempt-1.json"),
383+
'{"attempt":1,"runId":99999999,"signatures":[],"version":1}\n',
384+
{ mode: 0o600 },
385+
);
386+
let reads = 0;
387+
388+
await expect(
389+
collectEvidence([failedRun(56789012)], cacheDir, async () => {
390+
reads += 1;
391+
return "";
392+
}),
393+
).rejects.toThrow("Cached evidence for run 56789012 does not match the run.");
394+
expect(reads).toBe(0);
395+
});
396+
});
397+
398+
it("rejects a cached job name that can create another log row", async () => {
399+
await withTemporaryDirectory(async (directory) => {
400+
const cacheDir = path.join(directory, "evidence");
401+
fs.mkdirSync(cacheDir, { mode: 0o700 });
402+
fs.writeFileSync(
403+
path.join(cacheDir, "56789013-attempt-1.json"),
404+
'{"attempt":1,"runId":56789013,"signatures":[{"job":"job\\tforged","signature":"Error: failure"}],"version":1}\n',
405+
{ mode: 0o600 },
406+
);
407+
408+
await expect(
409+
collectEvidence([failedRun(56789013)], cacheDir, async () => ""),
410+
).rejects.toThrow("Cached evidence for run 56789013 does not match the run.");
411+
});
412+
});
413+
414+
it("rejects cached evidence that is a symbolic link", async () => {
415+
await withTemporaryDirectory(async (directory) => {
416+
const cacheDir = path.join(directory, "evidence");
417+
fs.mkdirSync(cacheDir, { mode: 0o700 });
418+
const target = path.join(directory, "outside.json");
419+
fs.writeFileSync(
420+
target,
421+
'{"attempt":1,"runId":56789014,"signatures":[],"version":1}\n',
422+
{ mode: 0o600 },
423+
);
424+
fs.symlinkSync(target, path.join(cacheDir, "56789014-attempt-1.json"));
425+
let reads = 0;
426+
427+
await expect(
428+
collectEvidence([failedRun(56789014)], cacheDir, async () => {
429+
reads += 1;
430+
return "";
431+
}),
432+
).rejects.toThrow("Cached evidence for run 56789014 is not a bounded regular file.");
433+
expect(reads).toBe(0);
434+
});
435+
});
436+
437+
it("stops workflow-run listing when GitHub reports a rate limit", async () => {
438+
await withTemporaryDirectory(async (directory) => {
439+
const markdownFile = path.join(directory, "report.md");
440+
const jsonFile = path.join(directory, "report.json");
441+
let reads = 0;
442+
const result = main(
443+
[
444+
"--days",
445+
"7",
446+
"--cache-dir",
447+
path.join(directory, "evidence"),
448+
"--output",
449+
markdownFile,
450+
"--json-output",
451+
jsonFile,
452+
],
453+
{
454+
now: new Date("2026-08-16T20:00:00.000Z"),
455+
runGh: async () => {
456+
reads += 1;
457+
throw Object.assign(new Error("HTTP 403: API rate limit exceeded"), {
458+
stderr: "HTTP 403: API rate limit exceeded",
459+
});
460+
},
461+
},
462+
);
463+
464+
await expect(result).rejects.toEqual(
465+
expect.objectContaining({ kind: "rate-limit", runId: null }),
466+
);
467+
expect(reads).toBe(1);
468+
expect(fs.existsSync(markdownFile)).toBe(false);
469+
expect(fs.existsSync(jsonFile)).toBe(false);
470+
});
471+
});
472+
473+
it(
474+
"runs the npm collector entry point with offline evidence",
475+
async () => {
476+
await withTemporaryDirectory(async (directory) => {
477+
const logsDir = path.join(directory, "logs");
478+
const runsFile = path.join(directory, "runs.json");
479+
const markdownFile = path.join(directory, "report.md");
480+
const jsonFile = path.join(directory, "report.json");
481+
fs.mkdirSync(logsDir, { mode: 0o700 });
482+
fs.writeFileSync(runsFile, `${JSON.stringify([failedRun(67890123)])}\n`, {
483+
mode: 0o600,
484+
});
485+
fs.writeFileSync(
486+
path.join(logsDir, "67890123.log"),
487+
"job\tstep\tError: offline entry-point failure\n",
488+
{ mode: 0o600 },
489+
);
490+
491+
const { stdout } = await execFileAsync(
492+
"npm",
493+
[
494+
"run",
495+
"e2e:unit-gaps",
496+
"--",
497+
"--runs-file",
498+
runsFile,
499+
"--logs-dir",
500+
logsDir,
501+
"--output",
502+
markdownFile,
503+
"--json-output",
504+
jsonFile,
505+
],
506+
{ cwd: process.cwd(), encoding: "utf8", maxBuffer: 8 * 1024 * 1024, timeout: 60_000 },
507+
);
508+
509+
expect(stdout).toContain("Wrote 1 cause candidates from 1 runs");
510+
expect(fs.existsSync(markdownFile)).toBe(true);
511+
expect(JSON.parse(fs.readFileSync(jsonFile, "utf8"))).toMatchObject({
512+
incompleteRuns: [],
513+
runCounts: { failure: 1 },
514+
});
515+
});
516+
},
517+
90_000,
518+
);
232519
});

0 commit comments

Comments
 (0)