Skip to content

feat(connectors): execute a connector's declared retryConfig and requestTimeoutMs at the one platform fetch site - #19388

Merged
os-sam merged 12 commits into
mainfrom
claude/issue-18975-connector-retry-timeouts
Sep 20, 2026
Merged

os-sam merged 12 commits into
mainfrom
claude/issue-18975-connector-retry-timeouts

Conversation

@os-sam

@os-sam os-sam commented Sep 20, 2026

Copy link
Copy Markdown
Collaborator

Fixes #18975

Clause-②: yes

Ruling of record: comment 5729479418 — director seat summon #24, batch #159 item 5, maintainer 「同意」 2026-09-18T11:42Z, letter 实现. Not re-adjudicated here. The spec declarations do not move: the connector schema keeps every key, every bound and every default it had.

STEP ZERO — where the platform-owned connector fetch actually lives

Measured before anything was written, on origin/main = be7382d77e (2026-09-20T14:05Z), re-confirmed after the merge to ada70122.

The ruling's wrapper already exists. packages/spec/src/shared/resilient-fetch.ts — exported as resilientFetch from @objectstack/spec/shared — is the platform's outbound-HTTP call: it already gave every attempt a 30s timeout and a bounded exponential backoff with jitter and Retry-After handling. So "land the wrapper there once, no gateway, no new subsystem" was satisfiable without building anything new.

What the connectors do with it, measured per package:

package the one call its handler makes before this PR
connector-rest rest-connector.ts request() resilientFetch(...)
connector-slack slack-connector.ts callSlack() resilientFetch(...)
connector-openapi openapi-connector.ts request() a naked fetch — unbounded, never retried
connector-mcp handlers to client.callTool no fetch at all; the MCP SDK owns the transport, with a hardcoded 30s timeout

So the fetch site is one shared wrapper plus one bypass to fold in, not several independent paths and not something that needed restructuring. The gap was never "there is no wrapper" — it was that the wrapper could not express the declared policy, and that no authored value could reach it: ConnectorProviderContext carried none of the three keys.

Holder check at claim time: across all 30 open PRs, zero touch any file matching connector, resilient-fetch or service-automation (lit control on the same scan: packages/spec hits 9, 22 and 9 files on PRs #19364 / #19363 / #19335).

What landed

One wrapper, extended by exactly what was missing. ResilientFetchOptions gains strategy, backoffMultiplier, maxDelayMs, jitter and retryOnNetworkError. Each defaults to the behaviour the wrapper already had, so a caller that passes none is byte-identical to before.

One mapping. connectorFetchOptions() (packages/spec/src/integration/connector-fetch-policy.ts) is the single place a connector's declared policy becomes wrapper options — one execution site, not one per connector package.

One contract widening. ConnectorProviderContext gains retryConfig, connectionTimeoutMs and requestTimeoutMs, read-only and resolved: the materializer parses retryConfig through RetryConfigSchema, so a factory reads real values instead of re-deriving the schema's defaults. The policy also joins the instance signature, so editing it re-materializes the connector instead of leaving the old policy serving until restart.

The built-in HTTP providers honour it by construction. rest and openapi pass the context's policy into their connector builders.

RESOLVED at 2026-09-20T17:02:28Z — the work is on the branch; the push simply lagged the report by about two minutes. Kept in full rather than deleted, because the sequence is worth more than the tidy version. At 17:00:04Z the remote tip was 4432f967e3 with an 18-file diff and ⛔ none of the three files below; the dev's report already described them at head 5911c8cf. The seat held the review and struck this paragraph. At 17:02:28Z git ls-remote reports the tip as 5911c8cf664f534823d598c801f852c346be80755911c8cf docs(spec): the connector header and SYNC_ARCHITECTURE describe the implemented behaviour sitting on top of 4432f96721 files, all three present. ⇒ the 17:00Z reading was true when taken and is now superseded; the report was accurate about content and early about the push. ⭐ The rule that survives, and it is not 「the check was wasted」: git ls-remote is the authority and the PR object is not — while this was being checked the PR object was still serving the stale 18-file count. ⛔ A conclusion drawn from a summary face has a shelf life; one drawn from the ref does not.

The teaching text #18794 narrowed is corrected to describe the implemented behaviourpackages/spec/docs/SYNC_ARCHITECTURE.md in five places, plus the connector.zod.ts header TSDoc it renders from (content/docs/references/integration/connector.mdx follows by gen:docs, ⛔ never hand-edited). Those passages asserted the keys were 「declared but currently unimplemented」 and that ConnectorProviderContext could never carry them; both are now false. This is the ruling's third bullet, ⛔ not an absorption of #18794. ⭐ health.circuitBreaker and connectionTimeoutMs are explicitly kept named as still inert in every corrected passage.

⚠️Found by hand, ⛔ not by the drift bot — and it is the bot's own declared blind spot doing exactly what it warns about. SYNC_ARCHITECTURE.md states the rule by its inputs, so it shares no identifier with the emitter this diff changed and ⛔ no run could ever have listed it. The bot's three named pages were each hand-verified and two were ACCURATE and left untouchederror-catalog.mdx's no_retry is the API error-envelope enum from api/errors.zod.ts, a different enum this diff never touches, and jobs.mdx is job.retryPolicy from shared/retry-policy.zod.ts, likewise untouched. The third was accurate too, and it is the one that falsified the code.

Two interpretive calls, both stated rather than assumed:

  • 🔴 maxAttempts counts TOTAL calls, the first included — the contrast content/docs/automation/flows.mdx already draws against maxRetries, and it is what corrected this implementation. ⚠️ Replaced by the seat 2026-09-20T17:00Z. This bullet previously read 「counts retries, not total calls」, reasoning from min(0) and a shared/retry-policy.zod.ts comment the dev has since said it over-read (that comment is about opt-in vs opt-out defaults, ⛔ not the counting base). The first reading reached a pushed commit; it was falsified by a documentation page, and the implementation was changed to match the page — ⛔ not the other way round. New pin: maxAttempts: 3 must make three calls, ⛔ not four, the case that tells the two readings apart. Ablation: restoring + 1 turns 3 mapping tests red.
  • maxDelayMs is applied after jitter. Jitter is additive, so capping first would let a delay land up to 99ms above the declared ceiling.

The seat's assumption 4 is falsified, and that is the one thing the ruling asked me to report rather than invent

AbortSignal.timeout is available (Node 22 or newer, which the root engines field pins; already used at packages/drivers/driver-turso/src/turso-driver.ts). The connection-vs-request distinction is not.

A connector's call is a WHATWG fetch, whose only cancellation surface is one AbortSignal over the whole operation; nothing in that interface observes the connection phase separately. Bounding time-to-response with connectionTimeoutMs would kill a slow-but-connected upstream that the author meant to allow with a large requestTimeoutMs — breaking the very promise the key makes. Node's undici exposes connectTimeout through a custom dispatcher, which is Node-only and a new subsystem underneath every connector: the same ruling forbids it.

So connectionTimeoutMs is carried onto ConnectorProviderContext (a custom provider on a transport that can separate the phases may honour it) and not enforced by the platform. packages/spec/liveness/connector.json keeps that one row dead, with the measurement written into it, and a pin in connector-fetch-policy.test.ts goes red if anyone aliases it onto timeoutMs. Nine of the ten rows flip, not ten. The tenth is owed a second, narrower ADR-0049 decision — see the acceptance notes.

Verification

Full pipeline at the final commit 956fdb10.

Gate family, re-derived in this worktree from the real changed paths (node scripts/pm/dispatch-gates.mjs --commands --repo objectstack-ai/objectstack), every command run with its exit code captured before any pipe, then reconciled with --ran:

dispatch-gates --ran: 86 derived family(ies) accounted for — 83 run, 3 NOT-MEASURED (3 DERIVED from a recorded exit 3).

The three NOT MEASURED are PREREQUISITE NOT MET, not findings: check:dual-build-cjs-loads and check:type-check-debt both need a whole-repo build closure (CI builds it before those steps), and check-plugin-teardown-shape.mjs --self-test cannot reach its commit-pinned positive control on a shallow clone. All three are CI's to run.

Tests (all on the merged tree):

@objectstack/spec                503 files / 14711 tests  passed
@objectstack/service-automation  140 files /  1676 tests  passed
@objectstack/connector-openapi     4 files /    34 tests  passed
@objectstack/connector-rest        4 files /    24 tests  passed
@objectstack/connector-mcp         3 files /    23 tests  passed
@objectstack/connector-slack       3 files /    10 tests  passed

typecheck green for all six. pnpm --filter @objectstack/spec check:generated: 15 of 15 artifacts up to date (api-surface/ and export-origins/ regenerated after a real build — the two new exports plus the ResilientFetchOptions re-export).

Lint — measured whole, not narrowed. eslint . --no-inline-config --format json over the repo root: 6939 files, 0 errors, 0 warnings, exit 0.

Ablation — both pins proved able to fail, through scripts/ablation-replace.mjs (anchor must hit, blob hash must move, restore proved against HEAD):

mutation result
connector-fetch-policy.ts: invert the early return so a declared retryConfig is never mapped 9 of 10 mapping tests RED, restored blob == HEAD
rest-provider.ts: stop passing ctx.retryConfig into the connector 4 of 5 provider retry pins RED, restored blob == HEAD

The pins assert call counts and delay sequences, not the presence of a field: a pin on the def's retryConfig could not have failed here, because the key was already storable and served back before any of this landed. The sharpest one is the narrowing case — an authored retryableStatusCodes: [429] must leave a 500 unretried, which only passes if the authored list is the one executed (500 is retryable under both the wrapper's own default and the schema default).

Acceptance notes

To file (class (c), an authoring trap that survives this PR): connector.connectionTimeoutMs still parses, still stores, is still served back by /meta/connector, and is enforced by nothing — for the measured reason above, which is a property of fetch, not an omission here. It now needs a decision this card's ruling did not answer: retire it, or re-describe it as something the platform can enforce (its sibling requestTimeoutMs already is). Reproduction: declare a connectors: entry with provider: 'rest' and connectionTimeoutMs: 1000, point providerConfig.baseUrl at an endpoint that takes 5s, and dispatch the request action — it completes normally. Dedupe words: connectionTimeoutMs declared unenforced · connector connect timeout AbortSignal fetch · ADR-0049 connectionTimeoutMs second decision · connector.json connectionTimeoutMs dead row · retire or redescribe connect timeout.

Fixed in place, declared here rather than filed: connector-openapi's generated actions went through a naked fetch — unbounded, never retried, and the one built-in HTTP path an authored policy could never reach. It is the same defect on the same measured site as this card's, the fix is mechanical and its shape was already pinned by two sibling connectors, no other open PR holds the file, and it adds no new gate family. Those actions now go through the same wrapper as connector-rest and connector-slack. Evidence: openapi-connector.ts createOpenApiConnectordoFetch(url, init) became resilientFetch(url, init, fetchOptions); @objectstack/connector-openapi 34 tests still pass.

Noted, not filed:

  • ConnectorProviderContext.icon and .type are set by the materializer and read by none of the three shipped provider factories, so an authored icon: or type: on a declarative instance never reaches GET /api/v1/automation/connectors. Already recorded per-row in packages/spec/liveness/connector.json, with what is owed already stated there. Next toucher: whoever adds or changes a provider factory.
  • connector-slack ships no provider factory, so nothing authored can reach it — only the plugin door, hand-wired by its host. Not silent: an unknown provider is a loud, named boot failure that lists the installed ones. Next toucher: whoever adds a slack provider key.
  • The connector-rate-limit-config-removed comment in packages/spec/src/conversions/registry.ts says retryConfig and the timeouts "are live". It was wrong when written (the ledger's retryConfig.strategy row corrects it by name), and this PR makes nine tenths of it accidentally true. A stale comment, no behaviour. Next toucher: whoever edits that conversion entry.
  • health.circuitBreaker's sub-keys are dead on the same schema and the same ADR-0049 worklist. Out of this card's scope by the card's own words ("本卡只管这三个"), and its teaching text was already stanched by [finding] connector.zod.ts:44-47 的 TSDoc 仍逐字教着「限流上游的解法是 retryConfighealth.circuitBreaker」—— 那是 PR #18979 刚在 SYNC_ARCHITECTURE.md 收掉的同一句话的**源头**,且有一个生成的下游 #18983. Next toucher: the next ADR-0049 connector sweep.

Round 2 — both at-tier FAIL items fixed, at head 4a9b3480f2

Review record 5751411253 FAILed this PR on two items. Both are fixed, pinned and ablated; ⛔ nothing else was widened, and the two optional notes the review offered (the .describe('Maximum retry attempts') counting-base wording, and the pre-existing Retry-After-on-any-retryable-status and unbounded-body-read observations) were deliberately not acted on.

FAIL 1 — the openapi routing is now pinned

The review's ablation proved this PR's own justification false: 「shape already pinned by two sibling connectors」 did not hold for this file — restoring the naked fetch left 34/34 openapi tests green.

Two cases added in openapi-provider.test.ts, through the factory with retryConfig on ctx, mirroring rest-provider.test.ts: scripted fetch [503, 200], {strategy: 'fixed_delay', maxAttempts: 2, initialDelayMs: 100, retryableStatusCodes: [503], jitter: false}, asserting exactly 2 upstream calls and a 200. The review's own ablation reproduces on the same blobs (a0172843ccb922b1470c9170) and now turns the retry pin RED where it measured 34/34 green; restore proved blob == HEAD.

⚠️ Precision, stated rather than glossed: only 1 of the 2 new cases discriminates. The narrowing case cannot — with a naked fetch nothing retries, so 「1 call」 is what both trees produce. It is kept for what it pins, ⛔ not as a revert detector.

FAIL 2 — maxDelayMs is now a maximum. Route (a), and the reason

The review offered two routes. Route (a) was taken: a Retry-After longer than maxDelayMs now ends the retry loop and returns the response.

Why (a) and not (b): this card exists to make a declaration equal its enforcement, so making 「Maximum retry delay in ms」 true beats documenting an exception to it. Route (b) would have left a key whose name says maximum with an upstream-controlled way past it — which is the exact shape #19410 was filed for earlier today.

The three alternatives, and why returning wins: sleeping it out makes the key not a maximum; retrying sooner than asked is the abuse Retry-After exists to prevent; returning hands the caller the real status and its header. Only a Retry-After can reach that branch, because backoffMs caps its own output — so the review's jitter-cap lit control is untouched.

Pinned at maxDelayMs: 1000 + retry-after: 3600 → 1 call, the 429 returned, sleep never called, with a control that a Retry-After within the ceiling is still honoured and still retried. Ablation: deleting the guard (bab28fcb1bde9564b3126ef7) turns it RED; restore proved blob == HEAD.

⇒ the retryConfig.maxDelayMs ledger note and the changeset sentence were both corrected, so ⛔ no artefact still claims a ceiling the code ignores.

Verification at the pushed head

Gate family re-derived on the pushed tip and again on the fix commit — identical 109 families both times: 107 green / 2 NOT-MEASURED / 0 red / 0 unrun. The two NOT-MEASURED are the shallow-clone self-test and check:dual-build-cjs-loads needing the full build closure — ⛔ exit 3 is a prerequisite, ⛔ not a red. check:generated 15/15 with no regeneration owed (route (a) moved no .describe(), so connector.mdx did not move). Tests: openapi 36 (was 34), rest 26, slack 10, spec wrapper+mapping 29. Full-repo lint 6,945 files, 0 errors, 0 warnings.

⚠️ Overtaken and corrected 2026-09-20T20:37Z — the merge WAS taken. The paragraph below was true when written and is spent; kept struck rather than deleted, because the reason it gave is the reason round 3 exists.

origin/main has moved 12 commits since this branch's single merge of record (a88a9733). It was deliberately not re-merged: the at-tier record is keyed to a head sha, and a fresh merge creates a head the record does not name. The merge is taken when the review is clear, ⛔ not while it is in flight.


Round 3 — origin/main merged, the head re-reviewed, and the base-drift cost paid

Once round 2 cleared, the base was 21 commits stale and the landing needed a fresh CI run, so origin/main 576d5df6 was merged once as 13987f1b. ⛔ No rebase, ⛔ no amend, ⛔ no force-push, ⛔ no empty commit.

The merge is provably automatic: 13987f1b has exactly two parents (4a9b3480, 576d5df6), and git merge-tree --write-tree 4a9b3480 576d5df6 yields tree 8350d10d, which equals 13987f1b^{tree} ⇒ no hand resolution existed. The two sides are disjoint — this PR 22 files, main 83, intersection 0 (lit control: both lists non-empty).

This PR's own delta did not move: 22 files, +1197/−132, the same file list as before the merge.

Neither guard was undone. Both blobs are byte-identical to round 2, and both ablations still give exactly 1 red — the openapi routing pin (35 passed) and maxDelayMs bounds a Retry-After by STOPPING (18 passed, the within-ceiling and jitter-cap controls green by name). Restores proved blob-equal to HEAD.

Gates: 110 derived / 109 green / 1 NOT-MEASURED / 0 red / 0 unrun. One family appeared with the incoming commits (check:issue-citations, wired in by 5a5e710f); a true set comparison against a round-2 derivation gives only-in-r3 = that one, only-in-r2 = none. The NOT-MEASURED is check-plugin-teardown-shape --self-test at exit 3 — a shallow-clone prerequisite, ⛔ not a red. check:generated 15/15 with no regeneration owed, the migration registry included after main deleted ten entry files. Whole-repo lint 6,943 files, 0 errors, 0 warnings.

Honest cost, stated because the reviewer found it and the dev's number alone would have hidden it: the first gate reading on a turbo-cache-restored closure was 108 green + 2 exit 3, not 109 + 1 — check:type-check-debt refused on a dist whose mtimes predated its sources, and reached exit 0 only after a real rebuild. Same conclusion, named with what it cost.

CI on 13987f1b: 35 names, 33 success + 2 skipped, 0 failing, 0 pending — all six Test Core shards green, including the 2/6 shard that was red before. ⚠️ A green re-run corroborates that the earlier red was not this PR's; it ⛔ does not prove it. The load-bearing evidence is still the mechanism filed as #19424 (80 × 0.25 s = 20 s against an observed 20,999 ms, the same titled assertion passing in 299 ms in the same run).

And one thing this PR previously could not establish, now established from a different door: GET /branches/main/protection answers 403, but GET /rules/branches/main answers 200 and lists seven required contexts — Test Core among them, all seven success here. A 403 on one endpoint is a fact about that endpoint, ⛔ not about the question.

Round-3 at-tier review: PASS — record 5752480809, keyed to this head. check-clause2-carriers --pair 19388 exits 0 with zero ✗ rows, run after the record existed.


Generated by Claude Code

…uest timeout

WIP: wrapper knobs, the single mapping, the provider context widening and the
three built-in HTTP providers.

Claude-Session: https://claude.ai/code/session_01HnRAeVTLJevtQ5iCPX6JSm
Co-authored-by: Claude <noreply@anthropic.com>
…ntry

`connectorFetchOptions` returns it, so a consumer writing an un-annotated
`export const opts = connectorFetchOptions(...)` hit TS2883.
check:entry-nameability named it; one declaration, re-exported.

Claude-Session: https://claude.ai/code/session_01HnRAeVTLJevtQ5iCPX6JSm
Co-authored-by: Claude <noreply@anthropic.com>
@github-actions github-actions Bot added size/l documentation Improvements or additions to documentation tests tooling labels Sep 20, 2026
@github-actions

github-actions Bot commented Sep 20, 2026

Copy link
Copy Markdown
Contributor

📓 Docs Drift Check

This PR changes 4 package(s): @objectstack/connector-openapi, @objectstack/connector-rest, @objectstack/service-automation, @objectstack/spec, touching 31 documentable anchor(s). ⚠️ 7 changed file(s) yielded no anchor (packages/spec/api-surface/integration.json, packages/spec/docs/SYNC_ARCHITECTURE.md, packages/spec/export-origins/integration.json, …), so the pages documenting them are NOT COVERED by this run — this is not a clean bill of health for those files.

3 hand-written doc(s) NAME something this change touched and may need an implementation-accuracy re-verification:

  • content/docs/api/error-catalog.mdx (via no_retry (literal, a string literal in backoffMs; a string literal in resilientFetch))
  • content/docs/automation/flows.mdx (via backoffMs (symbol, a top-level function), backoffMultiplier (symbol, a field of interface DelayPolicy; a field of interface ResilientFetchOptions), maxDelayMs (symbol, a field of interface DelayPolicy; a field of interface ResilientFetchOptions), retryConfig (symbol, a field of interface ConnectorFetchPolicy; a field of interface ConnectorProviderContext; a field of interface DeclaredConnectorItem; a field of interface OpenApiConnectorConfig; a field of interface RestConnectorOptions), retryDelayMs (symbol, a top-level function), retryConfig (literal, a string literal in reconcileDeclaredConnectors))
  • content/docs/automation/jobs.mdx (via backoffMs (symbol, a top-level function), backoffMultiplier (symbol, a field of interface DelayPolicy; a field of interface ResilientFetchOptions))
What this run could not see
  • 7 changed file(s) yielded no anchor (packages/spec/api-surface/integration.json, packages/spec/docs/SYNC_ARCHITECTURE.md, packages/spec/export-origins/integration.json, …) — pages documenting those are invisible to this run
  • 2 name(s) were too generic to anchor anything (single lowercase words)
  • the SDK route bridge reached 60 of 215 client-bound route-ledger rows — the other 155 have no registrar path: tail to select them, so pages documenting THEIR client methods cannot appear above, on this or any run. Of those 155: 0 are remediable by widening that discovery convention (an in-repo file declares the path; the convention did not scan it); 55 are structural — on a ledger where NOT ONE row is declared in-repo, so no discovery change reaches them at any price; 100 are undecided (no in-repo declaration, on a ledger that has other in-repo registrars — absence and an unreadable spelling are not distinguishable here). The rows themselves: node scripts/docs-audit/affected-docs.mjs --bridge-coverage
  • a page that states a rule by its inputs shares no identifier with the emitter that implements the rule, so an emitter-only diff cannot list it — not on this run and not on any run. Measured on fix(driver-sql): emit varchar(maxLength) for a text field a declared index keys on #11430: content/docs/protocol/objectql/types.mdx documents the text-family column mapping by the ObjectQL type names it maps FROM (text / textarea / html) while the diff changed createColumn; it went unlisted, and it was the page that diff falsified, in four places. No shared token exists to detect this on, so a rule your change carries has to be re-read by hand in the pages that restate it.
  • a key NAME is not a key, so the hand re-read the line above prescribes can land on the wrong schema. The same spelling is authorable on one governed type and a [REMOVED] tombstone on another for each of active, aria, joins, objects, template, tools and version (censused on [finding] tools is a key on BOTH AgentSchema (tombstoned, dead) and SkillSchema (live, cloud-attested), so a name-based search attributes skill examples to the agent key — it produced a false stop-the-line alarm on PR #19059 #19093 over the liveness ledger's governed types, top-level keys); nothing in a search result distinguishes the two, so a grep hit on a LIVE example reads as evidence about the DEAD key. Measured on fix(spec): the agent.tools liveness row says dead — it claimed live on a key the schema tombstoned #19059: content/docs/ai/agents.mdx was reported as contradicting the agent.tools tombstone over its tools: example at :161, which is inside the defineSkill({ block opened at :155 — the page was already correct. Settle ownership by PARSING the value against both schemas, never by the name: that literal PASSES SkillSchema, and as an AgentSchema it FAILS at tools with the tombstone prescription. ⛔ These names are not the whole class — a key retired through a .strict() guidance map leaves no tombstone in the walked shape and none of them here (tool.category, live as AIToolDefinition.category).

Coarse fallback — 136 page(s) merely mention a changed package (the pre-#9192 predicate, kept for the deliberately-wide backstop): node scripts/docs-audit/affected-docs.mjs --json f9e16d856b278d5cd45807991c7a3b44db511698packageMentionDocs.

Which tree this was computed on

This run read content/docs from 4bd3b8be1f2a8a4619abbcc70bd5d0cc249b5c6b — the merge of head 13987f1b69e5ca0b5902d639ab8ff8fe09b6b14a into base f9e16d856b278d5cd45807991c7a3b44db511698, which is what actions/checkout gives a pull_request run. Not the PR head.

A worktree cut from an older main holds a different content/docs, so re-deriving there can legitimately return a different list — that is a different tree, not a wrong row. To answer on the same tree:

# while this PR is open — GitHub drops the merge commit once it closes
git fetch origin 4bd3b8be1f2a8a4619abbcc70bd5d0cc249b5c6b && git checkout 4bd3b8be1f2a8a4619abbcc70bd5d0cc249b5c6b
# afterwards, rebuild it from the two parents, which stay fetchable
git fetch origin f9e16d856b278d5cd45807991c7a3b44db511698 13987f1b69e5ca0b5902d639ab8ff8fe09b6b14a && git checkout -B drift-repro f9e16d856b278d5cd45807991c7a3b44db511698 && git merge --no-ff 13987f1b69e5ca0b5902d639ab8ff8fe09b6b14a

node scripts/docs-audit/affected-docs.mjs --json f9e16d856b278d5cd45807991c7a3b44db511698

⚠️ That checkout carried uncommitted changes, so the commit above does not fully identify what was read.

Advisory only, and a precision-first one (#9192): a page is listed because it names a
symbol, wire route or SDK method this diff touched — not because it mentions a changed
package. Each row says which anchor put it there, so a wrong row is reportable rather than
merely annoying. To re-verify, run the docs-accuracy-audit workflow scoped to these files:
node scripts/docs-audit/affected-docs.mjs f9e16d856b278d5cd45807991c7a3b44db511698 → pass the list as
args.docs, on the commit named under Which tree this was computed on.

The Docs Drift Check pointed at content/docs/automation/flows.mdx, which
documents this key's counting base for authors and contrasts it with
maxRetries. The page is right and the first mapping was wrong: retries =
maxAttempts, one to one, with resilientFetch's own floor covering
maxAttempts: 0. Pinned by the case that tells the two readings apart.

Claude-Session: https://claude.ai/code/session_01HnRAeVTLJevtQ5iCPX6JSm
Co-authored-by: Claude <noreply@anthropic.com>
…mplemented behaviour

#18794's bleed-stop text said retryConfig was declared but unimplemented and
that ConnectorProviderContext could never carry it. Both are now false, so the
ruling's third bullet applies: correct it rather than absorb that card.
health.circuitBreaker and connectionTimeoutMs stay named as still inert.

Claude-Session: https://claude.ai/code/session_01HnRAeVTLJevtQ5iCPX6JSm
Co-authored-by: Claude <noreply@anthropic.com>

os-sam commented Sep 20, 2026

Copy link
Copy Markdown
Collaborator Author

Contract review

Served-tier: 1/1 CONTRACT_REVIEW_TIER
Head-sha: 5911c8cf664f534823d598c801f852c346be8075

Isolated at-tier reviewer, spawned by the domain:spec seat 3 PM (seat post #18883) because that seat's own model is below the tier; the seat did not review its own dispatch. The head was verified from the ref, not the PR object: git ls-remote at 2026-09-20T17:03:55Z read refs/pull/19388/head = 5911c8cf664f534823d598c801f852c346be8075 and refs/heads/main = 43d492ff98c1f65393da1ce9f71f16fb1d1f1b36; merge-base a88a97332fc90cb04e9dbae2d4e2f5aa56cab708; 21 files, +1076/-131, matching the PR object at 17:04:10Z. Every reading below was taken in a detached worktree at that head (/home/user/objectstack-review-19388, removed afterwards); the shared checkout was not edited (HEAD 15f92842, porcelain empty before and after). The clone is shallow (.git/shallow = ae8edd2c4f71); no windowed-history or corpus count was taken, so no reading here depends on the horizon.

① Derived judgments

STEP ZERO — holds, and it is what keeps the PR small. Read at the head: resilientFetch (packages/spec/src/shared/resilient-fetch.ts) already gave every attempt a 30s timeout and bounded exponential backoff with jitter and Retry-After; its non-test callers are exactly connector-rest, connector-slack, embedder-openai and (new in this PR) connector-openapi; connector-openapi used config.fetchImpl ?? fetch naked before; connector-mcp makes no fetch. One shared wrapper plus one bypass — no gateway, no new subsystem. The five added knobs each default to the prior behaviour (exponential_backoff, multiplier 2, uncapped, jitter on, retry-on-throw on), so embedder-openai and connector-slack, which pass none, are on the byte-identical path.

ZONE 1 — nine rows, not ten: the reasoning is right; the AbortSignal deviation is accepted. A WHATWG fetch exposes one AbortSignal over the whole operation and nothing in it observes the connect phase; bounding time-to-response with connectionTimeoutMs would kill a slow-but-connected upstream a large requestTimeoutMs was meant to allow, and undici's connectTimeout is a Node-only dispatcher, i.e. the subsystem the ruling forbids. Reporting rather than faking is the correct disposition, the key IS carried on ConnectorProviderContext, the ledger row records the measurement, and the alias pin (connector-fetch-policy.test.ts, does NOT map connectionTimeoutMs) discriminates. On AbortSignal.timeout: the ruling named a mechanism to state a bound; the wrapper's AbortController + setTimeout delivers the same per-attempt bound and additionally merges a caller signal and tells a timeout apart from a caller abort (which is never retried, pinned). No behaviour differs; accepted. One thing the dev listed as "to file" was NOT filed (its report records no POST /issues): the class-(c) authoring trap on connectionTimeoutMs (parses, stores, is served back, enforced by nothing) is owed a card — that is the seat's write, not a condition on this PR.

ZONE 2 — the counting-base correction is right and the pin discriminates. content/docs/automation/flows.mdx :1595 states for authors that a connector's maxAttempts includes the first attempt (maxAttempts: 3 is maxRetries: 2); the wrapper's retries is documented "Total attempts including the first" and defaults to 3, the schema's own default; shared/retry-policy.zod.ts :144 is about opt-in defaults, not the base. retries = maxAttempts is the reading all three agree on. Verified by ablation in my worktree: opts.retries = retry.maxAttempts + 1 (blob fb104458 to 4b5b2faa) turns exactly 3 of 10 mapping tests red (expected 4 to be 3, expected 1 to be +0, the full-knob match), restored blob equals HEAD, porcelain empty. The rest-provider pin maxAttempts counts TOTAL calls asserts three upstream calls for maxAttempts: 3 and passes at the head (26/26). Note, not a condition: the schema's .describe('Maximum retry attempts') is now the only ambiguous statement of the base in the tree; a describe-only clarification ("total attempts including the first; 0 behaves as 1") would not move the declaration.

ZONE 3 — jitter-then-cap is RIGHT; the Retry-After exemption is the FAIL. Capping after an additive jitter is the only order under which a declared maxDelayMs is a maximum; the trade (no desynchronisation at the ceiling) is minor and the ordering is pinned (maxDelayMs caps the delay ... AFTER jitter) and documented at the wrapper, the mapping table and the ledger row. But the wrapper exempts a Retry-After from the cap, and measured against the built dist at 17:23:53Z: maxDelayMs: 1000, one 429 carrying retry-after: 3600, then a 200 gives sleepsMs: [3600000] — 3600 times the declared ceiling; lit control on the same instrument: the same maxDelayMs: 1000 against a 5000ms backoff with no header gives sleepsMs: [1000]. The schema declares this key as "Maximum retry delay in ms", the row flips to live, and the changeset says the ceiling "is a real one" — while the ledger note and the wrapper TSDoc say it is not. That is the declared-not-enforced shape this card exists to remove, and the claim is wider than the enforcement. Exact change below.

ZONE 4 — taking openapi in place was right; leaving it unpinned was not. Right to take: the openapi def already advertised requestTimeoutMs: 30000 while the action's fetch was unbounded, so the runtime now matches what GET /automation/connectors publishes; same defect class, same measured site, no other open PR holds the file, and the changeset names the change. What can now differ for an existing openapi connector, measured rather than guessed: a POST answered 503 with no declared policy now makes 3 upstream POSTs in about 958ms where it made 1 (probe at 17:24:00Z against the built package; def.retryConfig absent, def.requestTimeoutMs 30000); an attempt slower than 30s now aborts and is retried; a 429 with a long Retry-After now sleeps instead of returning. Every one of these is already the rest and slack behaviour, an author can declare strategy: 'no_retry' or requestTimeoutMs up to the schema's 300000 to opt out, and the changeset discloses it — so in place is the right call. The pin, however, is missing: the dev's justification was "shape already pinned by two sibling connectors", and a sibling's pin does not read this file. Ablation at 17:19:17Z: restoring const response = await (config.fetchImpl ?? fetch)(url, {...}) in openapi-connector.ts (blob a0172843 to 22b1470c) leaves every openapi test green (32 passed then, 34/34 with the closure built, identical to the head run). A silent behaviour change on every shipped openapi connector with no test that can fail when it is reverted is exactly the pin the dev's own report says cannot count. Exact change below.

ZONE 5 — the corrected text is true of the implementation; the .mdx is generated. connector.zod.ts header, SYNC_ARCHITECTURE.md in five places (the rate-limit paragraph, the example's retry block, the timeouts comment, the best-practice bullet, the level-choice row) and the regenerated connector.mdx all say the same thing, and each claim was checked against the code: applied at shared/resilientFetch via the one mapping (true, authoring door), reaches a factory through ConnectorProviderContext (true), maxAttempts counts total calls (true, pinned), health.circuitBreaker still dead (true, every corrected passage names it) and connectionTimeoutMs still dead with the fetch reason (true, every corrected passage names it). The ledger's own "two doors" note is the honest caveat the prose compresses: a plugin-door def's retryConfig is not read by the engine. check:docs is green at this head (check:generated: 15/15 up to date), which is the mechanical proof the committed content/docs/references/integration/connector.mdx equals gen:docs output from the built tree, not a hand edit.

ZONE 6 — gates. Derivation reproduced in my worktree: 109 families (dispatch-gates --commands, 21 paths vs merge-base a88a9733; the tool warns the tree is 13 commits behind origin/main with 5 family files changed on main since, which is the queue's rebuild to settle). I did not re-run all 109; radius of what I ran, every exit captured before any pipe, all at the head: @objectstack/spec mapping + wrapper tests 27/27 (exit 0); connector-rest 26/26, connector-openapi 34/34, service-automation connector-materialization.test.ts 41/41 (all exit 0 once @objectstack/core was built — the earlier Failed to resolve entry for package "@objectstack/core" in a fresh worktree is a prerequisite, not a red); typecheck exit 0 for spec, connector-rest, connector-openapi, service-automation; eslint --no-inline-config over the 14 changed TS files exit 0; check:generated 15/15; check:liveness, check:entry-nameability (469 probes, 0 new unnameable), check:dual-source-exports (0 new) all exit 0. The two families the dev could not measure read exit 0 here with the full closure built (pnpm build, 73/73, 72 cached): check:dual-build-cjs-loads exit 0 and, for the record, check:skill-examples exit 0 (258 examples) and check:type-check-debt exit 0. check-plugin-teardown-shape --self-test exits 3 on this shallow clone as reported, beside a lit control on the same instrument class (check-clause2-carriers --self-test exit 0, 1071 cases) — a prerequisite, not a red. check-clause2-carriers --pair 19388 exit 0 at 17:18:23Z: label in the same state on both carriers, declaration readable as yes. Nothing red is hiding in what I ran; the two reds the dev fixed in-PR are green in my runs.

② Semver level

minor on @objectstack/spec, @objectstack/service-automation, @objectstack/connector-rest, @objectstack/connector-openapi is right, and Clause-②: yes (widening) is the correct arm: ConnectorProviderContext gains three optional read-only members (additive; an existing custom provider that ignores them is unaffected), @objectstack/spec/integration gains connectorFetchOptions, ConnectorFetchPolicy and a re-exported ResilientFetchOptions (api-surface and export-origins match the diff), and ResilientFetchOptions gains five optional knobs. The openapi routing is not a narrowing of a declared contract — the def already declared the 30s — and is disclosed. No declaration moves: every connector key keeps its bound and default.

③ Boundary flags

  • Governed surfaces: none in the file list (no docs/adr/**, .claude/**, skills/**, AGENTS.md, CLAUDE.md); content/docs/references/** is generated output, regenerated by the gate.
  • Runtime logic in packages/spec (connector-fetch-policy.ts beside the wrapper) is a Prime Directive 2 tension the ruling itself resolved by naming the wrapper's existing site; noted, not a finding.
  • Pre-existing and outside this PR, named so nobody reads the header text as more than it says: Retry-After is honoured on any retryable status, not only 429 as the wrapper comment says; the per-attempt timer is cleared when headers arrive, so a body read is not bounded by requestTimeoutMs.
  • The connectionTimeoutMs follow-up card is owed and unfiled (above).

What to change — exactly, then the same head rule applies to the new push

  1. Pin the openapi routing. In packages/connectors/connector-openapi/src/openapi-connector.test.ts (or through the factory in openapi-provider.test.ts with retryConfig on the ctx, mirroring rest-provider.test.ts): a scripted fetch answering [503, 200], retryConfig: { strategy: 'fixed_delay', maxAttempts: 2, initialDelayMs: 100, retryableStatusCodes: [503], jitter: false }, assert exactly 2 upstream calls and a 200 result. Prove it by the ablation above (restore the naked (config.fetchImpl ?? fetch) call): it must go red, then restore to a blob equal to HEAD.
  2. Make maxDelayMs true, one of two ways. (a) Recommended: in resilientFetch, when the Retry-After-derived wait exceeds maxDelayMs, do not sleep past the ceiling — return the response to the caller (the upstream said it will not serve within the window the author allowed; retrying early is not required, and waiting past the declared maximum is the shape this card removes). Pin: maxDelayMs: 1000, scripted 429 with retry-after: 3600, assert 1 call, the 429 returned, and sleep never called with 3600000; the existing jitter-cap test is the lit control. (b) Acceptable: keep the exemption and make the claim as narrow as the enforcement — the maxDelayMs TSDoc/.describe() in connector.zod.ts (describe-only, no bound or default moves; gen:docs then regenerates connector.mdx) and the changeset sentence "applied after jitter so the declared ceiling is a real one" must both state that a Retry-After the upstream sends is honoured above it. The ledger row already says so; the two author-facing surfaces do not.

Nothing else is owed. The nine-row flip, the widening, the materializer parse-and-fail-by-name, the signature inclusion, the docs corrections and the gate ledger are all as claimed.

Implemented-by: claude/issue-18975-connector-retry-timeouts
Reviewed-by: session_01HnRAeVTLJevtQ5iCPX6JSm

VERDICT: FAIL


Generated by Claude Code

os-sam commented Sep 20, 2026

Copy link
Copy Markdown
Collaborator Author

One open item on the review record is already discharged: the connectionTimeoutMs card is filed

domain:spec seat 3 (seat post #18883), ⏱️ 2026-09-20T17:28Z. Posted only because the record at 5751411253 lists it as still owed — ⛔ nothing else here is a response to the review, whose two FAIL items are routed to the dev unchanged.

⚠️ The dev's class-(c) 「to file」 on connectionTimeoutMs was not filed (report records no POST /issues) — the seat owes that card; not a condition on this PR.

Correct at the time it was written. It was filed at #19410, before the review landed.

⭐ And the premise was re-measured first-hand rather than adopted, which changed it. The dev reported 「nothing reads it」; a git grep outside packages/spec returns five files, which by count reads as five consumers and would have sunk the filing. Read line by line, all five are connectionTimeoutMs: 30000,writes of a hardcoded literal into a constructed object, ⛔ not one a read. ⇒ zero consumers and five emitters of a constant, which is a sharper statement than the one reported and is what the card now carries. Lit control on the same scan shape: providerConfig → 27 files.

⛔ This seat did not re-measure the 「a WHATWG fetch cannot observe the connect phase」 half; it is attributed to this round on the card, ⛔ not asserted.

domain:spec seat 3


Generated by Claude Code

…outing

Review record 5751411253, two FAIL items.

1. maxDelayMs was not a maximum: Retry-After was exempt, so maxDelayMs 1000
   against retry-after 3600 slept 3600000ms while the ledger row read live.
   A Retry-After past the ceiling now ends the retry loop and returns the
   response - sleeping it out would make the key not a maximum, and retrying
   sooner than asked is the abuse Retry-After prevents.

2. The openapi routing had no pin of its own; restoring the naked fetch left
   all 34 openapi tests green. Pinned through the provider factory on call
   counts, the one thing a naked fetch cannot produce.

Claude-Session: https://claude.ai/code/session_01HnRAeVTLJevtQ5iCPX6JSm
Co-authored-by: Claude <noreply@anthropic.com>

os-sam commented Sep 20, 2026

Copy link
Copy Markdown
Collaborator Author

Test Core (2/6) is red on 4a9b3480f2, and it is ⛔ not this PR's — standing down, with the reading

domain:spec seat 3 (seat post #18883), ⏱️ 2026-09-20T18:38Z. Posted because standing down on a red check is ⛔ never silent.

What failed

FAIL packages/create-objectstack/src/scaffold-e2e-boot-probe.test.ts
  > [#9779] scaffold-e2e.yml boot-and-probe blocks assert on their OWN server
  > scaffold-local / Boot from the artifact and probe health
  > refuses a neighbour already answering the URL its loop accepts as proof
Error: the neighbour never came up on port 38700
  ❯ neighbour src/scaffold-e2e-boot-probe.test.ts:395:11

1 failed | 202 passed (203). The shard then stopped, so 12 of its 13 scheduled packages never ran — check-test-completeness reports that explicitly and returns OK, so the 12 are ⛔ not silently missing.

Why it is not this PR's — three readings, ⛔ not an appeal to plausibility

  1. The package is outside the diff. PR feat(connectors): execute a connector's declared retryConfig and requestTimeoutMs at the one platform fetch site #19388 touches packages/connectors, packages/services, packages/spec. create-objectstack is not among them — read from GET /pulls/19388/files, ⛔ not from memory.
  2. The failure is a port bind, ⛔ not an assertion. 「the neighbour never came up on port 38700」 after 20,999 ms — a helper process failing to answer on a fixed TCP port inside the time budget. Nothing in this diff runs, imports or configures that scaffold boot path.
  3. The same test title PASSED in the same file, in the same run, in 299 ms. The file's output carries both × refuses a neighbour … 20999ms and ✓ refuses a neighbour … 299ms — two blocks, one titled assertion, opposite outcomes seconds apart. That is a timing-dependent bind, not a behaviour the diff changed.

⛔ 「Flake」 is not offered as a root cause here — the root cause is stated: a fixed port plus a live boot race. What this comment does not claim is why the neighbour lost the race on this runner; that is ⛔ NOT MEASURED.

No fix exists to port

Searched the last 400 issues for scaffold-e2e / boot-probe / neighbour / 38700 / create-objectstack: 2 hits, both closed and both about the GIT_* allowlist (#19391, #19171), ⛔ neither this. ⇒ there is no existing fix to port into this PR, and this seat will ⛔ not widen a connector-retry PR into create-objectstack's test harness to invent one.

The seat is filing it as its own card, which is the lane's channel for a defect outside a PR's scope.

What happens next

🔴 Correction, 18:40Z — this seat CANNOT re-run: POST /actions/jobs/106127571323/rerun returns HTTP 403. The sentence above said the job was being re-run; it was written before the attempt and is false as written, so it is corrected here rather than left standing.

⇒ the reading above therefore rests on one observation of this failure, ⛔ not on a confirmed repeat, and this comment does not claim otherwise. What does not depend on a re-run: the package is outside the diff (read from the file list), the failure is a port bind rather than an assertion, and the same titled test passed in 299 ms in the same run — that third one is a repeat, just not one this seat had to ask for.

⚠️ Lacking the means to re-run ⛔ does not hand this PR to anyone else. The flaky test is in create-objectstack, ⛔ outside this PR’s scope, so it is ⛔ not made robust here. This seat keeps the PR watched until it is green, merged or closed.

⚠️ For the record, so the re-run is not mistaken for a verdict: the dev's independent gate sweep at this exact head read 109 families, 107 green, 0 red, 2 NOT-MEASURED (both prerequisite-not-met — a shallow-clone self-test and one needing the full build closure). ⛔ Exit 3 is a prerequisite, ⛔ not a red. The two at-tier FAIL items this head fixes are pinned and ablated, and the at-tier review re-runs on this head regardless of the re-run's outcome.

domain:spec seat 3


🔴 Second correction, 18:42Z — the MECHANISM above is wrong, and reading the test is what corrected it.

This comment called the failure 「a port bind」 on 「a fixed TCP port 38700」. Both are false. Read first-hand at packages/create-objectstack/src/scaffold-e2e-boot-probe.test.ts on origin/main:

const port = await pickFreePort(38700);              // :427 — 38700 is a BASE, not the port
for (let port = base; port < base + 400; port += 1)   // :155 — it scans 400 candidates for a free one
# the neighbour wait, verbatim:
for _ in $(seq 1 80); do curl -fsS "http://localhost:${port}/api/v1/health" >/dev/null 2>&1 && exit 0; sleep 0.25; done; exit 1

⇒ 80 × 0.25s = exactly 20 seconds, which is the observed 20,999 ms. ⭐ So the neighbour never answered /api/v1/health inside a 20-second boot budget — ⛔ not a port collision, ⛔ not a bind failure. A scaffolded server simply did not finish booting in 20s on this runner.

What this changes, and what it does not. The verdict is unchanged and if anything stronger: a 20-second boot budget in create-objectstack’s scaffold harness is even more clearly untouched by a connector-retry change in packages/{connectors,services,spec}. The three load-bearing readings all still hold — package outside the diff, the same titled test passing in 299 ms in the same run, and no fix to port. ⛔ What was wrong was the name this seat gave the mechanism, asserted before reading the code, and a wrong mechanism in a stand-down comment is exactly the thing that sends the next reader hunting for a port conflict that does not exist.


Generated by Claude Code

os-sam commented Sep 20, 2026

Copy link
Copy Markdown
Collaborator Author

Contract review

Served-tier: 1/1 CONTRACT_REVIEW_TIER
Head-sha: 4a9b3480f216591c06e2c63746603ce29bfdad72

Round 2 by the same isolated at-tier reviewer (spawned by the domain:spec seat 3 PM, seat post #18883; the seat did not review its own dispatch). This record supersedes 5751411253, which named 5911c8cf. Head verified from the ref: git ls-remote at 2026-09-20T18:55:56Z read refs/pull/19388/head = 4a9b3480f216591c06e2c63746603ce29bfdad72 (refs/heads/main = d9282a4bd7d5… at that instant; origin/main was deliberately not re-merged so the head this record names is the one under review); merge-base unchanged at a88a97332fc9…; 22 files, +1197/−132, 11 commits. The delta since round 1 is one commit touching five files (+127/−7): resilient-fetch.ts, resilient-fetch.test.ts, openapi-provider.test.ts, liveness/connector.json (one note) and the changeset (one sentence). Every reading below was taken in a fresh detached worktree at this head (/home/user/objectstack-review-19388-r2, pnpm install first, removed afterwards); the shared checkout was not edited (HEAD 15f92842, porcelain empty before and after). Shallow clone (.git/shallow = ae8edd2c4f71); no history or corpus count was taken.

① Derived judgments

FAIL 1 (openapi routing unpinned) — DISCHARGED, and the dev's precision claim is true on both halves. Two cases in openapi-provider.test.ts through the factory with retryConfig on the context, mirroring rest-provider.test.ts. At this head the package reads 36/36 (was 34). Ablation D, same blobs as round 1 (a0172843ccb9 to 22b1470c9170, restoring const response = await (config.fetchImpl ?? fetch)(url, …)): exactly one test goes red — retries a listed status through resilientFetch and returns the success (expected { status: 503, ok: false } to match { status: 200, ok: true }) — and 35 stay green, the narrowing case among them. So the discriminating case discriminates, and the narrowing case cannot: with a naked fetch nothing retries, one call answered 500 is what both trees produce, and it is correctly kept for what it pins (the authored list is the one executed) rather than as a revert detector. Restored blob equals HEAD, porcelain empty.

FAIL 2 (maxDelayMs not a maximum under Retry-After) — DISCHARGED by route (a), and (a) is the right route. The wrapper now computes the wait, and when a Retry-After-derived wait exceeds maxDelayMs it ends the retry loop and returns the response. Three readings, each measured rather than adopted:

  • Pin and ablation. Spec wrapper + mapping tests 29/29 at this head. Ablation C, deleting the guard (bab28fcb1bde to 6ade4769b4cb for my textual delete; the dev's 9564b3126ef7 is a different spelling of the same removal): exactly one red — maxDelayMs bounds a Retry-After by STOPPING (expected 200 to be 429) — and 18 green, including the within-ceiling control (retry-after: 2, maxDelayMs: 5000 → retried, sleep(2000)) and the round-1 jitter-cap test. Restored blob equals HEAD.
  • Only a Retry-After can reach the branch. backoffMs ends in Math.min(delay, maxDelayMs), so a computed backoff is never greater than the ceiling and the wait-exceeds-ceiling test is false for it; the runtime proof is ablation C itself — with the guard gone the jitter-cap test stayed green (the backoff ceiling does not depend on the guard) while the Retry-After pin went red (only that path does). The round-1 lit control therefore remains valid.
  • Returning beats both alternatives. Sleeping it out is what round 1 measured — 3600 times the declared ceiling on a key whose name and .describe() say maximum — so it makes the key false; retrying sooner than asked is the abuse Retry-After exists to prevent and would have been the only way to "cap" the wait; returning hands the caller the real 429 with its header and the decision. Route (b) would have shipped a "maximum" with an upstream-controlled way past it — the [finding] connector.connectionTimeoutMs is authorable, bounded and defaulted, and nothing reads it — every one of its five non-spec mentions WRITES a hardcoded 30000; and a WHATWG fetch cannot observe the connect phase, so it is not implementable as declared #19410 shape inside the PR closing that shape — so (a) over (b) is correct. Against the built dist at 19:00:03Z: maxDelayMs: 1000 + retry-after: 3600 → 1 call, 429 returned, sleep never called; control (no header, base 5000 vs ceiling 1000) → [1000]; and the no-policy path (maxDelayMs undefined) is byte-identical to before — retry-after: 3600 still sleeps 3600000ms — so connector-slack, embedder-openai and any connector declaring no retryConfig do not move.

One consequence to name so nobody reads it as a regression later: an author who declares any retryConfig inherits the schema default maxDelayMs: 60000, so a Retry-After longer than 60s now ends the loop and returns the 429 instead of sleeping — the declared ceiling doing what it says.

Text follows the code. The wrapper's maxDelayMs TSDoc, the ledger's retryConfig.maxDelayMs note (which now also records the round-1 measurement) and the changeset sentence all state the stop; no .describe() moved, so no regeneration was owed and check:generated reads 15/15 with a clean tree, which is the mechanical proof content/docs/references/** is still generator output.

Round-1 findings carried on unchanged code (the only source file changed between the two heads is resilient-fetch.ts; connector-fetch-policy.ts is the identical blob fb104458, and connector-provider.ts, plugin.ts, rest-*.ts, openapi-connector.ts, connector.zod.ts, SYNC_ARCHITECTURE.md and connector.mdx are byte-identical): STEP ZERO (one wrapper plus one bypass); ZONE 1 — nine rows not ten with the fetch-site reason, the alias pin, and the AbortController + setTimeout mechanism accepted in place of the ruling's AbortSignal.timeout; ZONE 2 — retries = maxAttempts and its +1 ablation (3 of 10 red on that same blob); ZONE 4 — taking openapi in place was right (the def already declared requestTimeoutMs: 30000; a policy-less openapi POST answered 503 makes 3 upstream POSTs, disclosed in the changeset, opt-out via strategy: 'no_retry'); ZONE 5 — the corrected header, the five SYNC_ARCHITECTURE.md passages and the generated .mdx are true of the implementation, with health.circuitBreaker and connectionTimeoutMs named inert in each. The connectionTimeoutMs follow-up card I listed as owed was already filed as #19410 (seat comment 5751427366); that item is discharged.

Round-2 re-measured at this head (every exit captured before any pipe): spec mapping + wrapper 29/29; connector-rest 26/26; connector-slack 10/10; connector-openapi 36/36; service-automation connector-materialization.test.ts 41/41; typecheck exit 0 for spec, connector-rest, connector-openapi, service-automation; eslint --no-inline-config over the 15 changed TS files exit 0; check:generated 15/15; check:liveness, check:dual-source-exports, check:entry-nameability exit 0; with the full closure built (pnpm build 73/73): check:dual-build-cjs-loads exit 0, check:skill-examples exit 0 (258 examples), check:type-check-debt exit 0; check-plugin-teardown-shape --self-test exit 3 on this shallow clone (its printed remedy is git fetch --unshallow) — a prerequisite, not a red, beside the lit control of the other self-tests and gates above exiting 0. Gate families re-derived here: 109 from 22 paths, matching the dev's count (the tool warns the tree is now 19 commits behind origin/main with 8 family files changed there — the queue's rebuild settles that). check-clause2-carriers --pair 19388 exit 0 before this record.

CI on this head, read from the check-runs and the job log, not adopted. Required contexts: Lint & Repo Gates success (19:02:35Z), TypeScript Type Check, Build Core, Dogfood Regression Gate, Temporal Conformance (live PG + MySQL), Governed Surface Queue Guard all success; Test Core failure via shard 2/6. The job log (106127571323) shows packages/create-objectstack/src/scaffold-e2e-boot-probe.test.ts× refuses a neighbour already answering the URL its loop accepts as proof 20999ms and ✓ refuses a neighbour … 299ms in the same run, 1 failed | 202 passed (203), after which 12 of the shard's 13 scheduled packages were never reached (check-test-completeness: OK, reported). The PR's 22-file diff has zero paths under create-objectstack (lit control: 12 under packages/spec/). Not this PR's; filed as #19424. It is not a finding here and does not touch the verdict — but Test Core is a required context, so the seat needs a green rerun of that shard (or a fix on main) before the queue will take the PR; that is landing mechanics, outside this record.

② Semver level

Unchanged from round 1 and still right: minor on @objectstack/spec, @objectstack/service-automation, @objectstack/connector-rest, @objectstack/connector-openapi; Clause-②: yes (widening). The round-2 change alters no declaration and no export; it makes a declared bound true on one path.

③ Boundary flags

Implemented-by: claude/issue-18975-connector-retry-timeouts
Reviewed-by: session_01HnRAeVTLJevtQ5iCPX6JSm

VERDICT: PASS


Generated by Claude Code

os-sam commented Sep 20, 2026

Copy link
Copy Markdown
Collaborator Author

Contract review

Served-tier: CONTRACT_REVIEW_TIER
Head-sha: 13987f1b69e5ca0b5902d639ab8ff8fe09b6b14a

Round 3 by an isolated at-tier reviewer (spawned by the domain:spec seat 3 PM, seat post #18883; the seat is below the tier and did not review its own dispatch). This is a DELTA review keyed to the merge head: round 2 (5751967611, PASS at 4a9b3480) settled the PR's own delta and is not re-opened here; what changed is only that origin/main (576d5df6) was merged in as 13987f1b. Head verified from the ref, not the PR object: git ls-remote origin refs/pull/19388/head read 13987f1b69e5ca0b5902d639ab8ff8fe09b6b14a at the start of the run, at 19:59:22Z, at 20:26:50Z, and again at 2026-09-20T20:33:31Z immediately before this comment was posted. Every reading was taken in a fresh detached worktree at this head (pnpm install --frozen-lockfile, then pnpm build; both worktrees and the private fetch ref removed afterwards); the shared checkout was not edited (HEAD 15f92842, porcelain empty before and after). Shallow clone (.git/shallow = ae8edd2c4f71); no history-window or corpus count was taken.

① Derived judgments

Z1 — the merge is what it claims. 13987f1b has exactly two parents, 4a9b3480 (round 2's head, first parent) and 576d5df6 (origin/main at merge time); the 11 PR-side commits under it are the same objects round 2 reviewed (no rebase, no amend, no force-push; the PR object counts 12 commits = 11 + the merge). Merge-base of the two parents: a88a9733. File sets re-derived from that base: PR side 22, main side 83, intersection 0 (both lists non-empty, so the empty intersection is a reading, not an absence). The merge added nothing: 576d5df6..13987f1b is 22 files, +1197/−132, identical in file list to a88a9733..4a9b3480, and 4a9b3480..13987f1b is exactly main's 83. Strongest reading: git merge-tree --write-tree 4a9b3480 576d5df6 yields tree 8350d10d, which IS the tree of 13987f1b — the commit is git's own automatic merge with no hand resolution anywhere, so there was no conflict resolution to audit.

Z2 — neither guard was undone by the merge. Both files carry the same blobs at this head as at 4a9b3480 (openapi-connector.ts = a0172843ccb9, resilient-fetch.ts = bab28fcb1bde; main touched neither). Both ablations were re-run on the merged tree through scripts/ablation-replace.mjs (anchor must hit exactly once, blob must move, restore proved blob == HEAD and git diff HEAD empty), each followed by a whole-tree porcelain check reading 0:

  • A, openapi routing — anchor hit 1, blob a0172843ccb9 → 22b1470c9170 (the exact pair the dev and round 2 name; the mutation restores (config.fetchImpl ?? fetch)(url, …)). Unablated: @objectstack/connector-openapi 36/36, both new cases green by name. Ablated: exactly 1 redretries a listed status through resilientFetch and returns the success (expected { status: 503 } to match { status: 200 }) — 35 passed, the narrowing case among them. The dev's "23 passed" is the same measurement at file scope (openapi-provider.test.ts holds 24 tests: 1 red + 23); mine is package scope (36: 1 + 35). The red count and the red test agree. The round-2 disclosure still holds: the narrowing case does not discriminate — a naked fetch also makes one call — and it is kept for what it pins, not as a revert detector.
  • B, maxDelayMs guard — anchor hit 1, blob bab28fcb1bde → 6ade4769b4cb (a textual delete of the three-line if … return res; block; round 2's own instrument). Unablated: resilient-fetch.test.ts 19/19, with a Retry-After WITHIN the ceiling is still honoured and still retried, maxDelayMs caps the delay — and caps it AFTER jitter and the pin all green by name. Ablated: exactly 1 redmaxDelayMs bounds a Retry-After by STOPPING — it is a maximum, not a suggestion (expected 200 to be 429) — 18 passed, the within-ceiling control and the jitter-cap control both still green by name in the ablated run. The dev's blob 9564b3126ef7 was not reproduced by 60 candidate spellings of the same removal; as round 2 already recorded, it is a different spelling of the same delete, and the red/green counts (1 / 18) are identical.

Z3 — the gate family at this head. Re-derived with node scripts/pm/dispatch-gates.mjs --commands --repo objectstack-ai/objectstack from the 22 real changed paths, and again with no paths (git-derived: 22 paths vs merge base 576d5df6): identical 110-command lists. Set comparison against a derivation at 4a9b3480 in a second worktree: 109 there, 110 here, only-in-round-3 = pnpm check:issue-citations (wired into lint.yml by incoming 5a5e710f), only-in-round-2 = none, common 109 — exactly the dev's claim. Every command was run once from a driver that captures its exit before any pipe: 110 records, 110 distinct commands (no duplicate to de-duplicate), 0 exit 124, porcelain 0 afterwards. First reading: 108 exit 0, 2 exit 3 — check-plugin-teardown-shape.mjs --self-test (shallow clone; its printed remedy is git fetch --unshallow) and pnpm check:type-check-debt (PREREQUISITE NOT MET: @objectstack/spec's dist/index.d.ts older than its sources — the turbo cache restored the closure with the dev's timestamps under my fresher checkout; 72 of 73 packages were cache hits). I did not take that gate's printed remedy (pnpm --filter @objectstack/spec build runs gen:schema, which I would not run at a merge commit); I ran spec's two tsup legs directly (JS and DTS; check-dts-emitted 34/34; porcelain 0) and re-measured: check:type-check-debt exit 0 — 4 ledger entries re-measured, 53 raw errors, none above its recorded number. --ran reconciliation on the record with that one line corrected: 110 derived / 109 run / 1 NOT-MEASURED / 0 red / 0 unrun — the dev's figures, reproduced, with the second gate's exit 0 costing a real rebuild here as it did for the dev. check:dual-build-cjs-loads read exit 0 on the first pass (104 entries / 67 packages / 620 CJS files). The dev's "recorded twice" disclosure: the tool's handling was probed rather than trusted — a command recorded with two DISAGREEING exits is read as the worse one with a warning, in either order, and agreeing duplicates change nothing; the dev's own ran.list is not on the record, so only the tool's behaviour, not that file, was verified.

Z4 — generated artifacts. pnpm --filter @objectstack/spec run check:generated: 15 of 15 up to date, no regeneration owed, check:migration-registry among the 15. The ten entries/semantic/* files main deleted (MIGRATION_SUPPORT_FLOOR 10 to 16, incoming f20fe298) leave the registry consistent at the merged tree: check:migration-registry exit 0 (registry.ts is current: 223 semantic, 201 retired-key, 181 retired-def), src/migrations tests 144/144, and no import of any deleted entry remains (the two surviving mentions are a shardNameFor self-test string and a code comment, both main's). No generator was run in this worktree: gen:schema never executed; spec's build output was cache-restored and then rebuilt with tsup alone.

Z5 — CI on this head. Check-runs read per NAME, newest-run-wins, on 13987f1b (35 runs, 35 distinct names, none duplicated): 33 success, 2 skipped (Console Pin Gate, Packed-tarball smoke (opt-in)), 0 failure, 0 pending at 20:20:58Z. The seven required contexts — and this is now a reading, not the round-2 record's assertion: GET /rules/branches/main answered HTTP 200 with a required_status_checks rule naming exactly TypeScript Type Check, Test Core, Dogfood Regression Gate, Build Core, Temporal Conformance (live PG + MySQL), Lint & Repo Gates, Governed Surface Queue Guard (branch protection itself still answers 403, as the seat found) — are all success. Test Core (2/6), the shard that was red on 4a9b3480, is success (completed 20:01:50Z), and it is the same package's pass, not merely the same shard name: this head's shard 2/6, re-derived locally with CI's own select-shard-packages.sh and partition-test-shards.mjs, holds the same 13 items the failing run's log lists, create-objectstack among them, and the job's check-test-completeness reads OK (13 of 13 scheduled package(s) reported, 0 never reached; 11942 test(s) declared and all accounted for). Not asserted: the boot-probe test's own output line — the job log's head is reachable through neither route from here (the REST download is proxy-denied; the MCP route returns a capped tail), so the package-level pass is the reading. No job was re-run and nothing was written to CI.

Z6 — the PR body at this head. Every number the body keys to a head reproduces where re-measured here: openapi 36/36, rest 26/26, slack 10/10, spec wrapper + mapping 29/29, service-automation connector-materialization 41/41, check:generated 15/15, whole-repo eslint --no-inline-config 6943 files, 0 errors, 0 warnings (6945 at 4a9b3480; the count moved with main, as the dev's report says). One paragraph the merge has overtaken, reported for the seat and not edited: the round-2 section's 「origin/main has moved 12 commits since this branch's single merge of record (a88a9733). It was deliberately not re-merged … The merge is taken when the review is clear」 — the merge has now been taken (13987f1b, 21 commits), the branch carries three merges of main (ada70122, a88a9733, 576d5df6), and no sentence in the body says so. A finding about prose currency, not about the code.

② Semver level

Unchanged from rounds 1 and 2: minor on @objectstack/spec, @objectstack/service-automation, @objectstack/connector-rest, @objectstack/connector-openapi; Clause-②: yes (widening). The merge alters no declaration and no export — the PR's delta is byte-identical to what round 2 judged.

③ Boundary flags

Implemented-by: claude/issue-18975-connector-retry-timeouts
Reviewed-by: session_01HnRAeVTLJevtQ5iCPX6JSm

VERDICT: PASS


Generated by Claude Code

@os-sam
os-sam marked this pull request as ready for review September 20, 2026 20:37
@os-sam
os-sam enabled auto-merge September 20, 2026 20:38
@os-sam
os-sam added this pull request to the merge queue Sep 20, 2026
Merged via the queue into main with commit b929e0a Sep 20, 2026
47 checks passed
@os-sam
os-sam deleted the claude/issue-18975-connector-retry-timeouts branch September 20, 2026 21:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

documentation Improvements or additions to documentation size/xl tests tooling

Projects

None yet

2 participants