|
| 1 | +# PR4: Sentry `skip_sentry` Leak Fix + Dead `is_X_installed` Conjuncts Cleanup |
| 2 | + |
| 3 | +> **For agentic workers:** REQUIRED SUB-SKILL: Use superpowers:subagent-driven-development (recommended) or superpowers:executing-plans to implement this plan task-by-task. Steps use checkbox (`- [ ]`) syntax for tracking. |
| 4 | +
|
| 5 | +**Goal:** Ship two small audit cleanups in one PR: |
| 6 | + |
| 7 | +- **DES-4:** Add `"skip_sentry"` to `IGNORED_STRUCTLOG_ATTRIBUTES` so the flag stops leaking into Sentry's `contexts.structlog` when set to falsy (the function already suppresses the event for truthy values, but doesn't strip the field for falsy ones). |
| 8 | +- **DES-5:** Delete the dead `and import_checker.is_X_installed` conjuncts from four instruments' `is_ready()` methods. They're provably unreachable: `_register_or_skip` runs `check_dependencies()` before instantiating the instrument; if `check_dependencies()` returns False, `is_ready()` is never called. |
| 9 | + |
| 10 | +**Architecture:** Five files modified — four production deletions/additions and one test case. No new abstractions. No API changes. |
| 11 | + |
| 12 | +**Tech Stack:** Python 3.10+, pytest parametrized tests, sentry_sdk types. |
| 13 | + |
| 14 | +**Parent spec:** `docs/superpowers/specs/2026-05-31-audit-implementation-sequencing.md` (PR4 section). |
| 15 | +**Parent audit:** `docs/superpowers/specs/2026-05-31-bug-refactor-audit.md` (DES-4, DES-5). |
| 16 | + |
| 17 | +--- |
| 18 | + |
| 19 | +## File Structure |
| 20 | + |
| 21 | +Four production files modified; one test file modified. |
| 22 | + |
| 23 | +- Modify: `lite_bootstrap/instruments/sentry_instrument.py` — add `"skip_sentry"` to `IGNORED_STRUCTLOG_ATTRIBUTES` (DES-4); drop dead conjunct from `SentryInstrument.is_ready()` (DES-5). |
| 24 | +- Modify: `lite_bootstrap/instruments/logging_instrument.py:139-140` — drop dead conjunct from `LoggingInstrument.is_ready()` (DES-5). |
| 25 | +- Modify: `lite_bootstrap/instruments/opentelemetry_instrument.py:82-86` — drop dead conjunct from `OpenTelemetryInstrument.is_ready()` (DES-5). |
| 26 | +- Modify: `lite_bootstrap/instruments/pyroscope_instrument.py:28-29` — drop dead conjunct from `PyroscopeInstrument.is_ready()` (DES-5). |
| 27 | +- Modify: `tests/instruments/test_sentry_instrument.py` — add parametrize case to `TestSentryEnrichEventFromStructlog::test_modify` covering the `skip_sentry=False` case. |
| 28 | + |
| 29 | +--- |
| 30 | + |
| 31 | +## Locked decisions |
| 32 | + |
| 33 | +- **Bundling DES-4 + DES-5:** They're independent in scope but both trivial and both touch instrument files. Reviewing them together is cheaper than two PRs. |
| 34 | +- **DES-4 test placement:** Extend the existing `test_modify` parametrize block in `TestSentryEnrichEventFromStructlog`. Same shape as adjacent cases; no new test method. |
| 35 | +- **DES-5 testing:** No new tests. Pure dead-code deletion. The existing test suite already exercises the `is_ready()` paths via the framework integration tests; any breakage shows up there. |
| 36 | + |
| 37 | +--- |
| 38 | + |
| 39 | +## Task 1: Create branch |
| 40 | + |
| 41 | +**Files:** (no files; git only) |
| 42 | + |
| 43 | +- [ ] **Step 1: Branch off `main`** |
| 44 | + |
| 45 | +```bash |
| 46 | +git checkout main |
| 47 | +git pull --ff-only origin main |
| 48 | +git checkout -b fix/des-4-5-small-cleanups |
| 49 | +``` |
| 50 | + |
| 51 | +Expected: `Switched to a new branch 'fix/des-4-5-small-cleanups'`. |
| 52 | + |
| 53 | +If PR3 (`fix/crit-3-idempotent-teardown`) has not yet merged, that's fine — PR4 touches different files. Branch from current `main` regardless. |
| 54 | + |
| 55 | +--- |
| 56 | + |
| 57 | +## Task 2: Add the failing regression test (DES-4) |
| 58 | + |
| 59 | +**File:** `tests/instruments/test_sentry_instrument.py` |
| 60 | + |
| 61 | +The file already has a class `TestSentryEnrichEventFromStructlog` with a parametrized `test_modify` method. Add a third case to its parametrize list that covers the `skip_sentry=False` scenario. |
| 62 | + |
| 63 | +- [ ] **Step 1: Add the new parametrize case** |
| 64 | + |
| 65 | +Current `test_modify` (around line 92 of the file) has two cases in its parametrize list. The list looks like: |
| 66 | + |
| 67 | +```python |
| 68 | + @pytest.mark.parametrize( |
| 69 | + ("event_before", "event_after"), |
| 70 | + [ |
| 71 | + ( |
| 72 | + {"logentry": {"formatted": '{"event": "event name"}'}, "contexts": {}}, |
| 73 | + {"logentry": {"formatted": "event name"}, "contexts": {}}, |
| 74 | + ), |
| 75 | + ( |
| 76 | + { |
| 77 | + "logentry": { |
| 78 | + "formatted": '{"event": "event name", "timestamp": 1, "level": "error", "logger": "event.logger", "tracing": {}, "foo": "bar"}' # noqa: E501 |
| 79 | + }, |
| 80 | + "contexts": {}, |
| 81 | + }, |
| 82 | + { |
| 83 | + "logentry": {"formatted": "event name"}, |
| 84 | + "contexts": {"structlog": {"foo": "bar"}}, |
| 85 | + }, |
| 86 | + ), |
| 87 | + ], |
| 88 | + ) |
| 89 | + def test_modify(self, event_before: "sentry_types.Event", event_after: "sentry_types.Event") -> None: |
| 90 | + assert enrich_sentry_event_from_structlog_log(event_before, {}) == event_after |
| 91 | +``` |
| 92 | + |
| 93 | +Add a third tuple to the parametrize list, after the existing two cases (preserving trailing comma in the list): |
| 94 | + |
| 95 | +```python |
| 96 | + ( |
| 97 | + { |
| 98 | + "logentry": { |
| 99 | + "formatted": '{"event": "event name", "skip_sentry": false, "foo": "bar"}' |
| 100 | + }, |
| 101 | + "contexts": {}, |
| 102 | + }, |
| 103 | + { |
| 104 | + "logentry": {"formatted": "event name"}, |
| 105 | + "contexts": {"structlog": {"foo": "bar"}}, |
| 106 | + }, |
| 107 | + ), |
| 108 | +``` |
| 109 | + |
| 110 | +The contract: when a structlog payload contains `skip_sentry=false` (a falsy value that doesn't trigger event suppression), the resulting `contexts.structlog` should contain `{"foo": "bar"}` only — `skip_sentry` should be stripped. |
| 111 | + |
| 112 | +- [ ] **Step 2: Run the test and verify it FAILS** |
| 113 | + |
| 114 | +```bash |
| 115 | +just test -- 'tests/instruments/test_sentry_instrument.py::TestSentryEnrichEventFromStructlog::test_modify' -v |
| 116 | +``` |
| 117 | + |
| 118 | +Expected: one of the three parametrize cases (the new one) **FAILS** because the current `IGNORED_STRUCTLOG_ATTRIBUTES` set doesn't include `"skip_sentry"`. The actual `contexts.structlog` will be `{"skip_sentry": False, "foo": "bar"}`, which doesn't equal the expected `{"foo": "bar"}`. The other two cases should still PASS. |
| 119 | + |
| 120 | +If the new case passes, stop and investigate — either the assertion is wrong, or the bug isn't present. |
| 121 | + |
| 122 | +--- |
| 123 | + |
| 124 | +## Task 3: Implement all changes |
| 125 | + |
| 126 | +Five small edits across four production files. Apply them all, run tests, lint, commit. |
| 127 | + |
| 128 | +### Fix 1 (DES-4): Strip `skip_sentry` from Sentry context |
| 129 | + |
| 130 | +- [ ] **Step 1: Add `"skip_sentry"` to `IGNORED_STRUCTLOG_ATTRIBUTES`** |
| 131 | + |
| 132 | +**File:** `lite_bootstrap/instruments/sentry_instrument.py:19-21` |
| 133 | + |
| 134 | +Current: |
| 135 | + |
| 136 | +```python |
| 137 | +IGNORED_STRUCTLOG_ATTRIBUTES: typing.Final = frozenset( |
| 138 | + {"event", "level", "logger", "tracing", "timestamp", "exception"} |
| 139 | +) |
| 140 | +``` |
| 141 | + |
| 142 | +Replace with: |
| 143 | + |
| 144 | +```python |
| 145 | +IGNORED_STRUCTLOG_ATTRIBUTES: typing.Final = frozenset( |
| 146 | + {"event", "level", "logger", "tracing", "timestamp", "exception", "skip_sentry"} |
| 147 | +) |
| 148 | +``` |
| 149 | + |
| 150 | +### Fix 2 (DES-5): Drop dead conjunct from `SentryInstrument.is_ready()` |
| 151 | + |
| 152 | +- [ ] **Step 2: Simplify `SentryInstrument.is_ready()`** |
| 153 | + |
| 154 | +**File:** `lite_bootstrap/instruments/sentry_instrument.py:100-101` |
| 155 | + |
| 156 | +Current: |
| 157 | + |
| 158 | +```python |
| 159 | + def is_ready(self) -> bool: |
| 160 | + return bool(self.bootstrap_config.sentry_dsn) and import_checker.is_sentry_installed |
| 161 | +``` |
| 162 | + |
| 163 | +Replace with: |
| 164 | + |
| 165 | +```python |
| 166 | + def is_ready(self) -> bool: |
| 167 | + return bool(self.bootstrap_config.sentry_dsn) |
| 168 | +``` |
| 169 | + |
| 170 | +### Fix 3 (DES-5): Drop dead conjunct from `LoggingInstrument.is_ready()` |
| 171 | + |
| 172 | +- [ ] **Step 3: Simplify `LoggingInstrument.is_ready()`** |
| 173 | + |
| 174 | +**File:** `lite_bootstrap/instruments/logging_instrument.py:139-140` |
| 175 | + |
| 176 | +Current: |
| 177 | + |
| 178 | +```python |
| 179 | + def is_ready(self) -> bool: |
| 180 | + return self.bootstrap_config.logging_enabled and import_checker.is_structlog_installed |
| 181 | +``` |
| 182 | + |
| 183 | +Replace with: |
| 184 | + |
| 185 | +```python |
| 186 | + def is_ready(self) -> bool: |
| 187 | + return self.bootstrap_config.logging_enabled |
| 188 | +``` |
| 189 | + |
| 190 | +### Fix 4 (DES-5): Drop dead conjunct from `OpenTelemetryInstrument.is_ready()` |
| 191 | + |
| 192 | +- [ ] **Step 4: Simplify `OpenTelemetryInstrument.is_ready()`** |
| 193 | + |
| 194 | +**File:** `lite_bootstrap/instruments/opentelemetry_instrument.py:82-86` |
| 195 | + |
| 196 | +Current: |
| 197 | + |
| 198 | +```python |
| 199 | + def is_ready(self) -> bool: |
| 200 | + return ( |
| 201 | + bool(self.bootstrap_config.opentelemetry_endpoint or self.bootstrap_config.opentelemetry_log_traces) |
| 202 | + and import_checker.is_opentelemetry_installed |
| 203 | + ) |
| 204 | +``` |
| 205 | + |
| 206 | +Replace with: |
| 207 | + |
| 208 | +```python |
| 209 | + def is_ready(self) -> bool: |
| 210 | + return bool(self.bootstrap_config.opentelemetry_endpoint or self.bootstrap_config.opentelemetry_log_traces) |
| 211 | +``` |
| 212 | + |
| 213 | +### Fix 5 (DES-5): Drop dead conjunct from `PyroscopeInstrument.is_ready()` |
| 214 | + |
| 215 | +- [ ] **Step 5: Simplify `PyroscopeInstrument.is_ready()`** |
| 216 | + |
| 217 | +**File:** `lite_bootstrap/instruments/pyroscope_instrument.py:28-29` |
| 218 | + |
| 219 | +Current: |
| 220 | + |
| 221 | +```python |
| 222 | + def is_ready(self) -> bool: |
| 223 | + return bool(self.bootstrap_config.pyroscope_endpoint) and import_checker.is_pyroscope_installed |
| 224 | +``` |
| 225 | + |
| 226 | +Replace with: |
| 227 | + |
| 228 | +```python |
| 229 | + def is_ready(self) -> bool: |
| 230 | + return bool(self.bootstrap_config.pyroscope_endpoint) |
| 231 | +``` |
| 232 | + |
| 233 | +### Verify and commit |
| 234 | + |
| 235 | +- [ ] **Step 6: Run the previously-failing test, verify PASS** |
| 236 | + |
| 237 | +```bash |
| 238 | +just test -- 'tests/instruments/test_sentry_instrument.py::TestSentryEnrichEventFromStructlog' -v |
| 239 | +``` |
| 240 | + |
| 241 | +Expected: all three `test_modify` cases PASS. |
| 242 | + |
| 243 | +- [ ] **Step 7: Run the full test suite** |
| 244 | + |
| 245 | +```bash |
| 246 | +just test |
| 247 | +``` |
| 248 | + |
| 249 | +Expected: all tests PASS. The dead-conjunct deletions are provably no-ops at runtime (the `_register_or_skip` flow in `bootstrappers/base.py` checks `check_dependencies()` before any `is_ready()` call), so existing tests that exercise the missing-dependency path — e.g., `test_fastapi_bootstrapper_with_missing_instrument_dependency`, `test_litestar_bootstrapper_with_missing_instrument_dependency`, `test_free_bootstrapper_with_missing_instrument_dependency` — should still pass unchanged. If any of those fail, stop and investigate: the invariant we're relying on may not hold somewhere. |
| 250 | + |
| 251 | +- [ ] **Step 8: Run lint** |
| 252 | + |
| 253 | +```bash |
| 254 | +just lint |
| 255 | +``` |
| 256 | + |
| 257 | +Expected: no errors. The four `import_checker` references being removed leave the import statement still used elsewhere in each file (e.g., `bootstrap()` methods), so no unused-import warnings should fire. Confirm. |
| 258 | + |
| 259 | +If a file ends up with `from lite_bootstrap import import_checker` no longer referenced anywhere, ruff `F401` will flag it. In that case, also remove the import. Most likely candidate is `pyroscope_instrument.py` (verify by reading the file). |
| 260 | + |
| 261 | +Actually, all four instrument files use `import_checker` in their `check_dependencies()` method as well, so the import will remain needed. Just confirm with `just lint`. |
| 262 | + |
| 263 | +- [ ] **Step 9: Commit (stage exactly 5 files)** |
| 264 | + |
| 265 | +```bash |
| 266 | +git add \ |
| 267 | + lite_bootstrap/instruments/sentry_instrument.py \ |
| 268 | + lite_bootstrap/instruments/logging_instrument.py \ |
| 269 | + lite_bootstrap/instruments/opentelemetry_instrument.py \ |
| 270 | + lite_bootstrap/instruments/pyroscope_instrument.py \ |
| 271 | + tests/instruments/test_sentry_instrument.py |
| 272 | +git commit -m "$(cat <<'EOF' |
| 273 | +fix: strip skip_sentry from Sentry context; drop dead is_X_installed conjuncts |
| 274 | +
|
| 275 | +DES-4: enrich_sentry_event_from_structlog_log was already returning None |
| 276 | +(suppressing the event) when skip_sentry was truthy, but for falsy values |
| 277 | +(False, missing, "") the flag itself was not stripped from the structlog |
| 278 | +payload before it was attached to event["contexts"]["structlog"]. Add |
| 279 | +"skip_sentry" to IGNORED_STRUCTLOG_ATTRIBUTES so the field never leaks |
| 280 | +into Sentry context noise. Regression test added as a parametrize case |
| 281 | +on the existing test_modify. |
| 282 | +
|
| 283 | +DES-5: each affected instrument's is_ready() returned `<config check> and |
| 284 | +import_checker.is_X_installed`. The conjunct is provably dead: BaseBootstrapper |
| 285 | +calls check_dependencies() in _register_or_skip before instantiating the |
| 286 | +instrument, and only invokes is_ready() if check_dependencies() returned True. |
| 287 | +Drop the redundant conjunct from SentryInstrument, LoggingInstrument, |
| 288 | +OpenTelemetryInstrument, and PyroscopeInstrument. Behavior is unchanged. |
| 289 | +
|
| 290 | +Closes DES-4 and DES-5 from the audit. |
| 291 | +EOF |
| 292 | +)" |
| 293 | +``` |
| 294 | + |
| 295 | +--- |
| 296 | + |
| 297 | +## Task 4: Push and open PR |
| 298 | + |
| 299 | +**Files:** (no files; git push + gh) |
| 300 | + |
| 301 | +- [ ] **Step 1: Push the branch** |
| 302 | + |
| 303 | +```bash |
| 304 | +git push -u origin fix/des-4-5-small-cleanups |
| 305 | +``` |
| 306 | + |
| 307 | +- [ ] **Step 2: Open the PR** |
| 308 | + |
| 309 | +```bash |
| 310 | +gh pr create --title "fix: strip skip_sentry from Sentry context; drop dead is_X_installed conjuncts" --body "$(cat <<'EOF' |
| 311 | +## Summary |
| 312 | +Two small audit cleanups bundled: |
| 313 | +
|
| 314 | +- **DES-4 (Sentry):** \`skip_sentry\` was already triggering event suppression when truthy, but for falsy values (False/missing/"") the flag itself wasn't stripped from the structlog payload and ended up as noise in \`event["contexts"]["structlog"]\`. Add \`"skip_sentry"\` to \`IGNORED_STRUCTLOG_ATTRIBUTES\`. Regression test added as a parametrize case on the existing \`test_modify\`. |
| 315 | +- **DES-5 (dead conjuncts):** Four instruments' \`is_ready()\` methods ended with \`and import_checker.is_X_installed\`. That conjunct is provably unreachable — \`BaseBootstrapper._register_or_skip\` calls \`check_dependencies()\` first and only invokes \`is_ready()\` if it returned True. Behavior is unchanged. Cleanup makes the lifecycle easier to reason about. |
| 316 | +
|
| 317 | +Closes DES-4 and DES-5 from an internal audit. |
| 318 | +
|
| 319 | +## Test plan |
| 320 | +- [x] \`just test -- tests/instruments/test_sentry_instrument.py -v\` — pass. |
| 321 | +- [x] \`just test\` — full suite passes. |
| 322 | +- [x] \`just lint\` — clean (no unused-import warnings from the conjunct removals). |
| 323 | +- [ ] Reviewer: confirm the invariant claim — that \`is_ready()\` is only called after \`check_dependencies()\` has returned True — by reading \`bootstrappers/base.py:44-64\`. |
| 324 | +
|
| 325 | +🤖 Generated with [Claude Code](https://claude.com/claude-code) |
| 326 | +EOF |
| 327 | +)" |
| 328 | +``` |
| 329 | + |
| 330 | +--- |
| 331 | + |
| 332 | +## Self-Review |
| 333 | + |
| 334 | +**Spec coverage check** against the sequencing spec (PR4 section) and audit (DES-4, DES-5): |
| 335 | + |
| 336 | +| Spec item | Task | |
| 337 | +|-----------|------| |
| 338 | +| Add `"skip_sentry"` to `IGNORED_STRUCTLOG_ATTRIBUTES` | Task 3, Step 1 | |
| 339 | +| Regression test asserting `skip_sentry` doesn't appear in context | Task 2, Step 1 | |
| 340 | +| Delete dead conjunct from `SentryInstrument.is_ready()` | Task 3, Step 2 | |
| 341 | +| Delete dead conjunct from `LoggingInstrument.is_ready()` | Task 3, Step 3 | |
| 342 | +| Delete dead conjunct from `OpenTelemetryInstrument.is_ready()` | Task 3, Step 4 | |
| 343 | +| Delete dead conjunct from `PyroscopeInstrument.is_ready()` | Task 3, Step 5 | |
| 344 | +| Branch name `fix/des-4-5-small-cleanups` | Task 1, Step 1 | |
| 345 | +| Verification: `just test` + `just lint` pass | Task 3, Steps 7-8 | |
| 346 | + |
| 347 | +All spec items covered. No placeholders. Parametrize-case shape matches adjacent cases byte-for-byte except for the payload values. |
| 348 | + |
| 349 | +**Deferred:** |
| 350 | +- Documenting the lifecycle invariant on `BaseInstrument` (mentioned as "optional" in the sequencing spec) — skip for now to keep the PR focused. Worth noting somewhere later (a `CONTRIBUTING.md`, or class docstrings as part of REF-3). |
0 commit comments