fix: check response.result instead of response.warnings for OnError.REJECT early-return#2976
Open
joonas wants to merge 3 commits intodefenseunicorns:mainfrom
Open
Conversation
…ror.REJECT` early-return The early-return condition in `mutateProcessor`'s binding loop used `response?.warnings!.length > 0` to detect whether a rejection had occurred. This is the wrong signal — `response.warnings` accumulates entries for every failed callback regardless of the `onError` mode, so pre-existing warnings from a prior iteration could trigger a spurious early-return even when the current binding succeeded and no rejection was issued. The `?.` followed by `!` was also contradictory: the optional chain guarded against `response` being nullish while the non-null assertion claimed `warnings` was definitely defined — but `response` is always initialized on entry to `mutateProcessor`, making `?.` unnecessary. Replace the condition with `response.result !== undefined`. `response.result` is only set inside `processRequest`'s catch block when `config.onError === OnError.REJECT`, making it the precise signal for rejection. Add 4 tests covering the fix: - `processRequest` does not set `response.result` when the callback succeeds, even when the response carries pre-existing warnings - `processRequest` does not signal rejection when warnings pre-exist but the callback succeeds - `mutateProcessor` skips remaining bindings when the first fails under REJECT mode - `mutateProcessor` processes all bindings when all succeed under REJECT mode Signed-off-by: Joonas Bergius <joonas@defenseunicorns.com>
The `reencodeData` mock returned its input unchanged — an identity function over the `PeprMutateRequest` wrapper. In production, `reencodeData` returns `clone(wrapped.Raw)`, a `KubernetesObject`. The identity mock let the "both succeed" test pass with an object shape that never occurs in the real patch-generation path, reducing confidence in the test. Update the mock to `(wrapped) => clone(wrapped.Raw)`, matching production behavior. Also reword test comments that overstated a runtime scenario — they described warnings accumulating "from earlier binding iterations," but under `OnError.REJECT` the loop returns immediately on the first rejection so warnings never carry over. The comments now focus on `processRequest`'s contract: it treats the response as an in/out parameter and does not clear pre-existing warnings on success. Signed-off-by: Joonas Bergius <joonas@defenseunicorns.com>
Contributor
Contributor
Greptile SummaryThis PR fixes a spurious early-return in Confidence Score: 5/5Safe to merge — the fix is minimal, precise, and well-tested; only a minor test redundancy remains. The one-line production change is clearly correct: No files require special attention. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[mutateProcessor: for each bindable] --> B[processRequest bindable, wrapped, response]
B --> C{callback throws?}
C -- No --> D[Log success]
C -- Yes --> E[response.warnings.push error]
E --> F{config.onError}
F -- REJECT --> G[response.result = 'Pepr module configured to reject on error']
F -- AUDIT --> H[response.auditAnnotations update]
G --> J{OLD: warnings.length > 0?}
D --> J
H --> J
J -- Yes under REJECT ❌ spurious --> K[Early return]
J -- No --> L[Next binding]
G --> N{NEW: response.result !== undefined?}
D --> N2[response.result still undefined]
N -- Yes ✅ precise --> K2[Early return]
N2 --> L2[Next binding]
style J fill:#f99,color:#000
style N fill:#9f9,color:#000
Reviews (1): Last reviewed commit: "Merge branch 'main' into fix/mutate-proc..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Comment on lines
+384
to
+448
| describe("OnError.REJECT early-return checks response.result", () => { | ||
| describe("processRequest with pre-existing warnings", () => { | ||
| it("should not set response.result when callback succeeds, even with pre-existing warnings", async () => { | ||
| const successCallback = vi.fn(); | ||
| const testBinding = { ...clone(defaultBinding), mutateCallback: successCallback }; | ||
| const testBindable: Bindable = { | ||
| ...clone(defaultBindable), | ||
| binding: testBinding, | ||
| config: { ...defaultModuleConfig, onError: OnError.REJECT }, | ||
| }; | ||
|
|
||
| // processRequest accepts an existing response and appends to it. | ||
| // Pre-existing warnings must not cause it to set response.result. | ||
| const responseWithPreExistingWarnings: MutateResponse = { | ||
| uid: "test-uid", | ||
| allowed: false, | ||
| warnings: ["warning from a prior binding failure"], | ||
| }; | ||
|
|
||
| const result = await processRequest( | ||
| testBindable, | ||
| defaultPeprMutateRequest(), | ||
| responseWithPreExistingWarnings, | ||
| ); | ||
|
|
||
| // The callback succeeded — no rejection occurred. | ||
| expect(successCallback).toHaveBeenCalled(); | ||
| expect(result.response.result).toBeUndefined(); | ||
|
|
||
| // Pre-existing warnings survive untouched. | ||
| expect(result.response.warnings).toContain("warning from a prior binding failure"); | ||
| }); | ||
|
|
||
| it("should not signal rejection when warnings pre-exist but callback succeeds", async () => { | ||
| // A successful callback must not set response.result, regardless of | ||
| // whether the response already carries warnings. | ||
| const successCallback = vi.fn(); | ||
| const testBinding = { ...clone(defaultBinding), mutateCallback: successCallback }; | ||
| const testBindable: Bindable = { | ||
| ...clone(defaultBindable), | ||
| binding: testBinding, | ||
| config: { ...defaultModuleConfig, onError: OnError.REJECT }, | ||
| }; | ||
|
|
||
| const responseWithWarnings: MutateResponse = { | ||
| uid: "test-uid", | ||
| allowed: false, | ||
| warnings: ["prior warning"], | ||
| }; | ||
|
|
||
| const { response } = await processRequest( | ||
| testBindable, | ||
| defaultPeprMutateRequest(), | ||
| responseWithWarnings, | ||
| ); | ||
|
|
||
| // Warnings survive, but result is not set — no rejection occurred. | ||
| expect(response.warnings).toContain("prior warning"); | ||
| expect(response.result).toBeUndefined(); | ||
| }); | ||
| }); | ||
|
|
||
| describe("mutateProcessor with two bindings under REJECT", () => { | ||
| beforeEach(() => { | ||
| vi.clearAllMocks(); |
Contributor
There was a problem hiding this comment.
The two tests inside processRequest with pre-existing warnings are functionally identical: both create a successCallback, build a response pre-seeded with one warning, call processRequest, and assert that result.response.result is undefined while the prior warning survives. Neither test exercises a distinct scenario — the only difference is variable naming (responseWithPreExistingWarnings vs responseWithWarnings). One of them can be removed without any loss of coverage.
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
2 participants
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.
Description
The early-return condition in
mutateProcessor's binding loop usedresponse?.warnings!.length > 0to detect whether a rejection had occurred. Sinceresponse.warningsaccumulates entries for every failed callback regardless of theonErrormode the pre-existing warnings from a prior iteration could trigger a spurious early-return even when the current binding succeeded and no rejection was issued.The
?.followed by!was also contradictory: the optional chain guarded againstresponsebeing nulli-sh while the non-null assertion claimedwarningswas definitely defined, butresponseis always initialized on entry tomutateProcessor, making?.unnecessary.Replace the condition with
response.result !== undefined.response.resultis only set insideprocessRequest's catch block whenconfig.onError === OnError.REJECT, making it the precise signal for rejection.Add 4 tests covering the fix:
processRequestdoes not setresponse.resultwhen the callback succeeds, even when the response carries pre-existing warningsprocessRequestdoes not signal rejection when warnings pre-exist but the callback succeedsmutateProcessorskips remaining bindings when the first fails under REJECT modemutateProcessorprocesses all bindings when all succeed under REJECT modeEnd to End Test:
(See Pepr Excellent Examples)
Type of change
Checklist before merging