Repository navigation
Safer attachment downloads; delete uploaded attachments - #118
Merged
Merged
Conversation
list_profiles() turned any index read error or corrupt JSON into an empty list. After a denied macOS keychain prompt, the next login rewrote the index to hold only the new profile, orphaning the rest, and `auth logout --all` reported "Logged out from 0 profile(s)" as a success. resolve_token likewise mapped every get_token error to "Not authenticated" (exit 3), sending a user with a locked keychain to log in again. get_token now returns Ok(None) for an absent token (and when the keyring is disabled); only that is "Not authenticated". Any other store error propagates as KEYRING_ERROR (exit 10) with its cause, and keychain read errors add a hint to unlock it or set TEAMS_CLI_TOKEN_STORE=file. A Windows token with a missing chunk is now exit 10 too. list_profiles returns Result: an absent index is empty, an unreadable or corrupt one is an error, and add/remove never write after a failed read. `auth list` and `logout --all` surface it. No on-disk or keychain format change. Tested with unit tests on in-memory and always-failing stores (corrupt or unreadable index is never rewritten) and file-store CLI tests: corrupt or unreadable index makes `auth list` / `logout` exit 10 with the index unchanged; absent token exits 3; unreadable or unparseable token exits 10 with the cause. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Hd5XXx9UkpLZpAAkvPPyUg
`auth switch` saved with `save_config(.., None)`, so with `--config customer.toml` it wrote customer.toml's contents plus the new default profile over the default config file and left customer.toml unchanged. A failed save was only a tracing warning beside a success. `config path` always reported the default path, ignoring `--config`. `save_config` truncated the file and wrote it in place, so a failed write (full disk, crash) could leave a partial config. `auth switch` now saves to the `--config` path, and a failed save is a config error (exit 10). `config path` reports the `--config` file when given and adds `source`: `flag` or `default`. `save_config` goes through `write_config_text`, the temp-file-and-rename writer login already uses (0600, writes through symlinks), so `config set`, `config init` and `auth switch` keep the old file on failure. No cross-process locking. Tests: switch with `--config` updates that file and creates no default config under a temp HOME; a switch whose save fails exits 10; `config path` with and without `--config`; `save_config` into a read-only directory fails and leaves the original file intact. All four fail without the fix. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Hd5XXx9UkpLZpAAkvPPyUg
With TEAMS_CLI_TOKEN_STORE=file, token file names map ':', '/' and '\' to '.', so profiles `customer/a` and `customer.a` shared one token file and a command could run as the wrong account. A new profile's name must now be 1-64 of A-Z a-z 0-9 . _ - and not `.` or `..`. `auth login` (via --profile, TEAMS_CLI_PROFILE or default.profile), `config set profiles.<name>...` and `config set default.profile` refuse other names with exit code 2. A name already in the config or the token index is accepted, so existing profiles keep working. The only name the CLI derives itself, `default`, conforms. The file store's name encoding is unchanged. Tests: unit tests for the name check, the `default` fallback and the existing-profile exemption; CLI tests for each refusal path, valid names, and legacy names in the config and in the file store index. An invalid name is checked against the token index; if that index cannot be read, the storage error (exit 10) is reported rather than read as no profiles. Valid names never read the index. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Hd5XXx9UkpLZpAAkvPPyUg
The file token store maps ':', '/' and '\' to '.', and existing profiles keep names outside the new set. So a legacy 'customer/a' and a new, valid 'customer.a' still mapped to one token file: logging in to the new one overwrote the old one's credentials. A new name is now also refused (exit 2) when its token file name matches any existing profile's, in the config or the token index. The mapping is shared with the file store, so the two cannot drift. New valid names now read the index; an unreadable index is still reported (exit 10), not taken as no profiles. Tests: unit cases for an indexed and a configured legacy name, and the CLI login case with 'customer/a' in the file store index; the CLI case fails against the previous check. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Hd5XXx9UkpLZpAAkvPPyUg
macOS and Windows file systems ignore case by default, so 'Work' and 'work' address one token file. The collision check compared names exactly, letting a new 'work' replace an existing 'Work' profile's credentials. Compare them ignoring ASCII case. Test: an existing 'Work' refuses a new 'work'; fails with an exact comparison. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Hd5XXx9UkpLZpAAkvPPyUg
auth status put the signed-in user only inside token_diagnostics, so .data.user read null although auth list shows it; and any failure to load a token, a locked or denied keychain included, came out as authenticated: false with exit 1, the "signed out" answer. Report user and tenant_id at the top level as well. A token store that cannot be read is now KEYRING_ERROR (exit 10), as for every other command; any other failure stays authenticated: false (exit 1) and says why in reason. Tests: top-level user and tenant from a token's claims; a missing token is exit 1 with a reason, an unreadable store is exit 10. Both fail against the previous handler. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Hd5XXx9UkpLZpAAkvPPyUg
List commands without --all-pages fetched one page and printed it with
success true, discarding Graph's @odata.nextLink. A caller could not
tell a complete collection from a truncated one, nor resume: `teams chat
list --page-size 1` looked the same whether one chat existed or fifty.
get_paged and get_paged_without_top now return a Page { items,
next_link }; list API wrappers pass it through and CLI list commands
print it with output::print_success_page. JSON metadata gains has_more
and, when true, next_link (the existing, previously unused field).
--all-pages gives has_more false. data is unchanged. Human and plain
output print "More results available; use --all-pages" on stderr, so
stdout stays parseable. Envelope docs (README, docs/README.md,
teams-agent-contract.7) describe the fields.
Tests: wiremock tests of get_paged for a truncated page (next_link
kept), the last page (none) and --all-pages (links followed, none);
output tests for has_more/next_link in the envelope and for data
matching the plain list envelope.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Hd5XXx9UkpLZpAAkvPPyUg
Attachment downloads wrote to a fixed `<target>.part`, opened with
truncate and no exclusive create, then renamed it into place. A symlink
planted at that path redirected the write to its target, a stray
`.part` was clobbered, and two downloads to one target shared one temp
file. `file download --path` wrote straight to the target, leaving a
truncated file if the write failed.
Both now create a uniquely named temp file in the target directory
with O_EXCL (tempfile::Builder::tempfile_in), write, and persist it
over the target. The temp file is removed on failure. An existing
target is still overwritten and keeps its mode; a new one gets 0666
less umask as before. `attachments download --path -` is unchanged.
tempfile moves from dev-dependencies to dependencies for its exclusive,
self-cleaning temp file and atomic persist.
Tests: symlink at the old `.part` path left alone with its target
unmodified; stray `.part` untouched; a failed persist leaves no temp
file; two concurrent writers leave one whole file; mode kept on
replace; long filenames still fit.
On Windows a rename onto a file another rename is replacing can be
refused ("Access is denied"), so the concurrent-writers test lets one
writer lose there; the target must still be one whole payload with no
temp file left.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Hd5XXx9UkpLZpAAkvPPyUg
`message send --chat C --attach FILE` uploads FILE to the sender's
OneDrive ("Microsoft Teams Chat Files"), but nothing in the CLI could
remove it: `file delete` only handles channel files, and deleting the
message leaves the upload behind.
`teams message attachments delete` takes the same targets as
`attachments list` plus `--index N` and `--yes`. Each selected
`reference` attachment is resolved via GET /shares/{u!token}/driveItem
and deleted with DELETE /drives/{id}/items/{id} only when it is a file
whose parentReference.driveId matches GET /me/drive. Inline images,
cards, folders, other people's files and channel files are skipped
with a reason; so is a share Graph reports as 403/404, since that
cannot be told apart from a file the caller cannot see. A DELETE 404
counts as deleted. Other Graph failures are errors that name the items
already deleted. Missing `--yes` is refused before any token is
resolved (exit 2), as in `message delete`. The message is not changed.
Tested with wiremock at the handler layer (share token path, own-drive
check, delete, 404 as deleted, foreign drive and folder never deleted,
inline image skipped, --index selection, partial-failure notes) and
CLI tests for --yes and target conflicts.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Hd5XXx9UkpLZpAAkvPPyUg
Merged
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Attachment downloads wrote through a fixed
<target>.part, so a symlink there redirected the write and concurrent downloads shared one temp file. Separately, files uploaded withmessage send --attachcouldn't be deleted from the CLI.Behaviour changes:
.partis left alone, and the target keeps its mode.file download --pathno longer truncates the target on failure, and replaces a symlink at the target instead of writing through it.message attachments delete --chat C --message M [--index N] --yesdeletes the OneDrive file behind each uploaded attachment. Only files in your own OneDrive are deleted; files shared with you from another drive, channel files, folders and inline images are skipped with a reason. The message is left as it is.Review guide:
d321216write_atomically, especially permission handling on replace.9c39520the delete command. The safety checks are indelete_item_at.Testing: mock tests show a file in another drive never gets a DELETE request. Checked live: an
--attachupload was deleted, and a second run skipped it.https://claude.ai/code/session_01Hd5XXx9UkpLZpAAkvPPyUg