test: cover null-action guard and isolate explanation test fixtures#81
Merged
Conversation
Two follow-ups from the post-merge review of #78: - Add the missing test for assertValidExecutionInputs(null, ...). The branch is unreachable via TypeScript, but defensive code on a safety boundary deserves a test so future refactors can't silently delete it. - Convert the seven shared-fixture describe blocks in explanation-generator.test.ts from module-level const to per-test beforeEach. Saved records no longer accumulate across it() calls in the same describe, so future "save called once" assertions won't inherit cross-contamination. Net new tests: +1. Existing tests still 30/30 explanations, now 67/67 execution-router. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
This PR tightens test reliability around safety-boundary invariants and eliminates cross-test state contamination in explanation generator unit tests.
Changes:
- Adds a unit test covering the defensive “null CandidateAction” guard in
ExecutionRouter.executeWithRouting. - Refactors multiple
ExplanationGeneratortest suites to use per-test setup (beforeEach) instead of sharing a single in-memory repo instance acrossit()calls.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| packages/explanations/src/tests/explanation-generator.test.ts | Switches shared repo/gen instances to beforeEach initialization to prevent fixture state leakage across tests. |
| packages/execution-router/src/tests/execution-router.test.ts | Adds coverage for the unreachable-in-practice null-action invariant guard in executeWithRouting. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| const assessment = makeRiskAssessment(); | ||
| await expect( | ||
| router.executeWithRouting(null as unknown as CandidateAction, assessment, 'user-1'), | ||
| ).rejects.toThrow(/without a CandidateAction/); |
There was a problem hiding this comment.
This test title says it throws an InvariantViolationError, but the assertion only matches the error message regex. Consider also asserting the thrown error is an instance of InvariantViolationError (or using rejects.toThrow(InvariantViolationError)), so the test can’t pass due to some other unexpected error with a similar message.
Suggested change
| ).rejects.toThrow(/without a CandidateAction/); | |
| ).rejects.toBeInstanceOf(InvariantViolationError); |
2 tasks
jayzalowitz
added a commit
that referenced
this pull request
Apr 27, 2026
…h work (#87) Captures eight PRs (#77, #78, #79, #81, #82, #83, #84, #85, #86) under an [Unreleased] section. No code changes — pure documentation. Bumping VERSION + cutting a tag is left as a separate decision so the release moment stays explicit. Co-authored-by: Claude Opus 4.7 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Two follow-ups surfaced during the post-merge review of #78:
The larger gaps from #78 (e2e tests, partial-block leak in `whatWouldIDo`, blocked-by-policy ExplanationRecord persistence) are tracked separately in #80.
Test plan
🤖 Generated with Claude Code