Codex/windows build GitHub

[thememuse]

No description provided.

[crisng95]

Deep Review

199 files, ~57k LOC, 15 commits — bundles 4 largely independent initiatives (CI workflows, Electron desktop app, Cloudflare license worker, backend rate-limit/upscale overhaul). Strongly recommend splitting into separate PRs for follow-ups; this single review covers them together.

What this PR does

Theme	Highlights
CI	.github/workflows/build-windows.yml, build-macos.yml — sparse checkout, PyInstaller agent bundle, auto icon generation, non-blocking release upload
Desktop (Electron)	desktop/** (~100 files): main/preload/sidecar, license enforcement, machine fingerprinting, React UI on shadcn, ffmpeg + Real-ESRGAN runtime staging
Cloudflare license	cloudflare-license/**: D1 schema (devices/licenses/audit_logs), POST /v1/device/check, admin dashboard, HMAC sessions, plan model (TRIAL_3D/1M/3M/6M/1Y/LIFE)
Backend overhaul	CAPTCHA-aware multi-tier rate limiter, exponential backoff, operation reconciliation (_try_reconcile_operation_success), local 4K upscaler (local_upscaler.py, 721 LOC), ElevenLabs TTS, signed-URL caching
Extension	background.js: 154→1108 LOC — URL cache, multi-tab project tracking, signed-URL extraction
Filename hygiene	35 skills/fk:*.md + 22 .claude/commands/gla:*.md removed (Windows : issue). Matches main which already renamed to fk-* — no functional loss but expect merge conflicts on the rename pairs

---

🔴 Blockers

1. Remote Flow upscale is silently retired, with retroactive DB rewriting

agent/worker/processor.py does not just add a new local path — it forces all existing and future upscale requests through the local pipeline:

Location	Behavior
submit_upscale_request()	New rows written as UPSCALE_VIDEO_LOCAL (was UPSCALE_VIDEO)
Input normalization (×2)	if data["req_type"] == "UPSCALE_VIDEO": data["req_type"] = "UPSCALE_VIDEO_LOCAL"
Startup migration	UPDATE requests SET type='UPSCALE_VIDEO_LOCAL' WHERE type='UPSCALE_VIDEO' — mutates user's historical data
_dispatch()	Both types route to upscale_scene_video_local()

Concerns:

• Undisclosed product change — nothing in PR title/description mentions remote upscale removal.
• No fallback — users without ffmpeg + Real-ESRGAN binaries (CLI users, devs without the desktop bundle, anyone running the agent outside the Electron shell) will see every upscale fail with no actionable error.
• Irreversible startup migration rewrites the user's request history. A user who pulls this build, hits a problem, and rolls back will find their old UPSCALE_VIDEO rows are now UPSCALE_VIDEO_LOCAL — non-recoverable without a separate down-migration.

Quality concern — the fast engine is not real super-resolution.
upscale_scene_video_local() exposes two engines: fast (ffmpeg -vf scale=...:flags=lanczos) and ai (Real-ESRGAN per-frame). These are not interchangeable:

Engine	Mechanism	Adds detail?	Output reality
fast (ffmpeg lanczos)	Interpolation — averages neighboring pixels with weighted kernel	❌ No	4K resolution, 1080p information density. File is 4× larger; perceived sharpness ≈ source.
ai (Real-ESRGAN)	Neural network trained on (low-res, high-res) pairs hallucinates plausible detail	✅ Yes (synthesized)	True super-resolution — adds hair strands, edge sharpness, texture. Can produce artifacts on faces.
Remote (Google Veo upscaler, the path being removed)	Google's video-aware super-resolution model	✅ Yes	Highest quality of the three; understands video context, not just per-frame.

Calling lanczos "upscale" is a marketing-grade abuse of the term — it's a resize, not a super-resolution. After this PR, a user invoking the long-standing UPSCALE_VIDEO request type will, in the best case, get a Real-ESRGAN run if they have the binaries; in the worst case, a lanczos resize that produces a 4K-pixel-count file with no actual detail gain — losing the Veo upscaler's quality. Neither is what the existing UPSCALE_VIDEO contract promised.

Requested:

1. Restore UPSCALE_VIDEO → ops.upscale_scene_video() (remote Veo upscaler) as the default behavior.
2. Make UPSCALE_VIDEO_LOCAL an explicit opt-in selected by the caller (CLI flag / API param), not a silent rewrite.
3. Within UPSCALE_VIDEO_LOCAL, default the engine to ai (Real-ESRGAN) and treat fast as an explicit "I just need 4K resolution metadata for YouTube spec" override — document this clearly in the engine selection so users aren't surprised by lanczos output.
4. If UPSCALE_VIDEO_LOCAL is selected and runtime binaries are missing, fail loudly with a clear "Run prepare-upscale-runtime first" message — not a generic ffmpeg error.
5. Drop the implicit req_type rewrite at submit/dispatch and the startup UPDATE migration. If a migration is desired for desktop-bundle users, gate it behind a config flag (e.g. FLOWKIT_PREFER_LOCAL_UPSCALE=1).
6. Document the new behavior in the PR description and add release notes.

2. Hardcoded Cloudflare account/database IDs in wrangler.toml

# cloudflare-license/wrangler.toml:5
account_id = "cf907123cee7dcfd4737e29c64a9b356"
# :9
database_id = "70c14ac4-7a18-44ee-b9d1-bdead2f3090b"

The accompanying docs (docs/license-cloudflare.md) correctly tell the operator to set their own values, but the committed file ships the contributor's. Anyone running wrangler deploy from a fresh clone will deploy against the wrong account or hit permission errors. Not a secret, but leaks infra identity and creates merge churn for every operator.

Requested: replace both with placeholders (<YOUR_CLOUDFLARE_ACCOUNT_ID> / <YOUR_DATABASE_ID>) consistent with the docs, or move to .dev.vars and gitignore.

3. Admin login uses non-constant-time string comparison

cloudflare-license/src/index.ts handleAdminLogin:

if (username !== expectedUsername || password !== expectedPassword) {
  return json(req, env, { error: 'INVALID_CREDENTIALS' }, 401)
}

The file already defines timingSafeEqual() and uses it correctly when verifying session-cookie HMACs. The login path — exposed publicly via Cloudflare Worker — is the one place it should matter most, and it's the one place it's missing.

Requested:

const userOk = timingSafeEqual(username, expectedUsername)
const passOk = timingSafeEqual(password, expectedPassword)
if (!userOk || !passOk) { ... }

(Compute both unconditionally to avoid short-circuit timing leaks too.)

4. electron-builder.yml publish target points to the wrong repo

# desktop/electron-builder.yml:49-51
publish:
  provider: github
  owner: thememuse
  repo: flowkit

After tagging a release, auto-update will fetch from the contributor's fork instead of crisng95/flowkit. Must fix before any release tag is pushed.

---

🟡 Significant warnings

• POLL_INTERVAL 5s → 1s (agent/config.py): 5× more DB hits on the worker loop. Increases SQLite contention on users with many projects. No benchmark provided.
• API_COOLDOWN 10s → 1s: new per-type cooldowns compensate for media calls, but metadata/status calls now fire 10× more often against Google. Risk of triggering tighter rate limits on accounts with sustained activity.
• Sidecar may kill non-FlowKit processes (desktop/electron/sidecar.ts::_replaceIncompatibleProcess): matches command line contains "agent.main". A user running an unrelated python -m agent.main script on port 8100 will be killed silently. Add a stronger marker (PID file, unique env-var stamp).
• License fallbackSeed invalidates on hostname/MAC change (desktop/electron/license.ts): renaming the machine, OS upgrade, or swapping a USB Ethernet adapter all invalidate the license. Persist the first-generated machine ID to app.getPath('userData')/machine-id and prefer it on subsequent boots.
• _mark_scene_failed defensively awaits sync mocks (agent/worker/processor.py): if asyncio.iscoroutine(maybe_scene): scene = await maybe_scene only exists to accommodate MagicMock. Masks real bugs where a sync function is accidentally called and returns None. Drop the check; fix tests with AsyncMock.
• Inline ~600 LOC HTML/CSS/JS in cloudflare-license/src/index.ts::renderAdminHtml: hard to test, hard to localize, no CSP. Move to Cloudflare Pages or static-asset binding.

---

🔵 Nits

• Lockfile additions (desktop/package-lock.json ~9k, cloudflare-license/package-lock.json ~1.5k) inflate review surface — call out in PR body so reviewers can skip.
• Mixed Vietnamese / English in user-facing error strings (local_upscaler.py, parts of cloudflare-license/src/index.ts). Pick one or thread through i18n.
• Several commits are ci: fix workflow yaml follow-ups — squash before merge.
• MAX_CONCURRENT_REQUESTS = 4 contradicts the operational note "Google Flow handles max 5 at once" used elsewhere in this project. If 4 is correct, update docs; if 5 is correct, revert.

---

What's good (worth keeping as-is)

• CAPTCHA pipeline — group-pause, per-type cooldowns, exponential backoff, and the _is_flow_tab_unavailable_error / _is_unusual_traffic_error classifiers are a real improvement over the previous "retry 10 times" loop.
• _try_reconcile_operation_success() — re-checks Flow operation status before marking FAILED. Recovers transient errors that previously lost work. Genuine correctness win.
• _mark_scene_failed no longer downgrades COMPLETED rows — closes a real race condition.
• Sidecar adoption logic — adopting an existing healthy agent rather than spawning a duplicate is the right design.
• License worker structure — audit log, session HMAC, offline cache, plan model are cleanly separated.
• local_upscaler.py ai engine — Real-ESRGAN integration is well-engineered (frame extract → upscale → audio re-mux). The dispatch_timeout_sec knob is sensible. (Just don't make it the only path — see Blocker 1.)

---

Recommendation

Request changes on the 4 blockers above. After those land, this PR is mergeable, but I strongly suggest splitting the remaining work into:

1. CI workflows (low risk, can ship independently)
2. Electron desktop app
3. Cloudflare license worker
4. Backend rate-limit + reconciliation overhaul

57k LOC in one review is unfair to reviewers and makes regressions hard to bisect later. The upscale change in particular deserves its own dedicated discussion thread.

[crisng95]

@thememuse — quick housekeeping request before this PR moves forward.

This PR is opened by @thememuse but every one of the 17 commits is authored under a different GitHub identity (netbaseagency). For attribution, audit, and CODEOWNERS purposes I'd like contributions on this repo to come from a single, consistent GitHub account.

Please pick one identity (whichever is your primary) and:

1. Set your local git config to that identity's email:

   git config user.name "<your-display-name>"
   git config user.email "<email-tied-to-the-chosen-github-account>"

   Use the verified email from https://github.com/settings/emails (or a noreply address like <id>+<username>@users.noreply.github.com if you prefer to keep your real email private).

2. Rewrite the existing commits on this branch so the author matches:

   git rebase -i $(git merge-base HEAD origin/main) --exec \
     'git commit --amend --no-edit --reset-author'
   git push --force-with-lease

   (Or use git filter-repo --mailmap if you prefer — either works.)

3. Confirm the push by replying here so I can re-review.

After that, I'll continue with the technical review (the request-changes comment above still stands — please address those points in the same push if convenient).

Note: I'm not asking which account you use — thememuse and netbaseagency are both fine. Just consistent within this repo. Thanks.

[crisng95]

Follow-up — status check after 9 new commits

I reviewed the 9 commits pushed since my request-changes review. Status of the items raised:

Item	Status	Evidence
C1 — Remote UPSCALE_VIDEO silently rewritten to local	❌ Not addressed	agent/worker/processor.py still rewrites req_type at submit/dispatch; startup UPDATE requests SET type='UPSCALE_VIDEO_LOCAL' migration still present; submit_upscale_request() still writes UPSCALE_VIDEO_LOCAL
C2 — Hardcoded Cloudflare account_id / database_id	❌ Not addressed	cloudflare-license/wrangler.toml still contains account_id = "cf907123..." and database_id = "70c14ac4-..."
C3 — Timing-attack-vulnerable admin login	❌ Not addressed	handleAdminLogin still uses username !== expectedUsername || password !== expectedPassword. The file already defines timingSafeEqual() and uses it for HMAC verification — please use it here too.
W1 — electron-builder.yml publish target	✅ Fixed	publish: block removed entirely. Thanks.
Identity consolidation — single GitHub account	❌ Not addressed	All 9 new commits are still authored as admin@netbase.asia (netbaseagency) while the PR is opened by @thememuse.

Concern: scope is growing, not shrinking

The 9 new commits add new functionality rather than addressing review feedback:

• DeepSeek integration (30d2174)
• New skills + workspace sync (30d2174, 8d0c07d)
• Extension runtime watchdog, hash routing, WS port auto-discovery, etc. (Windows-specific hardening — appreciated, but separate scope)

The PR was already 199 files / ~57k LOC. It is now larger. Please do not add new features in this PR. Either:

1. Address the 3 remaining blockers (C1, C2, C3) + identity in the next push, with no new functionality, or
2. Close this PR and open separate, smaller PRs — one per logical theme (CI workflows / Electron desktop / Cloudflare license / backend overhaul / Windows hardening / DeepSeek integration). I will review each on its own merits and they will land much faster.

Reviewing 57k+ LOC of mixed concerns in one pass is not sustainable for either of us, and the upscale change in particular needs its own dedicated discussion thread, not a footnote inside a Windows-build PR.

Holding CHANGES_REQUESTED. Please respond with which path (1 or 2) you intend to take so we can move forward.