feat(labeler): production Workflow shell over AssessmentOrchestrator (#1978)#2072
Conversation
|
Scope checkThis PR changes 1,362 lines across 14 files. Large PRs are harder to review and more likely to be closed without review. If this scope is intentional, no action needed. A maintainer will review it. If not, please consider splitting this into smaller PRs. See CONTRIBUTING.md for contribution guidelines. |
github-actions
Bot
added
size/XL
review/needs-review
Deploying with
|
| Status | Name | Latest Commit | Updated (UTC) |
|---|---|---|---|
| ✅ Deployment successful! View logs |
emdash-playground | c46a9cc | Jul 16 2026, 01:19 PM |
Deploying with
|
| Status | Name | Latest Commit | Updated (UTC) |
|---|---|---|---|
| ✅ Deployment successful! View logs |
emdash-demo-do | c46a9cc | Jul 16 2026, 01:19 PM |
Deploying with
|
| Status | Name | Latest Commit | Updated (UTC) |
|---|---|---|---|
| ✅ Deployment successful! View logs |
emdash-demo-cache | c46a9cc | Jul 16 2026, 01:19 PM |
![emdashbot[bot]](https://avatars.githubusercontent.com/in/3255304?s=60&v=4){kind=small}
There was a problem hiding this comment.
This is a sound approach to the #1978 queue-vs-Workflow question. Using the existing per-run runKey as the Cloudflare Workflow instance id is the right granularity: same discovery event redelivery dedups on the same instance, while a re-assessment (distinct triggerId) gets its own instance id and is not stranded by the prior run’s retained id. Wrapping AssessmentOrchestrator in a thin durable shell and driving it through one step.do keeps the existing atomic finalization intact, and relaxing the runAssessment entry guard to resume from running fixes the crash-after-CAS stranding problem described in the PR.
I read the diff, the new assessment-dispatch.ts/assessment-workflow.ts files, the assessment-orchestrator.ts and discovery-consumer.ts integration, the updated tests, wrangler.jsonc, and the generated worker-configuration.d.ts. I also checked against AGENTS.md conventions (SQL safety, changesets, localization, RTL, API envelopes, authorization, indexes, locale filtering); nothing in this labeler app triggers those conventions.
Headline conclusion: the implementation is clean and well tested, but two safety gaps stand out:
- Comment-only deploy gate. With
stubStages, every subject finalizespassedand issues a real signedassessment-passedlabel. The only guard against an accidental production deployment is the comment inassessment-workflow.ts. A mechanical guard is missing. - Finalization batch CAS result is ignored. The code correctly narrows the delete/cancel window with a second currency check, but it does not verify that the state-transition UPDATE actually changed the row, so a concurrent cancel can commit labels while leaving the run non-terminal. The PR acknowledges this, but verification should be local.
Other observations: the operator rerun path is intentionally left unwired (called out as a follow-on), and the auto-generated worker-configuration.d.ts diff contains a broad workerd version bump and unrelated type reshuffling that should be kept minimal if possible.
Findings
-
[needs fixing]
apps/labeler/src/assessment-workflow.ts:99return stubStagesmeans every assessment finalizespassedand issues a real signedassessment-passedlabel. The only protection preventing this from reaching a label-consuming production deployment before W7/W8 is the comment above it. Comments are not a deploy gate: a mis-merge, a deployment from this branch, or a configuration drift could unconditionally vouch for unscanned content.Make the gate mechanical by refusing to run
stubStagesin production:function buildStages(): OrchestratorStages { if (import.meta.env.PROD) { throw new Error( "stubStages cannot run in production; wire real stage adapters in buildStages()", ); } return stubStages; }Tests remain unaffected because
import.meta.env.PRODis false in the test harness, while a production Worker that has not yet had real stages wired fails loudly instead of issuing false reassurance labels. -
[needs fixing]
apps/labeler/src/assessment-orchestrator.ts:345The finalization batch is committed here, but the result of the state-transition CAS statement is never inspected. If a concurrent cancel/delete moved the run out of
runningafter the secondisSubjectCurrentcheck, the conditionalUPDATE ... WHERE state = 'running'no-ops while the label inserts still commit. The method then returns the unchanged row (still inrunningorcancelled), andexecuteAssessmentInstancereturns a non-terminal state to the Workflow step.The PR’s comments correctly identify this race, but leaving the batch result unverified means the orchestrator can silently finish without actually changing the run state. At minimum, assert the row ended in the expected terminal state and throw so the durable step retries:
await this.db.batch(statements); for (const postCommit of postCommits) await postCommit(); const finalised = await getAssessment(this.db, assessment.id); if (!finalised) throw new Error(`assessment ${assessment.id} disappeared after finalization`); if (finalised.state !== toState) { throw new Error( `assessment ${assessment.id} finalization CAS failed: expected ${toState}, got ${finalised.state}`, ); } return finalised;On retry,
executeAssessmentInstancewill short-circuit on the terminal row or resume therunningrow as appropriate, avoiding a successful Workflow step that never finalized.
github-actions
Bot
added
review/awaiting-author
…gate Address emdashbot review findings on #2072. Finding 2 (finalization CAS race): the run→toState CAS and the label inserts shared one db.batch, but a 0-row CAS (concurrent cancel/delete moved the run out of `running`) did not roll back the inserts — signed assessment-passed labels leaked for a cancelled subject, and a post-hoc re-read could not un-leak them. Now every finalization issuance action is gated in-batch on the assessment reaching `toState` (buildIssuanceStatements gains an optional requireAssessmentState; the label insert selects from the action, so gating the action gates the label). The batch is all-or-nothing against the CAS: a cancel/delete that no-ops the CAS also no-ops every label. The CAS row count is checked after commit — 0 means the race was lost, so the postCommits (which would otherwise mis-diagnose the absent label as a signing failure and throw) are skipped and AssessmentFinalizationConflictError is raised; the Workflow step retries and short-circuits on the now-terminal row. The existing second isSubjectCurrent check only covers delete/supersede down to the check-to-commit gap (the delete path tombstones the subject); it does not cover an operator cancel that leaves the subject current. The in-batch state guard closes both. Finding 1 (deploy gate): buildStages() now throws when import.meta.env.PROD, so a production build with only stub stages fails loudly instead of signing assessment-passed for every unscanned subject. import.meta.env.PROD is a Vite compile-time constant (true in `vite build`, false in dev and the vitest pool); a minimal ambient declaration types it without pulling in vite/client browser globals. Test: a stage that cancels the run mid-flight proves finalization issues no labels and throws the conflict.
github-actions
Bot
added
review/needs-rereview
Wire the AssessmentOrchestrator to production as a Cloudflare Workflow: each verified subject runs as one Workflow instance whose id is a deterministic SHA-256 of (uri, cid). Creating an instance with an existing id throws, so a duplicate subject cannot start a second concurrent run — the instance-per- subject serialization IS the §14.1 per-subject lock, closing the duplicate-run race #1978 deferred. - assessment-workflow.ts: AssessmentWorkflow (WorkflowEntrypoint) runs the orchestrator inside a single durable step over executeAssessmentInstance, which loads the pending run, composes the orchestrator (stub stages for now), and finalizes. Idempotent resume: an already-finalized run returns its terminal state instead of re-entering the pending-guarded orchestrator. - assessment-dispatch.ts: instance-id derivation + dispatchAssessmentWorkflow, which treats an already-exists collision as an idempotent no-op and surfaces infra failures as AssessmentDispatchError. - discovery-consumer.ts narrows to "verified discovery → dispatch"; a dispatch failure retries the message (upstream steps are idempotent). - ASSESSMENT_WORKFLOW binding in wrangler.jsonc + worker entry export. Stages are stubStages until W7/W8 land the real acquire/deterministic/ dependency/AI/history adapters; the per-run AcquisitionHolder they need is constructed at the per-run buildStages() seam. The orchestrator's finalization race comments are corrected: the lock closes duplicate runs, not a delete or cancel racing an in-flight run — that still needs an in-batch state guard.
Address the adversary pass on the Workflow shell. Fix B (Matt's ruling — instance id from runKey, not subject): a subject-only id (SHA-256 of uri+cid) is permanent (Cloudflare retains instance ids ~30 days for completed AND failed runs), so it blocked all re-assessment — operator rerun / intel re-triggers mint a new run (new runKey, which already includes triggerId) for the same subject and would collide with the retained id and strand the run pending forever. The instance id is now the run's runKey itself (already a 64-char SHA-256 hex from computeRunKey, reused verbatim — no second formula). Same-trigger discovery redelivery → same runKey → same id → dedup; distinct triggers → distinct runKeys → distinct instances → re-assessment works. Two different triggers for one subject can now run concurrently — that falls back to the currency/supersede + idempotency-keyed issuance path, not the instance lock. Fix A (resumable-from-running): any failure after the pending→running CAS (reachable even with stub stages) left the row `running`; the step retry re-entered runAssessment, whose pending-guard threw again → run stranded `running` forever. runAssessment now accepts a `running` row and resumes it to finalization (stages recomputed, findings held in memory not persisted mid-run, label issuance idempotency-keyed). Minimal orchestrator change: broaden the entry guard + skip the CAS when already `running`. Fix C (deploy gate): with stub stages, a run finalizes `passed` AND issues a real signed assessment-passed label for every subject. Corrected the orchestrator header to say so and added a prominent DEPLOY GATE note in the workflow docblock — the shell must not reach an enforcing/consumed deployment until real stages land. Also: corrected the stale console-mutation rerun comment (discovery now dispatches) and reconciled the orchestrator's finalize race comments to the per-run (runKey) dedup semantics. Track D (delete/cancel/signing-flip vs finalize) remains gated on the in-batch assessment-state guard. Tests: runKey-id dispatch semantics (dedup on same runKey, distinct runKey → own instance); shell resume from a running row; orchestrator resume after a post-transition failure then success.
…gate Address emdashbot review findings on #2072. Finding 2 (finalization CAS race): the run→toState CAS and the label inserts shared one db.batch, but a 0-row CAS (concurrent cancel/delete moved the run out of `running`) did not roll back the inserts — signed assessment-passed labels leaked for a cancelled subject, and a post-hoc re-read could not un-leak them. Now every finalization issuance action is gated in-batch on the assessment reaching `toState` (buildIssuanceStatements gains an optional requireAssessmentState; the label insert selects from the action, so gating the action gates the label). The batch is all-or-nothing against the CAS: a cancel/delete that no-ops the CAS also no-ops every label. The CAS row count is checked after commit — 0 means the race was lost, so the postCommits (which would otherwise mis-diagnose the absent label as a signing failure and throw) are skipped and AssessmentFinalizationConflictError is raised; the Workflow step retries and short-circuits on the now-terminal row. The existing second isSubjectCurrent check only covers delete/supersede down to the check-to-commit gap (the delete path tombstones the subject); it does not cover an operator cancel that leaves the subject current. The in-batch state guard closes both. Finding 1 (deploy gate): buildStages() now throws when import.meta.env.PROD, so a production build with only stub stages fails loudly instead of signing assessment-passed for every unscanned subject. import.meta.env.PROD is a Vite compile-time constant (true in `vite build`, false in dev and the vitest pool); a minimal ambient declaration types it without pulling in vite/client browser globals. Test: a stage that cancels the run mid-flight proves finalization issues no labels and throws the conflict.
d3f6640 to
c46a9cc
Compare
There was a problem hiding this comment.
Both prior blockers are resolved in this revision.
-
Mechanical deploy gate (fixed).
assessment-workflow.tsnow refuses to build stages in a production build (import.meta.env.PROD), replacing the comment-only guard. Thevite-env.d.tstyping keepsimport.meta.env.PRODavailable to TypeScript without pulling in browser globals, and the calibration tsconfig is updated to include it. -
Finalization CAS verification (fixed).
assessment-orchestrator.tsnow checksresults[finalization.assessmentUpdateIndex].meta.changesafterdb.batch, and throwsAssessmentFinalizationConflictErrorwhen the state-transition update changed no row. Combined with the new in-batchrequireAssessmentState: toStateguard on every issuance statement, a concurrent cancel/delete can no longer commit labels after the CAS no-ops.
The rest of the implementation remains sound: the AssessmentWorkflow is a thin durable shell over AssessmentOrchestrator, the runKey-derived Workflow instance id gives the right dedup/re-assessment granularity, executeAssessmentInstance short-circuits on terminal rows and resumes running rows, and the new assessment-dispatch.ts helper plus production-boundary.test.ts keep the orchestrator reachable only through the Workflow entrypoint. Tests cover the CAS race, resume-from-running, and dispatch deduplication.
No new logical or convention issues were found in this pass. The tracked delete/cancel/signing-flip race is still correctly disclosed as a follow-up for the real-stage wiring.
github-actions
Bot
added
review/approved
722cb9c
into
feat/plugin-registry-labelling-service
1 participant
What does this PR do?
Wires the labeler's assessment pipeline into production execution via a Cloudflare Workflow — the ratified resolution to the #1978 queue-vs-Workflow question. Each assessment run executes as one Workflow instance, a thin durable shell over the already-tested
AssessmentOrchestrator; the queue consumer narrows to verified discovery → Workflow dispatch. All stages are currentlystubStages(no findings); the real acquire/AI stages attach atbuildStages()in the W7/W8 follow-on.Instance id = the run's
runKey. Following maintainer direction, the Workflow instance id is the run's existingrunKey(a deterministic SHA-256 over uri, cid, policyVersion, modelId, promptHash, scannerSetVersion, triggerId — computed by the discovery consumer, reused verbatim, not re-derived). Redelivery of the same discovery event recomputes the same runKey →createcollides → idempotent no-op (dedup). A distinct trigger (operator rerun, intel re-trigger) yields a distinct runKey → its own instance → re-assessment dispatches instead of being silently deduped. Two different triggers for one subject running concurrently is acceptable — backstopped by the currency/supersede mechanism and idempotency-keyed issuance, not by the instance id.Resumable finalization.
runAssessment's entry guard now accepts arunningrow left by a crashed prior attempt and resumes it to finalization (previously it threw, so any post-pending→runningfailure — e.g. a transient D1 error infinalize— stranded the run inrunningforever, permanently advertisingassessment-pending, with reconciliation only logging it). Re-running is safe: stages are recomputed (findings held in memory, not persisted mid-run) and all label issuance is idempotency-keyed. The change is the entry guard only — the stage loop, finalize, CAS, and batch are untouched. Terminal rows (incl.cancelled/stale) short-circuit before re-entry.An adversarial review pass drove these changes (the runKey id, the resumability fix, and the disclosures below).
Deploy gate and tracked prerequisites
passedand finalization issues a real signedassessment-passedlabel for every subject with zero analysis. This must NOT reach an enforcing / label-consuming production deployment until the real analysis stages land. Documented prominently inassessment-workflow.tsand corrected in the orchestrator header (which previously undersold it as only clearingassessment-pending).finalize's comments; acceptable to ship only because the deploy gate above holds (nothing consumes the labels yet). Finer per-stage durable resume also lands with the real stages.Part of the plugin-registry labelling service (RFC #694, umbrella #1909). Targets the integration branch.
Type of change
Covered by the approved RFC #694 umbrella.
Checklist
pnpm typecheckpassespnpm lintpasses (zero diagnostics in changed files; pre-existing baseline unchanged)pnpm testpasses (full labeler suite 637; new dispatch runKey-id, resume-from-running, and orchestrator resume tests)pnpm formathas been run (touched paths)@emdash-cms/labeleris unpublished (private: true)AI-generated code disclosure
Screenshots / test output
New tests: dispatch-helper runKey-id dedup (same runKey → one instance; distinct runKey → its own; created/exists/exists-on-lost-ack/throws-when-absent); a resume-from-
runningcase inassessment-workflow.test.ts; and an orchestrator resume test where a stage throws post-transition on attempt 1 (row leftrunning) and the re-invocation resumes and finalizespassedwith the pending negation applied exactly once.Try this PR
Open a fresh playground →
A full working EmDash site, deployed from this branch. Each visit gets its own session-scoped sandbox: no login needed and no shared state. Try the admin, edit content, hit the public site.
Tracks
feat/labeler-workflow-shell. Updated automatically when the playground redeploys.