refactor: wave 2 — typed errors, structured logging, no mutable globals#49
Merged
refactor: wave 2 — typed errors, structured logging, no mutable globals#49
Conversation
Per docs/clean-code.md §4-5: typed exception hierarchy, structured logging, no mutable module globals, de-duplicated JSON parsing. New modules ----------- - skillforge/errors.py: SkldError base + SpawnError, BreedError, JudgeError, ParseError, ValidationError, AgentSDKError, DBError, etc. ParseError/ValidationError inherit from ValueError so legacy callers catching ValueError keep working. - skillforge/agents/_json.py: extract_json_array() consolidated from identical copies in spawner.py + challenge_designer.py. Raises ParseError (which is-a ValueError) instead of raw ValueError. Both agents now import it. - skillforge/engine/run_registry.py: RunRegistry encapsulates the two mutable module globals (PENDING_PARENTS in engine/evolution.py:46 and _active_runs in api/routes.py:36) behind explicit accessors. Module-level singleton 'registry' threaded through engine/evolution.py, api/routes.py, api/debug.py, and main.py. Exception hygiene ----------------- Ruff now enforces BLE + TRY on the whole codebase. All 21 remaining bare 'except Exception' catches surfaced by BLE001 are now justified at genuine boundaries (boot-time lifespan, LLM SDK calls, background tasks, WS handlers, event persistence) with a noqa tag carrying a one-line rationale. Narrow catches replaced with specific types where possible (OSError, json.JSONDecodeError, YAMLError, etc.). Logging ------- All 'print(...)' diagnostics in library code replaced with logger.exception / logger.warning / logger.debug calls: - breeder.py: 6 prints -> structured logger.exception - competitor_sdk.py: stderr print -> logger.exception - events.py: 'except Exception: pass' -> logger.debug + noqa TRY400/TRY401 fixes applied uniformly: logger.error inside except -> logger.exception; dropped redundant exc args from logger.exception format strings (the logger already captures the traceback). Tests ----- tests/test_seeds.py and tests/test_uploads.py updated to access pending-parent state through the new RunRegistry instead of the old PENDING_PARENTS module global. QA -- ruff check skillforge - clean (BLE + TRY now enabled) mypy skillforge - 54 files pass pytest tests/ - 403 passed, 2 skipped, 0 failed frontend build + vitest - untouched, still passing Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Wave 2 of the clean-code overhaul — cross-cutting hygiene across the backend. Enforces
docs/clean-code.md§4 (error handling) and §5 (data / no mutable globals).New modules
skillforge/errors.py—SkldErrorbase + domain subtypes (SpawnError,BreedError,JudgeError,ParseError,ValidationError,AgentSDKError,DBError, etc.).ParseErrorandValidationErroralso inherit fromValueErrorso legacy call sites catchingValueErrorkeep working.skillforge/agents/_json.py—extract_json_array()consolidated from two identical copies inspawner.pyandchallenge_designer.py. RaisesParseError(is-aValueError).skillforge/engine/run_registry.py—RunRegistryencapsulates the two mutable module globals (PENDING_PARENTSand_active_runs) behind explicit accessors. Singletonregistryinstance threaded throughengine/evolution.py,api/routes.py,api/debug.py, andmain.py.Exception + logging hygiene
Ruff now enforces BLE + TRY on the whole codebase (deferred from Wave 1). Outcome:
except Exceptionnow carries a one-line# noqa: BLE001 —rationale explaining why the catch is a legitimate boundary (boot-time lifespan, LLM SDK calls, background tasks, WebSocket handlers, event persistence).OSError,json.JSONDecodeError,yaml.YAMLError,UnicodeDecodeError,zipfile.BadZipFile, etc.print(...)diagnostics in library code replaced withlogger.exception/logger.warning/logger.debug(breeder + competitor_sdk + events).TRY400fixes:logger.errorinsideexcept→logger.exception.TRY401fixes: dropped redundantexcargs fromlogger.exceptionformat strings (logger already captures the traceback).TRY004left ignored with rationale — ourisinstancechecks validate LLM-parsed payloads, soValueErroris the right semantic ("malformed value"), notTypeError("programmer passed wrong type").Tests
tests/test_seeds.pyandtests/test_uploads.pynow access pending-parent state through the newRunRegistryinstead of the oldPENDING_PARENTSmodule global.Test plan
uv run ruff check skillforge— clean (BLE + TRY now enabled)uv run mypy skillforge— 54 files passuv run pytest tests/— 403 passed, 2 skipped, 0 failedcd frontend && npm run build / lint / format:check / test— all green (frontend untouched)What's next
Wave 3: backend hotspot decomposition (queries.py at 1362 LOC → package split; routes.py at 707 LOC → by-resource split; spawner/breeder/variant_evolution decomposed into pure planners + thin I/O shells).
🤖 Generated with Claude Code