Skip to content

fix(workflows): load custom steps before add - #4473

Open
ton-katsu wants to merge 1 commit into
github:mainfrom
ton-katsu:fix/workflow-load-custom-steps
Open

fix(workflows): load custom steps before add#4473
ton-katsu wants to merge 1 commit into
github:mainfrom
ton-katsu:fix/workflow-load-custom-steps

Conversation

@ton-katsu

@ton-katsu ton-katsu commented Sep 8, 2026

Copy link
Copy Markdown

Assisted-by: GitHub Copilot (model: unknown, supervised)

Description

Fix the workflow add command so it loads custom workflow steps before
validating a workflow.

Previously, workflow add did not call load_custom_steps(project_root).
As a result, workflows containing custom steps such as feedback could not be
validated and added successfully.

A regression test was added to verify that the project root is resolved before
custom steps are loaded.

Testing

  • Tested locally with uv run specify --help
  • Ran focused tests with .venv/bin/python -m pytest tests/test_workflows.py -q
  • Ran the full test suite with uv sync && uv run pytest
  • Tested with a sample project
  • Verified the regression test on macOS and Linux

Focused test result:

970 passed, 1 skipped

The sample project test verified that:

  • The feedback step was installed from a step catalog.
  • A workflow containing the feedback step was added successfully with
    specify workflow add --dev.

The full test suite was not marked as passing because running uv sync && uv run pytest without the test extra removed the test dependencies and resulted
in collection errors. The focused workflow test suite passed successfully.

AI Disclosure

  • I did not use AI assistance for this contribution
  • I did use AI assistance

The code changes and regression tests were created with GitHub Copilot. The
changes were reviewed and tested by the contributor.

@ton-katsu
ton-katsu requested a review from mnriem as a code owner September 8, 2026 18:25
@mnriem
mnriem requested a balanced review from Copilot September 9, 2026 14:12
@mnriem mnriem added the triage-nice-to-have Verdict: evidence-backed fix or greenlit feature — land after review label Sep 9, 2026

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.

🟢 Approval recommended

The targeted fix is correct, consistent with existing workflow commands, and covered by a regression test.

Pull request overview

Loads project-installed workflow steps before validating newly added workflows.

Changes:

  • Loads custom steps after resolving the project root.
  • Adds a regression test for the loading order and root argument.
File summaries
File Description
src/specify_cli/workflows/_commands.py Loads custom steps before workflow validation.
tests/test_workflows.py Verifies custom-step loading uses the resolved project root.
Review details
  • Files reviewed: 2/2 changed files
  • Comments generated: 0
  • Review effort level: Balanced

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

@mnriem mnriem added author-awaiting Waiting on author response author-needs-tests Real change but missing a regression test — add one that fails before / passes after labels Sep 9, 2026
@mnriem

mnriem commented Sep 9, 2026

Copy link
Copy Markdown
Collaborator

The fix itself is right (review's green, ubuntu passes), but your new regression test test_add_loads_custom_steps_after_resolving_project fails on macOS and Windows — CI shows AssertionError: assert [PosixPath('/...')] == [PosixPath('/...')] where the paths look identical but compare unequal. That's the macOS /tmp/private/tmp (and Windows short-path) symlink normalization: tempfile paths don't round-trip through .resolve(). Please make the assertion resolve both sides first — e.g. compare [p.resolve() for p in loaded_roots] == [tmp_path.resolve()] (or assert on the resolved project_root). Re-request review once CI is green across all runners

Assisted-by: GitHub Copilot (model: unknown, supervised)
@ton-katsu
ton-katsu force-pushed the fix/workflow-load-custom-steps branch from bfb33d7 to 900042a Compare September 10, 2026 01:44
@ton-katsu

Copy link
Copy Markdown
Author

@mnriem
The test fix is pushed.
I also ran the focused regression test on macOS and Linux, and it passed:

tests/test_workflows.py::TestWorkflowAddCaseInsensitiveSuffix::test_add_loads_custom_steps_after_resolving_project PASSED

The path comparison now resolves both paths before asserting equality.
Could a maintainer please approve or rerun the CI workflow so the Windows runners can verify the fix?

@mnriem
mnriem requested a balanced review from Copilot September 10, 2026 14:11
@mnriem mnriem removed the author-needs-tests Real change but missing a regression test — add one that fails before / passes after label Sep 10, 2026

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

The regression test does not verify that a workflow using a custom step validates and installs successfully.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details
  • Files reviewed: 2/2 changed files
  • Comments generated: 1
  • Review effort level: Balanced

Comment thread tests/test_workflows.py
assert result.exit_code == 0, result.output
assert "installed" in result.output

def test_add_loads_custom_steps_after_resolving_project(self, temp_dir, monkeypatch, sample_workflow_yaml):
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-nice-to-have Verdict: evidence-backed fix or greenlit feature — land after review

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants