feat(labeler): reconsideration case management + outcome notice (W10.6 server)#2083
Conversation
…6 server) Operator-side manual reconsideration workflow. Publishers quote the opaque public assessment id (already served) to the monitored inbox; operators open a case, attach private notes, and resolve it with an outcome, which fires a publisher outcome notice through the W10.5 double-opt-in pipeline. - migration 0009: `reconsiderations` (mutable case keyed on subject uri+cid, stable across reruns; state open->resolved; partial unique index bounds one open case per subject; CHECK binds state<->outcome<->resolved provenance) and `reconsideration_notes` (append-only, immutable) for operator-only notes kept off the semi-public operator_actions.reason. - three reviewer console mutations (open/note/resolve) via guardMutation/ commitMutation: audit row + effects commit atomically; duplicate-open and double-resolve are 409s backstopped by the unique index and a state-guarded UPDATE. - reconsideration-outcome notice (granted/denied; withdrawn is silent) keyed on the resolve operator_action so the retry sweep re-renders identical public copy; private note text never enters notice copy. The notice is gated on the resolve winning its guarded UPDATE, on both the fresh and replay paths, so it fires at most once per case even under a concurrent-resolve + key-replay race. - read API: paginated case list + case-with-notes detail (reviewer-gated). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
Scope checkThis PR changes 1,836 lines across 9 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. |
Deploying with
|
| Status | Name | Latest Commit | Updated (UTC) |
|---|---|---|---|
| ✅ Deployment successful! View logs |
emdash-playground | 2ffd826 | Jul 17 2026, 05:50 AM |
Deploying with
|
| Status | Name | Latest Commit | Updated (UTC) |
|---|---|---|---|
| ✅ Deployment successful! View logs |
emdash-demo-do | 2ffd826 | Jul 17 2026, 05:51 AM |
Deploying with
|
| Status | Name | Latest Commit | Updated (UTC) |
|---|---|---|---|
| ✅ Deployment successful! View logs |
emdash-demo-cache | 2ffd826 | Jul 17 2026, 05:51 AM |
There was a problem hiding this comment.
This PR adds the server-side W10.6 reconsideration workflow for the labeler. It is the right change, at the right layer, and follows the existing guardMutation/commitMutation seam, so Access auth, reviewer authorization, CSRF, idempotency, and post-commit tails all come for free.
What I checked:
- Migration
0009_reconsiderations.sql: sensible split between mutablereconsiderationsand append-onlyreconsideration_notes; the partial unique index and table CHECK correctly bound the open↔resolved lifecycle. - SQL safety: all parameterized D1 queries; no raw interpolation (the
RECONSIDERATION_COLUMNSstring is a constant, not user input). - API shape & auth: mutations route through
guardMutationwithrequiredRole: "reviewer"; reads route throughguardReadwithminRole: "reviewer"; responses use the expected{ data }/{ error: { code, message } }envelopes. - Notification integration: the W10.5
notifyReconsiderationOutcomepath, sweep parity, and the replay-sideresolveWongate are all present, which closes the concurrent-resolve-plus-key-replay race the PR description highlights. - AGENTS.md conventions: no Lingui/RTL concerns (server-only slice), no content-table locale filters to miss, the labeler package is
private: trueso changeset n/a is correct, and indexes are reasonable for the access patterns. - Tests: the new
console-reconsiderations.test.tscovers open/note/resolve, idempotency replay, double-open, double-resolve, granted/denied/withdrawn outcomes, the concurrent-loser notice race, and sweep parity.
Headline conclusion: the implementation is solid and the idempotency/replay race is handled correctly. I have one design tradeoff worth surfacing rather than a blocking defect.
The tradeoff is in runReconsiderationResolve: a resolve that loses the state = 'open' UPDATE race still commits its audit row and a reconsideration-resolved operational event, and returns 200 with its own descriptor/outcome. The post-commit resolveWon gate correctly prevents a duplicate notice, but the audit/event trail and the HTTP response can still claim an outcome that the case state does not reflect. This mirrors runDeadLetterAction, so the pattern is not new, but for reconsideration specifically it means the case can end up resolved as granted while a concurrent operator request receives 200 with a denied descriptor and an event that says the case was resolved as denied. Slicing the event insert into a WHERE EXISTS (SELECT 1 FROM reconsiderations WHERE id = ? AND outcome_action_id = ?) gated insert (analogous to the in-batch label gates elsewhere) would keep the audit row needed for replay while avoiding a misleading operational event. If the team is comfortable with the current tradeoff, a code comment at the resolve handler spelling it out would help future maintainers.
| statements.push( | ||
| buildReconsiderationNoteInsert(deps.db, { | ||
| id: newReconsiderationNoteId(), | ||
| reconsiderationId: id, | ||
| authorId: actor.id, | ||
| authorEmail: actor.email, | ||
| authorCommonName: actor.commonName, | ||
| authorRole: actor.role, | ||
| note: ctx.body.note, | ||
| createdAt: ctx.now.toISOString(), | ||
| createdAtEpochMs: ctx.now.getTime(), | ||
| }), | ||
| ); | ||
| } | ||
| statements.push( | ||
| buildOperationalEventInsert(deps.db, { | ||
| id: newOperationalEventId(), | ||
| eventType: "reconsideration-resolved", | ||
| severity: "info", |
There was a problem hiding this comment.
[suggestion] The reconsideration-resolved event is pushed into the batch unconditionally, so a resolve that loses the guarded UPDATE race still emits a reconsideration-resolved operational event under its own action id. That event then disagrees with reconsiderations.outcome_action_id and the case's actual outcome. The notice is already gated on resolveWon; consider gating this event insert the same way (e.g. an INSERT ... WHERE EXISTS (SELECT 1 FROM reconsiderations WHERE id = ? AND outcome_action_id = ?)), or at least add a comment explaining that a race-loser intentionally leaves a resolved event for replay/idempotency reasons.
| statements.push( | |
| buildReconsiderationNoteInsert(deps.db, { | |
| id: newReconsiderationNoteId(), | |
| reconsiderationId: id, | |
| authorId: actor.id, | |
| authorEmail: actor.email, | |
| authorCommonName: actor.commonName, | |
| authorRole: actor.role, | |
| note: ctx.body.note, | |
| createdAt: ctx.now.toISOString(), | |
| createdAtEpochMs: ctx.now.getTime(), | |
| }), | |
| ); | |
| } | |
| statements.push( | |
| buildOperationalEventInsert(deps.db, { | |
| id: newOperationalEventId(), | |
| eventType: "reconsideration-resolved", | |
| severity: "info", | |
| statements.push( | |
| buildOperationalEventInsert(deps.db, { | |
| id: newOperationalEventId(), | |
| eventType: "reconsideration-resolved", | |
| severity: "info", | |
| actionId: ctx.actionId, | |
| subjectUri: existing.subjectUri, | |
| payload: { reason: ctx.reason }, | |
| now: ctx.now, | |
| }), | |
| ); | |
| // TODO: gate the event on winning the UPDATE so a race-loser does not emit a | |
| // `reconsideration-resolved` event that disagrees with the case state. |
| const returned = await commitMutation(deps.db, ctx, commitSpec, statements, descriptor); | ||
| if (await resolveWon(deps.db, id, returned.actionId)) deferReconsiderationNotify(deps, returned); | ||
| return jsonData(returned); |
There was a problem hiding this comment.
[suggestion] A concurrent resolve whose guarded UPDATE no-ops gets a 200 with its own descriptor (including its own outcome), even though the case was resolved by another action. The PR description says double-resolve is a 409, so this behavior is likely to surprise operators and console UI authors. This is the same tradeoff runDeadLetterAction makes, but it is worth documenting at the return point so slice 2 UI knows a 200 does not always mean "your outcome won".
| const returned = await commitMutation(deps.db, ctx, commitSpec, statements, descriptor); | |
| if (await resolveWon(deps.db, id, returned.actionId)) deferReconsiderationNotify(deps, returned); | |
| return jsonData(returned); | |
| const returned = await commitMutation(deps.db, ctx, commitSpec, statements, descriptor); | |
| // A concurrent resolve may have lost the state='open' race: the audit row | |
| // commits (so replay/idempotency works) but the case state was flipped by a | |
| // different action. Only fire the notice when this action actually won. | |
| if (await resolveWon(deps.db, id, returned.actionId)) deferReconsiderationNotify(deps, returned); | |
| return jsonData(returned); |
…ollow-up) Two-stage ladder driven by the reconciliation cron: 24h -> operator operational_event, 72h -> publisher notice if the error is still the live unsuperseded run. Records the fire-once tracking table (0010), the issuance-source reuse that avoids a notifications.source_type rebuild, and the finalization-path invariant (assessmentNoticeContent stays null for error so only the cron notifies). Migration 0010 lands after 0009 (#2083). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
A losing concurrent resolve no-ops its state='open'-guarded UPDATE but its audit row still commits (needed for idempotency replay). Gate the reconsideration-resolved operational_event on the case's outcome_action_id being this action's, via a new gateOnResolvedReconsideration in-batch gate mirroring gateOnIssuedLabelActionKey, so the loser emits no phantom event claiming an outcome the case did not take. The loser still echoes its requested outcome in its own 200 descriptor; case state, event, and notice reflect only the winner. Addresses the emdashbot review tradeoff on #2083. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
Thanks — addressed the resolved-event tradeoff you flagged (commit 83232c9). The I deliberately left the loser's HTTP response as a 200 with its own descriptor rather than a 409: ~ 🤖 Claude Fable 5 |
There was a problem hiding this comment.
Re-review of the W10.6 reconsideration server-side slice. The changes since the prior review correctly close the concurrent-resolve race the author highlighted:
- The
reconsideration-resolvedoperational event is now gated viagateOnResolvedReconsideration, so a losing concurrent resolve commits its audit row but emits no phantom resolved event. - The replay branch runs the same
resolveWoncheck as the fresh path, so replaying a losing resolve's idempotency key does not fire a contradictory outcome notice. - An explicit comment notes why a losing resolve still echoes its own requested outcome in the 200 descriptor (matching the existing
runDeadLetterActiontradeoff), while the authoritative case state/event/notice reflect only the winner.
What I checked:
- Migration 0009: sensible split between mutable
reconsiderationsand append-onlyreconsideration_notes; partial unique index and state/outcome CHECK correctly bound the lifecycle. - SQL safety: parameterized D1 statements; the
RECONSIDERATION_COLUMNSconstant is never user input. - Auth: mutations route through
guardMutationwithrequiredRole: "reviewer"; reads route throughguardReadwithminRole: "reviewer"; responses use the expected{ data }/{ error: { code, message } }envelopes. - Notification integration: sweep parity via
resolveNoticeForSource, live trigger vianotifyReconsiderationOutcome, source-based dedup insendNotification, and the replay-sideresolveWongate. - AGENTS.md conventions: no Lingui/RTL concerns (server-only slice), no content-table locale filters to miss,
@emdash-cms/labelerisprivate: trueso changeset n/a is correct.
The implementation is solid. I have one minor schema-defensiveness suggestion: the table CHECK doesn't fully enforce the "resolved-provenance" it describes, which is otherwise guaranteed only by application code.
| CHECK ( | ||
| (state = 'open' AND outcome IS NULL AND resolved_at IS NULL AND outcome_action_id IS NULL) | ||
| OR (state = 'resolved' AND outcome IS NOT NULL AND resolved_at IS NOT NULL) | ||
| ) |
There was a problem hiding this comment.
[suggestion] The migration comment says this CHECK binds state ↔ outcome ↔ resolved-provenance, but the resolved branch only enforces outcome and resolved_at. It allows a resolved row where outcome_action_id is NULL, which would break resolveWon (it tests current?.outcomeActionId === actionId) and would leave no winner reference for the operational event/notice pipeline. Since each resolving path always sets outcome_action_id to the operator action id, the schema can safely require it for resolved rows.
| CHECK ( | |
| (state = 'open' AND outcome IS NULL AND resolved_at IS NULL AND outcome_action_id IS NULL) | |
| OR (state = 'resolved' AND outcome IS NOT NULL AND resolved_at IS NOT NULL) | |
| ) | |
| CHECK ( | |
| (state = 'open' AND outcome IS NULL AND resolved_at IS NULL AND outcome_action_id IS NULL) | |
| OR (state = 'resolved' AND outcome IS NOT NULL AND resolved_at IS NOT NULL AND outcome_action_id IS NOT NULL) | |
| ) |
…e CHECK The table CHECK described a resolved row as carrying full provenance but only enforced outcome + resolved_at NOT NULL, leaving outcome_action_id, resolved_by_id, and resolved_at_epoch_ms to app code alone. Extend both branches so the schema enforces the whole lifecycle invariant: an open row carries zero resolve provenance; a resolved row carries all of it. Tests assert the CHECK rejects a partial-resolved row and an open row with stray provenance. Addresses the emdashbot re-review schema-defensiveness suggestion on #2083. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
Took the schema-defensiveness suggestion (commit 2ffd826). The
Two tests assert the DB rejects a partial-resolved row and an open row with stray provenance; the existing resolve tests confirm legitimate resolves (which always set the full tuple) still pass. 20/20 green. ~ 🤖 Claude Fable 5 |
There was a problem hiding this comment.
Re-review of the W10.6 reconsideration server-side slice. The implementation remains sound and the prior review's concern is addressed:
- Schema CHECK / resolved-provenance (prior finding, fixed): Migration
0009_reconsiderations.sqlnow bindsstate='open'↔ all resolve fields NULL andstate='resolved'↔outcome,resolved_at,resolved_at_epoch_ms,resolved_by_id, andoutcome_action_idall NOT NULL. Two integration tests explicitly reject rows that violate these invariants, so the schema is defensively guarded rather than relying solely on application code. - Concurrent-resolve race (correctly closed): The
reconsideration-resolvedoperational event is gated viagateOnResolvedReconsideration, and the replay branch runs the sameresolveWoncheck as the fresh path. A losing concurrent resolve commits its audit row but emits no phantom event and no contradictory outcome notice. - Auth / envelope shape: Mutations route through
guardMutation/commitMutationwithrequiredRole: "reviewer"; reads route throughguardReadwithminRole: "reviewer"; responses use{ data }/{ error: { code, message } }consistently. - SQL safety & privacy: All D1 statements are parameterized; private note text never reaches notice copy; outcome notices source from the resolve
operator_actionand dedup on(source_type, source_id). - AGENTS.md compliance: No admin UI / Lingui / RTL concerns in this server-only slice;
@emdash-cms/labelerisprivate: true, so no changeset is needed.
I read the diff, the touched source files, the mutation guard, notification-send, and the test suite. No new blocking issues or regressions. The code is ready to merge.
4c849c6
into
feat/plugin-registry-labelling-service
What does this PR do?
Implements the server side of the W10.6 manual reconsideration workflow for the plugin-registry labelling service (RFC #694, umbrella #1909). Targets the
feat/plugin-registry-labelling-serviceintegration branch.The workflow is deliberately operator-side only — no authenticated appeals portal or SLA (per the plan). A publisher who wants a decision reconsidered emails the monitored inbox (already published in
/.well-known/emdash-labeler-policy.jsonviacontact.reconsiderationUrl/reconsiderationEmail) quoting the opaque public assessment id (the existingasmt_<ulid>, already served by the public assessment API and every notice). An operator then manages the case in the console:granted/denied/withdrawn), which — forgranted/denied— fires a publisher outcome notice through the W10.5 double-opt-in pipeline.withdrawnnotifies nothing.Design decisions were ratified and recorded in the plan doc (
docs(labeler-plan): ratify W10.6 reconsideration design decisions).What's here (slice 1 — server only):
0009_reconsiderations.sql:reconsiderations(mutable case, keyed on subjecturi+cidso it survives reruns;stateopen→resolved; a partial unique index bounds one open case per subject; a table CHECK bindsstate↔outcome↔ resolved-provenance so no half-resolved row can exist) andreconsideration_notes(append-only, immutable via triggers). Private notes live here, deliberately off the semi-publicoperator_actions.reasonso note text can never reach notice copy.reconsiderations/open,:id/note,:id/resolve) via the existingguardMutation/commitMutationseam — Access JWT + reviewer role + CSRF + request-fingerprint idempotency + atomic audit-row-plus-effect batch, none of it hand-rolled. Duplicate-open and double-resolve are409s, backstopped by the partial unique index and astate='open'-guarded UPDATE respectively.reconsideration-outcome) keyed on the resolveoperator_action(source_type: 'operator'), so the W10.5 retry sweep re-renders byte-identical public copy. The notice is gated on the resolve winning its guarded UPDATE — on both the fresh and the replay paths — so it fires at most once per case even under a concurrent-resolve-plus-key-replay race.The operator console UI for this is slice 2 (a follow-up PR). This PR is server + tests only.
Part of #1909. Related to #694.
Type of change
Checklist
pnpm typecheckpasses (all three labeler projects)pnpm lintpasses (0 diagnostics on touched files)pnpm testpasses (18 reconsideration integration tests; regression re-run of console-mutation-api + notification-triggers + notification-sweep green)pnpm formathas been run@emdash-cms/labelerisprivate: true, not in the published set.AI-generated code disclosure
Screenshots / test output
The adversarial review pass confirmed one real defect and it is fixed here: the resolve replay branch fired the outcome notice without the win-check the fresh path uses, so a losing concurrent resolve whose idempotency key was later replayed could send a second — possibly contradictory — outcome email under its own source id. The replay branch now runs the identical
resolveWongate; a reproduction test (does not re-notify when a losing concurrent resolve's key is replayed) fails without the gate (two notices leak) and passes with it.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-reconsideration. Updated automatically when the playground redeploys.