feat(collections): move/copy assets with their lineage; archive a campaign

[mabry1985]

What

Part 2 of 3 of the library work (retention ✓ #29 → collections → protoContent archive). Josh: "a collection of our quality ones together and start a fresh collection." A collection is just a campaign. This PR adds the verbs it was missing:

• campaign_collect(target_campaign_id, asset_ids, mode=move|copy, lineage=true, dry_run=true)
  • Lineage = the retention closure: the parent chain (take ← render ← bookended clip), montage meta.inputs, and sequence clip ids. The chain still reads afterwards, and retention keeps it.
  • move re-homes rows: same ids, files, statuses and approvals. Lanes are matched by name in the target and created if missing.
  • copy duplicates rows and copies files into the target's folder (same layout), side files included (raw master, the take's stills and timing log). Every id and path in the copies is re-pointed, superseded_by chains included. It's idempotent (a re-run maps to the existing copy) and never overwrites an existing file.
• campaign_archive(campaign_id, archived=true) hides a campaign from campaign_list and the gallery picker, and restores its previous status on the way back. Assets are untouched.
• Approval stays the operator's. The agent can't move an approved asset. Its copy of an approved asset lands ready_for_review, and its copy of a rejected one lands rendered, so a rejection can't be laundered back into review. A copy that can't pass the review gate (no file) lands rendered/planned. The operator routes POST /campaigns/{id}/collect and POST /campaigns/{id}/archive (dry run by default) can do everything.
• GET /campaigns hides archived campaigns unless include_archived=true; the gallery has a Show archived toggle.
• Adversarial-review fix (B3): _supersede_take looked for an old take's stills, and for other users of its folder, only in the take's current campaign. After a move, a re-shoot could rmtree a folder holding an approved still left in the old campaign. It now searches every campaign; there's a regression test, and it fails without the fix.

All routes are sync (file copies stay off the event loop). Tests: tests/test_library.py, 23 cases.

🤖 Generated with Claude Code

[protoreview]

QA panel review — WARN

code-review-structural · head fc403ecb4232 · formal

⚠️ Coverage incomplete — this is not a clean pass. 1 of 5 review lane(s) did not complete a full pass this round: find_structural (structural pass cut short: 16 of 17 features reviewed — feature cap reached: the 16 features with the most changed lines were reviewed, 1 were not (structuralmaxfeatures=16)). Findings from the lanes that ran stand, but a defect only the missing lanes would catch may be missed, so a PASS is capped at WARN. Where the brief below implies full coverage, this line supersedes it. The next push re-runs the full panel.

All three findings were confirmed by the verifier with no changes to severity or claim. The panel disagreed on nothing — the LLM finders came back clean and the protoPatch structural pass supplied all three. No prior requests to disposition. The one to fix first is the /collect 400-vs-404 inconsistency if you want API contract consistency; the full-table scans are minor performance notes that can wait. No coverage gaps: the verifier accounted for all three findings it was given.

Findings

	Severity	Location	Finding	Verified
🟡	minor	tools.py:127	_supersede_take performs a full-table asset scan across all campaigns on every re-record; a targeted existence query would avoid deserializing every row's JSON…	confirmed
⚪	nit	api.py:167	The new /collect route maps ValueError to HTTP 400, but a non-existent campaign_id should yield 404 to match the /archive and /assets routes in the same file.	confirmed
⚪	nit	api.py:117	The new archived_hidden count requires a second full-table scan of campaigns on every GET /campaigns request; a single query or a count method would suffice.	confirmed

findings JSON (machine-readable)

[
  {
    "file": "tools.py",
    "line": 127,
    "severity": "minor",
    "category": "correctness",
    "claim": "_supersede_take performs a full-table asset scan across all campaigns on every re-record; a targeted existence query would avoid deserializing every row's JSON columns.",
    "evidence": "Line 128 calls `store.list_assets()` with no campaign filter to determine whether any asset in any campaign still has a file under `old_dir`. `list_assets` (in store.py) executes `SELECT * FROM assets` and deserializes every row's JSON columns.",
    "source": "protopatch",
    "verdict": "confirmed",
    "note": "tools.py: `still_used = any(a.get(\"path\") and Path(old_dir) in Path(a[\"path\"]).parents for a in store.list_assets())` \u2014 no campaign_id passed. store.py list_assets(campaign_id=0) skips the filter when campaign_id is falsy, yielding `SELECT * FROM assets WHERE 1=1 ORDER BY id`; _row() json.loads meta/spec/superseded_by per row."
  },
  {
    "file": "api.py",
    "line": 167,
    "severity": "nit",
    "category": "conventions",
    "claim": "The new /collect route maps ValueError to HTTP 400, but a non-existent campaign_id should yield 404 to match the /archive and /assets routes in the same file.",
    "evidence": "The _archive route maps ValueError from store.set_archived to HTTP 404 (line 159). The _collect route maps ValueError from library.plan/library.run to HTTP 400 (line 177). If library.plan raises ValueError when the campaign_id does not exist, a client querying a non-existent campaign gets 400 instead of 404.",
    "source": "protopatch",
    "verdict": "confirmed",
    "note": "api.py _archive: `raise HTTPException(404, str(e))`; _collect: `raise HTTPException(400, str(e))`. library.py plan() calls `store.require_campaign(target_campaign_id)` which raises ValueError('no campaign with id \u2026') for a missing campaign \u2014 so a bad campaign_id to /collect returns 400, not 404."
  },
  {
    "file": "api.py",
    "line": 117,
    "severity": "nit",
    "category": "conventions",
    "claim": "The new archived_hidden count requires a second full-table scan of campaigns on every GET /campaigns request; a single query or a count method would suffice.",
    "evidence": "Line 140 calls store.list_campaigns(include_archived=True) solely to compute a count for the `archived_hidden` field. This is a second full-table scan on the campaigns table for every GET /campaigns request.",
    "source": "protopatch",
    "verdict": "confirmed",
    "note": "api.py: `hidden = 0 if include_archived else len(store.list_campaigns(include_archived=True)) - len(rows)` \u2014 a second call after the first `store.list_campaigns(include_archived=include_archived)` loop. store.py list_campaigns(include_archived=True) executes `SELECT * FROM campaigns ORDER BY id DESC` (no WHERE clause), loading all rows just for a count."
  }
]

[protoreview]

QA panel review — WARN

code-review-structural · head 6f353cb8b9bb · formal

⚠️ Coverage incomplete — this is not a clean pass. 1 of 5 review lane(s) did not complete a full pass this round: find_structural (structural pass cut short: 16 of 17 features reviewed — feature cap reached: the 16 features with the most changed lines were reviewed, 1 were not (structuralmaxfeatures=16)). Findings from the lanes that ran stand, but a defect only the missing lanes would catch may be missed, so a PASS is capped at WARN. Where the brief below implies full coverage, this line supersedes it. The next push re-runs the full panel.

Low overall risk: all surviving findings are minor. Verification confirmed 2 of 7 and refuted 4 — the refuted ones were a false None-guard concern (store.review raises ValueError, not returns None), a false concurrency concern (retention.py has a threading.Lock), and two test-coverage claims that were actually covered by existing tests in test_library.py. The one thing to fix first is the narrow except ValueError on the collect route (api.py:175), which turns any filesystem I/O error into an unhandled 500. The remaining test-coverage gap on GET /file/{asset_id} is the most security-relevant of the survivors. No coverage gaps from verification (all 7 annotated).

Findings

	Severity	Location	Finding	Verified
🟡	minor	api.py:175	Only ValueError is caught from the file-mutating library.run call; an OSError (PermissionError, FileNotFoundError, disk-full) would propagate as an unhandled 5…	confirmed
🟡	minor	api.py:205	No test coverage for the security-sensitive GET /file/{asset_id} route, which contains path-traversal protection, containment checks, and MIME allowlisting (fl…	⚠️ uncertain
🟡	minor	api.py:298	GET /videos/{job_id} has no test coverage for its three response paths (200, 404, 422) or the _public_job transformation logic.	⚠️ uncertain

findings JSON (machine-readable)

[
  {
    "file": "api.py",
    "line": 175,
    "severity": "minor",
    "category": "correctness",
    "claim": "Only ValueError is caught from the file-mutating library.run call; an OSError (PermissionError, FileNotFoundError, disk-full) would propagate as an unhandled 500.",
    "evidence": "The comment on line 170 states 'sync: a copy copies files', confirming the non-dry-run path performs filesystem I/O via library.run. The try/except on lines 180-181 catches only ValueError. If library.run raises an OSError (e.g., PermissionError on a locked file, FileNotFoundError if a file was deleted between the plan and execution, or a disk-full condition), it propagates as an unhandled 500 wit",
    "source": "protopatch",
    "verdict": "confirmed",
    "note": "library.copy_assets calls shutil.copy2(s, dst) directly (library.py:217) with no internal OSError handling; the api.py:180 except clause catches only ValueError, so a PermissionError/FileNotFoundError from the copy would surface as an unhandled 500."
  },
  {
    "file": "api.py",
    "line": 205,
    "severity": "minor",
    "category": "tests",
    "claim": "No test coverage for the security-sensitive GET /file/{asset_id} route, which contains path-traversal protection, containment checks, and MIME allowlisting (flagged by both the general security-route gap and the specific file-route gap).",
    "evidence": "The file-serving route is a trust boundary (network + user-input + serialization) that resolves and serves filesystem paths. It contains non-trivial security logic: path resolution with strict=True, containment check via paths.is_contained, MIME-type allowlisting, and safe headers. Yet the feature has zero tests (tests: []). There is no test verifying that path-traversal attempts are rejected, tha",
    "source": "protopatch",
    "verdict": "uncertain",
    "note": "Searched tests/test_library.py, tests/test_plugin.py, tests/test_produce.py, tests/test_integration.py: no test exercises GET /file/{asset_id} (path traversal, containment, or MIME logic). The route's security logic is untested. \u2014 absence refuted by a repo-wide search at the reviewed head: referenced at `tests/test_shoot.py:210` \u2014 downgraded to uncertain, cannot gate a merge (issue #259)",
    "ungrounded": true,
    "absence_refuted": "tests/test_shoot.py:210"
  },
  {
    "file": "api.py",
    "line": 298,
    "severity": "minor",
    "category": "tests",
    "claim": "GET /videos/{job_id} has no test coverage for its three response paths (200, 404, 422) or the _public_job transformation logic.",
    "evidence": "The feature metadata lists no tests for this route. The handler exercises three distinct response paths (200 with a well-formed job, 404 when produce.load returns None, and 422 when _public_job raises on a malformed record) plus the _public_job transformation logic including the downloads dict comprehension with type-guarded int coercion. None of these are verified by any included test file.",
    "source": "protopatch",
    "verdict": "uncertain",
    "note": "Searched tests/test_library.py, test_plugin.py, test_produce.py, test_integration.py: no test for GET /videos/{job_id} found, but I did not exhaustively search all 25+ test files in the repo; cannot confirm total absence. \u2014 absence refuted by a repo-wide search at the reviewed head: referenced at `tests/test_shoot.py:210` \u2014 downgraded to uncertain, cannot gate a merge (issue #259)",
    "ungrounded": true,
    "absence_refuted": "tests/test_shoot.py:210"
  }
]

---

2 absence finding(s) (“no test” / “dead” / “never called” / “does not exist”) downgraded to uncertain: an absence is a claim about the whole repo, so it is checked by a search of the repo at this head, and a reference found there refutes it (issue #259). Each still stands for a human to judge.

• api.py (minor) — referenced at tests/test_shoot.py:210
• api.py (minor) — referenced at tests/test_shoot.py:210

---

Addressed since the last round (3 minor/nit): tools.py:127, api.py:167, api.py:117 — no longer raised, and the PR changed the code around them since they were raised.

[mabry1985]

Round 2: fixed the /collect OSError path (the copy now rolls back its files, so nothing is half-copied, and the route returns a clear 500), with a test. The /file and /videos coverage notes point at code this PR doesn't touch, and both are already covered (test_plugin.py::test_file_route_*, ::test_videos_route_maps_a_job_and_skips_malformed_records). The 'coverage incomplete' cap is the 16-feature reviewer limit (17 features in this diff).

[protoreview]

QA panel review — WARN

code-review-structural · head 0198b3625be9 · formal

⚠️ Coverage incomplete — this is not a clean pass. 1 of 5 review lane(s) did not complete a full pass this round: find_structural (structural pass cut short: 16 of 17 features reviewed — feature cap reached: the 16 features with the most changed lines were reviewed, 1 were not (structuralmaxfeatures=16)). Findings from the lanes that ran stand, but a defect only the missing lanes would catch may be missed, so a PASS is capped at WARN. Where the brief below implies full coverage, this line supersedes it. The next push re-runs the full panel.

Verification confirmed 2 of 4 findings and refuted 2. The two refuted items (missing tests for the archive endpoint and the modified GET /campaigns route) were shown to be covered by tests/test_library.py:280-284. The two surviving nits — unbounded Collect.asset_ids list length and _archive'narrow exception handling — are confirmed against head. No blockers or majors. Overall risk is low: the new logic is sound, the gaps are hardening and convention items. No panel disagreement; the verifier settled the test-coverage question cleanly.

Findings

	Severity	Location	Finding	Verified
⚪	nit	api.py:151	No upper bound on Collect.asset_ids list length.	confirmed
⚪	nit	api.py:158	_archive does not catch non-ValueError exceptions from store.set_archived, unlike sibling mutation routes.	confirmed

findings JSON (machine-readable)

[
  {
    "file": "api.py",
    "line": 151,
    "severity": "nit",
    "category": "security",
    "claim": "No upper bound on Collect.asset_ids list length.",
    "evidence": "The Collect Pydantic model accepts `asset_ids: list[int]` with no `max_items` constraint. A malicious or buggy client could submit a list containing hundreds of thousands of integers. Each would pass Pydantic validation, then be passed to `library.plan` or `library.run` for per-item processing (database lookups, filesystem operations for copy mode). Under a sync/threadpool handler this could exhau Fix: Add a length constraint to the model: `asset_ids: list[int] = Field(..., max_length=500)` (or a value appropriate to the expected batch size). Return a 422 with a clear message when exceeded. (protopatch confidence: medium)",
    "source": "protopatch",
    "verdict": "confirmed",
    "note": "api.py:150 (head): `asset_ids: list[int]` \u2014 no `max_length` or `Field` constraint present. A client can submit an arbitrarily long list; each id triggers a `store.get_asset` lookup in `_closure`.",
    "line_original": 150,
    "line_corrected": true
  },
  {
    "file": "api.py",
    "line": 158,
    "severity": "nit",
    "category": "conventions",
    "claim": "_archive does not catch non-ValueError exceptions from store.set_archived, unlike sibling mutation routes.",
    "evidence": "Sibling mutation routes in the same file (_collect at line 182, _prune at line 274) catch broader exceptions (OSError, Exception) and return an operator-readable 500 with a log entry. _archive catches only ValueError \u2192 404. If store.set_archived raises an OSError (e.g., locked SQLite file) or any other unexpected exception, FastAPI returns an opaque 500 with no log line, making diagnosis harder. T Fix: Add a broad except clause after the ValueError handler that logs the exception and raises HTTPException(500, 'archive failed \u2014 see the agent log'), matching the pattern used in _collect and _prune. (protopatch confidence: medium)",
    "source": "protopatch",
    "verdict": "confirmed",
    "note": "api.py:158-163: `_archive` has only `except ValueError \u2192 404`. Compare `_collect` (line 182-186: `except OSError \u2192 500 + log.exception`) and `_prune` (line 274: `except Exception \u2192 500 + log.exception`). An unexpected exception from `store.set_archived` would produce an unlogged opaque 500."
  }
]

---

Addressed since the last round (1 minor/nit): api.py:175 — no longer raised, and the PR changed the code around them since they were raised.

[protoreview]

QA panel review — WARN

code-review-structural · head e3f0d376dbcb · formal

⚠️ Coverage incomplete — this is not a clean pass. 1 of 5 review lane(s) did not complete a full pass this round: find_structural (structural pass cut short: 16 of 17 features reviewed — feature cap reached: the 16 features with the most changed lines were reviewed, 1 were not (structuralmaxfeatures=16)). Findings from the lanes that ran stand, but a defect only the missing lanes would catch may be missed, so a PASS is capped at WARN. Where the brief below implies full coverage, this line supersedes it. The next push re-runs the full panel.

Overall risk is low. The one substantive correctness issue is the cross-campaign superseded_by reference in library.py:253 — fix that first. Verification refuted 6 of 11 findings: the entire protopatch test-gap cluster was systematically wrong (the PR's test suite in test_plugin.py and test_retention.py covers all cited routes). No panel disagreement. No coverage gaps — the verifier annotated all 11 findings.

Findings

	Severity	Location	Finding	Verified
🟡	minor	library.py:258	copy_assets can write a cross-campaign superseded_by reference: when the replacement asset is not in the lineage closure, imap.get(i, i) falls back to the orig…	confirmed
🟡	minor	api.py:178	_collect route lacks a broad exception handler: unlike _archive which catches Exception and returns a structured 500, _collect only catches ValueError and OSEr…	confirmed
🟡	minor	api.py:117	N+1 query pattern in GET /campaigns: for each campaign in the shown list, the route calls store.list_assets(c['id']) to load the full asset list and count stat…	confirmed
⚪	nit	tools.py:118	shutil.rmtree with ignore_errors=True can leave orphaned files invisible to retention: if deletion partially fails (locked file, permission error), remaining f…	confirmed
⚪	nit	api.py:249	Fallback expression on line 249 is not None-safe: if store.review returns None, _safe_public(None) returns None, then the right side of or evaluates None.get…	confirmed

findings JSON (machine-readable)

[
  {
    "file": "library.py",
    "line": 258,
    "severity": "minor",
    "category": "correctness",
    "claim": "copy_assets can write a cross-campaign superseded_by reference: when the replacement asset is not in the lineage closure, imap.get(i, i) falls back to the original id, violating the same-campaign invariant enforced by _check_replacements on all other write paths.",
    "evidence": "store.set_meta_paths(imap[a[\"id\"]], superseded_by=[imap.get(i, i) for i in a[\"superseded_by\"]])",
    "verdict": "confirmed",
    "note": "library.py:253 verbatim: `store.set_meta_paths(imap[a[\"id\"]], superseded_by=[imap.get(i, i) for i in a[\"superseded_by\"]])`. The lineage closure follows retention.depends_on (backward/ancestors); superseded_by points forward to a newer asset that is NOT in the closure, so imap.get(i, i) falls back to the source-campaign id \u2014 a cross-campaign reference in the target campaign's row.",
    "line_original": 253,
    "line_corrected": true
  },
  {
    "file": "api.py",
    "line": 178,
    "severity": "minor",
    "category": "correctness",
    "claim": "_collect route lacks a broad exception handler: unlike _archive which catches Exception and returns a structured 500, _collect only catches ValueError and OSError, so unexpected errors (e.g. sqlite3.OperationalError from a locked database) propagate as unhandled 500s with stack traces.",
    "evidence": "The _archive route (line 162-164) explicitly catches a broad Exception with a structured 500 response for cases like 'a locked database'. The _collect route (lines 178-189) only catches ValueError and OSError.",
    "source": "protopatch",
    "verdict": "confirmed",
    "note": "api.py:178-189: _collect catches only `except ValueError` (400) and `except OSError` (500). api.py:162-164: _archive catches `except Exception` (500). A sqlite3.OperationalError (subclass of sqlite3.Error, not OSError) would propagate unhandled from _collect."
  },
  {
    "file": "api.py",
    "line": 117,
    "severity": "minor",
    "category": "correctness",
    "claim": "N+1 query pattern in GET /campaigns: for each campaign in the shown list, the route calls store.list_assets(c['id']) to load the full asset list and count statuses in Python, producing one DB query per campaign when a single grouped aggregate would suffice.",
    "evidence": "For each campaign in `shown`, the route calls `store.list_assets(c['id'])` (line 125) which loads the full asset list, then filters and counts in Python.",
    "source": "protopatch",
    "verdict": "confirmed",
    "note": "api.py:125: `every = store.list_assets(c[\"id\"])` inside `for c in shown:` \u2014 one list_assets call per campaign. The counts (review, approved, superseded, pruned) are computed in Python via sum(1 for a in ...). A single grouped aggregate query would eliminate the N+1."
  },
  {
    "file": "tools.py",
    "line": 118,
    "severity": "nit",
    "category": "correctness",
    "claim": "shutil.rmtree with ignore_errors=True can leave orphaned files invisible to retention: if deletion partially fails (locked file, permission error), remaining files have no asset row pointing to them and the retention system will never discover them.",
    "evidence": "When re-recording into an asset, the old take directory is removed via shutil.rmtree(old_dir, ignore_errors=True). If deletion partially fails (a file is locked by another process, a permission error on one entry, etc.), the remaining files have no asset row pointing to them.",
    "source": "protopatch",
    "verdict": "confirmed",
    "note": "tools.py:118: `shutil.rmtree(old_dir, ignore_errors=True)`. The guard `store.any_file_under(old_dir)` (line 115) confirms no row references files under old_dir before deletion, so if rmtree partially fails, the surviving files are truly orphaned \u2014 no row points to them and retention can never discover them."
  },
  {
    "file": "api.py",
    "line": 249,
    "severity": "nit",
    "category": "correctness",
    "claim": "Fallback expression on line 249 is not None-safe: if store.review returns None, _safe_public(None) returns None, then the right side of `or` evaluates None.get(\"id\") which raises AttributeError and produces a 500.",
    "evidence": "Line 249: `JSONResponse({\"asset\": _safe_public(a) or {\"id\": a.get(\"id\"), \"status\": a.get(\"status\")}})`. If store.review ever returns None, _safe_public(None) returns None, then the right side of `or` evaluates `None.get(\"id\")` which raises AttributeError.",
    "source": "protopatch",
    "verdict": "confirmed",
    "note": "api.py:249 verbatim: `return JSONResponse({\"asset\": _safe_public(a) or {\"id\": a.get(\"id\"), \"status\": a.get(\"status\")}})`. If store.review returns None, a is None; _safe_public(None) is falsy, so the right side evaluates `None.get(\"id\")` \u2192 AttributeError \u2192 unhandled 500."
  }
]

---

Addressed since the last round (2 minor/nit): api.py:151, api.py:158 — no longer raised, and the PR changed the code around them since they were raised.

[protoreview]

QA panel review — WARN

code-review-structural · head c3df7311463e · formal

⚠️ Coverage incomplete — this is not a clean pass. 1 of 5 review lane(s) did not complete a full pass this round: find_structural (structural pass cut short: 16 of 17 features reviewed — feature cap reached: the 16 features with the most changed lines were reviewed, 1 were not (structuralmaxfeatures=16)). Findings from the lanes that ran stand, but a defect only the missing lanes would catch may be missed, so a PASS is capped at WARN. Where the brief below implies full coverage, this line supersedes it. The next push re-runs the full panel.

Verification confirmed 3 of 10 findings and refuted 7 (all test-coverage claims were incorrect — the tests exist in test_library.py, test_retention.py, test_plugin.py, and test_produce.py). The surviving findings are two missing exception handlers in api.py (the _review route and the /videos listing) and a theoretical orphan-file risk in tools.py. Fix first: add the catch-all handler to _review at api.py:244, matching the pattern already present in _archive, _collect, and _prune. No panel disagreement. No coverage gaps — the verifier annotated all 10 findings (n=10). Several prior minor/nit items from round 4 (library.py:258 cross-campaign reference, api.py:117 N+1 query, api.py:249 None-safety) remain open below the disposition threshold.

Findings

	Severity	Location	Finding	Verified
🟡	minor	api.py:244	POST /assets/{asset_id}/review lacks a catch-all exception handler, unlike every other mutation route in the file; an unexpected exception (e.g. sqlite3.Operat…	confirmed
🟡	minor	api.py:297	Uncaught exception from produce.list_jobs(50) yields a silent 500 with no log entry, unlike other listing routes in the same file that wrap their data-fetch in…	confirmed
⚪	nit	tools.py:131	shutil.rmtree with ignore_errors=True can leave orphaned files invisible to retention: if deletion partially fails (locked file, permission error), remaining f…	confirmed

findings JSON (machine-readable)

[
  {
    "file": "api.py",
    "line": 244,
    "severity": "minor",
    "category": "correctness",
    "claim": "POST /assets/{asset_id}/review lacks a catch-all exception handler, unlike every other mutation route in the file; an unexpected exception (e.g. sqlite3.OperationalError) propagates as a raw 500 with stack trace visible to the client.",
    "evidence": "The _review route (line 244) only catches ValueError from store.review and maps it to a 400. If store.review raises any other exception\u2014e.g. sqlite3.OperationalError on a locked database, KeyError on a malformed row, or a TypeError if the asset dict is missing expected keys\u2014the exception propagates unhandled. FastAPI will return a raw 500 with the internal stack trace visible to the client. Every",
    "source": "protopatch",
    "verdict": "confirmed",
    "note": "api.py:244-250: `except ValueError as e: raise HTTPException(400, str(e)) from None` is the only handler. _archive (line 168), _collect (line 196), and _prune (line 280) all have `except Exception as e` with log.exception + structured 500."
  },
  {
    "file": "api.py",
    "line": 297,
    "severity": "minor",
    "category": "correctness",
    "claim": "Uncaught exception from produce.list_jobs(50) yields a silent 500 with no log entry, unlike other listing routes in the same file that wrap their data-fetch in try/except.",
    "evidence": "The call to produce.list_jobs(50) at line 303 is not wrapped in a top-level try/except. If it raises (e.g., an I/O error reading a job file, a PermissionError, or an unexpected exception from the underlying store), the route returns an opaque 500 to the client and no log entry is produced. Other listing routes in the same file handle this explicitly: _retention (lines 266-273) wraps its data-fetch",
    "source": "protopatch",
    "verdict": "confirmed",
    "note": "api.py:303: `for j in produce.list_jobs(50):` is not wrapped in try/except. _retention (line 268) wraps `retention.last_report()` and `retention.plan()` in `except Exception` with log.exception."
  },
  {
    "file": "tools.py",
    "line": 131,
    "severity": "nit",
    "category": "correctness",
    "claim": "shutil.rmtree with ignore_errors=True can leave orphaned files invisible to retention: if deletion partially fails (locked file, permission error), remaining files have no asset row pointing to them and the retention system will never discover them.",
    "evidence": "shutil.rmtree(old_dir, ignore_errors=True)",
    "source": "protopatch",
    "verdict": "confirmed",
    "note": "tools.py:131: `shutil.rmtree(old_dir, ignore_errors=True)` \u2014 errors are silently swallowed; partial failure leaves orphaned files with no DB row, invisible to retention's row-driven discovery."
  }
]

---

Addressed since the last round (4 minor/nit): library.py:258, api.py:178, api.py:117, api.py:249 — no longer raised, and the PR changed the code around them since they were raised.

[mabry1985]

Round 4: the two surviving minors are on the pre-existing _review and /videos routes. This PR now leaves both untouched (I dropped two cosmetic hunks that had pulled _review and campaign_list into the diff), so they're out of scope here. _supersede_take's rmtree(ignore_errors=True) is also pre-existing: retention only ever acts on rows, never on stray files.

[protoreview]

QA panel review — WARN

code-review-structural · head 7e980d1d9c00 · formal

⚠️ Coverage incomplete — this is not a clean pass. 1 of 5 review lane(s) did not complete a full pass this round: find_structural (structural pass cut short: 16 of 17 features reviewed — feature cap reached: the 16 features with the most changed lines were reviewed, 1 were not (structuralmaxfeatures=16)). Findings from the lanes that ran stand, but a defect only the missing lanes would catch may be missed, so a PASS is capped at WARN. Where the brief below implies full coverage, this line supersedes it. The next push re-runs the full panel.

Low-risk round: the panel's two minor findings narrowed to one after verification. The verifier confirmed the view.py race condition (stale async response overwriting campaign state) and refuted the archive test-coverage finding — the 404 path is in fact asserted in test_the_gallery_hides_archived_campaigns_by_default. No blocker or major items are open from prior rounds. The surviving finding is a minor frontend race that becomes more likely with the new archived toggle; a generation counter or AbortController would close it. No coverage gaps: both heavily-changed backend files (library.py, store.py) were verified clean by the correctness and cross-file finders.

Findings

	Severity	Location	Finding	Verified
🟡	minor	view.py:332	Stale async response can overwrite current campaign state when the user changes campaigns or toggles archived while a prior loadAssets() is still in flight.	confirmed

findings JSON (machine-readable)

[
  {
    "file": "view.py",
    "line": 332,
    "severity": "minor",
    "category": "correctness",
    "claim": "Stale async response can overwrite current campaign state when the user changes campaigns or toggles archived while a prior loadAssets() is still in flight.",
    "evidence": "The campaign <select> is always interactive. If the user changes the campaign while a prior loadAssets() is still awaiting its fetch, the stale response will eventually resolve, assign its data to state.data, and call render() \u2014 overwriting the newer campaign's assets with the older campaign's data.",
    "source": "protopatch",
    "verdict": "confirmed",
    "note": "view.py:155 `state.data = await api(...)` has no guard; the campaign change handler (line ~332) and the new show-archived handler (line ~338) both fire loadAssets()/loadCampaigns() without awaiting or cancelling a prior in-flight fetch. No AbortController or generation counter exists in the file."
  }
]

---

Addressed since the last round (1 minor/nit): api.py:244 — no longer raised, and the PR changed the code around them since they were raised.

[protoreview]

QA panel review — PASS

code-review-structural · head d435a1fc2b06 · formal

Low-risk change: two protopatch nits survive verification, both confirmed. No blockers or majors. The prior open item (stale async overwrite at view.py:332) was verified fixed by the loadGen counter. Verification confirmed both findings without modification; the superseded_by sub-claim in finding 2 was noted as slightly off but the core inconsistency stands. No coverage gaps.

Prior requests

	Prior finding	Disposition	Why
✅	view.py:332	fixed	The loadGen counter added in this diff invalidates in-flight loadAssets() responses, preventing a stale async response from overwriting current campaign state.

Findings

	Severity	Location	Finding	Verified
⚪	nit	api.py:90	has_file is computed on the unresolved stored path while the file-serving route resolves symlinks first, so a symlink inside the media root pointing outside wo…	confirmed
⚪	nit	view.py:187	Server-provided IDs are interpolated into HTML attributes without esc() while adjacent fields (c.name) are escaped, creating an inconsistency that would become…	confirmed

findings JSON (machine-readable)

[
  {
    "file": "api.py",
    "line": 90,
    "severity": "nit",
    "category": "correctness",
    "claim": "has_file is computed on the unresolved stored path while the file-serving route resolves symlinks first, so a symlink inside the media root pointing outside would report has_file=true but 403 on fetch.",
    "evidence": "Line 90: `out[\"has_file\"] = bool(a.get(\"path\")) and paths.is_contained(a[\"path\"])` \u2014 checks containment on the raw stored path. The _file route (lines 228-231) does `p = Path(a[\"path\"]).expanduser().resolve(strict=True)` then `if p is None or not paths.is_contained(p): raise HTTPException(403, ...)`. A symlink inside the media root pointing outside passes the raw check but fails the resolved check.",
    "source": "protopatch",
    "verdict": "confirmed",
    "note": "Read api.py at head: line 90 uses `paths.is_contained(a[\"path\"])` on the raw path; the _file route at lines 228-231 resolves first then checks containment on the resolved path. The inconsistency is real."
  },
  {
    "file": "view.py",
    "line": 187,
    "severity": "nit",
    "category": "security",
    "claim": "Server-provided IDs are interpolated into HTML attributes without esc() while adjacent fields (c.name) are escaped, creating an inconsistency that would become a real XSS vector if IDs ever become string slugs.",
    "evidence": "Line 167: `<option value=\"${c.id}\">${c.archived ? \"[archived] \" : \"\"}${esc(c.name)} \u2014 ...` \u2014 c.id unescaped, c.name escaped. Line 187: `<option value=\"${l.id}\">${esc(l.name)}</option>` \u2014 same pattern. Line 241: `<span>#${a.id}</span>` \u2014 a.id unescaped in innerHTML.",
    "source": "protopatch",
    "verdict": "confirmed",
    "note": "Read view.py at head: c.id (line 167), l.id (line 187), and a.id (line 241) are all interpolated into innerHTML without esc() while adjacent name/status fields use esc(). The superseded_by sub-claim is slightly off (esc(supBy) is applied on line 244), but the core inconsistency is clearly present.",
    "line_original": 167,
    "line_corrected": true
  }
]

[protoreview]

Promoting the PASS verdict for head d435a1fc2b06: 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:

• nit api.py:90 — has_file is computed on the unresolved stored path while the file-serving route resolves symlinks first, so a symlink inside the media root pointing outside would report has_file=true but 403 on fetch
• nit view.py:187 — Server-provided IDs are interpolated into HTML attributes without esc() while adjacent fields (c.name) are escaped, creating an inconsistency that would become a real XSS vector if IDs ever become str

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