chore(brain): remove brain_write legacy params domain/type/knowledge-path (ADR-0012) #131

Merged

mathias merged 2 commits from chore/79-remove-brain-write-legacy-params into main 2026-09-23 05:43:28 +00:00

Conversation 1 Commits 2 Files Changed 15

+188 -607

mathias commented 2026-09-23 05:32:46 +00:00

Owner

Copy Link

Copy Source

Closes #79.

Why (audit findings, from a prior session's step-1 pass)

1. The promote/pending flow (#38/#81) does not depend on knowledge/. brain_pending/brain_promote (ingestion/internal/api/promote.go) operate exclusively on brainDir/raw/ → brainDir/wiki/<wing>/<hall>/. They never touch brainDir/knowledge/. Removing the knowledge/ write path does not break #38/#81 — the issue's own caution flag is resolved.
2. No live caller uses domain/type/the legacy knowledge/ path. Checked hyperguild itself, mathias/skills (every skill using brain_write uses wing/hall only), mathias/agentsquad, mathias/dispatch, mathias/cad, mathias/cad-atlas — none found.
3. Bonus: dead orphaned code. internal/skills/brain/ (skill.go + handlers.go, repo-root module) defined a third, even-more-legacy brain_write shape (type/domain/filename, no wing/hall at all) for an old "supervisor" MCP server whose cmd/supervisor/main.go no longer exists. Confirmed nothing outside its own test file imported it — deleted entirely, since it encodes the exact legacy shape this issue removes, just in a dead code path.

What changed

• ingestion/internal/mcp/handlers.go — brain_write's tool schema and brainWriteArgs drop type/domain; wing+hall are now required in the schema.
• ingestion/internal/api/handler.go — WriteNote has one behaviour: wing+hall required, routes to wiki/<wing>/<hall>/. writeLegacyNote and the knowledge/-path branch are deleted.
• ingestion/internal/capture/ports.go, ingestion/internal/brainstore/store.go — drop Type/Domain from capture.Note and its WriteNoteOptions construction. pipeline/*.go's domain field (used by brain_ingest_raw) is a different, legitimate concept — untouched.
• internal/skills/brain/ — deleted (orphaned, dead since cmd/supervisor was removed).

Judgment call: what happens when wing/hall is omitted

With the knowledge/ fallback gone, there's no other valid destination for a write, so WriteNote now returns a flat "wing and hall are required" error whenever either is missing (previously: mixed messages — "wing and hall must be set together" only when exactly one was set, silent legacy fallback when neither was set). This surfaces as a 400 over HTTP and a tool-call error over MCP. I judged this the sane, unsurprising failure mode: one required-fields error, no special-cased partial state, no silent write to a location the caller didn't ask for.

A caller still sending stale domain/type args (there are none live, per the audit) does not get an error — the fields no longer exist on the arg struct, so json.Unmarshal silently drops them, and the write proceeds as a normal wing+hall write. Pinned by TestBrainWriteIgnoresLegacyTypeAndDomainParams.

Tests

TDD: added the new required-wing/hall test first, watched it fail for the right reason (RequiresWingAndHall/RequiresError on the old two-path split), then implemented.

• Added: TestWriteNote_RequiresWingAndHall (api), TestBrainWriteRequiresWingAndHall + TestBrainWriteIgnoresLegacyTypeAndDomainParams (mcp), TestWrite_RequiresWingAndHall (HTTP-contract counterpart).
• Removed (tested behaviour that's intentionally gone): TestWrite_CreatesKnowledgeFile (rewritten as TestWrite_CreatesWikiFile), TestWrite_IncludesFrontmatterWhenTypeProvided, TestWriteNote_HallRouteExistingTypeWinsOverOptsType, TestBrainWriteRejectsTraversal, TestBrainWriteAcceptsDoubleDotInName — the latter two tested the deleted legacy path's own filename-validation; the underlying traversal-safety property (via brain.Sanitise) on the surviving wing/hall route is independently covered by internal/brain/path_test.go's TestNotePath_SanitisesWingAndSlug / TestSanitise, unaffected by this change.

Verification

task check (both Go modules: test -race, vet, govulncheck, build, gitleaks) exits 0, zero FAIL.

Risk/medium (removes public MCP tool-schema surface) — not merging; awaiting independent adversarial review per the sprint's review discipline.

🤖 Generated with Claude Code

---

Created via git-mcp on behalf of @mathias

Closes #79. ## Why (audit findings, from a prior session's step-1 pass) 1. **The promote/pending flow (#38/#81) does not depend on `knowledge/`.** `brain_pending`/`brain_promote` (`ingestion/internal/api/promote.go`) operate exclusively on `brainDir/raw/` → `brainDir/wiki/<wing>/<hall>/`. They never touch `brainDir/knowledge/`. Removing the `knowledge/` write path does not break #38/#81 — the issue's own caution flag is resolved. 2. **No live caller uses `domain`/`type`/the legacy `knowledge/` path.** Checked hyperguild itself, `mathias/skills` (every skill using `brain_write` uses `wing`/`hall` only), `mathias/agentsquad`, `mathias/dispatch`, `mathias/cad`, `mathias/cad-atlas` — none found. 3. **Bonus: dead orphaned code.** `internal/skills/brain/` (`skill.go` + `handlers.go`, repo-root module) defined a third, even-more-legacy `brain_write` shape (`type`/`domain`/`filename`, no wing/hall at all) for an old "supervisor" MCP server whose `cmd/supervisor/main.go` no longer exists. Confirmed nothing outside its own test file imported it — deleted entirely, since it encodes the exact legacy shape this issue removes, just in a dead code path. ## What changed - `ingestion/internal/mcp/handlers.go` — `brain_write`'s tool schema and `brainWriteArgs` drop `type`/`domain`; `wing`+`hall` are now `required` in the schema. - `ingestion/internal/api/handler.go` — `WriteNote` has **one** behaviour: wing+hall required, routes to `wiki/<wing>/<hall>/`. `writeLegacyNote` and the `knowledge/`-path branch are deleted. - `ingestion/internal/capture/ports.go`, `ingestion/internal/brainstore/store.go` — drop `Type`/`Domain` from `capture.Note` and its `WriteNoteOptions` construction. `pipeline/*.go`'s `domain` field (used by `brain_ingest_raw`) is a different, legitimate concept — untouched. - `internal/skills/brain/` — deleted (orphaned, dead since `cmd/supervisor` was removed). ## Judgment call: what happens when wing/hall is omitted With the `knowledge/` fallback gone, there's no other valid destination for a write, so `WriteNote` now returns a flat `"wing and hall are required"` error whenever either is missing (previously: mixed messages — `"wing and hall must be set together"` only when exactly one was set, silent legacy fallback when neither was set). This surfaces as a 400 over HTTP and a tool-call error over MCP. I judged this the sane, unsurprising failure mode: one required-fields error, no special-cased partial state, no silent write to a location the caller didn't ask for. A caller still sending stale `domain`/`type` args (there are none live, per the audit) does **not** get an error — the fields no longer exist on the arg struct, so `json.Unmarshal` silently drops them, and the write proceeds as a normal wing+hall write. Pinned by `TestBrainWriteIgnoresLegacyTypeAndDomainParams`. ## Tests TDD: added the new required-wing/hall test first, watched it fail for the right reason (`RequiresWingAndHall`/`RequiresError` on the old two-path split), then implemented. - Added: `TestWriteNote_RequiresWingAndHall` (api), `TestBrainWriteRequiresWingAndHall` + `TestBrainWriteIgnoresLegacyTypeAndDomainParams` (mcp), `TestWrite_RequiresWingAndHall` (HTTP-contract counterpart). - Removed (tested behaviour that's intentionally gone): `TestWrite_CreatesKnowledgeFile` (rewritten as `TestWrite_CreatesWikiFile`), `TestWrite_IncludesFrontmatterWhenTypeProvided`, `TestWriteNote_HallRouteExistingTypeWinsOverOptsType`, `TestBrainWriteRejectsTraversal`, `TestBrainWriteAcceptsDoubleDotInName` — the latter two tested the deleted legacy path's own filename-validation; the underlying traversal-safety property (via `brain.Sanitise`) on the surviving wing/hall route is independently covered by `internal/brain/path_test.go`'s `TestNotePath_SanitisesWingAndSlug` / `TestSanitise`, unaffected by this change. ## Verification `task check` (both Go modules: test -race, vet, govulncheck, build, gitleaks) exits 0, zero FAIL. Risk/medium (removes public MCP tool-schema surface) — not merging; awaiting independent adversarial review per the sprint's review discipline. 🤖 Generated with [Claude Code](https://claude.com/claude-code) --- _Created via git-mcp on behalf of @mathias_

mathias added 1 commit 2026-09-23 05:32:46 +00:00

chore(brain): remove brain_write legacy params domain/type/knowledge-path (ADR-0012) ... e8e233823d

hyperguild#79's caller audit (done in a prior session) found the removal
was safe:

- The promote/pending flow (#38/#81, ingestion/internal/api/promote.go)
  operates exclusively on brainDir/raw/ -> brainDir/wiki/<wing>/<hall>/. It
  never touches brainDir/knowledge/, so removing the knowledge/ write path
  does not break it.
- No live caller uses domain/type or the knowledge/ path: checked
  hyperguild itself, mathias/skills, mathias/agentsquad, mathias/dispatch,
  mathias/cad, mathias/cad-atlas.
- internal/skills/brain (a skill.go + handlers.go pair) defined a third,
  even-more-legacy brain_write shape (type/domain/filename, no wing/hall)
  for an old "supervisor" MCP server whose cmd/supervisor/main.go no
  longer exists. Nothing outside its own test imported it; deleted
  entirely as it encodes the exact legacy shape this issue removes.

Changes:
- ingestion/internal/mcp/handlers.go: brain_write's tool schema and
  brainWriteArgs drop type/domain; wing+hall are now marked required in
  the schema.
- ingestion/internal/api/handler.go: WriteNote has one behaviour — wing+
  hall required, routing to wiki/<wing>/<hall>/. writeLegacyNote and the
  knowledge/-path branch are deleted. Omitting wing or hall now returns
  "wing and hall are required" (400 over HTTP, a tool error over MCP) —
  the only sane failure mode left once there is no other valid
  destination.
- ingestion/internal/capture/ports.go, internal/brainstore/store.go: drop
  Type/Domain from capture.Note and its WriteNoteOptions construction.
  pipeline/*.go's unrelated "domain" field (a different, legitimate
  concept for brain_ingest_raw) is untouched.
- internal/skills/brain/: deleted (orphaned legacy tool schema, dead
  since cmd/supervisor was removed).

Tests: added TestWriteNote_RequiresWingAndHall (api),
TestBrainWriteRequiresWingAndHall +
TestBrainWriteIgnoresLegacyTypeAndDomainParams (mcp) for the new single
behaviour; removed tests that only exercised the deleted legacy path
(TestWrite_IncludesFrontmatterWhenTypeProvided,
TestWriteNote_HallRouteExistingTypeWinsOverOptsType,
TestBrainWriteRejectsTraversal, TestBrainWriteAcceptsDoubleDotInName —
the latter two's filename-sanitisation behaviour is covered independently
by internal/brain/path_test.go's TestNotePath_SanitisesWingAndSlug /
TestSanitise, which is unaffected by this change).

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>

mathias referenced this pull request 2026-09-23 05:35:59 +00:00

chore(brain): deprecate + remove brain_write legacy params (domain/type/knowledge-path) (ADR-0012) #79

mathias referenced this issue from a commit 2026-09-23 05:40:18 +00:00

fix: hyperguild brain write requires wing/hall, not legacy type/slug

mathias added 1 commit 2026-09-23 05:40:18 +00:00

fix: hyperguild brain write requires wing/hall, not legacy type/slug ... 975b61c9d7

PR #131 removed WriteNote's legacy brain/knowledge/ fallback branch that
activated when wing/hall were both empty, but never updated the CLI that
was the only caller relying on it. Since `hyperguild brain write` never
sent wing/hall, every invocation of the CLI's most load-bearing brain
command started hard-erroring "wing and hall are required" — a live,
README-documented command broken on every call.

Separately, writeRequest never had a `slug` field (pre-existing, unrelated
to #131): the CLI's slug value was silently dropped server-side even
before this PR, falling back to a timestamp filename.

Fix:
- `hyperguild brain write <wing> <hall> <slug>` replaces the old
  `<type> <slug>` shape. Positional args, mirroring the existing
  `brain query <topic>` pattern and matching writeHallNote's own
  wing/hall/slug parameter order server-side (handler.go's
  brain.NotePath(brainDir, wing, hall, slug)) — no new flags needed since
  wing/hall/slug are all required, not optional filters.
- brainClient.Write now sends {wing, hall, filename, content}, matching
  writeRequest exactly (writeRequest already gained a Filename field in
  #131, so no server-side change was needed — only the CLI was sending
  the wrong shape). This also fixes the dead slug bug: slug now arrives
  server-side as filename instead of being silently dropped.
- Added TestWrite_CLIPayloadRoundTrips in ingestion/internal/api, which
  POSTs the CLI's exact JSON envelope to the real Handler.Write (not a
  mock) and asserts a real file with real wing/hall frontmatter lands on
  disk. cmd/hyperguild and ingestion are separate Go modules (no
  go.work), so this can't be a single cross-module test — instead the
  wire contract is pinned from the server side, closing the gap where
  cmd/hyperguild/http_test.go's TestBrainClient_Write_Success used only a
  hand-rolled httptest mock and stayed green through the wing/hall
  regression.
- Updated cmd/hyperguild/README.md and the 2026-05-03 CLI design spec's
  stale `<type> <slug>` examples.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>

mathias commented 2026-09-23 05:40:28 +00:00

Author

Owner

Copy Link

Copy Source

Fixed the regression found in review (commit 975b61c, pushed on top of this PR's branch).

Root cause (recap): WriteNote's legacy brain/knowledge/ fallback (activated when wing/hall were both empty) was removed by this PR, but hyperguild brain write — the only real caller — never sent wing/hall. Every invocation of the CLI's write command started hard-erroring "wing and hall are required". Separately (pre-existing, unrelated to this PR): writeRequest never had a slug field, so the CLI's slug was always silently dropped server-side.

Fix:

• New CLI shape: hyperguild brain write <wing> <hall> <slug> (was <type> <slug>). Chose three positional args over flags because wing/hall/slug are all required (not optional filters like query's wing/hall), and this mirrors writeHallNote's own parameter order server-side (brain.NotePath(brainDir, wing, hall, slug)).
• brainClient.Write now sends {wing, hall, filename, content}, matching writeRequest exactly — writeRequest already had a Filename field from this PR, so no server-side change was needed, only the CLI's payload. This also fixes the dead-slug bug: slug now arrives as filename instead of being dropped.
• Closed the test gap the reviewer flagged: added TestWrite_CLIPayloadRoundTrips in ingestion/internal/api/handler_test.go, which POSTs the CLI's exact JSON envelope to the real Handler.Write (not a mock) and asserts a real file with real wing/hall frontmatter lands on disk under a temp brainDir. cmd/hyperguild and ingestion are separate Go modules (no go.work), so this can't be one cross-module test — the wire contract is pinned from the server side instead, which is what actually catches this class of break (the old cmd/hyperguild/http_test.go mock couldn't).
• Updated cmd/hyperguild/README.md and the 2026-05-03 CLI design spec's stale <type> <slug> examples.

task check (test+vet+govulncheck+build+gitleaks across both modules) is green, exit 0, zero FAIL, verified directly (not grep-filtered).

Not merging — leaving that to you.

---

Created via git-mcp on behalf of @mathias

Fixed the regression found in review (commit 975b61c, pushed on top of this PR's branch). **Root cause (recap):** `WriteNote`'s legacy `brain/knowledge/` fallback (activated when wing/hall were both empty) was removed by this PR, but `hyperguild brain write` — the only real caller — never sent wing/hall. Every invocation of the CLI's write command started hard-erroring `"wing and hall are required"`. Separately (pre-existing, unrelated to this PR): `writeRequest` never had a `slug` field, so the CLI's slug was always silently dropped server-side. **Fix:** - New CLI shape: `hyperguild brain write <wing> <hall> <slug>` (was `<type> <slug>`). Chose three positional args over flags because wing/hall/slug are all required (not optional filters like `query`'s wing/hall), and this mirrors `writeHallNote`'s own parameter order server-side (`brain.NotePath(brainDir, wing, hall, slug)`). - `brainClient.Write` now sends `{wing, hall, filename, content}`, matching `writeRequest` exactly — `writeRequest` already had a `Filename` field from this PR, so no server-side change was needed, only the CLI's payload. This also fixes the dead-slug bug: slug now arrives as `filename` instead of being dropped. - Closed the test gap the reviewer flagged: added `TestWrite_CLIPayloadRoundTrips` in `ingestion/internal/api/handler_test.go`, which POSTs the CLI's exact JSON envelope to the **real** `Handler.Write` (not a mock) and asserts a real file with real wing/hall frontmatter lands on disk under a temp brainDir. `cmd/hyperguild` and `ingestion` are separate Go modules (no `go.work`), so this can't be one cross-module test — the wire contract is pinned from the server side instead, which is what actually catches this class of break (the old `cmd/hyperguild/http_test.go` mock couldn't). - Updated `cmd/hyperguild/README.md` and the 2026-05-03 CLI design spec's stale `<type> <slug>` examples. `task check` (test+vet+govulncheck+build+gitleaks across both modules) is green, exit 0, zero FAIL, verified directly (not grep-filtered). Not merging — leaving that to you. --- _Created via git-mcp on behalf of @mathias_

mathias merged commit f324f04944 into main 2026-09-23 05:43:28 +00:00

mathias referenced this pull request 2026-09-23 05:43:58 +00:00

chore(brain): deprecate + remove brain_write legacy params (domain/type/knowledge-path) (ADR-0012) #79

Sign in to join this conversation.

Reviewers

No Reviewers

Labels

Clear labels

agent/ready

Meets the agent-ready contract; authorizes unattended execution

blocked/client

Waiting on the client

blocked/supplier

Waiting on a supplier

blocked/upstream

Waiting on an upstream project or vendor

risk/high

Irreversible, security-relevant, or control plane

risk/low

Reversible, local, test-covered

risk/medium

Touches shared state, deploys or data

size/l

Too big - split it

size/m

Under two days

size/s

Under half a day

size/xs

Under an hour

status:dispatch

status:dispatched

status:review

status:reviewing

type/chore

Maintenance, no behaviour change

type/decision

Human decision required; never agent/ready

type/docs

Documentation only

type/feat

New capability

type/fix

Corrects broken behaviour

type/ops

Infrastructure, deploy, runtime

type/research

Investigate; output is a finding, not code

type/test

Test coverage only

No labels

Milestone

No items

No Milestone

Projects

Clear projects

No projects

Assignees

Clear assignees

renovate-bot mathias

No Assignees

1 Participants

Notifications

Subscribe

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: mathias/hyperguild#131