feat(frontend): isolate public trust and add adjudication#445
feat(frontend): isolate public trust and add adjudication#445ShivamB25 wants to merge 10 commits into
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
This PR is intentionally stacked. If it were marked as a standalone ready-to-merge change, reviewers would be looking at a diff that conceptually depends on code still under review in the earlier PR(s). Reviewing it in order keeps the trust-boundary changes, replay semantics, and public-surface changes easier to reason about. |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
4 issues found across 45 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/leaderboard/getLeaderboard.ts">
<violation number="1" location="packages/frontend/src/lib/leaderboard/getLeaderboard.ts:354">
P1: All-time leaderboard and user-rank now rank the full verified-user set in memory on each cache miss instead of using DB-side pagination/ranking.</violation>
</file>
<file name="packages/frontend/src/lib/submissionPersistence.ts">
<violation number="1" location="packages/frontend/src/lib/submissionPersistence.ts:329">
P3: Unsorted Set materialization makes persisted client/model arrays nondeterministic across identical submissions.</violation>
</file>
<file name="packages/frontend/src/app/u/[username]/ProfilePageClient.tsx">
<violation number="1" location="packages/frontend/src/app/u/[username]/ProfilePageClient.tsx:235">
P2: Locale/timezone-dependent `toLocaleString()` can cause server/client hydration mismatches and unstable displayed timestamps.</violation>
</file>
<file name="packages/frontend/__tests__/api/settingsSubmittedDataDelete.test.ts">
<violation number="1" location="packages/frontend/__tests__/api/settingsSubmittedDataDelete.test.ts:145">
P3: Use different mocked row counts for submissions vs. reviews so the test can catch a mix-up in which delete result is used to compute `deletedSubmissions`.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| @@ -1,7 +1,8 @@ | |||
| import { unstable_cache } from "next/cache"; | |||
There was a problem hiding this comment.
P1: All-time leaderboard and user-rank now rank the full verified-user set in memory on each cache miss instead of using DB-side pagination/ranking.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/frontend/src/lib/leaderboard/getLeaderboard.ts, line 354:
<comment>All-time leaderboard and user-rank now rank the full verified-user set in memory on each cache miss instead of using DB-side pagination/ranking.</comment>
<file context>
@@ -397,59 +307,65 @@ async function fetchLeaderboardData(
- const totalUsers = Number(globalStats[0]?.uniqueUsers) || 0;
- const totalPages = Math.ceil(totalUsers / limit);
+ const rankedUsers = rankLeaderboardUsers(allTimeUsers, sortBy);
+ const filteredUsers = rankedUsers.filter((user) =>
+ matchesLeaderboardSearch(user, search)
</file context>
| <ReviewMetaList> | ||
| <ReviewMetaItem> | ||
| <ReviewMetaLabel>Updated</ReviewMetaLabel> | ||
| <ReviewMetaValue>{new Date(data.nonCompetitive.updatedAt).toLocaleString()}</ReviewMetaValue> |
There was a problem hiding this comment.
P2: Locale/timezone-dependent toLocaleString() can cause server/client hydration mismatches and unstable displayed timestamps.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/frontend/src/app/u/[username]/ProfilePageClient.tsx, line 235:
<comment>Locale/timezone-dependent `toLocaleString()` can cause server/client hydration mismatches and unstable displayed timestamps.</comment>
<file context>
@@ -164,6 +190,80 @@ const EARLY_ADOPTERS = ["code-yeongyu", "gtg7784", "qodot"];
+ <ReviewMetaList>
+ <ReviewMetaItem>
+ <ReviewMetaLabel>Updated</ReviewMetaLabel>
+ <ReviewMetaValue>{new Date(data.nonCompetitive.updatedAt).toLocaleString()}</ReviewMetaValue>
+ </ReviewMetaItem>
+ {data.nonCompetitive.reasonCodes.length > 0 ? (
</file context>
| <ReviewMetaValue>{new Date(data.nonCompetitive.updatedAt).toLocaleString()}</ReviewMetaValue> | |
| <ReviewMetaValue> | |
| {new Intl.DateTimeFormat("en-US", { | |
| dateStyle: "medium", | |
| timeStyle: "short", | |
| timeZone: "UTC", | |
| }).format(new Date(data.nonCompetitive.updatedAt))} | |
| </ReviewMetaValue> |
| @@ -0,0 +1,353 @@ | |||
| import { eq, inArray, sql } from "drizzle-orm"; | |||
There was a problem hiding this comment.
P3: Unsorted Set materialization makes persisted client/model arrays nondeterministic across identical submissions.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/frontend/src/lib/submissionPersistence.ts, line 329:
<comment>Unsorted Set materialization makes persisted client/model arrays nondeterministic across identical submissions.</comment>
<file context>
@@ -0,0 +1,353 @@
+ reasoningTokens: totalReasoning,
+ dateStart: aggregates.dateStart,
+ dateEnd: aggregates.dateEnd,
+ sourcesUsed: Array.from(allClients),
+ modelsUsed: Array.from(allModels),
+ cliVersion: data.meta.version,
</file context>
| }); | ||
| mockState.setDeletedRows([{ id: "submission-1" }]); | ||
| mockState.setDeletedSubmissionRows([{ id: "submission-1" }]); | ||
| mockState.setDeletedReviewRows([{ id: "review-1" }]); |
There was a problem hiding this comment.
P3: Use different mocked row counts for submissions vs. reviews so the test can catch a mix-up in which delete result is used to compute deletedSubmissions.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/frontend/__tests__/api/settingsSubmittedDataDelete.test.ts, line 145:
<comment>Use different mocked row counts for submissions vs. reviews so the test can catch a mix-up in which delete result is used to compute `deletedSubmissions`.</comment>
<file context>
@@ -125,7 +141,8 @@ describe("DELETE /api/settings/submitted-data", () => {
});
- mockState.setDeletedRows([{ id: "submission-1" }]);
+ mockState.setDeletedSubmissionRows([{ id: "submission-1" }]);
+ mockState.setDeletedReviewRows([{ id: "review-1" }]);
const response = await DELETE(createRequest());
</file context>
| mockState.setDeletedReviewRows([{ id: "review-1" }]); | |
| mockState.setDeletedReviewRows([{ id: "review-1-1" }, { id: "review-1-2" }]); |
|
Hi @ShivamB25 — first, thank you for this substantial piece of work, and apologies that it sat unreviewed for so long. The public-trust isolation + adjudication design here is genuinely valuable and is not something main has implemented in the meantime, so we'd love to land it — but the branch can no longer be merged as-is. Where things stand: the merge base (48aa05c, Apr 17) is now ~350 commits behind main, and a test merge produces 20 conflicted files. The conflicts sit exactly on the subsystems this PR rewrites, and main has since rewritten them too:
A few smaller items from the earlier automated review that are still valid and worth folding in:
Also worth confirming before you invest: the adjudication routes gate on How this fits with the rest of your stack: this PR stacks on #443 and #444. I've left rebase notes on #443 (which we want landed close together with this one, so REVIEW_REQUIRED submissions have an adjudication path and nothing gets stranded), and I've closed #444 since main's regression-guarded merge superseded that branch structurally — see the note there for what remains salvageable. So the practical path is: rebase #443 first, then rework this one on top — or, given how much main has moved, possibly collapse the remaining work into a single fresh PR against current main. Happy to review promptly this time either way, and thanks again for the effort you put into this. |
2b04e66 to
2b30a80
Compare
|
@junhoyeo #445 is now rebased as a separate layer on the final #443 head. I adapted adjudication to current |
2b30a80 to
674524a
Compare
|
@junhoyeo #445 is restacked on final #443 head The follow-up fixes two delayed-adjudication invariants:
Regression coverage now includes sticky review upserts, first-write count = 1, stale metadata omission, receipt timestamp forwarding, and delayed A/B ordering. Verification: 73 files / 651 tests passed, typecheck and production build passed, lint has 0 errors (6 pre-existing warnings), fresh PostgreSQL migrations/checker passed through |
674524a to
61c561d
Compare
|
@junhoyeo Restacked #445’s five adjudication-only commits onto repaired #443 head GitHub now reports |
|
fk this. |
2 participants
Summary
Rebase onto current
mainand #443main's submission model: competitive rows are trusted writes, while quarantined payloads remain insubmission_reviews0016_nostalgic_senator_kelly.sqlplus generated snapshot/journal metadata forreviewed_at,reviewed_by_username,review_note, and the submission metadata receipt watermarknumeric(18,4)and extended migration checks through journal index 16Adjudication contract
SUBMISSION_REVIEW_API_TOKENis configured; browser sessions and ordinary personal tokens are not acceptedSUBMISSION_REVIEW_OPERATOR_USERNAMErecords the external operator identity, with a bounded fallback when unsettrustedorrejected, validates UUIDs, bounds request bodies, rejects non-object JSON, and records audit metadatasubmit_countwhen its trusted partition is first written; later approval of its queued partition does not increment it againsubmit_countonce when approval creates or updates competitive dataMetadata ordering
submissions.metadata_received_atis a dedicated receipt-order watermark; migration0016backfills it from each row's existingupdated_atbefore setting the default andNOT NULLsubmission_reviews.updated_atbefore adjudication) against that watermarkcliVersion, submission hash, session-shape metrics, schema metadata, and MCP server metadata cannot overwrite a newer direct submissioncompetitive_write_applied = truemonotonically with SQLORCache and rank consistency
RANK()snapshots using the same aggregate sources andnumeric(18,4)cost semantics as current public rank queriesVerification
bun run typecheckmain.rsconflict resolution: 1004 tests passed, 1 ignoredbun run build0..16, every migration applied, receipt watermark and competitive-write marker defaults/nullability verified, plus constraints/indexes/concurrency/rollback checks passed1; partial review approval kept1while adding its queued day1.0.0, accepted newer direct version9.0.0, then approved the older review; aggregate totals reached 200 whilecli_version = 9.0.0, MCP metadata stayednewer-direct-mcp, andsubmit_count = 2Stack
Depends on #443 commit
095a6975and is intentionally kept as a separate stack layer. #444 remains closed and untouched.Refs #441