Skip to content

fix(pptx): flag an avLst adjustment value the preset does not declare - #422

Open
argszero wants to merge 1 commit into
iOfficeAI:mainfrom
argszero:fix/undeclared-adjust-value
Open

argszero wants to merge 1 commit into
iOfficeAI:mainfrom
argszero:fix/undeclared-adjust-value

Conversation

@argszero

Copy link
Copy Markdown
Contributor

Summary

add / set geometry= copy an adj=name:fmla name straight into a shape's <a:avLst> unless the preset is one CanonicalAdjName knows (donut, noSmoking) or one MultiGuidePresetDefaults covers. That lets the CLI write an adjustment value the preset never declares:

$ officecli create repro.pptx
$ officecli add repro.pptx / --type slide
$ officecli add repro.pptx /slide[1] --type shape --prop geometry=rect --prop adj=adj:val 14000
Added shape at /slide[1]/shape[@id=100000]

$ unzip -p repro.pptx ppt/slides/slide1.xml | grep -o '<a:prstGeom.*</a:prstGeom>'
<a:prstGeom prst="rect"><a:avLst><a:gd name="adj" fmla="val 14000" /></a:avLst></a:prstGeom>

rect declares no adjustment value at all, and --prop geometry=roundRect --prop adj=adj1:val 10000 writes the wrong name for the single value roundRect does declare. ECMA-376 leaves an undeclared value unused rather than invalid, so the file still opens and validate stays green — the geometry the author asked for silently never takes effect.

This PR makes that observable: view issues now emits one Warning per offending shape, under the new undeclared_adjust_value subtype. add / set and validate are unchanged.

What a reviewer should see

$ officecli view repro.pptx issues
Found 1 issue(s):

Format Issues (1):
  [A1] /slide[1]/shape[@id=100000]: Adjustment value "adj" is not declared by preset "rect" (preset declares none) — PowerPoint ignores an undeclared adjustment value, so the geometry the author asked for never takes effect.
       Context: "<a:prstGeom prst="rect"><a:avLst><a:gd name="adj" fmla="val 14000"/></a:avLst>"
       Suggestion: Drop the undeclared value from the avLst, or use a preset that declares it.

In JSON the same record with "subtype": "undeclared_adjust_value", "severity": 1, "type": 0; --type format, --type f and --type undeclared_adjust_value all select it, --type structure / content do not. validate is untouched in every respect, including its exit code.

Why view issues and not validate

Making it explicit rather than quietly choosing a different surface:

  1. SKILL.md draws the line itself — validate is "Validate against OpenXML schema" (line 99), issues is "Formatting/content/structure problems" (line 108). validate's extra detectors (DetectOutOfGridSheetRefs, DetectMissingDefaultRelsContentType) exist for files that are schema-legal but refused by the app; here the app accepts the file and ignores the value instead.
  2. validate has no non-fatal tier: its list count is the exit code and the delivery gate (CommandBuilder.Check.cs, and the resident path). A lint there would turn a deck that opens fine into rc=1.
  3. view issues already carries exactly this species of finding with Severity.Warning and a filterable subtype — low_contrast is the closest precedent.

If you would rather have it inside validate, that needs a non-fatal tier in ValidationError plus an exit-code decision — a separate atomic change; the table and the detector here would be shared.

The table is generated from the definitions, not recalled

A preset's own <a:avLst> in presetShapeDefinitions.xml is its list of adjustment values (adj, adj1adj8, plus the star/hexagon hf/vf); the computed guides in its <gdLst> are not authorable through prstGeom, so they are deliberately excluded. The table covers all 187 presets and was generated from the ECMA-376 definitions as vendored verbatim by LibreOffice at oox/source/drawingml/customshapes/presetShapeDefinitions.xml (538972 bytes, sha256 eaff19f4405b3be6822428c96ef46cc685499217366fd58e5e63a5a24739f02f), not typed from memory.

The 64 definitions that declare no adjustment value (rect, ellipse, cloud, …) are listed with an empty array rather than omitted, so "declares none" stays distinguishable from "preset unknown to this table" — an unknown preset token is never judged, so the lint fails open. Keyed by the prstGeom@prst token, not the SDK enum, so it also covers presets officecli cannot write itself but real files carry.

It is the same knowledge the file already hand-codes, not a second opinion: CanonicalAdjName hand-codes donut/noSmoking and MultiGuidePresetDefaults hand-codes five shapes, and the generated rows are checked to equal those hand-written rows name for name (tier 1 below).

Deliberate limits, stated rather than implied: slides and their top-level shapes only — the same scope as the neighbouring lints in that loop; a subset of a multi-guide preset's declared set is not flagged (that is a completeness concern, not a name problem, and PowerPoint handles those decks); name comparison is ordinal, matching the definitions.

Validation

Rule 2 — how to confirm it works. Before/after with the same three commands (also the reproduction in #235):

view issues validate written <a:gd>
before this PR Found 0 issue(s) Validation passed. (rc 0) <a:gd name="adj" fmla="val 14000"/> inside prst="rect"
after this PR Found 1 issue(s) ([A1], undeclared_adjust_value, Warning) Validation passed. (rc 0) identical — the writer is untouched

Beyond the single command sequence, a four-tier script (29 assertions, 0 failures), run against two builds of the same commit — main unpatched, and main + this PR:

  1. Provenance and agreement — re-derives the table from presetShapeDefinitions.xml (sha256-checked) and requires it to equal the rows in this PR exactly, in order; requires the generated rows to equal the hand-written CanonicalAdjName / MultiGuidePresetDefaults rows name for name.
  2. Filter contractformat / f / undeclared_adjust_value (any case) select the finding, structure / content do not, the unfiltered text and JSON agree, validate still returns rc 0, and the pre-fix binary rejects the new subtype name (so registering it is what enables the filter).
  3. Fail-open — a deck whose prst is a token the table does not know produces no finding and does not throw.
  4. False-positive sweep over the repo's own decks — all 63 examples/**/*.pptx, 3621 prstGeom shapes: the two binaries report the exact same 1976 findings (including 5 real roundRect adjustment values in the corpus, which are not flagged), i.e. zero false positives and zero regressions on real files.

A 15-scenario A/B harness (undeclared names, correct names, multi-value specs, empty avLst, multi-guide presets) shows the same split: the pre-fix binary fails 5 scenarios (writes the value, reports 0 issues), the patched binary passes all 15 — with the add output and the on-disk <a:gd> set identical between the two binaries in every scenario, which is the evidence that only the lint was added.

dotnet build src/officecli/officecli.csproj -c Release: 0 errors, and the pre-existing 2 warnings only (no new ones).

Rule 1 self-check (one atomic change)

Asked of this diff: can it be decomposed into multiple PRs that could each be merged or reverted independently? No. The table, the detector, the subtype registration and its single consumer are one change with one root cause: the surface cannot report an undeclared value without all four, and no subset of them has standalone value (a table without a consumer is dead code; a consumer without the table cannot tell declared from undeclared). Splitting add-side rejection out is a separate change, and deliberately not made here.

Related

#235 — this is the lint half only. The write path is unchanged, per the 2026-07-19 ruling in that thread that add / set behaviour should not change until the corruption mechanism is pinned down. The same table is what a write-side guard would need, and the same split exists for xlsx (whose handler also writes prstGeom) — happy to send either as a separate change if it is wanted.

`add` / `set geometry=` copy an `adj=name:fmla` name straight into the
`<a:avLst>` unless the preset is one `CanonicalAdjName` knows (donut,
noSmoking) or one `MultiGuidePresetDefaults` covers. So
`--prop geometry=rect --prop adj=adj:val 14000` writes `<a:gd name="adj"/>`
into a preset that declares no adjustment value at all, and
`--prop geometry=roundRect --prop adj=adj1:val 10000` writes the wrong name
for the single value roundRect does declare.

ECMA-376 leaves an undeclared value unused rather than invalid: the file
opens and the schema validator (so `validate`) stays green, while the
geometry the author asked for silently never takes effect. `view issues`
now reports it as a Warning with the `undeclared_adjust_value` subtype, so
the defect is observable. The writer and `validate` are untouched.

The `preset -> declared adjustment values` table is generated from the
ECMA-376 definitions rather than recalled, and covers all 187 presets:
a preset's own `<a:avLst>` in presetShapeDefinitions.xml is its list of
authorable values, while the computed guides in its `<gdLst>` are not
authorable through `prstGeom`. The 64 definitions that declare none are
listed with an empty array, so "declares none" stays distinguishable from
"preset unknown to this table" — an unknown token is never judged.

Related: iOfficeAI#235 (the lint half only; the write path is unchanged, per the
maintainer's 2026-07-19 ruling that `add`/`set` behaviour should not change
before the corruption mechanism is pinned down).
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