Skip to content

refactor(auth): extract TLS verification helpers (S3) - #80676

Open
andrexibiza wants to merge 1 commit into
NousResearch:mainfrom
andrexibiza:gfg/auth-s3-20260806-ship
Open

refactor(auth): extract TLS verification helpers (S3)#80676
andrexibiza wants to merge 1 commit into
NousResearch:mainfrom
andrexibiza:gfg/auth-s3-20260806-ship

Conversation

@andrexibiza

@andrexibiza andrexibiza commented Aug 7, 2026

Copy link
Copy Markdown
Contributor

What changed

Extract the S3 TLS verification helpers from hermes_cli/auth.py into hermes_cli/auth_tls_verify.py.

  • Exact pinned context window: auth.py:5069–5125.
  • Exact moved definitions: _default_verify and _resolve_verify (5073–5123).
  • The extraction is byte/AST faithful and keeps the legacy hermes_cli.auth compatibility seam.
  • The destination remains standalone-importable and keeps certifi lazy.

This is one independently reviewable slice of the auth.py god-file . It does not claim the full auth.py decomposition is complete.

Verification

Base: origin/main 0957277f2f468bac22bbfcfa7c43029858c9597e.

  • Two independent fresh W3 witnesses: PASS on byte/AST fidelity, standalone import, lazy certifi, zero orphans, monkeypatch transparency, delattr/NameError behavior, py_compile, reverse-apply, and diff hygiene.
  • Baseline differential at the pinned source proved the only selected failures are pre-existing Windows mode-bit assertions: the mandatory pair was 33 passed / 1 identical failure on both baseline and candidate; the expanded 83-test union was 81 passed / 2 identical baseline failures on both trees.
  • Current-main merger commit: 51b382430b25dc0eb88e6f7e3f535d68c2ffd07e.
  • Current-main selected suite: 81 passed, 0 failed, with only the two proven baseline failure IDs deselected.
  • git diff --check: clean.
  • py_compile: both changed modules pass.
  • Changed files only: hermes_cli/auth.py, hermes_cli/auth_tls_verify.py.

The two proven baseline-only failures are:

  • tests/hermes_cli/test_auth_nous_provider.py::test_shared_store_write_and_read_roundtrip
  • tests/hermes_cli/test_web_server_oauth_write.py::test_dashboard_oauth_write_uses_owner_only_permissions

They assert POSIX mode bits unavailable on this Windows host and fail byte-for-byte identically on the pristine pin and candidate. No candidate-related failure was deselected.

Why this matters to users

The extraction removes a self-contained TLS verification cluster from the auth god-file while preserving the existing hermes_cli.auth patch/import contract. Users get the same TLS behavior with a smaller, independently maintainable module.

Links

Part of #78647
Part of #78637

Scope note

This PR is S3 only. S1 is separately linked in #80673; S4 and S5 remain gated by their own evidence and are not included here.

Credit

The shipped commit is authored and DCO-signed by andrexibiza <84248988+andrexibiza@users.noreply.github.com>.

Interlock

The auth and scoreboard live on #78637. This PR is separately bound to the tracker and file tracker above.

Part of #78647
Part of #78637

Signed-off-by: andrexibiza <84248988+andrexibiza@users.noreply.github.com>
@alt-glitch alt-glitch added type/refactor Code restructuring, no behavior change P3 Low — cosmetic, nice to have comp/cli CLI entry point, hermes_cli/, setup wizard area/auth Authentication, OAuth, credential pools sweeper:risk-security-boundary Sweeper risk: may affect sandboxing, auth, credentials, or sensitive data labels Aug 7, 2026
@Enough1122

Copy link
Copy Markdown
Contributor

AI code review — automated review for reference, author can ignore or act on any point.

refactor(auth): extract TLS verification helpers (S3)

  1. Per-call mutation of a shared module attribute is a concurrency smellhermes_cli/auth.py _resolve_verify (~L5089-5108): every call temporarily swaps _auth_tls_verify._default_verify under _AUTH_TLS_VERIFY_COMPAT_LOCK so that monkeypatches of hermes_cli.auth._default_verify stay effective. A thread calling auth_tls_verify._resolve_verify directly during the swap window observes the compat proxy instead of the leaf implementation — behaviorally equivalent today (the proxy funnels back to the captured original), but surprising. A cleaner shape: pass the default-verify callable as a parameter (_resolve_verify(default_verify=...)) and let auth._resolve_verify supply auth._default_verify — no global swap, same monkeypatch compat.
  2. The capture _AUTH_TLS_DEFAULT_VERIFY_IMPL = _auth_tls_verify._default_verify (import time) is what prevents recursion — worth a comment, because a reader might otherwise see _default_verify() calling into the module it's defined in and worry about infinite recursion. The lock + swap + finally-restore is correct as written.
  3. Logger name is duplicated deliberatelylogger = logging.getLogger("hermes_cli.auth") in the leaf keeps log routing identical to before the extraction; good, and the pattern is consistent with auth_errors.py. Consider extracting that logger-name constant to a shared spot if more S-series extractions follow.
  4. Behavior is preserved (insecure → False, ca_bundle existence check with fallback warning, default verify). Tests aren't shown in this diff for the moved functions — confirm the existing _resolve_verify tests still run against the extracted module.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area/auth Authentication, OAuth, credential pools comp/cli CLI entry point, hermes_cli/, setup wizard P3 Low — cosmetic, nice to have sweeper:risk-security-boundary Sweeper risk: may affect sandboxing, auth, credentials, or sensitive data type/refactor Code restructuring, no behavior change

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants