feat(mag): docs/swe-textbook.md as Mag-side write memory for filter-rejected retros#499
feat(mag): docs/swe-textbook.md as Mag-side write memory for filter-rejected retros#499
Conversation
…er-rejected retro learnings Implements task-c4e0e80a. Introduces docs/swe-textbook.md as a write-only journal of competent-SWE-filter-rejected retro learnings, plus a `capture-textbook` disposition in /ludics-process-suggestions and the /ludics-feedback-digest worker. The textbook is consulted only by Mag and the feedback-digest worker; coder/reviewer agent prompts gain no pull-side pointer, preserving always-loaded prompt leanness (AC7 negative control). AC coverage: - AC1/AC2/AC6: docs/swe-textbook.md preamble enumerates five directionality statements + four labelled entry-shape fields; seed entry instantiates the gh-ocannl-270 lesson. - AC3: ludics-process-suggestions.md classification step rewritten three-way; new <!-- section:capture-textbook --> step routes to the canonical idempotency check; result-JSON example gains a sibling textbookCaptures array; Judgment Criteria gains a third bucket. - AC4: feedback-digest worker gains ### 3a filter step and a 7th Response Contract field (textbookCaptures); orchestrator preserves the field via Status routing + Result fields + output template. worker-conventions.md ## Field Contract Reference gains a row. - AC5: idempotency guard lives in exactly one location (docs/swe-textbook.md#capture-idempotency); both skills cite the anchor and describe inputs/outputs in prose only — neither duplicates the bash snippet, enforced by AC5-negative shape-test assertions on each skill body. - AC7/AC8: zero src/coder|reviewer|orchestration paths in diff; zero non-allowlisted skills/* files mention swe-textbook. Regression coverage: docs/swe-textbook.shape.test.ts (22 tests, 66 expect calls). Existing scripts/lint-contracts.ts cross-checks worker/orchestrator field-pair coherence on textbookCaptures. scope-expansion: docs/swe-textbook.shape.test.ts — regression-test infrastructure for the new doc and skill markdown. scope-expansion: skills/worker-conventions.md row — canonical field-contract reference must include textbookCaptures (the lint-contracts.ts REVERSE_EXCLUDE skips this file, so silent drift is otherwise unchecked); the new row contains no `swe-textbook` substring, keeping AC7 clean. Test plan: - bun test → 2227 tests / 0 fail (was 2205 pre-edit; +22 new tests). - bun test docs/swe-textbook.shape.test.ts → 22/22 pass. - bun test scripts/lint-contracts.test.ts → 31/31 pass. - bun run lint:contracts → clean. - bun run lint:skill-cli-refs → clean. - bun run typecheck → clean. - AC7 grep (verbatim from proposal) → zero lines. - AC8 grep on src/coder|reviewer|orchestration → zero lines. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The previous commit's seed-entry slice used `/^\0NEVER_MATCH/` as a never-match closer regex, which embedded a literal NUL byte in the test source. Git treated the file as binary (`git diff --numstat main...HEAD` reported `- -`), making the regression test opaque in normal review diffs. Replace the NUL-bearing impossible-regex pattern with a small `sliceToEnd(body, opener)` helper that walks from the opener line to end of file. Same semantics for the seed-entry slice, no embedded control bytes — git now sees a normal text blob. Test plan: - bun test docs/swe-textbook.shape.test.ts → 22/22 pass. - python3 NUL-count probe on the file → 0. - file(1) reports the file as UTF-8 text, no longer binary. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9f911dbc3e
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
|
||
| Inputs from the calling skill: | ||
|
|
||
| - `ENTRY_HEADLINE` — the proposed `### <headline>` text. |
There was a problem hiding this comment.
Normalize ENTRY_HEADLINE before idempotency grep
The input contract says ENTRY_HEADLINE is the full ### <headline> text, but the canonical guard searches with grep -Fq "### ${ENTRY_HEADLINE}"; if a caller follows the documented contract literally, the pattern becomes ### ### ... and existing entries are not detected. This causes the duplicate guard to return append for already-captured headlines, so repeated retros can create duplicate textbook entries instead of updating Second occurrence:.
Useful? React with 👍 / 👎.
… `### ` prefix PR #499 review (codex): the canonical idempotency guard's input contract said "ENTRY_HEADLINE — the proposed `### <headline>` text" but the bash snippet does `grep -Fq "### ${ENTRY_HEADLINE}"`. A caller following the contract literally produces `### ### <headline>` and the guard never matches existing entries — false-`append` on already-captured headlines, so repeated retros create duplicate textbook entries instead of amending `Second occurrence:`. AC5's falsifier is reachable through this contract bug. Fix: clarify the input contract — `ENTRY_HEADLINE` is the bare headline phrase WITHOUT the leading `### ` markdown prefix; the guard prepends `### ` itself. Add an explicit example. The bash snippet is unchanged (it was already correct); only the contract description needed to match it. Both skills' prose ("derive `ENTRY_HEADLINE` (a short pattern-naming phrase)") was already consistent with the corrected contract — only the doc needed tightening. Add a regression test in `docs/swe-textbook.shape.test.ts` (AC5-positive — input contract …) that pins the corrected phrasing and the "guard prepends `### `" claim. Mutation-tested by reverting to the buggy phrasing → assertion fails (1/23); restored → 23/23. Test plan: - bun test docs/swe-textbook.shape.test.ts → 23/23 pass. - bun run typecheck → clean. - bun run lint:contracts → clean. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
note: branch state has drifted since this body was written (baseline: 2 commits at 2026-05-05T09:15:28Z, current: 3 commits). consider |
|
note: branch state has drifted since this body was written (baseline: 2 commits at 2026-05-05T09:15:32Z, current: 3 commits). consider |
What I'd do differently next time — task-c4e0e80a (PR #499)Five durable lessons from this round, ranked by cost paid: 1. Dry-run bash snippets against an example before publishing the contractCodex caught the Refactor: when a doc-contract describes a snippet's inputs, paste 2. Pick ASCII-printable sentinels for "impossible regex" closersUsed Refactor: sentinels stay ASCII-printable
3.
|
Summary
Implements task-c4e0e80a. Introduces
docs/swe-textbook.mdas a write-only journal for competent-SWE-filter-rejected retro learnings, plus acapture-textbookdisposition in/ludics-process-suggestionsand the/ludics-feedback-digestworker. The textbook is consulted only by Mag and the feedback-digest worker; coder/reviewer agent prompts gain no pull-side pointer (AC7 negative control).docs/swe-textbook.md— preamble enumerates five directionality statements, four labelled entry-shape fields, single canonical## Capture Idempotencysection with the bash snippet, seed entry citinggh-ocannl-270.capture-textbook:ludics-process-suggestions.md(three-way classify, new<!-- section:capture-textbook -->step, siblingtextbookCapturesarray, third Judgment Criteria bucket);ludics-feedback-digest-worker.md(new### 3astep, 7th Response Contract field);ludics-feedback-digest.md(Status routing + Result fields + output template).worker-conventions.md: one new row in## Field Contract Reference(feedback-digest | textbookCaptures). The row contains noswe-textbooksubstring → AC7 stays clean.AC verification
docs/swe-textbook.mdpreamble has the five directionality statements + four labelled fields; seed entry instantiates fields a–d citinggh-ocannl-270. Pinned bydocs/swe-textbook.shape.test.ts(AC1/AC2/AC6 tests).<!-- section:classify -->slice has the three-way split withcapture-textbook;<!-- section:write-result -->JSON example hastextbookCaptures: [{suggestion, entryHeadline, precipitatingRetro}];## Judgment Criteriahas a third bucket. Pinned by AC3a/AC3b/AC3c shape-test slices.### 3astep + 7th Response Contract field; orchestrator## Status routingrow +## Result fieldsJSON + output template extension;worker-conventions.mdfield-contract row. Pinned by AC4a–AC4d shape-test assertions; cross-checked byscripts/lint-contracts.ts(worker### Response Contract↔ orchestrator## Status routingfield-pair coherence).docs/swe-textbook.md#capture-idempotency. Both skills cite the anchor and describe inputs/outputs in prose; neither duplicates the bash snippet. Enforced by AC5-positive (cardinality + canonical-snippet presence + corrected ENTRY_HEADLINE input contract per feat(mag): docs/swe-textbook.md as Mag-side write memory for filter-rejected retros #499 review) AND AC5-negative (no skill body containsgrep -Fq "### ${ENTRY_HEADLINE}",grep -Fq "${PRECIPITATING_RETRO}",echo "skip-duplicate", orecho "append") shape-test assertions.docs/swe-textbook.shape.test.tsasserts no non-allowlistedskills/**.mdfile containsswe-textbook; mutation-tested by appending the string toworker-conventions.md(assertion fired) and reverting. The proposal's literal grep has a known mechanical defect (its-vonly strips diff header lines, not in-scope-file content lines); the corrected per-file grep returns zero hits, and the shape-test walker is the canonical enforcement.git diff --name-only main...HEAD | grep -E '^src/(coder|reviewer|orchestration)/'returns zero lines.Test plan
bun test→ 2228 pass / 0 fail (was 2205 pre-edit; +23 new tests).bun test docs/swe-textbook.shape.test.ts→ 23/23 pass / 68 expect() calls.bun test scripts/lint-contracts.test.ts→ 31/31 pass.bun run lint:contracts→ clean (worker/orchestrator field contracts in sync).bun run lint:skill-cli-refs→ clean (all 92 refs across 51 files resolve).bun run typecheck→ clean.skills/files.src/(coder|reviewer|orchestration)/→ zero hits.Scope expansions (declared)
docs/swe-textbook.shape.test.ts(new) — regression-test infrastructure for the new doc and skill markdown. Mirrorsdocs/task-frontmatter-reference.shape.test.tsshape.skills/worker-conventions.mdrow addition — canonical field-contract reference must includetextbookCapturesto avoid silent drift the runtime lint cannot catch (scripts/lint-contracts.tsREVERSE_EXCLUDEskips this file). The new row contains noswe-textbooksubstring, keeping AC7 clean.Commits
37c8893— feature implementation (six files, +222/−11).9f911db— fix(test): remove a literal NUL byte from the new shape test that turned it into a git-binary blob; refactored to asliceToEnd(body, opener)helper.1f1815e— fix: clarifyENTRY_HEADLINEinput contract excludes the leading###prefix (codex review on PR feat(mag): docs/swe-textbook.md as Mag-side write memory for filter-rejected retros #499 — guard would have false-appended on already-captured headlines under the original contract phrasing); regression test added (mutation-confirmed).🤖 Generated with Claude Code