Two focused structured-output reviews using ask. See: ask code review pattern
Source files: /tmp/review2_imap.json, /tmp/review2_prompt_eng.json
What changed from round 1
- Used structured JSON output (--schema-file) — findings as {severity, category, location, title, description, suggestion}
- Two targeted questions instead of one broad — IMAP robustness and LLM prompt quality separately
- Smaller file set per run — no full 2000-line source, just diff + description + relevant file
- Result: tighter, more actionable findings. 11 IMAP + 8 prompt findings, all categorised and located
IMAP / Production findings (fixed)
- F1 CRITICAL: Folder selection side-effects — fetch_thread_chain left connection in arbitrary folder. Fixed: track client.current_folder in IMAPClient.select_folder(), restore at end of fetch_thread_chain in finally-style block.
- F4 MAJOR: _fetch_headers_only assumed data[0] is always the response tuple. Fixed: iterate data items for (bytes, bytes) tuple.
- F7 MINOR: Bundle note scan uncapped — large notes could add hundreds of Message-IDs. Fixed: cap at max_ancestors + 10 extra.
IMAP / Production findings (deferred)
- F2 MAJOR: Connection drops silently produce 'not found' — need to distinguish IMAP errors from genuine missing messages. Deferred — requires retry architecture.
- F3 MAJOR: O(N*M) folder scanning — 15 ancestors × N folders. Deferred — performance not yet a problem.
- F5 MAJOR: imap_id is a sequence number, not UID — can become stale if mailbox mutates between header fetch and body upgrade. Deferred — requires UID-based ops throughout IMAPClient.
- F6 MAJOR: Thread pool built once before iteration loop — stale if messages expunged during processing. Deferred.
- F8 MAJOR: IMAP IDLE readline blocks on half-open TCP. Pre-existing, deferred.
- F9 MAJOR: COPY+STORE not atomic — RFC 6851 MOVE preferred. Pre-existing, deferred.
Prompt engineering findings (fixed)
- F2 MAJOR: 'Thread parent' label was wrong — position #2 may not be the In-Reply-To. Fixed: detect direct parent via In-Reply-To match; label as 'Thread message (direct parent)'. Other ancestors labelled 'Thread message'.
- F3 MINOR: Quick-ID reminder buried in system message. Fixed: added reminder near iteration block close to where IDs appear.
- F4 MINOR: Headers-only text didn't explain upgrade-in-place. Fixed: now says 'request add_emails: ["#N"] to upgrade this email to full body. The Quick-ID #N will remain unchanged.'
- F5 MINOR: '#NotAvailable' looked like a Quick-ID token. Fixed: removed, now just '(not found in mailbox)' style wording.
- F8 MINOR: In-Reply-To header could show parent Quick-ID for pool entries. Fixed: when parent is in pool, show 'In-Reply-To: <mid> (Quick-ID #N)'.
Positive feedback
- 〶 section markers called 'strong structural improvement' — token-distinct, not repeated chars
- Quick-ID + headers-only pool: 'excellent context compression strategy'
- Action failure feedback loop: 'strong production-safety improvement'