Skip to content

A missing required int is a 400, not a 500 (rc#2342)

What broke

On 2026-09-23, POST /api/events returned an unhandled 500 five times in two hours. All five were the same request, sent by rig-agent-runtime's stream consumer whenever it decides Review-E approved a PR (src/stream-consumer.js:1206, rar main @657b88a):

{"type": "REVIEW_PASSED", "agentId": "review-e", "repo": "dashecorp/rig-gitops", "prNumber": 841, "reason": "approved"}

The request has no issueNumber. The conductor turns it into a 500 in four steps:

  1. The endpoint (Program.cs, app.MapPost("/api/events", …)) had no field check for it.
  2. SubmitEvent.TryExecuteAsync calls MapToEvent first. That arm is "REVIEW_PASSED" => new ReviewPassed(r.Repo!, r.IssueNumber!.Value, r.PrNumber!.Value). The ! only silences the compiler. .Value on an empty int? throws InvalidOperationException.
  3. ConcurrencyRetry catches only concurrency exceptions. The app has no UseExceptionHandler or ProblemDetails, so Kestrel answers 500.
  4. The throw happens before AppendAsync, so nothing is ever stored.

The runtime has sent this request this way since rar#137 (2026-04-23), so every approval it detected has crashed. It never found out, because submitEvent (stream-consumer.js:167-177) ignores the response status and swallows errors.

The crash has done no damage so far. The GitHub pull_request_review webhook records the real approval, with an issue number, in-process (Program.cs:2572-2580). MergeGate does not read ReviewPassed.

The pattern is not specific to REVIEW_PASSED. 68 MapToEvent arms force-unwrap a nullable int, and until now only EPIC_DECOMPOSED guarded against a missing one.

Decision

1 · A Core policy lists what each type cannot do without

SubmitEventRequiredFields (src/ConductorE.Core/UseCases/) is a pure static policy, shaped like EventDropPolicy. Table maps each event type to the int fields its arm unwraps with !.Value, and lists exactly those. Missing(request) returns the ones that are absent.

Requires Count Types
IssueNumber 45 ISSUE_*, WORK_STARTED, CLI_*, TOKEN_USAGE, TOOL_USED, TESTS_RUN, SMOKE_*, …
IssueNumber, PrNumber 16 PR_CREATED, CI_PASSED, CI_FAILED, REVIEW_PASSED, MERGED, MERGE_GATE_*, …
PrNumber 5 PR_OPENED, PR_REVIEW_*, POST_MERGE_RETRO_REQUESTED
IssueNumber, PrNumber, ClosedPrNumber 1 DUPLICATE_PR_CLOSED
PrNumber, ClosedPrNumber 1 REVERT_RETRO_REQUESTED

Some types are not listed, and for them Missing always returns an empty list:

  • HEARTBEAT, AGENT_DEGRADED and EPIC_DECOMPOSED, whose when guards already reject their own bad input.
  • Arms that read their ints with ?? 0 or pass them through as nullable, such as MAIN_CI_FAILED, MILESTONE_COMPLETE and MEMORY_QUERIED.
  • Unknown types, which keep the existing "Unknown event type" 400.

CLI_COMPLETED requires only IssueNumber, because its prNumber is nullable. A session that opened no PR still reports its completion.

2 · The endpoint answers 400 and names the field

Right after the HEARTBEAT pre-check:

400 {"error": "REVIEW_PASSED requires IssueNumber", "missing": ["IssueNumber"]}

The endpoint returns the 400 itself rather than a SuppressionReason. The endpoint turns suppression reasons into 200 {skipped: true}, and a malformed request is an error the caller must see.

3 · Defence in depth in SubmitEvent

TryExecuteAsync checks Missing before MapToEvent. For an incomplete request it logs and returns a null outcome with a null reason, the same shape as HEARTBEAT without an agentId. So the internal callers (webhook, reconciler, services) cannot reach the throwing unwrap either.

How the table is kept true

The tests prove the table against MapToEvent itself, not against a second reading of it. SubmitEventRequiredFieldsTests reads the arm labels out of SubmitEvent.cs (89 types, pinned) and runs three theories:

  • Completeness. Every mapped type with all its ints null is either reported as missing or maps without throwing. If someone adds an arm that unwraps an int, this test fails until the arm is added to the table.
  • Sufficiency. A request with only the listed ints set always maps.
  • Necessity. Dropping any one listed int really throws. The table never lists a field the arm does not need, so no valid request is rejected.

The table size is pinned at 68. Each of five mutations built cleanly and turned tests red:

  • removing REVIEW_PASSED from the table;
  • making Missing always return an empty list;
  • deleting the endpoint branch;
  • deleting the TryExecuteAsync guard;
  • removing PR_OPENED from the table.

★ Order constraint: never make the runtime send issueNumber

It would be easy to "fix" the runtime by adding issueNumber to its REVIEW_PASSED body. Do not.

The runtime decides that a PR was "approved" by substring match on the model's response text (stream-consumer.js:1182: includes('approved') || includes('approve')). Today the crash, and from now on this 400, is the only thing that keeps those text-matched approvals out of the event store. If the request carried an issue number, they would be stored. The rc#2342 investigation found that on rc#2341 this would have moved the issue to ready_to_merge and suppressed a correct UNREVIEWED MERGE alert. rig-conductor#1976 describes the mechanism.

This PR keeps that property. The same request is still refused, now with a 400 that names the missing field.

Runtime follow-up: delete the emit, do not repair it

This goes in a separate rig-agent-runtime PR, after this one:

  • Delete the REVIEW_PASSED emit (stream-consumer.js:1206). It has never been stored, so deleting it changes nothing that is stored today. The dead review emits (CHANGES_REQUESTED, REVIEW_COMMENTED, COI_BLOCKED) have no arm and already get a 400, so they go too.
  • Make submitEvent log non-2xx responses. A 400 that nobody reads is the same silent failure as the 500 it replaces.
  • Leave the /api/merge call until rig-conductor#1976 gates it on GitHub's review state.

Out of scope

  • An app-wide exception handler. The rc#2342 investigation found 3 more 500s in the same log window: TOOL_USED concurrency retries that ran out (rc#2341). A handler would turn those into a proper response. This PR does not touch exception handling.
  • The runtime's TESTS_RUN with issueNumber: null. hooks/pre-push-tests.sh sends it when the branch name has no digits. It now gets a 400 instead of a 500. Whether it should send the event at all is the runtime's call.

What changed
added src/ConductorE.Core/UseCases/SubmitEventRequiredFields.cs, the policy
edited SubmitEvent.cs: TryExecuteAsync guard. MapToEvent is now internal so the tests can call it
edited Program.cs: the 400 in POST /api/events
added tests/ConductorE.Core.Tests/SubmitEventRequiredFieldsTests.cs, and guard cases in SubmitEventTests.cs
added 3 cases in ApiIntegrationTests.cs: the incident payload returns 400, DUPLICATE_PR_CLOSED names all three fields, and CLI_COMPLETED without prNumber returns 200
edited docs/api.md (the 400) and docs/event-store.md (the REVIEW_PASSED row listed only prNumber; now a Required fields section)

Refs rc#2342 · rc#1976 (text-matched approvals; the order constraint) · rc#2341 (the concurrency 500s) · rc#789 / rc#716 (the HEARTBEAT and EPIC_DECOMPOSED guards this generalises) · rar#137 (origin of the emit)