diff --git a/.agents/tasks/2025/08/21-0939-codetype-interface b/.agents/tasks/2025/08/21-0939-codetype-interface index 2814139..0d01245 100644 --- a/.agents/tasks/2025/08/21-0939-codetype-interface +++ b/.agents/tasks/2025/08/21-0939-codetype-interface @@ -40,3 +40,6 @@ Implement the CodeObjectWrapper as designed. Update the Tracer trait as well as There is an issue in the current implementation. We don't use caching effectively, since we create a new CodeObjectWrapper at each callback_xxx call. We need a global cache, probably keyed by the code object id. Propose design changes and update the design documents. Don't implement the changes themselves before I approve them. --- FOLLOW UP TASK --- Implement the global code object registry. + +--- FOLLOW UP TASK --- +I want to refactor the code of codetracer-python-recorder in order to follow a single-responsibility-principle on the level of files - each file should be concerned with a single cohesive topic. Propose a plan for refactoring and create a self-contained ADR to be given to junior devs for implementation. Specifiy which tasks can be done in parallel \ No newline at end of file diff --git a/design-docs/adr/0001-file-level-single-responsibility.md b/design-docs/adr/0001-file-level-single-responsibility.md new file mode 100644 index 0000000..9252118 --- /dev/null +++ b/design-docs/adr/0001-file-level-single-responsibility.md @@ -0,0 +1,65 @@ +# ADR 0001: File-Level Single Responsibility Refactor + +- **Status:** Proposed +- **Date:** 2025-10-01 +- **Deciders:** Platform / Runtime Tracing Team +- **Consulted:** Python Tooling WG, Developer Experience WG + +## Context + +The codetracer Python recorder crate has evolved quickly and several source files now mix unrelated concerns: +- [`src/lib.rs`](../../codetracer-python-recorder/src/lib.rs) hosts PyO3 module wiring, global logging setup, tracing session state, and filesystem validation in one place. +- [`src/runtime_tracer.rs`](../../codetracer-python-recorder/src/runtime_tracer.rs) interleaves activation gating, writer lifecycle control, PyFrame helpers, and Python value encoding logic, making it challenging to test or extend any portion independently. +- [`src/tracer.rs`](../../codetracer-python-recorder/src/tracer.rs) combines sys.monitoring shim code with the `Tracer` trait, callback registration, and global caches. +- [`codetracer_python_recorder/api.py`](../../codetracer-python-recorder/codetracer_python_recorder/api.py) mixes format constants, backend interaction, context manager ergonomics, and environment based auto-start side effects. + +This violates the Single Responsibility Principle (SRP) at the file level, obscures ownership boundaries, and increases the risk of merge conflicts and regressions. Upcoming work on richer value capture and optional streaming writers will add more logic to these files unless we carve out cohesive modules now. + +## Decision + +We will reorganise both the Rust crate and supporting Python package so that each file covers a single cohesive topic and exposes a narrow interface. Concretely: +1. Restrict `src/lib.rs` to PyO3 module definition and `pub use` re-exports. Move logging configuration into `src/logging.rs` and tracing session lifecycle into `src/session.rs`. +2. Split the current runtime tracer into a `runtime` module directory with dedicated files for activation control, value encoding, and output file management. The façade in `runtime/mod.rs` will assemble these pieces and expose the existing `RuntimeTracer` API. +3. Introduce a `monitoring` module directory that separates sys.monitoring primitive bindings (`EventId`, `ToolId`, registration helpers) from the `Tracer` trait and callback dispatch logic. +4. Decompose the Python helper package by moving session state management into `session.py`, format constants and validation into `formats.py`, and environment auto-start into `auto_start.py`, while keeping public functions surfaced through `api.py` and `__init__.py`. + +These changes are mechanical reorganisations—no behavioural changes are expected. Public Rust and Python APIs must remain source compatible during the refactor. + +## Consequences + +- **Positive:** + - Easier onboarding for new contributors because each file advertises a single purpose. + - Improved unit testability; e.g., Python value encoding can be tested without instantiating the full tracer. + - Lower merge conflict risk: teams can edit activation logic without touching writer code. + - Clearer extension points for upcoming streaming writer and richer metadata work. +- **Negative / Risks:** + - Temporary churn in module paths may invalidate outstanding branches; mitigation is to stage work in small, reviewable PRs. + - Developers unfamiliar with Rust module hierarchies will need guidance to update `mod` declarations and `use` paths correctly. + - Python packaging changes require careful coordination to avoid circular imports when moving auto-start logic. + +## Implementation Guidelines for Junior Developers + +1. **Work Incrementally.** Aim for small PRs (≤500 LOC diff) that move one responsibility at a time. After each PR run `just test` and ensure all linters stay green. +2. **Preserve APIs.** When moving functions, re-export them from their new module so that existing callers (Rust and Python) compile without modification in the same PR. +3. **Add Focused Tests.** Whenever a helper is extracted (e.g., value encoding), add or migrate unit tests that cover its edge cases. +4. **Document Moves.** Update doc comments and module-level docs to reflect the new structure. Remove outdated TODOs or convert them into follow-up issues. +5. **Coordinate on Shared Types.** When splitting `runtime_tracer.rs`, agree on ownership for shared structs (e.g., `RuntimeTracer` remains in `runtime/mod.rs`). Use `pub(crate)` to keep internals encapsulated. +6. **Python Imports.** After splitting the Python modules, ensure `__all__` in `__init__.py` continues to export the public API. Use relative imports to avoid accidental circular dependencies. +7. **Parallel Work.** Follow the sequencing from `design-docs/file-level-srp-refactor-plan.md` to know when tasks can proceed in parallel. + +## Testing Strategy + +- Run `just test` locally before submitting each PR. +- Add targeted Rust tests for new modules (e.g., `activation` and `value_encoder`). +- Extend Python tests to cover auto-start logic and the context manager after extraction. +- Compare trace outputs against saved fixtures to ensure refactors do not alter serialized data. + +## Alternatives Considered + +- **Leave the layout as-is:** rejected because it impedes planned features and increases onboarding cost. +- **Large rewrite in a single PR:** rejected due to high risk and code review burden. + +## Follow-Up Actions + +- After completing the refactor, update architecture diagrams in `design-docs` to match the new module structure. +- Schedule knowledge-sharing sessions for new module owners to walk through their areas. diff --git a/design-docs/file-level-srp-refactor-plan.md b/design-docs/file-level-srp-refactor-plan.md new file mode 100644 index 0000000..476bfc6 --- /dev/null +++ b/design-docs/file-level-srp-refactor-plan.md @@ -0,0 +1,87 @@ +# File-Level Single Responsibility Refactor Plan + +## Goals +- Reshape the Rust crate and Python support package so that every source file encapsulates a single cohesive topic. +- Reduce the amount of ad-hoc cross-module knowledge currently required to understand tracing start-up, event handling, and encoding logic. +- Preserve the public Python API and Rust crate interfaces during the refactor to avoid disruptions for downstream tooling. + +## Current State Observations +- `src/lib.rs` is responsible for PyO3 module registration, lifecycle management for tracing sessions, global logging initialisation, and runtime format selection, which mixes unrelated concerns in one file. +- `src/runtime_tracer.rs` couples trace lifecycle control, activation toggling, and Python value encoding in a single module, making it difficult to unit test or substitute individual pieces. +- `src/tracer.rs` combines the `Tracer` trait definition, sys.monitoring shims, callback registration utilities, and thread-safe storage, meaning small changes can ripple through unrelated logic. +- `codetracer_python_recorder/api.py` interleaves environment based auto-start, context-manager ergonomics, backend state management, and format constants, leaving no clearly isolated entry-point for CLI or library callers. + +## Target Rust Module Layout +| Topic | Target file | Notes | +| --- | --- | --- | +| PyO3 module definition & re-exports | `src/lib.rs` | Limit to module wiring plus `pub use` statements. +| Global logging defaults | `src/logging.rs` | Provide helper to configure env_logger defaults reused by both lib.rs and tests. +| Tracing session lifecycle (`start_tracing`, `stop_tracing`, `flush_tracing`, `is_tracing`) | `src/session.rs` | Own global `ACTIVE` flag and filesystem validation. +| Runtime tracer orchestration (activation gating, writer plumbing) | `src/runtime/mod.rs` | Public `RuntimeTracer` facade constructed by session. +| Value encoding helpers | `src/runtime/value_encoder.rs` | Convert Python objects into `runtime_tracing::ValueRecord` values; unit test in isolation. +| Activation management (start-on-enter logic) | `src/runtime/activation.rs` | Encapsulate `activation_path`, `activation_code_id`, and toggling state. +| Writer initialisation and file path selection | `src/runtime/output_paths.rs` | Determine file names for JSON/Binary and wrap TraceWriter begin/finish. +| sys.monitoring integration utilities | `src/monitoring/mod.rs` | Provide `ToolId`, `EventId`, `MonitoringEvents`, `set_events`, etc. +| Tracer trait & callback dispatch | `src/monitoring/tracer.rs` | Define `Tracer` trait and per-event callbacks; depend on `monitoring::events`. +| Code object caching | `src/code_object.rs` | Remains focused on caching; consider relocating question comments to doc tests. + +The `runtime` and `monitoring` modules become directories with focused submodules, while `session.rs` consumes them via narrow interfaces. Any PyO3 FFI helper functions should live close to their domain (e.g., frame locals helpers inside `runtime/mod.rs`). + +## Target Python Package Layout +| Topic | Target file | Notes | +| --- | --- | --- | +| Public API surface (`start`, `stop`, `is_tracing`, constants) | `codetracer_python_recorder/api.py` | Keep the public signatures unchanged; delegate to new helpers. +| Session handle implementation | `codetracer_python_recorder/session.py` | Own `TraceSession` class and backend delegation logic. +| Auto-start via environment variables | `codetracer_python_recorder/auto_start.py` | Move `_auto_start_from_env` and constants needed only for boot-time configuration. +| Format constants & validation | `codetracer_python_recorder/formats.py` | Define `TRACE_BINARY`, `TRACE_JSON`, `DEFAULT_FORMAT`, and any helpers to negotiate format strings. +| Module-level `__init__` exports | `codetracer_python_recorder/__init__.py` | Re-export the API and trigger optional auto-start. + +Splitting the Python helper package along these lines isolates side-effectful auto-start logic from the plain API and simplifies targeted testing. + +## Implementation Roadmap + +1. **Stabilise tests and build scripts** + - Ensure `just test` passes to establish a green baseline. + - Capture benchmarks or representative trace outputs to validate parity later. + +2. **Introduce foundational Rust modules (serial)** + - Extract logging initialisation into `logging.rs` and update `lib.rs` to call the helper. + - Move session lifecycle logic from `lib.rs` into a new `session.rs`, keeping function signatures untouched and re-exporting via `lib.rs`. + - Update module declarations and adjust imports; verify tests. + +3. **Restructure runtime tracer internals (can parallelise subtasks)** + - Create `src/runtime/mod.rs` as façade exposing `RuntimeTracer`. + - **Task 3A (Team A)**: Extract activation control into `runtime/activation.rs`, exposing a small struct consumed by the tracer. + - **Task 3B (Team B)**: Extract value encoding routines into `runtime/value_encoder.rs`, providing unit tests and benchmarks. + - **Task 3C (Team C)**: Introduce `runtime/output_paths.rs` to encapsulate format-to-filename mapping and writer initialisation. + - Integrate submodules back into `runtime/mod.rs` sequentially once individual tasks are complete; resolve merge conflicts around struct fields. + +4. **Modularise sys.monitoring glue (partially parallel)** + - Add `monitoring/mod.rs` hosting shared types (`EventId`, `EventSet`, `ToolId`). + - Split trait and dispatcher logic into `monitoring/tracer.rs`; keep callback registration helpers near the sys.monitoring bindings. + - **Task 4A (Team A)**: Port OnceLock caches and registration helpers. + - **Task 4B (Team B)**: Move `Tracer` trait definition and default implementations, updating call sites in runtime tracer and tests. + +5. **Python package decomposition (parallel with Step 4 once Step 2 is merged)** + - Create `session.py`, `formats.py`, and `auto_start.py` with extracted logic. + - Update `api.py` to delegate to the new modules but maintain backward-compatible imports. + - Adjust `__init__.py` to import from `api` and trigger optional auto-start via the new helper. + - Update Python tests and examples to use the reorganised structure. + +6. **Clean-up and follow-up tasks** + - Remove obsolete comments (e.g., `//TODO AI!` placeholders) or move them into GitHub issues. + - Update documentation and diagrams to reflect the new module tree. + - Re-run `just test` and linting for both Rust and Python components; capture trace artifacts to confirm unchanged output format. + +## Parallelisation Notes +- Step 2 touches the global entry points and should complete before deeper refactors to minimise rebasing pain. +- Step 3 subtasks (activation, value encoding, output paths) operate on distinct sections of the existing `RuntimeTracer`; they can be implemented in parallel once `runtime/mod.rs` scaffolding exists. +- Step 4's subtasks can proceed concurrently with Step 3 once the new `monitoring` module is introduced; teams should coordinate on shared types but work on separate files. +- Step 5 (Python package) depends on Step 2 so that backend entry-points remain stable; it can overlap with late Step 3/4 work because it touches only the Python tree. +- Documentation updates and clean-up in Step 6 can be distributed among contributors after core refactors merge. + +## Testing & Verification Strategy +- Maintain existing integration and unit tests; add focused tests for newly separated modules (e.g., pure Rust tests for `value_encoder` conversions). +- Extend Python tests to cover environment auto-start logic now that it lives in its own module. +- For each phase, compare generated trace files against baseline fixtures to guarantee no behavioural regressions. +- Require code review sign-off from domain owners for each phase to ensure the single-responsibility intent is preserved.