fix: verified findings from the analytics-plan research pass (consent boundary, soul fingerprint, dispatch exit status, doc counts) - #365
Open
oratis wants to merge 15 commits into
Open
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 |
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
1 participant
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