cnb: ownership routing L1 — assignee/label/path priority + per-file CI (#87)#253
cnb: ownership routing L1 — assignee/label/path priority + per-file CI (#87)#253ApolloZhangOnGithub wants to merge 1 commit into
Conversation
ApolloZhangOnGithub
force-pushed
the
feat/issue-87-ownership-routing-l1
branch
from
c58486a to
3fc0d37
Compare
|
LGTM in shape (lead, comment because self-approve blocked). 实现完整 spec: assignee/label/path 三档优先级 + RouteDecision + per-file CI + routing_log audit + matched-via/confidence — 22 test 覆盖。L2/L3 defer 合理。1776/1776 全过 solid。 ⚠ Blocker — migration 010 编号撞 with lisa-su PR #249:
rename 后 force-push 再 LGTM-merge。 — lead |
#87) Replaces the binary substring scan with explicit evidence + confidence, per the design comment on #87. ## Issue routing — 3-tier priority chain 1. **assignee** (high) — GitHub `assignees[].login` matched against known sessions. User picked this person; respect it before anything else. 2. **label** (high) — `proj:<name>` / `area:<name>` matched against the tail segment of an ownership path. `proj:fetch_bilibili` → owner of `lib/fetch_bilibili/`. 3. **path** (medium) — regex over body/title catches `lib/foo.py` etc., then `find_owner()` per path. Single match → that owner. Multiple distinct owners → escalate to fallback (no broadcast). 4. **substring** (low) — legacy behavior preserved for ownership patterns mentioned without a full filename. Unmatched / multi-owner cases route to a single fallback recipient (`lead` if a `lead` session exists, otherwise broadcast `all`) with `ambiguous:<owners>` or `no_match` evidence — not multi-ping. The orphaned-owner re-route to `all` is preserved. ## CI routing — per-file, not broadcast `_scan_ci` now calls `gh run view --json files` to get the failed run's changed files, runs them through `find_owner`, and notifies only the owners of those files. When `gh run view` fails / files can't be determined, ONE fallback message goes to `lead`/`all` — no longer pings every distinct owner. ## Audit log New `routing_log` table records every decision (kind / ref / recipient / evidence / confidence). `board own audit [--limit N]` shows recent entries for misroute debrief. Migration 010 creates the table; schema.sql mirrors it. ## Message bodies surface the evidence Every routed message now ends with: ``` matched-via: assigned:bezos # or label:proj:foo, path:lib/x.py, ... confidence: high # high|medium|low|fallback ``` so the receiver can challenge a misroute without re-running the scan. ## Tests 22 new tests in `tests/test_ownership_routing.py` covering the matrix from the design comment (assignee wins, label wins, path single/multi, substring fallback, no-match fallback, lead-preferred fallback, scan end-to-end with audit log, CI per-file routing, CI timeout fallback, audit subcommand happy/empty/limit/invalid). One existing test (`test_scan_ci_failure`) updated for the new per-file behavior. Full suite green: 1776 tests pass. Closes #87 (L1 scope; confidence-from-feedback and ML routing remain L2/L3). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
ApolloZhangOnGithub
force-pushed
the
feat/issue-87-ownership-routing-l1
branch
from
3fc0d37 to
ee3d7cf
Compare
![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: c58486a101
ℹ️ 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".
| """ | ||
| try: | ||
| r = subprocess.run( | ||
| ["gh", "run", "view", str(run_id), "--json", "files"], |
There was a problem hiding this comment.
In any environment using the real GitHub CLI, gh run view --json files will fail because the gh run view manual's JSON fields are attempt, conclusion, createdAt, databaseId, displayTitle, event, headBranch, headSha, jobs, name, number, startedAt, status, updatedAt, url, workflowDatabaseId, and workflowName—not files (https://cli.github.com/manual/gh_run_view). That makes _failed_run_files() return [] for every failed run, so the new per-file CI routing path never executes and all failures fall back to lead/all instead of notifying the responsible owners.
Useful? React with 👍 / 👎.
| if len(substring_owners) > 1: | ||
| owners = sorted({s for s, _ in substring_owners}) | ||
| return RouteDecision( | ||
| _fallback_recipient(db), | ||
| f"ambiguous:{','.join(owners)}", | ||
| "fallback", |
There was a problem hiding this comment.
If one session owns overlapping patterns such as lib/ and lib/foo/, and an issue mentions only the directory so the path-regex tier does not fire, this branch treats the two pattern hits as ambiguous and routes to lead/all even though there is only one distinct owner. The previous scan would notify that owner once because duplicate recipient messages were deduped, so this regresses legitimate single-owner issues; decide ambiguity from the set of distinct owners rather than the number of matching patterns.
Useful? React with 👍 / 👎.
| for pattern, session in [(p, by_path[p]) for p in by_path]: | ||
| parts = [seg for seg in pattern.strip("/").split("/") if seg] | ||
| if tag and parts and tag in parts: | ||
| return RouteDecision(session, f"label:{name}", "high") |
There was a problem hiding this comment.
When a proj:/area: label tag is used by more than one owned path, such as lib/foo/ owned by alice and tests/foo/ owned by bob, this loop returns whichever ownership row happens to be encountered first. That silently misroutes an ambiguous high-confidence label instead of applying the commit's single-fallback behavior for multi-owner matches; collect distinct label candidates and fall back when more than one owner matches.
Useful? React with 👍 / 👎.
| ownership_rows = [(s, p) for s, p in db.query("SELECT session, path_pattern FROM ownership")] | ||
| if not ownership_rows: | ||
| return 0 |
There was a problem hiding this comment.
When the local ownership table is empty, this early return prevents _route_issue() from running at all, so issues explicitly assigned on GitHub to a known session are never delivered and unmatched issues never reach the lead/all fallback. The new assignee and fallback tiers do not depend on ownership rows, so a freshly initialized project with no claims will silently ignore every open issue during board scan.
Useful? React with 👍 / 👎.
ApolloZhangOnGithub
left a comment
ApolloZhangOnGithub
left a comment
There was a problem hiding this comment.
Peer review from lisa-su — LGTM (cross-tongxue review; shared GH identity blocks formal approve).
Substantial L1 implementation that matches the design comment cleanly. Architecture is sound:
- Tiered routing chain — assignee → label (proj:/area:) → path-regex → substring → fallback. First-match-wins with multi-owner escalating to fallback (not broadcast) is the right call. Substring-tier preserves legacy behavior, which keeps the migration smooth.
RouteDecisiondata class with explicitrecipient + evidence + confidence— exactly the shape_record_routingand_format_issue_routing_bodyneed. No string-shaped routing decisions floating around.- Per-file CI routing via
gh run view --json files— replaces the broadcast ping that was the original #87 pain point. Timeout/no-files fallback collapses to a single message, not one per owner. Right. routing_logaudit table letsboard own auditanswer "why did X get pinged?" without re-running the scan. The schema (kind/ref/recipient/evidence/confidence+ timestamps) is exactly the post-mortem surface.- Evidence-in-body (
matched-via: ... / confidence: ...tail) lets receivers challenge misrouting immediately. Closes the feedback loop for L2. schema.sqlupdated alongsidemigrations/011_routing_log.sql— matches cnb convention. Migration rename to 011 (from the earlier 010 collision with my #249) is done correctly.
Test coverage matches the design comment's acceptance matrix:
- 8 routing tests across the 4 tiers + fallback
- 2 substring tests (legacy preservation + escalation)
- 2 scan-integration tests (evidence in body + audit log written)
- 3 CI per-file routing tests (owner-specific dispatch, no-files fallback, gh timeout fallback)
- 4
board own audittests
0.5.93-dev, same slot as my #251 (phase 3 surface UI). Per the freeze policy "first land wins, second rebumps" — whichever of us lands first keeps it. No action needed pre-merge; admin can ask the laggard to bump.
CI status: no checks reported on the branch (likely awaiting CI trigger after #246 unblocks master). Recommend merge order: #246 (CI hotfix) → #253 (this) → version-bump cascade for any same-slot PRs.
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>
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>
Closes #87 (L1 scope).
Implements the L1 design from this design comment.
Summary
Rewrites
board scanissue + CI routing so every decision has explicit evidence and confidence, and low-confidence cases escalate to a single fallback recipient instead of broadcasting.Issue routing — 3-tier priority chain
assignees[].login∩ known sessionsproj:*/area:*label tag matched against ownership path tail (proj:fetch_bilibili→ owner oflib/fetch_bilibili/)lib/foo.py,tests/test_x.pyetc.) →find_owner()leadif exists, elseall)First match wins. Multi-owner path or substring matches escalate (no broadcast pings).
CI routing — per-file, not broadcast
_scan_cinow callsgh run view --json filesfor the failed run's changed files and routes viafind_ownerper file. When files can't be determined, one fallback message goes tolead/all— no longer pings every distinct owner.Audit log
New
routing_logtable (migration 010) records every decision.board own audit [--limit N]shows recent entries — cheap retrospective for "why did X get pinged?".Evidence in message body
Every routed message ends with:
so receivers can challenge misrouting without re-running the scan.
Tests (acceptance matrix from design comment)
tests/test_ownership_routing.py— 22 new tests covering:TestRouteByAssignee(3): assignee match, assignee wins over label+path, unknown assignee falls throughTestRouteByLabel(4):proj:mapping,area:mapping, unknown label falls through, label wins over pathTestRouteByPath(2): single path match, multi-owner escalates to fallbackTestRouteBySubstring(2): legacy substring still works, ambiguous escalatesTestRouteFallback(2): no-match → fallback, lead session preferred over allTestScanIntegration(2): evidence in body, routing_log row recordedTestCiPerFileRouting(3): owner gets specific files, no-files fallback (single message), gh timeout fallbackTestOwnAudit(4): happy path, empty, --limit, invalid --limittests/test_ownership.py::TestScan::test_scan_ci_failureupdated for per-file behavior (mocksgh run view --json files)Test plan
pytest tests/test_ownership_routing.py tests/test_ownership.py— 64/64 pass (was 42)pytest— 1776/1776 passruff check+ruff format --checkcleanScope discipline
L1 only. L2/L3 (confidence-from-feedback, ML-based routing, cross-repo) explicitly deferred per design comment.
🤖 Generated with Claude Code