diff --git a/.context/decisions/0022-web-approval-is-dispatched-not-page-driven.md b/.context/decisions/0022-web-approval-is-dispatched-not-page-driven.md index 1f8e218..0d20772 100644 --- a/.context/decisions/0022-web-approval-is-dispatched-not-page-driven.md +++ b/.context/decisions/0022-web-approval-is-dispatched-not-page-driven.md @@ -45,3 +45,13 @@ A run's progress is only as fresh as the page's 20 second refresh. - website#407 (list fix and interim guard) and website#408 (this change). - nemar-cli `src/lib/api/publish.ts` (`approvePublication`, the loop) and `backend/src/services/publication-orchestrator.ts` (step 14, `hasMore`). - `nemarDatasets/.github` `onboard-openneuro.yml` (the existing Actions-runs-the-CLI pattern). + +## Update, 2026-10-14: Approve now carries a body + +The decision stands: the page asks the backend to start an approval, the backend asks an executor, and the page reads the result. +What changed is the request the page sends. +Approve and Deny now send a body that names the request (`request_id`), and Approve sends the review in force and the reasons shown when it approves over a non-pass. +The page also reads the API's review detail before it offers Approve, and prints the API's own `statement` and neutral `cure` for each reason, never the terminal's sentences. +It withholds Approve when that detail cannot be read, so the old bodyless click no longer exists. +The deploy order in Consequences gains a consequence: the page needs an API that serves the review detail (nemar-cli#1731), so the API deploys first. +See ADR 0027. diff --git a/.context/decisions/0027-the-admin-approval-page-prints-the-apis-review-words.md b/.context/decisions/0027-the-admin-approval-page-prints-the-apis-review-words.md new file mode 100644 index 0000000..40669fb --- /dev/null +++ b/.context/decisions/0027-the-admin-approval-page-prints-the-apis-review-words.md @@ -0,0 +1,91 @@ +# ADR 0027: The admin approval page prints the API's review words and computes no verdict + +**Status:** accepted +**Date:** 2026-10-14 +**Owner:** Seyed Yahya Shirazi + +## Context + +ADR 0022 made approval a dispatched action: the page asks the backend to start it and reads what the backend reports. +Since then the Application Programming Interface (API) has learned to derive a verdict for each publication request, from an automated review and an identifier screen, and to gate approval on it (nemar-cli ADR 0102, with its Phase 4 amendment, and ADR 0103). +The page still showed a status badge and sent an empty body, so an administrator could not see why a request was held, and the API's gates could only refuse the click after the fact. +The terminal words a reason with a command inside it (its `line`, and a `cure` that names the command to run), so the page cannot print those as they are. +The API sends each reason a neutral `statement` and `cure` beside them, and the page prints those. + +## Decision + +The page fetches the API's review detail for a request and prints the API's own words. +Each reason shows its `statement` and its neutral `cure`, the identifier screen shows its `headline` and `lines`, and the model's text appears only under a label that says the model wrote it. +The page does not print the terminal's sentences: the terminal's `cure` carries commands, so the page never prints `line`, `command`, `description.lines`, `description.next`, or the `message` of a refusal that names a terminal step. +It transcribes only a 22-key table of the sentences the API words for a terminal, kept in `src/lib/publication-review-copy.ts` and checked against the nemar-cli copy by a drift test, the arrangement of ADR 0015. +It never computes a verdict. +It never offers Approve for a reason no override clears, and it sends `request_id` with every approve and deny and, for an override, the review in force and exactly the reasons it showed. +The review start sends no `request_id`: it acts on the active request, and its own refusals cover a request that has moved. +The rulings below settle the cases the design left open. + +- **A hard reason withholds Approve whatever `gate` says.** + The API does not gate on an advice-only review, but the page fails closed on a hard reason regardless, so an unknown `cleared_by` word is also a hard reason. + An administrator who needs to approve such a request uses the terminal. + A run that has started publishing is exempt: it passed the gates when it started, so its panel offers "Continue approval" whatever reasons now stand. +- **A blocked request, and a screen that says `blocks`, `rerun` or `wait`, withhold Approve.** + The API refuses every blocked request, even a walked-back run, so the page withholds Approve for one before it looks at anything else. + The identifier screen's `blocks` (a direct identifier), `rerun` (no verdict) and `wait` (still running) each withhold it with the shared sentence for that gate, added to the hard-reason line when both apply. + Of the other screen words, `acknowledge` offers Approve with a reason to record, and `clear` and `exempt` offer it with none. +- **When an override-class reason stands and there is no review row, the words depend on whether a review can be started.** + If the detail offers a start (`start_offered` is true), the line is the shared `gate.no_review_row` sentence. + If it does not, the line is a page-owned word in `src/lib/publication-review.ts`, outside the mirrored table: "No review of this commit is on record and none can be started from this page, so Approve is not offered." + The shared sentence ends by telling the reader to start the review, which is wrong for a request no start can reach. +- **There is no fallback to the old bodyless click.** + If the detail cannot be read, the page withholds Approve and prints the shared "could not be read just now" sentence (`approve.unavailable`). + It says it is out of step with the API only when the API is older than the page or the answer cannot be trusted, and then it withholds the whole panel and prints no reason at all. + The answer cannot be trusted when any of these holds: + `request_id` is not an integer; + `request_status` is not a string; + `gate` is not one of `enforced`, `off` or `n/a`; + `screen_gate` is not one of `clear`, `blocks`, `acknowledge`, `rerun`, `wait` or `exempt`; + `anonymous`, `applies`, `started_publishing` or `start_offered` is not a boolean; + `reasons` is not a list in which every reason has string `code`, `class`, `cleared_by`, `sentence` and `cure` and a non-empty string `statement`; + `description` or `identifier_screen` lacks a string `headline` or `tone`, or the screen's `lines` is not a list of strings; + or `review` is neither null nor a record with an integer `id`, a list of fact notes and a list of findings. + An old API that sends no `statement`, or an empty one (the page prints `statement`, never `sentence`), therefore gets the out-of-step sentence and never has a terminal `line` printed in its place. + A fallback would be a way around the page's own rules. +- **A detail that is not found prints the Worker's sentence.** + A 404 from the detail route means the request is closed, and the Worker's sentence says so; "reload" would be wrong advice, so the page prints that sentence and offers no refresh link. +- **The reason for the identifier screen is always asked for when the screen says `acknowledge`.** + The API would pass without one when this administrator's own acknowledgment is already on the request, but the page does not read that, so it asks again. + Sending it again is accepted and recorded again, so the cost is friction and no wrong approval. +- **A stopped approval that never started publishing is offered as a fresh approval.** + The API gates a stopped run like a fresh one, so the panel asks for the typed word, and for an override or a reason when they are needed. + Its button reads "Approve and publish" where the row's own button reads "Resume approval"; only a run that has started publishing is continued without the gates, and its panel button reads "Continue approval". + When the row's own button reads "Retry approval", an approve offered again, the panel's button says "Retry approval" too, so the two never name one action two ways. +- **A failure is worded by the page, never by the Worker's sentence, where that sentence could name a terminal.** + A 401 or 403 that carries only a sentence prints the page's own "sign in again" word, because an inactive account's refusal names a terminal command. + The one exception is the origin guard's 403 (code `origin_not_allowed`): its sentence names no terminal, so the page prints the Worker's sentence for it. + An approve that gets no answer (a missed deadline, a dead connection, or the proxy's own `upstream_unreachable` or `upstream_redirect` 502) says the approval may have started and asks the administrator to refresh and check before trying again, because the dispatch may have gone out; the API's lease makes a retry a harmless refusal. +- **The API deploys before the website.** + A page promoted ahead of the API gets a detail without `statement`, so it withholds the panel, prints "out of step" and offers no Approve, and it never falls back to the terminal's `line`. + The release that carries this page therefore waits for the API that carries nemar-cli#1731. + +## Consequences + +The page prints one authoritative reading of each request, and a cure that changes with the request (the dataset, whether a start can reach it, the detail of a failed question) needs no second copy on the website. +A stale page is caught by the API's guards, which answer with a typed refusal; the page then refetches the detail rather than rendering the refusal's own reasons. +The words with no counterpart in the API, the class tags and the "out of step" sentence among them, stay page-owned and unmirrored. +The 22-key table is held by its own tests, a rule that every key has a consumer on the page, and a drift test against nemar-cli `dev`; the strict path in nemar-cli's continuous integration needs this file on `staging` first. +An administrator cannot approve from the page a request whose only blocker is a hard reason or a missing review that cannot be started; the terminal remains for those. +The page cannot run the identifier screen again (nemarOrg/website#461) or mail the depositor a comment (nemarOrg/website#462). +The `identifiers_found` badge, "Identifiers found", also shows on the depositor's dashboard card, because both surfaces draw it from the one `PublishStateBadge` component. + +## Alternatives considered + +- **Transcribe all 24 reasons and their composition:** a second copy of what changes per request, which the drift tests of ADR 0015 exist to catch. +- **Run the verdict in the browser:** a second implementation of the rules would drift from the API's, which is the failure ADR 0015 exists to prevent. +- **Extend `ConfirmDialog` with a slot:** it is a shared component, and the page needs the controls (a confirmation and a recorded reason of 10 to 500 characters) before the typed `PUBLISH` dialog, which stays the last step and is unchanged. +- **Keep the bodyless click when the detail fails:** it would let an administrator past the rules the page exists to show. + +## Receipts + +- nemar-cli#1731 (Phase 4) of epic nemar-cli#1726; website#459. +- nemar-cli ADR 0102 (Phase 4 amendment) and ADR 0103. +- Website ADR 0015 (mirrored copy and drift tests) and ADR 0022 (dispatched approval). +- The captured API response fixtures under `test/fixtures/`, each with a note on how it was captured. diff --git a/.context/decisions/README.md b/.context/decisions/README.md index 583f4de..c7ec792 100644 --- a/.context/decisions/README.md +++ b/.context/decisions/README.md @@ -58,3 +58,4 @@ Add new entries here as you create ADRs: - ADR 0024 - [Embed calls are counted at the edge, from the request alone](0024-embed-calls-counted-at-the-edge.md) (accepted) - ADR 0025 - [Catalog reads are cached for minutes to hours and fall back to a stale copy](0025-catalog-reads-cached-with-stale-if-error.md) (accepted) - ADR 0026 - [Server-side calls to the API go over a service binding that names the visitor](0026-server-side-api-calls-go-over-a-service-binding.md) (accepted) +- ADR 0027 - [The admin approval page prints the API's review words and computes no verdict](0027-the-admin-approval-page-prints-the-apis-review-words.md) (accepted) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 3493569..8c44661 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -105,6 +105,10 @@ jobs: # sparse-checkout list is the whole cost of covering another contract. # test/private-site-contract-drift.test.ts is the third, against # shared/contract/private-site.ts. + # test/publication-review-copy-drift.test.ts is a FOURTH, against + # shared/contract/publication-review-copy.ts; same `dev` pairing. + # The same test also compares the page's numbers with shared/publication-limits.ts, which + # sits beside the contract directory; without it that test skips, visibly. - uses: actions/checkout@v4 with: repository: nemarOrg/nemar-cli @@ -113,6 +117,8 @@ jobs: /shared/contract/account-copy.ts /shared/contract/docs-auth.ts /shared/contract/private-site.ts + /shared/contract/publication-review-copy.ts + /shared/publication-limits.ts sparse-checkout-cone-mode: false ref: dev - name: Vitest @@ -121,6 +127,7 @@ jobs: NEMAR_CLI_ACCOUNT_COPY: nemar-cli-contract/shared/contract/account-copy.ts NEMAR_CLI_DOCS_CONTRACT: nemar-cli-contract/shared/contract/docs-auth.ts NEMAR_CLI_PRIVATE_SITE_CONTRACT: nemar-cli-contract/shared/contract/private-site.ts + NEMAR_CLI_REVIEW_COPY: nemar-cli-contract/shared/contract/publication-review-copy.ts build: name: build diff --git a/CHANGELOG.md b/CHANGELOG.md index fdfdc6b..ae1f2e6 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -13,6 +13,24 @@ The release pull request from `staging` to `main` moves those entries under the ## [Unreleased] +### Added + +- **The admin publication page shows the review and the identifier screen** (#459, ADR 0027; nemar-cli#1731). + Each pending, approving or blocked request shows a review chip and a screen chip from the list. + A review panel, fetched when an administrator opens it, shows the reasons in the API's own `statement` and neutral `cure` (never the terminal's sentences, whose `cure` carries commands), the identifier screen, and the model's summary and notes under a label that says the model wrote them. + Approve is absent for a reason no override clears, needs an explicit confirmation to approve over a non-pass, and needs a recorded reason of 10 to 500 characters when the identifier screen found something to read. + Every Approve and Deny now names the request it was for, and each refusal the API gives is shown in plain words. + A review can be started from the panel when the API offers one. + There is no fallback to the old click that sent nothing: if the review detail cannot be read, the page says so and withholds Approve, and if the API predates the page or sends an answer the page cannot trust, it says it is out of step. + An approve that gets no answer, or only the proxy's own 502, says the approval may have started and asks for a refresh. + A 401 or 403 for a signed-out or not-allowed account shows the page's own "sign in again" sentence, never the API's, because an inactive account's refusal can name a terminal command. + The one exception is the origin guard's 403 (code `origin_not_allowed`), whose sentence names no terminal and is printed as the API words it. + A result for a review panel that was closed meanwhile appears in the page's error region instead of a hidden panel. + This needs the API side (nemar-cli#1731), which must be deployed to the API before the website: an older API sends no `statement`, so the page withholds the panel and Approve and says it is out of step. +- **A request blocked by the identifier screen has its own badge** (#459). + The seventh block reason, `identifier_screen_findings`, reads "Identifiers found" instead of the neutral "Blocked". + The badge also shows on the depositor's dashboard card. + ### Changed - **Account copy matches nemar-cli `dev` again** (#464). diff --git a/src/components/ConfirmDialog.astro b/src/components/ConfirmDialog.astro index e6b5c35..2ad572b 100644 --- a/src/components/ConfirmDialog.astro +++ b/src/components/ConfirmDialog.astro @@ -205,6 +205,9 @@ const bodyId = `${dialogId}-body`; if (e.detail?.dialogId !== id) return; if (verifyInput) verifyInput.value = ""; syncSubmitEnabled(); + // An Escape close leaves returnValue as the last confirm set it, so a stale "confirm" would + // fire the confirm event on the next Escape. Start every opening from nothing. + dialog.returnValue = ""; dialog.showModal(); (verifyInput ?? cancelBtn)?.focus(); }); @@ -226,6 +229,7 @@ const bodyId = `${dialogId}-body`; ); } } + dialog.returnValue = ""; if (verifyInput) verifyInput.value = ""; syncSubmitEnabled(); }); diff --git a/src/components/PublicationRequestRow.astro b/src/components/PublicationRequestRow.astro index c39e358..06aac79 100644 --- a/src/components/PublicationRequestRow.astro +++ b/src/components/PublicationRequestRow.astro @@ -3,6 +3,8 @@ import { type PublicationRequest, deriveAdminBadgeState } from "../lib/admin-api import { WEB_PUBLISH_APPROVE_ENABLED } from "../lib/flags"; import { formatRelativeTime, parseBackendTimestamp } from "../lib/format"; import { rowView } from "../lib/publication-queue"; +import { ACTIVE_STATUSES, rowChips } from "../lib/publication-review"; +import { titleIdOf, toggleLabels } from "../lib/publication-review-panel"; import PublishStateBadge from "./PublishStateBadge.astro"; interface Props { @@ -43,6 +45,15 @@ const decided = // The username is nullable on the backend; fall back to the email so the line // is never blank. const requester = request.requested_by_username || request.requested_by_email; +// The review chips and the review panel exist only for a request the detail route answers for: an +// active one. The chips print the API's own headlines and decide nothing; the panel is filled by +// the page script when an administrator opens it, so its `data-` attributes are all it is given. +const reviewable = ACTIVE_STATUSES.includes(request.status); +const chips = rowChips(request); +const panelId = `pubreq-review-${datasetId}`; +// The toggle's word and accessible name: the word is constant and the name starts with it; every +// row's says the same, so the id tells them apart. A stylesheet draws the open state. +const toggle = toggleLabels(false, datasetId); ---
@@ -52,6 +63,21 @@ const requester = request.requested_by_username || request.requested_by_email; + {(chips.review || chips.screen) && ( +
+ {chips.review && ( + + {chips.review.text} + + )} + {chips.screen && ( + + {chips.screen.text} + + )} +
+ )} +
Requested by
@@ -120,6 +146,25 @@ const requester = request.requested_by_username || request.requested_by_email;

)} + {reviewable && ( +

+ )} + + {reviewable && ( +