Skip to content

feat: remix tier gating — t1/t2 behind explicit opt-in (default unchanged)#20

Open
mtmtian wants to merge 13 commits into
oratis:mainfrom
mtmtian:feat/remix-tiers
Open

feat: remix tier gating — t1/t2 behind explicit opt-in (default unchanged)#20
mtmtian wants to merge 13 commits into
oratis:mainfrom
mtmtian:feat/remix-tiers

Conversation

@mtmtian

@mtmtian mtmtian commented Jul 16, 2026

Copy link
Copy Markdown
Contributor

This PR does NOT change the shipped default: REMIX_ENABLED_TIERS defaults to t0_5 only — the borrow-structure-not-pixels policy in remix-brief.ts / competitor-media.ts remains the effective behavior out of the box.

What this adds

Tier codes for t1/t2 exist in the remix worker engine's control plane, but stay behind an explicit env opt-in. Nothing about the default request path changes.

Tier What it does Competitor pixels in output?
t0 Your existing single-shot remix path (/api/creatives/remix) untouched by this PR
t0.5 (#19) Pure text-derived storyboard, no visual reference zero
t1 Generates using the already-stored Tier-2 competitor video as an @reference for the worker zero — reference only, no competitor frames in the output
t2 Reuses specific, human-approved clean segments from the stored competitor video (segmentPlan) yes — output is tagged containsCompetitorFootage: true

Gating mechanics

  • REMIX_ENABLED_TIERS (comma-separated, default "t0_5") gates which tiers POST /api/creatives/remix-jobs will accept. Tier not in the set → 403. Tier not a known code → 400.
  • t2 requires a validated segmentPlan with at least one action: "reuse" segment, or 400.
  • t1/t2 both require the competitor's video to already be stored via your existing Tier-2 "Save video" flow (CompetitorCreative.assetIdAsset.fileUrl) — this reuses your hand-picked, legal-cleared winners gate rather than adding a new one. Missing it → 400 with a "use the Tier-2 Save video flow first" message.
  • POST /api/worker/remix-jobs/claim now returns refs: [{url, kind:'video'}] for t1/t2 jobs, pointing the worker at that stored Asset. t0_5 is untouched.
  • Every tier still creates the Creative with reviewStatus: 'pending' — the human review gate is identical across tiers, this PR only changes what's allowed to be generated, never what's allowed to ship without review.

Why gate via env instead of code-only

This moves the t1/t2 IP boundary from "not possible in code" to "off by default, explicit config choice + auditable via REMIX_ENABLED_TIERS, gated behind your own Tier-2 hand-picked-video approval step." That gives you (or legal/product) a deliberate lever to pull per deployment, rather than a hardcoded wall — while leaving the out-of-the-box behavior identical to today.

Stacked on #19

This branch is based on feat/remix-worker (#19, still pending review) and includes its commits. Once #19 merges, this diff will automatically shrink to just the tier-gating commit (51c1ce8).

Test plan

  • npm run lint — 0 errors (pre-existing warnings only)
  • npx tsc --noEmit — clean
  • npx playwright test e2e/remix-jobs.spec.ts — 14/14 passing, including new coverage for: unknown-tier 400, t2-without-segmentPlan 400, t2/t1-without-stored-video 400. (The e2e webServer runs with REMIX_ENABLED_TIERS="t0_5,t1,t2" so these 400 branches — which sit downstream of the 403 gate — are exercised; the refs[] happy path requires a stored competitor Asset and is left to cross-repo integration with the worker.)

🤖 Generated with Claude Code

mtmtian added 3 commits July 15, 2026 16:47
Add an independent worker-engine RemixJob path alongside the existing
Seedance2-direct remix: control plane creates a pending RemixJob + brief via
POST /api/creatives/remix-jobs, an external HMAC-authenticated worker claims
it, generates beats, assembles/QCs the clip, and reports progress/results
back via /api/worker/remix-jobs/{claim,report,upload}. Purely additive —
reuses buildRemixBrief/competitorCreativeToAnalysis from remix-brief.ts
without touching any existing files.
…n upload

Close the code-review gaps on the worker-facing RemixJob surface: report/
now enforces an atomic ALLOWED_PREV transition table (succeeded/failed are
terminal), validates outputUrl against the canonical GCS upload path, and
surfaces brand-QC failures via Creative.reviewNotes instead of silently
promoting. upload/ pre-checks HMAC headers and content-length before
buffering the body. claim/ recycles stale in-flight jobs after a configurable
lease window and collapses the duplicated jobId/queue branches into one
atomic updateMany. De-dupes asJson + canvas-dims math into
src/lib/growth/remix-job.ts, and shares the e2e HMAC-signing helper between
remix-jobs.spec.ts and competitor-ingest.spec.ts.
@oratis

oratis commented Jul 20, 2026

Copy link
Copy Markdown
Owner

Review — remix tier gating

Everything in the #19 review applies here too, since this branch contains those commits — in particular the /upload state-machine bypass and the non-transactional report writes. This comment covers only what's specific to the tier gating.

First, the good news, because it's the part the PR description leads with and it checks out. parseEnabledTiers() is genuinely fail-closed: unset, empty string, whitespace-only, and malformed CSV (",,,") all collapse to {t0_5}. Tier is computed entirely from server state, validated against KNOWN_TIERS before the enabled-set check, and there's no path for a client to smuggle or escalate one — report and upload never write tier, and the Prisma @default("t0_5") backstops it. With the env var unset, the 403 fires before any DB row is touched. The shipped default does not weaken the current guarantee. The org-scoping fix in bbf02d4 is also complete — claim/route.ts:100-105 correctly scopes both the CompetitorCreative and Asset lookups by job.orgId.

The problem is with the argument built on top of that, which is roughly "this is safe because it's opt-in." Two findings undercut it.

🔴 containsCompetitorFootage is a write-only field

It's set at creatives/remix-jobs/route.ts:173-174 and read nowhere — a repo-wide grep for containsCompetitorFootage and competitorReferenced returns exactly those two write sites and nothing else. It lands in Creative.tags, but creatives/review/page.tsx:31-43 passes an explicit field whitelist to <ReviewClient> that omits tags entirely. And source is hardcoded to 'remix' for every tier at :164.

So in the review queue, a pure structure-derived t0.5 remix and a t2 creative assembled from spliced competitor footage render identically. The human review gate is the backstop this design leans on once t1/t2 is enabled, and as written it has no way to see the one fact that matters.

🔴 REMIX_ENABLED_TIERS is a create-time allowlist, not a kill switch

parseEnabledTiers() is called only at creatives/remix-jobs/route.ts:82. claim/route.ts:97 gates refs[] purely on the already-persisted job.tier and never re-checks the current env.

Concretely: if a competitor asset turns out to be a problem and the env var is reverted, every t2 job already sitting in pending/claimed/running still gets claimed, still receives refs[].url pointing at the stored competitor video, and still gets spliced. The only lever the design offers can't stop work already in flight.

🟡 Supporting gaps

  • No logAudit() on t1/t2 job creation. Opting a job into "use real competitor footage" is exactly the class of action AuditAction.competitor.video_store exists for. There's currently no record of who created a t2 job, for which org, against which asset.
  • refs[].url is an unsigned, non-expiring public GCS URL (storage.ts:74). This convention predates this PR, but this is the first thing to route IP-sensitive material through it — once obtained (logs, worker dependency, reviewer's browser history) access is permanent and unrevocable.
  • The refs[] happy path has no test at all. The added e2e covers only the 400 branches; nothing stores a competitor Asset, creates a t1/t2 job, claims it, and asserts refs[] contents and org scoping. That's the most sensitive path in the PR — the one that hands a competitor video to an external process — and it's also where 51c1ce8 shipped the org-leak that bbf02d4 had to fix, with no regression test now locking that fix in. Compounded by the CI skip noted in feat: remix worker engine (t0.5) — RemixJob orchestration + claim/report/upload #19: none of it runs on the runner.
  • segmentPlan duration bound is skipped when cc.duration is falsy (creatives/remix-jobs/route.ts:130-138). If ingest never populated duration, an end far past the real video length passes server-side validation, contrary to the docstring's stated intent of failing here rather than in ffmpeg.
  • parseEnabledTiers is case-sensitive with no warningREMIX_ENABLED_TIERS="T1" silently doesn't enable t1. Fails in the safe direction, but it's a quiet misconfiguration footgun.

Where that leaves us

The env gate is correctly built for what it does. But "no competitor pixels in output" is currently enforced by t1/t2 being unreachable, not by anything that would contain the risk once they're reachable — the audit trail, the reviewer-visible flag, and the ability to halt in-flight work are all absent. The moment the flag includes t1 or t2, that guarantee stops being code-enforced and becomes a process assumption.

That's a product and legal call rather than a purely technical one, and it's ours to make — we'll come back to you on it separately. On the engineering side, before this could be turned on anywhere real we'd want: claim-time re-validation against the current env (or an explicit separate kill switch), the footage flag surfaced in the review UI as a hard visual marker, logAudit() on t1/t2 creation, and real coverage of the refs[] path.

Review assisted by Claude Code; every finding above was manually verified against the diff.

mtmtian and others added 10 commits July 20, 2026 18:42
…-path index

Review fix for PR oratis#19: claim/upload/report need a way to detect a preempted
worker so it can never overwrite a new holder's output. Adds claimToken
(reissued per claim) + attempt (bumps per claim, versions the output object
key) plus @@index([status, updatedAt]) for the claim hot path. audit.ts gets
the four remix.job_* AuditAction variants used by the routes in the next commits.
Review fixes for PR oratis#19:
- claim: mint+return claimToken/attempt, rate-limit (worker-claim, 600/min/IP),
  audit-log remix.job_claim.
- upload: require x-adex-claim-token, bind the HMAC signature to
  jobId:claimToken:sha256 so a preempted worker's old signature can't be
  replayed; verify claimToken + in-flight status before writing; version the
  object key as v${attempt}.mp4; rate-limit (worker-upload, 120/min/IP);
  audit-log remix.job_upload.
- report: require claimToken in the body, include it in both updateMany
  wheres (atomic fencing), disambiguate count!==1 into 404/409-stale-claim/
  409-illegal-transition; wrap updateMany + re-read + linked-Creative update
  in a single $transaction so the job can never end up terminal with an
  orphaned Creative; exact-match the canonical (versioned) outputUrl instead
  of substring; best-effort delete the current attempt's blob on failed;
  rate-limit (worker-report, 300/min/IP); audit-log remix.job_report.
- creatives/remix-jobs: wrap Creative+RemixJob create in one transaction,
  move the rate-limit key from 'remix-jobs' to 'remix' (shared bucket with
  the sibling /api/creatives/remix so the two endpoints can't double the
  effective burst), audit-log remix.job_create.
Review fix for PR oratis#19: e2e.yml had no DB, so competitor-ingest.spec.ts and
remix-jobs.spec.ts self-skipped in CI, silently. Adds a postgres:16-alpine
service container + DATABASE_URL, INGEST_WEBHOOK_SECRET/WORKER_WEBHOOK_SECRET
(testing-only values, not real secrets) and INVITE_CODES_DISABLED=true so
register works without minting codes, plus a `prisma migrate deploy` step.

playwright.config.ts's webServer.env now reads WORKER_WEBHOOK_SECRET and
INGEST_WEBHOOK_SECRET from process.env (falling back to the same literals the
specs themselves fall back to) instead of hardcoding WORKER_WEBHOOK_SECRET —
previously a runner that set the secrets in its own env to un-skip the specs
but didn't also override this hardcoded value would sign requests the dev
server couldn't verify.
…e cases

Review fix for PR oratis#19: existing claim/report/upload cases updated for the new
contract (claimToken/attempt from claim, claimToken required on report,
versioned canonical outputUrl). New negative cases: report with a wrong
claimToken → 409 stale claim; upload missing x-adex-claim-token → 401; upload
signed against the wrong claimToken → 409. All reject before touching GCS, so
no credentials needed; the upload happy path stays untestable locally (same
as before).

.env.example documents REMIX_JOB_LEASE_MINUTES (already read by claim/route.ts,
was undocumented).
Adds t1/t2 tier codes to the remix worker engine behind an explicit
REMIX_ENABLED_TIERS opt-in (defaults to t0_5 only, matching current
IP policy). t1/t2 require a segmentPlan (t2) and a Tier-2 hand-picked
competitor video already stored via the Save video flow; claim now
returns refs[] pointing the worker at that stored Asset.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
WHY: post-PR adversarial review of 51c1ce8
WHAT: segmentPlan requires non-negative ordered non-overlapping segments
(max 64) within the source video duration; REMIX_ENABLED_TIERS always
keeps t0_5 enabled; t2 Creative.duration derives from kept segments;
claim refs lookups scoped to the job's org
…TIERS

Review fixes for PR oratis#20:
- claim: REMIX_ENABLED_TIERS was only enforced at job creation. Re-check it
  at claim time too (oldestPending/oldestStale lookups + the claiming
  updateMany all filter tier IN enabledTiers) — pulling a tier from the env
  now takes effect immediately for any t1/t2 job still pending/stale, not
  just new creates. Known boundary: a job already claimed and within its
  lease when the tier is disabled can't be recalled (the worker already
  holds its refs) — noted in the route's header comment, not fixed here.
- claim: audit-log competitor.video_reference (IP-sensitive, same as
  competitor.video_store) whenever a t1/t2 claim actually hands the worker a
  ref.
- remix-job.ts: parseEnabledTiers now trims + lower-cases each token and
  validates against KNOWN_TIERS before adding it to the enabled set — an
  unrecognized/mis-cased token is dropped with a console.warn instead of
  silently joining the allowlist (a typo there is a misconfiguration, not a
  new tier to trust).
…known

Review fix for PR oratis#20: the segmentPlan/maxEnd bounds check silently skipped
whenever cc.duration was unset, so a segmentPlan reaching well past the
actual (unknown) video length sailed through validation to fail — or
silently truncate — only once it hit the worker. Now a caller-supplied
segmentPlan on a CompetitorCreative with no known duration is rejected up
front with a 400 telling them to re-ingest with duration first. The existing
maxEnd-vs-duration check is unchanged for rows where duration IS known.
Review fix for PR oratis#20 (owner-requested, minimal change to review/{page,client}.tsx):
page.tsx safe-parses Creative.tags (JSON, defensive try/catch since tags is
free-form across sources) and passes tier/containsCompetitorFootage/
competitorReferenced through to the client. client.tsx renders a hard badge
first in the badge row — rose "⚠ competitor footage · t2" when the creative
was assembled from competitor-video segments, amber "competitor-referenced ·
t1" when it was only generated with a competitor video as a style reference
— so a reviewer sees the IP-provenance flag before anything else on the card.
…egression)

Review fix for PR oratis#20: no e2e coverage existed for claim's t1/t2 refs
resolution (CompetitorCreative.assetId → Asset.fileUrl) or the org-isolation
fix in bbf02d4. There's no hermetic HTTP path to get a stored Asset — the
real Tier-2 "Save video" flow (/api/competitors/media) fetches the source URL
over the network — so these insert the Asset row directly via SQL (raw `pg`,
not the generated Prisma client: importing `src/generated/prisma/client`
from an e2e spec hit a hard CJS/ESM incompatibility, since that file assumes
ESM (`import.meta.url`) but Playwright's test transform loads specs as
CommonJS — documented in the file header).

Two new cases:
- t2 claim returns refs pointing at the org-scoped stored Asset.
- org isolation (regression, bbf02d4): a CompetitorCreative whose assetId is
  mutated to point at a *different* org's Asset must never leak that org's
  fileUrl through claim's refs.

Also fixes the pre-existing sampleBatch fixture's `duration: 24` field, which
(per src/lib/growth/competitor-import.ts) is actually parsed as a
firstSeen~lastSeen date range, not the video length — the real field is
`videoDuration`. No prior test depended on CompetitorCreative.duration being
set, so this was latent until the new segmentPlan-duration test exercised it.
@mtmtian
mtmtian force-pushed the feat/remix-tiers branch from bbf02d4 to 608ec23 Compare July 20, 2026 11:04
@mtmtian

mtmtian commented Jul 20, 2026

Copy link
Copy Markdown
Contributor Author

Fixes pushed — rebased onto the updated feat/remix-worker + 4 commits (d5c83a6..608ec23)

Everything from the #19 fix round (claim fencing, transactions, CI wiring) applies here via the rebase. Tier-specific findings:

🔴 containsCompetitorFootage write-only → hard visual marker in review (76f0415)

The review page now parses Creative.tags server-side (defensively — tags is free-form for other sources) and passes tier / containsCompetitorFootage / competitorReferenced through the explicit field whitelist. The review card renders a leading badge no reviewer can miss: red ⚠ competitor footage · t2, amber competitor-referenced · t1. A t0.5 remix and a t2 splice no longer render identically.

🔴 REMIX_ENABLED_TIERS create-time-only → claim-time re-validation (d5c83a6)

parseEnabledTiers() is now re-checked inside /claim: the tier filter is in both candidate queries and the atomic updateMany guard. Reverting the env immediately stops every pending/stale t1/t2 job from being claimed — no redeploy-and-wait. Known boundary (documented in the route header): a job already claimed and inside its lease keeps running — the worker already holds the refs; revoking mid-render is not attempted.

🟡 Supporting gaps

  • Audit: remix.job_create on every job creation (metadata carries tier), and a new competitor.video_reference action fired at the moment /claim hands out refs[] — filed next to competitor.video_store as the same IP-sensitivity class. Who created a t2 job, and every handout of stored footage to a worker, now leaves a trail.
  • refs[] coverage (608ec23): new e2e — t2 happy path (stored org-scoped Asset → claim → refs equals [{url, kind:'video'}]) plus an org-isolation regression that locks the bbf02d4 fix (foreign assetIdrefs: []). Asset fixture is inserted via direct SQL (pg, already a dependency) — the generated Prisma client is ESM-only and won't load under Playwright's CJS transform; explained in the spec header. Runs in CI now that feat: remix worker engine (t0.5) — RemixJob orchestration + claim/report/upload #19 wires a real Postgres.
  • segmentPlan with unknown source duration (497a01d): now a 400 (re-ingest with duration first) instead of a silent skip of the bounds check.
  • Case sensitivity (d5c83a6): tokens are trimmed + lowercased and validated against KNOWN_TIERS; unknown tokens are dropped with a console.warn instead of silently joining the enabled set.
  • Unsigned public refs[].url: acknowledged — that's the bucket-wide storage convention predating this PR. If you want signed/expiring URLs for competitor-sourced assets specifically, happy to take that as a follow-up change.

Where that leaves the preconditions you listed

Claim-time re-validation ✅ · reviewer-visible hard marker ✅ · logAudit on t1/t2 ✅ · real refs[] coverage ✅. The shipped default is still t0_5-only and create-time behavior is unchanged. The product/legal call on ever enabling t1/t2 stays yours — code-side, the containment you asked for is in place.

Verification: tsc clean, full Playwright suite 45/45 green locally against live Postgres. One honest gap: the kill-switch filter itself isn't directly e2e-tested — the suite's dev server runs with a fixed env, so flipping REMIX_ENABLED_TIERS mid-suite isn't possible; it's exercised implicitly (all claims run through the tier-filtered path with every tier enabled).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants