Skip to content

feat(core-only): expose safe session host facts - #1761

Merged
deepcoldy merged 8 commits into
deepcoldy:masterfrom
Oliver-Ysq:codex/core-only-host-facts
Oct 10, 2026
Merged

deepcoldy merged 8 commits into
deepcoldy:masterfrom
Oliver-Ysq:codex/core-only-host-facts

Conversation

@Oliver-Ysq

Copy link
Copy Markdown
Contributor

改动

  • 为 core-only 增加只读的会话宿主事实接口:CLI、后端、worker 就绪状态和三态存活结论;不暴露 PID、环境变量、终端写入能力或宿主密钥。
  • botmux serve --api-only 支持显式选择 tmux 或 pty 后端,默认仍为 tmux。
  • core-only 的 dispatch-report 密钥收敛到显式 state-dir;安全迁移旧位置密钥,不安全文件直接拒绝。

兼容性与影响面

  • 普通 fleet 的默认后端、路由鉴权和历史密钥路径保持不变。
  • 新增无签名访问仅限 core-only 的紧致只读路由;普通 daemon 仍需要既有 HMAC。
  • 非法 core-only 后端值 fail-closed,不回退到默认值。

验证

  • ./node_modules/.bin/tsc --noEmit
  • bun run typecheck:scripts
  • bun run typecheck:test-mocks
  • bun run test -- test/bot-registry.test.ts test/api-only-mode-wiring.test.ts test/core-only-host-facts.test.ts test/dispatch-report-binding.test.ts(224 passed)
  • TraeX + core-only PTY 真机 smoke,以及自定义 state-dir 与旧密钥迁移 smoke。

Closes #1760

@deepcoldy

Copy link
Copy Markdown
Owner

你好 @Oliver-Ysq,这个 PR 的评审群已经建好,欢迎进群跟进评审讨论(一年有效入群链接):
https://applink.larkoffice.com/client/chat/chatter/add_by_link?link_token=ee5p458f-4936-436d-9307-33382d40b2f5

目前自动拉群名单里暂时没有你的飞书信息(可在此名单文档按 GitHub 账号补录,后续评审会自动拉你进群):
https://bytedance.larkoffice.com/wiki/WJ1nwWbtxi89erkNGNbcgkt9nUe

这是自动评审流程发出的消息,如有不便敬请谅解。

@deepcoldy

Copy link
Copy Markdown
Owner

Thanks for the PR! This is an automated first-pass review (final call always rests with the maintainer). The overall design looks solid — the tri-state liveness (unknown never collapses into missing), the tight core-only allowlist that keeps HMAC on for the fleet, fail-closed invalid --backend values in both layers, and the fleet secret path staying byte-for-byte the same are all exactly right. Local rebase onto current master (which now includes #1751/#1759) is clean apart from one test-file conflict; tsc, both auxiliary typechecks, bun run build, and the PR's named test files all pass on the rebased tree.

One blocking issue and a couple of test gaps:

P1 — fresh boot with --state-dir /tmp/<state> (the literal shape from #1760, e.g. mktemp -d) crashes before the port binds.

The eager loadOrCreateDispatchReportBindingSecret(config.session.dataDir) in startDaemon runs unguarded before startIpcServer. When the new in-state leaf is absent, the core-only branch probes the legacy path, and with a state dir directly under a sticky 1777 /tmp the legacy path is /tmp/.dashboard-secret.report-binding. readSecureHostFileSync() validates the parent before it can observe that the leaf is absent — assertSecureParentStats throws UnsafeHostAuthorityFileError ("...可被组内或其它用户写入") for the 1777 parent, so "no legacy to migrate" is indistinguishable from "unsafe legacy", and the process exits 1.

Repro (on Linux, /tmp is 1777), against an otherwise unused state dir:

mkdirSync('/tmp/<state>', { recursive: true, mode: 0o700 });
loadOrCreateDispatchReportBindingSecret('/tmp/<state>', { coreOnly: true });
// throws UnsafeHostAuthorityFileError: 宿主凭证目录可被组内或其它用户写入

Observed matrix: direct child of /tmp → throws; nested (/tmp/<root>/data, the shape the new migration test uses with a 0700 root) → works; default HOME shape → works.

Suggested direction (validated locally with a temporary patch; existing 7 binding tests stay green): gate the legacy read on leaf existence first —

let legacyPresent = false;
try { lstatSync(legacyPath); legacyPresent = true; }
catch (err) { if ((err as NodeJS.ErrnoException).code !== 'ENOENT') throw err; }
const legacy = legacyPresent ? readSecureHostFileSync(legacyPath, 256)?.trim() : null;

A present leaf still goes through the strict read and fails closed (0644/symlink throw as before); a genuinely absent leaf under an untrusted shared parent simply proceeds to mint the new key inside the state dir. A direct regression test using a state directory whose parent is a 1777 sticky dir would lock this in.

P2 — the host-facts handler has no behavioral test. It's an HMAC-free route in core-only, but removing the larkAppId ownership check, the apiOnly check, or the isHttpVirtualSession(session.chatId) check (or breaking the probe-throw → unknown fallback) wouldn't turn any test red — only the allowlist regex is source-locked. A source-lock over the handler's guard chain (or a sliced handler test covering non-apiOnly / non-HTTP-virtual / foreign-app → 404) would close that.

P3 — the projector unit tests survive straightforward mutations. There's no present: true, ready: false → unknown case (that's the readiness contract in #1760), and protocolVersion, sessionStatus, cli, backend, worker.present/generation are never asserted (7/7 such mutations survive locally). One mixed-ready case plus a full-shape assertion would fix it.

Minor nits: workerPresent adds exitCode === null on top of the codebase's usual !!worker && !worker.killed predicate — stricter and fail-safe here, fine; the inline import('...').SessionProbe in the handler could be a top-level import.

Heads-up on the rebase conflict: the last test commit collides with #1759 (merged to master 43 minutes after that commit) in test/find-launched-cli-pid.test.ts — both independently removed sandboxRequested: true from the ineligible-wrapper table and added a sandbox case. When rebasing, the merged semantics should keep #1759's harness injection and the "bridge resolver stays off (bridgeCliPid undefined)" assertion while also retaining this PR's assertions that findLaunchedCliPid(100, 'codex') runs and the attestation is published for the resolved leaf (both hold against current worker.ts).

Happy to help with any of the above; please take this as preliminary input — the maintainer's review is authoritative.

@deepcoldy

Copy link
Copy Markdown
Owner

Thanks — this is an automated second-pass (independent verification) review; the maintainer's call remains authoritative. I independently reproduced the first-pass findings on a fresh rebase over current master and confirmed all three. Details and one extra nuance below.

P1 — reproduced end-to-end, not just at the unit level. Booting the real entrypoint with a state dir that is a direct child of the sticky /tmp (the mktemp -d shape from the issue) aborts before the port binds:

$ ST=$(mktemp -d /tmp/cohf-XXXX)
$ BOTMUX_CORE_ONLY=1 BOTMUX_API_PORT=8931 BOTMUX_CORE_STATE_DIR="$ST" bun src/index-core-only.ts
...
[core-only] fatal: 宿主凭证目录可被组内或其它用户写入

ls -la "$ST" confirms the store/pid/wrapper files were created and only the secret step threw. Nested /tmp/<root>/data with a 0700 root works; the default HOME shape works.

Two nuances worth folding into the fix:

  1. This shape is already broken on current master (8f4d0b1) — the old path is dirname(dataDir), i.e. /tmp/.dashboard-secret.report-binding, and the eager loadOrCreateDashboardSecret before bind hits the same 1777-parent check. So the PR does not regress it, but it also does not deliver the "explicit /tmp state-dir smoke: health check succeeds" item listed in the issue. The move of the new key into the state dir is necessary but not sufficient while the absent-leaf legacy probe still validates the parent first.
  2. The suggested lstat-gate fixes the fresh case but not a present legacy leaf under an untrusted parent: if /tmp/.dashboard-secret.report-binding exists from a prior run, the strict read still throws on the 1777 parent and boot still hard-fails. That may be the right fail-closed answer, but if the goal is "explicit /tmp state dir always boots", the author should decide whether a present-but-untrusted legacy leaf should be treated as "cannot migrate → mint in-state" rather than fatal. Worth a test either way.

I validated the lstat-gate direction locally (temporary patch, reverted): fresh /tmp/<state> boots to [core-only] listening on ... with curl /healthz → {"ok":true} and the 0600 key written inside the state dir; secure 0700 legacy leaf still migrates byte-identically; 0644 legacy leaf and symlinked legacy leaf still fail closed with no in-state key created.

P2 — confirmed. Deleting all three guards (larkAppId ownership, apiOnly, isHttpVirtualSession) leaves the unit + wiring suites green (58 passed). Only the allowlist regex in routeIsCoreOnlyPublic is source-locked. Since this is the new HMAC-free core-only route, a source-lock over the guard chain or a sliced handler test (foreign app / non-apiOnly / non-HTTP-virtual → 404) would close it.

P3 — confirmed. 7/7 straightforward projector mutations survive (present&&ready → present; hardcode sessionStatus; null out cli / backend / worker.generation; force worker.ready=true; bump protocolVersion). No present:true, ready:false → unknown case, and most fields are never asserted. A full-shape assertion plus that readiness case would fix it.

One extra observation (minor). The handler calls probePersistentBackendTarget → TmuxBackend.probeSession → execFileSync('tmux', …, { timeout: 3000 }) synchronously on the event loop, from an unauthenticated core-only route. A same-host co-resident process can therefore stall the daemon event loop for up to ~3s per request when the tmux server is wedged. Under single-tenant loopback this is low risk, but it is a new no-HMAC surface, so it is worth at least a comment; caching or an async probe would remove it.

Rebase note. gh still reports the PR CONFLICTING against current master; the only textual conflict is the test/find-launched-cli-pid.test.ts collision described in the first pass (both sides removed sandboxRequested: true and added a sandbox case). Merge should keep #1759's injection + "bridge resolver stays off" assertion and this PR's findLaunchedCliPid(100,'codex') / attestation assertions.

@deepcoldy

Copy link
Copy Markdown
Owner

Quick consolidation now that the independent second pass has landed above — both reviews converge, so here is the single must-address list to make re-review fast:

  1. P1 — explicit /tmp state-dir must boot, in both legacy shapes. Confirmed end-to-end that a direct-child-of-/tmp state dir aborts before the port binds (pre-existing on master, but the issue's smoke item requires this PR to fix it). The fix needs an explicit policy + tests for:
    • legacy leaf absent under a sticky/1777 parent → boot, mint the new key inside the state dir (the lstat-before-strict-read direction; verified);
    • legacy leaf present but unreadable under the strict parent gate (including a 0600 leaf directly under a 1777 dir — I reproduced this still throwing) → decide deliberately: fail-closed (fatal, documented) or "cannot migrate → mint fresh in-state and ignore the legacy". Either is defensible; please pick one and lock it with a test (a temp 1777 dir simulates /tmp without touching the real one).
  2. P2 — guard coverage for the new HMAC-free route: source-lock or sliced tests that foreign-app / non-apiOnly / non-HTTP-virtual sessions all answer 404.
  3. P3 — projector tests: add the present: true, ready: false → unknown readiness case and a full-shape assertion (the current tests survive 7/7 straightforward mutations).

Nit (non-blocking): host-facts runs tmux has-session synchronously on the event loop with a 3s timeout from an unauthenticated route — a comment, short cache, or async probe would be enough.

Plus the rebase note: when rebasing onto current master, resolve the test/find-launched-cli-pid.test.ts collision by keeping both sides' assertions (#1759's harness injection + "bridge resolver stays off", and this PR's findLaunchedCliPid(100, 'codex') + attestation assertions) — all four hold against today's worker.ts.

Thanks again — automated preliminary input; maintainer review remains authoritative.

@Oliver-Ysq
Oliver-Ysq force-pushed the codex/core-only-host-facts branch from 28bfbef to 5aa3b74 Compare October 10, 2026 06:10
@deepcoldy

Copy link
Copy Markdown
Owner

Automated re-review after the new push (e5c64e7), rebased locally onto current master (2bec52ed5; the author's own rebase applies with zero textual conflicts and GitHub now reports MERGEABLE). Preliminary input — the maintainer's review remains authoritative.

All three blocking/gap findings from the prior rounds are resolved:

  • P1 (direct-child-of-/tmp state dir) — fixed and policy made explicit. The legacy leaf is now probed for existence with lstatSync before the strict reader: an absent leaf below a sticky/1777 parent boots and mints the key inside the state dir; a present legacy leaf still goes through strict validation and fails closed (the deliberate decision requested last round). I verified end-to-end with the real entrypoint: BOTMUX_CORE_STATE_DIR=$(mktemp -d /tmp/cohf-XXXX) bun src/index-core-only.ts now reaches listening on 127.0.0.1:…, /healthz → {"ok":true}, the 0600 secret lives inside the state dir, and nothing is written at the shared parent. Two new tests pin both sticky-parent shapes (absent → create; present 0600 under 1777 → throw, no in-state key).
  • P2 (unguarded HMAC-free route) — fixed. The handler guard chain (app ownership / apiOnly / HTTP-virtual / probe-throw→unknown) is now source-locked. Deleting the HTTP-virtual guard turns the new test red.
  • P3 (projector test teeth) — fixed. Added the present: true, ready: false → unknown readiness case and a full-shape assertion; dropping the ready gate now fails the suite.

One incidental commit worth calling out positively: test(send): align ordinary chat reply routing is outside this PR's topic but fixes a genuinely red test on current master — I confirmed test/cli-send-doc-comment.test.ts fails 1/33 on pristine 2bec52ed5 (the CLI routes an ordinary chat turn to POST /messages/:id/reply, while the stale assertion expected POST /messages with receive_id) and passes 33/33 here. The new assertion matches production routing exactly, and the capture fixture simply accepts the real endpoint.

Verification on the rebased tree: tsc --noEmit clean, bun run build green; the PR's named files plus seam/neighbor suites green (353 in the core-only/dispatch/worker set; api-only + secure-host-file + the 47s real-CLI send suite all pass); CI is fully green on the new head. Mutation checks: removing the lstat gate, swallowing an unsafe present legacy, and dropping the HTTP-virtual guard are all killed.

Remaining (non-blocking, can be follow-up):

  1. Two projector field mutations still survive: hardcoding cli to 'codex' or backend to 'tmux' — the full-shape test only feeds one shape, so those two passes-of-values aren't pinned. A second non-default shape (e.g. cli: null, backend: null) would close it.
  2. The earlier nit stands: host-facts still runs tmux has-session synchronously on the event loop (3s timeout) from an unauthenticated route — a short cache, async probe, or a comment documenting the accepted risk would do.
  3. Still owed (not testable here): real-machine smoke of --backend pty core-only (the author reports a TraeX pass) and a tmux-default core-only regression.

From this review's standpoint this is mergeable once the maintainer is happy with those non-blocking items (or defers them to follow-up). Thanks for the thorough revision.

@deepcoldy

Copy link
Copy Markdown
Owner

Independent cross-check complete (automated second reviewer; maintainer's call remains authoritative). Rebased the new head onto current master (2bec52e) — zero conflicts, zero markers. I reproduced every headline claim and found one detail worth correcting in the non-blocking list.

✅ /tmp-direct real-machine boot (P1 fix works end-to-end). Real entrypoint, not a unit test:

$ ST=$(mktemp -d /tmp/cohf-e2e-XXXX)
$ BOTMUX_CORE_ONLY=1 BOTMUX_API_PORT=8944 BOTMUX_CORE_STATE_DIR="$ST" bun src/index-core-only.ts
[core-only] listening on 127.0.0.1:8944 (bot local_riff, cli codex-app)
$ curl -s http://127.0.0.1:8944/healthz
{"ok":true}
$ ls -la "$ST"/.dashboard-secret.report-binding   # -rw------- 43 bytes, inside state-dir
$ ls -la /tmp/.dashboard-secret.report-binding    # No such file or directory

Both directions of the author's chosen policy hold: absent legacy leaf under a 1777 parent → new in-state key; present 0600 legacy leaf under a 1777 parent → hard fail with no in-state key (verified via a temp 1777 fixture). --backend pty also boots to listening + /healthz 200, and an invalid --backend zellij fails closed at the entrypoint.

✅ The master send test really is red, and the PR commit is test-only. On pristine origin/master (991e1c7): cli-send-doc-comment.test.ts → 1 failed / 32 passed at :280 (expects POST /open-apis/im/v1/messages, prod sends POST /open-apis/im/v1/messages/om_chat_turn/reply). That commit has zero src/ changes, and neither the test file, fixture, send-dispatch.ts, nor client.ts differ between the PR base and master HEAD — so the red predates the PR and the PR just aligns the assertion with current production. On the PR head: 33/33 green.

✅ Mutations killed. Deleted the lstat gate → red; swallowed an unsafe legacy leaf (catch → mint new) → red; removed the !isHttpVirtualSession guard → red; broke present && ready → exists (dropped the ready term) → red. Handler guard chain and ready gate are now genuinely pinned.

⚠️ Correction to non-blocking ① — the surviving mutation is narrower than "cli/backend passthrough". cli: input.cli → null and backend: input.backend → null are both killed (test#1's full-shape toEqual catches them). What actually survives is hardcoding each passthrough to a constant equal to test#1's fixture value: cli → 'codex' and backend → 'tmux' stay green, because no test asserts a second distinct value for those fields (test#3 uses null/null, which the literal-null mutation already fails on). So the precise remaining gap is "no second non-null shape", not "untested fields" — still non-blocking, but the fix is one extra case with different non-null values, not a full-shape rewrite.

✅ Local checks. tsc --noEmit clean; named + seam files 292 passed; GitHub head CI 10/10 SUCCESS; mergeable: MERGEABLE.

Non-blocking nit still open. host-facts → probePersistentBackendTarget → TmuxBackend.probeSession → execFileSync('tmux', ['has-session', …], { timeout: 3000 }) (tmux-backend.ts:258) runs synchronously on the event loop from the HMAC-free route; a same-host process can stall the daemon up to ~3s. Async/cache or an explicit comment.

From the review side this is 0-blocker and mergeable pending your sign-off; the only human-acceptance debt left is real-machine core-only PTY and default tmux session bring-up (I verified boot/bind, not a driven turn). I'll re-verify the head once more if it changes before you give the go-ahead.

# Conflicts:
#	test/cli-send-doc-comment.test.ts
@deepcoldy

Copy link
Copy Markdown
Owner

Pre-merge re-verification of head a68c43de4 done (automated second reviewer; your "merge" call is authoritative). All green; one forward-looking note since master moved again.

✅ Integration commit is structurally sound. a68c43de4's parents are e5c64e7b0 (author's last pre-merge commit, confirmed ancestor) + 46cb3686b (master tip at merge time). Author's 7 commits are all retained. No conflict markers anywhere in src/test. The author's reply-endpoint fix is subsumed by master's it.each([false, true]) parameterized version at :276, which also asserts reply_in_thread; fixture is byte-identical between the PR-side commit and master.

✅ Resolution verified against both bases. test/cli-send-doc-comment.test.ts in a68c43de4 is byte-identical to the master version (empty diff vs both 46cb3686b and current origin/master). On the integration tree: tsc 0, send suite 34/34 (the extra case is exactly #1790's parameterized form), named+seam files 292/292, and real-machine mktemp -d /tmp state-dir boots to listening + /healthz 200 with the 0600 key inside the state dir and nothing under /tmp. P1 lstat gate and the handler guards are still present in the head.

ℹ️ Forward-looking: master advanced once more, to 6fa2bb831 (#1795). It touches none of the resolved files, and I re-ran a fresh three-way merge (rerere disabled) of origin/master + a68c43de4 → clean, zero markers, and the merged tree keeps master's version of the conflict file. So the squash-merge against current master will not re-conflict on that file. If anything else lands on master before you merge, the only file at risk is still test/cli-send-doc-comment.test.ts, and the resolution is unconditionally "take master's parameterized version" (what the head already has). PR shows MERGEABLE; CI 10 for the new oid is still finishing (3 long test legs + bun-test IN_PROGRESS).

From my side: 0 blocker, resolution is the correct one, ready to merge as soon as the new CI run is fully green.

@deepcoldy
deepcoldy merged commit cfde85a into deepcoldy:master Oct 10, 2026
10 checks passed
@deepcoldy

Copy link
Copy Markdown
Owner

Independent cross-check of the latest synthetic tree done (automated second reviewer; your execution remains authoritative). I did a real merge (not just merge-tree), rerere disabled, of current origin/master (dbc3c7b67) onto a68c43de4:

✅ Zero conflicts, zero markers. Real git merge --no-ff auto-merged src/cli.ts, src/core/dashboard-ipc-server.ts, src/daemon.ts, etc. cleanly; no unmerged paths, no <<<<<<</>>>>>>> anywhere in src/test.

✅ Content is what you describe. test/cli-send-doc-comment.test.ts is master's it.each([false, true]) parameterized form (:276); #1372's workspace-recycle* files are present; the host-facts route, the P1 legacyPresent lstat gate, and the !apiOnly || !isHttpVirtualSession(session.chatId) guard are all intact.

✅ Checks on the merged tree: tsc --noEmit 0; the host-facts/seam ∪ send ∪ workspace-recycle files → 354 passed (9 files), no failures; real-machine mktemp -d /tmp state-dir boots to listening + /healthz 200 with the 0600 key inside the state-dir.

CI is now fully green for a68c43de4: all 10 checks SUCCESS (build, network-boundary, test 1–3/3, bun-test, bun-binary + musl + darwin, test). GitHub is momentarily showing mergeable: UNKNOWN while it recomputes, but the head is unchanged and the fresh merge is clean.

From my side: clear to squash-merge. I'll stand down unless the head oid changes.

@github-actions

Copy link
Copy Markdown

🚀 Released in v3.41.0

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

core-only: expose safe session host facts and PTY backend selection

2 participants