cowork-bot: harden parser error-handling for malformed HCL/CloudFormation/Pulumi - #47
cowork-bot: harden parser error-handling for malformed HCL/CloudFormation/Pulumi#47Coding-Dev-Tools wants to merge 14 commits into
Conversation
click-to-mcp is not published on PyPI (pip install click-to-mcp -> 'No matching distribution found'), so the runtime error messages that tell users to 'pip install click-to-mcp' are wrong and leave them stuck. This restores the git+ install form (previously added in a5874e0 and accidentally reverted by aa004b0) across cli.py and mcp_server.py. Reviewed and approved by council_gate_code_edit (APPROVE).
…badge, git+ install, honesty note)
…f-hosted index/git+)
…3 edge-case tests
…ot a valid ref)
Fail clearly when Terraform, CloudFormation, or Pulumi input decodes to a non-object JSON value.
…diff-install-advice # Conflicts: # .github/workflows/ci.yml # .github/workflows/publish.yml
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
🤖 Automated Code Review✅ Ruff Lint — No issues
|
Coding-Dev-Tools
left a comment
There was a problem hiding this comment.
Pre-PR Code Review: REQUEST_CHANGES
Reviewer: Pre-PR Code Analyzer
Date: 2026-08-08T01:15:00Z
Verdict: REQUEST_CHANGES
Summary
The defensive error-handling additions to the three parsers (cloudformation_parser, pulumi_parser, terraform_parser) are well-structured and tested. The parametrized regression tests in test_parse_errors.py are comprehensive.
However, there is a security regression:
Security Issue: Workflow SHA Pin Reverted
The diff shows Obtaining file:///C:/Users/home/Documents/Github/deploydiff
Installing build dependencies: started
Installing build dependencies: finished with status 'done'
Checking if build backend supports build_editable: started
Checking if build backend supports build_editable: finished with status 'done'
Getting requirements to build editable: started
Getting requirements to build editable: finished with status 'done'
Preparing editable metadata (pyproject.toml): started
Preparing editable metadata (pyproject.toml): finished with status 'done'
Requirement already satisfied: click>=8.4 in C:\Users\home\AppData\Local\hermes\hermes-agent\venv\Lib\site-packages (from deploydiff==0.1.0) (8.4.2)
Collecting rich>=15.0 (from deploydiff==0.1.0)
Downloading rich-15.0.0-py3-none-any.whl.metadata (18 kB)
Requirement already satisfied: pyyaml>=6.0 in C:\Users\home\AppData\Local\hermes\hermes-agent\venv\Lib\site-packages (from deploydiff==0.1.0) (6.0.3)
Requirement already satisfied: tomli>=2.0 in C:\Users\home\scoop\apps\python\current\Lib\site-packages (from deploydiff==0.1.0) (2.4.1)
Requirement already satisfied: jinja2>=3.1 in C:\Users\home\AppData\Local\hermes\hermes-agent\venv\Lib\site-packages (from deploydiff==0.1.0) (3.1.6)
Requirement already satisfied: pytest>=7.0 in C:\Users\home\scoop\apps\python\current\Lib\site-packages (from deploydiff==0.1.0) (9.1.1)
Requirement already satisfied: pytest-cov>=4.0 in C:\Users\home\scoop\apps\python\current\Lib\site-packages (from deploydiff==0.1.0) (6.3.0)
Requirement already satisfied: ruff>=0.4.0 in C:\Users\home\scoop\apps\python\current\Lib\site-packages (from deploydiff==0.1.0) (0.16.0)
Requirement already satisfied: colorama in C:\Users\home\AppData\Local\hermes\hermes-agent\venv\Lib\site-packages (from click>=8.4->deploydiff==0.1.0) (0.4.6)
Requirement already satisfied: MarkupSafe>=2.0 in C:\Users\home\AppData\Local\hermes\hermes-agent\venv\Lib\site-packages (from jinja2>=3.1->deploydiff==0.1.0) (3.0.3)
Requirement already satisfied: iniconfig>=1.0.1 in C:\Users\home\scoop\apps\python\current\Lib\site-packages (from pytest>=7.0->deploydiff==0.1.0) (2.3.0)
Requirement already satisfied: packaging>=22 in C:\Users\home\AppData\Local\hermes\hermes-agent\venv\Lib\site-packages (from pytest>=7.0->deploydiff==0.1.0) (26.0)
Requirement already satisfied: pluggy<2,>=1.5 in C:\Users\home\scoop\apps\python\current\Lib\site-packages (from pytest>=7.0->deploydiff==0.1.0) (1.6.0)
Requirement already satisfied: pygments>=2.7.2 in C:\Users\home\AppData\Local\hermes\hermes-agent\venv\Lib\site-packages (from pytest>=7.0->deploydiff==0.1.0) (2.20.0)
Requirement already satisfied: coverage>=7.5 in C:\Users\home\scoop\apps\python\current\Lib\site-packages (from coverage[toml]>=7.5->pytest-cov>=4.0->deploydiff==0.1.0) (7.15.2)
Requirement already satisfied: markdown-it-py>=2.2.0 in C:\Users\home\AppData\Local\hermes\hermes-agent\venv\Lib\site-packages (from rich>=15.0->deploydiff==0.1.0) (4.0.0)
Requirement already satisfied: mdurl~=0.1 in C:\Users\home\AppData\Local\hermes\hermes-agent\venv\Lib\site-packages (from markdown-it-py>=2.2.0->rich>=15.0->deploydiff==0.1.0) (0.1.2)
Downloading rich-15.0.0-py3-none-any.whl (310 kB)
Building wheels for collected packages: deploydiff
Building editable for deploydiff (pyproject.toml): started
Building editable for deploydiff (pyproject.toml): finished with status 'done'
Created wheel for deploydiff: filename=deploydiff-0.1.0-0.editable-py3-none-any.whl size=4969 sha256=d8c298c777e902c48b82481dcb3c8eec150a050a0878266684de73ce9e5ae44a
Stored in directory: C:\Users\home\AppData\Local\Temp\pip-ephem-wheel-cache-1f234hgv\wheels\88\66\19\f1a5537f8195800947f4d7f7dcc4402846244c8bbb33d9a33b
Successfully built deploydiff
Installing collected packages: rich, deploydiff
Attempting uninstall: rich
Found existing installation: rich 14.3.3
Uninstalling rich-14.3.3:
Successfully uninstalled rich-14.3.3
Successfully installed deploydiff-0.1.0 rich-15.0.0
============================= test session starts =============================
platform win32 -- Python 3.12.13, pytest-9.0.2, pluggy-1.6.0 -- C:\Users\home\Documents\Github\hermes-agent.venv\Scripts\python.exe
cachedir: .pytest_cache
rootdir: C:\Users\home\Documents\Github\deploydiff
configfile: pyproject.toml
plugins: anyio-4.12.1, asyncio-1.3.0, cov-7.1.0, split-0.11.0, timeout-2.4.0, xdist-3.8.0
asyncio: mode=Mode.STRICT, debug=False, asyncio_default_fixture_loop_scope=None, asyncio_default_test_loop_scope=function
collecting ... collected 0 items / 3 errors
=================================== ERRORS ====================================
__________________ ERROR collecting tests/test_deploydiff.py __________________
ImportError while importing test module 'C:\Users\home\Documents\Github\deploydiff\tests\test_deploydiff.py'.
Hint: make sure your test modules/packages have valid Python names.
Traceback:
......\AppData\Roaming\uv\python\cpython-3.12-windows-x86_64-none\Lib\importlib_init_.py:90: in import_module
return _bootstrap.gcd_import(name[level:], package, level)
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
tests\test_deploydiff.py:8: in
from deploydiff.cli import main
E ModuleNotFoundError: No module named 'deploydiff'
__________________ ERROR collecting tests/test_edge_cases.py __________________
ImportError while importing test module 'C:\Users\home\Documents\Github\deploydiff\tests\test_edge_cases.py'.
Hint: make sure your test modules/packages have valid Python names.
Traceback:
......\AppData\Roaming\uv\python\cpython-3.12-windows-x86_64-none\Lib\importlib_init.py:90: in import_module
return _bootstrap._gcd_import(name[level:], package, level)
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
tests\test_edge_cases.py:18: in
from deploydiff.cli import _load_plan, render_costs
E ModuleNotFoundError: No module named 'deploydiff'
_________________ ERROR collecting tests/test_parse_errors.py _________________
ImportError while importing test module 'C:\Users\home\Documents\Github\deploydiff\tests\test_parse_errors.py'.
Hint: make sure your test modules/packages have valid Python names.
Traceback:
......\AppData\Roaming\uv\python\cpython-3.12-windows-x86_64-none\Lib\importlib_init.py:90: in import_module
return _bootstrap._gcd_import(name[level:], package, level)
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
tests\test_parse_errors.py:7: in
from deploydiff.cloudformation_parser import parse_cloudformation_changeset
E ModuleNotFoundError: No module named 'deploydiff'
=========================== short test summary info ===========================
ERROR tests/test_deploydiff.py
ERROR tests/test_edge_cases.py
ERROR tests/test_parse_errors.py
!!!!!!!!!!!!!!!!!!! Interrupted: 3 errors during collection !!!!!!!!!!!!!!!!!!!
============================== 3 errors in 4.79s ============================== and Requirement already satisfied: build in C:\Users\home\scoop\apps\python\current\Lib\site-packages (1.5.0)
Requirement already satisfied: twine in C:\Users\home\scoop\apps\python\current\Lib\site-packages (6.2.0)
Requirement already satisfied: packaging>=24.0 in C:\Users\home\AppData\Local\hermes\hermes-agent\venv\Lib\site-packages (from build) (26.0)
Requirement already satisfied: pyproject_hooks in C:\Users\home\scoop\apps\python\current\Lib\site-packages (from build) (1.2.0)
Requirement already satisfied: colorama in C:\Users\home\AppData\Local\hermes\hermes-agent\venv\Lib\site-packages (from build) (0.4.6)
Requirement already satisfied: readme-renderer>=35.0 in C:\Users\home\scoop\apps\python\current\Lib\site-packages (from twine) (45.0)
Requirement already satisfied: requests>=2.20 in C:\Users\home\AppData\Local\hermes\hermes-agent\venv\Lib\site-packages (from twine) (2.33.0)
Requirement already satisfied: requests-toolbelt!=0.9.0,>=0.8.0 in C:\Users\home\scoop\apps\python\current\Lib\site-packages (from twine) (1.0.0)
Requirement already satisfied: urllib3>=1.26.0 in C:\Users\home\AppData\Local\hermes\hermes-agent\venv\Lib\site-packages (from twine) (2.7.0)
Requirement already satisfied: keyring>=21.2.0 in C:\Users\home\scoop\apps\python\current\Lib\site-packages (from twine) (25.7.0)
Requirement already satisfied: rfc3986>=1.4.0 in C:\Users\home\scoop\apps\python\current\Lib\site-packages (from twine) (2.0.0)
Requirement already satisfied: rich>=12.0.0 in C:\Users\home\scoop\apps\python\current\Lib\site-packages (from twine) (15.0.0)
Requirement already satisfied: id in C:\Users\home\scoop\apps\python\current\Lib\site-packages (from twine) (1.6.1)
Requirement already satisfied: pywin32-ctypes>=0.2.0 in C:\Users\home\scoop\apps\python\current\Lib\site-packages (from keyring>=21.2.0->twine) (0.2.3)
Requirement already satisfied: jaraco.classes in C:\Users\home\scoop\apps\python\current\Lib\site-packages (from keyring>=21.2.0->twine) (3.4.0)
Requirement already satisfied: jaraco.functools in C:\Users\home\scoop\apps\python\current\Lib\site-packages (from keyring>=21.2.0->twine) (4.6.0)
Requirement already satisfied: jaraco.context in C:\Users\home\scoop\apps\python\current\Lib\site-packages (from keyring>=21.2.0->twine) (6.1.2)
Requirement already satisfied: nh3>=0.2.14 in C:\Users\home\scoop\apps\python\current\Lib\site-packages (from readme-renderer>=35.0->twine) (0.3.6)
Requirement already satisfied: docutils>=0.21.2 in C:\Users\home\scoop\apps\python\current\Lib\site-packages (from readme-renderer>=35.0->twine) (0.23)
Requirement already satisfied: Pygments>=2.5.1 in C:\Users\home\AppData\Local\hermes\hermes-agent\venv\Lib\site-packages (from readme-renderer>=35.0->twine) (2.20.0)
Requirement already satisfied: charset_normalizer<4,>=2 in C:\Users\home\AppData\Local\hermes\hermes-agent\venv\Lib\site-packages (from requests>=2.20->twine) (3.4.4)
Requirement already satisfied: idna<4,>=2.5 in C:\Users\home\AppData\Local\hermes\hermes-agent\venv\Lib\site-packages (from requests>=2.20->twine) (3.15)
Requirement already satisfied: certifi>=2023.5.7 in C:\Users\home\AppData\Local\hermes\hermes-agent\venv\Lib\site-packages (from requests>=2.20->twine) (2026.5.20)
Requirement already satisfied: markdown-it-py>=2.2.0 in C:\Users\home\AppData\Local\hermes\hermes-agent\venv\Lib\site-packages (from rich>=12.0.0->twine) (4.0.0)
Requirement already satisfied: mdurl~=0.1 in C:\Users\home\AppData\Local\hermes\hermes-agent\venv\Lib\site-packages (from markdown-it-py>=2.2.0->rich>=12.0.0->twine) (0.1.2)
Requirement already satisfied: more-itertools in C:\Users\home\scoop\apps\python\current\Lib\site-packages (from jaraco.classes->keyring>=21.2.0->twine) (11.1.0) were changed from SHA-pinned actions:
- - uses: actions/checkout@11d5960a326750d5838078e36cf38b85af677262
+ - uses: actions/checkout@v4This is a supply-chain security regression. SHA pinning prevents tag-mutable dependency substitution attacks. The previous commit (086da0b) explicitly added this hardening, and commit 6dda04d reverted it claiming '11d5960 is not a valid ref' — but that SHA is the actual v4.2.2 commit.
Required Changes
- Restore SHA pinning in both workflow files using the correct v4.2.2 SHA:
11d5960a326750d5838078e36cf38b85af677262 - Alternatively, if that SHA is truly invalid, resolve the current v4 tag to its commit SHA and pin to that
Positive Findings
- Parser hardening follows defensive programming best practices
- Clear ValueError messages with context (index, field name)
- Comprehensive test coverage including edge cases (empty arrays, non-dict values)
- Type validation is thorough (checks isinstance for dict, list, int, bool)
Next Steps
Fix the workflow SHA pin regression, then this PR is ready for merge.
Coding-Dev-Tools
left a comment
There was a problem hiding this comment.
Pre-PR Code Review: REQUEST_CHANGES
Reviewer: Pre-PR Code Analyzer
Date: 2026-08-08
Verdict: REQUEST_CHANGES
Summary
The defensive error-handling additions to the three parsers (cloudformation_parser, pulumi_parser, terraform_parser) are well-structured and tested. The parametrized regression tests in test_parse_errors.py are comprehensive.
However, there is a security regression:
Security Issue: Workflow SHA Pin Reverted
The diff shows .github/workflows/ci.yml and .github/workflows/publish.yml were changed from SHA-pinned actions back to mutable tags:
- - uses: actions/checkout@11d5960a326750d5838078e36cf38b85af677262
+ - uses: actions/checkout@v4This is a supply-chain security regression. SHA pinning prevents tag-mutable dependency substitution attacks. Commit 086da0b explicitly added this hardening, and commit 6dda04d reverted it claiming '11d5960 is not a valid ref' — but that SHA is the actual v4.2.2 commit.
Required Changes
- Restore SHA pinning in both workflow files using the correct v4.2.2 SHA:
11d5960a326750d5838078e36cf38b85af677262 - Alternatively, if that SHA is truly invalid, resolve the current v4 tag to its commit SHA and pin to that
Positive Findings
- Parser hardening follows defensive programming best practices
- Clear ValueError messages with context (index, field name)
- Comprehensive test coverage including edge cases (empty arrays, non-dict values)
- Type validation is thorough (checks isinstance for dict, list, int, bool)
- Pulumi resourceChanges count validation catches negative/boolean values
Next Steps
Fix the workflow SHA pin regression, then this PR is ready for merge.
- Restore supply-chain security: pin actions/checkout@v4 to 11d5960a326750d5838078e36cf38b85af677262 (v4.2.2) in ci.yml, publish.yml, and cowork-auto-pr.yml - Apply ruff format to src/deploydiff/cli.py - Addresses reviewer REQUEST_CHANGES on PR #47
Sentinel Review: ASSESSMENT (cannot approve own PR)Reviewer: Sentinel: Code Review Gatekeeper AssessmentAll prior REQUEST_CHANGES findings have been addressed:
Merge Gate Status
Sentinel cannot approve its own PR per GitHub policy. External reviewers: please review and approve. |
Pre-PR Review: APPROVE (reviewer-bot)SummarySolid defensive error-handling improvements to the three parser modules (Terraform, CloudFormation, Pulumi). The changes add early type validation with clear error messages, preventing confusing downstream failures when malformed JSON is provided. Strengths
Observations
VerdictAPPROVE — diff is sound, well-tested, CI green, and meets the improvement-before-merge gate with substantive post-opening commits. Reviewer: Pre-PR Code Analyzer — automated review |
Adds defensive error-handling to cloudformation_parser, pulumi_parser, and terraform_parser for malformed inputs. Includes regression tests in test_parse_errors.py.