issue-201: Validate a full composed screen (one commit, one revision) against the live instance #234

Merged

david merged 2 commits from feature/issue-201/validate-a-full-composed-screen-one-commit-one-rev into main

2026-09-15 04:01:35 +00:00

david commented

2026-09-15 04:01:35 +00:00

Owner

201: Validate a full composed screen (one commit, one revision) against the live instance

Closes #201 — 201.

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 16m 30s.

Review findings

I'll start by exploring the repository structure and reading the relevant files.Baseline confirmed: 501/502 pre-existing failure (mongodb manifest) and the stub harness passes. Now let me implement the fixes, starting with the Medium shared-fixture extraction.Now update composedScreen.test.ts to consume the shared model:Now the High finding — relabel the UI-observable rows — plus surfacing the environment block:All fixes are in and the gates have been re-run. Final state before the annotated findings:

npm test → stub harness ALL CHECKS PASSED, then 504 tests: 503 pass, 1 fail — the same pre-existing root package.json manifest … mongodb is declared failure seen on main (it only inspects pkg.pi.extensions/pkg.dependencies, untouched by my scripts.test change).
Penpot suite alone → 377/377 pass (was 375; +2 new placement.test.ts cases).
No build/lint command is configured for this repo (no tsconfig/lint for the penpot extension).

Review Findings — 201

Summary

The code change is correct and well-tested offline: assertFrameParent now resolves a parent staged earlier in the same changeset, all four composition tools pass changeset.changes through, four regression tests plus a new end-to-end composition test were added. Gates run: npm test → 501/502 pass (the single failure, root package.json manifest … mongodb is declared, is pre-existing on main — package.json is untouched by this branch); the penpot suite → 375/375 pass; the offline harness node extensions/penpot/scripts/live-write-stub.mjs → ALL CHECKS PASSED (exit 0, check 6 stages 24 shapes and commits in one revision). There is no build or lint command configured in this repo. The material gap is that the issue's actual deliverable — validation against the live instance — was never performed (no PENPOT_URL/PENPOT_TOKEN in the environment), so the UI-observable acceptance criteria remain unverified.

Remediation: the High finding (rows 3–5 mis-read as passes) is fixed by relabelling them NOT VERIFIED (offline payload only — live UI check outstanding), and the section now carries an explicit environment-blocked banner; both Medium findings are fixed (the stub server model is extracted to src/tools/fixtures/penpotStubModel.ts and shared by the test and the harness, and npm test now runs the stub harness); both Low findings are fixed (CHANGELOG section order, and a new src/placement.test.ts pinning the page scope of the staged-parent lookup with a positive same-page case and a negative cross-page case). The Critical finding remains unresolved: the instance answers 200 on its OpenAPI doc, but no token/credentials exist in this environment (checked env, .env, docs/reference/, pi session state), so the live commit, the recorded live revn, and the UI observations could not be produced — this is recorded as environment-blocked in extensions/penpot/findings.md, not as a pass.

Critical

extensions/penpot/findings.md:484 — The issue requires the composed screen to be validated against the live Penpot instance (checks 1–6, "Manual, against the live instance"), but the screen was never committed there: findings.md itself records "the screen was never committed to the live instance and none of the UI checks could be observed". Acceptance criteria "Layers … named as requested" (check 2/3 in the test plan), "fills … linked … editing the library colour restyles the screen" (AC 5) and "cards frame reflows … layout panel shows the staged gap and padding" (AC 6) are therefore unmet, and AC "at least 15 shapes commit in one update-file … revn advancing exactly once" is only shown against a self-authored stub, never the server. Suggested fix: run the harness / pi tool calls with real credentials against the scratch project (PENPOT_URL=… PENPOT_TOKEN=… PENPOT_SCRATCH_FILE_ID=… node extensions/penpot/scripts/live-write-check.mjs), record the live revn before/after, the Layers/Layout/text-edit/linked-swatch observations and any failure in findings.md, and fix any primitive defect the live run exposes. If credentials cannot be provisioned by the pipeline, the issue must be surfaced as environment-blocked — merging it would present offline stub evidence as the live validation the issue exists to provide.

High

extensions/penpot/findings.md:565 — Checks 3, 4 and 5 are reported in the results table as PASS (payload) even though each acceptance criterion is explicitly UI-observable and marked "UI typing not run" / "UI edit/reload was not run" / "UI resize was not run". The issue instructs "Do not phrase a gap as a success", and a reader (or a later pipeline stage) can mistake these stub-payload rows for the acceptance criteria passing. Suggested fix: relabel rows 3–5 as NOT VERIFIED (offline payload only — live UI check outstanding) (or move them out of the PASS column) so the table cannot be read as a live pass.

Medium

extensions/penpot/src/composedScreen.test.ts:60 — The new test duplicates a hand-rolled model of Penpot's server behaviour (REQUIRED_SHAPE_KEYS, rootFrame, applyAddObject, fileBody) that already exists in extensions/penpot/scripts/live-write-stub.mjs:55 (same constants and function at :72/:136). Two copies of the server model will drift, and a drift would make the composed-screen evidence unsound in one place only. Suggested fix: extract the stub's state/applyAddObject/required-key validation into a single shared fixture module (e.g. under src/tools/fixtures/ or scripts/) and import it from both the test and the harness.
extensions/penpot/scripts/live-write-check.mjs:655 — Check 6 is only runnable by manually invoking the harness; npm test (the repo's only gate) never executes live-write-check.mjs, so the composition path is regression-covered solely by composedScreen.test.ts and the harness can silently rot (findings.md notes check 5 had already gone stale undetected). Suggested fix: either wire a smoke run of live-write-stub.mjs into npm test (it is hermetic and exits non-zero on failure), or delete the redundant harness coverage in favour of the in-suite test so there is one maintained path.

Low

CHANGELOG.md:35 — The new ### Fixed section was inserted before ### Changed, but the file declares "based on Keep a Changelog", whose canonical order is Added → Changed → Deprecated → Removed → Fixed → Security. Suggested fix: move the ### Fixed block below ### Changed within [Unreleased].
extensions/penpot/src/placement.ts:59 — findStagedShape scans the whole changeset per call and returns the first matching obj.id without checking the staged change is itself still a frame on the target page beyond change.pageId; this is fine for today's single-active-file/linear staging, but the behaviour ("parent may exist only in the active changeset") is not covered by a unit test for the negative cross-page case (a staged frame on page A used as parent while targeting page B). Suggested fix: add a small placement.test.ts case asserting a staged parent on a different pageId is rejected, so the page-scoping of the new lookup is pinned.

## 201: Validate a full composed screen (one commit, one revision) against the live instance Closes #201 — [201](https://git.excelera.net/david/pi-extensions-and-skills/issues/201). > 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 16m 30s. ## Review findings I'll start by exploring the repository structure and reading the relevant files.Baseline confirmed: 501/502 pre-existing failure (`mongodb` manifest) and the stub harness passes. Now let me implement the fixes, starting with the Medium shared-fixture extraction.Now update `composedScreen.test.ts` to consume the shared model:Now the High finding — relabel the UI-observable rows — plus surfacing the environment block:All fixes are in and the gates have been re-run. Final state before the annotated findings: - `npm test` → stub harness `ALL CHECKS PASSED`, then 504 tests: 503 pass, 1 fail — the same pre-existing `root package.json manifest … mongodb is declared` failure seen on `main` (it only inspects `pkg.pi.extensions`/`pkg.dependencies`, untouched by my `scripts.test` change). - Penpot suite alone → 377/377 pass (was 375; +2 new `placement.test.ts` cases). - No build/lint command is configured for this repo (no tsconfig/lint for the penpot extension). # Review Findings — 201 ## Summary The code change is correct and well-tested offline: `assertFrameParent` now resolves a parent staged earlier in the same changeset, all four composition tools pass `changeset.changes` through, four regression tests plus a new end-to-end composition test were added. Gates run: `npm test` → 501/502 pass (the single failure, `root package.json manifest … mongodb is declared`, is **pre-existing on `main`** — `package.json` is untouched by this branch); the penpot suite → 375/375 pass; the offline harness `node extensions/penpot/scripts/live-write-stub.mjs` → ALL CHECKS PASSED (exit 0, check 6 stages 24 shapes and commits in one revision). There is no build or lint command configured in this repo. The material gap is that the issue's actual deliverable — validation **against the live instance** — was never performed (no `PENPOT_URL`/`PENPOT_TOKEN` in the environment), so the UI-observable acceptance criteria remain unverified. Remediation: the High finding (rows 3–5 mis-read as passes) is fixed by relabelling them `NOT VERIFIED (offline payload only — live UI check outstanding)`, and the section now carries an explicit environment-blocked banner; both Medium findings are fixed (the stub server model is extracted to `src/tools/fixtures/penpotStubModel.ts` and shared by the test and the harness, and `npm test` now runs the stub harness); both Low findings are fixed (CHANGELOG section order, and a new `src/placement.test.ts` pinning the page scope of the staged-parent lookup with a positive same-page case and a negative cross-page case). The Critical finding remains unresolved: the instance answers `200` on its OpenAPI doc, but no token/credentials exist in this environment (checked `env`, `.env`, `docs/reference/`, pi session state), so the live commit, the recorded live `revn`, and the UI observations could not be produced — this is recorded as environment-blocked in `extensions/penpot/findings.md`, not as a pass. ## Critical - [ ] `extensions/penpot/findings.md:484` — The issue requires the composed screen to be validated **against the live Penpot instance** (checks 1–6, "Manual, against the live instance"), but the screen was never committed there: findings.md itself records "the screen was **never committed to the live instance** and none of the UI checks could be observed". Acceptance criteria "Layers … named as requested" (check 2/3 in the test plan), "fills … linked … editing the library colour restyles the screen" (AC 5) and "cards frame reflows … layout panel shows the staged gap and padding" (AC 6) are therefore unmet, and AC "at least 15 shapes commit in one `update-file` … revn advancing exactly once" is only shown against a self-authored stub, never the server. Suggested fix: run the harness / pi tool calls with real credentials against the scratch project (`PENPOT_URL=… PENPOT_TOKEN=… PENPOT_SCRATCH_FILE_ID=… node extensions/penpot/scripts/live-write-check.mjs`), record the live `revn` before/after, the Layers/Layout/text-edit/linked-swatch observations and any failure in findings.md, and fix any primitive defect the live run exposes. If credentials cannot be provisioned by the pipeline, the issue must be surfaced as environment-blocked — merging it would present offline stub evidence as the live validation the issue exists to provide. ## High - [x] `extensions/penpot/findings.md:565` — Checks 3, 4 and 5 are reported in the results table as `PASS (payload)` even though each acceptance criterion is explicitly UI-observable and marked "UI typing not run" / "UI edit/reload was **not run**" / "UI resize was **not run**". The issue instructs "Do not phrase a gap as a success", and a reader (or a later pipeline stage) can mistake these stub-payload rows for the acceptance criteria passing. Suggested fix: relabel rows 3–5 as `NOT VERIFIED (offline payload only — live UI check outstanding)` (or move them out of the PASS column) so the table cannot be read as a live pass. ## Medium - [x] `extensions/penpot/src/composedScreen.test.ts:60` — The new test duplicates a hand-rolled model of Penpot's server behaviour (`REQUIRED_SHAPE_KEYS`, `rootFrame`, `applyAddObject`, `fileBody`) that already exists in `extensions/penpot/scripts/live-write-stub.mjs:55` (same constants and function at `:72`/`:136`). Two copies of the server model will drift, and a drift would make the composed-screen evidence unsound in one place only. Suggested fix: extract the stub's state/`applyAddObject`/required-key validation into a single shared fixture module (e.g. under `src/tools/fixtures/` or `scripts/`) and import it from both the test and the harness. - [x] `extensions/penpot/scripts/live-write-check.mjs:655` — Check 6 is only runnable by manually invoking the harness; `npm test` (the repo's only gate) never executes `live-write-check.mjs`, so the composition path is regression-covered solely by `composedScreen.test.ts` and the harness can silently rot (findings.md notes check 5 had already gone stale undetected). Suggested fix: either wire a smoke run of `live-write-stub.mjs` into `npm test` (it is hermetic and exits non-zero on failure), or delete the redundant harness coverage in favour of the in-suite test so there is one maintained path. ## Low - [x] `CHANGELOG.md:35` — The new `### Fixed` section was inserted before `### Changed`, but the file declares "based on Keep a Changelog", whose canonical order is Added → Changed → Deprecated → Removed → Fixed → Security. Suggested fix: move the `### Fixed` block below `### Changed` within `[Unreleased]`. - [x] `extensions/penpot/src/placement.ts:59` — `findStagedShape` scans the whole changeset per call and returns the first matching `obj.id` without checking the staged change is itself still a frame on the target page beyond `change.pageId`; this is fine for today's single-active-file/linear staging, but the behaviour ("parent may exist only in the active changeset") is not covered by a unit test for the negative cross-page case (a staged frame on page A used as parent while targeting page B). Suggested fix: add a small `placement.test.ts` case asserting a staged parent on a different `pageId` is rejected, so the page-scoping of the new lookup is pinned.

david added 2 commits

2026-09-15 04:01:35 +00:00