Skip to content

feat: add specify workflow definition for run-scoped definitions - #4912

Open
markuswondrak wants to merge 21 commits into
github:mainfrom
markuswondrak:feat/4792-workflow-run-definition
Open

markuswondrak wants to merge 21 commits into
github:mainfrom
markuswondrak:feat/4792-workflow-run-definition

Conversation

@markuswondrak

@markuswondrak markuswondrak commented Oct 10, 2026 •

Copy link
Copy Markdown
Contributor

Description

Closes #4792.

Adds specify workflow definition <run_id> --json, a read-only command that
returns the workflow definition a run was started with. It reads only the
persisted run snapshot and frozen definitions for workflow calls that were
actually bound during execution, so output does not drift after installed
workflows are edited, overlaid, or removed.

  • definition is the persisted, overlay-composed root definition, passed
    through without filling defaults or restructuring fields.
  • workflow_scopes is always present and contains each bound workflow call's
    scope_path, workflow ID, and frozen definition. Its paths and workflow IDs
    match workflow status --json.
  • RunState.definition_path, load_definition(), and
    load_workflow_scopes() make the snapshot read surface explicit; resume and
    definition share the same persisted snapshot path.
  • --json is required. Successful output is one JSON object on stdout; errors
    are one {"error": ...} object on stderr.
  • Strict JSON output stringifies YAML-native values, non-string mapping keys,
    and non-finite floats, and rejects recursive YAML aliases with the same
    strict JSON error contract.

Decisions that diverge from the original issue

  • Output mirrors the persisted workflow structure rather than defining a new,
    normalized definition model. schema_version therefore versions the exposed
    definition shape; the JSON envelope is additive-only. This avoids partially
    applying engine defaults and preserves the definition that was executed.

  • The command is JSON-only for now. It requires --json and exits 2 without it;
    a human text renderer is intentionally deferred rather than introducing a
    second output contract.

  • A composed run includes only definitions that are already bound in its
    execution tree. Unreached calls are not resolved from current installed
    workflows, because doing so would reintroduce definition drift.

  • The root definition remains authored: call steps are not expanded inline.
    Frozen callees appear separately in workflow_scopes, which also supports a
    call invoked more than once through loops or fan-out.

  • Errors follow the established artifact strict-JSON contract, including
    project-resolution and unsafe-workflow-storage failures. workflow status --json intentionally retains its existing Rich error rendering.

  • Dynamically generated steps are out of scope. Steps that a custom step type
    produces at run time through StepResult.next_steps are persisted in the
    execution state but are not exported; definition and workflow_scopes
    cover only the authored definition and bound workflow calls. The
    documentation states this limit.

Follow-ups

Testing

  • Ran focused workflow, overlay, preset, extension, and engine tests:
    1996 passed, 1 skipped.
  • Ran uvx ruff@0.15.0 check src tests.
  • Ran git diff --check.
  • Ran the primary workflow suites again: 850 passed.
  • Ran tests/specify_cli/workflows/test_command_definition.py: 36 passed,
    including recursive YAML alias rejection.
  • Manually tested the working-tree CLI in /tmp/test-workflow-run-definition:
    initialized a project, ran root and composed paused workflows, verified the
    persisted snapshot and JSON envelope, checked workflow_scopes parity with
    status --json, exercised required-flag and unknown-run errors, and confirmed
    unchanged output after editing then removing the installed child workflow.

The full suite was attempted twice. Both runs showed the known pre-existing
failure tests/integration/test_preset_update_workflow.py::test_preset_update_cli_contract,
which expects an older Typer missing-argument string; the isolated test still
fails unchanged. The second full run continued cleanly through 83% before the
available execution timeout.

AI Disclosure

  • I did not use AI assistance for this contribution
  • I did use AI assistance (fill in the disclosure below)

AI disclosure: Implemented and updated with OpenCode using gpt-5.6-terra
(github-copilot/gpt-5.6-terra) in autonomous mode with default reasoning.
The agent generated implementation, tests, documentation, follow-up issues, and
review responses from the reviewed issue plan and review decisions, and ran the
listed automated and manual checks. No claim of human line-by-line review is
made.

Add a strict JSON command for inspecting the definition persisted with a workflow run, including frozen definitions for bound workflow calls.\n\nCloses github#4792.\n\nAssisted-by: OpenCode (model: gpt-5.6-terra, autonomous)
@markuswondrak
markuswondrak requested a review from mnriem as a code owner October 10, 2026 08:24
Copilot AI balanced review requested due to automatic review settings October 10, 2026 08:24

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🟡 Changes recommended

Symlink traversal, recursive YAML handling, and missing MCP inventory coverage remain unresolved.

3 open findings
What changed in this PR

Adds run-scoped workflow definition retrieval using persisted snapshots and frozen called-workflow definitions.

Changes:

  • Adds the JSON-only workflow definition command and documentation.
  • Exposes persisted definitions and bound workflow scopes through RunState.
  • Adds extensive command, persistence, composition, and error tests.
File Description
src/​specify_cli/​workflows/​command_definition.py Implements the new command.
src/​specify_cli/​workflows/​engine.py Adds snapshot and scope-loading APIs.
src/​specify_cli/​workflows/​_execution.py Shares bound-scope traversal.
src/​specify_cli/​workflows/​_commands.py Adds registration and JSON conversion.
src/​specify_cli/​workflows/​command_status.py Reuses run-state loading.
src/​specify_cli/​_installed_list_json.py Generalizes strict JSON helpers.
tests/​specify_cli/​workflows/​test_command_definition.py Tests the command contract.
tests/​test_workflows.py Tests snapshot persistence APIs.
docs/​reference/​workflows.md Documents usage and output.

🧠 Review effort: Balanced


💡 Configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread src/specify_cli/workflows/command_definition.py
Comment thread src/specify_cli/workflows/_commands.py Outdated
Comment thread src/specify_cli/workflows/_commands.py
@markuswondrak

Copy link
Copy Markdown
Contributor Author

Manual validation completed after opening this PR. Using the working-tree CLI in a disposable initialized project, I verified root and composed paused runs, persisted snapshots, definition/status scope parity, required-flag and unknown-run errors, and stable definition output after editing and removing the installed child workflow.

AI disclosure: Posted by Markus Wondrak with OpenCode (github-copilot/gpt-5.6-terra), default reasoning, autonomous mode; the agent performed and summarized the manual CLI validation.

Assisted-by: OpenCode (model: gpt-5.6-terra, autonomous)
Copilot AI balanced review requested due to automatic review settings October 10, 2026 08:47
@markuswondrak

Copy link
Copy Markdown
Contributor Author

Review update for commit 57b4136:

Validation: 36 passed for tests/specify_cli/workflows/test_command_definition.py; 133 passed across definition, run, resume, and status command suites; Ruff and git diff --check passed.

AI disclosure: Posted by Markus Wondrak with OpenCode (github-copilot/gpt-5.6-terra), default reasoning, autonomous mode. The agent implemented the fix, tests, follow-up issues, and this review summary, and ran the stated automated checks; no claim of human line-by-line review is made.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🔵 Needs a closer look

Mapping-key normalization can silently discard persisted definition data, and scope rendering introduces avoidable traversal overhead.

0 open findings

3 resolved since last review
Previously missed (2)

In code that hasn't changed since last review

Medium severity Stringified mapping keys can silently overwrite existing keys

src/​specify_cli/​workflows/​_commands.py:1082

Stringifying a non-string mapping key inside this comprehension can silently overwrite an existing string key. For example, YAML containing both an unquoted 2026-01-01 key (loaded as date) and a quoted "2026-01-01" key has two entries before this code and only the latter afterward, so the advertised persisted definition is returned with data missing. Normalize keys explicitly and reject collisions (with a negative test) rather than allowing the comprehension to drop an entry.

Medium severity Scope rendering adds avoidable traversal and per-node memory

src/​specify_cli/​workflows/​_execution.py:368

This refactor turns scope rendering from one tree walk into two and retains an additional entry for every execution node in nodes_by_path. Since workflow trees grow with loop iterations and fan-out items and status is commonly polled, this adds avoidable O(nodes) memory and a full traversal even when there are few or no bound scopes. Iterate the nodes directly (as before), or make the shared iterator yield the node together with the path and binding.

🧠 Review effort: Balanced

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🟡 Changes recommended

JSON key normalization can silently discard definition entries when stringified keys collide.

1 open finding

🧠 Review effort: Balanced

Comment thread src/specify_cli/workflows/_commands.py Outdated
@mnriem

mnriem commented Oct 12, 2026

Copy link
Copy Markdown
Collaborator

Please address Copilot feedback

Canonicalize every mapping key to the JSON property name json.dumps emits
before inserting it, and raise when two keys collapse to the same name.
Previously the definition command could emit duplicate JSON properties for
keys such as 1 and "1", and json.loads then silently dropped an entry.

Assisted-by: OpenCode (model: deepseek-v4.1-flash, autonomous)
Copilot AI balanced review requested due to automatic review settings October 12, 2026 18:45
@markuswondrak

Copy link
Copy Markdown
Contributor Author

Addressed the duplicate-canonical-key finding in a08f50e.

_emit_workflow_json now canonicalizes every mapping key to the JSON property name json.dumps emits before insertion, and raises ValueError when two keys collapse to the same name (e.g. 1/"1", true/"true", or a date and its string form). That routes through the existing strict-JSON error path (stdout empty, exit 1) instead of emitting duplicate properties that json.loads silently truncates. Added a parametrized regression test for int/string, bool/string and date/string collisions, and documented the behavior in docs/reference/workflows.md.

Verification: tests/specify_cli/workflows/test_command_definition.py 39 passed; tests/specify_cli/workflows/ tests/test_workflows.py 1433 passed, 1 skipped; uvx ruff@0.15.0 check clean.

AI disclosure: posted by OpenCode (model: deepseek-v4.1-flash, autonomous) on behalf of @markuswondrak. The human selected and directed the fix and reviewed the plan; the code was not line-by-line human-reviewed before commit.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🔵 Needs a closer look

Non-recursive YAML alias graphs can cause exponential resource consumption during JSON normalization.

0 open findings

1 resolved since last review
Previously missed (1)

In code that hasn't changed since last review

Medium severity YAML alias DAG expansion causes exponential resource usage

src/​specify_cli/​workflows/​_commands.py:1109

active_containers rejects cycles but forgets each container after returning, so a non-recursive YAML alias DAG is expanded independently at every reference. A linear-size definition such as xN: &xN [*xN-1, *xN-1] therefore causes exponential CPU, memory, and stdout growth; the repository already treats this exact pattern as a regression hazard in tests/test_resolve_template_python_parity.py:1088-1095. Reject repeated container identities or enforce a bounded expansion/output policy, and add a negative test for a deep non-cyclic alias graph.

🧠 Review effort: Balanced

`_emit_workflow_json` rejected recursive YAML aliases but re-expanded a
non-recursive alias DAG independently at every reference, so a linear-size
definition such as `xN: &xN [*xN-1, *xN-1]` produced exponential CPU,
memory, and stdout. Add a node budget that raises the existing strict-JSON
error instead of rejecting every reused alias, and cover both a bounded
shared alias and a deep non-cyclic DAG.

Assisted-by: opencode (model: deepseek-v4.1-flash, autonomous)
Copilot AI balanced review requested due to automatic review settings October 12, 2026 19:07
@markuswondrak

Copy link
Copy Markdown
Contributor Author

Addresses the alias-expansion finding from review 5494817862.

  • _emit_workflow_json now enforces a node budget in plain_json, so a non-recursive YAML alias DAG raises the strict-JSON error instead of expanding exponentially. The budget bounds expansion rather than rejecting every reused alias, so legitimate shared anchors still emit.
  • Added a negative test (deep non-cyclic DAG) and a positive test (bounded shared alias).
  • Regression evidence: the negative test fails on the parent commit (exit_code 0, ~31s emitting the exponential payload at depth 20) and passes after.
  • Updated docs/reference/workflows.md (Plain JSON and Errors).
  • tests/specify_cli/workflows: 638 passed, 1 skipped. uvx ruff@0.15.0 check: clean.

Commit: 32dff35

Disclosure: posted on behalf of @markuswondrak by opencode (model: deepseek-v4.1-flash), autonomous, without human line-by-line review.

100000 still leaves >1000x headroom over the largest shipped workflow
definition (~67 nodes) while capping worst-case memory and stdout.

Assisted-by: opencode (model: deepseek-v4.1-flash, autonomous)

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🟡 Changes recommended

Persisted dynamic custom-step expansions are absent from the response, leaving the documented full step tree incomplete.

1 open finding

🧠 Review effort: Balanced

Comment thread src/specify_cli/workflows/command_definition.py
Copilot AI balanced review requested due to automatic review settings October 12, 2026 19:12
@markuswondrak

Copy link
Copy Markdown
Contributor Author

@mnriem Review update for commit 594a44c.

  • Aggregate budget (Copilot review, command_definition.py): Valid. The node and byte budget covered the root and every workflow_scopes entry together, so an alias-free run with many bound workflow calls could be rejected although each definition was small. The conversion now has its own budget per call (_plain_workflow_json), and the command budgets the root and each bound definition separately. The limits themselves are unchanged, and status, run and resume are unaffected. docs/reference/workflows.md states that the budget is per definition.
  • Tests: test_budget_applies_per_definition_not_to_the_whole_response fails on the previous commit and passes now. test_over_budget_single_definition_still_fails confirms a single over-budget definition still returns the JSON error with empty stdout.

Validation: tests/specify_cli/workflows and tests/test_workflows.py 1450 passed, 1 skipped; ruff@0.15.0 check src tests and markdownlint-cli2 docs/reference/workflows.md clean. Python 3.13/3.14 were not run locally; CI will confirm.

Correction: the trailer on the earlier commit cb9ae84 says supervised. It should read autonomous, as no human reviewed that change line by line.

AI disclosure: posted on behalf of @markuswondrak by GitHub Copilot CLI (model: Claude Sonnet 5.5, agent mode), autonomous mode in a session directed by the contributor. The agent verified the finding, wrote the fix, tests and docs change, ran the stated checks, and wrote this comment; no claim of human line-by-line review is made.

Markus added 2 commits October 24, 2026 16:50
…run-definition

Assisted-by: pi coding agent (model: opencode-go/deepseek-v4.1-flash, autonomous)
Register `specify workflow definition` in the hierarchy-owned workflow MCP
inventory. The inventory and its CLI-registration parity test landed on main
after this branch point, so the merge exposed a missing disposition for the
new leaf: `_assert_inventory` reported `Missing CLI paths: ['specify
workflow definition']`.

The command stays unavailable over MCP: run-scoped definition export,
per-definition JSON budgeting and CLI error mapping still live in
command_definition, with no shared typed operation or adapter.

Assisted-by: pi coding agent (model: opencode-go/deepseek-v4.1-flash, autonomous)
@mnriem
mnriem requested a max review from Copilot and removed request for Copilot October 26, 2026 13:21

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🟡 Changes recommended

A malformed persisted binding can produce a traceback instead of the required JSON error.

1 open issue
1 resolved since last review

Review effort: Max

Comment thread src/specify_cli/workflows/engine.py
Ensure check_node rejects persisted workflow bindings whose workflow ID is missing or not a non-empty string before bound_scopes callers index binding['workflow']. Add a regression test for a corrupted binding that omits both binding.workflow and the frozen definition's workflow.id.

Assisted-by: Google Antigravity (model: Gemini 4 Argon Medium, autonomous)
Copilot AI max review requested due to automatic review settings October 26, 2026 14:21
@markuswondrak

Copy link
Copy Markdown
Contributor Author

Review update for commit 54cbada.

  • Missing bound workflow ID (Copilot review, _execution.py / engine.py): check_node now verifies that binding.get("workflow") is a non-empty string before comparing it against bound_definition["workflow"].get("id"). Previously, a corrupted persisted binding that omitted both binding.workflow and the frozen header's workflow.id passed None != None during validate_execution and then raised KeyError: 'workflow' when load_workflow_scopes (or scope_summaries) indexed binding["workflow"]. It now raises ValueError("Invalid bound workflow definition or inputs"), which specify workflow definition --json reports through the JSON error contract.
  • Tests: Added TestNegative.test_missing_bound_workflow_id_returns_json_error in tests/specify_cli/workflows/test_command_definition.py, which failed with KeyError: 'workflow' before the fix and passes with it.

Validation: .venv/bin/python -m pytest tests/specify_cli/workflows/ tests/test_workflows.py -q (1610 passed, 28 skipped on Python 3.14.4) and uvx ruff@0.15.0 check src tests clean.

AI disclosure: posted on behalf of @markuswondrak by Google Antigravity (model: Gemini 4 Argon Medium, autonomous mode in a session directed by the contributor). Extent: verification of the review finding, code fix, regression test, local test/lint execution, and this comment; no claim of human line-by-line review is made.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Copilot was unable to review this pull request because the user who requested the review has insufficient remaining quota.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🔵 Needs a closer look

The new public JSON contract and shared serialization and persisted-run changes warrant final human validation.

2 open issues

Review effort: Max

Comment thread docs/reference/workflows.md Outdated
…inition reference

Document that an omitted schema_version in the verbatim definition output defaults to "1.0" and that validation compares str(schema_version), and state that the additive-only guarantee applies to the top-level response envelope.

Assisted-by: Google Antigravity (model: Gemini 4 Argon Medium, autonomous)
Copilot AI max review requested due to automatic review settings October 26, 2026 17:01
@markuswondrak

Copy link
Copy Markdown
Contributor Author

Review update for 7a794456 (with 54cbadae):

  • Clarified in docs/reference/workflows.md (7a794456) that because definition is emitted as authored, an omitted schema_version defaults to "1.0" and the engine compares str(schema_version) (so unquoted schema_version: 1.0 in YAML appears as the JSON number 1.0), and scoped the additive-only key guarantee to the top-level response envelope (run_id, definition, workflow_scopes).
  • Validated in src/specify_cli/workflows/_execution.py (54cbadae) that bound workflow records include a non-empty string workflow ID before bound_scopes callers index binding["workflow"], with regression coverage in tests/specify_cli/workflows/test_command_definition.py.

AI disclosure: Posted on behalf of @markuswondrak by Google Antigravity (model: Gemini 4 Argon Medium, autonomous). The human reviewed the findings and directed the fix; the agent authored the documentation edit, commit, and review comments.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Copilot was unable to review this pull request because the user who requested the review has insufficient remaining quota.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🟡 Changes recommended

Nested aliases can exceed the per-definition byte limit because JSON punctuation and indentation are not counted.

1 open issue

Review effort: Max

Comment thread src/specify_cli/workflows/_commands.py Outdated
Comment on lines +1191 to +1192
try:
return plain_json(payload, set())
@mnriem

mnriem commented Oct 26, 2026

Copy link
Copy Markdown
Collaborator

Please address Copilot feedback

Check the complete serialized JSON length (including brackets, whitespace, and non-string scalars emitted by json.dumps(..., indent=2)) at the end of _plain_workflow_json when max_nodes is active, while keeping the incremental string byte budget during traversal. Add a regression test for a shared 150-level nested list referenced 150 times and verify per-definition byte budgeting across workflow_scopes.

Assisted-by: Google Antigravity (model: Gemini 4 Argon Medium, autonomous)
Copilot AI max review requested due to automatic review settings October 27, 2026 07:26
@markuswondrak

Copy link
Copy Markdown
Contributor Author

Addressed in 3b218665:

  • _plain_workflow_json now checks len(json.dumps(plain, indent=2)) > _MAX_JSON_BYTES when max_nodes is enabled, while retaining the incremental string byte budget during traversal. Because workflow_definition calls _plain_workflow_json for the root definition and each workflow_scopes entry separately, the serialized size limit continues to apply per definition.
  • Added TestNegative.test_byte_budget_stops_nested_alias_indentation_expansion for a 150-level nested list referenced 150 times via YAML alias and extended TestComposition.test_budget_applies_per_definition_not_to_the_whole_response to cover _MAX_JSON_BYTES.

AI disclosure: Posted on behalf of @markuswondrak by Google Antigravity (model: Gemini 4 Argon Medium, autonomous); the agent implemented the fix, ran the test suite, pushed commit 3b218665, and drafted this comment.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Copilot was unable to review this pull request because the user who requested the review has insufficient remaining quota.

This branch has not been deployed

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

Labels

author-awaiting Waiting on author response triage-can-wait Verdict: valid and in-scope but deprioritized; held behind the evidence gate

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Feature]: Supported run-scoped access to a workflow run's definition

3 participants