Skip to content

refactor: give the asset protocol one owner and unpin effect from the API - #41

Merged
altaywtf merged 1 commit into
mainfrom
fix/audit-round-2b
Sep 18, 2026
Merged

altaywtf merged 1 commit into
mainfrom
fix/audit-round-2b

Conversation

@altaywtf

Copy link
Copy Markdown
Member

The last four audit findings. No behaviour changes — the CLI does the same things it did before this PR.

Effect no longer reaches the published API

dist/index.d.mts opened with import { Schema } from "effect" and referenced it fourteen times, for exactly one reason: VrefScreenshotDraft was declared as typeof VrefScreenshotDraftSchema.Type. Every other public export is a plain Promise or string-returning function, so the runtime surface has no Effect coupling at all — but anyone importing that one type had to resolve the same release candidate this package pins, and Schema.optionalKey / Schema.NonEmptyString / Schema.$Array are precisely the names that move between Effect 4 RCs.

The draft shape is now written out in types.ts, and manifest.ts carries assignability assertions in both directions so the schema and the type cannot drift apart silently. dist/index.d.mts references Effect zero times.

The asset protocol has one owner

convert.ts imported writeAsset from screenshot-add.ts — a command module reaching into another command module for a path-safety primitive. readIfExists and readReplacedAsset were the same function under two names, down to an identical five-line comment. The "is this path already claimed" fold was a documented helper in one file and a different inline expression in the other; they agreed only because safeManifestAssetPath happens to normalise separators first.

The invariant underneath all of it — an interrupted write never leaves an asset the manifest does not describe — had no single owner and no single test target. It now lives in asset.ts. hasErrorCode was copied verbatim into four modules and messageFrom into five; both are in errors.ts.

Docs match the code

SOURCE_EXTENSIONS accepts .jpeg and describe reports four source formats; the README, guide, and skill all said three. An agent trusting the prose over describe would reject a valid capture before calling the CLI.

docs/DISTRIBUTION.md listed package contents predating CONTEXT.md, and linked ../.github/workflows/ci.yml and ../CONTRIBUTING.md from inside a doc that ships — neither file is ever in the tarball. Those are absolute now, with a line saying why.

Coverage is measured

Nothing reported it, though clean still deleted a coverage/ directory nothing produced. That is how four unexercised path-safety rejection branches survived a 73-test suite.

pnpm run coverage reports it. It is deliberately not in verify: a thin patch should fail review, not CI. Baseline is 84.98% of statements, 75.99% of branches, with serve.ts (72.5%) and cli.ts (80.2%) the thinnest.

The provider is pinned to 4.1.10 rather than a range — vite-plus refuses to run a coverage provider that does not exactly match its bundled vitest, since a mismatch silently produces unreliable numbers.

Verification

pnpm run verify exits 0: 37 files formatted, 19 lint-clean, 105 tests. The refactor is behaviour-preserving, so the existing failure-injection tests for both rollback paths are the proof — they passed before and after with no edits.

… API

Four audit findings, none behavioural.

The published declarations opened with `import { Schema } from "effect"`, with
fourteen references, for one reason: `VrefScreenshotDraft` was
`typeof Schema.Type`. Nothing else in the public surface touches Effect, so a
consumer of that type had to resolve the same release candidate as this package
for a runtime that does not care. The draft shape is written out in `types.ts`
and `manifest.ts` asserts assignability both ways, so the two cannot drift.
`dist/index.d.mts` now references Effect zero times.

`convert.ts` reached into `screenshot-add.ts` for `writeAsset`, a path-safety
primitive; `readIfExists` and `readReplacedAsset` were the same function under
two names down to an identical comment; and the claimed-path fold was written
one way in one file and inlined differently in the other. The invariant they
implement — an interrupted write never leaves an asset the manifest does not
describe — now has a single owner in `asset.ts`. `hasErrorCode` and
`messageFrom`, copied into four and five modules, live in `errors.ts`.

Three docs said sources may be png, jpg, or webp while the code and `describe`
both accept jpeg. The distribution doc listed package contents that no longer
matched `files`, and linked two files the tarball never contains.

Nothing measured coverage, which is how four unexercised rejection branches
survived a 73-test suite. `pnpm run coverage` reports it; `verify` does not run
it, so a thin patch fails review rather than CI. Current baseline is 84.98% of
statements and 75.99% of branches.
Copilot AI lite review requested due to automatic review settings September 18, 2026 13:12
@chatgpt-codex-connector

chatgpt-codex-connector Bot commented Sep 18, 2026 •

Copy link
Copy Markdown

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review Status Commit Review trigger
📝 Code Review ✅ Completed 2026-09-18T13:14:48.300580Z 731143b PR opened
ℹ️ 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" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟢 Approval recommended

The changes are cohesive refactors and documentation/config updates that preserve behavior while improving API decoupling and maintainability.

Pull request overview

This PR refactors shared “asset protocol” behavior into a single module, removes Effect type leakage from the published TypeScript declarations by hand-writing VrefScreenshotDraft, aligns docs with the actual supported source formats, and adds measurable test coverage reporting.

Changes:

  • Move asset write/read/restore + cross-platform path comparison into src/asset.ts and reuse it from commands.
  • Move VrefScreenshotDraft to src/types.ts and add compile-time assignability checks to keep the schema/type in sync.
  • Add Vitest coverage configuration + a pnpm run coverage script, and update docs/skill text and distribution links accordingly.
File summaries
File Description
vite.config.ts Enables coverage reporting for src/**/*.ts with text/html reporters.
test/vref.test.ts Updates type import location for VrefScreenshotDraft.
src/types.ts Adds hand-written VrefScreenshotDraft to avoid Effect in published d.ts.
src/serve.ts Centralizes messageFrom usage via errors.ts.
src/screenshot-add.ts Switches asset handling/path comparisons to asset.ts and shared error helpers.
src/path-safety.ts Uses shared hasErrorCode helper from errors.ts.
src/manifest.ts Imports VrefScreenshotDraft from types.ts and adds schema/type drift assertions; uses shared error helpers.
src/manifest-edit.ts Moves VrefScreenshotDraft type import to types.ts; uses shared error helpers.
src/index.ts Re-exports VrefScreenshotDraft from types.ts; stops re-exporting it from manifest.ts.
src/image.ts Uses shared messageFrom helper from errors.ts.
src/errors.ts Adds shared hasErrorCode + messageFrom utilities.
src/convert.ts Uses asset.ts for reads/writes and shared path normalization + error helpers.
src/asset.ts New single owner for asset write/read/restore and filesystem path comparison (pathKey).
skills/vref/SKILL.md Updates supported source formats to include .jpeg.
README.md Updates supported source formats to include .jpeg; fixes contributing link for packaged docs.
pnpm-lock.yaml Adds @vitest/coverage-v8@4.1.10 resolution and related dependency graph updates.
package.json Adds coverage script and pins @vitest/coverage-v8 dev dependency.
docs/VREF.md Updates supported source formats to include .jpeg.
docs/DISTRIBUTION.md Makes CI/contributing links absolute (since they don’t ship in tarball) and updates packaged contents list.
CONTRIBUTING.md Documents the new pnpm run coverage workflow and that it is review-gated, not CI-gated.
AGENTS.md Adds pnpm run coverage to the documented command list.
Review details

Files not reviewed (1)

  • pnpm-lock.yaml: Generated file
  • Files reviewed: 20/21 changed files
  • Comments generated: 0
  • Review effort level: Lite

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

@altaywtf
altaywtf merged commit 179eb38 into main Sep 18, 2026
3 checks passed
@altaywtf
altaywtf deleted the fix/audit-round-2b branch September 18, 2026 13:19
@putio-ci

putio-ci Bot commented Sep 18, 2026

Copy link
Copy Markdown
Contributor

🎉 This PR is included in version 1.4.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

@putio-ci putio-ci Bot added the released label Sep 18, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants