Skip to content

fix: save_notice follow-ups: name the array triggerType, pin the persisted approval round trip, classify the node in flow lint - #28805

Merged
objectstack-fleet[bot] merged 5 commits into
mainfrom
claude/project-thread-9kerqb
Nov 1, 2026
Merged

objectstack-fleet[bot] merged 5 commits into
mainfrom
claude/project-thread-9kerqb

Conversation

@objectstack-fleet

Copy link
Copy Markdown
Contributor

domain:server#3 · os-project-manager · session_01FEkQ5omF1nCbQTGgW1nbRA

Fixes #28786

Clause-②: no
Metadata-protocol: none

维护者速读

  • 这是 Save notice from a sandboxed hook or a flow: hook context and flow node set the write answer's notice #28251(save_notice)合并后审查 6562490492 提出的三条 should-fix,只补措辞、测试和 lint 分类,契约、放置判定和线上的 notice 都没变。
  • 数组形式的 triggerType(比如 ['record-after-create', 'record-after-update'])仍然被拒,但拒绝文案现在会照原样引出这个数组,说明这种写法不支持、永远不会绑定,并让作者只写一个触发类型('record-after-write' 同时覆盖新建和更新)。
  • 新增一个集成测试,走真实的 sys_automation_run 持久化往返:审批之前的 save_notice 让新建应答带回了提示;审批决定后从存储恢复的分支上,第二个 save_notice 按名字被拒。实测行为与推断一致,没有发现缺陷。
  • lint 的两张节点表都加入了 save_notice:写在它上面的 condition 会提示无效,放在没有 try_catch 的循环体里会提示可能失败。
  • 没有需要你拍板的事项。

What changes

Spec: the array trigger form is named (save-notice-placement.ts). A save_notice node in a flow whose start writes triggerType as an array was refused with "This flow's start has no record trigger", which misnames the shape. The verdict is unchanged, and the refusal now says what the author wrote:

This flow's start writes triggerType as an array (['record-after-create', 'record-after-update']). That form is not supported and never binds, so no write waits on the run and the notice would reach nobody. Give the start node ONE trigger type: 'record-after-write' fires on both a create and an update.

A string trigger and a missing trigger keep their existing sentence.

Coverage: the persisted approval round trip (plugin-approvals/src/save-notice-approval-round-trip.integration.test.ts). It uses a real kernel: ObjectQLPlugin over SqlDriver (better-sqlite3), AutomationServicePlugin with its default durable store (ObjectStoreSuspendedRunStore over sys_automation_run), and RecordChangeTriggerPlugin. The real ApprovalService runs behind the real approval node. The flow is record-after-create → save_notice → approval → (approve) save_notice → end. The test reads:

Lint: save_notice in the node-type lists (lint-flow-patterns.ts), classified by reading its executor, as the lists' headers ask:

  • INERT_CONDITION_NODE_TYPES: the executor parses message only and never reads condition.
  • FALLIBLE_NODE_TYPES: it returns success: false on data alone. That happens when the run holds no write (a resumed leg, a subflow or map child), when the channel is already closed, or when every hole of the message rendered empty (save-notice-node.ts:67, :94, :104).

Why

These are the three should-fixes of review 6562490492 on #28753.

Verification

  • Red first.
    • Item 1's sentence pin (spec/src/automation/save-notice-node.test.ts) was 1 failed of 18 before the fix and 18/18 after.
    • Item 3's two lint readings (lint/src/lint-flow-patterns.test.ts: inert condition on a save_notice, and save_notice in the fallible-builtin list) were 2 failed before and 245/245 after.
  • Ablation of the coverage pin (item 2). I replaced the executor's no-channel refuseNode(...) with return { success: true }. The round-trip test went red: decide no longer threw, so "expected 'undefined' to contain 'save_notice 'tell_after': this run …'". I restored it and checked it was byte-identical by blob (ea8be53d), and the test was green again. The test resolves @objectstack/service-automation through its built dist/, so I ablated the built file the test runs.
  • Gates. dispatch-gates.mjs --commands derived 84; --ran reconciles 84 run, 0 NOT-MEASURED, 0 UNRUN. Five gates first exited 3 (prerequisite not met): four spec gates read a spec dist older than a later test edit, and check:dual-build-cjs-loads needs every package's dist. I rebuilt them and reran all five: exit 0.
  • Suites. Full vitest run of @objectstack/spec, @objectstack/lint and @objectstack/plugin-approvals: exit 0 each.
  • eslint --no-inline-config over packages/spec, packages/lint and packages/plugins/plugin-approvals exits 0.

Acceptance notes

  • With a save_notice placed after an approval in a record-triggered flow, the parse still accepts the flow, because it cannot see where a run suspends. The approver's decision is recorded, and the resumed run fails at the node. The decision answers the stranded-decision failure, which names the node. This PR pins that reading and does not change it.
  • No ui/** and no hotcrm changes.

Human Minimum

  • 1. Schemas start as Zod; packages/spec holds no runtime logic.
  • 2. Every name I added uses the casing for its kind.
  • 3. No formatter ran; pnpm lint passes.
  • 4. One issue, one branch off an up-to-date main, one pull request.
  • 5. A change that publishes carries a changeset, and both declaration lines are filled in here and in the changeset.
  • 6. packages/spec touched: no generated artifact moves (the change is refusal text only).
  • 7. No hand edits to content/docs/references/, packages/*/CHANGELOG.md or content/docs/releases/.
  • 8. Title, body and comments are in English.
  • The Before Submitting list in CONTRIBUTING.md is done.

Generated by Claude Code

A save_notice before an approval answers the triggering create; the run
suspends into sys_automation_run without the channel's function, and a
save_notice on the leg the decision resumes from storage refuses by name.

Co-Authored-By: Claude <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01FEkQ5omF1nCbQTGgW1nbRA
@github-actions

github-actions Bot commented Nov 1, 2026

Copy link
Copy Markdown
Contributor

📓 Docs Drift Check

4 anchor(s) derived from 2 changed package(s); no hand-written page names any of them, so this run has nothing to list — not a clean bill of health. This check sees only pages that NAME a derived anchor: one that documents this change in prose, or enumerates it in an authoring dialect, names none and stays invisible to it on every run.

What this run could not see
  • the SDK route bridge reached 60 of 216 client-bound route-ledger rows — the other 156 have no registrar path: tail to select them, so pages documenting THEIR client methods cannot appear above, on this or any run. Of those 156: 0 are remediable by widening that discovery convention (an in-repo file declares the path; the convention did not scan it); 55 are structural — on a ledger where NOT ONE row is declared in-repo, so no discovery change reaches them at any price; 101 are undecided (no in-repo declaration, on a ledger that has other in-repo registrars — absence and an unreadable spelling are not distinguishable here). The rows themselves: node scripts/docs-audit/affected-docs.mjs --bridge-coverage
  • a page that states a rule by its inputs shares no identifier with the emitter that implements the rule, so an emitter-only diff cannot list it — not on this run and not on any run. Measured on fix(driver-sql): emit varchar(maxLength) for a text field a declared index keys on #11430: content/docs/protocol/objectql/types.mdx documents the text-family column mapping by the ObjectQL type names it maps FROM (text / textarea / html) while the diff changed createColumn; it went unlisted, and it was the page that diff falsified, in four places. No shared token exists to detect this on, so a rule your change carries has to be re-read by hand in the pages that restate it.
  • a key NAME is not a key, so the hand re-read the line above prescribes can land on the wrong schema. The same spelling is authorable on one governed type and a [REMOVED] tombstone on another for each of active, aria, joins, objects, template, tools and version (censused on [finding] tools is a key on BOTH AgentSchema (tombstoned, dead) and SkillSchema (live, cloud-attested), so a name-based search attributes skill examples to the agent key — it produced a false stop-the-line alarm on PR #19059 #19093 over the liveness ledger's governed types, top-level keys); nothing in a search result distinguishes the two, so a grep hit on a LIVE example reads as evidence about the DEAD key. Measured on fix(spec): the agent.tools liveness row says dead — it claimed live on a key the schema tombstoned #19059: content/docs/ai/agents.mdx was reported as contradicting the agent.tools tombstone over its tools: example at :161, which is inside the defineSkill({ block opened at :155 — the page was already correct. Settle ownership by PARSING the value against both schemas, never by the name: that literal PASSES SkillSchema, and as an AgentSchema it FAILS at tools with the tombstone prescription. ⛔ These names are not the whole class — a key retired through a .strict() guidance map leaves no tombstone in the walked shape and none of them here (tool.category, live as AIToolDefinition.category).

Coarse fallback — 168 page(s) merely mention a changed package (the pre-#9192 predicate, kept for the deliberately-wide backstop): node scripts/docs-audit/affected-docs.mjs --json c064d3a24610508918ccefd1e0a94b6a759448d9 → packageMentionDocs.

Which tree this was computed on

This run read content/docs from 30344b92cd008d8e421e7e27fd22bdf064634113 — the merge of head 6f15ad27f7406f90547af738b90b99797147bbb3 into base c064d3a24610508918ccefd1e0a94b6a759448d9, which is what actions/checkout gives a pull_request run. Not the PR head.

A worktree cut from an older main holds a different content/docs, so re-deriving there can legitimately return a different list — that is a different tree, not a wrong row. To answer on the same tree:

# while this PR is open — GitHub drops the merge commit once it closes
git fetch origin 30344b92cd008d8e421e7e27fd22bdf064634113 && git checkout 30344b92cd008d8e421e7e27fd22bdf064634113
# afterwards, rebuild it from the two parents, which stay fetchable
git fetch origin c064d3a24610508918ccefd1e0a94b6a759448d9 6f15ad27f7406f90547af738b90b99797147bbb3 && git checkout -B drift-repro c064d3a24610508918ccefd1e0a94b6a759448d9 && git merge --no-ff 6f15ad27f7406f90547af738b90b99797147bbb3

node scripts/docs-audit/affected-docs.mjs --json c064d3a24610508918ccefd1e0a94b6a759448d9

⚠️ That checkout carried uncommitted changes, so the commit above does not fully identify what was read.

@objectstack-fleet

Copy link
Copy Markdown
Contributor Author

domain:server#3 · os-project-manager · session_01FEkQ5kE5BrPUPNMMyv6oCH

Contract review

Served-tier: CONTRACT_REVIEW_TIER
Head-sha: 6f15ad27f7406f90547af738b90b99797147bbb3
Local-runs: none
Scope: packages/spec/src/automation/save-notice-placement.ts + packages/spec/src/automation/save-notice-node.test.ts, packages/triggers/trigger-record-change/src/record-change-trigger.ts (triggerTypeToHookEvents, start()'s array-form warn-and-return), packages/services/service-automation/src/plugin-extension-api.ts (deriveTriggerBinding), packages/lint/src/validate-flow-trigger-readiness.ts (the array-form finding), packages/lint/src/lint-flow-patterns.ts (INERT_CONDITION_NODE_TYPES, FALLIBLE_NODE_TYPES and their two use sites) and its test, packages/services/service-automation/src/builtin/save-notice-node.ts, packages/services/service-automation/src/plugin.ts (suspendedRunStore: 'auto'), packages/services/service-automation/src/engine.ts (suspendedRuns is not a read-through cache, #13617), packages/services/service-automation/src/suspended-run-store.ts (ObjectStoreSuspendedRunStore, JSON columns), packages/plugins/plugin-approvals/src/save-notice-approval-round-trip.integration.test.ts, packages/plugins/plugin-approvals/src/approval-service.ts (decide, the stranded exit), packages/types/src/stranded-decision.ts, .changeset/28786-save-notice-follow-ups.md

Inputs read: card #28786 (body; comments 6562950408 triage, 6562970286 claim, 6563740409 os-dev-report); PR #28805 body and 6-file list; the net diff git diff 56fdd563 6f15ad27 (6 files, +265 −5; the branch restarted on main at 56fdd563, five commits, no merge); the check-runs on the head at two reads. Judged on the tree, not the PR body's account.

① Derived judgments

  1. saveNoticePlacementRefusals keeps its verdict and changes only the sentence for an array triggerType. Right. The early return is untouched (typeof trigger === 'string' && SAVE_NOTICE_TRIGGER.test(trigger.trim())), so the accept set is byte-for-byte what main accepts; an array still falls to the refusal, now spelled as the shape the author wrote. The array form is unsupported and never binds (start() in record-change-trigger.ts logs and returns for a joined event that maps to no hook; deriveTriggerBinding routes it there for that purpose; validate-flow-trigger-readiness already flags it), so "not supported and never binds, so no write waits on the run" is a true statement about the consumer. The advice — ONE trigger type, 'record-after-write' for create-or-update — is the trigger's own prescription for the array case and is the right one for a save_notice, whose write is a create or an update. The string case and the missing case keep their previous sentence verbatim. No tracker number in the runtime string (the #3457 cite is in the code comment).
  2. No published surface moves. Right: no export, type, schema shape, JSON Schema or generated artifact changes; the file list carries no api-surface/, authorable-surface/ or manifest file, and none is owed for a refusal-text change.
  3. Lint: save_notice joins INERT_CONDITION_NODE_TYPES and FALLIBLE_NODE_TYPES. Right, both by reading the executor as the lists' headers demand. save-notice-node.ts parses message only through SaveNoticeConfigSchema (a strictObject; condition on the node config would itself be refused at parse) and never reads condition — the word does not occur in the file — so a condition on the node is inert and flow-inert-node-condition (use site at line 1228) is the right finding. The executor returns success: false on data alone at exactly the three lines the row cites (67: no channel; 94: every hole rendered empty; 104: the channel's own refusal), so flow-loop-body-uncontained (use site at line 1763) is the right warning for one in a loop body with no try_catch. Both rows are pinned (inert: 1 finding with the node named; fallible: n12 added to the sweep's expected list).
  4. No governed surface in the file list; head repo is the base repo; 270 changed lines; draft. Governed Surface Queue Guard green.

② Semver level

.changeset/28786-save-notice-follow-ups.md: @objectstack/spec: patch, @objectstack/lint: patch. Both names match their package.json.

  • Clause-②: no on the PR body and the changeset. Right: nothing is accepted or refused differently (①1), no key, value or requirement moves, no (narrowing) arm, so no BREAKING banner, migration or ADR-0087 disposition is owed.
  • Metadata-protocol: none on both. Right: a refusal sentence and lint classifications change nothing an author can write. none must grade every entry patch and move no authorable-surface/ baseline — both hold.
  • @objectstack/plugin-approvals gains only a test and publishes nothing from it: correctly absent from the changeset. @objectstack/service-automation is read, not changed. Check Changeset is success on both runs at the last read.

Clause-②: no

③ Boundary flags

  • Does the round-trip test drive a persisted SuspendedRunStore path? Yes, read in the tree. AutomationServicePlugin with suspendedRunStore unset takes 'auto' (plugin.ts 192, 1834–1851), which builds ObjectStoreSuspendedRunStore over sys_automation_run on the kernel's data engine; the test registers the approvals package's objects, syncSchemas() creates the table under a real SqlDriver, and RecordChangeTriggerPlugin is real. The engine's in-memory suspendedRuns map is "NOT a read-through cache sitting in front of the store" (automation/approvals: 多副本集群下审批流每级节点(除首级)被重复创建 —— approve 后恢复读到滞后一拍的流运行态,同级要批两次(单副本零重复) #13617), so the decision resumes off the row. The test's own evidence confirms it: it reads the paused row's context_json (no saveNotice.set), and the resumed node's refusal is the no-channel sentence (this run holds no record write to answer), which only a context without a set function produces — a leg resumed from the live in-memory channel would have carried the trigger's closed-channel sentence instead. Then the pause row is consumed and the run_ history row is failed with the node's error. The dev's ablation (the no-channel refuseNode replaced by success, test red, restored by blob) is the coverage pin; it was made on service-automation's built dist, the file the test resolves, which is the honest target. Item 2 of the card is met.
  • The stranded-decision RESUME_FAILED pin — right, or a defect to escalate? Right as pinned, and not a defect of this PR or of the save-notice design. stranded-decision.ts records the maintainer-ruled posture: an approval decision finalises first (request row terminal, audit written, mirrored status advanced) and only then resumes the run; when the resume fails, the writes stand, the run is stranded, and decide throws the 500-class RESUME_FAILED carrying strandedDecisionDetails — deliberately, so a recorded decision whose flow never advanced is never reported as "did not happen". A save_notice on the resumed leg is the Save notice from a sandboxed hook or a flow: hook context and flow node set the write answer's notice #28251 design's "refused by name, never dropped silently" (the alternative — the node silently succeeding after the answer was sent — is exactly what Save notice from a sandboxed hook or a flow: hook context and flow node set the write answer's notice #28251 refused), so the test reads two existing contracts meeting and pins what they do. The test asserts finalized: true, decision: 'approve', runId and that the listener heard nothing more, which is the whole of the truthful-throw ruling. Escalated, as a finding to file, not a change here: an author can publish a flow with a save_notice downstream of a suspending node (approval, wait, screen); the parse cannot see suspension, so every decision on such a flow strands its run. That is a metadata-authoring trap in PD#10's sense, with its evidence (this test), and the remedy is a static lint finding (a save_notice reachable only through a suspending node), which the dev noted in out_of_scope_findings but did not file under "new gates default to no". The seat should file it as its own card (p3, packages/lint); a lint finding is a repo gate only if wired as one, and a warn-class finding needs no gate ruling.
  • Deviation 1 (ablation on dist, restored by blob). Accepted; the restore is verified and the working tree is not what is reviewed here.
  • Deviation 2 (terminal row's node_id is the suspension node, not the failing one, so not pinned). Accepted; the error text names the failing node, which is what the test reads.
  • Test wall time. The single it declares a 20 s ceiling; no measured wall is reported. A file over 10 s of wall becomes its lane's card unless it declares a measured budget — the dev should state the measured wall in the report or the file header (a note, not a finding; Test Core on the head is the measurement).
  • open_questions: []. Nothing else is flagged.

④ Design judgment

packages/spec/src/automation/save-notice-placement.ts — against the trigger grammar it cites and the three shapes an author can write:

  • String trigger in the grammar: accepted, unchanged. String trigger outside it ('record-after-delete', 'schedule', 'api'): refused with the previous sentence, unchanged, and save-notice-node.test.ts keeps those pins.
  • Array trigger: refused with the new sentence. Tried to break it: (a) ['record-after-create', 'record-after-update'] — the exact shape the pin writes; the sentence quotes it, says it never binds, and the advice ('record-after-write') is the one token that covers both members; (b) ['record-after-create', 'record-after-delete'] — still refused; the advice is still right for a save_notice (its write is never a delete) and matches the trigger's own warning text; (c) an empty array or a non-string member — the trigger.map over String(t) renders [] or the stringified member; nothing throws inside the FlowSchema parse, which is the property that matters (a throw there would escape safeParse). (d) The array branch is checked before the string branch, so a string can never fall into it. (e) Index anchoring is unchanged from the peer arms.
  • The verdict is unchanged, which is the card's ⛔ line and the Clause-②: no claim; ①1 reads it off the untouched early return.
  • The pin asserts the issue code and path (['custom', ['nodes', 1, 'type']]), the new sentence verbatim, and the absence of the old "no record trigger" phrase.

No finding on the hits file.

Lane remainder, read for ③ and not reviewed here: the integration test and the lint rows hold against the code they cite (①3, ③). One note for the lane seat: the test's header says a store that "started keeping (or re-binding) the channel" is caught here — true for set (the assertion is typeof stored.saveNotice?.set === 'undefined'), while locale surviving is allowed by the test, which is the intended shape.

No must-fix, no should-fix. One escalation (③): file the authoring-trap lint finding as its own card.

Implemented-by: claude/project-thread-9kerqb
Reviewed-by: session_01FEkQ5kE5BrPUPNMMyv6oCH-review-28805

VERDICT: PASS

@objectstack-fleet

Copy link
Copy Markdown
Contributor Author

domain:server#3 · os-project-manager · session_01FEkQ5kE5BrPUPNMMyv6oCH

席位核对 · ACCEPT · PR #28805 · 2026-11-01T21:15Z

Head-sha: 6f15ad27f7406f90547af738b90b99797147bbb3

维护者速读(席位终稿)

核对六项

  1. 形态:draft → main;Fixes #28786,合并关卡正确,与派单 pm-orders/server/order-28786.md 一致。文件 6 个,在认领 6562970286 申报面内。
  2. 前提实测:持久化往返的表现与派单推断一致,没有发现缺陷。
    • 新建请求收到了提示。
    • 挂起后 sys_automation_run 行里没有 saveNotice.set。
    • 恢复后的 save_notice 以"no channel"的语句按名字拒绝。这条语句只有经过持久化才会出现,复核据此确认测试确实走了存储路径。
    • 审批 decide 因此抛出既有的 stranded-decision RESUME_FAILED,dev 把它钉住、未作改动。
    • 消融后变红,已还原。
  3. 门禁:本 head check-runs 57 success、15 skipped(路径未命中)、0 失败或待跑。dev 本地 84 条派生门禁全部 exit 0,0 NOT-MEASURED。
  4. 接受的偏差:无。复核无 must-fix、无 should-fix。
  5. 范围外去向:save_notice 放在 approval/wait/screen 之后能通过发布,但每次审批都会 RESUME_FAILED。已立卡 automation: a save_notice placed after an approval, wait or screen node passes publish and then strands every decision with RESUME_FAILED #28806(p3,扩展现有放置拒绝,不新增门禁),待本 PR 合入后派。Metadata-protocol: none,Skills-docs:见 skills(automation): teach the save notice — the save_notice flow node and ctx.notice in hooks (#28251) #28787。
  6. 落地条款:packages/spec/src 路径 ⇒ CONTRACT_REVIEW_TIER 复核 PASS(6563799258,Head-sha 与现 head 一致),Clause-②: no。semver:spec 与 lint patch,与 changeset 一致(Check Changeset 本 head success)。check-governed-merges --pr 28805:NOT governed,270 行。落地授权,维护者原话:「按照 pm-dispatch 规则复核过的都授权」(2026-10-25T11:35Z)。

@objectstack-fleet
objectstack-fleet Bot marked this pull request as ready for review November 1, 2026 21:17
@objectstack-fleet
objectstack-fleet Bot enabled auto-merge November 1, 2026 21:17
@objectstack-fleet
objectstack-fleet Bot added this pull request to the merge queue Nov 1, 2026
Merged via the queue into main with commit 70943dd Nov 1, 2026
73 checks passed
@objectstack-fleet
objectstack-fleet Bot deleted the claude/project-thread-9kerqb branch November 1, 2026 21:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

documentation Improvements or additions to documentation size/m tests tooling

Projects

None yet

1 participant