Skip to content

feat: add proactive cross-chat issue review - #1372

Open
xboxmasters wants to merge 10 commits into
kenn-io:mainfrom
xboxmasters:codex/complete-issue-review-postgres-gate
Open

feat: add proactive cross-chat issue review#1372
xboxmasters wants to merge 10 commits into
kenn-io:mainfrom
xboxmasters:codex/complete-issue-review-postgres-gate

Conversation

@xboxmasters

@xboxmasters xboxmasters commented Aug 10, 2026

Copy link
Copy Markdown

Adds a local-first Issue Review workflow for recurring failures, blockers, slow tools, repeated work, and remediation opportunities across scoped sessions. It includes deterministic evidence links, complete filters and pagination, cached and forced refresh, telemetry coverage, privacy redaction, and allocation reductions for production-scale archives.

Named browser-local filter views preserve complete Issue Review filter sets, support case-insensitive updates and deletion, reject invalid stored data, and cap retained views at 50.

Acknowledgement and timed or permanent suppression decisions now persist in SQLite and PostgreSQL, survive full archive rebuilds, and overlay the cached detector result on every request. DuckDB remains read-only and active-only; no automatic remediation or external repository, chat, skill, or GitHub mutation is introduced.

@roborev-ci

roborev-ci Bot commented Aug 10, 2026

Copy link
Copy Markdown

roborev: Combined Review (9c3c4a3)

High-severity correctness issues remain in command parsing, with several medium-severity problems affecting deduplication, metrics, timestamps, and backend consistency.

High

  • internal/db/issue_review.go:434 — Shell input extraction recognizes only "command", but Codex exec_command records use "cmd". Commands such as {"cmd":"rg missing"} bypass search detection, so normal rg exit-code 1 results can be misreported as recurring failures.
    • Fix: Decode both command and cmd, preserving the nested command when attributing single-tool exec wrappers.

Medium

  • internal/db/issue_review.go:869 — Tool calls are globally deduplicated by tool name and ToolUseID. Providers such as Roo Code and Kilo generate IDs from the tool name and per-session ordinal, causing legitimate calls from separate chats to collide and preventing accurate failure counts and cross-chat detection.

    • Fix: Include session identity unless source provenance proves that sessions are imported copies.
  • internal/db/issue_review.go:1022 — Repeated polling/read findings reuse the first call’s evidence and duration for every call in a run. This produces incorrect totals, p95 values, wasted-time rankings, and duplicate evidence links.

    • Fix: Iterate through rows[start:end] and use each call’s ordinal, input, status, and duration.
  • internal/db/issue_review.go:707 — Evidence dates and LastSeen always use the session start date rather than each occurrence timestamp. Failures in long-running chats are consequently displayed and sorted as if they occurred when the session began.

    • Fix: Carry occurrence timestamps into evidence, convert them with the selected timezone, and derive LastSeen accordingly.
  • internal/duckdb/issue_review.go:32 — DuckDB applies day-of-week and hour filters, while SQLite and PostgreSQL ignore them for equivalent issue-review requests. API results therefore vary by backend.

    • Fix: Apply equivalent message-time predicates in SQLite and PostgreSQL, or consistently disable these predicates across all backends.

Reviewers: 2 done | Synthesis: codex, 17s | Total: 11m37s

@roborev-ci

roborev-ci Bot commented Aug 10, 2026

Copy link
Copy Markdown

roborev: Combined Review (1a45e5e)

Review identified five medium-severity correctness and performance issues requiring changes before merge.

Medium

  • internal/db/issue_review.go:1805 — Codex telemetry IDs do not match archived session IDs. Telemetry queries use archived IDs such as codex:<uuid>, while Codex logs store the raw thread UUID. No telemetry rows or duration supplements match, despite the response reporting telemetry as available. Query with raw Codex thread IDs and map results back to their full archived session IDs before filtering and tool-call lookup.

  • internal/parser/codex.go:381 — Existing archives will not gain commentary events. Persisting commentary events changes Codex message histories, but dataVersion remains 80. Previously archived sessions will not be reparsed, leaving historical commentary and blocker findings absent after upgrade. Bump the archive data version and document the Codex commentary reparse.

  • internal/db/issue_review.go:869 — Global tool-call deduplication discards genuine calls. Deduplicating by tool name and tool-use ID is unsafe because several parsers generate session-local IDs; for example, Kilo Legacy uses kilo-legacy:<tool>:<ordinal>. Calls from different chats can be treated as imported copies, suppressing cross-chat recurrence findings. Use explicit source-session/import provenance, or include the archived session identity in the key.

  • internal/db/issue_review.go:1024 — Repeated-call evidence and durations use the first call for every row. Polling/read findings repeatedly add the first call’s evidence and duration, causing duplicate links, incorrect total and wasted durations, and potentially duration coverage above 100%. Iterate over rows[start:end] and use each call’s ordinal, index, status, and duration.

  • internal/db/issue_review.go:1657 — Rolling timestamps defeat the issue-review cache. The cache key includes the exact ActiveSince value, while the frontend’s recently-active filter generates a fresh timestamp on every request. Sync-triggered refreshes therefore miss the one-hour cache and rerun expensive archive analysis. Stabilize or bucket the cutoff while preserving the intended cache semantics.


Reviewers: 2 done | Synthesis: codex, 15s | Total: 17m7s

@roborev-ci

roborev-ci Bot commented Aug 10, 2026

Copy link
Copy Markdown

roborev: Combined Review (7892936)

Review found five medium-severity issues affecting historical parsing, PostgreSQL compatibility checks, analysis caching, evidence timestamps, and wasted-duration calculations.

Medium

  • internal/parser/codex.go:381 — Commentary events are now persisted as messages, but the parser data version remains 80. Unchanged historical Codex sessions will not be reparsed, leaving commentary absent from transcripts and Issue Review. Bump dataVersion, document the reparse reason, and update its version assertion.

  • internal/postgres/schema.go:2222CheckSchemaCompat does not probe issue_review_finding_states, although every PostgreSQL Issue Review request reads it. An outdated schema used by a role that cannot migrate can pass startup validation and then fail at runtime. Probe the table and all required columns during compatibility checks.

  • frontend/src/lib/components/insights/IssueReviewPanel.svelte:143, internal/db/issue_review.go:1818 — With “recently active” enabled, each refresh generates a new active_since timestamp that becomes part of the cache key. Hourly and sync-triggered refreshes therefore miss the one-hour analysis cache and repeat an expensive scan. Stabilize or quantize the cutoff, or normalize it when constructing the cache key.

  • internal/db/issue_review.go:759 — Evidence dates and LastSeen always use the session start date rather than message, tool-call, or telemetry timestamps. Findings recurring later in resumed multi-day chats can remain acknowledged and sort as though they occurred on the original date. Derive dates from each occurrence timestamp in the selected timezone, falling back to the session date only when necessary.

  • internal/db/issue_review.go:1069 — Failures, retries, repeated polling, and workflows assign their full duration to WastedDurationMS, although the API and UI define this metric as excess over 30 seconds. Calculate max(duration - 30s, 0) consistently, or expose full failed/repeated duration as a separately labeled metric.


Reviewers: 2 done | Synthesis: codex, 17s | Total: 19m48s

@wesm

wesm commented Aug 19, 2026

Copy link
Copy Markdown
Member

Thanks for your contribution. I'll need a little time to dig into this

@wesm wesm self-assigned this Aug 28, 2026
@wesm

wesm commented Aug 28, 2026

Copy link
Copy Markdown
Member

Thanks for the work here — there is a lot of solid engineering in this PR (the detector is well tested, redaction and read-only handling look careful, and backend parity was clearly taken seriously). I did a close review and found several things that need to be addressed before it can move forward. Posting them up front to see if you'd like to pick them up.

The PR renames the Insights page. nav_insights becomes "Issue Review" in all locales, and existing section labels are rewritten ("Deterministic Recommendations" → "What to fix next", etc.), with test expectations edited to match. That's a product decision that shouldn't ride along in a feature PR. Please revert the renames — the new panel can live inside Insights as-is.

The Codex parser change needs a data-version bump. internal/parser/codex.go now stores commentary events as assistant messages, and the detector's blocker detection depends on them. But dataVersion in internal/db/db.go is still 80, so existing archives never reparse and old sessions silently miss those findings. It needs a bump to 81 with a history note, like every prior parser-output change.

Two detector bugs:

  • Repeated-polling findings multiply the first call's duration by the run length instead of summing each call, and attach N identical copies of the same evidence entry (the run loop around internal/db/issue_review.go:1095). Reproduced: four polls of 40s/10s/10s/10s report 160s total and wasted instead of 70s.
  • dedupeIssueCalls keys on tool + tool_use_id with no session scoping. RooCode, Kilo-legacy, and Poolside synthesize ordinal-based IDs (e.g. roocode:execute_command:3), so unrelated calls from different sessions collide and get merged or dropped. The fix needs care, since cross-session dedupe of imported copies looks intentional.

Personal working files are committed. docs/issue-review-handover.md is a personal release/ops log (machine-specific details, a local scheduled task), and .claude/skills/run-postgres-integration-tests/ is a Windows-specific test flow that conflicts with the documented make test-postgres path. Both should come out, along with the docs/insights.md link to the handover doc.

Some new frontend tests assert source text, not behavior. The "detector contract" block in InsightsPage.test.ts greps the component source for strings instead of exercising it. IssueReviewPanel.test.ts in the same PR shows the right pattern — please rewrite or drop the grep block.

There are smaller items too (a SQLite-vs-Postgres join difference for orphaned tool calls, "At least 1 chats" pluralization, evidence links swallowing cmd/ctrl-click), but the above are the blockers. Happy to go into detail on any of these — let me know if you want to take them, or if you'd prefer I finish it off.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

2 participants