Skip to content

test: remove unused recovery preload helper - #8369

Open
nightcityblade wants to merge 6 commits into
NVIDIA:mainfrom
nightcityblade:fix/issue-8346
Open

test: remove unused recovery preload helper#8369
nightcityblade wants to merge 6 commits into
NVIDIA:mainfrom
nightcityblade:fix/issue-8346

Conversation

@nightcityblade

@nightcityblade nightcityblade commented Aug 5, 2026

Copy link
Copy Markdown

Summary

Deletes an unused recovery-preload test helper that has no consumers. The active fixture and runtime recovery behavior do not change.

Related Issue

Fixes #8346

Changes

  • Remove test/helpers/runtime-recovery-preload-test-helpers.ts.
  • Preserve the active fixture in src/lib/agent/runtime-recovery-preload.test.ts.

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: This PR deletes an unreferenced test helper and does not change behavior.
  • Docs updated for user-facing behavior changes
  • Docs not applicable — justification: No user-visible behavior or supported product surface changes.
  • 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: Nine-category maintainer security review passed with no findings: test: remove unused recovery preload helper #8369 (comment)
  • Non-success, skipped, or missing CI check accepted by maintainer — check name, approval link, and follow-up issue: No waiver. Required checks must pass for the current branch revision.

Documentation Writer Review

  • Documentation writer subagent reviewed the completed changes
  • Result: no-docs-needed
  • Evidence: This PR deletes one unreferenced test-helper file. Repository search finds no consumers, and active runtime-recovery fixtures, production behavior, commands, configuration, defaults, output, and supported workflows are unchanged, so no documentation update is needed.
  • Agent: Codex Desktop

DGX Station Hardware Evidence

  • Tested on DGX Station
  • Tested commit: Not applicable; scripts/prepare-dgx-station-host.sh does not change.
  • Station profile/scenario: Not applicable.
  • Result: Not applicable.
  • Supporting evidence: Not applicable.

Verification

  • PR description includes a Signed-off-by: line and every commit appears as Verified in GitHub — all 6 commits appear as Verified.
  • 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 — contributor validation is recorded.
  • Targeted behavior tests pass for the current change set, or tests are marked not applicable above — At validated commit b409663788f0, npm run test:projects:check (2,203 candidates across 7 projects), npm run checks:repository, and npm run check:diff pass.
  • Applicable broad gate passed — Fresh required CI is running for the current branch revision; no waiver is requested.
  • Quality Gates section completed with required justifications or waivers
  • No secrets, API keys, or credentials committed — the deleted helper contains no credential material.
  • 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: nightcityblade jackchen@haloailabs.com
Signed-off-by: nightcityblade nightcityblade@gmail.com

Summary by CodeRabbit

  • Chores
    • Removed obsolete runtime recovery preload test helpers.
    • No end-user-facing behavior changes.

Signed-off-by: nightcityblade <jackchen@haloailabs.com>
Signed-off-by: nightcityblade <nightcityblade@gmail.com>
@copy-pr-bot

copy-pr-bot Bot commented Aug 5, 2026

Copy link
Copy Markdown

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

coderabbitai Bot commented Aug 5, 2026

Copy link
Copy Markdown
Contributor
📝 Walkthrough

Walkthrough

The unused runtime-recovery-preload-test-helpers.ts test helper was deleted. Its recovery preload path interface and two helper functions were removed. Production recovery behavior was not changed.

Changes

Cohort / File(s) Summary
Runtime recovery test cleanup
test/helpers/runtime-recovery-preload-test-helpers.ts
Deletes the unused recovery preload harness interface and path-rewriting helpers.

Estimated code review effort: 1 (Trivial) | ~2 minutes

Suggested labels: refactor

Suggested reviewers: cv, apurvvkumaria

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Linked Issues check ✅ Passed The change deletes the specified unused helper and preserves production behavior and the active source-local fixture.
Out of Scope Changes check ✅ Passed The pull request contains only the helper deletion required by issue #8346, with no unrelated code changes.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly and concisely describes the deletion of the unused recovery preload test helper.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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

@github-actions

github-actions Bot commented Aug 5, 2026

Copy link
Copy Markdown
Contributor

PR Review Advisor — No blocking findings reported

Advisor assessment: No blocking advisor findings reported
Next action: No advisor follow-up needed.
Findings: 0 blockers · 0 warnings · 0 suggestions

Model lanes

  • GPT-5.6 Terra (primary): Completed · high 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 match; normalized E2E selections differ; severity counts match.
3 additional E2E selections from the second opinion

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

  • issue-2478-crash-loop-recovery: The completed second-opinion lane identified E2E coverage that the primary lane omitted.
  • onboard-repair: The completed second-opinion lane identified E2E coverage that the primary lane omitted.
  • onboard-resume: The completed second-opinion lane identified E2E coverage that the primary lane omitted.

Second-opinion terminology and E2E selections are advisory. Live E2E does not run automatically for pull requests.

E2E guidance

Advisory only. A maintainer can dispatch the default E2E suite against this exact revision.

Recommended E2E: None

1 optional E2E recommendation
  • gateway-guard-recovery

Workflow run details

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

@apurvvkumaria

Copy link
Copy Markdown
Collaborator

Automated-review classification: GPT-5.6 Terra and the publisher completed successfully with no actionable finding. The Nemotron lane failed because its analysis omitted a required workflow tool result, not because of the patch. Independent structural validation also found no references to the deleted file or any of its exported helpers, and the contributor commit is GitHub Verified with DCO.

I am using one bounded failed-job retry for the advisor protocol failure. If it repeats, I will stop retrying and document the external blocker. Protected E2E remains in progress.

@apurvvkumaria

Copy link
Copy Markdown
Collaborator

The bounded advisor retry repeated the same external protocol failure: the Nemotron analysis omitted the required terminology-review result and continued before that step completed. GPT-5.6 Terra and the publisher passed again with zero actionable findings.

No PR code change is indicated, and I will not retry this lane again. The remaining automated-review blocker is now documented as external. Protected E2E is still pending, and independent approval is still required before merge.

@cjagwani cjagwani added v0.0.104 Release target and removed v0.0.103 labels Aug 5, 2026

@cjagwani cjagwani left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Verdict

Security review — exact head 850cb8ad2687a69c674916f4c963afafa5b879db against current base b148bc13ee53b7a1f612d07e42275ec732e71877: PASS with no findings, subject to every repository merge gate.

Findings Table

No findings.

Detailed Analysis

  1. Secrets and Credentials — PASS. The effective PR patch is unchanged and adds no secret, credential source, environment exposure, or sensitive logging. The refreshed base introduces no overlapping credential path.

  2. Input Validation and Data Sanitization — PASS. Reviewed validation, command construction, and path handling remain unchanged. The portable-profile base delta has no exact file overlap with this PR.

  3. Authentication and Authorization — PASS. No identity, ownership, permission, or authorization boundary is widened by the PR patch or its integration with the refreshed base.

  4. Dependencies and Third-Party Libraries — PASS. The PR's reviewed dependency and artifact identities are unchanged. Base PRs #8333 and #8376 add disjoint E2E artifact restoration and portable experimental onboarding behavior.

  5. Error Handling and Logging — PASS. Reviewed fail-closed behavior and diagnostics remain unchanged; the signed integrations add no overlapping error or logging path.

  6. Cryptography and Data Protection — PASS. Reviewed cryptographic and protected-data behavior remains unchanged, with no new cross-patch data flow.

  7. Configuration and Security Headers — PASS. The effective PR patch preserves its reviewed configuration, network, privilege, capability, port, and header boundaries. The hidden portable profile does not bypass this PR's authority checks.

  8. Security Testing — PASS. Raw patch SHA-256 remains 815a26adc43690a45b80e5e67842480c7af8970a32dd2910ca11835c5fdd26b0 and stable patch ID remains 2a7991e4d11f27011b169a7c241da17a1d65ce6e. Repository search confirms no remaining helper reference; Vitest project membership is exact across 2,173 candidate files; repository structural checks, CLI build/typecheck, and diff hygiene pass. Exact-head documentation writer revalidation passed; fresh GitHub Actions is authoritative.

  9. System Security — PASS. Both signed refreshes are conflict-free, preserve the effective patch byte-for-byte, and introduce no new runtime trust-boundary interaction. For #7853 specifically, the portable profile supplies onboarding defaults but neither stages messaging credentials nor bypasses registry-driven credential-drift validation.

Provenance and Required Gates

The exact head preserves contributor history through signed merge commits and applies the current rubric from .agents tree 7320c9e331ddbcd9ff83e0fce9387a004e3bd1aa.

Fresh required CI, protected E2E, the exact documentation receipt, unresolved-review resolution, GitHub commit verification, and an independent exact-head approval remain mandatory. This review waives none of those gates.

@cjagwani
cjagwani requested review from apurvvkumaria and cv August 5, 2026 20:51
@apurvvkumaria

Copy link
Copy Markdown
Collaborator

Documentation follow-up for the current branch revision:

  • Independent documentation review concluded no-docs-needed. The change only removes an unreferenced test helper and does not alter runtime behavior, supported interfaces, configuration, commands, or user workflows.
  • npm run test:projects:check passed across 2,175 candidates.
  • npm run checks:repository passed.
  • The PR description now records the current documentation disposition and validation evidence; stale revision references were removed.

No additional branch change is indicated. The advisor failures are external service failures with no patch finding, and their bounded retry has already been documented. The protected E2E gate and remaining CI are still in progress, and independent approval is still required before merge.

@apurvvkumaria

Copy link
Copy Markdown
Collaborator

Protected E2E has completed successfully for the current branch revision: the gate result passed, and every required automated check is green. The implementation, documentation disposition, and security review are complete. Remaining gates are independent approval and the active merge freeze; no merge was performed.

Signed-off-by: Charan Jagwani <cjagwani@nvidia.com>

@cjagwani cjagwani left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Security review — PASS

Exact head: c9fdc5d4f37ef47d596a9518b0901fbceb9b2902
Exact base: db31c286129e878c3356eed49f76ab259561e47e

I reviewed the complete effective diff and current-base integration. No security findings remain.

  1. Secrets and credentials — PASS. The patch deletes test-only helper code and adds no credential source, value, logging, or redaction behavior.
  2. Input validation and sanitization — PASS / unchanged. No production parser, validation path, command construction, or user input handling changes.
  3. Authentication and authorization — PASS / unchanged. No identity, ownership, permission, or authorization boundary changes.
  4. Dependencies and third-party code — PASS. No dependency, lockfile, artifact, image, or runtime pin changes.
  5. Error handling and logging — PASS / unchanged. No production error path, diagnostic, or logging behavior changes.
  6. Cryptography and data protection — PASS / unchanged. No cryptographic operation, protected-data flow, persistence behavior, or serialization changes.
  7. Configuration and exposure — PASS / unchanged. No configuration, network policy, port, privilege, capability, service, or security-header change.
  8. Security testing — PASS. Repository search confirms the deleted helper has no consumer. Exact Vitest project membership (2,177 candidates across 7 projects), repository checks, diff hygiene, and npm run check:diff pass.
  9. System security — PASS. The effective patch is exactly one unreferenced 42-line test-helper deletion. The current-main merge is conflict-free with no resolution diff, and active runtime-recovery fixtures and production behavior are unchanged.

Documentation-writer revalidation at this exact head returned no-docs-needed. Fresh GitHub Actions, protected E2E, automated review, commit verification, and an independent approval remain mandatory; this review waives none of those gates.

@apurvvkumaria

Copy link
Copy Markdown
Collaborator

Refreshed-branch validation is complete. Reconciliation with current main preserved the effective contributor patch as the same one-file deletion of the unused recovery preload helper, and the refresh commit is GitHub Verified.

Full required CI, both advisor lanes, DCO, commit lint, documentation review, and the existing security review are green with no unresolved review threads. The protected risk evaluation selected no live journey for this test-helper-only change and finalized the required gate successfully.

This PR is technically ready for independent approval. It remains unmerged under the release-tag freeze.

Signed-off-by: Apurv Kumaria <akumaria@nvidia.com>
@coderabbitai

coderabbitai Bot commented Aug 6, 2026

Copy link
Copy Markdown
Contributor

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.

Signed-off-by: Apurv Kumaria <akumaria@nvidia.com>
@apurvvkumaria

Copy link
Copy Markdown
Collaborator

Maintainer follow-up for the current branch revision (b40966378):

  • Refreshed the contributor branch from current main without rewriting history. The effective PR diff remains exactly the deletion of the unreferenced 42-line test/helpers/runtime-recovery-preload-test-helpers.ts; no merge resolution changed that deletion.
  • Repository-wide search finds no remaining filename, interface, or exported-helper consumer. The active fixture in src/lib/agent/runtime-recovery-preload.test.ts and all production code remain unchanged.
  • Validation passes: Vitest project membership is exact across 2,203 candidate files and seven projects; repository structural checks, diff validation, plugin and CLI builds, CLI type checking, repository hooks, and diff hygiene pass.
  • Independent documentation-writer review returns no-docs-needed: the deleted test-only helper changes no runtime behavior, command, configuration, API, support boundary, user workflow, docs route, or public claim.

Nine-category security review: PASS, no findings. (1) no secrets or credentials are added or exposed; (2) no input-validation or command-construction path changes; (3) no authentication or authorization changes; (4) no dependency or artifact change in the PR diff; (5) no production error or logging change; (6) no cryptographic or protected-data change; (7) no configuration, network, privilege, port, capability, or header change; (8) project-membership and consumer searches confirm the deleted helper is inactive; (9) the holistic runtime and recovery security boundary is unchanged because the effective patch removes only unused test support code.

No CI waiver or merge bypass is requested. Fresh required checks and an independent approval remain mandatory before merge.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Delete the unused runtime-recovery preload test helper

3 participants