Repository navigation
Fix fresh-chat send acknowledgement false negative - #54
lekthailtd-bit wants to merge 1 commit into
Conversation
There was a problem hiding this comment.
🤖 AI Code Review — 💬 Needs discussion
Confidence: low · Findings: 5
AI judgment: NEEDS DISCUSSION · Evidence: COMPLETE · Authority: advisory
Evidence complete · 2/2 changed files accounted for
Other findings (5)
- [low] URL validation is looser than a canonical UUID check
- [medium] No regression test for the caller-side fresh_chat wiring
- [medium] fresh_chat flag may be silently always-False if fallback_anchor.mode is an Enum or differently-cased string
⚠️ unverified - [medium] No test pinning fresh_chat=False behavior of _verify_send_acknowledged (non-fresh chats)
⚠️ unverified - [medium] Unhandled exceptions in the new fresh-chat verification path could turn a false negative into a hard send failure
⚠️ unverified
🔮 Devil's Advocate (2 turns): 3 missed risks found · 2 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] fresh_chat flag may be silently always-False if fallback_anchor.mode is an Enum or differently-cased string
The fix hinges on the raw comparison
fallback_anchor.mode == "fresh_chat". If mode is an Enum member (mode == "fresh_chat" is always False), a camelCase variant, or set via a .value elsewhere, the kwarg never activates and the 'fixed' false negative persists invisibly — worse than before, because the code now looks corrected. The review validated the helper's logic but did not verify the type and literal value of mode at the anchor construction site. A one-line assertion or isinstance/value check would settle it. -
[MEDIUM] No test pinning fresh_chat=False behavior of _verify_send_acknowledged (non-fresh chats)
The three new tests target the helper directly, and per the review all exercise the fresh-chat path. The fresh_chat parameter presumably gates new logic; without a test asserting that fresh_chat=False leaves the legacy acknowledgement behavior byte-for-byte unchanged, a future edit to the helper could alter behavior for ordinary (non-fresh) chats — the far larger traffic share — with no failing test. The review flagged the wiring gap but missed the negative-path gap.
-
[MEDIUM] Unhandled exceptions in the new fresh-chat verification path could turn a false negative into a hard send failure
A brand-new conversation's DOM (ack element, conversation URL) may not exist yet when verification runs; fresh-chat checks are therefore inherently prone to timeouts/element-not-found. If _verify_send_acknowledged (or the fresh branch in send_and_stream) raises and the exception isn't contained, a send that previously succeeded — just without ack confirmation — now fails outright for users on slow connections. The review evaluated the false-negative logic but never asked whether the new path can throw and who catches it. Worst case this is high severity; it should be verified before merge.
| return True | ||
| if fresh_chat and state.get("composerEmpty"): | ||
| # Fresh-chat /c/{uuid} navigation can precede user DOM hydration. | ||
| import re as _re |
There was a problem hiding this comment.
[LOW] URL validation is looser than a canonical UUID check
The regex [0-9a-fA-F-]{36} accepts any 36 characters of hex digits and hyphens, and uuid.UUID() strips hyphens before validating, so it accepts non-canonical hyphen placement too. For example, /c/0123456789abcdef0123456789abcdef-abc (36 chars, 32 hex digits) passes both checks despite not being a canonical UUID form. Practical impact is minimal since real ChatGPT conversation URLs are canonical 8-4-4-4-12, but if the intent is 'valid /c/{uuid}', a canonical regex such as [0-9a-fA-F]{8}-[0-9a-fA-F]{4}-[0-9a-fA-F]{4}-[0-9a-fA-F]{4}-[0-9a-fA-F]{12} would make the gate exact and let you drop the redundant uuid.UUID round-trip.
| try: | ||
| acknowledged = await self._verify_send_acknowledged() | ||
| acknowledged = await self._verify_send_acknowledged( | ||
| fresh_chat=(fallback_anchor.mode == "fresh_chat"), |
There was a problem hiding this comment.
[MEDIUM] No regression test for the caller-side fresh_chat wiring
The crux of the fix in production is the call site fresh_chat=(fallback_anchor.mode == "fresh_chat") in send_and_stream, but all three new tests invoke _verify_send_acknowledged directly. If the kwarg stops being forwarded (e.g., a refactor renames the mode string or drops the argument), the original false negative silently returns with no failing test. Consider a test that exercises the send_and_stream path (or asserts kwarg propagation with a stubbed anchor) so the integration point is pinned, not just the helper's logic.
|
Live validation failed: chat_completion still timed out after an accepted fresh-project send, and the runtime log showed the same probe submission path invoked twice. Deployment was rolled back to 497527d. Closing rather than presenting this as a verified fix. |
Fresh project chats can navigate to /c/{uuid} before the new user-message DOM node hydrates. Treat a valid fresh-chat /c/{uuid} transition plus an empty composer as positive send acknowledgement, while preserving existing-conversation semantics and automatic-resend prohibition. Adds focused regression tests. Focused suite: 7 passed. pip check: clean.