Skip to content

Propose che-pptx geometry tools - #95

Closed
kiki830621 wants to merge 1 commit into
mainfrom
codex/che-pptx-geometry-tooling-spec
Closed

kiki830621 wants to merge 1 commit into
mainfrom
codex/che-pptx-geometry-tooling-spec

Conversation

@kiki830621

Copy link
Copy Markdown
Member

Summary

Validation

  • spectra analyze che-pptx-geometry-tools --json
  • spectra validate che-pptx-geometry-tools
  • git diff --check -- openspec/changes/che-pptx-geometry-tools

Refs #90

@kiki830621

Copy link
Copy Markdown
Member Author

Verify Report — PR #95

Engine

6-AI ensemble: 5 general-purpose Agents (Claude reviewers) + Codex (gpt-5.5 xhigh). Initial spawn rate-limited; Recovery Protocol Step 2.5b retry with FULL context re-paste succeeded. 5/5 findings present + non-empty. No Process Gaps.

Aggregate

NEEDS WORK — Scope slice (geometry-only) is legitimate and dual-capability split is justified, but the three cm-based tools' contracts are not implementation-ready. Multiple BLOCKER-level gaps converge across reviewers + Codex + Devil's Advocate.

Scope coverage


#90 (first slice: geometry) — che-pptx-geometry-tools

Requirements coverage: 3/3 in-scope tools FULLY addressed (set_placeholder_geometry, place_picture_at, fit_picture_to_native_aspect). 6 deferred slices explicitly Non-Goal'd. Scope slice discipline ✓.

Findings (merged + deduplicated across 6 sources)

# Severity Finding Source Action
1 BLOCKER fit_picture_to_native_aspect positioning contract incomplete — only anchor (horizontal), max_width_cm, max_height_cm specified. Missing left_cm/top_cm (or "max box origin = whole slide" rule), missing vertical anchor. Same image + same slide → multiple valid results = non-deterministic spec. agents:codex+logic+devils-advocate Spec rewrite
2 BLOCKER place_picture_at fit parameter has no default, but Issue #90 sketch says "default contain". Direct contract drift from issue → spec; not documented as intentional change. agents:codex+devils-advocate Lock default OR document intentional removal
3 BLOCKER contain / cover / stretch not fully deterministic. contain left-blank offset rule unspecified. cover allows both crop metadata OR oversized picture rectangle (design.md:27-29) — persisted geometry diverges from readback. Implementation choice leaks through spec. agents:codex+logic+devils-advocate Pick one representation, spec exact rectangle behavior
4 BLOCKER occurrence_index semantics unspecified — XML document order? OOXML idx attribute? PowerPoint UI ordinal? Three different reasonable interpretations. Logic reviewer + DA both flag. agents:logic+devils-advocate Lock to one with rationale
5 BLOCKER cm→EMU half-up rounding mode ambiguous — Double.rounded() in Swift defaults to .toNearestOrEven (banker's), not .toNearestOrAwayFromZero (typical "half-up"). Spec text "half-up" doesn't pin Swift rule. Negative + boundary cases need SBE Examples. agents:logic Spec text + Examples
6 BLOCKER EMU overflow guard missing. OOXML ST_PositiveCoordinate / ST_Coordinate is 32-bit (Int32.max ≈ 5965 cm). fit_picture_to_native_aspect does multiplication, easier to overflow. Spec has zero overflow language → server crash on width_cm: 6000. agents:logic+security Spec invariant + typed error
7 BLOCKER cover writer support not pre-assessed. Spec says writer SHALL throw unsupported-fit error if it can't persist crop, BUT design.md doesn't audit whether current packages/pptx-swift/Sources/PPTXSwift/IO/ writer supports any crop form. If it's "none", every cover call throws — cover becomes dead spec. agents:regression+codex Pre-assess writer support
8 HIGH 9 spec-level security gaps: path traversal in image_path; MIME magic byte not validated; image size cap unset; cm geometry bounds unset (Int64 overflow → Swift trap → server crash); MCP arg validation missing (slide_idx negative? OOB?); NaN/Inf not rejected (Float trap); -0.0 sign handling; doc_id concurrent access undefined; use-after-close undefined. agents:security New Spec section "Geometry input validation invariants" + "doc_id lifecycle"
9 HIGH New place_picture_at (cm + image_path) and existing insert_image (EMU + base64) coexistence strategy not addressed. New set_*_geometry_cm vs existing set_shape_position / set_shape_size (Server.swift:884, 902 — EMU integer) — supersede / coexist policy missing. design.md silent. agents:regression design.md add "Relationship to existing geometry tools"
10 HIGH "deferred ≠ tracked". 4 deferred slices (text runs / template / notes / export) + 2 sub-proposals (scripts / references) have ZERO follow-up GitHub issues. Violates .claude/rules/cross-repo-umbrella.md discipline. Once #90 issue body's umbrella check-list is updated, those slices' status will be invisible. agents:devils-advocate+regression Open follow-up child issues; update #90 umbrella
11 MEDIUM Dual capability split (pptx-slide-write + pptx-mcp-server) justified by "non-MCP callers like Swift scripts" — but those scripts are simultaneously deferred. YAGNI / premature abstraction. Could collapse to single capability for Phase 1. agents:devils-advocate Document or collapse
12 MEDIUM cm chosen over mm/pt/in without rationale. mm is mathematically cleaner (EMU/100 = no rounding for many values); pt will be needed for the deferred text-runs slice. Per-tool unit param would be future-proof. agents:devils-advocate Document choice OR change to mm
13 MEDIUM "Geometry first" among 5 slices — no Decision explains why. Issue body's motivation is "iteration cost" which is collapsed better by export_pdf / render_slide_png (slice 5: 4-step Keynote→PDF→inspect loop). Geometry was picked for engineering convenience, not maximum user value. agents:devils-advocate Add Decision in design.md
14 MEDIUM Tasks acceptance criteria loose — no response schema, error name list, default value tests, cover/contain offset numeric SBE, doc-content minimum. agents:codex Tighten tasks.md
15 MEDIUM set_placeholder_geometry error conditions incomplete: shape_id + placeholder_type both given → priority? error? Width/height = 0? NaN? placeholder_type legal-value list? image_path missing? Format unsupported? Spec silent. agents:codex Spec exhaustive errors
16 LOW Spec only covers Shape + Picture. Slide elements include graphicFrame (table) + group — these also have position/size and could be addressed by set_placeholder_geometry semantically. Non-Goal not declared. agents:logic Document as Non-Goal
17 LOW Rounding boundary SBE missing. Only easy cases (2.54 cm → 914400 EMU) shown; halfway cases (1.5 cm at EMU rounding boundary, negative cm halfway) not in Examples. agents:regression Add Examples
18 LOW Tool response shape (return JSON) never locked with SBE Example. agents:devils-advocate Add response Example
19 LOW references/python-pptx/ framing — clone-on-demand operational concern per CLAUDE.md, NOT a Spectra capability. Listing it as a Non-Goal is category error. python-docx mentioned in issue but not in PR's deferred list — inconsistency. agents:devils-advocate Drop from Non-Goal; or rename framing
20 LOW Tool README docstring inconsistent with MCP tool registration text (logic reviewer noted in findings file). agents:logic Doc fix in apply phase

Scope Check

Pure spec proposal, no code. Scope reduction from #90 (umbrella) to first-slice geometry is well-documented. No scope creep within the proposal.

Security

SPEC-level security review only. Per finding #8 above: 9 dimensions where spec must address but currently doesn't. Implementation phase would have to retrofit which breaks the API contract.

Process Gaps

Initial spawn rate-limited. Recovery Protocol Step 2.5b retry succeeded — all 5/5 reviewer findings produced.


Recommendation

Do NOT merge as-is. Two paths consistent with the PR #94 disposition:

Path A (proper IDD): Block merge. Send Codex back with revision request covering:

  1. fit_picture_to_native_aspect: add left_cm/top_cm or document "max box = whole slide" rule + add vertical anchor
  2. place_picture_at fit: lock default (contain per issue) OR document intentional removal
  3. contain / cover / stretch: pick exact rectangle representation, decide crop persistence policy, define offset rules
  4. occurrence_index: pick one semantic (recommend: OOXML XML document order with explicit Note)
  5. cm→EMU rounding mode: spec .toNearestOrAwayFromZero explicitly + Examples covering negative + halfway
  6. EMU overflow guard: spec ST_PositiveCoordinate / ST_Coordinate bounds + typed error
  7. cover writer pre-assessment: audit current pptx-swift writer; if no crop, downgrade cover to Non-Goal or accept oversized rectangle
  8. Adversarial input handling section (9 security gaps consolidated)
  9. Existing-tools coexistence section in design.md
  10. Open follow-up child issues for 4 deferred slices + 2 sub-proposals; update [docs/feature] 提議擴充 che-pptx-mcp + Swift 腳本工作流;references/ 放 python-pptx 當參考 #90 umbrella

Path B (pragmatic): Merge as Phase 1 minimum + file 10 follow-up issues / Spectra changes. Path B for #95 is harder to justify than for #94 because the BLOCKERs (1-7) make the spec ambiguous to implement — Phase 1 implementer would have to make 7 unilateral choices that lock the API. That's no longer "Phase 1" but "design via implementation".

Recommendation: Path A for #95.

@kiki830621

Copy link
Copy Markdown
Member Author

Blocked pending spec revision per verify report (7 BLOCKERs). Full list at #95 (comment) — items 1-7 are spec ambiguities that would force Phase 1 implementer to make unilateral API-locking choices. Recommend revising spec before merge.

@kiki830621

Copy link
Copy Markdown
Member Author

Closing — Superseded by ooxml-edit-isomorphism-foundation (#99 merged in PR #106)

Per #99 ADR-009, this proposal is reframed as a PPTX specialization front-end of the architectural foundation. The current spec has known gaps (see verify report findings) that the foundation's locked contract addresses naturally.

The re-framing work is tracked at #104 — Codex (or other authors) can open a new PR citing ooxml-edit-algebra capability spec + foundation design.md ADRs as inputs. The Codex work in this PR remains accessible in repo history (git show codex/<branch>) for reference when authoring the re-frame.

Issue #90 (original umbrella) was closed as absorbed into #99 architecture; #104 is the operational follow-up.

Refs #99 #90 #104

@kiki830621 kiki830621 closed this May 25, 2026
kiki830621 added a commit that referenced this pull request Sep 29, 2026
Pins the Word↔Swift edit-isomorphism contract as the macdoc OOXML
toolchain's core architectural contract via:
- New capability spec 'ooxml-edit-algebra' (8 Requirements)
- design.md with 9 ADRs (canonical-identity, Edit-as-first-class,
  two-layer algebra, module split, naming, Word UI ground truth,
  conformance suite, lens migration path deferred, downstream rerouting)
- tasks.md with hybrid scope: Edit type elevation + property-based
  functor tests on 3-5 representative OOXMLEdit cases only;
  downstream migrations explicitly deferred to follow-up Spectra changes

Cross-references active 'word-aligned-state-sync' change in design.md
Relationship section. Coordinates downstream PRs #94/#95/#96/#97/#98
that 6-AI verify identified as blocked on spec ambiguity — they will
be reframed as front-ends to this foundation per ADR-009.

Refs #99
kiki830621 added a commit that referenced this pull request Sep 29, 2026
…-§10)

Shipped (apply phase, 11/35 tasks):
- §8.1 Cross-reference to ooxml-edit-isomorphism-foundation in
  word-aligned-state-sync/design.md Relationship section
- §8.2 Follow-up issue #101 (word-builder-swift lens migration)
- §8.3 Follow-up issue PsychQuant/che-word-mcp#162 (MCP boundary refactor)
- §8.4 Follow-up issue #102 (PR #94 dxedit re-frame)
- §8.5 Follow-up issue #103 (PR #96 R-wordbuilder re-frame)
- §8.6 Follow-up issue #104 (PR #95 pptx-mcp re-frame)
- §9.1 .github/PULL_REQUEST_TEMPLATE.md with CD-diagram requirement
- §9.2 EditAlgebra/README.md documenting CD discipline + worked examples
- §10.1 spectra validate green
- §10.3 docs/structural-editing-paradigm.md cross-reference
- §10.4 docs/lossless-conversion.md cross-reference

Deferred to Phase 2 Spectra change (§1-§7 + §10.2 Swift implementation,
23/35 tasks): Edit protocol code, OOXMLEdit/WordEdit enums, Document.apply
API, property-based functor tests. Deferral rationale documented in
tasks.md ASSUMPTION block; new follow-up Spectra change
'ooxml-edit-algebra-implementation' opens after this foundation archives.

Decision-pinning (this change) + runtime implementation (Phase 2) serve
different review modes and benefit from being separate changes.

Refs #99
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