fix(submit): harden trusted submission ingestion - #443
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
There was a problem hiding this comment.
2 issues found across 14 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="packages/frontend/src/lib/validation/submissionTrust.ts">
<violation number="1" location="packages/frontend/src/lib/validation/submissionTrust.ts:33">
P1: Unvalidated `timestampMs` conversion can throw `RangeError` and crash trust assessment instead of returning structured trust-state output.</violation>
</file>
<file name="packages/frontend/src/lib/db/migrations/0005_concerned_justin_hammer.sql">
<violation number="1" location="packages/frontend/src/lib/db/migrations/0005_concerned_justin_hammer.sql:5">
P2: `submission_reviews` lacks DB integrity constraints for trust state/domain and numeric/date invariants, allowing invalid records despite ingestion hardening.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
|
@junhoyeo @IvGolovach sorry to bother you I may be missing context here, but I noticed the public all-time leaderboard still appears to show some unusually large entries (hundreds of billions of tokens / very large cost totals across a relatively small number of submits). Since this PR is focused on hardening submit trust/persistence, could you share whether there has already been any review/backfill/removal plan for existing inflated leaderboard rows, or if there are follow-up steps planned? Thanks for taking a look. |
|
@ShivamB25 Hi, thank you for all your efforts! I’ll definitely review them when I have some spare time. |
|
Hey @ShivamB25 — first, apologies that this sat for so long; it's been open since April and main moved a lot underneath it in the meantime. That's on us, not you. The direction here is exactly right and still needed: trust-state decisioning with a review queue is the remediation path for the inflated-leaderboard entries you reported, and nothing on main has implemented it another way. Your latest commit (f0abfbe) also already addressed both cubic findings (timestamp guard + CHECK constraints), and the 55-test suite passes on the branch. Great work. That said, this needs a real rebase from you rather than a mechanical resolve on our side — the submit pipeline was substantially rewritten on main since you branched, and a few pieces of this PR now conflict with shipped work. Concretely: Migrations (blocker)
Validation (
Submit route (
Design questions to settle in the rebase
How this fits with the rest of your stack: once #443 is rebased, #445 (adjudication) needs the same treatment on top of it — see the notes I left there. We're closing #444 for now since main's regression-guarded merge covers the worst of the resubmit problem and the replay design needs a rethink on the new architecture (details on that PR), but #443 and #445 are the pieces we actively want to land. Happy to review promptly this time — this is work we want in. |
|
Thanks for the breakdown, this is useful. Main moving fast makes sense, I've got other things on my plate too, so not blaming the delay. I'll handle all four blockers: renumber the migration, drop the duplicate submit_count, widen total_cost, keep validation on main's stricter side. Will re-author the route on top of #517, #611, #616, #627, #675. On the three design questions: agree on REVIEW_REQUIRED over hard-reject for the modelId date heuristic. Will scope the 30-day rule to just the affected days. Will surface trustState in the response instead of silently returning submissionId: null. One ask. The leaderboard still shows inflated entries live in prod while this is open. Once I push the rebase, can we get a faster turnaround on review this time? I'll ping directly when it's ready. Planning to land #443 and #445 close together as you suggested. (example https://tokscale.ai/u/grenadeoftacoss ) |
f0abfbe to
2addd7c
Compare
|
@junhoyeo Rebase and re-authoring are complete against current |
|
@junhoyeo One final cross-stack correctness pass found and fixed submit-count accounting: the first accepted write now remains Exact #443 head is now |
3a4d529 to
095a697
Compare
|
@junhoyeo Rebased again onto current New #443 head: |
Summary
trusted,rejected, orreview_required) with machine-readable reason codesRebase onto current
main0015_submit_trust_gate.sqland regenerated0015_snapshot.jsonplusmeta/_journal.jsonsubmit_countcolumn migration and schema change becausemainalready provides it through0010_submit_count_safety.sqlsubmission_reviews.total_costtonumeric(18,4)to match0014_widen_cost_columns.sqlmain's stricter validation: client/token and cost-sum mismatches remain errors, including cost-without-tokens and token-breakdown checkssubmit_count = 1competitive_write_appliedreview marker so the stacked adjudication flow can avoid counting a partial submission twiceDesign decisions
timestampMs→ day-scoped review. Only affected days are queued. Trusted days from the same payload continue through the normal transaction, avoiding whole-submission withholding for legitimate historical backfills.trustState: "review_required",submissionId: null, separatemetricsandreviewMetrics, andcompetitiveWriteApplied. The Rust CLI printsSubmitted for review.orSubmitted; flagged days queued for review.rather thanSuccessfully submitted!.Additional ingestion bounds
Verification
bun run typecheckbun run build0..15, all migrations applied, schema/constraints/indexes verified, rollback and concurrent pending-review checks passedmain.rsconflict resolution: 1004 tests passed, 1 ignoredtrusted; duplicate-date →rejected; future-date →rejected; old day withouttimestampMs→review_required; model/date mismatch →review_required; no scenario returned 500submit_count = 1Stack
#445 remains separate and is rebased onto this exact branch head. #444 remains untouched and out of scope.