Repository navigation
Fix ChatGPT turn detection after DOM role selector change - #55
lekthailtd-bit wants to merge 2 commits into
Conversation
There was a problem hiding this comment.
🤖 AI Code Review — ✅ Approved
Confidence: high · Findings: 3
AI judgment: APPROVE · Evidence: COMPLETE · Authority: advisory
Evidence complete · 6/6 changed files accounted for
Other findings (3)
- [low] New selector test asserts on source text rather than behavior
- [medium] No test validates that the embedded CDP JavaScript is syntactically valid
⚠️ unverified - [low] Single hard-coded selector with no fallback means the next ChatGPT DOM change re-breaks detection
⚠️ unverified
🔮 Devil's Advocate (2 turns): 2 missed risks found · 1 upheld
GitWire AI Review Gate (bundle-driven v2) · Structured schema · Scope-validated · Devil's Advocate · AI recommendation — maintainers and repository policy retain authority
Finding details (not annotatable inline)
-
[MEDIUM] No test validates that the embedded CDP JavaScript is syntactically valid
The source-text test cannot catch a quoting error in the JS embedding of the selectors (e.g., unescaped double quotes in [data-testid="..."] inside a single-quoted JS string, or a malformed attribute selector). Such a bug passes the entire suite and only surfaces at runtime inside Runtime.evaluate — potentially silently if CDP exceptions are swallowed — causing turn detection to hang or never fire, which is the exact production symptom this hotfix addresses. Eval'ing the assembled JS string (or running it through a JS parser) in a unit test would close this gap without requiring a ChatGPT DOM fixture, making it a cheaper and more valuable improvement than the behavioral test suggested in Finding #0.
-
[LOW] Single hard-coded selector with no fallback means the next ChatGPT DOM change re-breaks detection
The PR replaces one exact selector with another, leaving turn detection coupled to a single point of failure in a third-party DOM that has already changed at least once. If the code supported a fallback list of selectors (old + new, or role + data-testid variants), the next upstream change would degrade gracefully instead of silently breaking completion detection. Understandable out of scope for a hotfix, but the review should have noted this as the structural root cause rather than treating the selector swap as a one-off fix.
|
|
||
| def test_current_chatgpt_role_selectors_are_supported(): | ||
| src = inspect.getsource(CDPDriver) + inspect.getsource(CompletionDetector) | ||
| assert "data-content-search-unit-key$=\\\":assistant\\\"" in src |
There was a problem hiding this comment.
[LOW] New selector test asserts on source text rather than behavior
tests/test_current_dom_role_selectors.py uses inspect.getsource() and substring matching to verify the new selectors exist in CDPDriver/CompletionDetector source. This is a valid smoke guard against revert regressions, but it is brittle in both directions: (1) it passes even if the selectors appear in dead code, comments, or malformed JS that never executes, and (2) it will spuriously fail if the JS string quoting/escaping is ever reformatted (e.g., switching to single-quoted attribute selectors), even when behavior is unchanged. Since the selectors are static strings with no runtime injection surface, a behavioral test (e.g., running the compiled JS against a small fixture DOM via the existing CDP test harness/mocks) would pin actual querySelectorAll semantics. Acceptable to merge as-is for a live hotfix, but consider tightening later.
Live regression fix: current ChatGPT DOM no longer exposes data-message-author-role on turn nodes, leaving phase-1 completion detection stuck even after a successful send. This adds compatibility selectors for current data-content-search-unit-key/data-chatgpt-search-unit-key markup while retaining legacy selectors, and aligns anchored projection with the unpaginated conversation-detail endpoint. Focused tests pass; live MCP probe returned normally with backend end_turn recognition and exactly one turn.