feat(activity): instrumentation-only baseline for activity-source probes (lr-58c813) - #403
Conversation
… latch (lr-58c813) Instrumentation-only baseline for the lr-5edd64 redesign, per lr-58c813 item 1/2. Records (msgType, msgLocalId, activeSessionId) accept/reject outcomes for shouldApplyActivityEdge at its three call sites in app-messages.js (status/done/auth_required), and counts the msgLocalId==null fail-open branch SEPARATELY per the task spec — a nonzero fail-open count in production means a live send site still fails to stamp localId and item (a) of lr-96e7da has a hole. Zero behavior change: recordActivityEdgeDecision is called AFTER the real shouldApplyActivityEdge result is already computed, with that same result, and never gates anything itself. Bounded by construction (4 integers, no per-event array/log).
…val (lr-58c813) Wires getActivityEdgeLedger onto window.__clagenticActivityLedger so Andy can read the ledger from a running browser session's devtools console without a debugger: window.__clagenticActivityLedger(). Read-only accessor over the plain counter object added in the previous commit; does not affect rendering or any store state.
…r-58c813) Instrumentation-only baseline for the lr-5edd64 redesign, per lr-58c813 item 3 — the highest-value probe, since it sizes the redesign. Counts every session where session.isProcessing (the plain mutable boolean, ~14 raw writers) disagrees with sessionActivity.isSessionActive(session) (the token-registry-derived value, correct at exactly one write site per lr-5edd64's diagnosis). Read-only: _recordActivityDivergenceIfAny never assigns session.isProcessing or session.activity, and never calls a registry-mutating export — a CI invariant test enforces this by source inspection. Bounded by construction: sampled once per session per existing idle-reaper tick (IDLE_CHECK_INTERVAL_MS = 60s), not per tool call — adds zero new per-event hot-path cost, reusing the reaper's own timer as the sampling clock. The total count increments exactly forever; a 20-entry ring buffer caps the retained per-event detail so a long-running daemon cannot grow memory unboundedly. getActivityDivergenceStats() is exported from lib/sdk-bridge.js and folded into bridge.getMemoryStats(), the existing accessor project-sessions.js already reads for the process_stats WS response.
…tats response (lr-58c813)
Folds sdk.getMemoryStats()'s new activityDivergenceCount and
activityDivergenceRecentSamples fields into the existing process_stats
WS response, mirroring the null-guarded pattern already used for
activeLiveCount/maxConcurrentSessions. This is the retrieval path for
the lr-58c813 item-3 baseline: send {type:'process_stats'} over the
admin/session WS connection and read activityDivergenceCount /
activityDivergenceRecentSamples off the response.
|
BOBBIE — blocking (1)
lr-c4da07 relationship: ORTHOGONAL. lr-c4da07 fixed getAllProjectSessions/computeAllProjectSessions (lib/server-hub-sessions.js, feeds hub_recent_sessions_list, cross-project Home Hub view) with per-user canAccessSession/canAccessProject filtering, merged and verified. This PR does not touch that function/file. process_stats is a different, per-project handler never brought under the same remediation -- same defect class (aggregation endpoint missing per-user filtering), not the same code, not worsened by this PR. window.__clagenticActivityLedger (lib/public/app.js, lib/public/modules/activity-latch.js): clean. recordActivityEdgeDecision receives the full (msgType, msgLocalId, activeSessionId, accepted) tuple but only increments three integer counters (accepted/rejected/acceptedFailOpen); msgLocalId/activeSessionId/msgType are never stored. No session ids retained, no per-event array, client-local same-origin. Registry hardening intact: lib/session-activity.js has zero diff in this PR; Object.create(null) on registry.tokens (line 83) confirmed present. Probe only calls the pure-read sessionActivity.isSessionActive(session); no new bracket-write key source anywhere. Read-only enforcement is real, not theater: test/activity-divergence-probe-lr-58c813.test.js:239-250 source-inspects the actual _recordActivityDivergenceIfAny function body from lib/sdk-bridge.js and asserts no session.isProcessing/session.activity write and no call to any registry mutator. Independently confirmed by direct read of the function body. Bounding clean: total divergence counter is an unbounded monotonic integer, not a growing collection; ring buffer hard-capped at 20 via length truncation; sampling driven by the idle-reaper own 60s setInterval tick, not client messages -- not client-amplifiable. Client ledger holds 3 integers, resets on page reload. package.json/package-lock.json confirmed zero diff in base..head, matching the PR claim. osv-scanner findings against the lockfile are pre-existing baseline, out of scope for this review. Secrets: gitleaks (4 commits) and trufflehog (base..head) both report zero findings. SAST: semgrep p/javascript ran clean against all 5 changed source files. scanners_run: gitleaks(clean), trufflehog(clean), semgrep(p/javascript,clean), osv-scanner(ran,pre-existing findings out of diff scope) |
|
PEACHES — blocking This instrumentation PR claims zero behavior change with a read-only probe, but the divergence probe violates that contract on its first idle-reaper tick. Blocking finding: The probe calls This violates the PR's central claim: ZERO BEHAVIOR CHANGE. No activity write site is touched. A lazy-initialized property on session objects is a structural mutation that can affect hidden-class optimization and object shape. The test misses this because it pre-calls Secondary finding: unbounded console.warn volume (medium severity) The probe emits No findings on gate refactors The app-messages.js refactors (status/done/auth_required handlers) correctly preserve behavior — each computes the same Existing tests intact test/activity-latch-lr-96e7da.test.js remains unchanged. |
…g (lr-58c813) The probe called sessionActivity.isSessionActive(session), which transitively calls ensureRegistry(session) and lazily assigns session.activity on first call. lib/sessions.js's two session constructors never initialize session.activity, so the probe was creating and writing session.activity on every session's first idle-reaper tick -- a real mutation, violating the instrumentation- only contract (PEACHES finding on PR #403). Adds _peekIsSessionActive(session), a non-mutating read that treats an absent session.activity as zero live tokens instead of calling the registry's own lazy-init path. lib/session-activity.js is untouched (out of scope, proven correct twice already) -- the probe is adapted to the existing API instead. Also fixes the CI source-inspection invariant, which previously only checked the probe body for direct assignments and could not see this transitive mutation. It now also bans calling sessionActivity.isSessionActive/getActiveCount directly, and the existing behavioral test is strengthened to construct a session with no activity property (matching the real constructors) and assert it stays absent after the probe runs -- this is the assertion that actually catches a transitive mutation; verified by stash-testing that it fails against the pre-fix probe. Drops the per-session sessionId field from divergence samples (BOBBIE finding): process_stats, which folds these samples into its response, has no admin/role gate unlike its update_now/kill_process siblings, so any authenticated client on a shared project could read another user's session id through it. Fixing that pre-existing gate gap is a separate, unrelated change; this PR just stops handing identifying data through the ungated path. Drops the per-divergence console.warn (PEACHES nit): MILLER's diagnosis predicts divergence may be common, and the warn fired once per diverging session per idle-reaper tick forever with no rate limit -- an unbounded log-volume risk. The counter + bounded sample ring are the actual deliverable; relying on those instead of a log line avoids the flood risk outright. Tests: npm test, 1464/1464 passing, 0 failures. TASK: lr-58c813
|
PEACHES — clean (lr-58c813 re-review) Re-verifying prior blocking findings against new head 42c17ff:
BOBBIE finding (sessionId removed) — VERIFIED. Sample fields (ts, rawIsProcessing, derivedIsActive, hasQueryInstance) remain actionable for sizing lr-5edd64 redesign. No auth gate on process_stats, so sessionId removal is correct. Checked:
All prior findings resolved. No new findings. |
|
BOBBIE clean. Re-audit of PR #403 at head 42c17ff (base ebba348), re-authored branch (5 commits: 951fb9c, c0bc259, c24c482, dbb8476, 42c17ff). Prior blocking finding (comment 5371676126, head dbb8476): process_stats WS handler (lib/project-sessions.js:668) has no auth/role gate, contrasting update_now:660 and kill_process:736, both gated on ws._clayUser.role === admin. This PR had folded session-identifying data (sessionId via getActivityDivergenceStats -> getMemoryStats -> activityDivergenceRecentSamples) into that ungated response. Verified at the new SHA: AMoS removed sessionId at the source rather than gating the handler. lib/sdk-bridge.js _recordActivityDivergenceIfAny now builds each divergence sample as ts, rawIsProcessing, derivedIsActive, hasQueryInstance only -- no sessionId, no session.localId under any name. Full path traced: _recordActivityDivergenceIfAny -> getActivityDivergenceStats() -> getMemoryStats() -> project-sessions.js:706-707 (process_stats response). No re-introduction anywhere in the chain. Sibling-field check: ts (epoch ms), two booleans, one boolean-coerced field. No path, title, user id, or any other identifying field present in the sample or in the client-side ledger (accepted, rejected, acceptedFailOpen -- three integers, lib/public/modules/activity-latch.js). Rest of process_stats payload unchanged vs base; only the two new fields were added. process_stats gate gap is confirmed still unfixed (project-sessions.js:668, no ws._clayUser.role check), explicitly deferred per the PR body and inline comments at project-sessions.js:701-705 and sdk-bridge.js. Acceptable: the only identifying data that had ridden this ungated path is now removed at the source, so the original exposure is resolved by removal, not relocated. Re-verified: lib/session-activity.js zero-diff, Object.create(null) hardening intact at line 83. New _peekIsSessionActive(session) in sdk-bridge.js is a plain for-in bracket-read only against registry.tokens, no bracket-write, no client-controlled key path, cannot mutate session or registry state. window.__clagenticActivityLedger returns three plain integers only, resets on page load. Bounding sound: activityDivergenceCount monotonic, sample ring buffer capped at 20, sampled once per session per idle-reaper tick, not client-amplifiable. package.json and package-lock.json zero diff. lr-c4da07 confirmed still orthogonal, no file it touched appears in this PR diff. scanners_run: gitleaks detect (5 commits scanned, 0 leaks), trufflehog git since-commit (0 verified/unverified secrets), semgrep config=auto (6 findings, all at pre-existing lines outside this PR diff hunks, not in scope), osv-scanner (skipped, package.json/lockfile zero-diff vs prior audit, no new dependency surface). |
|
Merged via clagentic-loadout v0.2.0
|
PR 403 follow-up: fixes both BLOCKING gate findings from PEACHES and BOBBIE on this instrumentation-only branch. Folded into this PR per instruction; no new PR opened.
What changed
PEACHES BLOCKING: probe was not read-only. _recordActivityDivergenceIfAny() called sessionActivity.isSessionActive(session), which transitively calls ensureRegistry(session) and lazily assigns session.activity on first call (lib/session-activity.js). lib/sessions.js session constructors (line 697 and 835) never initialize session.activity, so the probe was creating and writing that property on every session first idle-reaper tick, a real mutation that violated this PR original zero-behavior-change claim.
Fixed with a new non-mutating helper, _peekIsSessionActive(session), that reads session.activity directly and treats an absent registry as zero live tokens instead of calling the registry own lazy-init path. lib/session-activity.js is untouched, out of scope, proven correct twice already; the probe is adapted to the existing API rather than changing it.
CI invariant fixed, not just the code. The prior source-inspection invariant only checked the probe function own body for direct assignments (session.activity =), so it could not see the transitive mutation through a called function, exactly the blind spot PEACHES named, and the same class of gap that sank three consecutive fixes in this lineage (lr-6e20f7, lr-96e7da, and this one). Two things now cover it:
BOBBIE BLOCKING: session-id exposure through an ungated handler. Chose option (a): removed sessionId from divergence samples entirely, rather than gating process_stats on admin. Checked both frontend call sites (lib/public/modules/app-panels.js line 838, lib/public/modules/server-settings.js line 588); neither is admin-gated client-side, and neither currently reads sessionId out of activityDivergenceRecentSamples since nothing renders it yet, so removing the field breaks no consumer. The measurement goal (how often the two sources disagree, and roughly under what conditions: rawIsProcessing, derivedIsActive, hasQueryInstance, ts) survives without a per-session identifier. process_stats pre-existing lack of an admin/role gate, unlike its update_now/kill_process siblings, is a real, separate defect but is explicitly NOT bundled into this PR; filing it separately rather than expanding this instrumentation-only PR scope.
PEACHES nit: unbounded log volume. Dropped the per-divergence console.warn entirely rather than rate-limiting it. MILLER diagnosis predicts divergence may be common; the warn fired once per diverging session per idle-reaper tick (60s) forever with no bound. The counter (getActivityDivergenceStats().count) and the bounded sample ring are the actual deliverable; relying on those via process_stats avoids the flood risk outright instead of trying to calibrate a rate limit.
Zero-behavior-change claim, corrected
The original PR claimed zero behavior change. That claim was wrong: the probe was mutating session.activity on first observation of every session, exactly as PEACHES found. It is not merely reasserted here; it is now backed by a behavioral test that constructs a session identically to production and asserts the mutation does not happen, and that test was confirmed by stash-testing to fail against the pre-fix code. The corrected, actually-true claim is: the probe reads session.isProcessing and session.activity (when already present) without ever creating or writing either, and never calls a sessionActivity export that has a lazy-init side effect.
Constraints honored
Tests
npm test from repo root: 1464/1464 passing, 0 failures, run to completion (raw TAP tail confirms tests 1464, pass 1464, fail 0, and the new/changed tests in test/activity-divergence-probe-lr-58c813.test.js are individually visible as ok in the TAP output, not just implied by the aggregate). Known pre-existing flakes (test/daemon-bootstrap-guard.test.js, test/project-connection-hydrate-session-model-lr-041af8.test.js in isolation) not observed in this run and are not this PR concern regardless.
TASK: lr-58c813