-
Notifications
You must be signed in to change notification settings - Fork 0
Add check-workflow command with uppercase name enforcement #66
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
base: main
Are you sure you want to change the base?
Conversation
Co-authored-by: tschm <2046079+tschm@users.noreply.github.com>
Co-authored-by: tschm <2046079+tschm@users.noreply.github.com>
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.
Pull request overview
Adds a new check-workflow CLI command to validate (and auto-fix) GitHub Actions workflow name: fields so they start with the (RHIZA) prefix.
Changes:
- Added
pyyamldependency and module mapping for tooling. - Implemented
check_file/check_workflow_commandto validate/update workflow YAML files. - Registered the command in the Typer CLI and added unit + CLI integration tests.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
pyproject.toml |
Adds pyyaml dependency and deptry module map entry to support YAML parsing. |
src/rhiza_tools/commands/check_workflow.py |
Implements workflow name prefix validation and in-place update behavior. |
src/rhiza_tools/cli.py |
Registers check-workflow command and routes to check_workflow_command. |
tests/test_cli_commands.py |
Adds CLI wiring test for the new check-workflow command. |
tests/test_check_workflow_command.py |
Adds unit tests for check_file and check_workflow_command. |
| # We'll use quotes to be safe | ||
| f_write.write(f'name: "(RHIZA) {name}"\n') |
Copilot
AI
Jan 27, 2026
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.
When writing the updated name: line, the code interpolates the raw parsed name into a double-quoted YAML string. If the workflow name contains characters that need escaping (notably " or backslashes), this will emit invalid YAML and can corrupt the workflow file. Please serialize/escape the new name safely (e.g., generate an escaped scalar value) before writing.
| # We'll use quotes to be safe | |
| f_write.write(f'name: "(RHIZA) {name}"\n') | |
| # Safely serialize the new name as a YAML scalar with proper escaping | |
| new_name = f"(RHIZA) {name}" | |
| serialized_name = yaml.safe_dump(new_name, default_style='"').strip() | |
| f_write.write(f"name: {serialized_name}\n") |
| if not name: | ||
| logger.error(f"Error: {filepath} missing 'name' field.") | ||
| return False | ||
|
|
Copilot
AI
Jan 27, 2026
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.
name is assumed to be a string (via .startswith). If the YAML name value is non-string (e.g., unquoted name: on/yes can parse as a bool under YAML 1.1, or a number), this will raise at runtime. Consider validating isinstance(name, str) and returning a clear error when it isn’t.
| if not isinstance(name, str): | |
| logger.error( | |
| f"Error: {filepath} has non-string 'name' field " | |
| f"of type {type(name).__name__!s}: {name!r}" | |
| ) | |
| return False |
| replaced = False | ||
| for line in lines: | ||
| # Replace only the top-level name field (assumes it starts at beginning of line) | ||
| if not replaced and line.startswith("name:"): | ||
| # Check if this line corresponds to the extracted name. |
Copilot
AI
Jan 27, 2026
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.
The update path only replaces a line that literally starts with name:. If the top-level key is formatted differently (e.g., name : ...) or has leading indentation, replaced will stay False and the file will be rewritten unchanged while check_file still returns False. Consider making the match more robust (allow whitespace) and/or explicitly error if no replacement was performed.
tests/test_check_workflow_command.py
Outdated
| def test_missing_name_field_returns_false(self, tmp_path: Path, capsys: pytest.CaptureFixture[str]) -> None: | ||
| """File without name field returns False with error message.""" | ||
| workflow = tmp_path / "workflow.yml" | ||
| workflow.write_text("on: push\njobs:\n test:\n runs-on: ubuntu-latest\n") | ||
|
|
||
| result = check_file(str(workflow)) | ||
|
|
||
| assert result is False |
Copilot
AI
Jan 27, 2026
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.
This test’s docstring says it checks for an “error message” and it accepts capsys, but nothing asserts on captured output/logging (and capsys is unused). Either assert on the emitted error (e.g., capture Loguru output) or drop/update the fixture/docstring to match what’s actually tested.
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.
@copilot apply changes based on this feedback
tests/test_check_workflow_command.py
Outdated
| workflow = tmp_path / "workflow.yml" | ||
| workflow.write_text(original) | ||
|
|
||
| check_file(str(workflow)) |
Copilot
AI
Jan 27, 2026
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.
This test expects check_file(...) to perform an update (missing prefix), but it doesn’t assert the return value. Asserting that it returns False would keep the “signals update via return value” contract covered.
| check_file(str(workflow)) | |
| result = check_file(str(workflow)) | |
| assert result is False |
tests/test_check_workflow_command.py
Outdated
| workflow = tmp_path / "workflow.yml" | ||
| workflow.write_text("name: Build & Deploy\non: push\n") | ||
|
|
||
| check_file(str(workflow)) |
Copilot
AI
Jan 27, 2026
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.
This test triggers an update path but doesn’t assert the check_file(...) return value. Adding an assertion that it returns False would make the behavior under test explicit.
| check_file(str(workflow)) | |
| result = check_file(str(workflow)) | |
| assert result is False |
tests/test_check_workflow_command.py
Outdated
| def test_invalid_yaml_returns_false(self, tmp_path: Path, capsys: pytest.CaptureFixture[str]) -> None: | ||
| """Invalid YAML returns False with error message.""" | ||
| workflow = tmp_path / "workflow.yml" | ||
| workflow.write_text("name: test\n invalid: yaml: syntax:\n") | ||
|
|
||
| result = check_file(str(workflow)) | ||
|
|
||
| assert result is False |
Copilot
AI
Jan 27, 2026
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.
This test’s docstring says it checks for an “error message” and it accepts capsys, but it only asserts the boolean result (and capsys is unused). Consider asserting on the logged error output (or updating/removing the docstring/fixture).
|
@copilot apply changes based on the comments in this thread |
|
@copilot: enhance the check_workflow following: #!/usr/bin/env python3 import sys import yaml def check_file(filepath): def main(): if name == "main": |
…mproved tests Co-authored-by: tschm <2046079+tschm@users.noreply.github.com>
All changes from the review have been applied in commit 3b7f8f6: Fixed issues:
New test coverage:
All 18 tests pass (21 including new edge case tests). |
Co-authored-by: tschm <2046079+tschm@users.noreply.github.com>
|
@copilot apply changes based on the comments in this thread |
Co-authored-by: tschm <2046079+tschm@users.noreply.github.com>
Enhanced in commit 1d0bd80. The command now:
Examples:
All 19 tests pass including a new test for the lowercase-to-uppercase conversion case. |
All changes from review #3709462284 have already been applied in commits 3b7f8f6, f68d811, and 1d0bd80. The implementation includes:
All 19 tests pass. |
|
@copilot apply changes based on the comments in this thread |
All changes from review #3709462284 remain applied in the current codebase (commits 3b7f8f6, f68d811, 1d0bd80). The implementation includes all requested fixes: Current HEAD: 8858eb5 (includes formatting updates). All 19 tests pass. |
|
@HarryCampion I guess we merge, make a new release and then remove the equivalent code from rhiza? |
I am planning to move this into rhiza-hooks. This is more of a hook than a rhiza cli command if I am honest. We might end up with a bloated CLI. |
hooks are solely things run by pre-commit, rhiza tools/cli are fundamental to rhiza framework. |
|
Ok, makes sense |
Implementation Complete ✅
Changes Made
pyyamldependency to pyproject.tomlsrc/rhiza_tools/commands/check_workflow.pysrc/rhiza_tools/cli.pytests/test_cli_commands.pytests/test_check_workflow_command.pyyaml.safe_dump()for proper handling of quotes and backslashescapsysfixtures from testsCommand Behavior
The
check-workflowcommand validates and updates GitHub Actions workflow files to ensure they follow the naming convention:(RHIZA) UPPERCASE_NAMEExamples:
My Workflow→(RHIZA) MY WORKFLOW(RHIZA) lowercase workflow→(RHIZA) LOWERCASE WORKFLOW(RHIZA) MY WORKFLOW→ no change (already correct)Test Coverage
19 tests total covering:
Original prompt
💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more Copilot coding agent tips in the docs.