Skip to content

fix(doctor): validate successful API responses - #123

Merged
HafidIdrissi merged 2 commits into
HafidIdrissi:mainfrom
Y2XNishan:fix-doctor-response-validation
Oct 16, 2026
Merged

HafidIdrissi merged 2 commits into
HafidIdrissi:mainfrom
Y2XNishan:fix-doctor-response-validation

Conversation

@Y2XNishan

Copy link
Copy Markdown
Contributor

Closes #75

Validate successful /user and traffic responses before using their data. Malformed responses produce concise diagnostics without echoing payloads. An invalid /user response stops further probes; empty views remains valid.

Validation

  • python -m pytest tests/test_doctor.py — 51 passed, no skips.
  • python -m pytest — 114 passed, no skips, using a fresh pytest temp directory after the shared Windows temp directory denied access during fixture setup.
  • git diff --check — passed.
  • Before the fix, the new regression selection produced 21 failures and 1 pass.

Environment: Windows 11, Python 3.13.5.

AI assistance: OpenAI Codex (GPT-6).

Checklist

  • python -m pytest passes
  • Behaviour changes come with a test in the same commit
  • No new runtime dependencies
  • No invented data — nothing estimated or interpolated beyond what the API returns

Reject malformed successful /user and traffic responses without echoing their payloads. Stop probing after an invalid /user response and keep valid empty views accepted.

AI assistance: OpenAI Codex (GPT-6).

@HafidIdrissi HafidIdrissi left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Reviewed the complete doctor/test diff at a26ed25 against #75 and its confirmed schema/diagnostic scope. Invalid successful JSON/root/login responses fail before subsequent probes; traffic requires an explicit list, accepts an empty list and validates each entry's timestamp/count/uniques types, rejecting booleans and negative counts. Diagnostics do not echo malformed payloads, and existing permission/header behavior remains separate from #25. The revised positive fixture and regressions cover valid/invalid responses, probe ordering and synthetic body/token non-disclosure. No blocking findings remain; standard-library-only runtime compatibility is preserved.

I inspected all four logs of current-head Tests 38428493176: 114 tests pass in each Ubuntu/Windows Python 3.9/3.12 job. The author reports 51 focused/114 full local Windows 11/Python 3.13.5 tests and before-fix regression failures, with the temporary-directory setup issue described separately. I have not run tests locally. Rechecked exact head, four successful checks and absence of auto-merge. Approved on this revision; no merge performed.

…-validation

# Conflicts:
#	tests/test_doctor.py

@HafidIdrissi HafidIdrissi left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Approved on revised head e8ecb4d9acada75cb6fe79c1374b8737ae6480e2. I reread CONTRIBUTING.md, the full current doctor/test diff and the surrounding implementation after the update onto main cc2ff86be32f4b28217e130abeb2dced590c3223. The malformed-response handling, explicit empty-views control, probe ordering and synthetic payload/token non-disclosure assertions remain intact. The header-contract and response-closure tests merged in #122 are retained; no blocking finding remains.

I inspected all four logs of current run 38689414726: 118 tests pass per Windows/Ubuntu Python 3.9/3.12 job, including the installed entry-point checks. These CI results supersede the earlier head's 114-test matrix. The description's Windows 11/Python 3.13.5 focused/full and before-fix results remain author-reported earlier validation; I have not rerun tests locally. Rechecked exact head, all four successful checks and absence of auto-merge immediately before this approval.

The previous unknown merge state is resolved: GitHub now reports the revised PR clean. No further execution approval is pending for this run. This review does not merge the PR or enable auto-merge.

@HafidIdrissi
HafidIdrissi merged commit 76eca13 into HafidIdrissi:main Oct 16, 2026
4 checks passed

Copy link
Copy Markdown
Owner

Merged after a final complete diff review and independent local validation of exact head e8ecb4d: on Windows build 26200 / Python 3.11.9, created an isolated venv, installed pytest and the project with python -m pip install --no-deps -e ., then ran .\.venv\Scripts\python.exe -m pytest -q: 118 passed. All four existing current-head CI checks were successful immediately before merging. #75 is now closed as completed. This supplements the earlier author and CI evidence.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Doctor: handle malformed successful JSON without a traceback

2 participants