-
Notifications
You must be signed in to change notification settings - Fork 17
feat(copilot): ship portable flow authoring resources #91
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
8a01f0e
73c19c7
0927b28
c30a9a5
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,94 @@ | ||
| // SPDX-License-Identifier: GPL-3.0-or-later | ||
| //! Portable settlement rules for saved workflow run history. | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Add the GPL-3.0-or-later license header This new source file has no GPL-3.0-or-later license header, so it still violates the repository's licensing rule carried over from the earlier review. Add the repository-standard GPL header before the module documentation. Additional
|
||
|
|
||
| use serde_json::Value; | ||
|
|
||
| use crate::FlowRunStep; | ||
|
|
||
| /// Reconstructs lightweight steps from an engine run output. | ||
| pub fn reconstruct_steps(output: &Value) -> Vec<FlowRunStep> { | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Add tests for all public functions in run_summary The new [RULE] missing-unit-tests · |
||
| output | ||
| .get("nodes") | ||
| .and_then(Value::as_object) | ||
| .map(|nodes| { | ||
| nodes | ||
| .iter() | ||
| .map(|(node_id, slot)| FlowRunStep { | ||
| node_id: node_id.clone(), | ||
| output: slot.get("items").cloned().unwrap_or(Value::Null), | ||
| port: slot.get("port").and_then(Value::as_str).map(str::to_string), | ||
| ..Default::default() | ||
| }) | ||
| .collect() | ||
| }) | ||
| .unwrap_or_default() | ||
| } | ||
|
|
||
| /// Merges observed steps with reconstructed output, retaining richer observed | ||
| /// timing, status, and diagnostic fields for matching nodes. | ||
| pub fn settle_steps(observed: Vec<FlowRunStep>, output: &Value) -> Vec<FlowRunStep> { | ||
| let reconstructed = reconstruct_steps(output); | ||
| if observed.is_empty() { | ||
| return reconstructed; | ||
| } | ||
| let mut settled = observed; | ||
| for step in reconstructed { | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Merge reconstructed routing metadata into observed steps When an observed step has the same [RULE] incomplete-merge · |
||
| if let Some(existing) = settled | ||
| .iter_mut() | ||
| .find(|existing| existing.node_id == step.node_id) | ||
| { | ||
| // The live observer has richer timing/status data, but only the | ||
| // post-hoc output knows which branch a routing node selected. | ||
| if existing.port.is_none() { | ||
| existing.port = step.port; | ||
| } | ||
| } else { | ||
| settled.push(step); | ||
| } | ||
| } | ||
| settled | ||
| } | ||
|
|
||
| /// A terminal run classification suitable for persisting in [`crate::FlowRun`]. | ||
| #[derive(Debug, Clone, PartialEq, Eq)] | ||
| pub struct TerminalRunStatus { | ||
| /// Persisted status string. | ||
| pub status: &'static str, | ||
| /// Error text when a continued/routed step failed. | ||
| pub error: Option<String>, | ||
| } | ||
|
|
||
| /// Classifies settled steps, with a pending approval taking precedence. | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Add e2e test for terminal_status classification The new [RULE] missing-e2e-coverage · |
||
| pub fn terminal_status(steps: &[FlowRunStep], pending_approvals: &[String]) -> TerminalRunStatus { | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Add e2e test for terminal_status classification The new [RULE] missing-e2e-coverage · There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Add an end-to-end test for terminal status classification The added tests exercise Additional
|
||
| if !pending_approvals.is_empty() { | ||
| return TerminalRunStatus { | ||
| status: "pending_approval", | ||
| error: None, | ||
| }; | ||
| } | ||
| let failed: Vec<&str> = steps | ||
| .iter() | ||
| .filter(|step| step.status.as_deref() == Some("error")) | ||
| .map(|step| step.node_id.as_str()) | ||
| .collect(); | ||
| if !failed.is_empty() { | ||
| return TerminalRunStatus { | ||
| status: "failed", | ||
| error: Some(format!( | ||
| "node(s) failed after retries: {}", | ||
| failed.join(", ") | ||
| )), | ||
| }; | ||
| } | ||
| if steps.iter().any(|step| !step.diagnostics.is_empty()) { | ||
| TerminalRunStatus { | ||
| status: "completed_with_warnings", | ||
| error: None, | ||
| } | ||
| } else { | ||
| TerminalRunStatus { | ||
| status: "completed", | ||
| error: None, | ||
| } | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,55 @@ | ||
| // SPDX-License-Identifier: GPL-3.0-or-later | ||
| use crate::FlowRunStep; | ||
| use crate::run_summary::*; | ||
| use serde_json::json; | ||
|
|
||
| fn step(node_id: &str) -> FlowRunStep { | ||
| FlowRunStep { | ||
| node_id: node_id.to_string(), | ||
| output: json!([]), | ||
| ..Default::default() | ||
| } | ||
| } | ||
|
|
||
| #[test] | ||
| fn reconstruct_steps_reads_items_and_port() { | ||
| let steps = reconstruct_steps(&json!({ | ||
| "nodes": {"switch": {"items": [{"json": {"ok": true}}], "port": "true"}} | ||
| })); | ||
| assert_eq!(steps.len(), 1); | ||
| assert_eq!(steps[0].node_id, "switch"); | ||
| assert_eq!(steps[0].port.as_deref(), Some("true")); | ||
| assert_eq!(steps[0].output, json!([{"json": {"ok": true}}])); | ||
| } | ||
|
|
||
| #[test] | ||
| fn settle_steps_keeps_observed_data_and_fills_routing_port() { | ||
| let mut observed = step("switch"); | ||
| observed.status = Some("success".to_string()); | ||
| let settled = settle_steps( | ||
| vec![observed], | ||
| &json!({"nodes": {"switch": {"items": [], "port": "false"}, "done": {"items": []}}}), | ||
| ); | ||
| assert_eq!(settled.len(), 2); | ||
| assert_eq!(settled[0].status.as_deref(), Some("success")); | ||
| assert_eq!(settled[0].port.as_deref(), Some("false")); | ||
| assert_eq!(settled[1].node_id, "done"); | ||
| } | ||
|
|
||
| #[test] | ||
| fn terminal_status_prioritizes_approval_then_failure_then_warnings() { | ||
| assert_eq!( | ||
| terminal_status(&[], &["approval".to_string()]).status, | ||
| "pending_approval" | ||
| ); | ||
| let mut failed = step("broken"); | ||
| failed.status = Some("error".to_string()); | ||
| assert_eq!(terminal_status(&[failed], &[]).status, "failed"); | ||
| let mut warning = step("warn"); | ||
| warning.diagnostics.push(json!({"location": "args.x"})); | ||
| assert_eq!( | ||
| terminal_status(&[warning], &[]).status, | ||
| "completed_with_warnings" | ||
| ); | ||
| assert_eq!(terminal_status(&[], &[]).status, "completed"); | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,51 @@ | ||
| // SPDX-License-Identifier: GPL-3.0-or-later | ||
| //! Reference material for authors of tinyflows graphs. | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Add GPL-3.0-or-later license header to new files New files This was already flagged in the previous review and remains unfixed. Additional
|
||
| //! | ||
| //! Hosts decide how these bytes are exposed to an agent. The content itself | ||
| //! is portable: it documents the graph language and deliberately avoids a | ||
| //! dependency on a particular harness or skill runtime. | ||
|
|
||
| /// One file in a portable authoring resource bundle. | ||
| #[derive(Debug, Clone, Copy, PartialEq, Eq)] | ||
| pub struct ResourceFile { | ||
| /// Slash-separated path relative to the bundle root. | ||
| pub path: &'static str, | ||
| /// UTF-8 resource contents. | ||
| pub contents: &'static str, | ||
| } | ||
|
|
||
| /// The flow-authoring manual's manifest page. | ||
| pub const FLOW_AUTHORING_WORKFLOW: &str = include_str!("resources/flow-authoring/WORKFLOW.md"); | ||
| /// Expression and jq reference page. | ||
| pub const FLOW_AUTHORING_EXPRESSIONS: &str = | ||
| include_str!("resources/flow-authoring/references/expressions.md"); | ||
| /// Node-configuration reference page. | ||
| pub const FLOW_AUTHORING_NODE_CONFIG: &str = | ||
| include_str!("resources/flow-authoring/references/node-config.md"); | ||
| /// Dry-run interpretation reference page. | ||
| pub const FLOW_AUTHORING_DRY_RUN: &str = | ||
| include_str!("resources/flow-authoring/references/dry-run.md"); | ||
|
|
||
| /// Every file in the portable `flow-authoring` manual. | ||
| pub const FLOW_AUTHORING_FILES: &[ResourceFile] = &[ | ||
| ResourceFile { | ||
| path: "WORKFLOW.md", | ||
| contents: FLOW_AUTHORING_WORKFLOW, | ||
| }, | ||
| ResourceFile { | ||
| path: "references/expressions.md", | ||
| contents: FLOW_AUTHORING_EXPRESSIONS, | ||
| }, | ||
| ResourceFile { | ||
| path: "references/node-config.md", | ||
| contents: FLOW_AUTHORING_NODE_CONFIG, | ||
| }, | ||
| ResourceFile { | ||
| path: "references/dry-run.md", | ||
| contents: FLOW_AUTHORING_DRY_RUN, | ||
| }, | ||
| ]; | ||
|
|
||
| #[cfg(test)] | ||
| #[path = "resources_tests.rs"] | ||
| mod tests; | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,54 @@ | ||
| <!-- SPDX-License-Identifier: GPL-3.0-or-later --> | ||
| --- | ||
|
Comment on lines
+1
to
+2
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
When a host loads this portable resource with a conventional Markdown/YAML frontmatter parser, the parser expects the opening Useful? React with 👍 / 👎. |
||
| name: flow-authoring | ||
| description: The tinyflows authoring reference — expression and jq syntax, node configuration for memory/dedup/trigger nodes, per-node error handling, and how to read a dry run honestly. Read a page before configuring the thing it covers. | ||
| metadata: | ||
| version: "1.0.0" | ||
| author: tinyflows | ||
| tags: | ||
| - flows | ||
| - workflows | ||
| - authoring | ||
| - reference | ||
| allowed-tools: | ||
| - read_workflow_resource | ||
| - get_node_kind_contract | ||
| - list_node_kinds | ||
| --- | ||
|
|
||
| # Authoring a tinyflows workflow | ||
|
|
||
| This is a **reference manual, not a procedure**. It holds the exact rules that | ||
| are too long to keep in a system prompt and too precise to reconstruct from | ||
| memory: an expression convention you half-remember produces a graph that | ||
| validates and then does the wrong thing at run time. | ||
|
|
||
| Read the one page that covers what you are about to configure. | ||
|
|
||
| | page | read it before you | | ||
| | --- | --- | | ||
| | `references/expressions.md` | write any `=` expression or jq filter, or attach a produced file to an outbound action | | ||
| | `references/node-config.md` | configure a `memory`, `dedup` or `trigger` node, or set per-node error handling | | ||
| | `references/dry-run.md` | report what a dry run did and did not prove | | ||
|
|
||
| Fetch one with: | ||
|
|
||
| ``` | ||
| read_workflow_resource { skill_id: "flow-authoring", relative_path: "references/expressions.md" } | ||
| ``` | ||
|
|
||
| ## What is deliberately not here | ||
|
|
||
| **Per-kind configuration.** `get_node_kind_contract { kind }` returns a node | ||
| kind's config fields, ports, a worked example and its gotchas, and it is | ||
| generated from the same catalog the validator enforces — so it cannot go stale | ||
| the way this text can. Where the two disagree, the contract tool is right. | ||
| These pages cover the rules that span kinds, which is why they have nowhere | ||
| generated to live. | ||
|
|
||
| **The rules you must not break.** Propose rather than persist, ask before a | ||
| real run, ground every slug, prefer the minimal viable graph — those stay in | ||
| the system prompt, because a rule that only binds once someone chooses to read | ||
| it is not a rule. Graph sizing was moved here during an earlier pass and moved | ||
| back for exactly that reason: it shapes every graph, including the ones built | ||
| without opening a manual. | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,22 @@ | ||
| <!-- SPDX-License-Identifier: GPL-3.0-or-later --> | ||
| # Reading a dry run | ||
|
|
||
| A dry run evaluates the graph without committing the real side effects. It is | ||
| useful for checking node wiring, expression results, branches, and the actions | ||
| the graph would request. It is not evidence that external services accepted a | ||
| request, that credentials are valid, or that a real action completed. | ||
|
|
||
| ## Report results precisely | ||
|
|
||
| Say which input was used, which branches executed, and which proposed actions | ||
| were observed. Separate values that were evaluated from actions that would | ||
| have happened. Never describe a dry run as sending a message, writing memory, | ||
| or modifying an integration. | ||
|
|
||
| ## Limits | ||
|
|
||
| Use non-secret representative data. Test alternate branches and absent | ||
| optional values when they affect a decision. A dry run cannot prove behavior | ||
| behind a live provider, approval prompt, network failure, rate limit, or | ||
| permission boundary; name those remaining checks before asking to run the | ||
| workflow for real. |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,31 @@ | ||
| <!-- SPDX-License-Identifier: GPL-3.0-or-later --> | ||
| # Expressions | ||
|
|
||
| Use a literal value unless a field must be derived at run time. A value that | ||
| starts with `=` is evaluated as an expression; all other values are passed to | ||
| the node unchanged. | ||
|
|
||
| ## Inputs and results | ||
|
|
||
| Build expressions from the values exposed by the node contract. Start by | ||
| reading `get_node_kind_contract` for the node being configured, then use the | ||
| documented input and output names exactly. Do not guess a path from a label: | ||
| a graph can validate while a guessed path evaluates to `null` at run time. | ||
|
|
||
| Use jq only for a transformation that cannot be represented by selecting a | ||
| field. Keep filters small, preserve the value type expected by the destination | ||
| port, and account for absent optional fields with `?` or an explicit default. | ||
|
Comment on lines
+15
to
+17
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
When an author expands a simple binding into a jq transformation, this guidance does not explain that only a simple dotted path may use the bare shorthand. For example, the natural filter Useful? React with 👍 / 👎. |
||
|
|
||
| ## Files and outbound actions | ||
|
|
||
| An attachment must come from an output that is documented as a file or binary | ||
| artifact. Do not turn arbitrary text into a path, and do not put a local path | ||
| in a remote-action configuration. Inspect the producing node's output | ||
|
Comment on lines
+21
to
+23
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
When a workflow generates an attachment locally, the standing builder contract explicitly requires the producer to write to a chosen workspace-relative path without exposing it as node output, then passes that literal path to a storage-upload node ( Useful? React with 👍 / 👎. |
||
| contract before wiring it to an outbound action. | ||
|
|
||
| ## Check before proposing | ||
|
|
||
| Before presenting a graph, use the dry-run tool on representative, non-secret | ||
| input. A successful expression evaluation proves only the exercised shape of | ||
| data; it does not prove that optional fields will exist in every production | ||
| run. | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,31 @@ | ||
| <!-- SPDX-License-Identifier: GPL-3.0-or-later --> | ||
| # Node configuration | ||
|
|
||
| `get_node_kind_contract` is the source of truth for a node's fields, ports, | ||
| examples, and validation rules. Read it before configuring any node; this | ||
| page records only conventions shared by several node kinds. | ||
|
|
||
| ## Memory and deduplication | ||
|
|
||
| Give memory operations a stable source scope. Scope identifies where a fact | ||
| comes from; an item ID is only a deduplication key and must not be used as the | ||
| collection scope. Choose a deterministic deduplication key from data that is | ||
|
Comment on lines
+10
to
+12
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
When configuring a Useful? React with 👍 / 👎. |
||
| available on every run. Do not use a timestamp or generated UUID when the | ||
| intent is to suppress repeated work. | ||
|
|
||
| ## Triggers | ||
|
|
||
| Make a trigger narrow enough that its input shape and authorization boundary | ||
| are clear. A trigger begins a run; it does not grant an action permission. | ||
| Any side effect still needs the normal approval and policy path. | ||
|
|
||
| ## Error handling | ||
|
|
||
| Set per-node error handling deliberately. Continue only when later nodes can | ||
| produce a correct result without this node's output. Prefer a visible failure | ||
| for required inputs and side effects; swallowing an error may make a completed | ||
| run misleading. | ||
|
|
||
| After configuring a node, inspect its port types and connect only compatible | ||
| outputs. A graph proposal should state the purpose of each non-default error | ||
| policy so the user can review its consequence. | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add an end-to-end terminal-status test
The added test module covers
terminal_statusonly with fabricatedFlowRunStepvalues. It does not run a workflow through the engine and verify that the resulting output is classified correctly, so integration-level mismatches between engine output and the settlement rules can still ship undetected. Add an end-to-end test that exercises the engine and asserts the terminal status for the relevant output shapes.[RULE] missing-e2e-coverage ·