Skip to content

fix(rest): one error envelope across the three /security/suggested-bindings routes (#7981) - #8071

Merged
hotlong merged 3 commits into
mainfrom
claude/issue-7981-security-endpoints-envelope-convergence
Aug 12, 2026
Merged

fix(rest): one error envelope across the three /security/suggested-bindings routes (#7981)#8071
hotlong merged 3 commits into
mainfrom
claude/issue-7981-security-endpoints-envelope-convergence

Conversation

@hotlong

@hotlonghotlong commented Aug 12, 2026

Copy link
Copy Markdown
Contributor

Fixes#7981

What was wrong

registerSecurityEndpoints answered three mutually incompatible refusal envelopes, decided only by which arm refused — on three routes a single client calls in sequence (list → confirm / dismiss):

armshape before
validation refusals (repeated query param #6877, unknown ?status#7678){ error: { code, message } } — ADR-0112
security service not registered (respond501){ code, message } — no error wrapper at all
thrown service error 403 / 404 / 409 / 500 (handleError){ code, error: 'a bare string' }

So body.error.code — the one position ADR-0112 D5 declares for the semantic code — read undefined on two arms out of three, and the two it failed on include the arm carrying the typed PERMISSION_DENIED 403 / SUGGESTION_NOT_FOUND 404 / SUGGESTION_STATE 409 codes the routes' own docblock advertises, i.e. the arm a consumer is most likely to branch on. None of the three was wrong on its own; they were wrong as a set.

What changed

All three arms now emit { error: { code, message } } through one shared respondError helper inside registerSecurityEndpoints, so "the arms agree" is a property of the code rather than of three literals that happen to match today.

The target shape was taken from a sibling arm in this same file, not re-derived from the ADR — the ?status guard PR #7979 landed here earlier today, and the multiplicity gate it sits beside, both write exactly this body from query-multiplicity.ts. The region therefore converges rather than acquiring a fourth reading of the same document.

No code value and no HTTP status moves: NOT_IMPLEMENTED / VALIDATION_ERROR stay the standard catalog's members for 501 / 400, the thrown arm still passes the service's own err.code through, and nothing in packages/spec changes. respond501's status was explicitly out of scope and is untouched.

The wire-visible half — TWO readers exist, both multi-envelope by design

The bare-string error is wire-visible, so the consumer sweep ran before the edit (#7035 methodology: code === '…' as well as code: '…'), each negative paired with a control query.

Readers found — 2:

  1. packages/client SDK, ObjectStackClient.fetch (packages/client/src/index.ts, the if (!res.ok) branch — not unwrapResponse, which never sees a failure because fetch throws first):
consterrorMessage=errorBody?.message??errorBody?.error?.message??(typeoferrorBody?.error==='string' ? errorBody.error : undefined)??res.statusText;consterrorCode=asSemanticCode(errorBody?.code)??asSemanticCode(errorBody?.error?.code);

Its own comment states the intent: the two reads are "the two LIVE envelopes' declared spots, not a fallback chain". So it is deliberately bi-dialectal, and every field it exposes comes out identical in value after this change — 501: message moves body.messagebody.error.message, code body.codebody.error.code, same strings; thrown arm: message moves off the plain-string limb onto error.message, code likewise. err.httpStatus is taken from the transport, and err.details already fell through to the whole body on both flat shapes and still does.

  1. objectui console, packages/app-shell/src/services/suggestedBindingsApi.ts:41-42payload?.error?.code ?? 'HTTP_'+status and payload?.error?.message ?? payload?.error ?? res.statusText: canonical-first, so this change improves it (a 403 confirm denial currently reports HTTP_403; afterwards the real PERMISSION_DENIED, with the same message text). It cannot render [object Object] — the chain ends in a typeof message === 'string' guard. Nothing branches on the code (errorCodeIs is used on the marketplace API, never here).

Negatives, each with its control:

surfaceresultcontrol
cloudno consumer at allcontrol api/v1/security returns 2 hits in cloud; target returns 0
this repo's testsnone assert these arms' bodies; SUGGESTION_* appear only at the emitter and in the spec ledgercontrol NOT_IMPLEMENTED is asserted in 8+ rest test files
docs / QA checklistname codes and statuses, never a body positioncontrol security/explain returns 52 files

Both readers therefore keep working, and neither loses information. The go/no-go on a wire change is still the PM's, not mine — this PR is deliberately parked in draft for that ruling.

Verification

  • @objectstack/rest suite: 98 files / 1589 tests passed.
  • pnpm --filter @objectstack/rest typecheck (tsc --noEmit): clean.
  • pnpm check:type-check-debt: OK, none above its recorded number; @objectstack/rest is absent from the "can be lowered" list, i.e. it re-measured at exactly its recorded 155. The ledger does not move.
  • check:error-code-casing and check-nul-bytes: clean.
  • Reverse verification — with the fix committed, the handleError arm alone was reverted to { code, error: 'string' } and the new suite re-run: 12 failed / 11 passed, all on shape, none a compile error. Representative reds: typed 403 list has no nested code: expected 'undefined' to be 'string'; typed 403 list still has a top-level code sibling: expected { code: 'PERMISSION_DENIED' } to not have property "code"; and the derived cross-arm pin expected 1, received 2 distinct skeletons. Restored with git checkout HEAD -- packages/rest/src/rest-server.ts.

The cross-arm pin is derived, not three literals: shapeOf() reduces a body to its key-path/type skeleton and the family case asserts every arm reduces to the same one without naming it — so a fourth dialect fails there even if someone also adds a matching literal case.

Out-of-scope finding filed from this work: #8073 (the two adjacent registrars in this same file still answer the retired dialects).


Generated by Claude Code

…on ADR-0112 (#7981)
The three /security/suggested-bindings routes answered three mutually
incompatible refusal shapes decided only by which arm refused. All three
now emit the ADR-0112 nested body through one helper.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01B3Kurx8qufrDzNjk4rag7V
@vercel

vercelBot commented Aug 12, 2026

Copy link
Copy Markdown

The latest updates on your projects. Learn more about Vercel for GitHub.

1 Skipped Deployment
ProjectDeploymentActionsUpdated (UTC)
objectstackIgnoredIgnoredAug 12, 2026 2:24pm

Request Review

@github-actions

Copy link
Copy Markdown
Contributor

📓 Docs Drift Check

This PR changes 1 package(s): @objectstack/rest.

9 hand-written doc(s) reference the affected code and may need an implementation-accuracy re-verification:

  • content/docs/ai/connect-mcp.mdx(via @objectstack/rest)
  • content/docs/api/error-handling-server.mdx(via @objectstack/rest)
  • content/docs/api/index.mdx(via @objectstack/rest)
  • content/docs/permissions/authentication.mdx(via @objectstack/rest)
  • content/docs/permissions/system-context.mdx(via packages/rest)
  • content/docs/plugins/index.mdx(via @objectstack/rest)
  • content/docs/plugins/packages.mdx(via @objectstack/rest)
  • content/docs/protocol/kernel/http-protocol.mdx(via @objectstack/rest)
  • content/docs/protocol/kernel/i18n-standard.mdx(via packages/rest)

3 release-owned page(s) also reference the affected code. These are read-only:

  • content/docs/releases/implementation-status.mdx(via @objectstack/rest)
  • content/docs/releases/v12.mdx(via @objectstack/rest)
  • content/docs/releases/v17.mdx(via @objectstack/rest)

content/docs/releases/ is RELEASE-OWNED (AGENTS.md "Documentation Guardrails"): release
notes are written centrally at release time, and a code PR that edits them is the exact PR
that guardrail exists to stop. They are still audited — read-only. If one of them is actually
wrong, file an issue or open a dedicated docs-only PR; do not edit it here.

Advisory only. To re-verify, run the docs-accuracy-audit workflow scoped to these files:
node scripts/docs-audit/affected-docs.mjs origin/main → pass the list as args.docs.

@hotlongClaude

Copy link
Copy Markdown
ContributorAuthor

PM ruling — domain:cli seat (#6024). Option A: land it. Flipping ready; auto-merge on.

The stop rule fired correctly and is overruled on its own evidence

My dispatch said: "If a reader exists, say so and stop — that turns this from a convergence into a breaking change needing a ruling." Two readers exist, the report stopped, and it did not take the decision. That is the rule working.

It is overruled because the measurement shows the rule's trigger is met while its rationale is not. Neither reader is single-dialect:

  • The SDK reads errorBody?.error?.messagebefore the plain-string limb, and its own comment calls the two reads "the two LIVE envelopes declared spots, not a fallback chain." Every exposed field comes out value-identical.
  • The console is canonical-firstpayload?.error?.code ?? 'HTTP_' + status — so this change stops the string limb being reached at all and a 403 confirm denial goes from HTTP_403 to PERMISSION_DENIED. Strictly better. The chain ends in a typeof message === 'string' guard, so [object Object] is unreachable.

Those string limbs are consumer-side tolerance for a producer's inconsistency. Prime Directive #12 says pay that down at the producer, which is what this does. ADR-0112 D5 was already ruled and #7035 already retired this dialect in this same file — the destination was not in question, only whether today was the day.

I am recording the reasoning rather than just the verdict, because the next person to hit this rule should know a bare-string reader is not automatically a break: ask whether the reader is single-dialect, not whether it touches the field.

My claim comment's anchor was wrong

I named respondSharingError as the security family's bare-string emitter. Measured: it is at rest-server.ts:9218 inside registerSharingEndpoints — one registrar earlier. The security family's is handleError. The issue body named it correctly; my paraphrase pointed at the neighbour.

Checking that rather than fixing what the PM pointed at is what kept this PR inside its scope — and the neighbour is filed as #8073 rather than quietly swept in. Fourth time today a dispatch of mine has been corrected by measurement; the escape hatch keeps earning its place.

The cross-arm pin is the part that outlives this PR

shapeOf() reduces a body to its sorted key-path/value-type skeleton, and the family case asserts all nine refusals collapse to one skeleton without naming it. So a fourth dialect fails even if someone adds a matching literal case alongside it — the failure mode that let three dialects coexist here in the first place. Reverse verification confirms it discriminates: reverting one arm produced expected 1, received 2 distinct skeletons, detecting the split without being told what the shape is.

Also correct: the ?status guard from #7979 was re-routed through the shared emitter rather than left alone, so today's reference point is one by construction instead of by coincidence.

@objectstack/rest re-measured at exactly 155 and is absent from the "can be lowered" list — the zero-margin ceiling holds.

⚠️ There is a CI red on this PR's ESLint job to work through before it can merge. Read the job log, not the name: that job has gone red twice today and neither time was ESLint (check:driver-memory-census, check:verify-stand-in).


Generated by Claude Code

@hotlongClaude

Copy link
Copy Markdown
ContributorAuthor

Correction to the ruling comment above: it says "Flipping ready; auto-merge on." That has not happened and should not until the ESLint job is green. The PR stays draft.

The ruling itself stands unchanged — Option A, land it — and it is the decision that was unblocked, not the merge. Sequence from here:

  1. Resolve the ESLint red. ⛔ Read the job log, not the job name: that job has gone red twice today and neither time was ESLint (check:driver-memory-census on fix(plugin-dev): tell "not installed" apart from "installed and failed to construct" (#7926) #8048, check:verify-stand-in on feat(verify): --rls runs one probe persona per declared POSITION, so app-authored narrowing is exercised (#7978) #8067). A likely candidate here, given this PR adds a test file and a shared emitter, is a lint-family gate rather than a rule violation — but measure it, do not assume.
  2. ⛔ Whatever it is, resolve it the way the gate asks. Do not silence, bypass, or delete-to-match. If it turns out the gate is wrong rather than the change, that is a finding.
  3. Green → I flip ready and enable auto-merge (both steps).

Flagging my own error plainly because a PR comment is durable: someone reading this thread later would otherwise believe it was enqueued and stop looking for the red.


Generated by Claude Code

… 75)
registerSecurityEndpoints' conversion onto the shared respondError helper
moved 2 write sites off the { code, error } sibling-code dialect. The
route-envelope conformance gate (#3843) treats a lower found-count as
progress to bank, not a pass: lower the declared ceiling in MODULES so the
ground cannot be silently given back.
Measured: merge-base (e3c8ed0) siblingCode=77, branch head siblingCode=75.
The 2 vanished sites are merge-base lines 9529/9532, both inside
registerSecurityEndpoints -- exactly the function PR #8071 converted.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01B3Kurx8qufrDzNjk4rag7V
@hotlongClaude

Copy link
Copy Markdown
ContributorAuthor

The red ESLint job was not a lint failure — that job runs a series of check:* scripts, and the one that failed was pnpm check:route-envelope. It was not reporting a violation; it was refusing to let progress go unbanked.

What the gate said

packages/rest/src/rest-server.ts
siblingCode: found 75, declared 77 — 2 fewer than pinned.
That is progress, and banking it is the other half of the ratchet: lower the
declared number to 75 in MODULES so the ground cannot be given back.
(ratchet for #7035 (option 1: convert onto the shared sendOk/sendError))

This commit's registerSecurityEndpoints conversion onto the shared respondError helper moved 2 write sites off the { code, error } sibling-code dialect. scripts/check-route-envelope.mjs's dialect-only ratchet for this file only ever ticks down — an unlowered ceiling is ground that could be silently given back later — so the fix is lowering the declared siblingCode from 77 to 75 in MODULES.

How the delta was confirmed as this PR's

Measured siblingCode (via the script's own scanSource) at both ends of this branch:

  • merge-base with origin/main (e3c8ed0f8): 77
  • branch head (5eb732c84, before this commit): 75

Delta is exactly 2, matching the gate's report. Diffing the two measurements' site line numbers under a uniform line-shift (the PR's net +42/-12 change sits entirely inside registerSecurityEndpoints) showed every other siblingCode site persists unmoved; the only two that vanished are merge-base lines 9529 and 9532 — the handleError helper's two { code: err?.code ?? defaultCode, error: ... } / { code: defaultCode, error: ... } literals, both inside registerSecurityEndpoints, both converted onto respondError by this PR's own diff. No unexpected sites moved.

Fix

scripts/check-route-envelope.mjs: lowered the declared siblingCode for packages/rest/src/rest-server.ts from 77 to 75, with a one-line note pointing at #7981. Nothing else in the file changed.

Verification

  • pnpm check:route-envelope (self-test + real audit): green.
  • pnpm --filter @objectstack/rest test: 98 files / 1589 tests passed.
  • pnpm --filter @objectstack/rest typecheck: clean.
  • pnpm check:type-check-debt (full closure built first): OK, none above its recorded ceiling; @objectstack/rest's TEST_DEBT entry (155, zero margin) is absent from the "can be lowered" list, i.e. unchanged.
  • node scripts/check-nul-bytes.mjs: OK.

Pushed to the same branch; PR left in draft.


Generated by Claude Code

@hotlongClaude

Copy link
Copy Markdown
ContributorAuthor

PM review — domain:cli seat (#6024): substance accepted; enqueueing held until the ESLint job confirms green

The red was never ESLint

Recording this at the top so the next reader is not sent hunting for a lint error that does not exist. The job named ESLint runs a series of check:* scripts and reports under the name of the first. The actual failure was pnpm check:route-envelope, and it was not a violation — it was the ratchet refusing to let banked progress go unrecorded:

siblingCode: found 75, declared 77 — 2 fewer than pinned.
That is progress, and banking it is the other half of the ratchet: lower the
declared number to 75 in MODULES so the ground cannot be given back.

This PR improved the file — converting registerSecurityEndpoints' two handleError arms onto the shared respondError helper moved two sites off the siblingCode dialect — and the gate demanded the ledger come down to meet the code. This is the opposite direction from check:type-check-debt, which must never be raised. Both rules exist, they point opposite ways, and reading one as the other is how an agent freezes or does the wrong thing. That is now ops-intel ㉟ on the seat post.

The verification is the part I want on the record

The dispatch told the executor the delta was 2 and to lower 77 → 75. It also told them to verify that rather than inherit it, because a ratchet number written from a stale measurement silently licenses regressions. They did:

  • siblingCode at the merge-base with origin/main (e3c8ed0f8) = 77
  • at the branch head before this commit (5eb732c84) = 75
  • delta exactly 2, matching the gate's own report

and — the step that actually closes it — they identified which two vanished, under the PR's own uniform line-shift: merge-base lines 9529 and 9532, both inside registerSecurityEndpoints, both converted by this PR's diff, with every other siblingCode site persisting unmoved. So the drop is provably this PR's own and not a stale-main artifact. A bare "the count is 2 lower" would not have distinguished those.

Gate now reads "9 route module(s) audited: 7 conformant, 1 ratcheted, 1 exempt" with siblingCode 75.

Checks

@objectstack/rest — 98 files / 1589 tests pass. typecheck clean. check:type-check-debt OK, and the zero-margin constraint was read precisely: @objectstack/rest's ceiling of 155 is absent from the "can be lowered" list, i.e. unchanged and not raised — the correct way to demonstrate a zero-margin ledger held, rather than asserting "the gate was green." check-nul-bytes OK. No numbers raised, no envelope-divergent sites reintroduced, content/docs/releases/ untouched, draft preserved, no redundant changeset.

Why this is still a draft for a few more minutes

My standing ruling on this PR was ⛔ draft until the ESLint job red is resolved. It is addressed but not yet confirmed — the re-run started at 16:18 and that job is still in flight. "The fix should work" is not the same claim as "the gate is green," and this seat does not enqueue on the first. Flipping ready and enabling auto-merge as soon as that job reports green — both steps, since flipping out of draft alone does not enqueue.


Generated by Claude Code

@hotlong
hotlong marked this pull request as ready for review August 12, 2026 16:28
@hotlong
hotlong enabled auto-merge August 12, 2026 16:28
@hotlong
hotlong added this pull request to the merge queueAug 12, 2026
Merged via the queue into main with commit 422e97bAug 12, 2026
24 checks passed
@hotlong
hotlong deleted the claude/issue-7981-security-endpoints-envelope-convergence branch August 12, 2026 16:43
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

documentationImprovements or additions to documentationsize/mteststooling

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[finding] registerSecurityEndpoints answers THREE different error-envelope shapes on the same three routes (ADR-0112)

2 participants

@hotlong@claude