diff --git a/.claude/rules/node-standard.md b/.claude/rules/node-standard.md new file mode 100644 index 00000000..d24dc520 --- /dev/null +++ b/.claude/rules/node-standard.md @@ -0,0 +1,139 @@ +--- +paths: + - "internal/domain/composition/**" +--- + +# The Mill Node Standard + +Every `NodeType` registered in `internal/domain/composition` (a +`RegisterNodeType` call, one per node — capture/process/apply/trigger/ +decision/terminal) is reviewed against this checklist before it ships. +Adopted, not invented (CLAUDE.md's Research→Plan→Implement): converged +from the published conformance guidelines three real workflow/extension +platforms enforce on third-party nodes/plugins — +[n8n's community-node verification guidelines](https://docs.n8n.io/integrations/creating-nodes/build/reference/verification-guidelines/), +[n8n's UX guidelines](https://docs.n8n.io/integrations/creating-nodes/build/reference/ux-guidelines/), +[n8n's error-handling guidelines](https://docs.n8n.io/integrations/creating-nodes/build/reference/error-handling/), +[Zapier's app publishing requirements](https://platform.zapier.com/publish/app-publishing-requirements), +and the [Raycast store guidelines](https://developers.raycast.com/basics/prepare-an-extension-for-store) +(full research summary: `docs/goals/0030-node-standard.md` item 1, +2026-08-12). Mill's own three hard constraints (§1.1: no phone-home, no +AI API calls, single binary) rule out anything in those guidelines that +assumes a hosted marketplace or a network-calling extension host — +resolved item by item below, not silently dropped. + +## The 8-item checklist + +| # | Requirement | Enforced by | +|---|---|---| +| 1 | Typed `ConfigField`s, not raw JSON, wherever the shape is expressible | `TestNodeTypes` (`Key`/`Label` non-empty) + `typedfield.Field`'s typed `Type`/`Options`/`Default` — reviewed by eye per new node (no full JSON-schema-vs-typed-field detector exists) | +| 2 | Every `ConfigField` documents itself (`Description` non-empty) | `TestNodeTypes` (nodetypes_test.go) — machine-checked | +| 3 | Declared effect class (`Effect`), never the silently-permissive zero value | `TestNodeTypes` via the closed `pureNodeTypes` allow-list — machine-checked, see below | +| 4 | `Output` names what leaves the step, for every `NodeType` | `TestNodeTypes` — machine-checked, no kind exemption (verified: every registered `NodeType`, `apply-*`/terminal included, already declares one) | +| 5 | ID prefixed by its `Kind`'s naming convention | `TestNodeTypes` via the closed `idPrefixExceptions` allow-list — machine-checked | +| 6 | Fail-safe error semantics: an unevaluable/ambiguous condition counts as the *restrictive* outcome, never silently passes (ruleset's "a rule that cannot evaluate counts as failed"; guardrail's own condition-eval-failure rule) | Reviewed per node at authoring time — see "Error-prefix convention" below for why this stays a review checklist, not a grep-test | +| 7 | Seeded proof at the right layer (a built-in workflow/example exercising the node, or a unit/integration test for pure logic) — `.claude/rules/testing.md`'s layering | `TestBuiltInWorkflows_AllNodesFullyResolvedAndExecutable` + the node's own `*_test.go` | +| 8 | Secrets only via an existing credential-backed entity, never a raw `ConfigField` | Reviewed per node — see "The credential rule" below | + +Items 1/4/5/6/8 were already true of every node in this package before +this standard was written down (`TestNodeTypes` already checked Key/ +Label; `internal/adapters/credential` already write-only; every +existing node already seeded — item 7's own bar, stricter than any of +the three researched platforms, all of which stop at "document an +example," not "ship a runnable one"). Items 2/3/4-verified/5-verified +are what this goal (0030) added as new machine checks; conformance +audit against all of them found and fixed three real gaps (`list-lookup`/ +`list-search` had no declared `Effect`, defaulting to the silently- +permissive zero value despite doing a real local read; `child-workflow` +had the same gap, resolved as explicit `ClassNone` per ADR-0022's own +stated design; `decision-route` had no `Output`) — see the commit that +introduced this file for the fixes. + +## The credential rule (item 8) + +A `ConfigField` never carries a raw secret (an API key, a bearer +token, a client secret) as its value. Every node that needs +authenticated access to something external goes through an existing +credential-backed entity instead — a `Connector`/`HTTPRequest` +(`internal/domain/connector`, `AuthType` dispatched through a +registered `AuthStrategy`, secret resolved via +`internal/adapters/credential`'s `zalando/go-keyring`-backed, **write- +only** storage) or an `MCPServer`. The node's own `ConfigField` only +ever holds that entity's ID (`RefKind: "request"` / +`RefKind: "mcpserver"` — `docs/adr/0009`), resolved server-side at +execution time; `composition` itself never reads a secret out of +`Node.Config` directly. This is why `integration-http`/`mcp-tool-call` +have no "API key" field of their own — the credential lives one layer +down, behind the picker. + +## Effect (item 3) — the priority machine check + +`NodeType.Effect`'s Go zero value (`""`) is silently indistinguishable +from `guardrail.ClassNone` at run time +(`composition.NodeTypeEffect`, `execute.go`), and every class except +`ClassExternal` defaults to **allow, no guardrail gate at all** +(`guardrail.DefaultEffect`, ADR-0022). A node with real I/O left at the +zero value runs ungated by accident, not by anyone's decision — the one +genuinely dangerous gap this standard exists to close. `TestNodeTypes` +enforces this with a **closed allow-list** +(`pureNodeTypes` in `nodetypes_test.go`): a node may only leave `Effect` +unset if its ID is on that list, with an inline reason (an entry-point +trigger/`decision-route` whose `exec` is `nil` and never reaches the +gate at all, or a node that provably touches only the in-memory +`ExecContext`, no I/O). Every other node must declare `Effect` +explicitly in its `RegisterNodeType` call — `ruleset`/`human-review`'s +`Effect: guardrail.ClassNone` written out is the house style, not +`ClassNone`-by-omission. + +## Explicit rejections + +Researched and deliberately NOT adopted, one line each: + +- **n8n's publishing ceremony** (npm package naming/versioning, README/ + changelog requirements, submission review queue) — Mill has no + hosted marketplace; a node ships in the same binary as everything + else (§1.1's single-binary lock), so there is no separate publish + step to gate. +- **CRUD completeness** (n8n/Zapier's expectation that a resource node + expose create/read/update/delete symmetrically) — Mill's nodes are + workflow *steps*, not resource-management SDKs; a node exposes + whatever operation the workflow actually needs, not a full CRUD + surface speculatively. +- **Raycast's no-keychain rule** (the store checklist steers extensions + away from the OS keychain toward Raycast's own encrypted-preferences + storage) — contradicts Mill's own deliberate `go-keyring` design + (SPEC.md, `internal/adapters/credential`), adopted specifically + *because* it's the OS-native secret store; Raycast's constraint comes + from being a hosted extension platform managing many third-party + extensions' secrets centrally, a shape Mill (single binary, single + user, no hosted anything) doesn't have. + +## `NodeType`-level versioning — latent, not built + +Every one of the three researched platforms versions nodes/extensions +independently of the app that hosts them (n8n's node `version` field, +Zapier's app versions, Raycast's extension releases) so an existing +workflow keeps running against the node shape it was authored with +while a newer node version ships. Mill has the identical real need +(changing a `NodeType`'s `ConfigFields` today can silently break a +persisted `Node.Config`) but nothing analogous is built — +`Workflow.Versions`/`PublishedVersion` (ADR-0021) version the +*workflow*, not the `NodeType` definitions it references. Named here so +it isn't rediscovered as a surprise; not built speculatively ahead of a +concrete `NodeType` shape change that needs it (CLAUDE.md's Research→ +Plan→Implement, same discipline `ConfigFieldType`'s own doc comment in +`types.go` already applies to Decision/Parallel's unbuilt field types). + +## Error-prefix convention (item 6/8's sibling — reviewed, not grep-tested) + +Every `nodeExec` function's returned errors are prefixed with that node +type's ID (e.g. `"child-workflow: %w"`, `"list-lookup: %w"` — see any +`*.go` file in this package) so a run's error trail names which step +failed without re-deriving it from context. This is checked at code +review time, not by an automated test: a test that greps this +package's `*.go` source for `return ctx, fmt.Errorf(...)` call sites to +verify a literal ID-prefix convention is exactly the kind of brittle +grep-over-source check that breaks on a harmless refactor (an extracted +helper, a wrapped error, a renamed local) without catching a real +regression. Skipped deliberately, not by oversight — revisit only if +this convention actually regresses in a way code review misses. diff --git a/docs/SPEC.md b/docs/SPEC.md index 7a8e8e67..e5a84f16 100644 --- a/docs/SPEC.md +++ b/docs/SPEC.md @@ -1661,6 +1661,7 @@ Plan step for this as a standing rule. | **Guardrail preview / policy gate** | Approve/deny before a step actually runs | Build (core domain: `internal/domain/guardrail`); durable parking: adopt (DBOS `Send`/`Recv`/`SetEvent`, already adopted §7) | `LOCKED`, built — §8/ADR-0022: effect classes on every NodeType, ambient gate + explicit "Wait for approval" node, Configure → Guardrails authoring + dry-run tester | | **Human review / HITL step** | Pause a run for a person: queue it, take their typed input, resume or stop | Park mechanism: adopt (DBOS Send/Recv, §7). Queue surface + input model: build (thin — composed over ListRuns + pending, not a case-management engine) | `LOCKED`, built — ADR-0023: `human-review` node + the Review queue (sidebar); reviewer input coerces via the same path as the test-input form | | **Ruleset validation** | Validate the payload/attributes flowing through a step against named business rules | Data model: build (JDM's shape reduced — GoRules ZEN is CGO/Rust, disqualified; grule rejected). Evaluation: adopt (`expr-lang`, already adopted) | `LOCKED`, built — ADR-0023: `ruleset` node, fail-safe (unevaluable rule counts as failed), failures named per rule | +| **Node standard** (minimum conformance every `NodeType` is reviewed against) | Adopt a published node/plugin conformance checklist rather than reviewing each new node ad hoc | Adopt the converged checklist (research against n8n's community-node verification/UX/error-handling guidelines, Zapier's publishing requirements, Raycast's store checklist — never invented); build the enforcement itself, since no library has an opinion on Mill's own `NodeType` shape | `LOCKED`, built — goal 0030, `.claude/rules/node-standard.md`: an 8-item checklist, 5 items machine-checked in `TestNodeTypes` (`nodetypes_test.go`) — `ConfigField.Description` non-empty, an explicit `Effect` class (closed `pureNodeTypes` allow-list catches the zero-value-silently-means-ClassNone/allow danger), `Output` non-empty universally, ID prefixed by its `Kind` (closed `idPrefixExceptions` allow-list for pre-pattern IDs); the error-prefix convention stays review-checked, not grep-tested (fragile-test tradeoff, recorded in the rule file). The audit fixed three real gaps: `list-lookup`/`list-search`/`child-workflow` had no declared `Effect` (child-workflow's fix makes ADR-0022's already-decided `ClassNone` explicit rather than accidental; list-lookup/list-search get `ClassRead`, matching capture-file's precedent), and `decision-route` had no `Output`. `NodeType`-level versioning is named as a real, latent gap (independent of `Workflow.Versions`) — not built speculatively ahead of a concrete need | | **Visual composition surface** | Author a DAG, not just a list | Adopt (React Flow / `@xyflow/react`) — built ahead of ADR-0005 B2's original deferral trigger, by explicit decision (see the ADR's Update section) | §3, `CompositionCanvas.tsx`, `UX: PROTOTYPE`. **View vs. edit mode (goal 0022):** a workflow row click opens the canvas READ-ONLY — React Flow interactions inert (`nodesDraggable`/`nodesConnectable`/`deleteKeyCode` off; `elementsSelectable` stays on, so a node's config is still inspectable), no authoring toolbar, `NodeInspector` wrapped in a disabled `
` (cascades to every sub-editor, no per-field prop threading); Run/step-debug/Runs/Versions all work. Edit is the explicit switch (the row pencil, or a canvas Edit button) — same tab, in place, no remount (`mode: 'view'\|'edit'` on the `workflow-edit` WorkTabSpec). Extends ADR-0014's inspect-vs-edit split (built for Integrations) to workflows | **React Flow, checked directly against its actual source/docs (not diff --git a/docs/goals/0030-node-standard.md b/docs/goals/0030-node-standard.md deleted file mode 100644 index 616d0092..00000000 --- a/docs/goals/0030-node-standard.md +++ /dev/null @@ -1,48 +0,0 @@ -# 0030 — Node standard: minimum requirements every NodeType meets - -## Goal -Owner-mandated 2026-08-12: "define what is a standard for all plugins -going forward including minimum requirement schemas... when I looked -into some nodes it hasn't been reviewed against industry standards." -Adopt (never invent) a node/plugin conformance standard — the checklist -every existing and future NodeType is reviewed against — modeled on the -published standards real platforms enforce (n8n's community-node -verification guidelines are the named precedent to research first). - -## Plan -1. [x] Research DONE 2026-08-12 (primary sources: n8n verification/UX/ - error-handling guidelines, Zapier publishing requirements, Raycast - store checklist — full report in session; key verdicts below). - Converged 8-item checklist mapped onto Mill: items 1/4/5/6/8 already - enforced (TestNodeTypes, keyring credentials, seed-per-capability — - stricter than all three platforms; Effect = a machine-enforced - version of n8n's no-unscoped-access rule). NEW machine-checkable - items: (a) ConfigField.Description non-empty; (b) **Effect must be - explicitly set — the zero value silently becomes ClassNone = NO - GUARDRAIL GATE, the one dangerous gap** (priority); (c) every - nodeExec error prefixed with its NodeType ID (already convention, - un-checked); (d) ID prefixed by Kind's prefix (allow-list for - pre-pattern IDs); (e) Output non-empty for non-terminal kinds. - Rejected with reasons: n8n publishing/license ceremony, CRUD - completeness, Zapier marketing copy, Raycast store assets, and - Raycast's no-keychain rule (contradicts Mill's deliberate go-keyring - design). NodeType-level versioning named as real-but-latent — not - built speculatively. -2. [ ] Write the Mill Node Standard (a rules/ file or docs/ page + - ADR): minimum per-NodeType requirements — typed ConfigFields with - descriptions/defaults, declared effect class, Output description, - payload contract documented, error semantics (fail-safe, named - errors with remedies), seeded proof at the right layer, SPEC row, - naming conventions (Kind prefixes, label style), Inspector UX bar - (no raw-JSON-only config where typed fields are expressible). -3. [ ] Conformance audit: every existing NodeType reviewed against the - standard; gaps become checklist items fixed in the same wave or - recorded as explicit debt entries (delivery-discipline rule). -4. [ ] Enforcement where mechanical: extend seedproof-style checks if - any standard item is machine-checkable (e.g. every NodeType has a - nonempty Description + effect class — a Go test over the registry). - -## Acceptance -A written standard citing its precedents; every current NodeType either -conforms or has a named debt entry; a new node's DoR includes the -standard; at least one machine-check enforces the checkable subset. diff --git a/docs/goals/BACKLOG.md b/docs/goals/BACKLOG.md index 0d6d5bc6..0a031793 100644 --- a/docs/goals/BACKLOG.md +++ b/docs/goals/BACKLOG.md @@ -73,10 +73,20 @@ this pipeline and on this code)** the DEV·live badge's Go-liveness blind spot, now having claimed a second scalp (ADR-0035's Consequences note); a third badge state (amber DEV·go-stale) not yet built. -8. [ ] [0030 — Node standard](0030-node-standard.md) — owner-mandated - 2026-08-12: a written, precedent-researched (n8n community-node - review) conformance standard every NodeType is checked against; - not started. +8. [x] [0030 — Node standard](archive/0030-node-standard.md) — + DELIVERED 2026-08-12: `.claude/rules/node-standard.md` (8-item + checklist, citing n8n's community-node/UX/error-handling + guidelines, Zapier's publishing requirements, Raycast's store + checklist; explicit rejections + the credential rule + + NodeType-versioning-is-latent note); `TestNodeTypes` + (nodetypes_test.go) machine-checks 4 of the 8 items (Description + non-empty, explicit Effect via a closed pureNodeTypes allow-list, + universal Output, Kind-ID-prefix via a closed idPrefixExceptions + allow-list) — the error-prefix convention stays review-checked, + not grep-tested (fragility tradeoff recorded in the rule file). + Audit found and fixed 3 real gaps: list-lookup/list-search/ + child-workflow had no declared Effect (silently defaulting to the + permissive zero value), decision-route had no Output. 9. [ ] [0031 — AI node family](0031-ai-node-family.md) — owner-engaged 2026-08-12: the guardrailed AI-node family (n8n/Make/Zapier/ Dify taxonomy convergence), Mill's category-defining capability; diff --git a/docs/goals/archive/0030-node-standard.md b/docs/goals/archive/0030-node-standard.md new file mode 100644 index 00000000..39a03093 --- /dev/null +++ b/docs/goals/archive/0030-node-standard.md @@ -0,0 +1,75 @@ +# 0030 — Node standard: minimum requirements every NodeType meets + +## Goal +Owner-mandated 2026-08-12: "define what is a standard for all plugins +going forward including minimum requirement schemas... when I looked +into some nodes it hasn't been reviewed against industry standards." +Adopt (never invent) a node/plugin conformance standard — the checklist +every existing and future NodeType is reviewed against — modeled on the +published standards real platforms enforce (n8n's community-node +verification guidelines are the named precedent to research first). + +## Plan +1. [x] Research DONE 2026-08-12 (primary sources: n8n verification/UX/ + error-handling guidelines, Zapier publishing requirements, Raycast + store checklist — full report in session; key verdicts below). + Converged 8-item checklist mapped onto Mill: items 1/4/5/6/8 already + enforced (TestNodeTypes, keyring credentials, seed-per-capability — + stricter than all three platforms; Effect = a machine-enforced + version of n8n's no-unscoped-access rule). NEW machine-checkable + items: (a) ConfigField.Description non-empty; (b) **Effect must be + explicitly set — the zero value silently becomes ClassNone = NO + GUARDRAIL GATE, the one dangerous gap** (priority); (c) every + nodeExec error prefixed with its NodeType ID (already convention, + un-checked); (d) ID prefixed by Kind's prefix (allow-list for + pre-pattern IDs); (e) Output non-empty for non-terminal kinds. + Rejected with reasons: n8n publishing/license ceremony, CRUD + completeness, Zapier marketing copy, Raycast store assets, and + Raycast's no-keychain rule (contradicts Mill's deliberate go-keyring + design). NodeType-level versioning named as real-but-latent — not + built speculatively. +2. [x] Write the Mill Node Standard: `.claude/rules/node-standard.md` + (paths-scoped to `internal/domain/composition/**`) — the 8-item + checklist table (each marked enforced-by-what), the credential rule, + the explicit rejections (n8n publishing ceremony, CRUD completeness, + Raycast no-keychain), NodeType-level versioning named latent-not- + built, and why the error-prefix convention stays review-checked + rather than grep-tested. `docs/SPEC.md`'s §3.3 capability-map table + gets a `Node standard` row, `LOCKED`. +3. [x] Conformance audit: every registered NodeType reviewed against + items (a)/(b)/(c)/(d) via the new `TestNodeTypes` checks (run before + finalizing any allow-list, per the goal's own instruction). Found and + fixed 3 real gaps: `list-lookup`/`list-search` had no declared + `Effect` despite doing a real local List read (fixed to + `ClassRead`, matching `capture-file`'s precedent); `child-workflow` + had no declared `Effect` either — fixed to an explicit `ClassNone` + (ADR-0022 already named this as the correct class, just never + written down); `decision-route` had no `Output` (fixed — it was the + only NodeType missing one, so item (d) needs no Kind exemption at + all, universal). No other violations found. +4. [x] Enforcement where mechanical: `TestNodeTypes` + (`nodetypes_test.go`) now checks (a) `ConfigField.Description` + non-empty, (b) `Effect` explicit via a closed `pureNodeTypes` + allow-list, (c) ID prefixed by its `Kind` via a closed + `idPrefixExceptions` allow-list (verified against the actual + registry: `child-workflow`, `code-execution`, `human-review`, + `ruleset`, `integration-http`, `list-lookup`, `list-search`, + `mcp-tool-call`, plus `decision-outcome` — a `KindTerminal` node + named for its pre-ADR-0027 "Decision" identity, found by running the + check rather than assumed), (d) `Output` non-empty universally. Item + (e) (error-prefix convention) is documented as review-checked, not + machine-checked — a grep-over-source test was judged too brittle + (recorded in the rule file, not silently skipped). + +## Acceptance +A written standard citing its precedents; every current NodeType either +conforms or has a named debt entry; a new node's DoR includes the +standard; at least one machine-check enforces the checkable subset. + +**Met 2026-08-12.** `.claude/rules/node-standard.md` cites all three +platforms' guideline URLs; `TestNodeTypes` machine-checks 4 of the 8 +items with zero outstanding violations (3 real gaps found were fixed +in the same change, not deferred as debt entries); the rule file's +`paths` frontmatter means a future node-authoring session in +`internal/domain/composition/**` gets the standard loaded automatically +as part of its DoR. diff --git a/internal/domain/composition/childworkflow.go b/internal/domain/composition/childworkflow.go index 0740701d..504ededa 100644 --- a/internal/domain/composition/childworkflow.go +++ b/internal/domain/composition/childworkflow.go @@ -4,6 +4,8 @@ import ( "fmt" "strconv" "strings" + + "github.com/alicoding/mill/internal/domain/guardrail" ) // runChildWorkflowFn defaults to erroring so a child-workflow node run @@ -36,6 +38,11 @@ func init() { RegisterNodeType(NodeType{ ID: "child-workflow", Kind: KindProcess, Label: "Run another workflow", + // Effect is explicitly ClassNone (never left at the zero value) -- + // docs/adr/0022: "Child workflows carry no class of their own + // (none): the child's own steps are gated inside the child's own + // run -- gating the invocation too would double-charge." + Effect: guardrail.ClassNone, Output: "the child workflow's result", Description: "Runs another of your workflows as a step and uses its result as this workflow's payload. The other workflow must start with the \"callable by another workflow\" trigger (docs/adr/0010) -- that's what marks it as safe to be invoked from here rather than by a hotkey or schedule of its own.", ConfigFields: []ConfigField{ diff --git a/internal/domain/composition/decision.go b/internal/domain/composition/decision.go index dec56595..6d615fc5 100644 --- a/internal/domain/composition/decision.go +++ b/internal/domain/composition/decision.go @@ -14,6 +14,7 @@ func init() { RegisterNodeType(NodeType{ ID: "decision-route", Kind: KindDecision, Label: "Branch: route", + Output: "payload and Attributes unchanged; routes to the matching outgoing edge", Description: "Routes to one of several next steps based on a rule evaluated against this workflow's Attributes. A pure routing point -- its conditions live on its outgoing edges (SPEC.md §3.5), not here.", }, nil) } diff --git a/internal/domain/composition/listlookup.go b/internal/domain/composition/listlookup.go index 6b992f14..38193d27 100644 --- a/internal/domain/composition/listlookup.go +++ b/internal/domain/composition/listlookup.go @@ -3,6 +3,7 @@ package composition import ( "fmt" + "github.com/alicoding/mill/internal/domain/guardrail" "github.com/alicoding/mill/internal/domain/list" "github.com/alicoding/mill/internal/domain/typedfield" ) @@ -43,6 +44,12 @@ func init() { RegisterNodeType(NodeType{ ID: "list-lookup", Kind: KindProcess, Label: "List: lookup", + // ClassRead: resolves a Configure-authored List's persisted + // entries (lookupListFn) -- the same "reads state outside this + // workflow's own payload/Attributes" classification capture-file + // declares for a local filesystem read, not left at the zero + // value (docs/goals/0030-node-standard.md item b). + Effect: guardrail.ClassRead, Output: "payload unchanged; match → attribute", Description: "Looks up an Attributes value in a Configure-authored List and writes the matched entry back into Attributes. listId is FieldText for the same reason integration-http's requestId is above -- Lists are runtime, Configure-authored data (the Inspector renders a live picker for it, RefKind, docs/adr/0009).", ConfigFields: []ConfigField{ diff --git a/internal/domain/composition/listsearch.go b/internal/domain/composition/listsearch.go index 1d291bf2..4b140e43 100644 --- a/internal/domain/composition/listsearch.go +++ b/internal/domain/composition/listsearch.go @@ -5,6 +5,7 @@ import ( "fmt" "github.com/alicoding/mill/internal/adapters/fuzzymatch" + "github.com/alicoding/mill/internal/domain/guardrail" "github.com/alicoding/mill/internal/domain/list" ) @@ -39,6 +40,10 @@ func init() { RegisterNodeType(NodeType{ ID: "list-search", Kind: KindProcess, Label: "List: search", + // ClassRead: same classification as list-lookup above -- reads a + // Configure-authored List's persisted rows, not left at the zero + // value (docs/goals/0030-node-standard.md item b). + Effect: guardrail.ClassRead, Output: "payload unchanged; matches -> a typed Object attribute " + "({results, matched, first_match, match_count, list_id})", Description: "Searches a Configure-authored List's rows against one or more match parameters " + diff --git a/internal/domain/composition/nodetypes_test.go b/internal/domain/composition/nodetypes_test.go index fcddf592..5496e4af 100644 --- a/internal/domain/composition/nodetypes_test.go +++ b/internal/domain/composition/nodetypes_test.go @@ -1,9 +1,79 @@ package composition import ( + "strings" "testing" ) +// pureNodeTypes is the CLOSED allow-list of NodeType IDs permitted to +// leave Effect at its Go zero value ("") -- docs/goals/0030-node- +// standard.md item (b), the standard's priority machine check. The +// zero value is silently indistinguishable from guardrail.ClassNone at +// run time (NodeTypeEffect, execute.go: `entry.nodeType.Effect == ""` +// resolves to ClassNone), and ClassNone -- like every class except +// ClassExternal -- defaults to ALLOW with no guardrail gate at all +// (guardrail.DefaultEffect). A node type with real I/O left at the zero +// value would silently run ungated; this list exists so that can only +// happen for an ID a human explicitly reviewed and named here, never by +// omission. New node types MUST declare Effect explicitly (ruleset and +// human-review are the house style: `Effect: guardrail.ClassNone` +// written out, not left blank) -- add to this list ONLY for a node +// verified to have no I/O of its own, with the reason recorded inline. +var pureNodeTypes = map[string]string{ + // Trigger kinds and decision-route register with exec: nil + // (registry.go) -- ExecuteWorkflow's own Kind check skips them + // structurally before the guardrail gate is ever reached + // (execute.go), so no Effect class is meaningful for any of them. + "trigger-manual": "exec=nil entry point, never reaches the guardrail gate", + "trigger-hotkey": "exec=nil entry point, never reaches the guardrail gate", + "trigger-schedule": "exec=nil entry point, never reaches the guardrail gate", + "trigger-clipboard-watch": "exec=nil entry point, never reaches the guardrail gate", + "trigger-filesystem-watch": "exec=nil entry point, never reaches the guardrail gate", + "trigger-callable": "exec=nil entry point, never reaches the guardrail gate", + "trigger-system-event": "exec=nil entry point, never reaches the guardrail gate", + "decision-route": "exec=nil, pure routing -- conditions live on its outgoing edges", + // Genuine no-I/O transforms/reads: operate only on the in-memory + // ExecContext (Payload/Attributes) already threaded through, no + // external/local/read effect of any kind. + "capture-attribute": "reads ExecContext.Attributes in-memory, no I/O", + "process-extract-html": "pure string/DOM transform on the in-memory payload, no I/O", + "process-inject-text": "pure string transform on the in-memory payload, no I/O", + "process-html-to-markdown": "pure string transform on the in-memory payload, no I/O", +} + +// kindIDPrefix is the Kind -> required ID-prefix convention item (c) +// enforces (docs/goals/0030-node-standard.md). Every NEW NodeType's ID +// must start with its Kind's prefix; idPrefixExceptions below is the +// CLOSED list of IDs that predate this convention. +var kindIDPrefix = map[NodeKind]string{ + KindTrigger: "trigger-", + KindCapture: "capture-", + KindProcess: "process-", + KindApply: "apply-", + KindDecision: "decision-", + KindTerminal: "terminal-", +} + +// idPrefixExceptions is the CLOSED allow-list of pre-pattern IDs that +// don't follow kindIDPrefix -- every one of them shipped before this +// standard existed. This list must never grow; a new node type's ID +// has to follow its Kind's prefix. +var idPrefixExceptions = map[string]bool{ + "child-workflow": true, // KindProcess + "code-execution": true, // KindProcess + "human-review": true, // KindProcess + "ruleset": true, // KindProcess + "integration-http": true, // KindProcess + "list-lookup": true, // KindProcess + "list-search": true, // KindProcess + "mcp-tool-call": true, // KindProcess + "decision-outcome": true, // KindTerminal, named for its pre-ADR-0027 "Decision" identity +} + +// TestNodeTypes is the Mill Node Standard's machine-checkable subset +// (docs/goals/0030-node-standard.md, .claude/rules/node-standard.md): +// every registered NodeType is reviewed against these checks, not just +// spot-checked by eye. func TestNodeTypes(t *testing.T) { types := NodeTypes() if len(types) == 0 { @@ -18,10 +88,48 @@ func TestNodeTypes(t *testing.T) { t.Errorf("duplicate node type ID %q", nt.ID) } seen[nt.ID] = true + + // (a) Every ConfigField carries a Description -- the standard's + // "typed fields document themselves" requirement; an + // undocumented field forces guessing at authoring time. for _, f := range nt.ConfigFields { if f.Key == "" || f.Label == "" { t.Errorf("node type %q has a config field with an empty Key/Label: %+v", nt.ID, f) } + if f.Description == "" { + t.Errorf("node type %q config field %q has an empty Description (standard item a)", nt.ID, f.Key) + } + } + + // (b) Effect must be explicit, not the silently-permissive zero + // value -- see pureNodeTypes' own doc comment for the danger. + if nt.Effect == "" { + if _, ok := pureNodeTypes[nt.ID]; !ok { + t.Errorf("node type %q has no explicit Effect class (standard item b): "+ + "the zero value silently resolves to ClassNone/allow-with-no-guardrail-gate "+ + "(NodeTypeEffect, execute.go) -- either declare Effect explicitly in its "+ + "RegisterNodeType call, or add its ID to pureNodeTypes in nodetypes_test.go "+ + "with a reason, if it genuinely performs no I/O", nt.ID) + } + } + + // (c) ID is prefixed by its Kind's naming convention, unless + // it's a named pre-pattern exception. + if prefix, ok := kindIDPrefix[nt.Kind]; ok && !strings.HasPrefix(nt.ID, prefix) { + if !idPrefixExceptions[nt.ID] { + t.Errorf("node type %q (Kind %q) doesn't start with the Kind's %q prefix (standard item c) "+ + "and isn't in idPrefixExceptions", nt.ID, nt.Kind, prefix) + } + } + + // (d) Output is non-empty for every NodeType -- required + // universally (every currently-registered NodeType, including + // every terminal/apply kind, already declares one; no kind gets + // a free pass). + if nt.Output == "" { + t.Errorf("node type %q has an empty Output (standard item d): "+ + "name what payload/Attributes state leaves this step, even if it's "+ + "\"payload unchanged\" (see ruleset/decision-route)", nt.ID) } } }