refactor(wechat): recover the store key in-container without host execution

[zhangfand]

What this PR does

The personal WeChat connection (wechat_user) reads a SQLCipher store whose key the client only ever derives at login and holds in memory. Recovering it needs ptrace on the client as it signs in. That one step ran as a host-root script: it entered the container's namespaces with nsenter and launched the client under gdb from the hosting VM.

That coupling made WeChat the only reason to turn on host execution — a per-host execution grant, the installed host helper, and the standing exposure of every tenant credential on the VM to root scripts. It was the largest blocker to offering the connection more widely.

The container already carries the capability the debugger needs (SYS_ADMIN, AppArmor unconfined, both in the base Compose file), and gdb traces a child it launched. So the capture can run as an ordinary local subprocess inside the container, with no host root and no namespace crossing. This PR makes that switch.

Design & Invariants

• Recovery is local to the container. recoverWechatPassphrase runs the staged driver directly through the runtime's process seam instead of a RootScriptRunner. The driver, the client launch, the breakpoint, and the key derivation all happen in the container, where the store already lives. The passphrase never leaves it.
• The debugger owns the client from birth. Unchanged: the driver launches the client under gdb rather than attaching, so the first login's one-time key derivation is caught without asking the guardian to scan twice.
• No host execution dependency. The connection no longer takes a runner, and registration no longer threads actionEngine / hostExecutionEnabled. Host execution remains in the tree as general infrastructure; after this change no built-in connection consumes it.
• Deployment requirements shrink to WECHAT_USER_ENABLED=true and ROME_DOCKER_USER_MODE=root. Root user mode is kept so the client, the capture, and the reader run as one user and share file ownership; the driver still targets /run/user/0. The 1 GB shared memory and the capabilities are already fixed in docker-compose.yml.

Test plan

• pnpm typecheck (in the Nix dev shell).
• WeChat unit tests: wechat-user-keys.test.ts and wechat-user.test.ts, 21 pass. Key-recovery tests now cover the in-process path and its failure branches.
• Full pnpm test:unit: the only failures are in scripts/docker/rome-apply-cdp-stealth.test.ts, pre-existing and environmental (it runs /bin/bash with PATH=/nonexistent and gets a null status on NixOS), unrelated to this change.
• Manual mechanism check in the production image with the Compose security profile: launched the driver in-container as an ordinary subprocess (no host helper), confirmed gdb traced the client (TracerPid pointed at gdb), the breakpoint armed across execs, and the client drew its login QR.
• End-to-end capture of a real passphrase requires a live phone scan; not run here.

Not in this PR

• rome-cloud provisioning. The enable-personal-wechat maintenance operation still preflights a host-execution grant and a running helper. Relaxing it to just write the two .env values and recreate the container is a follow-up in that repo.
• Dropping root user mode. Now that all three roles run as one container user, root mode may be unnecessary, but the driver hardcodes /run/user/0; changing that needs its own verification.

🤖 Generated with Claude Code

[zoolsher]

🔁 This review has been superseded. See the latest review.

[Jessie-QingYu]

🔁 This review has been superseded. See the latest review.

[zhangfand]

Both automated reviews explicitly describe their findings as optional or non-blocking. Following the repository's respond-to-review classification rule, I am declining those observations, grouped as preflight suggestions, error reporting/API cleanup, and comment/documentation cleanup. No runtime changes or PR claims were made in this response. Separately, CI failed because the new architecture paragraph contained a semicolon. Commit cb6a859 splits that sentence into two. All CI and CodeQL checks now pass on cb6a859. The latest review repeats the already handled cancellation and stale-comment findings, so the babysitting loop is complete under the no-new-points stop condition.

[zoolsher]

Code Review: ✅ APPROVE

This PR moves WeChat store-key recovery from a host-root nsenter+gdb script into an ordinary in-container subprocess driven through the existing runCommand process seam. The change is coherent and self-consistent: recoverWechatPassphrase drops the RootScriptRunner, anchorPid/pidNamespace guards, and the shell-generating rootScript/sq helpers; WechatUserSetupDeps/WechatUserDescriptorDeps shed ensureRecoveryAvailable/rootScriptRunner; and registration no longer threads actionEngine/hostExecutionEnabled. I verified there are no lingering references to the removed symbols outside the (intentionally retained) host-execution infrastructure, that the new wechat-user-keys.ts → wechat-user.ts import is not circular, and that the docs/env/compose text all match the new "no host execution" contract. Tests were updated to exercise the in-process path and its non-zero-exit and no-passphrase failure branches. Net security posture improves: the standing host-root grant and namespace crossing are gone, and the client/store were already in-container.

Overall this is safe to merge. The only observations are low-severity: an abort/cancel of the recovery now surfaces through runCommand as a misleading generic error, and a couple of comments referencing the old "root script" are now stale.

Verdict: APPROVE — A well-motivated, correctly-wired refactor that removes the host-root dependency and shrinks attack surface; only low-severity polish items remain.

2 finding(s) posted as inline comments below.

Severity	Category	File	Title
P3	error-handling	packages/core/src/channels/wechat-user-keys.ts	Cancelling recovery surfaces a misleading "python3 could not be run" error
P3	code-quality	packages/core/src/channels/wechat-user-launch-driver.py	Stale "root script" comment in launch driver

---

Automated review by RomeOS Code Review · commit cb6a859

[zoolsher]

[P3] error-handling — Cancelling recovery surfaces a misleading "python3 could not be run" error

Recovery now runs through runCommand, which rejects (rather than resolving) whenever execFile returns an error with a string .code. That includes AbortError (code: 'ABORT_ERR') when the guardian cancels the setup and ENOENT when python3/gdb is missing. In those cases the caller throws a generic Error("python3 could not be run: The operation was aborted") instead of a WechatUserKeyRecoveryError (or a clean cancellation), so a normal cancel reads as a spawn failure. The old RootScriptRunner path took the signal directly and could distinguish abort. Consider catching the abort/ENOENT case here and re-throwing a clearer message (e.g. treat signal.aborted as cancellation, and wrap other spawn failures as WechatUserKeyRecoveryError). Consistent with install/start today, hence low severity.

[zoolsher]

[P3] code-quality — Stale "root script" comment in launch driver

The docstring here is correctly updated to "Rome spawns it as a local subprocess," but the HOME comment a bit further down still reads "the root script sets it, so read it back rather than assuming /root." There is no root script anymore — Rome sets HOME via runCommand's env. Update that comment to avoid implying the removed mechanism. (The WechatUserStatus.pid doc comment in channels/wechat-user.ts — "The key-recovery step translates it, because host root sees a different number" — is now stale for the same reason, though that line is outside this diff.)

[Jessie-QingYu]

Code Review: 💬 COMMENT

This PR moves WeChat's store-key recovery from a host-root script (nsenter from the hosting VM into the container's namespaces, driven through RootScriptRunner/the action engine) to an ordinary local subprocess inside the container, and drops the actionEngine / hostExecutionEnabled wiring from connection registration. I verified the full post-change content of wechat-user-keys.ts, both wechat-user.ts files, the Python launch driver, the integrations barrel, and the index.ts call site, plus the Dockerfile, docker-compose.yml, and the docs, and traced every call site of the changed symbols.\n\nThe direction is right and the change is a net simplification and a security improvement: gdb tracing a child it launched needs no CAP_SYS_PTRACE, so the host-execution grant, the helper dependency, and the standing root-script exposure all fall away; the shell-string construction (rootScript() with its sq() quoting and NSpid scan) is replaced by execFile with an argv array, which removes an injection surface along with the anchorPid/pidNamespace entities. Nothing about the new shape is architecturally wrong. My findings are about what the removal left uncovered rather than about the mechanism: the ensureRecoveryAvailable preflight was deleted without a replacement, so environment misconfiguration now surfaces mid-scan instead of before a 744 MB download; the descriptor lost the only seam that let tests stub the capture; the driver discards gdb's log, which was tolerable when a preflight caught most failures and is not now; and several comments in untouched files (notably the Dockerfile block that installs gdb) still assert the host-root design this PR removes.

Verdict: COMMENT — The architectural direction is sound and well executed, but the deleted preflight leaves a misconfigured instance failing opaquely after a 744 MB install and mid-scan, and several comments — including the Dockerfile block that installs gdb — now assert the opposite of the new design.

7 finding(s) posted as inline comments below.

Severity	Category	File	Title
P1	error-handling	packages/core/src/connections/integrations/wechat-user.ts	No preflight left: a misconfigured instance now fails after the 744 MB install and after the guardian has scanned
P2	design	packages/core/src/connections/integrations/wechat-user.ts	The descriptor lost its injectable seam for key recovery
P2	code-quality	packages/core/src/channels/wechat-user-keys.ts	Comments elsewhere in the tree still describe the host-root path and now contradict the code
P2	documentation	docs/wechat-personal.md	docs/wechat-personal.md still credits the privileged helper with the PID-namespace check
P2	error-handling	packages/core/src/channels/wechat-user-launch-driver.py	gdb's log is discarded, so a failed capture reports nothing diagnosable
P3	architecture	packages/core/src/connections/integrations/index.ts	RootScriptRunner now has no production consumer
P3	security	packages/core/src/channels/wechat-user-keys.ts	The capture now hands Rome's full process environment to gdb and the WeChat binary

---

Automated review by RomeOS Code Review · commit cb6a859

[Jessie-QingYu]

[P1] error-handling — No preflight left: a misconfigured instance now fails after the 744 MB install and after the guardian has scanned

Removing ensureRecoveryAvailable removes the only environment guard, and nothing replaced it. It used to run in the ensure-runtime step before runtime.install, so a wrongly-wired instance failed fast with a sentence the guardian could act on.

Now the capture's prerequisites (ROME_DOCKER_USER_MODE=root, gdb present, /run owned by the runtime uid) are first touched inside recoverPassphrase, i.e. in the capture-login step — after a 744 MB download, the reader venv install, and after the scan view is already on screen. WECHAT_USER_ENABLED and ROME_DOCKER_USER_MODE are independent knobs and the latter defaults to multi, so the enable-one-forget-the-other case is easy to hit; it now surfaces as "Capture requires a private parent directory owned by the runtime user." mid-scan.

deps.stageDriver is already the seam for exactly this and is currently a no-op. Wiring the real staging there costs nothing and restores the early failure:

sessionScheme.setup = makeWechatUserSetup({
  runtime,
  recoverPassphrase,
  stageDriver: async () => { await rm(await stageCaptureDriver(), { recursive: true, force: true }); },
  ...
});

Better still, add an explicit check (process.getuid?.() === 0, since the driver hardcodes /run/user/0) with a message naming ROME_DOCKER_USER_MODE=root.

[Jessie-QingYu]

[P2] design — The descriptor lost its injectable seam for key recovery

rootScriptRunner was the descriptor's injection point for the capture; with it gone, createWechatUserDescriptor({ runtime }) has no way to stub recovery, so a test that drives the real descriptor through capture-login will call stageCaptureDriver() against the real /run and then spawn python3 — whose driver opens with pkill -x wechat. The rest of this module is careful about this (WechatUserRuntimeConfig.run, WechatUserSetupDeps.recoverPassphrase); this is the one path that now bypasses it. That is also why the removed "rejects missing host execution" test could be deleted rather than replaced — the descriptor-level path is no longer reachable from a test.

Add run?: RunCommand (or recoverPassphrase?) to WechatUserDescriptorDeps and thread it through. Relatedly, prefer options.run over the third positional parameter of recoverWechatPassphrase — callers currently have to pass undefined for signal to reach it, as wechat-user-keys.test.ts does.

[Jessie-QingYu]

[P2] code-quality — Comments elsewhere in the tree still describe the host-root path and now contradict the code

The PR updated the docs and this file's header but left several load-bearing comments asserting the opposite design. Most serious is Dockerfile:217-222, the place an operator or reviewer looks to understand why gdb is in the image:

"This does not grant the container ptrace: a normal container process still lacks CAP_SYS_PTRACE. The recovery runs gdb only through host root, which enters this container's namespaces from the VM"

That now states the exact claim this PR disproves. Also stale:

• packages/core/src/channels/wechat-user.ts:17-20 ("that half runs as root on the hosting VM"), :138-140 (WechatUserStatus.pid — "The key-recovery step translates it, because host root sees a different number"), :242-247 ("the one privileged step runs on the hosting VM instead"), :629-630 (readHelperSource — "callers that stage it elsewhere (the root script)").
• packages/core/src/config.ts:42-46 ("recovers its store key via a host-root script, so it needs host execution enabled") — .env.example was updated but its sibling in config.ts was not.
• packages/core/src/connections/integrations/wechat-user.test.ts:8 ("recovers the key via the injected root-script step"), in a file this PR edits.

[Jessie-QingYu]

[P2] documentation — docs/wechat-personal.md still credits the privileged helper with the PID-namespace check

Line 41 of this file, just below the rewritten section, still reads:

"The privileged helper matches both the container PID and its PID namespace before entering it."

That sentence described rootScript()'s NSpid scan, which this PR deletes. It is in the paragraph a reader consults for the connection's security properties, so leaving it implies a check that no longer exists (and no longer needs to). Drop it and keep the surrounding two sentences about /run cleanup and 0600 key files, which are still accurate.

[Jessie-QingYu]

[P2] error-handling — gdb's log is discarded, so a failed capture reports nothing diagnosable

gdb's stdout and stderr go to gdb.log inside the TemporaryDirectory, which is removed when the driver exits (and the staging dir is rm -rf'd by the caller right after). When gdb cannot trace — the likeliest failure of the new in-container path, e.g. a Compose file without apparmor:unconfined or a seccomp profile blocking ptrace — wait_for_capture sees proc.poll() return immediately and exits with only "ERROR no passphrase captured" on stderr. That string is what parsePassphrase splices into the guardian-facing error.

This mattered less when the host-execution preflight caught most misconfigurations up front; it is now the only signal there is. Echo the tail of gdb.log (and gdb's exit code) to stderr before sys.exit(5) — the log contains the GDB_READY / BP_SET= markers the driver already emits, so a few hundred bytes distinguishes "gdb never started" from "breakpoint never armed" from "guardian never scanned".

[Jessie-QingYu]

[P3] architecture — RootScriptRunner now has no production consumer

After this change createActionRootScriptRunner and the RootScriptRunner / RootScriptRequest / RootScriptOutcome triple in packages/core/src/host-execution/root-script-runner.ts are referenced only by root-script-runner.test.ts. The PR description frames this as intentional ("host execution remains in the tree as general infrastructure"), and HostExecutionService itself is still reachable from the action, so this is a judgement call rather than a defect — but the wrapper is a convenience layer built for exactly one caller that no longer exists, and it will drift untested-against-reality.

Either delete it with the caller (it is recoverable from git) or add a line to its header naming the intended future consumer, so the next reader does not have to re-derive that it is deliberately dormant. The same applies to WechatUserStatus.pid, whose only documented purpose was the PID translation this PR removes and which has no remaining consumer outside tests.

[Jessie-QingYu]

[P3] security — The capture now hands Rome's full process environment to gdb and the WeChat binary

runCommand merges rather than replaces: env: { ...process.env, ...opts.env }. So { HOME: options.home } means the driver, gdb, and the closed-source WeChat client inherit every variable in Rome's environment — the whole .env, API keys included. Under the old path the script ran in the host helper's environment, so the client saw essentially nothing of Rome's.

startClient already leaks the same set on the ordinary launch path, so this is not a new exposure so much as a second instance of one, and it is worth noting given the PR's stated goal of shrinking credential exposure. A replaceEnv?: boolean (or envMode: "replace") on RunOptions, used by both launch sites with the explicit clientEnv() set, would close both.