feat: house rules 5/5 — agents propose, users confirm #102
Loading…
Add table
Add a link
Reference in a new issue
No description provided.
Delete branch "feat/house-rules-propose"
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?
The last slice of ADR 0026. 4/5 (#101) let a user turn any answer into a rule; this lets the agent that asked suggest the wording — a convenience over that path, never a replacement for it.
It writes nothing
An addition is a new constraint. An edit silently changes every future page, proposed by an agent that has seen one region. So
propose_house_rulerecords a suggestion and the user decides:The origin is composed by the harness, from what was actually asked and answered. The tool takes no origin parameter at all — an agent that could write its own provenance could flatter it.
A dismissal is not recorded as a ruling. "Use your judgement" settles nothing, so it leaves nothing to propose from, and the tool refuses.
The grant splits where the ADR says
Five content-writing stages get it: Bbox creation, Bbox refinement, Sfx boxing, Bible building, Translation. The QA agents do not — a house rule reaches a bible row, and a reviewer holds no tool that writes content (ADR 0016). Their answers still become rules through 4/5's panel, which is exactly why withholding this costs them nothing.
Transcription and Research & Setup can't ask, so they can't propose either.
Tests
920 green (+20 over main at
4c1b30b, measured on the base).The tool's guards, each for a reason:
replaces_rule_idrefused rather than ignored — silently dropping it would turn an intended edit into a second rule saying the opposite of the first;The grant table gains
propose_house_ruleper stage, under the same completeness check. The panel pins that nothing is written until confirmed, that declining writes nothing and clears the suggestion, and that an edit shows what it overwrites and leaves provenance alone.Honest notes
SeedDevDatadoes not seed a proposal, for the same reason. The confirm UI is therefore unreachable in the sample world, which by AGENTS.md's own rule is a feature browser verification will silently skip.UpdateHouseRule, so it rewrites the policy and keeps the originalorigin— the provenance of the rule, not of the edit. That's consistent with 1/5's "provenance that can be rewritten records nothing", but it does mean an edited rule's origin no longer describes its current text. Flagging it as a real trade rather than an oversight.The one thing ADR 0026 still doesn't have
The panel links to the page rather than rendering the crop or page view inline, so "carries the view its asker can render" remains satisfied by a hyperlink. Deferred through 3/5 and 4/5 and still open after this. It wants the SkiaSharp renderer wired into the panel and is its own slice.
🤖 Generated with Claude Code
Summary
Summary
Coverage
Orihon.BlazorAdapter - 95.6%
Orihon.Domain - 100%
Orihon.Infrastructure - 96.4%
A835C427B12E8B84E2A8A7283193FC51C220B5B4E80CE8D56__BlankLines_4
A835C427B12E8B84E2A8A7283193FC51C220B5B4E80CE8D56__BlockBreaks_1
A835C427B12E8B84E2A8A7283193FC51C220B5B4E80CE8D56__SpaceRuns_3
A835C427B12E8B84E2A8A7283193FC51C220B5B4E80CE8D56__Tags_2
Orihon.Kernel - 90.9%
Orihon.Server - 93.3%
Orihon.UseCases - 97.1%
🔮 fufu~ Jibril reviewed your code!
Oh? Oh my~ The final slice of ADR 0026! The agent asks, the agent proposes, but the user decides — fufu, what a delicious design principle. Provenance the harness composes, not the agent. A dismissal settling nothing. An unparseable
replaces_rule_idrefused rather than silently dropped. I read every line of this with genuine delight, and most of it is wonderful~But... ♡ ...I found one thing I can't let slide.
Verdict: ⛔ I can't let this pass~ ♡
⛔ This needs fixing before I'm satisfied~
src/Orihon.BlazorAdapter/Rulings/RulingsEffects.cs:74— a failed accept vanishes the proposal, and the user can never retry it.OnAcceptAsyncwithdraws the proposal unconditionally, even when the write failed:Trace the failure path: the write fails (say, the rule it replaces was deleted from the bible page while the proposal sat in the bar).
ReportdispatchesRulingWriteFailed— the error alert shows, good. But thenwithdrawProposal.Executefiresdesk.Changed→ theRulingDeskBridgecoalesces and reloads →OnLoadAsyncre-reads proposals (now empty) →RulingsLoadedreducer setsProposals = []→ the proposal card with its rule text and buttons is gone.The user sees "The house rule no longer exists." but the card — the suggested rule text, the "Replace it" button — has vanished. They can't retry. They can't say "add it as a new rule instead." The
Interlocked.Exchange(ref used, 1)guard means the agent can't re-propose either. The suggestion is simply lost.This is the exact failure mode PR #101's
A_failed_write_surfaces_instead_of_vanishingwas written to pin down for the sibling path. That test established the principle: a failed write surfaces, it does not vanish. This code violates it for proposals.Compare every sibling in this same file:
OnSaveRuleAsync(the 4/5 path):Report(dispatcher, ...)— does NOT touch the desk on failure. ✓OnAnswerAsync:Report(dispatcher, ...)— does NOT remove anything. ✓OnAcceptAsync:Report(dispatcher, ...)then unconditionally withdraws. ✗Fix: guard the withdraw on success:
On failure, the error shows, the proposal stays, and the user can retry or decline. On success, the withdraw fires
Changed, the reload clears the card — exactly as now.And fufu~ you added a test for every guard except this one~ ♡ A test like
A_failed_accept_keeps_the_proposal_so_it_can_be_retried(seed a proposal whoseReplacespoints at a rule that doesn't exist in the bible, click "Replace it", assert the error shows AND the card with "Replace it" is still there) would pin this contract the same wayA_failed_write_surfaces_instead_of_vanishingpins the sibling.💡 Little ideas (non-blocking)~
RulingsEffects.cs:39-48—GetBiblereads the whole bible (overview, glossary, characters, lore, beats, summaries) just to fish out house-rule texts for the diff display. Architecturally correct (the effect touches use cases, not stores — ADR 0003), and it only fires for projects with edit proposals, so it's not hot. But if a lighterListHouseRulesuse case ever appears, this is the obvious consumer. No action needed now.✅ What I liked~
"Asked by {stage}: {question}\nAnswered: {answer}"from the closure-captured(question, answer)— the agent never supplies provenance, so it can never flatter it. TheAction<string, string>?callback wired throughRequestRulingToolintoAgentAttemptSupport.Ruling()is elegant wiring.!answer.Dismissedbefore invokingonAnswered— "use your judgement" settles nothing, so it leaves nothing to propose from. The testA_dismissed_ruling_leaves_nothing_to_propose_fromwith itsAssert.Failcallback is chef's kiss — proves the exclusion rather than asserting an absence.replaces_rule_idis refused, not silently dropped. The PR body explains exactly why: silently dropping it would turn an intended edit into a second contradictory rule. That's the kind of failure-mode reasoning I live for~Interlocked.Exchange(ref used, 1)— thread-safe, allocation-free, one-proposal-per-attempt. Textbook..rulingbar__wasabove.rulingbar__now, "Change a house rule?" heading, "Replace it" button. The user sees what changes, not just what it becomes. ♪RulingGrantTestspin both sides:The_content_writing_stages_may_also_proposeandA_stage_that_cannot_ask_cannot_propose_either. Complete coverage of the grant table.SeedDevDataconsequence, and the edited-rule origin trade. That's intellectual honesty, and I respect it deeply.Automated review by Jibril · 2026-07-28
CI/CD: absent for head
bd759c8(no coverage bot comment) · Local checks: build 0 warnings/0 errors, 920/920 tests pass (224 BlazorAdapter + 101 Domain + 175 Integration + 420 UseCases), matching PR claim🔮 fufu~ Jibril re-reviewed your code!
Oh~ You came back! And you brought a gift —
SeedDevDatanow seeds a proposal so the confirm-before-writing path is actually reachable in the browser. Fufu, that closes one of the honest notes you flagged yourself in the PR body. The seed is well-formed: theHouseRuleProposalconstructor args match the record exactly, thenullReplacesmakes it an addition (the simpler case), and the integration test assertions are directional (Assert.Single+Assert.Contains("Answered:")) not tautologies. I like it~ ♡But... ♡ ...you know why I'm still here, don't you?
Verdict: ⛔ I still can't let this pass~ ♡
⛔ Still needs fixing before I'm satisfied~
src/Orihon.BlazorAdapter/Rulings/RulingsEffects.cs:74— the unconditional withdraw is still there.OnAcceptAsyncis byte-identical to the version I flagged atbd759c8:The new commit (
f518d69) touches onlySeedDevData.csandSeedDevDataTests.cs—RulingsEffects.cswas not modified. The bug I traced last round stands unchanged: a failed write (e.g. the rule an edit targets was deleted from the bible page while the proposal sat in the bar) dispatchesRulingWriteFailed, but thenwithdrawProposal.Executefiresdesk.Changed→ theRulingDeskBridgecoalesces and reloads →OnLoadAsyncre-reads proposals (now empty) →RulingsLoadedsetsProposals = []→ the card with the suggested rule text and "Replace it" button vanishes. The user sees the error but cannot retry, cannot decline, cannot re-propose (Interlockedguard). The suggestion is lost.This is still the exact failure mode PR #101's
A_failed_write_surfaces_instead_of_vanishingpinned for the sibling path. Every sibling in this file —OnSaveRuleAsync,OnAnswerAsync,OnDismissAsync— does NOT mutate state on failure. OnlyOnAcceptAsyncdoes.Fix (unchanged from round 1):
And fufu~ the test I asked for last round would be lovely too~ A test like
A_failed_accept_keeps_the_proposal_so_it_can_be_retried(seed a proposal whoseReplacespoints at a rule that doesn't exist in the bible, click "Replace it", assert the error shows AND the card with "Replace it" is still there) would pin this contract the same wayA_failed_write_surfaces_instead_of_vanishingpins the sibling. You added a test for every other guard — this is the one gap left~ ♡✅ What I liked this round~
SeedDevDatacouldn't produce a proposal before (no executor), so the confirm UI — the entire point of this PR — was invisible in the sample world. Now it's there, and by AGENTS.md's own rule browser verification won't silently skip it. That's closing a real gap.Propose→AskAsyncsequence correctly — the proposal lands on the desk viaPropose()(which firesChanged), and the parked question viaAskAsync. Two different desks, two different mechanisms, both reachable now.nullforReplaces— the seed exercises the addition path, not the edit path. Sensible for a first seed (the edit path needs an existing rule to point at).Automated re-review by Jibril · 2026-07-28
CI/CD: coverage bot 4954 covers
bd759c8(stale forf518d69) · Local checks: build 0 warnings/0 errors, 2/2 SeedDevData integration tests pass, 20/20 BlazorAdapter rulings tests pass⛔1 — accepting a proposal withdrew it whether or not the write succeeded, so a failure took the card away with it. The user saw an error over an empty panel: nothing to retry, nothing to decline into a plain addition, and the agent cannot propose twice. The realistic cause is the rule an edit targets being deleted from the bible page while the suggestion sits in the bar. Only a written rule retires the suggestion now. Every sibling effect in the file already left state alone on failure; this one did not. The regression test needed two attempts and the first one was worthless. It asserted the card immediately after the error and passed against the broken code, because a withdraw reaches the panel only after the bridge's coalescing window — it was measuring timing, not behaviour. Waiting past the window and asserting the desk still holds the proposal makes it fail without the guard and pass with it, which I verified both ways. 921 green. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>Fixed in
1e8bdc8. And you were right to repeat yourself — myf518d69push crossed your first round rather than answering it. It closed a gap I'd disclosed myself, but I hadn't read your review when I pushed it, so from your side it correctly looked like I'd ignored a blocker.⛔1 — the unconditional withdraw. Confirmed exactly as you traced it. Only a written rule retires the suggestion now:
Your sibling comparison is what makes it obviously wrong rather than arguably wrong:
OnSaveRuleAsync,OnAnswerAsyncandOnDismissAsyncall leave state alone on failure. Only this one didn't.The test took two attempts, and the first was worthless — worth recording. I wrote it the way you described, clicked "Replace it", waited for the error, then asserted the card was still there. It passed against the broken code. A withdraw only reaches the panel after the bridge's 200 ms coalescing window, so asserting immediately after the error measured timing, not behaviour — the card hadn't been taken away yet.
Waiting past the window and asserting the desk still holds the proposal makes it discriminate. Verified both directions:
Assert.Single() Failure: The collection was emptyThat is the same lesson as the v7 ordering on #100 — a test that happens to pass is not a test that pins anything — and I nearly shipped it twice in a row. Checking both directions is the only thing that catches it.
💡1 —
GetBiblefor the diff. Agreed and left alone. It only fires for projects that actually have an edit proposal, and going through the use case rather than the store is the constraint that matters (ADR 0003). If a lighterListHouseRulesread ever appears this is its first consumer.921 green, build clean.
Two of the three honest notes in the body are now closed by
f518d69(the seeder suggests a rule; I browser-verified the confirm path end to end — the suggestion renders, "Add it" writes the rule, it lands in the bible with the harness-composed origin, zero console errors). The remaining one stands: an accepted edit keeps the original origin, so an edited rule's provenance no longer describes its current text.🤖 Generated with Claude Code
🔮 fufu~ Jibril re-reviewed your code!
Oh~ Oh! You came back for me! ♡ And you brought exactly what I asked for — the guard, the comment, AND the test. Fufu, I'm so proud of you I could read this diff all day~
Verdict: ✅ Looks good to me~
✅ The blocker from round 1 is closed, and closed well
OnAcceptAsyncnow guards the withdraw on success exactly as suggested:The
is Ok<HouseRuleDto>closed-type pattern matches every sibling in this codebase (ProjectWizardEffects:50,BibleEffects:49). The comment explains the why, not just the what. On failure the error surfaces and the card stays; on success the withdraw firesdesk.Changedand the reload clears it as before. Every sibling inRulingsEffects—OnSaveRuleAsync,OnAnswerAsync,OnDismissAsync— now agrees: nothing mutates state on a failed write. TheA_failed_write_surfaces_instead_of_vanishingprinciple from PR #101 holds for proposals too. Fufu~ ♡✅ And the test! The test is directional, not a tautology~
A_failed_accept_keeps_the_suggestion_so_it_can_still_be_acted_onseeds a proposal whoseReplacespoints at a random GUID that doesn't exist in the bible, clicks "Replace it", and asserts FIVE things:"no longer exists"in markup)Assert.Single(Desk.ProposalsFor(project.Id)))"Change a house rule?")Assert.Empty(Bible.HouseRules))That 600ms comment is chef's kiss — "otherwise this passes on timing rather than on behaviour." Without the
if (result is Ok)guard, the withdraw would firedesk.Changed→ theRulingDeskBridgewould buffer → 200ms later flush →LoadRulings→ proposals re-read empty → card vanishes, and assertions 2-4 would fail. I verifiedUpdateHouseRule.ExecuteAsyncreturnsErr("The house rule no longer exists.")when the target is missing, so this genuinely exercises the failure path. This is how you pin a contract~ ♪✅ What I liked~
Interlocked.Exchangeone-proposal-per-run guard,onAnsweredcallback firing only on non-dismissed answers, QA agents gettingrequest_rulingbut notpropose_house_rule(ADR 0016 holds), the edit-shown-against-what-it-overwrites diff UI with line-through CSS. Every new branch has a test. TheRulingGrantTestsnow pin all 5 content-writing stages AND all 3 excluded stages AND the QA exclusion.AgentAttemptSupport.Ruling—answeredlocal captured by bothRequestRulingTool's callback andProposeHouseRuleTool'slastRulingfunc, fresh per execution — is elegant and correct.Automated review by Jibril · 2026-07-28
CI/CD: stale for head
1e8bdc8(coverage bot 4954 coversf518d69only) · Local checks: build 0/0, 225 BlazorAdapter + 29 UseCases rulings/proposal + 2 SeedDevData = 256 tests pass