Skip to content

Commit 0d1b6ec

Browse files
lesnik512claude
andcommitted
docs: addendum to audit retro covering PR #107 iteration
PR #107 (instrument skip rework) shipped a few days after the original arc closed. Captured three new datapoints worth recording: - Mid-design pivot worked: user pushback on the is_configured arg forced the design through pre-#88 history and confirmed the classmethod-with-arg pattern was correct. - Defensive workaround was wrong design: my _get_logger() fresh-per- call structlog proxy made tests pass but the user's pivot to stdlib logging + public build_summary() was the actual right answer. caplog (which the original plan flagged but the subagent ignored) was the correct test mechanism. - LSP violations on classmethod parameter overrides are an emergent pattern: 3 framework instrument overrides needed # ty: ignore[invalid-method-override] because they narrow bootstrap_config covariantly. Existing field-narrowing pattern is accepted; method-param narrowing now needs the explicit ignore. Added action items #8 (workaround-vs-framework-choice heuristic), #9 (cap subagent dispatch scope; the ~60-minute connection drop orphaned work), and #10 (document the LSP override pattern in CLAUDE.md). Reaffirms the original retro's closing observation: subagent loop reliably produces a green-tests implementation of the spec, but the spec is rarely the right design. Design emerges during review iteration. PR #107 needed 5 user follow-up commits after the mechanical migration landed. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
1 parent 50f3cd5 commit 0d1b6ec

1 file changed

Lines changed: 32 additions & 0 deletions

File tree

docs/superpowers/specs/2026-06-01-audit-implementation-retro.md

Lines changed: 32 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -145,3 +145,35 @@ The arc shipped what it set out to ship. 3 critical bugs closed, 5 design issues
145145
The methodology was also heavier than the work in places. The next time this team takes on an audit, the lightweight-plan template (action #1) and pre-flight grep (action #2) would meaningfully reduce overhead without giving up the review gates that caught real bugs.
146146

147147
The most underrated factor: the user remained in the loop as a quality reviewer. Without the cascade catch, the UnsetType contribution, the noqa policy, and the unreachable-guard cleanup, the codebase would have shipped a lower-quality version of these 15 PRs. The subagent loop produces consistent execution but does not produce judgment.
148+
149+
---
150+
151+
## Addendum (2026-06-02): PR #107 instrument skip rework
152+
153+
A second large refactor shipped after the original arc closed: PR #107, replacing `InstrumentNotReadyWarning` with `is_configured()` classmethod + structured `skipped_instruments` + summary log. Same methodology (brainstorm → spec → plan → subagent execution). Surfaced three new datapoints worth recording.
154+
155+
### What worked
156+
157+
- **Mid-design pivot to the right pattern.** During brainstorming the user pushed back on `is_configured` taking `bootstrap_config` as an arg ("why does it need it if config is on self?"). That question forced the design conversation through pre-#88 history (instance method + instantiation first) and led to confirming the classmethod-with-arg design was correct. Without the pushback I would have proposed the design without explaining the cascade of constraints.
158+
- **The lightweight template + combined-review pattern (action items #1 and #7 from the original retro) was validated again.** PR16 was the first test; PR #107's combined review structure followed the same pattern even though it didn't end up running formally (the subagent disconnect made the formal review unnecessary — I verified inline).
159+
- **Real-time spec correction (action #4) was honored.** During execution the design pivoted from `_get_logger()` to stdlib `logging` + public `build_summary()` method. A new spec doc (`2026-06-02-stdlib-logging-and-build-summary-design.md`) was written for the pivot rather than letting the doc drift from reality.
160+
161+
### What didn't work
162+
163+
- **Long-running subagent dispatches are fragile.** The implementer dispatch ran ~60 minutes (94 tool uses) before the socket dropped. Work was orphaned mid-flow — the production code edits were done but verification, commit, and docs (Task 9) were not. Recovery worked, but a smaller scope per dispatch would have lost less work.
164+
- **`_get_logger()` was a defensive workaround, not a design.** I introduced it to fix structlog's `cache_logger_on_first_use=True` caching interaction with `capture_logs()` at test time. It made the tests pass but produced an ugly API. The user's subsequent pivot — switch the bootstrapper to stdlib `logging` and expose `build_summary()` as a public method — was the actual right answer. `caplog` (pytest's stdlib-logging capture fixture) was the right test mechanism, which the original plan flagged but the subagent ignored in favor of `capture_logs()`. The lesson: when a fix feels like fighting the framework, the framework choice is probably wrong.
165+
- **LSP violations on framework instrument override parameter types are an emergent pattern.** Three `# ty: ignore[invalid-method-override]` were needed for `FastStreamOpenTelemetryInstrument.is_configured`, `FastStreamPrometheusInstrument.is_configured`, and `LitestarSwaggerInstrument.is_configured` because they narrow `bootstrap_config` to framework-specific types. The pattern was acceptable for `bootstrap_config:` field overrides (covariant) but ty enforces strict invariance on method parameters. Worth noting in CLAUDE.md if more `classmethod` overrides arise.
166+
167+
### Key insight
168+
169+
**The subagent does mechanical migration; design quality comes from human review iteration.** PR #107 needed 5 user follow-up commits after my work to reach the shipped design (`fa135d2`, `41d83bb`, `c14c455`, `4f051d6`, `86b43ef`). Each addressed a quality concern: silent-skip contract test, build_summary docstring tightening, empty-section handling, faststream warning leak fix, `isEnabledFor` guard on the summary log. None of these were in the original plan; all came from review iteration after the mechanical work landed.
170+
171+
This matches the original retro's closing observation. Worth restating concretely: the subagent loop reliably produces a green-tests implementation of the spec, but the spec is rarely the right design. The design emerges during review.
172+
173+
### New action items
174+
175+
| # | Action | Cost | Priority |
176+
|---|--------|------|----------|
177+
| 8 | When a fix requires a defensive workaround in production code to make tests pass, step back and ask whether the test mechanism (or the framework choice) is wrong. `_get_logger()` is the case study. | Low | High |
178+
| 9 | Cap single-dispatch subagent scope. The 94-tool-use, ~60-minute dispatch for PR #107 was too long. Either split into checkpointed sub-dispatches or set an explicit "implement only Tasks N–M, stop and report" boundary so progress doesn't get orphaned if the connection drops. | Low | High |
179+
| 10 | Document the LSP-violation pattern for classmethod overrides in CLAUDE.md alongside the existing covariant-narrowing note. `# ty: ignore[invalid-method-override]` is now established convention. | Low | Low |

0 commit comments

Comments
 (0)