Repository navigation
fix(heal): address Copilot review findings on heal CLI - #20
Merged
Merged
Conversation
…ts, stage only healed files - Reject model replacements that are not a chain of approved Playwright locator calls with literal args on the original receiver (ast-validated) - Accept a heal only if the exact target test ran and pytest exited 0; empty selection / collection error / missing report now reverts - git add/commit only healed files; --open-pr requires a clean tracked tree - Literal, in-process (pathlib) selector search instead of grep regex - Replace only the diagnosed occurrence; duplicate locators go to human review - Failures beyond --max are reported as untouched - HEAL_STORAGE_STATE for authenticated snapshots - Unit tests for all of the above
|
✅ All smoke tests passed! View report |
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
The validator accepts trailing comments that can suppress code following the replaced locator.
Review effort: Balanced
Findings: 1
What changed in this PR
Strengthens the locator-healing CLI’s safety, verification, source editing, and Git handling.
Changes:
- Validates model-generated locators and verifies exact test outcomes.
- Applies targeted edits, reports capped failures, and limits commits.
- Adds authentication-state support, documentation, and unit tests.
| File | Description |
|---|---|
scripts/heal.py |
Hardens healing and verification workflows. |
tests/unit/test_heal_cli.py |
Tests validation, editing, verification, and reversion. |
docs/USAGE.md |
Documents safety guarantees and authenticated snapshots. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+157
to
+163
| if not proposed or "\n" in proposed or ";" in proposed: | ||
| return None | ||
| try: | ||
| tree = ast.parse(proposed, mode="eval").body | ||
| original_tree = ast.parse(original, mode="eval").body | ||
| except SyntaxError: | ||
| return None |
|
✅ All chromium login tests passed! View report |
|
✅ All firefox login tests passed! View report |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Addresses the Copilot review findings on #16 (
scripts/heal.py).Fixed
ast-validated: single-line expression, chain of approved Playwright locator methods, literal arguments only, same receiver as the original. Anything else is discarded (reported "no usable replacement").verify_passedrequires the exact target test (name + classname) to have run, passed, and pytest to exit 0. No tests selected / collection error / no report => reverted.git commit -a- nowgit add -- <healed files>+git commit -- <healed files>;--open-pralso refuses to start on a dirty tracked tree.pathlibscan with literal matching.str.replacehits every occurrence - edit is applied by source span to the single located call; a selector appearing more than once is flagged ambiguous and left to a human.--maxdropped failures - capped failures are recorded as "not attempted" so they appear in the PR's human-review list.tests/unit/test_heal_cli.py(27 cases: extraction, literal/ambiguous location, validation incl. injection payloads, verification, revert-on-failure, malicious-output-never-written, --max).Partly addressed / pushed back
HEAL_STORAGE_STATEso authenticated pages are snapshotted as a signed-in user (documented), and the snapshot is only grounding - the verified re-run is the real gate, so a wrong snapshot yields a revert, not a bad change. Left as follow-up for true replay.Verified: unit suite 94 passed; ruff clean.