Repository navigation
feat(logs): accept Codex thread links in conversation filters - #6734
Merged
Merged
Conversation
Codex's copy-session-link clipboard carries a codex://threads/<id> deep link while clients send the bare thread id on the wire. Unwrap the wrapper inside the shared conversation matcher so the Logs conversation field, /api/logs?conversationId, and ocx logs filter --conversation all match the bare id (direct or via its persisted digest) without changing what is persisted. Co-Authored-By: Epinephrine <luvs01@hanmail.net> (cherry picked from commit 19a0f35)
Review feedback on #711: codex://threads/<id>?hostId=… pastes kept the query string in the candidate id so neither raw-id nor digest matching worked, and the lazy-wildcard regex had quadratic backtracking on malformed slash-flooded inputs run per log row. unwrapLogConversationQuery is now a bounded (512-char) linear scan: cut at ? or #, strip trailing slashes, and only return a single-segment id. Regression cases cover durable and percent-encoded remote-control hostId values across the dashboard matcher, GET /api/logs conversationId, and ocx logs filter --conversation. Co-Authored-By: Epinephrine <luvs01@hanmail.net> (cherry picked from commit 6d8225f)
Devin Review on #711: a client that literally sends a codex://threads/… session id has it hashed whole for storage, but unwrapping the paste before hashing meant that digest could never match again — the unwrap must add a match path, not replace one. logConversationQueryCandidates now yields the unwrapped id AND the untouched paste; the server matcher tries each (direct + digest) and the GUI hashes every candidate so filterLogs gets both digests. Co-Authored-By: Epinephrine <luvs01@hanmail.net> (cherry picked from commit 0df8335)
React Doctor flagged the sequential await-in-loop in hashLogConversationQuery; candidates are independent, so map them through crypto.subtle.digest concurrently and keep result order. Co-Authored-By: Devin AI <devin-ai-integration[bot]@users.noreply.github.com> (cherry picked from commit a600bf2)
Record the conversation filter contract for the work from #711, including literal URI compatibility and input bounds. The four original commits remain separately cherry-picked with their author identities and source SHA trailers. Co-authored-by: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com> Co-authored-by: Epinephrine <27862058+luvs01@users.noreply.github.com>
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configuration
📒 Files selected for processing (10)
Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 9 remain after this review. 📝 WalkthroughWalkthroughConversation filters in the server and browser now accept Codex thread links. They match using the extracted thread ID and the trimmed original query. The browser stores hashes for multiple query candidates. Documentation and tests cover the updated behavior. ChangesConversation filter thread links
Priority: ⬇️ Low Estimated code review effort: 3 (Moderate) | ~20 minutes Change: Feature Suggested reviewers: Merge Risk: ⚪ Minimal · up to Pasted Codex thread links now filter conversations in the dashboard, the API, and the CLI. I found no concrete merge-blocking risk in the supplied changes. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 70.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 10 functions across 8 files. (2 skipped: 2 unsupported.)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Contributor
|
✅ Deterministic PR hygiene checks passed. |
luvs01
marked this pull request as ready for review
October 8, 2026 21:21
|
The account paying for this security review has reached its Codex usage limits. The payer can check the Codex usage dashboard. For personal accounts, using credits requires enabling “Use credits for security reviews” in Code review settings. If you do not manage the paying account, contact this repository's admins. |
Owner
|
Maintainer integration record (MAINTAINERS.md,
Thanks @luvs01 for the contribution. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
codex://threads/<id>links in the dashboard Conversation filter,GET /api/logs?conversationId=(and itsconversationalias), andocx logs filter --conversation. Query/fragment metadata, trailing slashes, surrounding whitespace and prefix case are handled while preserving thread-id case. The original trimmed query remains a second candidate, so bare ids and whole-literal-URI digests stay searchable. Malformed or trimmed links over 512 characters remain ordinary queries, subject to the existing 4096-character digest-input bound. Persistence is unchanged.bf9ecf3d79c7b1b6436e2922f52d421ab550b744, including its separate layout-ratchet fix and runtime/test-runner Bun pins. The feature diff against dev remains 10 files, 235 additions / 22 deletions. No timeout, skip, file-size cap or exemption workaround was added.Source commits:
19a0f3545d74bf6b95e677e95412bd915f3c7412,6d8225fc92bccdd85140f3283120f8f5f520d4c4,0df83357939097d6c72853a29c8160476695cb74,a600bf259bff2d0fa916db18a509d5fdbe729ecd.Verification
Current published head:
ca2391373f10795bf9270ba388344bec2ec2301f, tree7181068612242434cb71182f8f372be189be7348, parents published0555d0cband upstream devbf9ecf3d. Linux used the repository's Bun 1.4.0 test runner, Bun 1.4.2 runtime, and Node 24.19.0. Dependencies were reused from a successful frozen install with byte-identical current manifests/locks; no dependency files changed. Current upstream dev ise6c6e13a22e465812f509a423990e9f09e32b1d0, 10 commits ahead of the integrated base. This meets the repository readiness gate's documented maximum of 10; the newest 400-file upstream union has not been locally integrated or validated in this PR.cd gui && bun run test --isolate: 3,005 pass, 332 files.bun run test:changed --timeout=60000: 13,945 pass / 37 skip / 95 fail / 0 unhandled errors, 14,077 tests across 602 files, 348.42s, exit 1. This is not a full-suite pass.bf9ecf3d: 766 pass / 94 fail, the same 860 tests, 110.01s, exit 1. All 93 currently failing PR cases also fail on that exact base; zero candidate-only failing names. Both runs remain red.responses-compaction-routingnative-combo seed case passed on both in the new comparison, and previously failed on clean historical basebbab02ab. Its intermittent seed/timing cause is unresolved; it is not claimed fixed. A plaintext headerless drip-feed timing case failed only on the base in this pair and had failed in the original broad PR run. Timing risks remain./tmpownership refusals appear directly in several failure blocks; not every secondary assertion's cause is established. No guard, ownership, timeout or skip bypass was applied.Local validation scope exception: the full
bun run testsuite is the default before review readiness. It was not repeated here because several prior roughly 600-file runs already took 345–348 seconds each and showed recurring ownership and timing failures on clean baselines in this managed environment. Instead, all changed-behavior regressions, direct GUI/API/CLI boundary checks, the complete GUI suite, strict typecheck, privacy/structure/lint/build gates, and the uncertain 18-file candidate/base comparison were run. This satisfies the documented focused-regression minimum under the resource exception, with broader red results and untested coverage retained above. It does not attest that the complete local repository suite passes.The exact paired command was
bun scripts/test.ts --timeout=60000followed by these same 18 paths on each worktree (candidateca2391373f10795bf9270ba388344bec2ec2301f; clean basebf9ecf3d79c7b1b6436e2922f52d421ab550b744):Historical exact 0555d0c checks: Cross-platform CI 37716613109 finished 14 successful jobs / 3 failed / 8 skipped. Shard 2 batch 4/56 failed the unchanged 2,000-line layout ratchet; shard 1 batch 4/52 hit the 120-second multi-file timeout, and final
ciaggregate failed. The workflow's automatic single-file attribution passed all 12 timeout-batch files, but the shard stopped early. These are historical results, not current-head validation. React Doctor 37716612981 passed on 0555.Timeout analysis localizes the historical stall after the first two GUI typecheck-gate assertions, before its synchronous child-process case. On unchanged 0555 files, the same 12-file batch passed 244 tests locally under unchanged 120-second process / 60-second test deadlines; a randomized seed 2 run also passed with cold-spawn warmup preceding the GUI gate. No proven shared-state/handle-release bug justified a speculative source rewrite. The current-head hosted run below completed successfully without changing those deadlines. This single successful run does not establish the historical timeout's root cause or eliminate timing-dependent risk.
Current-head Cross-platform CI 37725885408 completed successfully: 17 successful jobs / 0 failed / 8 skipped. All four test shards and the final
ciaggregate passed onca2391373f10795bf9270ba388344bec2ec2301f. Structure, gates, docs, API usage, storage policy, Docker and selected npm/keyring smokes passed. Current-head React Doctor 37725885435, hygiene and target checks also passed. Native Windows/macOS suites were skipped by path scope; those skips are not passing native test evidence. CodeRabbit subsequently reviewed all 10 files against this exact head (review markerca2391373f10795bf9270ba388344bec2ec2301f) and reported no actionable comments. The live review-thread query returned zero threads. Its generic docstring-coverage warning remains 70% against an 80% threshold; this is disclosed as an advisory warning, not a passed check or a fixed finding.The complete backend repository suite and native Windows/macOS behavior were not run locally. No live provider validation or paid provider/API calls were performed. Author minimum local validation is complete under the documented resource exception. Human review is requested for this published head with the red local suites, skipped native coverage, intermittent seed/timing uncertainty and docstring warning explicitly retained. This is a review-readiness claim, not a full-local-suite or merge-approval claim. No PR merge or force-push was performed.
Checklist
Summary by CodeRabbit
codex://threads/<id>links. Links with query parameters or fragments are also supported, and searches can match the extracted thread ID or the original link.