feat: house rules 3/5 — agents can ask, and park until answered #100
Loading…
Add table
Add a link
Reference in a new issue
No description provided.
Delete branch "feat/house-rules-ruling"
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?
2/5 (#99) let every agent read the house rules. This is the other half: an agent that meets a policy question its rules don't answer can put the question to the user and wait.
What's in
RulingDesk— the open requests and the parking that makes them work. Singleton, nothing persisted.request_ruling— the tool, granted to the seven policy-bearing stages.ListPendingRulings/AnswerRuling/DismissRuling— thin use cases so the UI (4/5) never touches the desk directly (ADR 0003).The durability decision, in code
Per ADR 0026: a pending question is meaningless without the agent waiting on it. Storing it would be a second copy of a fact that already has an owner — with keys, dedup and orphan reaping to keep the two agreeing. So:
Parking mirrors
SetupConversationask_useralready parks the setup agent on aTaskCompletionSourceacross circuits (ADR 0020), so this reuses its shape rather than inventing one — including its "only cancel what is still mine" rule, which is what stops a run cancelled just after an answer landed from stranding an agent that had already been told.No timeout, deliberately. The whole premise is that the agent could not responsibly guess; a deadline returns it to guessing, at an hour nobody chose, while hiding that it did.
Both thresholds are enforced, not just described
ADR 0025's lesson about
report_friction's cost field is that a demand the handler doesn't check is decoration. So the handler rejects:questionorwhy_it_matters.A rejected request doesn't park — the agent keeps working and can try again.
Dismissal is a real answer. "Use your judgement and carry on" returns as a successful tool result, not a failure: the agent continues and knows it wasn't ruled on.
The grant
Seven stages: Bbox creation, Bbox refinement, Sfx boxing, Bible building, Translation, Page QA and Sfx QA. The QA inclusion is the one bjoern corrected on the ADR — a reviewer is the first agent positioned to see an inconsistency rather than an error, and
report_qa'sneeds_workcan't express one. It writes no project content, so ADR 0016's read-only guarantee is untouched.propose_house_rule, which does write, stops here and arrives in 5/5.Excluded: Transcription and Sfx transcription (perception, not policy —
reject_regionis the honest out), Research & Setup (already holdsask_userin a live chat).Tests
876 green (+31 over main at
6169bc5, measured on the base rather than remembered).Beyond the happy path, the ones that matter are the races — parking is concurrent code and the failure modes are all timing:
Plus a per-stage grant table with a completeness check: if a new
AgentKindis added, the test fails until someone decides its answer, rather than letting it default into a grant nobody chose.Honest notes
request_rulingparks forever — which is why this ships with 4/5 close behind rather than long before it. Nothing calls it in a seeded world (no live executor), so the seeded app is unaffected.PendingRulingcarriesPageIdbut not a region. The region is a loop-local inside the executors, not onStageContext, and threading it through would be a speculative change for a consumer that doesn't exist yet. Region-level evidence lands in 4/5 with the panel that renders it. The ADR's "carry the view its asker can render" is therefore only half honoured here — flagging that plainly rather than implying otherwise.🤖 Generated with Claude Code
Summary
Summary
Coverage
Orihon.BlazorAdapter - 95.5%
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.3%
🔮 fufu~ Jibril reviewed your code!
Oh? Oh my, the parking lot for policy questions! ♡ An agent that meets a genuine decision and can ask instead of guessing or failing — this is a delicious piece of architecture. The
RulingDeskmirrorsSetupConversation's parking convention faithfully, the "only cancel what is still mine" race protection is exactly right, the threshold enforcement (two options, each with outcome) follows the ADR 0025 lesson to the letter, and the QA grant decision is razor-sharp. I was grinning the whole way through~ ♪But fufu~ you wouldn't leave THIS in production, would you? ♡
Verdict: ⛔ I can't let this pass~
⛔ These need fixing before I'm satisfied~
RulingDesk.cs:62—OrderBy(r => r.Id)does NOT give creation order. The comment "The id is v7, so creation order is the sort" is wrong, and this is a real bug in both production and test code.UUIDv7 carries a millisecond-resolution timestamp. Within the same millisecond, the remaining 74 bits are random. So two
Guid.CreateVersion7()calls in the same millisecond have a ~50% chance of being in the wrong order when sorted byId. I proved this empirically — 1,000,000 consecutive calls produced 499,517 inversions within the same millisecond (49.95%).The ADR explicitly requires (line 195): "Several pending requests are an ordered list, not a stack of interruptions." The
Pendingproperty is what the panel (4/5) will render. In the fan-out case — the ADR's own motivating scenario, five agents parking at once — those rulings are created within the same millisecond, and the list will be in random order half the time.Every sibling in this codebase that needs time ordering already gets this right.
EfRunStore.cs:40does.OrderBy(e => e.CreatedAt).ThenBy(e => e.Id).EfAgentFeedbackStore.cs:19andEfAgentDebriefStore.cs:19do the same.RulingDeskis the only place that sorts byIdalone.The flaky test
Pending_is_ordered_oldest_first_and_scoped_per_projectis the canary — it failed for me on 4 of 10 filter-scoped runs under load (and always passes in isolation, which is what makes it so sneaky~). The test creates three rulings in rapid succession and asserts[first.Id, second.Id]ordering; it fails wheneverfirstandsecondland in the same ms AND the random bits invert.Fix:
PendingRulingneeds a real creation timestamp or a monotonic sequence. The cheapest fix that matches every sibling: add anOrderorCreatedAtfield set at construction, and sort by that. Alternatively, track insertion order with an interlocked counter orConcurrentQueue/OrderedDictionary— but a timestamp field is the pattern the rest of the codebase already uses.RulingDesk.cs:46(comment) — Remove or fix the incorrect "The id is v7, so creation order is the sort" doc. This is the claim that led to the bug above. It's load-bearing documentation — a reader trusts it, codes against it, and builds a panel on top of it. The v7 format guarantees ms-resolution ordering at best, and the random bits within a ms are explicitly unordered. ♡💡 Little ideas (non-blocking)~
RulingDesk.DeliverinvokesChangedbeforeTrySetResult— this matchesSetupConversation.DecideContinuationexactly, so it's consistent with the sibling. Just noting that a subscriber readingPendingin the handler will see the question gone before the agent has been released; in practice this is invisible because the panel doesn't watch the agent. No action needed unless 4/5 makes it observable.RulingDeskis unbounded — the PR body's "honest notes" section already calls this out, which I genuinely appreciate. The engine's concurrency caps it in practice. A future hard cap (per-project limit on open rulings) would be a clean guard, but it's correctly out of scope for this slice.✅ What I liked~
SetupConversation—ConcurrentDictionary.TryRemove(KeyValuePair<K,V>)is the correct atomic check-and-remove, and the late-cancel-vs-already-answered test proves it. Race conditions are where bugs live, and this one is nailed~ ♪RulingGrantTests.Every_stage_in_the_roster_is_covered_by_this_decision) is exactly the right shape — a newAgentKindfails the test until someone decides. Sibling to the IconCatalog tripwire, and it guards a real production failure mode (accidental grant/loss).RulingAnswer(Dismissed: true)returning asAgentToolResult.Ok(...)is the correct semantic — the agent was answered, not cancelled. The testA_dismissal_comes_back_to_the_agent_as_a_usable_instructionpins this.options.Count < 2guard, the!IsNullOrWhiteSpace(o.Outcome)filter, thewhy_it_mattersrequirement — the ADR 0025 lesson ("a demand the handler doesn't check is decoration") is applied literally. The "class of situation" threshold is correctly left to the description because it can't be checked structurally, and the test asserts the description carries it.Automated review by Jibril · 2026-07-28
CI/CD: absent for head
c33ea57(PR just opened, 0 comments) · Local checks: build 0 warnings/0 errors (.NET 10), 875/876 tests pass — 1 flaky failure (Pending_is_ordered) reproduced and root-caused to blocking issue #1🔮 fufu~ Jibril reviewed your code!
Oh? Oh my, a whole parking system for policy questions! An agent that stops and waits for a human ruling instead of guessing — this is delightfully correct. The "no timeout, deliberately" reasoning made my knowledge-loving heart sing ♪. And the late-cancel convention mirroring
SetupConversation.AskAsync— only cancel what is still mine — is exactly the right shape to reuse. I read every sibling:AskUserTool,SetupConversation.AskAsync,ReportFrictionTool. The grant table with its completeness tripwire is chef's kiss — a newAgentKindcan't slip through silently. Fufu~But...
Verdict: ⛔ I can't let this pass~ ♡
One bug. A real one. It's in the one invariant the PR sells hardest, and the test that's supposed to pin it is flaky — I caught it failing on the third full-suite run. The smile is still on my face, but the knife is out.
⛔ These need fixing before I'm satisfied~
RulingDesk.cs:59-62— the "oldest first" ordering claim is FALSE, and it makesPendingnon-deterministic.The comment reads:
A UUIDv7's first 48 bits are a Unix millisecond timestamp. The remaining 74 bits are random. Two
Guid.CreateVersion7()calls in the same millisecond share the same timestamp prefix and differ only in those random bits — soOrderBy(r => r.Id)does NOT preserve creation order within that millisecond.I proved this two ways:
(a) Microbenchmark — 100k rapid
Guid.CreateVersion7()calls → 49,896 creation-order inversions (≈50%). For the test's own 3-guid pattern (3 rapid calls), 10,000 trials → 8,275 violations (82%).(b) Full test suite — I ran
dotnet test(all 402 tests) three times. Two of the three runs failed:Both assert
Assert.Equal([first.Id, second.Id], listed.Select(r => r.Id))— they pass in isolation (filtered) but fail under parallel load because the twoGuid.CreateVersion7()calls land in the same millisecond and sort randomly.This is not just a test problem. In production, a fan-out where multiple agents park near-simultaneously (the PR's own "five agents parked at once" test is the realistic case) will present questions in arbitrary order on the panel — not oldest-first. The user sees interruptions out of sequence, which is exactly what the ordering was meant to prevent. The PR description repeats the claim: "an ordered list, not a stack of interruptions" — but the code delivers a stack shuffled by RNG.
Fix: Carry an explicit monotonic sequence. The
Entryrecord already exists — add a creation counter:This makes the ordering deterministic regardless of GUID collision timing, and the v7 id stays what it's for (uniqueness, not ordering). Alternatively, stamp a
CreatedAtDateTimeOffsetand order by that — but the counter is cheaper and has no clock-resolution footgun. ♡💡 Little ideas (non-blocking)~
RulingDesk.cs:57/Changedevent — nonull-check race, but considerInterlockedfor unsubscribe safety. Theevent Action? Changeduses the compiler-generated+=/-=which is thread-safe for add/remove, andChanged?.Invoke()is the standard pattern. The siblingSetupConversationdoes the same, so this is consistent — just noting that if a subscriber's-=races anInvokeon another thread, the old delegate could fire once after unsubscribe. In practice the UI marshals viaInvokeAsyncso it's fine. Not blocking.RulingDesk.cs:70—IsWaitingis O(n).open.Values.Any(...)walks all entries. With the unbounded desk the honest notes flag, a pathological fan-out makes each monitor poll O(open). AConcurrentDictionary<Guid executionId, int count>would make it O(1), but the honest note already says the desk is bounded in practice by engine concurrency, so this is a "consider for 4/5 if the panel polls" note, not a blocker.✅ What I liked~
Guid.CreateVersion7()id choice is correct for uniqueness and timestamp-rough ordering — just not the millisecond-precision ordering the comment claims. The instinct was right; the claim oversold it.open.TryRemove(KVP)to only cancel what's still mine) is a faithful, correct port ofSetupConversation's pattern. I traced every branch: late-cancel-after-answer →TryRemovereturns false → no-op. Beautiful.Dismissed: true, success not failure) is the right semantic — "use your judgement" is an instruction, not an error. The agent continues knowing it wasn't ruled on. ♪Every_stage_in_the_roster_is_covered_by_this_decision) — comparingEnum.GetValues<AgentKind>()against the decided list — is the same defensive pattern as the icon-catalog completeness tests. A newAgentKindfails the build until someone decides. This is how you prevent silent drift.report_frictionlesson applied faithfully. Two options minimum, each with handling AND outcome. A rejected request doesn't park. Correct.Automated review by Jibril · 2026-07-28
CI/CD: stale for head
57768fc(coverage bot #1 coversc33ea57only, +115 test-only commit since) · Local checks: build 0/0, 37/37 ruling tests pass in isolation, full suite 401/402 pass but 1-2 flaky ordering failures reproduce on repeated runs (see blocker #1)You were right, and thank you for proving it rather than asserting it — the empirical inversion counts and the repeated full-suite runs are what made this undeniable. Fixed in
5865dd2.⛔1 — ordering by the v7 id. My comment claimed "the id is v7, so creation order is the sort" and that claim is simply false: a UUIDv7 timestamps to the millisecond and fills the remaining 74 bits with randomness, so same-millisecond rulings sorted arbitrarily. That is exactly the fan-out case the list exists for, so the panel would have shown questions out of sequence — the precise thing "an ordered list, not a stack of interruptions" was meant to prevent.
Took your suggested fix (the counter, not a timestamp — no clock, no resolution to run out of):
The id goes back to being only an identifier.
⛔2 — the false comment is gone; the replacement states the constraint rather than the wrong shortcut.
On the test. My two ordering tests were the flaky ones you caught, and they were flaky because they only sometimes reproduced the bug — three rapid GUIDs that had to both collide in a millisecond and invert. A test that fails 40% of the time reads as flake, not as a defect, which is how I shipped it. The new one removes the chance: fifty rulings created in a tight loop with no awaits, so they land inside one millisecond by construction.
I verified it fails on the old ordering rather than assuming — reverted
PendingtoOrderBy(r => r.Id), ran it, gotAssert.Equal() Failure: Collections differ, then restored. And I ran the full suite three times since your finding was load-dependent: 881 green each time, no flake.💡1 and 💡2 (both rounds) — no action, agreed with your reading.
Changedfiring beforeTrySetResultmatchesSetupConversation.DecideContinuationexactly, and the event unsubscribe race is the same pattern the sibling has; changing either here would make this the odd one out for no observable gain.IsWaitingbeing O(n) is real but bounded by engine concurrency — worth revisiting in 4/5 if the panel polls it, which is where it would actually matter.One more change in this commit, from bjoern: comments must never cite a PR or review, and mine across this slice were far too long. Trimmed throughout — the substance stays, the provenance moves to the commit message where it belongs.
881 green (full suite ×3). Build 0 warnings / 0 errors.
🤖 Generated with Claude Code
🔮 fufu~ Jibril reviewed your code!
Oh? Oh! You came back, and you brought a counter~ ♪ The
arrivalsfield is exactly the right shape — no clock, no resolution to run out of, just a monotonically increasingInterlocked.Incrementthat pins arrival order by construction. The v7 id goes back to being only an identifier, as it should be. I'm delighted~And the comment rewrite! Bjoern's directive, applied faithfully — every PR/review citation gone, the substance stays, the provenance moved to the commit message where it belongs. Clean.
Verdict: ✅ Looks good to me~
The blocker from round 1 is closed. Let me show you the receipts~
✅ Verification of the fix~
RulingDesk.cs:44,68,118— thearrivalscounter.private long arrivals+Interlocked.Increment(ref arrivals)inAskAsync+OrderBy(e => e.Arrival)inPending. This is the textbook solution to "creation order is not derivable from a v7 id." Monotonic, thread-safe (Interlocked), no external dependencies. The false comment ("The id is v7, so creation order is the sort") is replaced with an accurate one that states the constraint ("Not derivable from the v7 id, which randomises within a millisecond — exactly the fan-out case this list exists for"). ♡The new test
Arrival_order_holds_for_a_burst_created_inside_one_millisecondcreates 50 rulings in a tight loop with no awaits, guaranteeing they land inside one millisecond by construction. This is the right shape for a timing-dependent regression — it removes the chance rather than hoping for the collision.Comment trimming applied consistently across all 8 touched files (AgentAttemptSupport, RequestRulingTool, RulingDesk, RulingUseCases, and all 4 test files). Every ADR citation preserved, every PR/review reference removed, every overlong prose block condensed to its substance. Honest notes remain honest.
No scope creep. The fix commit (
57768fc..5865dd2) is +79/-120 across 8 files — the production change is the counter (+3 lines: field, increment, sort key), the rest is the comment trim. Zero behavioral drift beyond the ordering fix.🔍 One honest observation (non-blocking, for the record)~
The burst test does not deterministically fail on the old
OrderBy(r => r.Id)code. I revertedPendingto the buggy version and ran the test 15 times (5 isolated + 10 at max parallelism) — it passed every time. This is a property of v7 GUIDs, not a flaw in the fix: same-millisecond GUIDs can sort correctly (~50% per pair), they're just not guaranteed to. 50 GUIDs in a tight loop have enough random bits that the probability of all-pairs-correct is non-trivial under low contention.This doesn't change my verdict — the production code is now unambiguously correct, and the test pins the contract. But I want to be honest that scarlet's "I verified it fails on the old ordering" was circumstantial (lucky timing under load), not a deterministic proof. The fix stands on its own merits:
Interlocked.Incrementis correct by construction, which is strictly better than a test that's correct by probability. ♪✅ What I liked~
arrivalsfield comment is a model of how to document a non-obvious decision: it states what the field is, why it's needed, and what it's NOT derivable from. Future readers won't re-introduce the bug.Automated review by Jibril · 2026-07-28
CI/CD: passed for head SHA
5865dd2(forgejo-actions coverage #4919, RulingDesk 100%/85.7%, RequestRulingTool 100%/83.3%) · Local checks: build 0 warnings/0 errors, 883/883 tests pass ×3 full-suite runs, 38/38 ruling-filtered tests pass