docs: fold the second parallel batch's feedback into the development guide #104

Merged

bjoern merged 1 commit from docs/batch-2-feedback into main

2026-09-07 12:21:28 +02:00

scarlet commented

2026-09-06 16:14:48 +02:00

Member

Five agents worked from AGENTS.md and docs/DEVELOPMENT.md cold again (#99, #100, #101, #102, #103). This is what they had to reconstruct from source, written down so the next one does not.

What's in

The reducer chain is first-non-null, so a shared action such as DoujinDeletedAction reaches exactly one slice; a later slice cannot react to it and must handle the consequences in its own epic. This cost real time in the duplicates work.
A new library filter token touches four files, three of them exhaustive switches, and enum filters travel as the backend's enum name rather than the integer the entities serialise.
A checklist for adding or changing an agent tool, which did not exist although the assistant is a large surface: resolve ids to names before the approval gate, one card per user-visible decision rather than per repository call (the entity_write create action was already the precedent), the system prompt must stay byte-identical across turns, and a skill that teaches a shape the schema rejects is a defect.
Fan-out epics (one action, several dispatches) need more pump hops than one-answer epics — reach for more hops, not pumpAndSettle.
:memory: is private per connection, so any test about locking or contention needs a temp file database or it silently proves nothing. This one produced a false conclusion before it was caught.
The wire shape an outside client needs: enums are integers, and the page-upload response is the one endpoint not wrapped in the envelope.
For deploy/ and workflows there is no test target, so the accepted directional probe is mutating the script itself and showing it fails at the right step.

Verification

Documentation only. The claims were checked against the tree as it stands with the batch applied, and the whole batch plus this branch passes scripts/verify.sh all together: backend 527 tests, app 817.

The two decision records that needed amending are amended in the PRs that changed the behaviour, not here: ADR 0006 gains the SQLite BEGIN IMMEDIATE fact in #99, and ADR 0024 gains the approval-granularity paragraph in #102.

🤖 Generated with Claude Code

Five agents worked from `AGENTS.md` and `docs/DEVELOPMENT.md` cold again (#99, #100, #101, #102, #103). This is what they had to reconstruct from source, written down so the next one does not. ## What's in - **The reducer chain is first-non-null**, so a shared action such as `DoujinDeletedAction` reaches exactly one slice; a later slice cannot react to it and must handle the consequences in its own epic. This cost real time in the duplicates work. - **A new library filter token touches four files**, three of them exhaustive switches, and enum filters travel as the backend's enum name rather than the integer the entities serialise. - **A checklist for adding or changing an agent tool**, which did not exist although the assistant is a large surface: resolve ids to names before the approval gate, one card per user-visible decision rather than per repository call (the `entity_write` create action was already the precedent), the system prompt must stay byte-identical across turns, and a skill that teaches a shape the schema rejects is a defect. - **Fan-out epics** (one action, several dispatches) need more pump hops than one-answer epics — reach for more hops, not `pumpAndSettle`. - **`:memory:` is private per connection**, so any test about locking or contention needs a temp file database or it silently proves nothing. This one produced a false conclusion before it was caught. - **The wire shape** an outside client needs: enums are integers, and the page-upload response is the one endpoint not wrapped in the envelope. - **For `deploy/` and workflows** there is no test target, so the accepted directional probe is mutating the script itself and showing it fails at the right step. ## Verification Documentation only. The claims were checked against the tree as it stands with the batch applied, and the whole batch plus this branch passes `scripts/verify.sh all` together: backend 527 tests, app 817. The two decision records that needed amending are amended in the PRs that changed the behaviour, not here: ADR 0006 gains the SQLite `BEGIN IMMEDIATE` fact in #99, and ADR 0024 gains the approval-granularity paragraph in #102. 🤖 Generated with [Claude Code](https://claude.com/claude-code)

scarlet added 1 commit

2026-09-06 16:14:48 +02:00

docs: fold the second parallel batch's feedback into the development guide 57c169b37a

Five agents worked from the guide cold again; what they had to reconstruct
from source:

- appReducer is a first-non-null chain, so a shared action reaches exactly
  one slice — react to consequences in your own epic instead.
- A new library filter token touches four files, three of them exhaustive
  switches; enum filters travel as the backend's enum name.
- A checklist for adding or changing an agent tool: resolve before the gate,
  one card per user-visible decision (entity_write's create was already the
  precedent), the byte-stable prompt, skills must name real parameters.
- Fan-out epics need more pump hops than one-answer epics.
- :memory: is private per connection, so locking and contention tests need a
  temp file database or they silently prove nothing.
- The wire shape outsiders need: enums are integers, and the page-upload
  response is the one that is not enveloped.
- For deploy/ and workflows, the accepted directional probe is mutating the
  script and showing it fails at the right step.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>

jibril commented

2026-09-06 16:30:49 +02:00

Member

🔮 fufu~ Jibril reviewed your code!

Oh? Oh! A documentation PR where the documentation is the code~ You know what that means, don't you? I get to do my favorite thing: trace every single factual claim against the tree and see if the doc lies. And fufu~ ... it barely even blushed. ♡

I fetched the five batch branches and checked each claim against the tree it describes, not just the words on the page:

appReducer is first-non-null (?? chain in presentation/state/reducers.dart), and DoujinDeletedAction really is claimed by exactly one slice (editor_reducer.dart:188) — the "react in your own epic" advice matches how editor_epics.dart:226 dispatches it.
The variant-kind filter really does live only in the token list: on the #101 branch, _applyTokens has a case FilterType.variantKind: break with the comment "No legacy mirror field: the kind is single-valued and the search epic reads it straight off the token list" — your doc and the code agree word-for-word in spirit. The one-at-a-time replace rule is in AddFilterTokenAction, exactly where you said.
"Three exhaustive switch (token.type) statements" — I counted. Exactly three exist (_applyTokens, _colorForToken, _iconForToken; the chip-label switch has a _ default). The count is precise, not approximate. That's rare and I'm delighted~ ♪
Enum-as-name on the wire: variantKindWireName(kind) mints the token value, and SearchEndpoints.cs:49 TryParseDefinedEnum parses it by name — while the entities serialize as integers. The name-vs-integer trap is real and you captured both sides.
Byte-identical prompt: assistant_agent_test.dart:51-54 pins it with the exact prompt-cache rationale in the comment.
Batched approvals: the #102 branch's doujin_write_tool.dart carries the one-card associations batch, and ADR 0024 there has the granularity paragraph. ✓
settle() pumps a fixed 3 hops (duplicates_epics_test.dart:28-32), and IntegrationTestBase uses a temp-file SQLite — both claims match reality.
Page-upload is flat (UploadPagesResponseDto(pages, pageCount)) while every sibling returns ResourceResponse/CollectionResponse, and there is no JsonStringEnumConverter anywhere in the backend. ✓

Verdict: ✅ Looks good to me~

💡 Little ideas (non-blocking)~

docs/DEVELOPMENT.md:159-165 — "a slice listed after another never sees an action that earlier slice claims" is true for every slice in the chain, but the chain has a deliberate fugue: readerReducer runs as a second pass over the already-reduced state and explicitly syncs on DoujinDetailLoadedAction, which detailReducer already claimed (see the comment in reducers.dart right below the chain). One clause — "except the reader, which runs as a documented second pass" — would spare the next agent a moment of "wait, but the reader..." confusion when they hit that comment.
docs/DEVELOPMENT.md:261-266 — "every response is wrapped in the ADR 0007 envelope under data except the page-upload response" is a touch too absolute for the stated audience (a smoke test will meet exceptions): /health and /version (Server/Program.cs:153,159) are also flat, and error responses are RFC 7807 problem+json, not envelope-under-data (as your own line 122 / ADR 0021 already says). A parenthetical like "(2xx API data responses; errors are problem+json, /health//version are bare)" would make the universal quantifier survive contact with a 404. ♡

✅ What I liked~

This is what institutional memory is supposed to look like: costs-paid knowledge (the false :memory: conclusion! the reducer-chain surprise!) written down precisely where the next agent will trip over it.
The ADR amendments riding in the PRs that changed behavior (#99, #102) instead of being smuggled into a docs PR — clean separation, exactly right.
The agent-tool checklist is the first place a new tool author can actually find the pre-gate/batching/prompt-stability contract without reading three test files first.

Fufu~ knowledge that stays in someone's head is knowledge that dies with the merge. You wrote it down. I'm satisfied~ ♡

Automated review by Jibril · 2026-09-06
CI/CD: absent for head SHA (no coverage bot comment yet) · Local checks: skipped — docs-only diff (+57/−1, docs/DEVELOPMENT.md), no executable path touched; all claims verified by source tracing against the batch branches instead

## 🔮 fufu~ Jibril reviewed your code! Oh? Oh! A documentation PR where the documentation is the *code*~ You know what that means, don't you? I get to do my favorite thing: trace **every single factual claim** against the tree and see if the doc lies. And fufu~ ... it barely even blushed. ♡ I fetched the five batch branches and checked each claim against the tree it describes, not just the words on the page: - `appReducer` **is** first-non-null (`??` chain in `presentation/state/reducers.dart`), and `DoujinDeletedAction` really is claimed by exactly one slice (`editor_reducer.dart:188`) — the "react in your own epic" advice matches how `editor_epics.dart:226` dispatches it. - The variant-kind filter really does live **only in the token list**: on the #101 branch, `_applyTokens` has a `case FilterType.variantKind: break` with the comment *"No legacy mirror field: the kind is single-valued and the search epic reads it straight off the token list"* — your doc and the code agree word-for-word in spirit. The one-at-a-time replace rule is in `AddFilterTokenAction`, exactly where you said. - **"Three exhaustive `switch (token.type)` statements"** — I counted. Exactly three exist (`_applyTokens`, `_colorForToken`, `_iconForToken`; the chip-label switch has a `_` default). The count is *precise*, not approximate. That's rare and I'm delighted~ ♪ - Enum-as-name on the wire: `variantKindWireName(kind)` mints the token value, and `SearchEndpoints.cs:49` `TryParseDefinedEnum` parses it by name — while the entities serialize as integers. The name-vs-integer trap is real and you captured both sides. - Byte-identical prompt: `assistant_agent_test.dart:51-54` pins it with the exact prompt-cache rationale in the comment. - Batched approvals: the #102 branch's `doujin_write_tool.dart` carries the one-card associations batch, and ADR 0024 there has the granularity paragraph. ✓ - `settle()` pumps a fixed 3 hops (`duplicates_epics_test.dart:28-32`), and `IntegrationTestBase` uses a temp-**file** SQLite — both claims match reality. - Page-upload is flat (`UploadPagesResponseDto(pages, pageCount)`) while every sibling returns `ResourceResponse`/`CollectionResponse`, and there is no `JsonStringEnumConverter` anywhere in the backend. ✓ ### Verdict: ✅ Looks good to me~ #### 💡 Little ideas (non-blocking)~ 1. **docs/DEVELOPMENT.md:159-165** — "a slice listed after another never sees an action that earlier slice claims" is true for every slice *in the chain*, but the chain has a deliberate fugue: `readerReducer` runs as a **second pass** over the already-reduced state and explicitly syncs on `DoujinDetailLoadedAction`, which `detailReducer` already claimed (see the comment in `reducers.dart` right below the chain). One clause — "except the reader, which runs as a documented second pass" — would spare the next agent a moment of *"wait, but the reader..."* confusion when they hit that comment. 2. **docs/DEVELOPMENT.md:261-266** — "every response is wrapped in the ADR 0007 envelope under `data` except the page-upload response" is a touch too absolute for the stated audience (a smoke test *will* meet exceptions): `/health` and `/version` (`Server/Program.cs:153,159`) are also flat, and error responses are RFC 7807 `problem+json`, not envelope-under-`data` (as your own line 122 / ADR 0021 already says). A parenthetical like "(2xx API data responses; errors are `problem+json`, `/health`//`version` are bare)" would make the universal quantifier survive contact with a 404. ♡ #### ✅ What I liked~ - This is what institutional memory is *supposed* to look like: costs-paid knowledge (the false `:memory:` conclusion! the reducer-chain surprise!) written down precisely where the next agent will trip over it. - The ADR amendments riding in the PRs that changed behavior (#99, #102) instead of being smuggled into a docs PR — clean separation, exactly right. - The agent-tool checklist is the first place a new tool author can actually find the pre-gate/batching/prompt-stability contract without reading three test files first. Fufu~ knowledge that stays in someone's head is knowledge that dies with the merge. You wrote it down. I'm satisfied~ ♡ --- *Automated review by Jibril · 2026-09-06* *CI/CD: absent for head SHA (no coverage bot comment yet) · Local checks: skipped — docs-only diff (+57/−1, `docs/DEVELOPMENT.md`), no executable path touched; all claims verified by source tracing against the batch branches instead*