fix(ariba): retry a swallowed trigger click — both #183 documents now capture

CLAUDE.md

@@ -179,6 +179,7 @@ do not reopen it expecting a better parser to help (the #83 pattern).** First li
 - **Two guards of our own, each correct in intent, each scoped too broadly — they caused every failure in this widget, not Ariba.** Both were found only by measuring the DOM before and after a single action, and both had survived several live runs of hardening the *traversal*, which was never the broken part:
   - **The identity key was invalidated by the act of downloading.** `anchor_key` is `row_identity#ordinal#name` where `row_identity` is the anchor's `<tr>` text truncated to 120 chars. Downloading one file changed **36 of 39 keys at once** on an otherwise intact tree (same documents, same anchors, same names): opening an attachment's menu re-parents the menu containers, so `closest('tr')` stops resolving to the fused `Reference Documents Attachment 1 ... Tree` blob and starts resolving to the real `3.1` row, while the ordinal shifts by one as that row's own link joins the count. So **`anchor_key` is durable ACROSS runs and not WITHIN one** — every run traverses before it downloads, and the tree is pristine then. Use it for the fingerprint, resume and naming; never to re-find an element mid-run. That job belongs to the anchor's own DOM **`handle`** (unique across document anchors; the one id-less anchor is an external link `is_document_name` rejects), with `pick_unclaimed` behind it — and **neither may reach `make_fingerprint`**, since AribaWeb re-mints ids per session and a fingerprint holding one would discard the partials on every single run.
   - **The menu-dismissal guard hides the documents it protects.** `_await_menu_clear`/`_dismiss_menu` press Escape between downloads to close a stale menu (a real wrong-bytes hazard — the guard stays). But right after the traversal expands them all 36 reference attachments are visible, and **one Escape drops that to 0 visible with all 36 still in the DOM** — the entire cause of 36 `scroll_into_view` timeouts in one run. `_ensure_clickable` re-opens the sections per download. **It then VERIFIES visibility rather than trusting the re-open**, because the mechanism is genuinely not understood: re-running the expansion restores visibility (0 → 36) while reporting it opened **zero** sections. Resting on an unexplained side effect is how this issue produced six live runs; checking the post-condition is what makes it safe, and it turns a mystery timeout into a sentence naming what happened.
+- **A trigger click straight after an Escape is SWALLOWED, and that was both of #183's "unreachable" documents (#183).** The pattern names itself once both events are lined up: `Part 1 ok · Part 2 FAILED · Part 3 ok` on **Doc5713434353 and Doc5540340341 alike** — works, fails, works, i.e. **parity, not a bad row**. A live probe proved both halves: the PML triggers own **mutually exclusive** popup containers (opening one measurably closes another, `_nhkn2b` 1 → 0 visible), and a **clean** click on the very `Part 2` anchor that fails in a real run opens its menu in ~3s. So the row is fine and the widget's state is not — `_dismiss_menu`'s Escape hides the previous menu while AribaWeb still believes it is open, so the next trigger click is consumed as a close. `_open_attachment_menu` clicks again, **bounded**, and the retry is safe for exactly the reason `_await_menu_clear` is the wrong-bytes guard: that hazard needs a VISIBLE stale item, and `_await_menu_item` only raises when **zero** are visible — the identical precondition. The zero is **re-verified immediately before the retry** rather than inferred from the raise, because the menu can land in the gap and a second click would then close what just opened. **The other half is the same shape and the same fix, one widget over:** a hidden menu-ITEM needed **two** re-expansion passes, not one (`_EXPAND_ATTEMPTS`) — measured live, pass 1 left `Appendices ….zip` hidden and it was omitted, pass 2 revealed it and the event went **3/4 → 4/4 with no gap record at all**. **Do not write down why that second pass works.** It is tempting to reuse the parity proof above, and they may well be one bug — but parity was measured on the PML trigger and *nothing has measured the References trigger*; #174 already recorded this same behaviour as unexplained (re-running the expansion restores visibility while reporting it opened **zero** sections). The post-condition is checked after every pass for that reason, and a still-hidden control raises rather than falling into a `scroll_into_view` timeout that says nothing about why.
 - **The picker's `Total Number` and the tree's file count are NOT commensurable — the comparison was removed (#185).** On the validated event the picker says **54**, the traversal finds **39**, and the bundle's true content is **178 leaf documents** after nested expansion. Three quantities, no derivable relationship between any two — so `capture_files` no longer reads the picker's count at all: no `expected_count()`/`_read_expected_count`/`_restore_event_view` round trip (Download Content -> Download Attachments -> picker -> Done, the most expensive non-download step per event), no `SHORT by N` log line, no `expected_files`/`actual_files` in `Doc<n>.omitted.json`. Before this, every successful capture of that event wrote `omitted: []` beside `expected_files: 54, actual_files: 37` — a gap record permanently describing a shortfall that did not exist, and `clear_omitted_when_complete`'s own guard meant it could never self-clear either. `write_omitted`/`make_fingerprint` now carry only what is real: `omitted` (failed downloads) and `collided` (links the traversal found indistinguishable and collapsed). The capture floor from #182 (`_MIN_CAPTURE_RATIO`, a near-total download failure keeps partials pending) is unaffected — it always compared against the traversal's own listing, never the picker. `AribaPicker`/`_select_all_attachments` became unused by the live capture path here (their only caller was the removed round trip) but were kept on disk, tested, at the time — deleting working, hard-won code as a side effect was not this issue's call to make. `ariba_batch.py`, the module `AribaPicker` was originally built for, was itself retired shortly after (#186); `AribaPicker`/`_select_all_attachments` were then retired directly in #195, once #186 confirmed nothing else could plausibly need them either.
 - **Two halves split like fetch/normalize.** The **pure** half (`document_number_from_zip_name`, `index_zip`, `store_bundle`, `ingest_downloads`, `reindex_bundles`) indexes a bundle and writes the manifest — no browser, tested against fixture zips. `index_zip` reads `file_size` and `crc32` **from the zip central directory**, not decompressing the top-level bundle (a nested zip *is* read to reach its own directory — #123). The **browser** half (`login`, `capture_event`, `capture_attachments`) needs a headed Chromium behind the `council` extra, logged into a supplier account from `scrapers/.env` (`ARIBA_USERNAME`/`ARIBA_PASSWORD`, gitignored — the repo is public). The flow is Discovery preview → Respond → traverse the event's content tree, **downloading each document individually** — Download Content → Download Attachments is driven once per event only to read the picker's authoritative file count as an independent check, not to produce a bundle.
 - **`ariba_attachment` is the INDEX; the bytes are on disk only.** One row per LEAF file, keyed `(document_number, path)` — the *full nested path* (`Appendix C2.zip/drawings/site.pdf`), because leaf names collide across nested zips (#123). Bundle bytes under `<DATA_DIR>/ariba/attachments/Doc<n>.zip`, never committed (multi-GB). `document_number` joins `solicitation.document_number` (the Ariba event *is* the 10-digit doc). It is a **derived index of the on-disk zips**, so it is rebuilt from the bytes (`tb enrich-ariba-attachments --reindex`, offline) rather than diff-upserted — the sanctioned exception to "rows are never deleted", like the supplier dimension. `store_bundle` deletes a document's rows then re-inserts the recursive set.

scrapers/tests/test_ariba_menu_retry.py

@@ -0,0 +1,234 @@
+"""Opening an attachment's menu, and revealing a hidden one (#183).
+
+Two documents were being omitted from an archive that cannot be re-fetched, on every run:
+
+    Doc5713434353   Part 1 ok   Part 2 FAILED   Part 3 ok    + schedule-b2.pdf FAILED
+    Doc5540340341   Part 1 ok   Part 2 FAILED   Part 3 ok    + (reproduced live)
+
+Works, fails, works — parity, not a bad row. A live probe confirmed both halves: the PML
+triggers own mutually exclusive popup containers (opening one closes another), and a CLEAN
+click on the very `Part 2` anchor that fails in a real run opens its menu in ~3s. So the row is
+fine and the widget's state is not: `_dismiss_menu`'s Escape hides the previous menu while
+AribaWeb still believes it is open, so the next trigger click is consumed as a close.
+
+These tests pin the two behaviours that follow, with fakes rather than a browser:
+
+  * a swallowed trigger click is retried, and the retry is refused when a menu is already
+    visible (which would CLOSE it — the #174 trap, one widget over)
+  * `_ensure_clickable` runs more than one re-expansion pass before giving up
+
+The second one's MECHANISM is deliberately not asserted anywhere here — see
+`_ensure_clickable`'s docstring. What is pinned is the observable: one pass was not enough.
+"""
+import pytest
+
+from toronto_bids.sources import ariba_attachments as aa
+
+
+class _FakeLocator:
+    def __init__(self, page):
+        self._page = page
+
+    def click(self, timeout=None):
+        self._page.clicks += 1
+        self._page.on_click()
+
+    def is_visible(self):
+        return self._page.link_visible
+
+    def scroll_into_view_if_needed(self, timeout=None):
+        return None
+
+
+class _FakePage:
+    """Stands in for the bits `_open_attachment_menu` touches.
+
+    `visible_after` is the click number at which the menu finally opens — 1 models a healthy
+    trigger, 2 the swallowed-first-click that #183 measured, and None one that never opens.
+    """
+
+    def __init__(self, visible_after=1, preopen=0):
+        self.clicks = 0
+        self.visible_after = visible_after
+        self._visible = preopen
+        self.link_visible = True
+
+    def on_click(self):
+        if self.visible_after is not None and self.clicks >= self.visible_after:
+            self._visible = 1
+
+    def wait_for_timeout(self, _ms):
+        return None
+
+    # what `_visible_menu_items` drives
+    def get_by_text(self, _text, exact=False):
+        page = self
+
+        class _Items:
+            def count(self):
+                return 3                       # three PMLs, three menus — as measured live
+
+            def nth(self, i):
+                class _N:
+                    def is_visible(_self):
+                        return i == 0 and page._visible > 0
+                return _N()
+        return _Items()
+
+
+def _source(page):
+    src = aa.AribaFileSource.__new__(aa.AribaFileSource)
+    src.page = page
+    src.log = lambda _m: None
+    src._toggled = set()
+    return src
+
+
+FILE = {"name": "Part 2 - Construction Agreement_A1.pdf"}
+
+
+# --- the swallowed trigger click ----------------------------------------------------------
+
+def test_a_healthy_trigger_opens_on_the_first_click():
+    """The retry must cost nothing on every document that already worked."""
+    page = _FakePage(visible_after=1)
+    src = _source(page)
+
+    src._open_attachment_menu(FILE, _FakeLocator(page))
+
+    assert page.clicks == 1
+
+
+def test_a_swallowed_first_click_is_retried_and_succeeds():
+    """The #183 measurement: click 1 opens nothing, click 2 opens the menu."""
+    page = _FakePage(visible_after=2)
+    src = _source(page)
+
+    item = src._open_attachment_menu(FILE, _FakeLocator(page))
+
+    assert page.clicks == 2
+    assert item is not None
+
+
+def test_a_trigger_that_never_opens_still_raises_naming_the_document():
+    """A genuinely dead trigger must fail loudly, not hang or pass silently — the document is
+    then recorded in the durable gap record rather than vanishing."""
+    page = _FakePage(visible_after=None)
+    src = _source(page)
+
+    with pytest.raises(RuntimeError, match="the menu did not open"):
+        src._open_attachment_menu(FILE, _FakeLocator(page))
+    assert page.clicks == 2                     # bounded — not an unbounded click loop
+
+
+def test_the_retry_is_refused_when_a_menu_is_already_visible():
+    """The #174 trap, one widget over: a blind second click CLOSES what the first opened.
+
+    Here the menu lands in the gap between `_await_menu_item` giving up and the retry firing.
+    Clicking again would close it, so the retry must stand down and take what is there.
+    """
+    page = _FakePage(visible_after=None)
+    src = _source(page)
+    link = _FakeLocator(page)
+
+    calls = []
+
+    def _await(file, timeout_ms=15000):
+        calls.append(1)
+        if len(calls) == 1:
+            # As live: gives up with nothing visible. The menu then lands on its own, in the
+            # gap before the retry fires.
+            page._visible = 1
+            raise RuntimeError("the menu did not open within 15s (no VISIBLE x among 3)")
+        return "the-late-item"
+
+    src._await_menu_item = _await
+    item = src._open_attachment_menu(FILE, link)
+
+    assert page.clicks == 1, "clicked again while a menu was visible — that would close it"
+    assert item == "the-late-item"              # took what was there rather than re-clicking
+
+
+def test_visible_menu_items_reports_minus_one_when_the_dom_is_unreadable():
+    """Callers use ZERO as permission to click. An unreadable DOM is an unanswered question,
+    never permission."""
+    class _Broken:
+        def get_by_text(self, *_a, **_k):
+            raise RuntimeError("Execution context was destroyed")
+
+    src = _source(_Broken())
+    assert src._visible_menu_items() == -1
+
+
+# --- revealing a hidden control ------------------------------------------------------------
+
+class _ExpandPage:
+    """`link.is_visible()` flips to True only once `reveal_after` expansion passes have run."""
+
+    def __init__(self, reveal_after):
+        self.passes = 0
+        self.reveal_after = reveal_after
+
+    def visible(self):
+        return self.reveal_after is not None and self.passes >= self.reveal_after
+
+
+def _expanding_source(state):
+    src = aa.AribaFileSource.__new__(aa.AribaFileSource)
+    src.page = None
+    src.log = lambda _m: None
+    src._toggled = {"stale"}
+    src._expand_references = lambda: state.__setattr__("passes", state.passes + 1) or 0
+    return src
+
+
+class _RevealLocator:
+    def __init__(self, state):
+        self._state = state
+
+    def is_visible(self):
+        return self._state.visible()
+
+    def scroll_into_view_if_needed(self, timeout=None):
+        return None
+
+
+def test_a_control_already_visible_needs_no_expansion_pass():
+    state = _ExpandPage(reveal_after=0)
+    src = _expanding_source(state)
+
+    src._ensure_clickable({"name": "a.pdf"}, _RevealLocator(state))
+
+    assert state.passes == 0
+
+
+def test_a_second_expansion_pass_runs_when_one_was_not_enough():
+    """The #183 measurement: pass 1 left `Appendices ....zip` hidden and it was omitted; pass 2
+    revealed it and the event went 3/4 -> 4/4. WHY is not asserted — see the docstring."""
+    state = _ExpandPage(reveal_after=2)
+    src = _expanding_source(state)
+
+    src._ensure_clickable({"name": "Appendices.zip"}, _RevealLocator(state))
+
+    assert state.passes == 2
+
+
+def test_a_control_that_never_appears_raises_after_a_bounded_number_of_passes():
+    state = _ExpandPage(reveal_after=None)
+    src = _expanding_source(state)
+
+    with pytest.raises(RuntimeError, match="not visible"):
+        src._ensure_clickable({"name": "gone.pdf"}, _RevealLocator(state))
+    assert state.passes == aa._EXPAND_ATTEMPTS      # bounded, not an unbounded sweep loop
+
+
+def test_the_expansion_record_is_cleared_so_the_pass_can_act_at_all():
+    """`_expand_references` skips any section already in `_toggled`, so a retry that did not
+    clear it would re-run and do nothing at all."""
+    state = _ExpandPage(reveal_after=1)
+    src = _expanding_source(state)
+    assert src._toggled                                  # starts non-empty
+
+    src._ensure_clickable({"name": "a.pdf"}, _RevealLocator(state))
+
+    assert not src._toggled