crueber/walhub

Watch

Fork 0

[feature] [plan] Should be able to paste an image in to the comments and description of issues. #120

New issue

Closed

opened 2026-09-05 05:03:21 +00:00 by crueber · 6 comments

crueber commented

2026-09-05 05:03:21 +00:00

Owner

Plan this out before you implement it. It's a bigger feature. You need to be able to accept a file dropped on to the textarea, or allow the user to hit the paste shortcut on their keyboard ctrl+v, or command+v. That should upload the file automatically, and insert a markdown style link to the image at the cursor. Given that it'll store as an object, it may need a pass through api to be able to pick up the image. As I said, make sure it's planned before implementing. Get a good review afterward too.

crueber commented

2026-09-05 06:34:29 +00:00

Author

Owner

Plan: paste/drop image upload in issue comments/descriptions (Forgejo #120)

0. Objective / scope

Objective: In the issue create form and the issue thread comment composer, a user can
paste (Ctrl+V / Cmd+V) or drag-drop an image file onto the <textarea>. The file uploads
automatically and a markdown link ![name](url) is inserted at the cursor position.

In scope:

New shared composer behavior used by exactly two surfaces:
- web/src/pages/IssueNew.jsx (issue description textarea),
- web/src/pages/Issue.jsx (thread comment textarea).
- Implementation: one new pure-ish helper + thin DOM glue (see §7), so the PR
  description/comment composers (web/src/pages/Pull.jsx:134 textarea) get it free later
  by adopting the same helper — PR adoption is explicitly out of scope for this change
  (issue #120 names issues only), but the helper must not be issue-specific.
Image types allowlist: PNG, JPEG, GIF, WebP (magic-byte sniffed, never extension-sniffed).
SVG is REJECTED (scriptable XML; see §9 open question — decision: reject, don't sanitize).
Per-file size cap: attachments.max_image_bytes, default 8 MiB → 413 over cap
(precedent: lfs.max_object_bytes 16 GiB, release max_asset_bytes 2 GiB (spec-only),
settings 16 KiB; 8 MiB keeps a comment body ≤ 64 KiB meaningful while covering screenshots).
Bodies stay raw text ≤ 64 KiB per docs/features/02_issues.md §1.2 — images are stored
objects referenced by markdown links, never inline data: URIs (the sanitizer
web/src/lib/sanitize.js:25-31 drops data: URLs anyway, so inline paste could never render).

Out of scope: video/audio/files, PR bodies, release bodies, avatar uploads, editing/deleting
attachments after upload, thumbnailing, EXIF stripping (note as follow-up), quota per repo
beyond the per-file cap.

1. Storage decision — new bucket family (do NOT reuse release assets)

Decision: new family repos/<o>/<r>/attachments/<sha256hex>/<name>, raw bytes, Create-only.

Justification:

internal/releases does not exist yet (07 is spec-only; ls internal/ shows no releases
package) — there is nothing to reuse, and release assets are tag-bound
(releases/<tag.json>/assets/<name>) while attachments are tag-independent.
Content-addressed by sha256: concurrent pastes of the same bytes converge via store
Create (create-if-absent); a 412 with matching sha = idempotent success, mismatching sha
under the same key is impossible by construction (key embeds the hash). No CAS loop, no header
object, no locks — the CAS arbitrator pattern from 07 §1.2 reduces to a single Create.
<name> = sanitized original filename (single segment, 1–200 bytes, no /, no leading .,
percent-decoded once per 07_api §2 — same rule as 07 §1.2 asset names) for a readable
![name](…) alt text and Content-Disposition filename; the sha segment makes collisions
harmless.
No new entry in the frozen overwritable-key list (14 §14.11 rule 2): every object is
Create-only immutable; removal (if ever offered) is an explicit store-delete like 07 assets.
Retention: orphans are harmless and kept (same philosophy as 07 §1.2 / 14 §14.10.2:
bytes first, reference second; a crash between upload and comment-submit leaves unreferenced
bytes). No LIST/GC task in v1; a future attachment-sweep maintainer kind can reconcile
against bodies if the family grows (open question §9).

2. API (Seam 1 route provider, both lanes)

New provider in the issues package (internal/issues, which already fronts both lanes via
Handle per 02 Wave B notes — Seam 1 in code is the server.ExtraRoutes chain, not
api.Lanes; follow the existing internal/issues/http.go pattern exactly):

METHOD + path	Auth (P6)	Request → Response
`POST /{o}/{r}/api/attachments`	authenticated + read (same gate as `POST …/issues/{num}/comments`: any signed-in principal who can read the repo; anonymous blocked — `requireAuthenticated`, `internal/issues/service.go:103` precedent)	raw image bytes, required `Content-Length` + `X-Walgit-Attachment-Sha256` (hex, client-computed via `crypto.subtle.digest` — 07 §8 precedent) + `Content-Type` advisory → `201 {name, size, sha256, content_type, url}` where `url` = the GET byte path below
`GET /{o}/{r}/attachments/<sha256>/<name>`	read (P6 resolution — see §3)	bytes under the static contract (06 §5)

Upload flow (single-step, NOT the release two-step — images are ≤ 8 MiB, bounded, human-rate):

Bound the body: io.LimitReader(r.Body, max_image_bytes+1); one byte over → 413 plain text
(06 §4.3/LFS §6.2 precedent: per-feature limits, no global body middleware).
Stream to a cache-dir spool (<cache.dir>/attachments-spool/, LFS §6.2 pattern in
internal/server/lfs.go:408-457), hashing sha256 in the copy loop (io.TeeReader —
no second pass, no full buffering).
Verify: declared X-Walgit-Attachment-Sha256 matches computed (mismatch → 400), size ≤ cap,
sniff type from magic bytes (net/http.DetectContentType on the first 512 B + GIF/PNG/JPEG/WebP
magic check; stdlib only, dependency law) — mismatch with allowlist or SVG detected → 415
plain text. Extension and client Content-Type are ignored for the verdict.
Create the object at attachments/<sha>/<sanitized-name> (dedup: 412 → re-GET, same bytes =
idempotent 201 with the same body; cannot mismatch since the key is the hash).
Return the JSON record; the client inserts ![<name>](<url>) at the cursor (see §7).
Upload and comment-submit are decoupled (upload does not reference any issue number —
usable from both the new-issue form and thread comments, and from a form whose issue
doesn't exist yet).

Wire rules per 07_api §2: JSON success, plain-text errors, [] never null (n/a here),
RFC 3339 if timestamps added, arrays [] never null. Cache class: POST no-store;
byte GET per §3. Add the new routes to the discovery endpoints[] (07 §8 / 14 §14.12 —
adding a route without updating discovery is a bug).

3. Serving reads (auth-gated, private cache class)

Decision: auth-gated reads at read level, static contract with private cache class.

GET /{o}/{r}/attachments/… requires read on the repo (P6 order; anonymous iff
anonymous_read). Rationale: issue bodies inherit repo visibility (14 defers per-repo
private-read ACLs but the require_read hook is now specified per the Wave A amendment);
serving issue images publicly while the issue text is gated would leak private-repo
screenshots via URL guessing. Content-addressed URLs are unguessable but not a security
boundary — gate them.
Static contract otherwise verbatim (06 §5): strong ETag = store version, If-None-Match →
304, Range/If-Range → 206/416, HEAD supported, Accept-Ranges: bytes,
Content-Type = sniffed type (never the client claim), X-Content-Type-Options: nosniff.
Deviation from 06 §5 in one token: Cache-Control: private, max-age=31536000, immutable
(not public) — immutable bytes, but authenticated reads must not sit in shared caches.
Markdown render path needs no change: renderMarkdown (web/src/lib/markdown.js:20-21)
already emits <img src="…"> for ![](url), and the sanitizer (sanitize.js:13-16,25-31)
allows img[src,alt,title] with relative/same-origin URLs — a same-origin relative
attachment URL (/<o>/<r>/attachments/<sha>/<name>) passes the allowlist and renders
with the browser's session cookies. No sanitizer change required.

4. Abuse controls

Anonymous blocked on upload (authenticated + read); reads gated per §3.
Type sniffing, not extension sniffing (§2 step 3); allowlist PNG/JPEG/GIF/WebP; SVG
rejected with 415 (decision: reject — SVG is active content; sanitizing XML server-side
is a new attack surface and client-side re-encoding is out of scope).
Caps: per-file 8 MiB (attachments.max_image_bytes, host config, validated like other
size keys); Content-Length required (missing → 411/400 plain text, LFS precedent);
io.LimitReader + drain-abandon above cap; never buffer the object (fixed 1 MiB copy
buffer, hash in-loop).
No polling, no tasks: upload is synchronous and bounded (≤ 8 MiB spool-verify-Create);
progress UI is the composer's inline uploading state (law 7 — no silent spinner: placeholder
text ![uploading…](…) replaced on completion, error surfaced verbatim per the plain-text
error contract).
Rate limiting: none in v1 beyond auth + cap (human-rate reasoning per P2); note as follow-up
if abused.

5. Concurrency

No new locks, no single-flight, no task kinds. The only shared-state arbitration is the
store's create-if-absent on the content-addressed key — concurrent identical uploads both
succeed idempotently; distinct bytes never collide.
Handler holds no repo locks across store calls (13 §2 rule 4); the spool/verify/Create
sequence is lock-free straight-line I/O on the request goroutine, bounded by the 8 MiB cap.
Bulk/control-plane separation (13 §7 incident 2): attachment byte PUTs are small
(≤ 8 MiB, single object, no striping) — control-plane transport is fine; no bulk-pool
involvement. The byte GETs are static-contract streams like LFS GETs.
### Concurrency subsection required in the implementing doc amendment (law 3: every
concurrent design carries one — here: "hazard: none beyond Create races; avoidance: content
addressing + idempotent 412 handling").

6. EVIDENCE sketch

No hot-path (git push/fetch) impact: uploads ride the collaboration surface (P5: LIST/index
rules are for pages, and this path uses neither). Evidence entry only if a claim is made:

docs/EVIDENCE.md entry with harness in internal/devtools/ measuring upload round trips
(expect: 1 PUT + 0 GET on dedup-hit… actually 1 Create attempt; 1 GET on 412-revalidate) —
optional, only if the change claims a budget.
Otherwise the "budgets" are the existing sim assertions (untouched) + new handler tests.

7. Frontend (SolidJS SPA + Tailwind, D-WEB-6; dependency-free SDK)

New SDK submodule web/sdk/src/attachments.js: uploadAttachment(file|Blob) → {url,…}
(computes sha256 via crypto.subtle.digest, streams raw POST with the sha header —
07 §8 uploadAsset precedent), wired through ReposClient like attachIssues.
New shared helper web/src/lib/attachUpload.js (pure logic, headless-testable per D-WEB-4):
insertAtCursor(textarea, text) + uploadWithPlaceholder({file, signal}) semantics —
kept DOM-thin: the pages' onPaste/onDrop handlers call it, it returns the markdown to
splice. node --test unit tests for cursor-splice + placeholder replace (no DOM needed).
Thin glue in IssueNew.jsx + Issue.jsx textareas: onPaste (read e.clipboardData.files
/ items, image-kind only), onDrop (e.dataTransfer.files, preventDefault), per-file:
insert ![uploading <name>…]() placeholder at cursor → upload → replace placeholder with
![<name>](<url>); failure → replace with error text + reportError. Multiple files:
sequential inserts (no unbounded fan-out; each upload is its own request).
Preview (renderMarkdown + sanitize) renders the image immediately once the markdown
lands — no renderer change (see §3).
Styling: Tailwind classes on the uploading indicator; .markdown-body img { max-width: 100% }
already exists (web/css/repo.css:42 / web/src/ui.css — verify the SolidJS-era rule
covers prose-sm containers; add max-w-full if not).

8. Acceptance criteria

Backend: internal/issues (or new internal/attachments provider — decide at implementation;
recommendation: live in internal/issues since auth gates + bucket prefix are the issues
family's, registered as its own RouteProvider) holds ≥ 95% statement coverage
(make cover), table-driven httptest for every handler + every rejection (bad sha, over
cap, SVG, non-image magic, missing length, anonymous 401, insufficient-read 403/404),
-race clean.
Round-trip: paste PNG into IssueNew body → upload 201 → markdown inserted at cursor →
submit → thread page renders the image; same for thread comment composer; real Chromium
drive (AGENTS verification ladder #8: navigate, paste via CDP, screenshot, console clean)
in dark (default) and light themes.
data:-URI paste still rejected/dropped by the sanitizer (existing tests); SVG upload → 415.
git status clean of anything but the intended files; docs updated in the same change:
docs/features/02_issues.md (attachments subsection + Decisions entry), the provider doc's
wire table, discovery endpoints[]; DEVIATIONS.md untouched unless a deviation is taken
(private-cache-class token is a §5 clarification, recorded in 06's Decisions section).
make fmt && make vet clean; dependency budget untouched (stdlib only server-side, no new
npm deps).

9. Open questions / risks (need reviewer decision before implementation)

SVG: REJECT (recommended) vs sanitize-and-serve. This plan decides REJECT (415) —
serving user SVGs same-origin is script execution in our origin. Confirm.
Private-repo reads via markdown render: §3 gates byte GETs at read level, so an image
pasted then quoted into a public repo's issue breaks for readers without access to the
source repo (same as GitHub's behavior with private assets — acceptable?). Confirm no
signed-URL/public-alias scheme is wanted in v1.
Retention/orphans: uploads decoupled from issues ⇒ abandoned uploads accumulate.
v1 keeps them (harmless, Create-only). Is a future sweep/GC task acceptable tech debt, or
should upload be two-phase (spool → attach-on-comment-submit with server-side reference)?
Two-phase doubles the wire surface; recommend v1 as planned.
Package home: internal/issues (recommended — auth + prefix affinity) vs new
internal/attachments. Either satisfies Seam 1; reviewer picks.
Cap value + config key: 8 MiB default under attachments.max_image_bytes — confirm the
number and the section name (new [attachments] section vs extending an existing one).
GIF animation / WebP support in markdown-lite: renderer passes src through; browser
support assumed — no server work. Not a risk, noted for the browser test matrix.
Dedup privacy side-channel: identical bytes → identical URL; a user could probe whether
bytes exist by uploading and comparing URLs. Content is unreadable without read access
(§3), so the oracle reveals at most existence. Accept (same as release-asset dedup)?

10. Review ask

Reviewers: please confirm §9 items 1–5 explicitly (they pin wire shape, status codes, config
keys, and package home — all frozen-after-merge per 14 §14.12), plus whether an EVIDENCE
entry is wanted for the upload path.

# Plan: paste/drop image upload in issue comments/descriptions (Forgejo #120) ## 0. Objective / scope **Objective:** In the issue create form and the issue thread comment composer, a user can paste (Ctrl+V / Cmd+V) or drag-drop an image file onto the `<textarea>`. The file uploads automatically and a markdown link `![name](url)` is inserted at the cursor position. **In scope:** - New shared composer behavior used by exactly two surfaces: - `web/src/pages/IssueNew.jsx` (issue description textarea), - `web/src/pages/Issue.jsx` (thread comment textarea). - Implementation: one new pure-ish helper + thin DOM glue (see §7), so the PR description/comment composers (`web/src/pages/Pull.jsx:134` textarea) get it free later by adopting the same helper — PR adoption is explicitly **out of scope** for this change (issue #120 names issues only), but the helper must not be issue-specific. - Image types allowlist: **PNG, JPEG, GIF, WebP** (magic-byte sniffed, never extension-sniffed). **SVG is REJECTED** (scriptable XML; see §9 open question — decision: reject, don't sanitize). - Per-file size cap: `attachments.max_image_bytes`, default **8 MiB** → `413` over cap (precedent: `lfs.max_object_bytes` 16 GiB, release `max_asset_bytes` 2 GiB (spec-only), settings 16 KiB; 8 MiB keeps a comment body ≤ 64 KiB meaningful while covering screenshots). - Bodies stay **raw text ≤ 64 KiB** per `docs/features/02_issues.md` §1.2 — images are stored objects referenced by markdown links, **never inline `data:` URIs** (the sanitizer `web/src/lib/sanitize.js:25-31` drops `data:` URLs anyway, so inline paste could never render). **Out of scope:** video/audio/files, PR bodies, release bodies, avatar uploads, editing/deleting attachments after upload, thumbnailing, EXIF stripping (note as follow-up), quota per repo beyond the per-file cap. ## 1. Storage decision — new bucket family (do NOT reuse release assets) **Decision: new family `repos/<o>/<r>/attachments/<sha256hex>/<name>`**, raw bytes, `Create`-only. Justification: - `internal/releases` does **not exist yet** (07 is spec-only; `ls internal/` shows no releases package) — there is nothing to reuse, and release assets are tag-bound (`releases/<tag.json>/assets/<name>`) while attachments are tag-independent. - Content-addressed by sha256: concurrent pastes of the same bytes converge via store `Create` (create-if-absent); a 412 with matching sha = idempotent success, mismatching sha under the same key is impossible by construction (key embeds the hash). No CAS loop, no header object, no locks — the CAS arbitrator pattern from 07 §1.2 reduces to a single `Create`. - `<name>` = sanitized original filename (single segment, 1–200 bytes, no `/`, no leading `.`, percent-decoded once per 07_api §2 — same rule as 07 §1.2 asset names) for a readable `![name](…)` alt text and `Content-Disposition` filename; the sha segment makes collisions harmless. - No new entry in the frozen overwritable-key list (14 §14.11 rule 2): every object is Create-only immutable; removal (if ever offered) is an explicit store-delete like 07 assets. - Retention: **orphans are harmless and kept** (same philosophy as 07 §1.2 / 14 §14.10.2: bytes first, reference second; a crash between upload and comment-submit leaves unreferenced bytes). No LIST/GC task in v1; a future `attachment-sweep` maintainer kind can reconcile against bodies if the family grows (open question §9). ## 2. API (Seam 1 route provider, both lanes) New provider in the issues package (`internal/issues`, which already fronts both lanes via `Handle` per 02 Wave B notes — Seam 1 in code is the `server.ExtraRoutes` chain, not `api.Lanes`; follow the existing `internal/issues/http.go` pattern exactly): | METHOD + path | Auth (P6) | Request → Response | |---|---|---| | `POST /{o}/{r}/api/attachments` | authenticated + read (same gate as `POST …/issues/{num}/comments`: any signed-in principal who can read the repo; **anonymous blocked** — `requireAuthenticated`, `internal/issues/service.go:103` precedent) | raw image bytes, required `Content-Length` + `X-Walgit-Attachment-Sha256` (hex, client-computed via `crypto.subtle.digest` — 07 §8 precedent) + `Content-Type` advisory → `201 {name, size, sha256, content_type, url}` where `url` = the GET byte path below | | `GET /{o}/{r}/attachments/<sha256>/<name>` | read (P6 resolution — see §3) | bytes under the static contract (06 §5) | Upload flow (single-step, NOT the release two-step — images are ≤ 8 MiB, bounded, human-rate): 1. Bound the body: `io.LimitReader(r.Body, max_image_bytes+1)`; one byte over → `413` plain text (06 §4.3/LFS §6.2 precedent: per-feature limits, no global body middleware). 2. Stream to a cache-dir spool (`<cache.dir>/attachments-spool/`, LFS §6.2 pattern in `internal/server/lfs.go:408-457`), hashing sha256 in the copy loop (`io.TeeReader` — no second pass, no full buffering). 3. Verify: declared `X-Walgit-Attachment-Sha256` matches computed (mismatch → 400), size ≤ cap, **sniff type from magic bytes** (`net/http.DetectContentType` on the first 512 B + GIF/PNG/JPEG/WebP magic check; stdlib only, dependency law) — mismatch with allowlist or SVG detected → `415` plain text. Extension and client `Content-Type` are ignored for the verdict. 4. `Create` the object at `attachments/<sha>/<sanitized-name>` (dedup: 412 → re-GET, same bytes = idempotent `201` with the same body; cannot mismatch since the key is the hash). 5. Return the JSON record; the client inserts `![<name>](<url>)` at the cursor (see §7). Upload and comment-submit are **decoupled** (upload does not reference any issue number — usable from both the new-issue form and thread comments, and from a form whose issue doesn't exist yet). Wire rules per 07_api §2: JSON success, **plain-text errors**, `[]` never null (n/a here), RFC 3339 if timestamps added, arrays `[]` never `null`. Cache class: POST `no-store`; byte GET per §3. Add the new routes to the discovery `endpoints[]` (07 §8 / 14 §14.12 — adding a route without updating discovery is a bug). ## 3. Serving reads (auth-gated, private cache class) **Decision: auth-gated reads at read level, static contract with `private` cache class.** - `GET /{o}/{r}/attachments/…` requires **read** on the repo (P6 order; anonymous iff `anonymous_read`). Rationale: issue bodies inherit repo visibility (14 defers per-repo private-read ACLs but the `require_read` hook is now specified per the Wave A amendment); serving issue images publicly while the issue text is gated would leak private-repo screenshots via URL guessing. Content-addressed URLs are unguessable but not a security boundary — gate them. - Static contract otherwise verbatim (06 §5): strong `ETag` = store version, `If-None-Match` → 304, `Range`/`If-Range` → 206/416, HEAD supported, `Accept-Ranges: bytes`, `Content-Type` = sniffed type (never the client claim), `X-Content-Type-Options: nosniff`. **Deviation from 06 §5 in one token: `Cache-Control: private, max-age=31536000, immutable`** (not `public`) — immutable bytes, but authenticated reads must not sit in shared caches. - Markdown render path needs no change: `renderMarkdown` (`web/src/lib/markdown.js:20-21`) already emits `<img src="…">` for `![](url)`, and the sanitizer (`sanitize.js:13-16,25-31`) allows `img[src,alt,title]` with relative/same-origin URLs — a same-origin relative attachment URL (`/<o>/<r>/attachments/<sha>/<name>`) passes the allowlist and renders with the browser's session cookies. No sanitizer change required. ## 4. Abuse controls - **Anonymous blocked** on upload (authenticated + read); reads gated per §3. - **Type sniffing, not extension sniffing** (§2 step 3); allowlist PNG/JPEG/GIF/WebP; **SVG rejected with 415** (decision: reject — SVG is active content; sanitizing XML server-side is a new attack surface and client-side re-encoding is out of scope). - **Caps:** per-file 8 MiB (`attachments.max_image_bytes`, host config, validated like other size keys); `Content-Length` required (missing → 411/400 plain text, LFS precedent); `io.LimitReader` + drain-abandon above cap; never buffer the object (fixed 1 MiB copy buffer, hash in-loop). - **No polling, no tasks:** upload is synchronous and bounded (≤ 8 MiB spool-verify-Create); progress UI is the composer's inline uploading state (law 7 — no silent spinner: placeholder text `![uploading…](…)` replaced on completion, error surfaced verbatim per the plain-text error contract). - Rate limiting: none in v1 beyond auth + cap (human-rate reasoning per P2); note as follow-up if abused. ## 5. Concurrency - No new locks, no single-flight, no task kinds. The only shared-state arbitration is the store's create-if-absent on the content-addressed key — concurrent identical uploads both succeed idempotently; distinct bytes never collide. - Handler holds **no repo locks across store calls** (13 §2 rule 4); the spool/verify/Create sequence is lock-free straight-line I/O on the request goroutine, bounded by the 8 MiB cap. - Bulk/control-plane separation (13 §7 incident 2): attachment byte PUTs are small (≤ 8 MiB, single object, no striping) — control-plane transport is fine; no bulk-pool involvement. The byte GETs are static-contract streams like LFS GETs. - `### Concurrency` subsection required in the implementing doc amendment (law 3: every concurrent design carries one — here: "hazard: none beyond Create races; avoidance: content addressing + idempotent 412 handling"). ## 6. EVIDENCE sketch No hot-path (git push/fetch) impact: uploads ride the collaboration surface (P5: LIST/index rules are for pages, and this path uses neither). Evidence entry only if a claim is made: - `docs/EVIDENCE.md` entry with harness in `internal/devtools/` measuring upload round trips (expect: 1 PUT + 0 GET on dedup-hit… actually 1 Create attempt; 1 GET on 412-revalidate) — optional, only if the change claims a budget. - Otherwise the "budgets" are the existing sim assertions (untouched) + new handler tests. ## 7. Frontend (SolidJS SPA + Tailwind, D-WEB-6; dependency-free SDK) - New SDK submodule `web/sdk/src/attachments.js`: `uploadAttachment(file|Blob) → {url,…}` (computes sha256 via `crypto.subtle.digest`, streams raw POST with the sha header — 07 §8 `uploadAsset` precedent), wired through `ReposClient` like `attachIssues`. - New shared helper `web/src/lib/attachUpload.js` (pure logic, headless-testable per D-WEB-4): `insertAtCursor(textarea, text)` + `uploadWithPlaceholder({file, signal})` semantics — kept DOM-thin: the pages' `onPaste`/`onDrop` handlers call it, it returns the markdown to splice. `node --test` unit tests for cursor-splice + placeholder replace (no DOM needed). - Thin glue in `IssueNew.jsx` + `Issue.jsx` textareas: `onPaste` (read `e.clipboardData.files` / items, image-kind only), `onDrop` (`e.dataTransfer.files`, `preventDefault`), per-file: insert `![uploading <name>…]()` placeholder at cursor → upload → replace placeholder with `![<name>](<url>)`; failure → replace with error text + `reportError`. Multiple files: sequential inserts (no unbounded fan-out; each upload is its own request). - Preview (`renderMarkdown` + `sanitize`) renders the image immediately once the markdown lands — no renderer change (see §3). - Styling: Tailwind classes on the uploading indicator; `.markdown-body img { max-width: 100% }` already exists (`web/css/repo.css:42` / `web/src/ui.css` — verify the SolidJS-era rule covers `prose-sm` containers; add `max-w-full` if not). ## 8. Acceptance criteria - Backend: `internal/issues` (or new `internal/attachments` provider — decide at implementation; recommendation: live in `internal/issues` since auth gates + bucket prefix are the issues family's, registered as its own `RouteProvider`) holds **≥ 95% statement coverage** (`make cover`), table-driven httptest for every handler + every rejection (bad sha, over cap, SVG, non-image magic, missing length, anonymous 401, insufficient-read 403/404), `-race` clean. - Round-trip: paste PNG into IssueNew body → upload 201 → markdown inserted at cursor → submit → thread page renders the image; same for thread comment composer; **real Chromium drive** (AGENTS verification ladder #8: navigate, paste via CDP, screenshot, console clean) in **dark (default) and light** themes. - `data:`-URI paste still rejected/dropped by the sanitizer (existing tests); SVG upload → 415. - `git status` clean of anything but the intended files; docs updated in the same change: `docs/features/02_issues.md` (attachments subsection + Decisions entry), the provider doc's wire table, discovery `endpoints[]`; `DEVIATIONS.md` untouched unless a deviation is taken (private-cache-class token is a §5 clarification, recorded in 06's Decisions section). - `make fmt && make vet` clean; dependency budget untouched (stdlib only server-side, no new npm deps). ## 9. Open questions / risks (need reviewer decision before implementation) 1. **SVG: REJECT (recommended) vs sanitize-and-serve.** This plan decides REJECT (415) — serving user SVGs same-origin is script execution in our origin. Confirm. 2. **Private-repo reads via markdown render:** §3 gates byte GETs at read level, so an image pasted then quoted into a public repo's issue breaks for readers without access to the source repo (same as GitHub's behavior with private assets — acceptable?). Confirm no signed-URL/public-alias scheme is wanted in v1. 3. **Retention/orphans:** uploads decoupled from issues ⇒ abandoned uploads accumulate. v1 keeps them (harmless, Create-only). Is a future sweep/GC task acceptable tech debt, or should upload be two-phase (spool → attach-on-comment-submit with server-side reference)? Two-phase doubles the wire surface; recommend v1 as planned. 4. **Package home:** `internal/issues` (recommended — auth + prefix affinity) vs new `internal/attachments`. Either satisfies Seam 1; reviewer picks. 5. **Cap value + config key:** 8 MiB default under `attachments.max_image_bytes` — confirm the number and the section name (new `[attachments]` section vs extending an existing one). 6. **GIF animation / WebP support in markdown-lite:** renderer passes `src` through; browser support assumed — no server work. Not a risk, noted for the browser test matrix. 7. **Dedup privacy side-channel:** identical bytes → identical URL; a user could probe whether bytes exist by uploading and comparing URLs. Content is unreadable without read access (§3), so the oracle reveals at most existence. Accept (same as release-asset dedup)? ## 10. Review ask Reviewers: please confirm §9 items 1–5 explicitly (they pin wire shape, status codes, config keys, and package home — all frozen-after-merge per 14 §14.12), plus whether an EVIDENCE entry is wanted for the upload path.

crueber commented

2026-09-05 06:38:02 +00:00

Author

Owner

Review: image paste/drop plan for #120 (review-only, no code changed)

Verdict: proceed-with-fixes — the architecture is sound and almost every factual claim I checked against CODE holds up. Three items are BLOCKING (all wire/shape-freeze or seam issues that must be decided before merge, per the plan's own §10 note that §9 items pin frozen shape), the rest are should-fix/nits to fold into the implementing change.

What I verified against code (not docs)

§1 reuse decision — CONFIRMED. internal/releases does not exist (ls internal/ shows no releases package; only internal/pulls etc.). The 07 "precedents" (X-Walgit-Asset-Sha256, uploadAsset, two-step flow) are spec-only (docs/features/07_releases_stars.md, 08_ui_sdk.md); web/sdk/src/ has no releases.js/social.js. So: nothing to reuse, new repos/<o>/<r>/attachments/<sha>/<name> family is the right call, and the "07 §8 precedent" citations should be relabeled spec-following rather than code-following. (nit)
LFS spool pattern — CONFIRMED. internal/server/lfs.go:228-244 (lfsPut) is exactly the claimed shape: ensureDir(cacheDir("lfs-spool")) → os.CreateTemp → copyTee(io.LimitReader(...), tmp, hash) → verify-before-write, 413 over lfs.max_object_bytes (default 16 GiB, internal/config/config.go:220,373). Reusable shape, correct citation (line numbers have drifted slightly from the plan's 408-457, which is the read-through half — update refs).
Comment-create auth gate — CONFIRMED. internal/issues/service.go:199-205 (AddComment): requireAuthenticated + requireRead, matching the plan's "authenticated + read, anonymous blocked" upload gate. Reusing the same helpers gives parity by construction. Good.
Bodies ≤ 64 KiB — CONFIRMED. internal/issues/model.go:45,199-205 (MaxBodyBytes = 64<<10, validateBody). Markdown ![](url) links are ~80 chars, so the budget comfortably holds screenshots' worth of references.
Markdown/sanitizer — CONFIRMED, no changes needed. web/src/lib/markdown.js:20-21 emits <img src alt title>; web/src/lib/sanitize.js:13-16,25-31 allows img[src,alt,title] and drops data:/javascript: via safeUrl. A same-origin relative attachment URL renders with session cookies. The data:-URI claim (§0) is correct.
Seam 1 shape — CONFIRMED. internal/issues/http.go:22-58: Handle(w,r) bool fronting the core mux on both lanes, decodeSegment per-segment decoding — the plan's "follow http.go exactly" instruction is actionable as written.
SSE/timeline carry no bytes — CONFIRMED, private-repo story holds. StreamEvent (internal/issues/issues.go:100-106) is {Name, Repo, IssueNum} only; bodies travel exclusively in authed timeline GETs. So gating the byte GET at read level closes the leak: the only cross-boundary artifact is a URL string, and guessing it yields 401/403/404 without source-repo read. §3's rationale is correct.
Store CAS gives you the dedup — CONFIRMED. PutCreate is create-if-absent with 412-style ErrKindPreconditionFailed on exists (internal/store/store.go:61,118-153); hash-in-key means a 412 can only be same-bytes → idempotent 201 without re-read. No CAS loop needed. Correct.
web/css/repo.css:42 does NOT apply to the live UI. The SolidJS entry (web/src/index.jsx:6) imports only web/src/ui.css, whose .markdown-body block (lines 73-81) has no img rule. The plan's hedge ("verify…; add max-w-full if not") resolves to: the if not branch triggers — state it outright (should-fix #5 below).

BLOCKING (must be resolved before merge — frozen shape/seam)

B1. Content-Length: required → 411/400 contradicts its cited precedent and breaks chunked clients. lfsPut (internal/server/lfs.go:212-274) never checks Content-Length; the cap is enforced purely by io.LimitReader(max+1) → 413. There is no 411 anywhere in internal/server. Requiring Content-Length breaks chunked encodings for zero security benefit (the LimitReader already bounds the read). Ruling: drop the requirement; LimitReader + 413 exactly like LFS. Status codes freeze after merge — decide now.
B2. "Add the new routes to the discovery endpoints[]" is unimplementable as written. Core discovery is derived from the internal/api route table (internal/api/discovery.go:23-37, routes.go Expose flags). Feature routes via the Handle(w,r) bool ExtraRoutes chain (internal/issues, internal/identity) register nothing there — I verified neither package references Expose/discovery, so existing issue routes are already absent from endpoints[]. The plan must pick one: (a) rule that repo-scoped feature routes are exempt from the 14 §14.12 discovery sentence (with a one-line Decisions entry saying so — my recommendation; the SDK builds repo-scoped paths statically anyway, web/sdk/src/issues.js), or (b) amend the discovery seam so extras can contribute templates. Cannot merge with the instruction in its current form since it demands something no feature does.
B3. Name where the byte-GET static-contract code lives — it cannot be reused. serveLFSObject/conditionals are methods on server.Server (internal/server/static.go:15-60,135+); internal/issues imports only server/auth, store, identity, git — importing server would be an upward import into the package that wires it (law 8; likely a straight import cycle). So ETag/304/Range/206/416/HEAD handling must be reimplemented (~60 lines) inside internal/issues (or a new leaf). The plan says "static contract otherwise verbatim" without naming the home. Ruling: implement in-package, table-test it like the LFS static tests, and say so in the plan. (Related: persist the sniffed type via PutOptions.ContentType at Create — the object "carries no metadata" per 07 §1.2, so the stored content type is the only way the GET can emit it. With X-Content-Type-Options: nosniff, serving application/octet-stream will get the <img> blocked in Chrome — this detail is load-bearing for rendering, not cosmetic.)

Should-fix

S1. Filename → markdown injection. ![<name>](<url>) built from an unsanitized filename breaks on ], (, ) in names (e.g. a](b.png). The insertAtCursor helper must escape ]/(/) in alt text (and the <name> URL segment must be percent-encoded at insert time). One-line rule + unit test in the headless attachUpload.js suite.
S2. Spell out the non-lane GET branch in Handle. The byte GET (/{o}/{r}/attachments/<sha>/<name>) is outside api|api-browser, but current Handle only claims segs[2] == api|api-browser paths (http.go:50-56). The plan needs the new branch + collision analysis (there is none: no git sub-path is literally attachments, and it sits after {o}/{r} so no repo-name clash) + a fallthrough test (false → core 404, cf. final_test.go:393-396 style).
S3. Config key needs explicit config.go scope. attachments.max_image_bytes (new [attachments] section — consistent with [lfs] max_object_bytes) means: struct + 8<<20 default + validation + docs, all in the same change (law 12). Note internal/setup does not exist in code yet, so no setup-schema work is possible today — but record the key in the Decisions entry as setup-schema-pending so it isn't lost. The 8 MiB value itself is fine (screenshot-covering, orphan-bounding, keeps bodies meaningful).
S4. crypto.subtle needs a fallback position. crypto.subtle.digest is undefined on non-secure origins (http:// LAN hosts — a real walhub deployment shape). If the sha header stays mandatory (400 on mismatch), uploads break there with no recourse. Ruling: make X-Walgit-Attachment-Sha256 optional-when-present (server always hashes the spool; verifies only if the client sent a value) — strictly weaker client requirement, same server-side integrity, and it keeps the 07-spec shape for secure contexts. Decide before merge since it changes the 400 contract.
S5. Just say ui.css needs the img rule. Verification above: add .markdown-body img { @apply max-w-full h-auto; } (dark-theme check rides the existing Chromium drive). Delete the web/css/repo.css citation — that file isn't loaded by the SPA and citing it will confuse the implementer.
S6. PR adoption is not "free later". web/src/pages/Pull.jsx:124 renders comment bodies as plain text (whitespace-pre-wrap), not markdown — adopting the upload helper there later also requires switching PR rendering to renderMarkdown+sanitize (plus the release-body surface, spec-only). One line in §0/§7 noting the dependency; otherwise the "must not be issue-specific" requirement is still right and sufficient. (Third composer Pull.jsx:134 is correctly identified; Setup.jsx/Keys.jsx/Settings.jsx textareas are config/keys/paste surfaces — correctly excluded.)
S7. Abuse ceiling belongs in the doc. Authenticated-user orphan flood (≤8 MiB × attempts) is the same class as comment spam — no new anonymous vector since upload reuses the comment gate. Accept v1 as planned, but write the ceiling down (expected bytes/day at human rate) so the future attachment-sweep maintainer kind has a trigger criterion. Retention math: even 100 abandoned uploads/day = ≤800 MiB/day worst case, human-rate reality is orders of magnitude less; Create-only immutable objects need no frozen-list entry (14 §14.11 rule 2 covers overwritable keys only — the plan is right).

Nits

N1. 07 §8 citations are spec-following, not code precedent (no internal/releases, no uploadAsset in the SDK) — relabel to avoid sending the implementer hunting for code that isn't there. LFS-spool line refs should point at lfs.go:228-244, not 408-457 (the latter is the read-through half).
N2. Dedup oracle (§9.7): accept — and it's narrower than stated. Keys are repo-scoped (repos/<o>/<r>/…), so cross-repo probing is impossible; a same-repo prober already holds read+auth and learns existence-only, never bytes. One sentence covers it.
N3. Oversize-submit UX: upload and comment-submit are decoupled, so a body pushed over 64 KiB by many image links 400s after the bytes are stored (orphans by design). Accept — but have the composer pre-check projected length client-side before uploading, or at minimum surface the 400 verbatim per the plain-text contract (already planned in §4).
N4. No EVIDENCE entry needed — agreed with §6. Collaboration surface, human-rate, sim budgets untouched. The only numbers that belong anywhere are the S7 ceiling in the Decisions entry.

Rulings on §9 (explicit, as asked)

SVG: REJECT (415) — confirmed. Same-origin serving = script execution in our origin; the sanitizer is a preview-side string filter, not a server XML sanitizer, and the dependency budget forbids pulling one in. PNG/JPEG/GIF/WebP cover the screenshot use case.
Cross-repo quoting: accept the breakage — gate at source-repo read, no signed URLs in v1. Matches GitHub private-asset behavior; no byte-leak channel exists (SSE frames carry no bodies, verified).
Retention: keep orphans in v1 — accept the tech debt, but record the future sweep kind name + trigger + reference rule (body contains URL) in the Decisions entry. No two-phase upload (wire surface doubles for no v1 benefit).
Package home: internal/issues — auth-gate affinity, one coverage gate, no new wiring. Either satisfies Seam 1; this one is cheaper.
Cap + key: 8 MiB under [attachments] max_image_bytes — confirmed, with the S3 config-scope note.
GIF/WebP: agreed, no server work — include an animated GIF + a WebP in the Chromium matrix.
Dedup oracle: accept — repo-scoped keys make it existence-only for already-authorized readers (N2).

Acceptance delta (add to §8)

Byte-GET table tests: 304/206/416/HEAD + private, immutable + sniffed Content-Type + nosniff (B3); 411-must-not-exist (B1).
Discovery decision recorded (B2); config default/validation tests (S3); sha-header-optional test (S4).
node --test for alt-text escaping + placeholder replace (S1); ui.css img rule + dark/light Chromium paste-drive with console clean (S5).
make cover ≥95% on the touched package; make fmt && make vet; docs updated same-change (02_issues attachments subsection + Decisions entries in 02/06/14-as-touched).

Review method: read the plan end to end, then verified each load-bearing claim against code (internal/issues/service.go, http.go, model.go, issues.go; internal/server/lfs.go, static.go; internal/store/store.go; internal/api/discovery.go; web/src/lib/markdown.js, sanitize.js, ui.css, index.jsx; web/src/pages/IssueNew.jsx, Issue.jsx, Pull.jsx; web/sdk/src/core.js, issues.js; internal/config/config.go). No production code written, nothing committed, git status untouched (pre-existing worktree modifications left as found).

# Review: image paste/drop plan for #120 (review-only, no code changed) Verdict: **proceed-with-fixes** — the architecture is sound and almost every factual claim I checked against CODE holds up. Three items are BLOCKING (all wire/shape-freeze or seam issues that must be decided before merge, per the plan's own §10 note that §9 items pin frozen shape), the rest are should-fix/nits to fold into the implementing change. ## What I verified against code (not docs) - **§1 reuse decision — CONFIRMED.** `internal/releases` does not exist (`ls internal/` shows no releases package; only `internal/pulls` etc.). The 07 "precedents" (`X-Walgit-Asset-Sha256`, `uploadAsset`, two-step flow) are **spec-only** (`docs/features/07_releases_stars.md`, `08_ui_sdk.md`); `web/sdk/src/` has no `releases.js`/`social.js`. So: nothing to reuse, new `repos/<o>/<r>/attachments/<sha>/<name>` family is the right call, and the "07 §8 precedent" citations should be relabeled spec-following rather than code-following. (nit) - **LFS spool pattern — CONFIRMED.** `internal/server/lfs.go:228-244` (`lfsPut`) is exactly the claimed shape: `ensureDir(cacheDir("lfs-spool"))` → `os.CreateTemp` → `copyTee(io.LimitReader(...), tmp, hash)` → verify-before-write, 413 over `lfs.max_object_bytes` (default 16 GiB, `internal/config/config.go:220,373`). Reusable shape, correct citation (line numbers have drifted slightly from the plan's `408-457`, which is the read-through half — update refs). - **Comment-create auth gate — CONFIRMED.** `internal/issues/service.go:199-205` (`AddComment`): `requireAuthenticated` + `requireRead`, matching the plan's "authenticated + read, anonymous blocked" upload gate. Reusing the same helpers gives parity by construction. Good. - **Bodies ≤ 64 KiB — CONFIRMED.** `internal/issues/model.go:45,199-205` (`MaxBodyBytes = 64<<10`, `validateBody`). Markdown `![](url)` links are ~80 chars, so the budget comfortably holds screenshots' worth of references. - **Markdown/sanitizer — CONFIRMED, no changes needed.** `web/src/lib/markdown.js:20-21` emits `<img src alt title>`; `web/src/lib/sanitize.js:13-16,25-31` allows `img[src,alt,title]` and drops `data:`/`javascript:` via `safeUrl`. A same-origin relative attachment URL renders with session cookies. The `data:`-URI claim (§0) is correct. - **Seam 1 shape — CONFIRMED.** `internal/issues/http.go:22-58`: `Handle(w,r) bool` fronting the core mux on both lanes, `decodeSegment` per-segment decoding — the plan's "follow `http.go` exactly" instruction is actionable as written. - **SSE/timeline carry no bytes — CONFIRMED, private-repo story holds.** `StreamEvent` (`internal/issues/issues.go:100-106`) is `{Name, Repo, IssueNum}` only; bodies travel exclusively in authed timeline GETs. So gating the byte GET at read level closes the leak: the only cross-boundary artifact is a URL string, and guessing it yields 401/403/404 without source-repo read. §3's rationale is correct. - **Store CAS gives you the dedup — CONFIRMED.** `PutCreate` is create-if-absent with 412-style `ErrKindPreconditionFailed` on exists (`internal/store/store.go:61,118-153`); hash-in-key means a 412 can only be same-bytes → idempotent 201 without re-read. No CAS loop needed. Correct. - **`web/css/repo.css:42` does NOT apply to the live UI.** The SolidJS entry (`web/src/index.jsx:6`) imports only `web/src/ui.css`, whose `.markdown-body` block (lines 73-81) has **no `img` rule**. The plan's hedge ("verify…; add `max-w-full` if not") resolves to: the `if not` branch triggers — state it outright (should-fix #5 below). ## BLOCKING (must be resolved before merge — frozen shape/seam) **B1. `Content-Length: required → 411/400` contradicts its cited precedent and breaks chunked clients.** `lfsPut` (`internal/server/lfs.go:212-274`) never checks `Content-Length`; the cap is enforced purely by `io.LimitReader(max+1)` → 413. There is no 411 anywhere in `internal/server`. Requiring `Content-Length` breaks chunked encodings for zero security benefit (the LimitReader already bounds the read). **Ruling: drop the requirement; LimitReader + 413 exactly like LFS.** Status codes freeze after merge — decide now. **B2. "Add the new routes to the discovery `endpoints[]`" is unimplementable as written.** Core discovery is *derived* from the `internal/api` route table (`internal/api/discovery.go:23-37`, `routes.go` `Expose` flags). Feature routes via the `Handle(w,r) bool` ExtraRoutes chain (`internal/issues`, `internal/identity`) register nothing there — I verified neither package references `Expose`/discovery, so **existing issue routes are already absent from `endpoints[]`**. The plan must pick one: (a) rule that repo-scoped feature routes are exempt from the 14 §14.12 discovery sentence (with a one-line Decisions entry saying so — my recommendation; the SDK builds repo-scoped paths statically anyway, `web/sdk/src/issues.js`), or (b) amend the discovery seam so extras can contribute templates. Cannot merge with the instruction in its current form since it demands something no feature does. **B3. Name where the byte-GET static-contract code lives — it cannot be reused.** `serveLFSObject`/conditionals are methods on `server.Server` (`internal/server/static.go:15-60,135+`); `internal/issues` imports only `server/auth`, `store`, `identity`, `git` — importing `server` would be an upward import into the package that wires it (law 8; likely a straight import cycle). So ETag/304/Range/206/416/HEAD handling must be **reimplemented (~60 lines) inside `internal/issues`** (or a new leaf). The plan says "static contract otherwise verbatim" without naming the home. **Ruling: implement in-package, table-test it like the LFS static tests, and say so in the plan.** (Related: persist the sniffed type via `PutOptions.ContentType` at `Create` — the object "carries no metadata" per 07 §1.2, so the *stored* content type is the only way the GET can emit it. With `X-Content-Type-Options: nosniff`, serving `application/octet-stream` will get the `<img>` blocked in Chrome — this detail is load-bearing for rendering, not cosmetic.) ## Should-fix **S1. Filename → markdown injection.** `![<name>](<url>)` built from an unsanitized filename breaks on `]`, `(`, `)` in names (e.g. `a](b.png`). The `insertAtCursor` helper must escape `]`/`(`/`)` in alt text (and the `<name>` URL segment must be percent-encoded at insert time). One-line rule + unit test in the headless `attachUpload.js` suite. **S2. Spell out the non-lane GET branch in `Handle`.** The byte GET (`/{o}/{r}/attachments/<sha>/<name>`) is *outside* `api|api-browser`, but current `Handle` only claims `segs[2] == api|api-browser` paths (`http.go:50-56`). The plan needs the new branch + collision analysis (there is none: no git sub-path is literally `attachments`, and it sits after `{o}/{r}` so no repo-name clash) + a fallthrough test (`false → core 404`, cf. `final_test.go:393-396` style). **S3. Config key needs explicit `config.go` scope.** `attachments.max_image_bytes` (new `[attachments]` section — consistent with `[lfs] max_object_bytes`) means: struct + `8<<20` default + validation + docs, all in the same change (law 12). Note `internal/setup` does not exist in code yet, so no setup-schema work is possible today — but record the key in the Decisions entry as setup-schema-pending so it isn't lost. The 8 MiB value itself is fine (screenshot-covering, orphan-bounding, keeps bodies meaningful). **S4. `crypto.subtle` needs a fallback position.** `crypto.subtle.digest` is undefined on non-secure origins (`http://` LAN hosts — a real walhub deployment shape). If the sha header stays mandatory (400 on mismatch), uploads break there with no recourse. **Ruling: make `X-Walgit-Attachment-Sha256` optional-when-present** (server always hashes the spool; verifies only if the client sent a value) — strictly weaker client requirement, same server-side integrity, and it keeps the 07-spec shape for secure contexts. Decide before merge since it changes the 400 contract. **S5. Just say `ui.css` needs the img rule.** Verification above: add `.markdown-body img { @apply max-w-full h-auto; }` (dark-theme check rides the existing Chromium drive). Delete the `web/css/repo.css` citation — that file isn't loaded by the SPA and citing it will confuse the implementer. **S6. PR adoption is not "free later".** `web/src/pages/Pull.jsx:124` renders comment bodies as **plain text** (`whitespace-pre-wrap`), not markdown — adopting the upload helper there later also requires switching PR rendering to `renderMarkdown`+`sanitize` (plus the release-body surface, spec-only). One line in §0/§7 noting the dependency; otherwise the "must not be issue-specific" requirement is still right and sufficient. (Third composer `Pull.jsx:134` is correctly identified; `Setup.jsx`/`Keys.jsx`/`Settings.jsx` textareas are config/keys/paste surfaces — correctly excluded.) **S7. Abuse ceiling belongs in the doc.** Authenticated-user orphan flood (≤8 MiB × attempts) is the same class as comment spam — no new anonymous vector since upload reuses the comment gate. Accept v1 as planned, but write the ceiling down (expected bytes/day at human rate) so the future `attachment-sweep` maintainer kind has a trigger criterion. Retention math: even 100 abandoned uploads/day = ≤800 MiB/day worst case, human-rate reality is orders of magnitude less; Create-only immutable objects need no frozen-list entry (14 §14.11 rule 2 covers *overwritable* keys only — the plan is right). ## Nits - N1. `07 §8` citations are spec-following, not code precedent (no `internal/releases`, no `uploadAsset` in the SDK) — relabel to avoid sending the implementer hunting for code that isn't there. LFS-spool line refs should point at `lfs.go:228-244`, not `408-457` (the latter is the read-through half). - N2. Dedup oracle (§9.7): accept — and it's narrower than stated. Keys are repo-scoped (`repos/<o>/<r>/…`), so cross-repo probing is impossible; a same-repo prober already holds read+auth and learns existence-only, never bytes. One sentence covers it. - N3. Oversize-submit UX: upload and comment-submit are decoupled, so a body pushed over 64 KiB by many image links 400s *after* the bytes are stored (orphans by design). Accept — but have the composer pre-check projected length client-side before uploading, or at minimum surface the 400 verbatim per the plain-text contract (already planned in §4). - N4. No EVIDENCE entry needed — agreed with §6. Collaboration surface, human-rate, sim budgets untouched. The only numbers that belong anywhere are the S7 ceiling in the Decisions entry. ## Rulings on §9 (explicit, as asked) 1. **SVG: REJECT (415)** — confirmed. Same-origin serving = script execution in our origin; the sanitizer is a preview-side string filter, not a server XML sanitizer, and the dependency budget forbids pulling one in. PNG/JPEG/GIF/WebP cover the screenshot use case. 2. **Cross-repo quoting: accept the breakage** — gate at source-repo read, no signed URLs in v1. Matches GitHub private-asset behavior; no byte-leak channel exists (SSE frames carry no bodies, verified). 3. **Retention: keep orphans in v1** — accept the tech debt, but record the future sweep kind name + trigger + reference rule (body contains URL) in the Decisions entry. No two-phase upload (wire surface doubles for no v1 benefit). 4. **Package home: `internal/issues`** — auth-gate affinity, one coverage gate, no new wiring. Either satisfies Seam 1; this one is cheaper. 5. **Cap + key: 8 MiB under `[attachments] max_image_bytes`** — confirmed, with the S3 config-scope note. 6. **GIF/WebP: agreed, no server work** — include an animated GIF + a WebP in the Chromium matrix. 7. **Dedup oracle: accept** — repo-scoped keys make it existence-only for already-authorized readers (N2). ## Acceptance delta (add to §8) - Byte-GET table tests: 304/206/416/HEAD + `private, immutable` + sniffed `Content-Type` + `nosniff` (B3); 411-must-not-exist (B1). - Discovery decision recorded (B2); config default/validation tests (S3); sha-header-optional test (S4). - `node --test` for alt-text escaping + placeholder replace (S1); `ui.css` img rule + dark/light Chromium paste-drive with console clean (S5). - `make cover` ≥95% on the touched package; `make fmt && make vet`; docs updated same-change (02_issues attachments subsection + Decisions entries in 02/06/14-as-touched). --- *Review method: read the plan end to end, then verified each load-bearing claim against code (`internal/issues/service.go`, `http.go`, `model.go`, `issues.go`; `internal/server/lfs.go`, `static.go`; `internal/store/store.go`; `internal/api/discovery.go`; `web/src/lib/markdown.js`, `sanitize.js`, `ui.css`, `index.jsx`; `web/src/pages/IssueNew.jsx`, `Issue.jsx`, `Pull.jsx`; `web/sdk/src/core.js`, `issues.js`; `internal/config/config.go`). No production code written, nothing committed, `git status` untouched (pre-existing worktree modifications left as found).*

crueber commented

2026-09-05 06:39:15 +00:00

Author

Owner

Plan revision R1 (incorporates plan-review findings — R1 wins on conflict)

Blocking rulings (normative)

B1 — drop Content-Length-required/411. Cap enforced purely by io.LimitReader(max+1) → 413, exactly like lfsPut. No 411 anywhere.
B2 — discovery: repo-scoped ExtraRoutes features are EXEMPT from the 14 §14.12 discovery sentence (one-line Decisions entry; existing issue routes are already absent from endpoints[]; the SDK builds repo-scoped paths statically). No discovery-seam amendment.
B3 — byte-GET static contract reimplemented IN-PACKAGE (internal/issues; importing server would be upward/cycle). Table-test 304/206/416/HEAD + private, immutable + sniffed Content-Type + nosniff. Persist the sniffed type via PutOptions.ContentType at Create (else Chrome blocks <img> under nosniff). Add the non-lane attachments/<sha>/<name> branch in Handle + collision analysis + fallthrough test.

Should-fix adoptions (all)

S1: escape ]/(/) in alt text, percent-encode name segment at insert; unit tests in attachUpload.js suite.
S2: covered by B3 (branch + collision note + fallthrough test).
S3: [attachments] max_image_bytes = 8 MiB: struct + default + validation + docs same change; record as setup-schema-pending in Decisions (no internal/setup in code yet).
S4: X-Walgit-Attachment-Sha256 OPTIONAL-when-present (server always hashes spool; verifies only if sent) — keeps http:// LAN hosts working.
S5: add .markdown-body img { max-w-full h-auto } to web/src/ui.css; drop the web/css/repo.css citation (not loaded by the SPA).
S6: note PR-body adoption also requires switching Pull.jsx rendering to markdown+sanitize (dependency, out of scope).
S7: abuse ceiling + future attachment-sweep trigger in Decisions; orphans kept in v1 (no two-phase).

§9 rulings (all confirmed)

SVG → 415 reject. 2. Cross-repo quoting breakage accepted, no signed URLs. 3. Orphans kept; sweep kind named in Decisions. 4. Package home: internal/issues. 5. Cap/key confirmed (8 MiB, [attachments] max_image_bytes). 6. GIF/WebP in browser matrix, no server work. 7. Dedup oracle accepted (repo-scoped, existence-only).

Nits folded in

07 citations relabeled spec-following; LFS refs → lfs.go:228-244; composer pre-checks projected 64 KiB length before upload (or surfaces 400 verbatim); no EVIDENCE entry.

# Plan revision R1 (incorporates plan-review findings — R1 wins on conflict) ## Blocking rulings (normative) - **B1 — drop `Content-Length`-required/411.** Cap enforced purely by `io.LimitReader(max+1)` → 413, exactly like `lfsPut`. No 411 anywhere. - **B2 — discovery: repo-scoped ExtraRoutes features are EXEMPT from the 14 §14.12 discovery sentence** (one-line Decisions entry; existing issue routes are already absent from `endpoints[]`; the SDK builds repo-scoped paths statically). No discovery-seam amendment. - **B3 — byte-GET static contract reimplemented IN-PACKAGE** (`internal/issues`; importing `server` would be upward/cycle). Table-test 304/206/416/HEAD + `private, immutable` + sniffed `Content-Type` + `nosniff`. Persist the sniffed type via `PutOptions.ContentType` at `Create` (else Chrome blocks `<img>` under nosniff). Add the non-lane `attachments/<sha>/<name>` branch in `Handle` + collision analysis + fallthrough test. ## Should-fix adoptions (all) - **S1:** escape `]`/`(`/`)` in alt text, percent-encode name segment at insert; unit tests in `attachUpload.js` suite. - **S2:** covered by B3 (branch + collision note + fallthrough test). - **S3:** `[attachments] max_image_bytes` = 8 MiB: struct + default + validation + docs same change; record as setup-schema-pending in Decisions (no `internal/setup` in code yet). - **S4:** `X-Walgit-Attachment-Sha256` OPTIONAL-when-present (server always hashes spool; verifies only if sent) — keeps `http://` LAN hosts working. - **S5:** add `.markdown-body img { max-w-full h-auto }` to `web/src/ui.css`; drop the `web/css/repo.css` citation (not loaded by the SPA). - **S6:** note PR-body adoption also requires switching `Pull.jsx` rendering to markdown+sanitize (dependency, out of scope). - **S7:** abuse ceiling + future `attachment-sweep` trigger in Decisions; orphans kept in v1 (no two-phase). ## §9 rulings (all confirmed) 1. SVG → 415 reject. 2. Cross-repo quoting breakage accepted, no signed URLs. 3. Orphans kept; sweep kind named in Decisions. 4. Package home: `internal/issues`. 5. Cap/key confirmed (8 MiB, `[attachments] max_image_bytes`). 6. GIF/WebP in browser matrix, no server work. 7. Dedup oracle accepted (repo-scoped, existence-only). ## Nits folded in 07 citations relabeled spec-following; LFS refs → `lfs.go:228-244`; composer pre-checks projected 64 KiB length before upload (or surfaces 400 verbatim); no EVIDENCE entry.

crueber referenced this issue from a commit

2026-09-05 07:32:36 +00:00

Feature #120: paste/drop image attachments in issue comments (docs/features/02 §12)

crueber referenced this issue

2026-09-05 07:33:07 +00:00

Feature #120: image paste/drop attachments #129

crueber commented

2026-09-05 07:33:15 +00:00

Author

Owner

Implemented in PR #129 (#129) — ready for review, not merged.

Highlights: POST /{o}/{r}/api/attachments + GET|HEAD byte route (ChainRepo/HandleRepo seam — one correction from the review: a non-lane Handle branch would be dead code, verified live), [attachments] max_image_bytes = 8MiB, SDK + shared composer helper, glue in IssueNew + CommentComposer (Issue.jsx only). Verification: 96.1% cover on internal/issues (-race), 288/288 node tests, real-Chromium paste drive dark+light with rendered images and zero console errors. Deviations from plan: none (R1 followed verbatim); one seam refinement documented in the 02 Decisions entry.

Implemented in PR #129 (https://git.packden.us/crueber/walhub/pulls/129) — ready for review, not merged. Highlights: POST /{o}/{r}/api/attachments + GET|HEAD byte route (ChainRepo/HandleRepo seam — one correction from the review: a non-lane Handle branch would be dead code, verified live), [attachments] max_image_bytes = 8MiB, SDK + shared composer helper, glue in IssueNew + CommentComposer (Issue.jsx only). Verification: 96.1% cover on internal/issues (-race), 288/288 node tests, real-Chromium paste drive dark+light with rendered images and zero console errors. Deviations from plan: none (R1 followed verbatim); one seam refinement documented in the 02 Decisions entry.

crueber commented

2026-09-05 07:37:08 +00:00

Author

Owner

Review: PR #129 (feat/issue-120) vs R1 spec — PASS with 2 micro-fixes (pushed as 69c759a).

R1 blocking rulings

B1 (no 411, LimitReader/413): PASS. UploadAttachment uses io.LimitReader(max+1)->ErrTooLarge->413 (internal/issues/attachments.go:214-220, errors.go:47-48); no Content-Length check anywhere in the package. Table test 'no Content-Length required (B1)' with ContentLength=-1 passes (attachments_test.go:135-141).
B2 (discovery-exempt Decisions entry): PASS. docs/features/02_issues.md Decisions: 'Repo-scoped ExtraRoutes features are EXEMPT from the 14 §14.12 discovery sentence (existing issue routes are already absent from endpoints[]...)'. No discovery-seam amendment, endpoints[] untouched. Correct per 14:478 (01/02/03/C2/05/06 rule).
B3 (in-package static contract + ContentType + branch + collision + fallthrough): PASS with seam refinement. serveAttachment reimplemented in-package (attachments.go:326-442; no server import — law 8 clean), PutOptions{ContentType: ct, Immutable: true} persisted at Create (attachments.go:261-262). 304/206/416/HEAD + private,immutable + sniffed CT + nosniff all table-tested (attachments_test.go:313-411). Non-lane branch lives in HandleRepo, NOT Handle — CORRECT: ExtraRoutes chain is only reachable via apiServe on lane paths (internal/server/router.go:253-257, bind_api.go:76-84), so a Handle byte branch would be dead code; HandleRepo rides the sanctioned ChainRepo/RepoRoutes seam (repo_extra.go:22-30, established by the 07 amendment, 14_extensibility.md:478) exactly like releases (cmd/walhub/releases.go:50). Collision analysis in code comment (attachments.go:306-313) + fallthrough tests both chains (attachments_test.go:439-448, 462-521). Decisions entry documents the refinement.

Routing seam verdict (law 8)

Sanctioned seam, not invented: 07 amendment explicitly blesses 'ExtraRoutes chain for lanes plus ChainRepo for the byte family'. Composition wires both (cmd/walhub/collab.go:166-179). No new seam, no doc amendment needed beyond the 02 Decisions entry (present). The B3/S2 'Handle branch' wording is superseded by the documented correction.

§9 + S1–S7

SVG->415 (sniffImageType rejects; tests: svg/xml/truncated/riff-not-webp), client sha optional-when-present + server always hashes (normalizeSHA256/S4 test), 8 MiB cap + [attachments] struct/default/generic-ByteSize validation + TOML round-trip test (config.go:263-264,430-432; validate_test.go:327-353), read-gated bytes via requireRead (anon 401 upload; private-repo anon 401 / stranger 403 on GET+POST; anon public-GET 200 = read-gate parity — attachments_test.go:212-260, 523-547), S1 escapeAlt + PathEscape URL + headless tests (attachUpload.js:46-63, attach-upload.test.js:21-29), 64 KiB projectedLengthOk pre-check + verbatim reportError (attachUpload.js:97-100,143-170), placeholder->replacePlaceholder flow, sanitizer/markdown untouched (not in diff), ui.css .markdown-body img max-w-full h-auto, no new deps (stdlib-only backend; package.json untouched), auth = AddComment parity (requireAuthenticated+requireRead), orphans-kept + 800 MiB/day ceiling + attachment-sweep named in Decisions, docs accurate (wire tables in 02 §7/§12, 06 Decisions private-cache entry, 11 config row).

Reviewer fixes pushed (`69c759a`, re-tested)

Removed dead attachmentSpoolDir helper + its test (attachments.go:546-553; composition inlines the join like releases — helper was tested but never called, comment even mis-cited releases wiring).
Restored accidentally dropped repo.issues.get in the 02 SDK list (SDK still has get; docs/code disagreed — law 12).

Verification (scratch worktree /tmp/wt120; main worktree untouched read-only)

go test -race ./internal/issues/... ./internal/config/... : ok (16.5s/1.1s)
coverage: issues 96.1%, config 95.5% (gate holds)
node --test web/test/unit/*.test.js: 288/288 (2 initial failures were missing node_modules in the scratch worktree only — symlinked main's install to run, then removed the link)
gofmt clean, go vet clean
Not run (per review instructions): docker builds, browser drive. Author claims real-Chromium paste drive dark+light, zero console errors — taken on trust; ladder #8 stays with the author.

MERGE RECOMMENDATION: ready to merge.

Review: PR #129 (feat/issue-120) vs R1 spec — PASS with 2 micro-fixes (pushed as 69c759a). ## R1 blocking rulings - **B1 (no 411, LimitReader/413): PASS.** UploadAttachment uses io.LimitReader(max+1)->ErrTooLarge->413 (internal/issues/attachments.go:214-220, errors.go:47-48); no Content-Length check anywhere in the package. Table test 'no Content-Length required (B1)' with ContentLength=-1 passes (attachments_test.go:135-141). - **B2 (discovery-exempt Decisions entry): PASS.** docs/features/02_issues.md Decisions: 'Repo-scoped ExtraRoutes features are EXEMPT from the 14 §14.12 discovery sentence (existing issue routes are already absent from endpoints[]...)'. No discovery-seam amendment, endpoints[] untouched. Correct per 14:478 (01/02/03/C2/05/06 rule). - **B3 (in-package static contract + ContentType + branch + collision + fallthrough): PASS with seam refinement.** serveAttachment reimplemented in-package (attachments.go:326-442; no server import — law 8 clean), PutOptions{ContentType: ct, Immutable: true} persisted at Create (attachments.go:261-262). 304/206/416/HEAD + private,immutable + sniffed CT + nosniff all table-tested (attachments_test.go:313-411). Non-lane branch lives in HandleRepo, NOT Handle — CORRECT: ExtraRoutes chain is only reachable via apiServe on lane paths (internal/server/router.go:253-257, bind_api.go:76-84), so a Handle byte branch would be dead code; HandleRepo rides the sanctioned ChainRepo/RepoRoutes seam (repo_extra.go:22-30, established by the 07 amendment, 14_extensibility.md:478) exactly like releases (cmd/walhub/releases.go:50). Collision analysis in code comment (attachments.go:306-313) + fallthrough tests both chains (attachments_test.go:439-448, 462-521). Decisions entry documents the refinement. ## Routing seam verdict (law 8) Sanctioned seam, not invented: 07 amendment explicitly blesses 'ExtraRoutes chain for lanes plus ChainRepo for the byte family'. Composition wires both (cmd/walhub/collab.go:166-179). No new seam, no doc amendment needed beyond the 02 Decisions entry (present). The B3/S2 'Handle branch' wording is superseded by the documented correction. ## §9 + S1–S7 SVG->415 (sniffImageType rejects; tests: svg/xml/truncated/riff-not-webp), client sha optional-when-present + server always hashes (normalizeSHA256/S4 test), 8 MiB cap + [attachments] struct/default/generic-ByteSize validation + TOML round-trip test (config.go:263-264,430-432; validate_test.go:327-353), read-gated bytes via requireRead (anon 401 upload; private-repo anon 401 / stranger 403 on GET+POST; anon public-GET 200 = read-gate parity — attachments_test.go:212-260, 523-547), S1 escapeAlt + PathEscape URL + headless tests (attachUpload.js:46-63, attach-upload.test.js:21-29), 64 KiB projectedLengthOk pre-check + verbatim reportError (attachUpload.js:97-100,143-170), placeholder->replacePlaceholder flow, sanitizer/markdown untouched (not in diff), ui.css .markdown-body img max-w-full h-auto, no new deps (stdlib-only backend; package.json untouched), auth = AddComment parity (requireAuthenticated+requireRead), orphans-kept + 800 MiB/day ceiling + attachment-sweep named in Decisions, docs accurate (wire tables in 02 §7/§12, 06 Decisions private-cache entry, 11 config row). ## Reviewer fixes pushed (69c759a, re-tested) 1. Removed dead attachmentSpoolDir helper + its test (attachments.go:546-553; composition inlines the join like releases — helper was tested but never called, comment even mis-cited releases wiring). 2. Restored accidentally dropped repo.issues.get in the 02 SDK list (SDK still has get; docs/code disagreed — law 12). ## Verification (scratch worktree /tmp/wt120; main worktree untouched read-only) - go test -race ./internal/issues/... ./internal/config/... : ok (16.5s/1.1s) - coverage: issues 96.1%, config 95.5% (gate holds) - node --test web/test/unit/*.test.js: 288/288 (2 initial failures were missing node_modules in the scratch worktree only — symlinked main's install to run, then removed the link) - gofmt clean, go vet clean - Not run (per review instructions): docker builds, browser drive. Author claims real-Chromium paste drive dark+light, zero console errors — taken on trust; ladder #8 stays with the author. MERGE RECOMMENDATION: ready to merge.

crueber commented

2026-09-05 07:37:26 +00:00

Author

Owner

Implemented per plan+R1 in PR #129 (review: R1 compliance verified point-by-point, routing seam sanctioned; 96.1%/95.5% + 288/288), merged. Closing.

crueber closed this issue

2026-09-05 07:37:26 +00:00

crueber referenced this issue from a commit

2026-09-05 07:37:26 +00:00

Merge pull request 'Feature #120: image paste/drop attachments' (#129) from feat/issue-120 into main

crueber added the

issues