cleanup: deferred Nous review refinements#61
Merged
Conversation
Follow-up to the review pass on the Nous OAuth + agent_key work. Five small refinements that improve clarity or reduce a rare extra hop, none of which change correctness: _force_remint pre-checks OAuth expiry. When inference 401s and we force a re-mint, an OAuth that's also expiring is now refreshed first instead of being discovered via mint-401-triggers-refresh-retry. Saves one round-trip on the rare double-stale path (mint+refresh+mint = 2 hops vs the previous mint+401+refresh+mint = 3). The mint's own 401 retry still backstops portal-side OAuth revocation that wasn't visible through the skew check. _parse_error_body falls back to a body-text snippet when the response isn't JSON. A CDN HTML error page or a portal outage that returns text/plain previously degraded to a generic "unknown: status N" with no diagnostic. Now the error message includes up to 512 chars of the raw body so an operator can correlate the failure with what came back. _format_oauth_error tightens the "reuse" check to match the explicit code field, not the free-form detail string. Previously a substring search on detail could false-positive on unrelated portal messages like "this is not a reusable connection". Test added as a regression guard. Comment cleanup. _maybe_resolve_nous_lm docstring trimmed from a ~20-line protocol re-explanation to a 6-line summary that points at nous_lm.py for the canonical model. Three places previously documented the two-stage credential model independently; trimming two reduces the drift surface. _sync_from_shared_state lost its restate-the-body docstring. _absorb_mint_response gained a docstring documenting the api_key/agent_key field-name fallback and the expires_at-vs-expires_in precedence. _oauth_needs_refresh and _agent_key_needs_mint now cross- reference each other where they intentionally diverge on the unknown- expiry default (OAuth: don't refresh, mint backstops; agent_key: do re-mint, no equivalent backstop). Initial-mint comment trimmed to the cost-shape rationale only — the "missing or already expiring" half restated _ensure_credentials's already-clear name.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Follow-up to PR #60. Five small refinements deferred from that PR's review pass — none change correctness; all sharpen clarity, error messages, or save a round-trip on a rare path.
_force_remintpre-checks OAuth expiry_parse_error_bodyfalls back to body-text snippet on JSON parse failure_format_oauth_errormatches "reused" against the code field, not the free-form detaildetailcould false-positive on unrelated portal messages like "this is not a reusable connection". Regression test added._maybe_resolve_nous_lmdocstring trimmednous_lm.pyfor the canonical model. Three places were documenting the two-stage credential model independently; trimming two reduces the drift surface._oauth_needs_refresh↔_agent_key_needs_mintcross-reference_sync_from_shared_state+_absorb_mint_response+ initial-mint commentsTest plan
_force_remintwith stale OAuth (proves refresh-first ordering + Bearer carries refreshed token),_parse_error_bodyfallback to body text, regression guard against the "reusable connection" false-positive, empty-body fallback to status-only.tests/manual/nous_smoke.py) still passes all 5 scenarios.Out of scope
The third place documenting the two-stage model —
docs/model_resolution.md's Nous Portal section — stays detailed because that's the user-facing entrypoint. The trimming above only consolidates the internal documentation to one canonical location (nous_lm.py).