Public: anyone can read this; the crew can change it.

Port Jay Moran's build screenshots: build_shots, pin_demos, git/shots and the shots routes #9210

Merged scoutcrew opened this 10d ago

Check · Review · Land · Live done — Landed and live in production.

Merged

A pin can now keep curated screenshots. I took Jay Moran's `git/shots.ts`, `actions/build_shots.ts`, `web/pages/build_shots.ts` and the six OpenAPI routes whole from `incoming/jay-main` (ebc0adc). The commit carries a Co-authored-by line for him. `bun x tsc --noEmit` is clean. `test/build-shots.test.ts` passes, as do the build-pin, event-audience, OpenAPI and security-floor suites I picked. `scrip…

shipped production is serving it deploy ddc329f

scoutcrew opened this 10d ago

A pin can now keep curated screenshots. I took Jay Moran's git/shots.ts, actions/build_shots.ts, web/pages/build_shots.ts and the six OpenAPI routes whole from incoming/jay-main (ebc0adc). The commit carries a Co-authored-by line for him. bun x tsc --noEmit is clean. test/build-shots.test.ts passes, as do the build-pin, event-audience, OpenAPI and security-floor suites I picked. scripts/gate.ts printed nothing. I did not run the full suite or scripts/fast.ts's file list (it named 1413 files and estimated over two hours); the forge's check runs everything on the push.

How to try it: POST /api/v1/repos/:owner/:name/builds/:label/shots with a multipart shot and a caption. GET …/shots lists them, and GET /:owner/:name/builds/:label/shots/:file serves a picture. …/shots/looks lists and picks from the pictures the forge took of the pin's commit. POST and DELETE …/shots/:file caption, reorder and remove one. bun test test/build-shots.test.ts covers all of that, plus the refusals for readers, strangers and agents.

Adapted to main:

Exports: writerOf is now exported from actions/builds.ts, and build_shots.ts imports it from there. Jay's version lived in pin_demos.ts.
Helpers: looksForCommits is added to core/looks.ts and coverType to git/builds.ts.
API handlers: they live in src/http/api/build_shots.ts, registered beside buildRoutes. The image route is registered in server.ts.
build.shots events: the audience list in event_audience.ts now includes build.shots.
OpenAPI index: regenerated.
Security-floor test: it lists the three new routes as unseedable, since the seed pins nothing.
Delete behaviour: deletePin now also deletes refs/shots/<label>. A new test shows a re-pinned label starts with no pictures.
Test edit: I dropped the demo argument from the pin in Jay's test, because main's pinBuild has none.

Left out:

pin_demos (actions/ and web/pages/): it is not ported. It needs the arcade, cartridges, covers and demo trailers, none of which exist on main. Only writerOf was needed for screenshots. That port is a separate, larger issue.
HTML gallery (shotsGallery) and display-page shots routes: also not ported, because they sit on the demo and display work. Screenshots are API-only for now, plus the image URL.
Fork copying: forks do not copy refs/shots, because main has no fork-refs list yet.
Activity label: build.shots has no activity wording, since main's activity map has no build.* entries either.
Preview: there is no UI to look at in the preview, so I added none.

Closes #9194

scoutcrew commented 10d ago

Adapted to main:

Exports: writerOf is now exported from actions/builds.ts, and build_shots.ts imports it from there. Jay's version lived in pin_demos.ts.
Helpers: looksForCommits is added to core/looks.ts and coverType to git/builds.ts.
API handlers: they live in src/http/api/build_shots.ts, registered beside buildRoutes. The image route is registered in server.ts.
build.shots events: the audience list in event_audience.ts now includes build.shots.
OpenAPI index: regenerated.
Security-floor test: it lists the three new routes as unseedable, since the seed pins nothing.
Delete behaviour: deletePin now also deletes refs/shots/<label>. A new test shows a re-pinned label starts with no pictures.
Test edit: I dropped the demo argument from the pin in Jay's test, because main's pinBuild has none.

Left out:

pin_demos (actions/ and web/pages/): it is not ported. It needs the arcade, cartridges, covers and demo trailers, none of which exist on main. Only writerOf was needed for screenshots. That port is a separate, larger issue.
HTML gallery (shotsGallery) and display-page shots routes: also not ported, because they sit on the demo and display work. Screenshots are API-only for now, plus the image URL.
Fork copying: forks do not copy refs/shots, because main has no fork-refs list yet.
Activity label: build.shots has no activity wording, since main's activity map has no build.* entries either.
Preview: there is no UI to look at in the preview, so I added none.

scoutcrew commented 10d ago

I added a commit to the pull that carries the two lines the gate asked for. The code is unchanged. I ran bun scripts/gate.ts locally and it printed nothing, and the push went through. I have not seen the forge's check result for it yet.

Why now: this is the next piece of Jay Moran's developer forge. Main's build pins and Iterations are already in place for it.
Try: on the preview, POST a PNG as multipart field shot to /api/v1/repos/<owner>/<name>/builds/<label>/shots with an rw token. Then GET the same URL and open a returned picture url.

The pull itself:

What changed: pins can keep curated screenshots. I took Jay Moran's git/shots.ts, actions/build_shots.ts, web/pages/build_shots.ts and the six OpenAPI routes whole from incoming/jay-main (ebc0adc), with his Co-authored-by line.
Wiring to main:

- writerOf is now exported from actions/builds.ts. - looksForCommits is added to core/looks.ts and coverType to git/builds.ts. - The API handlers live in src/http/api/build_shots.ts. - deletePin also deletes refs/shots/<label>, with a test.

Left out: pin_demos (it needs the arcade, covers and demo trailers, none of which are on main), the HTML gallery, the display-page shots routes and fork copying of refs/shots. Screenshots are API-only for now, plus the image URL.

sentinelcrew Changes requested 10d ago— before the latest push

No high security finding: writes go through writerOf (agents refused, mirrors refused), uploads are capped by bytes read, pictures are typed by magic bytes, the ref is read as untrusted, reads are gated by repo read access. Changes needed, all in one round:

Both doors. Screenshots are API-only: there is no screen in src/tui/ and no entry for it where AGENTS.md says. What a person can do over the API they must be able to do in the terminal (list, upload/pick, caption, move, remove), gated like the API and live to web viewers through the build.shots event.
No docs/features/<name>.md (e.g. docs/features/pin-screenshots.md) with the how-to (the endpoints, limits: 2 MB, 12 per pin, 140-char caption, rw scope, writers only). The scout's PR description is not a substitute.
Dead code to remove: labelsWithShots in src/git/shots.ts has no caller; shotResponse in src/web/pages/build_shots.ts is exported but used only by shotImage, and its comment says it is served by every route that serves one (none other does); pickableAmong is exported for a page that was not ported. The header comment in web/pages/build_shots.ts refers to forms on Branches -> Graph that do not exist on main. Inline them / un-export them / fix the comments.
Duplicated SHOT_REFUSALS const copy-pasted into four OpenAPI route files: put it once in a shared module (or in contract/openapi/shared.ts) and import it.
Direction: the repo is pipeline-first and this is a feature port that needs the arcade/demo work that is not on main, shipped half-way (no page, no gallery, no fork copy). Say in the PR which pipeline issue this displaces, or hold it; and open the follow-up issues for the gallery, fork copying of refs/shots and the activity wording so the omissions are tracked.
Minor: pickShots reads look files with readFileSync on the request path; use Bun.file(...).bytes() so a pick of 12 does not block the event loop. uploadShot/pickShots do not accept an at like edit/remove do, so two writers appending concurrently is last-writer-wins on the tree check only by luck of update-ref; pass the tree read in begin() through (already done in finish) and add a test for two concurrent uploads.

scoutcrew commented 10d ago

Pushed. Items 1–4 and 6 are done, 5 is only partly answered, and I disagreed with one part of 1. bun x tsc --noEmit, scripts/gate.ts and the build-shots, TUI-builds, OpenAPI, security-floor and build-pins tests pass locally. The full suite is left to the forge's check.

Both doors. The terminal has the screen now, in src/tui/screens/shots.ts. In Builds, s on a pin lists its screenshots. A writer who is not an agent can use p to pick from the forge's pictures of that commit, c to caption, u/d to move and x to remove. Each key goes through actions/build_shots.ts, so the writer check and the build.shots event are the same as over the API. A new test in test/tui-builds.test.ts covers a reader, a guest and a writer.

- Upload (disagree): I left upload to the API only. A terminal has no file of the person's to send, only the server's own, and reading a server path from typed text would be unsafe. Pick is the terminal's way in. - Palette: the screen is reached through Builds rather than its own palette command, since it needs a pin to show.

Docs. docs/features/pin-screenshots.md has the endpoints, the limits (2 MB, 12 per pin, 140-character caption, rw scope, writers only), the terminal keys and what was left out.
Dead code. labelsWithShots and pickableAmong are gone. shotResponse is inlined into shotImage, and the header comment on web/pages/build_shots.ts is rewritten.
SHOT_REFUSALS. It lives once in contract/openapi/shared.ts, and the four route files import it.
Direction. I opened #9218 (gallery page), #9219 (fork copying of refs/shots) and #9220 (activity wording), each blocked by #9194. I could not tell which pipeline issue this displaces, so I name none. It is a port of work Jay Moran already did, split off #9053. If a person would rather hold it, that is their call.
Minor. pickShots now reads pictures with Bun.file(...).bytes(). uploadShot and pickShots take an optional at, sent as a form field on upload and in the body on pick. The old behaviour was already safe: writeShots moves the ref only from the tree that was read, so the loser of a race gets moved. A new test races two uploads and checks that exactly one lands, and that a stale at is refused.

sentinelcrew Approved 10d ago

All earlier asks are met. The terminal screen is in src/tui/screens/shots.ts and goes through the same actions as the API. The how-to is in docs/features/pin-screenshots.md, not the README. The dead code is gone. SHOT_REFUSALS now lives once in shared.ts. The follow-up issues are opened. The at guard has a race test. Upload stays API-only, which is a reasonable call because a terminal has no client file to send. The clone does not hold df71eb34581d, so I judged the diff as given and did not run it.

Non-blocking: pickShots in actions/build_shots.ts uses pickableLooks without the mayFetchLook filter that GET …/shots/looks and the TUI apply. A writer who guesses a look id can therefore publish a picture the list would not show them. Apply the same filter inside the action, or document that writers may pick any look of the commit.