Skip to content

Phase 4: Architecture Cleanup - #10

Merged
padak merged 1 commit into
mainfrom
phase-4-architecture-cleanup
Feb 28, 2026
Merged

Phase 4: Architecture Cleanup#10
padak merged 1 commit into
mainfrom
phase-4-architecture-cleanup

Conversation

@padak

@padak padak commented Feb 28, 2026

Copy link
Copy Markdown
Member

Implementation Summary

  • Extract DoctorService from doctor command (17 lines command vs original 285)
  • Fix redundant exception handling in org_service.py
  • Add asyncio.gather to MCP service with shared _gather_results helper
  • Refactor LineageService to use BaseService._run_parallel()

Acceptance Criteria

  • commands/doctor.py is thin (<50 lines) -- 17 lines
  • DoctorService testable independently -- 16 new unit tests
  • No except (KeboolaApiError, Exception) remains -- split into separate blocks
  • MCP service uses asyncio.gather for concurrent task collection
  • LineageService uses BaseService._run_parallel() -- no custom ThreadPoolExecutor
  • Shared gather helper (_gather_results) for MCP results
  • kbagent doctor works end-to-end

Tests

  • All 488 existing tests pass (3 skipped)
  • New: test_doctor_service.py with 16 tests covering:
    • config file existence/permissions checks
    • config validation checks
    • API connectivity checks (success, failure, mixed)
    • version check
    • full run_checks orchestration

Files Changed

  • NEW: src/keboola_agent_cli/services/doctor_service.py
  • NEW: tests/test_doctor_service.py
  • Modified: commands/doctor.py (285 -> 17 lines)
  • Modified: output.py (added format_doctor_panel)
  • Modified: cli.py (wire DoctorService)
  • Modified: services/org_service.py (split exception blocks)
  • Modified: services/mcp_service.py (asyncio.gather + shared helper)
  • Modified: services/lineage_service.py (use _run_parallel)
  • Modified: tests/test_cli.py (update patch paths)

@padak
Phase 4: Architecture cleanup - extract DoctorService, fix gather, re…
a7d2677 
…factor lineage

- Extract service-layer logic from commands/doctor.py into DoctorService (17 lines vs 285)
- Move doctor formatting to output.py (format_doctor_panel) for consistency
- Split redundant except (KeboolaApiError, Exception) into separate blocks in org_service
- Replace sequential task collection in MCP service with asyncio.gather + shared _gather_results helper
- Refactor LineageService to use BaseService._run_parallel() instead of custom ThreadPoolExecutor
- Wire DoctorService into cli.py context
- Add comprehensive DoctorService unit tests (test_doctor_service.py)
@padak
padak merged commit 7318852 into main Feb 28, 2026
@padak
padak deleted the phase-4-architecture-cleanup branch February 28, 2026 00:32
@padak padak mentioned this pull request Aug 10, 2026
Merged
martinsifra added a commit that referenced this pull request Aug 27, 2026
@martinsifra @claude
fix(service): apply the Opus wire-truth review -- 409 codes, allowed_…
a0160a4 
…actions, derive_state honesty [DMD-1899]

Adversarial review of every wire assumption against Connection source
(tasks/pr-703-opus-wire-review.md): 7 CONFIRMED, 3 MISMATCH. The fixes:

409 mapping (finding #4): the conflict 409 is NOT code-less -- an earlier
reading missed ExceptionConverter, which serializes
MergeValidationException's own string code storage.mergeRequests.validation
top-level as `code`, plus the conflicting configurations in
`params.errors`. _remap_merge_conflict now matches BOTH codes explicitly
(code-less 409 falls back to conflict for older stacks; a 409 with any
OTHER code passes through unmapped instead of being confidently mislabeled
a conflict), and http_base surfaces `params` as
details.api_error_params so the conflict list travels with the error
instead of demanding a second round trip. Stale 'carries no code' claims
corrected in errors.py, the L3 docstring, error-codes.md, the notes
wire-truth table and the RFC.

allowed_actions (finding #7): `approve` removed from the `approved`
tuple -- the transition's sole `from` place is in_review; from approved
the backend answers 422 (the UI button offering it there is wrong).
`update` added to in_merge (the server blocks update only in terminal
states). Docstring records the AddApprovalGuard gating and that with the
non-SOX default of 0 required approvals, approve is 422 in every state and
in_review itself is unreachable.

derive_state honesty (finding #10, the significant one): reviewers[].status
is populated only within a review round anchored by a review_requested
activity event -- which skip_review never writes -- and explicit reviewers
shadow every non-reviewer's decision (the creator can never BE a reviewer).
So in a default non-SOX project the rejected / self-closed derivations
never fire; the UI badge has the identical blind spot, since our table is
its port. Documented in the docstring and the RFC rather than re-derived
from the activity log client-side: the reliable fix is server-side
(commented on DMD-1988 -- derive from the activity log, not reviewers[]).

Sharp edges from confirmed items: a take side with a null/empty `name`
(nullable in the diff envelope, required non-empty by the rebase validator)
is refused as a contract violation instead of sailing into a server 400;
_branch_from_id_of documents that its null check is racy (branchFromId is
nulled by the FK when the async branch delete lands, not by the state
change); the viewer docstring notes detail/conflicts require an admin token
anyway (MergeRequestVoter).

Regression tests for each fix; 87 service tests total.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

1 participant

@padak