Event feed connector: bind the seams to the generated operations (5/5)

[jeremy]

Fifth of the stack — stacked on #897 (inbox lane), which stacks on #778 → #705 → #777 — and it also needs #898 (the spec operations and generated clients). The branch merges #898's head (7c1e8bef) so the adapters compile; until #898 merges, this PR's diff shows #898's changes too. The reviewable change is the last commit, 1cf3551a — the merge commit before it is #898 verbatim (plus a one-paragraph SPEC merge resolution noting that the connector text now absorbs the contract deltas #898's "Wire Operations" section listed). Findings in #898's files belong on #898.

What this is

eventfeed.NewLive binds the connector's two seams to the generated operations through the basecamp.EventFeedService wrapper: Live.Minter() over CreateStreamTicket, Live.Polls() over PollEvents (account lane) or PollInbox (inbox lane), and Live.Connect(opts...) builds the connector over them. One seam call is one fully-governed generated call — the operation keeps its SPEC §7 retry budget, backoff and Retry-After inside the seam — and each adapter maps every §6/§7 outcome onto exactly one seam error kind:

• 401/403 → unauthorized; a parsed Retry-After → throttled(retry_after); a retryable or transport-level failure → transient; context cancellation passes through untouched.
• The feed's matrix: 409 → filter_changed with both digests; 410 → gone with epoch_after_id (0 on the inbox) and the resume URL; a 400 is position_invalid when the server's message says so (Unrecognized position…, the one 400 a since= re-entry recovers) and filter_invalid with the message otherwise; anything else non-retryable → unrecoverable.
• A continuation or resume URL the connector has validated is followed by re-issuing the operation with the query the server wrote into it (PollEventsOptionsFromURL / PollInboxOptionsFromURL), never by fetching the URL raw — so "never construct API paths manually" and "one seam call is one generated call" both hold.

The redirect policy, and why the adapters build their own client

The client's default redirect policy follows a cross-origin hop with the bearer stripped — which still egresses to the foreign origin, and §23 "Continuation and Resume URL Validation" requires none. A per-operation override isn't reachable through the wrapper, so this adds one client option, basecamp.WithCheckRedirect, and NewLive builds the feed's client from the host's Config, TokenProvider and any ClientOptions with a policy installed last that validates every hop's resolved Location under §8's same-origin rule plus downgrade rejection before any request is issued. A refused hop reaches the connector as the seam's redirect_refused kind carrying only the Location's origin: the url.Error net/http wraps the refusal in renders the refused URL whole, which is exactly the value a hostile redirect can put the bearer into, so the typed error alone travels. The host can use Live.Client() for its own refetches — refusing cross-origin redirects is safe for every API call.

This is the Layer-1 302 test the tier-2 family assigned to this layer: TestLivePolls_RefusesACrossOriginRedirectWithZeroEgress has a real generated PollEvents call meet a real 302, and a sentinel listener behind the foreign Location records zero hits. The deferrals that pointed at it (SPEC §23, the README's row-15 note and rows 15/30, fixture 30 and its schema variant, the four spec/tracking-issues.yml entries) are discharged.

Tests

live_test.go drives the adapters against a loopback API through a real basecamp.NewClient: the mint (success, 401/403, 429 with Retry-After, 503, 404, a malformed success), the poll query for every dimension and the page mapping (performed_by_id, verbatim details), continuation-following through the operation, the full error matrix on both lanes, inbox items with their addressing, the cross-origin 302 refusal with zero egress, a same-origin 302 followed with the bearer intact, cancellation pass-through, and NewLive's construction validation. Gates run locally: go test ./pkg/basecamp/... , golangci-lint run, make event-feed-fixtures-check, make doc-constants-check, make check-known-defect-issues-open — all clean.

---

Summary by cubic

eventfeed.NewLive now binds account and inbox connectors directly to generated CreateStreamTicket, PollEvents, and PollInbox operations instead of requiring host-supplied seams. Feed requests answer every 3xx at the transport boundary, so no feed redirect is ever followed; a refused hop reaches feed error handling as redirect_refused with a redacted origin, or with the fixed tokens unparsable when the Location yielded no complete origin and unrecorded when the hop was refused but the origin could not be attributed to the call. Unrelated client requests keep normal redirect behavior.

Behavior

• Generated calls retain retry, backoff, and Retry-After handling; malformed responses, transport failures, and client resilience refusals map to distinct seam errors.
• Continuation and resume URLs are fully parsed, checked against the lane's canonical filters, and reissued through generated operations; next carries a position, resume carries since, and each connector preserves walk state.
• Poll responses validate lane-specific 400, 409, 410, 200, and inbox-item shapes, preserve row ordering, and forward valid UTF-8 details identically on both lanes; an inbox 410 re-enters at since=0, while a feed 410 must name its epoch.
• NewLive rejects invalid account IDs, non-canonical base URLs, and percent-encoded base paths that would misanchor the redirect guard; Connect rejects lane overrides, and each connector receives an independent poll source.
• Integration tests cover both lanes, redirects with zero foreign egress, continuation handling, error classification, detail parity, validation, and timeouts.
• Ruby's CreateStreamTicket remains single-attempt; retry support is tracked in issue Ruby: declared-idempotent mutations such as CreateStreamTicket bypass the retry budget #901.

Migration

• Go Event and WebhookEvent gain PerformedBy; use keyed composite literals.
• Swift WebhookEvent.details changes from String? to JSONValue?.
• Kotlin Event and WebhookEvent gain performedBy, changing positional constructors and destructuring.

^{Written for commit ebce4df. 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-16T17:47:19.531706Z	ebce4df	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.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 1cf3551af9

ℹ️ 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".

[Copilot]

🟡 Changes recommended

Unresolved critical and moderate findings remain in the documentation and live connector.

Get a fresh assessment by requesting another Copilot review.

Pull request overview

This PR binds the Go event-feed connector to generated minting and polling operations, adding redirect enforcement, error mapping, continuation handling, tests, and conformance documentation.

Changes:

• Added NewLive adapters for account and inbox feeds.
• Added same-origin redirect protection and typed error handling.
• Updated generated SDK artifacts, specifications, fixtures, and documentation.

Findings:

• Critical (1 vote): go/README.md:801 is missing context, errors, and fmt imports in its copyable example.
• Moderate (1 vote): live.go:55 can panic for invalid account IDs.
• Moderate (1 vote): live.go:61 should reject invalid UTF-8 before canonicalizing the origin.
• Moderate (1 vote): live.go:185 and :331 incorrectly classify non-retryable responses with Retry-After as throttled.
• Moderate (3 votes): live.go:363 loses unknown fields from raw event details.
• Moderate (1 vote): live.go:123 retries refused redirects before mapping the error.
• Moderate (1 vote): live.go:68 silently discards WithHTTPClient.
• Critical (1 vote): live.go:121, :176, and :319 do not safely classify redirects with missing or malformed Location headers.

File summaries

File	Summary
typescript/tests/services/event-feed.test.ts	TypeScript event-feed service tests
typescript/src/index.ts	TypeScript public exports
typescript/src/generated/services/index.ts	Generated service exports
typescript/src/generated/services/event-feed.ts	Generated event-feed service
typescript/src/generated/path-mapping.ts	Generated route mapping
typescript/src/generated/metadata.ts	Generated metadata
typescript/src/client.ts	TypeScript client
typescript/scripts/generate-services.ts	TypeScript service generator
swift/Sources/BasecampGenerator/Utilities.swift	Swift generator utilities
swift/Sources/Basecamp/Generated/Services/EventFeedService.swift	Swift event-feed service
swift/Sources/Basecamp/Generated/Models/WebhookEvent.swift	Swift webhook model
swift/Sources/Basecamp/Generated/Models/PollInboxResponseContent.swift	Swift inbox response model
swift/Sources/Basecamp/Generated/Models/PollEventsResponseContent.swift	Swift events response model
swift/Sources/Basecamp/Generated/Models/InboxItem.swift	Swift inbox item model
swift/Sources/Basecamp/Generated/Models/FeedEventDetails.swift	Swift event details model
swift/Sources/Basecamp/Generated/Models/FeedEvent.swift	Swift feed event model
swift/Sources/Basecamp/Generated/Models/Event.swift	Swift event model
swift/Sources/Basecamp/Generated/Models/CreateStreamTicketResponseContent.swift	Swift ticket response model
swift/Sources/Basecamp/Generated/Metadata.swift	Swift generated metadata
swift/Sources/Basecamp/Generated/AccountClient+Services.swift	Swift service accessors
swift/README.md	Swift SDK documentation
spec/tracking-issues.yml	Tracking issue updates
spec/overlays/tags.smithy	Specification tags
spec/bc3-route-allowlist.yml	Route allowlist
spec/api-gaps/README.md	API gap documentation
spec/api-gaps/event-feed.md	Event-feed API gap
spec/api-gaps/delegated-events-performed-by.md	Delegated-events API gap
SECURITY.md	Security guidance
scripts/check-idempotency-parity	Idempotency parity check
rust/generator/names.toml	Rust generator naming
rust/basecamp-sdk/tests/services.rs	Rust service tests
rust/basecamp-sdk/src/generated/types.rs	Rust generated types
rust/basecamp-sdk/src/generated/services/mod.rs	Rust service exports
rust/basecamp-sdk/src/generated/services/event_feed.rs	Rust event-feed service
rust/basecamp-sdk/src/generated/routes.rs	Rust generated routes
rust/basecamp-sdk/src/generated/mod.rs	Rust generated module
rust/basecamp-sdk/src/generated/metadata.rs	Rust generated metadata
rust/basecamp-sdk/src/generated/accessors.rs	Rust accessors
ruby/test/basecamp/services/event_feed_service_test.rb	Ruby event-feed tests
ruby/lib/basecamp/generated/types.rb	Ruby generated types
ruby/lib/basecamp/generated/services/event_feed_service.rb	Ruby event-feed service
ruby/lib/basecamp/generated/metadata.json	Ruby generated metadata
ruby/lib/basecamp/client.rb	Ruby client
python/tests/services/test_event_feed_service.py	Python event-feed tests
python/src/basecamp/generated/types.py	Python generated types
python/src/basecamp/generated/services/event_feed.py	Python event-feed service
python/src/basecamp/generated/services/__init__.py	Python service exports
python/src/basecamp/generated/metadata.json	Python generated metadata
python/src/basecamp/client.py	Python client
python/src/basecamp/async_client.py	Python async client
kotlin/sdk/src/commonMain/kotlin/com/basecamp/sdk/generated/services/Types.kt	Kotlin generated service types
kotlin/sdk/src/commonMain/kotlin/com/basecamp/sdk/generated/services/event-feed.kt	Kotlin event-feed service
kotlin/sdk/src/commonMain/kotlin/com/basecamp/sdk/generated/ServiceAccessors.kt	Kotlin service accessors
kotlin/sdk/src/commonMain/kotlin/com/basecamp/sdk/generated/options-param-order.json	Kotlin option ordering
kotlin/sdk/src/commonMain/kotlin/com/basecamp/sdk/generated/models/WebhookEvent.kt	Kotlin webhook model
kotlin/sdk/src/commonMain/kotlin/com/basecamp/sdk/generated/models/Event.kt	Kotlin event model
kotlin/sdk/src/commonMain/kotlin/com/basecamp/sdk/generated/Metadata.kt	Kotlin generated metadata
kotlin/README.md	Kotlin SDK documentation
kotlin/generator/src/main/kotlin/com/basecamp/sdk/generator/Config.kt	Kotlin generator configuration
kotlin/conformance/src/main/kotlin/com/basecamp/sdk/conformance/Main.kt	Kotlin conformance runner
go/README.md	Go SDK documentation
go/pkg/basecamp/webhooks.go	Go webhook services
go/pkg/basecamp/webhook_event.go	Go webhook event model
go/pkg/basecamp/url-routes.json	Go route metadata
go/pkg/basecamp/events.go	Go event services
go/pkg/basecamp/eventfeed/live.go	Go live event-feed connector
go/pkg/basecamp/eventfeed/doc.go	Go event-feed documentation
go/pkg/basecamp/client.go	Go client configuration
go/grouped-client-inventory.yml	Client inventory
conformance/tests/event_feed.json	Event-feed conformance tests
conformance/runner/typescript/runner.test.ts	TypeScript conformance runner
conformance/runner/swift/Sources/ConformanceRunner/Dispatch.swift	Swift conformance runner
conformance/runner/rust/src/operations.rs	Rust conformance operations
conformance/runner/ruby/runner.rb	Ruby conformance runner
conformance/runner/python/runner.py	Python conformance runner
conformance/runner/go/main.go	Go conformance runner
conformance/event-feed/schema.json	Event-feed fixture schema
conformance/event-feed/README.md	Event-feed conformance documentation
conformance/event-feed/fixtures/30-continuation-redirect-cross-origin.json	Redirect fixture
behavior-model.json	Behavior model
AGENTS.md	Repository guidance

Review details

Suppressed comments (8)

go/pkg/basecamp/eventfeed/live.go:57

• ForAccount panics when the account ID contains any non-ASCII digit, so this new constructor can panic instead of returning its documented construction/usage error (for example, accountID = "abc"). Validate the numeric account-ID invariant before calling ForAccount.

	if accountID == "" {
		return nil, usageError("accountID must be non-empty")
	}

go/pkg/basecamp/eventfeed/live.go:64

• CanonicalOrigin lowercases the host with strings.ToLower, which replaces invalid UTF-8 bytes with U+FFFD and can make distinct raw origins share one checkpoint identity. The connector's normal configuration path rejects invalid UTF-8 before canonicalization; NewLive should apply the same check to cfg.BaseURL before computing origin.

	origin, err := CanonicalOrigin(cfg.BaseURL)
	if err != nil {
		return nil, usageError(err.Error())
	}

go/pkg/basecamp/eventfeed/live.go:188

• Error.RetryAfter is populated for every HTTP status, but a parsed delay only means throttled when the outcome is retryable. As written, a non-retryable 404 (or malformed 409/410) carrying Retry-After is reported as MintThrottled even though the generated operation will not retry it; gate this case on apiErr.Retryable as well.

	case apiErr.RetryAfter > 0:
		// A retryable outcome exhausted inside the seam whose last response
		// carried a parsed Retry-After, whatever its status (§6).
		return &MintError{Kind: MintThrottled, RetryAfter: time.Duration(apiErr.RetryAfter) * time.Second, Err: err}

go/pkg/basecamp/eventfeed/live.go:332

• The poll adapter has the same precedence issue: Error.RetryAfter is carried at every status, so a non-retryable 404 (or malformed typed 409/410) with that header becomes PollThrottled instead of PollUnrecoverable. Only classify the delay as throttling when apiErr.Retryable is true.

	case apiErr.RetryAfter > 0:
		return &PollError{Kind: PollThrottled, RetryAfter: time.Duration(apiErr.RetryAfter) * time.Second, Err: err}

go/pkg/basecamp/eventfeed/live.go:180

• This branch treats every non-*basecamp.Error as transient, but generated response parsing returns plain JSON decode errors and EventFeedService returns a plain error for a 2xx response with no decoded body. Those are malformed response shapes, which the mint seam contract classifies as MintUnrecoverable; classifying them as transient causes the connector to retry a permanently invalid server response. Preserve a response-decode/shape distinction from the wrapper or handle it before defaulting non-API errors to transient.

	if !errors.As(err, &apiErr) {
		// A transport-level failure the generated call could not classify
		// (DNS, TLS, a dropped connection): transient, it rides the
		// reconnect cycle.
		return &MintError{Kind: MintTransient, Err: err}

go/pkg/basecamp/eventfeed/live.go:322

• As in mapMintError, this classifies plain generated JSON decode errors and the wrapper's unexpected empty response error as PollTransient. An invalid 200 body is an unexpected response shape and must be PollUnrecoverable; otherwise every poll attempt can retry the same malformed payload indefinitely. Preserve or classify response-shape errors separately from transport failures.

	var apiErr *basecamp.Error
	if !errors.As(err, &apiErr) {
		return &PollError{Kind: PollTransient, Err: err}
	}

go/pkg/basecamp/eventfeed/live.go:127

• This refusal is returned from http.Client.Do, but the generated retry loop treats every such error as a retryable network failure for idempotent PollEvents/PollInbox calls. With the default three attempts, one cross-origin 302 therefore repeats the original poll (and waits) before mapPollError sees redirect_refused, contrary to §23's requirement that this outcome is not retried; the Layer-1 test masks it by setting WithMaxRetries(0). Make the refusal non-retryable inside the generated call or intercept it before that retry loop.

		if len(via) >= 10 {
			return &redirectRefusedError{locationOrigin: locationOrigin(req)}
		}
		if terr := checkContinuation(origin, req.URL.String()); terr != nil {
			return &redirectRefusedError{locationOrigin: locationOrigin(req)}

go/pkg/basecamp/eventfeed/live.go:71

• NewLive accepts arbitrary ClientOptions, but WithHTTPClient is silently lost: NewClient applies it and then unconditionally replaces c.httpClient while constructing its logging client. A caller's custom http.Client (for example its transport, jar, or timeout) therefore does not govern any live seam call or Live.Client, despite being passed here. Preserve the supplied client while installing the feed policy, or narrow the options this constructor accepts.

	opts := make([]basecamp.ClientOption, 0, len(clientOpts)+1)
	opts = append(opts, clientOpts...)
	opts = append(opts, basecamp.WithCheckRedirect(feedRedirectPolicy(origin)))
	client := basecamp.NewClient(cfg, tokens, opts...)

• Files reviewed: 51/90 changed files
• Comments generated: 3
• Review effort level: Lite (auto)

Note

Copilot is running an experiment and ran this review at Lite.

---

💡 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: d471d850ef

ℹ️ 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: 73 resolved (57 fixed, 12 declined with the reasoning in each thread, 4 resolved by #898's merge), 0 open. Head b45b8eb: on #897 over main at a804626 with #912 merged — no merge commits; the diff against #897 is the adapters alone. Copilot re-requested once on 3dd0661 and reporting; Codex reports on every head.

Closed this round: a since re-entry starts a new walk (a position continuation carries the order over); a relative Location resolves against the request before it is reduced to an origin; a page without its events/items member is malformed, not empty (reversing my earlier decline on r4023457209 — the wrapper preserves nil); a doubled slash is refused at the root too; §23 says in every passage that no hop is followed inside the seam.

The six threads the operator's delegate decided are closed: a next continues at a position and never a since, a resume re-enters at a since and never a position (each mismatch a malformed page, no re-entry, no save); a followed URL's filters are not trusted — the request carries the lane's own canonical filters, compared to the URL's as sets, and a URL whose filters differ is refused unissued; a malformed 409 digest is treated as a missing one; an unknown 400 reason is surfaced as undifferentiated. Holding the digest and reason shapes at the wrapper's decode is #915, registered in spec/tracking-issues.yml.

The head consumes the generated error shapes #912 shipped (FeedPositionGoneError / InboxPositionGoneError per lane, each resume held to its fence; FeedRequestError.Reason keying the 400 with the message as the only fallback when a server sends none). The redirect class is closed by one mechanism: a guard composed over the host's transport (basecamp.WithTransportWrapper) answers every 3xx of a seam call at the wire — the call recognized by its exact route beneath the canonical base path, the Location reduced to its origin and stripped with the body before net/http, hooks or logs see it — and leaves every other request untouched; the same guard marks seam bodies so a mid-body failure is transient. Details pass through as the server's bytes on both lanes under the push decoder's rule, with a poll-versus-push parity test. A walk is in strict key order within and across pages, per connector. Connect refuses a WithLane that conflicts with the binding.

Declined:

• Event feed connector: bind the seams to the generated operations (5/5) #899 (comment) — superseded by Account event feed, inbox, and stream tickets: spec operations and generated clients #898, which moved FeedEvent.details to the verbatim document.
• Event feed connector: bind the seams to the generated operations (5/5) #899 (comment) — a redirect loop is a refused hop like any other; redirect_refused keeps the origin as data where poll_failed would carry nothing.
• Event feed connector: bind the seams to the generated operations (5/5) #899 (comment) — superseded on this head: a 409 without both well-formed digests is malformed.
• Event feed connector: bind the seams to the generated operations (5/5) #899 (comment) — superseded on this head: an absent or null collection is malformed (r4026946164).
• Event feed connector: bind the seams to the generated operations (5/5) #899 (comment) — WithHTTPClient is dead in NewClient for every caller on main, not through NewLive; owned by Go: WithHTTPClient is overwritten by NewClient and never takes effect #903.
• Event feed connector: bind the seams to the generated operations (5/5) #899 (comment) — nil tokens panic in NewClient's own auth defaulting for every caller; the check belongs in NewClient.
• Event feed connector: bind the seams to the generated operations (5/5) #899 (comment) — performed_by_id presence is not observable in the typed projection the seam receives.
• Event feed connector: bind the seams to the generated operations (5/5) #899 (comment) — the seam reports §6's retryable verdict for 5xx; §23 names 404/405 and unexpected shapes as the unrecoverable set.
• Event feed connector: bind the seams to the generated operations (5/5) #899 (comment) — under a hook that discards the request context, LocationOrigin degrades to the fixed unparsable token; the strip, the terminal kind and zero egress do not depend on it.
• Event feed connector: bind the seams to the generated operations (5/5) #899 (comment) — the registry rejects rows for spec/api-gaps/ by design; the Ruby: declared-idempotent mutations such as CreateStreamTicket bypass the retry budget #901 promise is registered at the runtime site.
• Event feed connector: bind the seams to the generated operations (5/5) #899 (comment) — a feed operation on another account through Live.Client() gets the same 3xx refusal; that is the guard's policy for feed operations.
• Event feed connector: bind the seams to the generated operations (5/5) #899 (comment) — the server emits valid JSON; a U+FFFD-mutated position draws invalid_position and re-enters through the existing recovery.

Not in this PR: the Ruby runtime's single-attempt path for a declared-idempotent mint — #901; wrapper-level decode validation of the 409 digests and 400 reason — #915; both registered in spec/tracking-issues.yml.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 3f9895ba18

ℹ️ 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".

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: a019cd0ba3

ℹ️ 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".

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: a65e6f4a44

ℹ️ 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".

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 56851cdd32

ℹ️ 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: 6 resolved (2 fixed, 4 declined with the reasoning in each thread).

Declined:

• Reject inbox epochs independently of the resume fence — the inbox lane's fence is the control and checkResumeCursor pins it to since=0 with no position; the undeclared epoch_after_id has no member in the generated 410 shape and no field on the resulting PollError, so it drives nothing. Refusing on its presence would turn a gap that recovers at since=0 into PollUnrecoverable. The general rule — a decode that refuses members a shape does not declare — is the wrapper-decode follow-up (Go event-feed wrapper: validate decoded error bodies at the decode (digest hex, 400 reason enum) #915). The second half of that finding was right and is fixed: the regression case proved "the wrong fence is refused" while claiming to prove "the epoch is refused", and the rows now say which member each is about.
• Verify the next cursor matches the page position — true of a malformed body, and the fifth envelope-shape check of one class. Its general form is Go event-feed wrapper: validate decoded error bodies at the decode (digest hex, 400 reason enum) #915, and a sixth condition inside checkNextCursor grows the adapter's rule count without covering a call site the existing checks miss.
• Preserve caller cancellation across replacing hooks — the premise is a host already outside the hook contract, which breaks the SDK's own per-request state and cancellation everywhere, not only on this seam. The remedy would be a watchdog goroutine per seam call standing guard over that violation; if the contract is worth enforcing, it belongs at the hooks, not in one adapter.
• A path-prefixed base resolves to its parent — the premise does not hold: initGeneratedClient trims the trailing slash and generated.NewClient puts it back (client.gen.go:5852), so directory semantics survive. Measured at the wire, /api/v1 issues /api/v1/99999/events.json, which is what the guard anchors on and what the existing test already asserts.

Fixed:

• Preserve the redirect origin through replacing hooks — a refusal whose per-call record did not survive now reports unrecorded; unparsable keeps §9's meaning, and both are named in the LocationOrigin doc and SPEC §23.
• Reject encoded trailing slashes in the base path — reproduced as an actual egress: https://host/api%2f anchored the guard at /api/ while requests went out as /api%2f/..., and the sentinel behind the foreign Location received the request with its query. NewLive now refuses a base URL whose escaped path is not the canonical encoding of its decoded one.

[Copilot]

🟡 Changes recommended

Resolve the path-prefixed BaseURL and custom HTTP client issues, along with the remaining validation and documentation findings.

Get a fresh assessment by requesting another Copilot review.

Review details

Suppressed comments (6)

go/pkg/basecamp/client.go:265

• NewLive forwards WithHTTPClient, but this construction path only wraps c.httpOpts.Transport; the supplied http.Client's transport and settings are discarded by the unconditional c.httpClient creation immediately below. Custom TLS/proxy/timeout clients therefore have no effect on Live calls; preserve or compose the supplied client while installing the feed guard.

	if c.transportWrapper != nil {
		transport = c.transportWrapper.WrapTransport(transport)
	}

go/pkg/basecamp/eventfeed/doc.go:30

• The implementation intentionally refuses every 3xx from a feed operation, including same-origin redirects (see the guard below and SPEC §23), but this package documentation says only cross-origin and downgraded redirects are refused. Update the description so callers are not led to expect same-origin feed redirects to be followed.

// refuses cross-origin and downgraded redirects before any request is
// issued, which is how the two obligations the seam contracts place on
// whichever adapters back them — zero egress to a foreign redirect target
// and no automatic redirect-following on the poll lane — are met here. A

go/pkg/basecamp/eventfeed/live.go:251

• The PR description says a same-origin 302 is followed with the bearer intact, but this branch answers every feed 3xx—including same-origin—at the transport and TestLivePolls_ASameOriginRedirectIsRefusedToo asserts that behavior. Please reconcile the advertised contract with the implementation/spec (or change the behavior and tests); otherwise users and reviewers get contradictory redirect guarantees.

// mid-body — reaches the seam as a bodyReadError the classifier can tell
// from a body that arrived whole and did not decode.
type redirectGuard struct {
	inner    http.RoundTripper
	basePath string

go/pkg/basecamp/eventfeed/live.go:103

• clientOpts are described as being forwarded, but basecamp.NewClient applies WithHTTPClient and then unconditionally replaces c.httpClient while building its internal client (go/pkg/basecamp/client.go:256-283). Consequently a caller that supplies a custom *http.Client to NewLive silently loses it, including its transport and client settings; preserve that option while composing the feed redirect guard, or explicitly disallow it.

	opts := make([]basecamp.ClientOption, 0, len(clientOpts)+1)
	opts = append(opts, clientOpts...)
	// The guard is anchored at the base URL's path, so that path must be

go/pkg/basecamp/eventfeed/live.go:719

• The conformance schema requires epoch_after_id to be non-negative, but the generated response type is an int64 and does not enforce that constraint. With a malformed 410 body containing epoch_after_id: -1 and resume?...since=-1, checkResumeCursor succeeds here and the connector emits PollGone, allowing recovery to reissue an invalid negative cursor instead of rejecting the response as unrecoverable.

		}

go/pkg/basecamp/eventfeed/live.go:888

• performed_by_id is required by the poll-row contract, but the generated *int64 field decodes both an omitted key and an explicit JSON null to nil. This condition therefore accepts a row that omits the required key (when the other fields are valid) and delivers it; preserve field presence in the wrapper decode or validate the raw response before converting it to FeedEvent.

}

// eventFromFeed maps the wrapper's FeedEvent onto the connector's Event. The

• Files reviewed: 16/16 changed files
• Comments generated: 1
• Review effort level: Lite (auto)

Note

Copilot is running an experiment and ran this review at Lite.

[Copilot]

🔵 Needs a closer look

Unresolved moderate findings affect client-option propagation, redirect guarding, malformed-response handling, and poll-row validation.

Review details

Suppressed comments (9)

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

go/pkg/basecamp/client.go:179

• The PR description and commit message say this change adds basecamp.WithCheckRedirect, but the codebase defines no such option; the only new option here is WithTransportWrapper. Consumers following the stated API cannot compile, and a transport wrapper is not an equivalent public redirect-policy hook. Please add the promised option or update the PR contract/description to the actual API.

SPEC.md:4248

• This hunk still introduces the redirect_refused matrix with the condition “whose Location fails” while the updated rule below says the adapter refuses every feed 3xx, including same-origin Locations. That leaves the normative error mapping internally contradictory; make this first matrix sentence describe the transport-boundary refusal of any feed redirect.

-- its origin — or, in its place, the fixed token `unparsable` when the Location yielded
-- no complete origin (§9) and `unrecorded` when the hop was refused but the adapter
-- could not attribute the origin to the call; the refusal and its zero egress never
-- depend on that attribution, and the two tokens are never interchanged, since one
-- reports what the server sent and the other what the adapter failed to keep

go/pkg/basecamp/client.go:264

• NewLive forwards clientOpts, but WithHTTPClient is silently discarded by NewClient: the wrapper is applied to c.httpOpts.Transport here and NewClient then constructs a new http.Client instead of using the supplied one. Consequently a Live binding created with the documented custom client loses its transport, timeout, proxy, and redirect configuration. Please preserve/clone the supplied client's settings while installing the feed guard, or reject this option explicitly for NewLive.

	if c.transportWrapper != nil {
		transport = c.transportWrapper.WrapTransport(transport)
	}

go/pkg/basecamp/eventfeed/live.go:525

• A generated 200 response can omit position (or set it to null), and the wrapper decodes that as an empty string. This code returns it as a successful PollPage; the connector happens to reject it later, but direct Live.Polls() consumers receive a malformed page and the PollSource contract says unexpected shapes are PollUnrecoverable. Validate a non-empty position here before returning the page for both lanes.

	if len(page.Events) > 0 {
		if page.Events[0].Key() <= p.lastKey {
			return PollPage{}, &PollError{Kind: PollUnrecoverable, Err: fmt.Errorf("eventfeed: the continuation's first row %d does not follow the previous page", page.Events[0].Key())}
		}
		p.lastKey = page.Events[len(page.Events)-1].Key()
	}
	return page, nil

go/pkg/basecamp/eventfeed/live.go:135

• With a path-prefixed base URL, this client is initialized with serverURL ending in /api/v1 because Client.initGeneratedClient trims the slash. The generated operations resolve their ./{account}/... paths relative to that URL, so /api/v1 is treated as a file and the actual request becomes /api/{account}/events.json; the guard anchored at /api/v1/ then does not recognize the feed call and a 3xx can be followed. Initialize this feed client with a copied config whose BaseURL has exactly one trailing slash (without mutating the host config) so generated calls and the guard use the same prefix.

	client := basecamp.NewClient(cfg, tokens, opts...)

go/pkg/basecamp/eventfeed/live.go:455

• io.EOF is also the error returned by json.Unmarshal for an empty 200 response. Because the generated response parsers unmarshal successful JSON bodies before EventFeedService can validate them, an empty/malformed success reaches this helper as plain io.EOF and is classified as transient, causing mint/poll retry or reconnect behavior instead of the documented unrecoverable malformed-response outcome. Body-read failures are already distinguished by bodyReadError/io.ErrUnexpectedEOF; do not treat plain io.EOF as transport failure here.

	if errors.As(err, &bodyErr) || errors.Is(err, io.ErrUnexpectedEOF) || errors.Is(err, io.EOF) {
		return true

go/pkg/basecamp/eventfeed/live.go:900

• The poll contract requires performed_by_id to be present on every row (null is the direct-action value), but the generated *int64 field collapses an absent member and JSON null to the same nil value. This check therefore accepts a row that omits performed_by_id and delivers it as valid on both feed and inbox lanes, despite the adapter claiming to validate required members. Preserve presence when decoding the generated response (or validate the raw row) before applying the nil/null rule.

	if fe.ID < 1 || fe.BucketID < 1 || fe.CreatorID < 1 || fe.RecordingID < 1 ||
		(fe.PerformedByID != nil && *fe.PerformedByID < 1) ||
		fe.Kind == "" || fe.EventType == "" || fe.Action == "" || fe.CreatedAt.IsZero() {

go/pkg/basecamp/eventfeed/live.go:580

• FeedEvent is decoded with encoding/json before this conversion, so any unknown poll-row members are discarded. That means a response containing push-only actor_type or visible_to_clients is accepted and delivered, although the event-feed schema requires both fields to be absent on poll rows (the detail-parity test currently sends exactly such a row). Validate the raw row shape or preserve member presence in the service response so the adapter rejects this class of malformed page.

	events := make([]Event, 0, len(page.Events))
	for _, fe := range page.Events {
		ev, err := eventFromFeed(fe)

go/pkg/basecamp/eventfeed/live.go:65

• clientOpts are passed to NewClient, but WithHTTPClient does not survive that call: NewClient applies it and then unconditionally replaces c.httpClient with a new client (client.go:269). A NewLive caller's custom transport/proxy/TLS settings, cookie jar, timeout, or redirect behavior is therefore silently ignored, contrary to this pass-through contract. Preserve the supplied client's settings while installing the feed guard, or explicitly reject this option.

// NewLive builds the seams for accountID on lane over a basecamp.Client
// constructed from cfg, tokens and clientOpts — with the feed's redirect
// guard installed last, so it wins over any transport wrapper in clientOpts
// (the host's transport itself, from WithTransport, is what the guard
// composes over). The client's base URL is the connector's origin: the checkpoint key's and the

• Files reviewed: 16/16 changed files
• Comments generated: 0 new
• Review effort level: Lite (auto)

Note

Copilot is running an experiment and ran this review at Lite.