Send X-Authorization header after JWT login

[irynamatveieva]

No description provided.

[irynamatveieva]

Review summary

Reviewed 5 changed files in Send X-Authorization header after JWT login. Left 6 comment(s) inline.

The diagnosis behind the fix checks out — Configuration.auth_settings() gates on 'ApiKeyForm' in self.api_key (configuration.py:533) while the only thing that would install it, refresh_api_key_hook, runs inside get_api_key_with_prefix() (configuration.py:499), so without the seed the JWT login path really could never emit a header. The four client.py copies are byte-identical, matching the generate-client.sh overlay workflow. Comments are about one uncovered auth-argument combination, some duplication of the ApiKeyForm/Bearer literals, and the shape of the new tests.

---

This review was auto-generated. Findings may contain errors — please verify before applying changes.

[irynamatveieva]

Re-review summary

Re-reviewed Send X-Authorization header after JWT login — verified 6 finding(s) from previous review.

Status	Count
✅ Resolved	6
💬 Acknowledged	0
❌ Unresolved	0

All previous findings have been addressed, and the reworked shape is better than the original fix: install_header() is now the single writer of the X-Authorization slot, the three scheme literals each appear exactly once in _auth.py, and using install_header() rather than hook() from the constructor correctly avoids firing a refresh request on the token= path. Also found 7 new issue(s) in the fix commits, commented inline — all refinements (untested branches, a duplicated explanation, an asymmetric guard in the new overlay test), none blocking.

Finding details

• ✅ common/client.py:137 — mixing api_key= with username= installed a JWT that hook() would never refresh — Fixed in code: __init__ now raises ValueError for mixed modes (common/client.py:98-111), with three tests. The ApiKeyForm/Bearer/ApiKey literals now appear exactly once each in common/_auth.py:40-42, written only by the new install_header().
• ✅ common/client.py:129 — nine-line comment restating the PR description, plus the inaccurate "exactly as the api_key branch" claim — Fixed in code: trimmed to five lines and the wrong claim is gone.
• ✅ paas/tb_paas_client/client.py:137 — nothing enforced that the committed edition copies match common/ — Fixed in code: tests/test_common_overlay.py compares them byte-for-byte, and the import-time copy in test_facade.py that was masking ce is removed.
• ✅ tests/test_client.py:54 — the same four-line patch-and-construct preamble repeated in each new test — Fixed in code: _logged_in_client() helper plus a module-level _LOGIN_PATCH_TARGET.
• ✅ tests/test_client.py:59 — test_jwt_login_seeds_configuration_api_key was subsumed by the auth_settings() test — Fixed in code: dropped.
• ✅ tests/test_client.py:93 — the rotation test never reached _refresh_if_needed(), so it only re-tested hook()'s overwrite — Fixed in code: rewritten with an expired access token and a patched _raw_post, asserting the /api/auth/token call and the refreshed header.

Additional findings

These observations are about existing code outside the PR's diff — spotted while reading surrounding context.

• tests/test_client.py:13 — # conftest.py handles sys.path; this import will work once client.py is copied is a leftover from the copy-at-import-time era this commit removed, and it now contradicts the module docstring three lines above ("the committed overlay copy"). Worth deleting so nobody goes looking for a copy step that no longer exists.

---

This re-review was auto-generated. Findings may contain errors — please verify before applying changes.

[irynamatveieva]

Now that install_header() carries this exact rationale in its own docstring, this five-line comment restates it a second time — and the docstring of test_jwt_login_emits_x_authorization_header says it a third. Three copies of one explanation drift apart the moment one of them is edited. Since the knowledge belongs to _AuthManager, could this shrink to something like # Seed the header slot for whichever mode ran — see _AuthManager.install_header and leave the reasoning in the single place that owns it?

[irynamatveieva]

Down to one line: # Seed the header slot for whichever mode ran — see _AuthManager.install_header. The reasoning stays in the install_header() docstring. Trimmed the third copy too — test_jwt_login_emits_x_authorization_header is now a one-line docstring.

[irynamatveieva]

The "Additional findings" note is fixed too — the stale # conftest.py handles sys.path; this import will work once client.py is copied comment in tests/test_client.py is gone; the module docstring already says the tests read the committed overlay copy.

All 7 inline findings from the re-review are addressed in aea18bb, each answered in its own thread.

[irynamatveieva]

Re-review summary

Re-reviewed Send X-Authorization header after JWT login — verified 7 finding(s) from previous review.

Status	Count
✅ Resolved	7
💬 Acknowledged	0
❌ Unresolved	0

All previous findings have been addressed, including the out-of-diff note about the stale comment at tests/test_client.py:13 — that's gone too. Two claims from the replies I checked rather than took on trust: _auth_type is assigned once (common/_auth.py:125) and only read at line 183, so the cached _header_prefix genuinely can't go stale; and the */tb_*_client glob matches only ce, pe, paas, since build-packages.sh builds into dist/ and leaves no depth-1 package directory to pollute discovery.

Also found 7 new issue(s) in the fix commits, commented inline. They are smaller than the previous rounds' — one unguarded mirror case in the validation, a partial-discovery hole in the new overlay test, and polish on the new shared test module. Nothing here blocks merging.

Finding details

• ✅ common/client.py:104 — password=/refresh_token= were silently dropped when passed without their own mode — Fixed in code: both now raise (common/client.py:113-118), with regex-pinned tests; the zero-modes case was correctly left alone and now has a test of its own.
• ✅ common/client.py:149 — the seed comment restated install_header()'s docstring — Fixed in code: one-line pointer, and the third copy in the test docstring is gone too.
• ✅ common/_auth.py:170 — the header prefix was recomputed from _auth_type on every hook call — Fixed in code: resolved once as self._header_prefix in __init__.
• ✅ common/_auth.py:166 — the if not token branch was untested and test_preexisting_token asserted no prefix — Fixed in code: test_no_auth_leaves_header_slot_absent pins construction, the absent slot and auth_settings() == {}; the prefix assertion makes the token= test symmetric with its api_key sibling.
• ✅ tests/test_common_overlay.py:18 — the module list had a meta-test but the edition list had none — Fixed in code: both lists are derived from the repo, and iterating common/ entries rather than globbing *.py also covers non-Python files, matching what generate-client.sh:181 actually copies.
• ✅ tests/test_client.py:17 — helpers were imported from another test module — Fixed in code: moved to a stdlib-only tests/_jwt.py that pulls in neither common/ nor tb_ce_client.
• ✅ tests/test_client.py:122 — the validation tests asserted only the exception type and skipped the three-mode case — Fixed in code: all use assertRaisesRegex pinning the mode names, plus test_all_three_modes_rejected.

---

This re-review was auto-generated. Findings may contain errors — please verify before applying changes.

[irynamatveieva]

Re-review summary

Re-reviewed Send X-Authorization header after JWT login — verified 7 finding(s) from previous review.

Status	Count
✅ Resolved	7
💬 Acknowledged	0
❌ Unresolved	0

All previous findings have been addressed. Correctness-wise this commit is clean — the things worth checking all held up: _auth_type is gone with zero dangling references in common/, the three edition copies or the tests; the hook() predicate flip from != "jwt" to _is_api_key is behaviour-preserving because every _AuthManager(...) call site passes only "jwt" or "api_key"; the new username= requires password= guard breaks no documented usage (every ThingsboardClient(...) example in README and common/docs/tb-examples.md passes both); all four copies of client.py and _auth.py are still byte-identical; and CI is green on all four jobs.

Also found 4 new issue(s) in the fix commits, commented inline — one docstring that didn't keep up with the new guard, and three maintainability notes on the reworked overlay test and the now-redundant auth_type parameter. Nothing blocking.

Finding details

• ✅ common/client.py:115 — username= without password= surfaced as a pydantic ValidationError from LoginRequest instead of a ValueError — Fixed in code: symmetric guard at common/client.py:121-124, with a regex-pinned test.
• ✅ common/client.py:56 — the no-auth construction path was tested but undocumented — Fixed in code: class docstring plus a "No authentication" entry in README, with the pairing rules spelled out.
• ✅ common/_auth.py:128 — the class decided "am I api_key auth?" in three places using two different literals — Fixed in code: _is_api_key drives all three branches and _auth_type is removed entirely (verified: no references left anywhere). Keeping _header_prefix derived from the flag was the right call — recomputing it inline would have put the conditional back on the per-request path.
• ✅ tests/test_common_overlay.py:43 — the non-empty guard caught total but not partial discovery failure — Fixed in code: editions are parsed from generate-client.sh's EDITIONS=(...) array, so a missing package directory now fails on copy.is_file() instead of dropping out of the parametrization.
• ✅ tests/test_common_overlay.py:28 — iterdir() yielded directories, so a subdirectory in common/ would have raised IsADirectoryError — Fixed in code: rglob with is_file(), path-aware exclusions, and "file" vocabulary throughout.
• ✅ tests/test_client.py:153 — the test's api_key= contradicted its "refresh_token= alone" docstring — Fixed in code: dropped, so it's now the bare case.
• ✅ tests/_jwt.py:40 — near-duplicate factories and private-looking names on a shared module — Fixed in code: underscores dropped; the two factories were deliberately kept, which I agree with — the docstring now carries the "refresh tokens carry no iat" property instead of restating the signature.

Additional findings

These observations are about existing code outside the PR's diff — spotted while reading surrounding context.

• common/docs/tb-examples.md:5 — the packaged usage examples cover only "JWT Login" and "API Key Login". The pre-existing-token mode was never documented there, and the no-auth mode README just gained isn't either. That gap predates this PR, but common/docs/ is what gets overlaid into every edition's docs/, so those readers see two of four modes. Worth closing while the auth documentation is already being touched.

---

This re-review was auto-generated. Findings may contain errors — please verify before applying changes.

[irynamatveieva]

The "Additional findings" note about common/docs/tb-examples.md is fixed too — it now documents all four modes. Added a Pre-existing Token section and a No Authentication section, plus a line stating that the three authenticated modes are mutually exclusive and that the companion arguments are required in pairs. Since common/docs/ is overlaid into every edition, the same content is synced to ce/docs/, pe/docs/ and paas/docs/.

All 4 inline findings are addressed in c304eed, each answered in its own thread.

[irynamatveieva]

Re-review summary

Re-reviewed Send X-Authorization header after JWT login — verified 4 finding(s) from previous review.

Status	Count
✅ Resolved	4
💬 Acknowledged	0
❌ Unresolved	0

All previous findings have been addressed, and so was the out-of-diff note about common/docs/tb-examples.md — it now documents the pre-existing-token and no-auth modes, propagated to all three edition copies. Verified independently: auth_type is fully gone (the only "jwt" literals left are inside a docstring example), _AuthManager is not re-exported from any package __init__.py so the signature change is internal, the EDITIONS regex still resolves ['ce', 'paas', 'pe'] with the new comment in place, all four copies of client.py, _auth.py and tb-examples.md are byte-identical, and CI is green on all four jobs.

Also found 5 new issue(s) in the fix commits, commented inline. One is worth acting on: the newly-worded Raises: block and docs sentence both claim token= requires refresh_token=, which the code does not enforce — and interestingly the README wording from the previous round has it right, so the two new texts diverge from it.

Finding details

• ✅ common/client.py:95 — the Raises: block omitted the new username= guard, and the class docstring's "three authentication modes" preceded a fourth — Fixed in code: both re-worded (though the new Raises: wording introduces a different inaccuracy — see the inline comment).
• ✅ common/_auth.py:129 — auth_type was vestigial once nothing read the string — Fixed in code: the parameter is gone, _is_api_key = api_key is not None is derived internally, and client.py no longer encodes a mode name only to have it decoded again. Doing it now rather than as a follow-up was the right call — it was 9 mechanical call sites.
• ✅ tests/test_common_overlay.py:51 — the regex's coupling to generate-client.sh was invisible from the script side — Fixed in code: a comment at generate-client.sh:63-64 states the one-line/column-0/double-quoted contract. Confirmed the regex still resolves with it in place.
• ✅ tests/test_common_overlay.py:27 — one comment block covered two sets with different anchoring, and neither the recursion nor the any-depth filter had real input behind it — Fixed in code: the sets are split with their own rationales, the second renamed _EXCLUDED_DIRS_ANY_DEPTH, and test_walk_exclusion_semantics pins the behaviour against a fixture tree.

---

This re-review was auto-generated. Findings may contain errors — please verify before applying changes.

[irynamatveieva]

Re-review summary

Re-reviewed Send X-Authorization header after JWT login — verified 5 finding(s) from previous review.

Status	Count
✅ Resolved	5
💬 Acknowledged	0
❌ Unresolved	0

Disclosure: this round is a self-review. Commit 4dda84f1 was authored by the same automated reviewer that produced the previous rounds' findings, so the independent-author check that normally catches this tooling's own mistakes is missing here. The findings below were deliberately held to the same bar as earlier rounds, and several of them are things the fix commit got wrong while fixing something else — but a human pass over 4dda84f1 is worth more than usual, not less.

Also found 9 new issue(s) in the fix commits, commented inline. Two are worth acting on: the common/docs/ discovery helper reintroduces the vacuous-pass hole this module exists to prevent, and a loose assertion in the new test papers over a real mismatch between get_refresh_token()'s documented None and the "" it actually returns.

Finding details

• ✅ common/client.py:95 — the Raises: block claimed token= requires refresh_token=, which the code never enforced — Fixed in code: reworded to the three checks that exist, plus a note that token= alone is valid. See the inline comment about where that note belongs.
• ✅ common/docs/tb-examples.md:52 — same inaccuracy in the shipped docs — Fixed in code, and propagated to all three edition copies.
• ✅ common/docs/tb-examples.md:26 — nothing pinned the four copies of the shared docs in sync — Fixed in code: test_edition_doc_copy_matches_common compares common/docs/* against each <edition>/docs/, and the two new sections were added to test_readme.py's required list.
• ✅ common/_auth.py:125 — the comment argued against the auth_type design that no longer exists — Fixed in code: trimmed to two lines (though see the inline comment — what survived is still largely restatement).
• ✅ tests/test_common_overlay.py:89 — str(Path(...)) gymnastics and a fixture that pinned only one half of the docs anchoring — Fixed in code: rel.as_posix() and a nested sub/docs/guide.md case. The failure messages still interpolate a Path, though — see inline.

---

This re-review was auto-generated. Findings may contain errors — please verify before applying changes.

[irynamatveieva]

Re-review summary

Re-reviewed Send X-Authorization header after JWT login — verified 9 finding(s) from previous review.

Status	Count
✅ Resolved	9
💬 Acknowledged	0
❌ Unresolved	0

Disclosure: one of the two commits in range is self-authored. 6eec0f2f was written by the same automated reviewer that produced the previous rounds' findings; 5180dfae was written by the repo owner. Most of the findings below land on 6eec0f2f, which is what you would expect when the author and reviewer are the same — a human pass over that commit is still worth more than this one.

5180dfae checks out cleanly: filtering its patch for anything that is not a path or alias substitution returns nothing, so no assertion changed, and the retarget loses no coverage — test_edition_doc_copy_matches_common still asserts each <edition>/docs/tb-examples.md exists, and DOC-04 appears nowhere outside that test file.

Also found 7 new issue(s) in the fix commits, commented inline. The two most concrete: the new assert root.is_dir() prints remediation advice that cannot work, and one row of the new required-sections table would still pass if its section were deleted.

Finding details

• ✅ tests/test_common_overlay.py:60 — the return [] guard let the docs check collect zero cases and pass vacuously — Fixed in code: it asserts now. See the inline comment — the assertion's message and placement have their own problems.
• ✅ tests/test_client.py:109 — assertFalse hid a mismatch between get_refresh_token()'s documented None and the "" it returned — Fixed in code at the source: set_external_token no longer coerces with or "", and the test asserts assertIsNone. Choosing to fix the contract rather than the assertion was the right call.
• ✅ tests/test_common_overlay.py:53 — the docs helper hardcoded its root and its flat-not-recursive contract was unexercised; "docs" was load-bearing in three places — Fixed in code: root parameter, test_doc_walk_is_flat, and _DOCS_DIRNAME.
• ✅ common/client.py:95 — the "token= alone is valid" note sat under Raises: — Fixed in code: moved to the refresh_token: arg and the mode-3 example.
• ✅ tests/test_readme.py:133 — the section checks matched substrings anywhere, repeated eight near-identical asserts, and validated the ce copy — Fixed in code across both commits: heading-anchored for the two auth-mode sections, collapsed into a table, and retargeted at common/docs/. See inline for two rows that stayed loose.
• ✅ tests/test_common_overlay.py:140 — the failure message interpolated a Path, and the two overlay bodies were near-clones — Fixed in code: as_posix() on the mismatch message and _assert_identical. See inline — one message in that helper still isn't converted.
• ✅ common/docs/tb-examples.md:52 — "without its companion argument" covered three arguments whose companions differ in kind — Fixed in code: spells out each pairing the way the docstring does.
• ✅ README.md:86 — the parallel note was less complete than the shipped docs — Fixed in code: both now carry the same clause.
• ✅ common/_auth.py:125 — the surviving comment was mostly restatement — Fixed in code: removed, with the durable fact moved to the class docstring.

---

This re-review was auto-generated. Findings may contain errors — please verify before applying changes.

[irynamatveieva]

Re-review summary

Re-reviewed Send X-Authorization header after JWT login — verified 7 finding(s) from previous review.

Status	Count
✅ Resolved	7
💬 Acknowledged	0
❌ Unresolved	0

All previous findings have been addressed, and two went further than suggested: the section table heading-anchored every auth-mode and usage section rather than only folding the JWT case in, and _assert_identical now composes the whole remediation from a destinations argument instead of taking it pre-built. I spot-checked the claim behind the heading change and it holds — deleting ## Context Manager now fails its row, where a bare "with " previously survived.

Also found 7 new issue(s) in the fix commit, commented inline. The one worth acting on: the new comment's justification for leaving the operation rows substring-matched is false for device, so that row still has the exact hole this commit closed for "with ".

Finding details

• ✅ tests/test_common_overlay.py:66 — the assert root.is_dir() printed unworkable remediation and fired at collection time, taking the whole module down — Fixed in code: back to a tolerant empty list, with the reasoning documented and the behaviour pinned by a test; the discovery test remains the single reporter.
• ✅ tests/test_readme.py:132 — the ("with ", "context manager") row survived deleting the section it guarded — Fixed in code, and more thoroughly than asked: every auth-mode and usage section is heading-anchored now. Verified the fix bites.
• ✅ common/_auth.py:155 — dropping or "" left refresh_token: str describing the old contract — Fixed in code: both set_external_token and _build_token_info widened to "str | None" in all four copies. See inline — the public constructor still has the same pattern.
• ✅ tests/test_common_overlay.py:91 — the missing-file message wasn't as_posix()-converted and the two remediation strings duplicated the source path — Fixed in code: both paths computed once, remediation composed in the helper.
• ✅ tests/test_common_overlay.py:27 — the _DOCS_DIRNAME comment overstated an invariant the script doesn't enforce — Fixed in code: the comment now says the destination is hardcoded separately.
• ✅ tests/test_client.py:86 — one extracted and one inline form of the same header assertion — Fixed in code: _assert_header_slot(client, token, prefix) serves all three, at the top of the class.
• ✅ tests/test_readme.py:113 — vestigial examples = TB_EXAMPLES aliases and an asymmetric README half — Fixed in code: aliases gone, README constant added.

---

This re-review was auto-generated. Findings may contain errors — please verify before applying changes.

[irynamatveieva]

Re-review summary

Re-reviewed Send X-Authorization header after JWT login — verified 7 finding(s) from previous review.

Status	Count
✅ Resolved	7
💬 Acknowledged	0
❌ Unresolved	0

All previous findings have been addressed. The device fix went further than asked — rather than anchoring the one bad row, every row is heading-anchored via _REQUIRED_HEADINGS, which let the alternatives-tuple machinery go entirely. I checked the result rather than taking it on trust: all eight headings occur exactly once, and deleting any of the eight sections now fails its row, including ## List Devices.

Also found 8 new issue(s) in the fix commit, commented inline. One matters: the heading match is case-folded and unanchored, so a ## → ### demotion or a casing change still satisfies it — the anchoring is tighter than before but not as tight as the tuple implies.

Finding details

• ✅ tests/test_readme.py:125 — the device row's substring survived deleting the section it guarded, and the comment's premise was false — Fixed in code: every row is now a heading match. Verified all eight are deletion-sensitive. See inline for a residual looseness in how the match is performed.
• ✅ common/_auth.py:149 — the annotation widening stopped short of the public constructor — Fixed in code: ThingsboardClient.__init__, _AuthManager.__init__ and _TokenInfo.__init__ all widened, in all four copies.
• ✅ tests/test_client.py:45 — the helper pinned internal slots only, and the api-key path had no emitted-header assertion — Fixed in code: _assert_header_slot now asserts the value auth_settings() emits.
• ✅ tests/test_common_overlay.py:155 — the missing-directory case sat inside the flatness test, and the claimed rglob parity was unpinned — Fixed in code: test_missing_directory_yields_empty_list, parametrized over both helpers so the parity is self-enforcing.
• ✅ tests/test_readme.py:128 — every row had one alternative, so the tuple/any() machinery was vestigial — Fixed in code: replaced by a flat _REQUIRED_HEADINGS tuple.
• ✅ tests/test_readme.py:20 — the two halves of the file were the same tests twice — Fixed in code: parametrized over DOCUMENTS with ids=.
• ✅ tests/test_common_overlay.py:106 — the remediation told readers to edit a source that was fine — Fixed in code: rephrased action-first. See inline — the trailing clause still carries the old premise.

---

This re-review was auto-generated. Findings may contain errors — please verify before applying changes.

[irynamatveieva]

Re-review summary

Re-reviewed Send X-Authorization header after JWT login — verified 8 finding(s) from previous review.

Status	Count
✅ Resolved	8
💬 Acknowledged	0
❌ Unresolved	0

All previous findings have been addressed. I checked the main one empirically rather than by reading: the anchored, case-sensitive regex genuinely closes both holes — ### List Devices, #### List Devices, ## list devices and ## List Devices extra all fail now, while all eight real headings still match. Also confirmed the new @cache can't go stale (the only tb-examples.md write in the suite is inside a tmp_path fixture), no stale references to the removed _extract_python_blocks, and the four _auth.py copies are still byte-identical.

Also found 6 new issue(s) in the fix commit, commented inline. One is worth acting on: the heading rule was tightened for tb-examples.md but the README side of the same module still uses a bare substring, so the hole this commit closed is still open one screen away.

Finding details

• ✅ tests/test_readme.py:139 — the heading match was case-folded and unanchored, so a ### demotion or casing change passed — Fixed in code: anchored case-sensitive regex. Verified all four bad variants now fail.
• ✅ tests/test_readme.py:119 — the comment's section count was wrong and most of it had become archaeology — Fixed in code: trimmed to the durable sentence, wrong count gone.
• ✅ common/_auth.py:157 — get_token/get_refresh_token were the only methods without return annotations — Fixed in code: both -> "str | None", in all four copies.
• ✅ tests/test_common_overlay.py:108 — the remediation's trailing clause contradicted its own comment on the missing-copy branch — Fixed in code: clause dropped. See inline — the replacement comment now describes text that isn't there.
• ✅ tests/test_readme.py:28 — each DOCUMENTS label duplicated a derivable relative path — Fixed in code: DOCUMENTS = (README, TB_EXAMPLES) plus a _label() helper used for ids= and every message.
• ✅ tests/test_readme.py:75 — the read/extract/guard trio was duplicated across both document tests — Fixed in code: _python_blocks() over a @cached _read(). See inline on both.
• ✅ tests/test_client.py:43 — the helper's docstring claimed to pin the header name but didn't — Fixed in code: it now asserts emitted["key"], which let the standalone JWT test collapse into a helper call.
• ✅ tests/test_common_overlay.py:161 — the new test's docstring restated the helper's — Fixed in code: trimmed.

---

This re-review was auto-generated. Findings may contain errors — please verify before applying changes.

[irynamatveieva]

Re-review summary

Re-reviewed Send X-Authorization header after JWT login — verified 6 finding(s) from the previous review.

There are no new commits since that review — the head is still bbf84bf0, and none of the six comments have replies. So nothing has been addressed yet and every finding carries forward unchanged; the table below is not a judgement on the fixes, there simply aren't any to judge.

Status	Count
✅ Resolved	0
💬 Acknowledged	0
❌ Unresolved	6

With no fix diff to review, this round instead re-read the PR's current state end to end and found 9 issues the previous nine rounds had not raised — commented inline, and one below that sits outside the diff.

Finding details

• ❌ tests/test_readme.py:113 — the README ## Quickstart check is a bare substring while the tb-examples heading check is anchored — Still present, not addressed.
• ❌ tests/test_readme.py:146 — \s*$ is looser than the "nothing but trailing spaces" it means, and the newly-anchored behaviour is asserted only in the docstring — Still present, not addressed.
• ❌ tests/test_readme.py:48 — _python_blocks reads as a getter but also fails the test; -> list unparameterised; the new guard is untested — Still present, not addressed.
• ❌ tests/test_readme.py:42 — the @cache on _read saves microseconds on two small files at the cost of a session-lifetime global — Still present, not addressed.
• ❌ tests/test_common_overlay.py:105 — the comment documents text that is no longer there ("with no trailing 'after editing it'") — Still present, not addressed.
• ❌ tests/test_client.py:70 — the docstring enumerates the helper's other call sites, a claim about neighbouring tests that nothing keeps true — Still present, not addressed.

Additional findings

This observation is about existing code outside the PR's diff — spotted while reading surrounding context.

• common/_auth.py:256 — _raw_post builds a bare urllib3.PoolManager(), so token refresh and re-login ignore everything the user configured on Configuration: verify_ssl, ssl_ca_cert, cert_file and proxy. Against an HTTPS server with a private CA or behind a proxy, ordinary API calls work (they go through RESTClientObject, which honours those settings) but every refresh fails — and since _do_refresh_token only logs a warning and _do_login only logs an error, the client keeps sending the expired token and the user sees 401s with no obvious cause. There is also no timeout= on the http.request(...) call, so a hung auth endpoint blocks the calling API thread indefinitely with _refreshing held. Reusing the pool manager from api_client.rest_client (or at minimum passing the same TLS settings and a timeout) would close both gaps. Pre-existing rather than introduced here, but it's on the path this PR is making load-bearing.

---

This re-review was auto-generated. Findings may contain errors — please verify before applying changes.

[irynamatveieva]

Re-review summary

Re-reviewed Send X-Authorization header after JWT login — verified 15 finding(s) from the previous review against commit 028f86c6.

Status	Count
✅ Resolved	12
💬 Acknowledged	2
❌ Unresolved	1

The two substantive ones landed well. The threading.Condition change is correct — waiters loop on the predicate, the refresher notifies under the lock in a finally, and the HTTP call stays outside the lock, so there's no deadlock and no missed wakeup. Empty-credential rejection covers all five arguments and runs before the mutual-exclusion check, so the root cause is what gets reported.

One correction on my side: my note about tb_ce_client in the shared docs was wrong. Line 3 of tb-examples.md has carried the substitution instruction since before this PR and I missed it — the pushback is right and no change is needed there.

Also found 7 new issue(s) in the fix commit, commented inline. The one worth reading first is on common/_auth.py:207: making waiters block is the right fix, but it converts the pre-existing missing timeout in _raw_post from a one-thread stall into a whole-process one.

Finding details

• ✅ tests/test_readme.py:157 — README ## Quickstart was a bare substring while tb-examples was anchored — Fixed in code: both now go through _assert_heading.
• ✅ tests/test_readme.py:58 — \s*$ looser than intended, anchored behaviour asserted only in prose — Fixed in code: [ \t]*$, plus test_has_heading_is_anchored_and_case_sensitive pinning all four bad variants.
• ✅ tests/test_readme.py:66 — _python_blocks read as a getter but asserted; -> list; untested guard — Fixed in code: renamed _require_python_blocks, -> list[str], guard pinned by test_require_python_blocks_fails_without_any.
• ✅ tests/test_readme.py:46 — @cache on _read not earning its keep — Fixed in code: cache dropped.
• ✅ tests/test_common_overlay.py:99 — comment documented text that was no longer there — Fixed in code, and the broader trimming pass was taken across the module.
• ✅ tests/test_client.py:68 — docstring enumerated the helper's other call sites — Fixed in code.
• 💬 common/docs/tb-examples.md:31 — examples import tb_ce_client in a file copied verbatim to pe/paas — Developer: "line 3 already says to substitute tb_pe_client/tb_paas_client." Correct — I missed it, and it predates this PR. Their reason for not templating per edition (it would end the byte-identity the overlay check relies on) is sound.
• ✅ common/_auth.py:206 — a thread arriving mid-refresh returned early and sent the stale expired token — Fixed in code: threading.Condition with waiters blocking on the predicate, pinned by TestConcurrentRefresh.
• ✅ common/client.py:53 — empty credential silently produced an unauthenticated client — Fixed in code: rejected for all five auth arguments, ahead of the mutual-exclusion check.
• ❌ generate-client.sh:64 — a partial EDITIONS regex match can go green with fewer editions than exist — Open, awaiting a decision; see my reply inline.
• ✅ common/client.py:38 — validation crowding __init__ — Fixed in code: module-level _validate_auth_args, called in one line.
• ✅ common/client.py:67 — error messages didn't read as a set and weren't actionable — Fixed in code, with a test pinning the actionable clause.
• ✅ tests/test_client.py:159 — inconsistent patch/assert convention across the validation tests — Fixed in code: convention stated on the class and applied.
• ✅ tests/test_client.py:39 — _assert_header_slot sat on a class whose docstring no longer described it — Fixed in code: moved to module level, class renamed TestThingsboardClientAuthModes.
• 💬 tests/test_common_overlay.py:109 — helper tests supporting a directory layout that doesn't exist — Developer: "the recursion was added in round 3 to fix a crash-or-miss; flattening fails open, and __pycache__ filtering is load-bearing for local runs." Agreed — this is a real asymmetry and the recursion should stay. Withdrawn.

---

This re-review was auto-generated. Findings may contain errors — please verify before applying changes.

[irynamatveieva]

Re-review summary

Re-reviewed Send X-Authorization header after JWT login — verified 8 finding(s) from the previous review against commit 3e8fb5df.

Status	Count
✅ Resolved	8
💬 Acknowledged	0
❌ Unresolved	0

All eight are genuinely fixed — I checked each against the code rather than the replies. On your two open questions: yes, a follow-up issue is the right home for the verify_ssl / ssl_ca_cert / cert_file / proxy work — it needs _AuthManager to take the Configuration, which is a different change from header seeding and shouldn't be smuggled into a review round. And test_docs.py / test_split.py converging on editions.txt is fine to leave out of scope.

Also found 9 new issue(s) in the fix commit, commented inline. Two are worth reading first:

• common/_auth.py:277 — the timeout is a real improvement but 30s isn't the ceiling it looks like. PoolManager.request keeps urllib3's default Retry(total=3), and connect-class failures retry irrespective of the method allow-list, so a blackholed auth host costs up to 4 × 30s inside one _raw_post — then _do_refresh_token falls back to _do_login for a second one. I verified both halves against urllib3 1.26.5: Retry.DEFAULT is total=3, increment() takes the _is_connection_error branch without consulting allowed_methods, and a bare float expands to Timeout(connect=30, read=30, total=None).
• generate-client.sh:69 — moving the list into editions.txt closed the regex hole, but the format contract is now implemented twice and the two implementations disagree. I ran the real script against a fixture containing pa as: it reports Must be one of: ce pe paas all, while _editions() returns ['ce', 'pa as', 'pe']. test_editions_parsing_matches_the_script doesn't catch it because it never invokes the script.

Nothing here is a regression in behaviour that ships — the client's auth path is correct, and the overlay copies are all byte-identical to their common/ sources (I checked all nine).

---

This re-review was auto-generated. Findings may contain errors — please verify before applying changes.

[irynamatveieva]

This is the one timeout in the client a caller can't tune, and arguably the one most likely to need it — a slow on-prem TB behind a VPN wants more, a latency-sensitive service wants much less. The neighbouring knobs of the same shape (max_retries, initial_retry_delay_ms, max_retry_delay_ms) are all ThingsboardClient.__init__ keywords in integer milliseconds; this one is a float in seconds in a private module, so anyone who needs to change it ends up monkeypatching _auth.AUTH_REQUEST_TIMEOUT_S.

Threading an auth_timeout_ms: int = 30_000 through to _AuthManager.__init__ alongside the existing retry knobs would match the established pattern and keep the unit convention consistent. If it's deliberately fixed for now, a line in the comment saying why would save the next person the guess.

[irynamatveieva]

Agreed, and threaded through as auth_timeout_ms: int = 30_000 on both ThingsboardClient.__init__ and _AuthManager.__init__ — same integer-milliseconds convention as max_retries / initial_retry_delay_ms / max_retry_delay_ms, converted to seconds once at construction.

You picked the right argument: it's the one timeout bounding how long every other thread is blocked, so a fixed value in a private module was the wrong place for it. test_timeout_is_configurable pins that the keyword reaches the request.

I left it out of README/tb-examples, since neither documents the sibling retry knobs either — happy to add all four together if you'd rather they were documented.

[irynamatveieva]

Re-review summary

Re-reviewed Send X-Authorization header after JWT login — verified 9 finding(s) from the previous review against commit 835fef07.

Status	Count
✅ Resolved	9
💬 Acknowledged	0
❌ Unresolved	0

All nine are fixed, and I checked the two structural ones by running them rather than reading them. --list-editions genuinely closes the parser gap: I copied the script and your fixture into a temp dir, and shell and Python both return ['ce', 'ce # note', 'pa as', 'pe']; reintroducing tr -d '[:space:]' flips the shell to ce ce#note paas pe, so the agreement test does fail as you said. The timeout is now a real ceiling — Timeout(total=...) with retries=False.

On your open question about documenting auth_timeout_ms: leaving all four knobs undocumented is self-consistent and fine. If you do document them, do all four together — a README that mentions the auth timeout but not max_retries would read as an oversight rather than a choice.

Also found 6 new issue(s) in the fix commit, commented inline. Two are worth reading first:

• common/_auth.py:296 — retries=False switches off redirect following as a side effect, not just retries. I confirmed against a local server: with the previous default a 307 on /api/auth/login was followed and returned 200; with retries=False urllib3 hands back the 307 unfollowed, so _raw_post raises RuntimeError("... returned HTTP 307"). The generated RESTClientObject still uses urllib3's default retries, so ordinary API calls keep following redirects while auth calls no longer do.
• scripts/build-packages.sh:49 — process substitution discards the child's exit status, so set -euo pipefail never sees a failing --list-editions. I reproduced it: EDITIONS ends up empty and the script exits 0 having built nothing. That's the failure mode the edition_count guard closes in generate-client.sh, now reopened in the consumer.

The overlay copies are all byte-identical to their common/ sources, and there are no stale references to the renamed AUTH_REQUEST_TIMEOUT_S.

Finding details

• ✅ common/_auth.py:296 — timeout was a bare float with urllib3's default Retry(total=3), so the real worst case was ~4 × 30s twice over — Fixed in code: Timeout(total=...) plus retries=False, pinned by TestRawPostBounds.
• ✅ common/_auth.py:133 — the auth timeout was the one knob a caller couldn't tune — Fixed in code: auth_timeout_ms: int = 30_000 on both constructors, matching the sibling millisecond convention.
• ✅ generate-client.sh:77 — tr -d '[:space:]' disagreed with Python's .strip() on the same file — Fixed in code: bare read -r line relies on default IFS trimming; verified the two now agree.
• ✅ generate-client.sh:94 — editions.txt claimed more authority than it had, and build-packages.sh carried its own copy of the list — Fixed in code: --list-editions, consumed by build-packages.sh; comment now scopes what the file does and doesn't govern.
• ✅ tests/test_common_overlay.py:153 — the agreement test never ran the script — Fixed in code: it now execs bash <script> --list-editions against a fixture built to separate the two parsers.
• ✅ tests/test_common_overlay.py:99 — stale failure message, plus _editions() could error at collection time — Fixed in code: message updated and a missing file yields [], matching the walk helpers.
• ✅ common/client.py:88 — three of the five ValueError messages were bare fragments — Fixed in code: all five state a remedy.
• ✅ common/client.py:52 — the ordering comment was missing its subject — Fixed in code.
• ✅ tests/test_auth.py:247 — assertGreater(timeout, 0) would have accepted 0.001 — Fixed in code: asserts Timeout instance and total == DEFAULT_AUTH_TIMEOUT_MS / 1000, with retries=False and configurability pinned alongside.

---

This re-review was auto-generated. Findings may contain errors — please verify before applying changes.

[irynamatveieva]

Re-review summary

Re-reviewed Send X-Authorization header after JWT login — verified 6 finding(s) from the previous review against commit a0f7869e.

Status	Count
✅ Resolved	6
💬 Acknowledged	0
❌ Unresolved	0

All six are fixed, and I re-ran the measurements rather than taking the reply's table on trust — your numbers reproduce exactly. _AUTH_RETRIES makes 1 connect attempt against a refused port where Retry.DEFAULT makes 4, and a 307 on /api/auth/login now resolves to 200 with the request body preserved end to end. The build-packages.sh capture aborts at the point of failure under set -e, and the [ -n "$line" ] && idiom inside the loop doesn't trip it.

Also found 4 new issue(s) in the fix commit, commented inline. The first one is mine to own — redirect=3 was my suggestion last round, and I didn't think through two of its consequences:

• common/_auth.py:58 — each redirect hop gets a fresh Timeout(total=...) budget rather than sharing one, so the ceiling is back to 4× auth_timeout_ms. Measured: Timeout(total=1.0) with four hops of 0.8s each returned 200 after 3.21s. That is the same multiplier the retry removal existed to eliminate, and the DEFAULT_AUTH_TIMEOUT_MS comment was edited in this commit to drop the caveat and now claims flatly that it "is the real ceiling".
• Same line, second consequence: following redirects re-sends the credential body to whatever Location names, including a different host. I stood up an origin that 307s to a second server and it received {"username": "tenant@thingsboard.org", "password": "s3cret"}, after which the client accepted the second host's token.

The other three are test-strength and comment-sweep items. All nine overlay copies are byte-identical to their common/ sources, --list-editions still returns ce pe paas, and build-packages.sh parses clean.

Finding details

• ✅ common/_auth.py:58 — retries=False disabled redirect following as a side effect, and the single-attempt trade wasn't stated — Fixed in code: _AUTH_RETRIES = Retry(connect=0, read=0, status=0, other=0, redirect=3); verified 1 connect attempt and a 307 followed with the body intact. Reasoning now sits next to the constant.
• ✅ scripts/build-packages.sh:48 — process substitution hid a failing --list-editions; wheel count hardcoded to 3 — Fixed in code: captured into editions_output so set -e sees it, explicit emptiness check, and EDITION_COUNT derived from the parsed list.
• ✅ common/_auth.py:163 — auth_timeout_ms was unvalidated and failed late, inside the swallowed _raw_post — Fixed in code: rejected in _AuthManager.__init__, covering direct construction too.
• ✅ tests/test_common_overlay.py:78 — _editions()'s docstring claimed neither side's formatting was load-bearing, which stopped being true once the test began executing the script — Fixed in code: now names --list-editions as canonical, itself as the mirror, and the agreement test as what holds them together.
• ✅ generate-client.sh:81 — a counter maintained in parallel with the array could drift — Fixed in code: [ -z "${EDITIONS[*]:-}" ].
• ✅ common/client.py:97 — the refresh_token= message dropped the clause its two siblings carry — Fixed in code: all three now identical in shape.

---

This re-review was auto-generated. Findings may contain errors — please verify before applying changes.

[irynamatveieva]

This guard is weaker than the sibling directly above it. test_does_not_retry pins exact values — (0, 0, 0, 0) — while this only asserts truthiness, so it would pass unchanged if the allowance became redirect=1 or redirect=True. Given the hop count turns out to drive the worst-case timeout (see my note on _auth.py:58), the number is load-bearing now and worth pinning: assertEqual(retries.redirect, 3) costs nothing and matches the neighbour's style.

Separately, both of these assert on kwargs captured from a mocked PoolManager, so what's proven is that a setting reaches urllib3 — not that a 307 on /api/auth/login resolves instead of surfacing as RuntimeError("... returned HTTP 307"), which was the reported symptom. You verified that end to end by hand and reported it in the reply; it just isn't committed. A ThreadingHTTPServer on port 0 in a daemon thread answering 307-then-200, with a real _raw_post against it, is about fifteen lines and no more flake-prone than TestConcurrentRefresh — I wrote one to check the timeout behaviour above, so it's cheap. Your call on the weight; the exact-value assertion I'd do regardless.

[irynamatveieva]

Both taken.

The exact-value assertion now covers total as well, since I set it explicitly: (total, connect, read, status, other) == (0, 0, 0, 0, 0), and redirect == 0 in its own test. (urllib3 normalises redirect=False to 0 — assertIs(..., False) fails, which I only found by running it.)

And you're right that verifying end to end in a reply and not committing it is worth nothing to the next person. Added _StubAuthServer — a HTTPServer on port 0 in a daemon thread, closed via a context manager — and two tests using it: a 200 is parsed into the token dict, and a 307 raises with the remedy in the message. That covers the reported symptom rather than the setting that addresses it.

[irynamatveieva]

Re-review summary

Re-reviewed Send X-Authorization header after JWT login — verified 4 finding(s) from the previous review against commit b04fdf05.

Status	Count
✅ Resolved	4
💬 Acknowledged	0
❌ Unresolved	0

All four are fixed, and I re-ran the checks rather than reading them: a 307 now comes back unfollowed and the third-host fixture receives nothing, the 200 path still parses, and there is exactly one connect attempt against a refused port. Your reasoning for choosing redirect=False over redirect=1 plus a same-origin check is sound — eliminating both problems beats bounding them, and "the credentials already went out in cleartext" is the right argument against rewarding an http → https redirect. Nice catch on total sitting at 10, too.

Also found 4 new issue(s) in the fix commit, commented inline. One is worth reading first:

• common/_auth.py:72 — redirect=False and redirect=0 are not interchangeable, and the difference is what makes your new error message reachable. Retry.__init__ has if redirect is False or total is False: redirect = 0; raise_on_redirect = False, so both spellings leave .redirect == 0 but only False clears raise_on_redirect. Measured against 1.26.5: with redirect=False a 307 returns as a response and _raw_post raises the friendly RuntimeError; with redirect=0 the same request raises MaxRetryError("too many redirects") and the remedy never prints. test_does_not_follow_redirects asserts retries.redirect == 0, which passes for both — so the assertion currently reads as an invitation to "simplify" the source into a regression only the stub-server test would catch, and it would fail with a confusing MaxRetryError.

The other three are the url= remedy wording, what the stub test pins, and one arithmetic point about the ceiling comment. All six overlay copies are byte-identical to their common/ sources, and the count sweep is complete in every file this commit touched.

Finding details

• ✅ common/_auth.py:72 — redirect=3 gave each hop a fresh timeout budget (4× ceiling) and forwarded credentials to the redirect target — Fixed in code: Retry(total=0, connect=0, read=0, status=0, other=0, redirect=False). Verified a 307 is not followed, the third host receives nothing, and one connect attempt is made.
• ✅ tests/test_auth.py:274 — assertTrue(retries.redirect) left the hop count unpinned, and no test exercised the symptom end to end — Fixed in code: exact values including total, plus _StubAuthServer driving a real _raw_post for both the 200 and 307 cases.
• ✅ tests/test_client.py:221 — auth_timeout_ms validation wasn't covered through the public constructor — Fixed in code: test_non_positive_auth_timeout_rejected in TestThingsboardClientAuthArgValidation, with mock_login.assert_not_called() pinning the ordering.
• ✅ scripts/build-packages.sh:28 — the "What it does" list still hardcoded a count of 3 — Fixed in code: lines 28/30 and the section 6 banner all derive from the list, the stale generate-client.sh all reference is corrected, and tests/test_readme.py:12 is swept too.

---

This re-review was auto-generated. Findings may contain errors — please verify before applying changes.

[irynamatveieva]

Re-review summary

Re-reviewed Send X-Authorization header after JWT login — verified 4 finding(s) from the previous review against commit 4695ed71.

Status	Count
✅ Resolved	4
💬 Acknowledged	0

All previous findings have been addressed. No new issues — nothing in this commit clears the bar for another round.

Disclosure worth making explicit: the patch under review here was written by this same automated reviewer acting on its own round-15 findings, and committed by the author. Verifying one's own patch is a weak check, so this round leaned on behavioural evidence rather than reading, and the quality pass was run with an explicit instruction to ignore comment and docstring polish and report only what would change an approve decision. It returned nothing.

What was checked at this commit, in the review worktree:

• tests/test_auth.py — 21/21 pass under python3 -m unittest.
• The 3xx hint fires on exactly the right statuses: 301, 303, 307 → hint; 200, 300, 304, 401 → no hint. The previous 300 <= status < 400 gate wrongly advised on 300 and 304; get_redirect_location() is urllib3's own "would I have followed this" predicate, so the advice is now tied to the case it describes.
• All three edition copies of _auth.py are byte-identical to common/_auth.py.
• No stale references to the old "final auth URL" wording anywhere in the tree.

On the one coupling this commit introduces — response.get_redirect_location() — the package declares urllib3 = ">=1.26,<3.0" and only 1.26.5 is installed here, so the 2.x half of that range was reasoned about rather than executed. It is a documented public method with the same REDIRECT_STATUSES in both majors, and binding to urllib3's own predicate is a tighter contract than the hand-rolled status range it replaced, so this is noted for the record rather than raised as a concern.

Finding details

• ✅ common/_auth.py:82 — redirect=False vs redirect=0 differ only in raise_on_redirect, and that difference is what makes the remedy message reachable; the comment didn't say so and the test couldn't tell the spellings apart — Fixed in code: the rationale now states it, and assertIs(retries.raise_on_redirect, False) pins the distinguishing property. Confirmed by mutation: switching the source to redirect=0 fails that assertion.
• ✅ common/_auth.py:344 — the remedy said "pass the final auth URL as url=", but url= is the base URL and _raw_post appends the path, so following it literally would request /api/auth/login/api/auth/login; the hint also fired on 300 and 304 — Fixed in code: now "set url= to the redirect target's base URL instead.", gated on get_redirect_location(). Verified across the status range.
• ✅ tests/test_auth.py:360 — the redirect test asserted only the message, so a change that forwarded the credential body once would still have matched — Fixed in code: _StubAuthServer records every requested path and the test asserts exactly ["/api/auth/login"]. Confirmed by mutation: a policy that genuinely forwards reports ['/api/auth/login', '/elsewhere'], and the message regex still matched — which is the hole this closes.
• ✅ common/_auth.py:49 — the comment tied total to how long other threads block, but that quantity is 2× because _do_refresh_token falls back to _do_login — Fixed in code: per-call ceiling and the 2× worst case are now stated separately, with the fallback named as the reason it is deliberate.

---

This re-review was auto-generated. Findings may contain errors — please verify before applying changes.