fix(PER-8519): initialize percy_screenshot_url before try to stop exception masking

[pranavz28]

What this fixes

PER-8519 / F-006 — UnboundLocalError masks the real exception in AppAutomate.screenshot

percy/providers/app_automate.py assigned percy_screenshot_url inside the try, on the line
after super().screenshot(), but referenced it in the except block:

try:
    response = super().screenshot(name, **kwargs)
    percy_screenshot_url = response.get('link', '')   # never reached if the line above raises
    ...
except Exception as e:
    self.execute_percy_screenshot_end(name, percy_screenshot_url, 'failure', sync, str(e))
    raise e

So whenever super().screenshot() itself raised, the handler touched an unbound local and blew up
inside the handler. Reproduced on master before the change:

RAISED TYPE: UnboundLocalError | cannot access local variable 'percy_screenshot_url' ...
MASKED ORIGINAL: ValueError | original boom
screenshot_end calls: []

Two consequences, not one:

1. The original exception is replaced. Callers see UnboundLocalError instead of the real
   capture failure. percy_screenshot() forwards str(e) to /percy/events via
   post_failed_event, so our own telemetry recorded "cannot access local variable
   'percy_screenshot_url'" rather than the actual fault — the error text is useless for triage.
2. The 'failure' notification is never sent. execute_percy_screenshot_end was never called
   (screenshot_end calls: []), so the BrowserStack session was left dangling in the begin state
   with no terminal status. This is the more damaging half, and the reason the fix is not merely
   "silence the name error".

The fix initializes percy_screenshot_url = '' before the try. After the change:

RAISED TYPE: ValueError | original boom
screenshot_end calls: [call('name', '', 'failure', None, 'original boom')]

The original exception propagates, and the session is properly closed out as failure with the
real message. An empty string is the right default: it is the same value the success path uses when
the response carries no link (response.get('link', '')), so the executor payload shape is
unchanged and the percyScreenshotUrl field stays a string.

Why this is safe. It is a strictly-widening change on a path that previously always crashed.
The success path is untouched (the assignment inside the try still overwrites the default). The
only behaviour that changes is a path that could only ever produce UnboundLocalError plus a
dangling session. It improves error visibility rather than changing failure policy — which serves
part of PER-8522's intent without any breaking change.

Regression test added: test_screenshot_propagates_capture_error_and_reports_failure asserts the
original ValueError propagates (not UnboundLocalError) and that the failure notification fires
exactly once with ('name', '', 'failure', None, 'capture failed'). The existing
test_screenshot_reraises_after_failure_notification only covered the case where the notification
fails after a successful capture, which is why this bug survived the 100% coverage gate — the line
was covered, the unbound path was not.

---

What was deliberately NOT done

The ticket's nominated chain-breaker — flipping PercyOptions.ignore_errors to False — is not in this PR

PER-8535 nominates flipping the ignore_errors default from True to False as the chain-breaker.
That should not be done as a unilateral change in this SDK, for four reasons.

1. Fail-open is a cross-SDK convention, implemented five independent times.

SDK	Location	Default
percy-appium-python	percy/lib/percy_options.py:15,22	True
percy-appium-js	percy/driver/driverWrapper.js:83	True
percy-appium-ruby	percy/lib/percy_options.rb:24,32	True
percy-appium-java	PercyOptions.java:37	True
percy-appium-dotnet	Percy/AppPercy.cs:11	True

Five independent implementations agreeing is a deliberate product decision, not five copies of one
oversight. Flipping Python alone makes it the sole outlier and breaks the same-capability
same-behaviour contract that polyglot suites depend on: the identical percy:options capability
would mean "swallow" in four SDKs and "fail the test" in the fifth.

2. The option is completely undocumented. ignoreErrors does not appear in any of the six SDK
READMEs. Customers cannot have knowingly opted into a behaviour they were never told exists, so a
flip would change behaviour under users who have no documented way to know the knob is there — and
no documentation to find when their suite starts failing.

3. The blast radius is every screenshot call. percy_screenshot() is the sole public entry
point of this SDK. Flipping the default converts every currently-silent Percy failure into a
test-suite failure. A Percy outage, a CLI version mismatch, or a transient healthcheck blip would
start failing customers' unrelated functional tests — visual-testing infrastructure taking down
functional CI is a much worse failure mode than a missed snapshot, and it is exactly what fail-open
was chosen to prevent.

4. There is a pre-existing opt-out bug that must be fixed first. percy/lib/percy_options.py:13
short-circuits before the legacy-capability fallback:

options = (options[0] or options[1]) if any(options) else {}
if options: return options          # <-- returns before the percy.ignoreErrors fallback below
if options is not None and self.IGNORE_ERRORS not in options:
    options[self.IGNORE_ERRORS] = self._capabilities.get(f'percy.{self.IGNORE_ERRORS}', True)

When percy:options is present and non-empty, the legacy percy.ignoreErrors capability is
silently ignored. Verified against this branch:

legacy cap alone             -> False   (opt-out honoured)
legacy cap + percy:options   -> True    (opt-out SILENTLY DROPPED)
modern percy:options opt-out -> False   (opt-out honoured)

Ruby has no such early return, so this is a Python-only divergence. Flipping the default while this
bug exists means the customers most likely to want the old behaviour — those on the legacy
capability — would find their opt-out does not take effect. They would have no working escape
hatch from the new failure mode.

Recommendation

Take the default flip as a cross-SDK product decision with a deprecation path, not an SDK-local
bugfix:

1. Document ignoreErrors in all six SDK READMEs first, so the opt-out is discoverable.
2. Fix the percy_options.py:13 opt-out bug so percy.ignoreErrors is honoured alongside
   percy:options, and confirm the other four SDKs honour their legacy capability too.
3. Warn-on-swallow in a minor release across all five SDKs — log loudly whenever an error is
   being swallowed, telling users the default will change and how to pin the current behaviour.
4. Flip in a major release, in all five SDKs together, so the same-capability same-behaviour
   contract survives.

PER-8535 therefore cannot be closed by this PR. This PR lands the safe, independently-correct
subset only. The chain-breaker it nominates remains open and needs the product decision above.

PER-8527 / F-014 — the TOCTOU race on class-level globals: investigated, not changed

I investigated the shared mutable state (percy/environment.py:7-9, written in cli_wrapper.py:26-28
inside an @lru_cached static method, read at screenshot.py:11) and could not reproduce the
described failure mode, because it is not reachable as stated. A 40-thread concurrent probe:

healthcheck body executions (lru_cache first-call race): 2
observers that saw enabled=True but session_type/build_id None: 0
distinct observations: {(True, 'automate', 'b1')}

The lru_cache first-call race is real — the healthcheck body ran twice for 40 threads, confirming
lru_cache is not atomic. But session_type=None cannot be observed after is_percy_enabled()
returns True, for three structural reasons:

• Writes precede the return, and lru_cache only publishes after the return. Every reader that
  receives True is reading values that were already written.
• Duplicate executions write identical values. PERCY_CLI_API is a module-level constant, so
  every execution in a process talks to the same CLI and writes the same build id / url / type. A
  "clobber" is a no-op. (Separate processes — e.g. pytest-xdist — have separate memory entirely.)
• No code path writes None over a good value. if not data['success']: raise precedes the
  assignments, and the except returns False without touching the globals, so a failing or
  transient healthcheck can never null out previously-good state.

The one genuine residue is the duplicated healthcheck HTTP request under a first-call race — benign
redundancy, not state corruption or wrong-provider selection.

I chose not to apply the ticket's preferred fix (returning the
(enabled, session_type, build_id, build_url) tuple) because the cost/benefit is inverted here:

• It changes is_percy_enabled's return type from bool to a tuple, which breaks the
  if not CLIWrapper.is_percy_enabled(): truthiness check at the call site and the three existing
  test patches that stub it as MagicMock(return_value=True).
• It would only relocate one of the three global reads (screenshot.py:11). The other two
  (app_automate.py:81-82 and :132, which need percy_build_id / percy_build_url) would still
  read the globals unless the values are threaded through
  AppPercy.__init__ → ProviderResolver.resolve → AppAutomate.__init__ → provider.screenshot()
  — four signature changes across the provider class surface.

That is a broad change to the SDK's constructor surface, plus a partial fix that leaves the shared
mutable state in place, in exchange for a failure mode I can show is unreachable. A clean small PR
is worth more here. If we want the lru_cache duplicate-healthcheck tidied up, that is a
self-contained follow-up in cli_wrapper.py alone and should be its own ticket.

---

Testing

• make coverage (venv on Python 3.12): 165 tests, OK, 100% coverage / fail_under = 100
  satisfied — percy/providers/app_automate.py at 100%, TOTAL 778 stmts / 0 missed. The new
  branch is covered, so the repo's coverage gate stays green.
• pylint percy/providers/app_automate.py → 10.00/10.
• Note: running the full make lint locally crashes with an astroid.AstroidError on any
  appium-importing file. This is pre-existing and environmental — it reproduces identically on
  an unmodified master tree (verified via git stash), and is a local Python 3.12 artifact
  (.python-version pins 3.10; CI lints on 3.11). Not introduced by this change.

🤖 Generated with Claude Code