issue-195: Implement penpot_commit with revn tracking, conflict retry and commit preview #228

Merged

david merged 3 commits from feature/issue-195/implement-penpot-commit-with-revn-tracking-conflic into main

2026-09-15 00:49:10 +00:00

david commented

2026-09-15 00:49:09 +00:00

Owner

195: Implement penpot_commit with revn tracking, conflict retry and commit preview

Closes #195 — 195.

This pull request was generated automatically by pi-loop and opened as a
Draft for human review. Do not merge without reviewing the changes.

Generated in 10m 02s.

Review findings

Now let me apply the fixes. First, the pure helper src/commit.ts (Low finding at line 80):Now the tool changes. First imports, guard, and conflict predicate:Now update the test file — authorize staged files, adjust the conflict assertion, and add coverage for the new behaviour:I've completed the remediation pass. Here is a summary of the changes:

Fixed (all Critical/High/Medium/Low code findings):

High tools/commit.ts:252 — commitFailure now branches on error.category: transport/server/unexpected get the "may or may not have been applied; re-read before retrying or discarding" wording; only provably-rejected categories claim "Nothing was written".
High tools/commit.ts:104 — assertCommitTargetAllowed now enforces isTargetAuthorized(fileId), so only a file named/staged by the staging primitives can reach update-file.
Medium tools/commit.ts:401 — both state.revn = writeRevn assignments removed, so the staged base revision is never overwritten by the commit.
Low src/commit.ts:80 — discrimination is now an explicit switch (change.type) with a default branch reporting an unverifiable change as missing.
Low tools/commit.ts:418 — isRevisionConflict treats vern-conflict as retryable alongside revn-conflict.
Low tools/commit.ts:462 — a verified commit now requires after.value.revn > writeRevn, failing with revisionNotAdvancedFailure otherwise.

Added 5 tests (vern-conflict retry, guard refusal, non-advancing revision, 5xx uncertainty wording, base-revn preservation). Penpot suites: 207/207. Full npm test: 333/334, the single failure pre-existing and unrelated (untouched package.json/scripts/local-install.test.mjs).

The AGENTS.md:23 finding concerns the branch name/commit subject/PR title, which cannot be changed here without forbidden git operations (commit/branch). Left unresolved for the pipeline.

Review Findings — 195

Summary

All code findings were fixed in a single pass: commitFailure now distinguishes provably-rejected categories from possibly-applied ones (transport/server/unexpected); the designated-target guard assertCommitTargetAllowed now enforces isTargetAuthorized; the commit no longer overwrites the staged base revn; verifyStagedChanges uses an explicit discriminated switch with a default that reports an unverifiable change as missing; vern-conflict is retried like revn-conflict; and a verified commit now requires the re-read revision to advance past writeRevn. Five focused tests were added (penpot suites 207/207; full npm test 333/334, the one failure pre-existing on main and untouched by this diff). The remaining unresolved item is the branch/commit-naming finding, which requires forbidden git operations (committing/amending a branch) and is the pipeline's responsibility before the MR is opened.

Critical

High

extensions/penpot/src/tools/commit.ts:252 — commitFailure is used for every non-revn-conflict failure, including transport (timeout / abort / network error) and server (5xx) categories. Its text asserts "was rejected by Penpot" and "Nothing was written to Penpot; the staged change remains staged", which the extension cannot know for those categories — the request may have reached the server and been applied, so a caller that retries or discards can leave a duplicate/leftover write. Suggested fix: branch on error.category in commitFailure; for transport, server and unexpected, emit the same uncertainty wording already used at extensions/penpot/src/tools/commit.ts:449 ("the write may or may not have been applied; re-read the file with penpot_get_file before retrying or discarding") instead of the false "Nothing was written" claim.
extensions/penpot/src/tools/commit.ts:104 — the designated-target guard assertCommitTargetAllowed is a deliberate no-op, so the only write path will write to any file id the token can edit (including files the user never named). This is the PENPOT_SKILL_DESIGN.md §9 / plan-M2 write-safety item, and the store already carries the authorisation (authorizeTarget/isTargetAuthorized, populated by the staging tools at extensions/penpot/src/tools/tokens.ts:288); only the commit call site ignores it. Suggested fix: implement the policy here (at minimum if (!isTargetAuthorized(fileId)) throw new Error(...)) so a file the user did not name/stage can never be written; if it stays deferred, add a test that pins the deferral so the placeholder cannot ship unnoticed.

Medium

extensions/penpot/src/tools/commit.ts:401 — the commit overwrites the store's base revision (state.revn = writeRevn;, repeated at :439 on a conflict refresh), but Changeset.revn is documented as "the base revision observed when staging began" (extensions/penpot/src/changeset.ts:465) and "never overwritten" (extensions/penpot/src/changeset.ts:493). After any failed commit, penpot_status and dry_run therefore report a "Base revn" (extensions/penpot/src/changeset.ts:671) that is not the staging base, which is misleading about how far the file has drifted. Suggested fix: keep the live revision in the local writeRevn/writeVern variables only (drop both state.revn = writeRevn assignments) or add an explicit lastWriteRevn field so the reported base stays the staging base.

Low

extensions/penpot/src/commit.ts:80 — the doc comment claims "a change that is not yet verifiable by this function … is treated as missing rather than assumed applied", but the code after the add-color branch unconditionally reads change.typography, so a third ChangesetChange member would be a compile error (or, with a widened type, would be mis-verified as a typography), not a missing entry. Suggested fix: make the discrimination explicit (switch (change.type) with a default branch that pushes an unverifiable/missing entry, and a never exhaustiveness check), or correct the comment to match the actual closed-union behaviour.
extensions/penpot/src/tools/commit.ts:418 — only revn-conflict is refreshed and retried; a vern-conflict (design §7: vern ≠ stored → vern-conflict) is reported as a generic rejection ("fix and retry") even though the retry path already re-reads vern and would very likely succeed on the next attempt. Suggested fix: treat vern-conflict as retryable by the same isRevnConflict-style predicate so the refresh-and-retry budget covers it too.
extensions/penpot/src/tools/commit.ts:462 — a verified commit reports Revision: previousRevn → newRevn without checking that after.value.revn actually advanced past writeRevn; a re-read returning an unchanged revision is still presented as a committed revision transition, and the design's validation criterion ("the file's revision advanced exactly once") is not asserted anywhere. Suggested fix: add after.value.revn > writeRevn to the verification result (and fail/flag when it does not hold), or word the line so a non-advance is not implied.
AGENTS.md:23 — the branch feature/issue-195/implement-penpot-commit-with-revn-tracking-conflic has a truncated kebab slug, and the commit subject is 195: Implement penpot_commit… rather than the required issue-<N>: <summary> form (AGENTS.md:26). Suggested fix: use the full title-derived slug (…-conflict-retry-and-commit-preview) and ensure the squashed commit / PR title use the issue-195: <title> form before the MR is opened.

Documentation

Updated files:

docs/PENPOT_SKILL_DESIGN.md
docs/PENPOT_SKILL_IMPLEMENTATION_PLAN.md

## 195: Implement penpot_commit with revn tracking, conflict retry and commit preview Closes #195 — [195](https://git.excelera.net/david/pi-extensions-and-skills/issues/195). > This pull request was generated automatically by pi-loop and opened as a > **Draft** for human review. Do not merge without reviewing the changes. Generated in 10m 02s. ## Review findings Now let me apply the fixes. First, the pure helper `src/commit.ts` (Low finding at line 80):Now the tool changes. First imports, guard, and conflict predicate:Now update the test file — authorize staged files, adjust the conflict assertion, and add coverage for the new behaviour:I've completed the remediation pass. Here is a summary of the changes: **Fixed (all Critical/High/Medium/Low code findings):** - **High `tools/commit.ts:252`** — `commitFailure` now branches on `error.category`: `transport`/`server`/`unexpected` get the "may or may not have been applied; re-read before retrying or discarding" wording; only provably-rejected categories claim "Nothing was written". - **High `tools/commit.ts:104`** — `assertCommitTargetAllowed` now enforces `isTargetAuthorized(fileId)`, so only a file named/staged by the staging primitives can reach `update-file`. - **Medium `tools/commit.ts:401`** — both `state.revn = writeRevn` assignments removed, so the staged base revision is never overwritten by the commit. - **Low `src/commit.ts:80`** — discrimination is now an explicit `switch (change.type)` with a `default` branch reporting an unverifiable change as missing. - **Low `tools/commit.ts:418`** — `isRevisionConflict` treats `vern-conflict` as retryable alongside `revn-conflict`. - **Low `tools/commit.ts:462`** — a verified commit now requires `after.value.revn > writeRevn`, failing with `revisionNotAdvancedFailure` otherwise. Added 5 tests (vern-conflict retry, guard refusal, non-advancing revision, 5xx uncertainty wording, base-revn preservation). Penpot suites: 207/207. Full `npm test`: 333/334, the single failure pre-existing and unrelated (untouched `package.json`/`scripts/local-install.test.mjs`). The `AGENTS.md:23` finding concerns the branch name/commit subject/PR title, which cannot be changed here without forbidden git operations (commit/branch). Left unresolved for the pipeline. # Review Findings — 195 ## Summary All code findings were fixed in a single pass: `commitFailure` now distinguishes provably-rejected categories from possibly-applied ones (`transport`/`server`/`unexpected`); the designated-target guard `assertCommitTargetAllowed` now enforces `isTargetAuthorized`; the commit no longer overwrites the staged base `revn`; `verifyStagedChanges` uses an explicit discriminated `switch` with a `default` that reports an unverifiable change as missing; `vern-conflict` is retried like `revn-conflict`; and a verified commit now requires the re-read revision to advance past `writeRevn`. Five focused tests were added (penpot suites 207/207; full `npm test` 333/334, the one failure pre-existing on main and untouched by this diff). The remaining unresolved item is the branch/commit-naming finding, which requires forbidden git operations (committing/amending a branch) and is the pipeline's responsibility before the MR is opened. ## Critical ## High - [x] `extensions/penpot/src/tools/commit.ts:252` — `commitFailure` is used for **every** non-`revn-conflict` failure, including `transport` (timeout / abort / network error) and `server` (5xx) categories. Its text asserts "was rejected by Penpot" and "Nothing was written to Penpot; the staged change remains staged", which the extension cannot know for those categories — the request may have reached the server and been applied, so a caller that retries or discards can leave a duplicate/leftover write. Suggested fix: branch on `error.category` in `commitFailure`; for `transport`, `server` and `unexpected`, emit the same uncertainty wording already used at `extensions/penpot/src/tools/commit.ts:449` ("the write may or may not have been applied; re-read the file with penpot_get_file before retrying or discarding") instead of the false "Nothing was written" claim. - [x] `extensions/penpot/src/tools/commit.ts:104` — the designated-target guard `assertCommitTargetAllowed` is a deliberate no-op, so the only write path will write to **any** file id the token can edit (including files the user never named). This is the `PENPOT_SKILL_DESIGN.md` §9 / plan-M2 write-safety item, and the store already carries the authorisation (`authorizeTarget`/`isTargetAuthorized`, populated by the staging tools at `extensions/penpot/src/tools/tokens.ts:288`); only the commit call site ignores it. Suggested fix: implement the policy here (at minimum `if (!isTargetAuthorized(fileId)) throw new Error(...)`) so a file the user did not name/stage can never be written; if it stays deferred, add a test that pins the deferral so the placeholder cannot ship unnoticed. ## Medium - [x] `extensions/penpot/src/tools/commit.ts:401` — the commit overwrites the store's base revision (`state.revn = writeRevn;`, repeated at `:439` on a conflict refresh), but `Changeset.revn` is documented as "the base revision observed when staging began" (`extensions/penpot/src/changeset.ts:465`) and "never overwritten" (`extensions/penpot/src/changeset.ts:493`). After any failed commit, `penpot_status` and `dry_run` therefore report a "Base revn" (`extensions/penpot/src/changeset.ts:671`) that is not the staging base, which is misleading about how far the file has drifted. Suggested fix: keep the live revision in the local `writeRevn`/`writeVern` variables only (drop both `state.revn = writeRevn` assignments) or add an explicit `lastWriteRevn` field so the reported base stays the staging base. ## Low - [x] `extensions/penpot/src/commit.ts:80` — the doc comment claims "a change that is not yet verifiable by this function … is treated as missing rather than assumed applied", but the code after the `add-color` branch unconditionally reads `change.typography`, so a third `ChangesetChange` member would be a compile error (or, with a widened type, would be mis-verified as a typography), not a `missing` entry. Suggested fix: make the discrimination explicit (`switch (change.type)` with a `default` branch that pushes an unverifiable/missing entry, and a `never` exhaustiveness check), or correct the comment to match the actual closed-union behaviour. - [x] `extensions/penpot/src/tools/commit.ts:418` — only `revn-conflict` is refreshed and retried; a `vern-conflict` (design §7: `vern ≠ stored → vern-conflict`) is reported as a generic rejection ("fix and retry") even though the retry path already re-reads `vern` and would very likely succeed on the next attempt. Suggested fix: treat `vern-conflict` as retryable by the same `isRevnConflict`-style predicate so the refresh-and-retry budget covers it too. - [x] `extensions/penpot/src/tools/commit.ts:462` — a verified commit reports `Revision: previousRevn → newRevn` without checking that `after.value.revn` actually advanced past `writeRevn`; a re-read returning an unchanged revision is still presented as a committed revision transition, and the design's validation criterion ("the file's revision advanced exactly once") is not asserted anywhere. Suggested fix: add `after.value.revn > writeRevn` to the verification result (and fail/flag when it does not hold), or word the line so a non-advance is not implied. - [ ] `AGENTS.md:23` — the branch `feature/issue-195/implement-penpot-commit-with-revn-tracking-conflic` has a truncated kebab slug, and the commit subject is `195: Implement penpot_commit…` rather than the required `issue-<N>: <summary>` form (`AGENTS.md:26`). Suggested fix: use the full title-derived slug (`…-conflict-retry-and-commit-preview`) and ensure the squashed commit / PR title use the `issue-195: <title>` form before the MR is opened. ## Documentation Updated files: - docs/PENPOT_SKILL_DESIGN.md - docs/PENPOT_SKILL_IMPLEMENTATION_PLAN.md

david added 3 commits

2026-09-15 00:49:09 +00:00