feat(witan): name config targets on both ends of `migrate merge` by blarghmatey · Pull Request #266 · mitodl/agent-kit

blarghmatey · 2026-08-20T15:17:12Z

What

witan migrate merge gains --from <name> and --to <name>, which resolve
[targets.<name>] config blocks. --target keeps its existing meaning of a
literal store URI.

witan migrate merge [SOURCE] [--from <name>] [--to <name>] [--target <uri>] [--dry-run]

--from <name> uses the block's server as the source. A target carrying
only a remote_url (ci/qa/production) has nothing to export — there is
no remote-export tool — so it fails by name instead of silently no-opping.
--to <name> builds that target's destination directly: the deployment's
RemoteServerProxy when it has a remote_url (the same object
WITAN_TARGET=<name> produced ambiently, chosen on the command line
instead), or its server as a target URI when it does not. The proxy still
refuses a client-named target; --to sidesteps that by building the proxy
rather than passing a URI through it.
--to and --target are mutually exclusive, as are SOURCE and --from.
Neither flag given resolves exactly as before: positional source, ambient
_srv().

Why

Raised 2026-08-19 during the go-live. migrate merge --target <the deployment's MCP URL> --dry-run failed confusingly (#260 documented the workaround), because
--target here means a store URI while witan login --target <name> means a
config target. The only supported destination selector was export WITAN_TARGET=ol — ambient, invisible in the command line, and silent when
absent: the merge runs happily against your local store instead.

Overloading --target to mean a name when it happens to match one was the
alternative considered. Rejected: a store path colliding with someone's target
name would silently change where the merge writes, depending on config-file
contents at the moment of the run. Two flags with fixed meanings read the same
way every time.

Docs

docs/migration-runbook.md is now a sequence of steps — register/login,
dry-run, merge, verify, keep the backup — with the cutover step spelled
witan migrate merge ~/.local/share/witan/graph.omni --to ol instead of an
export WITAN_TARGET line. The verified --mode merge collision behaviour,
the slug-collision arithmetic, and the BM25 measurement behind "verify by slug,
not by search" move to docs/store-merge-findings.md, linked once from the
runbook. The #local--shared-the-cutover anchor other docs link to is
preserved.

README.md, docs/CLI_REFERENCE.md, and docs/deployed-witan-onboarding.md
are updated for the new flags (the onboarding doc claimed --target existed
only on login/logout/whoami).

Tests

mcp/servers/witan/tests/test_migrate.py gains 11 tests: by-name resolution of
--from/--to (local and remote), the remote-only-source refusal, both
mutual-exclusion errors, the missing-source and misconfigured-target errors, a
pin that no-flags resolution is unchanged, and an integration test that merges
two named local stores end to end and asserts newest-wins reconciliation plus
repeatability.

just test-witan-council: 872 passed. prek run clean on the changed files.

Overlaps with #260

#260 is still open and edits the same runbook sections. It removed the "If you
have cluster credentials" port-forward subsection (same --target <URI>
confusion) — this branch removes it too — and added a note that
export WITAN_TARGET=ol is the only supported cutover path, which --to <name>
now replaces. #260 can be closed in favour of this, or rebased onto it.

🤖 Generated with Claude Code

https://claude.ai/code/session_01HhndvRa6kXACdTbvbTErLA

`--target` on `migrate merge` is a store URI, while `--target` on `login`/`whoami` is a config target name. During the go-live that cost an hour: `migrate merge --target <the deployment's MCP URL>` fails confusingly, and the runbook's only supported answer was `export WITAN_TARGET=ol`, which is invisible in the command line and fails silently when it is missing. Two new flags with fixed meanings, so nothing is inferred from the string: witan migrate merge [SOURCE] [--from <name>] [--to <name>] [--target <uri>] `--from <name>` takes the block's `server` as the source; a target carrying only a `remote_url` has nothing to export and is refused by name rather than silently no-opping. `--to <name>` builds that target's own destination — the deployment's proxy when it has a `remote_url` (what `WITAN_TARGET=<name>` produced ambiently), its `server` as a target URI otherwise. `--to` and `--target` are mutually exclusive. With neither flag, resolution is byte for byte what it was: positional source, ambient `_srv()`. Overloading `--target` to mean a name when it matches one was the alternative. Rejected: a store path colliding with a target name would silently change where the merge writes, depending on config-file contents at the moment of the run. The runbook is now a sequence of steps. The verified `--mode merge` collision behaviour, the slug-collision arithmetic, and the BM25 measurement behind "verify by slug, not by search" move to docs/store-merge-findings.md, which the runbook links once. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HhndvRa6kXACdTbvbTErLA

Copilot

Pull request overview

Adds explicit named source and destination targets to witan migrate merge.

Changes:

Adds --from and --to target resolution.
Adds merge-target tests and proxy construction support.
Reorganizes and updates migration documentation.

Reviewed changes

Copilot reviewed 9 out of 9 changed files in this pull request and generated 2 comments.

Show a summary per file

File	Description
`witan/config.py`	Adds named-target loading helpers.
`witan/cli/migrate.py`	Implements the new merge flags.
`witan/cli/_common.py`	Extracts remote proxy construction.
`tests/test_migrate.py`	Tests named merge targets.
`README.md`	Documents the new syntax.
`docs/store-merge-findings.md`	Records merge behavior findings.
`docs/migration-runbook.md`	Revises migration procedures.
`docs/deployed-witan-onboarding.md`	Explains target selection.
`docs/CLI_REFERENCE.md`	Documents the new flags.

Suppressed comments (1)

mcp/servers/witan/witan/cli/migrate.py:140

The local/self-hosted destination path has the same incomplete target resolution: only server is passed onward. A configured target with a separate graph (the supported witan target add --server ... --graph ... shape) is therefore rejected as a bare remote server, and its target-specific credential cannot be used; file:// servers are also malformed by _resolve_path. Build a complete destination descriptor from the named block rather than passing only block.server.

        return server_module, cfg_module._resolve_path(block.server)

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

`--from`/`--to` took the block's `server` string and dropped everything beside it, so three configs that work through `WITAN_TARGET` broke through the flag that replaces it: - A target with `server = "http://host:8080"` and `graph = "council"` reached `store_cli_args` as a bare server with no graph id and was rejected. The id is now folded into the URI as `/graphs/<id>`, the spelling that function already reads. - A target with its own `token` was silently authenticated with whatever `OMNIGRAPH_BEARER_TOKEN` happened to be exported. Nothing on the merge path carries a per-store credential, so this refuses by name instead. - `server = "file:///tmp/a.omni"` came back from `config._resolve_path` as `file:/tmp/a.omni` — `Path()` collapses the `//` — which then slipped past `merge_store`'s `file://` strip and read as a relative path named `file:`. Stripped directly instead. Also corrects `load_target`'s docstring, which had the global-config precedence backwards (env > target > global, not global > target), and normalises whitespace in the end-to-end merge assertion: rich wraps the summary line to the terminal width, so a raw substring match was a width test and failed under CI's narrower console. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HhndvRa6kXACdTbvbTErLA

…ng a command that does not exist The runbook told people to verify a merge with `witan memory show <slug>`. There is no such command — `witan memory` is search-or-list only, so the one step the "verify by slug, not by search" rule exists to protect had no working spelling. Replaced with the two reads that do exist and do bypass BM25: a `witan memory --kind <kind>` listing and `witan task <slug>`. Both smoke-tested against a scratch store; same fix in store-merge-findings.md and deployed-witan-onboarding.md, which repeated it. Adds a paste-into-your-agent prompt for the local-to-shared cutover alongside the manual steps. The guardrails are the content: stop and ask when the dry run's `updated` count is large (colliding slugs, each one dropping a record), never read an empty search as a failed merge, never clean up the local store. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HhndvRa6kXACdTbvbTErLA

…hared graph A local store accumulates whatever you worked on, personal repos included, and `migrate merge` is all-or-nothing — there is no filter flag. So the cutover now starts by exporting and counting rows per repo, and, when something on that list should not go, merging a filtered export instead of the store. The filter is two `jq` passes rather than one: `from`/`to` on an edge are slugs, so dropping a node without dropping the edges that touch it leaves them dangling. Both passes verified against a fixture covering a work row, a personal row, a `(no repo)` row, and edges on either side of the cut. `(no repo)` is called out explicitly because the reflex is wrong: those are mostly general engineering lessons belonging to no checkout, which are the ones most worth sharing. The agent prompt gets the same step, with the decision reserved for the human — it inventories and flags, then stops and asks which rows to leave behind. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HhndvRa6kXACdTbvbTErLA

blarghmatey · 2026-08-20T15:49:36Z

Addressed both Copilot threads (f428f5e) — both were real and both are fixed and resolved: a named target's graph id and token were being dropped when only its server was read, and file:// was mangled by _resolve_path's Path() round-trip. Five tests added.

Also fixed the CI failure, which was mine: the end-to-end merge assertion matched a raw substring of rich's output, so it was really a terminal-width test and broke under CI's narrower console. Now normalised.

Two follow-ups pushed since, per review in-session: the runbook's agent prompt for the cutover (468bf07), and a take-stock step that inventories the local store by repo before merging it into a shared graph, with a two-pass jq filter for anything that should stay behind (4aaa9f3). That commit also drops witan memory show <slug> from the docs — there is no such command; witan memory is search-or-list only, so the one step 'verify by slug, not by search' exists to protect had no working spelling.

witan-code (code graph) is still red and is not from this branch, which touches no witan-code file: the pinned omnigraph edge digest no longer matches upstream, so install_omnigraph refuses (correctly) and the tests then fail on a missing binary. The other three omnigraph jobs are green only because they restore a cached binary from before the tag moved. Filed as tk-witan-code-ci-is-red-for-every-pr-the-pinned-omn-ca3e6b — refreshing a pinned digest is a deliberate call, not something to slip into this PR.

…ts error-text rename `edge` was force-pushed past the pinned digest, so `install_omnigraph` refused (correctly) and every witan-code CI job since 2026-08-20T11:00Z has failed on a missing binary. The other three omnigraph jobs stayed green only because they restore a cached binary from before the move — that cache was hiding a broken pin, not proving a working one. Repins all three tiers to the 17:18Z build (through bee47cd465), verified by downloading each tarball, hashing it locally, and cross-checking the release's published `.sha256` in the same sitting. Version still reports 0.10.0 and internal-schema still 6, both read off the binary, so no format migration. Eight commits landed since the old pin. Five are RFC/docs and one is upstream's test inventory; two are a vocabulary sweep worth reading: - #534 renames the error PROSE. Two substrings `_classify_cli_error` matches on vanished — "manifest table version" and "ahead of manifest". Both turn out to be redundant (the same sentences still carry "refresh and retry" and "omnigraph repair"), so classification never actually broke, but the markers were dead. Added the new spellings beside the old ones, which stay because the list is matched against whatever binary is INSTALLED and this rename shipped without a version bump — both spellings are in the wild at once. - #538 renames the JSON output surface (`rows_loaded` → `entities_loaded`, `tables` → `nodes`/`edges`, …). Breaking for anyone parsing it; witan is not, since it runs the CLI without `--json` on purpose and reads stderr prose. The new retryable marker keeps its "expected " prefix deliberately: the bare phrase "published dataset version" also appears in "historical published dataset version N was reclaimed", which is terminal, and matching it would newly retry a permanent failure until the attempt budget ran out. Six tests pin all three messages in both vocabularies. Also records what this cost: `edge` moved three times in 75 minutes while this was being written. The comment now says to prefer a real `v<version>` tag as soon as 0.10.x has one — there is no v0.10.0 release yet, which is the only reason this is still on a moving tag. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HhndvRa6kXACdTbvbTErLA

…'s two names The binary-contract suite did its job: repinning to the current `edge` build turned three of its tests red, and one of them was covering real production code rather than a wording detail. `ecf1d6aedd` (#538) renamed `commit list --json`'s `manifest_branch` to `graph_branch` with no version bump. `witan_code.graph.branch_last_write` filters on that key, so under the new build no commit matched, `stamps` came back empty, and the function returned None. Its own docstring is what makes that serious: None is load-bearing there — it tells the reaper the branch has no commits of its own and must never be touched. The reaper would have gone quietly inert, reporting success while branch views accumulated, and nothing would have raised. Now reads either key, same dual-spelling reasoning as the error markers in the previous commit: both builds report 0.10.0, so both are in the wild at once. The contract tests move to checking "a branch tag under one of the known keys" rather than one literal, so they pass on either binary and still fail if the tag disappears altogether — which is the failure they exist to catch. Same for the row-cap refusal, where "keyed rows" became "keyed entities"; only the wording moved, and `chunking.py`'s quote of it is updated to say so. Verified by installing the newly pinned binary locally: all 20 contract tests pass against it, witan-code's reaper tests pass, and witan-council's 877 pass. The four `test_branches`/`test_graph` failures left on this machine are pre-existing and local — they reproduce identically with these two files restored to HEAD. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HhndvRa6kXACdTbvbTErLA

witan-council 0.19.0 ships `witan migrate merge --from/--to`, which is only reachable once it is published — the flags have been on main with no released version carrying them. witan-core 0.27.0 carries the refreshed omnigraph digests. That release is load-bearing rather than incidental: 0.26.0 pins the moved `edge` digest, so a fresh `uv tool install` resolving it gets a `witan setup` that fails the checksum and installs no binary at all, and witan/server.py bootstraps a graph at import time — the CLI is unusable, not degraded. Hence the witan-core floor moves to >=0.27, the one entry in that list that is not about a missing symbol. Same consequence, different cause, so it is spelled out where the next reader will look for it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HhndvRa6kXACdTbvbTErLA

Copilot AI balanced review requested due to automatic review settings August 20, 2026 15:17

Copilot AI reviewed Aug 20, 2026

View reviewed changes

Comment thread mcp/servers/witan/witan/cli/migrate.py Outdated

Comment thread mcp/servers/witan/witan/config.py Outdated

blarghmatey and others added 3 commits August 20, 2026 11:35

Copilot started reviewing on behalf of blarghmatey August 20, 2026 17:36 View session

blarghmatey and others added 3 commits August 20, 2026 16:01

blarghmatey force-pushed the worktree-witan-migrate-from-to branch from bdd2acc to 4a06fb0 Compare August 20, 2026 20:24

Navigation Menu

Uh oh!

feat(witan): name config targets on both ends of `migrate merge` - #266

feat(witan): name config targets on both ends of `migrate merge`#266
blarghmatey wants to merge 7 commits into
mainfrom
worktree-witan-migrate-from-to

blarghmatey commented Aug 20, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Uh oh!

blarghmatey commented Aug 20, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Uh oh!

Conversation

blarghmatey commented Aug 20, 2026

What

Why

Docs

Tests

Overlaps with #260

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Uh oh!

blarghmatey commented Aug 20, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants