feat(perception): activity_focus + activity_intensity domains - #6
Conversation
Co-Authored-By: Rooty
Centralizes the "is this session safe to publish into?" check. Validates status=="active" and rejects records older than max_age_h (default 12h) to avoid emitting events into a crashed-source-leftover "active" record. Co-Authored-By: Rooty
Catch Redis connection errors and naive/non-string started_at parse errors. All three would have surfaced as uncaught exceptions in real use. Add tests covering each case. Co-Authored-By: Rooty
Bare-entity keying broke as soon as two domains shared an entity name (activity_focus and activity_intensity both use the focused-app name). The second domain's PendingAdvice would overwrite the first's, and behavioral scoring would mix unrelated units. Tuple key makes each (domain, entity) its own tracking slot. Co-Authored-By: Rooty
Seven new bounded fields for the workstation-activity perception daemon. Title allowlist is a string (not tuple[str]) to avoid the tuple-coercion-of-chars footgun in from_env(); consumers split at use-site. Co-Authored-By: Rooty
Strip before non-empty check so " " is rejected as the spec intends. Add a test covering whitespace. Co-Authored-By: Rooty
Adds the Windows-only daemon file with lazy OS-import scaffolding, plus the three pure helpers: _normalize_app_name, _resolve_title, _CounterState. Module stays importable on Linux CI via sys.modules fakes; CLI entrypoint will check _WIN32_AVAILABLE in a later task. Co-Authored-By: Rooty
_FocusState tracks the active app + dwell start; on_focus_change emits a PerceptionEvent dict for the PREVIOUS span on every transition after the first (no zero-dwell sample on startup). Title fields obey the allowlist; idle clamped to total dwell. Co-Authored-By: Rooty
_IntensityWindow tracks keystroke + mouse counts per focus span. build() emits a PerceptionEvent dict only when window_duration_s >= min_window_s AND total events >= min_events; otherwise None. Title filtered via allowlist; idle clamped to window duration. Co-Authored-By: Rooty
_SessionReader wraps the Redis read of augur:session:current with validity (status=='active' + age <= max_age_h) and tracks session changes so the daemon can flush its NATS buffer on transition. Defensive against Redis errors and malformed records. Co-Authored-By: Rooty
All None-return paths in _SessionReader.read_current() now clear last_seen (except the bare Redis-exception path). Eliminates stale state after malformed input. Type annotation redis_client: Any for clarity. Add 2 tests covering malformed-JSON recovery and None→valid transition. Co-Authored-By: Rooty
Composes _FocusState + _IntensityWindow + _SessionReader + _BufferedPublisher into the daemon's run() loop. Emits truncated intensity samples before focus changes (preserves focus-span boundary). _probe_nats_reachable + main() guard refuses to start without Win32 deps or a reachable NATS, with explicit remediation hints on stderr. Co-Authored-By: Rooty
_BufferedPublisher docstring now explains the best-effort write-and-forget design (no runtime drain; buffer cleared on session change). ActivityMonitor.run() defers last_sampled initialization until first focus is established, so a long "waiting for active session" doesn't force an immediate sample on the first focus. Co-Authored-By: Rooty
Three changes from the daemon-chunk simplify pass: - Input listeners now use time.monotonic() to match the main loop's span tracking. Wall-clock vs monotonic mixing would have made last_input_time always look "in the future" and idle calculation always return 0. - Extract _clamp_idle_seconds() pure helper to remove duplication between _FocusState and _IntensityWindow. - Drop unused idle_threshold_s field from both state classes per YAGNI; AugurConfig knob stays for future use. Co-Authored-By: Rooty
Adds build_activity_focus_prompt + build_activity_intensity_prompt and registers them in DOMAIN_HANDLERS so single-domain advice paths work. Extends describe_signal with two cases so correlation prompts render the new domains correctly. Co-Authored-By: Rooty
Extends render_anomaly_line with activity-aware branches that surface raw human-meaningful values (active_dwell_s, ipm, keystroke counts) instead of the EWMA-input log1p_seconds. render_advice handles the new domains via domain-aware dispatch. Both functions now dispatch on domain instead of hard-coding chess logic. Chess and typing domains continue to work as before. Co-Authored-By: Rooty
T10's refactor keyed render_anomaly_line/render_advice on file names (chess_board, typing_monitor) instead of the domain values emitted by perception sources (chess, typing). Chess and typing events were silently falling through to the generic fallback. Add regression tests so the chess/typing branches stay reachable. Co-Authored-By: Rooty
Lists the Windows-host-only deps separately from requirements.txt so Linux installs don't try to compile pywin32. run_augur.sh gets an inline note explaining the optional activity_monitor companion that runs on Windows alongside the WSL backend. Co-Authored-By: Rooty
Adds tests/integration/test_activity_pipeline.py with 7 tests covering: activity_focus baseline creation, activity_intensity baseline creation, cross-domain (activity_focus+typing) correlation, cross-stream (activity_focus+activity_intensity) correlation, feedback_collector (domain, entity) keying isolation, and the get_active_session helper against real Redis. Co-Authored-By: Rooty
Skipped unless AUGUR_OLLAMA_URL is set. Asserts only the advice shape (string, non-empty) to keep it stable across model upgrades. Co-Authored-By: Rooty
- T16: assert both activity_focus and activity_intensity advice appear as distinct domains in feedback (keying isolation verified) - clean_redis: add augur:feedback:* to cleanup patterns so feedback records don't leak across tests - drop local session_id fixture in favor of conftest's uuid-based one Co-Authored-By: Rooty
- activity_monitor.py: rename loop var l → listener (ruff E741) - feedback_collector.py: ruff format wrapped active_tracking decl - test_feedback_collector_keying.py: regex tolerates the new wrap Co-Authored-By: Rooty
- T18 slow test: filter on 'domain' not 'primary_domain' (advisor output uses the former; the latter only appears in T16 fake payloads) - Drop unused activity_idle_threshold_s config (state classes dropped it during simplify; config field was left dangling) - Add Redis startup probe to activity_monitor main() matching the existing NATS probe pattern - Comment the severity-keyed adaptive-window limitation in correlator per spec §12 Co-Authored-By: Rooty
Wintersta7e
left a comment
There was a problem hiding this comment.
Review — comprehensive pass
Reviewed code quality, test coverage, error handling, comment accuracy, and type design against the project's domain-agnostic invariants. Findings grouped by severity; nothing here disqualifies the design (the domain-agnostic plumbing is clean and the prerequisite fixes are correct) — the criticals are concentrated in the new 657-LOC daemon, which is precisely the area that has no unit-test coverage of its integration logic (CR-7). Land that test first and the other criticals get cheap.
🔴 Critical — fix before merge
CR-1. Typing renderer reads non-existent top-level wpm — every real typing anomaly will render wpm=? in production. output/console_display.py:99. The detector publishes WPM as context.avg_wpm and only mirrors chess-specific fields to top level (detection/anomaly_detector.py:314-335). The new unit test hand-builds a payload with top-level wpm, masking the regression.
Fix: wpm = (data.get("context") or {}).get("avg_wpm", "?") and re-shape the test payload to match detector output.
CR-2. Session-boundary contamination — focus/intensity state survives a session_id change. perception/activity_monitor.py:449-457. On session A → session B, _resolve_session rewrites self.focus.session_id / self.intensity.session_id in place but does not reset current_app, current_focus_started_at, window_started_at, counters, or the main-loop tracking vars (last_app, last_title, last_sampled). The next on_focus_change emits a span belonging to session A — potentially hours-long if there was a gap — tagged with session B's id. The detector then fires a high-severity outlier anomaly on session start.
Fix: On changed_since_last, set self.focus = None / self.intensity = None so the next iteration rebuilds them, and clear last_app/last_title/last_sampled in the main loop.
CR-3. _get_foreground collapses 4+ distinct failure modes into one <unknown> entity → baseline poisoning. perception/activity_monitor.py:459-479. Locked workstation, psutil.AccessDenied on elevated processes, psutil.NoSuchProcess race, stale HWND pywintypes.error, System Idle Process (pid=0) all funnel to (_UNKNOWN_APP, None). The outer log is DEBUG, invisible at default INFO. The detector learns an EWMA baseline for <unknown> that is actually "average of all my errors" and permanently corrupts anomaly detection for that entity.
Fix: Catch specific exceptions; distinguish <no_foreground> (hwnd==0), <denied> (AccessDenied), <gone> (NoSuchProcess/ZombieProcess), <unknown> (genuinely unparseable). Bump outer log to WARNING with the exception class.
CR-4. pynput listener callbacks have no exception handling — single bad call silently disables the daemon. perception/activity_monitor.py:506-524. _on_key / _on_mouse reach into self.counter (which holds a threading.Lock); pynput's Listener runs callbacks on a worker thread and stops the listener on any callback exception. If the thread dies, counter.drain() returns (0, 0, 0) forever and intensity readings look exactly like an idle user — no log signal. The try/except at line 516-524 only guards .start(), not in-flight calls.
Fix: Wrap each callback body in try/except + log; periodically check kbd.is_alive() / mse.is_alive() in the main loop and log.error (or restart) on death.
CR-5. _BufferedPublisher is a drop log, not a buffer — silent data loss on NATS hiccups. perception/activity_monitor.py:340-374 + :481-486. Docstring is honest ("buffered events are NOT replayed — they accumulate until session change, then flush() clears them") but the name misleads. A 30s NATS blip silently loses every focus/intensity event during the window, no dropped_total, no replay, no synthetic gap marker.
Fix: Either implement actual replay-on-reconnect (stamp replayed_from_buffer=True so the detector can opt out) or rename to _DroppedEventLog and add a dropped_total counter surfaced at session-end / flush time.
CR-6. _resolve_primary_domain re-introduces the very bug this PR fixes. perception/feedback_collector.py:62-73.
return advice_data.get("domain") or advice_data.get("primary_anomaly", {}).get("domain") or "chess"The PR's premise is that (domain, entity) keying prevents bare-entity collisions across domains. The "chess" fallback silently re-attributes any malformed advice payload to "chess", builds a ("chess", entity) tracking key, and pollutes chess baselines + reflection statistics. The justification "preserves historical behaviour" describes exactly the buggy behaviour the PR is correcting.
Fix: Drop the fallback; log.error and skip the advice event when domain resolution fails.
CR-7. ActivityMonitor.run() has zero unit-test coverage. perception/activity_monitor.py:488 — explicitly annotated # pragma: no cover - exercised manually. The 110-line loop is the only place that wires _get_foreground ↔ _FocusState.on_focus_change ↔ _IntensityWindow.build ↔ _BufferedPublisher ↔ _SessionReader. The pure helpers are well-tested in isolation but the integration tests bypass the daemon entirely — they hand-craft PerceptionEvent objects with the schema the test author expected the daemon to emit. CR-2, CR-3, CR-4 above would all slip through CI.
Fix (low-cost, high-value): A single synthetic-clock unit test driving run() for 3-5 iterations with mocked time.monotonic + mocked _get_foreground + a MagicMock NATS client, asserting the (subject, payload) sequence. Plus a PerceptionEvent.from_dict(_FocusState.on_focus_change(...)) round-trip test (~10 LOC) to lock in schema compatibility forever.
🟠 Important — should fix
IM-1. Redis transport errors conflated with "no active session". blackboard/session.py:65-70 and perception/activity_monitor.py:296-337. Both treat redis.ConnectionError / TimeoutError / AuthenticationError identically to "session ended." A transient WSL-to-Windows blip causes a spurious None→real_id transition on recovery, which triggers changed_since_last=True, which flushes the buffer.
Fix: Catch redis.RedisError (not bare Exception), log at WARN, surface a last_read_failed flag so the run loop skips the buffer flush on recovery. Let AuthenticationError propagate.
IM-2. get_active_session helper is dead code; _SessionReader reimplements its validity logic. blackboard/session.py:52-93 vs perception/activity_monitor.py:296-337. The PR introduces a centralized helper, tests it, then doesn't call it from production. feedback_collector.get_session_id is a third copy with subtly different semantics (accepts status != "active").
Fix: _SessionReader should wrap get_active_session(...) + track changed_since_last. Document why feedback_collector intentionally skips status check, or fix it.
IM-3. New event builders return dict[str, Any] instead of PerceptionEvent. perception/activity_monitor.py:181-203, 255-276. typing_monitor.py:245-259 and chess_board.py construct PerceptionEvent(...) directly — the strict-schema-at-ingestion-boundary invariant. The new daemon is the only producer that bypasses it; schema drift only surfaces downstream at feedback_collector.on_perception.
Fix: Return PerceptionEvent from on_focus_change and build; _publish calls payload.to_bytes().
IM-4. Advisor handlers produce ?-filled prompts and pay the 68.5s Ollama cost. reasoning/augur_advisor.py:206-263. Every required context.* field defaults to "?". Malformed input → prompt full of question marks → Ollama hallucinates → bad advice published.
Fix: Validate required context fields up-front; raise ValueError(...) and have the advisor loop log + skip.
IM-5. Misapplied # noqa: F401 on imports actively used at runtime. perception/activity_monitor.py:18-30. Every one of asyncio, json, math, socket, sys, time, uuid, deque, datetime, timezone, Any is used at runtime. The PEP-563 ruff gotcha applies only to deferred-annotation-only imports. Blanket markers train the codebase to silence F401 reflexively, obscuring real cases.
Fix: Remove # noqa: F401 from all imports actually referenced at runtime.
IM-6. min_events ∈ [0, 100] admits a degenerate intensity gate. activity_intensity_min_events validation. min_events=0 makes total_events >= min_events always true; every window emits regardless of activity, flooding the detector with zero-IPM samples.
Fix: Tighten lower bound to 1.
IM-7. Test privacy assertion is too weak to lock in non-leakage. tests/test_advisor_activity.py:79:
assert "None" in prompt or "title" not in prompt.lower()Disjunction that passes with prompt = "" or prompt = "title is None". The contract being defended — raw titles must not appear in LLM input outside the allowlist — is not actually pinned.
Fix: Build a prompt with a sentinel-string title in context that should have been filtered, then assert "sentinel-string" not in prompt.
IM-8. Integration tests use blanket asyncio.sleep waits. tests/integration/test_activity_pipeline.py:170, 200, 248, 261, 329, 344, 398, 528. ~30s fast-suite latency plus 90s asyncio.sleep in the slow Ollama test. conftest.py already exposes wait_for_redis_pattern.
Fix: Replace fixed sleeps with wait_for_redis_pattern / list-polling against received.
IM-9. test_feedback_collector_keying.py is a regex test, not behavioural. Greps source for dict[tuple[str, str], PendingAdvice]. Breaks on a harmless type-alias refactor, silently passes if someone introduces a bare-entity lookup via a different variable name.
Fix: Seed two PendingAdvice under (activity_focus, code) and (activity_intensity, code), fire perception events, assert both records advance.
🟡 Suggestions
_CounterStatedocstring (activity_monitor.py:101-103) claimslast_input_timeis a unix timestamp; it'stime.monotonic().reasoning/correlator.py:144-147annotation is factually accurate but 4 lines for a 1-line policy. Drop the "Future spec…" reference.- Stringly-typed domain literals appear in 6 files. A
Domain = Literal[...]alias incontracts.pywould catch typos at every dispatch site. (domain, entity)tuple key →NamedTuplesoTrackingKey(domain=..., entity=...)is order-safe at every construction site.PendingAdvice.explicit_rating: str→StrEnumorLiteral["y", "n", "no_response"].- Multi-line docstrings on helper dataclasses (
_CounterState,_FocusState,_IntensityWindow,_SessionReader,_BufferedPublisher) and per-function docstrings across all 8 new test files violate project comment policy ("default to writing no comments; never multi-paragraph docstrings"). tests/test_console_display_legacy_domains.py:1-7references a task identifier that won't make sense later — drop the task-scope language.- Empty
except Exception: passin listener teardown (activity_monitor.py:594-598). infrastructure/run_augur.shWindows hint sayslocalhost:4222but the daemon's CLI correctly uses the WSL IP — pick one.- Section-header banner comments in
output/console_display.pyandreasoning/augur_advisor.pynarrate WHAT theif domain ==line below already says. tests/test_feedback_collector_keying.py:18usesPath("perception/feedback_collector.py")— relies on cwd being repo root.tests/test_activity_monitor.py:122-143"thread-safety" test only assertsjoin()completes; doesn't verify no increments are lost. Removing the lock wouldn't fail it.- Two parallel dispatch tables in
augur_advisor.py:DOMAIN_HANDLERS(registry) and thedescribe_signalif-chain. New domains require edits in both.
✅ Strengths
- Domain-agnostic invariant preserved. Detector + correlator untouched (only +4 LOC comment annotating the pre-existing severity-keyed limitation).
- PersistenceManager hard rule respected — no direct Redis writes in any new code.
- Centralized config respected — every new tunable in
AugurConfigwith__post_init__bounds validation andAUGUR_*env-var coverage; no hardcoded URLs. - Excellent CLI in
main()— pre-flight NATS/Redis probes, actionable remediation messages with exact env-var names + WSL commands, distinct non-zero exit codes (2/3/4) for wrapper scripts. - Privacy default for titles —
_resolve_titlereturnsNoneunless the app is on an opt-in allowlist. - Thread-safe input counter with explicit lock; atomic drain.
- Cross-domain correlation integration test correctly uses varied baseline values (zero-variance gotcha respected).
- Config bounds exhaustively parametrized (lower + upper for every numeric field).
- The
(domain, entity)keying fix infeedback_collector.on_adviceexplicitly finalizes displaced pending advice — the kind of error-handling discipline the daemon itself should match. - Slow Ollama test asserts schema not content — right discipline for LLM-touching tests.
Recommended action order
- Fix CR-1 (typing renderer) — single-line, ships visibly broken otherwise.
- Fix CR-6 (
_resolve_primary_domainfallback) — re-introduces the bug the PR fixes. - Fix CR-2 / CR-3 / CR-4 (daemon correctness: session boundary, foreground failure modes, listener exception handling) — these poison learned state, hard to recover from.
- Fix CR-5 (rename buffer or implement replay) and CR-7 (add the run-loop synthetic-clock test) together — same problem from two angles.
- IM-1 / IM-2 (consolidate session-validity logic) — removes 30 lines of duplication, prevents drift.
- IM-3 (return
PerceptionEventfrom builders) — protects schema invariant at producer. - IM-4 through IM-9 — pick off as time allows; none block merge alone.
- Suggestions: polish pass.
CR-1: typing renderer reads context.avg_wpm (was top-level wpm) CR-6: drop "chess" fallback in _resolve_primary_domain; log+skip IM-5: remove # noqa: F401 from imports actually used at runtime IM-6: tighten activity_intensity_min_events lower bound to 1 IM-7: sentinel-string privacy test for activity_focus prompt IM-9: behavioral feedback-keying test (replaces regex grep) S1: _CounterState docstring says monotonic, not unix S7: drop task-scope language from regression test docstring S8: listener.stop() failures logged, not swallowed S10: drop redundant section-header comments; update banner Co-Authored-By: Rooty
CR-2: reset _FocusState/_IntensityWindow on session change so
spans don't carry across session boundaries
CR-3: _get_foreground distinguishes <no_foreground>/<denied>/
<gone>/<unknown> instead of one bucket; log at WARNING
CR-4: listener callbacks have try/except; loop checks listener
health every ~10s and logs on death
CR-5: rename _BufferedPublisher → _DroppedEventLog + dropped_total
counter (no replay; name now matches semantics)
IM-1: catch transport errors specifically; last_read_failed flag
prevents recovery from triggering spurious session-change
buffer flush
IM-3: event builders return PerceptionEvent (was dict); _publish uses
.to_bytes() for schema validation at producer
S9: doc reconciliation — both daemon CLI and run_augur.sh
recommend localhost first, WSL IP fallback (no change needed)
Co-Authored-By: Rooty
CR-7: synthetic-clock test for ActivityMonitor.run() (skipped pending
richer harness) + PerceptionEvent round-trip tests pin schema
IM-2: _SessionReader delegates to get_active_session; removes
duplicate validity logic + IM-1 last_read_failed flag (spurious
flush on Redis recovery is bounded by drop-log)
IM-4: advisor prompt builders raise ValueError on missing required
context fields (no more ?-filled prompts → Ollama)
IM-8 (integration test polling) lands in a follow-up.
Co-Authored-By: Rooty
IM-8 from PR #6 review. T12-T16 now poll for Redis keys / NATS message arrival instead of sleeping a fixed 2-3s after publish. Test logic unchanged; flakiness drops and runtime tightens. Co-Authored-By: Rooty
S2: shorten severity-keyed-window comment in correlator
S3: expose Domain = Literal[...] alias in contracts.py
S4: promote (domain, entity) → TrackingKey NamedTuple in
feedback_collector
S5: PendingAdvice.explicit_rating is Literal["y","n","no_response"]
S6: strip multi-paragraph docstrings that narrated WHAT
S12: thread-safety test now verifies no lost increments (was
just asserting join completed)
S13: parallel DOMAIN_DESCRIBERS registry next to DOMAIN_HANDLERS;
test pins their keys identical
Co-Authored-By: Rooty
|
Review addressed across 5 commits. Mapping: 1e62e01 — R1 correctness + style 8099528 — R2 daemon correctness 7d5a97c — R3 tests + refactor 5bfd21f — R3 follow-up: int test polling e0e19f4 — R4 types + dispatch State
Notes on a few items
Ready for re-review. |
Wintersta7e
left a comment
There was a problem hiding this comment.
Re-review — verified at e0e19f4
Independently verified: unit 462 passed, 1 skipped (1.91s); fast integration 32 passed, 4 deselected (58s); ruff check + ruff format --check clean. Test/lint claims match reality.
Critical fixes — verified ✅
| # | Finding | Location | Verdict |
|---|---|---|---|
| CR-1 | typing renderer reads top-level wpm |
output/console_display.py:97 |
Fixed — reads context.avg_wpm |
| CR-2 | session-boundary state survives session change | activity_monitor.py:430-443 |
Fixed — self.focus=None / self.intensity=None on changed_since_last; main loop rebuilds; last_app/last_title reset via the if self.focus is None branch |
| CR-3 | foreground sentinels collapsed to one bucket | activity_monitor.py:445-480 |
Fixed — <no_foreground> / <denied> / <gone> / <unknown> distinguished; specific psutil.AccessDenied and (NoSuchProcess, ZombieProcess) exceptions; outer log now WARNING with exception class |
| CR-4 | pynput callback failure silently kills listener | activity_monitor.py:516-530, 566-575 |
Fixed — callbacks wrapped in try/except; periodic is_alive() health check every ~10s with log.error on death; daemon limps on with log rather than restart — defensible conservative choice |
| CR-5 | _BufferedPublisher is a drop log, not a buffer |
activity_monitor.py:312-355 |
Fixed — renamed to _DroppedEventLog; dropped_total counter; honest docstring "Does NOT replay"; overflow + flush both increment counter; if n: guard prevents noise on empty flush |
| CR-6 | _resolve_primary_domain "chess" fallback |
feedback_collector.py:74-82, 308-314 |
Fixed — returns None on missing; on_advice logs error + returns; baseline_raw correctly keyed on primary_domain not "chess" |
| CR-7 | run() zero unit coverage |
test_activity_monitor.py:560-614 |
Partial. Round-trip tests at 574, 597 pin the schema invariant — the most important contract. Synthetic-clock test is a @pytest.mark.skip(...) stub with TODO: Fix asyncio.sleep patching recursion; run-loop wiring (focus-change ordering, intensity-reset timing, last_app update sequencing) remains uncovered. Acceptable trade-off; see nits. |
Important fixes — verified ✅
All 9 confirmed. Two worth calling out explicitly:
- IM-2 trade-off analysis.
_SessionReader.read_currentnow delegates toget_active_session(activity_monitor.py:300-303). The droppedlast_read_failedflag: a Redis blip yieldsNone;_update_seen(None)setschanged_since_last=Falsebecause of the(sid is not None) and ...guard, so no flush during the blip. On recovery,None → real_siddoes flagchanged_since_last=Trueand triggersself.drops.flush()— but the drop-log is empty (NATS publish was unaffected by Redis being down), so theif n:guard atactivity_monitor.py:347makes the flush a no-op. The trade-off is genuinely cost-free in the common case. Good call. - IM-3. Builders return
PerceptionEvent;_publishuses.to_bytes(); round-trip tests pin the contract. Schema is now validated at the producer instead of downstream atfeedback_collector.on_perception. This was the largest structural win of the response.
Suggestions — verified ✅
Domain = Literal[...] at contracts.py:13, TrackingKey(NamedTuple) at feedback_collector.py:35 used at all construction sites, explicit_rating: Literal["y", "n", "no_response"] at feedback_collector.py:117, DOMAIN_DESCRIBERS registry at augur_advisor.py:374. All clean.
Three small nits worth tracking (none are blockers)
-
Stale comment at
perception/activity_monitor.py:55:# Runtime import; used in _SessionReader.read_current for specific error handling.After IM-2 consolidation,_SessionReaderdoes no specific redis-error handling —get_active_sessiondoes. Theredismodule is now used only for the type-hint string at line 294 (# redis.Redis at runtime). The# noqa: F401is correct under PEP-563, but the explanatory comment is now inaccurate. Either tighten to "Runtime import; only referenced in type-hint strings" or drop the import + noqa altogether. -
CR-7 skipped test body at
test_activity_monitor.py:560-571is barepass. The two round-trip tests cover the schema invariant, but the run-loop wiring is uncovered: a bug swappingawait self._publish(SUBJECT_INTENSITY, ev)andawait self._publish(SUBJECT_FOCUS, ev_focus)in the focus-change block (activity_monitor.py:591-602) would not be caught by any current test. Acceptable trade-off; worth tracking as a known gap (capture in a TODO comment or a follow-up issue). -
feedback_collector.get_session_idatfeedback_collector.py:258-268still does raw Redis read with different semantics fromget_active_session(accepts non-active sessions, falls back touuid.uuid4()). Per the response, this is intentional ("accepts ended sessions for late-arriving feedback"). A single-line comment in the code citing this would prevent the next reviewer from re-raising the same finding.
Verdict
LGTM — ship it. All 7 criticals addressed with sound engineering. The two trade-offs flagged in the response (CR-7 skip, IM-1 last_read_failed drop in service of IM-2 consolidation) are well-reasoned and bounded. The three nits above are paper cuts, not blockers.
The PR has come a long way: domain-agnostic invariants stay clean, schema is now enforced at the producer, session-validity logic has one source of truth, and the daemon's failure modes are honestly named (_DroppedEventLog, sentinel app names).
N1: drop dead redis import (only referenced in a type-hint string
since IM-2 removed _SessionReader's specific error handling)
N2: skipped run() test enumerates the wiring it would cover
(publish ordering, intensity reset timing, session-change reset)
N3: comment on feedback_collector.get_session_id explaining the
intentional lax semantics vs get_active_session
Co-Authored-By: Rooty
|
Nits addressed at c833ff8:
462 unit + 32 fast int still green; ruff clean. |
Summary
activity_focus+activity_intensityperception domains for workstation activity (per-app focus dwell + interaction intensity).perception/activity_monitor.py) publishes to WSL NATS via the existing pipeline (detector → correlator → advisor → feedback → reflection).get_active_sessionvalidity helper inblackboard/session.py, and(domain, entity)keying inperception/feedback_collector.py(bare-entity keying would collide when two domains both use an app name as entity).Notes
requirements-windows.txt(separate from mainrequirements.txtso Linux installs aren't affected). Daemon CLI guards against missing Win32 deps and unreachable NATS/Redis with explicit remediation hints.correlator.pyadaptive-window state remains severity-keyed (not domain-keyed). This is a pre-existing limitation; tuning an activity-typing pairwise rule affects any pair with the same severity combo. Now annotated in code.