Repository navigation
fix: verified findings from the analytics-plan research pass (consent boundary, soul fingerprint, dispatch exit status, doc counts) - #365
Merged
Conversation
consent state is stored per-machine, not per-tenant: src/consent/store.ts
resolves ~/.lisa/consent.json with its own lisaHome() instead of going
through the per-user home scope in src/paths.ts. the cloud deny-list in
src/web/capabilities.ts covered every other host-control route but missed
/api/consent, so in the hosted edition any signed-in tenant could:
- GET /api/consent read every tenant's grant list + timestamps
- POST /api/consent/grant turn on a signal (incl. "mail") for everyone
- POST /api/consent/revoke-all switch consent off for the whole deployment,
which stops the mail digest sweep for all
tenants (server.ts gates it on isGranted)
add the prefix to CLOUD_DENIED_ROUTE_PREFIXES. the existing matcher already
handles both the bare root and sub-paths, and does not over-match sibling
routes like /api/plans-public. local edition is unaffected — the deny gate
only runs when edition is cloud.
does not fix the underlying scoping bug in src/consent/store.ts; that needs
a decision about per-tenant consent and is filed separately.
BEHAVIOR CHANGE — see CONTRIBUTING: touching src/soul/* wants a discussion issue first. this needs a maintainer decision before merge. seed.bornOn is sha256(hostname + username): unsalted, over two low-entropy and often-guessable strings, and stable across rebirths on the same machine. that makes it a device fingerprint an adversary can confirm offline from a candidate (hostname, username) pair. dreamSoul() stringified the WHOLE seed into the birth prompt, so the fingerprint was sent to whichever model provider is configured (Anthropic / OpenAI / Gemini / a local endpoint) on every birth. it contributes nothing to the dream — the personality comes from randomness + bigFive — so this was pure unnecessary egress. add seedForPrompt(), which drops bornOn and nothing else, and use it in the one place the seed is serialized for the provider. writeSeed() still records the full seed on disk, so nothing about the stored soul changes. the behavior change: the birth prompt text is now shorter by one field, so a NEW birth will dream a slightly different soul than it would have before. already-born souls are untouched (birth refuses to re-run). what this does NOT fix: GET /api/soul still serves the full seed, bornOn included, to authenticated clients (the iOS companion reads it). narrowing or removing that is a separate decision — noted in the type comment. document the invariant on SoulSeed.bornOn so the next person does not re-add it, and pin the redaction with a regression test.
two bugs in the dispatch ledger. they share DispatchEntry and the same two
files, so they land together.
1. no exit code was ever captured. launchAgent() spawned detached and only
ever attached an "error" listener for the first 150ms, which catches
ENOENT-class launch failures and nothing about the run. dispatch_status
then rendered any dead pid as "✓ finished", so an agent that exited 1,
died on a missing API key, or was OOM-killed looked exactly like a clean
run.
attach a "close" listener and record (exitCode, exitSignal, exitedAt).
the listener has to go on BEFORE the 150ms launch race — an agent that
exits inside that window emits "close" first, and a listener attached
afterwards never fires — so the result is stashed and written once the
ledger row exists. both orderings are covered by tests.
BEHAVIOR CHANGE in dispatch_status output. the label is now derived from
what was actually observed:
▶ running / ✓ exit 0 / ✗ exit N / ✗ killed by SIG… /
• exited (status not captured)
the last one is the honest case for a dispatch that outlived LISA's own
process: because the child is detached + unref'd, the listener only fires
while LISA is alive (true for `lisa serve` and a live REPL, false for a
one-shot CLI invocation). we say so instead of showing a checkmark.
"✓ finished" is gone.
2. pid reuse could make signal_agent kill an unrelated process group.
entries are retained 24h and matched by bare pid; isAlive() was just
kill(pid, 0), so a recycled pid answers "alive". signal_agent then sends
SIGTERM/SIGKILL to -pid — a whole process group the user owns. that
directly contradicts the invariant in its own header ("LISA cannot kill
an arbitrary process").
record a start-time fingerprint at dispatch (/proc/<pid>/stat field 22 on
linux, `ps -o lstart=` elsewhere) and require it to match before
reporting alive or delivering a signal. pid + start time cannot be
reused, since a recycled pid necessarily started later. a null probe
means "cannot tell" and is treated as a match, and entries without a
token (older ledger files) keep the old pid-only behavior, so nothing
silently disappears.
adds entryIsAlive(e) and moves every ledger call site onto it, including
the two in web/server.ts, so the HTTP view is guarded too. DispatchView is
deliberately unchanged — exposing exit status over /api/dispatch would
change the generated API contract and is a separate call.
…emits
`repeated_failure` was declared in SuggestionCategory but no detector ever
produced it: the five literals in detectors.ts are stuck, conflict,
cost_spike, ready, idle. its supporting state was equally inert —
AdvisorState.errorCommandCounts was written once as {} in
emptyAdvisorState() and never read or incremented, under a comment
promising a "rolling memory of (command → error count)" that never rolled.
docs/PRODUCT_REVIEW_v0.9.md already called both out as dead code.
delete both. this matters beyond tidiness: it is what makes the README fix
in the next commit correct. a maintainer told "the README lists 4 categories
and the type has 6" would naturally document all six and thereby promise a
card the product cannot produce. the honest number is five.
to stop it drifting back, declare the categories as a runtime tuple and
derive the type from it, then assert in advisor.test.ts that the declared
set and the set the detectors actually emit are equal — in both directions.
the reverse direction is the one that catches this bug class.
this had to be a runtime check: tsconfig.json excludes src/**/*.test.ts and
tsx strips types without checking them, so a type-level pin inside a test
file is never evaluated by `npm run typecheck` or `npm test`. verified by
re-adding "repeated_failure" and watching the test fail.
removing errorCommandCounts is safe for existing ~/.lisa/advisor-state.json
files: loadAdvisorState() spreads parsed JSON over emptyAdvisorState(), so a
leftover key is simply ignored.
…gories two counts in both READMEs did not match the code. observers. the prose said "all five observers (Claude Code, Codex, OpenCode, Aider, GitHub PRs)" but ten ship: registry.ts imports ten observer modules, hub.ts configures ten keys, and src/integrations/ has ten adapter dirs. the README also contradicted itself — the directory tree 490 lines below already listed seven of them plus an ellipsis, so this was a copy-editing miss, not staleness. say ten, split honestly into the five coding-agent adapters and the five others (git, shell, takoapi, managed, pty), and add the fact the old sentence was probably reaching for: only three are on by default (claude-code, managed, pty) — the other seven are opt-in per integration. the tree line now lists all ten and drops the ellipsis, so the two places agree. advisor categories. the tree said "(stuck / conflict / ready / idle)" — 4 of the 5 the code emits. add cost_spike, which detectors.ts has emitted since COST_SPIKE_TOKENS landed. repeated_failure is deliberately NOT listed: it was dead and is deleted in the previous commit, and documenting a card the product never produces would make the docs more wrong, not less. prose can't be unit-tested, but the facts behind it can: hub.test.ts now pins the roster to exactly those ten names, checks that registerBuiltinIntegrations() really registers one observer per configured key, and pins the three-enabled-by-default split.
the checklist told contributors to add regression tests "under `test/`".
no such directory exists, and package.json's test script is
`node --import tsx --test "src/**/*.test.ts"` — so a test written where the
template said would never be collected, and CI's `npm test` step would go
green having never run it. the worst kind of stale doc: it silently produces
a test that does not test.
CONTRIBUTING.md already says it correctly ("co-located with the source
(`src/**/*.test.ts`, run by `npm test`)"). match it, and name the failure
mode so the next person does not re-introduce it.
the promise is real and currently true — there is no analytics SDK, no tracking pixel, no third-party script and no phone-home anywhere in the tree. nothing enforced it. one `npm install` of a convenience wrapper, or one snippet pasted into a layout, would turn a published privacy claim into a false statement with nobody noticing. add a deny-list check over src/, website/src/ and the iOS companion sources, plus package.json's dependency names. it lives in the normal suite, so it gates every PR through ci.yml and blocks prepublishOnly too — no CI edit needed, and it runs locally with `npm test`. two deliberate choices: - the promise pages are listed BY PATH, not found by grepping for English phrases. the Chinese pages say "无云同步、无遥测、无任何账号" and "没有分析 SDK", which no English keyword search would ever match, so a keyword-driven guard would have silently covered only half the site. listing paths also means renaming a promise page fails loudly instead of shrinking coverage. - the tokens are SDK-shaped, not bare words. "segment", "heap" and "plausible" occur in ordinary prose and identifiers here (13, 35 and 2 files), and "amplitude" is an audio term — a guard that cries wolf gets deleted. two tests pin this from both sides: one asserts the list really matches posthog-js / googletagmanager / @sentry, so it can never go vacuous, and one asserts it stays quiet on the English words. verified by injecting a tracker script into the Chinese homepage and confirming three of these tests fail, then reverting.
the design doc this batch of fixes came out of. it is the plan for a local-first instrumentation system: a bounded, user-readable JSONL ledger on the user's own disk as the only sink by default, with network upload as a second sink that is off by default, gated on the existing consent framework, and restricted to enums and buckets. it is a plan, not a description of current state — none of the src/telemetry/ files it describes exist yet. no telemetry is added by this PR. it is included here because it is the source of the verification pass that produced the other commits: §0.1 is what established the observer count is ten, §8.5 is what the no-telemetry CI guard implements a first slice of, and §5.1 is what flagged bornOn as unusable as an anonymous identifier.
the plan doc was written against the tree before these fixes, and three of its stated facts are no longer (or never were) true. left alone, the PR would ship a design doc contradicting the code in the same PR. - §0.1 said bornOn is "私有、从不外发" (private, never leaves the machine). it did leave: the whole seed was serialized into the birth prompt. now corrected to record what was actually true, what this PR fixed, and what is still open (GET /api/soul still serves the full seed). this makes §5.1's "never use bornOn as an identifier" red line better founded, not weaker. - §0.1 and §3.4-Q4 said advisor has 6 categories. it has 5 — repeated_failure was dead and is deleted here. - Phase 0's consent item now notes that this PR added only the route-level mitigation, so nobody reads the checkbox as already done: the underlying cross-tenant consent.json is untouched and is still a hard prerequisite for hanging telemetry consent off it.
the guard added in 7df0048 scanned src/, website/src/ and the iOS companion Sources only. website/public/ was not among them, and that is the one gap that matters most: astro copies public/ to the site root verbatim, so a tracker dropped there ships to the very site carrying the "no telemetry" promise without ever passing through a .astro file. verified before this commit — a googletagmanager snippet written to website/public/analytics.js left all 7 tests green. scan packaging/ whole rather than just ios-companion/Sources (the mac client, launcher and gcp-relay ship too), and add scripts/, deploy/, contracts/ and completions/ so the guard covers the same surface the promise was originally verified against. all six added roots are currently clean, so this is coverage, not a fix — 111 extra files, no measurable runtime cost. also skip build/, .build/, .swiftpm/ and DerivedData/, which appear under packaging/ once anyone builds the native clients locally, and pin the reach of the guard with a test asserting every SCAN_ROOTS entry still exists — a renamed directory now fails loudly instead of silently shrinking coverage. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
the doc's header cited a source document by absolute path (/Users/.../analytics-blueprint.md) and described its own path convention in terms of an absolute checkout location. this repo is public, so both are local-machine detail that means nothing to a reader and needlessly names a directory layout. the PR body flagged the first one as a follow-up; the second was missed. neither reference is load-bearing: the source is not published with this repo, and the path convention is simply "relative to the repo root". Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Same defect class as the /api/consent entry directly above it, found by sweeping the rest of the route table for the same shape. src/web/push.ts resolved ~/.lisa with its own private lisaHome(), outside the per-uid homeScope in src/paths.ts, and a PushSubscription carries no owner field. The hosted edition runs one container with one LISA_HOME (deploy/Dockerfile) and isolates tenants purely by entering homeScope.enterWith(homeForUid(uid)) per request — so push.json was one shared file, and none of the five /api/push routes sat behind denyRemote, a loopback check, or an owner check. Any signed-in tenant could GET /api/push/list and read every other tenant's ntfy topic (which IS the send/read secret for that channel) and APNs device token, unregister another tenant's device, or rewrite their prefs. Verified by running the real modules under two uid scopes: tenant B saw tenant A's subscription and could unregister it. The store stays machine-wide on purpose — every producer wired to PushBridge (agent activity, idle notes, mail digest, KB brief, billing anomalies) is a host-level concern, not a per-tenant one, so this is an operator channel and there is no per-tenant push to preserve. What was wrong is that tenants could reach it. So: deny the routes in cloud, and say lisaGlobalHome() out loud in push.ts instead of re-deriving ~/.lisa, which made a deliberate choice look like an accidental scope bypass. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
isCloudDeniedRoute matches the NORMALIZED pathname, but every route in
server.ts matches the RAW req.url with startsWith/===. A dot-segment path
exploits that disagreement in one request:
GET /api/agents/recap/%2e%2e/%2e%2e/%2e%2e?sinceMinutes=1440
normalizes to "/" — so the deny gate answers "not denied" — while still
satisfying url.startsWith("/api/agents/recap"), so the handler runs. The
same shape reaches /api/agents/steps, /api/agents/transcript,
/api/agents/pty/*/output, /api/dispatch/status (which returns raw captured
agent stdout) and /api/mail/accounts, and "//api/consent/grant" reparses
its authority so even the entry added earlier in this batch is bypassable.
%2e survives upstream proxies, so Cloud Run's frontend does not neutralize
it. Verified against the real exported function.
Teaching ~80 route checks to normalize would leave the next route to
remember, so reject non-canonical paths outright, before anything routes.
That fails closed for routes added later and costs legitimate callers
nothing: clients percent-encode, and an encoded separator that survives
normalization leaves the pathname — and therefore the deny-list decision —
unchanged, so it is left alone and the prefix still matches.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This batch taught the ledger to record exitCode/exitSignal/exitedAt, then never consulted them in the one predicate that decides the headline label. entryIsAlive still asked the OS about a pid we had already watched die. Reproduced against the real module: after recordDispatch() then recordExit(id, 3, null), entryIsAlive() is true, statusLabel() returns "▶ running", listLiveDispatches() still contains the entry and findDispatch() still returns it — while the row on disk carries exitCode: 3. GET /api/dispatch/list reported alive: true for the same row. Second consequence, and the reason this is more than cosmetic: an entry with no startToken falls back to bare pid identity, and that is not a rare case — recordDispatch runs after launchAgent's 150 ms launch race, so a fast-crashing agent is already reaped when processStartToken shells out and the token is dropped. Those entries are retained 24 h, during which the freed pid is available for reuse; signal_agent cancel would then deliver SIGTERM and SIGKILL to whatever process group now owns it. The recorded exit already proved the entry was dead. Gate on exitedAt rather than exitCode — a signal death legitimately stores exitCode: null. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The three privacy tests added in this batch all assert on seedForPrompt's own output — one of them re-derives the payload with JSON.stringify(seedForPrompt(seed), null, 2) under a comment calling it "the actual wire shape". Nothing enforced that coupling: reverting birth.ts back to JSON.stringify(seed, null, 2) left the suite at 9 pass / 0 fail. The headline privacy fix of this PR could be silently undone. dreamSoul is where the prompt is actually assembled and it already takes its provider as a parameter, so export it and hand it a fake that captures what gets sent, then assert the fingerprint is absent and that the fields the dream does need still travel. Reverting the fix now fails this test. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Owner
Author
Review pass — four fixes pushedA high-effort review of this branch (10 finder angles, adversarial verification, gap sweep) surfaced 15 findings. Four are addressed in the four commits just pushed; the rest are listed below as follow-ups. Every fix here carries a test that was checked by reverting the fix and confirming the suite goes red — the lesson from finding #4 below. Fixed here1. 2. The deny-list is bypassable by path. 3. 4. The soul privacy fix had no test that could fail. All three new tests assert on Follow-ups (not addressed here)
Still needs your callThe three items in the PR description remain open: the 🤖 Generated with Claude Code |
Four follow-ups from this PR's own review comment.
1. `ps -o lstart=` renders in the CALLER's locale and timezone, so the
token was never a process identity. Measured on macOS for one unchanged
pid: TZ alone moved the hour (13:23:46 UTC / 22:23:46 Asia/Tokyo /
09:23:46 America/New_York) and LC_ALL reshaped the whole string
("Mo. 7 Sep." de_DE, "一 9月/ 7" zh_CN). A `lisa serve` under launchd
and a `lisa` CLI from a configured login shell therefore disagreed about
every token, so isAlive() called every running dispatch dead — and
signal_agent, seeing "already exited", deleted the ledger row instead of
signalling, leaving a runaway agent permanently uncancellable. The probe
env is now pinned to LC_ALL=C / TZ=UTC.
Tokens are versioned (ps1:/lt1:) and compared only within one scheme, so
entries written by the old locale-dependent scheme read as "cannot tell"
and fall open, rather than reporting every pre-upgrade dispatch dead.
2. The Linux token was ticks-since-boot and not boot-scoped, so it could
collide across a reboot inside the 24h retention window. Scoped to btime
from /proc/stat.
3. toDispatchView carried no exit fields, so the exit status this PR
teaches the ledger to record reached 1 of 3 surfaces — the web dashboard
rendered `exit 127` as a green "Done" and the iOS ledger as "Alive: no".
DispatchView now carries a derived `status` (running | ok | failed |
unknown) plus exitCode/exitSignal/exitedAt, and the OpenAPI contract
declares them. `unknown` is deliberately distinct from `ok`: the child is
detached, so a crash, an OOM kill and a clean finish are identical from
the pid alone.
4. saveLedger was a bare writeFileSync, which truncates before it writes —
a concurrent reader hit loadLedger()'s catch and silently got [], losing
every live dispatch. recordExit() widened that window by adding a second
read-modify-write from an async listener. Now tmp + rename, the sync twin
of atomicWrite() in fs-utils.
Also: listLiveDispatches probed each entry twice (entryIsAlive can shell out
to `ps` — 1.79 ms vs 0.001 ms for kill(pid,0)) while running inside the
/api/dispatch/list request handler over an unbounded ledger. One probe per
entry now, cheap test first.
Every fix carries a test checked by reverting it and confirming the suite
goes red. The two existing tests that hardcoded a "ps:" prefix now derive
the scheme from a live probe — hardcoding it was also wrong on Linux, where
the token has always been "lt:".
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
processStartToken() ran after the 150 ms launch race, but the token is only obtainable while the process is alive. An agent that dies inside that window is already reaped by the time we ask, so the probe returns null and the entry falls back to bare-pid identity for the full 24h retention window. That is exactly the population most at risk: a process that exits in 150 ms frees its pid immediately, so `signal_agent cancel` on the recycled pid would deliver SIGTERM/SIGKILL to whatever unrelated process group inherited it — the precise failure the start token exists to prevent. child.pid is known the moment spawn() returns, so probe there and pass the token into recordDispatch(). Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Two more follow-ups from the review comment. _resetIntegrationsForTest() was a one-way door. The built-ins register as a side effect of importing their observer modules, and ESM caches modules — so the second registerBuiltinIntegrations() imported ten already-evaluated modules and registered nothing. Any test running after one that called the reset got `unknown integration "claude-code"`; hub.test.ts passed only because its reset happened to be the last thing in the file, and this PR appends a suite after it. The built-ins are now snapshotted on first registration and restored on a later call. loadOrchestratorConfig did `parsed.integrations ?? DEFAULT` — a whole-map replace. The READMEs point you at ~/.lisa/agents.json to switch on one of the seven opt-in observers, and the obvious hand-edit is to write just that one, which silently dropped the three defaults: enabling codex also turned Claude Code, managed and pty observation OFF, with no error and nothing in the UI to say so. This PR adds the README's only pointer to that file, so it now walks users into it. Merged per key; an explicit `enabled: false` still wins over the default. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Widening "all five observers" to "all ten" dragged the parenthetical "every adapter has a privacy test asserting prompts/replies/file-contents never leak" over five adapters it was never written for. Checked each: claude-code, codex, opencode, aider, git and shell carry privacy assertions in a test file. github-pr and takoapi have observer tests but no privacy assertion; managed and pty have no test file at all. Six of ten, so the READMEs now say six of ten and name the four that are missing. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Owner
Author
第二轮评审 — 六条 follow-up 已修，带回退验证上一轮留了 10 条 follow-up。这轮实证复核了每一条，修掉其中六条（四个提交），并核实了两处此前没被质疑的新增逻辑。 已修1.
launchd 下的 顺带把令牌方案版本化（ 2. Linux 令牌未按启动纪元隔离。 3. 现在 4. 5. 6. 7. README 的隐私测试断言。 把"五个 observer"扩写成"十个"时，括号里那句"每个 adapter 都有隐私测试"被一并拖过了五个它从未覆盖的 adapter。逐个核实：claude-code、codex、opencode、aider、git、shell 六个在测试文件里确有隐私断言；github-pr 和 takoapi 有 observer 测试但无隐私断言；managed 和 pty 连测试文件都没有。README 改为据实说"十个里有六个"并点名缺失的四个。 复核过、不改的两处
仍未处理（本 PR 不做，理由如下）
验证
每条修复都带一个回退即红的测试。以 TZ 那条为例，单独撤掉环境钉死： 另外修正了两个此前写死 🤖 Generated with Claude Code |
oratis
added a commit
that referenced
this pull request
Sep 7, 2026
GUIDE.md and GUIDE.zh-CN.md were written from the pre-#365 README, so moving the content into a new file silently reverted six corrections. Git saw a pure file addition, not a conflict, so nothing flagged it. Restored in both languages: - "all five observers" → "all ten observers", naming the five coding-agent adapters plus git, shell, takoapi, managed and pty, and saying which three are enabled by default. - "every adapter has a privacy test" → six of the ten, naming the four that have none. The wider claim is false for github-pr, takoapi, managed and pty; managed and pty have no test file at all. - the `integrations/` tree line: "observers: … · managed · …" → the real ten, no ellipsis. - the `advisor/` tree line: the category list was missing cost_spike, which src/advisor/types.ts now pins at runtime via SUGGESTION_CATEGORIES. Checked the split itself while here: the union of README + GUIDE headings covers every section of the old README (42 → 54), with only renames unaccounted — "IM channels" → "Channels", "空闲模式（Idle）" → "空闲模式（梦境 Reve）" — and the content present under the new names. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
oratis
added a commit
that referenced
this pull request
Sep 7, 2026
README 767→85 行、安装命令进前 20 行，其余内容移入 docs/GUIDE.md（中英同构）；零依赖链接检查 + 中英漂移检查 + docs-check 工作流；CHANGELOG 由发布说明生成；已完工计划归档进 docs/archive/plans/。 评审补：修复 README→GUIDE 搬迁静默回滚的 #365 六处修正（observer 数量、隐私测试断言、两处目录树行，中英各三处）。 🤖 Generated with [Claude Code](https://claude.com/claude-code)
oratis
added a commit
that referenced
this pull request
Sep 7, 2026
Delivers on the iOS half of the commitment made reviewing #365: the ledger learned to record an observed exit and DispatchView now carries it (status / exitCode / exitSignal / exitedAt), but the roster still rendered "alive" vs "exited" — so a crash, an OOM kill and a clean finish were one grey dot and one word. - DispatchKind (running | ok | failed | unknown) maps the server's `status`, with an explicit fallback for a pre-status server: no `status` and `alive: false` is **unknown**, never `ok`. The agent is spawned detached, so a dead pid alone never means success. - The row spells the state out in text and gives each state its own dot colour; colour is never the only carrier. - The detail row names the exit code or the signal ("failed (exit 127)", "killed by SIGKILL") — "failed" alone does not say whether the agent errored out or was killed. - Each state gets a distinct VoiceOver label, since the dot's colour carries nothing for VoiceOver. A test pins all four as distinct and non-empty. Also: HTTP 403 said "Server returned HTTP 403." Hosted Lisa denies the machine-level routes (push, dispatch, mail, consent, devices — the deny-list in src/web/capabilities.ts, which #365 extended to /api/push), and push registration is the route an iPhone hits first, so the generic message left "registered, but no notifications ever arrive" unexplained. It now names the hosted/local split and what to do about it. Verified: `./build.sh test` — 49 tests, 0 failures (44 existing + 5 new). Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This was referenced Sep 7, 2026
oratis
added a commit
that referenced
this pull request
Sep 7, 2026
The last of the three surfaces, and the second half of the commitment made reviewing #365 (iOS landed in #371). The dashboard rendered `d.alive ? 'Running' : 'Done'`, so a crash, an OOM kill, `exit 127` and a clean finish were one green "Done" — the card actively asserted a success the server never observed. The ledger has recorded a classified exit since #365 and DispatchView carries it; nothing consumed it here. dispatchState() maps the server's `status` to a label and a colour class: running, Done, `Failed · exit 127`, `Killed · SIGKILL`, or `Exited · status unknown`. A pre-status backend degrades via `alive`, and an exited dispatch there is **unknown, never Done** — a dead pid is not evidence of success when the child is detached. An unrecognised status lands in the same place rather than falling through to green. Tested by slicing dispatchState out of the exact bytes the browser is served and running it in a vm, the same technique this PR's i18n tests use — so the test cannot pass against a version of the function the page doesn't ship. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
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
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.
What this changes
While building the telemetry/analytics plan (
docs/analytics-plan-2026-08-21.md, included here), the research pass had to establish a lot of ground truth about this repo, and it turned up a set of real defects along the way. Each one was then independently verified against the source before being fixed. This PR is that batch: one security fix at the cloud HTTP boundary, one privacy fix in the soul birth path, two correctness fixes in the dispatch ledger, one dead-code removal, three documentation corrections, and a new CI guard for the published "no telemetry" promise. Every finding has a regression test, and each fix is its own commit so anything contentious can be dropped independently.Type
Three commits change behavior or need a human decision. Everything else is inert.
1.
fix(soul): keep the device fingerprint out of the birth prompt— touchessrc/soul/*, so per CONTRIBUTING it wants a discussion issue first. I could not open one; please treat this as the proposal.seed.bornOnissha256(hostname + username)— unsalted, over two low-entropy and often-guessable strings, stable across rebirths on the same machine.dreamSoul()serialized the whole seed into the birth prompt, so that fingerprint was sent to whichever model provider is configured, on every birth. It contributes nothing to the dream (personality comes fromrandomness+bigFive), so this was pure unnecessary egress.The behavior change: the birth prompt is now one field shorter, so a new birth will dream a slightly different soul than it would have before. Already-born souls are untouched. Stored seeds still contain
bornOn; only what crosses the wire narrowed.What this does NOT fix:
GET /api/soulstill serves the full seed,bornOnincluded, to authenticated clients — the iOS companion reads that endpoint. Narrowing that is a separate decision, flagged in the type comment and in the follow-ups below.2.
fix(dispatch): capture exit status…changesdispatch_statusoutput.✓ finishedis gone. Labels are now▶ running/✓ exit 0/✗ exit N/✗ killed by SIG…/• exited (status not captured). The last one is the honest case for a dispatch that outlived LISA's own process — the child is detached andunref'd, so the exit listener only fires while LISA is alive (true forlisa serveand a live REPL, false for a one-shot CLI invocation). Anything parsing that string will need updating.3.
refactor(advisor): drop the repeated_failure categoryremoves a member from the exportedSuggestionCategoryunion and theerrorCommandCountsfield fromAdvisorState. Existingadvisor-state.jsonfiles are unaffected —loadAdvisorState()spreads parsed JSON overemptyAdvisorState(), so a leftover key is ignored.What's fixed
/api/consentroutes were missing from the cloud deny-list.src/consent/store.tsresolves~/.lisa/consent.jsonwith its ownlisaHome()instead of the per-user home scope insrc/paths.ts, so consent state is per-machine, not per-tenant. Every other host-control route was denied in the hosted edition; these were not. Any signed-in tenant could read every tenant's grant list, grantmaildeployment-wide, orPOST /api/consent/revoke-alland stop the mail digest sweep for everyone. Added the prefix; the existing matcher already covers the bare root and sub-paths without over-matching siblings like/api/plans-public.launchAgent()only attached anerrorlistener for 150 ms, which catches launch failures and says nothing about the run, so a crashed agent and a clean one both rendered as✓ finished. Now recordsexitCode/exitSignal/exitedAt. Subtlety worth noting: the listener has to be attached before the 150 ms launch race — an agent that exits inside that window emitsclosefirst, and a listener attached afterwards never fires. Both orderings are covered by tests.signal_agentkill an unrelated process group. Entries are retained 24 h and matched by bare pid;isAlive()was justkill(pid, 0), so a recycled pid answers "alive" andsignalGroup()then sends SIGTERM/SIGKILL to-pid. That contradicts the invariant insignal_agent.ts's own header ("LISA cannot kill an arbitrary process"). Now records a start-time fingerprint (/proc/<pid>/statfield 22 on Linux,ps -o lstart=elsewhere) and requires it to match. A failed probe means "cannot tell" and is treated as a match; entries without a token keep the old behavior, so nothing silently disappears.repeated_failurewas a declared advisor category no detector ever emitted, with an equally inerterrorCommandCountsunder a comment describing a rolling memory that never rolled.docs/PRODUCT_REVIEW_v0.9.mdalready flagged both. Deleted.cost_spike.repeated_failuredeliberately not documented — it is deleted in this PR, and documenting a card the product never produces would make the docs more wrong, not less.test/directory that does not exist.npm testglobssrc/**/*.test.ts, so a test written where the template said would never run and CI would go green having never executed it. CONTRIBUTING.md already had it right.The no-telemetry guard
The promise is currently true — no analytics SDK, no tracking pixel, no third-party script, no phone-home anywhere in the tree. Nothing enforced it, so one
npm installof a convenience wrapper or one snippet pasted into a layout would turn a published privacy claim into a false statement with nobody noticing.src/no-telemetry.test.tschecks a deny-list oversrc/,website/src/, the iOS companion sources, andpackage.jsondependency names. It runs in the normal suite, so it gates every PR throughci.ymland blocksprepublishOnly— no CI edit needed.Two deliberate choices:
segment,heapandplausibleappear in ordinary prose and identifiers here (13, 35 and 2 files), andamplitudeis an audio term. A guard that cries wolf gets deleted. Two tests pin this from both sides: one asserts the list really matchesposthog-js/googletagmanager/@sentry, so it can never go vacuous, and one asserts it stays quiet on the English words.Verified by injecting a tracker
<script>into the Chinese homepage and confirming three of these tests fail, then reverting.Before / after
Follow-ups for a human (not fixable in this PR)
src/consent/store.tsstill writes one shared~/.lisa/consent.jsonoutside the per-user home scope, so consent remains per-machine in the hosted edition. It is also a hard prerequisite for hanging telemetry consent off that framework (plan §9.2).GET /api/soulshould expose. It still serves the full seed includingbornOnto authenticated clients. Narrowing it is a breaking change for the iOS companion, so it needs an owner's call.bornOnshould exist at all. Its only read is a display string (seed.bornOn.slice(0, 8)).crypto.randomByteswould serve that with no linkability — but it changes stored soul data.src/soul/*change, or drop commiteb92134from this PR.lisaHome()that bypasseshomeScope(sense/log.ts,dispatch-ledger.ts,control/policy.ts,web/push.ts,mail/{store,accounts}.ts,takoapi/ledger.ts). Currently unreachable in cloud mode because both the route deny-list and the tool allow-list hold — but that is two gates deep, not a fix. Do not bulk-changeweb/{accounts,devices,otp,sessions-auth}.ts; those four are correctly process-global./Users/oratis/Documents/Claude/analytics-blueprint.md) as its methodology source. Harmless but you may want it scrubbed before this lands on a public repo.Checklist
npm run typecheckpassesnpm run buildpassessrc/**/*.test.ts) — added for all seven code findingssrc/soul/*change is NOT linked to a discussion issue — see the warning above; this is the one checklist item this PR knowingly does not satisfyanyintroducedFull gate run locally, all four CI steps:
check:api-contractclean ·typecheckclean ·npm test1674 pass / 0 fail / 1 skipped (1638 → 1675 tests) ·buildclean, 0*.test.jsleaked intodist/.Related issues
n/a — findings came from a verification pass, not a filed issue.
🤖 Generated with Claude Code