Extract duplicated literals, add dedicated SSH exceptions (group 8, part 1) by karoltheguy · Pull Request #310 · karoltheguy/QuadletManager

karoltheguy · 2026-08-03T21:18:01Z

Fifth PR in the SonarCloud cleanup series (group 8: Python maintainability), after #303, #304, #308 and #309. Clears 13 of 19 findings. The 6 python:S3776 cognitive-complexity refactors are held back for a second PR, since a control-flow change deserves isolated review and a revert that does not drag constant extraction with it.

Rule	Count	Status
`python:S1192` duplicated literals	7	Fixed
`python:S108` empty block	2	Fixed, exposed two bad test mocks
`python:S1066` nested if	2	Fixed
`python:S112` generic exception	2	Fixed, unblocks a finding left open in #309
`python:S3776` cognitive complexity	6	Deferred to part 2

S1192, and a count discrepancy worth knowing

Sonar reports 3 occurrences each of "/login" and "/change-password". The file actually contains 5 of each, since Sonar's duplicate-literal count excludes route decorators. All occurrences were replaced, not just the flagged subset.

Verified by AST rather than by eye: each of the seven literals now appears exactly once, as its constant definition, and no other string literal in the file changed count. That check is what makes a bulk find-and-replace across a 1700-line routing module trustworthy.

S108 found a test that was propping up the outlier

The two async with db.execute(...): pass blocks become await db.execute(...), which is what every other write site in the codebase does. These two were the only exceptions.

That broke two tests, and the tests were wrong. Their hand-rolled SettingsMockDB.execute returned a cursor supporting only async with, while a real aiosqlite execute() result supports both protocols. The repo already knows this: tests/test_sync_engine.py:90 defines DualProtocolCM whose docstring says "Objects returned by aiosqlite execute() support both async with obj and await obj." Both mocks now use that same pattern, so the fake stops dictating which idiom production code may use.

Worth being explicit, since "the change broke tests so I changed the tests" deserves scrutiny: the mock was asserting an implementation detail, not behavior. The tests still prove the same thing, that settings persist and apply live.

S112, and why these do not subclass SSHCommandError

The two bare raise Exception in ssh_manager.py become ServerConfigurationError and KeyDecryptionError, both subclassing Exception directly.

Not SSHCommandError, because that type carries exit_status and stderr for a command that actually ran, and tests/test_ssh_manager.py:137-138 asserts on exactly those fields. Both new errors fire while assembling the connection, before anything reaches the host, so typing them as command failures would hand callers a null exit status and imply a command failed when none was issued.

Caller audit: all ~41 handlers in the codebase use except Exception, and there are no exact-type comparisons such as type(e) is Exception, so nothing downstream changes. Three new tests cover both types plus __cause__ chaining, and they fail against the previous code at import time.

Follow-up this enables: #309 left one python:S5958 open because ssh_manager.py:118 raised bare Exception, with a comment saying so. Once both merge, that comment is stale and the test can narrow to KeyDecryptionError. It could not be done in either PR alone without a conflict, since tests/test_crypto.py is modified in #309.

S1066

Two nested ifs merged. and short-circuits, so _drop_if_stale()'s side effect still fires only when the connection is cached, exactly as before, and the conditions are not reordered.

One deliberate extra, not flagged by Sonar: the structurally identical re-check inside get_connection's lock was merged too. Fixing only the flagged one left two adjacent, semantically identical checks written differently, which is worse than either form. Easy to drop if you would rather keep the diff strictly to the findings.

Verification

-m unit: 939 to 942, the three new exception tests, nothing lost
unmarked suite: 47 passed, unchanged
re-run in random order to catch cross-test contamination: 942 passed
AST verification of literal extraction as described above

Deliberately skipped, with reasons

Three S3776 findings will not be fixed even in part 2:

sync_engine.py:166 check_quadlets (complexity 25) and sync_engine.py:114 _fetch_mtimes. These are the subject of open bug External modification detection never runs: check_quadlets() polls an always-empty table #283: the function iterates the quadlets table, and the only INSERT INTO quadlets anywhere in the repo is in test files, so it is dead in production. Its uncovered branches are uncoverable by construction while the work set is always empty, meaning no test can protect a refactor of the most complex function in the group. Whoever fixes External modification detection never runs: check_quadlets() polls an always-empty table #283 rewrites it anyway.
scripts/import_servers.py:12, a one-off script not referenced by Dockerfile or start.sh, so not shipped, and not covered. Restructuring untested, unshipped code to satisfy a metric is risk without benefit.

…art 1) Clears 13 of the 19 open python maintainability findings: 7 python:S1192, 2 python:S108, 2 python:S1066, 2 python:S112. The 6 python:S3776 cognitive complexity refactors are deliberately left for a separate PR so a control-flow change can be reviewed and reverted on its own. S1192: seven duplicated literals in api/routes.py become module-level constants, following the placement and naming of the existing SESSION_DURATION_SETTING_KEY and LOG_LEVEL_CHOICES. Sonar counted 3 occurrences each of "/login" and "/change-password"; the file actually contains 5 of each, including the route decorators, and all were replaced. Verified by AST that each literal now appears exactly once, as its constant definition, and that no other string literal in the file changed. S108: the two `async with db.execute(...): pass` blocks become `await db.execute(...)`, the form used at every other write site in the codebase. These two were the only outliers. That change broke two tests, and the tests were at fault. Their hand-rolled SettingsMockDB.execute returned a cursor supporting only `async with`, while real aiosqlite execute() results support both protocols. The repo already knows this: tests/test_sync_engine.py defines DualProtocolCM with a docstring saying exactly that. Both mocks now use the same pattern, so the fake no longer dictates which idiom the route uses. S112: the two bare `raise Exception` in ssh_manager become ServerConfigurationError and KeyDecryptionError, with tests covering both plus __cause__ chaining. Both subclass Exception directly rather than SSHCommandError, because SSHCommandError carries a remote exit status and stderr for a command that actually ran, and these fail while assembling the connection, before anything reaches the host. All existing handlers catch Exception, so nothing downstream changes. S1066: two nested ifs merged. `and` short-circuits, so _drop_if_stale's side effect still fires only when the connection is cached, exactly as before. The structurally identical re-check inside get_connection's lock was merged too, though Sonar did not flag it, since leaving two adjacent identical checks written differently is worse than either form. Unit suite 939 to 942, the three new exception tests. Also re-run in random order to check for contamination.

codacy-production · 2026-08-03T21:18:44Z

Up to standards ✅

🟢 Issues 0 issues

Results:
0 new issues

View in Codacy

🟢 Metrics 0 complexity · 0 duplication

Metric Results

Complexity 0

Duplication 0

View in Codacy

_{NEW Get contextual insights on your PRs based on Codacy's metrics, along with PR and Jira context, without leaving GitHub. Enable AI reviewer}
_{TIP This summary will be updated as you push new changes.}

Fixes the quality gate on this PR. Adding DualProtocolCM to both test_log_level.py and test_session_duration.py pushed new-code duplication to 14% against a 3% threshold. The duplication was mostly pre-existing: the two files carried byte-identical copies of the login mock, the login helper and SettingsMockDB, 75 duplicated lines each. Touching those lines made them count as new code, which is the same inherit-the-debt effect seen on PR #304. Extracting the shared scaffolding fixes the cause rather than the symptom. The two files drop from 147 and 127 lines to 79 and 56, and shared blocks fall from 75 lines each to 22, which is now just imports and fixtures. Follows the existing tests/sudo_permissions.py precedent for a non-test helper module under tests/. test_sync_engine.py has its own DualProtocolCM copy, deliberately left alone here since it is modified on PR #309 and touching it would conflict. Unit 942 and unmarked 47, both unchanged.

karoltheguy · 2026-08-03T21:29:03Z

Pushed a fix for the failing quality gate, and a note on the three findings that remain.

Gate failure was mine

new_duplicated_lines_density hit 14% against a 3% threshold. Adding DualProtocolCM to both test_log_level.py and test_session_duration.py was the trigger, but the underlying duplication was mostly pre-existing: the two files carried byte-identical copies of the login mock, the login helper and SettingsMockDB, at 75 duplicated lines each. Touching those lines reclassified them as new code, the same inherit-the-debt effect that made #304's gate red.

Rather than just de-duplicating my own 13 lines to squeak under the threshold, the shared scaffolding now lives in tests/settings_mocks.py, following the existing tests/sudo_permissions.py precedent for a non-test helper module. Result:

File	Before	After
`test_session_duration.py`	147 lines	79
`test_log_level.py`	127 lines	56
shared blocks	75 lines each	22, only imports and fixtures

Suites unchanged: 942 unit, 47 unmarked.

test_sync_engine.py has its own DualProtocolCM copy, deliberately untouched here because it is modified on #309 and editing it would conflict.

The three `python:S8415` findings are staying

Sonar reports three new S8415 on api/routes.py lines 261, 265 and 272, asking that the HTTPException(status_code=303) redirects be documented in each route's responses parameter. They are new only because this PR replaced the literals on those lines with LOGIN_PATH and CHANGE_PASSWORD_PATH; the raises themselves are untouched pre-existing code.

They do not fail the gate, and I would rather not silence them, because open issue #300 argues the whole approach is wrong: raising HTTPException(303) means every auth redirect carries a JSON error body. Documenting a 303 in the responses of every route that depends on this auth dependency would entrench a pattern that #300 wants replaced with RedirectResponse. Better to leave them visible and let #300 resolve them properly.

Document HTTP 303 redirect response for change-password endpoints ### With PR reference: Document HTTP 303 redirect response for change-password endpoints (#310) ### Conventional Commits format: docs(fastapi): document 303 redirect response on change-password endpoints

…password routes

sonarqubecloud · 2026-08-03T23:09:43Z

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

Sixth PR in the SonarCloud cleanup series, completing group 8 alongside #310. Clears 3 of the 6 `python:S3776` findings. The other 3 are deliberately left open with reasons below. This is the first PR in the series that changes live control flow rather than adding attributes, constants or types, so the emphasis throughout is on proving nothing moved. ## api/sockets.py, two WebSocket handlers, complexity 18 each **`_reject_invalid_name()`**: both handlers opened with an identical validate-and-reject block differing only in regex, log wording and error text. One shared helper **deduplicates** it rather than relocating it twice. The log text is preserved exactly, `!r` formatting included, by passing the field label through. **`_build_journalctl_command()`**: lifts the `shlex` quoting and the since/scope branching out. Since this builds a command string that runs on a remote host, it was checked exhaustively rather than by eye. **96 combinations** of unit name, scope and `since`, covering unknown `since` values, unrecognised scopes, and unit names requiring shell quoting, compared against HEAD's logic. **0 mismatches.** **`_cleanup_terminal()`**: takes the `finally` block's nested cancel/kill/wait. Every `except Exception: pass` is preserved deliberately, because cleanup must not mask the original error. Order is unchanged, and `manager.disconnect()` plus its log stay in the caller's `finally`. ## services/stats_engine.py `fetch_server_stats`, complexity 18 Depth was the problem: `for server` into `try` into `if scope_filter` into `for scope_label` into `for c` into `if c["name"] not in seen`. Split into `_scope_stat_tasks()`, `_merge_scope_results()` and `_build_unit_rows()`, with code moved verbatim. **The trap here is precedence.** `if c["name"] not in seen` means the *first* scope reporting a container name wins, and user scope is appended first, so a user container shadows a global one with the same name. Rewriting that merge as a dict comprehension or `dict.update` would make the *last* writer win and silently invert it, with no test necessarily catching it. Verified directly: ``` duplicate 'web' resolved to cpu = 1 -> USER wins (correct) both scopes kept independently: ['global', 'user'] scope_filter='both' -> ['user', 'global'] 'user' -> ['user'] 'global' -> ['global'] ``` The ordering check matters too, since the labels are `zip`ped against `asyncio.gather` results, so a reordering would mislabel every scope. ## Verification * `-m unit`: 939 passed, 3 skipped, unchanged * unmarked: 47 passed, unchanged * socket, terminal and stats focused: 134 passed, 27 skipped * `pylint -e W0613`: no unused arguments on the new helpers * the two differential checks described above Coverage is what made these three safe to touch. `api/sockets.py` sits at 99% and `services/stats_engine.py` at 98%, so the existing suite genuinely exercises the paths being restructured. ## The three left open, and why **`sync_engine.py` `check_quadlets` (complexity 25, the largest in the group) and `_fetch_mtimes`.** Open bug #283 reports that `check_quadlets` iterates the `quadlets` table and that table is never populated. Confirmed: the only `INSERT INTO quadlets` anywhere in the repo is in test files. The function is dead in production. That is not just an argument about wasted effort. Its uncovered branches are **uncoverable by construction** while the work set is always empty, so no test can protect a refactor of the most complex function in the group. Whoever fixes #283 restructures it anyway. Refactoring it now would be unprotected churn on code that is about to be rewritten. **`scripts/import_servers.py`.** Not referenced by `Dockerfile` or `start.sh`, so not shipped, and not covered. Restructuring untested, unshipped code to satisfy a metric is risk without benefit.

karoltheguy mentioned this pull request Aug 3, 2026

Reduce cognitive complexity in three handlers (group 8, part 2) #311

Merged

Merge branch 'main' into fix/python-maintainability

7c9c031

fix(auth): enforce session dependency in change password routes

f65b5d9

karoltheguy force-pushed the fix/python-maintainability branch from 02eb3d5 to f65b5d9 Compare August 3, 2026 22:16

karoltheguy added 2 commits August 3, 2026 19:05

fix(routes): use Annotated for session and form parameters in change …

42b6a8d

…password routes

fix(routes): handle case when session_secret is not found in database

79b6b58

karoltheguy merged commit 96bc236 into main Aug 3, 2026
16 checks passed

karoltheguy deleted the fix/python-maintainability branch August 3, 2026 23:27

Navigation Menu

Extract duplicated literals, add dedicated SSH exceptions (group 8, part 1) - #310

Extract duplicated literals, add dedicated SSH exceptions (group 8, part 1)#310
karoltheguy merged 6 commits into
mainfrom
fix/python-maintainability

karoltheguy commented Aug 3, 2026

Uh oh!

codacy-production Bot commented Aug 3, 2026 •

edited

Loading

Uh oh!

karoltheguy commented Aug 3, 2026

Uh oh!

sonarqubecloud Bot commented Aug 3, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Conversation

karoltheguy commented Aug 3, 2026

S1192, and a count discrepancy worth knowing

S108 found a test that was propping up the outlier

S112, and why these do not subclass SSHCommandError

S1066

Verification

Deliberately skipped, with reasons

Uh oh!

codacy-production Bot commented Aug 3, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Up to standards ✅

Uh oh!

karoltheguy commented Aug 3, 2026

Gate failure was mine

The three python:S8415 findings are staying

Uh oh!

sonarqubecloud Bot commented Aug 3, 2026

Quality Gate passed

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

codacy-production Bot commented Aug 3, 2026 •

edited

Loading

The three `python:S8415` findings are staying