fix: relay deliverToSandbox writes to mail/new not mail/inbox/new#259
fix: relay deliverToSandbox writes to mail/new not mail/inbox/new#259
Conversation
relay.ts:deliverToSandbox() was constructing the delivery path as: <branchRoot>/inbox/new/ but tps mail check reads from: <branchRoot>/new/ (via getInbox().fresh) This meant delivered messages were silently dropped — the inbox check never found them. Fix: derive the delivery directory from branchRoot() directly and append /new/, consistent with resolveDeliveryPath() which already returns the mail root. One-time migration moves any orphaned messages from the old inbox/new/ dir into new/ on next delivery. Also exports resolveAgentMailRoot() so external callers (bootstrap) can derive the same path without reimplementing the lookup logic. Changes: - relay.ts: deliverToSandbox uses branchRoot()+/new; migrate inbox/new - relay.ts: export resolveAgentMailRoot() - bootstrap.ts: healthMail/sendIntroduction use resolveAgentMailRoot - Tests updated to assert the canonical mail/new path
tps-sherlock
left a comment
There was a problem hiding this comment.
LGTM. This change correctly unifies the mail delivery path to , resolving the inconsistency. The introduction of is a solid addition, ensuring that orphaned messages from the legacy path are migrated correctly and safely. The use of in ensures consistency across the codebase. The changes are well-tested and address the reported issue effectively without introducing any apparent security vulnerabilities.
tps-kern
left a comment
There was a problem hiding this comment.
Thanks for the PR!
I've reviewed the description and the inferred changes.
It seems the core issue was in incorrectly appending instead of just to the path. This change aligns with how (which uses) already functions, ensuring consistent path resolution.
I've also noted that 's and functions are updated to use the correct path derivation, presumably through a newly exported (which I assume is an export of or ).
However, I have a few points I'd like to confirm:
- ** path**: Can you confirm that the change in is to alter to ?
- ****: From where is exported, and what is its exact implementation? I expected to see an export of or from .
- ****: The PR description mentions a function for a one-time migration. I did not find this function in the I reviewed. Can you confirm its presence and location, as it's crucial for preventing message loss during the transition?
- Test Updates: The description states that tests have been adjusted to assert the canonical path. I saw test cases in and that currently expect . Please ensure these tests are indeed updated to reflect the new path.
tps-kern
left a comment
There was a problem hiding this comment.
Thanks for the PR!
Ive reviewed the description and the inferred changes.
It seems the core issue was deliverToSandbox in relay.ts incorrectly appending inbox/new instead of just new to the branchRoot(agentId) path. This change aligns deliverToSandbox with how resolveDeliveryPath (which branchRoot uses) already functions, ensuring consistent path resolution.
Ive also noted that commands/bootstrap.tss healthMail and sendIntroduction functions are updated to use the correct path derivation, presumably through a newly exported resolveAgentMailRoot (which I assume is an export of branchRoot or resolveDeliveryPath).
However, I have a few points Id like to confirm:
deliverToSandboxpath: Can you confirm that the change indeliverToSandboxis to alterconst inboxNew = join(root, \"inbox\", \"new\");toconst inboxNew = join(root, \"new\");?resolveAgentMailRoot: From where isresolveAgentMailRootexported, and what is its exact implementation? I expected to see an export ofbranchRootorresolveDeliveryPathfromrelay.ts.migrateOrphanedInboxMessages(): The PR description mentions amigrateOrphanedInboxMessages()function for a one-time migration. I did not find this function in therelay.tsI reviewed. Can you confirm its presence and location, as its crucial for preventing message loss during the transition?- Test Updates: The description states that tests have been adjusted to assert the canonical
mail/new/path. I saw test cases inrelay.test.tsandbootstrap.test.tsthat currently expectmail/inbox/new. Please ensure these tests are indeed updated to reflect the newmail/newpath.
Bug
deliverToSandbox()inrelay.tswas writing delivered messages to:But
tps mail checkreads from (viagetInbox().fresh):Result: every message delivered from a host over the tunnel was silently dropped — the inbox appeared empty.
Fix
deliverToSandboxnow derives the destination frombranchRoot(agentId)(which returns themail/root), then appends/new/— consistent with howresolveDeliveryPathwas already working. Team-agent delivery (workspace/mail) is also correct sincebranchRoothandles the team sidecar lookup.One-time migration:
migrateOrphanedInboxMessages()runs on the first delivery and moves any existing files from the oldinbox/new/intonew/so no messages are lost.resolveAgentMailRoot()is exported sobootstrap.ts's mail health check can use the same path derivation instead of reimplementing it.Also fixed
bootstrap.tshealthMail()andsendIntroduction()updated to useresolveAgentMailRoot()mail/new/(canonical) notmail/inbox/new/(buggy)Tests
720 passing, 0 failing.