Skip to content

Follow up items from maintainer for #1483 - afx send/afx inbox #1680

Open
Open
Follow up items from maintainer for #1483 - afx send/afx inbox#1680

Description

@mohidmakhdoomi
mohidmakhdoomi
opened on Sep 12, 2026

#1483 sub-issues — maintainer (waleedkadous) optional / future-change comments

Collected: 2026-09-12T17:04:20Z
Repo HEAD at collection: bcc46f11e (2026-09-12T16:48:19+10:00 / 2026-09-12T06:48:19Z)
Scope: the 13 sub-issues of #1483 —
1361, 1365, 1450, 1471, 1472, 1473, 1474, 1475, 1476, 1477, 1478, 1481, 1482
(sub-issue set confirmed via the GraphQL subIssues connection, 2026-09-12T17:04:20Z).

Where the comments actually live

Only one maintainer comment sits on a sub-issue itself (#1481). Every other
maintainer note about optional/future work arrived as a PR review on the PR that
closed a sub-issue. Both sources are collected below, each labelled with its sub-issue,
its surface, and its UTC timestamp.

Sub-issue Closing PR PR merged (UTC) Maintainer comment surface(s)
#1361 — (closed as stale; fixed by #1354 / PR #1402) — none
#1365 #1492 2026-09-01T11:19:24Z review APPROVED 2026-09-01T11:19:20Z
#1450 #1510 2026-09-03T12:23:07Z review APPROVED 2026-09-03T05:38:15Z
#1471 #1487 2026-09-03T12:22:57Z review APPROVED 2026-09-03T05:36:15Z
#1472 #1484 2026-09-03T12:22:27Z review APPROVED 2026-09-03T05:36:14Z
#1473 #1634 (no maintainer comment on PR) none
#1474 #1491 2026-09-04T15:22:13Z review CHANGES_REQUESTED 2026-09-03T19:45:15Z; DISMISSED 2026-09-04T15:16:58Z
#1475 #1511 2026-09-03T16:58:58Z review APPROVED 2026-09-03T15:35:37Z
#1476 #1485 2026-09-03T12:22:29Z review APPROVED 2026-09-03T05:36:16Z
#1477 #1625 2026-09-08T11:12:56Z issue-comment 2026-09-08T11:12:52Z (no optional items)
#1478 #1486 2026-09-04T15:22:36Z review CHANGES_REQUESTED 2026-09-03T19:44:48Z; DISMISSED 2026-09-04T15:17:01Z
#1481 #1640 (OPEN, CONFLICTING) — issue comment 2026-09-01T11:37:50Z
#1482 #1604 2026-09-04T15:16:56Z review CHANGES_REQUESTED 2026-09-03T19:45:28Z; APPROVED 2026-09-04T15:16:52Z

#1481 — issue comment, 2026-09-01T11:37:50Z

#1481 (comment)

Known corner to fold into this work (documented in the PR #1492 review, deliberately NOT
built standalone — owner call 2026-09-01): bounded in submitToSession is computed once at
enqueue (contended && ceiling && !behindOperator), so with a long delivery in flight and one
operator queued (ceiling armed), a second operator arriving meanwhile gets an unbounded wait —
it sits through the delivery's entire paced write plus the first operator, where pre-#1365 its
interrupt landed immediately. Needs a >2s delivery plus two operator gestures inside its
window, so it is rare, and the write-edge size cap (new issue, refs #1564/#1521) shrinks
worst-case delivery duration to a couple of seconds, which mostly evaporates the window.
If/when --interrupt-after is built: re-evaluate the bound when the operator ahead drains (or
race against all non-operator predecessors) rather than deciding once at enqueue.

Optional/future item

  • O1 — re-evaluate bounded when the operator ahead drains, instead of deciding once at enqueue.

#1365 — PR #1492 review APPROVED, 2026-09-01T11:19:20Z

#1492 (review)

One design corner to carry forward — non-blocking, I'll file it as a follow-up issue:
bounded in submitToSession is computed once at enqueue (contended && ceiling && !behindOperator). With a long delivery in flight and one operator queued (ceiling armed), a
second operator arriving meanwhile gets bounded=false and can wait out the delivery's
entire paced write — unbounded, where the pre-convergence behavior was immediate. It needs a

2s delivery plus two operator actions inside its window, so it's rare; re-arming the bound
when the operator ahead drains looks like the right shape, and it belongs with the
--interrupt-after work that's already sequenced after this PR.

Two accepted tradeoffs I'm explicitly endorsing rather than flagging: preempted → possible
duplicate delivery (the right direction of error vs silent loss), and the bypass counter's
benign scheduling-time false-positive window.

Optional/future item

#1472 — PR #1484 review APPROVED, 2026-09-03T05:36:14Z

#1484 (review)

Two small things you might consider, neither blocking:

Optional/future items

#1476 — PR #1485 review APPROVED, 2026-09-03T05:36:16Z

#1485 (review)

I'll take the arch.md follow-up myself (add db/migrations.ts to Core Components and the repo
tree, and name that test as the enforcing guard) — that's architect territory, as you correctly
left it.

One optional tidy-up for a follow-up: the literal GLOBAL_CURRENT_VERSION = 17 source guard in
send-architect-identity.test.ts:239 is now behaviorally covered and just adds a per-migration
edit tax.

Optional/future items

  • O4 — arch.md: add db/migrations.ts to Core Components and the repo tree, and name the v1→v17 convergence test as the enforcing guard. (maintainer took this one themselves)
  • O5 — drop the literal GLOBAL_CURRENT_VERSION source guard in send-architect-identity.test.ts (per-migration edit tax, now behaviorally covered).

#1478 — PR #1486 review CHANGES_REQUESTED, 2026-09-03T19:44:48Z

#1486 (review)
(The three blocking items were fixed and verified in the DISMISSED review of 2026-09-04T15:17:01Z; only the non-blocking tail is reproduced here.)

Worth a follow-up rather than this PR: give the VS Code extension's send an architect from
(it renders ? → builder today), and thread senderHeaderLabel through the any→builder path
so builder→builder frames stop claiming ARCHITECT (overlaps #1576). Also noting
message-format.ts now depends on the built sdk — a new build-order edge on a file the Tower
server loads; fine, just worth knowing.

Optional/future items

#1471 — PR #1487 review APPROVED, 2026-09-03T05:36:15Z

#1487 (review)

One small loose end you could fold in before merge, or I'll pick it up: issue #1471 names
codev/reviews/1313-afx-send-mailbox-first-delivery.md, whose lines ~341 and ~348 still
describe the CI-aware wall-clock bound as live and the op-count check as pending. That file's
own convention (line 347) is a strike-through + DONE pointer — two lines pointing at this PR.

Worth knowing rather than changing: the branch is well behind main, but the intervening
render-gate work doesn't alter classifyBuffer's measured behavior, and the facade's coupling
to xterm's descriptor shape fails loudly rather than silently on an upgrade. Claude's lane also
suggested the same technique could retire several sibling wall-clock assertions (tunnel-e2e,
shellper-client, session-manager, spec-1365) — a good follow-up issue if you're inclined.

Optional/future items

  • O8 — strike-through + DONE pointer on the two stale lines in codev/reviews/1313-afx-send-mailbox-first-delivery.md (wall-clock bound described as live; op-count check as pending).
  • O9 — apply the counting-facade technique to retire sibling wall-clock assertions (tunnel-e2e, shellper-client, session-manager, spec-1365); file as a follow-up issue.

#1474 — PR #1491 review CHANGES_REQUESTED, 2026-09-03T19:45:15Z

#1491 (review)
(The blocking sanitize.py item was fixed and verified in the DISMISSED review of 2026-09-04T15:16:58Z; only the non-blocking tail is reproduced here.)

Two small things you might consider alongside, neither blocking:

  • buf.cursorY is baseY-relative in xterm's public contract; the code relies on
    viewportY === baseY. buf.baseY + buf.cursorY - buf.viewportY removes that assumption for
    free, and it's now load-bearing for delivery.
  • Idle-session drift is silent — onLiveness gates on recent output, so a re-themed agy
    sitting idle holds forever with no alarm, the same shape as the bare-> bug this fixes. A
    follow-up on profile-drift observability (classifier-stuck escalation for idle sessions, or
    an afx doctor check that classifies each live mirror once) would close that.

Also noting markerFgPalette will want to accept multiple colors before agy ships a truecolor
theme — follow-up territory.

Optional/future items

  • O10 — compute the cursor row as buf.baseY + buf.cursorY - buf.viewportY, removing the viewportY === baseY assumption.
  • O11 — profile-drift observability for idle sessions: classifier-stuck escalation for idle sessions, or an afx doctor check that classifies each live mirror once.
  • O12 — let markerFgPalette accept multiple colors (ahead of an agy truecolor theme).

#1450 — PR #1510 review APPROVED, 2026-09-03T05:38:15Z

#1510 (review)

A few things you might consider, none blocking — happy for them to be a follow-up:

  • tower-routes.ts ~2170: the comment says const projected: HeldMessage[] prevents an extra
    body from compiling. The two lanes disagreed on whether contextual typing through .map()
    actually gives you excess-property checking there — cheapest resolution is
    satisfies HeldMessage on each projected object, which makes the guarantee real regardless.
  • Three of the post-consult fixes have no test pinning them (keep-rows-during-refetch,
    aria-live/aria-busy, and the ref-latched loader — the does not refetch on an unrelated rerender test reuses the same function reference, so it would pass against the old code
    too). A future refactor can undo all three silently.
  • handleInboxList is now exported purely as a test seam for a branch that's unreachable
    through handleRequest; the other route tests all go through the front door, and dropping
    the seam costs nothing.
  • Docs: the popover refreshes only on heldCount changes (matches the plan — just avoid
    implying every mailbox movement refreshes it), and the new arch.md table line ~672
    generalizes "omitted workspace = first known" while /api/inbox intentionally lists all —
    worth qualifying as handler-dependent.
  • The evidence script sits outside tsconfig include and vitest collection, and it's the third
    hand-rolled isolated-Tower fixture — extracting that fixture into a shared helper would be a
    nice small follow-up.

Also noted for the follow-up list: VS Code's mailbox-indicators.ts shows the same N held
with no path to the list, and mobile renders no badge at all — the route you added is what both
would need.

Optional/future items

  • O13 — satisfies HeldMessage on each projected object in tower-routes.ts (make the excess-property guarantee real).
  • O14 — tests pinning three post-consult fixes: keep-rows-during-refetch, aria-live/aria-busy, and the ref-latched loader (the existing "does not refetch on an unrelated rerender" test reuses the same function reference).
  • O15 — drop the handleInboxList test-seam export.
  • O16 — docs: don't imply every mailbox movement refreshes the popover; qualify arch.md's "omitted workspace = first known" as handler-dependent.
  • O17 — extract the hand-rolled isolated-Tower evidence fixture into a shared helper; bring the evidence script under tsconfig/vitest.
  • O18 — VS Code mailbox-indicators.ts shows N held with no path to the list; mobile renders no badge at all — both need the new route.

#1475 — PR #1511 review APPROVED, 2026-09-03T15:35:37Z

#1511 (review)

Non-blocking things you might consider, from the reviews:

  • The empty-command rejection in setIdentity is only safe because restartOnExit is gated on
    hasRestart in session-manager.ts:572 — a non-local invariant. A one-line comment at
    setIdentity (or a test pinning "a session with no restartOptions never spawns") would
    protect it from a future reconnectSession refactor.
  • console.warn in ShellperClient can surface inside a user's attached terminal
    (clientType: 'terminal'), not just Tower's log — an emitted event or injected logger would
    be safer.
  • identitySource: 'welcome' is reported for a Tower-issued SPAWN the shellper hasn't
    confirmed yet; a distinct label (e.g. spawn) would let an operator debugging a
    no-profile hold tell confirmed from optimistic.
  • Crash-loop relaunch end-to-end coverage is structurally identical to the clean-exit lane you
    covered — cheap to add as a follow-up.
  • The codev/evidence/ + scripts/*-evidence.mts convention now has three instances on main;
    worth documenting as a PIR convention and bringing those scripts under a typecheck so the
    lessons-learned citations don't rot. Happy to take that one as an architect follow-up if
    you'd rather.

Optional/future items

  • O19 — comment or test at setIdentity pinning the non-local restartOnExit/hasRestart invariant.
  • O20 — replace console.warn in ShellperClient with an emitted event or injected logger (it can surface in a user's attached terminal).
  • O21 — distinct identitySource label (e.g. spawn) for a Tower-issued SPAWN the shellper hasn't confirmed.
  • O22 — crash-loop relaunch end-to-end coverage.
  • O23 — document codev/evidence/ + scripts/*-evidence.mts as a PIR convention and bring those scripts under a typecheck.

#1482 — PR #1604 review CHANGES_REQUESTED, 2026-09-03T19:45:28Z

#1604 (review)
(The three blocking items were fixed and verified in the APPROVED review of 2026-09-04T15:16:52Z; only the non-blocking tail is reproduced here.)

Smaller, non-blocking: isClassifierStuck and isUnverifiableVerdict are now duplicate
predicates that must be kept in sync — one should call the other; HeldMessage.detail is
required while MailboxEscalationPayload.detail is optional for the same concept; and the 409
RESIZE_DROPPED behavior is in arch.md invariant #10 but not the endpoint table at ~778.
Merge-order note: this adds migration v18 — re-check GLOBAL_CURRENT_VERSION and the pinned
assertion in send-architect-identity.test.ts if another schema PR lands first.

Also from the same review (item 3, a deferral the maintainer explicitly permitted):

(Fine to land the move here and the three renderers as one AIR follow-up, as long as the toast
fix in item 2 isn't left dead.)

Optional/future items

  • O24 — have isClassifierStuck call isUnverifiableVerdict rather than restating the rule.
  • O25 — make HeldMessage.detail and MailboxEscalationPayload.detail agree on optionality.
  • O26 — add the 409 RESIZE_DROPPED behavior to the arch.md endpoint table (it is in invariant Directory Structure - Separate Framework from Project Files #10 only).
  • O27 — the "three renderers speak one vocabulary" AIR follow-up permitted after the hold-verdict move to packages/sdk.

#1477 — PR #1625 issue comment, 2026-09-08T11:12:52Z

#1625 (comment)

Thank you, this is exactly the missing third leg the issue asked for, and the mutation table
makes it easy to trust. Our three-way review approved with no blocking findings, and two
reviewers independently confirmed the ordering divergence you pinned, so #1633 is the right
home for it. Merging now.

Optional/future items: none (the spun-out divergence is tracked as #1633).

Sub-issues with no maintainer comment

Activity

  1. mohidmakhdoomi commented on Sep 12, 2026

    @mohidmakhdoomi
    mohidmakhdoomi
    on Sep 12, 2026
    CollaboratorAuthor

    #1483 maintainer optional/future items — triage: addressed vs pending

    Revised: 2026-09-12 — synthesis of two reviews of the original triage.
    Original verification: 2026-09-12T17:04:20Z; review B rechecked the evidence at 2026-09-12T17:32:12Z.
    Baseline commit: bcc46f11ecfea63ef1cea1f94f5318ffefd93ac9 — committed
    2026-09-12T16:48:19+10:00 (2026-09-12T06:48:19Z), branch main.
    Source of items: maintainer comments collection
    (#1680 issue body, collected 2026-09-12T17:04:20Z), with collection clarifications recorded below.
    Method: retain O1–O27, reconcile both reviews, and recheck disputed code/history claims against
    the baseline. Code references describe that commit; GitHub issue/PR states and search results retain
    the reviews' September 12 snapshot. Commit timestamps below are author dates in UTC; PR merge
    timestamps are labelled separately. Partial work has its own category and still needs follow-up.

    Scoreboard

    Verdict Count Items
    Addressed on main 4 O10, O24, O25, O26
    Partially addressed on main 2 O3, O27
    Implemented, not yet on main 1 O1 (in open PR #1640, CONFLICTING)
    Pending 20 O2, O4, O5, O6, O7, O8, O9, O11, O12, O13, O14, O15, O16, O17, O18, O19, O20, O21, O22, O23

    27 items total: 4 complete on main, 22 with follow-up work remaining, and 1 awaiting integration.
    The four completed items were folded into fix rounds following CHANGES_REQUESTED reviews on
    PR #1491 (2026-09-03T19:45:15Z) and PR #1604 (2026-09-03T19:45:28Z), before those PRs merged.
    That is an observed pattern, not a sufficient explanation: O27 from #1604 is only partial, and
    O6/O7 remain pending despite #1486 also receiving CHANGES_REQUESTED. O3, from an APPROVED review,
    has relevant rationale already present but still lacks the requested persistence explanation.

    The approvals for #1484, #1485, #1487 and #1510 were at 2026-09-03T05:36–05:38Z; #1511 was
    approved at 2026-09-03T15:35:37Z    . Related issues #1576 and #1594 both pre-date the comments that
    mention or overlap them. They provide existing tracking context, not evidence that follow-up issues
    were filed in response to these reviews. The searches found no follow-up issue for O2 or O9.

    Addressed on main

    O10 — cursor row through baseY (from #1474, PR #1491 review 2026-09-03T19:45:15Z)

    DONE in 8d9d666c8 — 2026-09-03T23:36:35Z (author date 19:36:35-04:00),
    3h 51m 20s after the review that asked for it; folded into the same fix round.
    render-gate.ts:371 now reads const cursorRow = buf.baseY + buf.cursorY - top; with a comment
    (:366-368) naming the retired viewportY === baseY assumption.

    O24 — isClassifierStuck delegates to isUnverifiableVerdict (from #1482, PR #1604 review 2026-09-03T19:45:28Z)

    DONE in dca5699ef — 2026-09-03T23:32:40Z (author date 19:32:40-04:00).
    mailbox-delivery.ts:517-522 is now a thin wrapper returning isUnverifiableVerdict(reason, detail),
    with a doc comment citing the maintainer review and explaining why the wrapper is kept (the two
    callers want different types: DB/gate unions vs plain strings).

    O25 — detail optionality reconciled (from #1482, same review)

    DONE in dca5699ef — 2026-09-03T23:32:40Z.
    packages/types/src/sse.ts:61-67 documents MailboxEscalationPayload.detail as REQUIRED,
    matching HeldMessage.detail and the payload's own reason. The field at :68 is the non-optional
    detail: string | null.

    O26 — 409 RESIZE_DROPPED in the arch.md endpoint table (from #1482, same review)

    DONE in dca5699ef — 2026-09-03T23:32:40Z.
    codev/resources/arch.md:782 now carries the row: "200 with the APPLIED dims; 409
    RESIZE_DROPPED     when the session exists but the resize never reached its process (dims
    unchanged); 404 only for an unknown id (#1482)"    . Also at :424 in the route summary and
    :112 in invariant 10.

    Partially addressed on main

    O3 — rationale for not persisting the escalation-toast seen-set

    (from #1472, PR #1484 review 2026-09-03T05:36:14Z)
    PARTIAL. apps/vscode/src/notifications/mailbox-escalation-toast.ts:46 says "no SSE replay, so
    an evicted id has no second event to be deduped against"    , explaining eviction safety. That text
    dates to 744995fa5 (2026-08-17T23:14:47Z), before the review; it does not establish that a
    requested follow-up landed. The explicit explanation of why this module does not persist to
    workspaceState the way gate-toast.ts does remains absent.
    Remaining: add the requested persistence explanation beside the existing rationale.

    O27 — the three renderers speak one vocabulary

    (from #1482, PR #1604 review 2026-09-03T19:45:28Z, item 3's permitted deferral)
    PARTIAL in dca5699ef — 2026-09-03T23:32:40Z, within PR #1604 itself.
    The original review
    names HeldCountBadge, escalationToastText, and approve.ts; the collection's excerpt omits
    that naming sentence. The first two use the shared formatter in packages/sdk/src/hold-verdict.ts
    (apps/web/src/components/HeldCountBadge.tsx:40, apps/vscode/src/mailbox-indicators.ts:16).
    apps/vscode/src/commands/approve.ts:119 still interpolates only result.reason, and its local
    SendResult type at :90 has no detail, although the SDK response carries it
    (packages/sdk/src/tower-client.ts:824-831).
    Remaining: carry the detail through the approval relay result and format it with the same
    shared vocabulary. 5e3deef4e (2026-09-09T17:20:48Z) later extended the formatter with
    composer-redraw; it did not complete this renderer conversion.

    Implemented but not yet on main

    O1 — re-evaluate bounded when the operator ahead drains

    (from PR #1492's approval 2026-09-01T11:19:20Z, carried into the #1481 issue comment
    2026-09-01T11:37:50Z)
    Implemented in PR #1640 — OPEN and
    CONFLICTING in the reviews' September 12 snapshot. The diff removes the enqueue-time
    !behindOperator disarm (const bounded = contended && ceilingMs !== undefined && ceilingMs >= 0;)
    and splits the wait into two concurrently-evaluated conditions — preceding operators finished
    (unbounded, by design) AND predecessors finished or the ceiling expired — which is exactly the
    shape the maintainer prescribed ("re-evaluate the bound when the operator ahead drains… rather than
    deciding once at enqueue"). On main, session-submit.ts:374 still computes
    bounded = contended && … && !behindOperator, so the corner remains at the baseline.
    Action: resolve #1640's conflicts, validate the resulting PR, and merge through the normal
    approval process. The implementation is already in that PR; no separate implementation is needed.

    Pending

    From #1472 — PR #1484 review, 2026-09-03T05:36:14Z (merged 2026-09-03T12:22:27Z)

    O2 — bound the builder-spawn-handler.ts seen set. apps/vscode/src/builder-spawn-handler.ts:11
    is still private seen = new Set<string>(); — add-only, keyed on terminalId, no eviction. The
    maintainer asked for a follow-up issue under #1483; none was found in the collection's
    2026-09-12T17:04:20Z search or the reviews' verification. Pending implementation and tracking.

    From #1476 — PR #1485 review, 2026-09-03T05:36:16Z (merged 2026-09-03T12:22:29Z)

    O4 — arch.md: db/migrations.ts + name the convergence test. db/migrations.ts appears
    nowhere in codev/resources/arch.md; the repo tree at :1388-1391 enumerates index.ts,
    schema.ts and migrate.ts under db/, but omits migrations.ts. The reference at :1836 to
    "the convergence test" is generic: the migration convergence test is not named as the enforcing
    guard. Add the module to both Core Components and the tree, and name the guard. The maintainer
    offered to take this documentation follow-up themselves; it remains pending.
    Related staleness found while checking: arch.md:463 still cites "GLOBAL_SCHEMA, migration
    v14" while the schema is at v18 — worth fixing in the same edit.

    O5 — drop the GLOBAL_CURRENT_VERSION source guard. Still present, and the predicted edit tax
    has already been paid once: send-architect-identity.test.ts:253 now asserts
    GLOBAL_CURRENT_VERSION = 18, bumped from 17 by 3ef84a726 (2026-09-03T18:20:48Z, author date
    14:20:48-04:00, PIR #1482's v18 migration) — 12h 44m 32s after the review, approximately
    12h 45m. PR #1640 touches the same test again for v19, so a second bump is queued. These bumps
    preserve the guard; O5 asks to remove it.

    From #1478 — PR #1486 review, 2026-09-03T19:44:48Z (merged 2026-09-04T15:22:36Z)

    O6 — VS Code send carries no architect from. Both call sites pass options without from:
    apps/vscode/src/commands/send.ts:37 ({ workspace: workspacePath }) and
    apps/vscode/src/extension.ts:1036-1040. The SDK supports it (tower-client.ts:778 — from?: string),
    so this is a caller-side gap; afx inbox still renders ? → builder for a VS Code send.

    O7 — any→builder still claims ARCHITECT for a builder sender.
    tower-routes.ts:1589 routes any→builder to formatArchitectToBuilderMessage, which
    (message-format.ts:239 → formatArchitectMessage:215) interpolates
    architectHeaderLabel(sender) — not senderHeaderLabel(sender). senderHeaderLabel
    (:99) exists and does the right shape-based dispatch, but only the builder→architect path uses it.
    Overlapping issue #1576 is OPEN
    ("builder→builder sends carry a reply hint pointing at the architect, not the sender"). It was
    created by waleedkadous at 2026-09-01T12:08:57Z, before the September 3 review. It is an
    existing related issue whose scope should include the header correction when O7 is picked up.

    From #1471 — PR #1487 review, 2026-09-03T05:36:15Z (merged 2026-09-03T12:22:57Z)

    O8 — stale lines in the 1313 review doc. codev/reviews/1313-afx-send-mailbox-first-delivery.md
    still carries both stale claims verbatim, with no strike-through and no DONE pointer: the Flaky
    Tests entry describes the CI-aware wall-clock bound (process.env.CI ? 800 : 250) as the live
    mitigation and calls the op-count check "the intended replacement (Follow-up)", and the Follow-up
    Items list still reads "Replace the render-gate perf wall-clock assertion with a deterministic
    op-count check" — nine days after PR #1487 did exactly that.

    O9 — retire sibling wall-clock assertions. Pending; no follow-up issue was found in the reviews'
    search. All four files named by the maintainer still have wall-clock assertions:

    • packages/codev/src/agent-farm/__tests__/tunnel-e2e.test.ts:863 — p95 below 50ms; another
      elapsed-time measurement uses performance.now() at :900.
    • packages/codev/src/terminal/__tests__/shellper-client.test.ts:676-679 — elapsed-time bounds.
    • packages/codev/src/terminal/__tests__/session-manager.test.ts:2248 — two measured start times
      must differ by less than 1000ms.
    • packages/codev/src/agent-farm/__tests__/spec-1365-serializer-convergence.test.ts:279 — wait below
      100ms (also timing bounds at :312 and :339).

    From #1474 — PR #1491 review, 2026-09-03T19:45:15Z (merged 2026-09-04T15:22:13Z)

    O11 — idle profile-drift observability. The remaining gap is idle-session classifier liveness:
    the onLiveness path still requires recent output (mailbox-wiring.ts:572), and there is no
    afx doctor check that classifies each live mirror once. Existing alerts provide some coverage:
    packages/codev/src/agent-farm/servers/mailbox-delivery.ts:1726 (noticeOverdue()) enqueues owner
    notices for overdue builder mail without requiring recent output, normally after 180 seconds,
    provided an owner can be resolved. That path skips architect recipients and does not supply the
    general idle-session classifier check requested here. An idle hold can already generate an owner
    notice; the targeted liveness/doctor follow-up remains pending.

    O12 — markerFgPalette single color. Still markerFgPalette?: number (render-gate.ts:132),
    compared with !== at :259, and set to the single value 12 in gate-profiles.ts:145. An agy
    truecolor theme would need this widened.

    From #1450 — PR #1510 review, 2026-09-03T05:38:15Z (merged 2026-09-03T12:23:07Z)

    O13 — satisfies HeldMessage. tower-routes.ts:2403 is still
    const projected: HeldMessage[] = rows.map((r) => ({…})) — the annotation form whose
    excess-property behavior the two review lanes disagreed about. No satisfies anywhere in the
    projection.

    O14 — the three unpinned post-consult fixes. All three remain unpinned in
    apps/web/__tests__/HeldCountBadge.test.tsx: there is no assertion on aria-live/aria-busy
    (both live in the component at HeldCountBadge.tsx:208-209, untested); no test asserts rows stay
    visible across a refetch; and does not refetch on an unrelated rerender (:321) still reuses one
    loadMessages reference across both renders, so it would pass against the pre-fix code — the exact
    vacuity the maintainer called out. A refactor can still undo all three silently.

    O15 — handleInboxList test seam. Still exported (tower-routes.ts:2388) and still imported by
    inbox-routes.test.ts:16, used directly at :421. Both production callers (tower-routes.ts:214
    and :2980) are internal to the same module, so they do not need the export. The direct test
    passes an empty-string workspaceOverride, a branch handleRequest cannot produce. Remove the
    test-only export and reconsider that unreachable-branch test while retaining route-level coverage
    through handleRequest and the internal handler used by production.

    O16 — doc qualifications. arch.md:676 still reads from ?workspace=, else "first known"
    with no handler-dependent qualifier, and nothing notes that /api/inbox intentionally lists all
    workspaces. The popover-refresh wording was not revisited either.

    O17 — shared isolated-Tower fixture + typecheck. There are seven hand-rolled harness/evidence
    scripts in packages/codev/scripts/ (bugfix-1567-head-loss-harness.mts,
    bugfix-1664-streaming-delivery-harness.mts, issue-1450-dashboard-evidence.mts,
    pir-1473-dev-approval-evidence.mts, pir-1473-human-harness.mts, pir-1475-dev-approval-evidence.mts,
    spec-1365-e2e-evidence.mts), up from the three the maintainer counted. A shared helper does
    exist    : startTower in packages/codev/src/agent-farm/__tests__/helpers/tower-test-utils.ts:200
    provides isolated sockets, database/state directory, startup and cleanup, and multiple integration
    tests use it. These seven scripts do not reuse that fixture and still duplicate setup.
    packages/codev/tsconfig.json includes only src/**/*, leaving the scripts outside the package's
    normal typecheck. The follow-up is to reuse or adapt the existing fixture for evidence scripts and
    give those scripts maintained typecheck/validation coverage.

    O18 — VS Code / mobile held-mail surfaces. apps/vscode/src/mailbox-indicators.ts:7-8 describes
    the indicators as count-only/read-only and says dismissal stays CLI-only (afx inbox).
    Separately, heldStatusSegment (:25-30) renders N held with no drill-in, and
    apps/web/src/components/MobileLayout.tsx contains no held-mail rendering.
    Existing related tracking: #1594 is OPEN
    ("vscode: held-mail drill-in — clickable held indicator with inspect + dismiss"). It was created
    by amrmelsayed at 2026-09-03T03:43:31Z, before the 05:38:15Z review, and covers the VS Code
    half. The reviews identified no issue tracking the mobile badge.

    From #1475 — PR #1511 review, 2026-09-03T15:35:37Z (merged 2026-09-03T16:58:58Z)

    O19 — explicit restartOnExit/hasRestart invariant still missing. setIdentity
    (packages/codev/src/terminal/shellper-client.ts:263) has a substantial doc comment, but it explains
    trust and coherence rather than the hasRestart dependency. session-manager.ts:570 computes
    const hasRestart = !!restartOptions;, and :583 assigns restartOnExit: hasRestart.
    There is already related coverage: packages/codev/src/terminal/__tests__/session-manager.test.ts:1101
    reconnects without restartOptions and checks that exit removes the session. It does not explicitly
    assert that no SPAWN is sent. The remaining ask is the linking comment at setIdentity or an
    explicit no-SPAWN assertion; existing teardown coverage should be retained.

    O20 — console.warn in ShellperClient. Still at shellper-client.ts:275, inside the reject
    path, so a discarded WELCOME identity can still print into a user's attached terminal. No emitted
    event, no injected logger.

    O21 — no distinct identitySource label for an unconfirmed SPAWN. The union is still
    'welcome' | 'config' in both places (pty-session.ts:971, tower-utils.ts:723). An operator
    debugging a no-profile hold still cannot tell a confirmed WELCOME identity from an optimistic
    Tower-issued one.

    O22 — crash-loop relaunch e2e coverage. Not added. The only match for restartOptions in
    packages/codev/src/terminal/__tests__/ is a signature comment
    (tower-shellper-integration.test.ts:603); the clean-exit lane remains the only one covered.

    O23 — evidence convention undocumented and untypechecked. No mention of codev/evidence/ or
    scripts/*-evidence.mts as a convention in codev/protocols/ or codev-skeleton/protocols/.
    codev/evidence/ holds six artifacts (1567-head-loss/, 1664-streaming-delivery/,
    1365-dev-approval-transcript.txt, 1473-dev-approval-transcript.txt, 1473-human-runbook.md,
    1475-dev-approval-transcript.txt), and lessons-learned.md:303 cites
    packages/codev/scripts/pir-1475-dev-approval-evidence.mts by path. Keeping those scripts outside
    the typecheck leaves cited evidence vulnerable to unchecked drift (the typecheck gap also appears
    in O17).

    Source collection clarifications

    Both reviews support keeping the 27-item inventory. Review A found all relevant optional asks;
    review B's broader surface inventory found three omitted rebase comments, which contain no optional
    items. These findings are compatible: the item harvest is complete, while the comment-surface
    table needs the following qualifications.

    Suggested grouping if these get picked up

    Group Items Why together
    Docs sweep (architect) O4, O8, O16, documentation part of O23, plus the stale arch.md:463 v14→v18 O4 was the maintainer's own action item; coordinate O23's typecheck work with the fixture group
    Attribution O6, O7 Related sender-label paths; O7 overlaps existing #1576, while O6 needs explicit scope/tracking
    Test and invariant cleanup O5, O9, O13, O14, O15, O19, O22 Related verification work; O13/O15 touch production source, and O19 can be resolved with a comment or test
    Evidence-script infrastructure O17, typecheck part of O23 Reuse/adapt the existing Tower helper and bring scripts under maintained checks
    Gate observability and profiles O11, O12 Idle liveness and marker-color support
    Shellper diagnostics O20, O21 Route identity-rejection logging appropriately and distinguish unconfirmed SPAWN identity
    VS Code follow-ups O2, O3, O27, VS Code part of O18 Finish partial O3/O27; O18's VS Code work overlaps existing #1594
    Mobile held-mail surface Mobile part of O18 Separate UI surface; the reviews found no tracking issue
    Integration O1 Resolve conflicts in PR #1640, validate, then complete the normal approval/merge process
  2. mohidmakhdoomi
    changed the title [-]Follow up items from maintainer for #1483[/-] [+]Follow up items from maintainer for #1483 - afx send/afx inbox[/+] on Sep 12, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions