Skip to content

fix(rest): the record-share family carries a demoted producer code on declaredCode, and the comment that said it could not is repaired (#12510) - #12670

Merged
os-litant merged 1 commit into
mainfrom
claude/issue-12510-sharing-error-declared-code
Aug 27, 2026
Merged

fix(rest): the record-share family carries a demoted producer code on declaredCode, and the comment that said it could not is repaired (#12510)#12670
os-litant merged 1 commit into
mainfrom
claude/issue-12510-sharing-error-declared-code

Conversation

@os-litant

@os-litantos-litant commented Aug 27, 2026

Copy link
Copy Markdown
Collaborator

Fixes#12510

The record-share family drops the producer's own error-code spelling, and the
comment explaining why says the block is upstream. Both halves are repaired
here, because leaving either one costs the next reader something different: the
drop is a live defect, and the sentence has already stopped one repair.

Measured on 9a2f3dfe4 (the ref this branch was cut from), not inherited

The card anchored the comment at :9657-9661; PR #12421 landed on this file
after that, so I re-measured. Everything below is my own reading.

Claim on the cardWhat I measured
the comment still exists verbatimyes, byte for byte — but at packages/rest/src/rest-server.ts:9654-9660, not the card's :9657-9661. ⛔ I do not reconcile the two: mine is the measurement, taken on 9a2f3dfe4
#11719 / db8c288 widened the Pickyesdb8c2881e "fix(types): let sendError's extra carry declaredCode (#12403)", found with git log -S"'declaredCode'" -- packages/types/src/response-envelope.ts; the Pick now names category ǀ httpStatus ǀ details ǀ requestId ǀ declaredCode ǀ userMessage
the defect is still liveyes — reproduced below

⚠️ One correction to the card's own re-check recipe:
git grep -n "does not accept that field" -- packages/rest/src/rest-server.ts
returns zero on a tree where the comment is present, because the phrase
wraps across a comment line break (… does not / // accept that field). A
zero there is not evidence of removal. The anchor that works is
Measured and NOT repaired here (1 on 9a2f3dfe4, 0 here). Reverse control for
both greps: sendError answers 7 in the same file on the same ref.

The drop, reproduced before it was repaired

One producer, both doors, on the unrepaired tree:

throw { code: 'CLOSE_PERIOD_LOCKED', status: 409 }
share door : 409 {"success":false,"error":{"code":"RESOURCE_CONFLICT","message":"invoices still open"}}
/data door : 409 {"error":"invoices still open","code":"RESOURCE_CONFLICT","declaredCode":"CLOSE_PERIOD_LOCKED"}

respondSharingError asks classifiedRefusalAnswer — the flat /data door's
own classification, which is already holding the demoted spelling — and
re-dressed only status, code and the message into the nested ADR-0112 D5
envelope. Nothing invalid shipped, since the closed code still carried the
member the status derives, which is what made the loss silent and
one-directional.

⭐ The call-site shape here differs from PR #12508, and why

PR #12508 (#12405) holds a ThrownHttpError and passes
demotedDeclaredCode(thrown). This call site does not hold one. It holds
classifiedRefusalAnswer's { status, body } — the flat answer whose code
and message it is already re-dressing. So the shape here is:

constdeclaredCode=typeofrefusal.body.declaredCode==='string'
? refusal.body.declaredCode
: undefined;respondError(res,refusal.status,code,String(refusal.body.error??''),declaredCode!==undefined ? { declaredCode } : undefined,);

This is not a second definition of the demote rule and it is not the raw
field the twin's ruling bars. body.declaredCode is the shared rule's OUTPUT:
the flat door computes it in thrownCodeFields (error-response.ts, #9232),
which is literally resolveThrownHttpError(error, status) followed by
demotedDeclaredCode. Presence therefore already means demotion, and the pair
code + declaredCode leaves this door exactly as that rule produced it.

Measured justification for reading the classification rather than calling the
resolver a second time at the re-dress:

  • The raw field really is the trap. A throw of { code: 'RECORD_LOCKED', status: 409 } resolves to code: 'RECORD_LOCKED'with
    declaredCode: 'RECORD_LOCKED' beside it — forwarding that would put two
    spellings of one fact on every registered refusal. The pinned answer is
    absence, and §2 of the new suite asserts it directly.
  • Asking the resolver again would be a second answer to a question this door
    deliberately asks once. classifiedRefusalAnswer exists so this family
    "cannot answer a refusal differently from every other face that catches it";
    a second read is the shape that let the two /api/v1/packages doors drift
    apart in the first place. §5 pins the equivalence from the outside instead, so
    either side drifting turns it red.
  • I measured all three candidate reads across 14 producer shapes — the
    classification's field, demotedDeclaredCode(resolveThrownHttpError(e, status))
    and the same with no status fallback. They agree on every shape measured.
    Reported as measured: my prediction that the no-fallback form would diverge on
    a status-derived member was WRONG — ledger membership does not depend on
    status, so the demote does not either. The choice above is therefore made on
    the one-classification-two-dialects ground, not on a measured divergence.

respondError's new optional parameter is typed from the shared writer's own
extra type — Parameters of sendEnvelopeError at index 4 — referenced
rather than restated, so this wrapper keeps deciding POSITION only and a locally
narrowed copy cannot quietly stop forwarding the next channel admitted there.

⚠️ That type's exact spelling carries angle brackets and this description cannot
hold them: GitHub's body sanitiser strips a short bracketed run even inside
backticks, and it silently ate this one from the first version of this text.
Read the type in the diff, not here.

The comment

The stale sentence is replaced by what is true now: the repair, the reason the
old reason is gone (db8c288), the ⛔ against reading the resolver's raw field
with the RECORD_LOCKED measurement that shows why, and the ⭐ for re-dressing
rather than re-resolving.

Tests — packages/rest/src/rest-share-declared-code.test.ts (16 cases)

Every case drives the real route through RestServer.registerRoutes() on
all three seams (GET/POST /shares, DELETE /shares/:shareId) with a
witness that the service verb was actually called, so a case that never reached
the seam cannot pass on a body it got for another reason.

  • §1 four demoting shapes carry the exact spelling (both status and
    statusCode, plus the sandbox limb with and without a declared status)
  • §2 absence unless the demote happened — the RECORD_LOCKED trap case,
    plugin-sharing's own FORBIDDEN gate, a sandbox body with no code, and the
    ADR-0111 prefix idiom. 'declaredCode' in error is false, not
    === undefined
  • §3 door-to-door: both doors answer the same spelling for the same producer —
    the reproduction turned into a pin, with an anti-vacuity case proving the
    /data door really answers both ways
  • §4 what must not move: message and closed code byte-identical, the 500
    terminal (SHARES_LIST_FAILED / SHARE_GRANT_FAILED / SHARE_REVOKE_FAILED)
    unchanged, and a non-string numeric code still context rather than a wire
    spelling (The dispatcher puts the HTTP status in error.code and parks the real code in details — pinned in #3687, still unfixed #3842)
  • §5 the wire answer IS demotedDeclaredCode, plus its own anti-vacuity case

Ablation

Predicted before running: §1 RED 4, §2 GREEN, §3 RED 1 of 2, §4 GREEN, §5 RED 1
of 2 → 6 red. Measured: 6 failed / 10 passed of 16 — exactly those six. The
pin names itself:

FAIL src/rest-share-declared-code.test.ts > [#12510] an UNREGISTERED producer spelling
rides `declaredCode` on the share family > an app spelling on a declared 409 (`status`)
AssertionError: GET shares: {"success":false,"error":{"code":"RESOURCE_CONFLICT",
"message":"invoices still open"}}: expected undefined to be 'CLOSE_PERIOD_LOCKED'

The mutation was confirmed on disk before any verdict was read: anchored
counts (declaredCode !== undefined ? { declaredCode } 1 → 0, the restored
pre-fix one-line respondError(…) 0 → 1) plus a blob hash differing from the
HEAD blob. Restore ran under a trap on EXIT INT TERM with absolute paths
and git checkout HEAD -- ABSOLUTE_PATH (never the bare -- form), and is
proven by the working blob matching HEAD:packages/rest/src/rest-server.ts
(1d1ab6604…) with an empty git diff HEAD — not by an exit code. No rebuild
leg applies: the suite imports the subject as ./rest-server.js, a same-package
relative specifier vitest resolves to SOURCE, not through exports to dist/.

Verification, at the final commit f2063854a

Heavy runs went through os-verify-lock (OS_VERIFY_LOCK_SLOT=issue-12510);
exit codes captured before any pipe.

  • pnpm --filter @objectstack/rest exec vitest run150 files / 2460 tests
    passed
    , VERDICT command-exit 0
  • pnpm --filter @objectstack/rest typecheck — clean; check:test-typecheck
    OK, 9 files / 20 errors held in test-typecheck-debt.json, unchanged (the new
    suite is inside the test project: tsconfig.test.json includes src/**/*,
    while the build config excludes **/*.test.ts)
  • gate families derived with node scripts/pm/dispatch-gates.mjs --repo objectstack-ai/objectstack (never a hand-built path list) — all 27 derived
    path/kind gates exit 0, including check:route-envelope,
    check:dispatcher-error-vocabulary, check:published-files,
    check:engine-double-contract, check:where-matcher,
    check:cross-package-test-inputs, check:type-check-coverage and the
    changeset family
  • pnpm check:type-check-debt — refused first as NOT MEASURED without the built
    closure, so the closure was built (turbo run build --filter='./packages/*' --filter='./packages/*/*', 70/70) and it then reported OK: 31 ledger entries
    re-measured, none above its recorded number
  • node scripts/check-nul-bytes.mjs — OK over 7027 files
  • ⚠️node scripts/pm/check-half-states.mjs exits 3 = NOT MEASURED from this
    seat (nothing swept: the REST issues API answers 403 here). Recorded as
    unmeasured, not as green

Changeset

.changeset/share-door-declared-code.md, @objectstack/rest: patch — ⛔ not
skip-changeset. This is published source and it changes what goes on the wire:
a consumer that ADR-0112 tells to read declaredCode starts receiving it on
this family, so the CHANGELOG is where an upgrading consumer learns the channel
arrived. Additive, and NOT breaking — ApiErrorSchema.declaredCode has been
z.string().optional() since #9106, so the contract's accept set does not move
and no ADR-0087 disposition is required.

Contract review

The PM ruled clause ② does not apply, and invited the ruling to be overturned by
measurement. My measurement agrees with the ruling: the accept set does not
widen (the schema already permits the field, and check:route-envelope and the
suite's ApiErrorSchema.safeParse both stay green), no consumer shape breaks
(no existing key moves or changes value on any path — §4 pins that), and the
only observable delta is an optional field appearing where ADR-0112 says it
belongs, on shapes that previously carried nothing there.

Out of scope, filed not fixed

The same re-dress still drops two other declared channels the flat /data door
carries for the same producer — userMessage and the structured context
(issues). Both measured on this branch; filed rather than folded in, since
each is a different declared channel with its own posture. Filed as #12669,
unassigned — and note the two halves differ in difficulty: userMessage is the
same one-line re-dress, while the flat issues to nested details mapping is a
shape decision, not a rename.


Generated by Claude Code

… `declaredCode` (#12510)
`respondSharingError` asks `classifiedRefusalAnswer` — the flat `/data` door's
own classification, which already holds the demoted producer spelling — and
re-dressed only `status`, `code` and the message into the nested ADR-0112 D5
envelope. The author's own code was dropped one line below the local that
carried it: nothing invalid shipped, since the closed `code` still carried the
member the status derives, which is what made the loss silent and
one-directional.
The pair is carried, not recomputed. `refusal.body.declaredCode` is the shared
rule's OUTPUT (`thrownCodeFields` = `resolveThrownHttpError` +
`demotedDeclaredCode`), so "presence means demotion" arrives with the value; the
resolver's RAW `declaredCode` is deliberately not read, because it is set for a
REGISTERED spelling too and forwarding it would put two spellings of one fact on
every registered refusal.
The comment that explained the omission is repaired in the same change. It said
`sendError`'s `extra` would not accept the field — true when written, false
since `db8c288` (PR #12403) added `declaredCode` to the writer's `Pick`. Left
alone it reads as "blocked upstream, nothing to do here" to the next author,
which is the direction that costs a repair rather than an understanding.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01UjujZN219uFzBhSYfMykCd
@github-actions

Copy link
Copy Markdown
Contributor

📓 Docs Drift Check

This PR changes 1 package(s): @objectstack/rest, touching 1 documentable anchor(s).

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

  • content/docs/kernel/contracts/metadata-service.mdx(via /api/v1/packages (route))
  • content/docs/kernel/services-checklist.mdx(via /api/v1/packages (route))
  • content/docs/permissions/permission-sets.mdx(via /api/v1/packages (route))
  • content/docs/protocol/kernel/http-protocol.mdx(via /api/v1/packages (route))
  • content/docs/ui/apps.mdx(via /api/v1/packages (route))

1 release-owned page(s) also name something this change touched. These are read-only:

  • content/docs/releases/v17.mdx(via /api/v1/packages (route))

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.

What this run could not see
  • 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.

Coarse fallback — 13 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 68bf4efc6ac3e18385496432012eaf9aba09cc39packageMentionDocs.

Which tree this was computed on

This run read content/docs from 63851fea12e0d604c186490feab3d929e3c17ce9 — the merge of head f2063854aeadd58e8112e89a65cdbe5580b88203 into base 68bf4efc6ac3e18385496432012eaf9aba09cc39, 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 63851fea12e0d604c186490feab3d929e3c17ce9 && git checkout 63851fea12e0d604c186490feab3d929e3c17ce9
# afterwards, rebuild it from the two parents, which stay fetchable
git fetch origin 68bf4efc6ac3e18385496432012eaf9aba09cc39 f2063854aeadd58e8112e89a65cdbe5580b88203 && git checkout -B drift-repro 68bf4efc6ac3e18385496432012eaf9aba09cc39 && git merge --no-ff f2063854aeadd58e8112e89a65cdbe5580b88203
node scripts/docs-audit/affected-docs.mjs --json 68bf4efc6ac3e18385496432012eaf9aba09cc39

⚠️ 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 68bf4efc6ac3e18385496432012eaf9aba09cc39 → pass the list as
args.docs, on the commit named under Which tree this was computed on.

@github-actionsgithub-actionsBot added documentation Improvements or additions to documentation tests tooling labels Aug 27, 2026
@os-litant
os-litant marked this pull request as ready for review August 27, 2026 06:23
@os-litant
os-litant added this pull request to the merge queueAug 27, 2026
Merged via the queue into main with commit 1e4d2ebAug 27, 2026
37 checks passed
@os-litant
os-litant deleted the claude/issue-12510-sharing-error-declared-code branch August 27, 2026 07:01
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

documentationImprovements or additions to documentationsize/lteststooling

Projects

None yet

2 participants

@os-litant@claude