Skip to content

Commit 641344a

Browse files
lukstaficlaude
andcommitted
docs(swe-textbook): capture two filter-rejected learnings (gh-ludics-496)
Both entries cite task-a670cdbf round-2 review of PR #493 as the precipitating retro: 1. "New OrchestrationConfig fields require parse+merge in adapter init" — names the four surfaces (interface, default, migrateState backfill, adapter parse+merge) and the shared-parser pattern; closed mechanically by the lint shipped in the sibling commits. 2. "'Adapter init reads YAML' is a separate AC for OrchestrationConfig field additions" — flags the AC-bundling anti-pattern that hides the YAML-read step under "config field exists". Both are competent-SWE-filter "obvious-to-experienced-engineer" items that survive deadline pressure across distant surfaces. Captured to the textbook's write-side memory rather than promoted to always-loaded coder/reviewer prompts (per AC11/AC12 negative control). AC9 idempotency: pre-append snapshot evaluation showed all three guard checks (both headlines, shared retro) miss against the existing seed entry; post-append self-check shows all three hit. Shape regression test in docs/swe-textbook.shape.test.ts pins each new headline + retro citation under the existing slice helpers. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
1 parent c3f266e commit 641344a

2 files changed

Lines changed: 117 additions & 0 deletions

File tree

docs/swe-textbook.md

Lines changed: 72 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -97,3 +97,75 @@ the "obvious-to-experienced-engineer" bucket and be discarded from a
9797
competent engineers under deadline pressure (the contracted artifact
9898
lives on the *other* side of the fence). Captured here rather than
9999
skipped silently.
100+
101+
### New OrchestrationConfig fields require parse+merge in adapter init
102+
103+
Description: A typed configuration object that drives runtime
104+
behaviour usually has four independent surfaces a new field has to
105+
land on: the interface declaration, the in-code default, any
106+
backfill the persistent-state migrator applies to old records, and
107+
the parse+merge step the initialisation path performs against the
108+
user-facing YAML. The first three are easy to spot — they live next
109+
to each other in the same file or test — and a typed compiler will
110+
flag drift between them. The fourth is the most distant from the
111+
field declaration and the least visible to the type checker: if the
112+
init path never reads the YAML key, the documented config is
113+
silently inert and the field always takes its default. Adding a
114+
shared parser called from each init consumer is the cheapest way to
115+
keep the fourth surface visible. The pattern: a single helper named
116+
after the value it produces, called from each adapter's existing
117+
config-load consumer, fed into the partial-config record that the
118+
defaults function consumes — so the adapter knows to read the YAML,
119+
the parser knows the shape, and the merge knows the precedence.
120+
Test the parser as a unit; integration coverage at the call sites
121+
catches regressions there.
122+
123+
Precipitating retro: `task-a670cdbf` round-2 review of PR #493
124+
(settled-no-signal / hung-detection split). The reviewer's blocking
125+
line: the new `mag.orchestration.substantive_stall.*` YAML keys
126+
were documented and runtime-honoured, but neither adapter init
127+
extracted them from the YAML — so the keys were silently inert
128+
until the round-2 fix shipped a shared
129+
`parseSubstantiveStallOverrides` parser called from both adapter
130+
call sites.
131+
132+
Filter decision: Under the competent-SWE filter this is an
133+
"obvious-to-experienced-engineer" doctrine reminder — wiring up the
134+
read site is part of the same change as adding the field, by
135+
definition. Captured here rather than promoted to always-loaded
136+
agent prompts because the failure mode survives competent engineers
137+
under deadline pressure when the four surfaces sit in different
138+
files. The same precipitating retro is closed mechanically by the
139+
adapter-call-site lint shipped in gh-ludics-496, which makes the
140+
typed-default-plus-backfill checkbox no longer sufficient.
141+
142+
### "Adapter init reads YAML" is a separate AC for OrchestrationConfig field additions
143+
144+
Description: When an acceptance criteria list enumerates the surfaces
145+
that have to change for a new typed config field — interface,
146+
default, migration backfill — the adapter init path's read of the
147+
user-facing YAML is easy to bundle into the umbrella line "config
148+
field exists". That bundling lets a typed-interface-plus-default
149+
checkbox count as completion even when the YAML is silently
150+
ignored. The remedy is to give the init-side read its own AC,
151+
phrased as a behavioural property at the user-visible boundary
152+
("setting the YAML key to a non-default value produces the
153+
non-default behaviour"), distinct from any structural AC about the
154+
type or the default. The behavioural framing makes the AC
155+
falsifiable by an end-to-end test that sets the YAML and observes
156+
the runtime, not by an inspection of the type declaration.
157+
158+
Precipitating retro: `task-a670cdbf` round-2 review of PR #493. The
159+
proposal's AC list bundled the YAML-read step into "config field
160+
exists"; round-1 implementation satisfied the structural ACs without
161+
satisfying the behavioural one, and the gap surfaced only at
162+
round-2 review.
163+
164+
Filter decision: Under the competent-SWE filter this is also
165+
"obvious-to-experienced-engineer" doctrine — separate ACs for
166+
separate surfaces is general AC-writing hygiene. Captured here
167+
rather than promoted to AC templates loaded by always-on agent
168+
prompts because the doctrine is most useful as guidance to humans
169+
writing proposals, not as a rule enforced at every coder turn. The
170+
mechanical lint from the sibling entry above closes the same
171+
failure mode for the specific case of `OrchestrationConfig` fields.

docs/swe-textbook.shape.test.ts

Lines changed: 45 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -308,6 +308,51 @@ describe("skills/worker-conventions.md (AC4d field-contract reference)", () => {
308308
});
309309
});
310310

311+
// gh-ludics-496 textbook entries (AC9 of that proposal). Pins both new
312+
// `### …` blocks and their shared `task-a670cdbf` precipitating retro.
313+
// Mutation: drop either entry → assertion fails on the headline literal;
314+
// drop the retro citation → assertion fails on `task-a670cdbf`.
315+
describe("docs/swe-textbook.md (gh-ludics-496 entries)", () => {
316+
const body = read(TEXTBOOK_PATH);
317+
318+
const ENTRY_A_HEADLINE =
319+
"### New OrchestrationConfig fields require parse+merge in adapter init";
320+
const ENTRY_B_HEADLINE =
321+
'### "Adapter init reads YAML" is a separate AC for OrchestrationConfig field additions';
322+
323+
test("entry A — present with all four labelled fields and the retro", () => {
324+
const entryA = slice(
325+
body,
326+
new RegExp("^" + ENTRY_A_HEADLINE.replace(/[+]/g, "\\+")),
327+
new RegExp(
328+
"^" + ENTRY_B_HEADLINE.replace(/[.*+?^${}()|[\]\\"]/g, "\\$&"),
329+
),
330+
);
331+
expect(entryA).toContain("Description:");
332+
expect(entryA).toContain("Precipitating retro:");
333+
expect(entryA).toContain("Filter decision:");
334+
expect(entryA).toContain("task-a670cdbf");
335+
});
336+
337+
test("entry B — present with all four labelled fields and the retro", () => {
338+
const entryB = sliceToEnd(
339+
body,
340+
new RegExp(
341+
"^" + ENTRY_B_HEADLINE.replace(/[.*+?^${}()|[\]\\"]/g, "\\$&"),
342+
),
343+
);
344+
expect(entryB).toContain("Description:");
345+
expect(entryB).toContain("Precipitating retro:");
346+
expect(entryB).toContain("Filter decision:");
347+
expect(entryB).toContain("task-a670cdbf");
348+
});
349+
350+
test("both headlines literally present in the textbook body", () => {
351+
expect(body).toContain(ENTRY_A_HEADLINE);
352+
expect(body).toContain(ENTRY_B_HEADLINE);
353+
});
354+
});
355+
311356
describe("AC7 — negative control on agent-loaded prompts", () => {
312357
test("no `skills/**.md` outside the AC7 allowlist mentions `swe-textbook`", () => {
313358
// Harness: walk every .md file under skills/ and check the

0 commit comments

Comments
 (0)