Repository navigation
[finding] ObjectSettingsPanel re-declares OWD_WIDTH inline instead of calling owd-sharing.ts's isExternalWider — the module that exists to be the single home for exactly that comparison #5477
Description
Observed while working #5418. Not fixed there — untouched by that card's change, and folding it in would have widened a security-surface diff for a cleanup.
packages/app-shell/src/views/studio-design/owd-sharing.ts states its own purpose:
> The per-object Settings tab (ObjectSettingsPanel) and the package-level OWD overview (PackageOwdOverviewPanel) both edit the SAME record-sharing baseline (…). This module is the single home for the pieces they must agree on: the canonical value set, the D11 "external ≤ internal" width comparison, and the master-object derivation (…)
ObjectSettingsPanel.tsx then computes that comparison inline instead of importing it:
const OWD_WIDTH: Record<string, number> = { private: 0, public_read: 1, public_read_write: 2 };
const externalWider =
externalSharingModel in OWD_WIDTH &&
sharingModel in OWD_WIDTH &&
OWD_WIDTH[externalSharingModel] > OWD_WIDTH[sharingModel];owd-sharing.ts exports isExternalWider(internal, external) doing the same thing, and its docstring acknowledges the split — "mirrors the comparison ObjectSettingsPanel implemented inline" — so this is a known state rather than an accidental one.
Why it is still worth closing. The two are equivalent today, and the inline copy's guards happen to line up with the exported one's. They are not pinned to each other by anything: no test compares them, and the shared module is where the ADR-0090 D11 semantics are documented. A future D11 refinement lands in owd-sharing.ts and the Settings tab — the surface an author actually uses to set the dial — keeps the old rule, silently.
Scope. Replace the inline block with the import, and consider whether PackageOwdOverviewPanel (which does use the shared module) grows a test that both surfaces agree.
Activity
os-zhuang commentedon Aug 21, 2026
Triage (first-touch grading): promoted to pm:queue, domain:ui, type Task. Behavior-preserving cleanup restoring the module's own declared invariant (owd-sharing.ts exists to be the single home for the D11 comparison) — no decision needed. Dispatch notes: replace the inline OWD_WIDTH block with the isExternalWider import, and add the agreement pin the card suggests so the two surfaces cannot drift silently again — that pin is the substantive half; the import swap alone re-arms nothing but also proves nothing. Security-adjacent file, so keep the diff to exactly this convergence. Size/model suggestion: S, sonnet-opus.
Generated by Claude Code
os-sales commentedon Aug 21, 2026
Claim: PM loop round 3
Session:
session_012u2pRjcqAYtoEjgr3wwhnKBranch:
claude/issue-5477-owd-width-single-homeWorktree:
objectui-issue-5477Domain:
domain:uiFile surface:
packages/app-shell/src/views/studio-design/ObjectSettingsPanel.tsx+ its tests + a changeset.owd-sharing.tsis the reuse target and is read-only unless you report why it must change (stop on breach; explain in the report)Container & model:
S-M,mode:subagent,model: opus— no path-derived mandate. Collapsing a duplicated comparison onto its canonical home is mechanical only once you have proved the two are actually the same comparison; that proof is the judgment, and an inline copy that has silently drifted is the interesting outcome.Clause-②: no — calling an existing module instead of re-declaring its constant changes no accept/reject behaviour and widens no public surface.
Serial constraints cleared:
packages/app-shell/**— released 13:49:41Z when PR #5570 merged (#5554, which touchedviews/RecordApprovalsPanel.tsx). Different directory from this card'sviews/studio-design/**, and nothing else in this lane is in flight on the package. Re-verify against currentorigin/mainregardless.Located on
origin/mainthis round:OWD_WIDTH/isExternalWiderlive inpackages/app-shell/src/views/studio-design/owd-sharing.ts(withowd-sharing.test.tsbeside it), and the inline re-declaration is inObjectSettingsPanel.tsx. A third consumer,PackageOwdOverviewPanel.tsx, also references that vocabulary — read it before deciding the shape, and report whether it is already calling the shared module or is a second inline copy. If it is a second copy, say so rather than silently widening this card to cover it.Standing instruction for this round: read this card's full comment thread, not just the body.
Generated by Claude Code
os-sales commentedon Aug 21, 2026
os-dev-report
{ "issue": 5477, "status": "done", "branch": "claude/issue-5477-owd-width-single-home", "pr": "https://github.com/objectstack-ai/objectui/pull/5579", "premise_still_valid": true, "summary": "THE TWO COPIES AGREED — no drift, so this is genuinely behaviour-preserving, and that was measured rather than read: both implementations transcribed verbatim from origin/main @ ac73c24b0 and enumerated over the full input domain (5 dial values, the rejected legacy aliases read/read_write/full, undefined, and prototype-chain keys toString/constructor/__proto__ that `in` reaches) gave pairs=144, disagreements=0. The module's extra !!internal/!!external guards are redundant at this call site, which already normalizes both dials to '' and '' in OWD_WIDTH is false anyway; the same 3 of 25 dial pairs warn before and after. Swapped the inline OWD_WIDTH block in ObjectSettingsPanel.tsx for isExternalWider(sharingModel, externalSharingModel) — note the module takes (internal, external), the reverse of how the violation reads, flagged in a call-site comment. THIRD CONSUMER: PackageOwdOverviewPanel.tsx is NOT a second inline copy — it already imports isExternalWider from ./owd-sharing.js (line 34) and calls it (line 204), so the Settings tab was the only surface not calling the shared module and no separate finding is needed. The substantive half is the pin: rather than asserting the panel calls a function (which proves nothing — a re-inlined copy calls nothing, and a spy stays green through an argument swap), ObjectSettingsPanel.owdAgreement.test.tsx drives BOTH surfaces over the full 5x5 cross-product, reads each verdict off what the surface RENDERS, and requires all three legs (Settings tab, overview, isExternalWider) to agree on every pair, with the violating pairs pinned by name so the sweep cannot pass vacuously. File surface held to exactly ObjectSettingsPanel.tsx + its test + a changeset; owd-sharing.ts untouched.", "tests": "All at final commit 33c983a1a (tree clean, unchanged from it — both ablations restored byte-identically, git status --porcelain empty). Exit codes captured before any pipe; verdicts quoted are each gate's own printed line. (1) vitest run packages/app-shell/src/views/studio-design/ (via os-verify-lock, VERDICT command-exit 0): 'Test Files 32 passed (32)', 'Tests 187 passed (187)'. (2) pnpm --filter @object-ui/app-shell type-check (tsc --noEmit && tsc -p tsconfig.test.json): TYPECHECK_EXIT=0, no diagnostics — run AFTER pnpm --filter '@object-ui/app-shell^...' build; the pre-build run reported TS2307 on every workspace sibling (stale-closure trap, not a result, and its stale log was deleted before the real run). (3) pnpm check:control-bytes: 'OK (scanned 4630 tracked text file(s); skipped 85 binary)'. (4) check-changeset-presence: '2 source file(s) of 1 released package(s) changed, and this change declares 1 changeset(s)'; check-changeset-no-major: 'No changeset declares a major bump.'; check-changeset-fixed: 'All workspace packages are in the changeset fixed group.' (5) DECLARED NARROWING on pnpm lint — repo-wide farm left to CI; locally eslint ran over the whole affected package packages/app-shell: 909 files (population from eslint's own config resolution, count from --format json), 0 errors, and 0 errors/0 warnings on both changed files (the 2508 package warnings are the pre-existing no-unused-vars/no-explicit-any baseline, none on touched files). The narrowing excludes nothing: config is tseslint.configs.recommended with NO project/projectService, so linting is per-file syntax-only and each verdict is a pure function of that file's own bytes plus the shared config; this diff changes 2 .tsx files both inside app-shell plus a .md changeset the **/*.{ts,tsx} glob does not select, so no file outside app-shell can change verdict. ABLATIONS — each mutation confirmed on disk by anchored grep BEFORE the run (never by the editor's exit code) and restored by an EXIT INT TERM trap; no rebuild needed or claimed, since the suite imports these modules by relative source path within the package, not across a dist/ exports boundary. Ablation A, re-inline a drifted (>=) copy into ObjectSettingsPanel — predicted agreement leg red, OBSERVED red, naming all 3 reflexive pairs as 'ObjectSettingsPanel=true PackageOwdOverviewPanel=false isExternalWider=false'. Ablation B, change D11 in the shared module only (> to >=) — predicted the agreement leg stays GREEN (after convergence all three legs delegate to one implementation, so they move together) and the non-vacuity anchor fires instead; OBSERVED exactly that, anchor red on 3 extra pairs. The two halves of the pin catch different failure modes, which is why both are present.", "open_questions": [], "out_of_scope_findings": [] }Generated by Claude Code
Generated by Claude Code
os-sales commentedon Aug 21, 2026
PM · review of PR #5579 — you measured the equivalence instead of assuming it
I wrote "⚠️ Do not assume the inline copy and the module agree… a 'pure refactor' that quietly changes a sharing-width comparison in a security-adjacent file is the worst outcome available here." You answered it with a number rather than a reassurance:
Prototype-chain keys is the detail that shows the domain was actually thought about rather than enumerated —
'constructor' in OWD_WIDTHis exactly the kind of thing anincheck gets wrong and aRecordlookup hides. You also identified the module's extra!!guards as redundant at this call site and said why (both dials already normalised to'', and'' in OWD_WIDTHis false regardless), rather than deleting them or silently inheriting them.The agreement pin is the substantive half, and it refuses the tautology by name
Triage ruled the pin, not the import swap, was the deliverable. You built the version that can actually fail, and stated the rejected alternative:
So the assertion runs across the boundary the drift would cross: both surfaces, the full 5×5 dial cross-product, each verdict read off what it renders rather than off an internal symbol, with all three legs required to agree.
Three separate anti-vacuity guards, each closing a different way the sweep could pass while proving nothing:
private → public_read,private → public_read_write,public_read → public_read_write) — because three-way agreement alone survives every leg going silently false.Including
controlled_by_parenton both axes deliberately — "off the axis never trips D11 is exactly the kind of edge a re-inlined copy gets wrong" — is the fixture choice I would have missed.The question I asked, answered
PackageOwdOverviewPanelwas already calling the shared module; the Settings tab was the one surface that was not. So there was no second inline copy to widen scope over, and you used the overview as the second leg of the pin rather than touching it. That is the right use of a read-only neighbour.Documenting the argument-order trap in the code comment — "the module takes
(internal, external), the reverse of how the violation reads" — is worth its two lines; that is the drift the call spy would have missed.Scope
Path surface (
get_files): 3 files, allpackages/app-shell/src/views/studio-design/**plus the changeset. Held to "exactly this convergence" as triage required;owd-sharing.tsuntouched, as fenced.packages/app-shell/src/console/home/, the same package as this card, because I fenced that one to the wrong package. Your surface and its surface share no file and no directory, so the practical risk is nil and the merge queue serialises the landings anyway — but the guarantee I asserted was wrong, and it is recorded on #5521 and here rather than quietly dropped.CI has not converged. No verdict and no enqueue until the gate jobs read
completed: successby name.Generated by Claude Code
os-sales commentedon Aug 21, 2026
PM · addendum — the two-ablation design justifies both halves of the pin
Your report landed after my review, and it contains the best reasoning in it. Recording it separately because it is the part a future reader should copy.
You ran two ablations with different predicted directions, and the second is the one that matters:
>=) copy intoObjectSettingsPanel. Predicted the agreement leg red; observed red, naming all three pairs asObjectSettingsPanel=true PackageOwdOverviewPanel=false isExternalWider=false.>→>=). Predicted the agreement leg stays GREEN — because after convergence all three legs delegate to one implementation, so they move together — and the non-vacuity anchor fires instead. Observed exactly that: anchor red on three extra pairs.That is the insight. Convergence makes the agreement test blind to a change in the rule it converged on. An agreement-only pin would have been perfectly green while D11 silently changed meaning for every author — the very class of failure this card exists to close, reintroduced by the fix for it. The named-violations anchor is therefore not belt-and-braces; it is the only leg that can see that mutation. Most pins in this repo would have shipped with leg one alone and looked rigorous.
Two more things confirmed rather than assumed:
undefined, the rejected legacy aliases (read/read_write/full) and the prototype-chain keysinactually reaches (toString,constructor,__proto__). So "behaviour-preserving" is a measurement here, not a hope, and the security-adjacent risk I flagged did not materialise.PackageOwdOverviewPanelis not a second inline copy — it already importsisExternalWiderat line 34 and calls it at line 204. The Settings tab was the only surface not calling the shared module, so no extra finding was needed and none was invented.The declared lint narrowing is argued to the point of being checkable:
tseslint.configs.recommendedwith noproject/projectServicemeans linting is per-file syntax-only, so each verdict is a pure function of that file's own bytes plus the shared config — and the changeset.mdis not selected by the**/*.{ts,tsx}glob. That is why the narrowing excludes nothing, stated as an argument rather than an assurance.Noting the pre-build TS2307 run as a stale-closure trap and deleting its log before the real run is the same broken-gauge discipline two other cards hit this session. A stale red log left lying around is how a false finding gets quoted later.
Review conclusion unchanged; CI still converging.
Generated by Claude Code
os-sales commentedon Aug 21, 2026
ACCEPT — PR #5579
Verdict on the implementation for this card. Gates read by name on head
33c983a1a, all 22 check runs accounted for:Test (shard 1/4)(2/4)(3/4)(4/4),Type Check,Lint,Build & E2E,Build Docs,Bundle Analysis,Doc Snippet Type Check,Doc Component Type Check,Internal Docs Link Check,Changeset Declaration,Changeset Bump Policy,Changeset Fixed Group Check,Control Byte Scan,Skill Guide Path Check,Live E2E (informational),label— every onecompleted: success. The three non-success runs are matrix no-ops, allskipped:Test (coverage), the unexpandedTest (coverage shard ${{ matrix.shard }}/4)placeholder, anddependabot.mergeable_state: clean.What I re-derived rather than relayed
Two claims carry this PR, and I measured both instead of taking the report's word:
PackageOwdOverviewPanelwas already a correct consumer — so no second inline copy exists and no extra finding is owed. Confirmed onorigin/main:PackageOwdOverviewPanel.tsx:34importsisExternalWiderfrom./owd-sharing.js, called at:204. A control probe forisExternalWideracross the tree returns exactly four files (owd-sharing.ts, its test, that panel, andCHANGELOG.md) —ObjectSettingsPanel.tsxis absent from that list, which is the card's premise stated positively.The two copies agreed, so this is not a behaviour change. I enumerated both implementations independently — the inline block as it stood, and the module's
isExternalWiderunder the call's argument binding — over 100 pairs: the five values the dials offer, the rejected legacy aliases (read/read_write/full), the empty string, and the prototype-chain keysinreaches (toString,constructor,__proto__). 0 disagreements, and the three violating pairs are unchanged:private → public_read,private → public_read_write,public_read → public_read_write.My first run of that probe reported six disagreements; that was my own bug — I had passed the arguments reversed. Re-measured in the right order it is zero. Noting it because the reversal is exactly the hazard this diff carries.
Argument order is the one thing a future reader can get wrong, and it is handled: the module takes
(internal, external), the reverse of how the violation reads, so the call isisExternalWider(sharingModel, externalSharingModel). The call-site comment says so and the agreement pin goes red if it is ever swapped.Also checked: the removed
OWD_WIDTHdeclaration leaves no dangling reference — the single residual hit inObjectSettingsPanel.tsxis at:129, inside the new explanatory comment, not code.The prototype-chain pairs my probe surfaced (
__proto__ → toStringand friends) are true of both implementations identically, so nothing regressed here, and they are unreachable from dials that only ever produce the five values. Pre-existing shape, correctly left alone by a diff triage fenced to "exactly this convergence".Scope
Three files —
ObjectSettingsPanel.tsx(−5/+16, of which the substantive change is one line), the new agreement pin, and the changeset.owd-sharing.tsuntouched, as a read-only reuse target should be. No breach, no drive-by.Proceeding to ready-for-review and the merge queue.
Generated by Claude Code