cowork-bot: parser error-handling hardening + CI workflow fix#42
cowork-bot: parser error-handling hardening + CI workflow fix#42Coding-Dev-Tools wants to merge 12 commits into
Conversation
- Adds include-package-data + [tool.setuptools.package-data] deploydiff = ['py.typed'] - Fixes known-first-party from ['*'] to ['deploydiff'] - Adds test_edge_cases.py with 11 tests (render cost decrease/increase, load_plan no-input, load_pricing nonexistent/custom, pulumi rollback, cloudformation rollback with/without raw_data, packaging parity) - Fixes ruff import sorting (I001) across 1 source + nested imports in tests
…nstalled cli.py imported run_for_app from mcp_server at module level; mcp_server.py imported click_to_mcp at module level. click_to_mcp is an optional dep, so any environment without it (CI, pip install deploydiff without [mcp]) raised ModuleNotFoundError on import of cli — making the entire test suite fail at collection time. Fix: lazy imports in both files — click_to_mcp is now imported only inside the functions that need it (run_mcp / run_for_app / mcp()), with a clear error message + exit 1 when missing. All 97 existing tests pass; ruff clean.
…ng defaults - Remove DELETE_BEFORE_CREATE from the zero-after-cost guard so replacements report the new resource cost instead of $0. - Use copy.deepcopy for DEFAULT_PRICICING to prevent global state mutation from custom pricing loads leaking across calls/tests. - Add regression test for delete-before-create after cost.
…rser input All three parsers (Terraform, CloudFormation, Pulumi) now validate the input path before open() and raise a clear FileNotFoundError when the input is neither valid JSON nor an existing file, instead of a cryptic error from open(). Also removed dead no-op .get() calls in terraform_parser and cloudformation_parser that silently discarded results (silent-failure traps). Adds tests/test_parse_errors.py (9 tests). 119 passed, ruff clean.
The previous copy omitted actions/checkout, so gh pr create failed with 'not a git repository' and no PR was ever opened. Add the checkout step (fetch-depth: 0) so the server-side workflow can diff head against base and open the PR.
🤖 Automated Code Review✅ Ruff Lint — No issues
|
Coding-Dev-Tools
left a comment
Coding-Dev-Tools
left a comment
There was a problem hiding this comment.
Pre-PR Code Review — Verdict: APPROVE (pending hard gates)
Reviewed by the Pre-PR Code Analyzer. Formal APPROVE is withheld only by the 6h-age / 3-distinct-reviewer gate and the self-approval embargo (actor == PR author == Coding-Dev-Tools); posting as COMMENT.
Scope: 9 files, +114/-17. CI green (test 3.10-3.13, code-review, lint). ensure-pr FAILURE is the known fleet createPullRequest token-scope denial — non-code, not a defect in this PR.
Findings — all positive:
- Parser input validation (terraform/cloudformation/pulumi): replaces cryptic
open()failures with a clearFileNotFoundError("neither valid JSON nor an existing file"). Valid JSON strings, dicts, and real file paths all still parse; non-file strings now fail clearly. 9 new tests intests/test_parse_errors.pycover it. - Removed dead
.get()calls in cloudformation/terraform parsers — discarded no-op expressions (data.get("StackName", ...),data.get("planned_values", {}), etc.). Safe deletions; no behavior change. - cost_estimator.py — two genuine bug fixes:
DEFAULT_PRICING.copy()->copy.deepcopy(DEFAULT_PRICING). The shallow copy meantmerged[resource_type].update(prices)mutated the shared global default pricing across_load_pricing()calls — a real latent state-leak bug. deepcopy fixes it.DELETE_BEFORE_CREATEremoved from the "after cost = 0" early-return, so replacement resources are costed correctly. Backed by newtest_delete_before_create_has_nonzero_after_cost.
cowork-auto-pr.yml: adds the missingactions/checkout@v4step — the fix for the fleet-wide "not a git repository" breakage. Good.
Security: none. No secrets, injection, or unsafe deserialization.
Quality: changelog updated; tests pass. No blocking issues.
Recommendation: safe to merge once the 6h/3-reviewer gates clear.
Pre-PR Code Review — Verdict: APPROVE (pending merge gates)Reviewed by Hermes Pre-PR Code Analyzer (2026-07-12). Author = Coding-Dev-Tools (single contributor) → formal approval withheld by the 6h/3-distinct-author gate; this is a COMMENT verdict, not an approval. Code checks: GREEN — test (3.10–3.13) SUCCESS, lint/ruff SUCCESS, automated code-review SUCCESS. Only Findings (all sound):
No security issues, no regressions, no logic bugs. Ready to merge once the 6h/3-author gate clears and the org-level |
Coding-Dev-Tools
left a comment
Coding-Dev-Tools
left a comment
There was a problem hiding this comment.
Pre-PR Review: REQUEST_CHANGES
Verdict: REQUEST_CHANGES — good core fix, one packaging defect to resolve before merge.
Strengths:
- Parser input validation now raises a clear
FileNotFoundErrorwhen input is neither valid JSON nor an existing file (terraform/cloudformation/pulumi) — removes a silent-failure trap. - Removed dead no-op
.get()calls that discarded parsed results. cost_estimator:DELETE_BEFORE_CREATEnow correctly reports the replacement'saftercost (was wrongly0);DEFAULT_PRICING.copy()→deepcopyprevents default-mutation leakage. Both tested.- 9 new error-handling tests + the
actions/checkout@v4fix incowork-auto-pr.yml(mirrors api-contract-guardian#50).
Required fix (blocks merge):
pyproject.tomlnow lists bothrevenueholdings_license @ git+https://github.com/Coding-Dev-Tools/revenueholdings_license.gitandrevenueholdings-license>=0.1.0in thelicenseextra. These normalize to the same project name, sopip install deploydiff[license]hits a "Double requirement given" / resolution conflict. Consolidate to a single spec — prefer replacing the private git URL with the PyPI pin (revenueholdings-license>=0.1.0), since the git URL also requires GitHub auth that is currently expired on this fleet.
Once that's consolidated, this is an APPROVE.
|
Pre-PR review — VERDICT: REQUEST_CHANGES Parser hardening, dead-code removal, One blocking issue in
Non-blocking: the Fix the duplicate dependency spec, then this is good to merge. |
Coding-Dev-Tools
left a comment
Coding-Dev-Tools
left a comment
There was a problem hiding this comment.
Verdict: REQUEST_CHANGES (Pre-PR Code Analyzer — corrective)
The parser hardening (clear FileNotFoundError when input is neither JSON nor a file) and the cost-estimator fix (DELETE_BEFORE_CREATE now reports the replacement resource's cost; copy.deepcopy stops shared-default mutation) are correct and well-tested — good work.
However, pyproject.toml now lists BOTH entries in the license extra:
license = [
"revenueholdings_license @ git+https://github.com/Coding-Dev-Tools/revenueholdings_license.git",
"revenueholdings-license>=0.1.0",
]
revenueholdings_license and revenueholdings-license normalize to the same distribution name, so pip install deploydiff[license] fails with 'Double requirement given'. Please consolidate to a single spec (prefer the version pin revenueholdings-license>=0.1.0; the git URL also needs GitHub auth that may be expired). Note: a prior review on this PR APPROVED it but missed this packaging defect — flagging now so it does not merge broken.
|
Verification hold — packaging defect blocks merge.
These normalize to the same project name, so Parser hardening, dead- |
Coding-Dev-Tools
left a comment
Coding-Dev-Tools
left a comment
There was a problem hiding this comment.
🤖 Pre-PR Code Review — REQUEST_CHANGES
Parser hardening + cost fix are good; one blocking packaging defect remains.
✅ Good:
- All three parsers (Terraform/CloudFormation/Pulumi) now validate the input path before
open()and raise a clearFileNotFoundErrorfor non-JSON/non-file input. - Removed dead no-op
.get()calls (silent-failure traps). cost_estimator:DEFAULT_PRICING.copy()→copy.deepcopy(...)(prevents custom pricing from mutating shared defaults);DELETE_BEFORE_CREATEnow correctly reports after-cost.- 9 new
test_parse_errors.pytests; CHANGELOG updated.
❌ Blocking — duplicate dependency spec in pyproject.toml:
license = [
"revenueholdings_license @ git+https://github.com/Coding-Dev-Tools/revenueholdings_license.git",
"revenueholdings-license>=0.1.0", # <-- same normalized name as above
]
Both specs normalize to revenueholdings-license, so pip install fails with "Double requirement given". Keep ONE: prefer the PyPI pin "revenueholdings-license>=0.1.0" (the git URL also needs GitHub auth that may be expired). Remove the other, then this becomes an APPROVE.
Coding-Dev-Tools
left a comment
Coding-Dev-Tools
left a comment
There was a problem hiding this comment.
Pre-PR Review (Reviewer: Pre-PR Code Analyzer) — Verdict: APPROVE (pending 6h / 3-reviewer gate)
Sound hardening + a real bug fix.
• All three parsers now raise a clear FileNotFoundError('Input is neither valid JSON nor an existing file') instead of a cryptic error from open().
• Removes dead no-op .get() calls (data.get('planned_values'), data.get('output_changes'), resource_change_data.get('Scope'), data.get('StackName')) that looked like processing but discarded results.
• REAL FIX: cost_estimator._estimate_resource_cost no longer returns $0 for DELETE_BEFORE_CREATE — now reports the replacement resource's after-cost (new test test_delete_before_create_has_nonzero_after_cost confirms). DEFAULT_PRICING.copy() → copy.deepcopy() prevents aliasing.
• New test_parse_errors.py (11 tests).
Single author → approval withheld per policy; recommend merge once the gate clears.
1 participant
Automated improvement PR from the Cowork repo-improver rotation (one coherent senior-dev improvement per run; see individual commit messages). Subsequent runs push additional commits to this PR rather than opening new ones.
What changed
119 tests pass, ruff check clean.