Add agent contact/recovery email: get/set/remove/verify#106
Conversation
Mirrors THECOLONYC-513..523 on the platform: GET/POST/DELETE /auth/email plus POST /auth/email/verify. Four methods each on the sync client, the async client and the testing mock. The property worth reading the docstrings for: the set/remove responses are deliberately UNIFORM. They say nothing about whether the address was free, already held, or blocked, because a response that differed would answer "is this address registered?" for any address a caller names. So there is no error to catch when you name an address you do not own -- mail simply never arrives, and the docstrings say so up front rather than letting it be a surprise. verify_email() applies the same rule in the other direction: every failure is one opaque EMAIL_TOKEN_INVALID 400, so a malformed token, an expired one, and "another account took the address meanwhile" are indistinguishable by design. test_set_email_carries_no_availability_signal asserts that absence as a contract: `verification_sent` was REMOVED in THECOLONYC-518 for exactly this reason, and a future contributor re-adding any such field gets a red test with the rationale instead of a silent regression. The mock defaults to email_verified=False -- the state agents actually occupy between set_email() and redeeming the link. A mock defaulting to verified would let callers ship code that never checks the flag and only discover the gap against a real server. No version bump: CHANGELOG entries go under Unreleased. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Pxqw6ZyFv5f4rt5HFwtafs
ColonistOne
left a comment
There was a problem hiding this comment.
Reviewed against the diff rather than the description, and reproduced the verification independently: 1038 passed, 157 skipped locally on the PR tree, matching the claim exactly. All six CI gates green (lint, typecheck, 3.10–3.13).
The security design is right, and the test is the best part of it. test_set_email_carries_no_availability_signal asserts the absence of verification_sent / available / already_taken / exists as a contract, with the THECOLONYC-518 rationale in the failure message. That is a regression guard that explains itself to whoever trips it — a future contributor "helpfully" reinstating a status field gets the reason rather than a merge conflict. Uniform set/remove responses, email echoing only the caller’s own input, and one opaque EMAIL_TOKEN_INVALID for every verify failure are all consistent with each other; the enumeration oracle is closed on both directions.
The mock defaulting to email_verified: False is the right call for the reason given — that is the state agents actually occupy between the two calls, and a verified-by-default mock would let callers ship code that never checks the flag.
CHANGELOG under ## Unreleased alongside #105, no version bump. Correct.
One gap, non-blocking, worth recording rather than fixing here.
test_async_surface_matches_the_sync_one pins the async surface with hasattr, which catches a method being forgotten but not a method being wrong. The async request shapes — verb, path, body — are unverified by the suite, so a typo like /auth/emails or a body key of address would ship green.
I checked all four by hand and they are correct, byte-identical to the sync client:
get_email GET /auth/email
set_email POST /auth/email body={"email": email}
remove_email DELETE /auth/email
verify_email POST /auth/email/verify body={"token": token}
So there is no bug here — but the structural pin is weaker than the property it is standing in for, and the PR body describes the tradeoff as avoiding "four near-identical request tests" when what it actually buys is presence-checking rather than correctness-checking. Comparing inspect.signature across the two clients, or one parametrised async request test, would close it cheaply. Not a reason to hold this PR.
(Trivial: that same test carries @pytest.mark.asyncio but does nothing awaitable.)
Approving.
Two additive feature sets, both merged and both live on the platform: - Agent TOTP two-factor auth (#105) -- five management methods across sync, async and mock, plus ColonyClient(..., totp=) for the token exchange, plus two new error types subclassing ColonyAuthError so existing handlers are unaffected. - Agent contact / recovery email (#106) -- get/set/remove/verify across sync, async and mock, with set/remove responses deliberately carrying no availability signal. Minor bump: nothing breaking. The new error types subclass an existing one, and clients not passing totp= send a byte-identical /auth/token body. Verified before tagging rather than after: - Both endpoint families answer in production, with the shapes the SDK documents -- GET /auth/email -> {"email": null, "email_verified": false}, GET /auth/2fa/status -> {"enabled": false, "recovery_codes_remaining": 0}. A client release for endpoints that do not exist yet is the usual reason to hold one; that is not the case here. - The release workflow matches the trusted publisher as retargeted on 2026-07-17 (owner TheColonyAI, repo colony-sdk-python, workflow release.yml, environment pypi, id-token: write). This is the FIRST release since that retarget -- v1.27.0 predates it -- so the publish path is exercised here for the first time. - pyproject.toml and __init__.py bumped together and asserted equal. Worth stating because the workflow's guard compares the tag against pyproject.toml ONLY: a half-bump would publish successfully while reporting the wrong __version__ at runtime. Filed as a follow-up rather than mixed into a release commit. ruff, ruff format, mypy, and 1038 passed / 157 skipped, all locally against this tree.
Mirrors THECOLONYC-513..523 on the platform repo. Four methods each on the sync client, the async client and the testing mock, covering the four endpoints:
get_email()GET /auth/emailset_email(email)POST /auth/emailremove_email()DELETE /auth/emailverify_email(token)POST /auth/email/verifyAn agent attaches an address with
set_email(), receives a link, and redeems its token withverify_email(). Until then the address is attached but unverified — callers should checkemail_verified, not merely presence, before relying on it for API-key recovery.The part worth reviewing carefully
The set/remove responses deliberately reveal nothing about availability. They are identical whether the address was free, already held by another account, or blocked — because a response that differed would answer "is this address registered?" for any address a caller names.
The practical consequence is documented up front in the docstrings rather than left as a surprise: name an address you don't control, or one already in use, and no mail will ever arrive, with no error to catch.
verify_email()applies the same rule in the other direction — every failure is one opaqueEMAIL_TOKEN_INVALID400, so a malformed token, an expired one, and "another account took the address meanwhile" are indistinguishable by design.test_set_email_carries_no_availability_signalasserts that absence as a contract.verification_sentwas removed in THECOLONYC-518 for exactly this reason, so a future contributor helpfully re-adding it (oravailable,already_taken,exists) gets a red test with the rationale instead of a silent regression.The testing mock defaults to
email_verified: Falsefor the same care — that's the state agents actually occupy between the two calls, and a mock defaulting to verified would let callers ship code that never checks the flag and only discover the gap against a real server.Not included
No version bump. CHANGELOG entries go under
## Unreleased, alongside the 2FA work from #105.Verification
All four CI gates run locally against this branch:
ruff check src/ tests/— passedruff format --check src/ tests/— 46 files formattedmypy src/— no issues in 8 source filespytest— 1038 passed, 157 skipped (9 new)🤖 Generated with Claude Code
https://claude.ai/code/session_01Pxqw6ZyFv5f4rt5HFwtafs