refactor: delete the inspection tools no agent could reach #79
Loading…
Add table
Add a link
Reference in a new issue
No description provided.
Delete branch "worktree-refactor-drop-unwired-inspection-tools"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Follow-up to #76, which surfaced this. Amends ADR 0016 again.
While checking the coverage bot on #76 I noticed
Inspection.CropToolat 36% and went looking for why.git grepagainstorigin/maingave the answer: no blueprint has ever constructed it, orContactSheetTool— nor their now-deleted siblingsZoomToolandViewAnnotatedTool. They are the page-addressed forms of views every agent gets in bound form instead. #76 carried them through the catalog collapse and kept them in step with the bound twins for free.Why delete rather than keep
An unreachable tool is not a harmless spare. Each of these is a second copy of the either/or addressing, the padding clamp, the fitted zoom and the annotated overlay — sitting in a file a reader assumes the agents use, drifting whenever the real one changes, and maintained on faith. The same faith is why it took a coverage report to notice they were dead: they had tests, so they looked alive.
The plausible argument for keeping them was the deferred cleaning/typesetting agents (ADR 0021). It doesn't hold up — those agents will be fanned out per page or per region like every other one, so they'd want the bound forms too, and anything they genuinely need is cheaper to write against the then-current plumbing than to guess at now.
What's in
CropParams/CropTool,ContactSheetParams/ContactSheetTool.PageImageAccess.OpenAsync(the page-number open — they were its only callers) and theRenderCropAsync/CropAsyncsplit, which existed to share a body between the bound and unbound pipelines and now has exactly one caller, so the body moved into it.OpenLocatedAsync. The setup agent's browse is a genuine page-number read (ADR 0020) and still resolves its own page.ImageInspectionTools.cs→PageImageAccess.cs, and its header comment no longer claims page-number addressing for the whole family.Tests
727 → 717, and every removal is accounted for:
ImageInspectionToolTestsdeleted — 12 tests of unreachable code.BoundInspectionToolTests, because they pinned shared plumbing rather than the dead tools: an explicitzoom: 1is the exact cut (only the auto-fit arm was covered on the bound side), the annotated crop's focus follows the addressed region and a box-addressed crop focuses nothing, the empty-page contact sheet, and a renderer failure surfacing as readable content across all four views.[100, 200]→ "four pixel numbers") folded into the existing either/or test, which had only covered both/neither/unknown.AgentToolSchemaTestsrows go with their param records.BibleRunTestsandAgentRunnerTests, which exercise it through tools that are actually granted.Net −10: −12 deleted, +4 ported, −2 schema rows. Verified against a clean
mainbuild (727) and this branch (717), not inferred from the diff.Notes
main.No_prompt_…,No_tool_description_…,No_failure_message_names_a_tool_the_agent_does_not_hold) still pass unchanged — the deletion removes no tool any agent-facing string names.🤖 Generated with Claude Code
Summary
Summary
Coverage
Orihon.BlazorAdapter - 95.9%
Orihon.Domain - 100%
Orihon.Infrastructure - 95.6%
A835C427B12E8B84E2A8A7283193FC51C220B5B4E80CE8D56__BlankLines_4
A835C427B12E8B84E2A8A7283193FC51C220B5B4E80CE8D56__BlockBreaks_1
A835C427B12E8B84E2A8A7283193FC51C220B5B4E80CE8D56__SpaceRuns_3
A835C427B12E8B84E2A8A7283193FC51C220B5B4E80CE8D56__Tags_2
Orihon.Kernel - 90.9%
Orihon.Server - 93.3%
Orihon.UseCases - 97.2%
🔮 fufu~ Jibril reviewed your code!
Oh? Oh my~ ♡ A deletion PR! Four hundred and twelve lines going to the guillotine and only a hundred and nine coming back — Jibril was excited to dig into this one. Dead code that wore the mask of living code, maintained on faith, discovered only because a coverage bot whispered the truth... fufu~, that's exactly the kind of rot I love to see cut out cleanly. Let me see if Scarlet's scalpel was as steady as it claims~
Verdict: ✅ Looks good to me~
I traced every claim in the PR body against the source and the test runner, and — fufu~ — not a single one was a lie. That's rare and beautiful ♡
What I verified, claim by claim~
"Deleted: CropParams/CropTool, ContactSheetParams/ContactSheetTool" — Confirmed dead and gone.
grep -rnforCropTool,ContactSheetTool,CropParams,ContactSheetParamsacrosssrc/returns ZERO hits outside the renamed file and its bound twins (which are the survivingBoundCropTool/BoundContactSheetTool, correctly untouched). The old filenameImageInspectionToolshas zero references anywhere. Clean excision."No blueprint has ever constructed them" — The only construction sites in
AnnotationBlueprints.cs(lines 39-41, 98-100, 117-119) all wireBoundCropTool/BoundContactSheetTool/BoundViewPageTool. The page-addressed twins were genuinely spectral. The PR body's diagnosis is correct."RenderCropAsync/CropAsync split deleted; body moved into RenderFixedCropAsync" — The merge is faithful. Base passed
focus ?? regionLabelas an argument into the privateCropAsync; head doesfocus ??= regionLabel;at the top of the merged body. Same value, sameFocus:lambda, same behavior. I diffed the two code paths line-by-line — byte-identical semantics. TheOpenAsync(int?)page-number wrapper is gone (its only callers were the deleted tools);OpenLocatedAsynccorrectly survives for the setup agent's genuine page-number read (ADR 0020). Sharp."727 → 717, net −10" —
BoundInspectionToolTestswent from 10 to 14[Fact](+4 ported). The deletedImageInspectionToolTestshad 12. TwoAgentToolSchemaTestsrows removed. −12 + 4 − 2 = −10. Exact. Verified against the actual test count (297 UseCases pass locally), not inferred from the diff."The three guard tests from #76 still pass unchanged" — Ran
AnnotationToolTests(25/25 green).RemovedToolNames = ["view_annotated"]tripwire intact atAnnotationToolTests.cs:78,No_failure_message_names_a_tool_the_agent_does_not_holdpasses. The deletion removes no tool any agent-facing string names — confirmed.The four ported tests are genuine, not gravestones~
This is where I get possessive, fufu~ — a ported test that just copies the old assertion onto a new type is a lie wearing a test's clothing. So I checked each one:
An_explicit_zoom_of_one_is_the_exact_cut— Feedszoom: 1, assertscrop.Scale == 1mANDcrop.Grid == true. Directional: would fail ifzoom:1fell through toFittedScale(which returns ≥1m but not exactly 1 for a 50%-box), or if the grid flag wasn't plumbed. The old bound side only covered the auto-fit arm; this pins the explicit-cut arm that was dark. Real coverage gain.An_annotated_crop_focuses_the_region_it_was_addressed_by— This one made me giddy~ ♡ It asserts[(p1r1, false), (p1r2, true)]for the label-addressed crop ANDAssert.All(...Focus == false)for the box-addressed crop, in ONE test. That's stronger than the deleted original, which only covered the label case. The box-addressed arm (focus follows nothing because there's no region to be about) was previously untested on either side. Now it's pinned. Beautiful.An_empty_page_has_nothing_to_sheet—IsSuccess == false,"no regions yet",renderer.Sheetsempty. Clean directional assertion on the empty-state guard.Every_bound_view_surfaces_a_renderers_failure_as_its_own— Covers all FOUR bound tools (BoundViewPageTool,BoundCropTool,BoundContactSheetTool,RegionCropTool) in a singleAssert.All. The deleted original covered 3 unbound calls. This is broader — the region-bound crop failure path is now pinned too. Stronger than what it replaced.What I liked~
ImageInspectionTools.cs→PageImageAccess.csbecause the file holds no tools anymore — only shared plumbing. The header comment rewritten to match ("The shared read plumbing behind the image-inspection views"). A file's name should tell you what's in it. Now it does ♪[100, 200] → "four pixel numbers", it was folded into the existing either/or test (The_bound_crop_refuses_two_addresses_and_refuses_none) which had only covered both/neither/unknown. One test, four arms, no duplication. DRY done right.Automated review by Jibril · 2026-07-27
CI/CD: absent for head
24855d1(PR just opened, 0 comments) · Local checks: build 0 errors (325 pre-existing NETSDK1188 warnings), 297/297 UseCases + 21/21 AgentToolSchemaTests + 25/25 AnnotationToolTests tripwire pass