Skip to content

fix(e2e): restore Hermes rebuild provider attachment - #10157

Merged
prekshivyas merged 1 commit into
mainfrom
codex/fix-openshell-state-transition-10155
Aug 25, 2026
Merged

fix(e2e): restore Hermes rebuild provider attachment#10157
prekshivyas merged 1 commit into
mainfrom
codex/fix-openshell-state-transition-10155

Conversation

@rsliter

@rsliter rsliter commented Aug 24, 2026

Copy link
Copy Markdown
Collaborator

Summary

Before this change, the Hermes rebuild E2E fixture deleted its sandbox and then tried to recreate it with a retained Discord policy but no attached replacement provider, so OpenShell rejected the create request. The fixture now supplies its existing fake Discord credential during recreation, allowing the current static provider profile to replace the legacy generic provider.

Related Issue

Part of #10155

Changes

  • Build the explicit Hermes recreation environment with the fake Discord credential, verbose rebuild diagnostics, and the selected base-image override.
  • Use that environment in both the normal and stale-base Hermes rebuild lanes while continuing to omit host inference credentials.
  • Add deterministic E2E support coverage for the provider credential, base-image override, and inference-credential exclusions.
  • Draft PR fix(onboard): restore lifecycle E2E qualification #10110 contains the same DISCORD_BOT_TOKEN call-site hunk inside a broader lifecycle change. This PR isolates that hunk and adds deterministic coverage at the owned child-environment boundary so the correction can be reviewed and landed independently.

The small recreation-environment helper is owned by the two rebuild-Hermes lanes. A call-site-only literal would not expose the complete child-environment contract to deterministic support coverage; test/e2e/support/rebuild-hermes-env.test.ts protects that contract.

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:
  • 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: Self-review completed. The change forwards only the existing fake E2E Discord token, which remains covered by the fixture's redaction values and backup credential leak scan. No production credential, messaging policy, or network boundary changes.
  • 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: Not applicable
  • 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
  • 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
  • Targeted behavior tests pass for the current change set, or tests are marked not applicable above — command/result or justification: npx vitest run --project e2e-support test/e2e/support/rebuild-hermes-env.test.ts passed 1 file and 6 tests.
  • Applicable broad gate passed — npm test for broad runtime/test-harness changes; npm run check for repo-wide validation/coverage changes — command/result:
  • 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)

Documentation Writer Review

  • Documentation writer subagent reviewed the completed changes
  • Result: no-docs-needed
  • Evidence: This commit changes only the Hermes rebuild E2E fixture. Existing documentation already states that Hermes rebuild preserves messaging credentials and attaches the exact validated static provider required by a retained Discord policy.
  • Agent: Codex Desktop

Signed-off-by: Rebecca Sliter 571084+rsliter@users.noreply.github.com

Summary by CodeRabbit

  • Tests
    • Expanded end-to-end coverage for Hermes rebuild environment recreation.
    • Verified that required Discord credentials and prepared base settings are preserved during rebuilds.
    • Confirmed unsupported credentials are excluded and verbose rebuild output is enabled.
    • Added checks to ensure environment configuration remains consistent across rebuild scenarios.

@rsliter rsliter self-assigned this Aug 24, 2026
@github-code-quality

github-code-quality Bot commented Aug 24, 2026

Copy link
Copy Markdown
Contributor

Code Coverage Overview

Languages: TypeScript

TypeScript / code-coverage/plugin

The overall line coverage in commit 4640fc9 in the codex/fix-openshell-... branch remains at 96%, unchanged from commit 7e7c814 in the main branch.

TypeScript / code-coverage/cli

The overall line coverage in commit 4640fc9 in the codex/fix-openshell-... branch remains at 84%, unchanged from commit a016511 in the main branch.


Updated August 24, 2026 23:02 UTC

@coderabbitai

coderabbitai Bot commented Aug 24, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Enterprise

Run ID: 847d0c58-3fdf-4407-9b72-d182d0b74150

📥 Commits

Reviewing files that changed from the base of the PR and between 6f9b3e2 and 4640fc9.

📒 Files selected for processing (3)
  • test/e2e/live/rebuild-hermes-env.ts
  • test/e2e/live/rebuild-hermes.test.ts
  • test/e2e/support/rebuild-hermes-env.test.ts

Included review availability: Your plan provides up to 12 included reviews per hour; 10 remain after this review.


📝 Walkthrough

Walkthrough

The change adds a helper that builds the Hermes rebuild environment. The live rebuild test uses the helper, and support tests verify token, base-image, verbosity, and credential handling.

Changes

Hermes rebuild environment

Layer / File(s) Summary
Rebuild environment construction
test/e2e/live/rebuild-hermes-env.ts, test/e2e/live/rebuild-hermes.test.ts
The helper preserves the supplied environment and adds DISCORD_BOT_TOKEN and NEMOCLAW_REBUILD_VERBOSE. The live rebuild test uses and checks the helper output.
Environment forwarding coverage
test/e2e/support/rebuild-hermes-env.test.ts
The test verifies Discord token, sandbox base-image, and verbose rebuild forwarding. It also verifies that unsupported API keys remain excluded.

Estimated code review effort: 2 (Simple) | ~10 minutes

Merge Risk: ⚪ Minimal · up to 4640f

This change restores the retained Discord credential during Hermes rebuild fixture recreation and adds focused coverage for the resulting environment contract; no actionable merge-blocking risk remains after normal checks and review.

Suggested reviewers: aasthajh

✨ Finishing Touches 💡 1
🛠️ Fix failing CI checks 💡
  • Create stacked PR
  • Commit on current branch
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch codex/fix-openshell-state-transition-10155

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

@apurvvkumaria

Copy link
Copy Markdown
Collaborator

Maintainer security and documentation review: PASS

I reviewed the current revision across the nine security categories:

  • Input validation: PASS — the helper accepts fixture-owned environment values and does not introduce a user-controlled command boundary.
  • Authentication and authorization: PASS — production identity and permission paths are unchanged.
  • Secrets and credentials: PASS — the fake Discord token is scoped to the live E2E fixture, is included in artifact redaction, and inference credentials remain excluded from the rebuild child environment.
  • Injection: PASS — the token is passed as environment data, not interpolated into a shell command.
  • Data exposure and privacy: PASS — recorded output uses the existing redaction list.
  • Cryptography: PASS — no cryptographic behavior changes.
  • Dependencies and supply chain: PASS — no dependency, workflow, or image-source change.
  • Infrastructure and deployment: PASS — this repairs the existing live E2E rebuild fixture without changing production deployment behavior.
  • Business logic and abuse resistance: PASS — the focused support test proves the required provider credential is forwarded while unrelated credentials remain blocked.

Documentation review: PASS. This is a test-fixture repair and does not change user-facing behavior, so public documentation does not need an update.

Validation: the focused E2E-support suite passed all 6 tests. The change is 43 additions and 5 deletions across 3 files, so it is not a large LOC increase.

Contributor credit: Rebecca Sliter authored this focused repair. Preserve her authorship in the final squash metadata.

The automated advisor is still blocked by external service throttling and must complete successfully before merge.

@rsliter

rsliter commented Aug 24, 2026

Copy link
Copy Markdown
Collaborator Author

Focused live E2E passed for commit 4640fc9.

Run: https://github.com/NVIDIA/NemoClaw/actions/runs/32787717358
Selectors: rebuild-hermes, rebuild-hermes-stale-base

Passing scenarios:

  • Rebuild preserves Hermes state and recovers cron dispatch
  • Rebuild refreshes a stale Hermes base and restores state

The PR remained open on the tested head and base when the run completed.

@apurvvkumaria apurvvkumaria 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 and documentation review: PASS for the current revision.

This is an E2E fixture repair. It forwards the existing fixture-owned Discord credential into the Hermes rebuild child environment so the retained static provider can be attached after recreation. The value is passed as environment data, not shell text; it remains covered by the fixture's redaction list. Host inference credentials and unrelated provider credentials remain excluded.

Security rubric:

  • Secrets and sensitive data: PASS — the fake Discord token is redacted from artifacts and unrelated credentials are excluded.
  • Input validation and injection: PASS — fixture-owned values cross no new user-controlled command boundary.
  • Authentication and authorization: PASS — production identity and permission paths are unchanged.
  • Dependencies and supply chain: PASS — no dependency, workflow, or image-source change.
  • Error handling and information disclosure: PASS — existing redacted diagnostics remain in use.
  • Cryptography: PASS — no cryptographic change.
  • Configuration and deployment: PASS — production behavior is unchanged.
  • Security testing: PASS — deterministic support tests cover the complete child environment.
  • System security: PASS — only the fixture's replacement provider receives the required fake credential.

No documentation update is needed because this changes test setup rather than user-facing behavior.

Validation passed: all 12 CLI shards, required aggregate CI, CodeQL, standard E2E smoke checks, 6 focused E2E-support tests, and both live rebuild-hermes scenarios. The automated Advisor was unavailable because of external service throttling and published no actionable finding. The diff adds 43 lines and removes 5 across 3 files, below the large-change flag threshold. DCO and the verified contributor commit are present, and no review threads remain.

Rebecca Sliter is the original contributor and must receive attribution in the final squash commit.

@prekshivyas
prekshivyas merged commit ec0e2d9 into main Aug 25, 2026
71 of 80 checks passed
@prekshivyas
prekshivyas deleted the codex/fix-openshell-state-transition-10155 branch August 25, 2026 00:36
@wscurran wscurran added the chore Build, CI, dependency, or tooling maintenance label Aug 25, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

chore Build, CI, dependency, or tooling maintenance

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants