Skip to content

docs(designs): record the frozen-null tolerance fix for older-Chromium text codecs - #1431

Open
kriscendobot wants to merge 1 commit into
llm-9a14ddffrom
design/hardened-text-codecs-encapsulation
Open

kriscendobot wants to merge 1 commit into
llm-9a14ddffrom
design/hardened-text-codecs-encapsulation

Conversation

@kriscendobot

@kriscendobot kriscendobot commented Oct 7, 2026 •

Copy link
Copy Markdown
Collaborator

Refs: endojs/endo#3369
Refs: endojs/endo#3376

Description

Documentation-only revision of designs/hardened-text-codecs-shim.md, rebuilt on current llm (frozen base llm-9a14ddf).

Earlier rounds of this PR documented conditional encapsulation of the codec constructors, implemented in #1349. That PR and its upstream mirror endojs/endo#3377 closed on 2026-10-29 as superseded by endojs/endo#3376, which merged the same day. This revision now records the remedy that landed:

Security Considerations

Documentation only. See the design's § Security considerations.

Scaling Considerations

None.

Documentation Considerations

This is the documentation.

Testing Considerations

None; the tests it cites landed with endojs/endo#3376.

Compatibility Considerations

None.

Upgrade Considerations

None.

🤖 Generated with Claude Code

@kriscendobot kriscendobot left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Panel review — round 1 (design panel, single-round): must-fix

Reviewed head 45c49139f2 against base fda1ff5 (llm-fda1ff5).

critic — comment-only

Verdict: comment-only

Findings

  1. should-fix: reusing the host prototype leaks the host constructor through inherited paths. The Revision section says the replacement "reuses the host prototype object as its own non-writable prototype" and repoints constructor. That makes Object.getPrototypeOf(instance).constructor the replacement, which is right. But the host constructor's __proto__ is Function.prototype, and the host prototype may still be reachable through other paths. Examples are Symbol.toStringTag getters and brand-checked accessors that close over the original. The design never says how it checks that the host constructor is unreachable "on every engine". Test plan item 7 asserts it. Please say how that assertion walks the graph: a full reachability walk from the permitted intrinsics, or only .constructor. Also state what happens to the host constructor's own .prototype link. The host constructor still points at the shared prototype, but only the host constructor holds that link, so this is fine only if nothing else retains the host constructor.

  2. should-fix: "unconditional where present" has an unexamined cost, and the rationale for it is thin. The design chooses unconditional replacement so the intrinsics have the same shape on every engine. But it loses TextEncoder === originalTextEncoder identity for host code that captured the constructors before lockdown. This hits the Compatibility considerations section (monkey-patching, pre-lockdown captures). The old text covered the monkey-patching case. Add the identity-break to that section, because cross-realm or pre-lockdown instanceof against the replacement still works only because the prototype is shared. State that explicitly as the reason the prototype is reused.

  3. should-fix: one rejected alternative is real but incomplete. The "tolerate the descriptor" rejection rests on the descriptor being identical for inert and live slots. That is sound. The design does not consider a narrower variant: tolerate the descriptor only on a constructor that is a native function (not a sloppy one) and has name TextEncoder or TextDecoder. That is an allowlist, not a shape check. It is probably still worse than encapsulation, since it grows the permits table with host-specific exceptions. Say so in one sentence so the rejection is complete rather than a strawman of the shape-only variant.

  4. comment-only: idempotency under XS. "Idempotent, because SES-for-XS must apply it at module load" is stated but not specified. Say how a second application detects an already-replaced constructor (a marker, a WeakSet, or comparing the prototype's constructor). Otherwise a second pass wraps the replacement and builds a chain of delegators.

  5. comment-only: sunset condition. "Possibly temporary" is good. Name the trigger so it can be acted on, for example when the oldest supported Chromium is 138 or later.

Out of scope: the new.target subclassing behavior and Reflect.construct error messages belong to the implementation PR's code panel.

Self-improvement: none to record; the diff was a single design revision and the brief fit it.
model claude-opus-5-5 · harness claude · provider anthropic

skeptic — request-changes

Verdict: request-changes

Findings:

  1. must-fix: the "same shape on every engine" claim is not backed by evidence. The Revision says the host prototype is reused as the replacement's non-writable prototype, and that "the host constructor object … is unreachable from the permitted intrinsics graph on every engine." Reachability is asserted only through constructor repointing. The host prototype still reaches the original constructor through other paths:

    • Any captured reference, including the shim's own Reflect.construct target.
    • Object.getPrototypeOf(instance).constructor, if anything re-defines or restores it.
    • Instances created before lockdown() (the pre-lockdown host codec instances a program already holds). These share the prototype, so their constructor is repointed too, and that is a visible mutation of host-shared state. The design does not discuss it.

    Test plan item 7 checks "unreachable", but it does not say how that is verified: a graph walk, or a single .constructor check. Specify the verification method. [proposed-rule: a reachability claim must name the traversal that tests it]

  2. should-fix: the "genuine host instances" premise is untested against realm and brand edge cases.

    • Subclassing: Reflect.construct(host, args, new.target) with new.target set to the SES constructor yields an object whose prototype is the host prototype. That works. But class X extends TextDecoder is the only claimed subclass case, and no test is listed for it.
    • TextDecoder.prototype.constructor === TextDecoder after lockdown, and instanceof against pre-lockdown captured host constructors (const { TextDecoder: Host } = globalThis before SES). That instanceof now behaves differently, because Host.prototype is the same object as the replacement's prototype but Host !== TextDecoder. Instances still pass, but identity comparisons on constructors break.
    • Node's util.TextDecoder, which Node exposes as a separate function from the global one in some versions. The design does not say whether it is covered.

    Add these to the test catalog.

  3. should-fix: the browser-affected band is measured, but the safety of the unconditional replacement on unaffected engines is not. The remedy replaces unconditionally "where present". On Chrome 138 and later, Node, and Firefox/Safari, this swaps working constructors for wrappers. That changes .length, .name, the toString output, and the Function.prototype chain of the replacement (the SES-owned function vs. host WebIDL). Code that feature-detects with Function.prototype.toString looking for [native code] will see a difference. The Compatibility section is not updated for this, even though the original design had one. Add a bullet.

  4. should-fix: the XS module-load idempotency claim ("SES-for-XS must apply it at module load") is unsupported. The text claims the shim compartment constructor samples intrinsics before lockdown(). No test is listed for double application, or for applying at module load and then lockdown() in a host where the codecs are provided partway through. Item 7 covers Node and Playwright only, not the XS runner. Request a double-application test.

  5. comment-only: the "temporary" retirement criterion is unfalsifiable as written. "Once every supported engine ships clean codec constructors" has no owner, trigger, or tracked issue. Link an issue to track it.

  6. comment-only: the measured boundary used only six Chrome versions. 128–132 and 134–135 are unmeasured, yet the text says "every Chromium before 138". It is plausible, but state it as an extrapolation.

Self-improvement: when a design claims "unreachable", ask for the traversal that proves it.
model claude-opus-5-5 · harness claude · provider anthropic

decomplector — request-changes

Verdict: request-changes

The diff is design-only (designs/hardened-text-codecs-shim.md). The ownership-map pre-pass flagged it as spanning engine and supervisor layers. That flag doesn't really fit here. Every layer in this design runs inside lockdown(), which samples once and hardens once, so there is no durable state, no commit or discard step, and no replay. The split that matters is host engine versus SES lockdown, and the design is explicit about it: the host owns codec behavior and brand checks on the shared prototypes, and SES owns the permitted constructor identity. No inner-layer type borrows an outer lifecycle name. I'm confirming the map and moving on.

Findings

  1. should-fix: one engine-wide hazard is fixed as if it were a codec problem. § Revision itself says the trigger is that "WebIDL constructors — including TextEncoder and TextDecoder — carry own legacy restricted properties" on Chromium before 138. But the remedy, test item 7, and Settled decision 4 cover only the codecs. The base already permits other WebIDL constructors:

    • URLSearchParams is permitted universally (packages/ses/src/permits.js:124).
    • URL, in the default urlBlobTaming: 'retain' mode, binds the host original as %InitialURL% (src/tame-url-constructor.js, InitialURL = … OriginalURL).

    Both should hit the same undeletable caller/arguments failure on the same Chromium band. As written, the design ties the fix for the restricted-property hazard to the codec feature. Fixing the codecs then won't make lockdown() work on Chrome 137 or earlier, and item 7's "lockdown completes" assertion can only pass by accident of which globals that host exposes. The design should either describe the remedy as a general rule ("a permitted host WebIDL constructor is never the host object") and list URL/URLSearchParams, or name them as known-affected follow-up work. [proposed-rule: when a design's root cause is stated about a class of objects, the remedy's scope and the test plan must name every permitted member of that class, or explicitly defer the rest]

  2. should-fix (category f): an existing repository path already does this; the design should reuse it or say why not. tameUrlConstructor (src/tame-url-constructor.js, called through addIntrinsics at lockdown.js:366) already implements the adopted mechanism:

    • a SES-owned constructor that throws without new and delegates through construct(Original, rest, new.target);
    • the host prototype reused as a non-writable prototype;
    • prototype.constructor repointed at the replacement.

    tameDateConstructor is the older precedent. That path returns the replacements as intrinsics. The revision instead mutates the global object "before the intrinsics collector samples the global object". That change is what forces the extra requirements to be idempotent and to run at SES-for-XS module load. A side effect at import time plus a replay requirement is accidental complexity created by the choice to work on the global (place-oriented) rather than return values. The revision should either adopt the addIntrinsics shape, or explain why the XS shim compartment's early sampling rules it out.

  3. comment-only: "possibly temporary … retired in favor of direct sampling" is fair. Once finding 2 is resolved, though, the retirement is just deleting one addIntrinsics line, which is a cleaner exit than undoing a global-mutation step that has to run at two separate points.

Self-improvement: when the ownership-map pre-pass fires on a single-pass mechanism like lockdown(), which has no durability, commit, or replay, the seat should say quickly that the map doesn't apply and spend the effort on category (f) against the sibling tame*Constructor modules instead.
model claude-opus-5-5 · harness claude · provider anthropic

ergonomist — comment-only

Verdict: comment-only

The design reads well for its audience, who are SES maintainers and downstream embedders. The user-facing surface is nearly unchanged, which suits an encapsulation fix. The findings are small.

Findings

  1. Should-fix: the Status cell reads as a mix of states. "Revised — permits landed upstream (endojs/endo#3322); encapsulation revision in PR #1349" puts two facts and two links in a cell that other designs use for a one-word state. A reader scanning a status column can't tell whether the design is Done, In Progress, or Revised.

    • Proposal: use a single state word (for example Revised), and move the upstream and PR references into the Revision section. That section already carries them.
    • [proposed-rule: design Status cells hold one state word; provenance goes in the body.]
  2. Comment-only: discoverability of the revision. Test plan item 7 and the Revision section both depend on the Rejected alternatives, which sit mid-document. A user of the shim who only reads the "What is the Problem Being Solved?" section never sees that TextEncoder !== hostTextEncoder after lockdown.

    • Proposal: add one sentence in Compatibility considerations. Identity comparison against a pre-lockdown host constructor now fails, while instanceof still holds because the prototype is shared.
    • This is the one place the design changes behavior users can observe, and it is currently implicit. The Revision section says the prototype's constructor is repointed, but it never states the consequence for callers.
  3. Comment-only: naming consistency. The text uses "encapsulation", "replacement", "SES-owned constructor" and "taming" for the same mechanism. Pick one term for the mechanism ("encapsulated constructor") and use it in the heading, the Decision entry and the prose. The file name tame-text-codecs.js is the odd one out; the design could note it.

  4. Comment-only: reversibility signal. The closing paragraph, "possibly temporary", is good. It would be more actionable with a stated retirement trigger, such as "when the minimum supported Chromium is ≥138". The measured boundary is already given, so the trigger costs one clause.

Out of scope: whether Reflect.construct with a preserved new.target behaves correctly for subclassing is an implementation concern for the code-panel review.

Self-improvement: none; no new rule beyond the proposals above.
model claude-opus-5-5 · harness claude · provider anthropic

copyeditor — request-changes

Verdict: request-changes

Findings:

  1. Grammar: Broken parallel structure (Revision section, first paragraph)
    Text: "arguments and caller are not in the FunctionInstance permit set, so the permits-enforcement pass tries to delete them, cannot (they are non-configurable), and lockdown() throws."
    Issue: The comma + implied verb in "cannot" breaks the parallel series without a connecting word like "but" to signal contrast. The sentence reads as if "cannot" were a separate action parallel to "tries," which is awkward.
    Recommendation: Restructure to "tries to delete them but cannot (they are non-configurable), and lockdown() throws." [must-fix]

  2. Clarity: Elliptical subject-verb in rejected alternative (Rejected alternatives, "Permit the properties" paragraph)
    Text: "Same exposure decision expressed in the permits table, with the added drawback that a configurable variant of the property would also be permitted."
    Issue: The opening phrase lacks an explicit verb structure. A reader pauses to parse what "Same exposure decision" modifies and how the action is expressed. The sentence needs a main clause.
    Recommendation: Restructure to something like "Expressing this same exposure decision through the permits table instead adds the drawback that a configurable variant would also be permitted." [should-fix]

  3. Prose clarity overall (Revision section as a whole)
    The new section is dense and technical, but well-structured and clearly explains the problem, rejected paths, and adopted remedy. Transitions between the problem statement and the "Rejection alternatives" header are smooth. No further issues flagged. [approve]

Self-improvement: Watched for jargon introduction (ocap, encapsulation, restricted properties, non-configurable) — all sufficiently explained in context for the technical audience. Checked em-dash usage against the typist-friendly-code-points rule: the brief exempts em-dashes from the flagging requirement, so no issue there. Verified tense consistency (present/conditional, appropriate for design rationale) and paragraph-to-paragraph flow through the revision narrative.
model claude-opus-5-5 · harness claude · provider anthropic

pedant — request-changes

Verdict: Request changes

Findings:

  1. Em-dash prohibition violation (must-fix) [rule: skills/em-dash-style/SKILL.md]
    Line in "Adopted remedy" section: Adopted remedy — **encapsulation** (mhofman: "The repair... uses an em-dash. Project forbids em-dashes; rewrite as colon, period, or parenthetical: either Adopted remedy: **encapsulation** or Adopted remedy (**encapsulation**) or split into sentences.

  2. Heading capitalization inconsistency (should-fix) [rule: Chicago Manual 8.159]
    The document has inconsistent ##-level heading style: ## What is the Problem Being Solved? and ## Dependencies use title case, while existing ## Open questions uses sentence case. The new ## Revision: encapsulated constructors (2026-10-07) adopts sentence case, matching the existing outlier. For publication polish, pick one and apply consistently across all main headings—either all title case or all sentence case. Current document leans title case as the dominant pattern.

  3. Reference format consistency (comment-only)
    The Status line introduces a fully qualified GitHub PR link [endojs/endo-but-for-bots#1349](https://github.com/endojs/endo-but-for-bots/pull/1349), while other issue cites in the same document use shorthand (endojs/endo#3369). Both are acceptable, but consider whether to normalize all PR/issue cites to one format for visual consistency. Fully qualified links are more portable; shorthand is terser.

Self-improvement: This review applied standing rules for em-dashes, heading capitalization (Chicago 8.159), and reference consistency. The em-dash violation is mechanical and clear; the heading issue is a pre-existing inconsistency the new section joins rather than creates—worth calling out for the author's awareness even if not a blocker.
model claude-opus-5-5 · harness claude · provider anthropic

novice — request-changes

Verdict: request-changes

Findings:

  1. should-fix: the revision opens with undefined context.
    The ## Revision section starts "The original design sampled the host constructors directly onto the permitted intrinsics."
    A new reader has only seen the original design far above, and the sections in between are about test plans.
    The revision never restates, in one sentence, what "permitted intrinsics", "the permits-enforcement pass", or "cauterizeProperty" are.
    Add a two-sentence primer, such as "lockdown() deletes every property that is not on an allowlist (the permits)", before the failure explanation.
    [proposed-rule: a revision section opens with a short recap of the mechanism it revises.]

  2. should-fix: the failure paragraph is a single dense chain.
    The paragraph runs from "WebIDL constructors" to "own legacy restricted properties" to "not in the FunctionInstance permit set" to "tries to delete them, cannot" to "lockdown() throws".
    Each step is correct, but the reader holds five facts in one sentence-run.
    Split it into a numbered cause-and-effect list.

  3. should-fix: "Rejected alternatives" arrive before the reader knows the options.
    The first bullet says "Withdrawn by the issue reporter".
    It also argues about a "live" slot, a sloppy function, and a descriptor "read identically".
    The reader has not been told why a live caller slot on a sloppy function leaks anything.
    Add one sentence: "On a sloppy function, caller returns the calling function during a call."
    [rule: roles/jurors/novice/AGENT.md, assumed background]

  4. should-fix: the adopted remedy is one very long paragraph.
    It packs six mechanisms into it: no-new throw, Reflect.construct, new.target, a shared prototype, a repointed constructor, and idempotence.
    No worked example shows an instance being made, so the reader cannot see why instanceof still works.
    A short code sketch or a bulleted list would help.
    The "shared prototype" idea is the key mental model and is only implied.

  5. comment-only: the "Status" cell and the mhofman quotes.
    The status cell links to a PR, but a reader cannot tell whether the revision is landed.
    The bare name mhofman is not introduced.
    The decision-record item 4 duplicates the revision's reasoning, which is fine.
    Its sentence is long and could be split.

  6. comment-only: "Measured boundary" gives a version list but no why.
    The reader can infer that 138 is the fix, but the section does not say whether the cause is known.

Overall: The logic is sound and complete, but the revision assumes the SES permits and intrinsics model and compresses its key argument into dense paragraphs.
A primer, a split of paragraphs 2 and 4, and one clarifying sentence on live caller would make it followable for a newcomer.

Self-improvement: no new standing rule is needed beyond the proposed one in finding 1.
model claude-opus-5-5 · harness claude · provider anthropic

orthographer — approve

Verdict: approve

Findings:

  • none — the deterministic divergence grep found no British spellings introduced by this change. [rule: skills/american-english-normalization/SKILL.md]
    model claude-opus-5-5 · harness claude · provider anthropic
thesaurus — approve

Verdict: approve

Findings:

  • none — the deterministic cliché grep found no Botese phrases introduced by this change. [rule: skills/botese-normalization/SKILL.md]
    model claude-opus-5-5 · harness claude · provider anthropic

model claude-opus-5-5 · harness claude · provider anthropic · host endolin-garden2-5bcdff64 · garden d9a55c17

@kriscendobot kriscendobot left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Panel round 2 — must-fix

Single-round design panel (9 seats, all ok) on head 324dc3dc42d vs base llm-fda1ff5 (fda1ff55233). Disposition: must-fix.

critic — comment-only

Verdict: comment-only

Findings

  1. should-fix: the "no back-reference" claim depends on prototype methods and accessors the design never examines (§ Revision, "key mental model"; test item 7).
    The design says nothing reachable from the permitted intrinsics points back at the host constructor once constructor is repointed.
    It also admits the walk "cannot see references held inside host closures".
    Two ways to reach the host constructor from an instance are not discussed:

    • Reflect.construct(Host, args, new.target) hands the caller an instance whose [[Prototype]] is the shared prototype. That is fine.
    • With subclassing or new.target games, Object.getPrototypeOf on a derived class's constructor yields the SES replacement, not Host. That is fine too.
      Neither path breaks the claim, so this is not a correctness hole. The weakness is that the design asserts the property and defers it wholly to the walk. Add a sentence on why host prototype methods never return or expose their constructor (WebIDL operations don't).
      [proposed-rule: when a design's security claim is "X is unreachable", state the argument as well as the test.]
  2. should-fix: the Rejected alternatives omit a prototype-copy variant.
    The design only considers "reuse the host prototype" and does not say why a fresh prototype is rejected.
    The Compatibility section implies the reason (preserving instanceof against pre-lockdown instances), but § Revision should say so where the choice is made.
    Mutating shared host state (Host.prototype.constructor repointed, then hardened) is also a larger cost than the text admits. After lockdown, a host-side non-SES consumer in the same realm sees the changed constructor.
    Name that cost in the alternatives, not only in Compatibility.

  3. should-fix: the scope of the fix doesn't match the stated rule.
    The design adopts the rule "a permitted host WebIDL constructor is never the host object". Yet it fixes only the codecs and leaves URL and URLSearchParams known-affected.
    lockdown() therefore still throws on Chromium before 138 in the default urlBlobTaming: 'retain' mode.
    The #3369 symptom is not resolved by this PR, and test item 7 "passes on an affected Chromium only once those are addressed".
    The design should say plainly whether it closes #3369 or only narrows it, and name the follow-up job.
    [proposed-rule: a revision that claims to fix a reported failure should say whether the failure is still reproducible after it lands.]

  4. comment-only: the idempotence mechanism (a module-level WeakSet) is a hidden-state coupling.
    A second copy of the ses module, for example from duplicated bundling, would have a separate WeakSet and would wrap twice.
    The design should note that the guard is per-module-instance. Alternatively, mark the replacement structurally, for example by detecting that the prototype.constructor back-link already points at a non-host function.

  5. comment-only: the Chromium range is an extrapolation. The design is honest about it, which is good. Consider stating the retirement trigger as "oldest supported Chromium ≥ 138" only after the unmeasured versions are checked.

The core approach (encapsulate rather than tolerate or permit) is sound, and the rejections of tolerance and permit are correct, not strawmanned. The sloppy-function caller argument holds.

Self-improvement: none this engagement.
model claude-opus-5-5 · harness claude · provider anthropic

skeptic — request-changes

Verdict: request-changes

Findings

  1. should-fix: the encapsulation's security claim leans on a premise the design never tests. The Revision says the replacement is safe because "nothing reachable from the permitted intrinsics points back at the host constructor once constructor is repointed."

    • The design admits the walk "cannot see references held inside host closures".
    • Host-created instances stay brand-linked to the host constructor in ways the descriptor walk cannot see. An example is Reflect.construct(Host, args, new.target) with an attacker-chosen new.target: it hands the host Function.prototype-derived behavior to arbitrary subclasses.
    • The design should state what new.target can leak, or constrain it. Item 9 tests subclassing for success only.
      [proposed-rule: a design that claims "unreachable" must name the escape channels it does not cover.]
  2. should-fix: item 7 cannot pass on the engines it targets. The design itself says URL and URLSearchParams are known-affected on the same Chromium band, so item 7 "passes on an affected Chromium only once those are addressed or absent."

    • Item 7 runs on a genuinely affected Chromium in the Playwright suite, where URLSearchParams is always present.
    • So the headline regression test is red until follow-up work lands. Without that work, #3369 is not actually fixed for those users.
    • The design should either put URLSearchParams and URL in scope, or scope item 7 to assert only the codecs. As written, the test catalog contradicts the stated fix.
  3. should-fix: the "measured boundary" is generalized past its evidence. Only 6 of the 21 versions from 120 to 140 were measured. The design extrapolates "every Chromium before 138". The design admits this, but the retirement trigger ("oldest supported Chromium is 138+") and the "unconditional replacement" rationale don't depend on it, so the extrapolation does no harm today. The test plan should still pin the measured versions rather than imply a band.

  4. should-fix: compatibility assumptions are understated.

    • Repointing Host.prototype.constructor mutates host-shared state. Other realms or iframes that share that prototype are affected, and a second SES instance in the same realm is affected too.
    • The WeakSet idempotence guard is per-module-instance. Two SES copies loaded in one realm (common with bundlers) would double-wrap, because the second copy's WeakSet has never seen the first copy's replacement. Item 8 covers only one module instance.
  5. comment-only: Function.prototype.toString and [native code]. The design mentions the difference but gives no guidance on whether any shipped SES or endo consumer feature-detects the codecs this way. A repo grep would settle it.

  6. comment-only: no evidence of a real run. The measurement is stated as made on Chrome for Testing. The design does not record that the proposed encapsulation was run against a real affected Chromium and made lockdown() succeed. Item 7 is currently an aspiration. Until the PR carries that run, any "fixes #3369" claim should read "not verified."

Self-improvement: when a design admits a known-failing follow-up, check whether its own headline test depends on it.
model claude-opus-5-5 · harness claude · provider anthropic

decomplector — comment-only

Verdict: comment-only (two should-fix items; no must-fix)

Ownership map (pre-pass). The flagged layers (engine, supervisor, lifecycle) don't fit this design. The boundaries that actually matter are host → SES lockdown() → compartments. The design states the map clearly enough to reconstruct:

  • The host owns the instance slots and all codec behavior on the prototype.
  • SES owns the identity of the permitted constructor, the repointed prototype.constructor, and the hardening.
  • Compartments own nothing.
  • The only state both sides touch is the shared prototype, and § Compatibility considerations names its one mutation.

No inner mechanism is given an outer lifecycle name. Nothing to flag here.

Findings

  1. should-fix — the same mechanism is now copied three times, and the design names no shared primitive (category f, minimum viable abstraction).

    • § Revision's delegator (throw without new, Reflect.construct with new.target, reuse the host prototype, repoint constructor) is line for line the SharedURL maker in packages/ses/src/tame-url-constructor.js (tameUrlConstructor). That file in turn copies the %SharedDate% precedent.
    • The revision states a general rule ("a permitted host WebIDL constructor is never the host object") and already lists URLSearchParams and %InitialURL% as known follow-ups. Even so, it puts the codec-only mechanism in a new tame-text-codecs.js.
    • That ties the general rule to one module. The follow-ups will produce a fourth and fifth copy, each with its own idempotence check.
    • Fix: name one maker, for example encapsulateHostConstructor(Host), that the codecs use now and SharedURL/URLSearchParams can adopt. Or say why the codecs need their own.
    • [proposed-rule: when a design states a rule that covers more than the surface it fixes, it names the primitive the remaining cases will reuse, or justifies a per-case mechanism.]
  2. should-fix — the old design text still makes claims the revision overturns (value vs place).

    • Adding the revision as a new section is good. But § What is the Problem ("straightforward: … sample during the existing intrinsics-collection pass") and § Lockdown sequencing ("no new lockdown phase"; step 1 "collects … from the host global") still describe the current design, and both are now false. The revision adds a replace-before-sample step, which on XS also runs at module load.
    • A reader can only work out what is currently true by reading the revision on top of the body.
    • Fix: mark those sections as superseded, or revise them and leave the history in § Revision.
    • [rule: skills/ownership-map/SKILL.md], on the grounds that a stale sequencing section is an unstated ownership claim about the lockdown phases.
  3. comment-only — idempotence is detected through a module-private WeakSet.

    • The double application exists because two separate sampling moments (module load on XS, then lockdown()) each apply the taming. The WeakSet remembers which wrappers this module installed.
    • That memory is private to one loaded copy of ses. If two copies run in the same realm, the second copy won't recognize the first copy's wrapper and wraps it again. Test item 8 doesn't cover that case.
    • A structural check would work across copies: for example, the replacement's prototype.constructor already points at itself, and its source is not native. Alternatively, the design can explicitly say that more than one copy of SES in a realm is out of scope.
    • Ties to finding 1: a shared maker would own this check once.

Out of scope (code panel): how test item 7's transitive walk is built, and whether it is complete.
model claude-opus-5-5 · harness claude · provider anthropic

ergonomist — comment-only

Verdict: comment-only

The surface is the user-visible behavior of TextEncoder and TextDecoder after lockdown(). The revision keeps that surface close to the host's, and the compatibility notes list the places where it differs. I have no must-fix findings.

Findings

  1. should-fix: the unconditional replacement makes the surface differ from the host in ways a user cannot predict. The revision says the replacement happens "unconditionally where the codecs exist", so the permitted shape is the same on every engine. Constructor identity, Function.prototype.toString output, and own properties of the constructor then change everywhere, even on Chromium 138 and later, where nothing is broken. The compatibility section does say this. The retirement trigger is a good affordance, but it is written for the maintainer. A user hitting Host !== TextDecoder will not look in a design doc. The design should name where the changeset note will carry the identity change and the 138-and-later retirement path, and say whether that note appears per release or only once. [proposed-rule: when a design adds an unconditional workaround, state the user-facing disclosure site, not only the internal retirement trigger]

  2. should-fix: calling the constructor without new is a weak error to surface. The sketch throws TypeError('requires new') with a bare string. The host's message names the constructor, for example "Failed to construct 'TextDecoder': Please use the 'new' operator". The design should require the thrown error to name the constructor and be consistent with @endo/errors. [rule: AGENTS.md § Error handling]

  3. comment-only: the argument-forwarding contract is implicit. The sketch forwards ...args to Reflect.construct, so host validation errors for bad labels and options propagate unchanged. That is the least-surprising behavior. A test-plan assertion would pin it: the error for an invalid new TextDecoder('bogus') should be the same class as the host's.

  4. comment-only: the length of the replacement is not pinned. function TextDecoder(...args) has length 0, while the host's is 0 for TextDecoder and 0 for TextEncoder. In this case that is fine, but a one-line statement in the design would say so. Test plan item 9 could assert that name and length match.

  5. comment-only: the URL and URLSearchParams follow-up leaves the surface inconsistent. Fixing the codecs now and the URL constructors later gives three behaviors during the interim window. Test plan item 7 passes on an affected Chromium only once those are addressed. A short sentence on the order of the follow-ups would help.

Out of scope: typing the replacement constructors for .d.ts consumers belongs to the code-panel typist.

Self-improvement: none this round.
model claude-opus-5-5 · harness claude · provider anthropic

copyeditor — approve

Verdict: Approve

Findings:

None. The revision section and supporting additions are well-written, with clear technical exposition, consistent voice and tense throughout, and smooth integration with the existing document structure.

The new content demonstrates strong prose mechanics:

  • Test plan items 7–9 follow established parallel structure from items 1–6.
  • The "Revision: encapsulated constructors" section opens with clear background, progresses through problem statement → rejected alternatives → adopted remedy → mental model explanation, maintaining technical accuracy without jargon leaps.
  • New compatibility bullets follow the established format and tone of the originals.
  • Section transitions are marked clearly (§ Revision references, subsection headers) and help readers navigate a dense technical change.
  • The "key mental model" explanation (line 307+) unpacks a complex concept in a way accessible to readers already familiar with intrinsics/permits machinery.
  • Design Decision #4 ties cleanly to the Revision section and provides concise rationale.

No grammatical errors, dangling modifiers, pronoun ambiguities, or tense drift detected. Jargon (permits, intrinsics, WebIDL, ocap, Reflect.construct) is used consistently with earlier sections, appropriate for the audience. No typist-hostile code points (arrows, curly quotes, math symbols).


Self-improvement: This review confirms that adding a substantial revision section to an existing design benefits from a clear problem → solution arc and concrete mental-model explanation. The pattern here—opening with what was measured, what broke, alternatives rejected, and why the chosen solution works—is a strong model for future revision sections.
model claude-opus-5-5 · harness claude · provider anthropic

pedant — approve

Verdict: Approve

Findings:

The new content maintains professional style and consistency with the existing design document. The additions—test plan items 7–9, expanded compatibility considerations, and the major "Revision" section—are well-structured and follow the document's conventions.

Specific observations:

  • Heading style: "## Revision: encapsulated constructors (2026-10-07)" uses sentence case after the colon, which aligns with Chicago Manual for subtitle-style headings. Consistent with document precedent.
  • List discipline: Test plan items 7–9 preserve parallel structure with existing items (numbered, bold assertion at head, supporting detail). Terminal punctuation is consistent.
  • Quotations: All quotation marks are straight ("...") not curly; inline code uses backticks correctly.
  • Project conventions: No em-dashes, hard-to-type code points (→, …, ≤/≥, etc.), or improper relative paths detected. Section cross-references (§ Revision, § Compatibility considerations) are internal and properly formatted.
  • Serial commas: Applied consistently (e.g., "120, 126, 127, 133, 136, and 137"; "138, 139, and 140").
  • Code formatting: The JavaScript example and inline code samples are properly delimited and readable.
  • Prose clarity: Complex sentences in items 7–8 (multi-clause descriptions of test scenarios) are appropriate for technical documentation; embedded clauses and semicolon joins are grammatically sound.

The document reads as edited and professional. No corrections required.

Self-improvement: This review was straightforward because the author applied Chicago Manual conventions and the project's layered rules (no em-dashes, straight quotes, relative section references) consistently throughout. The structured technical content (numbered test items, subsection headings, code example) benefited from consistent capitalization, punctuation, and list discipline. Future reviews of similar design expansions should look for the same touchpoints: heading level/case consistency, parallel list structure, and quotation-mark form.
model claude-opus-5-5 · harness claude · provider anthropic

novice — comment-only

Verdict: comment-only

The revision follows a clear chain. The problem is a failing lockdown(), the cause is undeletable properties, the alternatives are rejected, and encapsulation is adopted. A new reader can follow it. The findings below are mostly ordering and signposting problems.

Findings

  1. should-fix: the Revision sits after the test plan and compatibility sections that depend on it.

    • Test items 7–9 and the Compatibility bullets cite "§ Revision" and "the SES-owned replacements".
    • The reader meets those terms first and only learns what they mean about 100 lines later.
    • Add a one-sentence forward pointer at the top of the document, or at the first use. For example: "Since 2026-10-07 the codecs are replaced, not sampled; see § Revision."
    • The original ## Approach text is presumably still unchanged and still says the host constructors are sampled directly. A reader who goes top-down gets the wrong model first.
    • [proposed-rule: a revision appended to a design carries a banner at the top of the design saying which earlier sections it supersedes]
  2. should-fix: "Revision" is never defined as a section type.

    • The Status changed to Revised, but the document does not say that the Revision section overrides earlier prose.
    • A new reader cannot tell which sections are current.
  3. should-fix: test item 7's walk paragraph is dense.

    • It is a single long sentence that chains "every permitted intrinsic and every value bound on a fresh compartment's global, follow each own property descriptor (value, get, set, string and symbol keys) and each [[Prototype]] link".
    • Break it into a short list of steps.
    • The sentence "it cannot see references held inside host closures, which is why the walk is the claim" is clear in meaning but awkward. Say it in two sentences.
  4. should-fix: the "Measured boundary" paragraph mixes facts that are hard to hold together.

    • It gives a list of measured versions, a list of unmeasured versions, an extrapolation, a correction of the issue, and an untraced cause, all in one block.
    • Split the measured facts from the inference.
  5. comment-only: the code sketch comes before its explanation, and the cohort of terms is large.

    • The sketch uses Reflect.construct and new.target, then the mental-model paragraph explains the shared prototype.
    • Consider putting the one-line "why the prototype is shared" sentence before the sketch.
    • "WebIDL constructor", "sloppy function", "brand-checked", and "permits" are used with only light introduction. The opening Background paragraph helps with "permits".
    • Add a half-sentence gloss for "WebIDL" and "brand check" on first use.
  6. comment-only: the "Why the global binding is replaced…" paragraph assumes knowledge of SES-for-XS, getGlobalIntrinsics, and addIntrinsics.

    • It does not say what those are.
    • A one-line description of each would stop a new reader from losing the thread at the step that explains the design's most surprising choice.

Self-improvement: when a revision is appended to an existing design, check whether earlier sections still state the superseded model.
model claude-opus-5-5 · harness claude · provider anthropic

orthographer — approve

Verdict: approve

Findings:

  • none — the deterministic divergence grep found no British spellings introduced by this change. [rule: skills/american-english-normalization/SKILL.md]
    model claude-opus-5-5 · harness claude · provider anthropic
thesaurus — approve

Verdict: approve

Findings:

  • none — the deterministic cliché grep found no Botese phrases introduced by this change. [rule: skills/botese-normalization/SKILL.md]
    model claude-opus-5-5 · harness claude · provider anthropic

model claude-opus-5-5 · harness claude · provider anthropic · host endolin-garden2-5bcdff64 · garden d9a55c17

@kriscendobot kriscendobot left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Panel round 3 — must-fix

Single-round design panel (9 seats, all ok) on head 160e1c7481 vs base llm-fda1ff5 (fda1ff55233). Disposition: must-fix (copyeditor: request-changes; critic, skeptic, decomplector, ergonomist, novice and pedant: comment-only with should-fix findings; orthographer and thesaurus: approve).

critic — comment-only

Verdict: comment-only. The approach is sound and the rejected alternatives are fairly treated. The findings below are revisions the design can absorb.

Findings

  1. should-fix: the design never evaluates a conditional replacement against its stated cost. § Revision replaces the constructors unconditionally "so the permitted intrinsics have the same shape on affected and unaffected engines". The price is a permanent identity break (Host !== TextDecoder) and a repointed Host.prototype.constructor. Both hit every user on Chromium 138 and later, Node, Deno and XS, where nothing was broken. A conditional fix needs a feature test: replace only when the host constructor has an undeletable own caller or arguments. That is a different mechanism from the rejected descriptor-shape tolerance, because it gates the replacement and does not admit the host object. Add it as a considered-and-rejected alternative with a reason. "Uniform shape" is a defensible reason, but it is asserted here and never weighed against the cost to unaffected users. [proposed-rule: when a remedy is unconditional but the defect is engine-specific, the design must say why a feature-gated remedy was rejected]

  2. should-fix: the shared-prototype mutation looks unrecoverable and gets little scrutiny. Host.prototype.constructor is repointed and the host prototype is hardened, in host-shared state. Pre-lockdown code in the realm sees that change, and so do any host APIs that read .constructor. The shared prototype also cannot be freed later, so it is not a clean retirement. The document lists this under compatibility but never weighs it against copy-on-wrap, or against leaving the host constructor slot alone. Leaving it alone would give up new X().constructor === TextDecoder for pre-lockdown instances, and the design does not say why that is worse.

  3. should-fix: the security argument that the host constructor stays unreachable is partly circular. It depends on the claim that prototype methods never return a function or the constructor. That is true of today's WebIDL, but it is a property of a host surface that SES does not control. Test plan item 7 backs it with a walk that, by the design's own admission, cannot see closures or internal slots. The design should say plainly that the guarantee is empirical and tied to current host behavior, and name what would break it. A host adding a method that returns a class, or a new Symbol.species-style hook, would break it.

  4. should-fix: the cost of the maker is unclear. The revision introduces encapsulateHostConstructor and says the follow-up moves SharedURL onto it. Whether SharedURL's current delegation, which has its own existing error message, can adopt the same maker unchanged is asserted and not checked.

  5. comment-only: the document now has three layers of authority. The original sections carry "Superseded" patches, § Revision overrides them, and the Status line says "Revised". A reader following the original step-by-step flow meets contradictions before reaching the override. Folding the revision into the original sections would be cheaper to keep correct, if the team accepts the churn.

  6. comment-only: the dependence on a retirement date. The retirement trigger ("oldest supported Chromium ≥ 138") depends on a support matrix that is not named here. Point to where SES documents its supported-browser floor.

No ASCII diagrams appear, so no mermaid concern. The 7–9 test plans are thorough.

Self-improvement: no new standing rule is warranted beyond the proposed-rule above.
model claude-opus-5-5 · harness claude · provider anthropic

skeptic — comment-only

Verdict: comment-only

Findings:

  1. should-fix: the "host constructor unreachable" premise leaves the cross-realm path unexamined.

    • The Revision argues the host constructor is unreachable because the prototype methods return no functions and new.target flows one way. Nothing checks it for Host.prototype.constructor reads before repointing, or for other realms.
    • One route stays open. An instance built by a pre-lockdown caller can have Object.getPrototypeOf(x).constructor read by code that ran before the repoint. The design states this only as "visible mutation" in Compatibility.
    • A second route is a subclass-based leak. Reflect.construct(Host, args, F) also reads F.prototype via new.target. The design does not discuss whether a guest-supplied F with a prototype getter can observe anything. Item 9 tests only a plain F.
    • Add a Proxy or getter F.prototype case to item 9, or state why it is moot.
    • [proposed-rule: a design that claims capability unreachability must enumerate observable new.target and Proxy interactions]
  2. should-fix: the premise that every Chromium before 138 is affected rests on extrapolation, and the design's own reasoning shows it does not need that.

    • The design says the unconditional replacement avoids depending on the extrapolation. That is correct.
    • The Retirement trigger ("oldest Chromium SES supports is 138 or later") is then set against an unmeasured band. Items 121–125, 128–132, 134 and 135 are unmeasured, and no V8 cause is identified.
    • Firefox and WebKit are not mentioned. Do their codec constructors carry the same restricted properties? The Revision is silent. The unconditional design is safe, but the "Chromium only" framing could mislead the changeset text.
    • Either measure one more engine or reword the changeset note to say "observed on Chromium ≤137".
  3. should-fix: item 7's browser leg depends on deleting URL and URLSearchParams.

    • The design acknowledges that lockdown() still throws on the default path until the follow-up lands.
    • That means the shipped fix leaves endojs/endo#3369 reproducible for users on Chromium <138 in default mode. Releasing it alone gives no user-visible relief.
    • The design should say whether the codec revision ships independently, or is held for the follow-up.
  4. comment-only: the idempotence argument covers a second copy of SES but not a second lockdown() or repairIntrinsics call.

    • The "frozen prototype, leave alone" rule handles repeated calls, but the design does not say what happens when the prototype is frozen and the global binding is still the host constructor.
    • That state can arise if another party hardened the prototype early. It leaves the host constructor on the permits path with no error handling named.
    • Name the failure mode, such as a throw with a clear message, or an explicit "fall through to direct sampling".
  5. comment-only: no real-execution evidence is cited for the "measured" boundary table beyond the stated Chrome for Testing runs.

    • That is adequate for the 137/138 boundary.
    • No evidence is given that the replacement actually passes lockdown() on 137. That is only a plan (item 7), so it is not yet a verified claim.

Self-improvement: none to record for this review.
model claude-opus-5-5 · harness claude · provider anthropic

decomplector — comment-only

I've read the diff and checked the existing SharedURL code it builds on. Here is the block.

Verdict: comment-only. There is one should-fix finding. Nothing blocks merge from the decomplector lens.

Repeated-mechanism pre-pass (prototype / prototype.constructor). The mechanism is needed. Leaving the host prototype unshared breaks instanceof for instances created before lockdown(). Not repointing constructor leaves the host constructor reachable through X.prototype.constructor. The design also reuses an existing pattern instead of creating a new one: SharedURL in packages/ses/src/tame-url-constructor.js already shares the host prototype and repoints constructor. The design moves both onto one shared maker, encapsulateHostConstructor. That answers category (f): this is the smallest primitive that works, and it is not a new mechanism.

Ownership map (derived, since the design has no map section).

  • The host owns the prototype objects. SES mutates them once, before hardening, and the design says so (§ Compatibility, "The shared prototype's constructor is repointed").
  • lockdown() owns the commit step, which is hardening.
  • The shared maker owns replay (the double application on SES-for-XS) through its idempotence check.
  • The permits pass owns classification: whether a property may be kept or must be deleted.

The map is coherent, and no inner type borrows an outer lifecycle name. I confirm it.

Findings

  1. should-fix: the frozen-prototype guard treats "frozen" as if it meant "SES already tamed this."

    • § Revision says that "a pass that finds the binding's prototype already frozen also leaves it alone, because the binding was hardened by an earlier lockdown()." That reads a cause from an incidental state.
    • Suppose another shim, or a second copy of SES, froze TextDecoder.prototype before this copy's lockdown(). The binding is then the host constructor and is not in the maker's WeakSet.
    • The guard silently leaves the host constructor on the global, and it gets sampled.
    • On Chromium before 138, lockdown() then throws again with endojs/endo#3369, behind a misleading path. On clean engines, the host constructor becomes a permitted intrinsic, which breaks the rule the design states: "a permitted host WebIDL constructor is never the host object."
    • Fix: decide only by WeakSet membership. If the binding is not in the set and its prototype is frozen, throw a diagnostic. Do not quietly skip it. Add that case to test plan item 8.
    • [proposed-rule: an idempotence guard must check the marker of its own past action, never an incidental state that other actors can also produce]
  2. comment-only: replacing the global binding couples installation with sampling.

    • The design justifies replacing the global instead of using addIntrinsics because the SES-for-XS shim samples globals at module load (compartment-shim.js calls getGlobalIntrinsics(globalThis, …)). That reasoning holds.
    • The cost: importing SES-for-XS now mutates a host-shared prototype even if lockdown() is never called. § Compatibility does not say this.
    • Add one sentence there. The alternative is for getGlobalIntrinsics to look up a name→tamer table when it samples; that is a taste call.

Out of scope: none.

Self-improvement: nothing to change in the brief. The repeated-mechanism pre-pass fired on a mechanism that already has a precedent in this repo. I confirmed that precedent and moved on, as the brief prescribes.
model claude-opus-5-5 · harness claude · provider anthropic

ergonomist — comment-only

Verdict: comment-only

The design is mostly consistent. Its surface is the encapsulated constructor and its behavioral contract, and that contract is sound: name, length, forwarded arguments, the same RangeError, and instanceof in both directions. The findings below are about how readable the revision is and how people will meet the Host !== TextDecoder change. They are not defects in the surface.

Findings

  1. Should-fix (discoverability of the user-visible break).

    • A user who hits Host !== TextDecoder after upgrading starts at the symptom, not at this design.
    • § Compatibility says the SES changeset will state the identity change and the retirement trigger.
    • Make that a concrete deliverable: name the changeset text, and say whether the lockdown docs or a README note will carry a one-line explanation that links to this design.
    • Leave it as an intention and the user's path is a failing identity check with no signpost.
    • [proposed-rule: a design that changes an observable identity or behavior names the exact user-facing locations (changeset, docs) where the explanation will live.]
  2. Should-fix (error visibility, mismatched wording).

    • The no-new error message follows %SharedURL%'s phrasing: "Calling TextDecoder constructor as a function throws".
    • The sentence reads as a statement, not as an instruction to the caller, and the host's own message differs.
    • The test plan only requires that the message name the constructor.
    • Pick a message that tells the caller what to do (for example, "TextDecoder constructor requires 'new'").
    • If matching the sibling is the goal, say that explicitly and apply it across URL and URLSearchParams.
  3. Should-fix (document structure for a reader).

    • The revision is layered over the original with "Superseded" notes.
    • § What is the Problem and § Lockdown sequencing keep the original text, and a reader must hold the override rule in mind while reading them.
    • The banner helps, but the stale prose remains.
    • Either rewrite those two passages in place or keep the notes as short as possible.
    • The sequencing list now says "step 0" before "step 1", which is a minor oddity of the layering.
  4. Comment-only (naming for the user's mental model).

    • "Encapsulated constructor" and encapsulateHostConstructor name the mechanism.
    • What a user sees is an SES-owned stand-in that keeps the host prototype.
    • The term is defined, so this is taste.
  5. Comment-only (the test plan is hard to scan).

    • Items 7 and 9 are long and mix assertions with rationale, so they are difficult to review quickly.
    • Moving the transitive-walk specification into its own paragraph before the numbered list would read better.
  6. Comment-only (the rejected-alternatives wording).

    • The permits alternative says "adds the drawback that a configurable variant would also be permitted".
    • Say plainly what is lost.

Out of scope for this review: the typing of the shared maker, and whether the transitive walk is feasible across Chrome 137. Both belong to the implementation PR's code panel.

Self-improvement: for design revisions, check where the user-facing explanation of an observable break will live, before checking the break itself.
model claude-opus-5-5 · harness claude · provider anthropic

copyeditor — request-changes

Verdict: request-changes

Findings:

  1. Inconsistent section backreferences. The Revision section is titled "## Revision: encapsulated constructors (2026-10-07)", but it's referenced both as "§ Revision: encapsulated constructors" (opening revision note) and simply "§ Revision" (test plan item 7, "The walk covers..." paragraph). Other sections in the document are consistently backrefed by their full titles (e.g., "§ Lockdown sequencing", "§ What is the Problem Being Solved?"). Use the full title in all backreferences for consistency. [rule: relative-paths] Section: Revision note, test plan item 7, "The walk covers..." paragraph. Verdict: should-fix.

  2. Inconsistent backtick markup in codec-behavior list. "…so instanceof, encodeInto, streaming decode, labels, and the encoding/fatal/ignoreBOM getters retain host behavior." The list uses backticks inconsistently: instanceof and encodeInto are backquoted, "streaming decode" has an odd structure (plain text + backticks), "labels" lacks backticks, and the accessor names are grouped with a slash. For parallel structure, either backquote all technical terms (instanceof, encodeInto, streaming decode, labels, ...) or reformulate as "the instanceof operator, encodeInto method, streaming-decode methods, labels, and the encoding/fatal/ignoreBOM getters". Section: Revision section, paragraph opening "The key mental model is the shared prototype," third sentence. Verdict: should-fix.

  3. Very long sentence in Dependencies item 4. "The permitted codec constructors are SES-owned replacements that delegate to the captured host originals (§ Revision), because the host constructors carry undeletable legacy restricted properties on Chromium before 138 and the restricted-property descriptor is indistinguishable from a live sloppy-function slot, so no tolerance or permit can admit the host constructor objects safely." This accurately conveys multiple interdependent reasons, but is grammatically dense. Consider splitting for readability: "The permitted codec constructors are SES-owned replacements that delegate to the captured host originals (§ Revision), because the host constructors carry undeletable legacy restricted properties on Chromium before 138. The restricted-property descriptor is indistinguishable from a live sloppy-function slot, so no tolerance or permit can admit the host constructor objects safely." Section: Dependencies, item 4. Verdict: comment-only (meaning is clear; the density may be acceptable for technical content, but a split would improve readability).

Self-improvement: Reviewed the design end-to-end for prose mechanics—grammar, sentence structure, paragraph flow, jargon, and transitions. The document is well-organized and mostly clear; the issues above are refinements to consistency and readability rather than fundamental problems.
model claude-opus-5-5 · harness claude · provider anthropic

pedant — comment-only

Verdict: Comment-only

Findings:

  1. List parallelism in encapsulated-constructor design (§ List discipline, Chicago Manual 6.128).
    The four bullet points describing the encapsulated constructor should maintain parallel grammatical construction.
    Items 1–3 each begin with "It [verb]" ("It throws," "It delegates," "It reuses"), but item 4 shifts to a noun-phrase start: "The shared prototype's constructor is repointed..."
    Recommend: "It repoints the shared prototype's constructor at the encapsulated constructor." to match the parallel "It [verb]" pattern.
    Severity: comment-only (the meaning is clear; the inconsistency is within one paragraph and does not affect wider document coherence).

Notes:

  • Cross-references use consistent [name](file.md) markdown-link format per skills/relative-paths/SKILL.md.
  • No em-dashes detected; prose uses parentheses, semicolons, and periods appropriately per skills/em-dash-style/SKILL.md.
  • Special characters (arrows, ellipses, curly quotes) use ASCII forms throughout per skills/typist-friendly-code-points/SKILL.md.
  • Section heading "## Revision: encapsulated constructors" follows the sentence-case-after-colon precedent of "## Lockdown sequencing" in the existing file.
  • Numbered list (test plan items 7–9) and bulleted lists maintain clear structure and capitalization consistency.

Self-improvement: The list-parallelism observation is the sole mechanical style finding; the document otherwise adheres to the project's style rules consistently.
model claude-opus-5-5 · harness claude · provider anthropic

novice — comment-only

Verdict: comment-only

The revision is mostly readable. The banner, the "(Superseded…)" notes and the sketch help a new reader. The findings below are where a reader new to the project would lose the thread.

Findings

  1. should-fix: the revision's causal chain is split across four non-adjacent sections.

    • The reader hits "Superseded… see § Revision" in the Problem section, then a Lockdown note about "step 0", before they know why any of it matters.
    • The "why" (restricted properties, the permits pass, the failed deletion) only arrives about 200 lines later, in § Revision.
    • Suggested fix: add a 3-sentence plain summary to the top banner. Say what broke (lockdown() throws on old Chromium), why (undeletable caller and arguments), and the fix (SES swaps in its own constructor).
    • [rule: roles/jurors/novice/AGENT.md § Primary surface (a)]
  2. should-fix: the "permits" terminology and the "deletes… throws" step come after their first use.

    • The test-plan items 7 to 9, and the Compatibility bullets, say "replacement", "walk", "encapsulated" and "permitted intrinsics" before § Revision defines them.
    • The definitions inside § Revision ("Background:") are good, but they are positioned too late.
    • Suggested fix: move that Background paragraph above the test plan, or link to it from the first use.
    • [rule: roles/jurors/novice/AGENT.md § Secondary surface]
  3. should-fix: the sketch is explained before the reader has the mental model.

    • The text says "the sketch below shows the shape, and the paragraph after it explains why the shared prototype works".
    • Between them sit a 4-bullet list and the code. The list and the code appear in the opposite order from their introduction.
    • The "Key mental model" paragraph is dense. One sentence lists seven behaviors, and the one-way link claim is hard to follow.
    • Suggested fix: add a short worked trace for new TextDecoder(). Show which object is the host instance, which is the prototype, and which is the constructor, each in one line.
    • [rule: roles/jurors/novice/AGENT.md § Primary surface (d), (e)]
  4. should-fix: the "Why the global binding is replaced" paragraph assumes background the document never builds.

    • It leans on addIntrinsics, getGlobalIntrinsics, "universal names", and the SES-for-XS module-load order.
    • It is one long chain of inferences with no intermediate conclusions.
    • Suggested fix: break it into numbered steps. Name the conclusion after each one.
    • [rule: roles/jurors/novice/AGENT.md § Primary surface (c)]
  5. comment-only: test plan item 7 is very long, with nested bullets and several caveats.

    • The purpose of the transitive walk ("prove the host constructor is unreachable") is only clear from the end of the item.
    • Suggested fix: lead with that goal sentence.
    • [rule: roles/jurors/novice/AGENT.md § Primary surface (d)]
  6. comment-only: the Chromium version bands read as measured and inferred at once.

    • "Before 138" appears in the Problem section, the Compatibility notes and the Retirement trigger, even though § Revision labels it an inference.
    • A short qualifier, "inferred", at the first mention would keep the reader from taking it as measured.
    • [rule: roles/jurors/novice/AGENT.md § Primary surface (b)]

Self-improvement: no new rule proposed. Revision documents that patch older sections in place should put the summary at the top, not the bottom.
model claude-opus-5-5 · harness claude · provider anthropic

orthographer — approve

Verdict: approve

Findings:

  • none — the deterministic divergence grep found no British spellings introduced by this change. [rule: skills/american-english-normalization/SKILL.md]
    model claude-opus-5-5 · harness claude · provider anthropic
thesaurus — approve

Verdict: approve

Findings:

  • none — the deterministic cliché grep found no Botese phrases introduced by this change. [rule: skills/botese-normalization/SKILL.md]
    model claude-opus-5-5 · harness claude · provider anthropic

model claude-opus-5-5 · harness claude · provider anthropic · host endolin-garden2-5bcdff64 · garden d9a55c17

@kriscendobot kriscendobot left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Garden panel — round 4 (design panel, single-round): must-fix

Reviewed head 32c1368078de against base llm-fda1ff5 (fda1ff55233b). Seats: 9/9 ok. Request-changes: skeptic, decomplector, pedant, novice. Must-fix items come from novice (superseded prose left as the primary statement; remedy buried after ~100 lines of evidence).

critic — comment-only

Verdict: comment-only

Encapsulation is the right remedy. The rejected alternatives (tolerate the descriptor, fresh prototype, gate on a feature test, permit the properties) are weighed fairly and not strawmanned. The findings below are revisions the design can absorb. None is a must-fix.

Findings

  1. should-fix: the design never shows that the wrapper hides caller and arguments from Function.prototype.

    • The wrapper in the sketch is a sloppy-looking function expression written in SES's own module. Strict-mode functions get no own caller or arguments, so the sketch only holds if the SES source is strict. Say so explicitly.
    • If an implementation used a sloppy function or an arrow wrapper by mistake, the same undeletable-property failure would reappear. Test item 9 checks name and length but not getOwnPropertyNames. Add an assertion that the replacement has exactly length, name, and prototype as own keys. [proposed-rule: encapsulating-constructor tests assert the replacement's full own-key set]
  2. should-fix: the "unreachable host constructor" argument is weaker than it reads.

    • Reflect.construct(Host, args, new.target) reads new.target.prototype. With a Proxy new.target this is an observable trap, but the trap sees only the proxy and the key, as the design says.
    • The WebIDL claim is plausible, but the claim that a "changed existing member would not be pruned" admits that the permits table is not a guard.
    • The only guard is a test run against the engines the CI covers. A host that adds a constructor-returning member would silently re-expose Host to users on a browser the CI never ran.
    • Consider also hardening the walk's reach (for example, a lockdown-time dev assertion). Or state plainly that this residual risk is accepted.
  3. should-fix: the design is inconsistent about what the revision fixes.

    • It narrows endojs/endo#3369 but does not close it, because URLSearchParams and %InitialURL% still throw on Chromium before 138.
    • It also says the codec release "can ship on its own", with changeset wording that tells users it is not the fix.
    • Shipping a release that changes constructor identity and mutates host-shared prototypes, with zero user relief on the affected engines, is a poor trade. Recommend shipping the codec and URL pieces together, or say why the split is worth the churn.
  4. comment-only: side effect at import time on SES-for-XS.

    • Repointing Host.prototype.constructor when SES-for-XS is merely imported is a real escalation of blast radius.
    • It is documented. Consider deferring the XS module-load pass instead, if the compartment-constructor sampling can be restructured.
  5. comment-only: the pre-freeze guard may be too strict.

    • Throwing when a prototype was frozen by another shim, or by a second SES copy running after lockdown(), turns an interop edge into a hard failure.
    • It is defensible, because the alternative is silent exposure. Confirm that the diagnostic message points users to a fix.
  6. comment-only: the "Revised" status line. The doc header says "Revised", but the content still describes unimplemented behavior. "Revised" does not distinguish a design that is landed from one that is proposed.

Out of scope: implementation details of encapsulate-host-constructor.js, which belong to the code panel on the implementation PR.

Self-improvement: no new standing rule is needed beyond proposed-rule item 1.
model claude-opus-5-5 · harness claude · provider anthropic

skeptic — request-changes

Verdict: request-changes

Findings

  1. should-fix: the "unreachable" claim ignores the host prototype's own constructor chain through instances. The design rests on "nothing reachable from the permitted intrinsics points back at the host constructor once constructor is repointed." The shared prototype is also reachable by any pre-lockdown() code. That code can call Object.getPrototypeOf(Host.prototype.constructor) and similar before the repoint, but the plan does not say what happens when other code has already stored the host constructor on an object that a compartment can later reach. The design lists endowments as an escape channel, but item 7's walk starts only from permitted intrinsics and fresh-compartment globals. It does not cover a realm where Host was stashed on a hardened object before lockdown. Acknowledge this as a non-goal, or state the trust boundary explicitly. [proposed-rule: a design that claims unreachability states the threat boundary, meaning who may hold pre-lockdown references.]

  2. should-fix: the "unconditional replacement is safer than feature-gating" rationale is only partly supported. The argument "an unconditional break is visible everywhere and caught by every test run" assumes consumers test on Node. Consumers who test only on Node/Chrome 138+ still ship the identity change to every engine, and that change is invisible on engines they never test. The text does not weigh breaking every user, including unaffected Node and Deno users, to protect Chromium 137 and earlier. The retirement trigger depends on a support floor that the design admits SES does not document. That makes the "possibly temporary" label unfalsifiable, so the replacement may become permanent by default. Attach an owner or condition to producing that floor.

  3. should-fix: the "repointing constructor on the shared host prototype" mutation is more hazardous than the compatibility section treats it. The design says it is made "once, before the prototype is hardened." On SES-for-XS it happens at import time, even if lockdown() is never called. Hosts such as Node's util.TextDecoder and Deno internals may brand-check or compare proto.constructor === TextDecoder. The design does not say whether Node's internal util or fs code reads .constructor. Verify this against Node, or add a test that Node's own consumers of the codecs (fs.readFile with an encoding, StringDecoder) still behave after lockdown.

  4. comment-only: the verified-claim discipline is mostly met, but the 137/138 measurements are not reproducible from the document. The design lists the versions and platform ("Chrome for Testing headless shells, Linux x64") but gives no script, command, or link to the harness. Item 7 does pin 137 and 138 in a Playwright test, which partly compensates. The inferred range (121–125, 128–132, 134, 135) is correctly labeled as inference.

  5. comment-only: the "Shipping" section ships a codec release that does not fix endojs/endo#3369. On affected Chromium, lockdown() still throws on URLSearchParams. The changeset caveat helps, but it ships churn (identity change, repointed constructor) with no user-visible benefit until the follow-up lands. Consider landing the codec change together with the URL follow-up, or at least gating the codec release on the follow-up being ready.

  6. comment-only: the WeakSet per-copy idempotence guard throws when a second SES copy runs after lockdown(). The design accepts this as the lesser evil. Multi-copy realms, such as bundled vendored SES plus a page-level SES, are an actual deployment shape, and the failure is a hard throw. Say whether a documented workaround exists.
    model claude-opus-5-5 · harness claude · provider anthropic

decomplector — request-changes

Verdict: request-changes (one should-fix, two comment-only)

Ownership map (reconstructed). The pre-pass reports four layers. In practice there are four owners, and the design names each of them in prose:

  • The host engine owns the constructor objects and the prototype's internal slots and brand checks.
  • The SES codec taming owns the replacement function, repointing constructor, and the idempotence record.
  • The intrinsics collector owns sampling from the global.
  • The permits pass and harden own the decision to prune or throw.

No durable state, commit/discard, or replay concept is involved; the "persistence" and "lifecycle" layers the pre-pass flagged are false positives. The one datum with two owners is the shared host prototype. The design names that sharing on purpose, defends it in § Rejected alternatives ("Give the replacement a fresh prototype"), and lists the cost in § Compatibility considerations. That is a coherent map, so there is no ownership must-fix.

Findings

  1. should-fix: the shared maker mixes building the constructor with installing it on the global. In § Revision: encapsulated constructors, encapsulateHostConstructor(Host, name) is supposed to state "the rule once" (build a delegating constructor that shares the host prototype). But the same maker also "owns the idempotence check" and "keeps a WeakSet of the encapsulated constructors it has installed." It also throws on a frozen prototype.

    • Idempotence and the frozen-prototype throw only matter because the codecs are installed on the global and sampled twice on SES-for-XS (compartment-shim.js:17 runs getGlobalIntrinsics at module load).
    • SharedURL is the maker's named second consumer. It is returned through addIntrinsics and never installed on the global (tame-url-constructor.js:104-108), so it needs none of that.
    • Fix: split this into a pure maker, (Host, name) → constructor, with no state, and a codec-side installer that owns the WeakSet, the frozen-prototype throw, and the write to the global binding.
    • This also answers the design's own unchecked claim ("That move is not yet checked") that SharedURL will adopt the maker: if the maker carries no install policy, adopting it is trivial.
    • [proposed-rule: a shared helper whose stated purpose is a single rule must not also own the state needed by just one caller's installation strategy; state that exists for a particular call site belongs at that call site]
  2. comment-only: the idempotence state could be a value lookup instead of a record of past actions. The WeakSet records "what I installed." A WeakMap<Host, replacement> would make the second pass return the same replacement for the same host constructor, so idempotence comes from the lookup rather than from checking the global binding. The frozen-prototype case would still need its throw, so this is a matter of taste.

  3. comment-only: the document piles corrections on top of the old text. The problem section keeps its original sampling story and adds a "(Superseded: …)" parenthetical, while § Lockdown sequencing was rewritten in place. A reader has to merge three layers of text to learn the current behavior. Rewriting the problem paragraph to state only the current behavior, and leaving the history to the dated revision banner, would make the document describe the design as it is now. [rule: roles/jurors/decomplector/AGENT.md § Operating norms (d) value-oriented vs place-oriented]

Category (f) check. The design cites an existing surface, SharedURL, and builds a maker on top of it rather than a new mechanism. The maker is the smallest workable abstraction only once finding 1 is applied. The unconditional replacement (rejecting "Replace only where affected") is the simple-but-not-easy choice: there is one code path and one set of assertions, at the cost of a visible change in constructor identity. That choice is sound.

Out of scope (code panel): the test plan item 7 walk's coverage of host closures; the breaker and assessor will review that on the implementation PR.

Self-improvement: when a design names a shared maker "for the follow-up," check the follow-up's existing code for which of the maker's duties it actually needs.
model claude-opus-5-5 · harness claude · provider anthropic

ergonomist — comment-only

Verdict: comment-only

The proposed surface is small and mostly well shaped. The ergonomic risks are in how a user who hits the identity change finds out why, and in how the three constructor-taming surfaces line up with each other.

Findings

  1. should-fix: the user-facing symptom is reachable only through the changeset and guide.md.

    • Surface: § Compatibility considerations, "The replacement is a different function object".
    • The most likely first contact is a Host !== TextDecoder comparison, a missing [native code] string, or a lockdown() failure that now names a different constructor.
    • The design commits to a changeset entry and a guide.md note, which is good.
    • It does not say that the thrown diagnostics ("Cannot tame TextDecoder: its prototype was frozen…", "Calling TextDecoder constructor as a function throws") should point at the explanation.
    • Add a pointer to the guide.md section, or at least a stable keyword, to the frozen-prototype error.
    • That error is the one a user cannot debug without reading the source.
    • [proposed-rule: errors raised by SES taming steps name the remedy or a doc anchor, not only the condition]
  2. should-fix: the message wording is inconsistent across sibling constructors.

    • Surface: the "throws a TypeError when called without new" bullet.
    • The wording matches %SharedURL%'s message: "secure mode Calling %SharedURL% constructor as a function throws".
    • The design's own example is "Calling TextDecoder constructor as a function throws", without the "secure mode" prefix.
    • It claims "the same message shape" but also says the shared maker takes the name as a parameter so SharedURL can keep its existing message.
    • That means the shape is shared only by convention.
    • Decide whether the maker owns the full message, with SharedURL migrating to it, or whether the message is a parameter.
    • State which one, so the follow-up does not drift.
  3. comment-only: the maker's signature is asymmetric with how its callers differ.

    • Surface: encapsulateHostConstructor(Host, name).
    • Static members are left to the caller (SharedURL copies parse and its siblings), so every caller that needs statics repeats the same post-step.
    • An optional options bag ({ statics }) would be easier to adopt than a convention.
    • Leave the signature as is for the codecs.
    • Revisit it when the URL follow-up shows whether two or more callers need statics.
  4. comment-only: the design is hard to scan.

    • Surface: the structure of the document.
    • Revision notes are layered over the original sections with "(Superseded: …)" and "(Revised: …)" parentheticals, and the final section is long.
    • The header banner is a good entry point.
    • A reader landing on test plan item 7 or 8 first must still jump to the Revision section for the vocabulary.
    • Item 7 is dense: it interleaves the Node suite, the Playwright suite, and the walk definition.
    • Consider splitting it into sub-bullets by suite.

Out of scope: the typing of the replacement constructors and ThisType for subclassing belong on the implementation PR's code panel.

Self-improvement: no new standing rule needed beyond the proposed error-diagnostic pointer rule above.
model claude-opus-5-5 · harness claude · provider anthropic

copyeditor — approve

Verdict: Approve

The prose mechanics are sound. The document reads clearly end-to-end, with consistent voice and tense throughout the dense technical material. Sentences parse correctly, paragraph transitions work, and no jargon is left unintroduced.

Findings:

None at the must-fix or should-fix level. The additions maintain professional technical tone and flow naturally into the existing sections. The revision note at the top is clear. Cross-references to the new § Revision: encapsulated constructors section work as forward pointers. Test plan items 7–9 are well-structured with appropriate parallelism in their bullet lists. The expanded Compatibility considerations subsections are clear about the behavioral changes.

A few sentences are notably complex (e.g., "The host constructor still holds its own prototype link to the shared prototype, but that link runs one way: nothing reachable from the permitted intrinsics points back at the host constructor once constructor is repointed, and test plan item 7 checks that claim by a transitive walk of the permitted graph"), but they remain grammatically correct and understandable within a technical design document aimed at the SES team.

No typist-hostile code points (arrows, curly quotes, ellipsis). No ASCII diagrams.


Self-improvement: The design's technical density and length are appropriate for its audience and purpose. The copyeditor sees no prose blocks that impede comprehension, marking this a smooth readthrough for the intended maintainers.
model claude-opus-5-5 · harness claude · provider anthropic

pedant — request-changes

Verdict: request-changes

Findings:

[proposed-rule: hard-to-type code points per skills/typist-friendly-code-points/SKILL.md]
The section symbol (§) appears five times in new cross-references within the revision block and sections:

  • "The details are in § Revision: encapsulated constructors, which" (revision block)
  • "The sampling story in § What is the Problem Being Solved? and" (revision block)
  • "see § Revision: encapsulated constructors.)" (Superseded note, Lockdown sequencing note—appears twice)

Per the project rule, hard-to-type code points like § (which require special keystrokes, e.g. Alt+0167) should be replaced with ASCII equivalents. Replace § with the spelled-out word "section" or the abbreviation "sec." — for example, "The details are in the section Revision: encapsulated constructors, which" or "The details are in sec. Revision: encapsulated constructors, which".


Self-improvement: The review identified one class of violation (§ usage) across all instances. Future reviews should flag typographic symbols used in cross-references, since Markdown links or spelled-out section names are the documented alternatives.
model claude-opus-5-5 · harness claude · provider anthropic

novice — request-changes

Verdict: request-changes

The new section has a clear spine. The "shared prototype" mental model and the worked trace of new TextDecoder() help a new reader. Several places lose that reader first, though.

Findings

  1. must-fix: the document's own top reads as contradictory.

    • The Status banner, the problem statement and the "Lockdown sequencing" section each get a "Superseded" or "Revised" note, and the original text stays beneath each note.
    • A new reader reaches "straightforward: list them, sample, and harden", then a parenthetical says that is wrong.
    • They must then reconcile the two themselves.
    • Rewrite the original passages in place and keep the history in the revision section. Failing that, mark each stale passage clearly as historical.
    • [proposed-rule: a revised design should not leave superseded prose as the primary statement of the approach.]
  2. must-fix: "Revision: encapsulated constructors" front-loads too much before the remedy.

    • It runs from the failure chain to the measured boundary, the inference list, the URL follow-up and four rejected alternatives. The "Adopted remedy" only arrives after roughly 100 lines.
    • A reader loses the thread of what is being decided.
    • Put a three-sentence summary of the remedy first, then the evidence, then the alternatives.
  3. should-fix: assumed background.

    • The following are used without introduction: WebIDL constructor (a gloss is given late), cauterizeProperty, FunctionInstance permit set, SharedURL, %InitialURL%, urlBlobTaming: 'retain', addIntrinsics, and "universal names".
    • The caller and arguments slots are never explained for a reader who doesn't know V8's legacy restricted properties.
    • Add a one-line gloss on first use, or link to the permits and intrinsics docs.
  4. should-fix: the "Why the global binding is replaced" paragraph is hard to follow.

    • It asserts the XS double-sampling problem in one dense run of subordinate clauses.
    • A step-by-step list would help: who samples, when, what they see.
    • The idempotence paragraph after it is similarly dense. It packs the WeakSet rule, the frozen-prototype case and the throw into one block.
  5. should-fix: test plan items 7 and 9 are walls of conditions.

    • Item 7 mixes the Node suite, the Playwright suite, Chrome 137, Chrome 138 and the follow-up's scope.
    • Item 9 chains about eight assertions in prose.
    • Use sub-bullets with one assertion each.
  6. comment-only: the shared-maker paragraph mixes the codecs, SharedURL, error-message parameterization and static helpers. Move the follow-up detail into the URL follow-up section so the codec story stays linear.

  7. comment-only: the "Compatibility considerations" list is long. A short lead sentence giving the user-visible takeaway (identity changes, instanceof still works) would orient the reader before the details.

Self-improvement: no changes to the brief proposed.
model claude-opus-5-5 · harness claude · provider anthropic

orthographer — approve

Verdict: approve

Findings:

  • none — the deterministic divergence grep found no British spellings introduced by this change. [rule: skills/american-english-normalization/SKILL.md]
    model claude-opus-5-5 · harness claude · provider anthropic
thesaurus — approve

Verdict: approve

Findings:

  • none — the deterministic cliché grep found no Botese phrases introduced by this change. [rule: skills/botese-normalization/SKILL.md]
    model claude-opus-5-5 · harness claude · provider anthropic

model claude-opus-5-5 · harness claude · provider anthropic · host endolin-garden2-5bcdff64 · garden d9a55c17

@kriscendobot kriscendobot left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Garden panel — round 5 (design panel, single-round): must-fix

Reviewed head ecf10b2d31e4 against base llm-fda1ff5 (fda1ff55233b). Seats: 9/9 ok. Request-changes: decomplector, pedant. Must-fix items come from pedant (Chicago 6.60: capitalize the independent clause after the colon in "Summary of the remedy:"). The decomplector asks that the shared maker stay pure and that the global install and idempotence WeakSet move to the codec taming (should-fix).

critic — comment-only

Verdict: comment-only (leaning approve after small revisions)

The encapsulation approach is sound and the rejected alternatives are fairly weighed. I read the whole revision, including the compatibility, test-plan and retirement sections. The findings below are should-fix or comment-only. None locks in a cost the implementation could not recover from.

Findings

  1. should-fix: the stated reason for rejecting conditional replacement is weak. The "Replace only where the host constructor is affected" alternative is rejected partly because an unconditional break is "caught by every test run". That is a reason to pay the identity break everywhere. It does not show the break is acceptable. The consequences that matter are these:

    • Every Node, Deno, XS and modern-browser user loses Host === TextDecoder.
    • Every Node, Deno, XS and modern-browser user gets a repointed Host.prototype.constructor.
    • Every Node, Deno, XS and modern-browser user gets a non-native toString, to fix an engine band that is shrinking.

    The "shared maker would carry two policies" argument is circular, because SharedURL is replaced unconditionally by choice. The design should say plainly that it prefers uniformity over minimizing the blast radius. It should also say why conditional replacement with a single test matrix is not preferable. [proposed-rule: when rejecting a narrower-scope remedy, state the cost the broader remedy imposes on unaffected users and justify it on its own terms.]

  2. should-fix: the design depends on an unfinished follow-up.

    • This revision does not fix endojs/endo#3369. lockdown() still throws on Chromium before 138 until URLSearchParams and %InitialURL% are also encapsulated.
    • The design admits this and says the codec revision can ship alone.
    • Shipping a partial fix that changes constructor identity for all users, while giving affected users no relief, is a poor trade. Release them together, or at least make the changeset and ordering gate on the follow-up.
    • The maker's interface is also unsettled: its error-message parameter, and SharedURL's static helpers that the maker leaves to the caller. Settling it before landing codecs avoids reworking the maker later.
  3. should-fix: the unmeasured 121–125, 128–132, 134 and 135 versions deserve a runtime guard. Unconditional replacement makes the design robust to them, which is a point in its favor. The Playwright suite pins only 137 and 138. Add a cheap range sweep, or state why the unconditional design makes it unnecessary.

  4. comment-only: mutating the host prototype is the real cost, and it is underweighted.

    • Host.prototype.constructor is repointed and the prototype is frozen. This affects non-SES code in the realm, including code that runs before lockdown() on SES-for-XS, where merely importing SES triggers it.
    • The XS import-time side effect is documented. It deserves a sentence in the changeset as well.
  5. comment-only: the no-leak argument is empirical.

    • The guarantee rests on the WebIDL surface of the codecs.
    • The walk test guards it, but only against reachable properties. The design says so honestly. Good.
  6. comment-only: the "unfrozen vs. frozen prototype" fail-closed TypeError is correct but blunt.

    • A second SES copy loaded after lockdown() hits a hard failure.
    • Multiple SES copies are an edge case, but it is the one place the design chooses crash over degrade. Confirm that is intentional.

Out of scope: the exact Reflect.construct and new.target edge cases, and the implementation of the shared maker. The code panel handles those on the implementation PR.

Self-improvement: none to record.
model claude-opus-5-5 · harness claude · provider anthropic

skeptic — comment-only

Verdict: comment-only. The premise holds up. The findings below are should-fix and comment-only items, and none is a must-fix.

Findings

  1. should-fix: the Chrome 137 "affected" evidence is for headless shells only.
    The measurements (§ Revision, "Measured boundary") come from Chrome for Testing headless shells. The test plan item 7 Playwright suite and the changeset wording ("observed on Chromium 137 and earlier") generalize that to Chromium. Headless shell and full Chrome could differ in how they define WebIDL constructors. The design should say which build was measured, or have the browser suite run a headed or chromium channel build too. The design should also state that the CfT 137 pin is possible in the existing browser-test harness, since that is an unstated assumption. [proposed-rule: a "measured/verified" claim must name the exact binary or channel observed, and the test plan must be runnable in the repo's harness]

  2. should-fix: the "frozen prototype before taming → throw" guard can turn working setups into a hard lockdown() failure.
    Any shim or polyfill that freezes TextDecoder.prototype before SES runs now fails lockdown(). This includes Node-hosted test environments and a second SES copy on an unaffected engine, where direct sampling used to work. The design accepts this as the lesser evil. It does not weigh that the failure now hits unaffected engines too, because the replacement is unconditional. That is a new regression surface that is not tied to endojs/endo#3369. The design should either quantify who freezes host prototypes pre-lockdown or offer an escape hatch, such as sampling the host directly when its caller and arguments are absent or configurable.

  3. should-fix: the global-binding assumption is unstated.
    The remedy overwrites globalThis.TextEncoder and TextDecoder. It assumes they are writable or configurable data properties. The design does not say what happens if the property is non-configurable, an accessor, or defined on a locked-down host global. It should name that failure mode and say whether it throws or skips.

  4. comment-only: no partial-failure rollback.
    The taming mutates global state (Host.prototype.constructor) before lockdown() finishes. If lockdown() fails later, the host prototype stays repointed, with no cleanup path. Compatibility mentions this only for the XS import case.

  5. comment-only: the unreachability claim rests on current WebIDL.
    The design states this limit itself and mitigates it with the transitive-walk test, which is adequate. A walk of own descriptors cannot catch a host that adds a constructor-returning member later. The design should say the walk runs in CI against each pinned browser.

  6. comment-only: the "shared maker" premise is unchecked against SharedURL.
    SharedURL throws "secure mode Calling %SharedURL% constructor…" and copies parse/canParse. The design's maker takes a name parameter and leaves statics to the caller. The design flags this as unconfirmed, which is honest. The claim that the codec message "deliberately matches" %SharedURL%'s wording is imprecise: "Calling TextDecoder constructor as a function throws" lacks the "secure mode" prefix and the %…% name form.

Test-catalog completeness is otherwise good: double application, subclassing via Proxy and accessor prototype, the identity changes, and the absent-codec path are all covered.

Self-improvement: when a "measured" boundary is cited, check which binary variant was used.
model claude-opus-5-5 · harness claude · provider anthropic

decomplector — request-changes

Verdict: request-changes

This is a design-only PR: one file, designs/hardened-text-codecs-shim.md. I walked the categories and reconstructed the ownership map. The core remedy is simple, not just easy. It encapsulates the host constructor, shares the host prototype, and does it unconditionally on every engine. The design also decomplects one thing well: idempotence keys on the maker's own record (a WeakSet), not on incidental frozen-prototype state. One should-fix remains, in how the new shared maker divides its work.

Ownership map (the pre-pass's four layers map here as host engine / SES taming / intrinsics collector / lockdown() and the SES-for-XS module load):

  • Durable state. The global binding and Host.prototype.constructor are changed in place. The taming layer owns both. The install record (the WeakSet) is owned by the maker.
  • Commit decision. The taming layer decides whether to replace, skip, or throw. It is unclear whether that means tame-text-codecs.js or encapsulate-host-constructor.js; see finding 1.
  • Replay (applying the taming twice on XS). Owned by the maker's idempotence check.
  • Classification (host constructor vs. the maker's replacement vs. another copy's replacement). Owned by the maker, and the frozen-prototype throw is explicit.
  • No inner mechanism is named for an outer lifecycle concept.

Findings

  1. should-fix: the shared maker mixes building a value with changing global state. [rule: roles/jurors/decomplector/AGENT.md § ownership-map, (2) commit/discard owner; (d) value- vs place-oriented]

    • The signature in § Revision, encapsulateHostConstructor(Host, name), takes a value and returns a constructor.
    • But the same section says the maker "owns the idempotence check" and keeps a WeakSet of the constructors "it has installed". That check works by finding "the global binding already in that set", so the maker must also read and write globalThis, a place it is never passed.
    • This matters because the stated second consumer cannot use the install half. In the base tree, tame-url-constructor.js:47-109 builds SharedURL and returns it as %SharedURL% through addIntrinsics. It never writes the global and has no install record.
    • Shipped as written, the maker carries one policy (install on the global, idempotent) that only the codecs use. The design's own rejection of the conditional replacement warns against exactly this: a shared maker "carry[ing] two policies".
    • Fix: keep the maker pure. It builds the delegator, sets length, name and prototype, and repoints constructor. Put "replace the global binding, skip if already ours" in the codec taming. That is the only caller that needs it, because of the module-load sampling in compartment-shim.js:17.
    • State plainly which module owns the WeakSet.
  2. comment-only: the maker is generalized ahead of its second user. [rule: roles/jurors/decomplector/AGENT.md § Category (f), cited surfaces]

    • The design cites SharedURL as the existing shape, then builds a shared maker with a configurable error-message name to serve a follow-up. It admits the adoption is "not yet checked".
    • Moving SharedURL into the maker inside this revision, or leaving the maker private to the codec module until the follow-up proves the shape, avoids adding a third copy of the pattern.
  3. comment-only: the reason for writing the global could be stronger. [proposed-rule: when a taming writes a global binding rather than returning an intrinsic, the design names every reader that forces the write]

    • § Revision justifies writing the global only by the SES-for-XS sampling at module load.
    • The start compartment's own TextDecoder binding (shared prototype, frozen) is a second reader. Naming it closes the question of why the codec taming can't be a value-only step in getGlobalIntrinsics.

Out of scope: none.

Self-improvement: the "is the shared maker pure or does it install?" check generalizes. When a design extracts a shared helper from an existing site, compare the helper's described side effects against that site in the base tree.
model claude-opus-5-5 · harness claude · provider anthropic

ergonomist — comment-only

Verdict: comment-only

The surface this design proposes is small, and what it exposes to a guest is mostly right. The ergonomic gaps are in how a surprised user reaches the explanation.

Findings

  1. should-fix: the Host !== TextDecoder surprise has a weak discovery path.

    • Sections: Compatibility considerations, "Constructor identity changes after lockdown" and the user-facing explanation list.
    • Users will meet this as a failed === check, a failed [native code] feature test, or a changed constructor. None of these errors names SES.
    • The design points them to a changeset and a guide.md note. Changesets are read at upgrade time, not at the moment of surprise.
    • Ask: the guide.md note should list the symptoms (identity, toString, constructor) and show an instanceof or prototype-based check as the portable alternative. That is the mental model the design itself uses.
    • [proposed-rule: a design that changes observable identity must name the user-visible symptoms and the recommended replacement check in the user-facing doc.]
  2. should-fix: the shared maker's parameter shape is unsettled.

    • Section: Revision: encapsulated constructors, encapsulateHostConstructor(Host, name).
    • The design says the maker takes the error-message name as a parameter so that SharedURL can keep its own wording. It also says the codec message deliberately matches %SharedURL%'s existing message.
    • Those two statements pull in different directions. A name argument that is both a label and a message-template selector is ambiguous.
    • Ask: state whether the argument is just the constructor name, with a fixed message template and SharedURL's wording changed, or an options bag such as { name, callMessage }.
    • Return shape and statics ("leaves static members to the caller") also need one sentence: does the maker return the constructor, or install it on the global?
  3. should-fix: failure visibility on the guard path.

    • Section: idempotence paragraph.
    • A second SES copy running after lockdown() throws a TypeError that blames "its prototype was frozen before SES could encapsulate it."
    • The user's mental model is "I loaded SES twice." The message should name that cause as a likely reason. Otherwise they have to debug host-prototype freezing.
  4. comment-only: the lockdown() partial-fix window is clearly flagged, but it is a bad first-use experience.

    • Section: Shipping.
    • Chromium users upgrade, still see lockdown() throw, and now the failure names URLSearchParams or %InitialURL%.
    • Ask: have the thrown error or the changeset cross-link endojs/endo#3369, so the next failure points at the tracking issue.
    • Shipping the codecs first is defensible.
  5. comment-only: the test plan's items 7–9 are long, with nested assertions. Splitting them into named sub-tests would make failures easier to read, which is an implementation matter.

Out of scope: typing the maker (Host constructor generics) belongs to the code-panel typist.

Self-improvement: no new standing rule needed beyond proposed-rule in finding 1.
model claude-opus-5-5 · harness claude · provider anthropic

copyeditor — comment-only

Verdict: Comment-only

Findings:

  1. [Revision: encapsulated constructors section, paragraph beginning "Why the global binding is replaced..."] The comparative structure here is grammatically valid but awkwardly interrupted. The sentence reads: "Why the global binding is replaced, rather than returned through addIntrinsics (parenthetical explanation) as tameUrlConstructor and tameDateConstructor return theirs." The embedded parenthetical about the lockdown() call disrupts the parallel being drawn between the two approaches. Restructure to keep the "rather than Y / as X and Z do Y" comparison intact — either move the parenthetical explanation after the comparison or recast it as a separate sentence. Verdict: should-fix. [Proposed rule: comparative "rather than / as" structures should not have intervening parentheticals that separate the parallel clauses.]

  2. [Revision: encapsulated constructors section, paragraph "SES does not yet document..."] The phrasing "the retirement decision therefore needs one stated first" is grammatically correct but sounds formal/stiff for modern technical prose. Replace "needs one stated first" with "needs a stated supported-browser floor" or "requires the supported-browser floor to be stated first." Verdict: comment-only. [Applies existing rule on conversational register.]

  3. [Overall document] No typist-hostile code points detected. The code example in backticks under the adopted remedy is appropriately placed in a code block and does not need mermaid conversion. The prose reads end-to-end with strong structure and clear section transitions. The addition of the new "Revision: encapsulated constructors" section significantly expands the document but maintains logical flow; the subsections (Summary, Background, Measured boundary, Rejected alternatives, Adopted remedy) are well-organized and easy to follow.

Self-improvement: This document benefited from a staged expansion approach: retaining the original sections while appendeding the detailed revision section, which made the scope and intent of the change clear without obscuring the original design. The jargon (WebIDL, idempotent, permits graph, encapsulation) is introduced or explained in context, and the document's voice remains consistent throughout the new material.
model claude-opus-5-5 · harness claude · provider anthropic

pedant — request-changes

Verdict: Request changes

Findings:

  1. Capitalization after colon [rule: Chicago Manual of Style 6.60]

    • Line ~340 ("Summary of the remedy:"): When a colon introduces an independent clause, capitalize the first word. "before the intrinsics collector samples..." is an independent clause (subject "SES", verb "replaces"). Change to: "Summary of the remedy: Before the intrinsics collector samples the global object, SES replaces..."
    • Verdict: must-fix (Chicago Manual convention violation).
  2. Hard-to-type code-point compliance [rule: skills/typist-friendly-code-points/SKILL.md]

    • Scanned the entire diff; no Unicode arrows (→), ellipses (…), curly quotes, or comparison operators (≤/≥/≠) found. Code points are ASCII-friendly throughout.
    • Verdict: pass.
  3. Em-dash avoidance [rule: skills/em-dash-style/SKILL.md]

    • No em-dashes detected in the new text. Complex sentence breaks use periods, parentheses, commas, or colons appropriately.
    • Verdict: pass.
  4. Relative paths within document tree [rule: skills/relative-paths/SKILL.md]

    • Paths like packages/ses/src/encapsulate-host-constructor.js, packages/ses/docs/guide.md, and markdown links [hardened-url-shim](hardened-url-shim.md) are correctly relative.
    • Verdict: pass.
  5. Serial-comma consistency [rule: Chicago Manual of Style 6.18]

    • Serial commas applied consistently throughout (e.g., "A, B, and C" structure in lists and prose).
    • Verdict: pass.
  6. Cross-reference and citation form consistency [rule: Chicago Manual of Style 14.101–14.120]

    • Section references ("the section Revision: encapsulated constructors") and markdown links consistent throughout the new content.
    • Verdict: pass.
  7. List and heading discipline

    • New test-plan items 7, 8, 9 use consistent capitalization and parallel imperative structures within each item. Rejected-alternatives section uses bold titles with explanatory paragraphs consistently.
    • Verdict: pass.

Self-improvement: This review has reinforced that Chicago Manual 6.60's rule on capitalization after colons introducing independent clauses is load-bearing for formal document style; the revision section's introduction is a clear teaching instance of the rule in practice. The document's overall adherence to project-specific style rules (no em-dashes, typist-friendly code points, relative paths) is strong.
model claude-opus-5-5 · harness claude · provider anthropic

novice — comment-only

Verdict: comment-only

The document is mostly followable, and the worked trace and the shared-prototype explanation are the strongest parts. A new reader still stumbles in four places.

Findings:

  1. should-fix: the front half is not self-contained.

    • The problem statement (line 31 onward) leans on "the permits", "intrinsics collection" and "undeletable caller/arguments" before any of them is explained.
    • The plain explanation ("lockdown() walks an allowlist and throws if a deletion fails") is first given at line 318, deep inside the Revision section.
    • The reader is told why replacement is needed ("see the section Revision") before knowing what failed.
    • Suggestion: move the 3-step failure account and its definitions up into the problem statement, or into a short Background section ahead of the Design section.
      [proposed-rule: a design defines its core terms (permits, intrinsics, sample) in or before the problem statement]
  2. should-fix: the Revision section is hard to follow top-down.

    • It runs motivation, measurements, a rejected-alternatives list, the adopted remedy, then the mental model and worked trace.
    • The ordering is "remedy summary", then "background", then "failure", then "rejected", then "adopted remedy".
    • The summary at line 305 helps, but the actual remedy and its trace arrive about 200 lines later.
    • Splitting it into Failure, Remedy, Alternatives considered and Edge cases would let a reader stop after the remedy and still have the main idea.
  3. should-fix: paragraph density in the idempotence and multi-copy passages.

    • These are lines 591-614 and 278-295.
    • Each packs several conditions into long sentences, for example "a pass that finds a binding not in the set whose prototype is already frozen cannot repoint…".
    • A small table or numbered cases would work better: binding in the WeakSet, binding not in the WeakSet with an unfrozen prototype, and binding not in the WeakSet with a frozen prototype.
  4. should-fix: the test plan is unreadable to a newcomer.

    • Items 7 and 9 are very long lists of assertions with no stated purpose per cluster.
    • Item 9 mixes subclassing, Proxy F, own-keys, and error-message checks.
    • Short group headings would help.
  5. comment-only: undefined mental models.

    • "WebIDL constructor" and "brand check" get a one-line gloss at most.
    • "Encapsulated" is used throughout, but no sentence defines it as "a wrapper that hides the host constructor".
    • The SharedURL digression (lines 460-476) interrupts the codec explanation. It could move to a short "relation to the URL follow-up" note.
  6. comment-only: stale Phases section.

    • The Phases section (line 644 onward) never mentions the new tame-text-codecs.js or the shared maker.
    • A reader following Phases would miss the central piece of work.
    • Update it so the outline and the revision agree.

Self-improvement: none beyond noting that revision sections would read better if each opened with the failure, then the remedy, then the evidence.
model claude-opus-5-5 · harness claude · provider anthropic

orthographer — approve

Verdict: approve

Findings:

  • none — the deterministic divergence grep found no British spellings introduced by this change. [rule: skills/american-english-normalization/SKILL.md]
    model claude-opus-5-5 · harness claude · provider anthropic
thesaurus — approve

Verdict: approve

Findings:

  • none — the deterministic cliché grep found no Botese phrases introduced by this change. [rule: skills/botese-normalization/SKILL.md]
    model claude-opus-5-5 · harness claude · provider anthropic

model claude-opus-5-5 · harness claude · provider anthropic · host endolin-garden2-5bcdff64 · garden d9a55c17

@kriscendobot kriscendobot left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Garden panel — round 6 (design panel, single-round): must-fix

Reviewed head 8fc7ac258b3d against base llm-fda1ff5 (fda1ff55233b). Seats: 9/9 ok. Request-changes: critic, decomplector. The panel's mechanism-repeat pre-pass forced the decomplector's is-it-needed question. The must-fix item comes from the decomplector: the WeakSet, the double application, and the constructor-repoint idempotence machinery exist only because of the XS import-time sampling defect. Open PR #1425 removes that defect. Depend on #1425 (or fold it in), and rewrite § Lockdown sequencing as a single application inside lockdown(). Should-fix items: the critic and the skeptic both flag the codec-only partial fix of endojs/endo#3369 (it needs a tracked gate or a stated benefit). The skeptic asks for non-writable/non-configurable constructor descriptor states to be covered. The critic and the skeptic both say the unreachability walk needs cross-engine evidence or a narrower claim. The ergonomist and the decomplector flag the error-message and install-policy split with SharedURL. The novice asks for the adopted remedy to come before the rejected alternatives.

critic — request-changes

Verdict: request-changes

The approach is sound: replace the host constructor and share the host prototype, which avoids both tolerating the restricted properties and permitting them. The rejected alternatives are mostly rejected on real grounds. The issues below can all be absorbed in a revision.

Findings

  1. Should-fix: the shipped fix may not close endojs/endo#3369 for anyone on affected Chromium.

    • Section "Revision: encapsulated constructors" says lockdown() still throws on URLSearchParams and %InitialURL% on Chromium before 138 after this lands.
    • The design admits the codec revision gives no relief on its own, yet it argues for shipping the codecs first.
    • The stated reason is "new code with no existing users of a tamed constructor". That is an ordering preference, not a reason to ship a partial fix.
    • Either land all three constructors together, or state the concrete benefit of shipping the codecs alone.
    • [proposed-rule: a design that narrows a bug rather than closes it should justify splitting the release on user-visible benefit]
  2. Should-fix: the module-load taming on SES-for-XS has an unexamined cost.

    • The design says importing SES-for-XS repoints the shared prototype's constructor, even if lockdown() is never called.
    • Mutating host-shared state at import time is a larger side effect than the Node/browser path has.
    • The alternative of having the shim compartment sample from addIntrinsics, or deferring the sampling, is not weighed.
    • The "replace the global binding" rationale rests on getGlobalIntrinsics running outside lockdown() on XS. Say whether that call can be moved, or reject moving it explicitly.
  3. Should-fix: the "unconditional replacement" rationale is partly circular.

    • The case against feature-gating the replacement is that identity would differ by engine.
    • Unconditional replacement breaks identity on every engine today, for engines that did not need it.
    • Weigh that population (Node, Deno, Chromium 138 and later) against the testing-cost argument. As written, the cost is called "visible everywhere" and that is treated as a virtue.
  4. Should-fix: the "host constructor stays unreachable" argument is empirical and rests on an unproven premise.

    • The design admits the argument depends on current WebIDL shapes, and the walk in item 7 guards it.
    • The walk is a test of the current engine only. It does not catch a future host member that returns a constructor.
    • Say who re-runs it and when. As written it protects only the Chromium versions pinned in CI.
  5. Should-fix: the per-copy WeakSet leaves the multi-copy-after-lockdown() case unresolved.

    • The design chooses to throw a diagnostic rather than tolerate the case.
    • It then calls that outcome correct. A throw from a second SES copy is a new hard failure mode for apps that bundle two copies.
    • Weigh that against a realm-wide marker, such as a symbol-keyed non-enumerable property on the delegator or a Symbol.for registry entry.
    • The design rejects the WeakSet workaround for the wrong reason: a marker on the frozen replacement would distinguish another copy's replacement from a frozen host prototype.
  6. Comment-only: the test plan is heavy for a small mechanism.

    • Items 7 to 9 contain many assertions, including the Proxy and accessor Reflect.construct cases.
    • They are valuable but could be consolidated.

No ASCII diagrams are present.

Self-improvement: none.
model claude-opus-5-5 · harness claude · provider anthropic

skeptic — comment-only

Verdict: comment-only

Findings:

  1. should-fix, Host.prototype.constructor repointing assumes the property is writable or configurable. The remedy repoints the shared prototype's constructor. Test item 8 covers a prototype frozen before the taming. It does not cover a prototype that is unfrozen but has a non-configurable or non-writable constructor, or one that is sealed or non-extensible. The design names only the frozen case as a failure mode. A host that ships a non-writable constructor would make the maker throw an untyped error from deep inside lockdown(). State the expected behavior (a diagnostic TypeError, as in the frozen case) and add a test. [proposed-rule: failure-mode handling must enumerate every descriptor state of each mutated host property, not only the one that is already guarded]

  2. should-fix, the "unreachable host constructor" argument rests on an unverified premise. The section "Why the host constructor stays unreachable" asserts that no method or accessor returns a constructor. The design says itself that this is empirical, and it rests on the walk in item 7. The walk is run only in Node (with synthetic caller/arguments descriptors) and on Chrome 137. It is not run on Firefox, WebKit, Deno, or XS. The design admits that only Chromium has been measured, yet the unconditional replacement ships everywhere. The claim "same shape on all engines" is therefore asserted, not verified. Either scope the claim to the measured engines or add the walk to the other engine suites. [proposed-rule: cross-engine claims need cross-engine evidence or must be scoped]

  3. should-fix, the shipping path leaves users with a half-fix. The codec release does not fix lockdown() on affected Chromium while URLSearchParams and %InitialURL% remain host constructors. The design accepts this and says the changeset will say so. The premise that the follow-up lands promptly is untracked: no issue, owner, or ordering gate is named. A codec-only release does not remove the lockdown() throw (the thing endojs/endo#3369 reports) but does add the identity break and the constructor repoint for every user. Consider gating the codec release on the follow-up, or at least cite a tracking issue. [proposed-rule: a partial fix that carries a user-visible compatibility cost needs a tracked completion gate]

  4. comment-only, the measured boundary is extrapolated. Intermediate versions were not measured. The design correctly says so, and the unconditional replacement makes the gap harmless. The retirement trigger ("oldest supported Chromium is 138 or later") depends on a support floor that SES does not document. That makes the retirement trigger unactionable as written.

  5. comment-only, the [native code] and identity break is documented but not tested. A test could assert the replacement's toString and own-keys behavior. Item 9 covers own keys. Nothing covers toString.

No claims of "verified" without evidence were found. The Chrome 137/138 measurements are described with their version, platform, and date.

Self-improvement: no changes proposed.
model claude-opus-5-5 · harness claude · provider anthropic

decomplector — request-changes

Verdict: request-changes

Findings

  1. Must-fix: the double-application machinery exists only because of an XS sequencing defect that #1425 already fixes. [rule: roles/jurors/decomplector/AGENT.md § category (f) binds on repeated findings] Rounds 3 to 5 kept raising must-fix findings on the WeakSet, the constructor repointing, and the idempotence text in § Revision. They all come from one decision: replace the global binding and apply the taming twice (once at module load on SES-for-XS, once in lockdown()). The design's only reason for that, in § Revision ("Why the global binding is replaced…"), is that src-xs/compartment.js builds ShimStartCompartment at import time with getGlobalIntrinsics. Open PR #1425 ("sample XS compartment intrinsics at lockdown", which refs #1349) removes that import-time sampling from the post-lockdown path. It builds the adapter from makeShimStartCompartment() after hardenIntrinsics(), as Node already does through setGlobalObjectMutableProperties in src/lockdown.js. Once #1425 lands, the taming runs once, inside lockdown(), before makeIntrinsicsCollector samples universalPropertyNames (src/intrinsics.js:166).

    • These can then be deleted: the module-level WeakSet; the first half of test item 8; the claim in § Compatibility that importing SES-for-XS repoints constructor without lockdown(); and most of the "More than one copy of SES" bullet.
    • The fix: depend on #1425 (or fold it in), and rewrite § Lockdown sequencing step 1 and the idempotence paragraph as a single application.
    • Today the design braids XS bootstrap timing into a tamer that should be value-in, value-out.
  2. Should-fix: two install policies for one rule. [proposed-rule: when a design introduces a shared maker "to state the rule once", every consumer it names must use the same install path, or the design must say why they differ] encapsulateHostConstructor is meant to be shared with SharedURL. But the codecs write globalThis, while SharedURL goes back through addIntrinsics (tame-url-constructor.js:104-109). The URLSearchParams follow-up is universal like the codecs, so it will hit the same split. After finding 1, writing the global once just before the collector samples it is the only policy left. The design should say that this one write is the install rule for universal host WebIDL constructors, so the follow-up does not rebuild idempotence logic.

  3. Comment-only: the ownership map is implicit but coherent. I reconstructed it:

    • Persistent state: the host prototype. The design admits it is shared and mutated once.
    • Commit/discard: the taming decides replace or throw.
    • Restart/replay: the second-pass idempotence, which goes away under finding 1.
    • Execution classification: the permits pass.

    Nothing is claimed by two layers, and no inner type carries an outer-layer lifecycle name. After finding 1 there is no restart/replay row at all, which is the simplification.

  4. Comment-only: the shared prototype is essential complexity, not accidental. The rejected fresh-prototype alternative is argued correctly: instanceof must hold for instances the host builds. I am not raising anything further on it.

Out of scope: none.

Self-improvement: when a design justifies a mechanism with a sequencing quirk in another module, run gh pr list --search against that module for an open fix before endorsing the mechanism. Here #1425 is that fix.
model claude-opus-5-5 · harness claude · provider anthropic

ergonomist — comment-only

Verdict: comment-only

The design lands on a coherent surface. It keeps the user-visible API unchanged (new TextDecoder(), instanceof, subclassing). It also gives the one visible break, Host !== TextDecoder, a documented home. The findings below are small.

Findings

  1. Should-fix: the error message and permitted shapes are spelled inconsistently across siblings.
    The design says the maker's message "deliberately matches %SharedURL%'s existing error". The two wordings are different, though: SharedURL says "secure mode Calling %SharedURL% constructor as a function throws", and the codec example says "Calling TextDecoder constructor as a function throws".
    The design also hedges that the maker "takes the error-message name as a parameter".
    A user who greps logs for one family of failures would see two spellings.
    Either state that the maker emits exactly one template, and that SharedURL changes in the follow-up, or drop the claim that they match.
    [proposed-rule: when a design says two messages "match", quote both verbatim and say which one changes.]

  2. Should-fix: the user's discovery path for the identity break depends on two artifacts that don't exist yet.
    A user who hits Host !== TextDecoder will search for the symptom, not for this design.
    The design commits to a changeset and a packages/ses/docs/guide.md note, which is the right plan.
    But the error text in the double-lockdown diagnostic ("Cannot tame TextDecoder: its prototype was frozen...") doesn't link or name the design or the guide.
    Add the guide anchor or design name to that message, since the message is where this user will land.

  3. Comment-only: encapsulateHostConstructor(Host, name) has an under-specified positional signature.
    The design also describes an "error-message name" parameter and caller-supplied static members.
    With a third argument likely, an options bag such as { name, errorName } is easier to read than positional strings.
    name and the error-message name are both strings, so a swapped call would not be caught.
    The design should settle the signature before the SharedURL follow-up depends on it.

  4. Comment-only: the revision section is hard to find your way around.
    The banner "Revision: encapsulated constructors" is cross-referenced about ten times by section name.
    The section opens with a summary, then the background and evidence, then the rejected alternatives, and only then the adopted remedy.
    A reader looking for "what changed" has to scroll past the evidence. A one-line pointer from the summary to "Adopted remedy" would help.

  5. Comment-only: the retirement trigger is awkward for users.
    It depends on a supported-browser floor that "SES does not yet document". Say who owns stating that floor, so the trigger does not stay indefinitely open.

Out of scope for this seat: the soundness of the unreachability argument, and the adequacy of the transitive-walk test. These belong to the breaker, warden and critic seats.

Self-improvement: no new rule is warranted beyond the proposal in finding 1.
model claude-opus-5-5 · harness claude · provider anthropic

copyeditor — approve

Looking at the diff for PR #1431, I've read through the complete design document update. This is a substantial revision adding technical depth to the hardened-text-codecs-shim design.


Verdict: APPROVE

Findings:

Voice and tense. Consistent throughout. The revision maintains present tense where appropriate and conditional tense in the "Rejected alternatives" section where it's needed to discuss hypothetical approaches. The shift from the original design summary to the new Revision section is smooth and marked clearly with a date stamp.

Sentence structure. The document contains many long sentences, but they are grammatically sound and necessary for technical precision. Examples: "Before the intrinsics collector samples the global object, SES replaces each host TextEncoder and TextDecoder with an SES-owned constructor that delegates construction to the host through Reflect.construct and reuses the host prototype object, so instances and instanceof keep host behavior." This construction is appropriate for capturing a single cohesive technical concept. No comma splices, run-ons, or fragments.

Jargon introduction. Terms are introduced adequately. "WebIDL constructors" is explained by context; "permits" and "permits-enforcement pass" are defined in the Background paragraph; "encapsulation" is introduced as a concept in the Adopted remedy section. The progression from problem → alternatives → remedy is clear.

Paragraph and section flow. The Revision section opens with a clear summary, then moves through background, measured evidence, rejected alternatives, the adopted remedy with detailed explanation, and compatibility considerations. The "Worked trace of new TextDecoder() after lockdown()" uses a numbered sub-list that clarifies the conceptual sequence. Transitions between subsections in the Revision section are clear and logical.

Structural clarity. The reordering of the "Lockdown sequencing" steps (now 4 instead of 3, with codec taming as step 1) is an improvement that clarifies when the taming happens relative to the existing intrinsics-collection flow. The removed phrase "with no new lockdown phase" is appropriate to remove since the taming is now explicitly listed.

Typist-hostile code points. No arrows (→), curly quotes, ellipsis (…), or other problematic Unicode in the added prose. [rule: typist-friendly-code-points]

Diagrams. The code snippet showing the encapsulated constructor shape is properly fenced and appropriate to keep as runnable code, not converted to mermaid. No ASCII art diagrams. [rule: no-ascii-diagrams]


Self-improvement: I verified the diff structure carefully, checked for grammatical soundness across long technical sentences, traced jargon introduction, and confirmed all typist-hostile characters were absent. The document's technical depth required close reading to ensure clarity was not sacrificed; the prose holds up well despite its density.
model claude-opus-5-5 · harness claude · provider anthropic

pedant — comment-only

Verdict: comment-only

Findings

  1. Path format inconsistency in code references.
    The new section introduces references to code files with mixed formats: some use full paths from repository root (packages/ses/src/tame-text-codecs.js), others use bare filenames (tame-url-constructor.js). The project rule [skill: relative-paths] specifies that paths within the document tree should be relative. Either standardize to relative paths from the design document location (../../packages/ses/src/tame-text-codecs.js) or — if absolute paths better serve readability — apply the format consistently across all code-file citations. Currently the mix creates minor friction.

  2. List-item formatting in item 9 is sound. The sub-list under "Subclassing and pre-lockdown captures" uses imperative bullet points starting with assertions or descriptions (\class X extends TextDecoder {}`..., \new TextDecoder() instanceof Host`...). The structure is clear despite the density. No fix needed; noted as an exemplary handling of technical enumeration.

Self-improvement

The pedant's review caught one style consistency gap (path formats) and one area of strong existing form to learn from. The document's use of inline cross-references ("the section Revision: encapsulated constructors") and its constraint-reasoning prose ("Why the shared prototype's constructor is repointed.") set a high bar for design readability. The prose discipline around measured vs. inferred claims ("Measured boundary... Inference, not measurement...") is exemplary — a model to reference in future reviews when judging rigor in evidence-claiming sections.
model claude-opus-5-5 · harness claude · provider anthropic

novice — comment-only

Verdict: comment-only

Findings:

The revision opens well. The blockquote summary, the five-step failure chain, and the worked trace of new TextDecoder() carry a new reader through the change. The remaining problems are about order and density. None of them loses the reader at a step that decides what the design means.

  1. should-fix: the revision section buries its "what we do" under rejected alternatives.

    • The section runs: summary, background, failure, measurements, the URL follow-up, four rejected alternatives, and only then "Adopted remedy".
    • A new reader meets "Give the replacement a fresh prototype" and "Replace only where affected" before they have seen what the replacement is.
    • Moving "Adopted remedy" and its sketch ahead of "Rejected alternatives" would let each rejection read as a contrast with something already understood.
    • [proposed-rule: designs state the chosen approach before the alternatives it was chosen over.]
  2. should-fix: the URL/URLSearchParams follow-up detour interrupts the measurements-to-remedy flow.

    • The paragraph about URLSearchParams, %InitialURL%, SharedURL, and "closes endojs/endo#3369 only when it lands" sits between "Measured boundary" and "Rejected alternatives".
    • It introduces %InitialURL%, SharedURL, and urlBlobTaming: 'retain' with no explanation.
    • A reader who has not seen hardened-url-shim cannot follow it, and it is not needed to understand the codec fix.
    • Move it to a short "Scope and follow-up" subsection near the end, or define those terms briefly.
  3. should-fix: the adopted-remedy section has several long, multi-claim paragraphs.

    • The shared-maker paragraph covers the maker's purity, SharedURL's differences, the error-message parameter, and "confirms the adoption" before the bullet list that actually describes the constructor.
    • Reading it, I lost track of what the maker does, which is the thing I needed.
    • Put the four-bullet behavior list first.
    • Then move the SharedURL compatibility notes into a separate "Relation to SharedURL" paragraph.
  4. should-fix: the idempotence explanation arrives before its motivation is clear.

    • "Why the global binding is replaced rather than returned through addIntrinsics" depends on a clear picture of the SES-for-XS double sampling.
    • That picture is given in one dense paragraph, and the sentence "Replacing the binding once... makes every sampling agree" skips the inference.
    • A two-line timeline would help: module load samples the global, then lockdown() samples again.
    • Then explain why a returned intrinsic is seen only by the second sample.
  5. comment-only: the term "encapsulated", and the sentence that uses it, assume a model the reader has not built.

    • "Encapsulated constructor" is used as a term of art in the summary and the title.
    • It is defined only by its behavior later, and the word does not obviously mean "SES-owned delegating wrapper".
    • A one-line gloss at first use would help.
    • "Host" is also used for the engine and for the captured original constructor (Host). Distinguish them.
  6. comment-only: test plan items 7 and 9 are long, nested lists.

    • A new reader cannot tell which assertions matter most.
    • Lead each item with a one-sentence statement of what it proves.
  7. comment-only: the escape-channels list is dense.

    • It assumes the reader understands "host closures or internal slots".
    • One example would make it concrete.

Self-improvement: no new rule beyond the proposed ordering rule; the novice brief covers this case.
model claude-opus-5-5 · harness claude · provider anthropic

orthographer — approve

Verdict: approve

Findings:

  • none — the deterministic divergence grep found no British spellings introduced by this change. [rule: skills/american-english-normalization/SKILL.md]
    model claude-opus-5-5 · harness claude · provider anthropic
thesaurus — approve

Verdict: approve

Findings:

  • none — the deterministic cliché grep found no Botese phrases introduced by this change. [rule: skills/botese-normalization/SKILL.md]
    model claude-opus-5-5 · harness claude · provider anthropic

model claude-opus-5-5 · harness claude · provider anthropic · host endolin-garden2-5bcdff64 · garden d9a55c17

@kriscendobot

Copy link
Copy Markdown
Collaborator Author

Gauntlet terminal — review-budget-reached · rounds run: 6 · head 15dd235cc5551111d150dad600808f65d72b0873 · CI: green · next: awaiting maintainer merge/undraft or re-run decision

model automatic · host endolin-garden2-5bcdff64 · garden 6091cc10

@kriscendobot
kriscendobot force-pushed the design/hardened-text-codecs-encapsulation branch from 9076c29 to c3ab267 Compare October 29, 2026 15:25
@kriscendobot kriscendobot changed the title docs(designs): record the encapsulation revision of the hardened text codecs docs(designs): record the frozen-null tolerance fix for older-Chromium text codecs Oct 29, 2026
@kriscendobot
kriscendobot changed the base branch from llm-fda1ff5 to llm-9a14ddf October 29, 2026 15:25
…dec constructors

endojs/endo#3376 (merged 2026-10-29) fixed endojs/endo#3369 by tolerating
frozen null caller/arguments pills in cauterizeProperty. The encapsulation
remedy (#1349, endojs/endo#3377) closed as
superseded. Rebuild this revision on current llm: record the failure, the
measured Chromium boundary, the adopted tolerance and its sloppy-function
residual, and the rejected alternatives.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@kriscendobot
kriscendobot force-pushed the design/hardened-text-codecs-encapsulation branch from c3ab267 to 1286f42 Compare October 29, 2026 15:36

@kriscendobot kriscendobot left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

critic — comment-only

Verdict: comment-only

This is a doc-only revision of designs/hardened-text-codecs-shim.md that records a merged upstream fix. The chosen remedy is sound and the rejected alternatives are fairly represented. The findings below are about the revision's reasoning, not about blocking the merge.

Findings

  1. should-fix: the open question may not be answerable as framed (§ Security considerations and § Open questions).

    • The design says the tolerance is safe because "the permitted intrinsics contain no sloppy-mode functions", and asks whether a test should pin that.
    • Its own evidence is that the descriptor (frozen null) cannot tell a Web IDL function from a V8 sloppy function.
    • A test can't tell them apart through Function.prototype.toString either, because native functions print [native code].
    • So the test as phrased can only check the intrinsics SES itself defines, not host-provided ones.
    • Say what the assertion would inspect, or restate the question as "SES-authored permitted functions are strict" (checkable) versus "host functions are trusted by provenance" (not checkable). Either way, resolve it or drop it.
    • [proposed-rule: a design's security argument should not rest on an invariant it has no means to test]
  2. should-fix: the rejected-alternatives section leaves out the cost of the chosen remedy (§ The remedy and § Considered and rejected).

    • § The remedy notes that each affected function adds one warning per property to the lockdown() report. That is two per codec constructor, plus those for URLSearchParams, %InitialURL%, and others.
    • The encapsulation alternative would add no warnings. Its rejection reasons (identity, one code path) never weigh this against the warning noise.
    • Add one sentence on why the warnings are acceptable, for example that they are warn level and only on old engines.
  3. comment-only: claims that cross into another design (§ The remedy, bullet 2).

    • "hardened-url-shim needs no revision" rests on URLSearchParams and %InitialURL% carrying the same shape.
    • That claim is based on the cited upstream report and was not checked here. A one-line pointer to the url-shim design's own permits would make it verifiable.
  4. comment-only: measurement coverage (§ The failure).

    • The measurements cover Chrome 120–140 with gaps, and the Android WebView is cited only through the PR.
    • "Older Chromium" is then used as the scope.
    • This is fine as written because the design states the gaps. Don't generalize beyond them.
  5. comment-only: status wording.

    • The header still reads Implemented while the fix is not yet on llm.
    • The preamble covers this, but the status line could say "fix pending merge to llm".
  6. comment-only: item 8 depends on an unlanded workflow change (Test plan additions, item 8).

    • It relies on #1582, a workflow change that has not landed.
    • A manual-only check is acceptable for a revision note, but this should not be the sole regression guard.

Item 7 of the test plan covers the tolerance's boundaries well: frozen null passes, while frozen non-null and writable null fail.

Out of scope: the eventual cauterizeProperty code review belongs to endojs/endo#3376.

Self-improvement: none.
model claude-opus-5-5 · harness claude · provider anthropic

skeptic — comment-only

Verdict: comment-only

Findings:

  1. should-fix: the Security considerations argument rests on an unverified premise. The section says the tolerance is safe because "the permitted intrinsics contain no sloppy-mode functions". It asserts this for "engine built-ins and for SES's own strict-mode code" without evidence. It also never covers functions that user code or shims plant before lockdown() runs, such as polyfills or replacement Array.isArray functions. Its own test plan item 7 plants a replacement Array.isArray, which shows that pre-lockdown replacement of intrinsics is a real path. A sloppy replacement would read as a frozen null between calls and would expose live arguments or caller during one, and the tolerance would let it through. The design records this only as Open question 1, yet it presents the tolerance as safe. Either restate the claim as conditional, or resolve the question before calling the remedy settled. [proposed-rule: a design that states a security property must either prove it or mark it explicitly unproven in the section that asserts it]

  2. should-fix: acceptance claims lack matching real-execution evidence. Test plan item 8 ("lockdown() completes on Chrome for Testing 137 … round-trips") is labeled "Browser smoke (manual)". It cites no run, log, or date. The "Measured on Chrome for Testing headless shells (2026-10-07)" data does not show lockdown() succeeding with endojs/endo#3376 applied. The design also says the remedy "reaches llm with the next upstream merge". Until it lands, the claim that "§ Design above is unchanged … hold[s] on every engine" is unverified on this tree. [rule: skeptic verified-claim evidence discipline]

  3. comment-only: the measurement gap is acknowledged but not bounded. The versions between 120 and 137 were not measured, and 138 onward is clean. The claim that the shape belongs to Web IDL functions in general, and so that URLSearchParams and %InitialURL% are covered, is inferred and not tested. The sentence "hardened-url-shim needs no revision" depends on that inference.

  4. comment-only: the status line and provenance are inconsistent. The Status line still reads "Implemented (Phases 1-2)". The design also describes a regression and a remedy that is not yet in this tree. The Dependencies row says the fix is "not yet on llm". A reader can't tell what the codebase actually does today. Consider adding a status note.

  5. comment-only: the wording around the rejected alternatives is thin. "Closed as superseded" for endo-but-for-bots#1349 and "closed" for endojs/endo#3371 give no reason beyond the remedy's coverage. The #1349 change is mentioned twice, with conflicting characterizations in the test-plan hunk ("found by" versus "defines both codecs"). The test-plan hunk should state that the XS evidence comes from probing the toolchain, not from the closed PR.

Overall, the premises are mostly sound. The one that needs attention is the sloppy-mode assumption behind the tolerance's safety (finding 1).

Self-improvement: none this engagement; the brief's verified-claim discipline applied directly to finding 2.
model claude-opus-5-5 · harness claude · provider anthropic

decomplector — comment-only

Verdict: comment-only

The ownership-map pre-pass doesn't apply. This is a docs-only revision with one layer boundary, lockdown() against host intrinsics. The map is implicit but coherent: SES owns the permit table and the decision to delete or tolerate a property, and the host owns the constructors. There are no inner types named for outer lifecycle concepts.

The repeated-mechanism pre-pass keys (WeakSet, constructor) refer to the earlier encapsulating-constructor approach. This revision already applies category (f) correctly. It rejects the per-constructor wrapper (#1349/#3377) and the per-constructor permit (endojs/endo-but-for-bots#3371) in favor of one general tolerance in cauterizeProperty. That is the smaller primitive: one rule for the shape instead of a mechanism per intrinsic. No wrapper or WeakSet mechanism remains in the design, so there is nothing further to demand removal of.

Findings

  1. Should-fix (taste-adjacent): the tolerance rests on an unchecked assumption. [proposed-rule: a design that adds a descriptor-shape tolerance must state which invariant makes the shape inert, and say where that invariant is enforced.] The § Security considerations section admits that the frozen-null descriptor alone does not distinguish a Web IDL function from a V8 sloppy-mode function. A sloppy-mode function's arguments reads as live during a call. Safety therefore depends on a property of the permitted set, "no sloppy-mode intrinsics". That property lives outside cauterizeProperty, which is the code that makes the decision. The mechanism's safety and its enforcement are in different places. The design already puts this as an open question. I recommend resolving it in the design before merge by answering yes and naming the test, rather than leaving it open. An open question on a PR that is otherwise "Implemented" also keeps the review-PR carve-out in play.

  2. Comment-only: shape-based tolerance instead of identity-based. The tolerance matches a descriptor shape (null, non-writable, non-configurable) across all functions. It does not match a known set of Web IDL functions. That is simpler and generalizes, which is why it is the right call here. The cost is that the check's meaning is now "whatever a future permitted intrinsic looks like". Finding 1 is the guard against that.

  3. Comment-only: status and provenance state. The doc says "Implemented (Phases 1-2)" while the revision describes a fix that is merged upstream but "not yet on llm". That separates the design's claim from the branch's state. Keep the Dependencies row and the "reaches llm with the next upstream merge" note, and drop them once the merge lands, so the doc doesn't carry stale time-dependent values.

  4. Comment-only: the manual smoke test (item 8) is a place-oriented fact. It records that no CI job covers the check. That is honest, and it is tracked by #1582. Nothing to change.

Out of scope: the correctness of the cauterizeProperty code in endojs/endo#3376 belongs to that PR's code panel.

Self-improvement: no changes to the brief. The pre-pass keys (WeakSet, constructor) did not match this diff, which only mentions them as rejected history. The seat could note that a repeated-mechanism pre-pass whose keys no longer appear in the diff should be treated as resolved.
model claude-opus-5-5 · harness claude · provider anthropic

ergonomist — comment-only

Verdict: comment-only

The change is documentation only. It adds a revision section to the design, and the surface it describes is the lockdown report and the permits behavior. I read it as the operator and maintainer who will hit that surface.

Findings:

  1. Should-fix: the warning text is not specified exactly, and the doc describes it two ways.

    • § The remedy says the reporter's warn emits Tolerating undeletable <path> === null.
    • § Consequences says "one warning per property" for each affected function.
    • On an affected engine a user will see at least two warnings per codec constructor, plus more for URLSearchParams and %InitialURL%, on every lockdown().
    • The doc says nothing on whether this noise is acceptable, whether it is deduplicated, or whether it sits behind a reporting level. A warning that users cannot act on trains them to ignore the lockdown report.
    • Fix: state the reporter level (warn vs. debug). Also say that operators have no action to take, or say how they can silence it.
    • [proposed-rule: a design that adds a per-lockdown warning must state who can act on it and how it is silenced]
  2. Should-fix: the doc leaves the first open question undecided, and the two fixes in the tolerance pair would not spell the same way.

    • The tolerance accepts only a frozen null and fails everything else. The unresolved question is whether to add a test asserting that no permitted intrinsic is a sloppy-mode function.
    • Without that test, the safety of the tolerance depends on an assumption that is only written in prose.
    • The doc also notes that the same descriptor can read as live arguments during a call. For the person who later adds a permitted intrinsic, nothing in the surface signals this constraint.
    • Recommendation: resolve the question in favor of the test, since it is cheap. Then the constraint shows up at the call site as a failing test instead of as a lookup in a design doc.
  3. Comment-only: the doc keeps two parallel records of the same history.

    • The preamble pointer, the revision section, the test-plan note on #1349, the "Considered and rejected" list and the Dependencies row each restate parts of the same story.
    • A reader entering at the top gets a pointer and must go to the bottom for the detail. That works, but the rows are easy to let drift apart.
    • This is taste only. The cross-links are good and I would keep them.
  4. Comment-only: test plan item 8 numbers a manual check alongside automated ones.

    • The label "(manual)" is clear.
    • A reader skimming the list will assume items 1 to 8 are all in CI. Consider separating the manual item under its own heading so it is not mistaken for a gate that runs.

Overall the section is easy to read. It follows the failure, remedy, security, tests, rejected and open-questions order, and each section names its own sources. I have no must-fix findings.

Self-improvement: for documentation-only revisions, check that a "warns" claim states its level, count and silencing path. I should add that to my default checklist.
model claude-opus-5-5 · harness claude · provider anthropic

copyeditor — comment-only

Verdict: comment-only. No must-fix findings; three should-fix items are below, plus two comment-only notes.

Findings (all in designs/hardened-text-codecs-shim.md, read end-to-end)

  1. should-fix: [rule: roles/jurors/copyeditor/AGENT.md] "Remedy" paragraph after "A regression on older Chromium followed…". The added line § Revision: restricted \caller` and `arguments` on older Chromium records the failure and the remedy.` is a fragment that stands on its own line as if it were a sentence. Rewrite as a full sentence, e.g. "The failure and the remedy are recorded in § Revision: …".

  2. should-fix: [rule: roles/jurors/copyeditor/AGENT.md] XS-test paragraph (around line 189). "Current Moddable xst defines both codecs (found by [#1349], closed unmerged on 2026-10-29, so no XS run of (1) has landed)". "Found by" makes PR #1349 the discoverer of the fact, but #1349 is a proposed encapsulation fix that was closed. The citation and the claim have been merged into one clause. Separate them, e.g. "Current Moddable xst defines both codecs. The PR that would have wrapped them ([#1349]) closed unmerged on 2026-10-29, so no XS run of (1) has landed."

  3. should-fix: [rule: roles/jurors/copyeditor/AGENT.md] "Failure" section, first subsection. "The FunctionInstance permits do not name them, they cannot be deleted, and so lockdown() threw as soon as…" is a comma splice joining three clauses. Split it: "The FunctionInstance permits do not name them. They cannot be deleted, so lockdown() threw as soon as…"

  4. should-fix: [rule: roles/jurors/copyeditor/AGENT.md] Test plan item 7 uses "pills" ("it plants pills on a replacement Array.isArray", "a frozen null pill"). The term is not introduced in this diff. If it is defined elsewhere in the design, cite that section. If not, introduce it at first use or use plain wording such as "planted properties".

  5. comment-only: The section heading "Revision: restricted caller and arguments…" names a different remedy from the body, which is "tolerate a frozen null". "Restricted" reads as a restriction on the properties. Consider "Revision: tolerate frozen null caller and arguments on older Chromium" so the heading matches the body's verb.

  6. comment-only: The § symbol is used in the new lines (the revision line, the "§ Design above" bullet, the Dependencies row). It is not in the typist-friendly code-point set and the doc already uses it throughout, so I am not flagging it. No arrows, ellipses, curly quotes, or comparison glyphs appear in the added lines. Em-dash discipline is clean in the diff.

Self-improvement: no reusable pattern beyond the existing seat guidance; the comma-splice and attribution checks were covered by the standing norms.
model claude-opus-5-5 · harness claude · provider anthropic

pedant — comment-only

Verdict: comment-only

Findings

  1. Should-fix (heading hierarchy). ### Open questions (around the ## Revision section) is nested under a Revision H2, so it reads as a sub-part of the revision rather than a top-level section. The document's own top-level sections are H2, so promote it to ## Open questions (or move it out of the Revision block). A top-level heading also matters for the design-PR carve-out, which keys on an ## Open questions section. [rule: roles/pedant (Chicago heading discipline, list and heading hierarchy)]

  2. Comment-only (citation consistency). endojs/endo#3376 is linked on first mention in the Revision preamble, but later mentions in "The remedy" and "Considered and rejected" are bare text. Repeat mentions may stay plain, so this is a taste call. If the document links every first mention, it should do so consistently. [rule: roles/pedant (Chicago citation form, link text discipline)]

  3. Comment-only (symbol form). The added text uses § about six times ("§ Revision", "§ Design above", "§ Test plan item 6"). The garden's own docs use it, so I'm not calling it a violation. The typist rule's "similar symbols" clause might cover it, and I did not read skills/typist-friendly-code-points/SKILL.md to confirm. Also, at the start of a sentence (the bullet "§ Design above is unchanged") it reads awkwardly; "Section" or a rewording would be cleaner there. [rule: skills/typist-friendly-code-points/SKILL.md, unverified against the skill text]

Checked and clean: no em-dashes, ellipses, arrows, curly quotes, or ≤/≥/≠ in the added lines. Sentence-per-line holds in the new prose and bullets. Subheading case is consistently sentence case, and the colon-after-heading form is consistent. Test-plan and bullet items use parallel construction with terminal periods. Numbers and dates are consistent with the Updated field (2026-10-29). Serial comma is absent throughout, which matches the existing document, so I did not flag it. No diagrams or banner comments were added.
model claude-opus-5-5 · harness claude · provider anthropic

novice — comment-only

Verdict: comment-only

The revision follows a clear arc: failure, remedy, consequences, security, tests, rejected options, open question. A new reader can follow most of it. A few steps assume background the document does not give.

Findings

  1. should-fix: § The failure, first sentence. "Collects the intrinsics, then deletes every property the permits do not name" assumes a mental model of the permits table and of what lockdown does. A new reader has not built that model. The existing § Design may cover it, but the revision does not point back to it. Add a one-line pointer, such as "see § Design for the permits table". [rule: skills/panel-review/SKILL.md, top-down clarity]

  2. should-fix: § The remedy, "undeletable prototype that can be set to undefined". The text says this "parallels" an existing tolerance, but never explains it. A reader who does not know that tolerance cannot use the comparison. Either explain it in one sentence or drop the comparison.

  3. should-fix: § Security considerations. The argument moves from "inert" to "safe as long as no sloppy-mode functions" in compressed steps. The reader is never told why a sloppy-mode function's arguments is dangerous. The reason is that it reads as the live arguments object during a call, which is the leak. The text states this in a parenthetical, but it comes after the conclusion. Reorder it to premise, then inference, then conclusion. The "Node 22.23" check is stated without any step-by-step walk-through.

  4. comment-only: Header summary, "§ Revision: restricted caller and arguments on older Chromium records the failure and the remedy." This sentence stands alone as a fragment-like line. Merge it into the preceding sentence.

  5. comment-only: Dense dates and numbers. The Chromium version list (120, 126, ..., 140) is hard to hold in mind. A single summary line would help, for example "120 through 137 affected, 138 and later not". Also, the header says #1349 is "found by" and "closed unmerged", while § Considered and rejected says it was "closed as superseded". The story is consistent but spread across three places, which makes it hard to follow.

  6. comment-only: Test plan item 8 says "Browser Tests do not run on frozen-base PRs" without saying what a frozen-base PR is. This is a project-specific term. Link it or gloss it.

  7. comment-only: The Open questions section is a good, clearly stated yes/no question. It would help to say what the default is if the maintainer does not answer.

Self-improvement: no changes to the seat brief proposed; the revision-section pattern (failure, remedy, security) is easy to read top-down.
model claude-opus-5-5 · harness claude · provider anthropic

orthographer — approve

Verdict: approve

Findings:

  • none — the deterministic divergence grep found no British spellings introduced by this change. [rule: skills/american-english-normalization/SKILL.md]
    model claude-opus-5-5 · harness claude · provider anthropic
thesaurus — approve

Verdict: approve

Findings:

  • none — the deterministic cliché grep found no Botese phrases introduced by this change. [rule: skills/botese-normalization/SKILL.md]
    model claude-opus-5-5 · harness claude · provider anthropic

model claude-opus-5-5 · harness claude · provider anthropic · host endolin-garden-ece02cb4 · garden f943afda

@kriscendobot
kriscendobot marked this pull request as ready for review October 29, 2026 15:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant