fix(vendor): end the manual-intervention wait when nobody can answer it (#522)

[JarryShaw]

Closes #522.

Root cause

Vendor._request()'s last resort is to ask an operator to fetch the page by
hand. The wait for them was a while True whose only pause was an input()
wrapped in contextlib.suppress(Exception):

while True:
    with contextlib.suppress(Exception):
        input('Press ENTER to continue...')  # nosec
    if os.path.isfile(temp_file):
        break
    print('File not found; please save the page source at')
    print(f'    {temp_file}')

With nobody at a keyboard, the suppression is what does the damage rather than
the loop. input() raises instead of blocking, the suppression discards the
exception, os.path.isfile(temp_file) is false because nobody could save the
page, and the iteration repeats immediately -- three lines of output per pass,
which the issue measured at 26.7 million lines / 2.6 GB before the process
was killed. It was never a hang; it was a printer.

Two things make the defect wider than "stdin is closed":

• input() reports an unusable stdin three different ways, and the
  suppression discarded all three alike. Measured on CPython 3.14.7 in this
  venv: EOFError at end of file (redirected from /dev/null, a closed pipe),
  ValueError: I/O operation on closed file when stdin has been closed, and
  RuntimeError: lost sys.stdin when sys.stdin is None. A fix that caught
  only EOFError would leave two thirds of it live.
• The loudest real-world shape raises nothing at all. Under
  yes | make vendor, input() returns 'y' forever, the file still never
  appears, and the loop still spins at full speed. No amount of exception
  handling inside the loop catches that one.

The fix

Two halves, because neither covers the other.

1. A precondition. New stdin_is_interactive() decides whether there is a
   terminal at all, and a fetch failure without one re-raises the
   requests.RequestException -- exactly as PCAPKIT_CI_MODE already did --
   before a browser is opened, a temporary directory is created under
   os.curdir, or a word is printed. This is the half that covers the piped
   case above, and it also stops a non-interactive run littering the working
   directory and the log with instructions nobody will read. The predicate
   answers False rather than raising for an absent, closed, or isatty-less
   stdin, so a crawler reporting a fetch failure cannot have it replaced by an
   unrelated traceback.
2. A loop that cannot spin. The suppress(Exception) becomes an except
   that re-raises the fetch error with the prompt failure as its __cause__, so
   the caller gets the same failure PCAPKIT_CI_MODE would have given it and the
   traceback still says why the manual path gave up. The precondition does not
   make this redundant: a terminal can go away during the wait -- an SSH session
   dropping, a parent closing the descriptor -- and that lands in exactly the loop
   the precondition already let the process into.

KeyboardInterrupt is a BaseException, so it escaped the old
suppress(Exception) and escapes the new except Exception unchanged. Ctrl-C is
how an operator declines, and turning it into "connection failed" would be a lie
about what happened; that is pinned by a test rather than left to reading.

_request's docstring gains the Raises: clause it never had, and says which
runs take the manual path.

Behaviour in each of the three cases

Case	Before	After
tty present, PCAPKIT_CI_MODE unset	Browser opened, instructions printed, waits for ENTER, re-asks until the page is saved.	Unchanged. Same browser, same instructions, same re-ask loop. If the terminal goes away mid-wait, the wait now ends with the fetch error instead of spinning.
no tty, PCAPKIT_CI_MODE unset	Browser opened at nothing, temporary directory created, instructions printed, then an unbounded print loop -- whether input() raised (/dev/null, closed pipe) or returned forever (yes |).	Warns Connection failed; exit as stdin is not interactive... and re-raises the requests.RequestException. No browser, no temporary directory, nothing printed.
PCAPKIT_CI_MODE=1	Warns exit on CI mode... and re-raises before any of the above.	Unchanged, and still the first gate -- checked before stdin is consulted, so a CI job that happens to get an allocated tty keeps failing fast.

.github/workflows/cron-vendor.yml exports PCAPKIT_CI_MODE=1 (lines 63-64), so
this repo's weekly scheduled crawl was never exposed. The exposure was everything
else that drives the crawlers non-interactively -- a local make vendor under a
pipe, a container build, a cron entry that is not that workflow, an agent session
-- none of which has a reason to know the variable exists.

Tests, and the fails-without evidence

tests/vendor/test_request_prompt_unit.py, 16 cases (plus 3 sub-cases), unit
tier, no network: requests.get is replaced for the whole of every case, and one
case asserts the quiet direction -- that a crawler which can fetch never
consults stdin at all.

No case can wedge the suite, and none relies on a timeout to notice the
runaway. The fake input answers a bounded number of prompts and then raises
_Runaway, which derives from BaseException precisely so that neither the old
suppress(Exception) nor the new except Exception can catch it. Against the
unfixed code a case therefore fails in milliseconds with a message naming the
defect (input() was answered 16 times and the manual-intervention wait still had not ended: the loop is unbounded (#522)) instead of printing until something kills
it. tests._support.time_limit is layered on as a backstop, and stdout is
captured throughout so the bounded flood is counted rather than spilled.

Each half of the fix was reverted on its own, so the evidence is behavioural
rather than "the new helper is missing". Run as
PYTHONSAFEPATH=1 .venv/bin/python -m pytest tests/vendor/test_request_prompt_unit.py -v,
with pytest's exit status read from a file rather than a pipeline position:

Tree	Result	pytest exit code
Both halves in place	16 passed, 3 subtests passed	0
Only the loop fix reverted (suppress(Exception) restored, gate kept)	5 failed, 14 passed -- test_a_stdin_at_eof_ends_the_wait_instead_of_spinning, test_the_prompt_failure_is_chained_as_the_cause, and all 3 sub-cases of test_every_way_input_can_fail_ends_the_wait	1
Only the interactivity gate reverted (loop fix kept)	1 failed, 15 passed -- test_a_non_interactive_run_never_reaches_the_prompt	1
Both reverted, i.e. pcapkit/vendor/default.py exactly as it stands on main	17 failed, 1 passed	1

The full unit tier -- the selection .github/workflows/unit-tests.yml runs, i.e.
pytest tests --ignore=tests/integration --ignore-glob='*_runtime.py' --ignore-glob='*_regression.py'
-- is green on this branch rebased onto 691f12ab5: 1052 passed, 8 skipped,
2491 subtests passed, exit code 0, no failures.

Coverage of pcapkit/vendor/default.py under tests/vendor/, measured with
coverage run -m pytest (not pytest-cov): 50% -> 67%, misses 86 -> 59,
partial branches 9 -> 8. The manual-intervention branch was previously uncovered
in its entirety; every line of it is now covered, including both wordings of the
instructions. The remaining misses in that method are the pre-existing proxy-retry
path and the two MAX_RETRY-exhausted raises, which this change does not touch.

Which tree was measured, printed rather than assumed:

sys.executable   = /local/home/jarryx/GitHub/PyPCAPKit/.venv/bin/python
pcapkit.__file__ = <worktree>/pcapkit/__init__.py
default.__file__ = <worktree>/pcapkit/vendor/default.py

Both test classes also skip, rather than pass vacuously, if
pcapkit.vendor.default resolves outside the checkout under test. requests,
bs4 and html5lib ship in the vendor extra rather than test, so the module
is guarded with importlib.util.find_spec plus @unittest.skipUnless, mirroring
tests/protocols/test_dispatch_registry_unit.py.

Other checks on the changed module: mypy clean, bandit clean, isort --check
clean, pylint (the repo's own invocation from the Makefile) reports only the
three findings that were already there -- the long LINE template line and the two
pre-existing missing-timeout warnings on requests.get.

Deliberately NOT fixed

• No bound on the loop's iteration count. Once there genuinely is an operator,
  a bound would be the wrong thing: pressing ENTER before saving the file is a
  normal mistake, the retry notice is human-paced, and a cap would turn a mistimed
  keystroke into a failed crawl. Every non-interactive route into the loop is now
  closed, which is what the unboundedness was actually costing.
• CI_MODE's default was left as false. The issue floats deriving it from
  not sys.stdin.isatty(). That would fold two distinct facts into one flag and
  make the exit on CI mode... warning misleading when it fired for want of a
  terminal, so the interactivity check is a separate decision with its own message.
  PCAPKIT_CI_MODE keeps meaning exactly what it meant.
• An accepted trade-off: no way to force the prompt without a tty. In an
  environment where sys.stdin.isatty() is False and input() nevertheless
  works -- a Jupyter kernel is the real example, since ipykernel patches
  builtins.input -- the manual path is now refused where it would previously have
  worked. Judged acceptable: a vendor crawler writes generated constant modules
  into the package source tree and is a maintainer tool driven by make vendor, so
  a notebook is not a plausible host; and the failure is a clear
  RequestException with a warning naming the reason, against a 2.6 GB print loop
  on the other side of the trade.
• requests.get's missing timeout (pylint W3101 on both call sites,
  carrying # nosec: B113). A genuinely separate hang risk from this one, in code
  this change does not touch.
• The proxy-branch retry path and the two MAX_RETRY-exhausted raises remain
  uncovered. They belong to the retry logic rather than the prompt, and
  tests/vendor/test_user_agent_unit.py already owns the proxy branch.
• get_user_agent and stdin_is_interactive have no autofunction entry in
  docs/source/pcapkit/vendor/default.rst, which lists only get_proxies.
  Pre-existing for get_user_agent (added in Vendor crawlers: three take a Wikipedia 403 on the default User-Agent, three point at a dead IETF URL #518) and matched here for
  consistency rather than fixed, since the docs tree is outside this change.

[JarryShaw]

✅ GOOD TO MERGE (head 7a886e63a, files byte-identical to the pre-rebase head 6bccb2a5a per direct diff) -- independently reproduced all three input() failure modes (EOFError/ValueError/RuntimeError) on the exact Python version in this venv, confirmed the yes | make vendor case returns 'y' forever with isatty() == False (which is what makes the stdin_is_interactive() precondition necessary rather than an exception-catch-only fix), and confirmed the KeyboardInterrupt-boundary test genuinely catches the natural except BaseException over-correction. One judgement call for the owner: the Jupyter trade-off (refusing the manual path where isatty() is false but a patched input() would still work) is a real, disclosed behavior change and reads as an acceptable one for a maintainer-only crawler, but it is the owner's call to make, not mine.

[JarryShaw]

Detailed review (independent verification, falsify-not-bless)

Head sha reviewed: 7a886e63a2c4cb5b3f988fcbad391229010408d5 (rebased from 6bccb2a5ae7c81ceaccb0bdac45a660264c666b6 onto main at 691f12ab5 after #538 merged). Confirmed by direct diff (git diff <old-head> <new-head> -- pcapkit/vendor/default.py tests/vendor/test_request_prompt_unit.py, empty output) that both files are byte-for-byte identical between the two heads -- only the PR body's prose changed (the pre-existing-failure disclaimer removed, and the fails-without table's bottom row reworded from a confusing HEAD: reference to "exactly as it stands on main", both confirmed present and correct in the current body). So everything verified below on the pre-rebase head applies unchanged to this one, plus a fresh confirmation run at the end.

The three-way input() failure claim -- reproduced on this exact interpreter

Ran directly against /local/home/jarryx/GitHub/PyPCAPKit/.venv/bin/python (3.14.7, the same interpreter this venv uses):

• input() under stdin redirected from /dev/null -> EOFError: EOF when reading a line
• input() after sys.stdin.close() -> ValueError: I/O operation on closed file.
• input() with sys.stdin = None -> RuntimeError: lost sys.stdin

All three match the PR's claim exactly, on the same Python version. A fix that only caught EOFError would indeed leave two of these three live.

The yes | make vendor case -- confirmed to raise nothing at all

yes | timeout 2 python -c "for i in range(5): print(input('prompt: '))" returned 'y' five times with no exception whatsoever. Separately confirmed sys.stdin.isatty() is False under the same pipe. This is the direct, independent confirmation of the PR's most important architectural claim: no amount of exception handling inside the loop could ever detect this case, because nothing raises -- only a precondition checked before entering the loop (stdin_is_interactive(), which reads isatty()) can close it. This justifies the two-halves design rather than a simpler exception-only fix.

KeyboardInterrupt boundary -- the over-correction test genuinely catches the natural mistake

Changed except Exception as exc: to except BaseException as exc: in the loop (the "natural over-correction" the PR's own test docstring names) and re-ran test_keyboard_interrupt_still_aborts_as_itself: it failed, because the over-correction now catches the injected KeyboardInterrupt, chains it into a RequestException, and self.assertRaises(KeyboardInterrupt) no longer sees a KeyboardInterrupt at all. Exit code 1. Reverted afterward; diff against head empty.

Fails-without proofs -- three of the four rows independently reproduced

1. Loop fix reverted (restored with contextlib.suppress(Exception): input(...), gate kept): 5 failed, 14 passed -- exact match to the PR's row, exit code 1.
2. Both reverted (pcapkit/vendor/default.py fully back to main): I got 18 failed, 1 passed, not the claimed 17. Investigated rather than just flagging the mismatch: with the whole file reverted, stdin_is_interactive does not exist as a module attribute at all, and the test harness's _cornered context manager does mock.patch.object(self.default, 'stdin_is_interactive', return_value=interactive) without create=True -- so every test that reaches _cornered (including test_keyboard_interrupt_still_aborts_as_itself, which the PR's own comment says "passes on the unfixed code too") fails with AttributeError: <module> does not have the attribute 'stdin_is_interactive' rather than exercising the actual old behavior. This is a genuine, minor discrepancy in the PR's own count for this specific row (a harness artifact of testing new test code against an old module, not a defect in the fix), and it means this row's per-test breakdown is measuring "does the suite explode when the module lacks the new function" rather than a clean logic comparison against the pre-fix behavior. It does not change the substance -- nearly everything fails either way -- and I would not block on it, but it is worth naming since I cannot independently confirm the PR's exact "17" and the KeyboardInterrupt claim needs the caveat above.
3. Interactivity gate reverted alone (loop fix kept): confirmed via reading the single test test_a_non_interactive_run_never_reaches_the_prompt (tests/vendor/test_request_prompt_unit.py:507-533) rather than re-running it in isolation this round (already reproduced on the pre-rebase head). It is thin in test-method count (only one), but the method itself carries six independent assertions -- exception identity, zero input() calls, zero browser opens, zero temp directories, zero stdout, and the specific warning message -- so a regression in any one of those six behaviors would be caught even though only one test name appears in the table. Judged adequate, not a blocker; more granular test-splitting would only help diagnosis, not coverage.

Coverage -- independently measured, matches

coverage run -m pytest tests/vendor/ on the pre-rebase head: pcapkit/vendor/default.py 67%, 202 statements, 59 missed, 8 partial branches -- matches the PR's claimed 50%->67%, misses 86->59, partials 9->8 (I did not independently re-derive the "before" 50%/86 baseline against main, given time budget, but the "after" number is exact).

The Jupyter trade-off -- judgement call, not a defect

Confirmed by reading stdin_is_interactive() that it answers strictly from sys.stdin.isatty(), so an environment where isatty() is False but a patched input() still works (the PR names Jupyter/ipykernel specifically) now gets refused rather than the old spin. This is a real, disclosed behavior change. I agree with the PR's own framing that this is acceptable for what is a maintainer-only vendor crawler, weighed against a print loop that measured 26.7 million lines / 2.6 GB in the original issue -- but this is the owner's call, not mine, so it is on the verdict line rather than filed as NEEDS CHANGES.

Final confirmation on the rebased head

PYTHONSAFEPATH=1 PYTHONPATH=<worktree> pytest tests/vendor/test_request_prompt_unit.py -v on 7a886e63a directly: exit code 0, 16 passed, 3 subtests passed -- matches the "both halves in place" row exactly.

CI status

Not run. GitHub Actions backed up throughout this review session; verdict on local evidence only, per standing instruction.

What remains unverified

• The "before" coverage baseline (50%, 86 missed) against main was not independently re-derived this round.
• The exact "17 failed" count on the fully-reverted-file row could not be reproduced (I got 18, explained above as a harness AttributeError artifact rather than a logic discrepancy); the qualitative conclusion of that row (nearly everything fails without the fix) is unaffected.
• mypy/bandit/isort/pylint clean-run claims were not independently re-run.