oficina / estilo Public

Review record: estilo.json; model claude-fable-5-1; session 44684b65-6ef2-47f7-949a-6307c83e19a0

REQUEST CHANGES. Reviewed by Claude Fable 5.1, read only. The primitive is the right shape and the right home, but the spec misdescribes the second consumer and leaves three integration mechanics undefined that would change the Docs behaviour it promises to preserve.

Note on inputs: both fixtures/editor-entry.js files live at each worktree root, not under internal/…. I reviewed those.

What is correct

• Escaped template strings, textContent only, no copy in Estilo, closed state set with silent unknowns, app-owned sticky failure. All match how both apps already behave.
• estilo-* attributes satisfy internal/lint/prefix_test.go. Note AGENTS.md's Vocabulary example still shows rst-inspector for an estilo component, which contradicts the lint. Not this spec's fault, but worth a one-line fix there.
• Docs-first, Slides-second matches the one rule. The dependency ruling is no longer a blocker since go.mod pins rastrillo v0.22.0.

Issues

1. The Slides consumer description is wrong. The spec says Slides shows saved, saving, retrying and failed. The actual states in editor-entry.js are saved, saving, offline, error. Slides has no failed string. Its sticky dropped-batch case shows the offline text, and error is a slide-load failure, not a save state. The spec must state the mapping explicitly. Recommendation: offline maps to Retrying with its existing "Offline, retrying" string, sticky failure stays on that same string for this migration, and the load error stays an app-owned element outside the shared component. That keeps the closed set honest and introduces no new copy. If the adopter wants a real Slides "failed" message, that is new product copy and needs human review, so keep it out of the overnight run.

1. "Visually silent" plus floating placement will break announcements unless specified. Docs today uses display: none on the container and toggles it on. The spec moves to updating text in an always-present live region, which is better, but if the floating variant also hides via display: none when text is empty, the region leaves the accessibility tree and the first failure message is added in the same tick the region reappears. Many screen readers drop that. Specify that the silent state removes padding, background and shadow while keeping the element rendered, for example an :empty rule, and add a DOM test that the region exists in the tree before text changes.

1. The JavaScript delivery path is undefined. Both apps build their editor bundle from fixtures/, but the shared function lives in a Go module. "Served through the existing embedded asset route" says where the file is served, not how the bundle calls it. State one mechanism: an ES module served at an asset URL and imported by URL from the app bundle, or a tiny function the app bundle receives via the served module's global. Also state where the state texts live client-side. Slides already uses data-text-* attributes on the root, which is the right pattern and keeps copy out of the bundle.

1. Print behaviour is lost on Docs. The Docs print stylesheet hides [docs-save-status] at line 510 by app selector. Integration says to remove app-local status styling, which would print the floating pill. Either the floating variant ships its own @media print hide, or the spec names the print rule as one of the retained app selectors.

1. Inline variant needs reserved width. Slides sets min-width: 8ch so the header bar does not jitter between "Saved" and "Saving…". Add that requirement to the inline placement or the Slides header will shift on every keystroke.

1. Failed tone for Docs is under-specified. Docs' failed state uses the negative tone tokens plus weight. The spec says "words plus weight" only. Using existing --rst-tone-negative-* tokens is not a theme change and is the one state a person must not miss. State that the floating variant keeps the negative tone for failed and that colour is never the sole carrier. Otherwise the Docs migration is a visible regression in urgency, which contradicts "do not change failure" intent.

1. Pin the Slides revision. Docs is pinned at 499d200. Slides is described as "active slice-2-editor" while the tree reviewed is the slice-1 worktree, whose AGENTS.md still says no browser editor exists. Name the commit the adoption branches from, as the spec already does for Docs.

Verification additions

• A test that the Slides sticky-failure path never shows "Saved" after a dropped batch, mirroring the Docs test already required.
• A test that a state with no text yields an empty region that is still present in the DOM and accessibility tree.
• 320px check on the floating variant with Docs' longest failed string in the widest locale available.

Scope and copy

No new localized strings are needed if issue 1 is resolved as recommended. All four Slides strings and both Docs strings already exist in their catalogs and pass through unchanged. Any deviation from that is a copy change and must go to human review before merge.

Review record: estilo-r2.json; model claude-fable-5-1; session 44684b65-6ef2-47f7-949a-6307c83e19a0

APPROVE, reviewed by Claude Fable 5.1, read only. All seven prior findings are resolved in the spec as written. Two minor items to carry into implementation, neither blocking the spec.

Resolved

• Slides mapping is now exact and matches editor-entry.js: saved and saving direct, offline to retrying with its existing text, sticky dropped batch stays on offline text, load error stays app-owned. No new copy is introduced.
• Silent state keeps the live region rendered and in the accessibility tree. Floating silence strips padding, background and shadow only.
• Delivery is a same-origin ES module by URL, external to bundling, with texts on the element as escaped data attributes and a stated module-load failure path. That is consistent with the "no CDN, no bundler" rule.
• Print hiding, inline 8ch width, Docs' negative tone tokens for failed, and pinned Slides revision are all present.

Carry into implementation

1. Floating variant must keep pointer-events: none. Docs' current fixed notice has it at show.html:1274 so the pill never intercepts clicks on the page beneath. The spec says "non-interactive" but does not name this. Add it to the floating CSS or the migration regresses click-through on Docs.

1. Review record location. The spec cites a /tmp JSON file as review 1's record. Estilo's AGENTS.md requires the reviewing model to be recorded in the branch discussion, since /tmp does not survive. Post both reviews to the branch discussion before merge.

Not verified

I could not confirm that a863fb6 exists in the Slides repo or that 499d200 matches the Docs tree I inspected. No shell was available. The implementation reviewer should check both pins against the actual adoption branches.

Library code532746a APPROVED by canonical claude-fable-5-1; full verdict below. Required decisions-table row recordedc5f695a; new README/package documentation prepared via writing pipeline, awaiting user review at the supplied review servers. No docs text applied yet. Actual Docs first adoption f9c61de passes full local gate and browser fixture module-failure fallback; Slides second adoption94ac454 under separate Fable review. Consumer release pins must update to reachable merged/tagged library release before merge. No local replaces or public visibility changes.

APPROVE for the library branch at 532746a, reviewed by Claude Fable 5.1, read only. The component matches the approved revision-2 spec, and the Docs adoption uses it correctly. Three small items to fix before squash-merge, and one consumer-side trap the merge itself creates.

What checks out

• Markup, CSS and module match the spec: closed state set with idle fallback, textContent only, persistent empty live region, floating :not(:empty) chrome with pointer-events: none, negative tone plus weight for failed, inline 8ch, component-owned print hide. Escaping of text, label and placement is tested.
• Docs preserves its behaviour. The save queue's ok maps to idle, so routine saves stay silent. Sticky dropped-batch classification stays in app code. Old [docs-save-status] rules and DOM are gone.
• Module-load failure is handled correctly: mountEditor awaits the import before anything else, so a failed import throws into the existing catch, marks data-editor-failed, and leaves the read view. No editor runs without a working indicator.
• Prefix lint and byte-identical theme test are unaffected. Node-dependent Go test is acceptable since Estilo CI already passed with it.

Fix before merge

1. Record the module path change in AGENTS.md's decisions table. Renaming to amadan.net/oficina/estilo.git is a settled decision with rejected alternatives, and it is the kind of thing the next session will "correct" back. Add a row naming the reason, the rejected options, and the GOPRIVATE requirement. Also update README, which still says there is no code and whose first-primitives table lacks save status.

1. Add a package doc to savestatus. AGENTS.md requires the script-required line per primitive. State it there: without script the initial state text renders and nothing changes. Document Data, the fixed template name, and that consumers own state semantics.

1. Stale reference. Docs' show.html:1350 still names [docs-save-status] in a comment. Trivial, but the Docs test forbids that string in output, so keep the source consistent.

Trap the squash-merge creates

Docs pins the pseudo-version of branch commit 532746a. After squash-merge and branch deletion, that commit becomes unreachable and a fresh go mod download can fail depending on the Git server's fetch policy. Tag the merged main commit and have Docs re-pin to that tag before Docs merges. Slides should pin the same tag.

Assessment of the private-module amendment

The .git VCS-qualifier path is the documented Go route for a private repository without go-import metadata, and it correctly avoids publishing the repo, a replace directive, or a pseudo-version of an unreleased dependency. It is acceptable. Three conditions attach to it:

• Every builder needs GOPRIVATE and authenticated Git credentials: Docs CI, Slides CI, and whatever runs make release for CARLOS. Docs' gate sets GOPRIVATE, but runner credentials are unverified here.
• Consumers' go.sum entries are the only integrity record, since GOPRIVATE disables the checksum database. That is fine but worth stating in the decision row.
• The path is permanent. A later move to a hub that serves authenticated go-import metadata would be a breaking rename for every consumer.

Not validated yet

The inline variant has no consumer. Slides is the second adoption and the one that validates the interface. Until it lands, this remains an in-progress primitive per AGENTS.md, and this approval covers the library branch only, not deployment of either app.

Fresh remote still has startad0edf4 and feat/shared-save-statusc5f695a, no main/remoteHEAD. Source inspection of supported Amadan merge API confirms unborn default requires initial push; no bootstrap performed. Earlier Fable approved save-status delta against start, so complete initial library review is now explicitly queued at exact clean c5f695a after reset01:51. Prompt /tmp/oficina-estilo-full-release-review/prompt.txt includes inherited theme/pin/drift checks, actual Docs65019db and Slides6d4c90a adopters and supported initial-main sequence proposal. CI atc5f695a passed. README/package-doc copy reviews remain pending; review is not a waiver. Final reachable main/tagged release and consumer repins remain required. Root will not blindly initialize main with unreviewed inherited content.

APPROVE the complete library code and design at c5f695a, reviewed by Claude Fable 5.1 (claude-fable-5-1), read only. Two release prerequisites remain outside code: the pending human copy review, and correcting stale binding statements in AGENTS.md. The initialization sequence proposed is the right one, with the conditions below.

Full tree verified, not only the diff

• Theme gate is real. theme_test.go compares estilo.css byte-for-byte against ui.ThemeCSS("day") from the pinned rastrillo v0.22.0, stripping only the leading header comment, and refuses a token-set-only fallback. Byte-identical day holds.
• Prefix gate is real but narrow. prefix_test.go scans every .css file for .rst- or [rst- selectors. It does not scan templates or JS, and it does not require estilo-. status.css and status.html use only estilo-* attributes and --rst-* tokens, so the rule holds by inspection as well as by test.
• Framework markup and pin. Estilo ships no rastrillo markup of its own. Both consumers' app-owned rastrillo markup is theirs and was not reopened.
• CI is one definition. The four ci.d steps each exec a Makefile target. The Go test shells out to node, which the runner has since CI passed.
• Save status is unchanged since 532746a and matches the approved spec. AGENTS.md row 7 records the private module path with rejected alternatives, which closes my earlier finding.

Second adoption validates the interface

Slides uses the inline placement with its accessible label, maps offline and the sticky dropped batch to retrying with the existing "Offline, retrying" text, keeps slide-load errors in a separate app-owned live region, and awaits the module before any editing. A module-load failure writes the existing load-error text and throws, so no editor runs. Docs' floating placement, silent routine saves, and sticky failure are intact, and the stale comment I flagged is gone. Both consumers serve hash-addressed assets from their own routes and both gates set GOPRIVATE. The primitive now has two real consumers with the old duplicates removed, which is AGENTS.md's bar for extraction complete.

One observation, not a defect: Slides reuses its load-error string for a module-load failure. That is existing copy, so it needs no review, but it is imprecise and a later copy pass could give it its own key.

Findings that block a tagged release

1. AGENTS.md is stale in a binding way. Row 3 still says "No Go code until the next rastrillo release", and the Dependency section still says v0.19.0 lacks the needed functions and "we wait". The pin is v0.22.0 and the wait is over. A future session will read that as a live prohibition. Mark row 3 resolved and update the Dependency section to record the pin and date. This is a factual correction to an agent-facing file, not user copy.

1. README is factually wrong. It says there is no code yet, cites v0.19.0, and lists consumers without Slides. This is under human copy review with no approval. Do not tag a release consumers pin while the repo's front page contradicts its contents. Merging code to main is fine before that lands.

1. Package doc for savestatus does not exist. AGENTS.md requires the script-required line per primitive. The proposed prose is under copy review and is not shipped. Same condition as item 2: merge is fine, tag waits.

1. The two-pin drift check is Estilo-internal only. AGENTS.md promised a tokens-drift check in the first commit with code. The theme test is that check at Estilo's boundary. Nothing checks a consumer whose own rastrillo pin resolves higher than Estilo's. Today neither consumer imports theme.CSS(), so there is no live risk. Record the gap and add a consumer-side check when the first app adopts Estilo's theme.

1. Minor. implementation/save-status.md still cites a /tmp path for review 1 although line 3 says verdicts live in discussion 1. Drop the /tmp reference.

Initializing main

The merge API refuses an unborn default with "push to it first", so a plain push is the supported route. The proposed sequence is correct. Concretely:

1. Confirm the baseline. Every tracked file at ad0edf4 is in the current tree unchanged except go.mod line 1, AGENTS.md row 7, .gitignore draft entries, and the two new directories. I have read all of it. This review is the baseline's review record. Confirm ad0edf4 has a green CI run, or accept that the same code passed at 5f712c7 and c5f695a and say so in the discussion.
2. Push ad0edf4 as main from the agent account, and record in discussion 1 that main was initialized from the reviewed baseline with this review as its record.
3. Squash-merge feat/shared-save-status through the gated API with the expected SHA c5f695a, after items 1 and 5 above. Items 2 and 3 gate the tag, not the merge.
4. Verify merged-main CI is green.
5. Do not delete the feature branch until both consumers repin. Their pseudo-version points at 532746a, which becomes unreachable if the branch ref goes.
6. Consumers repin to the merged-main pseudo-version immediately, then to the tag once copy lands. Tag only after items 1 through 3 are done.

Safer alternatives considered and rejected: pushing c5f695a directly as main bypasses the squash gate for the feature content. An orphan root commit gives the feature branch no merge base. Both are worse than the proposed route.

This review does not approve user-facing copy and does not mark any deployment complete.

Initialized previously unborn main from reviewed baseline ad0edf4 through the supported initial push sequence approved by Fable in the full c5 review, now recorded above. The same inherited code passed full gates at 5f712c7/c5f695a; current complete descendant9fbeff9 has terminal local gate and exact hubCI success. Ancestry ad0→c5→672→dec→9f verified with git. All four actual Fable verdicts recorded in discussions1/2/3 before gated squash. Feature refs retained for existing consumer pseudo-versions; no tag/deployment from initialization.