Skip to content

refactor: close the Medium tech-debt findings from 20260803 - #107

Merged
mrojas54 merged 5 commits into
mainfrom
ai-ccd-cc/tech-debt-fixes
Aug 6, 2026
Merged

refactor: close the Medium tech-debt findings from 20260803#107
mrojas54 merged 5 commits into
mainfrom
ai-ccd-cc/tech-debt-fixes

Conversation

@mrojas54

@mrojas54 mrojas54 commented Aug 6, 2026

Copy link
Copy Markdown
Owner

Closes the remaining Medium findings from the 20260803 tech-debt assessment. Four commits, each independently green.

What changed

Commit Finding Result
dee95e4 12 duplicated fake-lmnr suppressions # type: ignore 21 → 10
c4f1a79 shell_safety.py had no co-located test file 24 tests moved, importing the module directly
82f7824 JSONL parse duplication one shared reader — plus a latent crash fixed
150c387 the package's two longest functions main() 176 → 104, _resolve_corpus() 141 → 57

The one behavior change: a real crash in find_located

The report called this "5 duplicated JSONL sites." Reading them, only two actually are (parsers.py ×2). Of the rest, adapters.py must not count malformed lines (S5 — counting there charges a session twice) and needs raw lines to replay via chain(buffered, lines); complex.py:326 decodes a payload embedded in a text block and is not a line reader at all. Both are left alone, with comments recording why.

The fifth, find_located, had no isinstance guard. A line of valid-but-non-object JSON reached entry.get(...):

AttributeError: 'int' object has no attribute 'get'

That took down the whole scoring run over one garbled transcript line. Routing it through the shared reader closes it, and mypy --strict then surfaced three more of the same class on the same path — message, content, and text were each dereferenced before their type was checked. All four now follow ClaudeParser.parse's existing discipline: a field arriving from outside is not a value until its type is checked.

test_a_bare_scalar_line_is_skipped_not_a_crash pins all four. Verified load-bearing — it fails with exactly that AttributeError against the previous find_located body, and passes after.

Notes for review

  • tests.fakes.make_module removes a suppression rather than relocating it. types.ModuleType declares no attributes, so module.Laminar = ... is an attr-defined error and each of the 12 sites carried its own ignore. setattr is typed (object, str, Any), so the factory is checked as written and needs no suppression at all.
  • The passive.py split is pure restructuring. Every comment moved with the code it explains; no control flow, disclosure string, or exit code changed. One ordering note: freeze_note is now computed at the render call instead of before the early return — same inputs, same value, and the early-return path never used it.
  • TRIAL_ROOT is deliberately defined in both test files. Scoring still needs it, and its "never stat it, so it need not exist" property is what the read-escape audit leans on.
  • The logfire High finding from the same report was already closed by 312d258 before this branch started.

Verification

Run on the merge of this branch with origin/main:

Check Result
uv run ruff check . clean — All checks passed!
uv run python -m toolbench.complexity_gate --base origin/main 0 errors, 0 warnings
uv run mypy --strict src/toolbench tests clean — 49 source files
uv run pytest -q 748 passed, 2 skipped, 10 subtests

Test count moved 747 → 748: the one addition is the regression test above. No test was lost or rewritten in the test_complex.pytest_shell_safety.py split.

Package functions over 100 lines: 12 → 10.

🤖 Generated with Claude Code

mrojas54 and others added 5 commits August 5, 2026 19:06
The `types.ModuleType("lmnr")` + attribute-assignment idiom was copy-pasted
across test_observability.py (9x), test_cli.py (2x), and test_tracing.py (1x),
each site carrying its own `# type: ignore[attr-defined]` because ModuleType
declares no attributes. That made the suppression count stop being a useful
signal: 21 total, 12 of them one duplicated pattern.

`tests.fakes.make_module` routes the assignment through `setattr`, which is
typed `(object, str, Any)` -- so the helper is checked as written and needs no
suppression of its own. The escape is designed out, not relocated.

Suppressions: 21 -> 10. Gate unchanged: ruff clean, mypy --strict clean,
pytest 747 passed / 2 skipped.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`shell_safety.py` was extracted from `complex.py` in 11142d3 without a
co-located test file, so the `test_<module>.py` convention pointed at nothing
for the safety-critical module -- bash tokenization, path containment,
gate-boundary checks. Coverage was real but reached only through `complex.py`'s
re-exports, which meant a broken re-export could masquerade as a passing audit.

Moves the three shell-safety groups out of test_complex.py:
  - the six `arm_violations` cases (split out of TrialScoringTests, which keeps
    its five score_trial cases) -> ArmViolationTests
  - GateTokenBoundaryTests (the F3 prefix-boundary regression)
  - ReadEscapeTests and its `_rc` / `_SERENA` helpers

They now import from `toolbench.shell_safety` directly. `TRIAL_ROOT` is defined
in both files: scoring still needs it, and its "never stat it" property is what
the read-escape audit leans on.

No test lost or rewritten: pytest 747 passed / 2 skipped, identical to the
commit before. test_complex.py 1012 -> 760 LOC; test_shell_safety.py 288 LOC,
24 tests.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The tech-debt report called this five duplicated JSONL parse sites. Reading
them, it is two -- and the other three differ for reasons worth keeping:

  - parsers.py:340 and parsers.py:536 ARE identical: strip, skip blank,
    json.loads, count malformed, isinstance-dict guard, count malformed. Both
    now use `transcript.JsonLines`, which owns the count and hands it to
    ParseResult.malformed.
  - adapters.py:75 must NOT count (S5: the parser counts, or a session is
    charged twice) and must keep every RAW line to replay via
    `chain(buffered, lines)`, which a decoded-object reader cannot give it.
    Left alone, with a comment saying why.
  - complex.py:326 is not a line reader at all -- it decodes a JSON payload
    embedded in an assistant text block.

The fifth, `find_located`, had no isinstance guard, so a line of valid
non-object JSON reached `entry.get(...)` and raised AttributeError, taking the
whole scoring run down over one garbled transcript line. Routing it through
JsonLines fixes that; mypy then surfaced three more of the same class on the
same path -- `message`, `content`, and `text` were each dereferenced before
their type was checked. All four now follow ClaudeParser.parse's discipline: a
field from outside is not a value until its type is checked.

`test_a_bare_scalar_line_is_skipped_not_a_crash` pins all four. Verified
load-bearing: it fails with the reported AttributeError against the previous
find_located body and passes after.

Gate: ruff clean, mypy --strict clean, pytest 748 passed / 2 skipped (+1, the
new regression test).

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…phases

`main()` was 176 lines and `_resolve_corpus()` 141 -- the two longest functions
in the package. Both were straight-line orchestration with the phase boundaries
implied by comment blocks rather than by names, so the only way to see what the
CLI does was to read all of it.

main() 176 -> 104, split at the boundaries that were already there:
  _plan_freeze       (17)  validate --freeze, settle replay-vs-discover
  _default_runner    (13)  bind --agentsview-timeout to the default runner
  _scan_refs         (44)  the parse loop -> Reducer + corpus fingerprint
  _freeze_note       (11)  the freeze provenance line
  _no_sessions_lines (32)  the empty-selection report, census notes included

_resolve_corpus() 141 -> 57, by lifting out the census chain it was mostly made
of. `_replay_census` (63) holds the four distinct ways a frozen manifest fails
to supply a usable denominator -- never recorded, recorded-but-failed, missing
its population filter, measured over a different population -- each still named
as itself. `_no_denominator` collapses the `AgentCensus(totals={},
archive_total=0, ...)` those four branches repeated. `_write_freeze` (20) takes
the write-once path.

Pure restructuring: every comment moved with the code it explains, and no
control flow, disclosure string, or exit code changed. `freeze_note` is now
computed at the render call instead of before the early return -- same inputs,
same value, and the early-return path never used it.

Gate: ruff clean, mypy --strict clean, pytest 748 passed / 2 skipped
(unchanged), and the repo's own complexity gate reports 0 errors / 0 warnings
against origin/main.

Package functions over 100 lines: 12 -> 10.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant