fix(parser,ast): resolve rStyle-vanished object text + body-object schema invariants - #653
Merged
Conversation
Resolved w:rStyle -> character-style w:vanish IDs, captured alongside a body object so capture and the edit rewrite path can share one source of truth without needing styles.xml at rewrite time (#650). Additive JSONB field, no migration needed: absent and [] are interchangeable, so an object row captured before this change loads/parses unchanged. openapi.yaml updated to match. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
parseCharacterStyles's isVanish and parseVanish's paragraph-style
check used bare `'w:vanish' in rPr` presence checks. w:vanish is an
OOXML ST_OnOff toggle (ECMA-376 §17.3.2.45): an explicit w:val in
{0,false,off} switches it OFF even though the element is present.
Presence-only checks incorrectly marked such styles vanish, which
would suppress otherwise-visible spec text once #650's Part A relies
on the toggle resolving correctly at the style's own definition.
Switch both checks to isOnOffEnabled (ported from document.ts's
runIsVanish/isOnOffEnabled pattern) and add regression tests pinning
w:vanish w:val="0" as non-vanish for both character and paragraph
styles.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…RunVanish Extends hasRunVanish (body-objects.ts) with an optional vanishCharStyleIds param so a run hidden via w:rPr>w:rStyle referencing a vanish character style is now caught by the same predicate capture and rewrite both share (#650) — a straight OR port of document.ts's runIsVanish, defaulting to an empty set so every existing single-argument call site keeps compiling and behaving byte-for-byte unchanged. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…d ObjectMeta Threads the resolved vanish character-style ID set from the StyleMap the builders already receive, down through the collectText chain, and persists it via toObjectMeta. Stored as a SORTED array rather than a Set so the JSONB column and fixture snapshots serialize deterministically, and omitted entirely when empty so existing rows round-trip byte-identical. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Two lint fixes on top of the prior vanishCharStyleIds threading commit: sonarjs/no-alphabetical-sort wants an explicit localeCompare comparator on the persisted-array sort, and the run-level vanish predicate (isOnOffEnabled/hasRunVanish/resolveRunVanish) is split out of body-objects.ts into its own body-object-vanish.ts — re-exported from its established public surface — to bring body-objects.ts back under the repo's enforced 400-line max-lines budget. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…ath (#650) replaceAnchoredParagraphText now accepts an optional vanishCharStyleIds set (default empty), threaded through rewriteFirstText/replaceParagraphContent/ rebuildMatchedSdt/rebuildNode so an edit skips exactly the runs capture skipped — whether hidden by a direct w:rPr>w:vanish or by a w:rStyle reference. Without this, an edit could land in an rStyle-hidden run and blank the visible run beside it, the same capture/rewrite drift #641 fixed for direct w:vanish. rewriteObjectTextBlob builds the set from the object row's own persisted ObjectMeta.vanishCharStyleIds (styles.xml is unavailable at rewrite time) and passes it through; a pre-#650 backfill row with the field entirely absent behaves identically to today (empty set). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…on (#650) Task 6/10: byte-identical round-trip proof for the #650 vanishCharStyleIds capture path, plus full 666-fixture corpus revalidation (0 changed). Writing the "with vanishCharStyleIds present" round-trip test surfaced a real privacy regression: generateDocx never emitted a `w:styles` character style definition for a captured object's referenced vanish character style, so re-parsing a regenerated document lost `vanishCharStyleIds` resolution and a run correctly hidden at first capture surfaced as plain visible text on the second parse. Fixed in a new generator/object-vanish-styles.ts, using the vanishCharStyleIds already persisted per object (#650 tasks 1-5) to reconstruct a minimal enabled-vanish character style stub per referenced id, wired into both generateDocx and generateManual. Also inspected merge/object-fingerprint.ts per the task brief: it is text-blind and structure-only (tag shapes, never rPr/rStyle/vanish), so it has no latent dependency on vanishCharStyleIds — no change needed there. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
ObjectMetaSchema gains a cross-field check (Zod v4 .check()) that rejects a textBox kind carrying rows and/or columns — table-grid dimensions a textBox has no grid for. The rule names only kind, rows, and columns; it never scans for or reacts to other fields, so additive fields like vanishCharStyleIds stay structurally untouched. table stays fully unconstrained. Part B of #650. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
'object' nodes require meta.object and no other node type may carry one — presence-only, scoped exactly to that coupling so classify.ts still owns deriving editability alongside it (#650 Part B). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…Schema (#650) Repo-wide grep found zero consumers spreading either schema's `.shape` anywhere in src/ — neither is ever an MCP tool inputSchema (or nested inside one); both are consumed exclusively via `.parse()`/`.safeParse()` on the full schema instance (db/queries/object-meta.ts, revision- snapshot.ts, revision-diff.ts, history-diff.ts). Non-applicability, not enforcement, mirroring contract-schema-sharing-map.ts's Item 5 gate but for these two schemas. Adds a CI-enforced regression guard instead of leaving the finding as a point-in-time PR-body note: a repo-wide static scan asserting no file spreads ObjectMetaSchema.shape/SpecNodeSchema.shape, plus a direct check that src/mcp/tools.ts never references either schema. Verified RED by temporarily injecting an offending spread and confirming the test catches it before reverting. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughChangesVanish style preservation
Sequence Diagram(s)sequenceDiagram
participant DOCX
participant Parser
participant Database
participant ObjectTextEdit
participant Generator
DOCX->>Parser: capture object text and vanish style references
Parser->>Database: persist ObjectMeta with vanishCharStyleIds
ObjectTextEdit->>Database: load object metadata and blob
ObjectTextEdit->>Parser: rewrite visible anchored text
Parser-->>ObjectTextEdit: preserve hidden runs
Generator->>Parser: namespace vanish style references
Parser-->>Generator: return rewritten trees and style IDs
Generator-->>DOCX: emit regenerated document.xml and styles.xml
Possibly related issues
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Comment |
…650) generateManual unioned every SpecTree's captured vanishCharStyleIds into ONE shared w:styles character-style block. Two different source documents combined into one manual that happen to define their own character style under the same raw id — one genuinely vanish, the other used only for unrelated formatting on visible text — collided: the vanish stub for the shared id silently overwrote the other tree's definition, which would hide that tree's previously-visible text on reopen. object-vanish-namespace.ts gives every source tree its own private namespace for the vanish ids its own captured objects reference (and rewrites the matching w:rStyle references in that tree's own object blobs), so two documents can never let one tree's vanish stub capture another tree's unrelated same-named style. A no-op for the common case of a single-section manual or no vanish ids anywhere. Also strengthens two review-flagged test gaps: - body-objects.test.ts's "capture and rewrite share one predicate" test called hasRunVanish twice with identical literal arguments, which can never fail for a deterministic pure function. Replaced with an end-to-end test that derives vanishCharStyleIds from a real extractBodyObjects capture and feeds that exact value into replaceAnchoredParagraphText on the same blob, using a hidden-run-first paragraph ordering where a predicate drift is actually observable. - body-object-attach.test.ts never exercised toObjectMeta's vanishCharStyleIds sort with 2+ ids, so a regression dropping the sort (Set-insertion order rather than deterministic) would go undetected by the fixture-snapshot corpus-determinism invariant it exists to serve. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
The "hasRunVanish is a pure, deterministic predicate" test called hasRunVanish once and compared its return value against a second call using the exact same node/Set references — a tautology that can never fail for a deterministic function, so it never actually exercised the implementation. Rewritten to call the predicate on two independently constructed (but structurally identical) node instances, asserting the concrete expected verdict on each call and confirming the predicate does not mutate its input. Mutation-verified against a deliberately broken hasRunVanish (forced to return false) to confirm the test now fails as expected. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
… ADR-092's scope-limit note ADR-092 documented hasRunVanish's w:rStyle character-style scope limit as an accepted, measured gap. #650's fix (already implemented on this branch) closes it by persisting the resolved vanishCharStyleIds set on the captured object; this ADR was the one piece of the issue's required work not yet recorded. ADR-092's Context/Consequences are amended to point at ADR-094 rather than describe the now-closed limit as current behaviour. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
The repository owner forbade new or amended ADR files for this sprint. The design rationale they carried now lives in the PR body under "Design decisions" and in code comments / test names on this branch. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…650) The pre-existing coverage inserted object rows with raw SQL, so it proved the JSONB column stores the field and rewriteObjectTextBlob reads it back, but never exercised the CANONICAL save path. That is exactly the shape of the pageSize regression in PR #536: an additive SpecTree field that passed every in-memory test while the save/load mappers silently dropped it. These cases drive insertTree (the same write path DOCX import uses) and assert twice — once against the raw `object_data->'vanishCharStyleIds'` JSONB column via SQL, so the value is proven on disk rather than in a cache, and once through the real read mappers (getSpecTree and fetchSubtreeNode). A third case pins the backfill invariant: an object with no vanish styles persists and reads back with the key ABSENT, never fabricated as an empty array. Mutation-verified in both directions rather than assumed load-bearing: dropping the field from ObjectMetaSchema fails the read-path case, and stripping it in flattenDfs's objectData mapping fails both the column and the round-trip case. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
All four are over-suppression or collision paths this branch itself introduced — visible text being hidden, which is the failure mode that produces no error and merely looks like correct privacy behaviour. 1. hasRunVanish resolved the two vanish signals with a plain OR, so a run carrying BOTH an rStyle vanish and a direct <w:vanish w:val="0"/> stayed hidden. ECMA-376 17.7.3 makes direct formatting definitive for a toggle property: writing w:val="0" is the author switching the style's vanish back OFF. Now resolved as a tri-state. document.ts's paragraph-tier runIsVanish keeps the older behaviour deliberately — this fix is scoped to the object tier's own new rStyle path rather than silently changing long-shipped paragraph classification in a bugfix PR. 2. Capture persisted the document-wide vanish-style set on EVERY object, O(objects x styles) into JSONB. It also made the generator mint a stub for ids no blob references, widening the collision surface below. Now narrowed to the ids the object's own blob actually references, which costs no correctness: rewrite only ever resolves runs inside that blob. 3. generateDocx and single-tree generateManual emitted RAW style ids, so a source document whose vanish style is named after one dolanmiu/docx also emits (Hyperlink, Strong, ...) produced a duplicate w:styleId whose minted w:vanish could attach to the document's own hyperlinks. Vanish ids are now namespaced on every generation path; the style's display name is de-namespaced so Word still shows the original name. 4. The namespace suffix was assumed unforgeable. Nothing reserves it, so minted ids are now collision-checked against every id that survives renaming, and the allocator strips before it mints — without that, repeated generate/re-parse cycles stacked a fresh suffix each time and broke the byte-identical blob round-trip. Two tests that encoded the old behaviour are flipped as explicit, commented oracle changes rather than quietly edited. Corpus revalidated: 0/666. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Contributor
Author
|
This was written agentically; verify its assertions and edit accordingly: Adversarial cross-review — Codex
|
thewrz
marked this pull request as ready for review
August 5, 2026 07:17
Contributor
Author
|
@coderabbitai full review |
✅ Action performedFull review finished. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (5)
src/generator/object-vanish-styles.test.ts (1)
71-82: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a case for a namespaced id to cover
stripVanishNamespace.Both assertions use ids with no
NAMESPACE_SEPARATOR, sonameandidare identical either way. If thestripVanishNamespacecall were removed fromvanishCharacterStyleOptions, this suite would still pass. Add one id carrying the minted suffix, which is the case the de-namespacing exists to serve.💚 Proposed additional test
it('builds one enabled-vanish character style option per id, name mirroring id', () => { expect(vanishCharacterStyleOptions(['HiddenChar', 'Other'])).toEqual([ { id: 'HiddenChar', name: 'HiddenChar', run: { vanish: true } }, { id: 'Other', name: 'Other', run: { vanish: true } }, ]); }); + + // The namespaced id is what generation actually emits. `id` must stay + // namespaced (the parser's only correlation key) while `name` shows the + // human the ORIGINAL style name their source document used. + it('de-namespaces the display name while keeping the namespaced id verbatim', () => { + expect(vanishCharacterStyleOptions(['HiddenChar#specr-vanish-t1'])).toEqual([ + { id: 'HiddenChar#specr-vanish-t1', name: 'HiddenChar', run: { vanish: true } }, + ]); + }); });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/generator/object-vanish-styles.test.ts` around lines 71 - 82, Add a test case in vanishCharacterStyleOptions covering an id containing NAMESPACE_SEPARATOR and its minted suffix, and assert the generated option keeps the full id while setting name to the de-namespaced value from stripVanishNamespace. Preserve the existing empty-input and ordinary-id assertions.src/generator/object-vanish-namespace.test.ts (1)
203-226: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd coverage for a numeric
w:valattribute.
rStyleValKeyinsrc/generator/object-vanish-namespace.tslines 145-149 exists becausefast-xml-parsercan coerce a numeric-looking@_w:valto a JS number. No test drives that branch. If the coercion handling regressed, a numeric style id would not be renamed in the blob while the style block still defines only the namespaced id. The run would then lose its vanish style and previously hidden text would render visible.💚 Proposed additional test
+ // rStyleValKey's numeric branch: fast-xml-parser may hand back `@_w:val` + // as a JS number for a numeric-looking style id. The rename must still + // match, or the blob keeps the raw id while styles.xml defines only the + // namespaced one — and the hidden run renders VISIBLE. + it('renames a w:rStyle whose `@_w`:val was coerced to a number', () => { + const numericRStyle = { 'w:rStyle': [], ':@': { '`@_w`:val': 42 } } as unknown as ObjectBlobNode; + const obj = node({ + type: 'object', + meta: { + object: { + kind: 'table', + floating: false, + generation: 'drawingml', + blob: [{ 'w:r': [{ 'w:rPr': [numericRStyle] }] }], + vanishCharStyleIds: ['42'], + }, + }, + }); + const result = namespaceVanishTrees([tree('t0', [obj])]); + expect(firstVanishIds(result)).toEqual(['42#specr-vanish-t0']); + expect(rStyleValOf(firstObjectMeta(result)?.blob ?? [])).toBe('42#specr-vanish-t0'); + });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/generator/object-vanish-namespace.test.ts` around lines 203 - 226, Add a test alongside the existing object namespace tests that constructs a blob with a numeric-looking w:rStyle `@w`:val and verifies namespaceVanishTrees renames it to the expected namespaced style id. Ensure the assertion exercises rStyleValKey’s numeric coercion branch and confirms the corresponding vanish style remains applied.src/generator/object-vanish-namespace.ts (1)
223-232: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReplace the
blobtype assertion with a spread copy.Line 229 uses
as ObjectMeta['blob']to widenreadonly ObjectBlobNode[]to the mutable array shape.src/db/queries/object-text-edit.tsline 88 handles the identical readonly-to-mutable boundary with a spread copy instead of an assertion. The spread copy is safe here: line 226 already returned whennewBlobis the original reference, so the remaining value is always a fresh array from.map.As per coding guidelines: "Do not use
any,as unknown as, cross-boundary type assertions, or non-null assertions (!) outside tests."♻️ Proposed fix
return { ...object, - blob: newBlob as ObjectMeta['blob'], + blob: [...newBlob], ...(newIds !== undefined ? { vanishCharStyleIds: [...newIds] } : {}), };🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/generator/object-vanish-namespace.ts` around lines 223 - 232, In rewriteObjectMeta, replace the blob type assertion on the returned object with a spread copy of newBlob. Preserve the existing early return and ensure the copied array is used to satisfy the mutable ObjectMeta blob shape without a type assertion.Source: Coding guidelines
src/generator/index.ts (1)
292-298: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse
scopedTreefor header/footer rendering and page size.This matches
generateManualand keeps both paths consistent if namespacing later updates fields beyondparts. Header/footer has noObjectMetaor objectblobfield, so novanishCharStyleIdscoverage change is needed.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/generator/index.ts` around lines 292 - 298, Update the document setup in the generator flow to pass scopedTree, rather than tree, to renderOptionalHeaderFooter and resolvePageSize. Keep the existing namespaceVanishTrees and buildSectionChildren usage unchanged, and do not alter vanishCharStyleIds coverage.src/generator/object-vanish-styles.ts (1)
44-48: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winUse locale-independent ordering for deterministic output.
Replace
localeComparewithsort()or an explicit fixed locale before emitting styles.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/generator/object-vanish-styles.ts` around lines 44 - 48, Update collectVanishCharacterStyleIds to use locale-independent sorting when ordering the collected IDs, replacing localeCompare with the default sort or an explicit fixed locale while preserving deduplication and deterministic output.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@openapi.yaml`:
- Around line 8135-8145: Update the OpenAPI schemas for ObjectMeta and SpecNode
to mirror the Zod cross-field validation: textBox objects must not allow rows or
columns, object nodes must include meta.object, and non-object node types must
not include meta.object. Express these constraints with equivalent conditional
or union schema definitions while preserving all valid existing shapes.
In `@src/parser/docx/body-object-attach.ts`:
- Around line 50-52: Update the vanishCharStyleIds sorting in the captured
object serialization to use the default locale-independent array sort instead of
localeCompare, preserving deterministic ordering across runtimes.
In `@src/parser/docx/body-object-vanish.ts`:
- Around line 48-58: Update the doc comment above resolveRunVanish to describe
the implemented tri-state behavior: enabled direct w:vanish hides the run,
disabled direct w:vanish makes it visible even when the character style matches,
and only an absent direct setting falls back to the vanishCharStyleIds result.
Remove the stale OR-behavior explanation and keep the existing
caller/default-set context accurate.
In `@src/parser/docx/body-objects.ts`:
- Around line 53-60: Update the `vanishCharStyleIds` documentation to state that
it contains the subset of `StyleMap.vanishCharStyleIds` referenced by the
specific object. Align the comment with the `referencedVanishCharStyleIds` calls
in `buildTableObject` and `buildTextBoxObject`, and preserve the documented
persistence/rewrite usage.
---
Nitpick comments:
In `@src/generator/index.ts`:
- Around line 292-298: Update the document setup in the generator flow to pass
scopedTree, rather than tree, to renderOptionalHeaderFooter and resolvePageSize.
Keep the existing namespaceVanishTrees and buildSectionChildren usage unchanged,
and do not alter vanishCharStyleIds coverage.
In `@src/generator/object-vanish-namespace.test.ts`:
- Around line 203-226: Add a test alongside the existing object namespace tests
that constructs a blob with a numeric-looking w:rStyle `@w`:val and verifies
namespaceVanishTrees renames it to the expected namespaced style id. Ensure the
assertion exercises rStyleValKey’s numeric coercion branch and confirms the
corresponding vanish style remains applied.
In `@src/generator/object-vanish-namespace.ts`:
- Around line 223-232: In rewriteObjectMeta, replace the blob type assertion on
the returned object with a spread copy of newBlob. Preserve the existing early
return and ensure the copied array is used to satisfy the mutable ObjectMeta
blob shape without a type assertion.
In `@src/generator/object-vanish-styles.test.ts`:
- Around line 71-82: Add a test case in vanishCharacterStyleOptions covering an
id containing NAMESPACE_SEPARATOR and its minted suffix, and assert the
generated option keeps the full id while setting name to the de-namespaced value
from stripVanishNamespace. Preserve the existing empty-input and ordinary-id
assertions.
In `@src/generator/object-vanish-styles.ts`:
- Around line 44-48: Update collectVanishCharacterStyleIds to use
locale-independent sorting when ordering the collected IDs, replacing
localeCompare with the default sort or an explicit fixed locale while preserving
deduplication and deterministic output.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 92a3103e-5611-4f2e-9f64-a5bba7d16de6
📒 Files selected for processing (23)
openapi.yamlsrc/ast/object-schemas.test.tssrc/ast/object-schemas.tssrc/ast/spec-tree-schemas.tssrc/db/queries/object-text-edit.integration.test.tssrc/db/queries/object-text-edit.tssrc/db/queries/paragraphs.integration.test.tssrc/generator/generate-manual-vanish-scope.test.tssrc/generator/index.tssrc/generator/object-vanish-namespace.test.tssrc/generator/object-vanish-namespace.tssrc/generator/object-vanish-styles.test.tssrc/generator/object-vanish-styles.tssrc/parser/docx/body-object-attach.test.tssrc/parser/docx/body-object-attach.tssrc/parser/docx/body-object-round-trip.test.tssrc/parser/docx/body-object-vanish.tssrc/parser/docx/body-objects.test.tssrc/parser/docx/body-objects.tssrc/parser/docx/object-blob-edit.test.tssrc/parser/docx/object-blob-edit.tssrc/parser/docx/styles.test.tssrc/parser/docx/styles.ts
1. body-object-attach.ts — vanishCharStyleIds sort was locale-dependent.
`localeCompare` with no explicit locale follows the HOST's collation, so
the same styles.xml persisted a different JSONB array order per machine,
defeating the determinism the sort exists for. Demonstrated: sv-SE/da-DK
order "ÄUml" last where en-US/de-DE/tr-TR order it second.
NOT the reviewer's suggested bare `.sort()` — that trips this repo's
`sonarjs/no-alphabetical-sort`, which requires an explicit comparator, so
the proposed remedy would have failed CI. An explicit code-unit comparator
satisfies the lint rule and cross-host reproducibility at once. Style IDs
are opaque OOXML identifiers, never shown to a user, so locale-aware
ordering has no value here.
2. body-object-vanish.ts — the doc comment above hasRunVanish still described
the pre-fix OR semantics ("a resolved-off <w:vanish w:val=0> does not
override a matching rStyle") while resolveRunVanish 40 lines below
implements the opposite tri-state. Two adjacent comments asserting
opposite contracts is how the OR logic gets "restored" later.
3. body-objects.ts — CapturedBodyObject.vanishCharStyleIds was documented as
"carried forward unfiltered (the full resolved set)", but both assignment
sites narrow it via referencedVanishCharStyleIds. The field doc is what a
consumer reads before persisting; persisting the document-wide set would
write every other object's hidden-style ids into this object's row.
4. openapi.yaml — expressed both new Zod cross-field rules structurally
(OpenAPI 3.1 if/then): textBox forbids rows/columns, and meta.object is
present on exactly the `object` node type. Note the reviewer's stated
failure mode ("consumers can validate data the server rejects") does not
apply — SpecNode/ObjectMeta appear only under `responses:`, never a
requestBody, so the server is the producer and never emits the rejected
shapes. Added anyway so consumers generating validators from the contract
match the server, and because openapi.yaml is authoritative here.
Verified: ajv-compiled the ObjectMeta constraint and asserted all five
accept/reject cases behave as the Zod .check() does, including that the
additive vanishCharStyleIds field stays unaffected. pnpm lint clean,
3688/3688 unit, contract gate 21/21 against real Postgres.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This was written agentically; verify its assertions and edit accordingly:
Why
Two related privacy/correctness gaps in the body-object (table/text-box) model:
hasRunVanish(the predicatecollectTextuses to decide which runs' text to capture, andrewriteFirstTextuses to decide which run an edit lands in) only resolved a directw:rPr/w:vanish. It never resolvedw:vanishreached indirectly through a character style (w:rPr/w:rStylepointing at a style that itself carries an enabledw:vanish) — the same resolutiondocument.ts'srunIsVanishalready does viaStyleMap.vanishCharStyleIds. The object tier was the odd one out, and the direction is a leak: text hidden only via a character style was captured intoobjectTextand surfaced in the AST/API/rendered output.ObjectMetaSchemaandSpecNodeSchemavalidated each field in isolation but didn't encode the body-object contract as cross-field invariants: atextBoxcould carry table-onlyrows/columns, and nothing tiedtype: 'object'tometa.object's presence.What
Part A (#650) — rStyle-resolved vanish, both directions:
hasRunVanish(now insrc/parser/docx/body-object-vanish.ts, split out to stay under the repo's 400-line file budget) gained an optionalvanishCharStyleIds: ReadonlySet<string>param (default empty set, so every pre-existing single-arg call site — including fix/issue-648'smerge/extract.ts— keeps compiling unchanged). It resolves the two signals as a tri-state (see Adversarial review): a directw:vanishon the run decides outright when present — enabled hides,w:val="0"keeps the run VISIBLE even against a vanish rStyle, per ECMA-376 §17.7.3 — and only its absence falls through to thew:rStylelookup.collectText/anchorInteriorParagraphs/buildTableObject/buildTextBoxObject) and persisted onCapturedBodyObject/ObjectMetaSchema.vanishCharStyleIds(sorted array in JSONB — additive field, no migration; absent and[]are fully interchangeable so pre-change rows load unchanged).replaceAnchoredParagraphText→rewriteFirstText), read back from the persistedObjectMetaat edit time (styles.xml isn't available inside that transaction), so capture and edit never drift on which runs are "hidden" — closing the exact class of bug that caused the PR fix(parser): nested text-box run-vanish leak (#641); refute object-table rule-row leak (#633) #647 regression this issue calls out.parseCharacterStyles'sisVanishandparseVanish's paragraph-style equivalent used a bare'w:vanish' in rPrpresence check, which doesn't honor the OOXMLST_OnOfftoggle — an explicitw:val="0"still read as "vanish". Fixed to reuseisOnOffEnabled, with regression tests (real CPI corpus fixtures carry 15w:vanish w:val="0"runs, several text-bearing).fix(cross):af3b1fe): writing the round-trip test for the new field surfaced a real regression —generateDocx/generateManualnever emitted aw:stylescharacter-style definition for a captured object's referenced vanish style, so regenerating a DOCX and re-parsing it lost the vanish resolution and previously-hidden text surfaced as visible. Fixed in newsrc/generator/object-vanish-styles.ts, which reconstructs a minimal enabled-vanish style stub per referenced id from the persistedvanishCharStyleIds.Part B (#516) — cross-field invariants, verified both directions:
ObjectMetaSchema.check(...):kind: 'textBox'now rejectsrows/columns;kind: 'table'stays unconstrained. The rule names onlykind/rows/columns— it never scans other keys, sovanishCharStyleIds(or any future additive field) is structurally untouched.SpecNodeSchema.check(...):type: 'object'now requiresmeta.object; every othertyperejects a presentmeta.object. Scoped to that presence coupling only — editability derivation stays owned byconventions/classify.ts, per the issue's explicit warning against over-coupling..shapespread audit (test(ast) f511efa): repo-wide scan confirms neither schema's.shapeis ever spread (in particular never becomes an MCP toolinputSchema), so these.check()rules can't silently be lost the way a.shapespread has dropped.strict()elsewhere in this repo before. Verified RED by temporarily injecting an offending spread and confirming the new test catches it, then reverted.Design decisions
No ADR ships with this branch (owner directive for this sprint — commit
b7a244bcreverts an earlier ADR-094 + ADR-092 amendment;git diff origin/main...HEAD -- docs/adr/is empty). The rationale that would have lived there is recorded here and in code comments / test names on the touched files.1. Why not the obvious fix — teach
hasRunVanishto resolvew:rStylefrom theStyleMapalready in scope at parse time.hasRunVanishis exported specifically so capture (collectText) and edit rewrite (rewriteFirstText) share ONE definition; a second, drifting copy is what produced the P1 caught on PR #647. ButrewriteObjectTextBlobreads a persistedobject_dataJSONB row inside an open transaction — there is nostyles.xmlthere and no route to aStyleMap(objects round-trip as an opaque blob, andstyles.xmlis a separate OOXML part the blob never contained). Fixing only capture's call site would desynchronize the two: capture would start skipping style-vanished runs that rewrite still cannot see, so an edit could land in a skipped (hidden) run while blanking the visible one. That is over-suppression — invisible data loss — which is strictly worse than the original leak, because a missed suppression is a visible leak a test can catch while a wrong suppression looks like correct privacy behaviour. Hence: persist the resolved set on the object itself, so both directions read one already-resolved source of truth.2.
vanishCharStyleIdsis persisted as a SORTED array, never aSet— so the JSONB column and fixture snapshots serialize deterministically regardless of theStyleMap's unordered iteration order.3. Absent === empty; no backfill, ever. A row captured before this change carries no set, and none can be re-derived — the originating
styles.xmlis not stored anywhere. Such a row keeps today's exact behaviour (no run treated as hidden by style) until its source document is re-imported. Nothing is fabricated. Pinned by thebackfill:cases inparagraphs.integration.test.tsandobject-text-edit.integration.test.ts.4. The second parameter stays OPTIONAL (
vanishCharStyleIds: ReadonlySet<string> = new Set()). fix/issue-648 (PR #654) callshasRunVanishwith a single argument fromsrc/merge/extract.tsthrough the../parser/index.jsbarrel. Note the barrel lines themselves (parser/index.tsanddocx/index.ts) are added by that branch, not this one — what this branch must preserve is the re-export atbody-objects.ts, which is the specifier PR #654's barrel line resolves through. Verified by compiling #654's exact single-argument call shape against this branch: clean.5.
w:vanishis an ST_OnOff toggle (ECMA-376 §17.3.2.45): aw:valof0/false/offmeans the run is VISIBLE, usually overriding an inherited vanish. Treating mere element presence as "hidden" silently drops visible spec text — real CPI corpus fixtures carry 15 such runs. Both directions are pinned by regression tests.6. The captured blob round-trips byte-identical — it is never normalized. The generator stub reconstructs only the vanish toggle, nothing else of the original character style (font, colour,
basedOn): SpecR does not retain the sourcestyles.xmlto reconstruct them, and none of it affects vanish resolution.7.
generateManualnamespaces vanish style ids per source tree (object-vanish-namespace.ts). Two source documents can each define their own character style under the same id — one genuinely vanish, one used only for bold/underline on visible text. One sharedstyles.xmlnamespace would let one tree's minted vanish stub overwrite another tree's unrelated same-named style, hiding text that tree's own capture correctly resolved as visible. Deliberately unconditional (rename every tree's ids rather than detecting a real collision), and a by-reference no-op fortrees.length <= 1or when no tree carries vanish ids — sogenerateDocxand the common manual case are unaffected. The renamed id is never read back by SpecR; it is only shown to a human in Word.Adversarial review
Codex
gpt-5.6-sol(effortxhigh) ran once as the final draft gate: 4 findings, 4 confirmed, 4 fixed in4b348cc8, none declined. All four were the same class — over-suppression, i.e. visible text getting hidden — and three were introduced by this branch. Per-finding detail, including one follow-on bug the review did not report but its fix exposed (namespacing was not idempotent across round-trip cycles), is in the review comment.Summary:
hasRunVanishnow resolves its two signals as a tri-state rather than an OR, so an explicit<w:vanish w:val="0"/>can un-hide an rStyle-vanished run (ECMA-376 §17.7.3 — direct formatting is definitive for a toggle property); capture persists only the vanish ids the object's own blob references; vanish style ids are namespaced on every generation path so a minted stub can never collide with adocxbuilt-in such asHyperlink; and minted ids are collision-checked and idempotent.Two tests that encoded the old behaviour were flipped as explicit, commented oracle changes, not quietly edited.
One item is deliberately not acted on and needs a human call:
document.ts's paragraph-tierrunIsVanishstill OR's the same two signals and is wrong by the same reading, but correcting long-shipped paragraph classification is outside this issue's scope.Verification evidence
DB round-trip of
vanishCharStyleIdsis PROVEN, not assumed (test(db)2f5613df). The pre-existing coverage inserted object rows with raw SQL, which proves the JSONB column stores the field but never exercises the canonical save path — precisely the shape of thepageSizeregression in PR #536, where an additiveSpecTreefield passed every in-memory test while the save/load mappers dropped it. The new cases inparagraphs.integration.test.tsdriveinsertTree(the same write path DOCX import uses) and assert twice: once against the rawobject_data->'vanishCharStyleIds'JSONB column via SQL (proof the value is on disk, not in a cache), and once through the real read mappers (getSpecTree,fetchSubtreeNode).Mutation-verified in both directions rather than assumed load-bearing:
vanishCharStyleIdsfromObjectMetaSchema(read path — Zod strips it)flattenDfs'sobjectDatamapping (write path — the exact #536 shape)Full-corpus revalidation, measured this session — not carried over from an earlier claim. Snapshotted
origin/mainin a throwaway detached worktree and this branch's HEAD, then diffed the two:Addition-only: 0 fixtures moved. No corpus file exercises a
w:rStyle-referenced vanish character style (measured in the issue — 0 of 39 DOCX fixtures), so this closes a production-reachable gap the corpus itself cannot exercise end-to-end; coverage comes instead from the hand-built fixtures inbody-object-round-trip.test.ts,object-schemas.test.ts, and the two integration suites above.Mutation verification (both directions, both new
.check()rules)Every new cross-field rule has a paired reject-invalid / accept-valid ("control") test in
object-schemas.test.ts:type:'object'with nometa.object→ rejected; non-objecttypewithmeta.object→ rejected;type:'object'withmeta.objectpresent → accepted (control).vanishCharStyleIdsis confirmed to never couple to either new rule (a textBox withvanishCharStyleIdspopulated and no rows/columns still validates).hasRunVanishfinal signature (for the fix/issue-648 broker)fix/issue-648's
merge/extract.tscall site (single-argument, pre-#650) keeps compiling and behaving byte-for-byte unchanged against this signature — no merge-time signature change required, only an opportunity (not obligation) to pass a realvanishCharStyleIdsset if that branch has one available at its call site.LOC note
Over the repo's ~500 LOC guidance (CI
loc-checkis warn-only). This is one coherent change — Part A and Part B are two Zod/parser slices of the same body-object-model gap (#650/#516 were explicitly linked as sibling issues) — plus one in-branch cross-fix required to make Part A's own round-trip claim true (fix(cross):af3b1fe) and a.shape-spread regression guard (f511efa) rather than a point-in-time PR-body note. Left un-split per this repo's standing rule that a discovered cross-fix ships in-branch, not deferred.Testing
pnpm lint— clean (eslint + tsc --noEmit + prettier --check)pnpm test— 258 files / 3688 tests passedpnpm test:integration— 158 files / 1817 tests passed (12 files / 141 skipped, pre-existingdescribe.runIfgating unrelated to this change)vanishCharStyleIdsproven against the realobject_datacolumn, mutation-verified on both the read and write pathsgit diff origin/main...HEAD -- docs/adr/empty — no ADR ships with this branchgpt-5.6-solxhigh): 4/4 findings fixed🤖 Co-authored by Claude Opus 5. Closes #650. Closes #516.
Summary by CodeRabbit
New Features
Bug Fixes
false,off, and0.