Skip to content

fix(quality): resolve Sonar S8413 + S125 in gui mount and defaults#1807

Merged
chernistry merged 1 commit into
mainfrom
fix/sonar-phase2-major-sweep
May 21, 2026
Merged

fix(quality): resolve Sonar S8413 + S125 in gui mount and defaults#1807
chernistry merged 1 commit into
mainfrom
fix/sonar-phase2-major-sweep

Conversation

@chernistry

@chernistry chernistry commented May 21, 2026

Copy link
Copy Markdown
Collaborator

Summary

Phase 2 Sonar sweep, cluster 1: resolves 16 findings across 3 files.

File Rule Count Fix
src/bernstein/gui/init.py:86 python:S8413 1 (BLOCKER) Build two independent APIRouter instances via a local factory, replacing the shared-instance-via-aggregator pattern
src/bernstein/core/defaults.py python:S125 14 Drop redundant 'arbitrary; tune in tuning:' end-of-line comments (the module docstring already documents tuning)
src/bernstein/core/routing/router_core.py:60 python:S125 1 Rephrase the inline 'float

Root-cause notes

  • S8413 fires when the same APIRouter object is included into a parent more than once, directly or via a nested aggregator that is itself mounted. PR fix(routes): Annotated query params + dedupe gui router include (S8410/S8413) #1788 nested the shared router into a fresh aggregator but the underlying object reuse remained. This PR replaces the shared router with a factory that returns a fresh APIRouter for each mount.
  • S125 misfires on the inline comments because the colon in 'tuning:orchestrator' / 'float | None' parses as commented-out Python syntax. The information is preserved (module docstring, prose comment) without that pattern.

Test plan

  • uv run pytest tests/integration/test_gui_pwa_integration.py -k gui_meta (gui-meta + versioned alias parity)
  • uv run pytest tests/unit/test_defaults.py (frozen dataclass defaults)
  • uv run pytest tests/unit/test_budget_aware_routing.py (router_core budget context)
  • uv run ruff check + format on changed files

Summary by CodeRabbit

  • Documentation

    • Clarified type documentation for internal state tracking.
  • Refactor

    • Improved internal route initialization structure for consistency.

Review Change Stack

- gui/__init__.py:86 (S8413): build two independent APIRouter instances
  via a local factory instead of reusing one router for both mounts.
  S8413 fires when the same APIRouter object is included into a parent
  more than once (directly or via an aggregator); fresh per-mount
  routers break that detection while keeping /gui-meta and
  /api/v1/gui-meta in parity.
- core/defaults.py:49-312 (S125 x14): drop the redundant
  'arbitrary; tune in tuning:<area>' inline comments. The module
  docstring already directs callers to bernstein.yaml tuning:* for
  overrides, and Sonar parses the colon-bearing comment as
  commented-out code.
- core/routing/router_core.py:60 (S125 x1): rephrase the
  'float | None - ...' inline annotation as a prose comment so it
  no longer scans as commented-out type syntax.

@sourcery-ai sourcery-ai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry @chernistry, you have reached your weekly rate limit of 2500000 diff characters.

Please try again later or upgrade to continue using Sourcery

@chernistry chernistry enabled auto-merge (squash) May 21, 2026 20:09
@coderabbitai

coderabbitai Bot commented May 21, 2026

Copy link
Copy Markdown

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: efcd0d41-72fd-4f52-a049-6eff68d43ee4

📥 Commits

Reviewing files that changed from the base of the PR and between 591b00d and 8beb0bb.

📒 Files selected for processing (3)
  • src/bernstein/core/defaults.py
  • src/bernstein/core/routing/router_core.py
  • src/bernstein/gui/__init__.py

📝 Walkthrough

Walkthrough

Removes inline tuning documentation comments from default configuration values across multiple dataclasses, clarifies routing budget documentation semantics, and refactors GUI meta route mounting to use independent APIRouter instances for dual mount points.

Changes

Configuration, routing, and GUI updates

Layer / File(s) Summary
Defaults configuration tuning comments cleanup
src/bernstein/core/defaults.py
Removes inline "tune in tuning:
" comments from default field assignments across OrchestratorDefaults (lines 49–58), AgentDefaults (100–102), CostDefaults.scope_budget_usd (198–200), CostDefaults.effort_base_turns (216–220), ProtocolDefaults.cluster_max_nodes (293), and PlanDefaults.tokens_by_scope (310–312). Default numeric/string values and dataclass structure unchanged.
Routing budget state documentation
src/bernstein/core/routing/router_core.py
Comment for _budget_context_state["budget_remaining_usd"] (lines 60–62) updated to explicitly state that None means unknown or budget-aware routing disabled, matching the Optional[float] type semantics. No logic changes.
GUI meta route independent router factory
src/bernstein/gui/__init__.py
Introduces _build_gui_meta_router() factory (lines 70–86) that creates a new APIRouter instance each call. The /gui-meta endpoint mounts twice—once at root and once under /api/v1 prefix—with independent router instances instead of prior single-router + nested aggregator wiring. Comments (lines 55–61) updated to reflect audit-focused routing constraints.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Possibly related PRs

  • sipyourdrink-ltd/bernstein#1788: Both PRs modify src/bernstein/gui/__init__.py to refactor how the /gui-meta route is mounted for / and /api/v1 endpoints by avoiding reuse of a single APIRouter instance.

Suggested labels

size/m, core

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 33.33% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title accurately summarizes the main change: resolving Sonar issues S8413 and S125 across the three modified files.
Description check ✅ Passed The description includes all required sections (What, Why, How) with concrete details: Sonar rules addressed, file-by-file breakdown with line numbers, root-cause analysis, and completed test plan covering all three modified files.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix/sonar-phase2-major-sweep

Comment @coderabbitai help to get the list of available commands and usage tips.

@github-actions

Copy link
Copy Markdown
Contributor

Sonar insights (advisory, no merge-block)

Snapshot of bernstein on the configured Sonar instance:

Metric Value
Coverage 13.5
Code smells 125
Bugs 11
Vulnerabilities 2
Security hotspots 87

Run bernstein doctor sonar locally for the full surface.

This comment is a soft signal. The Sonar scan runs on push to main; the PR check itself never fails on smells.

@github-actions

Copy link
Copy Markdown
Contributor

Review-bot acknowledgement summary

  • Must-address findings: 0 (0 acknowledged, 0 open)
  • Informational findings: 0

All must-address findings are resolved or acknowledged.

@github-actions

Copy link
Copy Markdown
Contributor

bernstein doctor observe for PR #1807 (fix/sonar-phase2-major-sweep): ok=0, warn=2, fail=0, error=0, skipped=2

sonar -- WARN (project bernstein)

metric value delta threshold status
coverage_pct 13.5% new 80.0% fail
code_smells 125 new 50 warn
bugs 11 new 0 fail
vulnerabilities 2 new 0 warn
security_hotspots 87 new 0 fail

code-scanning -- WARN (14 open alert(s))

metric value delta threshold status
open_alerts 14 new 0 fail
critical_alerts 0 new 0 ok
high_alerts 2 new 0 warn
medium_alerts 0 new - ok
low_alerts 0 new - ok
Skipped backends (credentials not configured)
  • glitchtip: BERNSTEIN_GLITCHTIP_TOKEN not set
  • dt: DTRACK_URL/TOKEN/PROJECT not set

See docs/observability/unified-doctor.md for backend setup notes.

@chernistry chernistry merged commit 2daf12b into main May 21, 2026
69 of 71 checks passed
@chernistry chernistry deleted the fix/sonar-phase2-major-sweep branch May 21, 2026 20:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant