fix(spec,service-automation): the wait executor reads its declared contract only (#4045) - #4161
Merged
Merged
Conversation
…ntract only (#4045) `wait` keeps its contract in `waitEventConfig` — a declared, `.describe()`-annotated block on `FlowNodeSchema` that sits in the authorable-field list, reaches the generated reference, and is what the showcase actually authors. Its descriptor publishes no `configSchema`, which is by design rather than the gap it looks like. The executor nevertheless also read six loose `config` keys behind `wec.X ?? loose.X`, two of them (`duration`, `signal`) spellings the spec never declared anywhere. That is the `notify.source` shape #4050 retired: a second de-facto contract announced only by a code comment, so an author who wrote it got a flow that worked forever and was never steered to the declared spelling (PD #12). Nothing in-repo authors it. - New ADR-0087 D2 conversion `flow-node-wait-event-config-lift` lifts config.{eventType,timerDuration,duration,timeoutMs,signalName,signal} onto the declared block, in the executor's own `??` precedence — a declared value wins and its loose counterpart is left shadowed, as `renameConfigKey` treats a shadowed alias. - `eventType` is stamped `'timer'` when the lift would otherwise leave the block without one. Load-bearing, not tidiness: the loader parses the CONVERTED flow (`applyConversionsToFlow` → `FlowSchema.parse`) and `waitEventConfig.eventType` is required once the block exists, so a stored flow carrying only `config: { duration: 'PT1M' }` would have gone from working to failing to load. `'timer'` is the exact default the executor applied to that shape. - The six `?? loose.*` fallbacks are deleted. The surviving `?? 'timer'` is not one: `waitEventConfig` is itself optional, and a node without it is a valid timer wait. Registered in both lists the layer keeps: `CONVERSIONS_BY_MAJOR[17]` and `step17.conversionIds`. The first draft missed the second and the repo's own ratchet caught it — `migrations.test.ts` replays every conversion fixture through the chain, so an unwired conversion fails there rather than shipping inert. Verified at the real seam: the new executor tests author the legacy shape and go through `registerFlow`, which is what applies the conversion. Negative control run by unregistering the conversion — two of the three fail without it; the third passes either way because it pins the executor's half of the precedence, and its name says so. A second negative control pins the `eventType` default: deleting it from the converted output makes `FlowSchema.parse` throw. Regenerated `spec-changes.json` and `docs/protocol-upgrade-guide.md` (the step rationale is a generated-artifact input). All 12 spec gates and 8 root gates pass; spec suite 7097 tests green. Deliberately unchanged and filed as #4158: `waitEventConfig.timeoutMs` is declared a timeout guard but read as a timer duration, and `onTimeout` has zero readers — `wait` has no timeout implementation at all, while the showcase authors `onTimeout: 'continue'`. Implementing or retracting that is a behaviour change, not a contract cleanup. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QoA8AV99Ss1RRkLLAYmDzq
|
The latest updates on your projects. Learn more about Vercel for GitHub. 1 Skipped Deployment
|
github-actions
Bot
added
documentation
Contributor
📓 Docs Drift CheckThis PR changes 2 package(s): 108 hand-written doc(s) reference the affected code and may need an implementation-accuracy re-verification:
|
…-flow-submit-jfpjy4
…#4045) Corrects a factual claim in the previous commit. It said "Nothing in-repo authors it" of the loose `config` back door — that is wrong. The showcase's own `wait_revision` node authored exactly that shape: config: { eventType: 'signal', signalName: 'budget_revision' } so the back door was not hypothetical, and the example that demonstrates `wait` was itself on the spelling this PR retires. It moves to `waitEventConfig`. The conversion already handled this shape correctly — the executor's behaviour is identical either way, and a new test pins the exact combination the showcase hit: the DECLARED key names sitting in the UNDECLARED location, which is what the candidate ordering has to get right (`signalName` before `signal`). It also asserts the converted flow still parses, since the lift creates the required block. Found while checking whether a stuck `objectstack verify` CI step could be caused by this change rather than by the runner — it could not, but the audit that ruled it out is what surfaced the false claim. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QoA8AV99Ss1RRkLLAYmDzq
os-zhuang
marked this pull request as ready for review
July 30, 2026 14:09
os-zhuang
added a commit
that referenced
this pull request
Jul 30, 2026
#4183) `packages/spec` has eight checked-in generated artifacts, each with its own gate, split across two CI jobs that run their gates SEQUENTIALLY. The first stale artifact therefore masks every one behind it: you fix it, push, and learn about the next on the following run. Two pushes on #4040 (`check:docs`, then `check:api-surface`), two more on #4161 (`check:spec-changes`, then `check:upgrade-guide`) — four round trips spent discovering something one local run could have said at once. Every gate runs; a failure does not stop the rest. The summary lists all stale artifacts with the exact `gen:` command for each. `--fix` regenerates ONLY what this run proved stale. Deliberately not a regenerate-everything button: blanket regeneration rewrites artifacts whose staleness you never saw, which is how a real semantic change lands silently inside a mechanical diff. The gate -> generator ledger reconciles against `package.json` on EVERY run, in both directions, rather than behind a `--self-test` flag. An unclassified `check:`/`gen:` script fails the run instead of quietly dropping out of coverage — otherwise the summary would still say "all artifacts up to date" while checking fewer, the exact class of lie this exists to remove. It proved itself by rejecting its own `package.json` entry on the first run. Two things it refuses to be silent about: the `check:api-surface` stale-`dist` trap is printed inline when that gate is the one failing (it reads the built `.d.ts`, so an unbuilt tree reports newly-added exports as breaking removals), and the four source audits it does NOT run are named, so "all up to date" never reads as "everything passed". It also reports that `gen:openapi` and `gen:sbom` have no gate at all. Verified both directions: clean built tree -> all 8 pass; a change injected into a migration step's `rationale` -> `check:upgrade-guide` fails and `--fix` regenerates that artifact and no other. That run also corrected an earlier reading of mine: the two ADR-0087 gates have different inputs — the conversion registry drives `spec-changes.json`, the rationale prose drives `protocol-upgrade-guide.md` — they had only appeared to fail together because #4161 changed both. AGENTS.md's hand-rolled loop over eight hardcoded gate names is replaced by the command; that list could not survive a ninth artifact, and the ledger can. Claude-Session: https://claude.ai/code/session_01QoA8AV99Ss1RRkLLAYmDzq Co-authored-by: Claude <noreply@anthropic.com>
This was referenced Jul 30, 2026
Merged
os-zhuang
added a commit
that referenced
this pull request
Jul 31, 2026
…never got (#4219) The drift comment on #4161 was computed by the mapper bug #4206 fixed: the service-automation change collapsed to `packages/services`, so these four docs were attributed to the wrong package — and a service-automation-only diff would have reported none of them. Re-derived with the fixed mapper and audited against the implementation. flows.mdx carries the #4161 axis itself: a `runAs:'user'` run that resolved no trigger user has its data operations refused. Also documents `node.type` as an open string checked against the live registry (ADR-0018), the script executor as naming a callable with logger-backed `email`/`slack` markers, the `.strict()` shells, and six previously undocumented node keys. implementation-status.mdx gains the real `/api/v1` route prefixes, the memory driver's actual `InMemoryDriver.supports` matrix, and the ADR-0090 D3 `Role` -> `Position` rename — which ratchets the role-word baseline down by one, so that gate-mandated update ships here too. Gates: docs build, check:doc-authoring, check:role-word, check:nul-bytes, check:release-notes, check:skill-examples (198 prose examples). Claude-Session: https://claude.ai/code/session_01J2x8Tie9WT1VgWFifqMKrR Co-authored-by: Claude <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
#4045 的
wait一半。另一半（超时契约压根没实现）单开为 #4158，刻意没有塞进本 PR。先纠正我自己在 #4045 上说错的判断
我一度把
wait报成「没有configSchema，所以设计器配不了它」。那是错的，已在 issue 上更正。wait的契约不在config里，而在FlowNodeSchema.waitEventConfig（flow.zod.ts:199）—— 每个属性都有.describe()、在可授权字段清单里、进了生成参考文档、showcase 也确实这么写。描述符不带configSchema是设计如此。站得住的那一半
执行器另外读了 6 个 loose
config键，其中duration和signal是 spec 从未声明过的拼法。这就是 #4050 退役掉的notify.source形状：一份只由代码注释宣告的第二契约，写了它的作者会得到一个永远能跑、也永远不会被引导到声明拼法的流程（PD #12）。而且它不是假想的 —— 见下方第二处更正。
做法
新增 ADR-0087 D2 转换
flow-node-wait-event-config-lift，把 6 个 loose 键提升到声明块，优先级严格镜像被删掉的??链 —— 已声明的值胜出，其 loose 对应物留在原地（和renameConfigKey对被遮蔽别名的处理一致）。一个会把存量流程写坏的陷阱
engine.ts:1465是applyConversionsToFlow(...)紧接FlowSchema.parse(converted)—— 加载器解析的是转换后的流程。而waitEventConfig.eventType没有.optional()：所以一个只写了
config: { duration: 'PT1M' }的存量流程，若提升后不带eventType，会从能跑变成加载失败。转换因此补上'timer'—— 执行器对这个形状原本的默认值。这不是收尾修饰，是这个改动能不能安全上线的关键。?? 'timer'在执行器里保留了，并在注释里说明它不是同类兜底：waitEventConfig本身可选，没有它的 wait 节点是合法的定时等待。第二处更正：
4450fba第一版 commit 里我写了 "Nothing in-repo authors it"。错的。 showcase 自己的
wait_revision节点写的正是这个形状：所以这道后门不是假想的，而且演示
wait的那个示例本身就在用被退役的拼法。它已改为waitEventConfig。转换本来就正确处理了这个形状（执行器行为两边一致），新增的测试钉住了 showcase 撞上的那个精确组合：声明的键名出现在未声明的位置 —— 这正是候选顺序必须处理对的地方（
signalName先于signal），并断言转换后的流程仍能 parse。这个错误是在排查一个卡住的
objectstack verifyCI 步骤时发现的 —— 排查结论是那不是本改动引起的，但排查过程本身翻出了这句假陈述。两份清单，第二份我第一版漏了
这个层维护两份注册：
CONVERSIONS_BY_MAJOR[17]和step17.conversionIds（链式回放走后者）。我第一版只加了前者，被仓库自己的棘轮抓住 ——migrations.test.ts把每个转换的 fixture 都过一遍链，未接线的转换会在那里失败，而不是静默地失效上线。顺带一提：这本身又是一个「两份手写清单」实例（#3786 那个模式），但这一个有闸门，所以它的表现正如设计。
验证
wait-node.test.tscheck:spec-changes/check:upgrade-guide）pnpm --filter @objectstack/spec exec tsc --noEmit（CI 的 typecheck 门禁）两处负控都做实了：
eventType——FlowSchema.parse抛错，证明那个默认值是承重的。新的执行器测试走
registerFlow（也就是真正施加转换的那个 seam），所以它们证明的是存量源端到端可用，不只是「执行器不再去看 config 了」。生成物
spec-changes.json和docs/protocol-upgrade-guide.md已重生成 —— 迁移步骤的rationale是生成物输入（该字段被声明为「the one place prose is load-bearing」，所以我也扩写了它）。两道闸门都是先报 FAIL 再修好的，不是猜的。一条存量情况，非本 PR 造成
packages/services/service-automation里src/engine.test.ts有 2 个类型错误（缺resumeAuthority，#3951 加该字段时带.default留下的）。我的 diff 里resumeAuthority出现 0 次，也不含engine.test.ts；而且 CI 的 typecheck 只覆盖@objectstack/spec，所以这两个错误既不是我引入的、也不在门禁内。记录在此，未在本 PR 处理。🤖 Generated with Claude Code
https://claude.ai/code/session_01QoA8AV99Ss1RRkLLAYmDzq