* feat(store): flea-market upload guardrails + soft delete + JOIN-based admin queue
Adds an end-to-end guardrails pipeline for store uploads (manifest +
static-security + LLM review), persists blocked bundles for forensics,
introduces soft-delete (Archive) semantics, consolidates the legacy
/store/{id} surface into /marketplace/flea/{id}, and reworks the admin
queue so lifecycle filters read live entity visibility via LEFT JOIN
rather than a denormalized submission column.
Schema v29 → v35:
* v29 store_submissions table + store_entities.visibility_status
* v30 file_size, bundle_sha256, bundle_purged_at on submissions
* v31 reshape store_submissions (drop legacy unique on entity_id)
* v32 store_entities.archived_at/by + 'archived' visibility value
* v33 drop store_submissions.retry_count (unused)
* v34 ensure idx_store_submissions_entity exists post column-drop
* v35 broaden visibility_status enum + JOIN architecture cutover
Pipeline (src/store_guardrails/):
* Inline checks: manifest_check, static_scan, quality_check
* LLM review configurable haiku|sonnet|opus (default haiku)
* BackgroundTasks-driven async path with structured-output JSON
* Per-submitter daily quota (default 50)
* 30-day TTL purge job (POST /api/admin/run-blocked-purge)
* Bundle SHA256 + size persisted; sha256 survives purge for forensics
Visibility model:
* pending | approved | hidden | archived
* _enforce_visibility returns 404 (no leak) for non-owner non-admin
* Owner sees own non-approved entries via include_owner_id widening
* Install refused with 409 entity_not_approved when not approved
Soft-delete (DELETE /api/store/entities/{id}):
* Default = soft (visibility_status='archived'); existing installs
keep getting served the bundle so users don't lose the plugin
* ?hard=true admin-only: drops bundle + cascades user_store_installs
* Hard-delete preserves entity_id on submission as tombstone so
audit_log linkage survives for the activity timeline
Admin queue lifecycle (the JOIN refactor):
* Verdict (store_submissions.status) is immutable forensic record
* Lifecycle (store_entities.visibility_status) is live state
* /admin/store/submissions Archived chip translates to
`e.visibility_status='archived'` via LEFT JOIN — any path that
flips visibility surfaces in the queue immediately
* Detail page renders Status (verdict) and Entity lifecycle side by
side so admins see "approved at review, now archived" at a glance
URL consolidation:
* /store/{id} deleted (no redirect, stale bookmarks 404)
* /marketplace/flea/{id} is the canonical detail surface
* Three in-tree callers (upload-success, my-stack card, store
listing card) updated to point at the new URL
* Quarantine banner extracted to _quarantine_banner.html partial,
self-guarded, included from both flea detail templates
* Banner JS auto-refreshes when the verdict lands by polling
/api/marketplace/flea/{id}/detail (visibility_status +
submission_status — the latter is needed because blocked_llm
keeps the entity at visibility_status='pending')
Audit log resource format:
* runner.py emits prefixed `store_submission:{id}` (post-fix)
* Detail-page timeline query handles three patterns: prefixed
submission, helper-emitted `store_entity:{sub_id}`, and bare-id
legacy rows — all surface in the activity timeline
UX fixes:
* Owner sees Under review / Quarantined / Hidden banner with status
* Install button gray-disabled (not blue) when non-approved
* Owner cannot delete quarantined entries (403); admin can
* Admin queue: filter chips, sortable columns, paging, page-size
* Auto-refresh queue every 5s while pending rows are visible
* Store upload page file picker no longer opens twice (label →
input default action collided with explicit JS handler)
Tests: 168 passed across the guardrails suites (admin submissions,
store API, inline / LLM / purge guardrails, store repositories,
marketplace filter, schema version). New regression coverage
includes: archive surfaces via JOIN even when API path is bypassed;
deleted submission renders activity timeline (tombstone); flea
detail surfaces submission_status only for owner/admin; detail page
renders Entity lifecycle row; audit log resource format covers both
helper and runner paths.
* fix(store-guardrails): PR #233 follow-up — prompt injection, atomic PUT, BG race, schema, reaper, sort whitelist
Addresses 9 of the 23 findings from the PR #233 review (spec at
docs/superpowers/specs/2026-05-09-pr233-guardrails-fixes-spec.md).
Merge-gate items #1-#6 plus high-value mediums #7, #9-#12, #23.
Architectural items (#8 enum split, #14 factory) and pure
maintainability (#15-#22) deferred to follow-ups.
Security:
* #1 prompt injection — SYSTEM_PROMPT now passed via the SDK's
dedicated system= parameter; bundle wrapped in <bundle>...</bundle>
sentinels declared data-only by the system prompt; literal
sentinel strings in user content are escaped so an adversarial
README can't forge a close tag.
* #6 static scan honesty — module docstring + admin copy + docs
declare static scan as signal not gate; .md/.txt/.rst/.html/.json/
.yaml/.yml/.toml skipped to avoid false positives on prose.
AST mode for Python deferred (separate flag, FP comparison work).
Correctness:
* #2 PUT atomicity — bundles bake into plugin.staging-<rand>/
alongside live, atomic-rename on success; failed checks leave
live tree byte-for-byte intact.
* #3 BG-task race — set_visibility_if_pending guards verdict flips
to the (pending, hidden) review window; admin archives during
review survive; skipped flips audit-logged.
* #4 v35 NOT NULL/DEFAULT — schema v35→v36 re-applies them on
store_entities.visibility_status. CHECK constraint enforced
application-side (DuckDB ADD CHECK on existing column unsupported).
* #7 stuck-review reaper — reap_stuck_llm_reviews flips pending_llm
rows older than guardrails.stuck_review_grace_seconds (default
1800) to review_error. Scheduler runs every 15 min via new
/api/admin/run-reap-stuck-reviews. Set knob to 0 to disable.
* #9 quota counter — count_blocked_for_submitter_since now counts
blocked_inline + blocked_llm + review_error so a submitter
triggering only LLM-blocked verdicts is bounded.
* #10 missing risk_level — surfaces as review_error with
error='missing_risk_level' instead of silently defaulting to
'medium' (which looked like a model-decided block).
* #11 archived_at clear — set_visibility nulls archived_at +
archived_by when transitioning out of 'archived' so a future
read doesn't show stale archive forensics on an approved row.
Maintainability:
* #12 FSM doc comment — accurate insert/transition/lifecycle
description in src/db.py near store_submissions schema.
* #23 sort-key whitelist — admin queue rejects unknown sort keys
with 400 invalid_sort_key; substring-replace footgun removed.
Deferred (separate PRs):
* #5 quota race — proper fix requires asyncio.Lock spanning the
full pipeline; threading.Lock blocks event loop, DuckDB MVCC
doesn't help. API-level slowapi bounds worst case for now.
* #6 part 3 (AST static scan), #8 (enum split), #13 (import
bundle docs), #14 (factory consolidation), #15-#22 (maint).
Tests:
* New: tests/test_store_guardrails_prompt_injection.py (corpus +
trust-boundary invariants), tests/test_store_put_atomic.py,
tests/test_store_guardrails_reaper.py.
* Extended: test_store_guardrails_llm.py (system param, missing
risk_level, BG race), test_admin_store_submissions.py (quota
counter widening, sort whitelist 400), test_store_repositories.py
(un-archive metadata clear), test_db_schema_version.py (v36).
* Full suite: 3738 passed; 17 pre-existing baseline failures
unchanged (db migration tests, cli binary rename, catalog export,
user mgmt v5 backfill — confirmed by stash + rerun on clean tree).
223 lines
8.2 KiB
Python
223 lines
8.2 KiB
Python
"""PUT /api/store/entities/{id} atomicity (#2 from PR #233 review).
|
|
|
|
Pre-fix: the bake wrote into the live `${DATA_DIR}/store/<id>/plugin/`
|
|
path BEFORE running guardrail checks. A concurrent GET during the
|
|
window saw partial / unverified content, and a failed check left the
|
|
on-disk tree in a partially-overwritten state until the rollback
|
|
copytree finished.
|
|
|
|
Post-fix: bake into a sibling `plugin.staging-<rand>/` dir, run checks
|
|
there, then atomic rename onto the live path. Failed checks leave the
|
|
live tree byte-for-byte intact.
|
|
"""
|
|
|
|
from __future__ import annotations
|
|
|
|
import hashlib
|
|
import io
|
|
import zipfile
|
|
from pathlib import Path
|
|
|
|
import pytest
|
|
from argon2 import PasswordHasher
|
|
from fastapi.testclient import TestClient
|
|
|
|
from src.db import close_system_db, get_system_db
|
|
from src.repositories.users import UserRepository
|
|
|
|
|
|
@pytest.fixture
|
|
def web_client(tmp_path, monkeypatch):
|
|
monkeypatch.setenv("DATA_DIR", str(tmp_path))
|
|
monkeypatch.setenv("TESTING", "1")
|
|
monkeypatch.setenv("JWT_SECRET_KEY", "test-secret-key-min-32-characters!!")
|
|
(tmp_path / "state").mkdir()
|
|
(tmp_path / "analytics").mkdir()
|
|
(tmp_path / "extracts").mkdir()
|
|
close_system_db()
|
|
from app.main import create_app
|
|
app = create_app()
|
|
yield TestClient(app)
|
|
close_system_db()
|
|
|
|
|
|
def _create_user(client, email, password="UserPass1!"):
|
|
ph = PasswordHasher()
|
|
conn = get_system_db()
|
|
user_id = email.split("@")[0]
|
|
UserRepository(conn).create(
|
|
id=user_id, email=email, name=user_id, password_hash=ph.hash(password),
|
|
)
|
|
conn.close()
|
|
r = client.post("/auth/token", json={"email": email, "password": password})
|
|
assert r.status_code == 200, r.text
|
|
return user_id, {"access_token": r.json()["access_token"]}
|
|
|
|
|
|
def _make_skill_zip(skill_name: str, body: str) -> bytes:
|
|
buf = io.BytesIO()
|
|
with zipfile.ZipFile(buf, "w") as zf:
|
|
zf.writestr(
|
|
f"{skill_name}/SKILL.md",
|
|
f"---\nname: {skill_name}\ndescription: A clean test skill for atomic-PUT testing.\n---\n\n"
|
|
+ body,
|
|
)
|
|
return buf.getvalue()
|
|
|
|
|
|
def _make_evil_zip(skill_name: str) -> bytes:
|
|
"""A skill containing a static-security violation (eval) — fails
|
|
inline checks during PUT, so the live tree must NOT be touched."""
|
|
buf = io.BytesIO()
|
|
with zipfile.ZipFile(buf, "w") as zf:
|
|
zf.writestr(
|
|
f"{skill_name}/SKILL.md",
|
|
f"---\nname: {skill_name}\ndescription: Updated body content.\n---\n\nBody. " * 30,
|
|
)
|
|
zf.writestr(f"{skill_name}/run.sh", "#!/bin/sh\neval $1\n")
|
|
return buf.getvalue()
|
|
|
|
|
|
def _hash_tree(root: Path) -> str:
|
|
"""Stable digest of the on-disk plugin tree (path + content)."""
|
|
h = hashlib.sha256()
|
|
for p in sorted(root.rglob("*")):
|
|
if not p.is_file():
|
|
continue
|
|
rel = p.relative_to(root).as_posix().encode()
|
|
h.update(rel + b"\0" + p.read_bytes() + b"\0")
|
|
return h.hexdigest()
|
|
|
|
|
|
def _plugin_dir_for(entity_id: str) -> Path:
|
|
"""Mirror app/api/store.py:_plugin_dir without importing private."""
|
|
from app.utils import get_store_dir
|
|
return Path(get_store_dir()) / entity_id / "plugin"
|
|
|
|
|
|
class TestPutAtomicity:
|
|
def test_failed_inline_check_leaves_live_tree_intact(self, web_client):
|
|
"""The live `plugin/` tree must be byte-for-byte identical
|
|
before and after a PUT whose bundle fails inline checks."""
|
|
owner_id, owner_cookies = _create_user(web_client, "ownerA@x.com")
|
|
clean_zip = _make_skill_zip("atomic-skill", "Clean body. " * 30)
|
|
c = web_client.post(
|
|
"/api/store/entities",
|
|
files={"file": ("s.zip", clean_zip, "application/zip")},
|
|
data={"type": "skill"}, cookies=owner_cookies,
|
|
)
|
|
assert c.status_code == 201, c.text
|
|
eid = c.json()["id"]
|
|
|
|
plugin_dir = _plugin_dir_for(eid)
|
|
before_hash = _hash_tree(plugin_dir)
|
|
assert before_hash, "expected non-empty plugin tree"
|
|
|
|
# PUT with a bundle that will fail static_security (contains eval).
|
|
evil_zip = _make_evil_zip("atomic-skill")
|
|
u = web_client.put(
|
|
f"/api/store/entities/{eid}",
|
|
files={"file": ("evil.zip", evil_zip, "application/zip")},
|
|
cookies=owner_cookies,
|
|
)
|
|
# Inline-blocked uploads return 422 with a structured detail.
|
|
assert u.status_code == 422, u.text
|
|
assert u.json()["detail"]["code"] == "submission_blocked"
|
|
|
|
after_hash = _hash_tree(plugin_dir)
|
|
assert after_hash == before_hash, (
|
|
"live plugin tree changed after a failed-check PUT — "
|
|
"atomic-rename invariant broken"
|
|
)
|
|
|
|
# Sibling staging dirs must not be left behind.
|
|
entity_root = plugin_dir.parent
|
|
leftovers = [
|
|
p for p in entity_root.iterdir()
|
|
if p.name.startswith("plugin.staging-")
|
|
or p.name.startswith("plugin.backup-")
|
|
]
|
|
assert not leftovers, (
|
|
f"staging/backup dirs leaked on disk: {leftovers}"
|
|
)
|
|
|
|
def test_successful_put_atomically_replaces_tree(self, web_client):
|
|
"""Successful PUT swaps the live tree to the new bundle without
|
|
leaving a staging dir behind."""
|
|
owner_id, owner_cookies = _create_user(web_client, "ownerB@x.com")
|
|
v1 = _make_skill_zip("swap-skill", "First body. " * 30)
|
|
c = web_client.post(
|
|
"/api/store/entities",
|
|
files={"file": ("v1.zip", v1, "application/zip")},
|
|
data={"type": "skill"}, cookies=owner_cookies,
|
|
)
|
|
assert c.status_code == 201, c.text
|
|
eid = c.json()["id"]
|
|
plugin_dir = _plugin_dir_for(eid)
|
|
before_hash = _hash_tree(plugin_dir)
|
|
|
|
v2 = _make_skill_zip("swap-skill", "Second different body. " * 30)
|
|
u = web_client.put(
|
|
f"/api/store/entities/{eid}",
|
|
files={"file": ("v2.zip", v2, "application/zip")},
|
|
cookies=owner_cookies,
|
|
)
|
|
assert u.status_code == 200, u.text
|
|
|
|
after_hash = _hash_tree(plugin_dir)
|
|
assert after_hash != before_hash, "PUT didn't change live tree"
|
|
|
|
entity_root = plugin_dir.parent
|
|
leftovers = [
|
|
p for p in entity_root.iterdir()
|
|
if p.name.startswith("plugin.staging-")
|
|
or p.name.startswith("plugin.backup-")
|
|
]
|
|
assert not leftovers, (
|
|
f"staging/backup dirs leaked on disk after success: {leftovers}"
|
|
)
|
|
|
|
def test_inline_check_failure_during_put_does_not_pollute_tree(
|
|
self, web_client, monkeypatch,
|
|
):
|
|
"""Force a check failure mid-bake by monkey-patching
|
|
run_inline_checks. Live tree must still be intact."""
|
|
from src.store_guardrails.runner import InlineResult
|
|
|
|
owner_id, owner_cookies = _create_user(web_client, "ownerC@x.com")
|
|
clean_zip = _make_skill_zip("monkey-skill", "Body. " * 30)
|
|
c = web_client.post(
|
|
"/api/store/entities",
|
|
files={"file": ("v1.zip", clean_zip, "application/zip")},
|
|
data={"type": "skill"}, cookies=owner_cookies,
|
|
)
|
|
assert c.status_code == 201, c.text
|
|
eid = c.json()["id"]
|
|
plugin_dir = _plugin_dir_for(eid)
|
|
before_hash = _hash_tree(plugin_dir)
|
|
|
|
# Force the PUT path to see a failed inline result without
|
|
# actually relying on a static_security regex match.
|
|
def fake_inline(*args, **kwargs):
|
|
return InlineResult(
|
|
manifest={"status": "fail", "issues": ["forced"]},
|
|
static_security={"status": "pass", "findings": []},
|
|
quality={"status": "pass", "issues": [],
|
|
"template_placeholders": 0,
|
|
"template_recommendation": None},
|
|
)
|
|
monkeypatch.setattr(
|
|
"app.api.store.run_inline_checks", fake_inline,
|
|
)
|
|
|
|
v2 = _make_skill_zip("monkey-skill", "Different. " * 30)
|
|
u = web_client.put(
|
|
f"/api/store/entities/{eid}",
|
|
files={"file": ("v2.zip", v2, "application/zip")},
|
|
cookies=owner_cookies,
|
|
)
|
|
assert u.status_code == 422, u.text
|
|
|
|
assert _hash_tree(plugin_dir) == before_hash, (
|
|
"monkey-patched check failure polluted the live tree"
|
|
)
|