chore(brain): deprecate + remove brain_write legacy params (domain/type/knowledge-path) (ADR-0012) #79
No Assignees
Notifications
Due Date
No due date set.
Dependencies
No dependencies set.
Reference: mathias/hyperguild#79
Reference in New Issue
Block a user
Charter: ADR-0012 ("legacy-param debt is real and in scope") · needs versioned-deprecation discipline
Problem
brain_write's schema carries legacy params from before the wing/hall model:domain,type, and theknowledge/landing path (the description literally says "(legacy)" several times). They're live API surface, so every caller has to read past them, and the two code paths (legacyknowledge/write with old frontmatter vs. wing+hallwiki/write) meanbrain_writehas two behaviours depending on which optional fields are set. That's exactly the mechanism-leak + surface-rot ADR-0012 warns about.What to do (versioned deprecation, NOT a hard break)
Per ADR-0012's migration discipline — add-new / deprecate-old / migrate-callers / then-remove:
domain/type/the knowledge/ path. Capture the real usage before changing anything.knowledge/-landing branch sobrain_writehas one behaviour.Caution: the
knowledge/vswiki/distinction is curation-state (per #43 — unreviewed vs promoted), not just legacy cruft. Confirm the promote/pending flow (#38) doesn't depend on theknowledge/write path before removing it. If it does, that path's removal is gated on the promote-verb work, not free.Acceptance
knowledge/-path dependency on the promote flow (#38) confirmed or ruled out before removalbrain_writehas one behaviourtask checkgreen; existing brain_write/update tests still passRelated
Review verdict: BLOCKED (not merged). PR #131, head
e8e2338.Verified independently:
WriteNoteOptions.Type/.Domain,writeLegacyNote, remainingknowledge/write-path callers, andinternal/skills/brainimports across both go.mod modules: none found. That part of the audit holds.go build ./... && go vet ./... && go test ./... -race -count=1green in both the root module andingestion/.type/domainpassthrough end-to-end: bothTestWriteNote_RequiresWingAndHall/TestBrainWriteRequiresWingAndHallandTestBrainWriteIgnoresLegacyTypeAndDomainParamscorrectly go red. Not decoration.Blocking finding — the caller audit missed a live caller in this repo itself:
cmd/hyperguild/brain.go(runBrainWrite) +cmd/hyperguild/http.go(brainClient.Write) implement the documented CLI commandhyperguild brain write <type> <slug>(seecmd/hyperguild/README.md§"hyperguild brain write ",cmd/hyperguild/main.gousage text,docs/superpowers/specs/2026-05-03-hyperguild-cli-design.md). It POSTs{"type": kind, "slug": slug, "content": ...}to/write— nowing/hallever sent. Before this PR that hit theneither wing nor hall setbranch and succeeded viawriteLegacyNote(intobrain/knowledge/, filename auto-generated sinceslugisn't a recognized JSON field). After this PR,WriteNotehard-errors"wing and hall are required"on every single invocation of this command — a live, documented feature goes from succeeding to failing 100% of the time.cmd/hyperguild/http_test.go'sTestBrainClient_Write_Successstays green through this because it mocks the/writeserver withhttptest.NewServerinstead of exercising the realapi.WriteNotehandler — so CI green here proves nothing about this regression.This directly fails issue #79's acceptance criteria: "Caller audit complete" and "Live callers migrated to wing/hall" — this caller was neither found nor migrated.
Fix needed before merge: either migrate
hyperguild brain writeto require/passwing/hall(and update its docs/usage text), or deprecate/remove the CLI subcommand in the same PR, with a test against the real handler (not a mock) proving the CLI still round-trips.CI (run 7859) was green on
e8e2338, root/ingestion modules build+vet+test clean,internal/skills/braindeletion confirmed safe — those are not the problem. The problem is this one missed caller.Merged PR #131 (squash) at commit
f324f04944, head975b61c.Re-review of the fix (
975b61c) that addressed the earlier BLOCK:cmd/hyperguild brain writeis now<wing> <hall> <slug>(3 required positional args), errors"wing, hall and slug required"when fewer are given. README anddocs/superpowers/specs/2026-05-03-hyperguild-cli-design.mdupdated to match.brainClient.Writenow sends{wing, hall, filename, content}matchingwriteRequestexactly.TestWrite_CLIPayloadRoundTripsiningestion/internal/api/handler_test.gogenuinely exercises the realHandler.Write->WriteNote->writeHallNotepath viahttptest.NewServer(http.HandlerFunc(h.Write))(not a mock). Flip-tested three ways on a truegit worktree add <sha> --detachcheckout (not an overlay diff, which silently keeps deleted files and gives false green):writeRequest.Filename's json tag -> test goes red (wrong path written, file not found).{type, slug, content}instead of{wing, hall, filename, content}) -> test goes red withwing and hall are required, 400 instead of 200. This is precisely the regression the first review caught.wing==""||hall==""guard inWriteNotedid NOT reopen a silent-write hole --writeHallNote's own taxonomy validation (invalid wing/invalid hall) still rejects empty values, just with a different error string.WriteNoteOptions.Type/.Domain,writeLegacyNote, orinternal/skills/brainanywhere in either module.go build ./... && go vet ./... && go test ./... -race -count=1in bothingestion/and the repo-root module.975b61c(run 7863, "CI / Lint / Test / Vet": success).Acceptance criteria in this issue: caller audit complete, legacy params removed, live callers (including the previously-missed CLI) migrated,
task check-equivalent green -- all hold.Known residual, not a blocker:
TestWrite_CLIPayloadRoundTripshardcodes the CLI's wire envelope locally (cmd/hyperguild and ingestion are separate Go modules, no go.work, so it can't importbrainClientdirectly). Combined withcmd/hyperguild/http_test.go'sTestBrainClient_Write_Success(which asserts the CLI's real marshal output field-by-field), both sides of the contract are pinned from real code, which is what actually would have caught the original regression. A literal built-binary integration test would close the last gap (nothing forces the two hardcoded envelopes to stay in sync if one side's JSON tags drift in a future change) but the cost didn't seem to be worth it for this fix; worth a follow-up ifcmd/hyperguild's HTTP payloads change again.