fix(v0.5): CVE patch, token preflight gate, destination fix, 22 tests, CI gate#14
fix(v0.5): CVE patch, token preflight gate, destination fix, 22 tests, CI gate#14aakifshamsi wants to merge 4 commits intomainfrom
Conversation
…preflight - ui/package.json: next 15.3.0 → 15.3.8 (CVE-2025-55182 RCE patch) - route.ts: add token preflight gate — dispatch is now blocked with HTTP 400 if source account token is 404/revoked/email-mismatch before GitHub dispatch - route.ts: fix boolean workflow inputs — dry_run/skip_dedup/delete_from_source now passed as actual booleans instead of "true"/"false" strings - route.ts: add WORKER_URL, WORKER_AUTH_TOKEN, GMAIL_SOURCE_USER to required env checks - migrate.py: add preflight_check() function — verifies token is valid and email matches expected account; exits FATAL before touching any mail if token is bad Fixes issue #12 (migration reported success with revoked token) https://claude.ai/code/session_01QYvJ82EXS3meeGykiutSrJ
…ation derivation
- page.tsx: restore full-featured dashboard (accounts, folder mirror table,
migration controls, per-destination status, jobs panel) — previously
overwritten by merge conflict resolution
- page.tsx: destination auto-derived from account dropdowns
(d1Acc && d2Acc → "both", d1Acc only → "dest1", d2Acc only → "dest2")
removes the separate manual destination dropdown that defaulted to "both"
regardless of which account slots were populated
- page.tsx: token status badge (✅/❌) shown per account card via new
/api/token-status endpoint; dispatch buttons disabled if source token is invalid
- page.tsx: source token invalid → inline re-auth link shown below accounts panel
- api/token-status/route.ts: new GET endpoint — checks CF Worker vault + Gmail
/profile, returns { email, valid, error?, checkedAt } (issue #13)
- api/status/route.ts: already returns per-destination data (dest1/dest2 separately)
— no change needed; step 7 is satisfied
https://claude.ai/code/session_01QYvJ82EXS3meeGykiutSrJ
- tests/conftest.py: shared mock fixtures (FakeHTTPResponse, mock_urlopen, valid/mismatched profile response helpers) - tests/test_migrate.py: 10 tests covering preflight_check (401, mismatch, case- insensitive, network error), extract_fingerprint_from_raw, record_migrated_message - tests/test_cleanup.py: 12 tests covering fingerprint_matches, load_cleanup_candidates (intersection, empty manifest, missing file, single dest), trash_message - ui/src/__tests__/trigger.test.ts: Vitest tests for /api/trigger — boolean inputs (dry_run/delete_from_source as booleans not strings), token preflight gate (404/401/ mismatch all return 400 without calling GitHub dispatch), destination passthrough - ui/package.json: add vitest + @vitest/coverage-v8 devDeps + npm test script - .github/workflows/ci.yml: pytest + vitest on push to main and PRs touching scripts/, ui/src/, tests/ — blocks merge on test failure All 22 pytest tests pass with zero real credentials https://claude.ai/code/session_01QYvJ82EXS3meeGykiutSrJ
- .devcontainer/devcontainer.json: Python 3.12 + Node 20, KiloCode extension,
forwards port 3000, mounts volumes for bash history + npm cache, reads
required env vars from Codespaces secrets. Enables full dev workflow from
Firefox on Android via github.dev / Codespaces without local setup.
- scripts/setup-dev.sh: bootstrap script for local and cloud dev:
Mode A (default): interactive prompts, writes ui/.env.local
Mode B (--gh): explains GitHub Secrets fetch (gh CLI)
--check: readiness summary without writes
Also configures ~/.kilo-code/config.json with OpenRouter + claude-sonnet-4-5
for AI assistance in Codespaces via KiloCode extension.
https://claude.ai/code/session_01QYvJ82EXS3meeGykiutSrJ
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 14
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/ci.yml:
- Around line 6-11: pull_request.paths currently only matches "ui/src/**" so
changes to UI dependency files can skip the CI; update the pull_request.paths
list to explicitly include the UI package files (add entries "ui/package.json"
and "ui/package-lock.json") or broaden the pattern to "ui/**" so updates to
Next/Vitest or lockfiles trigger the workflow; modify the pull_request.paths
block (the pull_request.paths symbol in the CI YAML) to include those entries.
In `@scripts/migrate.py`:
- Around line 601-606: The token acquisition and preflight validation
(get_source_token, get_dest_token, preflight_check, src_token/dst_token) must
run before the run is persisted as status="running" so failures don't leave a
stale state; move the calls to get_source_token/get_dest_token and
preflight_check earlier (before the code that writes status="running") or wrap
them in a try/except that updates the run state and sends failure notifications
on any exception, ensuring any exception during token validation always triggers
status cleanup/notification rather than leaving the file stuck as running.
In `@scripts/setup-dev.sh`:
- Around line 87-100: The read_secret function is returning prompt text and
malformed values when .env.local already exists because the prompt echo goes to
stdout and the empty-input branch echoes "$val=$existing" producing "=existing";
update read_secret so the prompt is written to stderr (e.g., use printf or echo
>&2) so it isn't captured by callers using $(read_secret ...), and change the
empty-input branch to output just the existing value (echo "$existing") instead
of "$val=$existing"; reference function name read_secret and variables ENV_FILE,
var, existing, val when making the fix.
In `@ui/src/__tests__/trigger.test.ts`:
- Around line 174-194: Add two more tests mirroring the existing "passes
destination=dest1" case to cover the other allowed values: create tests that
call the same POST handler (imported from "../app/api/trigger/route") with
request bodies containing destination: "dest2" and destination: "both", stub
fetch the same way using fetchMock and makeResponse, invoke await POST(req),
then assert the forwarded payload via capturedBody(fetchMock, 2) has
inputs.destination equal to "dest2" and "both" respectively; keep the test
setup, vi.stubGlobal("fetch", fetchMock), and the same sequence of mocked
responses so the tests match the structure around the existing POST, fetchMock,
makeResponse, and capturedBody usage.
- Around line 39-48: The tests mutate the shared process.env using
Object.assign(process.env, ENV) in beforeEach which can leak if a test aborts;
instead use vi.stubEnv(ENV) to isolate env vars (replace the Object.assign line
in the beforeEach with vi.stubEnv(ENV)) and remove the manual deletion loop in
afterEach (keep vi.restoreAllMocks() and vi.resetModules() as-is) so Vitest will
automatically restore the environment between tests.
In `@ui/src/app/api/token-status/route.ts`:
- Around line 14-16: The GET handler for /api/token-status (exported function
GET) must require and verify authenticated callers before using the email query
(e.g., check a session, JWT, or API key and return 401/403 for unauthenticated
requests) to prevent account enumeration; add an explicit authorization check at
the top of GET and validate that the caller is permitted to query the requested
email. Also wrap each upstream fetch used in this route (the Worker Vault and
Gmail API fetches) with an AbortController and a reasonable timeout (e.g.,
5–10s): create an AbortController, pass controller.signal to fetch, schedule
controller.abort() after the deadline, and ensure you clear the timeout after
the response to avoid leaks. Ensure error handling distinguishes aborted/timeout
errors and returns a proper 504 or 502 to the client.
- Around line 37-40: The token fetch in route.ts (the call that creates tokenRes
using workerUrl/workerToken/email) and the Gmail API fetch call need request
timeouts to avoid blocking the route handler; wrap each fetch with an
AbortController, start a setTimeout that calls controller.abort() after a
sensible timeout (e.g., a few seconds), pass controller.signal into the fetch
options for the token fetch (the code creating tokenRes) and the Gmail API fetch
(the function/method that calls Gmail), and clear the timeout on success or in
finally so you don’t leak timers; handle AbortError/DOMException to return a
proper timeout response from the route.
In `@ui/src/app/api/trigger/route.ts`:
- Around line 6-8: The WORKER_URL read into the workerUrl constant is not being
normalized like elsewhere—strip any trailing slash(es) when assigning workerUrl
(e.g., replace trailing / with empty string) so the preflight URL built from
workerUrl doesn’t end up with a double-slash; update the assignment of workerUrl
(the const workerUrl = process.env.WORKER_URL) to normalize by trimming trailing
slashes (overwriting workerUrl or storing into a clearly named workerBaseUrl) so
behavior matches token-status and migrate scripts.
- Around line 113-124: The inputs object is using unsafe Boolean(...) coercion
for fields like dryRun, skipDedup, and deleteFromSource which makes string
values like "false" truthy; replace those with a proper string-to-boolean
conversion helper (e.g., parseBoolean) that checks typeof value and treats
"true"/"1" (case-insensitive) as true, "false"/"0" as false and returns the
boolean for actual booleans/numbers, then use parseBoolean(dryRun),
parseBoolean(skipDedup), parseBoolean(deleteFromSource) when building inputs to
ensure JSON/string inputs are correctly interpreted.
In `@ui/src/app/page.tsx`:
- Around line 191-198: The doJobAction function needs proper error handling and
user feedback: wrap the fetch call in a try/catch (keeping the existing finally
that calls setJobAction(null)), check the fetch response.ok and throw on non-2xx
so failures are caught, and surface errors to the UI (e.g., call an existing
notification/toast helper or set an error state) so users know if cancel/rerun
failed; update references to runId, action, setJobAction and fetchJobs in this
change so the action still triggers fetchJobs on success (or after failure if
appropriate) and ensure errors are logged for debugging.
- Around line 228-235: The computed destination variable incorrectly defaults to
"dest2" when neither destination is selected; update the expression that defines
destination (currently using d1Acc and d2Acc) to explicitly handle the "neither
selected" case—e.g. change the ternary to return undefined (or an empty string)
when both d1Acc and d2Acc are falsy: const destination = d1Acc && d2Acc ? "both"
: d1Acc ? "dest1" : d2Acc ? "dest2" : undefined; also ensure any downstream use
in runMigration (and related checks around srcTokenValid/srcTokenInvalid) safely
handles the new undefined value or adjust types accordingly.
- Around line 289-306: The source selector is currently using the full accounts
list (accounts) so it can pick an account that is also chosen as a destination;
update the options prop for the source entry in the mapped array to use
otherAccounts(exclude) like the destination selectors do. Locate the array items
where label "Source (G1)" and the mapped render use value={srcAcc} and
onChange={setSrcAcc} and change its Select options from accounts to
otherAccounts(exclude) (keep placeholder "— select —"); no other behavior
changes needed.
- Around line 203-217: Both polling useEffect blocks can trigger fetchStatus
concurrently (status poll using pollRef and job poll using jobPollRef), causing
redundant network calls; modify the job-polling logic in the second useEffect
(the one referencing jobs, jobPollRef, fetchJobs, fetchStatus) so it does not
call fetchStatus if the status poll is already active (i.e., if pollRef.current
is truthy), or alternatively consolidate into a single shared interval that runs
fetchJobs and fetchStatus at the desired cadence; update the condition that sets
jobPollRef and the interval callback accordingly to only call fetchJobs when
pollRef.current exists, and ensure cleanup still clears jobPollRef as before.
- Around line 4-14: The FolderRow interface and UI assume
g1Count/g2Count/g3Count exist but the /api/folders response only provides
messagesTotal and estimatedBytes; update the UI mapping instead of the API:
change FolderRow to reflect the API (rename or add messagesTotal: number) and,
where the fetch/transform occurs (the code that builds rows from the
/api/folders response), map messagesTotal → g1Count and set g2Count/g3Count to 0
(or null-coalesce to 0) so mirrorCell(row.g1Count, row.g2Count) always receives
numbers and row.g1Count.toLocaleString() is safe; also update any usages of
FolderRow (e.g., FoldersData, mirrorCell calls and display code) to use the new
mapping/guarding.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 9c844c0b-4e28-49b8-b809-f1118f70457e
📒 Files selected for processing (12)
.devcontainer/devcontainer.json.github/workflows/ci.ymlscripts/migrate.pyscripts/setup-dev.shtests/conftest.pytests/test_cleanup.pytests/test_migrate.pyui/package.jsonui/src/__tests__/trigger.test.tsui/src/app/api/token-status/route.tsui/src/app/api/trigger/route.tsui/src/app/page.tsx
📜 Review details
🧰 Additional context used
🪛 Ruff (0.15.7)
scripts/migrate.py
[error] 134-136: Audit URL open for permitted schemes. Allowing use of file: or custom schemes is often unexpected.
(S310)
[error] 138-138: Audit URL open for permitted schemes. Allowing use of file: or custom schemes is often unexpected.
(S310)
[warning] 149-149: Do not catch blind exception: Exception
(BLE001)
tests/test_cleanup.py
[warning] 11-11: Use from unittest import mock in lieu of alias
Replace with from unittest import mock
(PLR0402)
[warning] 42-47: Mutable default value for class attribute
(RUF012)
tests/test_migrate.py
[warning] 11-11: Use from unittest import mock in lieu of alias
Replace with from unittest import mock
(PLR0402)
[error] 26-26: Probable insecure usage of temporary file or directory: "/tmp/test-state.json"
(S108)
tests/conftest.py
[warning] 11-11: Use from unittest import mock in lieu of alias
Replace with from unittest import mock
(PLR0402)
[warning] 21-21: Missing return type annotation for special method __init__
Add return type annotation: None
(ANN204)
[warning] 28-28: Missing return type annotation for special method __enter__
(ANN204)
[warning] 31-31: Missing return type annotation for special method __exit__
(ANN204)
[warning] 31-31: Missing type annotation for *_
(ANN002)
🪛 Shellcheck (0.11.0)
scripts/setup-dev.sh
[warning] 79-79: WORKER_URL appears unused. Verify use (or export if used externally).
(SC2034)
[warning] 150-150: Tilde does not expand in quotes. Use $HOME.
(SC2088)
🔇 Additional comments (8)
tests/test_migrate.py (2)
80-126: LGTM!The
TestExtractFingerprintFromRawandTestRecordMigratedMessagetest classes provide good coverage for edge cases including empty input and missing headers. The test data is well-structured with proper RFC-style email formatting.
39-74: No action needed—pytest fixtures are defined.The fixtures
mock_urlopen,valid_profile_response, andmismatched_profile_responseare already defined intests/conftest.pyand will be found by pytest. The test class will collect and run without fixture-related errors.ui/src/app/page.tsx (1)
37-90: LGTM!The utility functions (
bytesHuman,mirrorCell,statusBadge) and small components (ProgressBar,Select,TokenBadge) are well-implemented with proper edge case handling.ui/src/__tests__/trigger.test.ts (2)
52-102: LGTM!The boolean input tests correctly verify that
dry_runanddelete_from_sourceare sent as actual booleans to the GitHub Actions API, not stringified values. This aligns with the PR objective to fix boolean workflow inputs.
104-172: LGTM!The token preflight gate tests comprehensively cover the three failure scenarios (worker 404, Gmail 401, email mismatch) and correctly verify that GitHub dispatch is not called when preflight fails. The
toHaveBeenCalledTimesassertions confirm early-exit behavior.tests/test_cleanup.py (3)
40-72: LGTM!The
TestFingerprintMatchesclass provides thorough coverage of the fingerprint matching logic, including edge cases like empty Message-ID fields. The_BASEclass attribute is technically a mutable dict (Ruff RUF012), but since it's only read and never modified, this is a false positive.
78-138: LGTM!The
TestLoadCleanupCandidatesclass correctly tests the intersection-based candidate selection using pytest'stmp_pathandmonkeypatchfixtures. The tests cover key scenarios: intersection logic, empty states, missing files, and single-destination mode.
145-156: LGTM!The
TestTrashMessageclass correctly mockscleanup.gmail_postto test both success and error paths. The mock patching approach aligns with the module structure shown in the context snippets.
| pull_request: | ||
| paths: | ||
| - "scripts/**" | ||
| - "ui/src/**" | ||
| - "tests/**" | ||
| - ".github/workflows/ci.yml" |
There was a problem hiding this comment.
The PR test gate can be skipped by manifest-only changes.
pull_request.paths does not include ui/package.json or ui/package-lock.json, so a PR that only bumps Next/Vitest or updates the lockfile will skip this workflow entirely. That undermines the “block merges on test failure” goal for dependency changes.
🧪 Suggested fix
pull_request:
paths:
- "scripts/**"
+ - "ui/package.json"
+ - "ui/package-lock.json"
- "ui/src/**"
- "tests/**"
- ".github/workflows/ci.yml"📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| pull_request: | |
| paths: | |
| - "scripts/**" | |
| - "ui/src/**" | |
| - "tests/**" | |
| - ".github/workflows/ci.yml" | |
| pull_request: | |
| paths: | |
| - "scripts/**" | |
| - "ui/package.json" | |
| - "ui/package-lock.json" | |
| - "ui/src/**" | |
| - "tests/**" | |
| - ".github/workflows/ci.yml" |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.github/workflows/ci.yml around lines 6 - 11, pull_request.paths currently
only matches "ui/src/**" so changes to UI dependency files can skip the CI;
update the pull_request.paths list to explicitly include the UI package files
(add entries "ui/package.json" and "ui/package-lock.json") or broaden the
pattern to "ui/**" so updates to Next/Vitest or lockfiles trigger the workflow;
modify the pull_request.paths block (the pull_request.paths symbol in the CI
YAML) to include those entries.
| # Verify tokens work and match expected accounts before touching any mail | ||
| src_token = get_source_token() | ||
| dst_token = get_dest_token() | ||
| log("✅ Tokens acquired from CF Worker") | ||
| preflight_check(src_token, SOURCE_USER, "source") | ||
| preflight_check(dst_token, DEST_USER, "dest") | ||
| log("✅ Tokens acquired and verified from CF Worker") |
There was a problem hiding this comment.
Run token validation before persisting status="running".
This block executes after the run has already been saved as running. Any fatal exit from get_source_token(), get_dest_token(), or the new preflight_check() bypasses the except Exception path, so the state file is left stale and the failure notification is skipped.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@scripts/migrate.py` around lines 601 - 606, The token acquisition and
preflight validation (get_source_token, get_dest_token, preflight_check,
src_token/dst_token) must run before the run is persisted as status="running" so
failures don't leave a stale state; move the calls to
get_source_token/get_dest_token and preflight_check earlier (before the code
that writes status="running") or wrap them in a try/except that updates the run
state and sends failure notifications on any exception, ensuring any exception
during token validation always triggers status cleanup/notification rather than
leaving the file stuck as running.
| read_secret() { | ||
| local var="$1" prompt="$2" default="${3:-}" | ||
| local existing | ||
| existing=$(grep -s "^${var}=" "$ENV_FILE" | cut -d= -f2- || true) | ||
| if [[ -n "$existing" ]]; then | ||
| echo " $var [already set, press Enter to keep]: " | ||
| read -r val | ||
| [[ -z "$val" ]] && echo "$val=$existing" && return | ||
| echo "$val" | ||
| else | ||
| read -rp " $prompt [$default]: " val | ||
| val="${val:-$default}" | ||
| echo "$val" | ||
| fi |
There was a problem hiding this comment.
read_secret() corrupts values when .env.local already exists.
Because callers use $(read_secret ...), the echo prompt on Line 92 is captured into the variable instead of just being displayed, and Line 94 returns =existing instead of the raw value. Re-running setup against an existing file can therefore rewrite WORKER_URL/tokens with prompt text or ==....
🐛 Suggested fix
read_secret() {
local var="$1" prompt="$2" default="${3:-}"
local existing
existing=$(grep -s "^${var}=" "$ENV_FILE" | cut -d= -f2- || true)
if [[ -n "$existing" ]]; then
- echo " $var [already set, press Enter to keep]: "
- read -r val
- [[ -z "$val" ]] && echo "$val=$existing" && return
- echo "$val"
+ read -rp " $var [already set, press Enter to keep]: " val
+ [[ -z "$val" ]] && printf '%s\n' "$existing" && return
+ printf '%s\n' "$val"
else
read -rp " $prompt [$default]: " val
val="${val:-$default}"
- echo "$val"
+ printf '%s\n' "$val"
fi
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| read_secret() { | |
| local var="$1" prompt="$2" default="${3:-}" | |
| local existing | |
| existing=$(grep -s "^${var}=" "$ENV_FILE" | cut -d= -f2- || true) | |
| if [[ -n "$existing" ]]; then | |
| echo " $var [already set, press Enter to keep]: " | |
| read -r val | |
| [[ -z "$val" ]] && echo "$val=$existing" && return | |
| echo "$val" | |
| else | |
| read -rp " $prompt [$default]: " val | |
| val="${val:-$default}" | |
| echo "$val" | |
| fi | |
| read_secret() { | |
| local var="$1" prompt="$2" default="${3:-}" | |
| local existing | |
| existing=$(grep -s "^${var}=" "$ENV_FILE" | cut -d= -f2- || true) | |
| if [[ -n "$existing" ]]; then | |
| read -rp " $var [already set, press Enter to keep]: " val | |
| [[ -z "$val" ]] && printf '%s\n' "$existing" && return | |
| printf '%s\n' "$val" | |
| else | |
| read -rp " $prompt [$default]: " val | |
| val="${val:-$default}" | |
| printf '%s\n' "$val" | |
| fi | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@scripts/setup-dev.sh` around lines 87 - 100, The read_secret function is
returning prompt text and malformed values when .env.local already exists
because the prompt echo goes to stdout and the empty-input branch echoes
"$val=$existing" producing "=existing"; update read_secret so the prompt is
written to stderr (e.g., use printf or echo >&2) so it isn't captured by callers
using $(read_secret ...), and change the empty-input branch to output just the
existing value (echo "$existing") instead of "$val=$existing"; reference
function name read_secret and variables ENV_FILE, var, existing, val when making
the fix.
| beforeEach(() => { | ||
| vi.resetModules(); | ||
| // Inject env vars into process.env (Next.js API routes read from here) | ||
| Object.assign(process.env, ENV); | ||
| }); | ||
|
|
||
| afterEach(() => { | ||
| vi.restoreAllMocks(); | ||
| for (const k of Object.keys(ENV)) delete process.env[k]; | ||
| }); |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Environment cleanup may leak between tests if module caching persists.
The vi.resetModules() in beforeEach should clear the module cache, but Object.assign(process.env, ENV) modifies the shared process.env object. If a test fails mid-execution before afterEach runs, subsequent test runs in watch mode could have stale env vars. Consider using vi.stubEnv for safer environment isolation.
♻️ Suggested improvement using vi.stubEnv
beforeEach(() => {
vi.resetModules();
- // Inject env vars into process.env (Next.js API routes read from here)
- Object.assign(process.env, ENV);
+ // Use vi.stubEnv for automatic cleanup
+ for (const [k, v] of Object.entries(ENV)) {
+ vi.stubEnv(k, v);
+ }
});
afterEach(() => {
vi.restoreAllMocks();
- for (const k of Object.keys(ENV)) delete process.env[k];
+ vi.unstubAllEnvs();
});📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| beforeEach(() => { | |
| vi.resetModules(); | |
| // Inject env vars into process.env (Next.js API routes read from here) | |
| Object.assign(process.env, ENV); | |
| }); | |
| afterEach(() => { | |
| vi.restoreAllMocks(); | |
| for (const k of Object.keys(ENV)) delete process.env[k]; | |
| }); | |
| beforeEach(() => { | |
| vi.resetModules(); | |
| // Use vi.stubEnv for automatic cleanup | |
| for (const [k, v] of Object.entries(ENV)) { | |
| vi.stubEnv(k, v); | |
| } | |
| }); | |
| afterEach(() => { | |
| vi.restoreAllMocks(); | |
| vi.unstubAllEnvs(); | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@ui/src/__tests__/trigger.test.ts` around lines 39 - 48, The tests mutate the
shared process.env using Object.assign(process.env, ENV) in beforeEach which can
leak if a test aborts; instead use vi.stubEnv(ENV) to isolate env vars (replace
the Object.assign line in the beforeEach with vi.stubEnv(ENV)) and remove the
manual deletion loop in afterEach (keep vi.restoreAllMocks() and
vi.resetModules() as-is) so Vitest will automatically restore the environment
between tests.
| describe("POST /api/trigger — destination passthrough", () => { | ||
| it("passes destination=dest1 when UI sends dest1", async () => { | ||
| const fetchMock = vi.fn() | ||
| .mockResolvedValueOnce(makeResponse({ access_token: "token" })) | ||
| .mockResolvedValueOnce(makeResponse({ emailAddress: "source@gmail.com" })) | ||
| .mockResolvedValueOnce(new Response(null, { status: 204 })); | ||
|
|
||
| vi.stubGlobal("fetch", fetchMock); | ||
|
|
||
| const { POST } = await import("../app/api/trigger/route"); | ||
| const req = new Request("http://localhost/api/trigger", { | ||
| method: "POST", | ||
| headers: { "Content-Type": "application/json" }, | ||
| body: JSON.stringify({ destination: "dest1" }), | ||
| }); | ||
|
|
||
| await POST(req); | ||
|
|
||
| const body = capturedBody(fetchMock, 2); | ||
| expect((body.inputs as Record<string, unknown>).destination).toBe("dest1"); | ||
| }); |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider adding tests for other destination values.
The destination passthrough test only verifies dest1. For completeness, consider adding test cases for dest2 and "both" to ensure all destination values are correctly passed through.
♻️ Suggested additional test cases
it("passes destination=dest2 when UI sends dest2", async () => {
const fetchMock = vi.fn()
.mockResolvedValueOnce(makeResponse({ access_token: "token" }))
.mockResolvedValueOnce(makeResponse({ emailAddress: "source@gmail.com" }))
.mockResolvedValueOnce(new Response(null, { status: 204 }));
vi.stubGlobal("fetch", fetchMock);
const { POST } = await import("../app/api/trigger/route");
const req = new Request("http://localhost/api/trigger", {
method: "POST",
headers: { "Content-Type": "application/json" },
body: JSON.stringify({ destination: "dest2" }),
});
await POST(req);
const body = capturedBody(fetchMock, 2);
expect((body.inputs as Record<string, unknown>).destination).toBe("dest2");
});
it("passes destination=both when UI sends both", async () => {
// Similar structure...
});🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@ui/src/__tests__/trigger.test.ts` around lines 174 - 194, Add two more tests
mirroring the existing "passes destination=dest1" case to cover the other
allowed values: create tests that call the same POST handler (imported from
"../app/api/trigger/route") with request bodies containing destination: "dest2"
and destination: "both", stub fetch the same way using fetchMock and
makeResponse, invoke await POST(req), then assert the forwarded payload via
capturedBody(fetchMock, 2) has inputs.destination equal to "dest2" and "both"
respectively; keep the test setup, vi.stubGlobal("fetch", fetchMock), and the
same sequence of mocked responses so the tests match the structure around the
existing POST, fetchMock, makeResponse, and capturedBody usage.
| interface FolderRow { | ||
| id: string; name: string; type: string; | ||
| g1Count: number; estimatedBytes: number; | ||
| g2Count: number; g3Count: number; | ||
| } | ||
| interface FoldersData { | ||
| rows: FolderRow[]; | ||
| sourceUser: string | null; | ||
| dest1User: string | null; | ||
| dest2User: string | null; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify the /api/folders response shape
cat ui/src/app/api/folders/route.ts | head -100Repository: aakifshamsi/gmail-migrate
Length of output: 3098
🏁 Script executed:
# Get the fetchFolders call and data usage in page.tsx
rg -n "fetchFolders|g1Count|g2Count|g3Count" ui/src/app/page.tsx | head -30Repository: aakifshamsi/gmail-migrate
Length of output: 854
🏁 Script executed:
# Get lines 162-171 to see the API call
sed -n '162,171p' ui/src/app/page.tsxRepository: aakifshamsi/gmail-migrate
Length of output: 529
🏁 Script executed:
# Get lines 225 context
sed -n '220,230p' ui/src/app/page.tsxRepository: aakifshamsi/gmail-migrate
Length of output: 675
🏁 Script executed:
# Get lines 344-352 context
sed -n '340,355p' ui/src/app/page.tsxRepository: aakifshamsi/gmail-migrate
Length of output: 1139
Critical API/UI mismatch: FolderRow interface doesn't match /api/folders response.
The FolderRow interface expects g1Count, g2Count, g3Count fields, but /api/folders returns only { id, name, type, messagesTotal, estimatedBytes }. Additionally, query parameters source, dest1, dest2 sent on lines 163-164 are not used by the API route.
This causes runtime errors:
- Line 225:
r.g1Count→undefined(reduce over undefined) - Lines 344-345:
mirrorCell(row.g1Count, row.g2Count)→mirrorCell(undefined, undefined) - Line 352:
row.g1Count.toLocaleString()→ crashes (undefined has no toLocaleString)
The API needs to be updated to return the mirror count fields (or the interface/UI logic needs to map messagesTotal to g1Count and handle missing destination counts).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@ui/src/app/page.tsx` around lines 4 - 14, The FolderRow interface and UI
assume g1Count/g2Count/g3Count exist but the /api/folders response only provides
messagesTotal and estimatedBytes; update the UI mapping instead of the API:
change FolderRow to reflect the API (rename or add messagesTotal: number) and,
where the fetch/transform occurs (the code that builds rows from the
/api/folders response), map messagesTotal → g1Count and set g2Count/g3Count to 0
(or null-coalesce to 0) so mirrorCell(row.g1Count, row.g2Count) always receives
numbers and row.g1Count.toLocaleString() is safe; also update any usages of
FolderRow (e.g., FoldersData, mirrorCell calls and display code) to use the new
mapping/guarding.
| const doJobAction = async (runId: number, action: "cancel" | "rerun") => { | ||
| setJobAction(runId); | ||
| try { | ||
| await fetch("/api/jobs", { method: "POST", headers: { "Content-Type": "application/json" }, | ||
| body: JSON.stringify({ runId, action }) }); | ||
| setTimeout(fetchJobs, 2_000); | ||
| } finally { setJobAction(null); } | ||
| }; |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Missing error handling for job cancel/rerun actions.
The doJobAction function doesn't handle errors or provide feedback when the POST request fails. Users won't know if their cancel/rerun action succeeded or failed.
♻️ Proposed fix
const doJobAction = async (runId: number, action: "cancel" | "rerun") => {
setJobAction(runId);
try {
- await fetch("/api/jobs", { method: "POST", headers: { "Content-Type": "application/json" },
- body: JSON.stringify({ runId, action }) });
+ const res = await fetch("/api/jobs", { method: "POST", headers: { "Content-Type": "application/json" },
+ body: JSON.stringify({ runId, action }) });
+ if (!res.ok) {
+ const err = await res.json().catch(() => ({}));
+ setResult({ ok: false, msg: err.error || `Failed to ${action} job` });
+ }
setTimeout(fetchJobs, 2_000);
- } finally { setJobAction(null); }
+ } catch (e) {
+ setResult({ ok: false, msg: e instanceof Error ? e.message : String(e) });
+ } finally {
+ setJobAction(null);
+ }
};🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@ui/src/app/page.tsx` around lines 191 - 198, The doJobAction function needs
proper error handling and user feedback: wrap the fetch call in a try/catch
(keeping the existing finally that calls setJobAction(null)), check the fetch
response.ok and throw on non-2xx so failures are caught, and surface errors to
the UI (e.g., call an existing notification/toast helper or set an error state)
so users know if cancel/rerun failed; update references to runId, action,
setJobAction and fetchJobs in this change so the action still triggers fetchJobs
on success (or after failure if appropriate) and ensure errors are logged for
debugging.
| // Poll status while any run is active | ||
| useEffect(() => { | ||
| const running = status?.dest1?.status === "running" || status?.dest2?.status === "running"; | ||
| if (running && !pollRef.current) pollRef.current = setInterval(fetchStatus, 20_000); | ||
| if (!running && pollRef.current) { clearInterval(pollRef.current); pollRef.current = null; } | ||
| return () => { if (pollRef.current) clearInterval(pollRef.current); }; | ||
| }, [status, fetchStatus]); | ||
|
|
||
| // Poll jobs while any job is queued/in_progress | ||
| useEffect(() => { | ||
| const active = jobs.some(j => j.status === "queued" || j.status === "in_progress"); | ||
| if (active && !jobPollRef.current) jobPollRef.current = setInterval(() => { fetchJobs(); fetchStatus(); }, 15_000); | ||
| if (!active && jobPollRef.current) { clearInterval(jobPollRef.current); jobPollRef.current = null; } | ||
| return () => { if (jobPollRef.current) clearInterval(jobPollRef.current); }; | ||
| }, [jobs, fetchJobs, fetchStatus]); |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Potential double-fetching of status when both polling intervals are active.
When a migration is running (status?.dest1?.status === "running") AND a job is active (queued/in_progress), both polling intervals will be active. The status interval (line 206) calls fetchStatus every 20s, while the job interval (line 214) calls both fetchJobs() and fetchStatus() every 15s. This results in redundant status fetches.
♻️ Suggested optimization
Consider consolidating polling or checking if status polling is already active:
useEffect(() => {
const active = jobs.some(j => j.status === "queued" || j.status === "in_progress");
- if (active && !jobPollRef.current) jobPollRef.current = setInterval(() => { fetchJobs(); fetchStatus(); }, 15_000);
+ if (active && !jobPollRef.current) jobPollRef.current = setInterval(() => {
+ fetchJobs();
+ if (!pollRef.current) fetchStatus(); // Only fetch status if not already polling
+ }, 15_000);
if (!active && jobPollRef.current) { clearInterval(jobPollRef.current); jobPollRef.current = null; }
return () => { if (jobPollRef.current) clearInterval(jobPollRef.current); };
}, [jobs, fetchJobs, fetchStatus]);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // Poll status while any run is active | |
| useEffect(() => { | |
| const running = status?.dest1?.status === "running" || status?.dest2?.status === "running"; | |
| if (running && !pollRef.current) pollRef.current = setInterval(fetchStatus, 20_000); | |
| if (!running && pollRef.current) { clearInterval(pollRef.current); pollRef.current = null; } | |
| return () => { if (pollRef.current) clearInterval(pollRef.current); }; | |
| }, [status, fetchStatus]); | |
| // Poll jobs while any job is queued/in_progress | |
| useEffect(() => { | |
| const active = jobs.some(j => j.status === "queued" || j.status === "in_progress"); | |
| if (active && !jobPollRef.current) jobPollRef.current = setInterval(() => { fetchJobs(); fetchStatus(); }, 15_000); | |
| if (!active && jobPollRef.current) { clearInterval(jobPollRef.current); jobPollRef.current = null; } | |
| return () => { if (jobPollRef.current) clearInterval(jobPollRef.current); }; | |
| }, [jobs, fetchJobs, fetchStatus]); | |
| // Poll status while any run is active | |
| useEffect(() => { | |
| const running = status?.dest1?.status === "running" || status?.dest2?.status === "running"; | |
| if (running && !pollRef.current) pollRef.current = setInterval(fetchStatus, 20_000); | |
| if (!running && pollRef.current) { clearInterval(pollRef.current); pollRef.current = null; } | |
| return () => { if (pollRef.current) clearInterval(pollRef.current); }; | |
| }, [status, fetchStatus]); | |
| // Poll jobs while any job is queued/in_progress | |
| useEffect(() => { | |
| const active = jobs.some(j => j.status === "queued" || j.status === "in_progress"); | |
| if (active && !jobPollRef.current) jobPollRef.current = setInterval(() => { | |
| fetchJobs(); | |
| if (!pollRef.current) fetchStatus(); // Only fetch status if not already polling | |
| }, 15_000); | |
| if (!active && jobPollRef.current) { clearInterval(jobPollRef.current); jobPollRef.current = null; } | |
| return () => { if (jobPollRef.current) clearInterval(jobPollRef.current); }; | |
| }, [jobs, fetchJobs, fetchStatus]); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@ui/src/app/page.tsx` around lines 203 - 217, Both polling useEffect blocks
can trigger fetchStatus concurrently (status poll using pollRef and job poll
using jobPollRef), causing redundant network calls; modify the job-polling logic
in the second useEffect (the one referencing jobs, jobPollRef, fetchJobs,
fetchStatus) so it does not call fetchStatus if the status poll is already
active (i.e., if pollRef.current is truthy), or alternatively consolidate into a
single shared interval that runs fetchJobs and fetchStatus at the desired
cadence; update the condition that sets jobPollRef and the interval callback
accordingly to only call fetchJobs when pollRef.current exists, and ensure
cleanup still clears jobPollRef as before.
| // Derive destination from which account slots are populated | ||
| const destination = d1Acc && d2Acc ? "both" : d1Acc ? "dest1" : "dest2"; | ||
|
|
||
| const srcTokenValid = tokenStatuses[srcAcc]?.valid !== false; // true if valid or unchecked | ||
| const srcTokenInvalid = tokenStatuses[srcAcc]?.valid === false; | ||
|
|
||
| const runMigration = async (dry: boolean) => { | ||
| if (!d1Acc && !d2Acc) { setResult({ ok: false, msg: "Select at least one destination account." }); return; } |
There was a problem hiding this comment.
Destination defaults to "dest2" when no destinations are selected.
The ternary on line 229 evaluates to "dest2" when both d1Acc and d2Acc are empty. While line 235 blocks dispatch when no destinations are set, the destination variable is still "dest2" which could cause confusion if used elsewhere or logged.
🛡️ Proposed fix
- const destination = d1Acc && d2Acc ? "both" : d1Acc ? "dest1" : "dest2";
+ const destination = d1Acc && d2Acc ? "both" : d1Acc ? "dest1" : d2Acc ? "dest2" : "";📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // Derive destination from which account slots are populated | |
| const destination = d1Acc && d2Acc ? "both" : d1Acc ? "dest1" : "dest2"; | |
| const srcTokenValid = tokenStatuses[srcAcc]?.valid !== false; // true if valid or unchecked | |
| const srcTokenInvalid = tokenStatuses[srcAcc]?.valid === false; | |
| const runMigration = async (dry: boolean) => { | |
| if (!d1Acc && !d2Acc) { setResult({ ok: false, msg: "Select at least one destination account." }); return; } | |
| // Derive destination from which account slots are populated | |
| const destination = d1Acc && d2Acc ? "both" : d1Acc ? "dest1" : d2Acc ? "dest2" : ""; | |
| const srcTokenValid = tokenStatuses[srcAcc]?.valid !== false; // true if valid or unchecked | |
| const srcTokenInvalid = tokenStatuses[srcAcc]?.valid === false; | |
| const runMigration = async (dry: boolean) => { | |
| if (!d1Acc && !d2Acc) { setResult({ ok: false, msg: "Select at least one destination account." }); return; } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@ui/src/app/page.tsx` around lines 228 - 235, The computed destination
variable incorrectly defaults to "dest2" when neither destination is selected;
update the expression that defines destination (currently using d1Acc and d2Acc)
to explicitly handle the "neither selected" case—e.g. change the ternary to
return undefined (or an empty string) when both d1Acc and d2Acc are falsy: const
destination = d1Acc && d2Acc ? "both" : d1Acc ? "dest1" : d2Acc ? "dest2" :
undefined; also ensure any downstream use in runMigration (and related checks
around srcTokenValid/srcTokenInvalid) safely handles the new undefined value or
adjust types accordingly.
| {[ | ||
| { label: "Source (G1)", value: srcAcc, onChange: setSrcAcc, exclude: [d1Acc, d2Acc] }, | ||
| { label: "Dest 1 (G2)", value: d1Acc, onChange: setD1Acc, exclude: [srcAcc, d2Acc], optional: true }, | ||
| { label: "Dest 2 (G3)", value: d2Acc, onChange: setD2Acc, exclude: [srcAcc, d1Acc], optional: true }, | ||
| ].map(({ label, value, onChange, exclude, optional }) => ( | ||
| <div key={label}> | ||
| <div className="flex items-center justify-between mb-1"> | ||
| <p className="text-xs text-gray-600">{label}</p> | ||
| <TokenBadge | ||
| status={value ? tokenStatuses[value] : undefined} | ||
| loading={value ? checkingToken[value] : false} | ||
| /> | ||
| </div> | ||
| <Select value={value} onChange={onChange} | ||
| options={optional ? ["", ...otherAccounts(exclude)] : accounts} | ||
| placeholder={optional ? "— none —" : "— select —"} /> | ||
| </div> | ||
| ))} |
There was a problem hiding this comment.
Source account selector doesn't filter out destination accounts.
Line 303 uses accounts (unfiltered) for the source selector, but otherAccounts(exclude) for destinations. This allows selecting the same account as both source and destination, which would cause the migration to copy emails to the same account.
🛡️ Proposed fix
{label: "Source (G1)", value: srcAcc, onChange: setSrcAcc, exclude: [d1Acc, d2Acc] },Then update line 303:
- options={optional ? ["", ...otherAccounts(exclude)] : accounts}
+ options={optional ? ["", ...otherAccounts(exclude)] : otherAccounts(exclude)}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| {[ | |
| { label: "Source (G1)", value: srcAcc, onChange: setSrcAcc, exclude: [d1Acc, d2Acc] }, | |
| { label: "Dest 1 (G2)", value: d1Acc, onChange: setD1Acc, exclude: [srcAcc, d2Acc], optional: true }, | |
| { label: "Dest 2 (G3)", value: d2Acc, onChange: setD2Acc, exclude: [srcAcc, d1Acc], optional: true }, | |
| ].map(({ label, value, onChange, exclude, optional }) => ( | |
| <div key={label}> | |
| <div className="flex items-center justify-between mb-1"> | |
| <p className="text-xs text-gray-600">{label}</p> | |
| <TokenBadge | |
| status={value ? tokenStatuses[value] : undefined} | |
| loading={value ? checkingToken[value] : false} | |
| /> | |
| </div> | |
| <Select value={value} onChange={onChange} | |
| options={optional ? ["", ...otherAccounts(exclude)] : accounts} | |
| placeholder={optional ? "— none —" : "— select —"} /> | |
| </div> | |
| ))} | |
| {[ | |
| { label: "Source (G1)", value: srcAcc, onChange: setSrcAcc, exclude: [d1Acc, d2Acc] }, | |
| { label: "Dest 1 (G2)", value: d1Acc, onChange: setD1Acc, exclude: [srcAcc, d2Acc], optional: true }, | |
| { label: "Dest 2 (G3)", value: d2Acc, onChange: setD2Acc, exclude: [srcAcc, d1Acc], optional: true }, | |
| ].map(({ label, value, onChange, exclude, optional }) => ( | |
| <div key={label}> | |
| <div className="flex items-center justify-between mb-1"> | |
| <p className="text-xs text-gray-600">{label}</p> | |
| <TokenBadge | |
| status={value ? tokenStatuses[value] : undefined} | |
| loading={value ? checkingToken[value] : false} | |
| /> | |
| </div> | |
| <Select value={value} onChange={onChange} | |
| options={optional ? ["", ...otherAccounts(exclude)] : otherAccounts(exclude)} | |
| placeholder={optional ? "— none —" : "— select —"} /> | |
| </div> | |
| ))} |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@ui/src/app/page.tsx` around lines 289 - 306, The source selector is currently
using the full accounts list (accounts) so it can pick an account that is also
chosen as a destination; update the options prop for the source entry in the
mapped array to use otherAccounts(exclude) like the destination selectors do.
Locate the array items where label "Source (G1)" and the mapped render use
value={srcAcc} and onChange={setSrcAcc} and change its Select options from
accounts to otherAccounts(exclude) (keep placeholder "— select —"); no other
behavior changes needed.
2 participants
Sprint v0.5 — Safe Usable MVP
Closes the four issues that made v0.4 "no usable product":
P0 — Safety gates (4 fixes in one commit)
next 15.3.0 → 15.3.8— Vercel build unblockedmigrate.py:preflight_check()— exits FATAL before touching any mail if token is 404/revoked/email-mismatchroute.ts: dispatch now blocked with HTTP 400 if source token preflight fails — "✅ Workflow dispatched!" with a 404 token is no longer possibleroute.ts:dry_run/delete_from_source/skip_dedupsent as actual booleans, not"true"/"false"stringsP1 — Trust & accuracy
page.tsx: Restore full dashboard (accounts panel, folder mirror table, per-destination status, jobs panel with cancel/re-run) — previously lost in merge conflict resolutiondestinationnow auto-derived from account dropdowns (d1+d2 → "both",d1 only → "dest1",d2 only → "dest2") — removes the mismatch where triggered job always gotdestination=bothregardless of which accounts were selected/api/token-statusendpoint on load); dispatch buttons disabled when source token is invalid; inline re-auth link shown/api/token-status(new): per-account token health — CF Worker vault + Gmail/profilecheck (issue TASK: Dashboard — show real-time OAuth token status per account (valid/expired/revoked) #13)/api/status: already returnsdest1/dest2separately — no change needed; per-destination status is now surfaced in the UIP2 — Test coverage + CI gate
preflight_check: 401, email mismatch, case-insensitive, network errorfingerprint_matches: exact, field mismatches, empty message-idload_cleanup_candidates: intersection, empty manifest, missing file, single desttrash_message: success and RuntimeErrorextract_fingerprint_from_raw,record_migrated_message/api/trigger: boolean types, preflight gate (404/401/mismatch all block dispatch), destination passthrough.github/workflows/ci.yml: pytest + vitest on push to main and PRs — blocks merge on test failureDX — Reduce iteration cost
.devcontainer/devcontainer.json: GitHub Codespaces support — Python 3.12 + Node 20 + KiloCode extension, forwards port 3000. Enables full dev workflow from Firefox Android viagithub.devscripts/setup-dev.sh: bootstrap script — writesui/.env.local, configures KiloCode via OpenRouter (claude-sonnet-4-5), readiness summaryDefinition of Done ✅
migrate.pyexits FATAL before touching any mail if token revokeddestinationworkflow parampytest tests/ -v— 22 passed, 0 failed, 0 real credentialsTo merge
https://claude.ai/code/session_01QYvJ82EXS3meeGykiutSrJ