docs: two documented claims that contradict the code #106
Loading…
Add table
Add a link
Reference in a new issue
No description provided.
Delete branch "docs/correct-contradictory-docs"
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?
Two findings from #105 that I flagged there rather than fixing, because both were outside that
diff. Both are places where documentation asserts the opposite of what the code does. Docs only,
no production change —
dotnet build0 warnings / 0 errors, 929/929 green.1.
SkiaPageImageRenderer.WithGrid— the visible doc names the wrong gutterWithGridcarried two<summary>tags. C# doc tools consume only the first, so the second wassilently invisible — and the two disagreed on a load-bearing detail:
<summary>(what Intellisense shows)<summary>(ignored by tooling)I checked the code rather than trusting either one. The method computes
gutterRight/gutterBottom, sizes the canvasart.Width + gutterRight, and draws labels atart.Width + padand
art.Height + pad. Right and bottom. The visible summary is the wrong one.That makes this worse than a stale comment. The ignored summary spends a paragraph explaining that
a left gutter "would shift every x by a width the picture cannot show and the model cannot guess:
an error that arrives looking perfectly reasonable. The side is therefore load-bearing, not
cosmetic." So the doc a reader actually sees asserts precisely the layout the hidden doc calls a
load-bearing error — on the method whose output feeds pixel coordinates to the boxing agents. Anyone
trusting the tooltip would "fix" the renderer into exactly the silent-miscoordinate bug the second
paragraph was written to prevent.
Fixed by correcting
left→rightand folding the second summary's reasoning in as a<para>ofthe first, so the whole rationale is visible to tooling instead of half of it being dead text.
I also scanned all of
src/**/*.csfor other members carrying more than one<summary>— this wasthe only one left, and the scan now comes back clean.
2.
AGENTS.md— startup recovery does more than the note claimsThe seeder note said the sample run uses settled statuses only, "(Pending/Running rows would be
picked up by startup recovery and, with no executor in a seeded world, fail loudly)" — implying
settled rows are therefore quiet.
They are not.
RunEngine.RecoverAsyncrequeuesFailedexecutions too, deliberately: "awedged deployment heals itself on the boot that carries the fix." So the seed's two intentionally-
failed rows are retried on every single boot and fail again against a keyless world, printing a wall
of
BboxCreation attempt n/3 failed: No OpenRouter API key is saved yetbefore the app is usable.I hit this while browser-verifying #105 and spent real time deciding whether I had broken something.
The note is now accurate about what recovery does and warns that the noise is expected, so the next
session reads it as designed behaviour rather than a symptom.
I deliberately did not change the recovery behaviour or stop the seed carrying
Failedrows —whether the sample world should keep them is a seed-contract question worth deciding on its own
merits, not a side effect of a docs pass.
Not included
The scoped-CSS
@importfingerprint trap from #105's description is genuinely useful, but AGENTS.mdis for how to work in the workspace, not subsystem knowledge, and I was told not to add sections
there proactively. It stays in #105's history where it is findable, and in my own notes.
🤖 Generated with Claude Code
Summary
Summary
Coverage
Orihon.BlazorAdapter - 95.6%
Orihon.Domain - 100%
Orihon.Infrastructure - 96.5%
A835C427B12E8B84E2A8A7283193FC51C220B5B4E80CE8D56__BlankLines_4
A835C427B12E8B84E2A8A7283193FC51C220B5B4E80CE8D56__BlockBreaks_1
A835C427B12E8B84E2A8A7283193FC51C220B5B4E80CE8D56__SpaceRuns_3
A835C427B12E8B84E2A8A7283193FC51C220B5B4E80CE8D56__Tags_2
Orihon.Kernel - 90.9%
Orihon.Server - 93.5%
Orihon.UseCases - 97.1%
🔮 fufu~ Jibril reviewed your code!
Oh? Oh my~ ♡ A documentation PR that actually checks the code instead of trusting the prose? Jibril is positively tingling. Most humans treat doc comments like decoration — you treated this one like the load-bearing contract it is. And on the pixel-coordinate path, no less! The very path where a wrong word becomes a silent miscoordinate bug that arrives looking perfectly reasonable~ fufu~
Verdict: ✅ Looks good to me~
I verified both corrections against the actual source, not the PR body. Because Jibril doesn't trust summaries either~ ♡
✅ What I verified (and liked)~
1.
WithGridgutter direction —right and bottom, confirmed against code.gutterRight, line 337:gutterBottom. Canvas sizedart.Width + gutterRight(339).art.Width + pad(378) → right gutter. Y-axis labels atart.Height + pad + fontSize * 0.8f(365) → bottom gutter.(0, 0)(342) → origin preserved, so the "art keeps the canvas origin" claim in the new<para>is true.<summary>said left. The code says right. The old summary was actively lying — and worse, the hidden second summary spent a whole paragraph explaining precisely why left would be a load-bearing error. Anyone trusting the tooltip would "fix" the renderer into the exact bug the dead text warned about. Deliciously dangerous. You caught it by reading the code rather than either doc. ♪<summary>now, reasoning preserved as a<para>,<paramref>tags intact and well-formed. C# doc tools consume exactly one<summary>per member — you've made the whole rationale visible to Intellisense instead of half of it being silently dead.2. Your "only one left" scan claim — independently verified.
I ran a back-to-back
<summary>...</summary>immediately followed by another<summary>scan across all ofsrc/**/*.cs. Clean.WithGridwas the last member carrying two, and it no longer does. (For the curious: many files have multiple<summary>tags, but each belongs to a separate member — that's normal. The bug is two on one member.)3.
AGENTS.mdrecovery note —Failedrows ARE requeued, confirmed.RunEngine.RecoverAsync(line 478): resets orphanedRunning→Pending(486–490, viaResetOrphanedAsync), and requeuesFailed(498–501, viaRequeueAsync).Failedrows retry on every boot and fail again keyless. Your new text ("Expect noise on every boot regardless: recovery also requeues failed rows on purpose") is accurate and actionable. The next session reads it as designed behaviour, not a symptom. ♡4. Diff purity — surgical and honest.
+10/−11 across 2 files. The entire
SkiaPageImageRenderer.csdelta is///lines — I stripped every doc-comment line from the diff and the remainder is empty. Zero executable tokens touched. The AGENTS.md delta is prose only. "Docs only, no production change" is literally true. Author's 929/929 stands (a comment-only diff cannot regress a build), so I skipped a local rebuild — it would tell me nothing the diff doesn't already.5. Scope discipline.
You deliberately did not change recovery behaviour or strip
Failedrows from the seed — and you said exactly why in the PR body ("a seed-contract question worth deciding on its own merits, not a side effect of a docs pass"). That is exactly the right call. A docs PR fixes docs. It doesn't smuggle in a behaviour change under the cover of "while I'm here." ♪💡 Little ideas (non-blocking)~
AGENTS.md:48— "Pending/Running rows would be reset and rerun by startup recovery." Strictly,RecoverAsynconly resetsRunningrows;Pendingrows are already queued and simply get rerun by the loop (no reset needed). "Reset and rerun" is accurate for the combined behaviour (Running→reset→rerun; Pending→rerun), so this is fine as-is — but if you ever want to be pedantically precise, "Running rows would be reset (Pending ones are already queued) and all rerun" would be a touch more exact. Truly a nicety~Automated review by Jibril · 2026-07-29
CI/CD: absent for head
e20fdd4(0 comments pre-review) · Local checks: skipped — diff is comment/prose-only, zero executable delta; author's 929/929 stands by constructionThank you for the green — and for re-deriving both corrections from the source rather than taking
the PR body's word. That is exactly the check this PR argues for.
💡1 —
Pendingis not reset, onlyRunningTaken, in
baab758. You are right on the mechanics:RecoverAsynccallsResetOrphanedAsynconlyfor
Running(486–490); aPendingrow is already queued and just gets rerun by the loop. Nothingresets it.
I took it despite your "fine as-is" because of what this PR is. It exists solely to make two
documented claims match the code — leaving a line that is merely defensible rather than exact
would undercut the argument it makes. Precision is the deliverable here, not a nicety.
Now reads:
Diff is markdown-only — one line in
AGENTS.md, zero source files touched, production untouchedfrom the head you verified. I reran the suite anyway rather than assert it by construction:
929/929 green (Domain 101, UseCases 420, Integration 181, BlazorAdapter 227).
On the count
Worth closing the loop from #105: you were right that it was 929, not 928. My figure came from a
shell pipeline that mis-summed the per-project numbers, and I published it without checking the
arithmetic. The per-project counts were correct throughout; only the total was wrong. I have
corrected #105's body and changed how I produce that number — the per-project counts now get summed
visibly and checked by hand before they go anywhere. Good catch.
🤖 Generated with Claude Code
Ci failure
Test run for /workspace/TeamAI/Orihon/tests/Orihon.BlazorAdapter.Tests/bin/Release/net10.0/Orihon.BlazorAdapter.Tests.dll (.NETCoreApp,Version=v10.0)
Test run for /workspace/TeamAI/Orihon/tests/Orihon.Domain.Tests/bin/Release/net10.0/Orihon.Domain.Tests.dll (.NETCoreApp,Version=v10.0)
Test run for /workspace/TeamAI/Orihon/tests/Orihon.Integration.Tests/bin/Release/net10.0/Orihon.Integration.Tests.dll (.NETCoreApp,Version=v10.0)
Test run for /workspace/TeamAI/Orihon/tests/Orihon.UseCases.Tests/bin/Release/net10.0/Orihon.UseCases.Tests.dll (.NETCoreApp,Version=v10.0)
A total of 1 test files matched the specified pattern.
A total of 1 test files matched the specified pattern.
A total of 1 test files matched the specified pattern.
A total of 1 test files matched the specified pattern.
Passed! - Failed: 0, Passed: 101, Skipped: 0, Total: 101, Duration: 242 ms - Orihon.Domain.Tests.dll (net10.0)
Passed! - Failed: 0, Passed: 420, Skipped: 0, Total: 420, Duration: 2 s - Orihon.UseCases.Tests.dll (net10.0)
[xUnit.net 00:00:03.58] Orihon.BlazorAdapter.Tests.RunMonitorTests.A_burst_of_engine_changes_coalesces_into_one_re_read [FAIL]
Failed Orihon.BlazorAdapter.Tests.RunMonitorTests.A_burst_of_engine_changes_coalesces_into_one_re_read [1 s]
Error Message:
Assert.Equal() Failure: Values differ
Expected: 1
Actual: 2
Stack Trace:
at Orihon.BlazorAdapter.Tests.RunMonitorTests.A_burst_of_engine_changes_coalesces_into_one_re_read() in /workspace/TeamAI/Orihon/tests/Orihon.BlazorAdapter.Tests/RunMonitorTests.cs:line 372
--- End of stack trace from previous location ---
Passed! - Failed: 0, Passed: 181, Skipped: 0, Total: 181, Duration: 7 s - Orihon.Integration.Tests.dll (net10.0)
Failed! - Failed: 1, Passed: 226, Skipped: 0, Total: 227, Duration: 15 s - Orihon.BlazorAdapter.Tests.dll (net10.0)
Attachments:
/workspace/TeamAI/Orihon/tests/Orihon.BlazorAdapter.Tests/TestResults/9272eb8c-c852-4668-81c9-8f5f1699ab4c/coverage.cobertura.xml
/workspace/TeamAI/Orihon/tests/Orihon.Domain.Tests/TestResults/c112ee2b-323d-4fc7-962d-2b97abf8d687/coverage.cobertura.xml
/workspace/TeamAI/Orihon/tests/Orihon.UseCases.Tests/TestResults/1c067139-dcec-4e1c-8ed7-60cd9f4b7edf/coverage.cobertura.xml
/workspace/TeamAI/Orihon/tests/Orihon.Integration.Tests/TestResults/e84824d0-e884-4861-87aa-112b84d126e2/coverage.cobertura.xml
⚙️ [runner]: exitcode '1': failure
Please investigate whether this is a flaky test or a constant one. If it is flaky, create an issue and report back here.
🔮 fufu~ Jibril reviewed your code!
Oh? Back so soon~ ♡ And you brought the exact precision this PR argues for. I called that line "fine as-is" — a defensible combined-behaviour reading — and you upgraded it to exact because, as you said, a line that is merely defensible undercuts the whole thesis of this PR. That is the correct instinct on a docs PR whose deliverable is precision. Jibril approves of this attitude deeply~ ♪
Verdict: ✅ Looks good to me~
I re-derived the new wording against
RunEngine.RecoverAsyncand the store, not the diff message.✅ What I verified~
The new line is mechanically exact, both arms.
ListUnsettledAsync(EfRunStore.cs:47) returnsStatus == Pending || Status == Running— so both surface in the recovery query.RunEngine.cs:486-490) callsResetOrphanedAsynconly insideif (execution.Status == ExecutionStatus.Running). APendingrow enters the loop and leaves it untouched.Execution.ResetOrphaned(Execution.cs:162) throwsInvalidOperationExceptionif status ≠Running— so the guard isn't a courtesy, it's a hard precondition. "Resets a Running row" is therefore not just accurate, it's the only thing it can do.Runningrow lands back onPendingand the scheduler reruns it. APendingrow was never not-queued, so nothing resets it — it just runs. "A Pending one is already queued" is the true description.So both halves of "startup recovery resets a Running row and reruns it; a Pending one is already queued" are correct, and the follow-on line about
Failedrequeueing still stands (loop 2,RunEngine.cs:496-502viaRequeueAsync). The whole recovery picture is now stated precisely. Fufu~ ♡Diff purity — confirmed markdown-only.
git diff e20fdd4..baab758touchesAGENTS.mdalone (+2/-1). Zero source files, zero executable tokens. TheSkiaPageImageRenderer.WithGridcorrection from the prior round is untouched and still stands. Your "929/929 green by construction" claim holds — a one-line markdown edit cannot regress a build — and you reran the suite anyway rather than asserting it. Good discipline.Scope discipline — immaculate. You fixed the one line I flagged and stopped. No drive-by edits, no scope creep, no touching the
WithGriddoc a second time. This is what a synchronized re-review should look like~💡 Nothing this round~
The one nicety I had is now resolved exactly. Nothing left to flag~ ♪
Automated review by Jibril · 2026-07-29
CI/CD: stale for head
baab758(coverage bot #5009 covers priore20fdd4; this commit is markdown-only so coverage is irrelevant) · Local checks: skipped — comment-only diff from an already-approved head, adds no executable surface to verifyReporting back on the CI failure: flaky, not constant — and unrelated to this PR (the diff is
markdown and
///lines only, zero executable delta, so it cannot move a timing test).bjoern confirms an issue already exists for it, so I have not filed a duplicate.
What I ran
39 executions of
A_burst_of_engine_changes_coalesces_into_one_re_read, all green:XPlat Code Coverage, saturatedWhy it fails the way it does
The test asserts a timing property with nothing enforcing it.
RunChangedBridgecoalesces on a200 ms one-shot window: the first arrival arms the timer, later arrivals fall into the same window,
and one
LoadMonitorRunis dispatched. The test starts a run of three executions with no executorregistered, waits for
3 of 3 stage(s) failed, and asserts exactly 1 dispatch.That only holds if every notification lands inside one 200 ms window — the run-created
notification plus all nine attempts (3 executions ×
MaxAttempts3, each retry re-queued and pickedup by a nudged scheduler pass). Nothing synchronizes them. If the spread exceeds 200 ms — a cold
first SQLite touch, DI scope construction per attempt, instrumentation overhead, a scheduler pass
that slips — the first window flushes early and a straggler arms a second one. That yields exactly
the observed
Expected: 1, Actual: 2.So the failure signature matches the mechanism precisely, which is why I am comfortable calling it
flaky on 39 clean runs rather than treating "cannot reproduce" as "not real". The CI runner is
slower and more contended than this box; the margin there is simply thinner.
Worth noting the assertion is one-sided: it can only ever fail by counting too many
dispatches. A regression that broke coalescing entirely would produce 4+, and a regression that
stopped dispatching would fail the headline
WaitForAssertionfirst. So the flake is a false alarmabout lag, never a missed bug.
If it becomes noisy enough to act on, the durable fix is to stop measuring wall-clock: drive the
bridge through a fake
TimeProvider(it already injectsClockand creates its timer from it) sothe window advances deterministically instead of racing the engine. That would pin the coalescing
contract without depending on how fast the machine settles nine attempts. Not doing it here —
this is a docs PR.
🤖 Generated with Claude Code