fix(server): reject unsupported monitor-scheduling fields on PATCH /issues/:id (RBR-1101) - #26
Merged
Conversation
…ssues/:id (RBR-1101) RBR-1094 diagnosed (3x independent repro) that PATCH /issues/:id silently accepts and drops monitorNextCheckAt / monitorNotes / monitorScheduledBy (flat) and executionState.monitor.* (nested) -- HTTP 200, no error, zero persistence. updateIssueSchema (packages/shared/src/validators/issue.ts) has no shape entries for these fields and is never .strict()'d, so Zod's default non-strict .parse() silently strips unknown keys before the route handler ever sees them. Per CEO-approved product decision on RBR-1094: ship the narrow, low-blast-radius fix now rather than flipping updateIssueSchema to global .strict() (which would newly reject other currently-tolerated extra keys across every PATCH caller company-wide and needs a separate audit). Fix: add rejectUnsupportedIssuePatchMonitorSchedulingFields(), a z.preprocess() wrapper applied to the route-level PATCH schema (updateIssueRouteSchema in server/src/routes/issues.ts) that inspects the raw request body before Zod strips anything, and raises a 4xx naming the unsupported field(s) -- monitorNextCheckAt, monitorNotes, monitorScheduledBy, or executionState.monitor -- instead of the silent 200 no-op. Matches the RBR-807/RBR-826 unsupported-field-rejection pattern already used elsewhere in this codebase. Tests (server/src/__tests__/issue-recovery-actions.test.ts): 5 new cases -- one per unsupported field asserting a 4xx naming it, plus one proving a legitimate PATCH without these fields is unaffected. Full issue-recovery-actions.test.ts suite: 49/49 passing. AC-a/AC-b/AC-c of RBR-1101 satisfied. AC-d (comment back on parent + RBR-954 confirmation) to follow once this lands.
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.
Summary
RBR-1094 diagnosed (3x independent repro) that
PATCH /issues/:idsilently accepts and dropsmonitorNextCheckAt/monitorNotes/monitorScheduledBy(flat) andexecutionState.monitor.*(nested) — HTTP 200, no error, zero persistence.
updateIssueSchemahas no shape entries for thesefields and is never
.strict()'d, so Zod's default non-strict.parse()silently strips unknownkeys before the route handler ever sees them.
Per CEO-approved product decision on RBR-1094: ship the narrow, low-blast-radius fix now rather than
flipping
updateIssueSchemato global.strict()(untested blast radius across every other PATCHcaller company-wide; needs a separate audit).
Fix
Adds
rejectUnsupportedIssuePatchMonitorSchedulingFields(), az.preprocess()wrapper applied tothe route-level PATCH schema (
updateIssueRouteSchemainserver/src/routes/issues.ts) thatinspects the raw request body before Zod strips anything, and raises a 4xx naming the unsupported
field(s) instead of the silent 200 no-op. Matches the RBR-807/RBR-826 unsupported-field-rejection
pattern already used elsewhere in this codebase.
Tests
5 new cases in
server/src/__tests__/issue-recovery-actions.test.ts— one per unsupported fieldasserting a 4xx naming it, plus one proving a legitimate PATCH without these fields is unaffected.
Full suite run (this branch, fork/master base):
issue-recovery-actions.test.ts27/27 passing.packages/sharedvitest project: 170/170 passing.Notes
Retargeted onto
fork/masterper the RBR-974/RBR-1081 repo-governance policy —paperclipai/paperclipis upstream (no push access);
PraeSynBH/paperclipis the repo we actually control and deploy from.This branch was rebased off
fork/master(notorigin/master) to keep it mergeable/deployable.Closes RBR-1101. AC-d follow-up (comment back on parent RBR-1094 + confirm on RBR-954) to be done
after this merges.