fix(submit): preserve multi-machine submissions without reshaping daily breakdown#389
Open
fix(submit): preserve multi-machine submissions without reshaping daily breakdown#389
Conversation
…ly breakdown Mainline stored a single submission row per user, so a later submit from another machine could overwrite flat per-client breakdowns for overlapping days. This scopes submissions by stable source identity, upgrades a lone legacy unsourced row in place on first source-aware submit, and aggregates profile/embed/leaderboard reads across rows while keeping the existing daily_breakdown source_breakdown shape flat. Constraint: Must keep the current database and existing daily_breakdown.source_breakdown shape Constraint: Must remain compatible with legacy unsourced rows during cutover Rejected: Nested per-device JSON in daily_breakdown | broader read-path churn and token-identity pitfalls Rejected: Bundle /api/me/stats and remote TUI sync | unrelated scope increase for the overwrite fix Confidence: high Scope-risk: moderate Reversibility: messy Directive: Keep embed/profile/leaderboard reads source-row aware; do not reintroduce single-row-per-user assumptions Tested: cargo fmt --all --check; cargo clippy -p tokscale-cli --all-features -- -D warnings; cargo test -p tokscale-cli; bunx vitest run packages/frontend/__tests__/api/submit.test.ts packages/frontend/__tests__/api/submitAuth.test.ts packages/frontend/__tests__/api/usersProfile.test.ts packages/frontend/__tests__/lib/dbHelpers.test.ts packages/frontend/__tests__/lib/getUserEmbedStats.test.ts; bunx vitest run packages/frontend/__tests__/lib/getLeaderboard.test.ts packages/frontend/__tests__/lib/getLeaderboardAllTime.test.ts; targeted frontend eslint on changed files Not-tested: Full frontend typecheck remains blocked by the pre-existing packages/frontend/src/components/BlackholeHero.tsx asset import typing error; live database migration rehearsal against production-like Postgres
…h PR review This reverts commit 344c05d from main so the multi-machine submission change can land through the normal PR path instead of a direct push. Constraint: User requested reverting the direct push and reopening the change as a PR Constraint: Must restore main without losing the already-validated patch Rejected: Force-reset main | destructive history rewrite on a published branch Rejected: Leave change on main and open a follow-up PR | does not satisfy the requested rollback Confidence: high Scope-risk: narrow Reversibility: clean Directive: Keep the replacement PR branch aligned with commit 344c05d content; do not sneak in extra scope during re-land Tested: git revert --no-commit 344c05d; git status review Not-tested: Re-running the full verification matrix after reverting main (revert only removes the already-tested patch)
Contributor
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Owner
Author
|
Rollout checklist for this PR:
Verification completed before opening the PR:
Known unrelated pre-existing issue:
|
Owner
Author
|
Release-note friendly summary:
|
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
Owner
Author
|
Suggested short merge description:
|
(cherry picked from commit 7a035ac)
(cherry picked from commit d09ab0a)
(cherry picked from commit 6126e02)
…ests Constraint: Keep the cherry-picked PR #388 snapshot lint-clean under this repo's frontend ESLint rules Rejected: Leave the original variable name | fails @next/next/no-assign-module-variable in local lint Confidence: high Scope-risk: narrow Reversibility: clean Directive: Keep this as a tiny follow-up on top of the original authored commits; do not fold broader changes into the credit-preserving rewrite Tested: packages/frontend/node_modules/.bin/eslint --config packages/frontend/eslint.config.mjs packages/frontend/__tests__/lib/getUserEmbedStats.test.ts Not-tested: Full frontend verification matrix (history rewrite only; behavior unchanged from prior verified branch)
junhoyeo
force-pushed
the
fix/source-scoped-multi-machine-submissions
branch
from
a505380 to
a339495
Compare
Owner
Author
|
Credit note: this PR branch now preserves the original authored work from #388 by @IvGolovach via cherry-pick history, instead of a single squashed commit. Preserved authored commits on this branch:
Small follow-up by me on top:
That means commit-level authorship is now properly attributed to the original author, and the branch keeps a small separate follow-up for the local lint-only rename. |
Owner
Author
|
Attribution note: This PR is based on and now preserves the original authored work from #388 by @IvGolovach. The branch history was rewritten so the original source-scoped submission commits are carried forward directly, with one small follow-up commit by me for local lint compatibility in a test file. Original authored work preserved here:
Small follow-up on top:
So the implementation credit should primarily go to @IvGolovach / #388, with my contribution limited to the branch re-land + the tiny lint-only follow-up. |
The source-id lock previously trusted a matching live PID indefinitely. If the PID had been recycled by an unrelated process, an old lock file could block first-time source ID generation until submit fell back to unsourced payloads. Constraint: Source-id initialization should stay resilient without introducing a broader lock format migration Rejected: Trust PID liveness forever | stale lock can survive PID reuse and block initialization Rejected: Remove any lock older than the short stale threshold | risks breaking a legitimately active locker on a slow filesystem Confidence: high Scope-risk: narrow Reversibility: clean Directive: Keep a hard age-based escape hatch even when PID probes report alive; PID alone is not a stable lock owner identity Tested: cargo fmt --all --check; cargo clippy -p tokscale-cli --all-features -- -D warnings; cargo test -p tokscale-cli test_should_remove_stale_source_id_lock -- --nocapture Not-tested: Full tokscale-cli test suite rerun after this narrow auth-lock change
Source-scoped submissions make it possible to inspect usage by machine, so this adds a dedicated sources/devices view on profile pages and a matching API that aggregates per-source totals and recent contribution history. Constraint: Must build on the source-scoped submission model without reshaping daily_breakdown.source_breakdown Constraint: Must fit the existing profile page flow with minimal extra round-trips Rejected: Force all source detail into the existing /api/users/[username] payload | keeps the core profile response leaner and separates concerns Rejected: Wait for a separate per-source detail API before shipping UI | unnecessary delay for a useful first device view Confidence: high Scope-risk: moderate Reversibility: clean Directive: Keep device/source viewing aligned with source_id as the stable identity and source_name as display-only metadata Tested: bunx vitest run packages/frontend/__tests__/api/userSources.test.ts packages/frontend/__tests__/api/usersProfile.test.ts; packages/frontend/node_modules/.bin/eslint --config packages/frontend/eslint.config.mjs src/app/u/[username]/page.tsx src/app/u/[username]/ProfilePageClient.tsx src/app/api/users/[username]/sources/route.ts src/components/profile/index.tsx __tests__/api/userSources.test.ts Not-tested: Full frontend typecheck still blocked by the pre-existing packages/frontend/src/components/BlackholeHero.tsx asset import typing error
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
The first device-view pass bundled summary and detail payloads into one sources endpoint. This separates the lightweight source list from the heavier per-source detail response so profile pages can show device cards without shipping full contribution histories for every machine up front. Constraint: Must keep source/device views aligned with the source-scoped submission model already on this branch Constraint: Must avoid bloating the profile page payload with every source's full contribution history Rejected: Keep a single /sources endpoint with embedded detail for all sources | unnecessary payload growth and tighter coupling between card list and detail graph Rejected: Drop server-side initial source detail entirely | worse first-load UX for the default selected source Confidence: high Scope-risk: moderate Reversibility: clean Directive: Keep /sources for summaries and /sources/[sourceId] for detailed histories; if more device UI is added, build on this separation rather than rejoining the payloads Tested: bunx vitest run packages/frontend/__tests__/api/userSources.test.ts packages/frontend/__tests__/api/userSourceDetail.test.ts packages/frontend/__tests__/api/usersProfile.test.ts; packages/frontend/node_modules/.bin/eslint --config packages/frontend/eslint.config.mjs src/app/u/[username]/page.tsx src/app/u/[username]/ProfilePageClient.tsx src/app/api/users/[username]/sources/route.ts src/app/api/users/[username]/sources/[sourceId]/route.ts src/app/api/users/[username]/sources/shared.ts src/components/profile/index.tsx __tests__/api/userSources.test.ts __tests__/api/userSourceDetail.test.ts Not-tested: Full frontend typecheck remains blocked by the pre-existing packages/frontend/src/components/BlackholeHero.tsx asset import typing error
Owner
Author
|
Update: the source/device profile work on this branch has been refined. What changed on top of the previous device-view pass:
New commit:
Verification for this follow-up:
Known unchanged pre-existing issue:
|
The device/source view now has lightweight summary and detail endpoints, but external consumers still need a compact per-source payload for cards, embeds, badges, or quick previews. This adds a dedicated summary route so clients can fetch one source's headline metrics without pulling the full contribution history. Constraint: Must build on the split source summary/detail API shape already on this branch Constraint: Must stay lightweight and avoid returning the full per-day history payload Rejected: Reuse the full source detail endpoint for summary consumers | unnecessary payload size for badge/embed/preview use cases Rejected: Add source summary fields only to the top-level user profile API | couples a focused source capability back into a broader profile response Confidence: high Scope-risk: narrow Reversibility: clean Directive: Keep source summary routes compact; if more source-level consumers appear, expand this route before bloating the detail response Tested: bunx vitest run packages/frontend/__tests__/api/userSources.test.ts packages/frontend/__tests__/api/userSourceDetail.test.ts packages/frontend/__tests__/api/userSourceSummary.test.ts packages/frontend/__tests__/api/usersProfile.test.ts; packages/frontend/node_modules/.bin/eslint --config packages/frontend/eslint.config.mjs src/app/api/users/[username]/sources/route.ts src/app/api/users/[username]/sources/[sourceId]/route.ts src/app/api/users/[username]/sources/[sourceId]/summary/route.ts src/app/api/users/[username]/sources/shared.ts __tests__/api/userSources.test.ts __tests__/api/userSourceDetail.test.ts __tests__/api/userSourceSummary.test.ts Not-tested: Full frontend typecheck remains blocked by the pre-existing packages/frontend/src/components/BlackholeHero.tsx asset import typing error
Owner
Author
|
Follow-up update on the same branch:
New commit:
Verification for this follow-up:
Known unchanged pre-existing issue:
|
The branch already exposed a lightweight source summary route, but the profile UI still consumed only the summary list and full detail payloads. This wires the selected device panel to fetch and display a compact preview from `/sources/[sourceId]/summary`, so the new endpoint is used for actual UI affordances rather than existing only for future consumers. Constraint: Must reuse the lightweight source summary endpoint instead of duplicating top-client/top-model derivation in the client Constraint: Must preserve the existing default selected-device UX Rejected: Continue showing only the full detail panel | leaves the new summary endpoint unused by the profile UI Rejected: Move summary-only fields back into the source list payload | defeats the endpoint separation introduced earlier Confidence: high Scope-risk: narrow Reversibility: clean Directive: Keep quick-preview metadata sourced from the summary endpoint so summary/detail responsibilities stay distinct Tested: bunx vitest run packages/frontend/__tests__/api/userSourceSummary.test.ts packages/frontend/__tests__/api/userSources.test.ts packages/frontend/__tests__/api/userSourceDetail.test.ts packages/frontend/__tests__/api/usersProfile.test.ts; packages/frontend/node_modules/.bin/eslint --config packages/frontend/eslint.config.mjs src/app/u/[username]/page.tsx src/app/u/[username]/ProfilePageClient.tsx src/app/api/users/[username]/sources/[sourceId]/summary/route.ts __tests__/api/userSourceSummary.test.ts Not-tested: Full frontend typecheck remains blocked by the pre-existing packages/frontend/src/components/BlackholeHero.tsx asset import typing error
Wrap getSourceSummaries, getSourceDetail, and getSourceSummary in try/catch so a network-level fetch failure (DNS, connection refused) degrades to empty data instead of rejecting Promise.all and crashing the entire profile page render.
Addresses review feedback on PR #389 before rollout: - H1: add .for("update") to the onConflictDoNothing fallback SELECT so a second transaction racing the insert re-reads the conflicted row under the row lock instead of between releases. - H2: replace brittle string-match 409 signaling with a SourceIdentityRequiredError subclass + instanceof check so middleware wrapping the error cannot silently turn 409 into 500. - H4: wrap decodeURIComponent in safeDecodeURIComponent (throwing InvalidSourceParamError) so crawler probes of /api/users/:u/sources/source:%ZZ return 400 instead of 500. - M3: move loadUserSubmitMetrics inside the submit transaction so the returned metrics match exactly what was written and we don't re-run three SELECTs on every submit. - M1: treat mtime.elapsed() failure as a stale lock (not 0 age) so a malformed lock file with clock-skewed mtime recycles immediately. - M2: replace English-only "No tasks are running" substring match with locale-agnostic CSV parse of tasklist output. - M4: comment explaining why migration 0005 must run as one transaction (no CONCURRENTLY — window between old and new uniqueness). - M5: update generateSubmissionHash docstring to mark it informational only now that the db uniqueness was dropped. - M6: remove placeholder "concurrent submissions" test that only asserted Set.size; leave a comment pointing at the real coverage. - Rollout: include an upgrade hint in the 409 response body so CLI users without source identity see actionable text. Also add the server-side rename endpoint the user asked for: - PATCH /api/settings/sources/[sourceId] { name: string | null } Session-authenticated, scope-checked by (user_id, source_id), rejects control characters, invalidates the user's profile cache. null / empty clears the custom label and falls back to the default at render time. - 9 vitest cases cover 401 / 400 / 404 / happy-path / legacy sentinel. Constraint: Cannot break existing CLI clients mid-rollout — old CLI submits without meta.sourceId still hit the unsourced scope and succeed. Rejected: Drop submission_hash column in this PR | keep the column and re-label as informational; a column-drop migration is a separate change. Rejected: Auth via Bearer API token | rename is a profile-affecting action and belongs to the web session, matching /api/settings/tokens. Confidence: high Scope-risk: moderate Directive: SourceIdentityRequiredError and InvalidSourceParamError must stay exported — the tests depend on instanceof checks against them. Not-tested: Real concurrent insert race on the fallback SELECT path (requires integration tests with an actual Postgres fixture). Not-tested: Windows tasklist CSV parse in non-English Windows (unit test only covers the structural parse).
Resolves a single conflict in ProfilePageClient.tsx: main restructured the tab panels into <div role="tabpanel"> wrappers (for a11y) while this branch added a new "sources" tab with old-style inline conditionals. Kept main's a11y wrapper pattern and wrapped the new sources tab contents in the same <div role="tabpanel" id="tabpanel-sources"> so tablist/tabpanel aria-controls stays consistent. Other intersecting files auto-merged cleanly: - validation/submission.ts — main's SUPPORTED_SOURCES enum next to this branch's M5 "informational fingerprint" docstring. - submit.test.ts — our M6 placeholder deletion preserved against main's test additions. - main.rs — PR #448's warm-cache subcommand and this branch's source-id persistence sit side by side. Verification: cargo test -p tokscale-cli (355 passed), bunx vitest (175 passed), cargo clippy -D warnings clean, eslint clean on resolution and touched files. tsc surfaces type errors only in main's new renderIsometric3DSvg.test.ts / renderProfileEmbedSvg.test.ts fixtures — pre-existing, unrelated to this merge. Confidence: high Scope-risk: moderate Directive: do NOT re-introduce the inline `{activeTab === "sources" && ...}` form — accessibility wiring in ProfileTabBar's aria-controls targets `tabpanel-sources`, so the wrapping div must keep that id. Not-tested: interactive a11y traversal (screen reader / keyboard tab cycling) across the new tabpanel wrapper in sources.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
The column was originally a dedup key backing the submissions_user_hash_unique constraint. Migration 0005 already drops that constraint because source-scoped submissions can legitimately share a client/date fingerprint across machines, and nothing in the codebase reads submission_hash — there is no SELECT, no JOIN, no ORDER BY, and no implicit use in the onConflictDoNothing target on the submit path. It was paying djb2 compute per submit and storing dead bytes. Changes: - Append ALTER TABLE submissions DROP COLUMN submission_hash to migration 0005 so the reshape lands atomically with the schema changes that removed the constraint, keeping this PR's DB touches to a single migration file. - Remove submissionHash from the drizzle schema, drop the generateSubmissionHash generator from validation/submission.ts, and strip the two writes in api/submit/route.ts along with the now-dead hashData payload reshaping. - Update submitAuth.test.ts to drop the generateSubmissionHash mock. Constraint: Migration 0005 already reshapes this table in one transaction; bundling the DROP COLUMN into the same file keeps the rollback cost equivalent to what it already was — rolling back after source-scoped rows exist is not free either way. Rejected: Keep the column as a "debugging fingerprint" | no read site exists today, the CLI does not send or log the hash, and an index to make queries by it fast would just add more dead weight. If a future forensic use case appears, re-add with an index at that point. Rejected: Split into a separate follow-up PR | the PR author asked to bundle; application code and SQL stay consistent that way and a reviewer sees the constraint drop and the column drop together. Confidence: high Scope-risk: narrow Directive: Do NOT re-export generateSubmissionHash — the helper is gone and tests now assume that module surface. Not-tested: Post-deploy rollback path (same caveat as the rest of migration 0005).
Addresses the three residual low-severity findings from the review of PR #389 and hardens the test suite with the one integration scenario that was previously missing. - L3: reject \p{C} control / format / surrogate / private-use / unassigned characters in submit-time sourceId and sourceName. The rename endpoint already enforces this; mismatched validation between the two write paths let a malicious CLI plant strings the rename endpoint could not un-plant without a round trip. Test coverage: five new cases in submit.test.ts (null byte, ANSI escape, ZWJ, RTL override, and ZWJ in sourceId). - L4: tie-break topClient / topModel alphabetically in the /sources/ [sourceId]/summary route. Equal token counts previously returned whichever Object.entries iteration order the JSON serialization produced first, which depends on DB row ordering — not stable across requests and caused UI flicker. Test coverage: a dedicated case with deliberately zulu-first insertion to prove the resolver returns "alpha" on ties. - L5: stop swallowing fetch failures in ProfilePageClient. A dropped detail fetch used to leave the Devices tab stuck on the loading spinner forever because the only failure handler was console.error. Add a sourceDetailErrorCache keyed by sourceKey plus a SourceErrorCard rendered in place of the spinner. On a successful retry the error entry is cleared from within the async .then callback, keeping the effect body free of synchronous setState (which would trip react-hooks/set-state-in-effect). - Rollout scenario: add an end-to-end test that drives the real route through a transaction callback with a mocked tx.select returning a source-scoped row and verifies the 409+hint response comes out of the actual SourceIdentityRequiredError throw path — not just the outer catch invoked by a manually thrown error. Constraint: react-hooks/set-state-in-effect forbids synchronous setState in effect bodies; error-clear has to live inside the .then callback so it runs post-await. Rejected: A component-level Testing Library test for the ProfilePageClient error card | the repo has no existing React component test harness and adding one is scope creep; visual QA on staging is the faster path to confidence here. Confidence: high Scope-risk: narrow Directive: OptionalSourceMetadataSchema is the single source of truth for sourceId / sourceName validation. Adding a third write path that bypasses it would re-open L3. Not-tested: Right-to-left override glyphs actually rendering in the profile UI (schema now blocks them; no UI-level assertion needed). Not-tested: Real-DB integration test for the rollout 409 path (covered via tx mock, not a live Postgres fixture).
Contributor
|
You're iterating quickly on this pull request. To help protect your rate limits, cubic has paused automatic reviews on new pushes for now—when you're ready for another review, comment |
The multi-machine submit work in this PR rides on a pile of new helpers in auth.rs (lock serialization, stale takeover, per-device source-id persistence, hostname fallback) that previously only had happy-path coverage of their two public entry points. Fill in the internal helpers so regressions surface at the unit level rather than in production: - Lock state codec: serialize_source_id_lock_state format assertion, round-trip, whitespace tolerance, unknown-key ignore, partial/ malformed content rejection. - lock_age: future created_at_ms clamps to zero; past delta is in the expected window; absent metadata returns FORCE_STALE. - read_source_id_lock_state / remove_source_id_lock_if_matches: file missing, state mismatch (no delete), exact match (delete), missing path (false). - read_source_id: whitespace trim, empty-file → None, missing-file → None. - write_source_id: trailing newline, atomic overwrite, temp-file cleanup after rename. - get_device_name: format prefix + non-empty host component. - should_remove_stale_source_id_lock: dead-owner-but-fresh-age and unknown-probe-but-fresh-age branches (both should NOT remove). - acquire_source_id_lock: happy path (create + Drop cleanup) and stale-takeover past FORCE_STALE threshold. - get_source_id_path / get_source_id_lock_path: HOME-scoped paths. - get_submit_source_id: env override skips disk; whitespace env falls through and generates+persists. - get_submit_source_name: default fallback to get_device_name; empty env treated as unset. - current_unix_ms: sanity range check. Net effect on tokscale-cli tests: 355 → 383 passing (28 new cases). tarpaulin workspace coverage climbs from 41.18% to 50.52% (+9.34pp). Constraint: Tests must not collide on env mutation — all HOME/env tests carry #[serial]. Rejected: Integration tests covering logout / whoami / open_browser | these hit the network, the terminal, or the OS browser; out of scope for a unit test suite and risky to run in CI. Confidence: high Scope-risk: narrow Directive: If a helper in auth.rs gains a new branch, add a unit case here — the file is now the coverage anchor for the CLI crate. Not-tested: Real concurrent lock contention across processes (only single-process stale takeover is exercised).
![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.
Low-coverage parsers made the workspace coverage number look worse than it deserved. Adding parser-level unit tests for kilo, droid, and synthetic closes most of the gap and is low-risk: the tests drive real code paths with on-disk fixtures (temp SQLite DB, settings.json, sibling .jsonl), not mocks. Files touched and new coverage vs. previous tarpaulin run: - sessions/kilo.rs 0/61 → 56/61 (0% → 91.8%) - sessions/droid.rs 35/103 → 101/103 (34% → 98.1%) - sessions/synthetic.rs 35/105 → 104/105 (33% → 99.0%) Test additions: - kilo.rs — 10 new cases built on an in-memory SQLite fixture: happy path with full provider/token/agent fields; missing-file → empty; user-role filter via the parser's SQL WHERE; skip rows missing modelID; fallback_timestamp used when time is absent; negative tokens and cost clamp to zero; session_id defaults to "unknown"; agent field wins over mode fallback; provider inferred from model via provider_identity; defaults to "kilo" when inference fails; malformed-json row skipped without taking the batch down. - droid.rs — 11 new cases covering the settings.json loader: full happy path (providerLockTimestamp → millis); missing file; malformed JSON; missing tokenUsage; all-zero tokens; model-less payload falls back to get_default_model_from_provider; model-less payload extracts from sibling .jsonl system-reminder; provider inferred when providerLock absent; negative tokens clamp and timestamp falls back to file mtime; normalize_model_name duplicate hyphen collapse; extract_model_from_jsonl found vs. pattern absent. - synthetic.rs — 10 new cases: is_synthetic_gateway combined check; normalize strips "hf:" without slash; "accounts/…" without "/models/" passthrough; normalize_synthetic_gateway_fields returns false for non-gateway; empty-provider gets rewritten to "synthetic"; matches_synthetic_filter matches by client name alone. Plus five parse_octofriend_sqlite SQLite fixture tests: empty-when-no-known- tables, messages table happy path, zero-token row skip, seconds- timestamp → ms conversion, token_usage fallback table parsed when messages absent. Workspace totals: tarpaulin moves from 41.18% → 51.94% lines covered (+10.76pp absolute; was +9.34pp from auth.rs alone). tokscale-core lib tests 497 → 553 passing. No production code touched. Constraint: parser tests must not depend on the real ~/.factory or ~/.local/share paths — all fixtures go through tempfile::tempdir so test runs are hermetic and parallel-safe. Rejected: Mock rusqlite at the trait level | real SQLite fixtures are cheap, catch schema drift, and match the established pattern in sessions/opencode.rs. Confidence: high Scope-risk: narrow Directive: If a session parser gets a new column/branch, extend the corresponding test module in this file — the parser test layout is now the repository convention for this kind of change. Not-tested: Real filesystem races against a live Droid/Kilo client (scope creep; covered indirectly by integration runs).
…lock parse Address the three unresolved review threads on PR #389: 1. devin-ai-integration (🔴 real bug): CLI submits were clobbering user renames. Every `tokscale submit` sent `sourceName = "CLI on <host>"` and the route updated submissions.sourceName unconditionally, so a PATCH /api/settings/sources/:sourceId rename only survived until the next submit. Restrict sourceName (and sourceId) writes in the update branch to the upgradeLegacyRow case only — i.e., the first time a legacy unsourced row is being promoted to source-scoped. Existing source-scoped rows keep whatever sourceName is already in the DB. Two new vitest cases in submitAuth.test.ts lock this down: - preserves a user-renamed sourceName on subsequent merges - still writes sourceId+sourceName when upgrading a legacy row 2. cubic-dev-ai (P2): Windows PID probe split tasklist CSV output by ',' and read column 1 as the PID, which misreads any process whose image name legitimately contains a comma (CSV quotes the field but the naive split does not honor quoting). `tasklist /FI "PID eq N"` is already server-side filtered — zero rows on no match, one row on match — so we only need to detect whether any non-empty CSV row came back. No parsing of the PID column is required. 3. devin-ai-integration (🟡): parse_source_id_lock_state's `line.split_once('=')?` aborted the whole parse on the first line without an `=` (stray blank line, trailing whitespace, future metadata key). Skip unrecognized lines with `else { continue }`; a valid pid/created_at_ms pair still produces Some(state). Updated existing test to assert the new forgiving behavior via a fixture that mixes garbage, blank, and valid lines. Constraint: Write-path for sourceName has to stay split across insert / upgrade / update branches — the insert path already stamps sourceName on the fresh row, the upgrade path must stamp it for the first time on a former legacy row, and the plain update path must NEVER touch it (per user's rename intent). Rejected: Add a source_name_custom boolean | adds a column + a write-path branch for no extra invariant over the "only write on insert/upgrade" rule. Rejected: CSV-aware parse of tasklist output | the server-side PID filter gives us the presence check for free; parsing adds failure surface without buying anything. Confidence: high Scope-risk: narrow Directive: Do NOT re-add `submissionUpdate.sourceName = sourceName` outside the upgradeLegacyRow guard — the rename preservation contract depends on it and is covered by the submitAuth tests. Not-tested: Windows tasklist CSV with a commaed image name (no CI runner reproduces it; fix is pure simplification so there's nothing new to misparse).
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
The prior "any non-empty line is a match" simplification I pushed in
response to the cubic CSV-parse concern regressed into a different
bug: `tasklist /FI "PID eq N" /FO CSV /NH` still writes a localized
INFO banner to stdout when no PID matches — e.g. English emits
INFO: No tasks are running which match the specified criteria.
That line is non-empty, so the probe would report every dead PID
as alive on Windows, and the stale-lock cleanup would never fire
until FORCE_STALE_AFTER (10s) kicked in on every acquire.
Fix per the reviewer's exact suggestion: distinguish CSV data rows
from the INFO banner by `line.trim().starts_with('"')`. Because
`/FO CSV` wraps every field in double quotes, data rows always
start with `"`, while the banner (in any locale) does not. This
also keeps the "no naive `split(',')` over the PID column"
property the earlier change was trying to preserve — process
names with commas remain correctly quoted data rows.
Also extract the classification into `tasklist_output_indicates_match`
so it can be unit-tested on all platforms, not just cfg(windows).
Six new cases lock the contract:
- accepts a CSV data row
- rejects the English INFO banner
- rejects a non-English banner (Korean fixture) to prove the
locale-agnostic property
- rejects empty / whitespace-only output
- accepts a process name that contains a comma
- ignores leading/trailing blank lines around a data row
cargo test -p tokscale-cli: 383 → 389 passing. clippy clean.
Constraint: The real cfg(windows) `lock_owner_is_alive` can't run on
the macOS/Linux test runners; the helper is compiled on all
platforms and gated with `#[cfg_attr(not(windows), allow(dead_code))]`
so this regression surfaces in CI everywhere.
Rejected: Full CSV parser (csv crate) | overkill; the presence-of-
quoted-row check is sufficient and faster.
Rejected: Filter banner by prefix "INFO:" | locale-dependent — the
banner starts with 정보: on Korean Windows, información: on Spanish,
etc. `starts_with('"')` is the only locale-agnostic signal.
Confidence: high
Scope-risk: narrow
Directive: If a future refactor tries to simplify the Windows branch
back to a plain non-empty check, the INFO-banner test will fail —
keep the CSV-data-row guard.
Not-tested: Calling `tasklist.exe` for real on a Windows CI runner
(no Windows CI yet; helper is exercised via fixtures instead).
…it_count Surfaced by the prod-DB audit during PR #389 review. All changes safe to ship alongside 0005 because 0006 only touches indexes and a no-op ADD COLUMN IF NOT EXISTS. Index cleanup — pg_stat_user_indexes on prod at the time of this migration: idx_submissions_user_id 214 scans (redundant with idx_submissions_leaderboard, which starts with user_id and serves every plain user_id lookup as a left-prefix) idx_submissions_status 1 scan idx_submissions_total_tokens 0 scans idx_submissions_date_range 0 scans idx_submissions_leaderboard 3,270,000 scans ← kept, it earns its keep Dropping the four trims INSERT/UPDATE overhead on every `tokscale submit` for zero query-path loss. FK coverage — device_codes.user_id is the only FK column in the schema without a covering index. Small table, so cascade-delete on a user currently seq-scans it; adding the index pins the cost to log(n) forever. submit_count safety net — this column exists on prod (added via `drizzle-kit push` some time ago, which writes straight from schema.ts without emitting a SQL file) but no earlier `.sql` migration has an ALTER TABLE for it. A fresh developer restore via `drizzle-kit migrate` from 0000..0005 therefore ends up without the column, and the app crashes at first submit. `ADD COLUMN IF NOT EXISTS` here is a no-op on prod and a correctness fix on every fresh environment. Confirmed via a fresh-DB replay: all 7 migrations now produce the expected final schema including submit_count. Verification: - Dry-run on a full prod clone (pg_dump → local postgres:17): 0005 + 0006 applied in one transaction, total ~3 ms. Post- migration submissions has 5 indexes (pkey, created_at, leaderboard, user_source_unique, user_unsourced_unique), device_codes has 7 including the new user_id one, `submit_count` column reported as "already exists, skipping" on prod (expected) and is present on fresh replay. - schema.ts updated so drizzle's diff stays clean: removed the four dropped indexes, added idx_device_codes_user_id. - bunx vitest: 184/184 passing, tsc clean, eslint clean. Constraint: IF EXISTS / IF NOT EXISTS guards are required — prod already has submit_count and the to-be-dropped indexes, fresh DBs do not. Idempotent migration shape avoids divergence. Rejected: Split into two migrations (index cleanup + submit_count backfill) | they're all "schema drift fallout from the PR #389 audit" and shipping one migration in one transaction keeps the rollout window minimal. Rejected: Drop idx_submissions_created_at too | it has 11,668 prod scans, still used by time-range queries. Confidence: high Scope-risk: narrow Directive: If you add a new FK column elsewhere in schema.ts, give it a covering index in the same migration. The audit caught device_codes.user_id by accident — there's no lint for it. Not-tested: A prod-scale bloat or long-running query interaction while 0006 is in its ~3 ms transaction (no way to simulate without hitting live prod).
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
submissionsrows withsource_id/source_nameinstead of collapsing every account into a single rowdaily_breakdown.source_breakdownflat and avoid bundling#329-style/api/me/stats/ remote TUI sync scopeWhy
mainstored a singlesubmissionsrow per user and/api/submitselected that row byuserIdonly. That meant a later submit from another machine could overwrite overlapping flat per-client breakdowns for the same user/day instead of preserving both machines' contributions.What changed
Database / schema
submissions.source_idsubmissions.source_name(user_id, source_id)for source-scoped rowsdaily_breakdown.source_breakdownunchangedSubmit flow
meta.sourceId/meta.sourceNameundefined409when a scoped account later submits without source identityRead aggregation
CLI
TOKSCALE_SOURCE_IDTOKSCALE_SOURCE_NAMEScope notes
#329-style/api/me/statsdaily_breakdown.source_breakdownshapeVerification
cargo fmt --all --checkcargo clippy -p tokscale-cli --all-features -- -D warningscargo test -p tokscale-clibunx vitest run packages/frontend/__tests__/api/submit.test.ts packages/frontend/__tests__/api/submitAuth.test.ts packages/frontend/__tests__/api/usersProfile.test.ts packages/frontend/__tests__/lib/dbHelpers.test.ts packages/frontend/__tests__/lib/getUserEmbedStats.test.tsbunx vitest run packages/frontend/__tests__/lib/getLeaderboard.test.ts packages/frontend/__tests__/lib/getLeaderboardAllTime.test.tsKnown residual risks
409after an account enters source-scoped modesubmissionsrow per usersubmission_hashuniqueness under source scoping may need a follow-up decisionsourceNameRollout notes
409Summary by cubic
Preserves multi‑machine submissions by scoping
submissionsto stable source identity without changingdaily_breakdown. Adds device APIs/UI with rename, stricter validation, preserves user‑renamed device labels, and further hardens the CLI source‑id lock on Windows.New Features
(user_id, source_id)with in‑place upgrade for a lone legacy unsourced row; 409 with upgrade hint when a scoped account submits without identity; reject control characters insourceId/sourceName; stronger insert‑race handling; preserve user‑renamedsourceName(only setsourceId/sourceNamewhen upgrading a legacy row); per‑day merge unchanged.submitCount; invalid/encoded source keys return 400; namespaced route keys avoid__legacy__collisions; tie‑break top client/model alphabetically for stable ordering.GET /api/users/[username]/sources(summaries),GET /api/users/[username]/sources/[sourceId](detail),GET /api/users/[username]/sources/[sourceId]/summary(lightweight summary), andPATCH /api/settings/sources/[sourceId]to rename/clear a device label; Profile adds a “Devices” tab with per‑source totals and a summary‑powered preview; failed device‑detail fetch shows an inline error card; tab panels use a11y wrappers.meta.sourceId/meta.sourceName, add a stale source‑id lock timeout with takeover, harden Windows PID probe by detecting CSV data rows and ignoring localized INFO banners (no naive CSV parse), and make lock‑state parsing tolerant of stray lines.droid,kilo, andsyntheticsession parsers intokscale-core; expandedtokscale-clitests for lock handling and Windows probes; no runtime changes.Migration
submissions.source_id/source_name; drop single‑row‑per‑user uniqueness; add a partial unique index for unsourced rows and a unique(user_id, source_id)constraint for scoped rows; dropsubmission_hash.submissionsindexes, add a covering index ondevice_codes.user_id, and addsubmissions.submit_countif missing for fresh environments.daily_breakdown.source_breakdown.Written for commit 9268185. Summary will update on new commits.