From 9f6466335cf213ed2cf66b46e6b8bf41f8ec5787 Mon Sep 17 00:00:00 2001 From: "tao.chen" <93983997+taochen-ct@users.noreply.github.com> Date: Fri, 21 Aug 2026 15:38:31 +0800 Subject: [PATCH] docs(schedule): record the deliberate ExecutionResult dataclass upgrade Review (2026-08-21) found that stage 1 promoted ExecutionResult from a plain class to @dataclass(frozen=True) along the way. No caller mutates or compares these objects by identity, so the only externally visible change is structured log output. User opted to keep the upgrade. - domain/execution.py module docstring: explicit note that the frozen + value-equality form is a deliberate enhancement, not a behavioral accident - CLAUDE.md "Schedule service layering" lesson: add a "don't silently upgrade dataclass-ness during a structural-only refactor" note so future refactors copy class definitions verbatim unless they intend to tighten semantics explicitly No code change; tests still 29 green. Co-Authored-By: Claude --- CLAUDE.md | 1 + schedule/src/schedule/domain/execution.py | 6 ++++++ 2 files changed, 7 insertions(+) diff --git a/CLAUDE.md b/CLAUDE.md index e148072..06332b6 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -84,6 +84,7 @@ Lessons from the zero-behavior-change refactor that split the flat 11-file `sche - **Mock `patch()` string targets and in-function lazy imports are invisible to import-line greps.** `test_janitor.py` patched `"schedule.orchestrator.session_scope"` and `test_worker.py` patched `"schedule.service.build_object_store"`; `worker.py` also had a `from schedule.service import ...` *inside a function body*. When files move, these silently no-op against the old path — and hard-crash once the old file is deleted. After any move, grep the whole tree for the old module path (including tests) and rewrite every hit, not just top-level imports. - **A new package dir shadows a same-named flat module.** Creating `schedule/execution/` makes the old `schedule/execution.py` silently dead code (the package wins import resolution), so move-then-delete, don't just copy. `git` usually detects these as renames, which keeps the diff reviewable. - **Docstring references survive file deletion.** After removing flat files, `:class:\`schedule.worker.NodeExecutor\``-style text can linger in docstrings and render as broken links. Grep for the old module name one more time at cleanup and rewrite comment-only refs too. +- **Don't silently upgrade dataclass-ness during a "structural only" refactor.** Stage 1 moved `ExecutionResult` from the flat `schedule/execution.py` into `domain/execution.py` and *decorated* it with `@dataclass(frozen=True)` along the way. Pre-refactor it was a plain class. Review (2026-08-21) caught that this changes three things at once: identity-`==` becomes value-`==`, mutation raises `FrozenInstanceError`, and `repr()` becomes structured. No caller in the repo mutates or compares these objects, so the only externally visible change is log format — but it is *not* "zero behavior change." If you want strict behavioral equivalence during a structural move, copy the class definition verbatim and document any intentional semantic tightening. ### Frontend state + routing (zustand + React Router v8) diff --git a/schedule/src/schedule/domain/execution.py b/schedule/src/schedule/domain/execution.py index ceb355d..b864db9 100644 --- a/schedule/src/schedule/domain/execution.py +++ b/schedule/src/schedule/domain/execution.py @@ -2,6 +2,12 @@ Pure value objects — no I/O, no logging, no model imports. Safe to import from any layer. + +Note: ``ExecutionResult`` is a frozen dataclass here, while the pre-refactor +flat ``schedule/execution.py`` defined it as a plain class. The frozen + +value-equality upgrade is a deliberate enhancement (review verification, +2026-08-21): no caller mutates the object and no caller relies on identity +comparison, so the only visible change is structured log output. """ from __future__ import annotations