Skip to content

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

Description

@hotlong

observation-class finding, recorded while fixing #7678 (PR #7979). Not fixed there — that card was scoped to the ?status validation, and this is a separate, wider shape. Filed unassigned.

Fact

The three /api/v1/security/suggested-bindings routes in packages/rest/src/rest-server.ts → registerSecurityEndpoints emit three mutually incompatible error envelopes, depending only on which arm refuses:

arm shape where
repeated query param (#6877) — and, since #7678, unknown ?status { error: { code, message } } refuseRepeatedQueryParams / the new status guard
service not registered { code, message } — no error wrapper at all respond501
thrown service error (403 / 404 / 409 / 500) { code, error: "<string>" } — error is a bare string handleError

So a client cannot read one field to learn why its call failed on these routes: error is an object in one arm, absent in another, and a human-readable string in the third — and the semantic code sits at error.code in the first and at top-level code in the other two.

The third shape is specifically the dialect #7035 retired (PR #7293 converged this file's /meta 501 refusals off it). handleError is the arm that carries the typed service errors the route's own docblock advertises — permission 403, not-found 404, state-conflict 409 — i.e. the arm consumers are most likely to branch on.

Why it is worth recording separately

Under ADR-0112 D5 the destination is fixed: error.code carries the semantic code, the HTTP status lives on the transport. All three arms here predate that convergence, and none of them is wrong individually — they are wrong as a set, which is exactly the class no per-arm review catches.

It is also a behaviour divergence against the runtime dispatcher twin (packages/runtime/src/domains/security.ts), which routes every one of these outcomes through deps.error / deps.errorFromThrown → apiErrorResponse, i.e. one shape: { success: false, error: { code, message, httpStatus } }. That makes it a concrete input to the route-ledger↔live-mount parity gate #7526, and specifically to the argument #7678 makes — that the gate should cover behaviour divergence, not just mount presence. A gate comparing only mount presence is green on all three of these.

Not claimed here

Suggested handling

Triage-level, not decided here: converge the three arms in registerSecurityEndpoints onto the nested { error: { code, message } } shape the same route already uses for its two validation refusals — or fold it into whatever batch is carrying the #7035 convergence through rest-server.ts, since this is the same edit repeated per route family.


Generated by Claude Code

Activity

  1. added theissue type on Aug 12, 2026
  2. hotlong commented on Aug 12, 2026

    @hotlong
    ContributorAuthor

    Triage: pm:queue, routed domain:cli (packages/rest/src/rest-server.ts), type Bug.

    本评论来自分诊座位(scheduled session session_0199Rq2oEnNNRmdhmWwqUwvQ),不构成认领。


    Generated by Claude Code

  3. self-assigned this
    on Aug 12, 2026
  4. hotlong commented on Aug 12, 2026

    @hotlong
    ContributorAuthor

    Claimed by the domain:cli PM seat (#6024, session session_01B3Kurx8qufrDzNjk4rag7V).
    Branch: claude/issue-7981-security-endpoints-envelope-convergence · dispatch: mode:subagent, model: claude-opus-5.

    Anchor re-verified on origin/main: registerSecurityEndpoints carries respond501 doing res.status(501).json({…}) and respondSharingError doing res.status(status).json({ …, error: msg.replace(…) }) — a bare-string error field, the dialect #7035 retired.

    Ruling: converge on ADR-0112, and do not invent a fourth

    One region, three envelope shapes, on three routes that a single client calls in sequence. Land all three on the standard envelope the rest of the route family already answers in. ⛔ Do not author a new shape "to unify them" — the target already exists.

    Take the shape from a sibling in the same file rather than from the ADR text. This lane merged #7979 into registerSecurityEndpoints today; match what it answers so the region converges rather than acquiring a fourth dialect from a different reading of the same document.

    The one thing to decide carefully

    The bare-string error field is a wire-visible change for any caller currently reading body.error as a string. Determine whether anything reads it — client SDK, console, tests — with a control grep (a query you know returns hits) so a zero-hit result proves absence rather than a broken search. If a reader exists, say so and stop: that turns this from a convergence into a breaking change needing a ruling.

    respond501's status is not in scope — this is about the body shape, not about whether 501 is the right code.

    Pins

    • Each of the three arms asserts the ADR-0112 pair — code and HTTP status. Asserting one is not acceptable.
    • A pin that the three arms now answer the same shape as each other, derived rather than three hand-written literals.
    • Reverse-verify: revert one arm, confirm its test fails on the shape, restore.
    • ⛔ check:type-check-debt must not rise — @objectstack/rest is at 155 and that entry has no margin.

    Generated by Claude Code

  5. hotlong commented on Aug 12, 2026

    @hotlong
    ContributorAuthor
    {
      "issue": 7981,
      "branch": "claude/issue-7981-security-endpoints-envelope-convergence",
      "pr_url": "https://github.com/objectstack-ai/objectstack/pull/8071",
      "status": "needs_decision",
      "premise_still_valid": true,
      "summary": "The three arms of registerSecurityEndpoints now emit the ADR-0112 body { error: { code, message } } through ONE local respondError helper: respond501 (was flat { code, message }), handleError (was { code, error: 'bare string' }) and the ?status validation guard (already conformant, re-routed through the same emitter so it is the reference point by construction). No code value and no HTTP status moves; nothing in packages/spec changes. Implementation, tests, changeset and a DRAFT PR are complete and green. Status is needs_decision because the dispatch reserved the wire-visibility call for the PM: readers of the bare-string `error` DO exist (two), so per the card I report and stop short of the ruling rather than treating the change as landable.",
      "target_shape_source": "packages/rest/src/rest-server.ts — the sibling arm in the SAME file, not the ADR prose: the #7678 ?status guard PR #7979 landed in registerSecurityEndpoints today writing { error: { code, message } }, and refuseRepeatedQueryParams (packages/rest/src/query-multiplicity.ts:139-141) writes the identical body for the whole file. No fourth dialect authored.",
      "arms_converged": [
        "respond501 — 501 NOT_IMPLEMENTED: was { code, message } (no error wrapper)",
        "handleError typed arm — 403 PERMISSION_DENIED / 404 SUGGESTION_NOT_FOUND / 409 SUGGESTION_STATE: was { code, error: 'bare string' }",
        "handleError fault arm — 500 SUGGESTION_{LIST,CONFIRM,DISMISS}_FAILED: was { code, error: 'bare string' }, 500-char cap preserved",
        "?status validation guard — 400 VALIDATION_ERROR: already ADR-0112, re-routed through the shared emitter",
        "refuseRepeatedQueryParams — 400 VALIDATION_ERROR: untouched, shared with the whole file, pinned as the reference point"
      ],
      "bare_string_readers": {
        "found": true,
        "count": 2,
        "verdict": "Both readers are deliberately MULTI-ENVELOPE and read the ADR-0112 position too, so the migrated values arrive identical or better. No reader breaks. The go/no-go is still the PM's — reporting, not deciding.",
        "readers": [
          "packages/client/src/index.ts — ObjectStackClient.fetch, the if (!res.ok) branch (NOT unwrapResponse, which never sees a failure because fetch throws first). Chain: errorMessage = errorBody?.message ?? errorBody?.error?.message ?? (typeof errorBody?.error === 'string' ? errorBody.error : undefined) ?? res.statusText; errorCode = asSemanticCode(errorBody?.code) ?? asSemanticCode(errorBody?.error?.code). Its own comment calls the two reads 'the two LIVE envelopes declared spots, not a fallback chain'. After the change every exposed field is value-identical: 501 message/code move top-level -> nested with the same strings; the typed arm moves off the plain-string limb onto error.message; err.httpStatus comes from the transport; err.details already fell through to the whole body on both flat shapes and still does.",
          "objectui packages/app-shell/src/services/suggestedBindingsApi.ts:41-42 — payload?.error?.code ?? 'HTTP_'+status, and payload?.error?.message ?? payload?.error ?? res.statusText. Canonical-FIRST, so the change strictly improves it: a 403 confirm denial reports HTTP_403 today and PERMISSION_DENIED afterwards, with the same message text. 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 on this one); the panel toasts err.message, unchanged."
        ],
        "control_grep": [
          "objectui branching: grep -rn 'code ===' in the 3 suggested-bindings files -> 0 hits; CONTROL same query over packages/app-shell/src -> 15 hits (RecordApprovalsPanel THROTTLED, DatasetPreview ANALYTICS_NOT_INSTALLED, ...). Zero is absence, not a broken search.",
          "cloud: grep -rn 'suggested-bindings|suggestedBindings' -> 0 hits; CONTROL 'api/v1/security' -> 2 hits (service-cloud/src/cloud-stack.ts:307, apps/cloud/test/control-plane-default-profile.test.ts:505).",
          "objectstack tests: grep -rn 'SUGGESTION_LIST_FAILED|SUGGESTION_CONFIRM_FAILED|SUGGESTION_DISMISS_FAILED' -> 6 hits, all emitter (rest-server.ts) or vocabulary registry (spec/src/api/error-code-ledger.zod.ts); no assertion anywhere. CONTROL 'NOT_IMPLEMENTED' in packages/rest/src/*.test.ts -> 8+ files.",
          "docs/QA: docs/qa/platform-checklist/areas/access-security.json names statuses and codes ('409 SUGGESTION_STATE', '404 SUGGESTION_NOT_FOUND') but never a body POSITION; CONTROL grep 'body.code|body.error|error.code' over that file -> 3 hits, all on OTHER items, proving the query would have found a position clause."
        ]
      },
      "tests_added": "packages/rest/src/security-suggested-bindings-envelope.test.ts (23 cases). Every refusal case asserts the ADR-0112 PAIR — HTTP status AND nested body.error.code — plus both retired dialects absent (no top-level `code` sibling, `error` an object not a string). Arms: 501 on all three routes + the duck-typed 'service predates this surface' variant; typed 403/404/409 on all three routes (table-driven); untyped 500 with each route's own default code; the 500-char cap; the 400 validation and 400 multiplicity refusals. CROSS-ARM PIN IS DERIVED: shapeOf() reduces a body to its sorted key-path/value-type skeleton and the family case asserts all 9 refusals collapse to ONE skeleton without naming it, so a fourth dialect fails even if a matching literal case is added alongside. Preservation: list/confirm/dismiss still 200 with { data } and no `error` key.",
      "tests": "pnpm --filter @objectstack/rest test --maxWorkers=2 -> 'Test Files 98 passed (98) / Tests 1589 passed (1589)'. pnpm --filter @objectstack/rest typecheck (tsc --noEmit) -> clean, no output. pnpm check:type-check-debt (after pnpm exec turbo run build --filter='./packages/*' --filter='./packages/*/*', which the gate demands) -> 'check-type-check-coverage --re-measure: OK — 35 ledger entr(ies) re-measured, 1975 raw tsc error(s) total, none above its recorded number'; @objectstack/rest is absent from the 9-entry 'can be lowered' list, i.e. it re-measured at exactly its recorded 155 — the ledger does not move. pnpm check:error-code-casing -> 'no lowercase error codes in 3740 scanned file(s)'. node scripts/check-nul-bytes.mjs -> OK, 7333 files, no raw control bytes.",
      "reverse_verification": "Fix committed FIRST (083848eeb) so the restore point was a real commit. Reverted the handleError typed arm alone back to res.status(status).json({ code: err?.code ?? defaultCode, error: String(...) }) and re-ran the new suite: 12 failed / 11 passed — all SHAPE failures, no compile error, and the file still type-checked. Representative reds: (1) 'typed 403 list has no nested code: expected undefined to be string'; (2) 'typed 403 list still has a top-level code sibling: expected { code: PERMISSION_DENIED } to not have property code'; (3) the derived cross-arm pin 'expected 1, received 2' distinct skeletons — i.e. it detected the split without being told what the shape is. Direction was as predicted (before-green/after-red), no inversion. Restored with git checkout HEAD -- packages/rest/src/rest-server.ts; git status clean, line re-verified present. git stash never used (worktree does not isolate refs/stash).",
      "open_questions": [
        {
          "question": "Two readers of the bare-string `error` exist, so the card's stop rule fires — but its stated consequence ('this becomes a breaking change') is measurably FALSE for both: each reads the ADR-0112 position FIRST or alongside, and both come out value-identical (SDK) or strictly better (console: HTTP_403 -> PERMISSION_DENIED). Land the convergence, or hold it as a breaking change?",
          "options": [
            "A — flip PR #8071 ready and land it. Both readers keep working with no change on their side; the console gains the real semantic code it currently cannot see; the ADR-0112 D5 destination was already ruled and #7035 retired this dialect in this same file.",
            "B — treat any bare-string reader as breaking and hold for a maintainer ruling / a coordinated objectui change. Costs the convergence for now and leaves body.error.code reading undefined on the arm carrying the typed 403/404/409 codes."
          ],
          "recommendation": "A. The rule's trigger is met but its rationale is not: neither reader can break, because neither is single-dialect — the SDK's own comment declares it reads both live envelopes on purpose, and the console's string limb is the SECOND limb of a canonical-first chain that this change stops reaching. Under Prime Directive #12 those limbs are consumer-side tolerance to be paid down at the producer, which is exactly what this PR does. I have NOT taken that decision: the PR is parked in draft and no ready-flip was made."
        }
      ],
      "out_of_scope_findings": [
        "filed as #8073: the two registrars adjacent to this one in the same file still answer the retired dialects — registerSecurityExplainEndpoints (/security/explain, /security/my-delegable-scope: 401/501/400/403 flat { code, message }, two 500s bare-string) and registerSharingEndpoints' respondSharingError ({ code, error: msg.replace(...) } feeding 400/403/404/409/422). Searched first (keyword + file path) — no existing issue; #7295 and #7035 are the closed relatives. Filed unassigned and unlabeled for PM triage, standalone (not a sub-issue: neither fix falls inside this card's completion scope), and it does not block on this PR."
      ],
      "risks": [
        "Wire shape change on three live routes. Mitigated by the two-reader measurement above and stated plainly in .changeset/security-endpoints-envelope-convergence.md, which names the bare-string field as the wire-visible half.",
        "The claim comment's anchor named `respondSharingError` as living in registerSecurityEndpoints; measured, it is at rest-server.ts:9218 inside registerSharingEndpoints, one registrar earlier — the security family's bare-string emitter is `handleError` (the issue BODY named it correctly). The premise holds; only the paraphrase pointed at the neighbour, which is now filed as #8073. Nothing was fixed in the sharing family.",
        "objectui carries no test that stubs these wire bodies (its two touching tests mock the panel component wholesale), so nothing on that side goes red either way — the console's improvement is measured by reading the chain, not by a failing test. If the PM wants that pinned, it is an objectui-side card.",
        "Local verification was scoped per the dev contract (rest package + the gate families this card touches: type-check-debt, error-code-casing, nul-bytes) rather than the full local farm; CI convergence is the PM's read."
      ]
    }

    Generated by Claude Code

  6. hotlong commented on Aug 12, 2026

    @hotlong
    ContributorAuthor

    os-dev-report

    {
      "issue": 7981,
      "status": "done",
      "branch": "claude/issue-7981-security-endpoints-envelope-convergence",
      "pr": "https://github.com/objectstack-ai/objectstack/pull/8071",
      "premise_still_valid": true,
      "summary": "PR #8071's red 'ESLint' job was actually pnpm check:route-envelope refusing to let banked progress go unrecorded: the PR's conversion of registerSecurityEndpoints' two handleError arms onto the shared respondError helper moved 2 sites off the siblingCode dialect (found 75, declared 77). Confirmed the delta is this PR's own (not a stale-main artifact) by measuring siblingCode at the merge-base with origin/main (77) vs branch head before this commit (75) -- exact delta of 2 -- then diffing site line numbers under the PR's own uniform line-shift: every other siblingCode site persists unmoved, and the only two that vanished are merge-base lines 9529/9532, both inside registerSecurityEndpoints and both converted by this PR's diff. Lowered the declared siblingCode for packages/rest/src/rest-server.ts from 77 to 75 in scripts/check-route-envelope.mjs MODULES (one-line change plus a note), committed, and pushed to the existing branch. No numbers raised, no envelope-divergent sites reintroduced, releases/ untouched, PR left in draft, no new changeset (this PR already carries one).",
      "tests": "pnpm check:route-envelope (self-test + real audit): green -- '9 route module(s) audited: 7 conformant, 1 ratcheted, 1 exempt', ratchet line now reads 'siblingCode 75'. pnpm --filter @objectstack/rest test: 'Test Files 98 passed (98) / Tests 1589 passed (1589)'. pnpm --filter @objectstack/rest typecheck (tsc --noEmit): clean, no output. pnpm exec turbo run build --filter='./packages/*' --filter='./packages/*/*': 70 successful/70 total (closure needed before type-check-debt). pnpm check:type-check-debt: 'OK -- 64/77 workspace packages type-checked ... none above its recorded number'; @objectstack/rest's TEST_DEBT ceiling (155, zero margin) is absent from the 'can be lowered' informational list, i.e. unchanged and not raised. node scripts/check-nul-bytes.mjs: OK. Merge-base vs head siblingCode measurement (via scanSource copied out of the gate script, run against git show <rev>:packages/rest/src/rest-server.ts): merge-base e3c8ed0f8 -> 77, branch head 5eb732c84 (pre-fix) -> 75; delta 2, matching the gate's own report; the two vanished sites (merge-base lines 9529, 9532) fall inside registerSecurityEndpoints, the exact function PR #8071's diff touches.",
      "open_questions": [],
      "out_of_scope_findings": []
    }

    Generated by Claude Code


    Generated by Claude Code

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

Type

Projects

No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions