Validate the event feed's decoded bodies at the wrapper's decode

[jeremy]

Closes #915.

The event feed's poll lanes shape typed errors out of whatever the body carried. A 409's
position_digest / filters_digest became FeedFilterMismatchError fields unchecked, and a
400's reason became FeedRequestError.Reason unchecked — so a 409 whose digests were "x"
still triggered the position-reset recovery, and a 400 carrying a reason nobody defined fell
through to the message classifier that is documented for an ABSENT reason alone, where
Unrecognized position buys the same reset. The 200 path had the third variant: Go's JSON
decoder substitutes U+FFFD for invalid UTF-8 and for a lone surrogate escape without failing,
so position / next / a 410's resume could reach a caller as strings the server never
wrote, to be persisted or followed.

All three are now refused at the decode, in checkFeedResponse and at the poll lanes' service
boundary, as the SDK's malformed-response error: api_error, non-retryable, statusless.
The connector adapter's two stopgap selectors are deleted — the class has one home.

Generated, or hand-written per language?

Hand-written, and Go-only today. The evidence:

• The generator pipeline is Smithy → OpenAPI → per-SDK services. It emits shapes, params and
  response structs; it emits no error-body → typed-error mapping and no constraint validation
  at all. @pattern("^[0-9]+$") on AccountId produces nothing in
  go/pkg/generated/client.gen.go — grep for the pattern and there is no hit. There is no
  template to put this in.
• The typed feed errors (FeedFilterMismatchError, FeedRequestError, FeedPositionGoneError,
  InboxPositionGoneError) and their decode live only in go/pkg/basecamp/event_feed.go, which
  is a hand-written wrapper over the generated client.
• The other five SDKs' generated event-feed services
  (python/src/basecamp/generated/services/event_feed.py,
  rust/basecamp-sdk/src/generated/services/event_feed.rs, and their peers) return the generic
  error type. They do not lift the digests or the reason out of the body at all, so today there
  is nothing there to validate — which SPEC §23 already says: "the other SDKs surface the
  canonical error and leave the typed members to their connector adapters."

So this is Go first and complete, with no five-language tracking issue to open: the port of the
§23 connector to the other SDKs is already the tracked work, and the rule now rides in SPEC §23
("Decode validation") so each SDK inherits it with its typed layer rather than rediscovering it.
That is the one-change-covers-all lever available here; a generator change is not, because
nothing is generated at this layer.

What the decode now refuses

Each of the four bodies the lanes decode — the 400, the 409, the two 410s, and the 200 envelope
— is decided totally: the arm returns either the typed value or a statusless malformed
response, and never hands the body back for something further down to interpret.

Body	Before	After
409 whose digests are not both bare 16-lowercase-hex (wrong alphabet or length, uppercase, absent, null, wrong-typed)	*FeedFilterMismatchError → PollFilterChanged → position discarded, re-entry	malformed, statusless
400 whose reason is present and not invalid_position / invalid_filter (including "", a number, an array)	*FeedRequestError{Reason: …} → the legacy message classifier	malformed, statusless
any of the three shapes without a nonempty error, or with a body that is not a JSON object	canonical error, status intact	malformed, statusless
410 without its resume, or a feed 410 without a numeric epoch_after_id	canonical 410, or a fabricated 0 boundary	malformed, statusless
200 envelope without position or its rows, or a body that never decodes	the decoder's own error / an empty position	malformed
position, next, a 410 resume, a row's kind/action/event_type, the inbox's reason, or any member name at any depth carrying U+FFFD	persisted, followed, dispatched on, or read as absent	malformed

Two asymmetries, both following the contract rather than a preference. A 400 with no reason
— including an explicit null, since the member is optional — is still the undifferentiated
*FeedRequestError, because that is the documented case the message fallback exists for. And
details is outside the substitution walk entirely: it is json.RawMessage, never decoded into
a string, so nothing in it can be substituted, and refusing an escape there would contradict the
byte-identical carriage the push lane depends on.

Required members are enforced; unknown members are ignored. One rule from both ends — the
contract binds what the server must send, not what it may add. A body that grew a member is not
malformed, which is also the answer to the strict-unknown-fields half of #915.

Statuslessness is load-bearing, not cosmetic. A status is the server's verdict on the
request; these bodies are how the verdict is acted on. Left at 400, a refused body still reaches
a consumer's status-keyed fallback — the exact path a reason nobody defined must not reach. The
mutation check proves it: setting HTTPStatus: 400 on the malformed error turns three
eventfeed end-to-end cases from unrecoverable into filter_invalid. The canonical error
rides as Cause, and the response's request id rides on the refusal itself, since errors.As
stops there.

The adapter's selectors are deleted, not duplicated

eventfeed/live.go loses its 409 digest check (and isServerDigest) and its unknown-reason
default: arm. They are unreachable once the wrapper refuses those bodies, and the existing
end-to-end table in live_test.go — 409 with a malformed digest is unrecoverable, 409 missing a digest is unrecoverable, 400 with an unknown reason is surfaced, never guessed — stays green
without them. That table is the proof the deletion is safe: same inputs, same verdicts, one layer
fewer.

The refusal is routed, not just returned

Refusing at the decode closes these defects only if the connector takes the refusal somewhere
terminal. A decode error mapped to a re-entry, a retry, or a swallow would discard the held
position exactly as the unchecked value did, and a unit test that stops at "the decode returned an
error" passes either way. So the routing is traced and pinned:

PollUnrecoverable → recoverPoll → terminal ReasonPollFailed, and l.position is never
cleared — only PollFilterChanged (transition 19, recovery.go) discards it and fires
Observer.FilterConflict. The statusless malformed error reaches that verdict through
mapPollError's default arm: no typed error matches, no status arm matches, not retryable.

TestMalformedConflictNeverReachesTheReset drives all three malformed bodies through the WHOLE
connector — real Live seam over a loopback API, real decode, scripted mint and socket — and
asserts the ABSENCE of the recovery, not merely the presence of a terminal: no FilterConflict,
no PositionRejected, no checkpoint write (the held position survives the run), and exactly one
request. Reverting the three decode guards makes all three subtests fail — each one by hanging
until the harness watchdog fires, because the connector resets its position and re-enters into
the same malformed response forever. That loop is the defect, and it is what this test now holds
shut.

Declined: rejecting unknown fields

Not done, deliberately, and not because it is hard.

Strict unknown-field rejection is right for what the CALLER controls — request bodies, config,
anything where an unrecognized key is a typo the caller can fix before shipping. It is close to
always wrong for a RESPONSE from a live server that is expected to grow: the day BC3 adds a member
to a feed response, every already-deployed copy of the SDK starts failing on valid traffic, and SDK
users upgrade on their own schedule. That is a self-inflicted outage traded for nothing, because an
unknown field harms no decision the connector makes — unlike a malformed digest or a substituted
cursor, which corrupt one.

The repo already commits to the tolerant side, explicitly: details is carried verbatim as raw
bytes so that "members of newly cataloged types survive" (SPEC §23), and reason itself shipped as
an additive member to servers that had none. A strict decode would have made bc3 #13362 a breaking
change. Nothing in #915's text names unknown fields either.

Scope boundaries, said out loud

• The mint's url is not held to the substitution check. It is checked against cable-URL
  policy before it is dialed and the ticket is signed, and neither is persisted or re-issued —
  the property that makes a substitution durable.
• details is outside the walk, with the reason above, and both directions are pinned:
  the recursion refuses a substituted row key, and removing the details skip fails the test
  that says its bytes come through verbatim.
• Statuses with no typed arm are untouched. A 500 behind an HTML error page is still a
  retryable 500; only the three statuses whose body shape the contract fixes are decided here.
• No conformance case. conformance/tests/*.json asserts one behavior for all seven
  runners, and an SDK that never types these bodies cannot answer differently for them. Pinning
  it there would need per-SDK expectations the suite does not express. Go's tests carry it and
  SPEC §23 carries the rule.

Is hand-written validation the right instrument here?

Asked because three review rounds produced eleven findings, and it is a fair question whether
per-field checks over a permissive decode were fighting the grain. The honest answer is that the
findings split into two classes with different answers.

The shape class — nine of the eleven — was a structural defect, and it was not "per-field
versus typed". It was that every arm had a fallthrough: judge the body if it looks like the
documented shape, otherwise hand it back to the canonical error, which still carries the status
the recovery paths key off. Each new shape that failed the "looks like" test escaped through that
door, and the doors are not enumerable. Inverting the default closed the class: not that shape
means refused. Worth noting that a typed decode is what produced the first of them — a
wrong-typed member fails Unmarshal whole, and the arm then falls through carrying the server's
own Unrecognized position into the classifier. The detection was never the problem; the
routing of the detection was.

A typed struct with presence and nullity in the types would not have made these impossible,
and in Go it buys less than it sounds. encoding/json has no required-member concept and no
null-rejection: null unmarshals into any type and leaves the zero value, which is the single
behavior every null finding came through. A *string distinguishes absent from present but
still accepts null as absent, and nothing in the standard decoder enforces "this member must
be there". So the presence and nullity checks would still be hand-written — moved, not removed.
What the typed struct does buy is wrong-type rejection for free; what it costs is that the
rejection arrives as a whole-body decode failure, which is exactly the fallthrough that has to be
routed anyway. DisallowUnknownFields is the one real lever it adds, and this PR declines it
deliberately (above).

The substitution class — two of the eleven — is immune to decoder strictness altogether. No
typed struct, schema validator, or strict decoder in Go rejects a lone surrogate escape:
encoding/json substitutes U+FFFD and reports success, in keys as in values. It is visible only
in the decoded value, which is why it is checked there, and why the recursive name walk exists at
all. The walk is hand-written because there is nothing to generate it from.

Schema-driven validation was checked and is not available. The Go SDK ships no runtime
schema: openapi.json is a build input, client.gen.go carries no constraint metadata, and the
generator emits no validation — @pattern("^[0-9]+$") on AccountId produces nothing in the
generated client. Driving these arms from the Smithy shape would mean teaching the generator to
emit response validation, which is a real option and the right one if a second SDK grows a typed
layer — but it is a generator change, not a change to this file, and it is larger than this PR.

The one thing that IS a hand-rolled validator's structural weakness: it has to be wired at each
call site, and three times it was not. The recursive name walk existed and jsonObject did not
call it. The request id was carried on the error paths and not the 200 ones. Retry-After was
carried on neither. Each was a stated general property holding only where it happened to be
wired, and that is a fair indictment of "remember to validate here" as a design.

The answer that actually removes the question is not a typed struct — it is making the shared
reader the ONLY door, which is what this PR converged on: jsonObject performs the name walk so
no arm can skip it, requiredString is the one definition of "required", and the response's facts
travel as one struct rather than as fields each site copies by hand. Those are the three places
the recurrences happened, and each is now a single door rather than a convention. What remains
genuinely unguarded is a NEW body being decoded by a new code path that forgets to use them —
which a typed decode would not fix either, since in Go the presence and nullity checks would
still be hand-written beside it. The thing that would make it a non-question is generating these
arms from the Smithy shape, where every required member and type is already declared. That is a
generator change, it is the right follow-up if a second SDK grows a typed layer, and it is out of
scope here.

And the part I have now paid for three times: the walk itself is a lot of surface to get
right. It has had three defects of its own — quadratic in reads (it decoded each nested subtree
from raw bytes), duplicate member names accepted, and quadratic in memory (it copied each
container's path). None was a variation of one bypass; they were ordinary implementation bugs in
new code. But that is the honest cost of hand-writing a structural validator: roughly 120 lines
that have to be correct about JSON's shape, not about the feed's, and each round of review found
another way they were not. Generating this from the Smithy shape would remove the surface rather
than debug it — every required member, type and nesting fact is already declared there. That is
the follow-up this PR argues for, and the three defects are the argument.

So: the instrument is right for this PR, with the inversion that was missing. The finding
rate across rounds was 1 → 5 → 4 → 2, falling as the scope of each answer widened from the values
to the arms' default to the member readers and the keys. If a fourth round produces another batch
of the same shape, the conclusion is not a fifth reader but that these arms want generating from
the Smithy shape — worth its own issue rather than more of this file.

Tests

Every refusal has a test whose name states the claim, and the general claims are stated as
totality rather than as a list of known doors — two tables (27 off-contract error bodies, 12
off-contract envelopes) whose shapes are not enumerated anywhere in the code. Writing the first
of them found two doors that had not been raised in review.

Every guard was mutation-checked against the real code:

• isFeedDigest accepting anything → the 409 table fails.
• the malformed error carrying HTTPStatus: 400 → three eventfeed end-to-end cases flip from
  unrecoverable to filter_invalid.
• survivedDecoding always true → all five value-substitution cases fail.
• the recursive name walk reverted → the row-key cases fail; the details skip removed → the
  verbatim-carriage case fails. The boundary is pinned in both directions.
• requiredString accepting an empty string → four cases across three statuses fail.
• the key walk deleted → the substituted-member-name case fails.
• the response's facts not carried → the request-id and Retry-After cases fail on both the error
  and the 200 paths.
• the per-push path copy restored → the deep-body test fails on the number: 655 MiB against a
  32 MiB ceiling. The cost is stated as a bound in bytes rather than demonstrated as a blowup.
• feedEventStrings losing a member → the reflection test names it; the test drives the real
  function with a sentinel per string member, so it fails for a member added and not held as
  well as one dropped.
• all three decode guards reverted → the end-to-end connector test fails on all four bodies, each
  by hanging until the harness watchdog fires, because the connector resets its position and
  re-enters into the same malformed response forever. That loop is the defect.

go build, go vet, go test -race -count=1 ./..., make go-lint and make check are green.

---

Summary by cubic

Closes #915. The event feed's poll lanes now validate responses at the wrapper's decode, so malformed bodies surface as a statusless, non-retryable api_error instead of typed errors that could reset a held position or persist substituted cursors.

Behavior

• Each body the lanes decode — the 400, the 409, the two 410s, and the 200 envelope — is decided wholly against its documented shape; off-contract means refused, and the error shapes require a present, nonempty error.
• Cursors, resume URLs, row tokens, and member names at any depth are refused when decoding substituted U+FFFD. The name walk is one linear token pass carrying O(1) state per level, so deeply nested pages stay cheap — a 9,000-level body validates under a 32 MiB allocation ceiling, with no depth cap.
• Duplicate member names are refused; details passes through verbatim only at the declaring lane's path — items[*].event.details on feed pages, events[*].details on inbox pages.
• Required envelope and error-shape members are enforced and unknown members are ignored; an absent or null reason still yields the undifferentiated 400.
• A truncated read — a body that never finished arriving — stays a retryable transport failure, distinct from a body that decodes into the wrong shape.
• Refusals carry the response's request id and Retry-After; the connector maps them to terminal PollUnrecoverable, so the held position survives and FilterConflict never fires.

Migration

• The eventfeed connector's duplicate digest and reason checks are deleted as unreachable; treat refusals as terminal rather than recovery signals.
• Callers that matched these bodies by HTTPStatus or errors.As will stop matching for malformed responses.
• MIGRATING.md and SPEC §23 document the rule; other SDKs are unaffected.

^{Written for commit be93ef5. Summary will update on new commits.}

[chatgpt-codex-connector]

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review	Status	Commit	Review trigger
📝 Code Review	✅ Completed 2026-09-18T08:49:50.557610Z	be93ef5	New commits

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

[Copilot]

🟡 Changes recommended

Null or wrong-typed reason and digest fields can still bypass statusless malformed-response classification.

Get a fresh assessment by requesting another Copilot review.

Pull request overview

Validates Go event-feed responses before exposing recovery-driving typed values.

Changes:

• Validates 400 reasons, 409 digests, UTF-8, and durable cursors.
• Routes malformed responses terminally and removes duplicate adapter validation.
• Adds wrapper and end-to-end regression coverage.

[!TIP]
If you aren't ready for review, convert to a draft PR.
Click "Convert to draft" or run gh pr ready --undo.
Click "Ready for review" or run gh pr ready to reengage.

File summaries

File	Description
SPEC.md	Defines event-feed decode validation.
go/pkg/basecamp/event_feed.go	Implements response validation.
go/pkg/basecamp/event_feed_test.go	Tests malformed wrapper responses.
go/pkg/basecamp/eventfeed/live.go	Removes redundant adapter checks.
go/pkg/basecamp/eventfeed/live_test.go	Tests terminal connector routing.
go/pkg/basecamp/eventfeed/loop_test.go	Supports real poll sources in the harness.

Review details

Suppressed comments (1)

go/pkg/basecamp/event_feed.go:499

• A wrong-typed digest (for example "filters_digest": 42) makes json.Unmarshal fail, so this branch returns the original statusful 409 instead of the promised statusless malformed-response error. Since such a value is also not a bare 16-lowercase-hex digest, classify decode failure the same way as the subsequent shape check rather than falling through.

		if json.Unmarshal(body, &mismatch) == nil {

• Files reviewed: 6/6 changed files
• Comments generated: 1
• Review effort level: Balanced

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 8008853f8a

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[jeremy]

Review threads: 1 resolved (1 fixed + 1 declined in the same thread — Copilot raised two shapes on one thread).

• Fixed — a wrong-typed member bypassed the refusal (de1b5b3). A typed whole-body decode fails outright on {"reason": 42} or {"position_digest": 42}, and the arm fell through to the canonical error the refusal displaces. For the 400 that is the original defect through another door: the body arrived statusful, still carrying the server's own Unrecognized position. Members are now read one at a time off a raw map. Pinned by four wrapper cases and by a 400 whose reason is not even a string in the end-to-end connector test, which hangs on the reset/re-entry loop if reverted.
• Declined — a null reason is the absent case, not an unnamed value. The member is optional by contract, so null is a server saying it has no reason: the undifferentiated 400, whose answer is the message fallback. Refusing it would reject a body the contract permits. It is also the reading the 410 arm already gives a nulled epoch_after_id. Now pinned by its own test.

Copilot's effort level on the reviewed head was Balanced, and its finding was in the suppressed-comments block as well as the thread. Awaiting Codex on de1b5b36.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: de1b5b36c5

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[jeremy]

Review threads: 5 resolved (4 answered structurally, 1 declined with the reasoning in the thread).

The five findings were one class wearing different clothes — a null where a string belongs, a number where a string belongs, a missing required member, a body that is not an object, a top-level null — and they all escaped the same way. Each arm asked does this look like the documented shape, and whatever failed that test fell through to the canonical error, which still carries the status the recovery paths key off. Closing them one at a time closes one door each and leaves the structure that produces doors.

The instrument, reassessed. The arms are now TOTAL over the body (7ab00a8). Each of the three typed statuses has exactly one documented body shape, so the arm returns either the typed value or the statusless malformed response and never hands the body back for something further down to interpret. Doors are not enumerable; the default is. Two rules fall out, both in SPEC §23:

• Required members are enforced; unknown members are ignored. The contract binds what the server must send, not what it may add — which is also the answer to the strict-unknown-fields question Go event-feed wrapper: validate decoded error bodies at the decode (digest hex, 400 reason enum) #915 opened with, and what keeps the inbox 410 with a stray epoch_after_id typed as the gap it is.
• Null is not a way to satisfy a required member. encoding/json unmarshals null into any type and leaves the zero value — a nulled digest reads back as "", a nulled epoch as 0, a boundary at the bottom of history. One decoder behaviour behind every null finding, so it is answered once in the member readers rather than at each site.

TestEventFeedService_RefusesEveryOffContractErrorBody states that as a table of 19 off-contract bodies rather than a list of known doors. Writing it found two more I had not thought of — a nulled error member and a nulled epoch — which is the argument for stating it as totality.

Per thread:

• Ill-typed digests and missing error on a 400 — fixed by the inversion. The second was a regression I introduced in de1b5b3 and it was the real thing: {"reason":"invalid_position"} typed without its required error, a position reset off a body that never carried the message the reset is named for.
• Top-level null reported as an object — fixed, with a different remedy than proposed: a non-object body on one of these three statuses is itself off-contract and is refused, rather than keeping its canonical status. The SPEC sentence promising the opposite was the part that was wrong. Statuses with no typed arm are untouched — a 500 behind an HTML error page is still a retryable 500.
• Surrogate escapes outside the cursors — fixed, and worth separating from the rest: this class is not decoder strictness. No typed struct or strict decoder in Go rejects a lone surrogate escape; encoding/json substitutes U+FFFD and reports success. It is only visible in the decoded value. The check now covers every string the page decodes — cursors, resume, and the rows' kind/action/event_type plus the inbox's addressing reason — and the whole-body byte scan is gone rather than kept beside it, because details is json.RawMessage and no substitution can reach it. A reflection test drives the real enumeration so it fails both ways.
• Null reason — declined. It is the one optional member on these shapes, and for an optional member null is how JSON spells "no value". Treating it as absent yields the documented undifferentiated 400; refusing it would turn a body the contract permits into a terminal poll_failed and stop a running connector. The unsafe case — a reason the contract does not name being mistaken for absence — is refused. Reasoning and test are in the thread and in SPEC §23.

CI was red on de1b5b3 for an unrelated gate: spec/tracking-issues.yml still registered the two #915 promises whose sentences this PR discharges. Both rows are removed and make check-known-defect-issues-open passes.

Bot effort levels seen: Copilot Balanced on 8008853 (its finding was in the suppressed-comments block as well as a thread); Codex reported on 8008853 and de1b5b3.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 7ab00a8270

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[jeremy]

Review threads: 4 resolved (4 fixed).

Three of the four were the total-shape rule from 7ab00a8 applied unevenly, so they are fixed once rather than three times: one reader for required members (requiredString — present, a JSON string, nonempty) used by every arm, so "required" means the same thing at every site instead of whatever each site remembered. Nonempty is load-bearing: §6's body parse falls back to a message member when error is empty, which is how {"error":"","message":"Unrecognized position"} reached the classifier.

The fourth was a class, not a site: the substitution check reached values but not member names. A substituted key does not arrive wrong, it arrives missing — an optional member reads as absent, a required one as omitted — so jsonObject now refuses a body with any substituted key, over every key rather than the ones each arm happens to read.

Two things went with it, to stop the rule having a hole where the next finding would land:

• The 200 envelope goes through the same rule: an object carrying position and the lane's rows.
• The lanes split request from parse — the pattern DocumentsService.Get and the schedule-entry composite already use — so a body that never decodes into the envelope is the same malformed response as one that decodes into the wrong shape, rather than the generated decoder's own error that a caller switching on *Error never sees. Read failures still pass through verbatim via markBodyReadFailures (Go stamps a failed body read as a non-retryable api_error #773).

The two totality tables now carry 27 off-contract error bodies and 12 off-contract envelopes; every guard in this round was mutation-checked against the real code. make check is green, including check-service-drift (265/265 operations still wrapped after the split).

Finding rate across the three review rounds on this PR: 1 → 5 → 4. Each round has been the same underlying question — does the decoder admit a shape the contract does not — answered at a different scope: first the values, then the arms' default, now the required-member reader and the keys. If a fourth round produces another batch of the same shape, the conclusion is not a fifth reader but that these arms want generating from the Smithy shape rather than hand-writing, which is a larger change than this PR and would be its own issue.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 350c785b84

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[jeremy]

Review threads: 2 resolved (2 fixed).

Both were a stated general property holding only at the shallow end, which is the third time on this PR — so both are pinned as the general claim now, not as the instance.

• Member names are validated at every depth (P1). The key loop saw only the envelope's top level, because each row stays a json.RawMessage. A row key spelled performed_by_i\ud800d decoded as if performed_by_id were absent, so the event read as performed directly by its creator and the position committed over that attribution — which is also what the exclude_performers=self loop guard is computed from. The walk descends now, with its stopping boundary written into the code and SPEC §23: structural member names are validated; opaque details contents are not, because those bytes are carried verbatim and never decoded into strings. Pinned in both directions — reverting the recursion fails the row-key cases, removing the details skip fails the verbatim-carriage case.
• The request id is on every refusal (P2). It was sniffed off the cause, which worked only where the cause happened to be the canonical error and silently left every 200-path refusal without one — contradicting this PR's own claim. It is a parameter now: a question the compiler makes each call site answer.

Cost note, since this runs on a continuous poller: a substitution can only come from a lone surrogate escape or an invalid raw byte, so a body with neither cannot contain one. The fast path is two linear scans with no allocation, and the walk never runs on a real page.

The instrument question is answered in the PR body ("Is hand-written validation the right instrument here?"), since it is a decision a future reader will want rather than a thread. Short version: the shape class was a fallthrough defect, not a per-field-versus-typed one, and a typed decode is what produced the first of them; in Go a typed struct buys wrong-type rejection but no required-member or null semantics (null unmarshals into any type and leaves the zero value — the single behavior every null finding came through), so presence checks stay hand-written either way. The substitution class is immune to decoder strictness altogether — encoding/json substitutes U+FFFD and reports success — so it can only be checked after decoding. Schema-driven validation is not available: the Go SDK ships no runtime schema and the generator emits no constraint validation. Generating these arms from the Smithy shape is the real alternative and the right one if a second SDK grows a typed layer; it is a generator change and larger than this PR.

Finding rate across rounds: 1 → 5 → 4 → 2. Rebased onto ebe5fd5 after #922; final head 6d9482e1. make check green.

[Copilot]

🟡 Changes recommended

Nested error-body keys and required row members can still bypass the newly documented validation.

Get a fresh assessment by requesting another Copilot review.

Review details

• Files reviewed: 7/7 changed files
• Comments generated: 2
• Review effort level: Balanced

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 6d9482e129

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[jeremy]

Review threads: 2 resolved (1 fixed, 1 declined with the reasoning in the thread).

• Fixed — the depth rule was not applied to the error bodies (0c580cf). The recursive name walk landed on the page envelope and jsonObject still checked only top-level keys, so a valid 409 carrying {"metadata":{"bad\ud800key":1}} typed as the documented conflict while the contract claimed every depth. Same walk, all four bodies these lanes decode. Both regression cases (409 and 400) fail if it is removed.
• Declined — enforcing a ROW's required members at the decode. True that the generated decoder does not enforce Smithy @required, but a row's shape is a two-lane contract: the poll and push lanes must refuse identical row shapes for §23's byte-identical delivery to hold, and a cable frame never passes through this wrapper. Moving the poll half down here makes the contract three places instead of two rather than removing a copy — the opposite of the digests and the reason, where the wrapper was the only producer and the adapter's copy could be deleted outright. What the decode does hold for rows is the property no other layer can: the strings it decoded are the strings the server wrote, tokens and member names included, at every depth.

The claim the finding read it against was overreaching, so it is narrowed rather than left standing: SPEC §23 now says which members "required members are enforced" covers — the envelope's and the error shapes' — and why a row's are not among them.

Copilot's effort level on this head: Balanced, 7/7 files, 2 comments, no suppressed block. Finding rate: 1 → 5 → 4 → 2 → 2. Final head 0c580cf5.

[jeremy]

Review threads: 2 resolved (2 fixed). Rebased onto 51cdd007 — linear, seven commits, no merges; the inherited Known-defect tracking issues are open red clears with it (make check-known-defect-issues-open passes locally on this head, 7/7 referenced issues open).

• Nested member names in the typed error bodies — fixed in da469131. The walk existed and jsonObject did not call it, which is the failure worth naming: a shared reader that each arm has to remember is not a rule. jsonObject performs it now, so all four bodies these lanes decode get it and no arm can skip it.
• Retry-After dropped on a refusal — fixed in 2c04ead5. Both response-derived fields travel in one struct now (feedResponseFacts) rather than as assignments each site copies by hand — they are lost together and were lost one at a time. Retryable stays false: that is the SDK's verdict about repeating the call, a different question from the delay the server named.

On the instrument question, sharpened against exactly these cases (full answer in the PR body): the real structural weakness of a hand-rolled validator is not per-field checking, it is that it must be wired at each call site — and three times it was not (the walk not called, the request id on one path only, Retry-After on neither). A typed decode does not fix that: in Go, encoding/json has no required-member concept and accepts null into any type, so the presence and nullity checks stay hand-written beside it. What removes the question is making the shared reader the only door, which is what these three commits converged on — jsonObject walks names, requiredString is the one definition of required, feedResponseFacts carries what the response contributes. What would remove it structurally is generating these arms from the Smithy shape, where every required member and type is already declared; that is a generator change and the right follow-up if a second SDK grows a typed layer.

Finding rate: 1 → 5 → 4 → 2 → 2 → 2. Final head 2c04ead5; make check green.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 2c04ead534

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[jeremy]

Review threads: 1 resolved (1 fixed — a decline I reversed).

Both reviewers landed on the same point, and re-reading my own SPEC sentence they were right: it said the exemption is a FeedEvent's details while the code skipped any member named details anywhere in a page. I had declined it as a bounded residue — the interior of a member nothing reads — which was defensible about the behaviour and indefensible about the claim. A PR that ships a sentence its own code contradicts is the worse problem, and the exact version cost about ten lines.

Fixed in 256baa1a: the walk carries each container's member path with array indices elided, so the exemption is events[*].details and items[*].event.details and nothing else. metadata.details, a top-level details, and an additive details on an inbox item are walked and refused; the two declared paths keep their exemption on both lanes. Mutation-checked both ways.

Head 256baa1a.

[Copilot]

🔵 Needs a closer look

The page-wide details exemption violates the new shape-scoped validation contract, and one helper’s documentation is misplaced.

Review details

Suppressed comments (2)

Previously missed (2) — in code that hasn't changed since the last review.

go/pkg/basecamp/event_feed.go:774

• carriesRawDetails is page-wide, so this skips the contents of every property named details, not just FeedEvent.details. The new inbox test demonstrates that an undeclared InboxItem.details can therefore hide substituted or duplicate member names, contradicting SPEC.md:3492-3501, which says the exception belongs to the shape declaring the raw member and all other names are checked at every depth. Track enough path/shape state to skip only a feed event's declared raw details value (and update the additive-details test accordingly).
  go/pkg/basecamp/event_feed.go:946
• This checkPollPage documentation is contiguous with the following feedDecodeError comment and is therefore attached to feedDecodeError, while checkPollPage itself has no adjacent explanation. Move this block to immediately above checkPollPage so each helper's documentation describes the function it precedes.

• Files reviewed: 8/8 changed files
• Comments generated: 0 new
• Review effort level: Balanced

[jeremy]

Copilot's latest review (on 018b44ff, effort Balanced, 8/8 files) generated no new threads but carried two items in its suppressed-comments block, both worth acting on:

• The page-wide details exemption — already fixed in 256baa1a, which landed while that review ran: the exemption is by member path now (events[*].details, items[*].event.details) and nothing else, and the additive-details test was flipped to its inverse as suggested.
• A doc comment attached to the wrong function — real, and fixed in 8dc0dc3d. checkPollPage's block had drifted onto feedDecodeError, contiguous with that function's own comment, so it read as one comment on the wrong helper while checkPollPage had none. Moved, and its last paragraph corrected: it argued against a whole-body byte scan that no longer exists, where the live distinction is that this half reads decoded values and feedBodyWellFormed reads the same page's names.

Flagging the mechanism as much as the fixes: both items were invisible in the thread count, which was zero at the time. Reading the review body is what surfaced them.

Head 8dc0dc3d.

[jeremy]

@codex review

Three pushes landed close together while a run was in flight (018b44ff → 256baa1a → 8dc0dc3d), and the summary has reported a review in progress on the head for ~50 minutes without landing, so this is a nudge rather than a new request. Current head is 8dc0dc3d.

What changed since your last completed review (3c218a68):

• 018b44ff — the name walk is one linear token pass (your quadratic-nesting finding) and refuses a body that names a member twice (your duplicate-members finding).
• 256baa1a — the details exemption is by member path (events[*].details, items[*].event.details) rather than by name, reversing my earlier decline on that thread.
• 8dc0dc3d — a doc comment moved back onto the function it describes.

[Copilot]

🟡 Changes recommended

Path copying makes deep-body validation quadratic and capable of excessive memory consumption.

Get a fresh assessment by requesting another Copilot review.

Review details

• Files reviewed: 8/8 changed files
• Comments generated: 1
• Review effort level: Balanced

[jeremy]

Review threads: 1 resolved (1 fixed).

The walk was quadratic in memory — my own regression from making the details exemption exact two commits earlier. Every object-valued member copied its ancestor path and every copy stayed live on the stack: 655 MiB measured on the 9,000-level body, on a poller that runs continuously.

Fixed in c382bf83 by carrying only what the two exemptions consult: pathLen (named segments leading here) and tail (the last two), both O(1) per push and neither allocating. pathLen is what stops x.items.event matching on its tail alone.

The test now states the bound instead of demonstrating the blowup — total allocation for that body under a 32 MiB ceiling, mutation-checked by restoring the copy (fails at 655 MiB) — beside the 10-second guard that catches the quadratic-reads version.

No depth cap, deliberately, written into the code and SPEC §23 rather than left unsaid: a cap would refuse a page over the shape of a member nothing reads, which is what the additive-member rule forbids; the pass is linear with O(1) state per level, and encoding/json refuses beyond its own nesting limit, so a body deep enough to matter never decodes into a page.

Three defects in this one walk now — quadratic reads, duplicate names accepted, quadratic memory — none a variation of one bypass, all ordinary implementation bugs in new code. That is the honest cost of hand-writing ~120 lines that must be correct about JSON's shape rather than the feed's, and it is now written into the instrument section of the PR body: generating this from the Smithy shape would remove the surface rather than debug it, and these three are the argument for doing that when a second SDK grows a typed layer.

Head c382bf83.

[Copilot]

🔵 Needs a closer look

The raw-details exemption incorrectly applies both lane-specific paths to every poll response.

Review details

Suppressed comments (1)

Previously missed (1) — in code that hasn't changed since the last review.

go/pkg/basecamp/event_feed.go:847

• This predicate exempts both lane paths for every 200 response. Consequently, a valid PollEvents page with an additive items[*].event.details subtree (or a PollInbox page with additive events[*].details) skips that subtree, even though it is not the current operation's declared raw FeedEvent.details; a substituted or duplicate key there is accepted despite the at-every-depth rule. Pass the current collection/lane into the walk and enable only events[*].details for PollEvents or only items[*].event.details for PollInbox.

• Files reviewed: 8/8 changed files
• Comments generated: 0 new
• Review effort level: Balanced

[jeremy]

Copilot's review of c382bf83 (effort Balanced, 8/8 files) opened no threads and carried one item in its suppressed-comments block, which was real and is fixed in b4e85492.

The details exemption accepted both lanes' paths on every 200. So a PollEvents page carrying an additive items[*].event.details subtree skipped it, and a PollInbox page an additive events[*].details — members nothing reads on that response. The exemption belongs to the raw bytes the response actually carries: one path per operation, not two.

The walk takes the lane's collection now rather than a boolean — events, items, or "" for the error bodies, which declare no verbatim member — so it is not possible to exempt the other lane's path. Mutation-checked: restoring the both-paths predicate fails both new cases.

That is the second time a residue in this exemption produced a finding (name-shaped, then lane-shaped), and both times the fix was to make the rule exact rather than to argue the residue was harmless. Noted in the instrument section for what it says about hand-written validation surface.

Also confirming from the other side: Codex completed on c382bf8 with no findings (👍 on the PR), so its last word is clean on the commit immediately before this one, whose only change is this exemption scope plus its tests.

Head b4e85492.

[Copilot]

🔵 Needs a closer look

Both newly wired body-read error paths need regression tests proving transient read failures remain transport errors.

Review details

Suppressed comments (2)

Previously missed (1) — in code that hasn't changed since the last review.

go/pkg/basecamp/event_feed.go:298

• This new parse-error path has no event-feed regression test for its crucial transport-error exception. If markBodyReadFailures is omitted or stops wrapping a partial body read, feedDecodeError turns a transient io.ErrUnexpectedEOF into a statusless non-retryable API error, and the connector terminates instead of retrying. Add a custom response body that fails mid-read and assert PollEvents returns the original transport error.

This issue also appears on line 357 of the same file.

go/pkg/basecamp/event_feed.go:360

• PollInbox wires the body-read marker separately from PollEvents, but this branch is likewise untested. Add the same mid-body io.ErrUnexpectedEOF regression case for this method so an accidental omission cannot reclassify a transient read failure as terminal malformed data.

	httpResp.Body = markBodyReadFailures(httpResp.Body)
	resp, decodeErr := generated.ParsePollInboxResponse(httpResp)
	if decodeErr != nil {
		err = feedDecodeError(factsOf(httpResp), decodeErr)

• Files reviewed: 8/8 changed files
• Comments generated: 0 new
• Review effort level: Balanced

[jeremy]

Copilot's review of b4e85492 (effort Balanced, 8/8 files) opened no threads and carried one finding in its suppressed-comments block, twice — once per lane. It was right, and it is fixed in be93ef50.

The body-read marker had no test. The split that lets a non-decoding page be one verdict rests on markBodyReadFailures: io.ReadAll runs inside the generated parse (#773), so a body that never finished arriving reaches feedDecodeError by the same path as one that failed to decode, and only the marker tells them apart. The two are opposite verdicts — a truncated read is a transport failure the connector retries, a malformed body is the statusless refusal that terminates it — and nothing pinned the difference. An omitted marker would have quietly turned every transient read into a terminal poll failure.

The test serves a truncated body (Content-Length 4096, connection closed mid-JSON) via the existing brokenBodyServer and asserts io.ErrUnexpectedEOF survives as itself rather than becoming an *Error. Both lanes wire the marker separately, so both are covered. Mutation-checked: dropping the marker fails both subtests with not a "api_error" *Error (retryable=false).

Also confirming the other side: Codex completed on b4e8549 with no findings (👍), so the only outstanding word on that head was this one.

Head be93ef50.

[Copilot]

🔵 Needs a closer look

The hand-written recursive JSON validation and changed recovery semantics warrant final human review despite extensive coverage.

Review details

• Files reviewed: 8/8 changed files
• Comments generated: 0 new
• Review effort level: Balanced

[jeremy]

Converged on be93ef50.

• CI green (0 failures), MERGEABLE, rebased linear on main.
• Codex: Completed on be93ef5 with no findings.
• Copilot: reviewed be93ef50 — effort Balanced, 8/8 files, 0 comments, no suppressed block. Its verdict is "needs a closer look… warrants final human review despite extensive coverage", which is a judgment about the change rather than a finding, and it matches what the PR body says about hand-written structural validation.
• 0 unresolved threads, and a quiet probe 22 minutes after the last bot report shows no new activity.
• make check green on this head locally (all seven conformance manifests present).

One note for the record, since it produced a red I chased: an earlier make check failed check-fixture-execution with missing manifests. That was mine — I had two make check runs overlapping in one worktree, and conformance-manifests-reset wipes conformance/manifests before any runner writes, exactly as the Makefile documents. Re-run alone on this head: all seven manifests, EXIT=0. No code involved.

Review rounds on this PR: 1 → 5 → 4 → 2 → 2 → 2 → 1 → 3 → 2 → 1 → 1 → 0 findings, across 11 commits. Every fix was mutation-checked against the real code, and the three declines (strict unknown fields, null reason as malformed, row-level @required) have their reasoning in their threads and in SPEC §23.

Not merging — that is the operator's call, and the approval gate is the only thing outstanding.