feat(config): config.env is written by a command, not by the agent (ADR 0017) - #632
Open
nevamind-agent wants to merge 9 commits into
Open
feat(config): config.env is written by a command, not by the agent (ADR 0017)#632nevamind-agent wants to merge 9 commits into
nevamind-agent wants to merge 9 commits into
Conversation
…se (ADR 0017) `~/.memu/config.env` is shared by every host on the machine, carries a plaintext credential, and holds an invariant a guide can only state in words — record and inject must agree on the backend, or retrieval silently returns nothing. Until now the agent wrote it by hand from `INSTALL.md` §1.2, and field behaviour matched the exposure: agents rewrite the file rather than merge into it, taking another host's settings with them. Two verbs over one writer. `init` is the inferring front door named in `SKILL.md` — the surface memU cannot refresh from the server, so its contract is one optional flag. `config` is the explicit one driven by `INSTALL.md`, which is server-refreshable and can grow flags at the server's pace. `config show` is the read side, and is a name rather than a bare `config`: a verb that printed while bare `init` wrote is the asymmetry agents get wrong. The guard is state-based, and reads the *file*. Guarding on declaration alone breaks the most ordinary first install — bare `init` lands `local`, the guide then asks the question, the user says cloud, and the flip is refused as if it were a real one. So a mode change is refused only when the current mode has something to lose: `MEMU_DB` set, or a stored cloud key. Reading that state through `env.env()` would be equally wrong in the other direction, since it resolves the shell first: an exported credential must not make a file that declares nothing look like a configured install. A file with no `MEMU_MEMORY_MODE` at all counts as local, not as vacuous — that is the population most likely to hold a large legacy store. `client_instance_id()` moves onto the same writer. `init` now persists `MEMU_CLIENT_ID` too, and read-modify-write plus `os.replace` would silently discard the append it used to do; its fail-open `except` and its post-write re-read survive the move. This commit is the logic only — no guide names either verb yet, so nothing invokes them. `SKILL.md` and the seven `INSTALL.md` §1.2 sections follow separately, and must not be published server-side ahead of the release carrying this: `docs install` prefers the server's copy, so a guide naming `config` would reach machines whose binary has no such subcommand. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…wn event `cli_install_started` fired from `docs install`, because printing the guide was the first act on the install path that proved `memu-cli` resolves. ADR 0017 put `init` one step earlier — `SKILL.md` Step 2 — and left where the event belongs as an open issue. It belongs there: `init` proves the same thing sooner and additionally holds the two facts the event most wants, the memU Cloud key and `MEMU_CLIENT_ID`. So the start moves, and it is emitted *after* the write rather than beside the client id. `client_instance_id()` reads `MEMU_CLIENT_ID` from the resolved environment, and the id `_client_id` generates is not there until `_write` has persisted it and reloaded — report first and the event mints a second id, persists it, and is overwritten, leaving the machine's first event reporting under an id its own config no longer contains. The alternative of an extra `_write` early to bank the id buys no durability, since nothing between the two points does I/O or can raise, and costs a second read-modify-write and a `config` row printed twice. In the `--cloud-api-key` branch the report sits below the orphaned-store warning: it blocks on a POST, and the warning is the line a user has to read. The same ordering is what attaches the key to the header, so the first envelope of an install is attributable rather than anonymous — the attribution ADR 0017 was written for, now unconditional instead of dependent on the user having configured cloud before Step 3. The old emission is renamed, not deleted. `docs install` still records `install_guide_opened` and still flushes there; what it observes is unchanged and worth a row, but it is the funnel's second step now, and a second name ending in `_started` beside the real one is the pair a consumer sums by accident. Both legs stay code-observed, so `started >= opened >= succeeded` holds structurally against a prose-driven `report install` that undercounts. Consumers must update: `cli_install_started` means `init` ran from this release on, and history below the cutover means the guide was printed. `context.client_version` is the discriminator, as it was for the `memory_update` change. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
evan-ak
force-pushed
the
feat/config-env-command
branch
from
August 9, 2026 20:24
d636fba to
eb21f33
Compare
`memu.events` and `memu.hosts.templates` reach a memU server with `urllib`
rather than the `httpx` the rest of the package uses, so both verify against
whatever trust store OpenSSL was pointed at. A python.org framework build whose
bundled `Install Certificates.command` was never run is pointed at nothing —
neither `cafile` nor `capath` — and every HTTPS call raises
CERTIFICATE_VERIFY_FAILED. On two fail-open paths that is not an error anyone
sees: `_post` reads it as RETRY forever, so `report flush` prints `delivered 0
event(s)` while `.sending` files accumulate until `MAX_SPOOL_BYTES` starts
dropping, and `_get` reads it as "the server is unreachable", so the machine
silently keeps its embedded templates and never says so. `httpx` carries
`certifi` and is unaffected, which is why `retrieve` keeps working on exactly
the machine where delivery reports zero — the divergence that made this take an
afternoon to find, with nothing in the client's output and nothing in the
backend's log, because the request never left the host.
A fallback, and deliberately not a pin. Handing `certifi` to these two sites
unconditionally would fix that machine and break the opposite one: a corporate
root CA installed OS-wide is in the system store and is not in `certifi`, so
every user behind a TLS-inspecting proxy would begin failing exactly as
silently. So the system store still wins wherever one exists, and `certifi`
only fills a vacuum.
Moving both sites to `httpx` was the other candidate and is rejected on the same
ground. Its stated obstacle is already gone — `events` imports `memu.config_file`,
which imports `memu/__init__`, which imports `MemoryService` and therefore
`httpx`, in every process that can reach either call site, and `httpx.post` is
sync — but it verifies against `certifi` too, so it carries the identical
regression while additionally re-opening redirect, timeout and proxy semantics on
two paths whose whole contract is that they never raise. One HTTP client in the
codebase is worth having; it is a refactor to make on its own merits, not one to
smuggle in behind a bug fix, which is precisely how that regression would ship
unnoticed. `_post`'s docstring still cites the async stack as the reason for
`urllib` and is now wrong about it; correcting that is left to the change that
settles the question.
`urlopen_kwargs` returns `{}` rather than a context on a healthy machine, so the
call site passes no `context` argument at all. That makes an install that works
today unchanged by construction instead of unchanged by inspection, and it is
why no existing `urlopen` stub in the suite needed its signature touched. The
answer is cached because building the fallback parses `certifi`'s whole PEM
bundle and one `flush` may POST `MAX_FLUSH_POSTS` times, and every failure
returns `None` — including `certifi` being absent, since it arrives as an httpx
dependency rather than one memU declares. This must not become the reason a
fail-open path stops being one.
Verified on the interpreter that was broken, with no `SSL_CERT_FILE`: TLS now
completes and a spooled event flushes `(1, 0)` where it previously returned
`(0, 0)` and retained its file. `tests/test_trust.py` covers both directions,
because the fix has two ways to be wrong — a healthy store must be left alone,
and the vacuum must be filled with `CERT_REQUIRED` and `check_hostname` intact,
a fallback that disabled verification being worse than the bug it closes.
Refs #633
Two events were left to a later flush on the argument that one would come. In both cases the run that most needs reporting is the one where it doesn't. `memory_search_failed` was spool-only, deliberately, on the reasoning that a store the hook cannot reach fails `retrieve` on *every* turn — so that is when a per-turn blocking POST is least affordable. The reasoning held for the cost and inverted the value. The same unreachable store breaks `prepare` and `commit`, so the later flush this event waited for is the machinery its own failure implicates: a machine whose retrieval is broken reported least, which is the silent-failure mode ADR 0016 §5 names as its whole motivation. It now delivers like the success leg. That cost is accepted, not avoided, and the ADR says so in those words: a fully broken store adds up to `_TIMEOUT_SECONDS` to each turn until it is fixed. What makes it payable is that it is a constant — bounded by the timeout rather than by how far behind the spool has fallen, and the same one the success leg already pays. What does not follow is a flush. `host_cli.run` still exempts `retrieve` from the error handler's, because a drain there costs up to `MAX_FLUSH_POSTS` requests per turn, and that exemption is untouched: the `cli_error` behind a failed retrieve is still spooled. One POST versus all of them remains the distinction the whole design rests on. `cli_install_succeeded` was spooled on the argument that a completed install goes on to bridge and will flush there. That assumes the thing the event attests to. An install whose scheduled task never registered still reaches `INSTALL.md`'s last step and still reports completing, and it is exactly the run that never flushes — so the funnel lost its terminal row on the machines whose funnel matters most. `init` and `docs install` already flush for this reason; leaving one step of three behind also gave the completion a delivery latency unlike its own siblings', for nothing a consumer could use. Its `--help` now says "delivers immediately", as `uninstall`'s already did. Three tests move with it. `test_a_failing_retrieve_never_posts_from_the_per_turn _hook` is rewritten rather than patched — its name, docstring and assertion were the old decision — and now pins the half that did not change alongside the half that did. The install-funnel test expects three posted names and an empty spool. `test_report_verbs_exist_on_every_host` gains the `_Posted` stub it never needed before: without it `report install` now resolves the fixture's `example.invalid` for real, retries, and `_retain` writes the events back to the rotated `.sending` file, where `_spooled()` does not look — so the test read 0 events that were never lost, only moved. It also stops touching the network. The `deliver=True` tripwire stays file-granular and now says why: what it guards is which command may spend a POST per turn, not how many branches of that command do. ADR 0016 §2 and §4 carry both changes as amendments with the superseded reasoning kept, since both arguments are ones a later reader would otherwise make again. `flush()`'s docstring listed its callers and had already fallen three behind — `init`, `docs install`, `report error` — and is corrected along with the two. 439 unit tests pass; not yet exercised against a live endpoint.
6 tasks
1 task
Contributor
|
进度在这里 track （这次复核的点都 resolve 了）: 第五个点建议 patch PR 在这: |
This was referenced Aug 12, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
📝 Pull Request Summary
Implements ADR 0017 end to end:
~/.memu/config.envis now written by two commands —<binary> initand<binary> config— instead of by the agent, from prose, during the install. The ADR itself is included, along with the guide changes that drive the new verbs and the one open issue the last commit closes.✅ What does this PR do?
Four commits, in dependency order:
feat(config): writeconfig.envfrominitandconfig, not from prose — the writer (memu.config_file), the two verbs (memu.hosts.config_cmd), and the guards. Logic only; nothing invokes it yet.fix(config):initnever rejects on mode —initis the first command on the install path and has no--forceto hand back, so a refusal there strands every step after it. It warns instead.feat(config): name the verbs inSKILL.mdand all sevenINSTALL.md§1.2 sections — the guide side of the same change.feat(events): move the install-start event toinit, rename the guide's own event — closes ADR 0017's "where the install-start event belongs" open issue.Two verbs over one writer.
initis the inferring front door named inSKILL.md— the surface memU cannot refresh from the server, so its contract is one optional flag and is meant to hold for years.configis the explicit one driven byINSTALL.md, which is server-refreshable (ADR 0013) and can grow flags at the server's pace.config showis the read side, and is a name rather than a bareconfig: a verb that printed while a bareinitwrote is the asymmetry agents get wrong.The guard is state-based, and reads the file. A mode change is refused only when the current mode has something to lose —
MEMU_DBset, or a stored cloud key. Guarding on the declaration alone would refuse the most ordinary first install; reading the state throughenv.env()would be wrong in the other direction, since an exported credential in the calling shell must not make a file that declares nothing look like a configured install.The install funnel gains its true first step.
cli_install_startednow fires frominit(SKILL.mdStep 2) rather than fromdocs install(Step 3), and is emitted after the config write —initmintsMEMU_CLIENT_IDinto that same file, so an envelope built first would find no id, persist a second one, and be overwritten. The old emission is renamed toinstall_guide_openedand is otherwise untouched.🤔 Why is this change needed?
~/.memu/config.envis shared by every host on the machine, carries a plaintext credential, and holds an invariant a guide can only state in words: record and inject must agree on the backend — and in local mode on the DSN and embedding space too — or retrieval silently returns nothing (ADR 0009's opening argument).Until now
INSTALL.md§1.2 told the agent which keys to write and the agent wrote them, typically with a heredoc orecho >>. Field behaviour matched the exposure: agents rewrite the file rather than merge into it, taking another host's settings and the user's comments with them. The guide already spends a paragraph on "repair the connection, never the identity" and enforces it with nothing but emphasis.This is the same argument that produced
install-instruction: memU owns the text, so memU writes it.config.envis a strictly harder file — merge semantics, a secret, a cross-host invariant — and had no such command.Secondarily, the install-start event fired before the memU Cloud key existed, so a first install reported anonymously.
initholds the key and writes it before reporting, which makes attribution unconditional.🔍 Type of Change
✅ PR Quality Checklist
SKILL.mdand all sevenINSTALL.mdguides updatedThe breaking change
cli_install_startedchanges subject. From this release it meansinitran; history below the cutover means the install guide was printed. Nothing errors at the boundary —context.client_versionis the discriminator, the same treatment thememory_updaterename got in ADR 0016.install_guide_openedis a new name for the old event, so nothing that was observable stopped being observable; the funnel gained a step rather than trading one. Ingest needs no schema change — both carry emptyproperties.Deploy ordering⚠️
The guides in this PR must not be published server-side ahead of the release carrying the code.
docs installprefers the server's copy (ADR 0013), so a guide namingconfigwould reach machines whose binary has no such subcommand.📌 Optional
tests/test_config_cmd.py(406 lines: merge semantics, both guards,--force, legacy files declaring no mode, shell-exported values that must not be read) and the funnel tests intests/test_events.pyargvand memU mines transcripts; nothing validates the key untildoctor; local mode's flag surface will keep growing431 passed, 1 skipped; ruff and mypy clean.