Ship feed-first public Tests with durable preview enrichment - #28
Conversation
Co-authored-by: Codex <codex@openai.com>
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
Co-authored-by: Codex <codex@openai.com>
| file.originalSize <= MAX_TEXT_ENTRY_BYTES && | ||
| selectedTextBytes + file.originalSize <= MAX_SCREENED_ARCHIVE_TEXT_BYTES | ||
| ) { | ||
| selectedTextPaths.add(file.name); | ||
| selectedTextBytes += file.originalSize; |
There was a problem hiding this comment.
🔴 Credential checks silently stop inspecting files in large uploads
Text files inside an uploaded archive are skipped by the credential check (selectedTextBytes + file.originalSize <= MAX_SCREENED_ARCHIVE_TEXT_BYTES at lib/security/artifact-inspection.ts:137) once earlier files fill a 4 MB budget, so passwords or API keys in later files are published without being caught.
Impact: A contributor (accidentally or deliberately) can publish evidence containing live credentials by padding the upload with harmless text files first.
How the cumulative text budget changes scanning coverage
Before this change the ZIP filter selected every text entry up to MAX_TEXT_ENTRY_BYTES (1 MB each), and each selected entry was decoded and passed to detectSecretLabels (lib/security/artifact-inspection.ts:166-180). The new cumulative cap MAX_SCREENED_ARCHIVE_TEXT_BYTES = 4 MB (lib/security/artifact-inspection.ts:8) is applied in the same filter, so entries encountered after the budget is exhausted are never unzipped and therefore never secret-scanned, while inspectZipArchiveWithText still returns status: "approved".
Entry order is attacker-controlled (it is the ZIP central-directory order), so four 1 MB filler text files ahead of a .env are enough to bypass the check entirely. The cap appears to have been introduced to bound the new prompt-injection screening input; bounding only the textEntries returned for injection screening — while still decoding and secret-scanning every selected entry — would preserve the previous detection boundary.
Prompt for agents
In lib/security/artifact-inspection.ts the ZIP unzip filter now refuses to select text entries once a cumulative 4 MB budget (MAX_SCREENED_ARCHIVE_TEXT_BYTES) is exhausted. Because the same selected set is used both for the returned textEntries (new prompt-injection screening input) and for the existing per-file detectSecretLabels pass, the secret-detection coverage of large archives is silently reduced compared with the previous behaviour, and the archive is still reported as approved. Rework this so the cumulative bound only limits what is handed back for injection screening (or replaces it with a bounded truncation), while every text entry under MAX_TEXT_ENTRY_BYTES continues to be decoded and secret-scanned as before. Add a regression test covering an archive whose secret-bearing text file appears after several megabytes of benign text.
Was this helpful? React with 👍 or 👎 to provide feedback.
| <option value="medium">Medium</option> | ||
| <option value="high">High</option> | ||
| <option value="max">Max</option> | ||
| <option value="unknown">Unknown / adaptive</option> |
There was a problem hiding this comment.
🟡 Filtering tests by "Unknown / adaptive" reasoning always returns nothing
The reasoning filter sends the value unknown (app/tests/page.tsx:106) while the submission form suggests contributors record the wording "Unknown / adaptive", so choosing that filter never matches any Test and the feed looks empty.
Impact: Visitors filtering the feed for adaptive-reasoning Tests always see "No Tests match these filters" even when such Tests exist.
Free-text reasoning versus fixed filter values
Reasoning is now free text: app/upload/UploadWizard.tsx renders an <input list="reasoning-options"> whose datalist offers None, Low, Medium, High, Max, and Unknown / adaptive, and the value is stored verbatim in showcases.reasoning_level (lib/data/showcases.ts createShowcaseDraft).
The feed filter compares with strict equality on the lowercased stored value: lower(showcases.reasoningLevel) = ${reasoning} in lib/data/showcases.ts:320-322. The option value unknown can therefore never equal unknown / adaptive. The same mismatch affects any other free-text reasoning wording, but the shipped suggestion list makes this one guaranteed to fail.
| <option value="unknown">Unknown / adaptive</option> | |
| <option value="unknown / adaptive">Unknown / adaptive</option> |
Was this helpful? React with 👍 or 👎 to provide feedback.
| } catch { | ||
| judgeQueueDeferred = true; | ||
| enrichment = { | ||
| dispatchDeferred: true, | ||
| eligible: true, | ||
| enrichmentId: null, | ||
| }; | ||
| } |
There was a problem hiding this comment.
🟡 Publication record can claim an automated preview was scheduled when none exists
When scheduling the automated preview fails outright, the publication record is written as if the Test were preview-eligible and merely delayed (enrichment = { dispatchDeferred: true, eligible: true, ... } at app/api/showcases/[id]/publish/route.ts:42-46), so the audit trail and the response claim a preview is coming for Tests that will never have one.
Impact: Operators reading publication records, and the contributor reading the publish response, are told an automated preview is pending for submissions that are not eligible for one.
Why the catch branch cannot know eligibility
scheduleShowcaseEnrichment (lib/data/showcase-enrichment.ts:38-69) already swallows queue-dispatch failures internally and returns { dispatchDeferred: true, eligible: true } in that case. The only way it throws is when ensureShowcaseEnrichment itself fails (for example a database error) — at which point no showcase_enrichments row exists and eligibility is unknown: the Test may have no compatible source ZIP at all.
Hard-coding eligible: true in the route's catch therefore reports an eligibility the code never determined. Reporting eligible: false (or a distinct unknown marker) would keep the record honest; the scheduled reconciliation sweep still creates and dispatches the row later if the submission really is eligible.
| } catch { | |
| judgeQueueDeferred = true; | |
| enrichment = { | |
| dispatchDeferred: true, | |
| eligible: true, | |
| enrichmentId: null, | |
| }; | |
| } | |
| } catch { | |
| enrichment = { | |
| dispatchDeferred: true, | |
| eligible: false, | |
| enrichmentId: null, | |
| }; | |
| } |
Was this helpful? React with 👍 or 👎 to provide feedback.
| .where( | ||
| and( | ||
| eq(showcases.id, showcaseId), | ||
| eq(showcases.status, "draft"), | ||
| isNull(showcases.benchmarkVersionId), | ||
| inArray(showcases.safetyStatus, ["pending", "scanning", "approved"]), | ||
| ), | ||
| ) | ||
| .returning({ id: showcases.id }); | ||
| return recorded ? { code, failedAt: now } : null; |
There was a problem hiding this comment.
🔍 Legacy drafts attached to a benchmark version cannot record or retry a processing failure
recordShowcaseProcessingFailure only matches rows with benchmarkVersionId IS NULL, and retryShowcaseProcessing applies the same predicate when loading the showcase. If an artifact scan throws for a pre-existing draft that still references a benchmark version, the upload-complete route returns 503 telling the contributor to "Retry processing from your dashboard", but no failure is recorded, the dashboard shows no retry affordance, and the retry endpoint would 404. Worth confirming that no legacy drafts remain (or widening the predicate) before rollout.
Was this helpful? React with 👍 or 👎 to provide feedback.
| if (finalResult.injectionFlag) { | ||
| await getDb() | ||
| .update(showcases) | ||
| .set({ rankingStatus: "moderation_hold", updatedAt: now }) | ||
| .where(eq(showcases.id, artifact.showcaseId)); | ||
| } |
There was a problem hiding this comment.
🔍 Prompt-injection heuristics now place submissions on permanent moderation hold
Artifact scanning now runs screenJudgeInjection over submitted text and bounded archive text, and any hit sets the showcase's rankingStatus to moderation_hold (lib/security/artifact-scanner.ts:223-228). This is a one-way transition with no automated clearing path in the diff, and the heuristic fires on ordinary phrases in submitted content (the corpus matches things like "ignore previous instructions" appearing anywhere in a page). Since Tests are now published without judging, the practical effect is that false positives silently and permanently disqualify a submission from ever being ranked. A moderator-visible signal plus a clearing path would be safer than an irreversible status write.
Was this helpful? React with 👍 or 👎 to provide feedback.
| const id = crypto.randomUUID(); | ||
| const slug = `${slugify(parsed.title)}-${id.slice(0, 8)}`; | ||
| const [benchmarkVersion, modelVersion, harness] = await Promise.all([ | ||
| getDb() | ||
| .select({ | ||
| id: benchmarkVersions.id, | ||
| category: benchmarkVersions.category, | ||
| canonicalPrompt: benchmarkVersions.canonicalPrompt, | ||
| }) | ||
| .from(benchmarkVersions) | ||
| .innerJoin( | ||
| benchmarks, | ||
| eq(benchmarks.id, benchmarkVersions.benchmarkId), | ||
| ) | ||
| .where( | ||
| and( | ||
| eq(benchmarkVersions.id, parsed.benchmarkVersionId), | ||
| sql`${benchmarkVersions.publishedAt} IS NOT NULL`, | ||
| ), | ||
| ) | ||
| .limit(1), | ||
| const slug = `${slugify(title)}-${id.slice(0, 8)}`; |
There was a problem hiding this comment.
🔍 Auto-generated titles can produce a degenerate slug for non-Latin model names
With the title now optional, the fallback is ${modelLabel} model test and the public slug is slugify(title) plus eight hex characters. For a supplied title made entirely of non-ASCII characters, slugify can reduce to an empty string, yielding a slug like -1a2b3c4d. That value fails the slug pattern used by parseShowcaseSlug/parseReportTarget (lib/security/policy.ts:154) and by the user-content worker's RESULT_ARTIFACT_PATH, so the Test page would render while its evidence downloads and report link 404. This is pre-existing behaviour, but the free-text submission form makes non-Latin titles far more likely; consider falling back to the id when the slugified title is empty.
Was this helpful? React with 👍 or 👎 to provide feedback.
| }) | ||
| .from(showcases) | ||
| .innerJoin(users, eq(showcases.ownerId, users.id)) | ||
| .innerJoin( | ||
| .leftJoin( | ||
| resultConfigurations, | ||
| eq(resultConfigurations.id, showcases.resultConfigurationId), | ||
| ) | ||
| .innerJoin( | ||
| .leftJoin( | ||
| benchmarkVersions, | ||
| eq(benchmarkVersions.id, showcases.benchmarkVersionId), | ||
| ) | ||
| .innerJoin(benchmarks, eq(benchmarks.id, benchmarkVersions.benchmarkId)) | ||
| .leftJoin(benchmarks, eq(benchmarks.id, benchmarkVersions.benchmarkId)) | ||
| .leftJoin(runs, eq(runs.showcaseId, showcases.id)) |
There was a problem hiding this comment.
🔍 Detail-page join relaxation leaves benchmark fields nullable for consumers
getPublicShowcaseBySlug switched resultConfigurations, benchmarkVersions, and benchmarks from inner to left joins so feed-only Tests resolve. The selected testSlug, testTitle, testVersion, and configurationHash values are now null for every new Test. The new /tests/[slug] page does not read them, but app/api/public/results/[slug]/route.ts returns this shape to external consumers, so that public API response silently changes from always-present test metadata to nullable fields. Worth confirming the documented API contract (and any consumer) tolerates the nulls.
Was this helpful? React with 👍 or 👎 to provide feedback.
| file.originalSize <= MAX_TEXT_ENTRY_BYTES && | ||
| selectedTextBytes + file.originalSize <= MAX_SCREENED_ARCHIVE_TEXT_BYTES | ||
| ) { | ||
| selectedTextPaths.add(file.name); | ||
| selectedTextBytes += file.originalSize; |
There was a problem hiding this comment.
🟨 Cumulative 4 MB text budget lets secret scanning skip files in large archives
inspectZipArchiveWithText now refuses to select any further text entry once earlier entries consume a cumulative 4 MB budget (lib/security/artifact-inspection.ts:136-140). The same selected set feeds the existing detectSecretLabels pass (lib/security/artifact-inspection.ts:166-180), so text files beyond the budget are never decoded and never checked for credentials, yet the archive is still returned with status: "approved" and can be published as public evidence. Entry order is attacker-controlled, so padding an archive with a few megabytes of benign text ahead of a .env/key file bypasses the mandatory secret check.
Was this helpful? React with 👍 or 👎 to provide feedback.
1 participant
Summary
/and/teststhe public All Tests feed, with canonical Test detail pages and filters for model, harness, contributor, category, reasoning, and simple review status.Awaiting review, without creating a judge run or 24-hour judge deadline.Product flow
Awaiting review.Automated preview unavailableand is retryable by the owner.ReviewedandRanked.The leaderboard is intentionally a later top-rated-submissions showcase across different prompts, not a scientific like-for-like benchmark. Same-prompt comparison groups remain later work.
Data and rollout notes
0023_overrated_leader.sqladds only durable preview-enrichment, generated-artifact, and preview-spend records. It is included for the eventual rollout but was not applied in this task.benchmax.xyz/*still routes to the staging worker. That is intentional for now; the route moves when production rolls out.Validation
Executed as one
&&-gated chain through push: