Skip to content

Commit c2a1134

Browse files
os-zhuangclaude
andauthored
docs(automation): 屏幕流 resume 契约与审批越权标记的文档 + 两份 changeset(#4502 跟进) (#4517)
* docs(changeset): changesets for the stranded-request inspection and the harness store (#4469, #4470) Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017gEHJN2NFpS9VMeURvakgD * docs(automation): document the screen resume contract and the override marker (#4477, #4466, #4469) flows.mdx — a new subsection on the durable-pause seam, beside the existing `$`-namespace rule it mirrors: a `screen` node's `config.fields` is a contract, `resume` enforces it server-side with 400 `INVALID_SCREEN_INPUT`, `visibleWhen` is evaluated against the submitted values so a hidden field's `required` never fires, the refusal precedes consuming the suspension, and the three shapes that declare no contract keep the pass-through. The pausing-node table's `screen` row now says the inputs are validated rather than merely posted. approvals.mdx — the admin-override callout now states the rule it actually implements ("the actor is an admin", not "the slate is unstaffed"), which is why `sys_approval_action.via_override` records which door the decision came through, including why an admin who IS an approver records `false` and why a legacy row's absent value reads as "not recorded". The dead-run callout gains the terminal request shape its pending-only scan cannot see, and why the new inspection reports rather than rewrites. content/docs/references/ is generated and untouched. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017gEHJN2NFpS9VMeURvakgD * docs: point the "filed separately" references at #4518 Both the durable-suspend proof's KNOWN GAP note and the `@objectstack/verify` changeset told the reader the harness persistence gap was "filed separately" without naming it — a pointer to nothing, which is the shape of stale record this branch exists to avoid. It is #4518. Comment/changeset text only; no behaviour change. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017gEHJN2NFpS9VMeURvakgD --------- Co-authored-by: Claude <noreply@anthropic.com>
1 parent e5e7ee0 commit c2a1134

5 files changed

Lines changed: 159 additions & 2 deletions

File tree

Lines changed: 48 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,48 @@
1+
---
2+
"@objectstack/plugin-approvals": patch
3+
---
4+
5+
fix(approvals): find the zombie requests nothing was looking at (#4469)
6+
7+
#4460 stopped new zombies being produced; the rows already stuck had no mechanism
8+
to find or release them. The failure shape (#4420) is a request flipped to
9+
`approved` / `rejected` / `returned` whose `flow_run_id` points at a run that no
10+
longer exists — the decision landed, the flow never moved. Any deployment on
11+
17.0.0-rc.1 that hit the wiring hole and crossed a restart mid-approval can be
12+
carrying these rows.
13+
14+
`releaseDeadRunRequests` could not see them, and the reason is worth stating
15+
plainly: it scans `status: 'pending'`, and the very step that zombifies a request
16+
is the one that takes it OUT of `pending`. The act of breaking it removed it from
17+
the only sweeper's field of view — a large part of why this class of failure
18+
stayed silent. It could not have answered the question even if it had looked: its
19+
liveness oracle is `getRun`, which reads the execution LOG and returns `null` for
20+
a perfectly ALIVE suspended run after a restart. It treats `null` as alive
21+
(conservative, and correct for what it does) — which is exactly why it has no way
22+
to say "this run is really gone".
23+
24+
Adds `ApprovalService.inspectStrandedRequests()`, which uses BOTH oracles and
25+
reports only rows that fail both:
26+
27+
- `hasSuspendedRun(runId) === false` — the suspension store itself says no live
28+
pause exists. It THROWS when the store cannot be read, and that case is
29+
SKIPPED and counted as `undetermined`, never condemned: an unreadable store
30+
means "unknown", and a storage outage must not be published as a lost run.
31+
- `getRun(runId) == null` — no terminal history row either. A run that merely
32+
finished is not stranded; a request whose run neither waits nor ever completed
33+
is.
34+
35+
**It reports; it never rewrites.** No status is changed and no run is cancelled.
36+
The decision genuinely happened — a human approved or rejected — and silently
37+
rolling it back would make the audit trail disagree with the facts. The report
38+
carries what an operator needs to decide: which requests are stuck at which step,
39+
and what the mirrored status field on the business record still reads (usually
40+
the stale value the user is staring at). Whether to re-run the downstream actions
41+
or re-open the approval is a judgement call this cannot make.
42+
43+
It rides the existing escalation/dead-run sweep clock, so the finding surfaces in
44+
the logs without an operator knowing to go looking for it. `recalled` is
45+
deliberately out of scope: a recall abandons its run on purpose, and reporting
46+
those would bury the real findings under expected ones.
47+
48+
New export: `StrandedApprovalRequest` (the report row shape).
Lines changed: 42 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,42 @@
1+
---
2+
"@objectstack/verify": minor
3+
---
4+
5+
fix(verify): stop the harness pinning `suspendedRunStore: 'memory'` (#4470)
6+
7+
`bootStack` hardcoded `suspendedRunStore: 'memory'` when it registered
8+
`@objectstack/service-automation`. That made the DB-backed suspended-run store
9+
**structurally unreachable** from every dogfood/e2e fixture — not under-tested,
10+
untestable. The coverage map had a clean seam nothing crossed:
11+
12+
- unit tests covered ENGINE-side persistence (`suspended-run-store.test.ts`
13+
drives suspend → restart → resume against a fake table);
14+
- e2e covered the BUSINESS chain (approvals), but single-process and wholly in
15+
memory;
16+
- the ASSEMBLY between them — is `sys_automation_run` registered, is its table
17+
created, is the store actually attached to the engine — was covered by
18+
neither.
19+
20+
#4420 grew in precisely that seam: the store hung off a table that was never
21+
created, every write failed into a `warn` nobody read, the pause reported
22+
success, and the run died at the next restart. #4460 added assembly unit tests;
23+
this makes the e2e half possible.
24+
25+
The harness now boots the plugin's own `'auto'` default — the same wiring
26+
`objectstack dev` / `serve` get — so fixtures exercise the real assembly. Two
27+
new knobs:
28+
29+
- `automation` accepts `{ suspendedRunStore: 'auto' | 'memory' }` as well as
30+
`true`, so a fixture that wants the old in-memory behaviour asks for it
31+
explicitly rather than getting it by default.
32+
- `databaseFile` backs the in-process SQLite database with a file instead of
33+
`:memory:`, so state can outlive a kernel.
34+
35+
Answering the question the issue raised — was `'memory'` pinned for speed or
36+
because persistence could not run there? **Speed/simplicity.** The durable path
37+
works in this harness: the accompanying dogfood proof boots with it, and the
38+
whole existing dogfood suite passes on it unchanged (38 files, 239 tests). Note
39+
`databaseFile` does not yet deliver a true cold boot: a second `bootStack` over
40+
the same file reads a database whose tables exist but whose rows are gone —
41+
ordinary records do not survive it either, so it is a harness/driver persistence
42+
gap rather than anything to do with suspended runs, and it is filed as #4518.

content/docs/automation/approvals.mdx

Lines changed: 25 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -477,6 +477,19 @@ approver, or **recall** it — releasing the lock. An admin decision is
477477
authoritative: it finalizes the node even under `unanimous`/`quorum`/`per_group`,
478478
and is audited under the admin's own id. Prefer a guaranteed-staffed fallback
479479
approver so the set is never empty in the first place.
480+
481+
Note the rule is **"the actor is an admin"**, not "the slate is unstaffed" — so
482+
an admin can also act on a request whose slate *is* properly staffed, bypassing
483+
the people on it. That is why the decision records **which door it came
484+
through**: `sys_approval_action.via_override` is `true` when the actor was
485+
admitted *only* by this privileged path, holding no slot themselves. An admin who
486+
is also a designated approver is approving normally and records `false` — the
487+
flag is about the branch that authorized the call, not about who holds admin
488+
rights. A row written before the column existed carries no value at all, which
489+
reads as *not recorded* rather than as *not an override*. Without it, an
490+
override and an ordinary approval were byte-for-byte identical, and the only
491+
trace that a slate had been bypassed was the designated approver's later
492+
`409 INVALID_STATE` — if they happened to try.
480493
</Callout>
481494

482495
<Callout type="info">
@@ -491,6 +504,18 @@ The sweep only ever acts on a run it can positively confirm is terminal: a
491504
paused run (the normal state of a live approval), an unknown run, or an
492505
unreachable automation engine all count as *alive* and are left untouched. It
493506
frees orphaned records; it never cancels a live approval.
507+
508+
That sweep scans **pending** requests, which leaves one shape outside it: a
509+
request already *decided*`approved`, `rejected` or `returned` — whose run has
510+
since vanished. The decision landed and the flow never moved, and flipping the
511+
request out of `pending` is precisely what removed it from the sweep's view. A
512+
second, **read-only** inspection rides the same clock for those: it reports a
513+
terminal request only when the suspension store says no live pause exists **and**
514+
no terminal run record exists either, skipping (never condemning) any row whose
515+
store could not be read. It deliberately **does not rewrite** them — the decision
516+
really happened, and rolling it back automatically would put the audit trail at
517+
odds with the facts — so it names the stuck requests, their step, and the stale
518+
mirrored status for an operator to act on.
494519
</Callout>
495520

496521
### Progress and notification deep links

content/docs/automation/flows.mdx

Lines changed: 43 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -468,7 +468,7 @@ POST /api/v1/automation/{flow}/runs/{runId}/resume
468468
| Pausing node | Suspends until… | Resumed by |
469469
| :--- | :--- | :--- |
470470
| `approval` | a human decision | the approvals service (`POST /api/v1/approvals/requests/:id/approve\|reject`) — resumes down the matching `approve` / `reject` edge. **Decide through the approvals API**; the resume route above **refuses** an approval pause outright (see below). |
471-
| `screen` | a user submits the form | the UI runner posting the collected `inputs`; a `paused` response carrying the next `screen` chains multi-step wizards under one stable `runId` |
471+
| `screen` | a user submits the form | the UI runner posting the collected `inputs`**validated server-side against the screen's declared `fields`** (see below); a `paused` response carrying the next `screen` chains multi-step wizards under one stable `runId` |
472472
| `wait` (timer) | an ISO-8601 duration elapses | **automatically** — a one-shot job resumes the run; after a cold boot the engine re-arms pending timers from the durable store (overdue timers resume immediately) |
473473
| `wait` (signal) | a named external event | any caller invoking `resume(runId)` |
474474

@@ -513,6 +513,48 @@ key. A reserved name answers **400**, nothing is applied (not even legitimate
513513
keys sent alongside it), and the run stays parked. Ordinary author variables are
514514
unaffected, `$` mid-name (`price$`) included.
515515

516+
### A `screen` resume is checked against the declared fields
517+
518+
A screen node's `config.fields` is a **contract**, not just a rendering hint:
519+
the author declares which keys are collected, which are `required`, and — via
520+
`visibleWhen` — when a field is even asked for. `resume` enforces all of it
521+
server-side, so skipping the dialog and posting to the route directly is not a
522+
way around what the author declared:
523+
524+
```
525+
POST /api/v1/automation/{flow}/runs/{runId}/resume
526+
{ "inputs": { "kind": "escalate" } }
527+
528+
400 Invalid screen input: Screen field "escalation_reason" is required
529+
— declared fields: 'kind', 'escalation_reason'
530+
```
531+
532+
Two conditions are refused, both reported at once and both with
533+
`code: 'INVALID_SCREEN_INPUT'`:
534+
535+
- a **`required` field the caller was actually asked for** is missing (an empty
536+
or blank string counts as missing);
537+
- a key the screen **never declared** was sent.
538+
539+
`visibleWhen` is evaluated against the **submitted values** first, so a hidden
540+
field's `required` never fires — enforcing it would dead-end the run at a field
541+
the user was never shown. A predicate that cannot be evaluated is treated as
542+
hidden (and logged), because the client is the authority on what was rendered.
543+
544+
Like the `$`-namespace rule above, the refusal happens **before** the suspension
545+
is consumed: nothing is applied, the run stays parked, and the corrected
546+
submission still lands.
547+
548+
Three shapes declare no contract and so keep the pass-through — the same way an
549+
action with no `params` is untouched:
550+
551+
- an **object-form** screen (`config.objectName`), whose flat `fields` list is
552+
empty by construction; the client persists the record through the normal write
553+
path, which enforces that object's own `required` fields;
554+
- a **message-only** screen (`waitForInput: true` with no fields);
555+
- `signal.output`, which is the node-*output* namespace of the approval-style
556+
resume envelope rather than the screen's collected-values channel.
557+
516558
Registering a pausing node of your own? Declare `resumeAuthority: 'service'` on
517559
its descriptor when the decision to continue belongs to your service rather
518560
than to whoever holds the run id.

packages/qa/dogfood/test/flow-durable-suspend.dogfood.test.ts

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -32,7 +32,7 @@
3232
// but whose ROWS are gone, so it fails for a reason with nothing to do with
3333
// suspended runs. Ordinary records do not survive it either, which is what
3434
// identifies it as a harness/driver persistence gap rather than a defect in the
35-
// suspended-run store. Filed separately; when it is fixed, the natural next test
35+
// suspended-run store. Filed as #4518; when it is fixed, the natural next test
3636
// here is the one this file was originally written around.
3737

3838
import { describe, it, expect, beforeAll, afterAll } from 'vitest';

0 commit comments

Comments
 (0)