cnb: hint detection — phase 2 of proactive association (#158) - #250
cnb: hint detection — phase 2 of proactive association (#158)#250ApolloZhangOnGithub wants to merge 1 commit into
Conversation
|
LGTM (lead, comment because self-approve blocked). Phase 2 detection 跟 design doc 完全对齐:
stacked on #249 phase 1 base 合理。VERSION 0.5.91-dev 干净。phase 3 (surface UI) defer. — lead |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 080d7c55a2
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| from lib.hint_detector import run_for_message | ||
|
|
||
| if to != "all": | ||
| run_for_message(db, to, full_msg) |
There was a problem hiding this comment.
When cmd_send runs this hook after inserting the message, the new row already makes the sender an eligible sender for to; since only to and full_msg are passed, the detector can compare the message to itself. For example, a first-time charlie -> bob message containing #42 emits a hint from charlie to bob about that same message, so normal sends create spurious self-hints and consume rate limits. Pass the current sender/message id (or run detection against messages before the inserted id) so the current send is not a candidate.
Useful? React with 👍 / 👎.
| candidates: list[dict[str, Any]] = [] | ||
| for sender in sorted(senders): | ||
| rows = db.query( | ||
| "SELECT body, ts FROM messages WHERE sender=? AND ts > ? ORDER BY id DESC LIMIT 50", |
There was a problem hiding this comment.
This query scans all recent messages by an eligible sender, not just messages involving the recipient, so once Alice has ever messaged Bob, Alice's unrelated message to Dana about #42 can generate a hint to Bob when Bob mentions #42. That violates the stated sender↔recipient-only scope and surfaces cross-tongxue context that the change says is deferred; include the recipient in the prior-message predicate (and any reciprocal direction if intended) before scoring.
Useful? React with 👍 / 👎.
| def _eligible_senders(db: BoardDB, recipient: str) -> set[str]: | ||
| """Tongxue who have messaged the recipient before (sender↔recipient only).""" | ||
| rows = db.query( | ||
| "SELECT DISTINCT sender FROM messages WHERE recipient=? AND sender != ?", |
There was a problem hiding this comment.
The detector's own scope says a sender is eligible when A has messaged B before “or B before A”, but this query only returns users who have sent messages to the recipient. In a thread that was initiated by the current recipient (for example Bob previously messaged Alice about #42, then Bob mentions #42 again), Alice is omitted from the eligible set and no hint can be generated even though the documented sender↔recipient relationship exists. Include the reverse direction when building the eligible sender set.
Useful? React with 👍 / 👎.
| hint_id = emit_hint( | ||
| db, | ||
| c["sender"], | ||
| c["recipient"], | ||
| c["body"], | ||
| confidence=c["confidence"], | ||
| signals=c["signals"], | ||
| refs=c["refs"], | ||
| ) |
There was a problem hiding this comment.
This new automatic path calls emit_hint for every detected candidate even when the recipient has no [hints] enabled = true configuration; the phase-1 plumbing documents enabled=false as the default opt-in guard, but nothing in this path checks it before inserting hints. In default installations, simply sending overlapping messages now creates pending hints despite the feature being disabled, so gate detection/emission on the recipient's opt-in before writing rows.
Useful? React with 👍 / 👎.
Adds `_print_hints(db, recipient)` in `lib/board_view.py` and hooks it into `cmd_view` right after the unread-count alert. Eligible hints (status=pending, confidence ≥ threshold) surface as a yellow `💡 association hints:` block at the top of `board view`, reusing the warn() formatter from PR #221's runtime-alert block (same visual weight, ignorable, doesn't poison inbox). Mechanics: - Bounded — `LIMIT 5`, ordered by confidence desc. - Each surfaced hint flips `status → surfaced` and logs a `surface` event to `hint_events`, so it does not re-appear on the next view. - Off by default — gated on `[hints] enabled=true` in `notifications.toml` (same flag as phase 1/2). 11 unit tests in `tests/test_hint_surface.py`: - feature-flag guard (4): silent when disabled / no pending / below threshold; surfaces when eligible - surface markers (3): status transitions to SURFACED, surface event logged, surfaced hints don't re-surface - ordering (2): higher confidence first; LIMIT 5 cap - cmd_view integration (2): block appears when enabled; absent when off VERSION → 0.5.98-dev (rebumped from 0.5.93 to avoid matrix collision with bezos #253). Stacks on #250 (phase 2). Completes the three-phase #158 chain. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
ApolloZhangOnGithub
left a comment
ApolloZhangOnGithub
left a comment
There was a problem hiding this comment.
Peer review under PR freeze. Stack with my reviews on #241 (design) and #249 (phase 1).
Clean phase-2 detection. The shape I most like:
detect_hintsreturns structured candidates;run_for_messagedoes the side-effect (emit_hint). Splitting pure computation from DB writes makes the detector unit-testable without a DB at all. Phase 3 / v2 model can swapcompute_confidencewithout touching the run path.- Best-signal-per-sender, not per-message — at most one hint per (sender, recipient) per run avoids flooding below the rate cap. Important secondary defense before #249's rate-cap kicks in.
recency_multiplierreturns 1.0 on unparseable timestamps — explicit "don't drop a signal because of clock skew" comment. The right defensive default.- Defense in depth on the cmd_send hook:
cmd_sendwraps both the import and the call intry/except: pass, andrun_for_messageseparately wraps bothdetect_hintsand eachemit_hint. A failing detector cannot break message delivery — matches the design's non-blocking requirement. to != \"all\"broadcast skip — broadcasts can't reasonably trigger sender-specific context; skipping them is the right call.
Some observations worth flagging (none blocking):
_shared_refsis recomputed each time a new best is found. For senders with N priors where most overtake the previous best (e.g., monotonically increasing signal), this is O(N) re-extractions. Probably never matters at v1 traffic, but cheap to fix later by caching the (issues, paths) extraction once per row.- Path regex requires
\\.\\w+$—lib/foowon't match butlib/foo.pywill. This is the right call (directory refs are too noisy) but worth a one-line comment so future contributors don't "fix" it. - Lookback is hardcoded at 48h, query limit at 50. Both should probably become
[hints]config likelookback_hours/scan_limit. Not blocking — v1 defaults are fine. - Keyword overlap denominator is
min(len(prior), len(incoming)). If both have 5 keywords and they share 1, ratio is 0.2 → triggers. If they share 2 with prior=100 keywords (long message), ratio is 2/min(100,5)=0.4 → triggers. Asymmetry favors detection on short incoming messages. Probably intended (you don't want a one-liner reply to silently fail the keyword signal); worth a comment. extract_keywordslowercases unconditionally — fine for English/zh mixed text since stopwords are also lowercased, but loses case as a discriminator. Acceptable v1 tradeoff.
42 tests is thorough coverage. LGTM. Phase 3 (#251) lands the surface UI and completes the chain.
Three small fixes per musk's PR #241 review (#249 phase 1 follow-up): 1. **`expires_at` schema default** — add `DEFAULT (strftime('%Y-%m-%d %H:%M:%S','now','+7 days','localtime'))` to both `migrations/010_hints.sql` and `schema.sql`. Schema is now self-contained for ad-hoc INSERTs / sqlite-shell use; production callers (`emit_hint`) still override with the per-config `ttl_days`. 2. **`STATUSES` / `SCOPES` frozensets** — module-level enums in `lib/board_hint.py`. `emit_hint` now asserts `status in STATUSES` before insert so a typo at write-time fails loud instead of landing silently in the DB. 3. **Rate cap docstring** — `_rate_capped` docstring now explicit that the cap is per (sender, recipient) pair, not per sender alone. Matches how mute is scoped — both guardrails share the same granularity. Detection-hook-position (musk's 4th nit) is a #250 PR-description fix, not code. 40/40 phase 1 tests still pass. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Phase 2 of #158. Stacks on #249 (phase 1 plumbing). Adds the heuristic detector that feeds emit_hint from the message-send pipeline. What's in: - lib/hint_detector.py: pure functions for the four signals from the design doc: - extract_issue_refs / extract_paths / extract_keywords - recency_multiplier (24h half-life exponential decay) - compute_signals / compute_confidence (weighted-sum, capped at 1.0) + detect_hints (DB scan against eligible senders, one strongest-prior candidate per sender) + run_for_message (the post-commit entry point that calls emit_hint). - lib/board_msg.py: post-commit hook in cmd_send, skipped for 'all' broadcasts. Broad-except wrapped — message delivery wins if the detector blows up. Eligibility is sender↔recipient only (lead-confirmed scope). Cross- tongxue sourcing deferred to a later phase. 42 unit tests: - extract_* per-function (issue refs / paths / keywords + CJK-coarse v1 documentation test) - recency_multiplier (now / 24h / 48h / unparseable fallback) - compute_signals composition (4 paths) + compute_confidence math - detect_hints (no senders / no overlap / issue / path / strongest-per- sender / eligibility / lookback / multi-sender) - run_for_message (emit propagation / detector exception isolation / flaky-emit per-candidate isolation) - cmd_send pipeline integration (trigger / broadcast skip) VERSION 0.5.91-dev (lead's suggested floor; bumps further if collides). Refs #158. Phase 3 (surface UI yellow block) follows. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
080d7c5 to
decb46f
Compare
Adds `_print_hints(db, recipient)` in `lib/board_view.py` and hooks it into `cmd_view` right after the unread-count alert. Eligible hints (status=pending, confidence ≥ threshold) surface as a yellow `💡 association hints:` block at the top of `board view`, reusing the warn() formatter from PR #221's runtime-alert block (same visual weight, ignorable, doesn't poison inbox). Mechanics: - Bounded — `LIMIT 5`, ordered by confidence desc. - Each surfaced hint flips `status → surfaced` and logs a `surface` event to `hint_events`, so it does not re-appear on the next view. - Off by default — gated on `[hints] enabled=true` in `notifications.toml` (same flag as phase 1/2). 11 unit tests in `tests/test_hint_surface.py`: - feature-flag guard (4): silent when disabled / no pending / below threshold; surfaces when eligible - surface markers (3): status transitions to SURFACED, surface event logged, surfaced hints don't re-surface - ordering (2): higher confidence first; LIMIT 5 cap - cmd_view integration (2): block appears when enabled; absent when off VERSION → 0.5.98-dev (rebumped from 0.5.93 to avoid matrix collision with bezos #253). Stacks on #250 (phase 2). Completes the three-phase #158 chain. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
Note re: musk's review of #241 — "detection hook position" nit was flagged for this PR. Per the diff:
Position chosen because:
This is implicit in the diff but worth surfacing in the PR description proper — leaving it in this thread comment for now since the PR body lives in git history. |
Phase 2 of #158. Stacks on #249 (phase 1 plumbing). Adds the heuristic detector that feeds
emit_hintfrom the message-send pipeline.Base branch is PR #249 since this consumes the phase-1
emit_hintAPI.What this PR does
lib/hint_detector.py— implements the four signals from the design doc:extract_issue_refs/extract_paths/extract_keywords— pure regex extractionrecency_multiplier— exponential decay, 24h half-lifecompute_signals— combines all three signal weights × recencycompute_confidence— weighted sum, capped at 1.0detect_hints(db, recipient, incoming_text)— DB scan against eligible senders, returns one strongest-prior candidate per senderrun_for_message(db, recipient, text)— post-commit entry point that callsemit_hintfor each candidatelib/board_msg.py—cmd_sendpost-commit hook. Broadcasts (recipient='all') are skipped. Both layers of detection (run_for_messageand per-emit) are broad-except wrapped so a failing detector cannot break message delivery.Scope
Eligibility is sender ↔ recipient only (lead-confirmed): tongxue A is eligible to emit hints to tongxue B only if A has messaged B before. Cross-tongxue sourcing (A surfaces hints based on C's prior thread with B) is deferred per the design doc.
Test plan
pytest tests/test_hint_detector.py— 42/42 pass:TestExtractIssueRefs(5): simple / multiple / PR prefix / no refs / non-digit suffixTestExtractPaths(5): simple / nested / multiple / none / non-path textTestExtractKeywords(4): lowercase + stopword filter / pure stopwords / empty / CJK-coarse v1 documented as known limitationTestRecencyMultiplier(4): now=1 / 24h=0.5 / 48h=0.25 / unparseable fallbackTestComputeSignals(6): each signal in isolation / all-three / no-overlap / recency attenuationTestComputeConfidence(4): empty / single / sum / cappedTestDetectHints(7): no eligible senders / no overlap / issue ref / path overlap / strongest-prior-per-sender / eligibility enforced / lookback respected / multi-senderTestRunForMessage(4): emits viaemit_hint/ no candidates → empty / detector exception swallowed / flaky per-emit isolatedTestSendPipelineIntegration(2):cmd_sendtriggers detection / broadcast skips detectionruff check+ruff format --checkclean.Versioning
VERSION → 0.5.91-dev. Above the active matrix (master 0.5.76-dev + lead suggested 0.5.91+ floor). Bumps further if collisions appear.
Refs #158. Phase 3 (yellow-block surface in
board view) follows.🤖 Generated with Claude Code