Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
24 changes: 22 additions & 2 deletions .github/workflows/release.yml
Original file line number Diff line number Diff line change
Expand Up @@ -20,11 +20,31 @@ jobs:
- uses: actions/checkout@v4
- uses: astral-sh/setup-uv@v5
- run: uv build
# Publish with PEP 740 attestations β€” PyPI records signed build provenance
# (repo + workflow + commit) that anyone can verify. No token, OIDC only.
- uses: pypa/gh-action-pypi-publish@release/v1
- name: GitHub release
with:
attestations: true
# CycloneDX SBOM of the built wheel (best-effort; never blocks a release).
- name: SBOM (CycloneDX)
continue-on-error: true
run: |
uv venv /tmp/sbomenv
uv pip install --python /tmp/sbomenv dist/*.whl
uvx --from cyclonedx-bom cyclonedx-py environment /tmp/sbomenv -o 9lives.cdx.json
# Sigstore-sign the artifacts; the .sigstore bundles ship on the release
# so installs from GitHub can be cryptographically verified too.
- name: Sign artifacts with Sigstore
uses: sigstore/gh-action-sigstore-python@v3.0.0
with:
inputs: ./dist/*.whl ./dist/*.tar.gz
- name: GitHub release (artifacts + signatures + SBOM)
env:
GH_TOKEN: ${{ github.token }}
run: gh release create "$GITHUB_REF_NAME" dist/* --generate-notes

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 LOGICGROUNDED SBOM failure blocks release despite 'best-effort' comment

The GitHub release step references the literal path 9lives.cdx.json, but that file is only created if the preceding SBOM step succeeds. Because shopt -s nullglob only affects glob expansions, a missing SBOM file causes gh release create to fail, contradicting the SBOM step's continue-on-error: true and the 'best-effort; never blocks a release' comment. Change the literal path to a glob so nullglob can skip it when the SBOM step fails.

Example:

SBOM step fails (e.g., cyclonedx-bom install error).
Release step runs: gh release create v0.1.3 --generate-notes dist/*.whl dist/*.tar.gz dist/*.sigstore* 9lives.cdx.json
Result: gh: 9lives.cdx.json: no such file or directory
The release is not created despite the 'best-effort; never blocks a release' intent.

Current:

          attestations: true

Proposed:

            dist/*.whl dist/*.tar.gz dist/*.sigstore* *.cdx.json
Suggested change
run: gh release create "$GITHUB_REF_NAME" dist/* --generate-notes
dist/*.whl dist/*.tar.gz dist/*.sigstore* *.cdx.json
More Info
  • Threat model: A transient failure in the CycloneDX SBOM generator (network, tool install, etc.) prevents the release from being published, even though the build and signatures are ready.
  • Specific code citations: The SBOM step at .github/workflows/release.yml uses continue-on-error: true and writes 9lives.cdx.json; the release step on the next block passes that literal filename to gh release create.
  • Existing protections: continue-on-error: true on the SBOM step allows the job to continue, but the release step has no conditional guard for the literal file path.
  • Proposed mitigation: Change the literal 9lives.cdx.json to a glob like *.cdx.json so shopt -s nullglob skips it when the SBOM step fails, or build the file list conditionally before calling gh release create.
  • Alternative mitigations considered: Moving the SBOM generation into a separate job would also isolate failures, but is broader than needed for this one-line fix.
  • Severity calibration: Score 4 because it breaks the release pipeline on a non-essential step, which is a real operational failure under plausible conditions (SBOM tool flakiness), but it does not affect runtime correctness or security.
Prompt To Fix With AI
This is a comment left during a code review.
Path: .github/workflows/release.yml
Line: 27

Comment:
**SBOM failure blocks release despite 'best-effort' comment**

The GitHub release step references the literal path `9lives.cdx.json`, but that file is only created if the preceding SBOM step succeeds. Because `shopt -s nullglob` only affects glob expansions, a missing SBOM file causes `gh release create` to fail, contradicting the SBOM step's `continue-on-error: true` and the 'best-effort; never blocks a release' comment. Change the literal path to a glob so nullglob can skip it when the SBOM step fails.

Example:
SBOM step fails (e.g., cyclonedx-bom install error).
Release step runs: gh release create v0.1.3 --generate-notes dist/*.whl dist/*.tar.gz dist/*.sigstore* 9lives.cdx.json
Result: gh: 9lives.cdx.json: no such file or directory
The release is not created despite the 'best-effort; never blocks a release' intent.

Threat model:
A transient failure in the CycloneDX SBOM generator (network, tool install, etc.) prevents the release from being published, even though the build and signatures are ready.

Specific code citations:
The SBOM step at `.github/workflows/release.yml` uses `continue-on-error: true` and writes `9lives.cdx.json`; the release step on the next block passes that literal filename to `gh release create`.

Existing protections:
`continue-on-error: true` on the SBOM step allows the job to continue, but the release step has no conditional guard for the literal file path.

Proposed mitigation:
Change the literal `9lives.cdx.json` to a glob like `*.cdx.json` so `shopt -s nullglob` skips it when the SBOM step fails, or build the file list conditionally before calling `gh release create`.

Alternative mitigations considered:
Moving the SBOM generation into a separate job would also isolate failures, but is broader than needed for this one-line fix.

Severity calibration:
Score 4 because it breaks the release pipeline on a non-essential step, which is a real operational failure under plausible conditions (SBOM tool flakiness), but it does not affect runtime correctness or security.

How can I resolve this? If you propose a fix, please make it concise.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

βœ… Confirmed real (P1) β€” fixed in #7. The literal 9lives.cdx.json is now *.cdx.json so nullglob drops it when the best-effort SBOM step fails. v0.1.3 only shipped because that step happened to succeed. Good catch.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Confirmed β€” the P1 is resolved. The fix in #7 changed the literal 9lives.cdx.json path in .github/workflows/release.yml:27 to *.cdx.json, so shopt -s nullglob (already set earlier in the step) correctly drops the argument when the best-effort SBOM step fails and produces no file. v0.1.3 only shipped cleanly because the SBOM step happened to succeed; without the glob, any SBOM failure would have crashed gh release create despite continue-on-error: true. No further action needed on this finding.

run: |
shopt -s nullglob
gh release create "$GITHUB_REF_NAME" --generate-notes \
dist/*.whl dist/*.tar.gz dist/*.sigstore* 9lives.cdx.json

action-major-tag:
# Keep `uses: quality-max/9lives/action@v1` pointing at the newest v1.x.y
Expand Down
2 changes: 2 additions & 0 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,8 @@ Your coding agent shipped a change and your Playwright test went red? Don't rewr
2. **Tier 2 β€” the subscription you already pay for.** Structural change? 9lives shells out to your installed coding-agent CLI β€” **Claude Code (`claude -p`), Codex (`codex exec`), or OpenCode (`opencode run`)** β€” so your existing subscription does the thinking. No API key to mint, nothing to configure: if the CLI is logged in, healing works. (Prefer raw API? `ANTHROPIC_API_KEY` / `OPENAI_API_KEY` work too.)
3. **Always a diff, never a surprise.** Healed code is shown as a unified diff and applied only when you approve (or `--yes` in CI).

**Won't hide your bugs.** A failing *assertion* means the app's behavior changed β€” not that a selector moved. 9lives refuses to rewrite assertions to force a green (that's how naive auto-healers mask regressions) and flags it as a possible real bug instead. Opt in with `NINELIVES_HEAL_ASSERTIONS=1` if you really want it to propose an assertion update.

## Install

```bash
Expand Down
2 changes: 1 addition & 1 deletion pyproject.toml
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ build-backend = "hatchling.build"

[project]
name = "9lives"
version = "0.1.2"
version = "0.1.3"
description = "Self-healing QA for the coding-agent era. Your tests have nine lives."
readme = "README.md"
license = { text = "MIT" }
Expand Down
11 changes: 9 additions & 2 deletions src/ninelives/cli.py
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@
from . import __version__
from .healing.parse import extract_failed_selector
from .healing.patch import diff_stats, generate_unified_diff
from .healing.strategy import HealingTier, TestFailure, healing_strategy_selector
from .healing.strategy import FailureType, HealingTier, TestFailure, healing_strategy_selector
from .healing.tier1 import tier1_healer
from .healing.tier2 import Tier2AISuggest
from .llm.agent_cli import detect_agent_clis
Expand Down Expand Up @@ -194,7 +194,14 @@ def _heal_loop(spec: Path, working_spec: Path, *, auto_apply: bool, max_iteratio
elif tier == HealingTier.TIER2_AI_SUGGEST:

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 LOGICGROUNDED Tier 3 handler silently treats all failure types the same when NINELIVES_HEAL_ASSERTIONS is not set

The _heal_loop function now branches on failure.failure_type == FailureType.ASSERTION_FAILED to print a special message, but the else branch prints a generic message for all other failure types. If a new FailureType is added in the future, it will silently fall into the generic message without any explicit handling, which could mask a missing branch. The fix is to add an explicit elif for each known failure type or a final else that raises an error for unknown types.

Example:

A new FailureType.TIMEOUT is added to the enum. The select_strategy method returns HealingTier.TIER3_HUMAN for it. The _heal_loop handler prints the generic message instead of a timeout-specific message, and no one notices the missing branch.

Suggested fix:

if failure.failure_type == FailureType.ASSERTION_FAILED:
    ...
elif failure.failure_type == FailureType.LOCATOR_NOT_FOUND:
    print(f"{PAW} this failure needs a human ({tier.value}) β€” no automatic fix attempted.")
else:
    raise ValueError(f"Unknown failure type: {failure.failure_type}")
More Info
  • Threat model: A future developer adds a new FailureType enum value and forgets to update this handler. The new failure type will be silently treated as a generic human-needed failure, potentially hiding a bug or missing a special handling path.
  • Specific code citations: src/ninelives/cli.py line 194-214: the else branch in _heal_loop that handles HealingTier.TIER3_HUMAN and other tiers. src/ninelives/healing/strategy.py lines 199-212: the select_strategy method that returns HealingTier.TIER3_HUMAN for FailureType.ASSERTION_FAILED when the opt-in is not set.
  • Existing protections: The select_strategy method explicitly handles FailureType.ASSERTION_FAILED and FailureType.LOCATOR_NOT_FOUND, but the consumer in _heal_loop does not mirror this exhaustiveness.
  • Proposed mitigation: Add an explicit elif failure.failure_type == FailureType.LOCATOR_NOT_FOUND: branch before the else, or use a match statement to ensure all arms are covered.
  • Alternative mitigations considered: A match statement would provide exhaustiveness checking at the language level, but a simple elif chain is sufficient for the current two-arm enum.
  • Severity calibration: This is a maintainability issue, not a runtime bug. The current code works correctly for the two existing failure types, but the lack of exhaustiveness makes future changes error-prone. Score 3 reflects the moderate risk of silent misbehavior when the enum grows.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/ninelives/cli.py
Line: 194

Comment:
**Tier 3 handler silently treats all failure types the same when NINELIVES_HEAL_ASSERTIONS is not set**

The `_heal_loop` function now branches on `failure.failure_type == FailureType.ASSERTION_FAILED` to print a special message, but the `else` branch prints a generic message for all other failure types. If a new `FailureType` is added in the future, it will silently fall into the generic message without any explicit handling, which could mask a missing branch. The fix is to add an explicit `elif` for each known failure type or a final `else` that raises an error for unknown types.

Example:
A new FailureType.TIMEOUT is added to the enum. The select_strategy method returns HealingTier.TIER3_HUMAN for it. The _heal_loop handler prints the generic message instead of a timeout-specific message, and no one notices the missing branch.

Threat model:
A future developer adds a new `FailureType` enum value and forgets to update this handler. The new failure type will be silently treated as a generic human-needed failure, potentially hiding a bug or missing a special handling path.

Specific code citations:
`src/ninelives/cli.py` line 194-214: the `else` branch in `_heal_loop` that handles `HealingTier.TIER3_HUMAN` and other tiers. `src/ninelives/healing/strategy.py` lines 199-212: the `select_strategy` method that returns `HealingTier.TIER3_HUMAN` for `FailureType.ASSERTION_FAILED` when the opt-in is not set.

Existing protections:
The `select_strategy` method explicitly handles `FailureType.ASSERTION_FAILED` and `FailureType.LOCATOR_NOT_FOUND`, but the consumer in `_heal_loop` does not mirror this exhaustiveness.

Proposed mitigation:
Add an explicit `elif failure.failure_type == FailureType.LOCATOR_NOT_FOUND:` branch before the `else`, or use a match statement to ensure all arms are covered.

Alternative mitigations considered:
A `match` statement would provide exhaustiveness checking at the language level, but a simple `elif` chain is sufficient for the current two-arm enum.

Severity calibration:
This is a maintainability issue, not a runtime bug. The current code works correctly for the two existing failure types, but the lack of exhaustiveness makes future changes error-prone. Score 3 reflects the moderate risk of silent misbehavior when the enum grows.

Suggested fix shape:
if failure.failure_type == FailureType.ASSERTION_FAILED:
    ...
elif failure.failure_type == FailureType.LOCATOR_NOT_FOUND:
    print(f"{PAW} this failure needs a human ({tier.value}) β€” no automatic fix attempted.")
else:
    raise ValueError(f"Unknown failure type: {failure.failure_type}")

How can I resolve this? If you propose a fix, please make it concise.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Dismissing. The generic "needs a human" message for all non-assertion Tier-3 types (navigation, flow-changed) is the intended graceful fallback β€” only the assertion case warrants the special "possible real bug" wording. The suggested fix (raise on an unknown FailureType) would make the CLI crash on a failure it could otherwise report cleanly β€” strictly worse UX for an end-user tool. If we grow the enum and want per-type copy, that's a deliberate follow-up, not an exhaustiveness guard that aborts.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're right, the original finding was wrong. The code is intentionally designed this way.

Looking at the full picture:

  1. The strategy selector already gates exhaustiveness. select_strategy in src/ninelives/healing/strategy.py:191-208 explicitly handles every FailureType that should route to Tier 1 or Tier 2. The remaining types (NAVIGATION_FAILED, FLOW_CHANGED, NETWORK_ERROR, SYNTAX_ERROR, UNKNOWN) all fall through to TIER3_HUMAN β€” that's the correct, deliberate default.

  2. The CLI's else branch is the Tier 3 handler. At src/ninelives/cli.py:194-205, the if failure.failure_type == FailureType.ASSERTION_FAILED check carves out the one special message ("possible real bug β€” behavior changed, not a selector"), and the else prints the generic "needs a human" message for all other Tier 3 types. That's the right UX: only assertions get the extra warning about hidden bugs; everything else that escalates to a human is reported cleanly without crashing.

  3. raise on an unknown FailureType would be strictly worse. The CLI is an end-user tool. Crashing on a failure it could report is bad UX. The graceful fallback is the feature, not a bug.

  4. If the enum grows, the strategy selector is the right place to add per-type routing, and the CLI message can get per-type copy as a deliberate follow-up. An exhaustiveness guard that aborts is not appropriate here.

Retracting the finding. The else branch at cli.py:197 is the intended graceful fallback for all non-assertion Tier 3 failures.

healing = asyncio.run(tier2.suggest(failure))
else:
print(f"{PAW} this failure needs a human ({tier.value}) β€” no automatic fix attempted.")
if failure.failure_type == FailureType.ASSERTION_FAILED:
# Behavior-vs-drift guard: never rewrite a failing assertion to
# force a pass β€” that hides a real bug. Flag it for a human.
print(f"{PAW} possible real bug β€” an assertion failed (behavior changed, not a selector).")
print(" 9lives won't rewrite assertions to force a pass. Review it, or set")
print(" NINELIVES_HEAL_ASSERTIONS=1 to let Tier 2 propose an assertion update.")
else:
print(f"{PAW} this failure needs a human ({tier.value}) β€” no automatic fix attempted.")
return SpecOutcome(
spec=spec.name,
status="needs-human",
Expand Down
7 changes: 6 additions & 1 deletion src/ninelives/healing/strategy.py
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
"""

import logging
import os
import re
from dataclasses import dataclass, field
from enum import Enum
Expand Down Expand Up @@ -198,8 +199,12 @@ def select_strategy(self, failure: TestFailure) -> HealingTier:
return HealingTier.TIER1_AUTO
return HealingTier.TIER2_AI_SUGGEST

# Behavior-vs-drift guard: an assertion failure means the app behaved
# differently β€” not that a selector moved. Auto-rewriting the assertion
# to force a pass would hide a real bug, so 9lives never heals it and
# hands it to a human. Opt in explicitly with NINELIVES_HEAL_ASSERTIONS=1.
if failure_type == FailureType.ASSERTION_FAILED:
if self._is_value_change(failure):
if os.environ.get("NINELIVES_HEAL_ASSERTIONS") == "1" and self._is_value_change(failure):
return HealingTier.TIER2_AI_SUGGEST
return HealingTier.TIER3_HUMAN

Expand Down
44 changes: 27 additions & 17 deletions src/ninelives/healing/tier1.py
Original file line number Diff line number Diff line change
Expand Up @@ -31,17 +31,19 @@ async def heal(self, failure: TestFailure) -> HealingResult:
if not failure.test_code:
return self._no_heal_result("No test code provided")

alternative = self._find_alternative_selector(failure)
if alternative == failure.failed_selector:
alternative = None # same selector is not a heal
if alternative:
found = self._find_alternative_selector(failure)
if found and found[0] == failure.failed_selector:
found = None # same selector is not a heal
if found:
alternative, anchor = found
healed_code = self._replace_selector(failure.test_code, failure.failed_selector, alternative)
return HealingResult(
tier=HealingTier.TIER1_AUTO,
success=True,
healed_code=healed_code,
changes_made=[f"Replaced selector '{failure.failed_selector}' with '{alternative}'"],
changes_made=[f"Re-found via {anchor}: replaced '{failure.failed_selector}' with '{alternative}'"],
confidence=0.85,
metadata={"anchor": anchor},
)

transformed = self._try_transformations(failure.failed_selector)
Expand All @@ -67,24 +69,28 @@ async def heal(self, failure: TestFailure) -> HealingResult:

return self._no_heal_result("Could not find working alternative")

def _find_alternative_selector(self, failure: TestFailure) -> str | None:
"""Find an alternative selector in the captured page state."""
def _find_alternative_selector(self, failure: TestFailure) -> tuple[str, str] | None:
"""Find an alternative selector in the captured page state.

Returns (selector, anchor_type) β€” the anchor that re-identified the
element β€” so the report can show HOW it was re-found (e.g. "re-found
via testid"), or None when no anchor still resolves.
"""
if not failure.page_html:
return None
for identifier in self._extract_identifiers(failure.failed_selector):
alternative = self._find_in_html(identifier, failure.page_html)
for id_type, id_value in self._extract_identifiers(failure.failed_selector):
alternative = self._find_in_html((id_type, id_value), failure.page_html)
if alternative:
return alternative
return alternative, id_type
return None

def _extract_identifiers(self, selector: str) -> list[tuple[str, str]]:
"""Extract identifying parts from a selector."""
identifiers = []

text_match = re.search(r"text=['\"]([^'\"]+)['\"]", selector)
if text_match:
identifiers.append(("text", text_match.group(1)))

# Stability order: testid > id > aria-label > text > class. We adopt the
# first anchor that still resolves on the live page, so the most durable
# identity wins and fragile copy/CSS churn is only the last resort.
testid_match = re.search(r"data-testid=['\"]([^'\"]+)['\"]", selector)
if testid_match:
identifiers.append(("testid", testid_match.group(1)))
Expand All @@ -93,13 +99,17 @@ def _extract_identifiers(self, selector: str) -> list[tuple[str, str]]:
if id_match:
identifiers.append(("id", id_match.group(1)))

for cls in re.findall(r"\.([a-zA-Z][\w-]*)", selector):
identifiers.append(("class", cls))

aria_match = re.search(r"aria-label=['\"]([^'\"]+)['\"]", selector)
if aria_match:
identifiers.append(("aria-label", aria_match.group(1)))

text_match = re.search(r"text=['\"]([^'\"]+)['\"]", selector)
if text_match:
identifiers.append(("text", text_match.group(1)))

for cls in re.findall(r"\.([a-zA-Z][\w-]*)", selector):
identifiers.append(("class", cls))

return identifiers

def _find_in_html(self, identifier: tuple[str, str], html: str) -> str | None:
Expand Down
4 changes: 3 additions & 1 deletion src/ninelives/report/github.py
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,9 @@
logger = logging.getLogger(__name__)

COMMENT_BODY_FILENAME = ".9lives-report.md"
FOOTER = "🐾 checked by [9lives](https://9lives.run) β€” `curl -sL 9lives.run | sh`"
FOOTER = (
"🐾 healed by [9lives](https://9lives.run) β€” self-healing Playwright for the coding-agent era Β· `curl -sL 9lives.run | sh`"
)

_STATUS_EMOJI = {
"passed": "βœ…",
Expand Down
49 changes: 49 additions & 0 deletions tests/test_healing.py
Original file line number Diff line number Diff line change
Expand Up @@ -368,3 +368,52 @@ def test_preflight_project_missing_playwright_raises(tmp_path):
raised = True
assert "@playwright/test" in str(exc)
assert raised, "expected RunnerError when the enclosing project lacks @playwright/test"


def test_assertion_failure_is_not_auto_healed():
# Behavior-vs-drift guard: a failing assertion is a possible real bug, so it
# goes to a human β€” never a Tier 2 rewrite that would force the test green.
failure = TestFailure(
failure_type=FailureType.ASSERTION_FAILED,
error_message="Error: expect(received).toBe(expected)\nExpected: 5\nReceived: 4",
)
assert healing_strategy_selector.select_strategy(failure) == HealingTier.TIER3_HUMAN


def test_assertion_heal_is_opt_in(monkeypatch):
failure = TestFailure(
failure_type=FailureType.ASSERTION_FAILED,
error_message="Expected: 5 Received: 4",
)
monkeypatch.setenv("NINELIVES_HEAL_ASSERTIONS", "1")
assert healing_strategy_selector.select_strategy(failure) == HealingTier.TIER2_AI_SUGGEST


def test_tier1_records_winning_anchor():
# Anchor redundancy: the heal reports WHICH anchor re-identified the element.
failure = TestFailure(
failure_type=FailureType.LOCATOR_NOT_FOUND,
error_message="waiting for locator('#login-btn')",
failed_selector="#login-btn",
test_code="await page.locator('#login-btn').click();",
page_html='<button id="login-btn-v2">Sign in</button>',
)
result = asyncio.run(tier1_healer.heal(failure))
assert result.success
assert result.metadata.get("anchor") == "id"
assert "re-found via id" in result.changes_made[0].lower()


def test_tier1_prefers_stable_anchor_over_class():
# A selector carrying both a testid and a class should re-find via the
# stabler testid, not the fragile class.
failure = TestFailure(
failure_type=FailureType.LOCATOR_NOT_FOUND,
error_message="not found",
failed_selector="[data-testid='submit'].btn-old",
test_code="await page.locator(\"[data-testid='submit'].btn-old\").click();",
page_html='<button data-testid="submit" class="btn-new">Go</button>',
)
result = asyncio.run(tier1_healer.heal(failure))
assert result.success
assert result.metadata.get("anchor") == "testid"

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P3 TESTINGGROUNDED test_tier1_records_winning_anchor does not exercise anchor tracking because the selector is not parsed

The test test_tier1_records_winning_anchor uses #login-btn as the failed_selector, but _extract_identifiers only matches attribute selectors like id='login-btn' β€” it does not parse CSS id selectors (#id). Therefore _find_alternative_selector will return None, the anchor tracking path is never executed, and the test will either fail (because result.success is False) or exercise a different code path that does not record an anchor. The test does not verify the intended anchor-recording behavior.

Example:

failed_selector = '#login-btn'
page_html = '<button id="login-btn-v2">Sign in</button>'

Extracted identifiers: []  (no attribute match)
β†’ _find_alternative_selector returns None
β†’ Tier1 falls through to _try_transformations or _no_heal_result
β†’ anchor metadata never set, test fails

Suggested fix:

use 'id="login-btn"' as the failed_selector, or update _extract_identifiers to parse '#id' selectors
Detailed reasoning

The fix: either change the test's selector to id='login-btn' so the extraction logic recognizes it, or extend _extract_identifiers to handle CSS #id selectors. The test should still assert that the anchor is 'id' and the changes message includes re-found via id.

More Info
  • Threat model: A future change to the anchor tracking code could break the feature without this test catching it, because the test does not actually exercise the anchor path.
  • Specific code citations: _extract_identifiers in src/ninelives/healing/tier1.py only looks for id='...' attribute selectors; the test uses #login-btn.
  • Existing protections: No other test covers anchor tracking for CSS id selectors.
  • Proposed mitigation: Change the test's failed_selector to id='login-btn' so the extraction logic recognizes it and the anchor tracking path is exercised.
  • Alternative mitigations considered: Alternatively, the extraction logic could be extended to handle #id selectors. The test should then be updated to reflect that the anchor is still 'id'.
  • Severity calibration: The feature's anchor tracking is not properly tested for a common selector form, leaving a gap in regression coverage. Score 4 because the test as written is misleading and would fail.
Prompt To Fix With AI
This is a comment left during a code review.
Path: tests/test_healing.py
Line: 419

Comment:
**test_tier1_records_winning_anchor does not exercise anchor tracking because the selector is not parsed**

The test `test_tier1_records_winning_anchor` uses `#login-btn` as the `failed_selector`, but `_extract_identifiers` only matches attribute selectors like `id='login-btn'` β€” it does not parse CSS id selectors (`#id`). Therefore `_find_alternative_selector` will return `None`, the anchor tracking path is never executed, and the test will either fail (because `result.success` is `False`) or exercise a different code path that does not record an anchor. The test does not verify the intended anchor-recording behavior.

The fix: either change the test's selector to `id='login-btn'` so the extraction logic recognizes it, or extend `_extract_identifiers` to handle CSS `#id` selectors. The test should still assert that the anchor is `'id'` and the changes message includes `re-found via id`.

Example:
failed_selector = '#login-btn'
page_html = '<button id="login-btn-v2">Sign in</button>'

Extracted identifiers: []  (no attribute match)
β†’ _find_alternative_selector returns None
β†’ Tier1 falls through to _try_transformations or _no_heal_result
β†’ anchor metadata never set, test fails

Threat model:
A future change to the anchor tracking code could break the feature without this test catching it, because the test does not actually exercise the anchor path.

Specific code citations:
`_extract_identifiers` in `src/ninelives/healing/tier1.py` only looks for `id='...'` attribute selectors; the test uses `#login-btn`.

Existing protections:
No other test covers anchor tracking for CSS id selectors.

Proposed mitigation:
Change the test's `failed_selector` to `id='login-btn'` so the extraction logic recognizes it and the anchor tracking path is exercised.

Alternative mitigations considered:
Alternatively, the extraction logic could be extended to handle `#id` selectors. The test should then be updated to reflect that the anchor is still `'id'`.

Severity calibration:
The feature's anchor tracking is not properly tested for a common selector form, leaving a gap in regression coverage. Score 4 because the test as written is misleading and would fail.

Suggested fix shape:
use 'id="login-btn"' as the failed_selector, or update _extract_identifiers to parse '#id' selectors

How can I resolve this? If you propose a fix, please make it concise.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Dismissing as a false positive. _extract_identifiers does parse CSS #id selectors β€” id_match = re.search(r"#([a-zA-Z][\w-]*)", selector) (tier1.py:98) matches #login-btn. The path is exercised: this test passes asserting result.metadata['anchor'] == 'id' and 're-found via id', and the pre-existing test_tier1_heals_moved_id_from_page_snapshot also uses #login-btn and passes. The #(\w+) you may have seen is a separate TRANSFORMATIONS entry, not the extractor.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Retracting β€” the finding was a false positive. _extract_identifiers at tier1.py:98 uses re.search(r"#([a-zA-Z][\w-]*)", selector), which correctly parses CSS #id selectors like #login-btn. The #(\w+) pattern is a separate TRANSFORMATIONS entry (tier1.py:24), not the extractor. test_tier1_records_winning_anchor exercises the anchor-tracking path as intended: _extract_identifiers yields ("id", "login-btn"), _find_in_html resolves login-btn-v2, and the test's assertions on metadata["anchor"] and changes_made are valid. The pre-existing test_tier1_heals_moved_id_from_page_snapshot also uses #login-btn and passes, confirming the path works.

Learned something new

"For detecting CSS id selectors, use the extractor _extract_identifiers which handles #id selectors via r"#([a-zA-Z][\w-]*)" pattern, not the TRANSFORMATIONS entry which uses #(\w+)"
Undo: reply @sigilix forget to this comment.

Loading