Skip to content

feat(sdk): add immutable Flow Tool manifest contracts - #568

Draft
kjgbot wants to merge 8 commits into
mainfrom
feat/flow-tool-manifest-v1
Draft

kjgbot wants to merge 8 commits into
mainfrom
feat/flow-tool-manifest-v1

Conversation

@kjgbot

@kjgbot kjgbot commented Sep 23, 2026 •

Copy link
Copy Markdown
Contributor

Summary

Adds the first immutable Flow Tool manifest contract slice for callable Relayflows. This is intentionally a typed/schema/descriptor foundation; it does not execute flows or provide hosted admission.

Exact scope

  • FlowToolManifestV1 with explicit operator-authored name, description, pinned flow name/version/digest, input schema, result schema, and canonical manifest digest.
  • Bounded JSON Schema 2020-12 profile with fail-closed validation and behavior-free JSON snapshots.
  • Validation of tool input and result without coercion or mutation.
  • Provider-neutral native function descriptor and MCP tools/list descriptor adapters.
  • Explicit documentation of missing execution, auth/admission, async events, cancellation, resume, idempotency, and business-result guarantees.

Exact-head evidence

  • Branch: feat/flow-tool-manifest-v1
  • Head: f37ead4e125a2a9062ae3eea3f5ebbdde6f9afc9
  • Base: main at 46d994c205cb2d45bab30701a76d57650c938d6c
  • Delta at this head is the built-public-API contract test plus acceptance documentation/comments; no runtime execution, admission, workflow, or gate behavior is introduced.
  • Focused manifest/public-API coverage: 54 tests pass.
  • Package build, typecheck:tests, and unchanged surface-package gate pass.
  • Current CI is being revalidated at this head; this draft remains not merge-ready.

Review correction and readiness

The earlier maintainability B1/B2 claim that snapshotJsonValue accepted only two arguments and that FLOW_TOOL_LIMITS were unenforced is stale. At this exact head, packages/sdk/src/json-value.ts accepts the optional limits argument and enforces max depth, node/property, and byte budgets; the manifest/schema callers pass those limits. The current diff correctly does not modify that already-present implementation.

The remaining maintainability notes (diagnostic specificity, Ajv compilation lifecycle, provider adapter naming, and semver predicate readability) are non-blocking review considerations for this typed foundation. Please rerun independent maintainability and structure review against this exact head, including the public API test and corrected limits evidence.

Explicit readiness status

This PR remains DRAFT and NOT merge-ready. The full SDK suite evidence remains red in the declared environment: 3,141 passed, 45 failed, 72 skipped, and 2 errors, attributed to Linux-only isolation, Bun mismatch, and live-analyzer authorization. Execution/admission, hosted async lifecycle, cancellation/resume, idempotency, and end-to-end evidence are intentionally deferred and must be supplied before promotion.

@coderabbitai

coderabbitai Bot commented Sep 23, 2026

Copy link
Copy Markdown

Important

Draft PR not reviewed

Draft PRs are not automatically reviewed by default.

  • Trigger a manual review

To automatically review draft PRs, update your CodeRabbit configuration:

reviews:
  auto_review:
    drafts: true

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@kjgbot

kjgbot commented Sep 23, 2026

Copy link
Copy Markdown
Contributor Author

maintainability lens — FAIL

Not applicable — this is a review, not a multi-step build. Writing the review now.

Maintainability review — PR #568 (Flow Tool manifests)

Blockers

B1. Third argument to snapshotJsonValue does not exist.
packages/sdk/src/json-value.ts:12 defines snapshotJsonValue(value, at) — two parameters, no limits. The new code calls it with three at every site: flow-tool-manifest.ts:63, :73, :87, :99 and flow-tool-schema.ts:56, all passing FLOW_TOOL_LIMITS. This is a TS2554 (extra argument). Either the branch omits a companion change to json-value.ts, or the tests below cannot compile — a stranger in six months has no way to tell which.

B2. Advertised limits are unenforced.
FLOW-TOOL.md:63-70 and flow-tool-schema.ts:9 declare a 64/4,096/256 KiB envelope. Nothing consumes FLOW_TOOL_LIMITS. The test at flow-tool-manifest.test.ts:143-149 then asserts that 'x'.repeat(262145) and new Array(5000).fill(0) throw — neither will, because snapshotJsonValue has no byte-count or node-count budget. This is the exact "comment asserts what the code does not do" pattern the lens exists to catch, and the test would silently pass on the two conditions it looks like it covers (cycles, NaN, BigInt, Date, prototypes) while never actually protecting the limits the doc promises.

Concerns

C1. record() error swallows which field is wrong.
flow-tool-manifest.ts:26-30 throws "missing or unknown fields" for both cases without naming the offending key. Every top-level malformed manifest hits this path; debuggers will need to diff FIELDS by hand.

C2. Validation errors strip instance path.
flow-tool-manifest.ts:87-90 reduces Ajv failures to `schema validation failed (${keyword})`. The intent (// Report schema location, never data …) is good, but dropping instancePath too leaves the caller unable to say which property violated the schema. keyword alone (e.g., "required") is often useless.

C3. Ajv compile is not memoized.
flow-tool-schema.ts:52-54 builds a new Ajv2020 and recompiles the schema on every call to validateFlowToolInput/validateFlowToolResult (via flow-tool-manifest.ts:88). For a tool called from a loop this is a hidden hot path; a comment stating the intent (or a WeakMap<schema, validator>) would keep a future reader from "optimizing" wrongly.

C4. flowToolFunctionDefinition shape has no owning provider.
flow-tool-definitions.ts:20-23 returns {type:'function', name, description, parameters}. Neither Anthropic (input_schema) nor OpenAI Responses ({type:'function', function:{...}}) uses that literal shape. The JSDoc calls it "neutral"; a six-month reader wiring an adapter will not know whom to blame when it doesn't fit.

C5. Semver leading-zero rule is dense.
flow-tool-manifest.ts:38-40 chains six string ops to enforce "no numeric prerelease identifier with leading zero". A named predicate (hasLeadingZeroPrereleaseId) would carry the invariant forward.

Notes

  • unchecked = (value: unknown) => createFlowToolManifest(value as FlowToolDeclarationV1) in the test file (:17) is a deliberate malformed-input helper; a one-line comment would prevent someone from "fixing" the cast.
  • parseFlowToolManifest (:80) re-runs createFlowToolManifest for verification and equality-checks the digest. This is elegant but load-bearing: any future normalization inside createFlowToolManifest (trimming, case folding) will silently break already-signed manifests. Worth a one-line invariant comment.
  • FLOW-TOOL.md:57-60 correctly qualifies digest ≠ signature/provenance — good; keep this discipline as the module grows.

The identity/canonicalization design and the fail-closed schema profile are clear and well-scoped. But the missing limits plumbing means either the branch doesn't compile or the enforcement claims (doc + test) are false — both of which are exactly the "implicit contract that will bite a maintainer" the lens should stop.

REVIEW_FAILED

@kjgbot

kjgbot commented Sep 23, 2026

Copy link
Copy Markdown
Contributor Author

history lens — PASS

Blockers: None.

Concerns (non-blocking): RFC-0001 settled decision #14 ultimately requires a trusted, signed, content-addressed bundle. Here, flow.digest is only syntax-checked and bundle existence/admission is not verified (packages/sdk/src/flow-tool-manifest.ts:43-56). That is an explicitly documented deferral: the module does not execute flows, prove provenance, or provide admission (packages/sdk/FLOW-TOOLS.md:54-57, 88-94). Under the stated scaffolding policy, this is not a contradiction.

Notes: This does not repeat DRIVE-LOG’s prior “immutable gate” mistake, where mutable content and its checksum shared the same authority (ops/DRIVE-LOG.md:6278-6296). The new documentation expressly says an attacker can rehash metadata and that the digest is not a signature or authorization (packages/sdk/FLOW-TOOLS.md:54-57); callers can supply an independently trusted digest (packages/sdk/src/flow-tool-manifest.ts:67-78). Deep snapshots and freezing support the narrower immutability claim (packages/sdk/src/flow-tool-manifest.ts:60-64; packages/sdk/tests/flow-tool-manifest.test.ts:33-43).

The SDK-only descriptors do not add a kernel verb or execution path, so they fit settled decision #13’s open-surface/closed-kernel boundary (packages/sdk/src/flow-tool-definitions.ts:17-27). The commit message, “add immutable Flow Tool manifest contracts,” accurately describes the six-file diff; it makes no false test or execution claim. The PR description also honestly states that the full suite is red and the PR is not merge-ready.

REVIEW_PASSED

@kjgbot

kjgbot commented Sep 23, 2026

Copy link
Copy Markdown
Contributor Author

structure lens — MISSING

@kjgbot

kjgbot commented Sep 23, 2026

Copy link
Copy Markdown
Contributor Author

🎯 review-swarm: FAILED (M:fail H:pass S:missing)

Lens transcripts posted as sibling comments above.

@kjgbot

kjgbot commented Sep 23, 2026

Copy link
Copy Markdown
Contributor Author

Exact-head review correction — f37ead4e125a2a9062ae3eea3f5ebbdde6f9afc9

The prior maintainability B1/B2 failure is not applicable to this head. I verified the branch contents directly:

  • packages/sdk/src/json-value.ts defines snapshotJsonValue(value, at, limits?).
  • Its budget tracks and enforces max depth, max nodes/properties, and max bytes.
  • flow-tool-manifest.ts and flow-tool-schema.ts pass FLOW_TOOL_LIMITS at every relevant snapshot boundary.
  • The current head adds the built-public-API contract test and acceptance documentation/comments; it does not need to modify the already-present limits implementation.

The earlier review’s remaining maintainability observations (error specificity, validator compilation lifecycle, provider adapter naming, and semver predicate readability) are non-blocking considerations for this typed foundation. Please run fresh independent maintainability and structure reviews against this exact head, including the 54 focused tests and public API build-path evidence. Do not reuse the prior stale B1/B2 conclusion.

The PR remains draft and explicitly not merge-ready. No review thread is being resolved by this comment.

@khaliqgant

Copy link
Copy Markdown
Member

Runtime completion evidence is now on exact head 921645a (actual kernel-backed Flow Tool control plane; focused 139/139 plus live 4/4 reported). Remaining completion gates are explicit: rebase/update from stale base e07a190 onto current main, exact-head required CI/review, and account for the documented full-suite environment/baseline failures. Keep draft until those gates are terminal; no manifest-only claim.

Session-Id: 01a0d525-feb9-77e3-9f0c-a5fcb22f7d79

@khaliqgant khaliqgant left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Independent exact-head review of c9b8a1b (merge parent current main d090ad3). The runtime/control-plane implementation is present (not manifest-only): prior exact implementation review covered SDK focused 139/139 and live relayflowd 4/4; this head is that implementation plus guarded main merge. Exact-head required checks are terminal green: linux 36060317105, packed 36060317104, validate 36060317185, guard 36060313935; Cubic/CodeRabbit statuses pass. No substantive blocker found in the reviewed Flow Tool kernel/admission/journal path. Draft remains preserved pending owner/product completion and any full-suite baseline documentation.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants