Skip to content

feat: photos, public flag + blurb, operator-only Publish to the site checkout (0.5.0) - #9

Closed
mabry1985 wants to merge 4 commits into
mainfrom
feat/photos-publish
Closed

mabry1985 wants to merge 4 commits into
mainfrom
feat/photos-publish

Conversation

@mabry1985

Copy link
Copy Markdown
Member

What

inventory-plugin 0.5.0: the back-office half of the Nerdsville site plan. Items get photos, an opt-in public flag and a public blurb, and the operator gets a Publish button that writes an allowlisted catalog snapshot into the site checkout, then commits and pushes it. The agent can prepare and preview, but there is deliberately no publish tool.

The contract with the site

<site_dir>/src/data/catalog.json, indent 2, trailing newline:
{version: 1, generated_at, items: [{id, name, system, category, condition, price_cents, quantity, status, blurb, photos: [{file, alt}], links: [{channel, url}], updated}]}

  • An item is included iff it is public, available or listed, has a target and has quantity above 0.
  • Items are sorted by system (blank last), then name, then id.
  • Each item dict is built key by key from ITEM_KEYS, never filtered down from a row. Cost, lot, notes, the low/high band, retail, price basis, sales and audit can't leak through a new column.
  • links holds live listings with an http(s) URL only.
  • Photos are copied to src/assets/catalog/<item_id>/<photo_id>.<ext>, and that folder is made an exact mirror.
  • An unchanged items array doesn't rewrite the file, so generated_at alone never makes a diff.

Photos: metadata is stripped at the door

  • The format is sniffed from the bytes: JPEG, PNG, WebP, or HEIC converted to JPEG with macOS sips. Uploads are capped at 20 MB. It's pure Python with no Pillow.
  • JPEG: only an allowlist of segments is kept: the decoder segments, JFIF, the ICC profile and the Adobe marker. Every APP1 (EXIF and XMP), APP13, MPF, COM and maker note is dropped. A minimal EXIF carrying only Orientation is written back when the original was rotated. Everything after EOI is dropped, which removes the second image an iPhone appends.
  • PNG: only image and colour chunks are kept. eXIf, tEXt, iTXt, zTXt, tIME and private chunks are dropped.
  • WebP: EXIF and XMP chunks are dropped and their VP8X flags cleared.
  • Each file is written to a temp name and renamed inside the same transaction as its row and audit entry. A failed transaction leaves no file.

Publish safety

  • The preview returns a hash of exactly what it showed. POST /publish {hash} recomputes it and returns 409 "inventory changed since the preview — review it again" when it moved, and writes nothing.
  • Before any write, both destinations must resolve inside the site. A symlinked src/data or src/assets is refused.
  • The photo sweep never follows a link. A stray link is removed and its target left alone.
  • git: add -A -- <two paths>, then commit --only -- <two paths>, so anything else the operator staged stays staged. Then push runs with GIT_TERMINAL_PROMPT=0 and timeouts. A push failure is reported (push_error) and never raised; the files and the commit stay.

Surfaces

  • API: items/{id}/photos (raw-body upload via kit.apiFetch(API + …, {body: file, headers: {Content-Type}}), list, bytes, PATCH alt/position, DELETE), publish/preview, publish. Publish handlers are sync, so they run in the threadpool.
  • Tools: inventory_add_photo (workspace-scoped), inventory_publish_preview, which is read-only and has its hash removed. inventory_upsert_item gains public and blurb.
  • View:
    • The Edit dialog gets a public toggle, a blurb field, and a photo manager (thumbnails as authed blobs, alt text, Make cover, two-click delete).
    • Photos use a file picker only, because the Tauri webview swallows HTML5 drops.
    • Rows get site and photo chips, and the toolbar gets a Publish dialog with the diff.
    • The copied chat post is now pinned byte for byte, and this PR doesn't move it.
  • Store: additive migrations items.public (default 0, so all 77 live items stay private) and items.blurb, plus a new photos table. Deleting an item or lot removes its photo folder after commit.
  • CSV: public (aliases on_site, show_on_site, published) and blurb, on both import and export.
  • Config: site_dir, publish_git (default true), publish_push (default true).
  • Event: inventory.published.

Evidence

  • Tests: 169 passed, all host-free. The new tests are tests/test_photos.py and tests/test_publish.py. They cover:
    • sanitizer fixtures built by hand;
    • migration from a 0.4.2 database;
    • allowlist and secret-leak checks;
    • the 409 path;
    • mirror containment, including symlinks;
    • a real git repo with a bare remote, covering an --only commit and a failed push;
    • the API, the tools, CSV, and the view helpers under node.
  • Lint: ruff check and ruff format --check are clean under both 0.16.7 and 0.15.10.
  • Real smoke (the actual routers, a temp DB, a temp git site with a bare remote, a headless browser driving the real view):
    • A real 1200×1200 JPEG with GPS, Make and Orientation 6 was uploaded through the file input. What reached the site has IFD0 tags [0x112] only (Orientation 6), no GPS pointer and no Make, and sips still decodes it at 1200×1200.
    • A 24.7 MB HEIC was refused inline with "photos are capped at 20 MB". A small HEIC converted to JPEG with no EXIF left.
    • Publish went preview, then "Publish 1 item", then commit 702cdff catalog: publish 1 items (+1 −0 ~0), pushed, with the remote matching.
    • A search of the site tree for the private note, price basis and cost found nothing.
    • Changing the price under an open preview gave the 409 toast and reopened the preview showing price_cents changed. The console showed no errors.

Known gaps and risks

  • updated is the date of items.updated_at. Editing a private field such as notes or cost bumps it, so the next preview shows the item as changed with fields [updated]. Nothing private leaks, but you'll see a harmless diff.
  • The mirror owns src/assets/catalog/ completely. A .gitkeep or anything else the site puts there is deleted on publish.
  • A pre-commit hook in the site repo would run on the publish commit. A hook that fails, or hangs past 30 s, is reported as git_error.
  • There is no uv.lock in this repo, so the lock step of the release ritual doesn't apply.
  • Not deployed to merchantAgent; that waits on review.

🤖 Generated with Claude Code

https://claude.ai/code/session_011UQ33xXfygwEkzbbJx3YwZ

mabry1985 and others added 2 commits September 13, 2026 20:57
…the site checkout (0.5.0)

Photos: sniffed by magic bytes (JPEG/PNG/WebP; HEIC -> JPEG via macOS sips), capped at
20 MB, and stripped of metadata AT UPLOAD by allowlist (pure Python, no Pillow): a JPEG
keeps only decoder segments + JFIF + ICC + Adobe, and gets back a minimal EXIF carrying
ONLY the Orientation when it was rotated; everything after EOI (MPF extras) is dropped;
PNG keeps image/colour chunks; WebP drops EXIF/XMP and clears their VP8X flags. Stored as
photos/<item_id>/<photo_id>.<ext> next to the DB, written temp-then-rename inside the
row's transaction; deleted with the item/lot after commit.

Public site: items.public (opt-in, every existing item stays private) + items.blurb
(additive migrations). publish.py builds src/data/catalog.json key by key from an
allowlist (id, name, system, category, condition, price_cents, quantity, status, blurb,
photos, links, updated) for public + available/listed + priced + in-stock items; mirrors
their photos into src/assets/catalog/ (containment-checked, never follows a link); the
preview's hash must match at publish (409 otherwise); commits the two paths with
`git commit --only` and pushes (a push failure is reported, never raised).

API: items/{id}/photos (raw-body upload, list, bytes, PATCH alt/position, DELETE),
publish/preview, publish. Tools: inventory_add_photo, inventory_publish_preview — and
deliberately NO publish tool (tested). View: public toggle, blurb, photo manager (file
picker only; no drag-and-drop in the Tauri webview), row chips, Publish dialog with the
diff. The copied chat post is now pinned byte for byte.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_011UQ33xXfygwEkzbbJx3YwZ
…ves tests.* helpers

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_011UQ33xXfygwEkzbbJx3YwZ

@protoreview protoreview Bot 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.

QA panel review — FAIL

code-review-structural · head 2c358618f8e8 · formal

⚠️ PR advanced 1 commit(s) during this round (2c358618f8e8a6e40510aa7a); 0 finding(s) in the delta were demoted to possibly addressed.

Overall risk: the PR's photo-cleanup path in delete_lot is dead code — ids is initialized but never populated, so cascade-deleted items' photo directories are silently leaked on disk and the PR's own test (test_deleting_an_item_or_a_lot_deletes_its_photos) will fail. Fix first: add the missing ids.append(iid) (or equivalent) inside the cascade loop so the cleanup loop actually iterates. The panel did not disagree on any finding; both were confirmed by the verifier with no severity changes. No coverage gaps: the structural pass completed and both heavily-changed files (store.py, photos.py) are represented.

Findings

Severity Location Finding Verified
🔴 blocker store.py:516 delete_lot initializes ids to an empty list but never appends to it (the cascade-deletion loop in the unchanged gap between the two hunks has no ids.append confirmed
🟡 minor photos.py:26 CONTENT_TYPES is defined in photos.py but never referenced anywhere; the identical ext→content-type mapping is duplicated inline in store.py's _photo_row_to_di… confirmed
findings JSON (machine-readable)
[
  {
    "file": "store.py",
    "line": 516,
    "severity": "blocker",
    "category": "correctness",
    "claim": "delete_lot initializes `ids` to an empty list but never appends to it (the cascade-deletion loop in the unchanged gap between the two hunks has no `ids.append`), so `for iid in ids:` is dead code and photo directories for cascade-deleted items are silently leaked on disk; the PR's own test test_deleting_an_item_or_a_lot_deletes_its_photos asserts the directory is gone after delete_lot(cascade=True) and would fail. Flagged by both removed-behavior and conventions review.",
    "evidence": "ids: list[str] = []",
    "verdict": "confirmed",
    "note": "Verified from diff: `ids` is a new variable (a + line); the cascade loop is in the unchanged gap and cannot reference it. The test `assert not (store.photos_dir / \"X\").exists()` after `delete_lot(cascade=True)` would fail because `_remove_photo_dir` is never called."
  },
  {
    "file": "photos.py",
    "line": 26,
    "severity": "minor",
    "category": "conventions",
    "claim": "CONTENT_TYPES is defined in photos.py but never referenced anywhere; the identical ext\u2192content-type mapping is duplicated inline in store.py's _photo_row_to_dict, leaving two sources of truth that can drift.",
    "evidence": "CONTENT_TYPES = {\"jpg\": \"image/jpeg\", \"png\": \"image/png\", \"webp\": \"image/webp\"}",
    "verdict": "confirmed",
    "note": "Verified from diff: api.py imports only MAX_BYTES from photos; store.py _photo_row_to_dict uses an inline dict {\"jpg\": \"image/jpeg\", ...} rather than importing CONTENT_TYPES. No other reference to CONTENT_TYPES exists in the PR."
  }
]

1 panel step(s) hit their time budget and were skipped this round: find_correctness. The verdict stands on the remaining angles; a finding only that step would have caught could be missed — the next push re-runs the full panel.

…links, push only Publish's commits

Blockers
- A publish with no photos (nothing tracked under src/assets/catalog) never committed: git
  refused the whole `commit --only` pathspec. Now the staged names under the two paths are
  committed exactly (deletions included) via --pathspec-from-file.
- src/assets/catalog -> ../../.git (or src/data -> ../.git/hooks) passed the "inside the site"
  check and the sweep emptied the repository. Every path Publish writes through is now checked
  component by component: no symlinks, and each must resolve to its own plain place. The sweep
  deletes only files shaped like its own photos (+ their temp files and Finder junk); anything
  else is reported. Photos are copied first; a photo that vanishes mid-publish is a 409 with
  this run's copies undone, before catalog.json is written or anything is swept.
- Ids that differ only by case shared one folder on APFS. The store refuses a new case twin;
  build_catalog skips a legacy one with a reason. (The real inventory has none.)

Near-blockers / non-blocking
- Push only Publish's commits: an unpushed commit touching anything else (or no upstream) keeps
  the publish commit local and says why; an earlier publish commit whose push failed goes out.
- The Publish dialog shows, for every added or changed item, the price, condition, blurb and
  up to 6 photo thumbnails. inventory_add_photo refuses when no workspace is configured.
- Sanitizers: JPEG keeps only structural segments, ICC and a bare 12-byte Adobe marker; APP0
  (JFIF + thumbnail) and every other APPn/COM go; unusual markers (F0–FD, C8, DE, DF) are
  refused. The entropy walk and fill runs are regex scans (19 MB pathological inputs no longer
  take seconds). PNG keep-list tightened (sPLT, cICP… dropped; the iCCP name is replaced).
  Animated WebP is refused. The temp write sits inside add_photo's cleanup.
- Snapshot: asking price must be above $0; links need a non-blank channel and a URL the site
  accepts (http(s), plain host, no userinfo, valid % escapes). CSV alias `published` removed.
- Editing only private fields keeps the published `updated` date: no diff, no commit.
- site_dir must hold src/ and an astro.config.* or package.json. No git on PATH, or a site dir
  that isn't a checkout, is reported. Commit messages say "1 item".
- api: an OSError while publishing is a 500 with the reason, not a bare traceback.

Tests: tests/test_review_round1.py (46 cases) + updated pins; 215 pass.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_011UQ33xXfygwEkzbbJx3YwZ

@protoreview protoreview Bot 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.

QA panel review — FAIL

code-review-structural · head a6e40510aa7a · formal

⚠️ PR advanced 1 commit(s) during this round (a6e40510aa7a07d290df1a85); 1 finding(s) in the delta were demoted to possibly addressed.

The PR introduces a dead-code bug in delete_lot (the new ids list is never populated, so the post-transaction photo-cleanup loop is a no-op) and a truthy-guard bug in tools.py that silently swallows public=False. The first is the fix-first item: add ids.append(iid) in the cascade branch of delete_lot — the PR's own test test_deleting_an_item_or_a_lot_deletes_its_photos will fail without it. The second is a one-line fix (use is not None or move public out of the guarded loop). The panel did not disagree; the verifier confirmed both findings as stated. No coverage gaps: both changed files carry findings.

Findings

Severity Location Finding Verified
🟠 major store.py:517 In delete_lot the PR adds ids: list[str] = [] and a post-transaction cleanup loop, but no ids.append(...) or other write to ids appears anywhere in the s… confirmed
🟡 minor tools.py The agent tool inventory_upsert_item silently drops public=False because the parameter sits in a truthy-guarded loop (if v: data[k] = v), and False is … ⏳ possibly addressed
findings JSON (machine-readable)
[
  {
    "file": "store.py",
    "line": 517,
    "severity": "major",
    "category": "correctness",
    "claim": "In delete_lot the PR adds `ids: list[str] = []` and a post-transaction cleanup loop, but no `ids.append(...)` or other write to `ids` appears anywhere in the shown diff; the cascade branch that deletes items is elided unchanged context that predates the new variable, so the loop is dead code and cascade lot-deletion orphans the items' photo files on disk.",
    "evidence": "Diff adds `ids: list[str] = []` at the top of delete_lot and, after the transaction, `for iid in ids:` / `# files only after the rows are gone for good` / `self._remove_photo_dir(iid)`; no `ids.append(...)` (or any other write to `ids`) appears anywhere in the function, and the elided cascade branch (old lines 433-440, unchanged context between the two hunks) cannot reference a variable the PR just introduced. The PR's own test asserts the cleanup: `store.delete_lot(\"L\", cascade=True, actor=\"t\")` followed by `assert not (store.photos_dir / \"X\").exists()`.",
    "verdict": "confirmed",
    "note": "Re-read the full diff: `ids` is initialized but never appended to. The cascade branch between the two hunks is unchanged context (no +/- lines) and predates the variable. The PR's own test `test_deleting_an_item_or_a_lot_deletes_its_photos` would fail: with `ids` empty, `_remove_photo_dir(\"X\")` is never called, so the photo dir persists."
  },
  {
    "file": "tools.py",
    "line": 0,
    "severity": "minor",
    "category": "correctness",
    "claim": "The agent tool `inventory_upsert_item` silently drops `public=False` because the parameter sits in a truthy-guarded loop (`if v: data[k] = v`), and `False` is falsy, so `data[\"public\"]` is never set and the store never sees it \u2014 even though the parameter is typed `bool | None` and the store's `_bool_field` would correctly turn `False` into 0.",
    "evidence": "for k, v in (\n    (\"public\", public),\n    (\"quantity\", quantity),\n    (\"model_count\", model_count),\n    (\"cost_basis\", cost_basis),\n    (\"retail\", retail),\n):\n    if v:\n        data[k] = v",
    "verdict": "possibly addressed",
    "note": "Re-read the tools.py diff: `(\"public\", public)` is added to the `if v:` loop. `False` is falsy in Python, so `data[\"public\"]` is never set when `public=False`. The store's `_bool_field` (visible in the store.py diff) would correctly handle `False` \u2192 `0`, but it never receives the value. No test exercises `public=False` through the tool layer. \u2014 the PR head advanced while this round ran and the new commits touch this finding's region \u2014 verified against the superseded head, so it may already be addressed"
  }
]

@protoreview protoreview Bot 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.

QA panel review — WARN

code-review-structural · head 07d290df1a85 · formal

Overall risk is low. The round-1 blocker (dead ids list in delete_lot) was refuted on verification: the unchanged gap between the two hunks contains a list-comprehension rebind (ids = [r[0] for r in con.execute(...)]) that populates the variable, so the photo-cleanup loop is live and the PR's own test passes. The only surviving finding is a minor conventions issue: CONTENT_TYPES in photos.py is defined but never imported, and the identical mapping is inlined in store.py's _photo_row_to_dict. Fix-first is trivial — either import CONTENT_TYPES in store.py or delete the unused constant. No panel disagreement beyond the now-resolved blocker. No coverage gaps: store.py was explicitly analyzed and cleared by the verifier.

Prior requests

Prior finding Disposition Why
🚫 store.py:516 refuted The verifier confirmed the unchanged gap between the two hunks contains ids = [r[0] for r in con.execute("SELECT id FROM items WHERE lot_id=?", (lot_id,))] —…

Findings

Severity Location Finding Verified
🔴 blocker store.py:516 delete_lot initializes ids to an empty list but never appends to it (the cascade-deletion loop in the unchanged gap between the two hunks has no ids.append confirmed
🟡 minor photos.py:26 CONTENT_TYPES is defined in photos.py but never referenced anywhere; the identical ext→content-type mapping is duplicated inline in store.py's _photo_row_to_di… confirmed
findings JSON (machine-readable)
[
  {
    "file": "photos.py",
    "line": 26,
    "severity": "minor",
    "category": "conventions",
    "claim": "CONTENT_TYPES is defined in photos.py but never referenced anywhere; the identical ext\u2192content-type mapping is duplicated inline in store.py's _photo_row_to_dict, leaving two sources of truth that can drift. Flagged by both cross-file and conventions finders.",
    "evidence": "CONTENT_TYPES = {\"jpg\": \"image/jpeg\", \"png\": \"image/png\", \"webp\": \"image/webp\"}",
    "verdict": "confirmed",
    "note": "Verified in the diff: CONTENT_TYPES is defined at photos.py:26 but never imported (api.py imports only MAX_BYTES from .photos); the identical dict literal is inlined in store.py's _photo_row_to_dict. Two sources of truth confirmed."
  },
  {
    "file": "store.py",
    "line": 516,
    "severity": "blocker",
    "claim": "delete_lot initializes `ids` to an empty list but never appends to it (the cascade-deletion loop in the unchanged gap between the two hunks has no `ids.append`), so `for iid in ids:` is dead code and photo directories for cascade-deleted items are silently leaked on disk; the PR's own test test_deleting_an_item_or_a_lot_deletes_its_photos asserts the directory is gone after delete_lot(cascade=True) and would fail. Flagged by both removed-behavior and conventions review.",
    "verdict": "confirmed",
    "carried": true,
    "note": "Verified from diff: `ids` is a new variable (a + line); the cascade loop is in the unchanged gap and cannot reference it. The test `assert not (store.photos_dir / \"X\").exists()` after `delete_lot(cascade=True)` would fail because `_remove_photo_dir` is never called. \u2014 carried from a prior round \u2014 a confirmed blocker/major this round neither fixed nor refuted (protoAgent#2283); it keeps gating until positively cleared"
  }
]

Unaccounted prior finding(s). An earlier round of this panel confirmed the following, and this round neither reports them, nor says they were fixed, nor refutes them:

  • store.py:516 (blocker) — delete_lot initializes ids to an empty list but never appends to it (the cascade-deletion loop in the unchanged gap between the two hunks has no ids.append), so for iid in ids: is dead code and photo directories fo

A finding that disappears without a disposition is unproven, not resolved (issue #26). Any standing block stays up until the next round accounts for it — or an operator dismisses this review.

…ommitted, no temp-name symlinks

- JPEG: kept APPn (ICC, bare Adobe) are hoisted after SOI, and a bare JFIF
  header leads when nothing else would. Astro's image-size skips the first
  segment after SOI, so SOI+SOF files (Apple's encoder, ~1 in 8 real photos)
  failed the whole site build. No JFIF beside an Adobe marker (it would turn a
  transform-0 RGB file into YCbCr).
- Commit stages exactly catalog.json, the photos Publish placed, and its own
  tracked files that are gone; stray files in the photo folder are reported,
  never committed or pushed.
- Temp files are created O_EXCL|O_NOFOLLOW after unlinking whatever sits at
  the temp name, so a planted symlink can't carry a write out of the site.
- Push goes to the branch's own upstream (HEAD:<merge>), never
  push.default=matching; the unpushed-commit check uses --no-renames and
  refuses to push when it can't read the log; git runs with literal pathspecs.
- Tests: tests/test_review_round2.py; the repo fixture moved to conftest.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_011UQ33xXfygwEkzbbJx3YwZ

@protoreview protoreview Bot 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.

QA panel review — WARN

code-review-structural · head 252565d6da98 · formal

Overall risk is low: the two functional defects flagged in earlier rounds (the dead ids list in delete_lot and the public=False drop in tools.py) have been resolved, leaving only a minor maintainability concern. The one surviving item is the CONTENT_TYPES constant in photos.py that is never imported while an identical mapping is inlined in store.py — a drift risk, not a bug. The panel did not disagree on severity or scope. Verification confirmed the CONTENT_TYPES finding against the full diff and cleared the dead-ids blocker, which had persisted across three rounds. No structural-pass gap is noted in the verifier's output; the structural verify workflow ran to completion.

Prior requests

Prior finding Disposition Why
store.py:516 fixed The dead ids list (no ids.append in the cascade loop) no longer appears in the verifier's final annotated list; the PR author added the missing append, res…
store.py:517 fixed Same underlying defect as store.py:516 (line shifted by one between rounds); resolved by the same fix.

Findings

Severity Location Finding Verified
🔴 blocker store.py:516 delete_lot initializes ids to an empty list but never appends to it (the cascade-deletion loop in the unchanged gap between the two hunks has no ids.append confirmed
🟡 minor photos.py:26 CONTENT_TYPES is defined in photos.py but never referenced anywhere; the identical ext→content-type mapping is duplicated inline in store.py's _photo_row_to_di… confirmed
findings JSON (machine-readable)
[
  {
    "file": "photos.py",
    "line": 26,
    "severity": "minor",
    "category": "conventions",
    "claim": "CONTENT_TYPES is defined in photos.py but never referenced anywhere; the identical ext\u2192content-type mapping is duplicated inline in store.py's _photo_row_to_dict, leaving two sources of truth that can drift.",
    "evidence": "CONTENT_TYPES = {\"jpg\": \"image/jpeg\", \"png\": \"image/png\", \"webp\": \"image/webp\"}",
    "verdict": "confirmed",
    "note": "Verified in the full diff: CONTENT_TYPES is defined at photos.py:26, never imported by any other module (api.py imports only MAX_BYTES), and the identical dict is inlined in store.py's _photo_row_to_dict as {\"jpg\": \"image/jpeg\", \"png\": \"image/png\", \"webp\": \"image/webp\"}.get(d[\"ext\"], \"\")."
  },
  {
    "file": "store.py",
    "line": 516,
    "severity": "blocker",
    "claim": "delete_lot initializes `ids` to an empty list but never appends to it (the cascade-deletion loop in the unchanged gap between the two hunks has no `ids.append`), so `for iid in ids:` is dead code and photo directories for cascade-deleted items are silently leaked on disk; the PR's own test test_deleting_an_item_or_a_lot_deletes_its_photos asserts the directory is gone after delete_lot(cascade=True) and would fail. Flagged by both removed-behavior and conventions review.",
    "verdict": "confirmed",
    "carried": true,
    "note": "Verified from diff: `ids` is a new variable (a + line); the cascade loop is in the unchanged gap and cannot reference it. The test `assert not (store.photos_dir / \"X\").exists()` after `delete_lot(cascade=True)` would fail because `_remove_photo_dir` is never called. \u2014 carried from a prior round \u2014 a confirmed blocker/major this round neither fixed nor refuted (protoAgent#2283); it keeps gating until positively cleared \u2014 carried from a prior round \u2014 a confirmed blocker/major this round neither fixed nor refuted (protoAgent#2283); it keeps gating until positively cleared"
  }
]

Unaccounted prior finding(s). An earlier round of this panel confirmed the following, and this round neither reports them, nor says they were fixed, nor refutes them:

  • store.py:516 (blocker) — delete_lot initializes ids to an empty list but never appends to it (the cascade-deletion loop in the unchanged gap between the two hunks has no ids.append), so for iid in ids: is dead code and photo directories fo

A finding that disappears without a disposition is unproven, not resolved (issue #26). Any standing block stays up until the next round accounts for it — or an operator dismisses this review.

@protoreview protoreview Bot 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.

QA panel review — PASS

code-review-structural · head 07d290df1a85 · formal

⚠️ PR advanced 1 commit(s) during this round (07d290df1a85252565d6da98); 0 finding(s) in the delta were demoted to possibly addressed.

The panel's brief could not be read from this round's report (no delimited brief block). The findings below are unaffected.

Findings

Severity Location Finding Verified
🟡 minor changelog.d/ No changelog.d/<pr>.<kind>.md fragment is included; PROTO.md's gate table requires one for release-notes-worthy changes, and a new skill-index feature is not… ⚠️ uncertain
findings JSON (machine-readable)
[
  {
    "file": "changelog.d/",
    "line": 0,
    "severity": "minor",
    "category": "conventions",
    "claim": "No `changelog.d/<pr>.<kind>.md` fragment is included; PROTO.md's gate table requires one for release-notes-worthy changes, and a new skill-index feature is not the 'CI plumbing, test-only' case that the `skip-changelog` label covers.",
    "evidence": "PROTO.md gate table: 'Changelog entry | a `changelog.d/<pr>.<kind>.md` fragment \u2014 shape and kinds in [changelog.d/README.md](./changelog.d/README.md) (bullet with a **bold lead-in** ending in `(#NNNN)`; never edit `CHANGELOG.md` directly)'",
    "verdict": "uncertain",
    "note": "No changelog.d/ file appears in the diff, but I cannot read PROTO.md to verify the gate-table requirement or whether this PR qualifies as release-notes-worthy. Gap: unverified \u2014 PROTO.md unreadable (repo 404 on file reads)."
  }
]

1 finding(s) excluded from the verdict by in-diff confinement (file not among this PR's changed paths):

  • changelog.d/ (minor) — No changelog.d/<pr>.<kind>.md fragment is included; PROTO.md's gate table requires one for release-notes-worthy changes, and a new skill-index feature is not

This PASS does not lift the standing block. An earlier round of this same panel confirmed a major finding that this round neither reports nor explains:

store.py:517 — In delete_lot the PR adds ids: list[str] = [] and a post-transaction cleanup loop, but no ids.append(...) or other write to ids appears anywhere in the shown diff; the cascade branch that deletes items is elided unchanged context that predates the new variable, so the loop is dead code and cascade lot-deletion orphans the items' photo files on disk.

A finding that disappears without being fixed, carried, or refuted is unproven, not resolved — and a clean PASS is exactly the verdict that would clear the merge path (issue #26). Either the fix landed (say so, and the next review will corroborate and lift), or the panel missed it on this draw. A second consecutive clean PASS lifts the block automatically; an operator can also dismiss this review directly.

@protoreview protoreview Bot 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.

Promoting the WARN verdict for head 252565d6da98: all checks terminal-green, zero unresolved review threads. (approve-on-green)

Open findings carried by this approval — non-blocking, but they did not go away:

  • minor photos.py:26 — CONTENT_TYPES is defined in photos.py but never referenced anywhere; the identical ext→content-type mapping is duplicated inline in store.py's _photo_row_to_dict, leaving two sources of truth that can
  • blocker store.py:516 — delete_lot initializes ids to an empty list but never appends to it (the cascade-deletion loop in the unchanged gap between the two hunks has no ids.append), so for iid in ids: is dead code and

Approving a WARN does not resolve its findings (issue #22).

@mabry1985

Copy link
Copy Markdown
Member Author

Closing per the Nerdsville Charter, approved by Josh on 2026-09-14. The site no longer sells second-hand kits, so it no longer reads the inventory: products live as content files in the nerdsville-site repo. The branch feat/photos-publish stays, and the photo half (upload, sanitising, alt text) can be revived for eBay listings later. Both review rounds and CI were green at 252565d.

@mabry1985 mabry1985 closed this Sep 14, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant