Skip to content

Instantly share code, notes, and snippets.

@eric239

eric239/pr-4870-review.html

Created April 22, 2026 01:05
Show Gist options

  • Select an option

    Learn more about clone URLs
  • Save eric239/884cf43c4eef0f088b71908dd87ea9f8 to your computer and use it in GitHub Desktop.

Select an option

Learn more about clone URLs
Save eric239/884cf43c4eef0f088b71908dd87ea9f8 to your computer and use it in GitHub Desktop.
Download ZIP
PR #4870 agent review report
Raw
pr-4870-review.html
<!DOCTYPE html>
<html lang="en">
<head>
<meta charset="UTF-8">
<title>PR #4870 Review - shared-dispatch facade + SLA reset move</title>
<style>
:root {
--bg: #0d1117;
--surface: #161b22;
--border: #30363d;
--text: #e6edf3;
--text-muted: #8b949e;
--accent: #58a6ff;
--green: #3fb950;
--red: #f85149;
--green-bg: #12261e;
--red-bg: #2d1214;
--yellow: #d29922;
--purple: #bc8cff;
--orange: #f0883e;
--syn-keyword: #ff7b72;
--syn-string: #a5d6ff;
--syn-decorator: #d2a8ff;
--syn-func: #d2a8ff;
--syn-class: #ffa657;
--syn-builtin: #ffa657;
--syn-comment: #8b949e;
--syn-self: #ff7b72;
--syn-number: #79c0ff;
--syn-type: #ffa657;
}
* { box-sizing: border-box; }
body {
background: var(--bg);
color: var(--text);
font-family: -apple-system, BlinkMacSystemFont, 'Segoe UI', Helvetica, Arial, sans-serif;
line-height: 1.6;
margin: 0;
padding: 24px 32px;
max-width: 1400px;
margin-left: auto;
margin-right: auto;
}
h1 { font-size: 1.5rem; margin: 0 0 6px; }
h2 { font-size: 1.15rem; margin: 0 0 8px; color: var(--text); }
h3 { font-size: 0.95rem; margin: 16px 0 6px; color: var(--accent); }
.header {
background: var(--surface);
border: 1px solid var(--border);
border-radius: 8px;
padding: 20px 24px;
margin-bottom: 16px;
}
.header-meta { color: var(--text-muted); font-size: 0.85rem; }
.header-meta .num { color: var(--accent); font-weight: 600; }
.header-meta .add { color: var(--green); }
.header-meta .del { color: var(--red); }
.tldr {
background: var(--surface);
border: 1px solid var(--border);
border-left: 3px solid var(--accent);
border-radius: 6px;
padding: 14px 18px;
margin-bottom: 16px;
font-size: 0.92rem;
}
.tldr-note {
color: var(--text-muted);
font-style: italic;
font-size: 0.82rem;
margin-top: 8px;
}
.block {
background: var(--surface);
border: 1px solid var(--border);
border-radius: 8px;
margin-bottom: 16px;
overflow: hidden;
}
.block-header {
padding: 14px 20px;
border-bottom: 1px solid var(--border);
display: flex;
align-items: center;
gap: 12px;
}
.block-number {
background: var(--accent);
color: #0d1117;
width: 28px;
height: 28px;
border-radius: 50%;
display: flex;
align-items: center;
justify-content: center;
font-weight: 700;
font-size: 0.85rem;
flex-shrink: 0;
}
.block-row {
display: flex;
gap: 0;
}
.block-files {
background: #0d1117;
width: 260px;
flex-shrink: 0;
padding: 14px 16px;
font-size: 0.75rem;
border-right: 1px solid var(--border);
}
.block-files ul {
list-style: none;
padding: 0;
margin: 0;
}
.block-files li {
padding: 3px 0;
color: var(--text-muted);
word-break: break-all;
font-family: ui-monospace, SFMono-Regular, monospace;
display: flex;
align-items: flex-start;
gap: 6px;
}
.block-files .dot {
display: inline-block;
width: 6px;
height: 6px;
border-radius: 50%;
margin-top: 6px;
flex-shrink: 0;
}
.block-files .dot-add { background: var(--green); }
.block-files .dot-mod { background: var(--yellow); }
.block-content {
padding: 14px 20px;
flex: 1;
font-size: 0.9rem;
}
.block-content p { margin: 0 0 10px; }
.block-content strong { color: var(--text); }
.concerns {
padding: 10px 20px 16px;
border-top: 1px solid var(--border);
}
.concern {
background: rgba(210, 153, 34, 0.08);
border: 1px solid rgba(210, 153, 34, 0.35);
border-radius: 6px;
padding: 10px 14px;
margin-bottom: 8px;
font-size: 0.88rem;
}
.concern.ok {
background: rgba(63, 185, 80, 0.08);
border-color: rgba(63, 185, 80, 0.35);
}
.concern strong { color: var(--yellow); }
.concern.ok strong { color: var(--green); }
.concern code {
background: rgba(255,255,255,0.08);
padding: 1px 5px;
border-radius: 3px;
font-family: ui-monospace, SFMono-Regular, monospace;
font-size: 0.82rem;
}
.block-content code, .concern code { color: var(--syn-string); }
pre {
background: #0b0f14;
border: 1px solid var(--border);
border-radius: 6px;
padding: 10px 14px;
margin: 10px 20px;
overflow-x: auto;
font-size: 0;
line-height: 0;
font-family: ui-monospace, SFMono-Regular, monospace;
}
.diff-line {
display: block;
font-size: 0.72rem;
line-height: 1.45;
padding: 0 4px;
white-space: pre;
}
.diff-line.add { background: var(--green-bg); color: var(--green); }
.diff-line.remove { background: var(--red-bg); color: var(--red); }
.diff-line.context { color: var(--text-muted); }
.diff-line.ellipsis { color: var(--text-muted); text-align: center; font-style: italic; }
.diff-line .kw { color: var(--syn-keyword); }
.diff-line .str { color: var(--syn-string); }
.diff-line .dec { color: var(--syn-decorator); }
.diff-line .fn { color: var(--syn-func); }
.diff-line .cls { color: var(--syn-class); }
.diff-line .ty { color: var(--syn-type); }
.diff-line .bi { color: var(--syn-builtin); }
.diff-line .slf { color: var(--syn-self); font-style: italic; }
.diff-line .cmt { color: var(--syn-comment); font-style: italic; }
.diff-line .num {
color: var(--syn-number);
background: none;
width: auto;
height: auto;
border-radius: 0;
display: inline;
font-weight: normal;
font-size: inherit;
}
table.action-items {
width: 100%;
border-collapse: collapse;
margin-top: 12px;
font-size: 0.88rem;
}
table.action-items th, table.action-items td {
padding: 10px 14px;
border: 1px solid var(--border);
text-align: left;
vertical-align: top;
}
table.action-items th {
background: #1f2730;
color: var(--text);
font-weight: 600;
}
table.action-items td:first-child { width: 36px; text-align: center; color: var(--text-muted); }
table.action-items td:nth-child(2) { width: 120px; }
.severity {
display: inline-block;
padding: 2px 8px;
border-radius: 4px;
font-size: 0.75rem;
font-weight: 600;
text-transform: uppercase;
letter-spacing: 0.04em;
}
.severity.bug { background: rgba(248, 81, 73, 0.2); color: var(--red); border: 1px solid rgba(248, 81, 73, 0.5); }
.severity.design { background: rgba(240, 136, 62, 0.2); color: var(--orange); border: 1px solid rgba(240, 136, 62, 0.5); }
.severity.minor { background: rgba(210, 153, 34, 0.2); color: var(--yellow); border: 1px solid rgba(210, 153, 34, 0.5); }
.severity.question { background: rgba(188, 140, 255, 0.2); color: var(--purple); border: 1px solid rgba(188, 140, 255, 0.5); }
.severity.nit { background: rgba(139, 148, 158, 0.15); color: var(--text-muted); border: 1px solid rgba(139, 148, 158, 0.4); }
.severity.expected { background: rgba(88, 166, 255, 0.15); color: var(--accent); border: 1px solid rgba(88, 166, 255, 0.4); }
.tag {
display: inline-block;
font-size: 0.7rem;
color: var(--text-muted);
background: rgba(255,255,255,0.06);
padding: 1px 6px;
border-radius: 3px;
margin-left: 4px;
font-family: ui-monospace, SFMono-Regular, monospace;
}
.summary-block {
background: var(--surface);
border: 1px solid var(--border);
border-radius: 8px;
padding: 16px 20px;
}
</style>
</head>
<body>
<div class="header">
<h1>PR <span class="header-meta" style="color:var(--accent)">#4870</span> - shared-dispatch: facade for orchestrator runs; SLA reset in trigger processor</h1>
<div class="header-meta">
<strong>Author:</strong> eric239 &nbsp;|&nbsp;
<strong>Branch:</strong> tech-6345-extract-aio-dispatch-and-make-incoming-email-ingestion-prod &nbsp;|&nbsp;
<strong>Files:</strong> 19 &nbsp;|&nbsp;
<span class="add">+517</span> / <span class="del">-313</span> &nbsp;|&nbsp;
<strong>Ticket:</strong> TECH-6345
</div>
</div>
<div class="tldr">
<strong>TL;DR.</strong> Extracts <code>OrchestratorTrigger</code> / <code>OrchestratorRunInput</code> into <code>shared-data-mongo</code>, adds a new <code>shared-dispatch</code> lib with an <code>enqueue_orchestrator_run</code> facade that replaces the hardcoded task-name/queue/trigger string in <code>vendor-comms</code>, and moves the SLA reset (<code>process_vendor_email_response</code>) out of the inbound-email services into the <code>VENDOR_EMAIL_MESSAGE</code> trigger processor, pinned to a specific <code>side_conversation_message_id</code> passed in <code>run_input</code> to close a race where a later agent reply could be mistaken for the triggering vendor message. Dockerfile restructure rides along because the new libs glob needs it.
<div class="tldr-note">No pr-spec file exists for TECH-6345 in <code>pr-specs/platform_specs</code>. Spec-alignment filter applied against the PR description + Greptile summary only. Greptile's one inline comment (context-loss in <code>_find_or_create_conversation</code>) was already addressed in commit <code>91c4cbe35</code>.</div>
</div>
<!-- Block 1 -->
<div class="block" id="block-1">
<div class="block-header"><div class="block-number">1</div><h2>Shared schema extraction</h2></div>
<div class="block-row">
<div class="block-files">
<ul>
<li><span class="dot dot-add"></span>libs/shared-data-mongo/shared_data_mongo/apps/task_orchestrator/__init__.py</li>
<li><span class="dot dot-add"></span>libs/shared-data-mongo/shared_data_mongo/apps/task_orchestrator/schemas.py</li>
<li><span class="dot dot-mod"></span>api/apps/task_orchestrator/models.py</li>
<li><span class="dot dot-mod"></span>pyproject.toml</li>
</ul>
</div>
<div class="block-content">
<p><strong>What it does.</strong> Lifts <code>OrchestratorTrigger</code> (StrEnum) and <code>OrchestratorRunInput</code> (TypedDict) out of the monolith into <code>shared_data_mongo.apps.task_orchestrator.schemas</code>. The monolith's <code>models.py</code> re-exports them via <code>__all__</code> so 39+ existing import sites continue to work unchanged. Adds a new <code>side_conversation_message_id: str</code> field used by Block 3.</p>
<p><strong>Design.</strong> <code>__all__</code>-based re-export avoids a churn-heavy rename across the monolith. Root <code>pyproject.toml</code> now depends on <code>shared-data-mongo</code>, which aligns with the in-progress mongo extraction effort.</p>
</div>
</div>
<pre><span class="diff-line context"># api/apps/task_orchestrator/models.py</span>
<span class="diff-line remove">-<span class="kw">class</span> <span class="cls">OrchestratorTrigger</span>(<span class="cls">StrEnum</span>):</span>
<span class="diff-line remove">- ...</span>
<span class="diff-line remove">-<span class="kw">class</span> <span class="cls">OrchestratorRunInput</span>(<span class="bi">TypedDict</span>, total=<span class="kw">False</span>):</span>
<span class="diff-line remove">- ...</span>
<span class="diff-line add">+<span class="kw">from</span> shared_data_mongo.apps.task_orchestrator.schemas <span class="kw">import</span> OrchestratorRunInput <span class="cmt"># noqa: F401</span></span>
<span class="diff-line add">+<span class="kw">from</span> shared_data_mongo.apps.task_orchestrator.schemas <span class="kw">import</span> OrchestratorTrigger <span class="cmt"># noqa: F401</span></span>
<span class="diff-line add">+__all__ = [<span class="str">'OrchestratorRunInput'</span>, <span class="str">'OrchestratorTrigger'</span>]</span></pre>
<div class="concerns">
<div class="concern"><strong>Concern.</strong> The <code>__all__</code> re-export shim has no deprecation horizon or ticket reference. Without a target date, this lingers and both import paths stay valid forever — the worst state.</div>
<div class="concern"><strong>Concern.</strong> <code>OrchestratorRunInput</code> is <code>total=False</code> with 18+ trigger-specific fields mixing <code>str</code> (Mongo/message ext-ids) and <code>uuid.UUID</code> (Postgres ids). Tolerable as a monolith-internal type; now that it's a cross-service contract the lack of per-trigger payload typing is a mild smell. Follow-up territory.</div>
</div>
</div>
<!-- Block 2 -->
<div class="block" id="block-2">
<div class="block-header"><div class="block-number">2</div><h2>shared-dispatch library + facade</h2></div>
<div class="block-row">
<div class="block-files">
<ul>
<li><span class="dot dot-add"></span>libs/shared-dispatch/pyproject.toml</li>
<li><span class="dot dot-add"></span>libs/shared-dispatch/Makefile</li>
<li><span class="dot dot-add"></span>libs/shared-dispatch/shared_dispatch/__init__.py</li>
<li><span class="dot dot-add"></span>libs/shared-dispatch/shared_dispatch/task_orchestrator_facade.py</li>
<li><span class="dot dot-mod"></span>services/vendor-comms/pyproject.toml</li>
<li><span class="dot dot-mod"></span>tach.toml</li>
<li><span class="dot dot-mod"></span>pyproject.toml</li>
<li><span class="dot dot-mod"></span>uv.lock</li>
</ul>
</div>
<div class="block-content">
<p><strong>What it does.</strong> New third workspace lib with a single function <code>enqueue_orchestrator_run(celery_app, task_id, trigger, run_input)</code> wrapping <code>celery_app.send_task('api.apps.task_orchestrator.tasks.run_orchestrator_task', kwargs=..., queue='aio')</code>. Replaces the hardcoded constants and <code>FIXME</code> in vendor-comms. Deps: <code>celery</code>, <code>shared-data-mongo</code>, <code>shared-utils</code> — no API-project leakage.</p>
<p><strong>Design.</strong> Facade accepts the caller's <code>Celery</code> app (correct, each service binds its own broker). <code>task_id: UUID | str</code> union is permissive; always stringified before send. Default <code>run_input=None</code> is omitted rather than sent as an empty dict.</p>
</div>
</div>
<pre><span class="diff-line context"># libs/shared-dispatch/shared_dispatch/task_orchestrator_facade.py (new)</span>
<span class="diff-line add">+<span class="kw">from</span> <span class="ty">uuid</span> <span class="kw">import</span> UUID</span>
<span class="diff-line add">+<span class="kw">from</span> celery <span class="kw">import</span> Celery</span>
<span class="diff-line add">+<span class="kw">from</span> shared_data_mongo.apps.task_orchestrator.schemas <span class="kw">import</span> OrchestratorRunInput</span>
<span class="diff-line add">+<span class="kw">from</span> shared_data_mongo.apps.task_orchestrator.schemas <span class="kw">import</span> OrchestratorTrigger</span>
<span class="diff-line add">+<span class="kw">from</span> shared_utils.types <span class="kw">import</span> StrDict <span class="cmt"># noqa: TC002 - runtime import kept so tach recognizes shared-utils as used</span></span>
<span class="diff-line add">+_RUN_ORCHESTRATOR_TASK_NAME = <span class="str">'api.apps.task_orchestrator.tasks.run_orchestrator_task'</span></span>
<span class="diff-line add">+_RUN_ORCHESTRATOR_QUEUE = <span class="str">'aio'</span></span>
<span class="diff-line add">+<span class="kw">def</span> <span class="fn">enqueue_orchestrator_run</span>(</span>
<span class="diff-line add">+ celery_app: <span class="ty">Celery</span>,</span>
<span class="diff-line add">+ task_id: <span class="ty">UUID</span> | <span class="bi">str</span>,</span>
<span class="diff-line add">+ trigger: <span class="ty">OrchestratorTrigger</span>,</span>
<span class="diff-line add">+ run_input: <span class="ty">OrchestratorRunInput</span> | <span class="kw">None</span> = <span class="kw">None</span>,</span>
<span class="diff-line add">+) -&gt; <span class="kw">None</span>:</span>
<span class="diff-line add">+ kwargs: <span class="ty">StrDict</span> = {<span class="str">'task_id'</span>: <span class="bi">str</span>(task_id), <span class="str">'trigger'</span>: trigger.value}</span>
<span class="diff-line add">+ <span class="kw">if</span> run_input <span class="kw">is</span> <span class="kw">not</span> <span class="kw">None</span>:</span>
<span class="diff-line add">+ kwargs[<span class="str">'run_input'</span>] = run_input</span>
<span class="diff-line add">+ celery_app.send_task(_RUN_ORCHESTRATOR_TASK_NAME, kwargs=kwargs, queue=_RUN_ORCHESTRATOR_QUEUE)</span></pre>
<div class="concerns">
<div class="concern"><strong>Smell.</strong> <code>from shared_utils.types import StrDict # noqa: TC002 - runtime import kept so tach recognizes shared-utils as used</code>. <code>StrDict</code> appears only in a local annotation; moving under <code>TYPE_CHECKING</code> would be idiomatic, but tach's static graph gets confused. Either fix the tach config or drop <code>shared-utils</code> from the facade's deps if this really is the only use.</div>
<div class="concern"><strong>Gap.</strong> No unit tests under <code>libs/shared-dispatch/tests/</code>. The <code>pyproject.toml</code> declares per-file-ignores for <code>"**/tests/**/*.py"</code> but the directory doesn't exist. A smoke test asserting <code>send_task</code> is called with the right task name + queue + kwargs shape would lock down this cross-service wire contract cheaply.</div>
<div class="concern"><strong>Contract.</strong> <code>_RUN_ORCHESTRATOR_TASK_NAME</code> and <code>_RUN_ORCHESTRATOR_QUEUE</code> are private but effectively a public cross-service contract. A rename in the monolith's <code>@celery_task(queue='aio')</code> silently breaks every facade caller. No compile-time check.</div>
</div>
</div>
<!-- Block 3 -->
<div class="block" id="block-3">
<div class="block-header"><div class="block-number">3</div><h2>SLA reset moves into the trigger processor with message-id pinning</h2></div>
<div class="block-row">
<div class="block-files">
<ul>
<li><span class="dot dot-mod"></span>api/apps/task_orchestrator/trigger_processors.py</li>
<li><span class="dot dot-mod"></span>api/apps/side_conversations/services.py</li>
<li><span class="dot dot-mod"></span>services/vendor-comms/vendor_comms/side_conversations/services.py</li>
</ul>
</div>
<div class="block-content">
<p><strong>What it does.</strong> Previously both inbound paths fetched the <code>Task</code>, resolved the creating agent, called <code>process_vendor_email_response(...)</code> inline to reset SLA, then enqueued <code>run_orchestrator_task</code>. Now the callers only enqueue the orchestrator (with <code>side_conversation_message_id</code> pinned), and the trigger processor <code>process_vendor_email_message</code> does the SLA reset against the pinned message.</p>
<p><strong>Design.</strong> Pinning by <code>message_id</code> (a string <code>ext_id</code>, e.g. <code>randomString@domain</code>) rather than list index is correct — index would also race against prepends. The best-effort try/except around the SLA call is intentional and documented.</p>
<p><strong>New helpers.</strong> <code>_resolve_triggering_message</code> prefers <code>run_input['side_conversation_message_id']</code>, falls back to <code>messages[-1]</code> with a warning for legacy in-flight tasks. <code>_reset_sla_for_vendor_email</code> resolves the creating agent and calls <code>process_vendor_email_response</code>, swallowing exceptions.</p>
</div>
</div>
<pre><span class="diff-line context"># api/apps/task_orchestrator/trigger_processors.py (wedged between public process_* functions)</span>
<span class="diff-line add">+<span class="kw">def</span> <span class="fn">_resolve_triggering_message</span>(side_conversation: <span class="ty">SideConversation</span>, run_input: <span class="ty">OrchestratorRunInput</span>) -&gt; <span class="ty">Message</span> | <span class="kw">None</span>:</span>
<span class="diff-line add">+ <span class="str">"""...Falls back to the last message for legacy in-flight tasks dispatched before the field was introduced."""</span></span>
<span class="diff-line add">+ <span class="kw">with</span> bound_contextvars(side_conversation_id=<span class="bi">str</span>(side_conversation.id)):</span>
<span class="diff-line add">+ <span class="kw">if</span> <span class="kw">not</span> side_conversation.messages:</span>
<span class="diff-line add">+ logger.warning(<span class="str">'No messages found in side conversation'</span>)</span>
<span class="diff-line add">+ <span class="kw">return</span> <span class="kw">None</span></span>
<span class="diff-line add">+ <span class="kw">if</span> message_id := run_input.get(<span class="str">'side_conversation_message_id'</span>):</span>
<span class="diff-line add">+ <span class="kw">for</span> msg <span class="kw">in</span> side_conversation.messages:</span>
<span class="diff-line add">+ <span class="kw">if</span> msg.ext_id == message_id:</span>
<span class="diff-line add">+ <span class="kw">return</span> msg</span>
<span class="diff-line add">+ logger.warning(<span class="str">'Triggering message not found in side conversation'</span>, message_id=message_id)</span>
<span class="diff-line add">+ <span class="kw">return</span> <span class="kw">None</span></span>
<span class="diff-line add">+ logger.warning(<span class="str">'No side_conversation_message_id in run_input; falling back to latest message'</span>)</span>
<span class="diff-line add">+ <span class="kw">return</span> side_conversation.messages[-<span class="num">1</span>]</span>
<span class="diff-line add">+<span class="kw">def</span> <span class="fn">_reset_sla_for_vendor_email</span>(task, side_conversation, triggering_message) -&gt; <span class="kw">None</span>:</span>
<span class="diff-line add">+ <span class="kw">try</span>:</span>
<span class="diff-line add">+ process_vendor_email_response(task=task, agent=agent, ...)</span>
<span class="diff-line add">+ <span class="kw">except</span> <span class="bi">Exception</span>:</span>
<span class="diff-line add">+ logger.exception(<span class="str">'Failed to reset SLA for vendor email'</span>, task_id=task.id, conversation_id=side_conversation.id)</span></pre>
<div class="concerns">
<div class="concern"><strong>Placement.</strong> <code>_resolve_triggering_message</code> and <code>_reset_sla_for_vendor_email</code> are wedged between public <code>process_*</code> functions. 4/5 agents flagged this. Move to the bottom of the module per convention. <span class="tag">DB</span></div>
<div class="concern"><strong>Drain plan.</strong> Legacy fallback has a comment "remove the fallback once the queue drains" but no ticket, date, or drain-detection counter. Add a <code>TODO(ticket)</code> with a target date, or a distinct log field so we can tell when the <code>aio</code> queue has drained.</div>
<div class="concern"><strong>Missed call site.</strong> <code>api/apps/side_conversations/auto_triage.py:127</code> dispatches <code>VENDOR_EMAIL_MESSAGE</code> WITHOUT <code>side_conversation_message_id</code>. Every auto-triaged conversation permanently hits the legacy fallback branch and emits the drain-window warning — not just during queue drain. Pass the triggering message's <code>ext_id</code> here too for symmetry. <span class="tag">1/5</span></div>
<div class="concern"><strong>Idempotency under retry.</strong> <code>run_orchestrator_task</code> has <code>autoretry_for=ORCHESTRATOR_AUTORETRY_EXCEPTIONS, max_retries=2</code>. The trigger processor runs before <code>_execute_orchestrator</code>, so on an LLM-related retry the SLA reset runs again. Previously it happened once in the webhook handler. Confirm <code>process_vendor_email_response</code> → <code>_process_routing_trigger</code> is idempotent on repeat calls with the same <code>event_context</code>. <span class="tag">1/5</span></div>
<div class="concern"><strong>Broad except.</strong> The bare <code>except Exception</code> is intentional and documented (best-effort), and structlog-sentry auto-reports. Still, narrowing to expected exception types or adding <code>trigger='VENDOR_EMAIL_MESSAGE'</code> + <code>message_id=triggering_message.ext_id</code> to the log kwargs would help Sentry filtering.</div>
<div class="concern"><strong>Consistency.</strong> Monolith <code>side_conversations/services.py</code> still uses raw <code>run_orchestrator_task.delay(...)</code> while vendor-comms uses the new facade. Either migrate the monolith caller or document the facade's scope as "cross-service only" in its docstring. <span class="tag">TODO + follow-up</span></div>
</div>
</div>
<!-- Block 4 -->
<div class="block" id="block-4">
<div class="block-header"><div class="block-number">4</div><h2>Test coverage</h2></div>
<div class="block-row">
<div class="block-files">
<ul>
<li><span class="dot dot-mod"></span>api/apps/task_orchestrator/tests/test_trigger_processors.py</li>
<li><span class="dot dot-mod"></span>api/apps/side_conversations/tests/test_services.py</li>
<li><span class="dot dot-mod"></span>services/vendor-comms/tests/side_conversations/test_services.py</li>
</ul>
</div>
<div class="block-content">
<p><strong>What it does.</strong> Adds four new tests covering the SLA-reset happy path, SLA failure isolation, race-condition pinning, and missing-message abort. Deletes monolith tests that exercised behavior the service no longer owns. Vendor-comms test asserts the new <code>side_conversation_message_id</code> key in enqueued kwargs.</p>
</div>
</div>
<div class="concerns">
<div class="concern"><strong>Weak race test.</strong> <code>test_process_vendor_email_message_keys_to_specific_message_not_latest</code> appends a later <em>agent</em> message to prove the pin beats "pick latest" — but agent messages get short-circuited by the <code>from_agent_id</code> guard inside the processor anyway, so the test passes for the wrong reason. A tighter test would append a second <em>vendor</em> message (both <code>from_agent_id=None</code>) with a later timestamp. <span class="tag">1/5</span></div>
<div class="concern"><strong>Missing coverage.</strong> No test covers the legacy fallback branch (missing <code>side_conversation_message_id</code> in <code>run_input</code>). This is live code as long as queued messages without the field exist.</div>
<div class="concern"><strong>Loose assertion.</strong> <code>test_process_vendor_email_message_triggering_message_not_found</code> asserts the empty-list return but not that <code>process_vendor_email_response</code> was NOT called. Add <code>mock.assert_not_called()</code> to lock the fail-closed contract.</div>
</div>
</div>
<!-- Block 5 -->
<div class="block" id="block-5">
<div class="block-header"><div class="block-number">5</div><h2>Dockerfile restructure</h2></div>
<div class="block-row">
<div class="block-files">
<ul>
<li><span class="dot dot-mod"></span>Dockerfile</li>
<li><span class="dot dot-mod"></span>docker/Dockerfile.local</li>
</ul>
</div>
<div class="block-content">
<p><strong>What it does.</strong> Replaces per-lib <code>COPY pyproject.toml</code> + <code>uv sync --no-install-workspace</code> with <code>uv export --no-emit-workspace</code> → <code>uv pip install</code> of third-party deps, then <code>COPY libs ./libs</code> and <code>uv pip install --no-deps $(ls -d libs/*/ | grep -v shared-tests)</code>. Matches the services' pattern; auto-picks up new libs without Dockerfile edits.</p>
</div>
</div>
<div class="concerns">
<div class="concern"><strong>Fragile exclusion.</strong> <code>grep -v shared-tests</code> silently includes/excludes by substring. Any future lib containing "shared-tests" in its name is dropped; any test-only lib without that substring is included. Prefer <code>grep -vx 'libs/shared-tests/'</code> (exact) and a one-line comment explaining why <code>shared-tests</code> is excluded (it lives in <code>[dependency-groups].test</code>).</div>
</div>
</div>
<!-- Action items -->
<div class="summary-block" id="action-items">
<h2>Summary action items</h2>
<table class="action-items">
<thead><tr><th>#</th><th>Severity</th><th>Issue</th></tr></thead>
<tbody>
<tr><td>1</td><td><span class="severity design">Design</span></td><td><code>_resolve_triggering_message</code> and <code>_reset_sla_for_vendor_email</code> wedged between public <code>process_*</code> functions; move to bottom of <code>trigger_processors.py</code> per convention. <span class="tag">DB</span> <span class="tag">4/5</span></td></tr>
<tr><td>2</td><td><span class="severity design">Design</span></td><td>Legacy fallback in <code>_resolve_triggering_message</code> has no removal date/ticket/drain-detection counter. Add <code>TODO(ticket)</code> with target date or distinct log field so we know when the <code>aio</code> queue has drained.</td></tr>
<tr><td>3</td><td><span class="severity design">Design</span></td><td><code>__all__</code> re-export shim in <code>api/apps/task_orchestrator/models.py</code> has no deprecation horizon. Both import paths ossify otherwise.</td></tr>
<tr><td>4</td><td><span class="severity bug">Bug</span></td><td><code>test_process_vendor_email_message_keys_to_specific_message_not_latest</code> appends a later agent message that the <code>from_agent_id</code> guard skips anyway; test passes for the wrong reason. Use two vendor messages (<code>from_agent_id=None</code>). <span class="tag">1/5</span></td></tr>
<tr><td>5</td><td><span class="severity minor">Minor</span></td><td><code>auto_triage.py:127</code> still dispatches <code>VENDOR_EMAIL_MESSAGE</code> WITHOUT <code>side_conversation_message_id</code>; every auto-triaged conversation permanently hits the legacy fallback + warning log. Fix for symmetry. <span class="tag">1/5</span></td></tr>
<tr><td>6</td><td><span class="severity minor">Minor</span></td><td>No test covers the legacy fallback branch (missing <code>side_conversation_message_id</code>). Add one asserting fallback → last message + warning log.</td></tr>
<tr><td>7</td><td><span class="severity minor">Minor</span></td><td><code>test_process_vendor_email_message_triggering_message_not_found</code> doesn't assert <code>process_vendor_email_response</code> was NOT called. Add <code>mock.assert_not_called()</code>.</td></tr>
<tr><td>8</td><td><span class="severity minor">Minor</span></td><td>No unit tests under <code>libs/shared-dispatch/tests/</code>. Add a smoke test asserting <code>send_task</code> is called with the right task name + queue + kwargs shape.</td></tr>
<tr><td>9</td><td><span class="severity minor">Minor</span></td><td>Monolith uses raw <code>.delay()</code> while vendor-comms uses the facade. Migrate for consistency or document the facade as cross-service-only in its docstring. <span class="tag">TODO + follow-up</span></td></tr>
<tr><td>10</td><td><span class="severity question">Question</span></td><td><code>_reset_sla_for_vendor_email</code> runs inside an auto-retrying task (<code>max_retries=2</code>). Previously SLA was reset once in the webhook. Is <code>process_vendor_email_response</code> → <code>_process_routing_trigger</code> idempotent on repeat calls, or should we gate on "message_id already reset"?</td></tr>
<tr><td>11</td><td><span class="severity question">Question</span></td><td><code># noqa: TC002 - runtime import kept so tach recognizes shared-utils as used</code> on <code>StrDict</code>. Actually needed, or can tach read runtime deps from <code>pyproject.toml</code> and let the annotation move under <code>TYPE_CHECKING</code>?</td></tr>
<tr><td>12</td><td><span class="severity nit">Nit</span></td><td><code>_reset_sla_for_vendor_email</code> bare <code>except Exception</code>. Consider narrowing or adding <code>trigger</code> + <code>message_id</code> to the log kwargs for Sentry filtering.</td></tr>
<tr><td>13</td><td><span class="severity nit">Nit</span></td><td>Dockerfile <code>grep -v shared-tests</code> is brittle. Prefer <code>grep -vx 'libs/shared-tests/'</code> and leave an explanatory comment.</td></tr>
<tr><td>14</td><td><span class="severity expected">Expected</span></td><td>Other orchestrator types still live in the monolith — presumably follow-up PRs continue the extraction.</td></tr>
<tr><td>15</td><td><span class="severity expected">Expected</span></td><td>Per-trigger payload typing for <code>OrchestratorRunInput</code> (replacing <code>total=False</code> omnibus) — worth a follow-up ticket now that it's a cross-service contract.</td></tr>
</tbody>
</table>
</div>
</body>
</html>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment