issue-196: Implement the designated-target write guard with an allow/refuse matrix #229

Merged

david merged 3 commits from feature/issue-196/implement-the-designated-target-write-guard-with-a into main

2026-09-15 02:43:26 +00:00

david commented

2026-09-15 02:43:25 +00:00

Owner

196: Implement the designated-target write guard with an allow/refuse matrix

Closes #196 — 196.

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 12m 23s.

Review findings

Now let me look at the design doc §9 and Control row line numbers, plus library/tokens imports precisely.Let me create the shared constants module and start editing.All penpot tests pass (230/230). The full npm test shows only the pre-existing, unrelated scripts/local-install.test.mjs mongodb-manifest failure (package.json is untouched by this change).

Review Findings — 196

Summary

All seven findings were resolved in this pass.

High (project-resolution error reporting): resolveFileProject now returns a discriminated ProjectResolution (ok / read-failed + decoded message / no-project), and isWriteAllowed carries the decoded server/transport message into the fail-closed hint, which now leads with "could not be read" and the real remedy (connection/credentials) rather than misreporting auth/config failures as a designation problem.
Medium (created provenance docs): the registerCreatedFile doc, README write-safety section and CHANGELOG now state the provenance is reserved for the not-yet-shipped create/import path and is currently only exercised by tests.
Medium (commit description): the penpot_commit description now states the allow-list precondition and refusal behaviour.
Low (created-file gating divergence): documented that an extension-created file is authorised by construction and its recorded project is informational; aligned docs/PENPOT_SKILL_DESIGN.md §9 to match.
Low (missing allow test): added an end-to-end test where a nominated project resolves and the commit proceeds (update-file sent, verified === true).
Low (duplicated constants): extracted GET_FILE_COMMAND/FILE_ID_QUERY_KEY into src/fileCommand.ts and imported them from all five call sites (guard, commit, file, library, tokens).
Low (design inventory): added authorize_target to the Control row and a session-scoping line to §9.

Gates: no build/lint command is configured (no tsconfig, eslint/prettier config, or build script), so npm test was the only runnable gate. Penpot suites are 230/230 green (two tests added). The full run is 356/357, the single failure being the pre-existing, unrelated scripts/local-install.test.mjs:1875 mongodb-manifest assertion (package.json is untouched by this diff).

Critical

(none)

High

extensions/penpot/src/guard.ts:303 — resolveFileProject collapses every failure (!result.ok: config error, 401/403, timeout, 5xx) and a body with no project field into undefined, so isWriteAllowed reports project-resolution-failed ("the file's project could not be determined") and the hint tells the agent to call penpot_authorize_target. An expired PENPOT_TOKEN or a missing PENPOT_URL is thus misreported as a designation problem with a wrong remedy — contrary to the module's own documented convention (client.ts:9-11: "The API is internal and unversioned, so the client surfaces the server's own error payload rather than inventing messages"; penpotRequest returns categorised errors "so callers decide how to present them"). Suggested fix: return a discriminated result from resolveFileProject (e.g. { ok: true, projectId } | { ok: false, reason: "read-failed", message } | { ok: false, reason: "no-project" }) and have isWriteAllowed carry the decoded server/transport message into the project-resolution-failed hint, or at minimum pre-empt it in commit.ts when penpotRequest reports config/auth before the guard runs.

Medium

extensions/penpot/src/guard.ts:73 — registerCreatedFile has no production caller anywhere in the extension (only guard.test.ts calls it), and no create-file path exists (penpot_list_library is read-only; import-binfile/create-file are unimplemented M4 work). The "created" provenance therefore can never match in a real session, yet the docs assert it is live: guard.ts:70 ("Called by every create-file path (currently library import…)"), extensions/penpot/README.md:33 ("the extension created it this session (currently via library import, and any future create-file path)") and CHANGELOG.md:13. A reader concludes imports are auto-authorised when nothing can register a created file. Suggested fix: reword to state the provenance is reserved for the not-yet-shipped create/import path (which will call registerCreatedFile at creation time), and drop "currently via library import"; alternatively wire the call when that tool lands, and keep the unit test as the contract.
extensions/penpot/src/tools/commit.ts:421 — the penpot_commit tool description (the model-facing contract) documents the revn protocol, retry and dry_run in detail but never mentions the new precondition that the target must be on the session allow-list, so the agent discovers the guard only by being refused. Every other precondition in this extension is stated in its description. Suggested fix: add a sentence such as "The file must be on the session write allow-list (a file the extension created, a file the user named, or a file in a nominated project — recorded with penpot_authorize_target); otherwise the call is refused with nothing written and the changeset left staged."

Low

extensions/penpot/src/guard.ts:190 — a file registered via registerCreatedFile(fileId, projectId) is allowed by bare id membership; the recorded projectId is never consulted by the decision (only rendered in guardSnapshot), which diverges from the design's wording in docs/PENPOT_SKILL_DESIGN.md:200 ("files it created in a nominated project"). Suggested fix: either gate the created branch on the recorded project being nominated (matching the design), or state in the module doc and README that an extension-created file is authorised by construction and the recorded project is informational only.
extensions/penpot/src/tools/commit.test.ts:670 — the only commit-level test using a nominated project asserts the fail-closed refusal; there is no end-to-end test where resolveFileProject resolves a matching project and the commit proceeds, so the wiring at commit.ts:472-474 is only proven to refuse, never to allow. Suggested fix: add a test that nominates PROJECT_ID, serves a get-file body carrying projectId: PROJECT_ID (then the revn/verify reads) and asserts the update-file call happened and details.verified === true.
extensions/penpot/src/guard.ts:30 — GET_FILE_COMMAND/FILE_ID_QUERY_KEY duplicate the same constants in tools/commit.ts:68,73 and tools/file.ts:35,41; three copies of the same RPC contract must now stay in sync. Suggested fix: export them once (e.g. from client.ts or a small fileCommand.ts) and import in all three call sites.
docs/PENPOT_SKILL_DESIGN.md:214 — the primitive inventory's Control row still reads commit, status, discard; the new penpot_authorize_target (the mechanism §9's write-safety rule depends on) is missing, as is any mention of the allow-list recording step in §9. Suggested fix: add authorize_target to the Control group and one line to §9 noting that designations are recorded by that tool and are session-scoped.

Documentation

Updated files:

docs/PENPOT_SKILL_IMPLEMENTATION_PLAN.md

## 196: Implement the designated-target write guard with an allow/refuse matrix Closes #196 — [196](https://git.excelera.net/david/pi-extensions-and-skills/issues/196). > 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 12m 23s. ## Review findings Now let me look at the design doc §9 and Control row line numbers, plus library/tokens imports precisely.Let me create the shared constants module and start editing.All penpot tests pass (230/230). The full `npm test` shows only the pre-existing, unrelated `scripts/local-install.test.mjs` mongodb-manifest failure (package.json is untouched by this change). # Review Findings — 196 ## Summary All seven findings were resolved in this pass. - **High (project-resolution error reporting):** `resolveFileProject` now returns a discriminated `ProjectResolution` (`ok` / `read-failed` + decoded message / `no-project`), and `isWriteAllowed` carries the decoded server/transport message into the fail-closed hint, which now leads with "could not be read" and the real remedy (connection/credentials) rather than misreporting auth/config failures as a designation problem. - **Medium (created provenance docs):** the `registerCreatedFile` doc, README write-safety section and CHANGELOG now state the provenance is reserved for the not-yet-shipped create/import path and is currently only exercised by tests. - **Medium (commit description):** the `penpot_commit` description now states the allow-list precondition and refusal behaviour. - **Low (created-file gating divergence):** documented that an extension-created file is authorised by construction and its recorded project is informational; aligned `docs/PENPOT_SKILL_DESIGN.md` §9 to match. - **Low (missing allow test):** added an end-to-end test where a nominated project resolves and the commit proceeds (`update-file` sent, `verified === true`). - **Low (duplicated constants):** extracted `GET_FILE_COMMAND`/`FILE_ID_QUERY_KEY` into `src/fileCommand.ts` and imported them from all five call sites (guard, commit, file, library, tokens). - **Low (design inventory):** added `authorize_target` to the Control row and a session-scoping line to §9. **Gates:** no build/lint command is configured (no tsconfig, eslint/prettier config, or build script), so `npm test` was the only runnable gate. Penpot suites are **230/230** green (two tests added). The full run is **356/357**, the single failure being the pre-existing, unrelated `scripts/local-install.test.mjs:1875` mongodb-manifest assertion (package.json is untouched by this diff). ## Critical - [x] (none) ## High - [x] extensions/penpot/src/guard.ts:303 — `resolveFileProject` collapses **every** failure (`!result.ok`: config error, 401/403, timeout, 5xx) and a body with no project field into `undefined`, so `isWriteAllowed` reports `project-resolution-failed` ("the file's project could not be determined") and the hint tells the agent to call `penpot_authorize_target`. An expired `PENPOT_TOKEN` or a missing `PENPOT_URL` is thus misreported as a designation problem with a wrong remedy — contrary to the module's own documented convention (`client.ts:9-11`: "The API is internal and unversioned, so the client surfaces the server's own error payload rather than inventing messages"; `penpotRequest` returns categorised errors "so callers decide how to present them"). Suggested fix: return a discriminated result from `resolveFileProject` (e.g. `{ ok: true, projectId } | { ok: false, reason: "read-failed", message } | { ok: false, reason: "no-project" }`) and have `isWriteAllowed` carry the decoded server/transport message into the `project-resolution-failed` hint, or at minimum pre-empt it in `commit.ts` when `penpotRequest` reports `config`/`auth` before the guard runs. ## Medium - [x] extensions/penpot/src/guard.ts:73 — `registerCreatedFile` has no production caller anywhere in the extension (only `guard.test.ts` calls it), and no create-file path exists (`penpot_list_library` is read-only; `import-binfile`/`create-file` are unimplemented M4 work). The "created" provenance therefore can never match in a real session, yet the docs assert it is live: `guard.ts:70` ("Called by every create-file path (currently library import…)"), `extensions/penpot/README.md:33` ("the extension **created** it this session (currently via library import, and any future create-file path)") and `CHANGELOG.md:13`. A reader concludes imports are auto-authorised when nothing can register a created file. Suggested fix: reword to state the provenance is reserved for the not-yet-shipped create/import path (which will call `registerCreatedFile` at creation time), and drop "currently via library import"; alternatively wire the call when that tool lands, and keep the unit test as the contract. - [x] extensions/penpot/src/tools/commit.ts:421 — the `penpot_commit` tool description (the model-facing contract) documents the `revn` protocol, retry and `dry_run` in detail but never mentions the new precondition that the target must be on the session allow-list, so the agent discovers the guard only by being refused. Every other precondition in this extension is stated in its description. Suggested fix: add a sentence such as "The file must be on the session write allow-list (a file the extension created, a file the user named, or a file in a nominated project — recorded with `penpot_authorize_target`); otherwise the call is refused with nothing written and the changeset left staged." ## Low - [x] extensions/penpot/src/guard.ts:190 — a file registered via `registerCreatedFile(fileId, projectId)` is allowed by bare id membership; the recorded `projectId` is never consulted by the decision (only rendered in `guardSnapshot`), which diverges from the design's wording in `docs/PENPOT_SKILL_DESIGN.md:200` ("files it **created in a nominated project**"). Suggested fix: either gate the created branch on the recorded project being nominated (matching the design), or state in the module doc and README that an extension-created file is authorised by construction and the recorded project is informational only. - [x] extensions/penpot/src/tools/commit.test.ts:670 — the only commit-level test using a nominated project asserts the fail-closed refusal; there is no end-to-end test where `resolveFileProject` resolves a matching project and the commit proceeds, so the wiring at `commit.ts:472-474` is only proven to refuse, never to allow. Suggested fix: add a test that nominates `PROJECT_ID`, serves a `get-file` body carrying `projectId: PROJECT_ID` (then the revn/verify reads) and asserts the `update-file` call happened and `details.verified === true`. - [x] extensions/penpot/src/guard.ts:30 — `GET_FILE_COMMAND`/`FILE_ID_QUERY_KEY` duplicate the same constants in `tools/commit.ts:68,73` and `tools/file.ts:35,41`; three copies of the same RPC contract must now stay in sync. Suggested fix: export them once (e.g. from `client.ts` or a small `fileCommand.ts`) and import in all three call sites. - [x] docs/PENPOT_SKILL_DESIGN.md:214 — the primitive inventory's Control row still reads `commit`, `status`, `discard`; the new `penpot_authorize_target` (the mechanism §9's write-safety rule depends on) is missing, as is any mention of the allow-list recording step in §9. Suggested fix: add `authorize_target` to the Control group and one line to §9 noting that designations are recorded by that tool and are session-scoped. ## Documentation Updated files: - docs/PENPOT_SKILL_IMPLEMENTATION_PLAN.md

david added 3 commits

2026-09-15 02:43:25 +00:00