From 11c276a1c185d2e9ae2d48bfc953eb198f998568 Mon Sep 17 00:00:00 2001 From: Seth Wang Date: Sun, 13 Sep 2026 11:15:37 +0800 Subject: [PATCH 1/9] =?UTF-8?q?T-192:=20surface=20=E5=8F=AF=E7=B5=90?= =?UTF-8?q?=E6=A1=88=20/=20=E7=B5=90=E6=A1=88=20/=20=E5=BC=B7=E5=88=B6?= =?UTF-8?q?=E7=B5=90=E6=A1=88=20on=20the=20task=20card,=20and=20make=20for?= =?UTF-8?q?ce-done's=20reason=20optional?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit [why] A task whose every step is reported done does NOT close itself — it settles in `ready_for_done` and stays there until somebody acts, with no timer and nobody chased. That produced two problems the cockpit could not answer: * a ticket parked waiting for a human read exactly like a ticket being worked on — the state was one more word in the status badge and nothing else; * a ticket whose executor had already left (an outsource worker released, a member gone) could not be closed from this screen at all. The server has had `POST /api/tasks/{id}/force-done` since T-182, but the frontend had ZERO product code for it — it appeared only in the generated types. And the server demanded a `reason` on that force (422 on blank). owner ruled otherwise in rc-a92a6252c3bd:「可以不給理由」. [how] Frontend (the bulk): * a 可結案 line under the title of every ready_for_done card — on the COLLAPSED card, because "which tickets are waiting for somebody" is a question about the LIST — carrying a 結案 button (`mark_task_done`). * 強制結案 appended to the 狀態 dropdown, with a second-step confirm whose reason textarea is optional. Appended LAST so both of the owner's existing rulings about that menu's order stay literally true. * the item is gated on `viewerMayForceTaskDone()` (api/index.ts) and HIDDEN, not greyed, for a principal the route floor would refuse. That gate defends nothing — `Gated(principalAdminAgent, …)` is the real door; it only stops the cockpit offering a button that could only ever 403. * a refused close NAMES the status the task is actually in, re-read from the server first, because the card's own copy is what the refusal proves wrong. * a forced close is read back and shown: who forced it, and the reason — or a visible "no reason given". `forced_done_by`/`forced_done_reason` are on `TaskDTO` and NOT on `TaskListItemDTO`, so the card learns them by hydrating the one task; the light list row is left honestly `undefined`. * NO generic set-status entry (ticket DoD) — both closes go through their own named action, and a test refuses any control that would write an arbitrary status. The executor's ordinary close is not widened and no member gains the force. Backend (one place): * `reason` relaxed from required to optional: spec/openapi.json (route summary, the x-mcp description and its legacy descriptor, the DTO description and its `required` array), then `bin/gen-ocapi`, `bin/gen-mcp-catalog` and `npm run gen:api` re-run — no generated file hand-edited. The handler drops `decodeJSONBodyRequired(…, "reason")` and the blank 422; strict decoding (unknown keys → 422) is unchanged. * `forced_done_by` / `forced_done_reason` behave exactly as before: a reason that IS given is still trimmed, stored and served, and `forced_done_by` is stamped on every forced close — that stamp, never the reason, is what tells a forced close from a self-closed one. Claude-Session: https://claude.ai/code/session_01GsKVVAVhQYmEenC1qyNAsY Co-Authored-By: Claude Opus 5 --- frontend/src/api/adapter.ts | 52 +++ frontend/src/api/generated/schema.ts | 10 +- frontend/src/api/http.ts | 27 ++ frontend/src/api/index.ts | 38 ++ frontend/src/api/mappers.ts | 8 + frontend/src/api/mock.ts | 67 +++ .../components/TaskCard.force-done.test.tsx | 416 ++++++++++++++++++ frontend/src/components/TaskCard.tsx | 267 +++++++++++ frontend/src/components/TasksPage.tsx | 11 + frontend/src/components/tasks.css | 91 ++++ frontend/src/hooks/useTasks.ts | 41 ++ frontend/src/i18n/locales/en.ts | 32 ++ frontend/src/i18n/locales/zh.ts | 28 ++ frontend/src/i18n/messageKeys.generated.ts | 12 + server/ocserverd/api_tasks.go | 32 +- server/ocserverd/api_tasks_test.go | 70 ++- .../ocserverd/authz_surface_behavior_test.go | 7 +- server/ocserverd/dal_tasks.go | 6 +- server/ocserverd/message_keys_gen.go | 12 + server/ocserverd/ocapi_gen.go | 8 +- server/ocserverd/routes.go | 2 +- spec/mcp-catalog.json | 5 +- spec/openapi.json | 15 +- 23 files changed, 1213 insertions(+), 44 deletions(-) create mode 100644 frontend/src/components/TaskCard.force-done.test.tsx diff --git a/frontend/src/api/adapter.ts b/frontend/src/api/adapter.ts index ab46e7ed7..c9403f722 100644 --- a/frontend/src/api/adapter.ts +++ b/frontend/src/api/adapter.ts @@ -653,6 +653,26 @@ export interface TaskView { updatedTs: number; /** Epoch when the task closed (done/terminated/duplicated); null while open. */ closedTs: number | null; + /** Who forced this task closed and why (wire `forced_done_by` / + * `forced_done_reason`, T-182 — surfaced on the card by T-192). Non-empty + * ONLY on a task closed through `force_task_done`; a task that closed itself + * through `mark_task_done` carries "" on both, which is exactly how the card + * tells the two apart. + * + * 🔴 THE LIGHT LIST DOES NOT CARRY THEM. `TaskListItemDTO` declares neither + * field (`spec/openapi.json`), so `toTaskListItem` sets neither and they read + * `undefined` on every collapsed row that has not been hydrated. That is NOT + * the same as "" — "" is the server saying "this close was not forced", while + * `undefined` is "this projection does not answer the question", and the card + * must not print 強制結案 information from a projection that cannot deny it. + * Optional for that reason, and the card renders the row only on a truthy + * `forcedDoneBy` (so `undefined` renders nothing, never a blank 強制結案 row). + * + * The reason may be "" on a task that IS force-closed: the owner's ruling + * rc-a92a6252c3bd made it optional server-side. `forcedDoneBy` is the carrier + * of "this was forced"; the reason never was. */ + forcedDoneBy?: string; + forcedDoneReason?: string; progressDone: number; progressTotal: number; steps: TaskStepView[]; @@ -2357,6 +2377,38 @@ export interface Api { * caller refetches (the SSE delta also fans). */ terminateTask(id: string): Promise; + /** + * Close a task as done (`POST /api/tasks/{id}/mark-done` / MCP + * `mark_task_done`) — the action `ready_for_done` waits for. NO BODY. + * + * PRECONDITION: the task must be in `ready_for_done`; anything else is a 409 + * that NAMES the status the task is actually in (ApiError, thrown). The + * cockpit only offers the button from that status, so the 409 is the race + * (an SSE delta moved the task under the open menu), not the normal path — + * and the card surfaces the named status rather than a bare "failed". + * + * 🔴 AUTHZ IS THE EXECUTOR'S, NOT THE OWNER'S. The server admits the task's + * OWN executor here and 403s everyone else, the owner included. This method + * exists on the cockpit's port anyway because the port is the wire's shape, + * not the caller's permissions — see `forceTaskDone` for the owner's door. + */ + markTaskDone(id: string): Promise; + /** + * Force a task closed (`POST /api/tasks/{id}/force-done` / MCP + * `force_task_done`) — the exit for a task nobody is going to close: an + * executor that is gone, or a plan whose remaining steps will never be + * reported. + * + * WHO: the owner and the admin assistant only; every other principal is a + * 403, the task's own executor included (an executor that could force its own + * task would simply have `markTaskDone` with no precondition). + * + * `reason` is OPTIONAL (owner ruling rc-a92a6252c3bd): pass "" and the task + * still closes, with `forcedDoneReason` reading back "". Whatever IS passed + * is trimmed and stored, and `forcedDoneBy` is stamped either way. Any + * non-terminal status is accepted; an already-terminal task is a 409 (throws). + */ + forceTaskDone(id: string, reason: string): Promise; /** * Mark a task duplicated (`POST /api/tasks/{id}/duplicate`), pointing at the * ORIGINAL it duplicates — so whoever spots the duplicate closes it instead of diff --git a/frontend/src/api/generated/schema.ts b/frontend/src/api/generated/schema.ts index d4d870150..c3e47e872 100644 --- a/frontend/src/api/generated/schema.ts +++ b/frontend/src/api/generated/schema.ts @@ -3255,10 +3255,10 @@ export interface paths { get?: never; put?: never; /** - * Close this task as done OVER its own precondition — the exit for a task that is never going to be closed by the agent holding it. Two shapes reach it: one sitting in ``ready_for_done`` whose executor is gone, and one still mid-plan whose remaining steps will never be reported. WHO: the OWNER and the ADMIN ASSISTANT only. Every other principal is a 403, the task's own executor INCLUDED — an executor that can force its own task simply has ``mark_task_done`` without a precondition, and the precondition is the whole point. ``reason`` is REQUIRED and refused blank (422): a forced close is the one close nobody can reconstruct afterwards from the steps, because the steps do not agree that the work is finished. The forcing principal and the reason are recorded on the task and come back on every read as ``forced_done_by`` / ``forced_done_reason``, so a done task always says whether it got there by itself. ANY NON-TERMINAL STATUS IS ACCEPTED. Already terminal is a 409 — this forces the precondition, not the terminal wall. Answers with a bounded receipt (``artifact_count``, ``closed_ts``, ``deps``, ``description_sha256``, ``description_size_chars``, ``duplicate_of``, ``executor_id``, ``executor_kind``, ``lock``, ``progress_done``, ``progress_total``, ``status``, ``task_id``, ``title``), not the task — call ``get_task`` when you need the rest. THE BOUND OUTSOURCE WORKER IS DISMISSED HERE, and here is the only place it happens: the row releases AND the session is reclaimed at once, on all four closes alike. It used to wait for a separate close-out report, which is gone — the close-out now happens while the task is still open, so once a close lands there is nothing the worker is still allowed to do on it. + * Close this task as done OVER its own precondition — the exit for a task that is never going to be closed by the agent holding it. Two shapes reach it: one sitting in ``ready_for_done`` whose executor is gone, and one still mid-plan whose remaining steps will never be reported. WHO: the OWNER and the ADMIN ASSISTANT only. Every other principal is a 403, the task's own executor INCLUDED — an executor that can force its own task simply has ``mark_task_done`` without a precondition, and the precondition is the whole point. ``reason`` is OPTIONAL — omitted, blank or whitespace-only all close the task (owner ruling rc-a92a6252c3bd). A forced close is the one close nobody can reconstruct afterwards from the steps, because the steps do not agree that the work is finished, so a reason is worth giving: it is asked for, not demanded. The forcing principal and the reason are recorded on the task and come back on every read as ``forced_done_by`` / ``forced_done_reason``, so a done task always says whether it got there by itself. ANY NON-TERMINAL STATUS IS ACCEPTED. Already terminal is a 409 — this forces the precondition, not the terminal wall. Answers with a bounded receipt (``artifact_count``, ``closed_ts``, ``deps``, ``description_sha256``, ``description_size_chars``, ``duplicate_of``, ``executor_id``, ``executor_kind``, ``lock``, ``progress_done``, ``progress_total``, ``status``, ``task_id``, ``title``), not the task — call ``get_task`` when you need the rest. THE BOUND OUTSOURCE WORKER IS DISMISSED HERE, and here is the only place it happens: the row releases AND the session is reclaimed at once, on all four closes alike. It used to wait for a separate close-out report, which is gone — the close-out now happens while the task is still open, so once a close lands there is nothing the worker is still allowed to do on it. * @description - Closes any non-terminal task as done, ignoring the `ready_for_done` precondition. * - Callers: the owner and the admin assistant only — the executor is a 403 here. - * - `reason` is required (422 when blank) and is recorded with the forcing principal. + * - `reason` is OPTIONAL (owner ruling rc-a92a6252c3bd); when given it is recorded with the forcing principal. * - Reads carry `forced_done_by` / `forced_done_reason` afterwards. * - Already terminal is a 409. * - Stamps closed_ts and dismisses any bound outsource worker (row released, session reclaimed). @@ -9445,14 +9445,14 @@ export interface components { }; /** * TaskForceDoneDTO - * @description Force this task done (MCP ``force_task_done``), over the ``ready_for_done`` precondition ``mark_task_done`` enforces. OWNER AND ADMIN ASSISTANT ONLY — the executor is a 403 here, because an executor that can force its own task just has ``mark_task_done`` with no precondition. ``reason`` is REQUIRED and a blank one is refused (422): every other close leaves the steps agreeing that the work is finished, and this one does not, so the reason is the only record of why the task ended. It is stored with the forcing principal and read back as ``forced_done_by`` / ``forced_done_reason``. Any non-terminal status is accepted; an already-terminal task is a 409. + * @description Force this task done (MCP ``force_task_done``), over the ``ready_for_done`` precondition ``mark_task_done`` enforces. OWNER AND ADMIN ASSISTANT ONLY — the executor is a 403 here, because an executor that can force its own task just has ``mark_task_done`` with no precondition. ``reason`` is OPTIONAL (owner ruling rc-a92a6252c3bd) — omitted, blank or whitespace-only all close the task and leave ``forced_done_reason`` ''. Every other close leaves the steps agreeing that the work is finished and this one does not, so a reason is the only record of why the task ended: when one IS given it is trimmed and stored with the forcing principal, read back as ``forced_done_by`` / ``forced_done_reason``. ``forced_done_by`` is stamped either way. Any non-terminal status is accepted; an already-terminal task is a 409. */ TaskForceDoneDTO: { /** * Reason - * @description Why this task is being closed without its steps saying so. Required, and a blank or whitespace-only reason is refused. + * @description Why this task is being closed without its steps saying so. OPTIONAL: omit it, or send a blank/whitespace-only string, and the task still closes with ``forced_done_reason`` ''. When given it is trimmed and stored. */ - reason: string; + reason?: string; }; /** * TaskMarkDuplicatedDTO diff --git a/frontend/src/api/http.ts b/frontend/src/api/http.ts index e3672834c..2d3860c46 100644 --- a/frontend/src/api/http.ts +++ b/frontend/src/api/http.ts @@ -1777,6 +1777,33 @@ export const httpApi: Api = { }); }, + async markTaskDone(id: string): Promise { + // POST /api/tasks/{task_id}/mark-done -> TaskWriteReceiptDTO. NO BODY — + // the route declares none. The precondition (ready_for_done) and the authz + // (the task's OWN executor; the owner is a 403 here) are both the server's; + // a refusal arrives as an ApiError through the client middleware and the + // card renders the status the 409 names. + await client.POST("/api/tasks/{task_id}/mark-done", { + params: { path: { task_id: id } }, + }); + }, + + async forceTaskDone(id: string, reason: string): Promise { + // POST /api/tasks/{task_id}/force-done {reason?} -> TaskWriteReceiptDTO. + // + // 🔴 THE KEY IS OMITTED WHEN THE REASON IS BLANK, rather than sent as "". + // Both are accepted today (the server trims and stores ""), and the wire + // now declares `reason` optional — but sending `reason: ""` states that the + // caller HAS a reason and it is the empty string, which is not what an + // empty textarea means. Omitting says "no reason given", which is the thing + // owner ruling rc-a92a6252c3bd made expressible. + const trimmed = reason.trim(); + await client.POST("/api/tasks/{task_id}/force-done", { + params: { path: { task_id: id } }, + body: trimmed === "" ? {} : { reason: trimmed }, + }); + }, + async markTaskDuplicate(id: string, duplicateOf: string): Promise { // POST /api/tasks/{task_id}/mark-duplicated {duplicate_of} -> // TaskWriteReceiptDTO (T-182 renamed the route; the tool is now diff --git a/frontend/src/api/index.ts b/frontend/src/api/index.ts index ac4a0e667..31b57153a 100644 --- a/frontend/src/api/index.ts +++ b/frontend/src/api/index.ts @@ -5,6 +5,7 @@ // changes — that is the entire point of the seam. import type { Api } from "./adapter"; +import { hasToken } from "./auth"; import { mockApi } from "./mock"; import { httpApi } from "./http"; @@ -16,6 +17,43 @@ export const USE_MOCK = import.meta.env.VITE_USE_MOCK !== "false"; export const api: Api = USE_MOCK ? mockApi : httpApi; +/** + * May the principal this SPA is authenticated as force a task closed + * (`POST /api/tasks/{id}/force-done`)? T-192. + * + * 🔴 THIS DEFENDS NOTHING, and reading it as a permission check is the one way + * to get it wrong. The REAL gate is the server's route floor — + * `Gated(principalAdminAgent, …)` in `server/ocserverd/routes.go` — which admits + * the owner and the admin assistant and 403s every other principal, the task's + * OWN executor included. A caller who forces a `true` out of this function still + * gets that 403. What this decides is only whether the cockpit OFFERS the + * control, under one rule: never render a button that could only ever fail. + * + * The body is `USE_MOCK || hasToken()` — the SAME predicate `AuthGate` already + * uses to decide that a session is the owner's — and its honesty rests on a fact + * about this SPA rather than a guess about roles: THE COCKPIT HAS EXACTLY ONE + * PRINCIPAL. `/api/login` mints an OWNER token (`TokenDTO.owner_id`) and every + * gated call rides it; there is no member login, no impersonation, and + * `/api/auth/status` discloses nothing about who is asking (`password_set` / + * `mfa_required`, nothing else). So "this session holds an owner token" IS "this + * session is inside the set the route floor admits". The mock half is not a + * loophole: mock mode never renders the wall at all (`AuthGate`), so a token + * test there would say "not the owner" about the only principal that exists. + * + * The negative arm is therefore real, not decorative: a real-mode page with no + * owner token — a cleared or expired session, the unauthenticated share-link + * surfaces — is a caller the server refuses, and it is not shown the control. + * + * ⚠️ IF A MEMBER-SCOPED COCKPIT EVER EXISTS, THIS IS WRONG RATHER THAN MERELY + * INCOMPLETE: a plain member's token would make `hasToken()` true while the + * route floor still refuses them. Whoever adds that login must replace this body + * with a real principal read. Nothing in the type system would notice, which is + * why the warning is here and not in a ticket. + */ +export function viewerMayForceTaskDone(): boolean { + return USE_MOCK || hasToken(); +} + export type { Api, SseConnectionState, diff --git a/frontend/src/api/mappers.ts b/frontend/src/api/mappers.ts index 7d86ba1d4..fe17984f7 100644 --- a/frontend/src/api/mappers.ts +++ b/frontend/src/api/mappers.ts @@ -718,6 +718,14 @@ export function toTask(w: WireTask): TaskView { createdTs: w.created_ts ?? 0, updatedTs: w.updated_ts ?? 0, closedTs: w.closed_ts, + // Who forced this close and why (T-182 wire, T-192 surface). `?? ""` is + // right HERE and would be a lie in `toTaskListItem`: `TaskDTO` always + // declares both, so an absent value on this response means "not forced", + // while the light list does not declare them at all — see the field docs on + // TaskView. The full task can therefore DENY a forced close; the list row + // can only fail to mention one. + forcedDoneBy: w.forced_done_by ?? "", + forcedDoneReason: w.forced_done_reason ?? "", progressDone: w.progress_done, progressTotal: w.progress_total, steps: (w.steps ?? []) diff --git a/frontend/src/api/mock.ts b/frontend/src/api/mock.ts index 91dafb4a8..c72cd0b64 100644 --- a/frontend/src/api/mock.ts +++ b/frontend/src/api/mock.ts @@ -3880,6 +3880,15 @@ export const mockApi: Api = { // literals, and these arrive through a spread), so destructuring is what // makes the type system enforce this instead of a comment. artifactCount: (stored ?? []).length, + // forced_done_by / forced_done_reason are declared on TaskDTO and NOT on + // TaskListItemDTO (spec/openapi.json), so the light list must not carry + // them — same rule, same reason, as depTasks being dropped in getTask + // below. Undefined rather than "": "" would be the server DENYING a + // forced close, and this projection cannot deny anything. A card that + // rendered 強制結案 from a list row would be reading a field the real + // wire never sends. + forcedDoneBy: undefined, + forcedDoneReason: undefined, })); }, @@ -4005,6 +4014,64 @@ export const mockApi: Api = { // response just stops carrying what nobody may render from it. }, + async markTaskDone(id: string): Promise { + // Mirrors HandleMarkTaskDone: the action ready_for_done waits for. The + // PRECONDITION is the whole point — anything other than ready_for_done is a + // 409 that NAMES the status the task is actually in, so the cockpit can say + // where the task went instead of "the action failed". + // + // ⚠️ THE AUTHZ IS NOT MODELLED, and it is the one that would 403 THIS + // caller: the server admits the task's OWN executor here and refuses the + // owner. The mock has no caller identity (the same limitation terminateTask + // documents), so it answers as the cockpit's token — do NOT read a green + // mock-backed test as evidence that the owner may mark_task_done. + const t = findTask(id); + if (t.status !== "ready_for_done") { + throw mockApiError( + `http 409 for POST /api/tasks/${id}/mark-done`, + 409, + `task '${id}' is not ready for done (${t.status})` + ); + } + t.status = "done"; + t.closedTs = Date.now() / 1000; + t.updatedTs = t.closedTs; + outsourceWorkers = outsourceWorkers.filter((w) => w.taskId !== id); + emitTopic("task"); + emitTopic("outsource_worker"); + }, + + async forceTaskDone(id: string, reason: string): Promise { + // Mirrors HandleForceTaskDone: closes ANY non-terminal task as done, over + // the ready_for_done precondition. An already-terminal task is a 409 — + // this forces the precondition, not the terminal wall. + // + // `reason` is OPTIONAL (owner ruling rc-a92a6252c3bd): blank closes the task + // and leaves forcedDoneReason "". What does NOT depend on the reason is the + // forcedDoneBy stamp — it is written on every forced close, which is what + // makes a forced close distinguishable from a self-closed one at all. + // + // ⚠️ THE OWNER/ADMIN-ONLY FLOOR IS NOT MODELLED HERE either (no caller + // identity). The route floor is the real gate; the cockpit's menu gate is + // only about not offering an impossible button. + const t = findTask(id); + if (TERMINAL_TASK_STATUSES.has(t.status)) { + throw mockApiError( + `http 409 for POST /api/tasks/${id}/force-done`, + 409, + `task '${id}' is already closed (${t.status})` + ); + } + t.status = "done"; + t.closedTs = Date.now() / 1000; + t.updatedTs = t.closedTs; + t.forcedDoneBy = MOCK_OWNER_ID; + t.forcedDoneReason = reason.trim(); + outsourceWorkers = outsourceWorkers.filter((w) => w.taskId !== id); + emitTopic("task"); + emitTopic("outsource_worker"); + }, + async markTaskDuplicate(id: string, duplicateOf: string): Promise { // Mirrors handle_mark_task_duplicate (T-02c9): mark the task a duplicate of // the original and close it. Keeps the depth-1 graph — the target must diff --git a/frontend/src/components/TaskCard.force-done.test.tsx b/frontend/src/components/TaskCard.force-done.test.tsx new file mode 100644 index 000000000..19302458d --- /dev/null +++ b/frontend/src/components/TaskCard.force-done.test.tsx @@ -0,0 +1,416 @@ +// TaskCard — 可結案 / 結案 / 強制結案 (T-192). +// +// WHAT THIS TICKET IS ABOUT. A task whose every step is reported done does NOT +// close itself: it settles in `ready_for_done` and stays there until somebody +// acts. The server has no timer and chases nobody. Before this change the +// cockpit had ZERO product code for either close — `mark_task_done` and +// `force_task_done` existed on the wire and appeared in the frontend only as +// generated types — so a ticket parked waiting for a human read exactly like a +// ticket being worked on, and a ticket whose executor had already left could +// not be closed from this screen at all. +// +// Locked here: +// ① a ready_for_done card SAYS it is waiting, on the COLLAPSED card, and +// offers 結案 (mark_task_done). Any other status offers neither. +// ② owner / admin assistant get 強制結案 in the 狀態 menu; a viewer the route +// floor would refuse does not — the item is absent, not greyed. +// ③ the 強制結案 confirm is a SECOND step (the menu item alone closes +// nothing), and its reason is OPTIONAL: an empty textarea still submits. +// That half is the owner's ruling rc-a92a6252c3bd, which this ticket also +// relaxed server-side. +// ④ what came back is READ BACK and shown: who forced it, and the reason — +// or a visible "no reason given" when there was none. +// ⑤ a refused close NAMES THE STATUS the task is actually in, rather than a +// bare 操作失敗. +// ⑥ 🔴 NO GENERIC SET-STATUS ENTRY (ticket DoD). Both closes go through their +// own named action; a test below refuses any control that would let the +// cockpit write an arbitrary status. +// +// ⚠️ THE GATE IN ② IS NOT A PERMISSION CHECK AND THIS FILE MUST NOT BE READ AS +// PROVING ONE. `Gated(principalAdminAgent, …)` in server/ocserverd/routes.go is +// what refuses a plain member, and it refuses them whatever the cockpit renders. +// What ② pins is only that the cockpit does not OFFER a button that could then +// only ever 403 — measured through the real prop the page threads down, with the +// other arm stated rather than assumed. + +import { describe, it, expect, beforeEach, vi } from "vitest"; +import { render, fireEvent, waitFor, within } from "@testing-library/react"; +import { I18nProvider } from "../i18n"; +import { zh } from "../i18n/locales/zh"; +import { TasksPage } from "./TasksPage"; +import { TaskCard } from "./TaskCard"; +import { __resetMock, __injectMockTask, mockApi } from "../api/mock"; +import type { TaskView } from "../api/adapter"; +import { toggleFilter } from "../test/tasksFilter"; + +let seq = 0; + +function mkTask(over: Partial): TaskView { + seq += 1; + return { + id: `task-${seq}`, + taskNo: `T-${2000 + seq}`, + title: `任務 ${seq}`, + typeKey: "", + description: "", + status: "ready_for_done", + priority: "mid", + executorKind: "staff", + executorId: "mira", + creatorId: "", + dedupeKey: "", + deps: [], + waitingReason: "", + duplicateOf: "", + createdTs: Date.now() / 1000 - 3600, + updatedTs: Date.now() / 1000 - 60, + closedTs: null, + progressDone: 0, + progressTotal: 0, + steps: [], + ...over, + }; +} + +function renderPage() { + return render( + + + + ); +} + +/** Reveal the 已完成 section so a card that just closed STAYS on screen. The + * page hides terminals by default, so without this a forced close makes the + * card vanish and there is nothing left to read the 強制結案 row off. + * + * ⚠️ TWO GESTURES, NOT ONE, and the second is easy to miss: ticking 已完成 in + * the 狀態 filter only makes the page FETCH the closed rows — they land in a + * 已結案 section that is itself COLLAPSED, so the card is in the DOM's mind but + * not on screen. `openClosedSection` is the click that actually shows it, and it + * can only be made once the section exists (i.e. after something closed). */ +function showDone() { + toggleFilter("filter-status", "done"); +} + +/** Expand the (single) card on screen. + * + * 🔴 REQUIRED BEFORE READING THE 強制結案 ROW, and the reason is a WIRE FACT + * rather than a test convenience: `forced_done_by` / `forced_done_reason` are + * declared on `TaskDTO` and NOT on `TaskListItemDTO` (spec/openapi.json), so the + * light list row the closed section renders from cannot carry them. The card + * learns them by hydrating the ONE task on expand — which is the same reason + * the card must not print 「不是強制結案」 from a collapsed row either: it has + * not been told. */ +function expandCard(card: HTMLElement) { + fireEvent.click(card.querySelector(".task-card__title")!); +} + +async function openClosedSection() { + const toggle = await waitFor(() => { + const el = document.querySelector('[data-testid="closed-toggle"]'); + expect(el).toBeTruthy(); + return el as HTMLElement; + }); + if (toggle.getAttribute("aria-expanded") !== "true") fireEvent.click(toggle); +} + +beforeEach(() => { + __resetMock(); + vi.restoreAllMocks(); + window.location.hash = ""; +}); + +describe("① 可結案: the card says it is waiting, and offers 結案", () => { + it("shows the waiting line and the 結案 button on a COLLAPSED ready_for_done card", async () => { + __injectMockTask(mkTask({ title: "等人按結案" })); + const { findByTestId } = renderPage(); + + const card = await findByTestId("task-card"); + // Collapsed — nothing was expanded, and the line is still there. This is + // the whole complaint: you must be able to see it in the LIST. + expect(card.getAttribute("aria-expanded")).toBe("false"); + const banner = within(card).getByTestId("task-ready-done"); + expect(banner.textContent).toContain(zh.tasks.readyForDoneHint); + // Not the raw status identifier, and not a bare status word either: the + // line has to say who is being waited for. + expect(banner.textContent).toContain("負責人"); + expect(within(card).getByTestId("task-mark-done").textContent).toContain( + zh.tasks.markDone + ); + }); + + it("offers neither the line nor 結案 on a status that has not reached the precondition", async () => { + for (const status of ["not_started", "in_progress", "waiting_owner", "waiting_external"]) { + __injectMockTask(mkTask({ title: `不可結案-${status}`, status })); + } + const { findAllByTestId } = renderPage(); + const cards = await findAllByTestId("task-card"); + expect(cards).toHaveLength(4); + for (const card of cards) { + expect(card.querySelector('[data-testid="task-ready-done"]')).toBeNull(); + expect(card.querySelector('[data-testid="task-mark-done"]')).toBeNull(); + } + }); + + it("結案 is a two-step: the button opens a confirm, and only the confirm calls mark_task_done", async () => { + __injectMockTask(mkTask({ title: "兩步結案" })); + const spy = vi.spyOn(mockApi, "markTaskDone"); + const { findByTestId } = renderPage(); + + fireEvent.click(await findByTestId("task-mark-done")); + // Opened, and NOTHING sent yet — a one-click terminal close is exactly the + // thing every other close on this card refuses to be. + expect(await findByTestId("mark-done-confirm")).toBeTruthy(); + expect(spy).not.toHaveBeenCalled(); + + fireEvent.click(await findByTestId("mark-done-confirm-btn")); + await waitFor(() => expect(spy).toHaveBeenCalledTimes(1)); + expect(spy.mock.calls[0][0]).toMatch(/^task-/); + }); +}); + +describe("② 強制結案 is offered to the principals the route floor admits — and to nobody else", () => { + it("owner/admin: the 狀態 menu carries 強制結案, alongside the items that were already there", async () => { + __injectMockTask(mkTask({ title: "可以強制", status: "in_progress" })); + const { findByTestId } = renderPage(); + + fireEvent.click(await findByTestId("task-status")); + const menu = await findByTestId("task-status-options"); + const force = within(menu).getByTestId("task-force-done"); + expect(force.textContent).toContain(zh.tasks.forceDone); + expect(force.hasAttribute("disabled")).toBe(false); + // The items this ticket did not touch are still there, in their ruled + // order: adding a close must not quietly displace 標記重複/終止. + expect(within(menu).getByTestId("task-mark-duplicate")).toBeTruthy(); + expect(within(menu).getByTestId("task-terminate")).toBeTruthy(); + }); + + it("a viewer the server would refuse does not get the item AT ALL — absent, not greyed", async () => { + // The negative arm, stated through the same prop TasksPage threads down. + // Greying would be the wrong shape here and that is the assertion's point: + // a disabled item says "yours, but not now", and for a principal that may + // never force a close that sentence is false. + const noop = async () => {}; + const { findByTestId } = render( + + mkTask({ id })} + onMarkDone={noop} + onForceDone={noop} + canForceDone={false} + /> + + ); + + fireEvent.click(await findByTestId("task-status")); + const menu = await findByTestId("task-status-options"); + expect(menu.querySelector('[data-testid="task-force-done"]')).toBeNull(); + // …and not smuggled in under another name: the menu must not contain the + // word at all. + expect(menu.textContent).not.toContain(zh.tasks.forceDone); + // The member's ORDINARY doors are untouched — this ticket narrows nobody. + expect(within(menu).getByTestId("task-terminate")).toBeTruthy(); + expect(within(menu).getByTestId("task-mark-duplicate")).toBeTruthy(); + }); + + it("a CLOSED card greys 強制結案 like its neighbours — the server 409s it, so it is shown-but-dead", async () => { + __injectMockTask(mkTask({ title: "已經結案了", status: "done", closedTs: 1 })); + const { findByTestId } = renderPage(); + showDone(); + await openClosedSection(); + + fireEvent.click(await findByTestId("task-status")); + const force = within(await findByTestId("task-status-options")).getByTestId( + "task-force-done" + ); + expect(force.hasAttribute("disabled")).toBe(true); + expect(force.getAttribute("aria-disabled")).toBe("true"); + }); +}); + +describe("③ the reason is asked for, not demanded (owner ruling rc-a92a6252c3bd)", () => { + it("submits with the textarea left EMPTY, and sends no reason rather than a blank one", async () => { + __injectMockTask(mkTask({ title: "不給理由", status: "in_progress" })); + const spy = vi.spyOn(mockApi, "forceTaskDone"); + const { findByTestId } = renderPage(); + + fireEvent.click(await findByTestId("task-status")); + fireEvent.click(await findByTestId("task-force-done")); + const dialog = await findByTestId("force-done-confirm"); + const reason = within(dialog).getByTestId("force-done-reason"); + expect((reason as HTMLTextAreaElement).value).toBe(""); + // The control is NOT required and the confirm is NOT disabled — a client + // that re-imposed the requirement would put back exactly what the ruling + // removed, one layer further from where anyone would look for it. + expect(reason.hasAttribute("required")).toBe(false); + const confirm = await findByTestId("force-done-confirm-btn"); + expect(confirm.hasAttribute("disabled")).toBe(false); + + fireEvent.click(confirm); + await waitFor(() => expect(spy).toHaveBeenCalledTimes(1)); + expect(spy.mock.calls[0][1]).toBe(""); + }); + + it("the menu item alone closes nothing — the confirm is a real second step", async () => { + __injectMockTask(mkTask({ title: "二次確認", status: "in_progress" })); + const spy = vi.spyOn(mockApi, "forceTaskDone"); + const { findByTestId, queryByTestId } = renderPage(); + + fireEvent.click(await findByTestId("task-status")); + fireEvent.click(await findByTestId("task-force-done")); + expect(await findByTestId("force-done-confirm")).toBeTruthy(); + expect(spy).not.toHaveBeenCalled(); + + // And cancelling really cancels — the dialog goes, the task does not. + fireEvent.click(within(await findByTestId("force-done-confirm")).getByText( + zh.common.cancel + )); + await waitFor(() => + expect(queryByTestId("force-done-confirm")).toBeNull() + ); + expect(spy).not.toHaveBeenCalled(); + }); +}); + +describe("④ what the server recorded comes back onto the card", () => { + it("a forced close with a reason shows WHO forced it and WHY", async () => { + __injectMockTask(mkTask({ title: "有理由", status: "in_progress" })); + const { findByTestId } = renderPage(); + showDone(); + + fireEvent.click(await findByTestId("task-status")); + fireEvent.click(await findByTestId("task-force-done")); + fireEvent.change(await findByTestId("force-done-reason"), { + target: { value: "負責人已退場,活早就交付了" }, + }); + fireEvent.click(await findByTestId("force-done-confirm-btn")); + await openClosedSection(); + expandCard(await findByTestId("task-card")); + + const row = await findByTestId("task-forced-done"); + expect(row.textContent).toContain(zh.tasks.forcedDoneLabel); + expect(row.textContent).toContain("負責人已退場,活早就交付了"); + // The card also actually moved — the row is not decoration on an open task. + await waitFor(() => + expect( + within(document.body).getByTestId("task-status").textContent?.trim() + ).toBe(zh.tasks.status.done) + ); + }); + + it("a forced close with NO reason says so out loud, rather than showing an empty row", async () => { + __injectMockTask(mkTask({ title: "沒有理由", status: "in_progress" })); + const { findByTestId } = renderPage(); + showDone(); + + fireEvent.click(await findByTestId("task-status")); + fireEvent.click(await findByTestId("task-force-done")); + fireEvent.click(await findByTestId("force-done-confirm-btn")); + await openClosedSection(); + expandCard(await findByTestId("task-card")); + + const reason = await findByTestId("task-forced-done-reason"); + // Absent is rendered AS absent. A row that silently dropped the line would + // read identically to a row that was never rendered, which is the one thing + // an optional field must never be allowed to look like. + expect(reason.textContent).toBe(zh.tasks.forcedDoneNoReason); + }); + + it("a task closed NORMALLY carries no 強制結案 row — the stamp is what tells the two closes apart", async () => { + __injectMockTask(mkTask({ title: "自己結案的" })); + const { findByTestId } = renderPage(); + showDone(); + + fireEvent.click(await findByTestId("task-mark-done")); + fireEvent.click(await findByTestId("mark-done-confirm-btn")); + await openClosedSection(); + // Expanded, so the card HAS the projection that could show a 強制結案 row — + // this is the arm where the absence is evidence rather than ignorance. + expandCard(await findByTestId("task-card")); + await waitFor(() => + expect(document.querySelector(".task-card__workflow, .task-card__meta")).toBeTruthy() + ); + + await waitFor(() => + expect( + within(document.body).getByTestId("task-status").textContent?.trim() + ).toBe(zh.tasks.status.done) + ); + expect(document.querySelector('[data-testid="task-forced-done"]')).toBeNull(); + }); +}); + +describe("⑤ a refused close says WHERE the task actually is", () => { + it("names the status instead of a bare 操作失敗", async () => { + // The race this exists for: the menu was opened on a ready_for_done card, + // and by the time the confirm landed the task had moved. The mock answers + // the same 409 the server does. + __injectMockTask(mkTask({ title: "被拒絕的結案" })); + const { findByTestId } = renderPage(); + + fireEvent.click(await findByTestId("task-mark-done")); + // Move it under the open dialog — now the close cannot succeed. + const id = (await findByTestId("task-card")).getAttribute("data-task-id"); + vi.spyOn(mockApi, "markTaskDone").mockRejectedValue( + Object.assign(new Error("http 409"), { status: 409 }) + ); + vi.spyOn(mockApi, "getTask").mockResolvedValue( + mkTask({ id: id ?? "task-1", status: "waiting_owner" }) + ); + fireEvent.click(await findByTestId("mark-done-confirm-btn")); + + const err = await waitFor(() => { + const el = document.querySelector(".task-card__error"); + expect(el).toBeTruthy(); + return el!; + }); + expect(err.textContent).toContain(zh.tasks.closeStateError); + // 🔴 THE STATUS IT NAMES IS THE FRESHLY READ ONE, not the one the card was + // holding when the click happened — the card's copy is exactly what the + // refusal proves wrong. + expect(err.textContent).toContain(zh.tasks.status.waiting_owner); + expect(err.textContent).not.toContain(zh.tasks.status.ready_for_done); + // And the generic line is NOT what was rendered. + expect(err.textContent).not.toBe(zh.tasks.actionError); + }); +}); + +describe("⑥ no generic set-status entry (ticket DoD)", () => { + it("the 狀態 menu offers only NAMED actions — nothing that writes an arbitrary status", async () => { + __injectMockTask(mkTask({ title: "只有具名動作", status: "in_progress" })); + const { findByTestId } = renderPage(); + + fireEvent.click(await findByTestId("task-status")); + const menu = await findByTestId("task-status-options"); + // Every control in the menu is one of the known named actions. A future + // 「設定狀態」 dropdown, radio group or free status picker would add a + //