From a694a30a5e43995aa811020f00239e167511eed4 Mon Sep 17 00:00:00 2001 From: Vojtech <119944107+cvrysanek@users.noreply.github.com> Date: Fri, 15 May 2026 17:52:07 +0400 Subject: [PATCH] fix(store): surface review failures + harden publish gate (#316) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix(store): surface review failures + harden publish gate Four independent fixes to the flea-market submission pipeline, all surfaced by an admin upload that landed at status='approved' without an LLM review. 1. LLM truncation no longer pins submissions in review_error. - Raised MAX_RESPONSE_TOKENS 2500 → 6000 in llm_review.py - Added one-shot retry-with-doubled-budget in anthropic_provider.py (capped at 4× initial) 2. Flea detail page surfaces the latest submission's failure verdict even when a previously-approved version is still serving (deferred-promotion path). The _quarantine_banner gate widened from `visibility != approved` to also fire on `blocked_inline / blocked_llm / review_error`, with copy that distinguishes the v2+ edit case ("Latest edit failed review — previously approved version (vN) keeps serving") from the initial-upload quarantine wording. 3. Restore button + endpoint no longer allow restoring a version that was never approved. Added StoreEntitiesRepository.get_with_version_approvals joining store_submissions, gated the UI button on submission_status in ('approved', None), rendered status pills for non-restorable rows, and added a 400 version_not_approved guard in POST /restore. 4. **BREAKING (operator-facing)**: publish gate is now fail-CLOSED on misconfig. The previous get_guardrails_enabled() silently fell back to "disabled, auto-approve everything" when guardrails.enabled=true in YAML but no ANTHROPIC_API_KEY was in env. Split into: - get_guardrails_enabled() (intent — YAML) - get_guardrails_llm_provider_ready() (readiness — env) Three-state matrix: enabled=false → auto-approve (unchanged) enabled=true + ready=true → normal pipeline (unchanged) enabled=true + ready=false (NEW) → submissions hold at pending_llm awaiting admin retry or override (was: silent auto-approve) Admin "Retry review" eligibility broadened to include pending_llm. Boot-time WARNING banner surfaces the misconfig in app/main.py. docs/STORE_GUARDRAILS.md updated with the three-state matrix. Operators relying on the auto-fallback for local-dev no-LLM setups must now explicitly set `guardrails.enabled: false` in instance.yaml. Tests: 4623 passed. Added TestPublishGateFailClosed (4 tests) and TestRestoreVersion::test_restore_rejects_* (3 tests). conftest.py adds an autouse fixture defaulting guardrails OFF so legacy tests don't need to know about the new toggle. * fix(store): admin override promotes v2+ edits to current The override handler at app/api/admin.py:3708 only flipped submission status → 'overridden' and entity visibility → 'approved'. Under the v37+ deferred-promotion model that's insufficient for v2+ edits / restores: the new bundle sits in versions/v/plugin/ and the entity row stays at the prior approved version_no + hash + on-disk live bundle. Installers kept getting the OLD bytes the admin had just intended to replace. Mirror the runner.run_llm_review auto-approval branch: look up the submission's version_hash in entity.version_history, and if its `n` differs from entity.version_no, promote_version + _swap_live_to_version. Initial v1 overrides are unaffected — the loop finds n=1 == version_no and skips promotion. Tests: - test_override_v2_edit_promotes_to_current: stage v1 approved + v2 blocked_llm; override the v2 sub; assert entity.version_no=2, entity.version flips off the v1 hash, and the live plugin/ dir mirrors versions/v2/plugin/. - test_override_v1_initial_upload_no_promote: regression guard so the promote loop doesn't accidentally bump a v1 override. Audit log gains a promoted_to_version_no field on the override action. * fix(store): retry/rescan review staged bundle; override forward-only Two adversarial-review findings from a Codex pass on the publish-gate work. C1. Admin retry + rescan were passing live `plugin/` to the LLM. For a v2+ submission held at `pending_llm` / `blocked_llm` / `review_error`, live still holds the prior approved version's bytes — so the LLM reviewed the WRONG bytes, and the runner's hash-match promotion in `run_llm_review` would then advance the entity to staged bytes that were never actually reviewed. Resolve the staged `/versions/v/plugin/` from the submission's `version_history` entry, with a fall-back to live for legacy pre-v37 rows that never seeded a versions/ dir. Helpers `_submission_plugin_dir` and `_version_no_for_submission` added to `app/api/store.py` so override / retry / rescan share one path. H1. Override's promote loop used `target != current`, which would silently demote the live bundle when admin overrode a stale v2 submission while v3 was already approved + live. Changed to `target > current` so override flips status + visibility on the row regardless, but on-disk promotion only fires forward. Same `>` defensive guard applied in `runner.run_llm_review` so a late LLM verdict racing with a newer approval can't demote either. Tests: - TestAdminRetryReviewsStagedBundle::test_retry_v2_blocked_passes_staged_dir_not_live - TestAdminRetryReviewsStagedBundle::test_rescan_v2_blocked_passes_staged_dir_not_live - TestOverrideForwardOnly::test_override_stale_v2_does_not_demote_when_v3_current * review polish: CHANGELOG drift, override eligibility, defensive copy Three small additions on top of the retry/rescan staged-bundle fix: 1. CHANGELOG: the PR's bullets had drifted into the released [0.54.17] section during rebase (context-match landed them next to already-released content). Moved them up to [Unreleased] where they belong; [0.54.17] now holds only what was actually released (refresh-marketplace ls-remote, /me/activity hero, CI sharding + workflow polish). 2. app/api/admin.py: admin override eligibility now accepts pending_llm alongside blocked_inline + blocked_llm + review_error. Closes a UX gap from the new fail-CLOSED behavior: under enabled-but-not-ready, a known-good submission would otherwise sit indefinitely until the admin set credentials AND clicked Retry. Override already routes through version_history (and is now forward-only on promote), so it stays safe for v2+ deferred- promotion submissions. 3. src/repositories/store_entities.py: get_with_version_approvals defensively copies each version_history entry before annotating with submission_status. self.get() re-parses JSON each call today so this is belt-and-suspenders against any future caching layer leaking the annotated key into a subsequent plain get() call. Tests: 112 passed (focused on test_store_entity_versions + test_admin_store_submissions, covering the retry/rescan staged- bundle fix the author shipped + this polish). --------- Co-authored-by: ZdenekSrotyr --- CHANGELOG.md | 97 ++++ app/api/admin.py | 130 +++++- app/api/store.py | 135 +++++- app/instance_config.py | 35 +- app/main.py | 27 ++ app/web/router.py | 50 ++- app/web/templates/_flea_versions.html | 47 +- app/web/templates/_quarantine_banner.html | 50 ++- connectors/llm/anthropic_provider.py | 33 +- docs/STORE_GUARDRAILS.md | 25 +- src/repositories/store_entities.py | 49 ++ src/store_guardrails/llm_review.py | 12 +- src/store_guardrails/runner.py | 5 +- tests/conftest.py | 38 ++ tests/test_admin_store_submissions.py | 523 ++++++++++++++++++++++ tests/test_llm_connector.py | 32 +- tests/test_store_entity_versions.py | 403 +++++++++++++++++ 17 files changed, 1597 insertions(+), 94 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index e38d0d1..dcd00fa 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -10,8 +10,105 @@ CalVer image tags (`stable-YYYY.MM.N`, `dev-YYYY.MM.N`) are produced for every C ## [Unreleased] +### Changed +- **BREAKING (operator-facing)**: flea-market guardrail pipeline now + fail-CLOSED on misconfig. `get_guardrails_enabled()` previously + conflated operator intent (`guardrails.enabled` YAML) with provider + readiness (`ANTHROPIC_API_KEY` env) — when intent was True but the + env var was missing, the pipeline silently auto-fell-back to disabled + and every upload landed `approved` without an LLM review. Split into + `get_guardrails_enabled()` (intent only) and a new + `get_guardrails_llm_provider_ready()` (env only). Three-state matrix: + `enabled=false → auto-approve` (unchanged), `enabled=true + ready → + normal hold-for-review` (unchanged), `enabled=true + not-ready → + submissions sit at `pending_llm`, no auto-approval` (new — was + silent auto-approval). Admin **Retry review** action on + `/admin/store/submissions/` now covers `pending_llm` too (was + `review_error` + `blocked_llm`). Boot-time `WARNING` banner surfaces + the misconfig in `app/main.py`. Operators who relied on the + auto-fallback for local-dev no-LLM setups must now explicitly set + `guardrails.enabled: false` in `instance.yaml` — same outcome, + explicit intent. +- Flea-market admin **Override** action on + `/admin/store/submissions/` now covers `pending_llm` submissions + too (was `blocked_inline` + `blocked_llm` + `review_error`). Closes + a UX gap created by the new fail-CLOSED behavior above: under + enabled-but-not-ready, a known-good submission would otherwise sit + indefinitely until the admin set credentials AND clicked **Retry + review**. Override already routes through `entity.version_history` + to resolve the correct version dir (and is now forward-only on + promote — see the Fixed bullet below), so it stays safe for v2+ + deferred-promotion submissions. + ### Fixed - **Unauthenticated browser requests to `GET /api/initial-workspace.zip` now redirect to `/login?next=/api/initial-workspace.zip` instead of returning a raw JSON 401** (#315). This is the one `/api/*` endpoint that's designed to be hit directly from a browser bookmark (the analyst clean-install zip), so it intentionally opts out of the global `_API_PATH_PREFIXES` "never redirect /api/*" contract in `app/main.py`. CLI / curl / other API clients (any `Accept` without `text/html` — including the `*/*` default) keep getting the 401 they can handle. +- Flea-market LLM security review failed with `LLMFormatError: Response + truncated (max_tokens) for schema store_guardrails_review` when the + reviewer emitted many findings or content-quality issues. Raised the + output budget (2500 → 6000 tokens) and added a one-shot + retry-with-doubled-budget in the Anthropic provider (up to 4× initial) + so the verdict survives an occasional verbose response instead of + pinning the submission in `review_error`. (We initially added + `maxItems=20` to the schema's `findings` and `content_quality.issues` + arrays, but Anthropic's structured-output validator rejects `maxItems` + on array types — removed.) +- Flea-market entity detail page surfaces the latest submission's + failure verdict even when a previously-approved version is still + serving (deferred-promotion path). The owner / admin used to see no + banner at all when a v2+ edit landed in `review_error`, + `blocked_llm`, or `blocked_inline` because the `_quarantine_banner` + partial gated on `entity.visibility_status != 'approved'`. The + banner now renders for those failure statuses too, with copy that + acknowledges the prior version is still live ("Latest edit failed + review — previously approved version (vN) keeps serving …"). +- Flea-market admin **Retry review** and **Rescan** now review the + STAGED version's bundle, not the live `plugin/` directory. For a + v2+ edit held at `pending_llm` / `blocked_llm` / `review_error`, + live still holds the prior approved version. Reviewing live would + produce a verdict against the WRONG bytes; the runner's hash-match + promotion would then advance the entity to staged bytes that were + never actually reviewed. Now resolves the staged + `versions/v/plugin/` from the submission's `version_history` + entry. (Critical — surfaced by adversarial review.) +- Flea-market admin **Override** is now forward-only on promotion. + Previously `target_version_no != current` would happily demote the + live bundle when admin overrode a stale v2 submission while v3 was + already approved + live. Changed to `target > current` so override + flips status + visibility on the row regardless, but on-disk + promotion only fires for newer versions. The same `> current` + guard is now applied defensively in `runner.run_llm_review` so a + late LLM verdict can't demote past a more recent approval either. + (High — surfaced by adversarial review.) +- Flea-market admin **Override** on a v2+ edit/restore submission now + promotes the entity to the overridden version + swaps the on-disk + live bundle. Pre-fix the override only flipped + `visibility_status='approved'` and `submission.status='overridden'`, + leaving `entity.version_no` at the prior approved version — so + installers (and the marketplace UI) kept serving the OLD bytes the + admin just intended to replace. Mirrors the auto-approval branch in + `runner.run_llm_review`: look up the submission's version in + `version_history`, `promote_version` + `_swap_live_to_version` when + it differs from current. Initial-v1 overrides unchanged (no + promotion needed). +- Flea-market Restore button + endpoint no longer allow restoring a + version that was never approved. The versions card hid the gate + entirely (showed Restore on any non-current row), and the backend + blocked only while the latest submission was `pending_*` — so a + `blocked_llm` / `review_error` version could be restored anyway. + Added `submission_status` decoration on `version_history` via a new + `StoreEntitiesRepository.get_with_version_approvals` helper, gated + the UI button on `submission_status in ('approved', None)` + (`None` is the legacy v1 seed, back-compat-treated as approved), + rendered status pills for blocked / errored / pending rows, and + added a 400 `version_not_approved` guard in + `POST /api/store/entities/{id}/versions/{n}/restore`. + +### Internal +- `StoreEntitiesRepository.get_with_version_approvals` now defensively + copies each `version_history` entry before annotating with + `submission_status`. `self.get()` re-parses JSON each call today so + this is belt-and-suspenders, but it protects any future caching layer + from leaking the annotated key into a subsequent plain `get()` call. ## [0.54.17] — 2026-05-15 diff --git a/app/api/admin.py b/app/api/admin.py index 3345a12..e7bbf60 100644 --- a/app/api/admin.py +++ b/app/api/admin.py @@ -3726,7 +3726,7 @@ async def admin_override_store_submission( sub = subs.get(submission_id) if sub is None: raise HTTPException(status_code=404, detail="submission_not_found") - if sub["status"] not in {"blocked_inline", "blocked_llm", "review_error"}: + if sub["status"] not in {"blocked_inline", "blocked_llm", "review_error", "pending_llm"}: raise HTTPException( status_code=409, detail=f"cannot_override_status:{sub['status']}", @@ -3747,8 +3747,47 @@ async def admin_override_store_submission( ents_repo = StoreEntitiesRepository(conn) ents_repo.set_visibility(entity_id, "approved") - # Update usage-attribution rows now that the entity is live. + # Mirror the runner's deferred-promotion path. An override on a + # v2+ edit/restore must promote the overridden version + swap the + # on-disk live bundle, otherwise the entity stays at the prior + # approved version and installers keep receiving stale bytes the + # admin just told us to replace. For an initial v1 submission + # (no prior approved) the version_no already matches — the loop + # just no-ops and we skip promotion harmlessly. entity_row = ents_repo.get(entity_id) or {} + promoted_to: Optional[int] = None + sub_hash = sub.get("version") + target_version_no: Optional[int] = None + for entry in (entity_row.get("version_history") or []): + if entry.get("hash") == sub_hash: + try: + target_version_no = int(entry.get("n")) + except (TypeError, ValueError): + target_version_no = None + break + # Forward-only: refuse to promote backwards. An admin overriding a + # stale v2 submission when v3 is already approved + live must NOT + # demote the live bundle back to v2's bytes. Override flips the + # row's status + visibility regardless; only the version-promote + # is gated. Forward (target > current) is the only motion the + # publish-gate model is designed to express. + if (target_version_no is not None + and target_version_no > int(entity_row.get("version_no") or 0)): + if ents_repo.promote_version(entity_id, target_version_no): + try: + from app.api.store import _swap_live_to_version + _swap_live_to_version(entity_id, target_version_no) + promoted_to = target_version_no + # Re-read after promotion so attribution picks up the + # new version's name/type if a rename was bundled in. + entity_row = ents_repo.get(entity_id) or entity_row + except Exception: + logger.exception( + "override: live swap failed for entity %s v%d", + entity_id, target_version_no, + ) + + # Update usage-attribution rows now that the entity is live. update_flea_attribution( conn, entity_id, entity_row.get("type", ""), @@ -3765,6 +3804,7 @@ async def admin_override_store_submission( "prior_status": sub["status"], "prior_findings": sub.get("llm_findings"), "prior_inline": sub.get("inline_checks"), + "promoted_to_version_no": promoted_to, }, result="ok", ) @@ -3799,7 +3839,11 @@ async def admin_rescan_store_submission( whose bundle was rolled back (no ``entity_id``) cannot be rescanned — nothing to scan. """ - from app.api.store import _plugin_dir + from app.api.store import ( + _plugin_dir, + _submission_plugin_dir, + _version_no_for_submission, + ) from src.db import get_system_db from src.repositories.store_entities import StoreEntitiesRepository from src.repositories.store_submissions import StoreSubmissionsRepository @@ -3809,7 +3853,10 @@ async def admin_rescan_store_submission( default_model_loader, run_llm_review, ) - from app.instance_config import get_guardrails_enabled + from app.instance_config import ( + get_guardrails_enabled, + get_guardrails_llm_provider_ready, + ) subs = StoreSubmissionsRepository(conn) sub = subs.get(submission_id) @@ -3819,12 +3866,21 @@ async def admin_rescan_store_submission( if not entity_id: raise HTTPException(status_code=409, detail="cannot_rescan_without_entity") - plugin_dir = _plugin_dir(entity_id) + ents = StoreEntitiesRepository(conn) + entity = ents.get(entity_id) + # Rescan the bundle this submission represents — not live. See the + # equivalent fix in /retry for the full reasoning. Same fall-back + # to live for legacy rows that never seeded a versions/v/plugin/. + target_n = _version_no_for_submission(entity or {}, submission_id) + if target_n is not None: + plugin_dir = _submission_plugin_dir(entity_id, target_n) + if not plugin_dir.exists(): + plugin_dir = _plugin_dir(entity_id) + else: + plugin_dir = _plugin_dir(entity_id) if not plugin_dir.exists(): raise HTTPException(status_code=410, detail="bundle_missing") - ents = StoreEntitiesRepository(conn) - entity = ents.get(entity_id) description = (entity or {}).get("description") inline = run_inline_checks( @@ -3849,20 +3905,28 @@ async def admin_rescan_store_submission( ) return {"ok": True, "submission_id": submission_id, "status": "blocked_inline"} - # Inline passes — schedule LLM if enabled, else auto-approve. - guardrails_on = get_guardrails_enabled() - new_status = "pending_llm" if guardrails_on else "approved" + # Inline passes. Three-state matrix: + # - intent False → auto-approve (operator opt-out) + # - intent True + ready → pending_llm, schedule LLM + # - intent True + not-ready → pending_llm, DO NOT schedule (admin + # retries from the same endpoint after providing credentials) + guardrails_enabled = get_guardrails_enabled() + provider_ready = get_guardrails_llm_provider_ready() + hold_for_review = guardrails_enabled + schedule_async_llm = guardrails_enabled and provider_ready + guardrails_on = hold_for_review # retained for audit-log compat + new_status = "pending_llm" if hold_for_review else "approved" subs.conn.execute( "UPDATE store_submissions SET inline_checks = ?, llm_findings = NULL, " "status = ?, updated_at = current_timestamp " "WHERE id = ?", [__import__("json").dumps(inline.to_response_dict()), new_status, submission_id], ) - if guardrails_on: + if hold_for_review: ents.set_visibility(entity_id, "pending") else: ents.set_visibility(entity_id, "approved") - # Guardrails off — immediately live; write attribution. + # Guardrails explicitly disabled — immediately live; write attribution. entity_row = ents.get(entity_id) or {} update_flea_attribution( conn, entity_id, @@ -3874,9 +3938,10 @@ async def admin_rescan_store_submission( action="store.submission.rescan", resource=f"store_submission:{submission_id}", params={"entity_id": entity_id, "outcome": new_status, - "guardrails_enabled": guardrails_on}, + "guardrails_enabled": guardrails_on, + "provider_ready": provider_ready}, ) - if guardrails_on: + if schedule_async_llm: background.add_task( run_llm_review, submission_id, @@ -3895,13 +3960,27 @@ async def admin_retry_store_submission( user: dict = Depends(require_admin), conn: duckdb.DuckDBPyConnection = Depends(_get_db), ): - """Re-queue the LLM review for a submission stuck in ``review_error``. + """Re-queue the LLM review for a submission. + + Eligible statuses: + * ``review_error`` — LLM call failed, admin retrying after the + underlying issue (rate limit, timeout, transient outage) clears. + * ``blocked_llm`` — admin disagrees with the prior verdict; rerun + from a clean slate (review rules may have shifted since). + * ``pending_llm`` — submission was held when the LLM provider had + no credentials in env (fail-CLOSED matrix: intent True + not + ready). Admin sets the key and re-fires from here. Only valid when the original submission's plugin tree is still on disk — for inline-blocked rows the bundle was deleted at POST time. """ - from app.api.store import _plugin_dir + from app.api.store import ( + _plugin_dir, + _submission_plugin_dir, + _version_no_for_submission, + ) from src.db import get_system_db + from src.repositories.store_entities import StoreEntitiesRepository from src.repositories.store_submissions import StoreSubmissionsRepository from src.store_guardrails.runner import ( default_api_key_loader, @@ -3913,7 +3992,7 @@ async def admin_retry_store_submission( sub = subs.get(submission_id) if sub is None: raise HTTPException(status_code=404, detail="submission_not_found") - if sub["status"] not in {"review_error", "blocked_llm"}: + if sub["status"] not in {"review_error", "blocked_llm", "pending_llm"}: raise HTTPException( status_code=409, detail=f"cannot_retry_status:{sub['status']}", ) @@ -3923,7 +4002,22 @@ async def admin_retry_store_submission( status_code=409, detail="cannot_retry_without_entity", ) - plugin_dir = _plugin_dir(entity_id) + # Review the STAGED version's bytes — not live. For a v2+ edit + # held at pending_llm or blocked_llm, live `plugin/` still holds + # the prior approved version. Reviewing live would produce a + # verdict against the wrong bytes; the runner's hash-match + # promotion would then advance the entity to staged bytes that + # were never actually reviewed. + ent = StoreEntitiesRepository(conn).get(entity_id) or {} + target_n = _version_no_for_submission(ent, submission_id) + if target_n is not None: + plugin_dir = _submission_plugin_dir(entity_id, target_n) + # Fall back to live for legacy pre-v37 rows where the version + # dir was never seeded. + if not plugin_dir.exists(): + plugin_dir = _plugin_dir(entity_id) + else: + plugin_dir = _plugin_dir(entity_id) if not plugin_dir.exists(): raise HTTPException(status_code=410, detail="bundle_missing") diff --git a/app/api/store.py b/app/api/store.py index e7dab2b..047258a 100644 --- a/app/api/store.py +++ b/app/api/store.py @@ -27,7 +27,7 @@ import uuid import zipfile from datetime import datetime, timezone from pathlib import Path -from typing import Any, List, Optional +from typing import Any, Dict, List, Optional from urllib.parse import urlparse import duckdb @@ -46,7 +46,10 @@ from pydantic import BaseModel from app.auth.access import is_user_admin, require_admin from app.auth.dependencies import _get_db, get_current_user -from app.instance_config import get_guardrails_enabled +from app.instance_config import ( + get_guardrails_enabled, + get_guardrails_llm_provider_ready, +) from app.utils import get_store_dir from src.db import get_system_db from src.repositories.audit import AuditRepository @@ -381,6 +384,37 @@ def _plugin_dir(entity_id: str) -> Path: return _entity_dir(entity_id) / "plugin" +def _submission_plugin_dir( + entity_id: str, version_no: int, +) -> Path: + """On-disk path of the bundle a particular submission represents. + + v37+ writes each version's bytes under + ``/versions/v/plugin/``. Live ``plugin/`` mirrors + whichever ``v`` is currently promoted. Admin retry / rescan + flows MUST review the staged version dir, not live — otherwise a + pending v2 retry would re-review v1's bytes, a clean verdict + would land, and the runner's hash-match promotion would advance + the entity to v2 bytes that were never actually reviewed. + """ + return _entity_dir(entity_id) / "versions" / f"v{int(version_no)}" / "plugin" + + +def _version_no_for_submission( + entity_row: Dict[str, Any], submission_id: str, +) -> Optional[int]: + """Locate the version_history entry produced by `submission_id` + and return its ``n``. Used by admin retry / rescan / override to + pick the right ``versions/v/plugin/`` directory.""" + for entry in (entity_row.get("version_history") or []): + if entry.get("submission_id") == submission_id: + try: + return int(entry.get("n")) + except (TypeError, ValueError): + return None + return None + + def _assets_dir(entity_id: str) -> Path: return _entity_dir(entity_id) / "assets" @@ -1318,11 +1352,22 @@ async def create_entity( bundle_meta = compute_bundle_meta(plugin_dir) subs_repo = StoreSubmissionsRepository(conn) - guardrails_on = get_guardrails_enabled() - # When the pipeline is disabled (local dev / no ANTHROPIC_API_KEY) - # we approve immediately. Inline checks ran above and recorded a - # passing verdict; skip the async LLM step and the 'pending' hold. - initial_visibility = "approved" if not guardrails_on else "pending" + # Three-state matrix (fail-CLOSED on misconfig): + # - intent False → auto-approve (operator opt-out, e.g. local dev) + # - intent True + ready → hold for review, schedule LLM async + # - intent True + NOT ready → hold for review, DO NOT auto-approve + # (submission sits at pending_llm; admin can set the key + click + # Retry review or override-publish manually). The previous + # auto-fallback silently approved everything when the env-var + # was missing — a fail-OPEN hole. + guardrails_enabled = get_guardrails_enabled() + provider_ready = get_guardrails_llm_provider_ready() + hold_for_review = guardrails_enabled # intent drives the hold + schedule_async_llm = guardrails_enabled and provider_ready + # `guardrails_on` retained for downstream audit-log compat — + # historical column meaning is "did the pipeline gate this row". + guardrails_on = hold_for_review + initial_visibility = "pending" if hold_for_review else "approved" photo_rel = await _save_photo(photo, entity_id) if photo else None doc_rels = await _save_docs(docs, entity_id) @@ -1369,11 +1414,16 @@ async def create_entity( "guardrails_enabled": guardrails_on, }, ) - if guardrails_on: + if schedule_async_llm: _schedule_llm_review(background_tasks, sub_id, plugin_dir) elif initial_visibility == "approved": - # Guardrails off — entity is immediately live; write attribution now. + # Guardrails explicitly disabled in YAML — entity is + # immediately live; write attribution now. update_flea_attribution(conn, entity_id, type, final_name) + # Else: enabled-but-not-ready. Submission sits at pending_llm; + # entity stays at visibility=pending. Admin retries from the + # admin UI once credentials are present, OR overrides + publishes + # the row manually. No silent auto-approval. finally: shutil.rmtree(scratch, ignore_errors=True) @@ -1703,7 +1753,17 @@ async def update_entity( # guardrails disabled the path collapses: submission lands at # 'approved' and we promote synchronously below. if file is not None and new_version_no is not None and new_version_dir is not None: - guardrails_on = get_guardrails_enabled() + # Same three-state matrix as the initial-upload path. Hold the + # new version (defer promotion) whenever guardrails are enabled + # — even when the provider isn't ready. Promotion only fires on + # an actual LLM approval OR when the operator explicitly opted + # out via `guardrails.enabled: false`. Misconfig (enabled + + # no key) sits at pending_llm awaiting admin action. + guardrails_enabled = get_guardrails_enabled() + provider_ready = get_guardrails_llm_provider_ready() + hold_for_review = guardrails_enabled + schedule_async_llm = guardrails_enabled and provider_ready + guardrails_on = hold_for_review subs_repo = StoreSubmissionsRepository(conn) from src.store_guardrails.bundle_meta import compute_bundle_meta # Hash the NEW version dir, not live (which still holds the @@ -1715,7 +1775,7 @@ async def update_entity( type=entity["type"], name=rename_to or entity["name"], version=new_version, - status="approved" if not guardrails_on else "pending_llm", + status="approved" if not hold_for_review else "pending_llm", entity_id=entity_id, inline_checks=inline_after_update.to_response_dict() if inline_after_update else None, @@ -1734,20 +1794,21 @@ async def update_entity( ) _audit( conn, user["id"], - "store.submission.accepted" if guardrails_on else "store.submission.approved", + "store.submission.accepted" if hold_for_review else "store.submission.approved", sub_id, {"entity_id": entity_id, "on": "update", "version_no": appended_n, "guardrails_enabled": guardrails_on}, ) - if guardrails_on: + if schedule_async_llm: # Live remains at prior approved bundle. LLM reviews the # new version dir; runner promotes on approval. _schedule_llm_review( background_tasks, sub_id, new_version_dir / "plugin", ) - else: - # Guardrails off → implicit approval. Promote inline: - # update entity columns + swap live to new version. + elif not hold_for_review: + # Guardrails explicitly disabled → implicit approval. + # Promote inline: update entity columns + swap live to new + # version. repo.promote_version(entity_id, appended_n) _swap_live_to_version(entity_id, appended_n) # Live bundle is now the new version; refresh attribution. @@ -1757,6 +1818,10 @@ async def update_entity( ent_after_swap.get("type") or entity["type"], ent_after_swap.get("name") or (rename_to or entity["name"]), ) + # Else (enabled + not-ready): submission sits at pending_llm, + # live continues serving the prior approved version. Admin + # retries from /admin/store/submissions once credentials are + # provided. # Use the freshly-appended version number when a bundle change # produced one, falling back to the planned new_version_no for @@ -1847,6 +1912,30 @@ async def restore_version( }, ) + # Refuse to restore a version that was never approved. Look up the + # submission that produced version `v` and gate on its + # status. Legacy v1 (no submission_id — seeded pre-v37) is + # back-compat treated as approved. The UI also hides the Restore + # button for these statuses, but defense in depth: a direct API + # caller bypasses the template. + src_sub_id = next( + (entry.get("submission_id") for entry in + (entity.get("version_history") or []) + if int(entry.get("n") or 0) == int(version_no)), + None, + ) + if src_sub_id: + src_sub = StoreSubmissionsRepository(conn).get(src_sub_id) + if src_sub and src_sub.get("status") not in ("approved",): + raise HTTPException( + status_code=400, + detail={ + "code": "version_not_approved", + "version_no": version_no, + "source_status": src_sub.get("status"), + }, + ) + # Locate the source version dir. source_dir = ( _entity_dir(entity_id) / "versions" / f"v{version_no}" / "plugin" @@ -1904,7 +1993,11 @@ async def restore_version( # swap + version_no/version/file_size bump) waits on LLM approval. from src.store_guardrails.bundle_meta import compute_bundle_meta target_meta = compute_bundle_meta(target_plugin) - guardrails_on = get_guardrails_enabled() + # Same three-state hold-for-review matrix as create/edit. + guardrails_enabled = get_guardrails_enabled() + provider_ready = get_guardrails_llm_provider_ready() + hold_for_review = guardrails_enabled + schedule_async_llm = guardrails_enabled and provider_ready subs_repo = StoreSubmissionsRepository(conn) sub_id = subs_repo.create( submitter_id=user["id"], @@ -1912,7 +2005,7 @@ async def restore_version( type=entity["type"], name=entity["name"], version=new_version, - status="approved" if not guardrails_on else "pending_llm", + status="approved" if not hold_for_review else "pending_llm", entity_id=entity_id, inline_checks=inline.to_response_dict(), file_size=target_meta.file_size, @@ -1932,11 +2025,13 @@ async def restore_version( "new_version_no": appended_n, "submission_id": sub_id}, ) - if guardrails_on: + if schedule_async_llm: _schedule_llm_review(background_tasks, sub_id, target_plugin) - else: + elif not hold_for_review: + # Guardrails explicitly disabled — inline-promote. repo.promote_version(entity_id, appended_n) _swap_live_to_version(entity_id, appended_n) + # Else (enabled + not-ready): defer promotion, await admin retry. _invalidate_etag() return _entity_to_response(conn, repo.get(entity_id)) # type: ignore[arg-type] diff --git a/app/instance_config.py b/app/instance_config.py index 14d27da..039e8ac 100644 --- a/app/instance_config.py +++ b/app/instance_config.py @@ -569,21 +569,32 @@ def get_guardrails_min_body_chars() -> int: def get_guardrails_enabled() -> bool: - """Master kill-switch for the guardrail pipeline. + """Operator's stated intent for the guardrail pipeline. - Defaults to True. Operators can disable by setting ``guardrails.enabled: - false`` in instance.yaml — useful for local development against the - UI without burning Anthropic tokens. Inline checks always run; this - flag only gates the LLM step (and skips the pending → approved hold). + Reads ``guardrails.enabled`` from instance.yaml. Defaults to True. + Operators can explicitly disable by setting ``guardrails.enabled: + false`` — useful for local development against the UI without + burning Anthropic tokens. - Auto-fallback: when the YAML says enabled but no ANTHROPIC_API_KEY / - LLM_API_KEY is set in the environment, behave as disabled. This - keeps the test suite + first-boot operator experience sane — uploads - auto-approve until the operator wires up an LLM provider rather than - silently piling up in ``review_error``. + Note: this returns intent ONLY. Whether the LLM provider has + working credentials is a separate concern — see + :func:`get_guardrails_llm_provider_ready`. The two are kept apart + so callers can implement fail-CLOSED behavior: hold submissions at + ``pending_llm`` (instead of silently auto-approving) when intent is + True but credentials are missing. + """ + return bool(get_value("guardrails", "enabled", default=True)) + + +def get_guardrails_llm_provider_ready() -> bool: + """Whether the LLM provider has credentials present in the + environment. + + Independent from :func:`get_guardrails_enabled` (operator intent). + A False return here when intent is True is a misconfiguration — + the caller should hold submissions at ``pending_llm`` and surface + a loud boot-time warning rather than silently auto-approving. """ - if not bool(get_value("guardrails", "enabled", default=True)): - return False if os.environ.get("ANTHROPIC_API_KEY", "").strip(): return True if os.environ.get("LLM_API_KEY", "").strip(): diff --git a/app/main.py b/app/main.py index cfa14db..6b43b4a 100644 --- a/app/main.py +++ b/app/main.py @@ -480,6 +480,33 @@ def create_app() -> FastAPI: logger.warning("NEVER enable this in a deployment reachable from the internet.") logger.warning("=" * 60) + # Guardrails misconfig surface — fail-CLOSED matrix means an enabled + # pipeline with no LLM credentials in env will hold every submission + # at `pending_llm` indefinitely. Surface this LOUDLY at boot so the + # operator finds the cause before the submission queue piles up. + try: + from app.instance_config import ( + get_guardrails_enabled, + get_guardrails_llm_provider_ready, + ) + if get_guardrails_enabled() and not get_guardrails_llm_provider_ready(): + logger.warning("=" * 60) + logger.warning( + "GUARDRAILS ENABLED BUT NO LLM PROVIDER CREDENTIALS FOUND.", + ) + logger.warning( + "Set ANTHROPIC_API_KEY (or LLM_API_KEY) in the environment, " + "or disable guardrails in instance.yaml.", + ) + logger.warning( + "Until then, every flea-market upload will sit at " + "status='pending_llm' awaiting admin retry — the LLM " + "review step cannot run.", + ) + logger.warning("=" * 60) + except Exception: + logger.exception("guardrails readiness probe failed at boot") + # Seed admin user (SEED_ADMIN_EMAIL) and add them to the Admin user_group. # Optional SEED_ADMIN_PASSWORD lets the seeded user sign in immediately # without going through bootstrap; never overwritten if already set. diff --git a/app/web/router.py b/app/web/router.py index cea3551..bd76c2c 100644 --- a/app/web/router.py +++ b/app/web/router.py @@ -1368,40 +1368,44 @@ async def marketplace_flea_detail( from src.repositories.store_entities import StoreEntitiesRepository from src.repositories.store_submissions import StoreSubmissionsRepository - entity = StoreEntitiesRepository(conn).get(entity_id) - if not entity: + repo = StoreEntitiesRepository(conn) + # Owner/admin get a version-status decorated entity so the versions + # card can gate the Restore button on past-version approval state. + # Plain viewers don't see the versions card at all, so the cheaper + # plain get() suffices. + base_entity = repo.get(entity_id) + if not base_entity: raise HTTPException(status_code=404, detail="Entity not found") # Refuse early — same gate as the API + the asset endpoints. 404 # (not 403) so the entity's existence isn't leaked. - _enforce_visibility(entity, user, conn) + _enforce_visibility(base_entity, user, conn) - is_owner = entity.get("owner_user_id") == user.get("id") + is_owner = base_entity.get("owner_user_id") == user.get("id") is_admin = is_user_admin(user["id"], conn) + entity = ( + repo.get_with_version_approvals(entity_id) + if (is_owner or is_admin) else base_entity + ) + # Pull the latest submission so the quarantine banner can render - # the most recent verdict (inline_checks + llm_findings). Skipped - # for plain non-owner non-admin viewers since they only see - # approved entities and don't need the diagnostic. + # the most recent verdict (inline_checks + llm_findings). v37: + # always load for owner/admin, even when the entity itself is + # approved at a prior version — under deferred promotion, a v2+ + # edit can leave the latest submission in `review_error` / + # `blocked_llm` while the entity row stays approved. Gating the + # fetch on `visibility_status != 'approved'` silently hid the + # failure from the owner. quarantine_sub = None - if (is_owner or is_admin) and entity.get("visibility_status") != "approved": + if is_owner or is_admin: quarantine_sub = StoreSubmissionsRepository(conn).latest_for_entity(entity_id) - # v37: even when entity is 'approved' (deferred promotion path — - # existing installers continue receiving the prior version), - # owner/admin needs to see if there's an edit-review in flight so - # the Edit button can lock + a small status surfaces. Look it up - # separately from quarantine_sub to keep the banner partial's - # gates intact. - edit_in_flight = False - if (is_owner or is_admin): - latest = ( - StoreSubmissionsRepository(conn).latest_for_entity(entity_id) - ) - if latest and latest.get("status") in ( - "pending_inline", "pending_llm", - ): - edit_in_flight = True + # v37: the Edit button locks while a submission is under review. + edit_in_flight = bool( + quarantine_sub + and quarantine_sub.get("status") in ("pending_inline", "pending_llm") + ) common = dict( source="flea", diff --git a/app/web/templates/_flea_versions.html b/app/web/templates/_flea_versions.html index f36bc09..e350c44 100644 --- a/app/web/templates/_flea_versions.html +++ b/app/web/templates/_flea_versions.html @@ -55,6 +55,14 @@ } .versions-card button.restore:hover { background: var(--surface-muted, #f9fafb); } .versions-card button.restore:disabled { opacity: 0.5; cursor: not-allowed; } + .versions-card .status-pill { + display: inline-block; padding: 1px 7px; border-radius: 999px; + font-size: 10px; font-weight: 600; text-transform: uppercase; + letter-spacing: 0.3px; vertical-align: middle; + } + .versions-card .status-pill.blocked { background: #fee2e2; color: #991b1b; } + .versions-card .status-pill.errored { background: #fef3c7; color: #92400e; } + .versions-card .status-pill.pending { background: #e0e7ff; color: #3730a3; }
@@ -91,12 +99,37 @@ {{ v.created_at[:10] if v.created_at else '' }} + {# Restore only when the version was actually approved. + ``submission_status=None`` is legacy v1 (seeded pre-v37 + before submission_id backfill) — treat as approved. + Anything else (blocked_inline / blocked_llm / + review_error / pending_*) renders a pill instead so the + owner understands why no button. #} + {% set _sub_status = v.submission_status %} + {% set _is_approvable = _sub_status in ['approved', None] %} {% if v.n != entity.version_no %} - + {% if _is_approvable %} + + {% elif _sub_status in ['blocked_inline', 'blocked_llm'] %} + + blocked + + {% elif _sub_status == 'review_error' %} + + errored + + {% elif _sub_status in ['pending_inline', 'pending_llm'] %} + + pending + + {% endif %} {% endif %} @@ -155,6 +188,10 @@ async function restoreVersion(versionNo) { return; } else if (code === 'prior_version_pending') { msg = 'A previous edit is still under review. Wait for the verdict before restoring.'; + } else if (code === 'version_not_approved') { + const src = j.detail.source_status || 'not approved'; + msg = `Restore blocked: v${versionNo} was never approved ` + + `(status: ${src}). Pick a version that passed review.`; } else if (code === 'version_not_found') { msg = 'That version is no longer on disk.'; } else if (code === 'already_current') { diff --git a/app/web/templates/_quarantine_banner.html b/app/web/templates/_quarantine_banner.html index 45a7e06..221607b 100644 --- a/app/web/templates/_quarantine_banner.html +++ b/app/web/templates/_quarantine_banner.html @@ -18,7 +18,21 @@ approved earlier — only the rendering location changed. #} -{% if entity.visibility_status != 'approved' and (is_owner or is_admin) %} +{# Gate widened for failure surfacing: under deferred promotion + (v37+), a v2+ edit can leave the entity approved at the prior + version while the latest submission landed in `review_error` / + `blocked_llm` / `blocked_inline`. The original + `visibility_status != 'approved'` gate silently hid those + failures from the owner. Render the banner whenever EITHER the + entity itself is non-approved OR the latest submission carries + a *failure* verdict the owner needs to see. Pending edits keep + the original behavior — Edit button locks instead, no banner. #} +{% if (is_owner or is_admin) and ( + entity.visibility_status != 'approved' + or (quarantine_sub and quarantine_sub.status in [ + 'blocked_inline', 'blocked_llm', 'review_error', + ]) + ) %}