Repository navigation
fix(app-shell): the package permission door refuses the save when its re-read rejects (objectui#9420) - #9483
Merged
Conversation
…ejects
`PermissionMatrixEditPage.doSave`'s package door merges the edited slice onto
a fresh `client.layered` read so rows contributed by other packages survive
byte-for-byte (ADR-0086 P0). The read was `.catch(() => null)` with
`base = fresh?.effective ?? payload`, and on the rejecting arm the guarantee
inverted: `payload` is the draft the load path already sliced down to this
package's objects, so `mergePermissionSlice` had no out-of-scope rows left to
copy and the write PUT a record with every other package's permission rows
deleted — 200, no error, no warning.
A rejected re-read now raises on the page's existing error strip (the channel a
failed `client.save` already uses) and nothing is written. Only a REJECTION
refuses: a record the server does not hold answers the 404 shape, which
`MetadataClient.layered` resolves as `{ effective: null, … }`, and that arm
keeps the `?? payload` base so the first save of a draft-only set is unchanged.
The environment (whole-record) door never took this path and is untouched.
`PermissionMatrixEditor.packageDoorRereadRefusal.test.tsx` drives the real page
through `doSave` on the rejecting arm and asserts `client.save` was not called,
with a resolving-arm control that PUTs through the same harness.
`readDecorationStrip.test.tsx` kept its subject and its assertions but moved to
the other entrance of the `?? payload` fallback, because the entrance it used
no longer reaches a PUT.
Co-Authored-By: Claude <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_011QreXiyMEqKLN4U5daMPVa
Waiting only on the refusal text made the pre-fix reading a lookup TIMEOUT rather than the PUT it actually is; waiting only on `layeredCalls` would race the save that follows it. Settling on "refusal rendered OR a PUT landed" makes the failure print the body the old code wrote — the one missing `b_order` and `b_order.total`, which is the card's measured probe. Co-Authored-By: Claude <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011QreXiyMEqKLN4U5daMPVa
Contributor
✅ Console Performance Budget
The eager closure is every chunk the entry reaches through static imports — what the browser fetches and parses before the app renders. The entry chunk on its own is a small fraction of it. 📦 Bundle Size Report
Size Limits
|
os-tesla
marked this pull request as ready for review
September 14, 2026 10:14
os-tesla
deleted the
claude/issue-9420-permission-door-reread-refusal
branch
September 14, 2026 10:34
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #9420
What was wrong
PermissionMatrixEditPage.doSave's package door documents an ADR-0086 P0guarantee in its own comment: merge only this package's slice back onto a fresh
read of the record, so rows contributed by other packages survive byte-for-byte.
The fresh read was
client.layered(...).catch(() => null)and the merge base wasfresh?.effective ?? payload.On the rejecting arm the guarantee inverted.
payloadis the editor's owndraft, which the load path already sliced down to this package's objects, so
mergePermissionSlicehad no out-of-scope rows left to copy and the write PUT arecord with every other package's permission rows deleted — 200, no error, no
warning, on a security surface.
What changed
A rejected re-read now refuses the save. Nothing is PUT, and the refusal
reaches the author on the page's existing error strip — the same
setErrorchannel a failed
client.savealready renders into. No new toast path, no retryadded (the client offers no retry idiom at this call site, and a retry would
narrow the window rather than restore the guarantee). The rejection stays inside
.catch, so it can never escape as an unhandled one; the refusal is raised insidedoSave's owntryand caught by the handler that was already there.One new i18n key,
perm.save.rereadFailed, in both theenandzhtables ofthe metadata-admin designer table.
What is deliberately NOT refused — and why it matters
Only a rejection refuses.
MetadataClient.layeredresolves a 404 as{ effective: null, … }rather than rejecting, and that arm keeps the?? payloadbase on purpose: a set that exists only as a package draft — what theStudio Access pillar's "+ New" creates, before any publish — has no published rows
for anyone to lose, and refusing there would make the first save of every newly
created permission set impossible. Pinned, so the refusal cannot widen onto it by
accident.
The environment (whole-record) door never took this path at all: with no
packageIdthere is no slice merge and no pre-save re-read. Pinned as a guard inthe same file — same client, same edit, same click, only
packageIddiffers — andit still PUTs the whole record with
force: falseand no draft options.Before / after — from the real run output, not an editor exit code
PermissionMatrixEditor.packageDoorRereadRefusal.test.tsxdrives the REAL page(load effect, real facet editors, real
doSave) withlayeredresolving for theload and rejecting for the save-time re-read.
BEFORE — the two production files reverted to
ff1d5ea8d(the branch point)with the new tests in place.
Test Files 1 failed | 2 passed (3),Tests 1 failed | 7 passed (8):That printed body is the card's one-shot probe, independently reproduced:
objectskeys are["a_account"]alone,fieldsis empty,b_orderandb_order.totalare gone.AFTER — same six files, fix in place:
Test Files 6 passed (6),Tests 24 passed (24).Mutation and restoration were both proved on disk rather than read off an editor's
exit code: the mutated file's
git hash-objectdiffered from its HEAD blob, theremoved text grepped to 0 occurrences, and after the restore
git statuswas emptyand
git hash-objectmatched the HEAD blob again for both files. The revert ranagainst the committed fix, so the red and green numbers come from real trees.
One sibling pin had to move, and it was measured rather than assumed
PermissionMatrixEditor.readDecorationStrip.test.tsx(landed with objectui#8181)reached the
base = payloadfallback through the rejected re-read, and assertedon the body that was PUT. This change removes that PUT, so its entrance no longer
exists. It was not weakened and not deleted: its subject, its assertions and
its controls are unchanged, and it now reaches the very same
?? payloadfallbackthrough the other entrance — the 404 shape described above, which
doSavestillsaves on.
Measured, so the retarget is not merely asserted: with the strip removed at the
unwrap (
stripReadDecorations(count 1 to 0 on disk, blob hash changed), theretargeted pin goes red with
expected { name: 'sales_perms', …(8) } to not have property "_diagnostics", and it is green again after restore with the file byteidentical to its HEAD blob. It still fails for exactly the reason it was written.
PermissionMatrixEditor.packageDoorFacets.test.tsx— the arm the card names asalready holding — is untouched and green.
Verification
Run from the repo root, in a dedicated worktree, all through the shared verify lock.
pnpm exec vitest runover the six permission filesTest Files 6 passed (6),Tests 24 passed (24)pnpm exec vitest run packages/app-shell/(the whole changed package)Test Files 703 passed (703),Tests 6889 passed | 1 skipped (6890)pnpm --filter @object-ui/app-shell run type-check(tsc --noEmit+tsconfig.test.json)pnpm --filter @object-ui/app-shell run lintcheck:i18n-keys·check:i18n-drift·check:i18n-dead-keys·check:i18n-designer-paritycheck:control-bytes·check:new-line-citationscheck-vi-mock-specifiers·check-vi-mock-inherit·check-vi-mock-override-shape·check-test-path-rootscheck-changeset-presence·check-changeset-no-major·check-changeset-overwritecheck-governed-queue-guard --testover this PR's five pathsThe dependency closure was built first (
pnpm --filter '@object-ui/app-shell^...' build),because an unbuilt closure makes
type-checkreport sixteen phantomCannot find module '@object-ui/…'errors that say nothing about this diff.Acceptance notes
Noted while reading the code, not filed and not fixed here:
The
?? payloadfallback has two entrances, and the card names only one.The second is a resolved-but-empty envelope (
effectivenullish), which is the404 shape. It is correct behaviour for a record the server does not hold, which
is why it is kept — but the card's prose reads as though rejection were the only
way in. Recorded here so the next reader of
doSavedoes not re-derive it.The card and the claim comment both cite the guarantee docblock at
PermissionMatrixEditor.tsx:670. Onff1d5ea8dthat line is a differentADR-0086 P0 mention, inside
bulkSetObject; the guarantee this PR is about sitsin
doSave, immediately above the merge. The two other cited lines (thebasefallback and the
mergePermissionSlicecall) matched exactly. This is AGENTS.mdcommandment [WIP] Update documentation for project #11 demonstrating itself.
Filed rather than noted, because it is a reproducible defect in its own right:
objectui#9484 — at environment scope a rejected post-save
layeredreadsurfaces on the same error strip after a write that succeeded, and skips
resetDraftBaseline, so the editor also keeps claiming unsaved changes for arecord that is already persisted. Pre-existing, untouched by this PR (the ruling
scoped this change to the package door), and reached by this PR's own
environment-door guard test.
Serial constraint checked: objectui#7727 is
pm:dispatchedon the same directorybut on
ConditionalFormattingEditor.tsx. No overlap — this PR touchesPermissionMatrixEditor.tsx,i18n.tsand two permission test files.Clause-② stays no: nothing new is exported from
@object-ui/app-shell, nopublished type moved, no authoring surface widened.
packages/app-shell/src/views/metadata-admin/index.tsis not touched, and
permission-slice.tswas read-only throughout — the refusalbelongs to the caller that chooses the merge base, not to the merge itself.
Session:
https://claude.ai/code/session_011QreXiyMEqKLN4U5daMPVaGenerated by Claude Code
Generated by Claude Code