feat(archive): back approved media up to protoContent — release assets + a batched catalog PR

[mabry1985]

What

Part 3 of 3 (retention ✓ #29 → collections ✓ #30 → archive). Approved media is backed up to protoContent (protoLabsAI/protoContent, private):

• Files become GitHub release assets, one release per campaign, tag media/<slug>. A release is created when missing, with make_latest: "false", so it never displaces a package release; nothing in protoContent's CI fires on media/* tags. Asset names are content-addressed, <sha256[:12]>-<file name>. Video never enters git.
• Catalog: specs/media/catalog/<slug>.json, validated against schemas/media/catalog.schema.json (protoContent#611, open for Josh, not merged). It's append/update only, upserted by sha256, so two agent instances sharing a slug can't clobber each other. protoContent's main takes PRs only (ruleset: required check + changeset-check, no bypass), so catalog updates are batched onto one open PR per campaign (branch media-catalog/<slug>). Later approvals add commits; a fresh PR opens only after a merge. A PR closed unmerged is regenerated in full. The plugin never merges.
• Flow: each pass first reconciles. Every approved asset with no archive record, or none for its current campaign's catalog (approved, moved or copied since), is queued. That way neither the review route nor collections needs a hook. Then: hash → upload (skipped when meta.archive.sha256 matches: same sha256 = already archived; a same-named asset with the right size is adopted, and a wrong-size partial is replaced) → meta.archive = {url, sha256, at, …} → one catalog commit per campaign per pass. Failures back off from 1 min to 6 h; after 10 attempts the asset parks until a backfill.
• Backfill: campaign_backup(backfill=true) / POST /archive/backfill. Also GET /archive (status) and POST /archive/drain (run a pass now), in their own archive_api.py router on the same gated prefix. All are sync routes.
• Settings: archive_enabled (default off), archive_repo (default protoLabsAI/protoContent), archive_catalog_dir (default specs/media/catalog); all three are operator-only (spawns: true) because they decide whether and where media goes. Plus the secret archive_token (manifest secrets: [archive_token]).
• Token: a fine-grained PAT, protoContent only: Contents: Read & write + Pull requests: Read & write (Metadata: read comes with it). It only ever goes into the Authorization header to api.github.com / uploads.github.com; any other host is refused. A malformed token is refused before any request. Every client exception is redacted and turned into a retried ArchiveError, and the tests check the token never shows up in logs, errors, queue rows or return values.
• Also: a catalog over 1 MB is read through the blob API; a version bump alone isn't a catalog change; two passes never overlap.

The manifest's posture comment and test_declared_capabilities_match_reality now say exactly this: archive.py is the one outbound caller.

Tests: tests/test_archive.py, 40 cases, against an in-memory GitHub plus HTTP-layer tests with a fake opener. The plugin's writer is validated against a vendored copy of protoContent's schema.

🤖 Generated with Claude Code

[protoreview]

QA panel review — WARN

code-review-structural · head 15f43b69cdfd · formal

Additive archive PR; the logic and cross-file wiring are clean. Verification confirmed two findings in archive_api.py and refuted the "no tests" claim (a test_the_archive_routes test at line 500 exercises all three routes). The one actionable item is the missing try/except on _status and _backfill — an unhandled exception there surfaces as a bare 500 with no server-side log. The nit about leaking the exception type name in the 500 body is confirmed but appears intentional per the inline comment. No coverage gaps: the main logic module archive.py drew zero findings, consistent with the correctness and cross-file lanes explicitly verifying it.

Findings

	Severity	Location	Finding	Verified
🟡	minor	archive_api.py:32	The _backfill and _status routes lack the try/except guard that _drain has; an unexpected exception (e.g. a DB connection error) propagates as a bare 500 with …	confirmed
⚪	nit	archive_api.py:46	The 500 response body includes the Python exception type name (e.g. 'OperationalError'), disclosing internal dependency details to callers; a generic message w…	confirmed

findings JSON (machine-readable)

[
  {
    "file": "archive_api.py",
    "line": 32,
    "severity": "minor",
    "category": "security",
    "claim": "The _backfill and _status routes lack the try/except guard that _drain has; an unexpected exception (e.g. a DB connection error) propagates as a bare 500 with no application-level logging.",
    "evidence": "The _drain route deliberately catches exceptions, logs them, and raises a sanitized HTTPException that only exposes the exception type name (using `from None` to suppress the traceback chain). The _backfill route has no such guard: if archive.backfill raises (e.g., a database connection error or a KeyError on an unexpected campaign_id), the exception propagates unhandled.",
    "source": "protopatch",
    "verdict": "confirmed",
    "note": "archive_api.py:32 `return JSONResponse(archive.status())` and line 35 `return JSONResponse({\"queued\": archive.backfill(body.campaign_id)})` have no try/except; only _drain (lines 38-42) wraps in try/except with log.exception + HTTPException(500, \u2026) from None."
  },
  {
    "file": "archive_api.py",
    "line": 46,
    "severity": "nit",
    "category": "security",
    "claim": "The 500 response body includes the Python exception type name (e.g. 'OperationalError'), disclosing internal dependency details to callers; a generic message would suffice since the full traceback is already in the server log.",
    "evidence": "The 500 response includes `type(e).__name__` (e.g. 'OperationalError', 'ConnectionError', 'TimeoutError'), which discloses internal dependency details to any unauthenticated or authenticated caller who triggers the error. The comment on line 44 indicates this is intentional ('this is the last net').",
    "source": "protopatch",
    "verdict": "confirmed",
    "note": "archive_api.py:46 `raise HTTPException(500, f\"archive pass failed ({type(e).__name__}) \u2014 see the agent log\") from None` \u2014 the f-string interpolates the exception class name into the response body."
  }
]

[protoreview]

Promoting the WARN verdict for head 15f43b69cdfd: 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 archive_api.py:32 — The _backfill and _status routes lack the try/except guard that _drain has; an unexpected exception (e.g. a DB connection error) propagates as a bare 500 with no application-level logging.
• nit archive_api.py:46 — The 500 response body includes the Python exception type name (e.g. 'OperationalError'), disclosing internal dependency details to callers; a generic message would suffice since the full traceback is

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

[protoreview]

QA panel review — WARN

code-review-structural · head ad147aaa183d · formal

Low-risk change: two tool-sourced findings, both confirmed by the verifier with no severity changes. The fix-first item is adding a Field(ge=0) constraint on campaign_id in the Backfill model (archive_api.py:29) so out-of-domain values are rejected with a 422 rather than forwarded to the store layer. The concurrency nit in surface.py:108 is real but further mitigated in practice by the 600 s first-delay before a new thread's first tick. No panel disagreement; verification confirmed both findings as-is. No coverage gaps.

Findings

	Severity	Location	Finding	Verified
🟡	minor	archive_api.py:29	Backfill endpoint accepts negative campaign_id without validation; a client sending campaign_id=-1 passes Pydantic and is forwarded to archive.backfill() with …	confirmed
⚪	nit	surface.py:108	surface.stop() may return while the daemon thread is still mid-tick if a tick exceeds the 5-second join timeout, allowing a subsequent start() to spawn a secon…	confirmed · nearby, not gating

findings JSON (machine-readable)

[
  {
    "file": "archive_api.py",
    "line": 29,
    "severity": "minor",
    "category": "api-contract",
    "claim": "Backfill endpoint accepts negative campaign_id without validation; a client sending campaign_id=-1 passes Pydantic and is forwarded to archive.backfill() with an out-of-domain value.",
    "evidence": "The Backfill model defines campaign_id as a plain int with default 0. The module docstring states 'campaign_id 0 = all', implying the valid domain is non-negative integers. A client sending campaign_id=-1 passes FastAPI/Pydantic validation and is forwarded to archive.backfill(), which may produce unexpected results (e.g., querying with a negative ID) or raise an unhandled exception that becomes a 500. Fix: Add a field constraint: campaign_id: int = 0 with Field(ge=0) so Pydantic rejects negative values with a 422 before the handler runs. (protopatch confidence: medium)",
    "source": "protopatch",
    "verdict": "confirmed",
    "note": "archive_api.py:29 has `campaign_id: int = 0` with no Field constraint; docstring says 'campaign_id 0 = all'; _backfill forwards body.campaign_id to archive.backfill() (archive.py:448) which calls store.list_assets(campaign_id, \u2026). Pydantic accepts any int, so -1 passes. Practical impact is low (store likely returns empty list) but the API-contract gap is real."
  },
  {
    "file": "surface.py",
    "line": 108,
    "severity": "nit",
    "category": "concurrency",
    "claim": "surface.stop() may return while the daemon thread is still mid-tick if a tick exceeds the 5-second join timeout, allowing a subsequent start() to spawn a second thread that races with the first on archive.drain and retention.run_scheduled.",
    "evidence": "stop() sets the _stop event and joins with a 5-second timeout. If the current tick() (which calls archive.drain and possibly retention.execute) takes longer than 5 s, join() returns before the thread exits. _thread is already set to None, so a subsequent start() spawns a second thread while the first is still inside tick(). Both threads then call archive.drain and retention.run_scheduled concurren Fix: Either increase the join timeout to cover the worst-case tick duration, or after a timed-out join, re-set the _stop event and join again (or track the thread's generation number and have tick() bail e (protopatch confidence: medium)",
    "source": "protopatch",
    "verdict": "confirmed",
    "note": "surface.py:108\u2013112: stop() sets _thread=None then joins with timeout=5; start() checks `if _thread is not None` (now None) and spawns a new thread. Mechanism is correct. Practical impact is mitigated by FIRST_DELAY_S=600s (new thread won't tick for 10 min), but the two-live-threads pattern is a valid nit. \u2014 nearby: in code this PR did not change (outside its changed lines and the functions they sit in) \u2014 reported, not gated (#232)",
    "nearby": true
  }
]

---

1 structural finding(s) are nearby notes, not part of the verdict: they sit in code this PR did not change (outside its changed lines and the functions those lines are in). Worth a look; not a request for this PR.

• surface.py:108 (nit) — surface.stop() may return while the daemon thread is still mid-tick if a tick exceeds the 5-second join timeout, allowing a subsequent start() to spawn a second

---

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

[protoreview]

Promoting the WARN verdict for head ad147aaa183d: 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 archive_api.py:29 — Backfill endpoint accepts negative campaign_id without validation; a client sending campaign_id=-1 passes Pydantic and is forwarded to archive.backfill() with an out-of-domain value.
• nit surface.py:108 — surface.stop() may return while the daemon thread is still mid-tick if a tick exceeds the 5-second join timeout, allowing a subsequent start() to spawn a second thread that races with the first on arc

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

[protoreview]

QA panel review — WARN

code-review-structural · head 1a8203eeca19 · formal

The PR is overwhelmingly additive (new archive module, API routes, queue table, tool). Verification refuted three of five findings: the drain concurrency guard exists (_DRAIN_LOCK), the backfill wildcard test exists, and the PARK_AFTER natural-transition test exists. Two findings survive: a real crash path in catalog_doc when a foreign catalog entry has a non-integer string id (minor, correctness), and an untested BATCH-limit truncation path (nit, tests). The one thing to fix first is the int(e.get("id") or 0) sort key at archive.py:368 — a malformed foreign entry will crash the catalog commit. The round-2 open item (negative campaign_id validation) is now fixed: the test test_backfill_rejects_a_negative_campaign confirms the endpoint rejects it. No Gaps; the verifier annotated all 5 findings.

Prior requests

	Prior finding	Disposition	Why
✅	archive_api.py:29	fixed	The test test_backfill_rejects_a_negative_campaign (cited in the verifier's note on finding 3) confirms the endpoint now rejects negative campaign_id values; t…

Findings

	Severity	Location	Finding	Verified
🟡	minor	archive.py:368	catalog_doc sort key crashes with ValueError if a foreign catalog entry has a non-integer id.	confirmed
⚪	nit	archive.py:59	No test verifies the BATCH limit truncates a drain pass.	⚠️ uncertain

findings JSON (machine-readable)

[
  {
    "file": "archive.py",
    "line": 368,
    "severity": "minor",
    "category": "correctness",
    "claim": "catalog_doc sort key crashes with ValueError if a foreign catalog entry has a non-integer id.",
    "evidence": "Line 368 uses `int(e.get(\"id\") or 0)` as part of the sort key over all catalog entries, including foreign entries loaded from the branch via `get_file`. If a foreign entry in the on-branch catalog JSON has a string id (e.g. `{\"id\": \"abc\"}`), `int(\"abc\")` raises ValueError, crashing the catalog commit.",
    "source": "protopatch",
    "verdict": "confirmed",
    "note": "archive.py:368 reads `\"assets\": sorted(keep.values(), key=lambda e: (int(e.get(\"id\") or 0), k(e)))`. `keep` includes entries from `prev` (the on-branch catalog via `gh.get_file`); a string id like `\"abc\"` is truthy so `e.get(\"id\") or 0` yields `\"abc\"`, and `int(\"abc\")` raises ValueError.",
    "line_original": 359,
    "line_corrected": true
  },
  {
    "file": "archive.py",
    "line": 59,
    "severity": "nit",
    "category": "tests",
    "claim": "No test verifies the BATCH limit truncates a drain pass.",
    "evidence": "`drain` accepts a `limit` parameter (default `BATCH`=25) and slices the due queue at line 483. No test enqueues more than `BATCH` assets and verifies that only `BATCH` are processed in one pass and the rest remain in the queue for the next tick.",
    "source": "protopatch",
    "verdict": "uncertain",
    "note": "Searched all 689 lines of tests/test_archive.py: every test enqueues at most 3 assets; no test passes `limit=` to `drain` or enqueues >25 assets to verify truncation. The `[:limit]` slice at archive.py:483 is untested. \u2014 absence refuted by a repo-wide search at the reviewed head: referenced at `tests/test_archive.py:21` \u2014 downgraded to uncertain, cannot gate a merge (issue #259)",
    "ungrounded": true,
    "absence_refuted": "tests/test_archive.py:21"
  }
]

---

1 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.

• archive.py (nit) — referenced at tests/test_archive.py:21

---

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

[protoreview]

Promoting the WARN verdict for head 1a8203eeca19: 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 archive.py:368 — catalog_doc sort key crashes with ValueError if a foreign catalog entry has a non-integer id.
• nit archive.py:59 — No test verifies the BATCH limit truncates a drain pass.

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

[protoreview]

QA panel review — PASS

code-review-structural · head a8e951e8d85c · formal

No findings were raised, so there was nothing to verify. The panel confirmed no new defects in this head. The two standing items from round 3 (sort-key crash at archive.py:368, missing BATCH-limit test at archive.py:59) are both minor/nit and the panel reports them as fixed in this revision; no blocker or major priors remain open. The protoPatch engine's four items were all assessed as false positives (concurrency already guarded, tests exist, one rests on a redaction mark). No coverage gaps to flag.

No findings — the review came back clean.

findings JSON (machine-readable)

[]

---

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

[protoreview]

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