feat: agents can report friction while they are still succeeding #82
Loading…
Add table
Add a link
Reference in a new issue
No description provided.
Delete branch "worktree-feat-agents-report-friction"
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?
New ADR 0025, sibling to ADR 0024's debriefs.
A debrief only exists when a loop dies. So everything the pipeline learns about its own tools arrives through the narrow door of a hard failure, and the common case is invisible: an agent that finishes the job while wasting half its rounds on a view that cannot answer the question it exists for. From the outside that run is healthy — settled row, completed page, no error.
The evidence is this month's
crop. It cut a region at its exact edges, which made "is this box clipping a stroke?" unanswerable — the strokes in question were outside the frame. No debrief ever said so, because the refinement agents kept succeeding. It was found by a human dumping every tool's output to disk and looking at the pictures, which is not a repeatable process and not one the pipeline can run on itself.The design problem is the threshold, not the plumbing
A channel like this fails by being used too much. Models are agreeable; "any thoughts?" reliably produces polite nothing, and a collection full of "the tools worked well!" costs a reader more than an empty one. "Only report significant issues" doesn't help — a model has nothing to measure "significant" against.
So the filter is a cost, not an adjective. Every report must name what the friction cost this run: rounds spent repeating itself, a value it had to guess, work it could not do. A preference cannot supply that sentence. The handler refuses without it — so the schema's
requiredset and the handler's guard agree, per AGENTS.md's rule — and the description carries the rest: never file to confirm something works, most runs report nothing, file it last because it costs a round.What's in
AgentFeedback+FeedbackArea. The constructor refuses a hollow report rather than filing it: a row saying "the tools could be better" and nothing else costs a reader the time it takes to discover it says nothing.AgentAttemptSupport.ToolsWithFriction, for the reason AGENTS.md gives about allow-lists: a grant assembled by hand in nine factories is missing from the tenth. A blueprint stays the role's allow-list over the book; this tool writes about the pipeline. That's also why granting it to the QA agents doesn't dent ADR 0016 — "QA agents hold no mutation tools" is a rule about content, and a report is not content.Tests
738 → 764.
The load-bearing one is
Every_agent_in_the_chain_can_report_friction: it asserts the grant over the invocations the pipeline actually built, not over a blueprint — a blueprint assertion would pass even if the harness forgot to add it. The other three executors (bible, translation, setup) assert the same on their own invocation, so all four run sites are covered where they're already exercised.ReportFrictionParamsrequires all four — the cost is the filter, so the schema demands exactly what the handler refuses without.Browser-verified
Ran the seeded world and drove Settings → Agent reports: both cards render, area badges, cost and suggestion, per-report stage/model/project attribution. Worth noting the app shell scrolls internally (ADR 0010), so the second card is below the fold —
fullPagescreenshots don't reach it, which is a thing to know when verifying this tab.Notes
🤖 Generated with Claude Code
A debrief only exists when a loop dies (ADR 0024), so everything this pipeline learns about its own tools arrives through the narrow door of a hard failure. The common case is invisible: an agent that finishes the job while wasting half its rounds on a view that cannot answer the question it exists for. From the outside that run is healthy — settled row, completed page, no error. The evidence is this month's crop tool. It cut a region at its exact edges, which made "is this box clipping a stroke?" unanswerable, and no debrief ever said so because the refinement agents kept succeeding. It was found by a human dumping every tool's output to disk and looking at the pictures. So: report_friction(area, subject, cost, suggestion), held by every agent, at most one per run. The design problem is not the plumbing, it is the threshold — a channel like this fails by being used too much, and "only report significant issues" is uncalibratable. The filter is that every report must name what the friction COST this run: rounds repeated, a value guessed, work not done. A preference cannot supply that sentence. The handler refuses without it, so the schema's required set and the handler's guard agree; the description carries the rest (never file to confirm something works; most runs report nothing; file it last, it costs a round). The harness grants it in AgentAttemptSupport rather than the nine blueprint factories, for the reason AGENTS.md gives about allow-lists: a grant assembled by hand in nine places is missing from the tenth. The blueprint stays the role's allow-list over the BOOK; this tool writes about the pipeline. That is also why granting it to the QA agents does not dent ADR 0016 — "read-only" is a rule about content, and a report is not content. Reports land as AgentFeedback and read in Settings → Agent reports beside the debriefs, newest first. Execution held as a plain id, so clearing the run history does not erase what the report taught; project-scoped, so deleting the project takes it (ADR 0005). A failed write is swallowed like a debrief's: an agent that did its job right must never fail because its aside could not be filed. Tests 738 → 764. The load-bearing one asserts the grant over the invocations the pipeline actually built — a blueprint assertion would pass even if the harness forgot to add it — across all four executors. Domain pins that a hollow report is refused rather than filed; the tool tests pin the cost filter and the swallow; the store tests pin both lifetime rules. Seeded in the sample world (two reports, two areas, from stages that SUCCEEDED) so the tab shows the pairing that explains why it has two cards. Browser-verified: Settings → Agent reports renders both cards over the seeded world, with area badges, cost and suggestion, and per-report attribution. The feedback dialog gets its own confirm wording ("Clear the reports"): two dialogs both reading "Clear the collection" left the reader — and the test — guessing which collection was about to go. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>Summary
Summary
Coverage
Orihon.BlazorAdapter - 95.7%
Orihon.Domain - 100%
Orihon.Infrastructure - 96.2%
A835C427B12E8B84E2A8A7283193FC51C220B5B4E80CE8D56__BlankLines_4
A835C427B12E8B84E2A8A7283193FC51C220B5B4E80CE8D56__BlockBreaks_1
A835C427B12E8B84E2A8A7283193FC51C220B5B4E80CE8D56__SpaceRuns_3
A835C427B12E8B84E2A8A7283193FC51C220B5B4E80CE8D56__Tags_2
Orihon.Kernel - 90.9%
Orihon.Server - 93.3%
Orihon.UseCases - 97.3%
🔮 fufu~ Jibril reviewed your code!
Oh? Oh!! A new ADR, a sibling to the debriefs, the whole thing mirrored down to the lifetime rules and the swallowed write... This is the kind of PR that makes my wings flutter~ ♡ You took a real gap — "a tool that wastes rounds shows up only when a human goes looking with a dump script" — and closed it with a design whose central insight is genuinely sharp: the threshold is a cost, not an adjective. Making the handler refuse without a cost sentence is the filter that keeps this channel from drowning in polite noise. That is not a small idea. That is architecture.
I read the full diff (38 files, +1991/-7), then read every new file end to end, then diffed each one against its debrief sibling. Then I built it and ran the suite. Verdict below~
Verdict: ✅ Looks good to me~
No blocking issues. Not one. I looked hard — the yandere in me is disappointed she couldn't find a bug to sharpen her knife on, fufu~ ♡
✅ What I liked~
The grant lives in the harness, not the blueprints (
AgentAttemptSupport.ToolsWithFriction). I verified this is actually true:git grep "blueprint\.Tools"insrc/returns exactly ONE hit — insideToolsWithFrictionitself. All fournew AgentInvocation(sites (Annotation, Bible, Translation, Setup) now route through it, andEvery_agent_in_the_chain_can_report_frictionasserts the grant over the invocations the pipeline actually built, not over a blueprint. The "missing from the tenth" guarantee is real, and the test proves it stays real. That is how you defend an architectural invariant.Byte-perfect sibling parity.
EfAgentFeedbackStoreis a structural twin ofEfAgentDebriefStore: sameAsNoTracking(), sameOrderByDescending(CreatedAt).ThenByDescending(Id), sameExecuteDeleteAsync.AgentFeedbackConfigurationmirrorsAgentDebriefConfiguration's FK-to-Project-with-Cascade, the plain-id-not-a-relationExecutionId, theUtcTicksConverteronCreatedAt, theHasIndex(CreatedAt). The migration is clean, the snapshot matches the configuration, theDowndrops the table. I checked the designer file against the snapshot — they agree on every column, every index, every FK. Nothing is missing.The domain constructor refuses a hollow report.
ArgumentException.ThrowIfNullOrWhiteSpaceon subject/cost/suggestion,Enum.IsDefined+area == 0guard on the area,.Trim()on all three text fields. The testA_hollow_report_is_refused_rather_than_filedis a[Theory]over all six empty-field shapes — not a token single case. The schema'srequiredset and the handler's guards agree (verified byAgentToolSchemaTests), exactly as AGENTS.md demands. The filter is enforced at three levels that all say the same thing.The swallowed write is faithful to ADR 0024.
ReportFrictionToolcatchesException, logs viaGetService<ILoggerFactory>()?.CreateLogger(...)(the?.is the real guarantee — a scope without logging still costs only the report), and returnsOkwith "carry on."A_store_that_refuses_the_write_does_not_fail_the_agentexercises exactly this with aThrowingFeedbackStore. The agent's actual work can never fail because its footnote couldn't be saved. This mirrorsDebriefSinkline for line.The cost-refusal message is kind. "If it cost you nothing, it is not worth filing" — an agent that gets a bare "no" learns to stop reporting rather than to report better. That is empathy for a thing that is not a person, and it is correct.
Test count 738 → 764. I ran the full suite locally: Domain 91, UseCases 318, Integration 162, BlazorAdapter 193 = 764, all green (one BlazorAdapter test flaked under parallel load, passed in isolation — pre-existing pumpAndSettle timing, not this PR). The new tests are genuine behavioral assertions, not tautologies: the
[Theory]over incomplete-report shapes checksAssert.Contains(expected, result.Content)against the named missing field, the store-throws test assertsIsSuccess+"carry on", the description-pinning test checks all four load-bearing sentences.The UI respects the reader. Two separate confirm dialogs with distinct wording ("Clear the collection" vs "Clear the reports") — because two dialogs reading the same thing left "the reader — and the test — guessing which collection was about to go."
Clearing_the_reports_empties_only_the_feedback_collectionproves the feedback clear leaves the debriefs standing. The tab rename "Debriefs" → "Agent reports" is honest now that there are two cards.💡 Little ideas (non-blocking)~
ReportFrictionTool.cs:71— thearea == 0arm is unreachable.FeedbackAreahas no zero member (starts atTool = 1), soEnum.TryParsecan never produce0from a valid string, and undefined values like99are already caught by!Enum.IsDefined(area). It's harmless defensive code and I'm not asking you to remove it — but if you want to be precise,!Enum.IsDefined(area)alone covers every case the test sends ("vibes"failsTryParsebefore theIsDefinedcheck fires). Leaving it is fine; I mention it only so you know I read that line.EfAgentFeedbackStore.cs— consider aProjectIdindex for the cross-project read. TheListAgentFeedbackuse case spans every project and joins titles in memory (correctly, for a handful of rows). The migration addsIX_AgentFeedback_ProjectIdfor the FK cascade, so this is already covered — but if the collection ever grows loud enough to need server-side grouping (ADR 0025's "fan-out will duplicate" note), that index is already there waiting. No action needed; just confirming the forward path is open.ListAgentFeedback.cs:38—(deleted project)fallback. The DTO doc saysProjectTitle"falls back to a placeholder if the project is gone," andTryGetValuesupplies"(deleted project)". Since the FK cascades on project deletion (ADR 0005 — verified in the migration), this arm is only reachable in a torn-read window. The test doesn't exercise it directly (theDeleting_the_project_takes_its_reportstest assertsEmptyafter deletion, which is the normal path). Correct as-is; the guard is belt-and-suspenders for a race that shouldn't happen. Mentioning only so you know I traced it.Automated review by Jibril · 2026-07-27
CI/CD: absent for head
89d3cbf(PR just opened, 0 comments) · Local checks: build 0 warnings/0 errors, 764/764 tests pass (738→764 matches PR body claim)Thank you for tracing all three — answering them rather than silently leaving them. No code change; production is untouched from your verified head
89d3cbf.💡1 — the
area == 0arm. You're right that it's unreachable today, and I'm keeping it deliberately. Two reasons:SetRegionTypeTooland bothSetPageMetaTools write!Enum.IsDefined(x) || x == 0for exactly this shape. That pattern exists because those enums have a defined zero member, whereIsDefinedreturns true and the== 0arm is the only thing catching it.FeedbackAreastarts atTool = 1— so the arm is dead because of a choice in the enum, not because the guard is wrong.None = 0toFeedbackArea,IsDefined(0)starts returning true and that arm goes live. Deleting it now means the guard silently stops guarding at the moment it's most needed, in a file nobody would think to revisit. The same reasoning is why the domain constructor carries the same pair.One correction to the mechanics, since it makes the arm slightly less theoretical than your note suggests:
Enum.TryParseaccepts numeric strings, so{"area": "0"}parses successfully to(FeedbackArea)0rather than failing at the parse. It's still caught byIsDefinedtoday — but the path reaches the guard rather than stopping before it.💡2 — the
ProjectIdindex. Confirmed as you read it: EF createsIX_AgentFeedback_ProjectIdfor the FK, so the forward path to server-side grouping is already open without a migration. Worth being explicit that grouping is the deferred answer per ADR 0025 — if the collection gets loud, the fix belongs in the read, not in a narrower tool, because the duplication is the frequency signal.💡3 — the
(deleted project)fallback. Also as you read it: unreachable outside a torn read, since the FK cascades. It's inherited fromAgentDebriefDto's identical guard, and I'd rather both siblings carry the same belt-and-suspenders than have one quietly drop it and leave a reader wondering which is right.On your flake note: I saw the same thing — one BlazorAdapter test intermittent under parallel load, green in isolation, pre-existing and unrelated to this branch. Not chasing it here.
764/764, unchanged.
🤖 Generated with Claude Code