Skip to content

fix(email): allow move_to_label to restore inbox - #3267

Open
mikemikimike wants to merge 4 commits into
amd:mainfrom
mikemikimike:fix/2626-restore-inbox
Open

fix(email): allow move_to_label to restore inbox#3267
mikemikimike wants to merge 4 commits into
amd:mainfrom
mikemikimike:fix/2626-restore-inbox

Conversation

@mikemikimike

Copy link
Copy Markdown
Contributor

Closes #2626

Summary

  • keep INBOX on a message when move_to_label is used to restore it
  • preserve the existing add-label-and-archive behavior for non-Inbox targets
  • add fake-mailbox regression coverage for both terminal states

Validation

  • python -m pytest hub/agents/email/python/tests/test_move_to_label_inbox_2626.py hub/agents/email/python/tests/test_label_resolution_2428.py -q — 11 passed
  • full email suite: 2035 passed, 4 skipped, 4 unrelated environment/timing failures
  • python -m black --check --target-version py311 hub/agents/email/python/gaia_agent_email/tools/organize_tools.py hub/agents/email/python/tests/test_move_to_label_inbox_2626.py
  • python -m isort --check-only hub/agents/email/python/gaia_agent_email/tools/organize_tools.py hub/agents/email/python/tests/test_move_to_label_inbox_2626.py
  • git diff --check
  • python -m compileall -q hub/agents/email/python/gaia_agent_email/tools/organize_tools.py hub/agents/email/python/tests/test_move_to_label_inbox_2626.py

The full-suite failures are the pre-existing scheduler timing test on this host, two source-read tests requiring UTF-8 under the Windows GBK locale, and the source-checkout package-metadata test for gaia-agent-email.

@github-actions github-actions Bot added the agent::email Email agent changes label Sep 1, 2026
@github-actions

github-actions Bot commented Sep 1, 2026

Copy link
Copy Markdown
Contributor

Request changes

The single-message fix is right and matches what the issue asked for: asking the email agent to put a message back in the inbox now actually leaves it there instead of silently archiving it again, and non-inbox moves are untouched. Two things to sort out before merge.

The bulk version still has the original bug. The batch "move these messages to a label" tool always archives after labelling, so restoring three or more messages at once still adds the message to the inbox and takes it right back out — while reporting success. Since this PR advertises inbox restore in the tool description the model reads, the model will reach for the bulk tool the moment a user names more than a couple of messages, and land on exactly the silent no-op #2626 is about. Either apply the same guard there, or make the bulk tool refuse an inbox target and point at the single-message tool, and file the follow-up.

Missing changelog entry. Comparable email-agent fixes (the label-resolution and archive-verification fixes) each added an entry to the agent package's changelog, and the project rule is that a behavior change lands in the package's docs in the same change. This is a user-visible behavior change to a shipped tool with no entry.

Real-world evidence

N/A — no evidence bundle was produced for this run, so this verdict rests on static review plus reading the changed code and its tests. Worth noting the PR description shows unit tests and lint only, while the issue explicitly asked for a before/after label set on a real or fixture message demonstrating the restore. The change also edits a tool description the model sees, which the project treats as an LLM-affecting surface — prior email PRs either ran the agent eval or stated in the description why it wasn't warranted; this one does neither. A one-line note either way would close that out.

🔍 Technical details

Issues

🟡 Batch path still archives unconditionally (hub/agents/email/python/gaia_agent_email/tools/organize_tools.py:1176-1180)

_move_op inside move_to_label_batch mirrors the old move_to_label_impl body — backend.add_label(mid, resolved) then backend.archive_message(mid) with no target check. With label_id="INBOX" the message is re-added to and immediately removed from the inbox, and _run_batch_with_prior records it in succeeded. The tool docstring at :1161 also still reads "Move multiple messages out of INBOX into a label", so it contradicts the new move_to_label docstring.

                def _move_op(backend, mid: str) -> str:
                    resolved = _resolve_label_id(backend, label_id_local, label_cache)
                    backend.add_label(mid, resolved)
                    if resolved != _INBOX_LABEL:
                        backend.archive_message(mid)
                    return resolved

If you'd rather keep the PR scoped to the single-message path, reject _INBOX_LABEL in move_to_label_batch with an actionable error naming move_to_label, and open the follow-up — a silent success is the failure mode #2626 exists to kill.

🟡 No CHANGELOG.md entry (hub/agents/email/python/CHANGELOG.md, and hub/agents/email/npm/CHANGELOG.md if this ships to the npm sidecar)

Precedent: #2428 (55010368) and #2406 (c2337178) each added an entry alongside the organize_tools.py change. CLAUDE.md's hub-agent doc rule requires the CHANGELOG to name the behavior change. Suggested [Unreleased] entry, in the changelog's plain-language voice:

Asking to put a message back in your inbox now works. move_to_label with INBOX as the target used to add the label and archive the message in the same call, undoing itself while reporting success. Other target labels keep the existing move-out-of-inbox behavior (#2626).

Nits

🟢 Use the existing _INBOX_LABEL constant (organize_tools.py:281) — the module already defines it at :32 and uses it at :70.

        if label_id != _INBOX_LABEL:

🟢 The new docstring is Gmail-only guidance (organize_tools.py:829-832) — OutlookBackend.list_labels returns master categories only, so _resolve_label_id(backend, "INBOX") raises Invalid label: 'INBOX' … on an Outlook mailbox. It fails loudly, which is correct, but the error lists categories rather than pointing at unarchive_message. A clause in the docstring scoping the INBOX hint to Gmail would keep the model from confidently trying it on Outlook.

Strengths

  • The fix is minimal and preserves the documented ordering invariant (backend call first, DB row only on success); label_id is compared after _resolve_label_id, so "Inbox" and "inbox" normalize to the INBOX id and take the restore path.
  • Both regression tests assert terminal label state and archive_calls, so a future refactor that reintroduces the archive can't pass by accident — and the non-INBOX test locks the old behavior in place.
  • Test scaffolding matches the package's conventions (DatabaseMixin + :memory:, action_store.init_schema, issue-numbered filename).

@kovtcharov-amd kovtcharov-amd left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Blocking on the review above: the bulk move tool still has the original bug, so restoring three or more messages at once still adds them to the inbox and takes them straight back out while reporting success. Since this PR advertises inbox restore in the description the model reads, that's the path the model will take. The package changelog entry is also still missing.

Marking this formally so the PR stops reading as ready to merge — CI going green doesn't clear an open review request. Push the fix and re-request review.

@mikemikimike

Copy link
Copy Markdown
Contributor Author

Implemented the batch follow-up in commit 3f68d283cf883f7ae9775f8d2e7a68fa0a7a62f9.

move_to_label_batch now uses the same INBOX guard as the single-message path: it adds INBOX without immediately archiving when restoring messages, while non-INBOX targets retain their existing archive behavior. Extended the #2626 regression coverage through the real batch tool boundary with three messages, and added the missing email package changelog entry.

Validation:

  • .venv\\Scripts\\python.exe -m pytest hub/agents/email/python/tests/test_move_to_label_inbox_2626.py -q --basetemp pytest-tmp-3267-inbox — 3 passed
  • .venv\\Scripts\\python.exe -m pytest hub/agents/email/python/tests/test_label_resolution_2428.py -q --basetemp pytest-tmp-3267-label — 9 passed
  • .venv\\Scripts\\python.exe -m compileall -q hub/agents/email/python/gaia_agent_email/tools/organize_tools.py hub/agents/email/python/tests/test_move_to_label_inbox_2626.py — passed
  • git diff --check — passed

The updated PR head is remotely verified and the new CI run is pending.

@mikemikimike

Copy link
Copy Markdown
Contributor Author

Follow-up for the CI failure: commit 72e74aa adds the missing minimal OrganizeToolsMixin test host and preserves the tool registry around registration. The batch regression now reaches the real move_to_label_batch closure and asserts all three messages retain INBOX with zero archive calls.

Validation:

  • test_move_to_label_inbox_2626.py and test_label_resolution_2428.py: 12 passed
  • git diff --check passed

The new GitHub Actions run is pending.

@github-actions

github-actions Bot commented Sep 3, 2026

Copy link
Copy Markdown
Contributor

🟡 The changelog entry for this fix landed in four places instead of one — the correct [Unreleased] section and three already-released version sections. Entries in released sections falsify the history by implying the batch-inbox fix shipped before this PR.

Remove the three copies from the released version blocks; keep only the one in [Unreleased].

🔍 Technical details

hub/agents/email/python/CHANGELOG.md lines 169, 452, and 838 each contain the same new entry that belongs only at line 12 (the [Unreleased] section). The three duplicates sit inside past-release ### Fixed blocks — each of those versions actually shipped without this fix.

Delete the duplicate blocks at lines 169–172, 452–455, and 838–841 (four-line block each: the bullet and its two continuation lines). Keep the entry at line 12.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

agent::email Email agent changes

Projects

None yet

Development

Successfully merging this pull request may close these issues.

fix(email-agent): move_to_label unconditionally archives, so it cannot restore a message to the inbox

2 participants