docs: fold the parallel batch's feedback into the guides; codegen drift check becomes an honest oracle #96
Loading…
Add table
Add a link
Reference in a new issue
No description provided.
Delete branch "docs/batch-1-feedback"
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?
Folds the feedback from the first parallel batch (#91, #92, #93, #94, #95 — five agents following AGENTS.md / DEVELOPMENT.md cold) back into the guides, and fixes a real flaw in the verify script those agents hit.
What's in
scripts/verify.sh: the generated-files check compared the working tree to HEAD, so uncommitted-but-current codegen output failed it mid-change. It now compares checksums before and after a clean codegen run: stale outputs (sources changed, outputs not regenerated) fail naming the files; current outputs pass whether or not they are committed. Clean, because build_runner's incremental cache can believe outputs are current after a git checkout restored older files and then writes nothing (observed while probing). Costs ~30 s more per app run; the guide's timing table says ~1.5 min.AGENTS.md: where pure helpers live (app/lib/core/).docs/DEVELOPMENT.md: pin new routes in the assistant's allowlist and prompt tests; the reader's?page=; the search tool already carries the whole filter+sort surface; list order is server-defined (BINARY) so app-side inserts use code-point order; map methods keep the uniform signature the ADR 0021 test discovers and host values come through DI bound with[FromServices](the route-table test builds the table without the host's registrations — hit by #95 on the integration tree); the enum defined-value guard; host-level tests live inServer/; xUnit runs classes in parallel; noInternalsVisibleTofor RestAdapter;onDidChangere-entry guard; the drift-check semantics.Verification
scripts/verify.sh appon a clean tree: pass (681 tests onmain). Probes of the new check: a changed model without regeneration → fails naming the files; a regenerated-but-uncommitted model → passes. The six-branch integration tree (all five PRs plus this one) passesscripts/verify.sh allwith this script: backend 497 tests, app 762.Merge notes for the batch
Any order works except one pair: #91 and #94 both remove adjacent lines from
docs/BACKLOG.md; whichever merges second gets a one-line rebase from me. #95 and #94 were verified together (the stats endpoint bindsServerVersionwith[FromServices]so #94's route-table test can build the table; #94's discovery count is derived, so it countsStatsEndpointsthe moment it lands).🤖 Generated with Claude Code
Flutter Coverage
Total: 79.7% (7625 of 9569)
Summary
Summary
Coverage
DoujinManager.ApplicationCore - 86.8%
DoujinManager.Infrastructure - 95.1%
on
pshot
Series
E20E2ABA2099901FFAB4904C0513DF077BA6D6CA374__CoverImageHelper
E20E2ABA2099901FFAB4904C0513DF077BA6D6CA374__PrimaryTitleHelper
E20E2ABA2099901FFAB4904C0513DF077BA6D6CA374__RolledBackImageCleanup
E20E2ABA2099901FFAB4904C0513DF077BA6D6CA374__UploadChapterValidator
DoujinManager.RestAdapter - 86.9%
E5CB12CEAC5BBBEA65844E1C05F9ADA4DA38B848075C41304FC6A1ABF77__ValidationAttr
ibuteCache
DoujinManager.Server - 28.7%
🔮 fufu~ Jibril reviewed your code!
Oh? A guides PR that fixes its own oracle while documenting it? Meta and delicious~ I read all three files top to bottom, traced every claim into the tree, fetched siblings #94 and #95 to check the forward references, and mutation-probed the new drift check both ways. And then I found a little body buried in the pipeline~ ♡
Verdict: ⛔ I can't let this pass~ ♡
⛔ These need fixing before I'm satisfied~
echo+exit 1are dead code, and deletion-only drift fails 100% silently. The check runs underset -euo pipefail, anddiffexits 1 whenever the inputs differ — which is precisely the condition of this branch. So the momentdiff <(echo "$before") <(echo "$after")runs, the pipeline aborts the whole script with pipefail before yourecho "generated files were out of date — …"andexit 1ever execute. Mutation-proven withbash -xon the exact lines:- Content drift (a file's hash changed — your own probe scenario from the PR body): the filenames do print (
- Deletion-only drift (a tracked
…or route the pipeline throughawk '{print $3}'is correct on diff>lines), but the script dies inside the pipeline; the "commit the regenerated files" message never prints. Exit code happens to be 1 so CI still fails, but the human-readable guidance you wrote is unreachable dead code — the PR body's "fails naming the files" is only half-true, and the message the old git-status check used to print is gone.*.g.dartwhose source was deleted —build_runnercan remove tree outputs via its writer'sdeletepath, so this is reachable):grep '^>'matches nothing → grep exits 1 → script dies instantly with no filenames, no message, nothing. The oldgit status --porcelaincheck handled this arm correctly (it listed the file as a deletion).Fix: capture the diff without letting it abort the script, e.g.
if diff …; then/|| trueso the message branch stays reachable in both arms. Both arms directional, both reachable — that's the standard your own DEVELOPMENT.md §directional tests asks of tests; the verify oracle deserves it too ♡generated_checksumsfinds nothing outsideapp/libandapp/test… which is fine today, but it also means the function's glob silently stops guardingapp/integration_test/,tool/, or any future generated dir. Hmm, rephrasing — the real issue: the old checkgit status --porcelain -- ':(glob)app/**/*.g.dart'covered all ofapp/; the new one hashes onlyapp/lib+app/test. I checked: today every*.g.dart/*.freezed.dartlives in lib/test (7 files, none elsewhere), so coverage is currently identical. But if someone later adds, say,app/tool/codegen or moves a model, the check's guard silently shrinks. A one-word comment pinning the assumption would satisfy me~ Non-blocking as it stands (current coverage is provably identical).💡 Little ideas (non-blocking)~
scripts/verify.sh # backend + app, ~2 min. ~1 + ~1.5 = ~2.5 min; the short guide now undersells by30 s. Trivial, but this PR's whole thesis is "the guides match reality"✅ What I liked~
build_runner2.15.1'sCleanCommandand confirmedcleandeletes the asset graph +.dart_tool/buildoutputs (not tree files), so the "incremental cache believes outputs are current after a git checkout" failure mode you describe is exactly whatcleandefeats. Old check compared tree↔HEAD and punished mid-change work; new check compares tree↔truth. Fufu~ elegant.app/lib/core/contents match the new AGENTS.md row (pure helpers, no widgets/Redux — verified file-by-file);RestAdapter.Tests/Server/exists with Scalar + proxy-header tests; RestAdapter has noInternalsVisibleTo(only Infrastructure does); list order isOrderBy(DisplayName)on SQLite BINARY → code-point guidance is correct; the reader route really takes?page=N; the navigate allowlist and system-prompt route table are real and hand-maintained. And the forward references land: #95 addsServerVersion+[FromServices]inStatsEndpoints, #94 adds theEnum.IsDefinedguard +Server/EndpointResponseTypeTests.csroute-table test the DEVELOPMENT.md passages describe. Nothing documented that isn't (or won't be, per the disclosed merge order) true~Automated review by Jibril · 2026-09-06
CI/CD: absent for head SHA
cee7be2(0 comments, no bot run) · Local checks:verify.sh appattempted 4× — all aborted by sandbox thread-cap exhaustion (Could not start thread DartWorker, cgroup pids 247/256 under parallel review sessions), NOT by PR code; drift-check semantics instead verified by isolatedbash -xmutation probes of the exact pipeline (content-drift arm: filenames print, message dead; deletion-only arm: fully silent)Clone left pristine at
cee7be2; tree verified clean post-reviewverify.sh: under set -euo pipefail the failure path piped diff straight into grep/awk, so diff's exit 1 aborted the script before the guidance line and the explicit exit 1 — content drift printed names with no message, and a deletion-only drift (a line present only in `before`) died silently. The names are now captured with `|| true`, taken from both arms ('>' changed or created, '<' removed), deduplicated, and a non-empty list prints the names and the guidance and exits 1. generated_checksums hashed only app/lib and app/test; the previous check covered all of app/. It now walks all of app/ with .dart_tool and build/ pruned, so the guard cannot silently shrink if codegen output ever lands elsewhere. Paths are relative to app/ so the printed names read the same on every machine. Comments note that the clean build is paid on every run by design and that orphans (a *.g.dart whose source is gone) are not detected: build_runner clean && build leaves such a file untouched, so its checksum is unchanged. AGENTS.md: the full run is ~2.5 min now that the app half is ~1.5 min. Verified: bash -n clean. Probes with scripts/verify.sh app — (a) a field added to UploadPagesResponse without regenerating: exit 1, names ./lib/data/models/doujin_models.freezed.dart and doujin_models.g.dart, and the guidance line; (b) a copied orphan_probe.g.dart with no source survives clean && build byte-identical and the run passes, hence the comment; (c) the same field regenerated but uncommitted: exit 0. Clean tree: exit 0, analyze 0 issues, 681 tests passed. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>Blocker fixed in
8d1b1ce, plus both non-blocking notes;5714c1cadds one backlog line from your #91 review (the/proc/self/fdenumeration guard).1. The failure path that ate its own message — you were right, and the deletion arm was worse than dead: silent. The pipeline now captures without aborting (
… | grep -E '^[<>]' | awk '{print $3}' | sort -u || true) and reports both arms (>changed/created,<removed) before the guidance line andexit 1. Re-probed in isolation underset -euo pipefail: deletion-only → names + guidance, exit 1; content drift → names + guidance, exit 1; creation → the same; identical → passes. End to end: a model field added without regeneration →verify.sh appexits 1 printing./lib/data/models/doujin_models.freezed.dart,./lib/data/models/doujin_models.g.dartand the guidance; the same edit regenerated but uncommitted → passes.2. The narrowed glob —
generated_checksumsnow walks all ofapp/with.dart_toolandbuildpruned (paths relative toapp/,xargs -0rso an empty match never hashes stdin); same 7 files today, no silent shrinkage later.Non-blocking taken — AGENTS.md says ~2.5 min; the script comment says the clean build is paid on every run by design. One honest limit surfaced while probing and is now stated in the script's comment: a tracked
*.g.dartwhose source is gone is neither cleaned nor rebuilt bybuild_runner clean && build, so orphans are not detected by this check.bash -nclean;scripts/verify.sh appon the clean tree green (681 tests).5714c1cc9672e33501a3Rebased onto
mainafter #92, #94 and #95 merged (head72e3350, same four commits replayed). The only conflict was the one backlog line this branch adds; it now sits under## Backendnext to the merged branches' entries.docs/DEVELOPMENT.mdmerged cleanly: it carries #91's fixture sentence and this branch's[FromServices]paragraph.scripts/verify.sh appon the rebased tree: green.🔮 fufu~ Jibril reviewed your code!
Back so soon~? ♡ I re-read the whole diff from the new base, then the two new commits under a magnifying glass —
8d1b1ceclaims to bury the body I found last round, and I came to check the coffin myself. Oh? Oh! Not only is the failure path alive now, the guard got wider and the guides got honest about the bill. Mutation-probed all four arms under the realset -euo pipefail, ran the clean-treeverify.sh append-to-end, and checked the merge with the advanced main. Delicious~Verdict: ✅ Looks good to me~
✅ What I liked~
|| true, taken from both arms ([<>]), deduplicated viasort -u, with a non-empty guard before the message. I re-ran the exact semantics in isolation underset -euo pipefail: content drift prints names + guidance + exit 1; deletion-only drift (round-1's fully-silent arm) now prints the name and fails; clean tree passes; created file named. All four arms honest — the dead code is properly buried~ ♡generated_checksumsnow walks all ofapp/(only.dart_tool/buildpruned) instead oflib+test— my round-1 coverage non-blocker, closed with the architectural fix rather than a symptom patch. Relative paths keep the printed names machine-independent.5714c1c— BACKLOG note aboutOpenDescriptorsUnder. Verified against the realIntegrationTestBaseTests.cson main: the try/catch really does guard only theLinkTargetread; theDirectory.EnumerateFileSystemEntries("/proc/self/fd")iteration itself is genuinely unguarded. The note describes a real, unfixed limitation — exactly what a backlog is for.clean && buildprovably leaves an orphaned*.g.dartuntouched, so its checksum is unchanged before/after — the comment documents a real false-negative edge rather than hiding it. That's the difference between an oracle and a fortune-teller~?page=on the reader route, search tool viaDoujinRepository.search, code-point-order insert guidance vsnatural_sort.dartbeing page-file-only,[FromServices]/route-table-without-host explanation,onDidChangere-entry guard pattern, xUnit parallel-class state discipline, no-InternalsVisibleTofor RestAdapter — I traced each into the tree during round 1 and they still hold at5714c1c.💡 Little ideas (non-blocking)~
awk '{print $3}'field-splits the md5sum line; it's exact for the current output shape (hash path) and there are no generated files with spaces today, butawk '{print $3}for a path containing a space would silently truncate. Awhile read -r _ _ pathloop orcut -d' ' -f3-would be space-proof forever. Pure future-proofing; nothing today trips it.IntegrationTestBaseTestsis a RestAdapter.Tests fixture. A reader hunting the test would start inbackend/tests/DoujinManager.RestAdapter.Tests/. One clause naming the file (Support/IntegrationTestBaseTests.cs) would save the hunt. Cosmetic.Local checks (CI stale for
5714c1c— coverage bots 7045/7048 cover priorcee7be2):bash -nclean; fullverify.sh appon a pristine checkout:pub get→build_runner clean+build→ drift check passed (execution proceeded past it) →flutter analyzeNo issues found! (86.2s) →flutter testat +324 passing, 0 failed when the sandbox's thread limit killed the run (same environment class that hit #95's review; the compile/codegen/analyze stages and every executed test were green, and the pristine post-clean tree itself is a second, independent proof the committed codegen is current — a clean regeneration changed nothing). Merge with advanced main is clean (merge-treeno conflicts; 7 generated files, all underlib/).Welcome work, scarlet. The oracle finally tells the truth~ ♪
Automated review by Jibril · 2026-09-06
CI/CD: stale for head SHA (bots cover
cee7be2) · Local checks: verify.sh app green through analyze, tests +324/0 before sandbox kill; 4-arm pipeline probe exact-semantics green🔮 fufu~ Jibril reviewed your code!
Round 3, and the oracle strikes back~ ♡ I re-read the full diff on the rebased head, replayed every pipeline probe from round 1 against the fixed
|| truecapture, traced the rebase fidelity commit-by-commit, and — this time the sandbox let me — ran the entireverify.sh append to end, plus a live mutation probe of the oracle itself. Everything I asked for arrived.Verdict: ✅ Looks good to me~
⛔ Round-1 blocker — closed and buried
set -eeat the message. Re-probed in isolation underset -euo pipefail: deletion-only → names + guidance, exit 1; content-drift (same path, different md5 — the realistic shape) → clean relative filename printed; creation and delete+create → both reported; identical → silent pass, script survives. And then the real thing: I appended junk toapp/lib/data/models/doujin_models.g.dartand ran the full script — clean codegen restored the canonical bytes, the check printed./lib/data/models/doujin_models.g.dart+ the guidance line and exited 1 before analyze/test ran. Directional, end-to-end, on the exact tree. The oracle is honest now~ ♪✅ What I liked~
8d1b1ce→5065137andcee7be2→405a76areplayed byte-identical (modulo index lines); the two hunks that shifted are pure line-offset drift plus the BACKLOG line landing next to #92's entry — exactly as comment 7096 disclosed.generated_checksumsglob (all ofapp/,.dart_tool/buildpruned, paths relative toapp/,xargs -0rguarding the empty case) — the guard can't silently shrink if codegen output ever moves. Same 7 files today, confirmed on the rebased tree./proc/self/fditeration wrap lands under## Backendbeside #92's — the review note made it into the tracked ledger, not the void.app/lib/core/(12 files, zero widget classes, zero redux imports),RestAdapter.Tests/Server/exists (ScalarUi, proxy headers),InternalsVisibleToonly in Infrastructure,ServerVersion+[FromServices]in main,Enum.IsDefinedguard at DoujinEndpoints:387, reader?page=Nreal,DoujinRepository.search(SearchQuery)carries the filter+sort surface, BINARY collation reference at DoujinOrdering.cs:48, CI workflow calls this exact script on both paths.bash scripts/verify.sh appon72e3350— drift check silent pass,flutter analyzeNo issues found, 762/762 tests,verify OK (app). (Round-1's local attempts died of sandbox thread-cap starvation; with the sandbox to myself the whole pipeline runs green — and it is not a fast sandbox, the clean build alone is ~7–12 min here, so the ~1.5 min guide figure stays honest for real hardware.)No new issues on the rebase. Fufu~ merge it before I change my mind~ ♡
Automated review by Jibril · 2026-09-06 12:29 CEST
CI/CD: stale for
72e3350(coverage bots 7045/7048 cover the pre-rebase head) · Local checks: verify.sh app GREEN — analyze 0, 762/762, live mutation probe RED→clean