spike(40): Proposed Change contract (RFC #8) piloted on Slides — VALIDATED - #46
Conversation
…DATED Shape-level ops (set_shape_text, set_shape_style, add_shape, remove_shape) over the pptx-engine dirty-flag model, atomic applyProposal (validate all ops before mutating any), RFC #8 lifecycle + audit persistence, and a harness proving: per-part byte-preservation (only slide1.xml differs), reject = byte-identical save, audit reloads in the RFC shape. Verdict: the contract fits Slides with no structural adaptation; the cross-app promise can be made public. Costs: Slides ~3-4d, Docs ~4-6d, PDF ~6-8d (preview is the bottleneck). Findings: element ids are process-local counters (not stable across parse sessions — audit elementId valid only in the applying session); fill color normalizes to #RRGGBB on re-parse.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8de1eb39b5
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const slide = slideOf(deck, op.slideIndex) | ||
| addElement(slide, { | ||
| kind: op.kind, | ||
| offset: { x: op.x, y: op.y, w: op.w, h: op.h }, |
There was a problem hiding this comment.
Every add_shape operation supplies w and h, but addElement expects an EmuRect containing cx and cy. Running the added harness therefore writes <a:ext cx="undefined" cy="undefined"/> into slide1.xml, so the supposedly validated output contains invalid geometry and the inserted shape may not render correctly in PowerPoint. Map the operation dimensions to cx/cy before calling the engine.
Useful? React with 👍 / 👎.
| } | ||
| if (op.elementId) elementOf(deck, op.slideIndex, op.elementId) | ||
| } | ||
| for (const op of proposal.operations) OPS[op.type](deck, op) |
There was a problem hiding this comment.
When operations interact, this second loop can throw after earlier operations have already mutated and dirtied the deck. For example, two removals of the same element both pass the preflight existence check, then the first removal succeeds and the second throws; a remove followed by a style operation has the same problem. This violates the adapter's atomicity guarantee and can leave a failed proposal partially applied, so validation must account for sequential effects or application must occur on a rollback-capable clone.
Useful? React with 👍 / 👎.
| export function applyProposal(deck, proposal) { | ||
| for (const op of proposal.operations) { |
There was a problem hiding this comment.
When a caller passes a draft, proposed, or rejected proposal, applyProposal ignores its status and mutates the deck anyway. In particular, applying a rejected proposal directly contradicts the lifecycle guarantee that rejection must not change document bytes and makes the public mutation gate unsafe unless every caller independently duplicates the check. Require proposal.status === 'accepted' before validating or executing operations.
Useful? React with 👍 / 👎.
| transform: el.transform | ||
| ? { x: el.transform.x, y: el.transform.y, w: el.transform.w, h: el.transform.h } | ||
| : null, |
There was a problem hiding this comment.
For every parsed slide element, the engine stores coordinates under el.transform.offset using x, y, cx, and cy; the properties read here do not exist. Consequently persisted previews contain transform: {} for every element, depriving the approval surface of the position and dimensions needed for a shape-level semantic preview. Read the offset object and map cx/cy to the desired preview width and height.
Useful? React with 👍 / 👎.
|
Reopening to trigger a fresh pull_request run — CI recovered after the Actions outage; this PR was created while events were being dropped. |
1 participant
Spike 40 — RFC 0008 Proposed Change no Slides
Valida o contrato unificado
Proposed Changeno caso mais difícil de preview (shape-level). VALIDADO em < 1 dia (time-box: 2 semanas).spikes/002-proposed-change-slides/— contract (ciclo RFC RFC: unified "proposed change" contract in agent-core #8), slides-adapter (ops tipadas + apply atômico), harness E2Eslide1.xmldifere)sp_*são contadores de processo (não estáveis entre sessões — audit elementId vale só na sessão que aplicou); cor normaliza para#RRGGBBno re-parseCloses #40