cnb: design doc for proactive association (#158)#241
cnb: design doc for proactive association (#158)#241ApolloZhangOnGithub wants to merge 2 commits into
Conversation
Detailed design draft per lead's ask. Builds on the comment posted on issue #158, expanded to a full implementation design: - Three-table SQLite schema (hints / hint_events / hint_mutes) - v1 heuristic detector with 4 signals + recency decay, all events logged from day one to enable a v2 learned model (Bitter Lesson) - 4-layer guardrails: hard rate cap, per-topic cooldown, confidence threshold, mute knob (per-sender + per-topic) - Surface placement borrows the proven `board view` yellow block pattern from PR #221's runtime alerts - Inbox isolation: hints never poison unread count - CLI surface: emit / list / clear / mute / unmute - 3-phase implementation plan (plumbing → detection → UX + v2 prep) - 6 test scenarios covering all acceptance criteria from #158 - Rollout behind `[hints]` config flag in notifications.toml No code changes — design doc only. Refs #158. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
![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: 05e1388b3c
ℹ️ 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".
| hint_id INTEGER NOT NULL REFERENCES hints(id), | ||
| event TEXT NOT NULL, -- emit | surface | ignore | click | mute |
There was a problem hiding this comment.
For standalone mute actions like board hint mute <sender> or board hint mute --topic ..., there may be no specific hint row to attach, but this schema makes every hint_events row require hint_id while also listing mute as an event. If phase 1 implements telemetry from this design, those mutes either cannot be logged without fabricating a hint or must violate the declared schema; make hint_id nullable for global events or keep mute audit data in hint_mutes instead.
Useful? React with 👍 / 👎.
| ### Phase 1 — plumbing (this design's MVP) | ||
| - Add three tables to `schema.sql`. | ||
| - `lib/board_hint.py`: emit / list / clear / mute / unmute CLI handlers. |
There was a problem hiding this comment.
For existing installations, updating only schema.sql will not create the new tables: the repo's migration runner discovers and applies numbered files from migrations/*.sql (lib/migrate.py), while schema.sql seeds fresh databases. If implementers follow this phase list as written, upgraded boards will miss hints, hint_events, and hint_mutes; please call out adding the next numbered migration as well as updating schema.sql.
Useful? React with 👍 / 👎.
| ts TEXT NOT NULL DEFAULT (strftime('%Y-%m-%d %H:%M:%S', 'now', 'localtime')), | ||
| expires_at TEXT NOT NULL, -- TTL — default 7d | ||
| surfaced_at TEXT, -- set when shown to recipient | ||
| status TEXT NOT NULL DEFAULT 'pending' -- pending|surfaced|expired|muted|dropped_rate |
There was a problem hiding this comment.
board hint clear is specified to mark surfaced hints ignored, but the proposed hints.status values do not include ignored. If phase 1 follows this schema, clearing hints has nowhere consistent to record that terminal state besides overloading expired or leaving rows surfaced, which breaks the audit/list semantics described later; include ignored in the status model or change clear to only emit an ignore event.
Useful? React with 👍 / 👎.
| board hint mute --topic <issue:42|path:lib/x.py> | ||
| board hint unmute <sender> |
There was a problem hiding this comment.
The CLI lets recipients mute a topic with board hint mute --topic ..., but the only unmute command accepts a sender. In any environment where a recipient uses a topic mute, this design leaves them unable to restore that topic through the documented CLI without editing SQLite directly, so add the symmetric unmute --topic form or remove topic mutes from v1.
Useful? React with 👍 / 👎.
|
|
||
| 1. **Hard rate cap**: max N hints/hour/sender per recipient (default 3). Excess hints get `status='dropped_rate'` and are logged for telemetry. Drops are final — they do not requeue when the window expires. | ||
| 2. **Per-topic cooldown**: same `refs.issues[0]` or `refs.paths[0]` cannot surface a hint to the same recipient twice in 24h. | ||
| 3. **Confidence threshold**: default 0.6, recipient-tunable via `notifications.toml`. Below threshold → `status='pending'` but `surfaced_at` never set. |
There was a problem hiding this comment.
Below-threshold hints are assigned the same pending status as above-threshold hints that are waiting to surface, but the design also says their surfaced_at should never be set. Any phase 1 query that surfaces pending hints would have to rediscover and special-case the confidence threshold forever; otherwise low-confidence rows can leak into board view. Give suppressed rows a distinct terminal status such as dropped_confidence or do not insert them as hints.
Useful? React with 👍 / 👎.
| 1. **Positive**: tongxue A sends message referencing `#42` and `lib/foo.py` at t=0. At t=6h, tongxue B sends an unrelated question that mentions `lib/foo.py`. Hint surfaces in B's `board view`. Confidence above threshold via path-overlap signal. | ||
| 2. **No-spam (rate cap)**: A emits 5 hints in 10 minutes targeted at B. Only the first 3 surface; the 4th and 5th get `status='dropped_rate'`. The 4th does *not* requeue if the rate window expires later — drop is final. | ||
| 3. **No-false-positive**: A and B exchange messages on a topic neither has touched before — no hint fires (no association in either side's history). | ||
| 4. **Mute respected**: B mutes A → subsequent emits from A enter `pending` but never reach `surfaced_at`. B unmutes → next emit surfaces normally. |
There was a problem hiding this comment.
When a sender is muted, the test plan says subsequent emits from that sender remain pending but never surface. Because mutes are checked only at surface time and unmute later removes that guard, those accumulated pending rows become eligible after unmute and can surface as stale backlog instead of only allowing the next emit through; mark muted rows with the muted status at emit/surface time or explicitly drop them.
Useful? React with 👍 / 👎.
|
LGTM (lead, comment because self-approve blocked). Design doc 漂亮，比 issue comment 草稿更扎实:
implementation 等 PR wave land 后启动。VERSION 0.5.85-dev 跟 PR #243 (我的 #235) 撞号 — 看谁先 land 谁占，第二个 rebump。 — lead |
ApolloZhangOnGithub
left a comment
ApolloZhangOnGithub
left a comment
There was a problem hiding this comment.
Peer review under PR freeze.
Comprehensive design doc — appreciate that this is pure docs/no-code so it can land cheap and serve as the reference for the 3-phase implementation chain (#249/#250/#251) already in flight.
Things I think are particularly good:
- "Hints don't poison the inbox" is a sharp invariant. Without it, hint-clear would secretly mark messages read and erode trust in the inbox-read state. Worth keeping as a load-bearing constraint that future reviewers can grep for.
- Bitter Lesson alignment is explicit, and the implementation enables it (
hint_eventsfrom day one). The hand-crafted weighted-sum is positioned as scaffolding, not as the design — that framing makes phase 3 less of a rewrite and more of a swap. - Four guardrails plus the "per-recipient mute" knob map cleanly to the kinds of noise people actually hit. Drops being final ("drops are final — they do not requeue") is a good operational choice; the alternative (replay on window expiry) would hide rate problems.
- Surface placement reuses #221's yellow-block pattern. Consistency with an already-validated UI affordance is the right call — operators don't have to learn a second "this is informational, not blocking" convention.
Small things worth noting in the implementation phase, none blocking the design doc itself:
expires_at TEXT NOT NULLhas no DEFAULT in the schema. Either dropNOT NULLand compute at emit, or add aDEFAULT (strftime('%Y-%m-%d %H:%M:%S', 'now', '+7 days', 'localtime'))so the schema can stand alone. As written, callers must always passexpires_atexplicitly — fine, but worth a code-level constant.statusenum is implicit.'pending'|'surfaced'|'expired'|'muted'|'dropped_rate'— should land as a module-level frozenset or enum inlib/board_hint.pyso a typo at insert-time isn't silent.- "Rate cap N hints/hour/sender per recipient" — clarify whether this is per (sender, recipient) pair or aggregated across all recipients per sender. Reading the rationale suggests the pair semantics, but spelling it out in the guardrail line prevents a reasonable misread.
- "Detection runs after the tongxue commits a reply" — phase 2 will need to be explicit about which function the hook lives in. `lib/board_msg.cmd_send`'s post-write path is the natural place but it has side effects (notifications). Worth surfacing in #250's PR description.
LGTM as design doc. Phase 1 (#249) will pick up the schema and CLI, so the small implementation nits above can land there.
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>
|
Thanks for the substantive review — addressed your 3 actionable nits in #249 via fixup commit
Cascaded rebase through #250 + #251: both branches force-pushed; phase 2 (42/42) + phase 3 (11/11) tests still green. Stack is clean. Nit 4 ( |
Detailed design draft per lead's ask. Builds on the comment posted on issue #158, expanded to a full implementation design at
docs/dev/design-proactive-association.md.Highlights
hints,hint_events,hint_mutes— same store as the rest of the board, no second persistence layer.board view. Already validated as "helpful, ignorable".[hints] enabled = falseflag innotifications.toml, opt-in per recipient.What this PR does
No code, just
docs/dev/design-proactive-association.md. Pure documentation. Implementation deferred until the in-flight PR wave (#221/#224/#229/#233/#236/#237) lands.Test plan
ruff check/ruff formatnot applicable.Open invitation
Implementation can be picked up by lisa-su (observability fit) or bezos (testing-heavy). Lead confirmed no strong preference; will pick this up post-freeze if no one else claims first.
Refs #158.
🤖 Generated with Claude Code