feat(mail): add Bot-bound Agent Mail commands by Liuxy20 · Pull Request #129 · Mininglamp-OSS/octo-cli

Liuxy20 · 2026-08-11T16:39:25Z

Summary

Add Bot-bound Agent Mail authorization and command support through the unified OCTO gateway.

Changes

Add PKCE mail auth login and mail auth status flows with authoritative Bot identity verification.
Store mailbox credentials and pending authorization proof in dedicated encrypted files, bound to RobotID, SpaceID, the normalized Octo API origin, and the authorizing Bot token fingerprint.
Add metadata-driven Mail commands for mailbox identity, messages, threads, attachments, controlled send, versioned Drafts, delivery state, and JMAP changes.
Select Bot and Mail credentials at separate transport boundaries, suppress Bot Space headers for mailbox operations, and disable retries for ambiguous Mail side effects.
Keep explicit --bot-id selection fail-closed, expose an unverified environment claim as identity.robot_id_claimed, and keep Mail --dry-run fully network-free.
Add auth update with the same API-origin validation as auth login, the embedded octo-mail skill, documentation, and regression tests.

Motivation

Fixes #128

Depends on Mininglamp-OSS/octo-mail#48 and the external /agent-mail-api routing in Mininglamp-OSS/octo-web#1315.

The CLI calls the device and token bootstrap endpoints without attaching Bot or Mail credentials. A real authorization login verifies the active Bot through /v1/bot/register before creating the device flow. Live end-to-end authorization was not exercised; the external gateway must admit those public PKCE bootstrap endpoints while stripping unrelated credentials.

Testing

Checklist

Code is in English (comments, commit messages, variable names)
New commands added to README Command Reference table
CLAUDE.md command list updated if commands changed
New features include tests
No unrelated changes included

Jerry-Xin

Summary: Well-integrated new command domain with a clean Bot/Mail credential-transport split and security-aware skill docs, but there is one blocking authorization-boundary gap: normal mailbox commands select the stored Mail credential from an unverified OCTO_BOT_ID, and the PR is currently CONFLICTING with a failing required check. Requesting changes.

🔴 Blocking

Bot-bound mailbox access is not verified for normal mail operations (cross-bot mailbox selection).
MailCredentialFunc resolves identity via ResolveBotIdentity(ctx) (internal/cmdutil/factory.go:154, :159), which calls resolveBotIdentity(ctx, verify=false). That function early-returns when RobotID is already populated: if strings.TrimSpace(bot.RobotID) != "" && !verify { return bot } (internal/cmdutil/factory.go:256). For token-only runtimes, EnvProvider.Resolve() populates RobotID directly from OCTO_BOT_ID with no verification (internal/credential/env_provider.go:60-66).
- Failing input: OCTO_BOT_TOKEN=<botA token> + OCTO_BOT_ID=botB → octo-cli mail message list (or send-intent) selects mail-credentials.enc["botB"] via storedMailCredential and drives botB's mailbox, without ever proving the active Bot token belongs to botB. The mail requests carry only the Mail token (client.NewMail drops the Bot token), so there is no server-side A==B check either.
- This contradicts the PR's stated Bot-bound model and the ResolveBotIdentity doc comment ("token-only runtimes resolve it from the standard Bot registration endpoint"). The login path already does the right thing with VerifyBotIdentity (internal/cmdutil/factory.go resolveBotIdentity(..., true), which cross-checks claimed vs /v1/bot/register resolved id); use-time selection must match.
- Fix: have Mail credential resolution call VerifyBotIdentity (or verify at least for env/OCTO_BOT_ID-derived, unverified Bot ids) before selecting the stored Mail token.
CI red + branch conflicts (merge gate). Live status: mergeable: CONFLICTING (mergeStateStatus: DIRTY) and the check-sprint check is failing. The required build & test (1.24.x) check is not present in the current rollup. Rebase onto main to clear the conflict and get a green required build/test run before merge. (Local go build ./... + go test ./... both pass at this SHA.)

🟡 Non-blocking

mail auth status connected-probe ignores global flags. showCurrentMailConnection builds the identity-probe client with only ErrOut (cmd/mail_auth.go:267), so --dry-run, --timeout, --verbose, --no-retry are silently ignored on that one path, diverging from both generated mail commands and the auth bootstrap client. Pass the same client.Options{Verbose/DryRun/NoRetry/Timeout} used elsewhere.
CHANGELOG not updated. This adds a whole new mail command domain (+3153) but CHANGELOG.md [Unreleased] has no entry, while the repo consistently documents new domains there (e.g. the message search and html domains). Add an ### Added entry for the mail domain.
submissionIds schema type mismatch. internal/registry/specs/mail.json:476 declares submissionIds as array of integer, but cmd/service/mail_test.go:239 returns ["S1"] (strings). Align the spec with the real backend contract so octo-cli schema / embedded docs are accurate.

✅ Highlights

Clean Bot vs Mail credential-transport separation via x-octo-credential: mail; NewMail keeps Mail tokens out of the Bot credential provider.
Side-effecting mail ops correctly set x-octo-retry: never → DisableRetry + UnknownOutcomeOnNetworkFailure (RESULT_UNKNOWN), and suppress X-Space-Id.
Pending device-flow proof and mailbox tokens stored in separate encrypted files, map-keyed by Bot id (no filename path-traversal surface); PKCE verifier generated with crypto/rand.
Strong security-oriented skills/octo-mail/SKILL.md (untrusted-input handling, confirmation-token flow, no chained login/status/me). Header injection is the backend's responsibility here — the CLI only ships JSON fields (to/subject/text), it never assembles raw MIME headers from user input.
Tests pass locally: go build ./..., go test ./... all green at fbe4ac13d61c.

Jerry-Xin

Full independent review of the new Bot-bound Agent Mail command domain (re-review on head 3815eec9). No blocking correctness, security, or contract issues found: the feature is metadata-driven through the existing validated command engine, mail credentials are Bot-bound and encrypted at rest, secret masking holds, and there is no client-side raw-header (CRLF) injection surface. APPROVE. One 🟡 doc omission below.

🟡 Non-blocking

CHANGELOG has no Agent Mail entry. CHANGELOG.md is not touched by this PR (git diff main..HEAD -- CHANGELOG.md is empty) and grep -i mail CHANGELOG.md returns nothing, while the [Unreleased] → Added section documents the drive domain. The PR adds an entire new domain (24 operations + hand-written mail auth, mail message state/changes, auth update) but the changelog does not mention it. README.md, CLAUDE.md and skills/octo-mail/SKILL.md are all updated correctly, so the command surface is documented — only the changelog is missing the entry. Recommend adding an octo-cli mail bullet under [Unreleased] → Added for release-notes parity.
No test for invalid mail-token-prefix rejection. cmd/mail_auth.go:191 rejects a token response whose AccessToken lacks the omb_ prefix, but no test exercises that branch (tests only assert stored values equal expected). Low risk; consider a negative case.
Per-write-verb error paths not individually tested. send/send-intent write paths have no-retry + idempotency coverage in cmd/service/mail_test.go, but reply/reply-all/forward/draft update error paths are not individually exercised. They share the generated runOperation path, so coverage is transitive; explicit cases would be defense-in-depth.

✅ Highlights / verified clean

No CRLF / raw-header injection surface. SendMessage/ReplyMessage to/cc/bcc/subject/text/html are serialized as JSON and POSTed to the WebAPI (internal/registry/specs/mail.json); the CLI never builds raw RFC 5322 headers or uses net/smtp. Address parsing / header sanitization is correctly delegated to the backend. Grep for Subject:/\r\n/net/smtp/mime in cmd/ + internal/ (non-test) = zero hits. Header injection is therefore not a client-side vector here.
Mail token never leaks. The mailbox omb_ token is a stored Authorization credential, not a body field. In --dry-run the Authorization header is masked via credential.MaskToken (internal/client/client.go renderDryRun); verbosef only logs method/URL/body, never the auth header; error paths route through redactError/redactExitError. Pending device-flow secrets (deviceCode, codeVerifier) are stored only in the encrypted, secPerm credential files (internal/authstore/mail.go), verified by a byte-scan test. No unmasked omb_/accessToken/codeVerifier/deviceCode reaches stdout/stderr (non-test grep = zero hits).
Bot-bound credential isolation. internal/credential/mail.go MailCredential is a distinct type; client.NewMail builds a transport BotCredential carrying only Token+Source (no SpaceID), so mailbox ops correctly suppress X-Space-Id (matches x-octo-space-header:false). Cross-Bot binding is rejected at authorization (cmd/mail_auth.go: token.BotID != bot.RobotID → auth error) and identity is verified against /v1/bot/register via VerifyBotIdentity (strict) on login. Encrypted-at-rest + cross-Bot transfer prevention are tested (internal/authstore/mail_test.go).
Non-idempotent side effects are safe. x-octo-retry: never (send/reply/reply-all/forward/reply-draft/send-intent/draft create/send/update, message/draft delete) maps to DisableRetry + UnknownOutcomeOnNetworkFailure in cmd/service/run.go; a lost response becomes RESULT_UNKNOWN (markResultUnknown) rather than a silent duplicate send. cmd/service/mail_test.go asserts exactly one call on 503 for send-intent.
Reused validation engine. Body/param validation (required/enum/uint64, rune-count length, JSON null handling, --data/--params UseNumber + decodeStrict) is the pre-existing engine; the loader change is limited to Credential and RetryMode plumbing (+14 lines). Required headers (X-Octo-Idempotency-Key, X-Octo-Confirmation) map via x-octo-flag.
Attachment/raw download reuses the atomic, 2xx-gated binary-write path (writeFileAtomic) shared with file.download; no new integrity regression.

Build / test / lint / CI

go build ./... — pass
go test ./... — pass (all 12 packages)
golangci-lint run (v2.1.6, repo .golangci.yml) — 0 issues
Live CI: check-sprint = failure, but this is a project-board Sprint-field check (linked issue #128 has no Sprint set on the Octo Board) — a maintainer action, not a code failure; label = pass; code-review = pending. No build & test workflow is configured on this repo, so local build/test/lint above is authoritative.

Threat-model correction (byte-verified at 3815eec): the cross-bot claim does not hold. Mail send/read is authorized solely by the mailbox token (omb_), which client.NewMail sends; the Bot token is never transmitted on mail calls. Reaching botB's mailbox requires botB's mail token to already exist in the local store, and that entry can only be created by mail auth login, which runs VerifyBotIdentity (verify=true → /v1/bot/register + token.BotID!=bot.RobotID rejection). Setting OCTO_BOT_ID=botB with bot A's Bot token grants no capability the caller lacks: the Bot token is not sent and cannot mint/refresh a mailbox token at use-time (SaveMailCredential is called only from the verified login path). This reduces to local-store possession, not privilege escalation. Dismissing this REQUEST_CHANGES in favor of the APPROVE.

yujiawei

Code Review — PR #129 (octo-cli)

Reviewed at head 3815eec958af7653367ee971ab6462fff838857b against merge-base 16949d9dea89c25c8d4a106950e1205dda90a670 (34 files, +3153/−36).

Verification performed locally: go build ./... OK, go vet ./... OK, go test -count=1 ./... OK (12/12 packages). Behaviour claims below were checked by running the built binary against a stub OCTO/Agent-Mail server, not by reading alone; one regression is reproduced with a failing test included in this review.

What is solid. I confirmed on the wire that mail operations carry only Bearer omb_* and no X-Space-Id, that the Bot token never reaches /agent-mail-api, and that all services stay on OCTO_API_BASE_URL. RESULT_UNKNOWN fires exactly on the ops marked x-octo-retry: never and read paths correctly stay NETWORK_ERROR. The PKCE construction is right (32 random bytes → 43-char base64.RawURLEncoding verifier, S256 challenge). The identity-mismatch guard at authorization time works: with a profile claiming robot-A and a token the server resolves as robot-B, mail auth login refuses. Good separation of concerns overall, and the transport-boundary comments are genuinely helpful.

1. Specification compliance

Spec: ❌

Measured against the acceptance criteria in #128.

Missing: none material. All listed command surfaces, auth update, the RFC 8621 state/changes pair, the embedded skill, and the documentation updates are present.

Over-build: none. auth update and the JMAP commands are both named in the linked issue; no direct octo-mail base URL and no mailbox-token environment variable were added; owner confirmation is not bypassed and ambiguous side effects are not retried. The declared out-of-scope list is respected.

Deviations (both against AC "Mail and pending-authorization secrets are encrypted, scoped to the RobotID, excluded from output, and cleared when the owning profile is removed or rebound"):

Scoped to the RobotID is not enforced on the read path. internal/cmdutil/factory.go:376 returns early whenever a RobotID is merely claimed, so internal/cmdutil/factory.go:154 → storedMailCredential (internal/cmdutil/factory.go:265) keys the mailbox-token lookup on an unverified string. Detail and reproduction in P0-1 below.
Excluded from output does not hold for the pending-authorization secrets. cmd/mail_auth.go:181-188 sends deviceCode and codeVerifier with no SecretValues, so internal/client/client.go:945 prints both in cleartext. Detail in P1-1 below. (Reading "output" as stdout only, this is arguable — but these are precisely the two values the AC names as secrets, and the same request's Authorization header is masked, so the asymmetry looks unintended rather than chosen.)

Additionally, CLAUDE.md:29-33 states that for token-only runtimes "an optional OCTO_BOT_ID is verified against that result." That is true for mail auth login/status but false for every other mail command — see P0-1. The documentation added by this PR describes an invariant the code added by this PR does not enforce.

2. Code quality

Quality: Changes-Requested

P0-1 — Mailbox credential is selected by an unverified RobotID claim

internal/cmdutil/factory.go:376, reached from internal/cmdutil/factory.go:154

if strings.TrimSpace(bot.RobotID) != "" && !verify {
    return bot, nil
}

MailCredentialFunc calls ResolveBotIdentity (non-strict), so any non-empty RobotID short-circuits the /v1/bot/register check. buildCredential (internal/cmdutil/factory.go:256-258) and EnvProvider.Resolve both populate RobotID straight from OCTO_BOT_ID, which is caller-supplied and unauthenticated.

Reproduced with the built binary. Given a store holding robot-B's mailbox token and no profiles:

OCTO_BOT_TOKEN=app_SOME_OTHER_BOT_TOKEN OCTO_BOT_ID=robot-B octo-cli mail message send --data '{...}'

the stub server logged:

POST /agent-mail-api/webapi/v0/messages auth=Bearer omb_MAILTOKEN_B space=None

and no /v1/bot/register request was made at all. The Bot token was never validated against anything — any syntactically plausible app_* string plus knowledge of the RobotID reaches the stored mailbox and can send mail as that identity.

This is deliberate rather than accidental: internal/cmdutil/factory_test.go:521 (TestFactory_MailCredentialSupportsRuntimeBotIdentity) asserts exactly this behaviour with no stub server present, i.e. it locks in the unverified path. That is why I am raising it as a decision to revisit rather than a typo.

Why it matters even though the store is same-OS-user scoped (the repo already documents OCTO_CONFIG_DIR / separate OS users as the isolation boundary, and that boundary still holds): the realistic trigger is not an attacker but a stale or wrong OCTO_BOT_ID in the environment, or a profile whose token was rotated while retaining the old RobotID. The consequence is silently sending email as another Bot's identity — the one outcome a "Bot-bound" mail feature exists to prevent, and the reason VerifyBotIdentity was written. Login verifies; use does not.

Please either (a) verify before handing out a mailbox credential — VerifyBotIdentity in MailCredentialFunc, accepting one /v1/bot/register round-trip per mail command, or gate it so a RobotID that came from OCTO_BOT_ID is always verified while a RobotID already confirmed this process is not; or (b) if the round-trip is intentionally being avoided, say so explicitly in CLAUDE.md and correct the sentence at CLAUDE.md:32, and drop the claim that OCTO_BOT_ID is verified.

P0-2 — Creating the default-named profile silently destroys a valid mailbox authorization

internal/authstore/authstore.go:183-193

previous, existed := profiles[name]
if !existed || previous.RobotID != meta.RobotID {
    delete(mailTokens, name)
    delete(pendingMail, name)

For a brand-new profile existed is false, so delete(mailTokens, name) runs unconditionally. auth login --bot-id X with no --profile names the profile X (asserted by cmd/auth_test.go:177, TestAuth_ProfileNameDefaultsToBotID) — the same string the mailbox token is keyed under. The Bot identity has not changed, yet its authorization is deleted.

Reproduced end to end: an env-token runtime completed mail auth login + mail auth status (mail-credentials.enc present on disk), then auth login --with-token --bot-id robot-B was run for the same Bot; mail-credentials.enc was gone and mail me returned Agent Mail is not connected for the active Bot. Recovering requires re-running the full human-approval device flow.

internal/authstore/mail_test.go:120 (TestSavingProfilePreservesExistingRobotIDMailCredential) was clearly written to prevent this, but uses profile name "agent" with RobotID "bot-a" — so delete(mailTokens, "agent") is a no-op and the test passes without exercising the guard. Per the repo's own Test Discipline section, here is the same test with the production default naming; it fails on this head:

func TestSavingDefaultNamedProfilePreservesMailCredential(t *testing.T) {
	t.Setenv(EnvConfigDir, t.TempDir())
	store, _ := New()
	_ = store.SaveMailCredential("bot-a", "omb_a")
	// profile name == RobotID: what `auth login --bot-id bot-a` actually produces
	_ = store.SaveProfile("bot-a", &ProfileMeta{RobotID: "bot-a"}, "app_a")
	got, err := store.GetMailCredential("bot-a")
	if err != nil || got != "omb_a" {
		t.Fatalf("mail credential after profile creation = %q, %v", got, err)
	}
}

--- FAIL: TestSavingDefaultNamedProfilePreservesMailCredential (0.00s)
    mail credential after profile creation = "", mail credential not found for Bot key "bot-a"

Suggested fix: skip the purge when the incoming meta.RobotID equals the key being deleted (identity unchanged), rather than keying the decision on existed. Rotating a token for the same Bot is already handled correctly; only the fresh-profile case is wrong.

P1-1 — `--verbose` prints the PKCE verifier and device code in cleartext

cmd/mail_auth.go:181-188

raw, err := cli.Do(cmd.Context(), &client.Request{
    Method: http.MethodPost, Path: mailTokenPath,
    Body: map[string]string{
        "deviceCode": pending.DeviceCode, "codeVerifier": pending.CodeVerifier,
    },

No SecretValues, so redactBodyForLog passes the body through untouched and internal/client/client.go:945 logs it. Observed:

$ octo-cli mail auth status --verbose
[octo] request body: {"codeVerifier":"VRr_TwCIF5kY0-Bol6BNfbUwWcmAdEDFo6U6Pus2ajk","deviceCode":"DEV-SECRET-123"}

Those two values are the complete proof material for the token exchange, and by design that endpoint is credential-free (newMailAuthorizationClient, cmd/mail_auth.go:232-244). Anyone holding this trace can redeem an approved authorization and obtain the mailbox token. This CLI is explicitly built to be driven by agent runtimes, which routinely capture subprocess stderr into transcripts and logs, so "it is only --verbose" is a weaker mitigation here than usual. Note the store deliberately encrypts these same two fields at rest (internal/authstore/mail.go:26-32) — the in-flight trace undoes that.

Add pending.DeviceCode and pending.CodeVerifier to SecretValues; the same applies to codeChallenge at cmd/mail_auth.go:104 if you consider it sensitive, though the challenge alone is not redeemable.

P2-1 — `--dry-run` is unusable for the whole `mail` domain in a token-only runtime, with a misleading error

Identity resolution happens before dry-run (correct per the architecture note), but it resolves by making a request through a client whose DryRun is set. internal/cmdutil/factory.go:381-408 therefore parses the dry-run description instead of a register response, finds no robot_id, and fails:

$ OCTO_BOT_TOKEN=app_… octo-cli mail message list --dry-run
"code": "INVALID_BOT_IDENTITY_RESPONSE"
"message": "OCTO did not return the current Bot id"
"hint": "check the Bot token and OCTO API endpoint"

The token and endpoint are both fine, so the hint sends the operator down the wrong path. Affects any runtime with no stored profile and no OCTO_BOT_ID. Either perform the register call on a non-dry-run client, or short-circuit identity resolution under --dry-run with an explicit placeholder. No test covers --dry-run on a mail operation.

P2-2 — `mail auth login` Space hint recommends a variable that is ignored for stored profiles

cmd/mail_auth.go:84-89 requires bot.SpaceID and advises "pass --space <space_id> or set OCTO_SPACE_ID for the active Bot". FileProvider.Resolve populates SpaceID from meta.SpaceID only, and buildCredential overrides it solely from the --space flag — OCTO_SPACE_ID is read exclusively by EnvProvider. Confirmed: with a profile stored without a space, OCTO_SPACE_ID=space-1 octo-cli mail auth login still fails with the same message, while --space space-1 succeeds. skills/octo-mail/SKILL.md:123 repeats the same advice, so an agent following the skill will loop on a suggestion that cannot work. Either honour OCTO_SPACE_ID for stored profiles or narrow the hint (and the skill text) to --space / auth login --space.

P2-3 — One-time confirmation tokens are not marked `x-octo-secret`

internal/registry/specs/mail.json:49 and the other four X-Octo-Confirmation declarations (lines 109, 178, and the drafts send/delete entries) omit x-octo-secret, so collectSecrets does not pick them up:

$ octo-cli mail message send --dry-run --confirmation-token ONE-TIME-SECRET-TOKEN …
"headers": { "Authorization": "Bearer ***", "X-Octo-Confirmation": "ONE-TIME-SECRET-TOKEN" }

The Authorization value beside it is masked. Impact is limited — skills/octo-mail/SKILL.md:56-58 documents the token as one-time, short-lived, and bound to both the active mailbox credential and the exact request, so it is not usable on its own, which is why I am rating this P2 rather than higher. But x-octo-secret exists for exactly this class of capability token, and marking it also covers the error-redaction path, not just dry-run. X-Octo-Idempotency-Key is genuinely not a secret and should stay as it is.

P2-4 — Credential-revocation writes are ordered so a mid-way failure strands the secret

internal/authstore/authstore.go:254-263: saveProfiles (which removes the profile) commits first; saveMailTokens commits last. A failure at saveTokens or saveMailTokens leaves the mailbox secret on disk with no profile left to target for a retry, while an environment credential for that Bot can still use it. SaveProfile has the mirror-image problem (internal/authstore/authstore.go:191): mail tokens are purged before the new profile is committed, so a later failure loses the authorization and gains nothing. On a revoke path, delete the secret first — that is the fail-safe direction.

Nits

cmd/mail_auth.go:102-105 never sends code_challenge_method. The server must infer S256. A plain-defaulting server would fail closed rather than open, so this is interop hygiene, not a hole — but stating the method is cheap.
storedMailCredentialForBot (cmd/mail_auth.go:299) and storedMailCredential (internal/cmdutil/factory.go:265) implement the same RobotID→profile fallback twice, with different shapes. One helper would keep them from drifting.
The success envelope for a mail operation echoes the Bot identity only (internal/cmdutil/factory.go:463); the mailbox actually used is not reported. For a surface where identity is the whole point, echoing mailbox_address would make misrouting visible.
cmd/mail_jmap.go:120,142 set Credential: "mail" on requests already issued through the mail client. Harmless, but it reads as if the transport resolves credentials, which the comment at internal/client/client.go:54-58 says it does not.

3. Overall verdict

CHANGES_REQUESTED

Spec ❌ and Quality Changes-Requested. P0-1 and P0-2 are the blockers; P1-1 should land with them.

4. Suggested direction

Verify Bot identity before releasing a mailbox credential, or drop the verification claim from CLAUDE.md and document the trust assumption.
Make the mail-credential purge in SaveProfile conditional on the Bot identity actually changing, and add the profile-name-equals-RobotID test above.
Add the device code and code verifier to SecretValues.
Fix --dry-run for mail, and add a test for it.
Correct the Space hint in both the command and the skill.
Reorder the revoke-path writes so the secret is removed first.

5. Points for manual human verification

Flagged because this touches authorization and because the PR states live end-to-end authorization was not exercised.

The device and token endpoints are called with no credential at all (cmd/mail_auth.go:232-244), by design. That means the backend cannot authenticate the caller of mail auth login — anyone who knows a botId and spaceId can start a device authorization for someone else's Bot. The owner-approval screen is the only gate. Please confirm that screen shows enough (which client, which Bot, which mailbox) for an owner to recognise and reject an authorization they did not initiate, and consider whether rate limiting is needed on /agent-auth/device.
The /agent-mail-api gateway must admit those two public bootstrap paths while stripping unrelated credentials (octo-web#1315). That behaviour is outside this repo and is not covered by any test here.
Worth a live run before merge: the full mail auth login → owner approval → mail auth status flow against a real backend, and one controlled send-intent in each outbound mode.

6. Additional observations

Concurrency: loadMailTokens → mutate → saveMailTokens is a read-modify-write over a whole-file map. atomicWrite makes each write atomic, but two CLI processes authorizing different Bots concurrently under one OCTO_CONFIG_DIR can lose one update. Pre-existing pattern for the Bot store; the new files inherit it. Not blocking.
Review coverage, for transparency: I read every changed file and exercised the built binary against a stub server. Areas I did not independently verify: the wire contract against a real Agent Mail backend, the gateway credential-stripping behaviour, the mail.json request/response schemas against the backend's actual DTOs, and the binary attachment/raw download paths beyond the existing unit tests.

mochashanyao

[Octo-Q · automated review]

Verdict: Approve — no blocking findings; notes below (data-flow traced).

Code Review — PR #129 (octo-cli)

Reviewer: Octo-Q (automated review)

Summary

This PR adds the Bot-bound Agent Mail domain to octo-cli: a mail.json spec (24 operations) routed through the metadata-driven engine on a dedicated x-octo-credential: mail boundary, an OAuth-style device-flow authorization (mail auth login|status) with PKCE that binds a mailbox token to the server-verified RobotID, encrypted storage for mail credentials and pending device state with profile-lifecycle cascades, hand-written RFC 8621 message state|changes polling leaves, retry-never + RESULT_UNKNOWN semantics for every non-idempotent write, and an agent-facing octo-mail skill. The security architecture is careful: bind-time identity is verified against /v1/bot/register, claimed-vs-resolved mismatches are rejected, and profile reuse/removal cascades scrub mailbox access. No P0/P1 issues found; six P2 items below (0 P1).

Verification

Static analysis only at head 3815eec958af7653367ee971ab6462fff838857b (merge-base 16949d9d); build and tests not executed in this environment.

✅ Auth gate nesting — walked the skipValidation parent chain for mail auth login|status: the case "auth" branch only exempts top-level octo-cli auth (c.Parent().Parent()==nil); nested leaves fall through to root octo-cli (no match) → validation enforced (cmd/root.go:103-114). The mail auth parent's annotation only covers the bare-help path.
✅ Bot identity binding — VerifyBotIdentity always calls /v1/bot/register and rejects claimed≠resolved RobotID before device authorization (internal/cmdutil/factory.go:377-423); token response re-checked against the verified RobotID (cmd/mail_auth.go:211-213).
✅ Credential boundary wiring — both engine send paths (emitOnce/runPaginated) select the client via ClientForCredential(req.Credential); all 24 mail ops declare credential=mail; the mail token is the only Authorization sent, X-Space-Id suppressed per spec.
✅ Side-effect safety — all 12 write ops carry x-octo-retry: never → DisableRetry + UnknownOutcomeOnNetworkFailure; retryableErr embeds/unwraps to *ExitError so network failures map to RESULT_UNKNOWN (internal/client/client.go:864-871,1178-1186,1208-1223).
✅ Store lifecycle parity — SaveProfile/RemoveProfile both cascade mail + pending entries under name and previous-RobotID keys (internal/authstore/authstore.go:172-197,237-263); files are sealed with the existing authenticated-encryption path at secPerm.
✅ Docs-contract flags — every flag in skills/octo-mail/SKILL.md and README examples verified against the spec: --confirmation-token, --idempotency-key, --draft-version, --mailbox, --search, --unread, --output (binary ops), camelCase --addKeywords/--removeKeywords.
✅ Test paths use the real command tree — mail auth/JMAP/engine tests drive execRoot/RegisterServiceCommands with registry.MustNew and httptest servers asserting headers (no hand-fed bypass), including spoofed-RobotID rejection and exactly-one-request-on-503.

Findings

No P0/P1 issues; six P2 items below.

P2 — Mail credential survives API base-URL change (`internal/authstore/authstore.go:172`)

SaveProfile clears mail credentials only when the RobotID changes, and auth update --api-base-url (UpdateProfileAPIBaseURL, :208) touches neither. Switching a profile to another endpoint while keeping the same RobotID leaves the old endpoint's omb_ token in place, so subsequent mail commands fail with raw auth errors until a manual re-login. Consider also clearing mail credentials (or at least pending authorizations) when APIBaseURL changes.

P2 — JMAP/spec mail commands lack the 401 cleanup that `mail auth status` has (`cmd/mail_jmap.go:115`)

showCurrentMailConnection removes the local credential on a 401 and reports unconnected, but message state|changes and every spec-generated mail op return the raw auth error and leave the revoked token in the store. The user stays in a broken loop until they happen to run mail auth status. Apply the same revoked-credential handling (clear + actionable re-login hint) in the shared mail client path.

P2 — Confirmation token not marked `x-octo-secret` (`internal/registry/specs/mail.json:49`)

The one-time X-Octo-Confirmation header (seven write ops: lines 49, 109, 178, 200, 222, 368, 404) lacks "x-octo-secret": true, so --verbose/--dry-run print the live token in cleartext stderr traces — collectSecrets (cmd/service/run.go) only masks header flags marked secret. The mechanism and precedent exist (drive.json marks share tokens). Add the marker to each occurrence.

P2 — Nested-auth gate branch has no regression test (`cmd/root.go:108`)

The new case "auth" branch distinguishing top-level auth (credential-free) from nested mail auth login|status (gated) is untested: TestSkipValidation (cmd/cmd_test.go:442) was not extended and the mail-auth tests always run with a valid credential. A future revert to the flat allowlist would silently exempt mail auth login from the gate with no failing test. Assert skipValidation false for mail auth login and true for top-level auth.

P2 — CLAUDE.md command tree omits the hand-written mail leaves (`CLAUDE.md:91`)

The new octo-cli mail tree lists only spec-generated verbs; message state|changes (the JMAP polling primitives the octo-mail skill depends on) and message attachment download are missing. Add them to the message line.

P2 — Crash window between credential save and pending cleanup (`cmd/mail_auth.go:200`)

The status flow saves the token before removing the pending authorization. If the process dies between the two writes, the next mail auth status replays the used device code, hits authorization_used, and errors once even though the Bot is already connected (self-heals on the following run). Treat authorization_used as a re-check of stored connection state, or clear pending first.

Human-verify

Server contract: Email/get with an empty ids list must return state without error (the state command relies on the RFC 8621 reading of [] ≠ null). Cross-repo — not a merge blocker for this PR.
Server-side device-flow guarantees (device-code one-time use/expiry, botId binding at token issuance) are enforced by agent-mail-api, outside this checkout — not a merge blocker.
Mail-token revocation propagates only at the next CLI call (per-command 401); confirm OCTO Web revocation timing expectations — not a merge blocker.

Things I checked that are fine

Device/token bootstrap endpoints are deliberately public PKCE: verifier is encrypted at rest, human approval gates issuance, and the returned botId is cross-checked against the server-verified RobotID — a stolen deviceCode without the local verifier cannot be exchanged.
Empty-store + OCTO_BOT_ID fall-through in FileProvider (internal/credential/file_provider.go:40-53) fails closed when the store is non-empty and no profile matches.
NewMail strips SpaceID/RobotID/BotKind from the transport credential, so mail requests can't leak Bot space context or be selected by the Bot provider chain.
Runtime mail-credential selection by claimed RobotID (ResolveBotIdentity, verify=false) only chooses among locally stored tokens of the same OS user; the mail token itself is the server-side authorization, and the bind path remains server-verified.
Skill doc state machine (authorization_required/pending/unconnected/connected) matches emitted CLI envelopes; hasMoreChanges continuation from newState is documented and the CLI passes JMAP responses through unmodified.

Verdict: COMMENT

No correctness, security, or build-breaking issues; the authorization and credential-boundary design is sound and well tested. All six P2 items are non-blocking hardening/consistency fixes.

数据流回溯（被消费数据 → 上游来源 → 是否真流到消费点）

req.Credential（cmd/service/run.go:70）← OperationDetail.Credential ← buildDetail 读 spec 顶层 x-octo-credential（internal/registry/loader.go:401）← mail.json = "mail"。真流到：emitOnce/runPaginated 两条发送路径都改用 f.ClientForCredential（cmd/service/run.go:832、:897），测试断言 bearer = mail token。✅
bot.RobotID（device body / 存储 key）← VerifyBotIdentity → /v1/bot/register 服务端权威解析（factory.go:377-423），claimed≠resolved 拒绝（:414），伪造场景有测试。✅ 运行期命令走 verify=false 分支信任本地声明（见额外发现 1）。
pending.DeviceCode/CodeVerifier ← login 阶段 device 响应 → 加密落盘（authstore/mail.go:165-178）；status 用同一 mailStorageKeys（[RobotID, Profile]）取回（mail_auth.go:299-311），两端 key 一致。✅
token.AccessToken/BotID ← token endpoint；消费前强制 omb_ 前缀 + mailbox 非空 + token.BotID == bot.RobotID（mail_auth.go:208-214）。✅
JMAP accountId ← session primaryAccounts[urn:ietf:params:jmap:mail]，空则 JMAP_MAIL_UNAVAILABLE fail-closed（mail_jmap.go:124-127）。✅
state/changes 游标 ← 服务端响应原样透传 newState/hasMoreChanges（mail_jmap.go:104-113），SKILL.md 要求 hasMoreChanges 时从 newState 续页；CLI 不自持游标，无跳页面。✅
mail credential 查找 ← RobotID key 优先 → legacy profile key 兜底（factory.go:264-281）；SaveProfile/RemoveProfile 双 key 级联（authstore.go:172-197,237-263）。✅
空 store + OCTO_BOT_ID 回落：Count()==0 → 回落 env；非空且无匹配 → fail-closed（file_provider.go:40-53）。✅
RESULT_UNKNOWN：retryableErr 内嵌 *ExitError 且有 Unwrap()，errors.As 可达，仅 network 类改写（client.go:1178-1186,1208-1223）。✅

盲点 checklist（security_sensitive 全项）

C1 双路径 parity — SaveProfile↔RemoveProfile 级联对称 ✅；login(存 pending)↔status(消费/清理) 对称 ✅；skipValidation 顶层 auth ↔ 嵌套 mail auth 逐层推演正确 ✅（但无回归测试 → P2）；emitOnce↔runPaginated 同步切换 ClientForCredential ✅。
C2 control-flow / 嵌套复用 — resolveBotIdentity verify=true/false 两用无顺序错位（verified 标志 + f.cred 回写）✅；输入边界试穿：--mailbox（>320/多@）、--since-state 必填、--max-changes>0、空 key 拒绝 ✅。
C3 授权边界 — 邮箱能力由 omb_ token 服务端 scope；device bootstrap 公开但 PKCE+人工审批+botId 交叉校验；mail auth login 未豁免 validation ✅。
C4 容器级联 — N/A：CLI 无容器/成员层级；Space 由 login 强制 + 服务端审批。
C5 build≠运行期 — 未执行 build/test（静态）；命令树命名/注册顺序（root.go:58-59 先引擎后 attach）、binary --output、docs flag 映射均逐项代码推演 ✅。
C6 文档自洽 — SKILL.md 安全语义与实现一致；CLAUDE.md 计数 158+24=182 与 loader_test 一致；命令树漏 state/changes（→ P2）。

跨轮 blocker 复检（R6）

N/A — 本 issue 无历史 comment，首次审查该 head。

额外发现（非定级观察）

运行期信任不对称：bind 路径经服务端验证 RobotID；运行期邮件命令在 profile/OCTO_BOT_ID 已带 RobotID 时信任本地声明做 key 选择（有意设计，mail token 自身即授权），建议在 CLAUDE.md/SKILL.md 明示以免误读。
Email/get ids:[] 依赖服务端 RFC 8621 空列表语义（见 Human-verify 1）。
space-scoped bot 本地无 SpaceID 时 login 被本地校验拒绝并给出 --space 提示（TestMailAuthRequiresSpaceContext 锚定为有意行为），可自救不阻塞。

[Octo-Q] verdict: APPROVE — 无 P0/P1（按 R1–R4 定级）；6 个 P2 均为非阻塞加固/一致性项（confirmation token 脱敏、base-URL 变更级联、JMAP 401 清理、嵌套 auth 回归测试、CLAUDE.md 树补全、pending 清理时序）。授权/凭据边界设计经数据流回溯与 C1–C6 全项验证，建议终审放行并附上述 P2。

yujiawei · 2026-08-11T17:28:56Z

Follow-up to the review above — one correction and five additional P2 items

A second pass over this head turned up items I missed, plus an error in my own review that I want to correct on the record. The verdict is unchanged (CHANGES_REQUESTED); nothing here is a new blocker.

Correction to my P2-3

I wrote "mail.json:49 and the other four X-Octo-Confirmation declarations". That count is wrong — there are seven, at lines 49, 109, 178, 200, 222, 368, 404. All seven need "x-octo-secret": true, not the five I named. drive.json already marks share tokens this way, so the precedent and mechanism exist.

Additional P2 items

P2-5 — auth update --api-base-url leaves a mailbox token bound to the old endpoint. UpdateProfileAPIBaseURL (internal/authstore/authstore.go:208) touches only profile metadata, and SaveProfile's purge is keyed on RobotID, so repointing a profile at a different origin keeps the previous origin's omb_ token. Subsequent mail commands then fail with a raw auth error until the operator works out that a re-login is needed. Consider clearing at least the pending authorization when APIBaseURL changes.

P2-6 — the 401 self-heal exists only in mail auth status. showCurrentMailConnection (cmd/mail_auth.go:274-283) clears the local credential on an auth error and reports unconnected with a re-login hint. No other mail path does: message state|changes (cmd/mail_jmap.go) and every spec-generated op surface the raw auth error and leave the revoked token in the store, so an agent stays in a broken loop until it happens to run mail auth status. Worth applying the same handling on the shared mail client path — this is the failure mode agents will actually hit after a revocation in OCTO Web.

P2-7 — the new nested-auth gate branch has no test, in either direction. cmd/root.go:106-113 now distinguishes credential-free top-level auth from gated nested mail auth login|status. TestSkipValidation (cmd/cmd_test.go:~460-482) covers version, schema, config, config show and a service leaf — not auth and not mail auth. I verified the branch behaves correctly by running it: with no credential in the environment, mail auth login returns UNAUTHORIZED, while auth list still works and reports count: 0. So the logic is right — but a future revert to the flat allowlist would silently exempt mail auth login from the credential gate with nothing going red. This repo's own guidance asks for a test in each direction on a security boundary; both directions are one-liners given the above.

P2-8 — CLAUDE.md command tree omits the hand-written mail leaves. The message line (CLAUDE.md:93-94) lists only spec-generated verbs. message state and message changes — the JMAP polling primitives skills/octo-mail/SKILL.md depends on — and message attachment download are all missing.

P2-9 — crash window between credential save and pending cleanup. cmd/mail_auth.go:212-217 saves the mailbox credential and then removes the pending authorization. A process death between the two writes makes the next mail auth status replay a spent device code and error once with authorization_used, even though the Bot is already connected. It self-heals on the following run, because that error path clears the pending entry (cmd/mail_auth.go:199-202) — so this is cosmetic rather than a stuck state. Clearing pending first, or treating authorization_used as a prompt to re-check stored connection state, would remove the spurious error.

On the two blockers, restated after re-checking

Both still stand, and I re-verified the second one because it is the kind of finding that is easy to get wrong in either direction.

internal/authstore/authstore.go:184 reads if !existed || previous.RobotID != meta.RobotID. It is tempting to summarise that as "clears mail credentials only when the RobotID changes" — but the !existed disjunct means a brand-new profile always purges, regardless of identity. Since auth login --bot-id X with no --profile names the profile X, delete(mailTokens, name) deletes the very key the credential lives under. That is P0-2, and the failing test in my review above is the direct evidence.

On P0-1, the fair counter-argument is that the credential store is same-OS-user scoped, so a caller who can set OCTO_BOT_ID could read mail-credentials.enc anyway — which is true, and it is why I noted that the documented isolation boundary still holds. I am keeping it as a blocker for a narrower reason: CLAUDE.md:32, added by this PR, states that OCTO_BOT_ID "is verified against that result", and on every mail command other than auth login|status it is not — I observed zero /v1/bot/register requests. Whichever way you resolve it, the code and the documentation added in the same change need to agree.

Similarly on P1-1: the at-rest protection for the device code and verifier is real, and a stolen device code alone is indeed not redeemable without the verifier. The problem is that --verbose prints both values in the same line, so that mitigation does not apply to the trace.

Jerry-Xin

Re-review of head 603455b (rebased onto main incl. #123 Fleet loop API). Isolated the true PR delta with git diff 6a943ac 603455b (the mail commit vs its new parent); the large compare list is merge-base drift from #123, not this PR. Verdict: APPROVE — the security model is unchanged and the delta is clean.

Delta since the previously-approved head (`3815eec` → `603455b`)

The branch was rebased (no longer a merge commit). The mail commit itself is coherent and adds only ~54 lines to the transport. Concrete mail-relevant changes vs the prior approved mail commit:

cmd/mail_auth.go: adds pending.DeviceCode/pending.CodeVerifier to SecretValues on the status poll (redacts them from --verbose/--dry-run output) — a hardening, not a regression; passes Verbose/NoRetry/Timeout globals through to the mail client; a clearer --space hint for profile vs active Bot.
internal/client/client.go: NewMail builds a transport credential carrying only the mailbox token + source; DisableRetry + UnknownOutcomeOnNetworkFailure/markResultUnknown give send-mail a RESULT_UNKNOWN on lost responses ("do not retry automatically"). Good idempotency-safety design.
internal/credential/env_provider.go / internal/cmdutil/factory.go: OCTO_BOT_ID is resolved into RobotID and used purely as a local mail-store lookup key.

Security model — re-verified, not regressed

Mail HTTP calls authenticate only via the mailbox token. NewMail (internal/client/client.go) puts just cred.Token on the wire; the Bot token is never sent on a mail call. Comment is explicit: mail credentials cannot be selected by the Bot credential provider or echoed as a Bot kind.
SaveMailCredential remains the sole write path and is still gated: newMailAuthStatusCmd resolves identity via VerifyBotIdentity (verify=true → always hits /v1/bot/register to get the authoritative RobotID), rejects on token.BotID != bot.RobotID (cmd/mail_auth.go:214), and only then writes store.SaveMailCredential(bot.RobotID, ...) (:217). internal/authstore/mail.go is byte-identical to the approved head.
Read-time ResolveBotIdentity (verify=false) trusts an existing profile's RobotID solely as a store lookup key; a mailbox entry could only have been created under a verified identity. This is a local-credential-store trust boundary, not cross-Bot privilege escalation.

Confirmed: go build ./... clean, go test ./... all packages pass, go vet ./... clean.

Non-blocking

CHANGELOG still missing the Agent Mail entry. CHANGELOG.md [Unreleased]/Added documents the loop and drive domains but has no octo-cli mail / Agent Mail line. Same note as the prior review; not yet addressed.
check-sprint CI is red for a non-code reason — "Linked issue has no Sprint set" (board metadata). Needs a maintainer to assign the linked issue to the current sprint on the Octo Board; unrelated to code. Build/tests/vet are green.

Highlights

Clean credential-boundary separation (NewMail / ClientForCredential) keeps mail and Bot auth from leaking into each other.
Idempotency handling for lossy send-mail responses (RESULT_UNKNOWN) is a thoughtful touch.
Secret-masking coverage extended to device-flow secrets.

mochashanyao

[Octo-Q · automated review]

Verdict: Approve — no blocking findings; notes below (data-flow traced).

Code Review — PR #129 (octo-cli)

Reviewer: Octo-Q (automated review)
Head: 603455b0e43db1bee48705cd3e942fcab9945c1a · Base: main (merge-base 6a943ac)

Summary

This PR adds the Agent Mail domain to octo-cli: a Bot-bound mailbox credential with a PKCE device-authorization flow (mail auth login/status), a dedicated encrypted credential file (mail-credentials.enc / mail-authorization.enc), 24 registry-driven WebAPI operations plus hand-written RFC 8621 mail message state/changes leaves, a mail credential boundary in the factory/transport (ClientForCredential, NewMail), per-request DisableRetry/RESULT_UNKNOWN semantics for non-idempotent sends, an auth update command, and OCTO_BOT_ID propagation through the credential chain. The design is careful: identity is bound to the stable RobotID rather than profile names, profile identity changes and removals cascade to mail credentials, and the skip-validation gate was tightened so nested mail auth requires a Bot credential while top-level auth stays credential-free. One P2 validation asymmetry found; no blocking issues.

Verification

Local checks at head 603455b0:

go vet ./cmd/... ./internal/... — clean
go test ./cmd/... ./internal/... ./skills/... -count=1 — all packages pass (incl. new cmd/service, internal/authstore, internal/client, internal/cmdutil suites)

Also traced end-to-end from changed code: device-flow state machine, credential storage round-trip, registry spec → runOperation → mail-client wiring, and verbose-mode secret masking.

Findings

No P0/P1 issues; one P2 below.

P2 — `auth update` skips the base-URL validation that `auth login` applies (`cmd/auth.go:67`)

newAuthUpdateCmd trims --api-base-url and persists it via UpdateProfileAPIBaseURL without config.NormalizeAPIBaseURL, while the paired login path normalizes first (cmd/auth.go:145). An invalid value (service path suffix, non-http(s) scheme, embedded credentials/query/fragment) is silently written to config.json; every later command for that profile then fails in overlayProfileBaseURL (internal/cmdutil/factory.go:359) with an error blaming OCTO_API_BASE_URL even when that env var is unset. Fail-closed — no request goes out on a malformed base — so this is a contract asymmetry, not an exposure. Fix: normalize in newAuthUpdateCmd before persisting, mirroring login, and extend TestAuth_UpdateAPIBaseURLPreservesCredentials with a rejected value.

Human-verify

Server-side binding of the device flow: the CLI verifies token.botId == bot.RobotID client-side and checks the claim against /v1/bot/register, but the agent-mail authorization service's own enforcement (device-code single use, Bot/mailbox ownership at approval time) is not visible from this repo. Not a merge blocker for this PR.
ResolveBotIdentity calls the pre-existing write-risk bootstrap POST /v1/bot/register on the first mail command for token-only runtimes; confirm server-side this is an idempotent authenticate with no session-rotating side effects. Not a merge blocker for this PR.
mail message state issues Email/get with an empty ids array to fetch state; confirm the octo-mail JMAP endpoint treats empty ids per RFC 8621 (empty list + state). Not a merge blocker for this PR.

Things I checked that are fine

Credential round-trip: token exchange validates omb_ prefix and Bot id, stores under RobotID; reads try RobotID then legacy profile key; SaveProfile identity-change and RemoveProfile cascade delete both name- and RobotID-keyed mail/pending entries (internal/authstore/authstore.go:172-268) — no stale mailbox inheritance across Bot identities.
Device/token bootstrap endpoints send no Authorization header; deviceCode/codeVerifier are in SecretValues; expired/used/denied outcomes purge pending state.
Retry safety: every send/delete/draft-mutation op declares x-octo-retry: never → DisableRetry + RESULT_UNKNOWN on network failure (internal/client/client.go:879-883, :1269-1284); message flag keeps default retry safely because addKeywords/removeKeywords are set-based idempotent mutations.
Mail requests never carry Bot space context (converted credential has no space + x-octo-space-header:false), asserted by TestMailCommandUsesMailEndpointAndCredential.
Verbose masking: unknown omb_ prefix masks to *** (internal/credential/token.go:45); confirmation header flags are collected by collectSecrets (cmd/service/run.go:317-321); the relaxed api tripwire is sound because octo-cli api has no arbitrary-header input.
skipValidation (cmd/root.go:137-145): nested mail auth passes the credential gate; deeper nesting fails toward authentication, not around it.
Spec/docs/tests parity: 24 operations pinned in internal/registry/loader_test.go, SKILL.md command shapes match spec flags, CLAUDE.md counts updated.

Verdict: APPROVED

The credential lifecycle, identity binding, retry/unknown-outcome semantics, and secret handling are all implemented and tested through their production paths. The single P2 (base-URL validation asymmetry in auth update) is fail-closed and non-blocking.

Appendix A — Severity rubric & diff-scope (per-finding)

P2 cmd/auth.go:67: (1) new — introduced by this PR's new auth update command; (2) the PR adds the path, so not pre-existing; (3) R1 check: does not make a previously-working path unusable, produces no incorrect user data, fails closed with a recoverable (if misleading) error → P2, not P1. Verdict mapping (R4): no P0/P1 → APPROVED.
Considered and cleared at P-level: concurrent load-modify-write on mail-credentials.enc (pre-existing pattern shared with credentials.enc, not changed by this PR — note only); mailStorageKeys legacy fallback (covered by SaveProfile cascade; within documented same-user trust boundary).

Appendix B — Data-flow traceback (consumed data → upstream source → verified at consumption point)

bot.RobotID (login body, storage key) ← VerifyBotIdentity → /v1/bot/register response robot_id/data.robot_id, cross-checked against profile claim, cached in f.cred → flows to SaveMailCredential/SavePendingMailAuthorization. Verified non-empty enforced at both store APIs.
token.AccessToken ← token endpoint response, gated on omb_ prefix + non-empty mailbox + BotID match before SaveMailCredential. Verified.
Mail token at request time ← MailCredentialFunc → storedMailCredential(store, bot) (RobotID key, legacy profile fallback) → client.NewMail → Authorization: Bearer. Verified by httptest assertions.
d.Credential/d.RetryMode ← buildDetail reading x-octo-credential (spec top-level) / x-octo-retry (op-level) in mail.json → runOperation → ClientForCredential / DisableRetry. Verified by registry + service tests.
JMAP accountID ← GET /jmap/session primaryAccounts[urn:ietf:params:jmap:mail], empty → explicit JMAP_MAIL_UNAVAILABLE. Verified.
PendingMailAuthorization ← device response (DeviceCode, CodeVerifier locally generated, ExpiresIn→RFC3339) → encrypted file → status poll; purged on terminal outcomes. Verified.

Appendix C — Blind-spot checklist (C1–C6)

C1 dual-path parity: clear — SaveProfile↔RemoveProfile cascades both handle mail+pending under name and RobotID keys; SaveMailCredential↔RemoveMailCredential, SavePendingMailAuthorization↔RemovePendingMailAuthorization symmetric. One asymmetry found: auth login (normalizes URL) vs auth update (does not) → the P2 above.
C2 control-flow ordering / reuse: clear — resolveBotIdentity serves both Resolve (trust claimed id) and Verify (always check) entry points; botIdentityVerified cache ordering-safe (Verify first → Resolve skips; Resolve with claimed id first → Verify still re-checks). No nested double-application found.
C3 authorization boundary: clear — device/token endpoints intentionally public bootstrap; Bot identity authenticated via /v1/bot/register before request construction; mail data path uses only the mailbox token; capability selection (ClientForCredential) cannot be reached with a Bot token for mail ops.
C4 lifecycle/container cascade: clear — profile removal and identity change cascade to mail credentials; UpdateProfileAPIBaseURL touches metadata only by design.
C5 build≠runtime: clear — command wiring exercised through RegisterServiceCommands + real registry in httptest-backed tests (not hand-fed unit seams); attachMail* ordering after registry registration confirmed in cmd/root.go:66-67.
C6 docs/governance coherence: clear — skills/octo-mail/SKILL.md, README.md, CLAUDE.md, npm/README.md consistent with spec contracts (confirmation flow, RESULT_UNKNOWN handling, state-machine wording); relaxed secrets tripwire keeps query-position and non-string protections and documents the header-input invariant.

Appendix D — Cross-round blocker recheck (R6)

N/A — first review round for this PR; no prior blockers to re-verify.

yujiawei

Code Review — PR #129 (octo-cli)

Re-review at head 603455b0e43db1bee48705cd3e942fcab9945c1a against its merge-base 6a943accb3f850349bf56d3e4180c97a26200c77 (35 files, +3317/−39). The branch was rebased onto a much newer main, so a naive head-to-head compare with the previously reviewed 3815eec9 shows unrelated drift (the Loop domain, config/base-URL work); everything below is scoped to the PR's own delta.

Verified locally at this head: go build ./... OK, go vet ./... OK, gofmt -l . clean, go test -race -shuffle=on -count=1 ./... OK (12/12 packages). golangci-lint is not installed in my environment, so I did not re-run it. Every behavioural claim below was checked by running the built binary against a stub OCTO / Agent-Mail server, not by reading alone; the commands and observed traffic are quoted.

Fixed since the previous round — verified, not just claimed

PKCE proof material no longer leaks to --verbose. cmd/mail_auth.go:190 now sets SecretValues. Observed: [octo] request body: {"codeVerifier":"***REDACTED***","deviceCode":"***REDACTED***"}, with the real device code appearing zero times in the trace. A regression test was added.
The default-named-profile purge regression is gone. internal/authstore/authstore.go:187-189. SaveMailCredential("bot-a") → SaveProfile("bot-a", {RobotID:"bot-a"}) now preserves the credential, and so does the friendly-name variant SaveProfile("agent", {RobotID:"bot-a"}). The existing test was corrected to use the production default naming instead of a name that made the assertion vacuous.
All seven X-Octo-Confirmation declarations carry x-octo-secret (internal/registry/specs/mail.json:49,109,178,200,222,368,404). Observed masked in both dry-run ("X-Octo-Confirmation": "***REDACTED***") and --verbose.
The mail auth status identity probe honours the global flags (cmd/mail_auth.go:274-280), with tests for --no-retry and --timeout.
The Space hint no longer recommends a variable that is ignored for stored profiles (cmd/mail_auth.go:84-91), and skills/octo-mail/SKILL.md:123 was corrected to match.
The nested-auth gate is now tested in both directions (cmd/cmd_test.go:477-482): top-level auth skips validation, mail auth login does not.
CLAUDE.md command tree lists the hand-written leaves (message state|changes, message attachment download).

That is a clean, well-targeted round of fixes. What follows is what is still open, plus findings that are new to this pass.

1. Specification compliance

Spec: ✅

Measured against the acceptance criteria in #128.

Missing: none. All listed command surfaces, auth update, the RFC 8621 state/changes pair, the embedded skill, and the documentation updates are present.
Over-build: none. No direct octo-mail base URL, no mailbox-token environment variable, owner confirmation is not bypassed, ambiguous side effects are not retried. The declared out-of-scope list is respected.
Deviations: none that I can sustain against the AC text.

Correction to my previous review, on the record. Last round I marked Spec: ❌ on two counts. One (secrets "excluded from output") is genuinely fixed. The other — that "scoped to the RobotID" requires verifying the claimed RobotID at use time — I now think was an over-reading on my part. The AC that speaks to identity checking is scoped to storage: "A claimed RobotID that does not match the authenticated Bot is rejected before a mailbox credential is stored", and that is satisfied (cmd/mail_auth.go:214 rejects token.BotID != bot.RobotID after VerifyBotIdentity). The Proposed Solution also says Mail operations "select the mailbox credential independently from the Bot credential". So the use-time behaviour is not an AC violation, and I should not have graded it as one.

It is still a defect, for reasons that stand on their own rather than on the spec. It moves to the quality section below, re-rated.

2. Code quality

Quality: Changes-Requested

P1-1 — An unverified `OCTO_BOT_ID` is now treated as identity: it unlocks a stored mailbox token and is echoed as `identity.robot_id`

Three changes in this PR combine here:

internal/credential/env_provider.go:81-82 — EnvProvider.Resolve now populates RobotID from OCTO_BOT_ID.
internal/credential/file_provider.go:45-56 — with an empty profile store and a --bot-id/OCTO_BOT_ID selector, resolution now falls through to the environment token instead of failing.
internal/cmdutil/factory.go:427 — resolveBotIdentity short-circuits whenever RobotID is non-empty and verify is false, and MailCredentialFunc (internal/cmdutil/factory.go:154) uses the non-strict path.

The env fallback itself is well-scoped and I think correct: with a non-empty store a non-matching --bot-id still fails closed (UNAUTHORIZED: no profile found for bot id ... — I checked). The problem is the two things the claimed id is now allowed to do without ever being checked.

(a) It releases another Bot's mailbox token. Reproduced at this head. Store contains only mail-credentials.enc with an entry for robot-VICTIM-B (no profiles — the env-token runtime shape):

OCTO_BOT_TOKEN=app_ANY_STRING OCTO_BOT_ID=robot-VICTIM-B \
  octo-cli mail message send --data '{"to":["x@example.com"],"subject":"s","text":"t"}'

Stub server log, complete:

POST /agent-mail-api/webapi/v0/messages auth=Bearer omb_MAILTOKEN_OF_B space=None body={"subject":"s","text":"t","to":["x@example.com"]}

Zero /v1/bot/register requests (grep -c on the server log returns 0). The Bot token was never validated against anything — app_ANY_STRING is not a real token — and the mail is sent from B's mailbox. The success envelope reports "robot_id": "robot-VICTIM-B".

(b) It is echoed as verified-looking identity on every command, in every domain. This one is not about mail at all, and I did not catch it last round. Same fabricated id, a non-mail operation, empty store:

$ OCTO_BOT_TOKEN=app_x OCTO_BOT_ID=totally-made-up-id octo-cli event list
{ "data": {...},
  "identity": { "bot_kind": "app_bot", "robot_id": "totally-made-up-id",
                "source": "env:OCTO_BOT_TOKEN", "type": "bot" }, "ok": true }

The same invocation on the merge-base binary:

{ "error": { "code": "UNAUTHORIZED",
             "message": "no profile found for bot id \"totally-made-up-id\"" }, "ok": false }

So identity.robot_id changed from "absent or profile-backed" to "whatever the caller put in the environment", for all 12 domains. The doc comment this PR deleted said exactly why that mattered — "Credentials resolved from the environment leave them empty — a raw env token carries no verifiable identity" — and CLAUDE.md:25, which this PR leaves in place, still tells readers the envelope's identity echo exists "so misuse is visible". It is now the mechanism by which misuse becomes invisible: the envelope confirms the caller's own claim. Anything downstream that records identity.robot_id as attribution (agent bookkeeping, task records, log pipelines) silently loses its guarantee.

Why I am rating this P1 rather than P2, having downgraded it from P0. It is not privilege escalation: the credential store is same-OS-user scoped with a machine-derived key, so anyone who can set OCTO_BOT_ID can already read mail-credentials.enc. The other review at this head reaches the same conclusion on that axis and I agree with it. But escalation is not the only axis:

The realistic trigger is misconfiguration, not an attacker — a stale or templated-wrong OCTO_BOT_ID in a multi-bot host sharing one OCTO_CONFIG_DIR, which is precisely the env-token runtime this feature is built for.
The consequence is an irreversible, externally visible side effect: email sent from another Bot's mailbox, with no error and a success envelope that confirms the wrong identity.
It breaks the codebase's own established pattern. Everywhere else, identity selection fails closed — a wrong --bot-id yields no profile found, as shown above. This is the first surface where a wrong id succeeds as a different identity, and it happens to be the surface with the least reversible effects.
The documentation added by this PR asserts the opposite twice: CLAUDE.md:35 ("an optional OCTO_BOT_ID is verified against that result" — true only for mail auth login|status), and internal/credential/provider.go:18-19 ("The server remains the authority that verifies whether a token belongs to the claimed RobotID" — on the mail path the server cannot do this, because the request carries only the mailbox token and no Bot token).

internal/cmdutil/factory_test.go:617 (TestFactory_MailCredentialSupportsRuntimeBotIdentity) asserts this behaviour with no stub server present, so it is a deliberate design choice rather than an oversight — which is why I am raising it as a decision to revisit, and why "just fix the code" may not be the right answer.

Any one of these resolves it, and I will not re-litigate the choice:

Track provenance on BotCredential (verified / from-profile / from-env-claim) and require verification before MailCredentialFunc releases a mailbox token. Only the env-claim path pays a /v1/bot/register round-trip; profile-backed ids already went through VerifyBotIdentity when the mailbox was bound. Then omit or mark robot_id in the envelope while it is an unverified claim.
Verify once per process on any mail command (VerifyBotIdentity in MailCredentialFunc), accepting one round-trip per mail invocation.
Keep the behaviour as designed, but make the documentation true: correct CLAUDE.md:35 and internal/credential/provider.go:18-19, note in CLAUDE.md:25 that robot_id is a caller claim for env credentials, and say plainly that a wrong OCTO_BOT_ID in an env-token runtime can address another Bot's mailbox. If you take this route, please also state it in skills/octo-mail/SKILL.md, since agents read that and not the Go doc comments.

I would accept (3). What I do not think can ship is the current state, where the code does one thing and two files added in the same change say it does another.

P2-1 — `--dry-run` is broken across the whole `mail` domain, and now for `mail auth` unconditionally

Identity resolution runs before dry-run (correct per the architecture note), but it resolves by making a request through a client whose DryRun is set, so internal/cmdutil/factory.go:419-460 parses the dry-run description instead of a register response. Observed at this head:

$ OCTO_BOT_TOKEN=app_x octo-cli mail message list --dry-run
$ OCTO_BOT_TOKEN=app_x octo-cli mail auth status --dry-run
$ OCTO_BOT_TOKEN=app_x OCTO_BOT_ID=robot-D OCTO_SPACE_ID=space-1 octo-cli mail auth login --dry-run
{ "error": { "code": "INVALID_BOT_IDENTITY_RESPONSE",
             "hint": "check the Bot token and OCTO API endpoint",
             "message": "OCTO did not return the current Bot id" }, "ok": false }

mail auth login|status --dry-run fail always, including when OCTO_BOT_ID is set, because they use the strict path. Generated mail operations fail whenever the id has to be resolved. The hint sends the operator after the token and the endpoint, both of which are fine. Since --dry-run is the documented safe-preview mechanism for an agent about to send mail, it is the one flag you most want working on this domain. Either issue the register call on a non-dry-run client, or short-circuit identity resolution under --dry-run with an explicit placeholder. No test covers --dry-run on any mail operation.

P2-2 — Creating a profile whose name equals another Bot's RobotID silently deletes that Bot's mailbox authorization

internal/authstore/authstore.go:188 — newProfileMayReuseLegacyName := !existed && name != meta.RobotID then delete(mailTokens, name). Because mailTokens keys both modern RobotIDs and legacy profile names in one namespace, a profile name can address another Bot's canonical entry. Reproduced:

save   bot-123 → omb_SECRET_OF_BOT123          # env-token runtime, no profile
profile bot-123 (RobotID bot-456)              # unrelated bot, name collides
get    bot-123 → mail credential not found for Bot key "bot-123"

Recovery requires re-running the full human-approval device flow. Realism is low — it takes deliberately naming a profile after a different Bot's id — which is why this is P2 and not higher, but the fix is narrow: purge only the alias key when it is not also somebody's canonical RobotID, or split the two namespaces. (The related rebind case, delete(mailTokens, previous.RobotID) at :191, I checked and do not consider a bug: after a rebind the old Bot has no profile and a non-empty store makes it unselectable, so the entry is genuinely dead.)

P2-3 — Mail raw/attachment downloads are read fully into memory, and the code comment that justified doing so no longer applies

internal/client/client.go:990 reads the whole body with io.ReadAll before writeFileAtomic (:1043). The comment immediately above it is explicit about the precondition: "Board PNG/SVG exports are bounded and small, so a size cap / streaming-to-temp path would add complexity without a real payoff today; deferred until an operation returns genuinely large bodies."

mail.message.raw (internal/registry/specs/mail.json:120) and mail.message.attachment.download (:153) are exactly that deferred case, and worse: the payload size is chosen by whoever emailed the agent. Anyone who can send mail to the mailbox picks the number of bytes the CLI allocates. The repo already has the streaming precedent for large transfers — cmd/drive.go:1062 uses io.Copy for drive file downloads. Suggest streaming to the temp file (keeping the atomic rename) for x-octo-binary-response operations, or capping declared-binary bodies.

To be fair to the design: with no -o the bytes are described and dropped rather than printed ({"status":200,"content_type":...,"size":...}), so there is no terminal-corruption or injection-into-stdout surface. Good call.

P2-4 — `-o` on a generated binary operation clobbers an existing file with no guard

internal/client/client.go:1043 writes unconditionally. Observed:

$ echo PRECIOUS-EXISTING-CONTENT > /tmp/precious.txt
$ octo-cli mail message attachment download MSG1 PART1 -o /tmp/precious.txt
$ cat /tmp/precious.txt   → attachment bytes; original gone

The hand-written drive download file refuses an existing destination unless --overwrite is passed (cmd/drive_download.go:37,48,62). The mechanism here is pre-existing in the generated engine, so this is not a defect the PR introduced — but this PR makes it reachable for the first time on content and filename suggestions supplied by an untrusted external sender, which is the combination that makes it worth raising now. An agent following an emailed instruction to "save the attached file to <path>" can silently destroy <path>. Consider honouring the same --overwrite contract for x-octo-binary-response operations.

P2-5 — The revoked-token self-heal exists only in `mail auth status`

cmd/mail_auth.go:284-293 clears the local credential on an auth error and reports unconnected with a re-login hint. No other mail path does. Observed with a mail server returning 401:

$ octo-cli mail message list
{ "error": { "code": "unauthorized", "message": "token revoked",
             "hint": "token invalid, revoked, or the user/bot is inactive; re-check the credential" } }
$ # credential still present in the store

$ octo-cli mail auth status
{ "data": { "status": "unconnected",
            "next": "The previous authorization was revoked. Run `octo-cli mail auth login`." } }
$ # credential now cleared

The generic hint never names mail auth login, so an agent whose authorization was revoked in OCTO Web has no signal that re-authorization is the fix and no reason to run mail auth status. This is the failure mode agents will actually hit in production. Applying the same handling on the shared mail client path — or just adding the re-login hint to auth errors on /agent-mail-api — would close the loop.

P2-6 — `auth update --api-base-url` leaves the mailbox token bound to the old origin

internal/authstore/authstore.go:212-225 touches only profile metadata, and SaveProfile's purge is keyed on RobotID, so repointing a profile at a different gateway keeps the previous origin's omb_ token. Subsequent mail commands fail with a raw auth error until someone works out that a re-login is needed. Clearing at least the pending authorization when APIBaseURL changes would make the failure self-describing.

P2-7 — `submissionIds` spec type still contradicts the repo's own test stub

internal/registry/specs/mail.json:476 and :501 declare submissionIds as an array of integer; cmd/service/mail_test.go:272 has the backend returning {"messageId":"E1","submissionIds":["S1"]} — strings. Response schemas are not validated locally so nothing breaks at runtime, but octo-cli schema and the embedded docs are wrong on one side or the other. This was raised in the previous round and is unaddressed. Worth confirming against the octo-mail DTO which one is real.

P2-8 — `CHANGELOG.md` has no Agent Mail entry

Untouched by this PR (git diff <merge-base>..HEAD -- CHANGELOG.md is empty; grep -i mail CHANGELOG.md returns nothing), while [Unreleased] → Added documents the loop and drive domains in detail. This PR adds a whole new domain plus auth update. Flagged in two separate reviews in the previous round and still open — worth clearing now rather than at release time.

P2-9 — Crash window between credential save and pending cleanup

cmd/mail_auth.go:217-222 saves the mailbox credential and then removes the pending authorization. A process death between the two writes makes the next mail auth status replay a spent device code and error once with authorization_used, even though the Bot is already connected. It self-heals on the following run (:206 clears the pending entry on that code), so this is cosmetic rather than a stuck state. Clearing pending first would remove the spurious error.

P2-10 — Revocation writes are ordered so a mid-way failure strands the secret

internal/authstore/authstore.go:259-269: saveProfiles (which removes the profile) commits first, saveMailTokens last. A failure in between leaves the mailbox secret on disk with no profile left to target for a retry. On a revoke path, delete the secret first — that is the fail-safe direction.

Nits

cmd/mail_auth.go:105-109 still never sends code_challenge_method. A plain-defaulting server would fail closed rather than open, so this is interop hygiene, not a hole — but stating S256 is one line.
storedMailCredentialForBot (cmd/mail_auth.go:309) and storedMailCredential (internal/cmdutil/factory.go:275) implement the same RobotID→profile fallback twice, in different shapes. One helper would keep them from drifting.
X-Octo-Idempotency-Key is documented as "8-200 character" (internal/registry/specs/mail.json:268,301,327) but carries no minLength/maxLength, so the pre-flight validator cannot enforce what the description promises.
The success envelope for a mail operation echoes the Bot identity only; the mailbox actually used is not reported. On a surface where identity is the whole point, echoing mailbox_address would make misrouting visible — and would have made P1-1(a) obvious at a glance.
cmd/mail_jmap.go:120,142 set Credential: "mail" and SuppressSpaceHeader: true on requests already issued through the mail client, where both are redundant. Harmless, but it reads as if the transport resolves credentials, which the comment at internal/client/client.go:57-60 says it does not.

3. Overall verdict

REQUEST_CHANGES

Spec ✅. Quality Changes-Requested on P1-1, which has a documentation-only resolution available if you disagree with the code change. Everything else is P2 or a nit and does not need to block.

4. Suggested direction

Pick one of the three resolutions under P1-1 and make code and documentation agree. If you keep the current behaviour, the CLAUDE.md:35 and internal/credential/provider.go:18-19 sentences both need correcting, and the envelope's robot_id semantics need to be stated.
Fix --dry-run for the mail domain and add one test for it.
Narrow the SaveProfile purge to the alias key, or split the alias and RobotID namespaces.
Stream or cap x-octo-binary-response bodies now that one of them is externally sized.
Add the re-login hint (or the same self-heal) to auth errors on the shared mail path.
Add the CHANGELOG.md entry and reconcile the submissionIds type.

5. Points for manual human verification

Flagged because this touches authorization, and because the PR states live end-to-end authorization was not exercised.

The device and token endpoints are called with no credential at all (cmd/mail_auth.go:237-249), by design. The backend therefore cannot authenticate the caller of mail auth login — anyone who knows a botId and spaceId can start a device authorization for someone else's Bot, and the owner-approval screen is the only gate. Please confirm that screen shows enough (which client, which Bot, which mailbox) for an owner to recognise and reject an authorization they did not initiate, and consider rate limiting /agent-auth/device.
The /agent-mail-api gateway must admit those two public bootstrap paths while stripping unrelated credentials (octo-web#1315). That behaviour lives outside this repo and no test here covers it.
Worth a live run before merge: the full mail auth login → owner approval → mail auth status flow against a real backend, and one controlled send-intent in each outbound mode.
The submissionIds type question (P2-7) needs someone who can read the octo-mail DTO.

6. Coverage, and where I disagree with the other review at this head

For transparency: I read every changed file, exercised the built binary against stub servers for each behavioural claim above, and diffed against a binary built at the merge-base to separate this PR's behaviour changes from pre-existing ones. Areas I did not independently verify: the wire contract against a real Agent Mail backend, the gateway's credential-stripping behaviour, the mail.json request/response schemas against the backend's actual DTOs, and golangci-lint (not installed here).

The other review at this head reaches APPROVE and addresses the read-time trust question directly, concluding it is "a local-credential-store trust boundary, not cross-Bot privilege escalation". I agree with that specific claim — the store is same-OS-user and a caller who can set OCTO_BOT_ID can read the credential file anyway. Where I differ is that the argument settles the escalation axis and not the correctness axis: it does not address the misconfiguration path, the loss of fail-closed identity selection, or the cross-domain identity.robot_id change, which is not a mail concern at all and which the merge-base comparison above shows is new. That reasoning ("a mailbox entry could only have been created under a verified identity") establishes that the key is authoritative; it does not establish that the presenter is the Bot that key belongs to. That gap is what P1-1 is about.

Also concurring with that review on two points: the NewMail / ClientForCredential credential-boundary separation is clean, the RESULT_UNKNOWN handling for lost send responses is genuinely thoughtful, and check-sprint being red is board metadata (the linked issue has no Sprint set), not a code failure — a maintainer action.

Round bookkeeping: this is the second review round on this PR (heads 3815eec9 → 603455b0). One previously blocking finding is fixed, one is unaddressed with a cheap resolution available. If a third round does not close P1-1 one way or the other, the right move is a decision from the repo owner on the trust model rather than another review pass.

yujiawei · 2026-08-12T07:25:59Z

Follow-up to the review above — one additional P2 I missed

A further pass over this head turned up one item my review did not cover. The verdict is unchanged (CHANGES_REQUESTED, on P1-1); this is not a new blocker.

P2-11 — `auth update --api-base-url` skips the validation its sibling `auth login` applies

newAuthUpdateCmd (cmd/auth.go:41-95) trims --api-base-url and persists it straight through UpdateProfileAPIBaseURL. newAuthLoginCmd runs the same value through config.NormalizeAPIBaseURL first (cmd/auth.go:144-149). auth update is new in this PR and writes the same field, so the new path is missing the guard the existing path has.

Observed at this head, with OCTO_API_BASE_URL unset:

$ octo-cli auth login --with-token --bot-id bot-y --api-base-url "http://host/fleet/api/v1"
{ "error": { "code": "VALIDATION_ERROR",
             "message": "OCTO_API_BASE_URL must not include a service or API path",
             "hint": "pass only the Octo gateway base URL, without a service path" } }

$ octo-cli auth update --bot-id bot-x --api-base-url "http://host/fleet/api/v1"
{ "data": { "api_base_url": "http://host/fleet/api/v1", "profile": "bot-x", "robot_id": "bot-x" }, "ok": true }

$ octo-cli auth update --bot-id bot-x --api-base-url "ftp://evil/"
{ "data": { "api_base_url": "ftp://evil/", ... }, "ok": true }     # written to config.json

$ octo-cli event list --bot-id bot-x
{ "error": { "code": "CLI_ERROR", "type": "config",
             "message": "profile \"bot-x\" has an invalid API base URL: OCTO_API_BASE_URL must use http or https; update it with `octo-cli auth login --profile bot-x`" } }

So auth update accepts a value auth login rejects, persists it to config.json, and every subsequent command for that profile then fails in overlayProfileBaseURL (internal/cmdutil/factory.go:359-368).

Rating it P2: it fails closed — no request is ever sent against the malformed base — and it is recoverable. Two smaller points inside it:

The error text names OCTO_API_BASE_URL, which is unset in the reproduction above. The wrapper does name the profile, so it is not fully misleading, but the inner clause points at the wrong knob.
The recovery hint says auth login --profile bot-x, which means re-entering the token. auth update is the command that can repair this without a token, and it is the one the hint should name.

Suggested fix: call config.NormalizeAPIBaseURL in newAuthUpdateCmd before persisting, mirroring login, and add a rejected-value case to TestAuth_UpdateAPIBaseURLPreservesCredentials.

Also confirmed while checking this

Three things I can now positively confirm rather than leave unstated: mail.message.flag deliberately keeps default retry and that is correct — addKeywords/removeKeywords are set-based and idempotent, so it does not belong in the twelve operations marked x-octo-retry: never. The registry test pins the domain at 24 operations (internal/registry/loader_test.go:51), matching the 24 operationId entries in mail.json. And mail message state issues Email/get with an empty ids array purely to read state, which is worth confirming against the octo-mail JMAP implementation before merge — an implementation that rejects an empty ids list would break that command, and no test here can catch it.

Jerry-Xin

Re-review of 7ad6aede9b08 (delta vs previously-reviewed 603455b: 1 commit fix(mail): verify runtime Bot identity, 6 files, no rebase drift). This addresses the prior CR. APPROVE.

Verification of prior CR items

1. Use-time RobotID verification — RESOLVED (real, narrow gap correctly closed).
internal/cmdutil/factory.go resolveBotIdentity previously short-circuited whenever bot.RobotID != "" && !verify, so a caller running with an environment token plus a claimed OCTO_BOT_ID could select a stored mailbox credential without any server confirmation of ownership. The fix tightens the short-circuit to bot.RobotID != "" && bot.Profile != "" && !verify: a stored profile is still trusted as a local selector, but a bare environment RobotID now falls through to the authoritative POST /v1/bot/register lookup. The existing mismatch guard then rejects with the active Bot token belongs to %q, not %q, so MailCredentialFunc (which calls ResolveBotIdentity before storedMailCredential) can no longer release another Bot's mailbox token on an unverified claim. This is a genuine hardening of the local-credential-store trust boundary, distinct from the earlier (correctly dismissed) cross-Bot-escalation concern.

Supporting changes are consistent:

botIdentityClient() forces a real (non-dry-run) client for the identity prerequisite even under --dry-run, so the authority check is never skipped and the synthetic dry-run description is never mis-parsed as a /v1/bot/register response. The user-requested operation still stays a dry-run (the f.Globals.DryRun early-returns added in cmd/mail_auth.go login/status/showCurrentMailConnection emit the redacted request and skip token exchange / mailbox probe).
identityValue() now omits robot_id unless it came from a stored profile or has been verified — an unverified env claim is no longer echoed as identity.
Test coverage is solid: TestFactory_MailCredentialRejectsUnverifiedRuntimeBotIdentity proves the victim-bot rejection, TestFactory_MailCredentialSupportsRuntimeBotIdentity asserts exactly one authority call, TestFactory_MailCredentialVerifiesRuntimeBotDuringDryRun covers the dry-run path, plus login/status dry-run tests asserting device/mailbox endpoints are not hit. Doc comments in factory.go/provider.go/CLAUDE.md updated to match.

2. golangci-lint check — ALREADY IN PLACE.
The branch already ships .golangci.yml (v2) and a CI golangci-lint job (golangci-lint-action v9.2.0, golangci-lint v2.12.2, pinned by SHA). Ran locally against this head: 0 issues.

Local verification (this head)

go build ./... — clean
go test ./... — all packages pass (incl. cmd, internal/cmdutil, internal/credential)
golangci-lint run ./... (v2.12.2) — 0 issues

Non-blocking (unchanged from prior)

🟡 cmd/auth.go newAuthUpdateCmd: --api-base-url is only trimmed and checked non-empty; a malformed value (no scheme / not a URL) is persisted to the profile and only fails on later requests. Low-severity (writes to the caller's own profile), but a lightweight url.Parse + scheme check would fail fast. Not part of this commit's delta.
CHANGELOG still has no Agent Mail entry under [Unreleased]. Worth adding before release.

The only failing check is check-sprint (project-board sprint field), unrelated to code.

yujiawei

Code Review — PR #129 (octo-cli)

Third review round, at head 7ad6aede9b08ac99e8cf4f5ebd653fca9f5a879b against merge-base 6a943accb3f850349bf56d3e4180c97a26200c77 (35 files, +3582/−41). The round-3 delta is one commit, 7ad6aede "fix(mail): verify runtime Bot identity", touching CLAUDE.md, cmd/mail_auth.go, internal/cmdutil/factory.go, internal/credential/provider.go and their tests (+273/−10).

Verified locally at this head: go build ./... OK, go vet ./... OK, gofmt -l . clean, go test -race -shuffle=on -count=1 ./... OK (12/12 packages), go mod tidy produces no drift. golangci-lint is not installed in my environment, so the lint claim is unverified by me.

Every behavioural claim below was checked by running the built binary against stub OCTO / Agent-Mail servers, and the new security assertions were mutation-tested. Commands and observed traffic are quoted. I also cross-checked my read against two independent automated analysis passes; where they and I disagree on severity, both positions are stated rather than averaged.

1. Specification compliance

Spec: ✅

Measured against the acceptance criteria and out-of-scope list in #128, re-derived rather than inherited from the previous round.

Missing: none.
- Device flow works from both a stored profile and an OCTO_BOT_TOKEN runtime (cmd/mail_auth.go:80, :264).
- A claimed RobotID that does not match the authenticated Bot is rejected before storage (cmd/mail_auth.go:227, internal/cmdutil/factory.go:470) — and, as of this commit, at use time too.
- Mail and pending secrets are AES-256-GCM at 0600 under a 0700 dir, keyed by RobotID, excluded from output, and cleared on profile removal (internal/authstore/authstore.go:242-268) and rebind (:186-199).
- mail uses the mailbox token on /agent-mail-api, sends no Bot token, and sends no X-Space-Id. The latter is double-guarded: mail.json:11 declares x-octo-space-header: false, and client.NewMail (internal/client/client.go:750) copies only {Token, Source} into the transport credential, so SpaceID is empty at client.go:954 regardless. Observed on the wire: POST /agent-mail-api/webapi/v0/messages auth=Bearer omb_… space=None.
- Side-effecting ops declare x-octo-retry: never and map lost responses to RESULT_UNKNOWN (internal/client/client.go:1269-1281), with a tripwire over the operation list (cmd/service/mail_test.go:117-139) and a behavioural test asserting exactly one attempt (:222).
- auth update, the RFC 8621 state/changes pair, the embedded skill, and the doc updates are all present.
Over-build: none. Round 3 adds no command, flag, or environment variable. No direct octo-mail base URL and no mailbox-token env var exist anywhere in the tree (grep -rn 'OCTO_MAIL\|MAIL_TOKEN\|MAIL_BASE' returns nothing outside tests). Owner confirmation is not bypassed; ambiguous side effects are not retried.
Deviations: none I can sustain against the AC text. 7ad6aede changes identity.robot_id behaviour for all domains, not just mail, which is worth naming explicitly as a cross-domain change — but it is a repair of a defect raised in review, and it serves the stated purpose of the identity echo ("so misuse is visible") rather than contradicting it.

One soft observation, not a spec failure: AC5 asks that "read, binary-download, controlled-send, reply, Draft, delivery, and RFC 8621 change-tracking paths are covered by tests". Read, binary download, controlled send and JMAP have behavioural tests; reply, Draft and delivery are covered by the registry tripwire plus the shared engine's own tests. For a metadata-driven domain that is a reasonable reading of the AC, so I am not grading it ❌ — see the note under quality about the one place where the absence of a behavioural --dry-run test did let a bug through.

2. Code quality

Quality: Approved

Verified fixed — the round-2 blocker is genuinely closed

The previous round blocked on a P1: an unverified OCTO_BOT_ID (a) released another Bot's stored mailbox token and (b) was echoed as identity.robot_id in every domain. 7ad6aede takes the "verify before release" route: internal/cmdutil/factory.go:431 now requires a stored profile, not merely a non-empty RobotID, before short-circuiting the authority lookup, and :559 withholds robot_id from the envelope until botIdentityVerified is set.

I reproduced the previous round's exact repro at this head. Store contains only mail-credentials.enc with an entry for robot-VICTIM-B, no profiles:

$ OCTO_BOT_TOKEN=app_ANY_STRING OCTO_BOT_ID=robot-VICTIM-B \
    octo-cli mail message send --data '{"to":["x@example.com"],"subject":"s","text":"t"}'
{ "error": { "code": "UNAUTHORIZED", "type": "auth_error",
             "message": "the active Bot token belongs to \"robot-REAL-A\", not \"robot-VICTIM-B\"",
             "hint": "select the credential that belongs to the current Bot" }, "ok": false }
exit=3

Stub server log, complete — one identity call, and zero requests to /agent-mail-api:

POST /v1/bot/register auth=Bearer app_ANY_STRING space=None body={}

And (b):

$ OCTO_BOT_TOKEN=app_x OCTO_BOT_ID=totally-made-up-id octo-cli event list
{ "data": {...},
  "identity": { "bot_kind": "app_bot", "source": "env:OCTO_BOT_TOKEN", "type": "bot" },
  "ok": true }

robot_id is gone. With a matching id the round-trip happens and the echo returns, so the guard has both directions. Fail-closed selection is also intact for the non-empty-store case that the file_provider.go:45-56 fallthrough could have widened:

$ # store has one profile for robot-ONE
$ OCTO_BOT_TOKEN=app_env OCTO_BOT_ID=robot-DOES-NOT-EXIST octo-cli event list
{ "error": { "code": "UNAUTHORIZED",
             "message": "no profile found for bot id \"robot-DOES-NOT-EXIST\"" }, "ok": false }

The four new tests are load-bearing, not decorative. I mutation-tested them by restoring the old condition at factory.go:431:

--- FAIL: TestFactory_MailCredentialSupportsRuntimeBotIdentity        Bot identity calls = 0, want 1
--- FAIL: TestFactory_MailCredentialRejectsUnverifiedRuntimeBotIdentity
        MailCredential = &{Token:omb_victim_mail BotID:victim-bot ...}, <nil>; want claimed Bot rejection
--- FAIL: TestFactory_MailCredentialVerifiesRuntimeBotDuringDryRun    Bot identity calls = 0, want 1

Each failed for exactly the predicted reason, and the rejection test additionally asserts f.mailCred == nil so the token is not released as a side effect of the failed path. That satisfies this repo's own "a test that passes is not evidence until it has failed" rule and its both-directions requirement for security boundaries.

The previous round's P2-1 (--dry-run broken across the mail domain) is also fixed for generated leaves and for mail auth login|status, with the pending authorization preserved and the device code masked in the emitted description:

$ octo-cli mail auth status --dry-run          # pending device flow present
{ "data": { "body": { "codeVerifier": "***REDACTED***", "deviceCode": "***REDACTED***" },
            "dry_run": true, "method": "POST",
            "url": ".../agent-auth/token" }, ... }
$ # mail-authorization.enc still on disk; zero requests to the token endpoint

Finally, the code/documentation contradiction that was the real substance of the round-2 P1 is resolved. internal/credential/provider.go:16-19 and CLAUDE.md:25 now describe what the code does, and factory.go:427-430 states the profile/environment asymmetry openly instead of papering over it.

P2-1 — `--dry-run` is still broken on the two JMAP leaves, with a hint that sends the operator the wrong way

cmd/mail_jmap.go:120,142 never check f.Globals.DryRun, so the synthetic request description is unmarshalled as a JMAP session and fails structural validation. This is the one corner of the domain the dry-run fix missed:

$ octo-cli mail message state --dry-run
$ octo-cli mail message changes --since-state abc --dry-run
{ "error": { "code": "JMAP_MAIL_UNAVAILABLE", "type": "api_error",
             "message": "JMAP session has no primary Mail account",
             "hint": "reconnect the Agent mailbox" }, "ok": false }
exit=1

Two things make this worth more than a nit. The error is misleading: nothing is wrong with the mailbox, but the hint tells the operator to reconnect it, and an agent that follows that hint starts a device flow and interrupts a human for an approval it does not need. And the reason it survived is structural — round 3 added dry-run tests at the credential layer and for the two mail auth leaves, but none for a generated mail leaf or for the hand-written JMAP leaves, so nothing pinned the rest of the domain.

I am rating this P2 rather than P1 on the ground that the previous round rated the entire mail-domain dry-run breakage P2-1, and fixing 28 of 30 leaves cannot promote the residue to blocking. state/changes are read-only discovery calls, so they are also the two commands an agent has least reason to preview. Fix it, but I do not think it justifies a fourth round on its own. Intercept f.Globals.DryRun in the two RunE delegates and add one test per leaf.

P2-2 — The authority that verifies the claim is chosen by the caller, so the new guard is a misconfiguration control, not a security boundary

internal/cmdutil/factory.go:438 issues the register call through a client built from cfg, whose APIBaseURL comes from OCTO_API_BASE_URL (internal/config/config.go:61). A caller who sets both that variable and OCTO_BOT_ID answers their own identity question. Reproduced at this head, store containing only a mailbox entry for robot-VICTIM-B, against a stub that returns {"robot_id":"robot-VICTIM-B"}:

$ OCTO_API_BASE_URL=http://127.0.0.1:8792 OCTO_BOT_TOKEN=app_ATTACKER OCTO_BOT_ID=robot-VICTIM-B \
    octo-cli mail message send --data '{"to":["x@example.com"],"subject":"s","text":"t"}'
{ "data": {...}, "identity": { "robot_id": "robot-VICTIM-B", "source": "env:OCTO_BOT_TOKEN" }, "ok": true }

Stub log — the victim's mailbox token is both released and sent to the caller-chosen origin:

POST /v1/bot/register auth=Bearer app_ATTACKER space=None body={}
POST /agent-mail-api/webapi/v0/messages auth=Bearer omb_MAILTOKEN_OF_B ...

Why P2 and not P1, explicitly. Setting OCTO_API_BASE_URL and OCTO_BOT_ID requires control of the process environment, and CLAUDE.md:26 states the boundary this store defends: "the encryption key is machine-derived, so the store resists off-machine leakage … but not a same-user process." A same-user process can read mail-credentials.enc directly, so this is not an escalation. Crucially, the realistic trigger the previous round rated P1 on — a stale or templated-wrong OCTO_BOT_ID on a multi-bot host sharing one OCTO_CONFIG_DIR — is now genuinely closed, because in that scenario the base URL is the real gateway and the check fails closed. To still get through by accident you would need the wrong origin to return the same robot id, which does not happen between environments.

What I do want on the record is the wording. provider.go:18-19 says the claim holds "until the server confirms that the token owns it"; what the code establishes is "until the configured origin says so". skills/octo-mail/SKILL.md:127-129 already tells agents not to override OCTO_API_BASE_URL during mail setup, which is the right instinct — but that is guidance, not enforcement. One sentence acknowledging that the check assumes a trusted origin would make the comment exactly true. Optionally, refusing to release a mailbox credential when OCTO_API_BASE_URL is http:// rather than https:// would remove the passive-capture half at low cost.

One of the automated passes rated this P1. I disagree only on severity, not on mechanism — I reproduced its exploit verbatim, as quoted above.

P2-3 — Stored profiles skip verification entirely, and a profile's RobotID is itself never verified

factory.go:431 grants the short-circuit to any credential with Profile != "". But cmd/auth.go:150-157 writes RobotID: botID straight from --bot-id with no server check, so a profile's RobotID is caller input too — just written earlier and through the filesystem. Reproduced:

$ # store has a mailbox entry for robot-VICTIM-B and no profiles
$ printf 'app_ATTACKER_TOKEN' | octo-cli auth login --bot-id robot-VICTIM-B --with-token
$ octo-cli mail message send --data '{"to":["x@example.com"],...}'
{ "data": {...}, "identity": { "profile": "robot-VICTIM-B", "robot_id": "robot-VICTIM-B",
                               "source": "profile:robot-VICTIM-B" }, "ok": true }

Stub log: POST /agent-mail-api/webapi/v0/messages auth=Bearer omb_MAILTOKEN_OF_B — and grep -c bot/register returns 0. The profile creation is preserved rather than purged because SaveProfile's newProfileMayReuseLegacyName is false when name == meta.RobotID, which is the intentional "runtime Bot gained Mail access before it was saved as a profile" case.

Same severity reasoning as P2-2: writing a profile requires write access to OCTO_CONFIG_DIR, which already grants read access to the mailbox secret. It is also not a regression — unverified profile RobotIDs predate this PR, and identity.robot_id from a profile was echoed unverified at the merge-base too. The mailbox binding path is strictly better than before, since mail auth login|status go through VerifyBotIdentity and cannot bind a mailbox under a stale profile. I raise it because the asymmetry is now load-bearing for a secret it was not load-bearing for before, and because the comment at factory.go:427-428 ("a stored profile is the local identity selector") is doing quiet work that deserves to be stated where operators read it, not only where Go developers do.

One automated pass rated this P1; I reproduced its mechanism and rate it P2 for the reason above.

P2-4 — `--dry-run` now performs a real, retryable, authenticated POST

botIdentityClient() (factory.go:488-507) deliberately builds a client with DryRun: false, so every mail command under --dry-run issues a live POST /v1/bot/register with the Bot bearer. Observed on all six mail surfaces I exercised. This is what the previous round asked for and it is the reason dry-run works at all now, so I am not asking for it to be reverted — but three consequences should be conscious choices rather than side effects:

--dry-run is documented as "print request without executing" (cmd/root.go:50) and is now non-offline and credential-requiring on this one domain.
DisableRetry is not set on that request, so a flaky network can turn one preview into up to four POST /v1/bot/register calls.
Whether /v1/bot/register is genuinely idempotent is a backend property this repo cannot assert. Flagged for human verification below.

Setting DisableRetry: true on the identity request is a one-line hardening, and a sentence in CLAUDE.md next to the existing "resolve identity before --dry-run" rule would make the network behaviour discoverable.

P2-5 — Carried over from round 2, unaddressed and re-confirmed at this head

7ad6aede touches six files, none of them these, so each of the following is unchanged. I re-verified rather than assuming:

SaveProfile purge is wider than the alias it targets (authstore.go:188). Creating a profile whose name equals another Bot's RobotID deletes that Bot's mailbox authorization, because mailTokens keys aliases and canonical RobotIDs in one namespace. Recovery requires the full human-approval flow. Narrow the purge to the alias key, or split the namespaces.
Unbounded io.ReadAll on binary mail responses (internal/client/client.go:990). The comment above it justifies buffering on the grounds that "board PNG/SVG exports are bounded and small … deferred until an operation returns genuinely large bodies". mail.message.raw (mail.json:120) and mail.message.attachment.download (:153) are that case, and the size is chosen by whoever emailed the agent. cmd/drive.go:1062 already has the io.Copy precedent. Both automated passes and I independently landed on this one.
-o clobbers an existing file with no guard (client.go:1043). Re-verified: echo PRECIOUS > /tmp/precious.txt then mail message attachment download MSG1 PART1 -o /tmp/precious.txt leaves the attachment bytes and no original. cmd/drive_download.go:37,48,62 requires --overwrite for the same operation shape. Now reachable with content supplied by an untrusted external sender.
Revoked-token self-heal exists only in mail auth status (cmd/mail_auth.go:298-306). Every other mail path returns a generic auth error that never names mail auth login, which is the failure mode agents will actually hit once an authorization is revoked in OCTO Web.
auth update --api-base-url leaves the mailbox token bound to the old origin (authstore.go:211-224).
submissionIds type contradiction. mail.json:476 and :501 declare an array of integer; the repo's own stub returns strings (cmd/service/mail_test.go:272). Nothing breaks at runtime, but octo-cli schema and the embedded docs are wrong on one side. Raised in two prior rounds.
CHANGELOG.md has no Agent Mail entry. git diff <merge-base>..HEAD -- CHANGELOG.md is empty and grep -ci mail CHANGELOG.md returns 0, while [Unreleased] → Added documents loop and drive in detail. This PR adds a whole domain plus auth update. Raised in three rounds now; cheapest item on the list.
Crash window between credential save and pending cleanup (cmd/mail_auth.go:230-235) — self-heals on the next run, cosmetic.
Revocation write ordering strands the secret if the second write fails (authstore.go:258-268) — on a revoke path, delete the secret first.

Nits

cmd/mail_auth.go:106-109 still omits code_challenge_method. A plain-defaulting server fails closed rather than open, so this is interop hygiene, but stating S256 is one line.
storedMailCredentialForBot (cmd/mail_auth.go:326) and storedMailCredential (factory.go:275) implement the same RobotID→profile fallback twice in different shapes.
X-Octo-Idempotency-Key is documented as "8-200 character" (mail.json:268,301,327) with no minLength/maxLength, so the pre-flight validator cannot enforce what the description promises.
The success envelope echoes the Bot identity but never the mailbox actually used. On a surface where identity is the point, mailbox_address in the envelope would have made the P2-2 and P2-3 reproductions obvious at a glance.
cmd/mail_jmap.go:120,142 set Credential: "mail" and SuppressSpaceHeader: true on requests already issued through the mail client, where both are redundant.
ClientForCredential (factory.go:391-402) treats every unrecognised credential kind as "Bot", so a spec typo ("mails") would send the Bot bearer to a mail endpoint. cmd/service/mail_test.go:98 pins the declaration for the mail domain, which covers this in practice, since x-octo-credential is a spec-top-level value. The weaker half is RetryMode: cmd/service/mail_test.go:117 enumerates operation ids by hand, so a new side-effecting mail operation added later without x-octo-retry: never would silently regain retries. Deriving that list from the domain would close it.
cmd/api_secrets_test.go:454 relaxes a tripwire to allowlist x-octo-secret on header parameters, which the PR needs for X-Octo-Confirmation. I checked the justification and it holds today — octo-cli api exposes only --params, --data and --service, with no arbitrary-header input, so there is no caller-supplied header value to recover. The forward guard ("if api ever gains a header option, its own tests must extend apiSecretsForRequest") is a comment rather than a test, which is the part worth strengthening. Query-position secrets are still caught.
internal/registry/specs/mail.json:47-49 is indented with tabs where the rest of the file uses spaces.

Things this round got right that are worth naming

Masking holds on the paths I exercised. --confirmation-token SUPER_SECRET… appears zero times in both --dry-run stdout ("X-Octo-Confirmation": "***REDACTED***") and --verbose stderr.
client.NewMail keeping the MailCredential→BotCredential conversion private to the transport is a clean boundary, and dropping SpaceID in that conversion means the no-X-Space-Id guarantee does not depend on the spec flag alone.
markResultUnknown copies the ExitError instead of mutating one that retry bookkeeping may still hold.
The dry-run early returns preserve the pending authorization and do not consume a device code — verified, not assumed.

3. Overall verdict

APPROVE

Spec ✅. Quality Approved: the round-2 blocker is closed, verified by reproduction rather than by reading, and the tests that pin it fail when the fix is reverted. Everything remaining is P2 or a nit.

Round bookkeeping: this is the third review round (heads 3815eec9 → 603455b0 → 7ad6aede) with four reviewers and seven submitted reviews. The previous round said that if a third round did not resolve the blocking finding one way or the other, the next step should be an owner decision on the trust model rather than another review pass. It resolved it. Holding the branch for a fourth round over a dry-run gap in two read-only commands, when ten P2s were already explicitly declared non-blocking, would be the whack-a-mole that rule exists to prevent. Land it and take the follow-ups as ordinary work.

4. Suggested direction

In the order I would do them:

--dry-run on mail message state|changes, plus one test per leaf — and, while there, one dry-run test on a generated mail leaf so the domain is pinned rather than sampled.
CHANGELOG.md entry, and reconcile the submissionIds type against the octo-mail DTO. Both are minutes of work and both have now been raised three times.
DisableRetry: true on the identity request, and one sentence in CLAUDE.md recording that mail --dry-run performs a real authority call.
One sentence in provider.go acknowledging that the verification assumes a trusted OCTO_API_BASE_URL; optionally refuse to release a mailbox credential over plaintext http://.
Narrow the SaveProfile purge to the alias key, or split the alias and RobotID namespaces.
Stream or cap x-octo-binary-response bodies, and honour the --overwrite contract for -o, now that both are reachable with attacker-sized, attacker-named content.
Add the re-login hint to auth errors on the shared mail path.

5. Points for manual human verification

This PR is labelled needs-human-review; these are the items I could not settle from inside this repo.

Is POST /v1/bot/register idempotent and safe to call on every mail invocation, including --dry-run? The CLI now depends on that. If it mutates any bot state, the dry-run path needs a read-only identity endpoint instead.
The device and token endpoints are called with no credential at all (cmd/mail_auth.go:250-261), by design. The backend therefore cannot authenticate the caller of mail auth login: anyone who knows a botId and spaceId can start a device authorization for someone else's Bot, and the owner-approval screen is the only gate. Please confirm that screen shows enough (which client, which Bot, which mailbox) for an owner to recognise and reject an authorization they did not initiate, and consider rate limiting /agent-auth/device.
The /agent-mail-api gateway must admit those two public bootstrap paths while stripping unrelated credentials (octo-web#1315). That behaviour lives outside this repo and no test here covers it.
The trust-model asymmetry in P2-2 and P2-3 is an owner call, not a reviewer call. Both sit inside the documented same-OS-user boundary, so I am not blocking on them — but if any deployment shares one OCTO_CONFIG_DIR across mutually-distrusting bots, that assumption is the one to revisit, and CLAUDE.md:26 already says separate OS users or OCTO_CONFIG_DIR values are the answer.
Worth a live run before merge, since the PR states live end-to-end authorization was not exercised: the full mail auth login → owner approval → mail auth status flow against a real backend, and one controlled send-intent in each outbound mode.
The submissionIds type question needs someone who can read the octo-mail DTO.

6. Coverage

What I did: read every changed file at this head; built the binary and drove it against stub OCTO and Agent-Mail servers for each behavioural claim above; reproduced the previous round's two blocking repros and all three bypass mechanisms raised by the automated passes; mutation-tested the four new security assertions; ran the race suite, vet, gofmt and go mod tidy; and re-derived the #128 acceptance criteria instead of inheriting the previous round's grading.

What I did not verify: the wire contract against a real Agent Mail backend; the gateway's credential-stripping behaviour; mail.json request/response schemas against the backend's actual DTOs; golangci-lint (not installed here); the npm wrapper; file-permission and key-derivation behaviour on Windows and macOS; and concurrent multi-process access to the credential store, which has no file locking — one automated pass sketched an interleaving in which a profile re-point racing a mail auth status completion can leave a legacy-name mailbox key readable by a different Bot through the factory.go:283 fallback. It needs a deliberate operator action plus a race, so I rate it P2 and did not attempt to reproduce it.

Both automated passes ran and returned findings; neither was absent. One of them was truncated by an output limit mid-coverage-note, so its own statement of what it did not examine is incomplete — treat its coverage as unknown beyond what is quoted above rather than as complete.

mochashanyao

[Octo-Q · automated review]

Verdict: Approve — no blocking findings; notes below (data-flow traced).

Code Review — PR #129 (octo-cli)

Reviewer: Octo-Q (automated review)

Summary

This PR adds Bot-bound Agent Mail to octo-cli: an OAuth-style device flow with PKCE (mail auth login / mail auth status), a dedicated encrypted mail-credential store keyed by verified RobotID, a new credential: mail authorization boundary routed through the spec engine (19 mail operations, non-idempotent side effects marked x-octo-retry: never and surfaced as RESULT_UNKNOWN on network failure), JMAP Email state/changes polling commands, an auth update command for profile endpoint metadata, and a careful treatment of OCTO_BOT_ID as an unverified claim until /v1/bot/register confirms it. The security design is largely sound: identity is verified before a mailbox is bound, the mail token is cryptographically separated from Bot tokens, and the confirmation-token surface is masked end to end. No P0/P1 issues; two P2 items below, both in the profile↔identity trust seam.

Verification

Local checks at head 7ad6aed (built and executed in this environment):

go build ./cmd/octo-cli — clean, binary produced
go test ./cmd/... ./internal/cmdutil/... ./internal/authstore/... ./internal/client/... ./internal/credential/... ./internal/registry/... — all pass
Flag-surface ground truth: every command and flag documented in skills/octo-mail/SKILL.md verified against the built binary via --help (send-intent --to/--subject/--text/--idempotency-key, draft send --draft-version/--confirmation-token, flag --addKeywords/--removeKeywords, all read paths, mail auth login --mailbox) — no drift
internal/registry/specs/bot.json: /v1/bot/register declares no required body fields, so the empty-body identity probe in resolveBotIdentity is valid against the documented contract
octo-cli api passthrough has no arbitrary-header input, so the relaxed secret tripwire in cmd/api_secrets_test.go (header-position secrets now allowed) is safe; generated commands collect header secrets via collectSecrets (cmd/service/run.go)

Findings

No P0/P1 issues; two P2 items below.

P2 — Profile-name/RobotID key collision can silently purge another Bot's mail credential (`internal/authstore/authstore.go:188`)

Diff-scope: new in this PR (the mail keyspace and this purge are introduced here). The mail store is a flat map[string]string whose keys are simultaneously RobotIDs (new layout) and legacy profile names. newProfileMayReuseLegacyName := !existed && name != meta.RobotID deletes mailTokens[name] and pendingMail[name] when creating any new profile whose name differs from its own RobotID. If Bot A's mail token sits under key R_A (its RobotID, stored under a friendly-named profile) and someone creates a brand-new profile named R_A for Bot B (auth login --profile R_A --bot-id R_B), Bot A's mail credential and any pending authorization are deleted. assertBotIDFree (cmd/auth.go) checks the new botID against existing profiles but never the new profile name against existing RobotID keys. Consequence is silent local credential loss — Bot A's mail commands fail with "Agent Mail is not connected" until a human re-runs the device flow; the server-side grant survives, so this is recoverable and grants no cross-Bot access, which keeps it below the P1 bar. Fix: skip the name-key purge when name equals the RobotID of a different stored profile, or namespace keys (robot:<id> vs profile:<name>).

P2 — Mail execute path trusts a login-time RobotID claim that nobody ever verifies (`internal/cmdutil/factory.go:431`)

Diff-scope: amplified + new asymmetry. auth login --bot-id recording the caller-supplied robot id without a server check is pre-existing (cmd/auth.go:154, present at base); what this PR adds is (a) releasing a stored mailbox token based on that claim and (b) a stricter verification path for env-supplied claims, making the asymmetry visible. resolveBotIdentity(verify=false) returns early whenever bot.RobotID != "" && bot.Profile != "", so ordinary mail commands select GetMailCredential(RobotID) with no /v1/bot/register round-trip. Re-logging into an existing profile with Bot B's token while claiming Bot A's robot id does not flip identityChanged (internal/authstore/authstore.go:187), so no purge fires and Bot A's mail token is handed to Bot B's runtime — with the identity echo printing robot_id: R_A as if verified. The PR's own epistemology for OCTO_BOT_ID ("a claim until the server confirms it") does not extend to the identical claim persisted by auth login. What keeps this below P1: the documented agent workflow mandates mail auth status first, which runs VerifyBotIdentity and fails the mismatch loudly; the trigger is explicit operator misconfiguration rather than a runtime-reachable state; and the documented isolation boundary is the OS user (a same-user process can read the store directly). Fix direction: verify the claimed robot id against /v1/bot/register at auth login time (the token is already in hand), or release mail credentials through VerifyBotIdentity on first use per process.

Human-verify

Server-side contract of the agent-mail-api endpoints — error codes authorization_pending/expired/used/denied, the omb_ token prefix, and the botId binding enforced at token exchange — cannot be confirmed from this checkout. The CLI side matches the documented contract; not a merge blocker for this PR.
Space-scoped (platform) bots: mail auth login requires local Space context and errors with a hint otherwise. If platform bots are ever eligible for Agent Mailboxes with server-side space resolution, this local requirement deserves a follow-up; the error hint covers it today. Not a merge blocker.

Things I checked that are fine

PKCE device flow: 32-byte verifier, S256 challenge, pending material stored only in the encrypted store, terminal errors (expired/used/denied) purge pending, token exchange validates omb_ prefix and token.BotID == bot.RobotID
Credential separation: NewMail converts mail credentials privately at the transport boundary; the Bot provider chain, octo-cli api, and the identity echo can never select or expose a mail token; robot_id is omitted from the identity echo until verified
Retry semantics: DisableRetry + UnknownOutcomeOnNetworkFailure applied to every x-octo-retry: never op via cmd/service/run.go; markResultUnknown only rewrites network-type errors and preserves API errors; unwrapping through retryableErr confirmed by test
Secret hygiene: confirmation/idempotency header flags marked x-octo-secret, collected by collectSecrets, masked in verbose/dry-run; verbose traces log response status/size only, never bodies; dry-run of mail auth login/status performs the real identity check but never starts a device flow or persists proof material
skipValidation nesting: only top-level octo-cli auth is credential-free; mail auth login/status pass the normal validation gate (verified by walking the command tree, annotation deliberately not inherited)
Profile lifecycle: SaveProfile rebind and RemoveProfile both purge mail + pending keys for the profile name and the RobotID; legacy profile-key fallback in both storedMailCredential sites is consistent (internal/cmdutil/factory.go and cmd/mail_auth.go); encrypted files written atomically with secPerm
Docs: README/CLAUDE.md updates match the implementation (schema list reports 308 ops, matching the claimed count)

Verdict: COMMENT

No correctness, security, or build-breaking issues; the two P2s sit in the profile↔identity seam and are worth addressing but do not block: both require explicit operator misconfiguration with unusual naming/claims, both are recoverable or loudly detected by the mandated mail auth status workflow, and the core Bot-binding guarantees (verify-before-bind, token separation, one-time confirmation, RESULT_UNKNOWN on unknown side-effect outcome) hold.

[Octo-Q] verdict: APPROVE — no P0/P1 per rubric R1–R4 (only 2×P2 + 2×Nit; nits dropped by merge policy). Suggested outbound verdict for final review: APPROVED / COMMENT-with-P2s at final reviewer's discretion.

Octo first-review supplemental (for final review; not part of the outbound GitHub review)

1. Verification conclusions

✅ Build + full changed-package test suite pass at head 7ad6aede9b08ac99e8cf4f5ebd653fca9f5a879b (executed in this environment)
✅ SKILL.md agent contract verified against built binary flag-by-flag
⚠️ Two P2 findings above (internal/authstore/authstore.go:188, internal/cmdutil/factory.go:431)

2. Data-flow backtrace (consumed data → upstream → does it really flow)

bot.RobotID for mail-credential selection ← profile meta (auth-login-time claim, unverified) OR env claim (verified via /v1/bot/register before release). Flows to GetMailCredential; verify=false fast path proven to fire only for stored profiles (internal/cmdutil/factory.go:431)
token.AccessToken ← device token-exchange response → validated (omb_ prefix, botId match, non-empty mailbox) → encrypted store → MailCredentialFunc/showCurrentMailConnection. Flow verified; revoked-token probe deletes local copy on auth_error
pending.DeviceCode/CodeVerifier ← device endpoint → encrypted store → token exchange body, both marked SecretValues. Flow verified
d.Credential="mail" ← x-octo-credential (doc-level) in internal/registry/specs/mail.json → runOperation/runPaginated → ClientForCredential → MailClientFunc → stored token. End-to-end verified incl. pagination path
JMAP accountId ← session GET primaryAccounts[urn:ietf:params:jmap:mail]; empty → hard JMAP_MAIL_UNAVAILABLE error (no silent empty-account fallthrough)
RESULT_UNKNOWN ← network-type error after doWithRetry; AsExitError unwraps retryableErr (Unwrap method + TestMarkResultUnknown). Flow verified
Identity echo robot_id ← gated on Profile != "" || botIdentityVerified — unverified env claims proven omitted (test TestFactory_IdentityEchoOmitsUnverifiedEnvironmentBotID)

3. Blind-spot checklist C1–C6 (security_sensitive → all items)

C1 dual-path parity — hit, audited: SaveProfile↔RemoveProfile both purge mail+pending (name key and RobotID key); login(save pending)↔status(consume+remove; terminal-error remove); credential save↔remove-on-revoke. Asymmetry found → finding F2 (create-gate verifies, execute-path trusts). Guard-equivalence: skipValidation annotation non-inheritance verified by command-tree walk; assertBotIDFree does NOT cover name↔RobotID collisions → finding F1
C2 control-flow ordering/reuse — clear: resolveBotIdentity reused across verify/non-verify callers with per-Factory botIdentityVerified cache; dry-run forces a real register call via botIdentityClient (DryRun:false) so preview can never release a credential on synthetic data; no double-effect in nested reuse
C3 authorization boundary — clear: capability question "who can reach the mail token" answered: only ClientForCredential("mail") via spec-declared ops + the two hand-written mail command files; api passthrough and Bot chain cannot reach it; header-secret tripwire relaxation justified (no arbitrary-header input on api)
C4 container/member cascade — N/A + reason: no space/admin hierarchy involved; the profile↔bot cascade is the analogue and is handled (with the F1 collision caveat)
C5 build ≠ runtime — clear: did not stop at build/test; flag surface, op count (308), register-body contract, and skipValidation nesting all re-verified against the built binary
C6 governance docs — clear: SECURITY.md/CONTRIBUTING/label policy untouched; README/CLAUDE.md additions consistent with implementation (op count cross-checked)

4. Cross-round blocker re-check (R6)

N/A — first review round for this PR on this issue (no prior blockers).

5. Additional findings (dropped by merge policy, recorded for final reviewer)

Nit — cmd/auth.go:49: auth update --api-base-url persists without config.NormalizeAPIBaseURL (login path normalizes). Fail-closed downstream at cfg.Validate, so no misrouting; write-time parity is the clean fix
Nit — internal/authstore/authstore.go:189: purge persisted before the profile/token write; a mid-transition failure strands the rebind incomplete with the mail credential already gone (recoverable; ordering swap would be all-or-nothing in the safer direction)

yujiawei · 2026-08-12T08:26:36Z

Follow-up to my review above — three additions from a second pass. None of them changes the verdict; the approval stands. Two are new defects, one sharpens a human-verify item I already raised.

1. `auth update --api-base-url` accepts input that `auth login` refuses (new command, new defect)

cmd/auth.go:44-60 (the new newAuthUpdateCmd) trims the value and persists it directly via store.UpdateProfileAPIBaseURL. It never calls config.NormalizeAPIBaseURL, which the login path does call at cmd/auth.go:145. NormalizeAPIBaseURL has exactly three non-test call sites — cmd/auth.go:145, internal/config/config.go:93, internal/cmdutil/factory.go:359 — and the new write path is not one of them.

Observed:

$ octo-cli auth login --bot-id robot-A --api-base-url 'https://good.example.com/some/path' --with-token
{ "error": { "message": "OCTO_API_BASE_URL must not include a service or API path",
             "hint": "pass only the Octo gateway base URL, without a service path" } }   # refused

$ octo-cli auth update --bot-id robot-A --api-base-url 'https://evil.example.com/some/path'
{ "data": { "api_base_url": "https://evil.example.com/some/path", "profile": "robot-A" } }   # accepted, persisted

Every subsequent command on that profile then fails at read-time validation, so there is no misrouting — it fails closed, which is why this is low severity. But the failure message names the wrong repair:

$ octo-cli mail me
{ "error": { "message": "profile \"robot-A\" has an invalid API base URL: … ;
             update it with `octo-cli auth login --profile robot-A`" } }

auth login means re-entering the Bot token. The actual one-line repair is auth update --api-base-url <clean-url>, which I confirmed works. Two small fixes: call NormalizeAPIBaseURL on the update write path (parity with login), and point that hint at auth update now that it exists.

2. `assertBotIDFree` cannot catch the profile-name/RobotID collision — a precise fix location for the `SaveProfile` purge

This refines the authstore.go:188 purge item in my review rather than adding a new one. assertBotIDFree (cmd/auth.go:314-328) checks the new botID against existing profiles' RobotIDs and deliberately allows owner == name. Nothing checks the new profile name against existing RobotID keys in the mail store, and those two namespaces share one map[string]string. So the guard that looks like it covers this cannot: it is checking the other axis.

Reproduced — Bot A under a friendly-named profile, its mail token keyed by RobotID:

$ octo-cli auth login --profile alpha --bot-id robot-A --with-token     # mail token stored under key "robot-A"
$ octo-cli auth login --profile robot-A --bot-id robot-B --with-token   # brand-new profile *named* robot-A
$ # Bot A's mail credential: mail credential not found for Bot key "robot-A"

Silent local credential loss; the server-side grant survives, so recovery is a re-run of the device flow. Either extend assertBotIDFree to reject a profile name that collides with another profile's RobotID, or namespace the keys (robot:<id> vs profile:<name>) — the namespacing option makes the whole class unrepresentable.

3. The identity probe hits an endpoint this repo's own spec classifies as a write

This sharpens human-verify item #1 in my review. I had asked whether POST /v1/bot/register is idempotent, treating it as unknowable from this checkout. It is partly knowable, and the answer points the wrong way:

internal/registry/specs/bot.json:14-17
  "operationId": "bot.register",
  "summary": "Register (authenticate) the bot and obtain IM token + URLs",
  "x-octo-risk": "write",

requestBody.required is false, so the empty-body probe in resolveBotIdentity is well-formed — but the same spec entry marks the operation x-octo-risk: write and describes it as minting an IM token. Combined with botIdentityClient() deliberately setting DryRun: false (internal/cmdutil/factory.go:488-507), the current behaviour is that --dry-run on any mail command performs an operation this repo classifies as a write, and does so with retries enabled since DisableRetry is not set on that request.

I still do not think this blocks: the previous round asked for the non-dry-run identity client, bot.register is the ordinary bootstrap call every bot already makes, and x-octo-risk may well be a coarse "it's a POST" classification. But it does mean the question is no longer "is this idempotent, we cannot tell" — the repo's own metadata says write, so someone who knows the backend should confirm that calling it on every mail invocation, including previews, is acceptable. If it is not, a read-only identity endpoint would be the cleaner dependency. Setting DisableRetry: true on that request is worth doing either way.

Jerry-Xin

Re-review of head 4ffe171 (Bot-bound Agent Mail). In scope for octo-cli. The mail-auth model is byte-identical to the previously-approved head 7ad6aede9b0, and the delta on this head is unrelated marketplace/registry work plus test additions. APPROVE.

Mail-auth model (byte-verified on this head)

Mail send/receive is authenticated solely by the mailbox omb_ token. client.NewMail (internal/client/client.go:750) builds a transport BotCredential{Token: cred.Token} from the MailCredential, and the Authorization: Bearer header (client.go:952) uses only that token. The Bot token is never transmitted in mail calls.
The only non-test persistence of a mailbox credential is SaveMailCredential (cmd/mail_auth.go:230), gated at login by VerifyBotIdentity (resolveBotIdentity(verify=true) → /v1/bot/register) plus a token.BotID != bot.RobotID reject (cmd/mail_auth.go:227). The mailbox is bound to an already-verified Bot at mint time.
Use-time MailCredentialFunc (internal/cmdutil/factory.go:149) calls ResolveBotIdentity(verify=false), which only selects an already-stored credential keyed by the resolved RobotID — it cannot mint or refresh a cross-Bot mailbox token from a Bot token. The bot.Profile != "" gate (factory.go:431) forces the authority lookup for a bare-env OCTO_BOT_ID. No incremental capability: any cross-Bot "attack" would require already possessing botB's mailbox token in the local store, which is the inherent trust boundary of a local credential store.
Encrypted cred files (mail-credentials.enc, mail-authorization.enc) are written secPerm = 0o600 (internal/authstore/authstore.go:31, mail.go:82/174). Tokens are masked in output (MaskToken) and errors are redacted at the transport boundary (client.go redactError).

Delta on this head (vs prior approved 7ad6aede9b0)
All mail/auth files are byte-identical (verified via git hash-object). The genuine new content is a build-time checkDuplicateOperationIDs guard in the registry loader (internal/registry/loader.go), embedded marketplace JSON specs, and skill docs/tests — no network, injection, or credential surface.

Non-blocking

🟡 auth update --api-base-url (cmd/auth.go:65) stores the value with only TrimSpace, whereas auth login (cmd/auth.go:144) runs config.NormalizeAPIBaseURL first. This is a fail-fast/consistency gap, not a security or corruption issue: at use-time overlayProfileBaseURL (internal/cmdutil/factory.go:359) re-runs NormalizeAPIBaseURL and, on a bad value, returns a clear actionable error (profile %q has an invalid API base URL: ...; update it with ...). NormalizeAPIBaseURL also rejects userinfo/query/fragment/path, so no credential-bearing or path-bearing URL ever reaches the transport. Recommend applying the same normalization in auth update so the error surfaces at update time rather than next use.

Highlights

Clean Bot/Mail credential separation with RobotID-scoped, encrypted, 0600 mail storage and independent revocation lifecycles.
Dry-run for mail auth avoids persisting device-flow proof material; identity verification still runs for real.
Transport-boundary error redaction makes secret-masking hold for sites that don't exist yet.
go test ./... passes locally.

yujiawei

Code Review — PR #129 (octo-cli)

Reviewed at head SHA 4ffe171058ac469cb89e441cf30da6e78a3a01e7 against merge-base 84655c91d4aaa66c33232ba7dffc68b0ab406292 (35 files, +3582/-41).

Verified locally on that SHA: go build ./..., go vet ./..., gofmt -l ., go test ./... -count=1, and go test -race -shuffle=on -count=1 ./... all pass. The three findings below were each reproduced with a throwaway probe test against this checkout, not inferred from reading alone; the observed output is quoted with each one.

This is a well-built change overall. The credential boundary is genuinely separated (client.NewMail constructs a transport credential carrying only the mailbox token, so SpaceID/RobotID cannot leak into a mail request), secrets stay in dedicated AES-GCM files, the x-octo-secret markers cover every confirmation-token header, and the required X-Octo-Idempotency-Key on the proactive send paths is the right call. The embedded skill's prompt-injection section is a real security control, not boilerplate. My objections are narrow and, I think, cheap to fix.

1. Specification compliance

Checked against the acceptance criteria in #128.

Acceptance criterion	Status
Stored-profile and `OCTO_BOT_TOKEN` runtimes can complete the device flow	Met
Claimed RobotID not matching the authenticated Bot is rejected before a mailbox credential is stored	Met — `resolveBotIdentity` rejects the mismatch; covered by tests
Secrets encrypted, RobotID-scoped, excluded from output, cleared on profile removal/rebind	Met, with a caveat — see P1-1
Mail uses the mailbox credential on `/agent-mail-api`, sends no Bot token or `X-Space-Id`, stays on `OCTO_API_BASE_URL`	Met — verified in `internal/registry/specs/mail.json` (`x-octo-credential: mail`, `x-octo-space-header: false`) and `client.NewMail`
Read / binary-download / controlled-send / reply / Draft / delivery / RFC 8621 paths covered by tests	Met
Side-effect operations do not retry; ambiguous network failures return `RESULT_UNKNOWN`	Met as written — see P2-4 for a gap outside the literal wording
build / vet / race / fmt / tidy pass	Met, reproduced locally

No scope creep found. Every non-mail edit I checked traces back to a stated requirement: auth update, the EnvProvider OCTO_BOT_ID claim, the FileProvider zero-profile fall-through, and the nested-auth skipValidation narrowing are each load-bearing for an acceptance criterion.

I specifically checked the riskiest cross-file edit — EnvProvider now populating BotKind, and buildCredential deriving it when empty — for regressions in existing non-mail domains. There are none: identityValue computes bot_kind from credential.TokenKind(f.cred.Token) rather than from cred.BotKind, the only cred.BotKind reads compare against "agent_task" (which TokenKind never returns), and the x-octo-allowed-token-kinds gate in cmd/service/identity.go does not consult BotKind at all. The identity.robot_id echo is likewise unchanged for existing users, since env credentials previously had an empty RobotID and the new guard keeps an unverified one omitted.

Spec verdict: PASS.

The one caveat: the criterion says mail secrets are cleared when the owning profile is "removed or rebound". SaveProfile (internal/authstore/authstore.go:186-189) treats "rebound" as the RobotID changed. Rebinding a profile to a different Bot's token while leaving the RobotID as-is is not covered, and that gap is reachable — which is P1-1.

2. Code quality

P1-1 — A profile whose token is swapped still unlocks the original Bot's mailbox, with no identity check

internal/cmdutil/factory.go:431

if strings.TrimSpace(bot.RobotID) != "" && bot.Profile != "" && !verify {
    return bot, nil
}

For a stored profile, ResolveBotIdentity trusts meta.RobotID and returns without contacting /v1/bot/register. But auth login never verifies --bot-id against the token it stores — meta.RobotID = botID (cmd/auth.go:151-155) is whatever the operator typed — and re-login to an existing profile is explicitly a supported operation. auth_test.go:161 documents it: "Re-login to the SAME profile (same robot id) is allowed — it updates the token."

Because the RobotID is unchanged, SaveProfile's purge (internal/authstore/authstore.go:186-189) does not fire, so the previous Bot's mailbox token survives the swap and is then released to the new token's session.

Reproduced against this SHA:

PROBE A RESULT: token="omb_MAILBOX_OF_BOT_A" botID="bot-a" source="bot:bot-a" identityCalls=0
CONFIRMED: bot-a's mailbox released while authenticated with bot-b's token; /v1/bot/register never called

identityCalls=0 is the part I would like fixed: the authoritative check that this PR builds specifically to prevent mailbox misbinding is never reached on the stored-profile path.

To be precise about severity: this is not a privilege escalation across the documented trust boundary. CLAUDE.md states the isolation boundary is the OS user, and anyone who can run auth login here can already read mail-credentials.enc directly. What I am flagging is an integrity hazard from a supported, non-adversarial operation — token rotation or a mistyped --bot-id silently rebinds the agent to a stale mailbox, and the resulting side effect is email sent to external recipients under the wrong identity, which is externally visible and not undoable. That asymmetry between "local misconfiguration" and "irreversible external effect" is why I am treating it as blocking rather than advisory.

Suggested fix: verify on release, not just on store — have MailCredentialFunc use the verify: true path (or a cached-per-process equivalent) before handing over a mailbox token, mirroring what the environment-token path already does. botIdentityVerified already gives you the once-per-process caching to keep the cost at one round-trip.

P1-2 — `mail message state --dry-run` and `mail message changes --dry-run` fail, and the error tells the user to re-authorize

cmd/mail_jmap.go:114-133

Under --dry-run the mail client returns its synthetic request description — {"dry_run":true,"method":...,"url":...,"headers":{...}} (internal/client/client.go:1148-1160). That has no primaryAccounts, so json.Unmarshal succeeds into a zero-valued mailJMAPSession, the map index yields "", and line 131 raises JMAP_MAIL_UNAVAILABLE.

Reproduced for both commands:

PROBE C [state] err=JMAP_MAIL_UNAVAILABLE: JMAP session has no primary Mail account
stderr={"error":{"code":"JMAP_MAIL_UNAVAILABLE","hint":"reconnect the Agent mailbox",
        "message":"JMAP session has no primary Mail account","type":"api_error"},"ok":false}
PROBE C [changes] err=JMAP_MAIL_UNAVAILABLE: JMAP session has no primary Mail account

The three sibling hand-written mail paths all handle this correctly — cmd/mail_auth.go:129, :220, and :310 each check f.Globals.DryRun before parsing. mail_jmap.go is the one that missed the pattern, and cmd/mail_jmap_test.go has no dry-run case to catch it.

What lifts this above cosmetic is the hint text. --dry-run is a universal flag, the primary consumer here is an LLM agent following skills/octo-mail/SKILL.md, and "reconnect the Agent mailbox" points that agent straight at mail auth login — a flow whose whole design is an intentional pause for a human approval click. A broken preview that reliably manufactures spurious re-authorization requests to a person is a worse failure mode than a plain error would be.

Fix: mirror the sibling pattern — return the preview before parsing, in both mailJMAPAccountID and callMailJMAP — and add the dry-run case to the JMAP test.

P2-1 — Profile names and RobotIDs share one untagged key namespace

internal/cmdutil/factory.go:283-287, internal/authstore/mail.go:90-113

mailTokens is keyed by either a RobotID or a legacy profile name, with nothing distinguishing the two. If a profile's friendly name equals another Bot's RobotID, the legacy fallback hands over that other Bot's mailbox token. SaveProfile's purge covers the case where the profile is created after the collision exists, but not the reverse ordering:

PROBE B RESULT: token="omb_MAILBOX_OF_BOT_B" botID="bot-a" source="bot:bot-b"
CONFIRMED: profile for bot-a received bot-b's mailbox token via the legacy name fallback

Note the returned MailCredential is internally contradictory — BotID: "bot-a" carrying bot-b's mailbox token — so nothing downstream can detect the crossing.

I am rating this P2 rather than P1 because it requires a profile named as an exact match for another Bot's opaque server-generated id, which is not a realistic accident. But it is the same root cause as P1-1, and prefixing the keys (robot:<id> vs profile:<name>) would close both the collision and the ambiguity cheaply.

P2-2 — The identity lookup is a retried write

internal/cmdutil/factory.go:437-442

/v1/bot/register is a POST documented in bot.json as "Register (authenticate) the bot and obtain IM token + URLs". botIdentityClient() returns the default client, so on a 502/503/504 this spec-declared write is retried up to three times. Given this PR's own (correct) position that non-idempotent side effects must not auto-retry, the prerequisite lookup deserves the same DisableRetry / UnknownOutcomeOnNetworkFailure treatment it applies to mail sends.

P2-3 — `--dry-run` now performs a real authenticated POST (please have a human confirm this is acceptable)

internal/cmdutil/factory.go:494-503

botIdentityClient() deliberately builds a client with DryRun: false, so every mail command under --dry-run on an environment-token runtime issues a genuine authenticated POST /v1/bot/register. The code comment argues the case and factory_test.go:718 asserts it (identityCalls == 1), so this is intentional, not an oversight — and the reasoning (an unverified env RobotID must never unlock a stored mailbox) is sound.

I am still surfacing it because it changes what --dry-run means for this domain from "no network" to "one authenticated write to a register endpoint", and the PR summary's "keep Mail --dry-run previews side-effect free" is narrower than a reader will take it. Worth an explicit line in the docs at minimum. This is the item I would most like a human to sign off on, since whether it is truly side-effect free depends on backend /v1/bot/register semantics that cannot be settled from this repo.

P2-4 — A gateway 5xx on a no-retry send is not reported as `RESULT_UNKNOWN`

internal/client/client.go:1273-1283

markResultUnknown only rewrites errors with ee.Type == "network". A 502/503/504 produces an api-typed error (client.go:1017), so it passes through unchanged. A gateway timeout on mail message send is precisely the ambiguous case the mechanism exists for — the upstream may well have accepted and delivered the message — yet the caller receives an ordinary API error carrying a retryable-looking status and none of the "do not retry automatically" hint.

This is compliant with the acceptance criterion as literally worded ("ambiguous network failures"), which is why it is P2. The exposure is also limited on send-intent / draft create-agent / reply-draft, where the mandatory idempotency key makes a retry safe. It is not limited on message send, reply, reply-all, forward, and draft send, which use one-time confirmation tokens instead — and those are the duplicate-email paths.

P2-5 — A stuck pending authorization wedges `mail auth status`

cmd/mail_auth.go:180-215

mail auth status always attempts the token exchange when a pending record exists, and only clears it for authorization_expired, authorization_used, or authorization_denied. Any other failure leaves the record in place, so the command can never fall through to showCurrentMailConnection — even when a perfectly good stored credential exists. pending.ExpiresAt is persisted and echoed but never checked locally. Recovery requires knowing to run mail auth login again. Checking ExpiresAt before the exchange would resolve it.

P2-6 — `auth update --api-base-url` retargets the origin while retaining the mailbox credential

cmd/auth.go:41-88, internal/authstore/authstore.go:213-225

UpdateProfileAPIBaseURL changes the profile origin and, by design, leaves both credentials in place — cmd/auth_test.go:92 asserts exactly that. The consequence is that a mailbox token minted by one gateway will be sent to a newly specified host on the next mail command.

An operator typing the URL is a deliberate act, and the Bot token has always behaved this way, so this is not a bug. But auth update is new in this PR and it is now the mechanism by which an omb_* credential can reach a different origin. Purging (or at least warning about) the mailbox credential when the origin actually changes seems more consistent with the RobotID-rebind purge that SaveProfile already performs.

P2-7 — PKCE sends `codeChallenge` without `codeChallengeMethod`

cmd/mail_auth.go:99-109

The verifier is hashed with SHA-256 and base64url-encoded correctly, but the method is never declared, leaving S256 as an implicit cross-repo contract with octo-mail#48. This fails safe rather than open — a server defaulting to plain would reject the exchange, not downgrade it — so it is a hardening/interop nit. Sending the method explicitly removes the ambiguity.

Nits

cmd/mail_auth.go:32,35 — mailDeviceResponse.VerificationURI and .Interval are decoded but never used (VerificationURIComplete is used throughout). Dead fields on an auth struct invite a later reader to assume polling honours Interval.
cmd/mail_auth.go:74 — strings.Count(addr, "@") != 1 admits @example.com and agent@. The server is authoritative for a preselection hint, so this is cosmetic, but it is weaker than the fail-fast validation elsewhere in the CLI.
cmd/mail_auth.go:45-55, cmd/mail_jmap.go:28-43 — both attach functions return silently when the parent command is absent. If mail were ever hidden by x-octo-disabled (as matter is today), mail auth and the JMAP commands would vanish with no diagnostic.
internal/registry/specs/mail.json — mail.message.flag (PATCH) has no x-octo-retry: never while mail.draft.update (PATCH) does. Flagging looks idempotent so retrying is presumably safe, but the asymmetry between two PATCH writes is worth a one-line comment so the next reader does not read it as an omission.

3. Coverage and blind spots

Things I could not settle from this repository, listed so they are not mistaken for having been cleared:

Backend contract. The device/token endpoint shapes, whether codeChallenge implies S256, and whether the gateway actually strips unrelated credentials from the public PKCE bootstrap paths all live in octo-mail#48 and octo-web#1315. The PR body states live end-to-end authorization was never exercised — that remains the largest untested surface here, and no amount of local review substitutes for it.
/v1/bot/register side-effect semantics. Whether it mutates or rotates server state determines how serious P2-2 and P2-3 actually are.
One-time confirmation-token semantics. Whether X-Octo-Confirmation is genuinely single-use and request-bound, as SKILL.md asserts, is enforced server-side only.
Latency. Every mail command on an env-token runtime now pays an extra /v1/bot/register round-trip, with no cross-process caching. Fine for interactive use; worth measuring for a polling agent running message changes on a tight loop.

Checked and found clean, so these need no further attention: AES-GCM reuse and 0600 permissions on the new secret files; Authorization stripped from --dry-run output and never written by --verbose (which logs status and byte count only); x-octo-secret present on every confirmation-token header; X-Space-Id unreachable on mail requests both by construction and by spec flag; retryableErr.Unwrap correctly feeding output.AsExitError so RESULT_UNKNOWN does fire on the transport path.

4. Verdict

CHANGES_REQUESTED, on P1-1 and P1-2 only.

Both have small, local fixes and both already have a correct pattern elsewhere in this same diff to copy — P1-1 from the environment-token verification path, P1-2 from the three sibling f.Globals.DryRun checks in mail_auth.go. Every P2 above is a follow-up, not a merge blocker; if you disagree with any of them I would rather they were argued than silently absorbed.

Given the needs-human-review label, the two items I would put in front of a human reviewer are P2-3 (--dry-run performing a real authenticated POST) and the unexercised live authorization flow, since neither can be resolved from inside this repository.

Jerry-Xin

Re-review of head 83d26d0 (previously APPROVED 1aa847). In scope for octo-cli. APPROVE — the mail-auth security model is intact and the sole change since my last approval is a behavior-preserving lint fix.

Delta since last-reviewed head (1aa847 → 83d26d)

Exactly one commit, linear (0 behind, 1 ahead — not a rebase). Byte-verified: every PR file is blob-identical to 1aa847 except cmd/mail_jmap.go.
cmd/mail_jmap.go: mailJMAPAccountID converted to named returns (accountID string, preview []byte, err error) and accountID := → accountID =. Purely a lint fix; every return path stays explicit (return "", nil, err / return "", raw, nil / return accountID, nil, nil), so no naked-return zero-value leak. No auth/cred/transport behavior change.

Security model re-confirmed (unchanged, byte-identical files)

Mail ops authenticate solely via the mailbox (mail) credential; the Bot token is never sent in mail calls. Login-gated mint via Bot identity verification before releasing a stored mail cred; use-time credential resolution only selects an already-stored cred and cannot mint/refresh a cross-bot mailbox token. No new incremental capability.
Prior blocking items (Token/RobotID consistency guard on token change; stale-cred cleanup on rebind) live in cmd/mail_auth.go / internal/authstore/mail.go, both byte-identical to the approved head — not regressed.

On the auth update --api-base-url item (non-blocking 🟡, not a blocking regression)

cmd/auth.go is byte-identical to the already-approved 1aa847; this commit did not touch it.
auth update writes the trimmed value via UpdateProfileAPIBaseURL without calling config.NormalizeAPIBaseURL, unlike auth login. However, at use time overlayProfileBaseURL (internal/cmdutil/factory.go:363) re-runs NormalizeAPIBaseURL and hard-fails on any stored value carrying embedded credentials / query / fragment / service path — such a value is never used to build an outbound request. So there is no SSRF or credential-in-request exposure; the residual effect is a plaintext-echo/UX inconsistency (an invalid value can be stored and shown by status/list, leaving the profile unusable until re-set). This is a hardening/consistency gap, not a security regression. Recommend normalizing in auth update for parity and a small regression test.

💬 Non-blocking (carried, unchanged from prior head)

🟡 auth update --api-base-url not normalized (see above) — parity fix + test suggested.
🟡 Error hint in internal/cmdutil/factory.go still points users to auth login to fix an invalid stored URL; with this PR it should mention auth update.
🟡 Prior peer notes still open: mail flag names camelCase (suggest kebab-case); JMAP state/changes schema registration; stale operation-count in command-tree header comment; duplicated stored-key lookup; mail auth status no local-expiry check; mail-credentials.enc read/write not file-locked (concurrent-update race). All non-blocking.

✅ Highlights

Mail operations cleanly isolated onto the mailbox credential boundary.
Bot identity verification before releasing a stored mail credential.
Side-effecting mail operations disable request-level retries.
Use-time base-URL normalization guard neutralizes the malformed-stored-URL risk.

No blocking issues on this head.

yujiawei

Code Review — PR #129 (octo-cli)

Reviewed at head 83d26d0, merge-base 84655c9. 35 files, +3709/−41. Verified locally: go build ./..., go vet ./..., go test ./... -count=1 all pass. The mail command tree registers correctly and mail auth login|status are wired under the generated mail domain.

This is a large, unusually well-commented change and the credential-separation design is sound in outline: a dedicated MailCredential type that never enters the Bot provider chain, a separate encrypted file with an independent lifecycle, NewMail narrowing the transport credential to {Token, Source} so X-Space-Id cannot be emitted, x-octo-retry: never on every non-idempotent mail operation, and x-octo-secret on the confirmation-token headers (correctly picked up by collectSecrets, cmd/service/run.go:317-321). The octo-mail skill's injection/confirmation boundary is genuinely good.

Two things block merge, plus one credential-binding inconsistency. Details below.

1. Spec compliance

Spec: ❌

Measured against issue #128 (goal, proposed solution, out-of-scope list, acceptance criteria).

Coverage is otherwise complete — device flow with PKCE, RobotID-scoped encrypted storage, the metadata-driven REST surface, RFC 8621 state/changes, auth update, the embedded skill, docs. Nothing on the Out-of-Scope list was built (no mail base-URL env var, no separate mailbox-token env var, no policy logic in the CLI). The gaps are:

Over-build — a global credential-selection change outside the Mail domain

internal/credential/file_provider.go:44-52 relaxes --bot-id / OCTO_BOT_ID from fail-closed to fall-through when the profile store is empty. That is a change to credential resolution for every command in the CLI, not just mail. Issue #128 asks only to "resolve the active Bot from the existing stored profile or OCTO_BOT_TOKEN"; it does not ask for the selector's failure semantics to change, and the Out-of-Scope list does not cover it either. See P1-1 below for the behavioural consequence — this needs an explicit decision rather than arriving as a side effect of the Mail feature.

Deviation — --dry-run no longer means "do not send"

--dry-run is documented as "Print the resolved request instead of sending it" (README.md:312) and the flag's own help text says "print request without executing". For every mail command that contract is now broken: internal/cmdutil/factory.go:492-508 deliberately builds a client with DryRun: false and issues a real POST /v1/bot/register. CLAUDE.md does say identity is resolved "before --dry-run", but that sentence describes the local token-kind gate in cmd/service/identity.go; it was not a licence for a network write. Neither README nor CLAUDE.md was updated to record the new exception. See P1-2.

Partial — "ambiguous network failures return RESULT_UNKNOWN"

internal/client/client.go:1274-1276 only converts errors whose type is network. A gateway 502/503/504 on a non-idempotent send is exactly the ambiguous case the criterion exists for, and it surfaces as an ordinary api_error. See P2-4.

2. Code quality

Quality: Changes-Requested

P1-1 — `--bot-id` stops failing closed; a mismatched selector now silently runs as a different Bot

internal/credential/file_provider.go:44-52

if p.ExplicitProfile == "" && p.ExplicitBotID != "" {
    count, err := p.Store.Count()
    if err != nil { return nil, err }
    if count == 0 { return nil, nil }   // fall through to env
}

authstore.ActiveProfile (internal/authstore/authstore.go:318-322) returns StatusMissing for botID != "" with zero profiles, which the file provider turns into a hard auth_error. The new early return bypasses that. Combined with internal/cmdutil/factory.go:270-272 (if cred.Profile == "" && botID != "" { cred.RobotID = botID }) and internal/cmdutil/factory.go:559-563 (which now omits an unverified robot_id from the identity echo), the result is that a mismatched selector produces no signal at all.

Verified by building both commits and running the same command with an empty config dir:

# base 84655c9
$ OCTO_CONFIG_DIR=/tmp/empty OCTO_BOT_TOKEN=bf_tokenForBotB \
  octo-cli message send --data '{...}' --bot-id ROBOT_A --dry-run
{"error":{"code":"UNAUTHORIZED","message":"no profile found for bot id \"ROBOT_A\"", ...},"ok":false}
exit=3

# head 83d26d0
$ (same command)
{"data":{"dry_run":true,"headers":{"Authorization":"Bearer bf_to***BotB"}, ...},
 "identity":{"bot_kind":"user_bot","source":"env:OCTO_BOT_TOKEN"}, ...}
exit=0

The command proceeds as Bot B while the caller asserted Bot A, and the envelope carries no robot_id at all — so this is strictly less visible than the pre-PR hard error. CLAUDE.md's own rule for this surface is "a selector is required (ambiguity is a hard error, never a silent guess)".

Mail is protected (MailCredentialFunc → VerifyBotIdentity compares the claim against /v1/bot/register), but message send, docs, drive, html and everything else are not.

I understand why the change is here: a token-only runtime that injects OCTO_BOT_ID alongside OCTO_BOT_TOKEN was previously rejected outright, and #128 requires that shape to work. The fix is reasonable; the silence is not. Options, in order of preference:

Echo the claim under a distinct key (e.g. identity.robot_id_claimed) instead of dropping it, so a mismatch is at least observable.
Apply the same lazy /v1/bot/register verification used for Mail when a bare OCTO_BOT_ID accompanies an env token, on any command.
At minimum, document the new empty-store semantics in CLAUDE.md's credential-selection bullet.

Per the repo's own Test Discipline ("Security boundaries additionally need a test in each direction"), TestFileProvider_ZeroProfilesWithBotIDFallsThrough covers only the permissive direction. TestFileProvider_BotIDNoMatchErrors still covers the non-empty store, which is good — but there is no test asserting what happens when the fallen-through env token belongs to a Bot other than the one --bot-id named.

P1-2 — every `mail` command performs a real write-risk `POST /v1/bot/register`, including under `--dry-run`

internal/cmdutil/factory.go:486-508, reached from factory.go:438 via MailCredentialFunc → VerifyBotIdentity → resolveBotIdentity.

Two separate problems:

(a) --dry-run executes. botIdentityClient() explicitly constructs a client with DryRun: false so the lookup runs for real. internal/registry/specs/bot.json:12-18 classifies bot.register as x-octo-risk: "write", with the description "Register (authenticate) the bot and obtain IM token + URLs" and a response containing im_token / ws_url. So octo-cli mail message send --dry-run — the exact command an operator runs to avoid touching the server — issues a live POST to a write-risk auth endpoint. The behaviour is enshrined in TestMailAuthLoginDryRunVerifiesBotWithoutStartingAuthorization and friends, so it is intentional; it still needs to be either changed or documented as an explicit carve-out in README's --dry-run row and in CLAUDE.md.

(b) Per-invocation cost against an auth endpoint. botIdentityVerified caches within one process, but nothing caches across invocations. Every single octo-cli mail ... call is now two round-trips, one of them a re-registration. An agent polling mail message changes on a 30s loop re-registers the Bot every 30s indefinitely. Whether that is harmless depends entirely on whether /v1/bot/register rotates im_token or disturbs a live WebSocket session — which this repo cannot answer.

For the human reviewer: please confirm with the backend owners that POST /v1/bot/register is a safe, idempotent, side-effect-free repeat call at that frequency. The PR body already states live end-to-end authorization was never exercised. If it is not idempotent, this is a production-availability issue, not a style point.

To be clear on the good part: verifying the RobotID against an authoritative endpoint before releasing a stored mailbox credential is the right design, and the comment explaining why a stored profile's RobotID is not proof (factory.go:150-155) is correct reasoning. The objection is only to when it runs and to it running under --dry-run.

P1-3 — the mailbox credential is keyed by RobotID alone, although authorization is Space-scoped

cmd/mail_auth.go:80-93 requires a Space and refuses to start the flow without one; cmd/mail_auth.go:105-108 transmits spaceId in the device request. The resulting credential is then stored at cmd/mail_auth.go:230 as store.SaveMailCredential(bot.RobotID, token.AccessToken) — Space discarded. Lookup (internal/cmdutil/factory.go:279-297) is likewise RobotID-only, and every mail request suppresses X-Space-Id (internal/registry/specs/mail.json:11), so the Space never reappears anywhere downstream.

For a platform-scoped Bot with mailboxes in two Spaces, this means the second authorization silently overwrites the first, and afterwards --space A transparently operates the Space-B mailbox. Sending mail from the wrong mailbox is a real-world harm, and the code contradicts itself: it asserts the authorization is Space-scoped at login and then drops that scope at rest.

Either key the credential (and the pending record) by RobotID + SpaceID, or record the authorizing Space in the stored entry and refuse/warn when the active Space differs.

Related, smaller: requiring a Space for mail auth login sits awkwardly with CLAUDE.md's "Space-scoped bots resolve their space server-side" — such a Bot has no SpaceID locally and is blocked from Mail authorization entirely. Worth confirming that is intended.

P2-1 — profile rebind/removal destroys the mailbox credential of an unrelated identity

internal/authstore/authstore.go:189-195 and :250-258

if existed && previous.RobotID != "" {
    delete(mailTokens, previous.RobotID)
    delete(pendingMail, previous.RobotID)
}

The mail credential is keyed by Bot identity, but it is deleted based on a profile-name event. If two profiles point at the same RobotID, or the same Bot is also reachable via OCTO_BOT_TOKEN, then rebinding or logging out of one profile silently revokes Mail for all of them. Recovery requires a fresh human approval round-trip, so this is not a cheap mistake.

Delete mailTokens[previous.RobotID] only when no remaining profile maps to that RobotID.

Also: SaveProfile writes four files with no transaction. RemoveProfile writes profiles/tokens before the mail files, so a mid-sequence failure during auth logout leaves the mailbox credential on disk after the Bot profile is gone. Reordering the mail deletions first would make the failure mode fail-safe.

P2-2 — the legacy profile-name key path is never written and creates a namespace collision

internal/cmdutil/factory.go:279-297, cmd/mail_auth.go:341-358

Both lookups fall back from bot.RobotID to bot.Profile in a single untyped map, described as "compatibility with credentials written by older CLI builds". But this PR introduces Mail — no older build ever wrote mail-credentials.enc, and the only writer here (cmd/mail_auth.go:230) always keys by RobotID. The path is unreachable in practice.

What it does add is a collision surface: if a profile is named the same string as another Bot's RobotID, a verified Bot B can retrieve the entry stored for profile A and send A's mailbox bearer. I am rating this P2 rather than P1 because the write path can never produce such an entry — reaching it requires a hand-authored store file, which implies same-user access that the threat model already concedes. Still: delete the fallback. Zero compatibility value, non-zero confusion risk.

P2-3 — any `401` deletes the stored mailbox credential

cmd/mail_auth.go:295-308

if ee := output.AsExitError(err); ee != nil &&
    (ee.Type == "auth_error" || ee.Code == "unauthorized") {
    _ = store.RemoveMailCredential(credentialKey)

Every 401 maps to auth_error (internal/output/errors.go:103-104, 130), including one produced by the gateway rather than the Mail service. The PR body states the /agent-mail-api gateway boundary is unverified and "must admit those public PKCE bootstrap endpoints while stripping unrelated credentials". If that routing is wrong on day one, the first mail auth status a user runs wipes their valid credential and forces a new human approval — for every Bot, from a purely transport-level misconfiguration.

Narrow the deletion to a Mail-service-authored revocation code, or require two consecutive failures, or simply report unconnected without deleting.

P2-4 — `RESULT_UNKNOWN` does not cover gateway 5xx

internal/client/client.go:1270-1281. Only ee.Type == "network" is converted. A 502/503/504 after the backend has already accepted a send is the canonical ambiguous outcome and the one most likely to appear through a gateway. Extend the mapping to retryable 5xx (and 429) when UnknownOutcomeOnNetworkFailure is set, so a lost mail send is never reported as a clean failure.

P2-5 — no domain separation between the two encrypted credential files

internal/authstore/crypto.go:129-138 seals with gcm.Seal(nonce, nonce, plaintext, nil) — no additional authenticated data. credentials.enc and mail-credentials.enc are derived from the same key and both hold map[string]string, so swapping the files passes GCM authentication and decodes cleanly. Where a profile name coincides with a RobotID, that puts a Bot token on the Mail transport and vice versa.

This mostly sits outside the stated "isolation boundary = OS user" threat model, but it is cheap insurance and matters for backup/restore mix-ups: bind the file purpose into the AAD, or derive per-file subkeys.

P2-6 — downloaded mail bodies and attachments are written world-readable

internal/client/client.go:1043 writes with mode 0o644. mail.message.raw and mail.message.attachment.download (internal/registry/specs/mail.json:114-135, :146-170) are the first operations to route private email content and attachments through that writer. On a shared host every local user can read them. Default binary output for mail operations to 0600, or add a per-operation permission policy.

P2-7 — JMAP `--dry-run` never shows the request the user asked about

cmd/mail_jmap.go:130-137 returns the session-discovery preview and stops, so mail message changes --since-state X --dry-run previews GET /jmap/session and never describes the POST /jmap/api call. The reason given (accountId is undiscoverable without executing) is sound, but the output is misleading as-is — emit both, with a placeholder accountId and a note, or state explicitly in the payload that this is a prerequisite preview only.

P2-8 — `mail.draft.create` is non-retryable but has no idempotency key

internal/registry/specs/mail.json:349-360 carries x-octo-retry: "never" with no X-Octo-Idempotency-Key, unlike its siblings mail.draft.create_agent (:321-330) and mail.message.reply_draft (:261-270), which both require one. On a lost response the caller gets RESULT_UNKNOWN and has no safe way to recover. mail.message.send and mail.draft.send are in the same position, mitigated only by the one-time confirmation token.

P2-9 — a stale pending authorization is never expired locally

cmd/mail_auth.go:135-142 persists ExpiresAt, but mail auth status never reads it: cmd/mail_auth.go:164-171 short-circuits into the token exchange whenever a pending record exists. An abandoned mail auth login therefore masks the connected state until the server happens to answer authorization_expired — at which point :191-196 removes the record but still returns an error rather than falling through to showCurrentMailConnection, so the user needs two invocations to see the truth. Check ExpiresAt before the exchange, and fall through to the current-connection path after clearing an expired record.

Nit

internal/registry/specs/mail.json mixes tab and space indentation (e.g. lines 47-49, 109, 178, 200, 222, 368, 404).

3. Overall verdict

CHANGES_REQUESTED — Spec: ❌ (one out-of-scope global change, one documented-contract deviation, one partially-met acceptance criterion) and Quality: Changes-Requested (three P1). Either gate alone blocks.

4. Suggested direction

P1-1 — keep the empty-store fall-through if the runtime needs it, but stop making it silent. Preferred: echo the unverified claim as identity.robot_id_claimed; better still, reuse the Mail verification path when a bare OCTO_BOT_ID accompanies an env token. Add the missing negative-direction test. Either way, land it as its own commit with its own rationale — it is not a Mail change.
P1-2 — decide and document. Either skip the identity lookup under --dry-run and fail locally with a clear "cannot preview without verifying identity" error, or add an explicit exception to README's --dry-run row and CLAUDE.md. Independently, get backend confirmation that /v1/bot/register is safe to call once per CLI invocation forever.
P1-3 — include the authorizing SpaceID in the stored mail credential key (and the pending record), or persist it and refuse on mismatch.
P2-1 — reference-count RobotID across profiles before deleting a mail token; reorder RemoveProfile so mail deletion happens first.
P2-2 — delete the profile-name fallback entirely.
P2-3 — stop deleting the credential on a bare transport 401.
P2-4/P2-6 — extend RESULT_UNKNOWN to ambiguous 5xx; write mail binaries 0600.

5. Additional observations (not blocking)

The transport-boundary design is the strongest part of this PR. NewMail (internal/client/client.go:744-754) narrowing to {Token, Source} structurally guarantees no X-Space-Id can leak, which is a better guarantee than a flag would give. Confirmed: attempt() sets X-Space-Id only from c.cred.SpaceID (client.go:954).
Confirmed no Bot-token leak to the PKCE bootstrap endpoints: newMailAuthorizationClient passes a nil credential, and attempt() sets Authorization only when c.cred != nil (client.go:951). cfg.BotToken is not consulted for auth.
Confirmed no secret leak via --verbose: response bodies are logged as a byte count only (client.go:995), so the im_token in the bot.register response never reaches stderr. Device code and verifier are correctly declared in SecretValues (cmd/mail_auth.go:176).
markResultUnknown correctly reaches the embedded *ExitError through retryableErr.Unwrap() — worth noting because with DisableRetry the loop returns the wrapper, not the bare error, and it would have silently no-opped without that Unwrap.
mail.json declares no x-octo-allowed-token-kinds, so the token-kind gate in cmd/service/identity.go is inert for Mail. Since it reads the Bot credential rather than the Mail one, that is the right call — but it means a uk_* user key reaching a mail command will fail with an opaque error from /v1/bot/register instead of the local TOKEN_KIND_NOT_ALLOWED. Consider declaring the allowed kinds anyway.
cmd/api.go:89 (the generic passthrough) still uses f.Client(), so octo-cli api GET /agent-mail-api/... sends the Bot token to the Mail route. Pre-existing escape-hatch behaviour, not introduced here, but it now has a reachable target.
mail auth status stores the credential before the caller can compare token.MailboxAddress against the --mailbox that was requested. The octo-mail skill instructs the agent to verify via mail me afterwards, which covers it in practice; a client-side comparison when --mailbox was supplied would be cheaper.

Review coverage and limits

Reviewed: the full diff at 83d26d0, plus the surrounding transport, credential-chain and authstore code the diff depends on but does not touch; issue #128 for scope; and a build-and-run comparison of base vs head for the credential-selection behaviour.

Not covered — please verify manually:

Live /agent-mail-api gateway routing: that it admits the unauthenticated PKCE bootstrap endpoints and strips unrelated credentials. The PR body states this was never exercised end to end, and P2-3 turns a failure here into destroyed local credentials.
Whether POST /v1/bot/register is idempotent and safe to call once per CLI invocation (P1-2).
Server-side Space scoping of Agent mailboxes, which determines whether P1-3 is a correctness bug or only a hygiene issue.
The omb_ token's server-side lifetime, revocation and rotation semantics; the CLI only prefix-checks it.
Backend behaviour of the X-Octo-Confirmation one-time token under concurrent use.

mochashanyao

[Octo-Q · automated review]

Verdict: Approve — no blocking findings; notes below (data-flow traced).

Code Review — PR #129 (octo-cli)

Summary

This PR adds Bot-bound Agent Mail to octo-cli: a PKCE device-flow authorization pair (mail auth login / mail auth status), a 24-operation mail service spec routed through a dedicated mailbox credential boundary (x-octo-credential: mail), hand-written RFC 8621 mail message state / changes commands, an auth update command for profile endpoint metadata, and the supporting factory/client/authstore/credential plumbing plus an embedded octo-mail skill. The security architecture is the interesting part: a separately stored mailbox token is only released after the active Bot token is checked against /v1/bot/register, an env-supplied OCTO_BOT_ID is treated as an unverified claim until that lookup confirms it, and non-idempotent mail writes disable transport retries and surface RESULT_UNKNOWN instead of risking duplicate sends.

Overall this is a careful, well-tested change. I found no P0/P1 issues; three P2 items are described below.

Verification

Static analysis only at head 83d26d03; build and tests were not executed in this environment. Reviewed against merge-base 84655c9 (pr-base...HEAD, 35 files, +3709/-41), including full reads of the new mail command/authstore/factory code, the mail.json spec, the credential provider chain, and the changed test suites.

✅ Mail credential boundary — client.NewMail converts the mailbox token into a transport credential with no SpaceID and no BotKind; the mail client is only reachable through ClientForCredential(ctx, "mail"), used by spec ops declaring x-octo-credential: mail (all /agent-mail-api/*) and the hand-written JMAP/identity commands. No path sends the mail token to Bot APIs or a Bot token to mail APIs. TestMailCommandUsesMailEndpointAndCredential asserts the Authorization header and the absence of X-Space-Id on the wire.
✅ Identity verified before credential release — MailCredentialFunc always goes through VerifyBotIdentity, which calls /v1/bot/register even when a profile claims a RobotID and even under --dry-run (botIdentityClient forces DryRun: false for the prerequisite). Claimed-vs-resolved mismatch is a hard auth_error; covered by TestFactory_MailCredentialRejectsStoredProfileTokenSwap, TestFactory_MailCredentialRejectsUnverifiedRuntimeBotIdentity, and TestMailAuthRejectsClaimedBotIDThatDoesNotOwnToken, all driving the real factory against httptest servers.
✅ Token exchange validation — the token response must carry an omb_-prefixed access token, a non-empty mailbox address, and a botId matching the verified RobotID before anything is persisted.
✅ Profile lifecycle cascades — SaveProfile deletes mail/pending keys on identity change or legacy-name reuse (keeping the correct binding when the default RobotID-named profile is created), and RemoveProfile cleans both key forms; fail-closed direction, covered by internal/authstore/mail_test.go.
✅ Validation gate for nested auth — the skipValidation annotation is leaf-only and the parent-chain walk denies mail auth login/status the credential-free exemption reserved for top-level auth; pinned by TestSkipValidation.
✅ Idempotency and secrets — send/delete/reply/forward/draft ops declare x-octo-retry: never, wired to DisableRetry + UnknownOutcomeOnNetworkFailure → RESULT_UNKNOWN on network failure; confirmation tokens are x-octo-secret headers and collectSecrets does collect header-flag values for verbose/dry-run masking.

Findings

No P0/P1 issues; three P2 items below.

P2 — `auth update` persists the API base URL without normalization (`cmd/auth.go:67`)

The new command only trims --api-base-url before store.UpdateProfileAPIBaseURL, while auth login (cmd/auth.go:144) rejects anything that fails config.NormalizeAPIBaseURL. The write pair is asymmetric: update can persist values (wrong scheme, embedded credentials/query/path components) that the runtime then rejects in overlayProfileBaseURL (internal/cmdutil/factory.go), so every subsequent command for that profile fails the validation gate with "profile has an invalid API base URL". This is fail-closed — no token is ever sent to a malformed URL — but the command accepts input it can never use, introduced by this PR. Run config.NormalizeAPIBaseURL inside newAuthUpdateCmd and fail with ErrValidation, mirroring login; the overlay error hint could also point at auth update now that it exists.

P2 — `config show` echoes an unverified `OCTO_BOT_ID` claim as `robot_id` (`internal/cmdutil/factory.go:270`)

buildCredential now attaches OCTO_BOT_ID to env-token credentials here (cred.RobotID = botID when no profile is active), and the PR gates the success-envelope echo on exactly this distinction: identityValue only emits robot_id when a profile backs it or /v1/bot/register verified it (internal/cmdutil/factory.go:561 — "Do not echo an unverified claim as if it were identity"). config show (cmd/config.go:72) reads cred.RobotID without that gate, so for env-token runtimes it reports a caller-supplied claim as resolved identity; a stale or wrong OCTO_BOT_ID becomes the bot's reported identity with no unverified marker. This is a diagnostic, not an authorization input, so it is not blocking — but it contradicts the contract this PR introduces. Apply the same gate in config show (suppress robot_id unless profile-backed or verified) or label it as a claim.

P2 — Expired pending mail authorizations are never swept client-side (`cmd/mail_auth.go:141`)

PendingMailAuthorization records ExpiresAt, but neither storedPendingMailAuthorization nor login checks it; an expired pending entry stays in mail-authorization.enc until a mail auth status call receives authorization_expired/used/denied from the server. Every reachable path recovers (login overwrites per-bot, status clears on terminal server codes) and the store is encrypted with one entry per bot, so impact is one guaranteed-failing token exchange and a stale file entry. Treating a locally expired pending as absent would close it.

Human-verify

The CLI treats the server as authoritative for identity and authorization; the actual behavior of /v1/bot/register, the device/token endpoints (error codes authorization_pending/authorization_expired/authorization_used/authorization_denied, response fields accessToken/botId/mailboxAddress), and the JMAP session/api endpoints cannot be confirmed from this repo. Worth one integration pass against octo-mail. Not a merge blocker for this PR.
mail.json encodes the WebAPI contract (paths, omb_ token prefix, confirmation-token semantics, idempotency-key requirements). Confirm it matches the deployed octo-mail WebAPI version. Not a merge blocker for this PR.

Things I checked that are fine

Docs/CLI parity: every command and flag in skills/octo-mail/SKILL.md (--idempotency-key, --draft-version, --addKeywords, --confirmation-token, --mailbox, --output on binary ops) maps to a real generated or hand-written flag; loader test pins 24 mail operations and the README/CLAUDE.md counts (12 domains / 308 ops) add up.
Device flow: PKCE S256 with a 32-byte verifier; device/token requests disable retries and mark network failures RESULT_UNKNOWN; pending proof material is persisted only in the encrypted store with device code and verifier declared secret material.
Email/get with empty ids fetches state only; changes requires --since-state and positive --max-changes, and the full JMAP response (including newState/hasMoreChanges) is emitted for the agent-side state ledger.
The updated api_secrets_test.go tripwire correctly allows header-position secrets now that generated commands collect them via bound flags, while the generic api passthrough still cannot receive arbitrary headers.
Required X-Octo-Idempotency-Key headers are registered as required flags (MarkFlagRequired), so send-intent/reply-draft/create-agent cannot be invoked without a key.

Verdict: APPROVED

The credential-boundary and identity-verification design is sound and covered by production-path tests; no finding makes a working path unavailable, produces wrong data, or widens access. The three P2 items are worth addressing (the auth update normalization gap most of all) but none is blocking.

yujiawei · 2026-08-13T10:15:10Z

Addendum to my review — two more issues, plus stronger evidence for P1-1

Follow-up on 83d26d0. Two additional findings, both reproduced against a built binary. Verdict is unchanged (CHANGES_REQUESTED); neither of these is itself blocking, but the second one materially sharpens P1-1, so please read them together with the original review.

P2-10 (new) — `auth update` skips the URL normalization that `auth login` enforces, and can leave a profile unusable

cmd/auth.go:41-52 trims --api-base-url and writes it straight through to store.UpdateProfileAPIBaseURL. auth login (cmd/auth.go:144-149) runs config.NormalizeAPIBaseURL first and rejects a bad value with ErrValidation. The write pair is asymmetric, so update persists values the runtime will then refuse in overlayProfileBaseURL:

$ octo-cli auth login --bot-id ROBOT_A --profile p1 --with-token
$ octo-cli auth update --profile p1 --api-base-url "https://evil.example.com/some/api/path?x=1"
{"data":{"api_base_url":"https://evil.example.com/some/api/path?x=1","profile":"p1","robot_id":"ROBOT_A"},"ok":true}

$ octo-cli message send --data '{...}' --profile p1 --dry-run
{"error":{"code":"CLI_ERROR","type":"config",
  "message":"profile \"p1\" has an invalid API base URL: OCTO_API_BASE_URL must be an absolute base URL
             without credentials, query parameters, or fragments;
             update it with `octo-cli auth login --profile p1`"},"ok":false}

The command reports ok: true for a value that makes every subsequent command on that profile fail. This is fail-closed — no token is ever sent to the malformed URL, which is the important part — and it is self-recoverable, since auth update sits under the top-level auth subtree and so bypasses the validation gate:

$ octo-cli auth update --profile p1 --api-base-url "https://good.example.com"   # recovers
$ octo-cli message send ... --profile p1 --dry-run
    "url": "https://good.example.com/v1/bot/sendMessage"

Two fixes:

Call config.NormalizeAPIBaseURL in newAuthUpdateCmd and fail with ErrValidation, mirroring login.
Update the overlayProfileBaseURL hint. It currently says "update it with octo-cli auth login --profile p1", which sends the user through a full token re-entry — precisely what auth update was added to avoid. It should name auth update.

P2-11 (new) — `config show` reports an unverified `OCTO_BOT_ID` as resolved identity, contradicting this PR's own contract

cmd/config.go:72 reads cred.RobotID with no gate. internal/cmdutil/factory.go:559-563 — added by this PR — gates the same field in the success envelope, with the comment "A RobotID from OCTO_BOT_ID is caller-supplied until /v1/bot/register confirms it. Do not echo an unverified claim as if it were identity." The two paths disagree, same process, same credential:

$ OCTO_CONFIG_DIR=/tmp/empty OCTO_BOT_TOKEN=bf_tokenForBotB OCTO_BOT_ID=ROBOT_A_CLAIMED \
  octo-cli config show
    "bot_token_source": "env:OCTO_BOT_TOKEN",
    "profile": null,
    "robot_id": "ROBOT_A_CLAIMED"        <-- caller-supplied, never verified

$ (same env) octo-cli message send --data '{...}' --dry-run
  "identity": {
    "bot_kind": "user_bot",
    "source": "env:OCTO_BOT_TOKEN",
    "type": "bot"                        <-- robot_id correctly withheld
  }

Apply the same gate in config show, or emit it under a distinct key that marks it as a claim.

Why P2-11 raises the stakes on P1-1

In my review I argued that the empty-store fall-through in internal/credential/file_provider.go:44-52 makes a mismatched --bot-id invisible — the command runs as whatever Bot the env token belongs to, and the envelope withholds robot_id entirely.

P2-11 shows it is not merely invisible. config show is the command an operator or agent runs to answer "which Bot am I acting as?", and in exactly this configuration it answers with the claim, confidently and unlabelled. So the sequence is:

--bot-id ROBOT_A with an env token belonging to Bot B, empty profile store.
Pre-PR: hard auth_error, exit 3. Post-PR: the command runs as Bot B.
The operator runs config show to diagnose and is told robot_id: ROBOT_A.

That is an actively wrong signal from the designated diagnostic command, not just a missing one. It does not change P1-1's severity, but it does mean fixing P1-1 by "echo the claim as robot_id_claimed" needs to cover config show too — otherwise the two commands stay inconsistent in the opposite direction.

Note on verification method

The findings in my review and here were checked against built binaries, not by reading alone: go build ./..., go vet ./... and go test ./... -count=1 all pass at 83d26d0, and the P1-1 and P2-10/P2-11 behaviours were reproduced by compiling both 84655c9 and 83d26d0 and running the same commands against each. Where I cite an output above, it is copied from a real run.

Jerry-Xin

Summary: The PR is in scope for octo-cli and the mail credential boundary, auth flow, retry policy, and embedded skill are generally well aligned with the existing architecture.

💬 Non-blocking

🟡 Warning: auth update --api-base-url stores the supplied URL without normalizing or validating it, unlike auth login. That means a command can succeed while persisting a URL that later breaks profile use during config overlay. Consider applying config.NormalizeAPIBaseURL before UpdateProfileAPIBaseURL. See cmd/auth.go:49-67. The follow-up error hint also still points to auth login instead of auth update at internal/cmdutil/factory.go:386-390.

✅ Highlights

Clear separation between Bot credentials and Mail credentials via ClientForCredential.
Mail side-effect operations disable retries and surface RESULT_UNKNOWN for ambiguous outcomes.
Good coverage for dry-run no-network behavior, token binding, space ambiguity, and secret redaction.

Validation run: go test ./... -count=1 passed.

Superseded by an enriched APPROVE at the same head with per-claim evidence.

Jerry-Xin

Re-review of head 08a512e (base main, merge-base 84655c9). In scope for octo-cli. APPROVE. go build ./..., go vet ./..., and go test ./... -count=1 all pass locally.

This head addresses the three blocking items @yujiawei raised at 83d26d0 (their P1-1 / P1-2 / P2-4). I verified each on the current bytes. Credit to @yujiawei — all three were legitimate catches and all three are now genuinely fixed.

Per-claim verification (all fixed at `08a512e`)

Y3 — global credential-selection change bled into non-mail commands (was P1-1): FIXED.
At 83d26d0, internal/credential/file_provider.go made the empty-store fall-through unconditional (if p.ExplicitProfile == "" && p.ExplicitBotID != ""), so an explicit --bot-id against an empty store silently ran as a different env Bot with no signal — affecting message send, docs, drive, html, everything.

Fix mechanism: the fall-through is now gated by a new AllowEmptyStoreBotIDFallback flag (internal/credential/file_provider.go:25-28,49). That flag is set to true only when the RobotID came from OCTO_BOT_ID env and the --bot-id flag was empty (internal/cmdutil/factory.go:266-272). An explicit --bot-id therefore keeps AllowEmptyStoreBotIDFallback=false and remains fail-closed on an empty store — restoring the pre-PR hard error for the flag path.
The remaining permissive case (bare OCTO_BOT_ID env token, the runtime shape #128 requires) is no longer silent: the unverified claim is echoed as identity.robot_id_claimed instead of being dropped (internal/cmdutil/factory.go:568-582), so a mismatch is observable. This is exactly @yujiawei's preferred remedy (option 1). Documented in CLAUDE.md. Covered by internal/credential/file_provider_test.go.

Y4 — --dry-run still issued a real network POST (was P1-2 / doc deviation): FIXED.
At 83d26d0, botIdentityClient() explicitly built a client with DryRun: false (factory.go:506) so MailCredentialFunc → VerifyBotIdentity → resolveBotIdentity fired a live write-risk POST /v1/bot/register even under --dry-run.

Fix mechanism: that DryRun:false client is removed; botIdentityClient() now just returns the ordinary client (internal/cmdutil/factory.go:516-521), and resolveBotIdentity returns a local validation error before any network call under dry-run (internal/cmdutil/factory.go:462-467).
The mail request path no longer force-verifies at all: MailCredentialFunc now resolves via local CredentialFunc() + exact local binding lookup (FindMailCredential) instead of VerifyBotIdentity (internal/cmdutil/factory.go:151-166). mail auth login passes verify=!DryRun and mail auth status passes verify=false (cmd/mail_auth.go:79,181,283-292).
No DryRun:false construction remains anywhere in non-test code. New tests enshrine the corrected contract: TestMailAuthLoginDryRunDoesNotVerifyBotOrStartAuthorization, TestMailAuthStatusDryRunDoesNotVerifyBotOrProbeMailbox. --dry-run is now network-free.

Y5 — ambiguous gateway 5xx did not return RESULT_UNKNOWN (was P2-4): FIXED.
At 83d26d0, markResultUnknown only converted ee.Type == "network" (client.go:1275), so a 502/503/504 after a non-idempotent send surfaced as an ordinary api_error.

Fix mechanism: retryableErr now carries status (set in attempt() and preserved through redactError), and markResultUnknown converts 502/503/504 to RESULT_UNKNOWN in addition to network failures (internal/client/client.go:1276-1289). New regression test TestDo_AmbiguousGatewayFailureReturnsResultUnknown covers all three statuses plus a negative case.
Note: 429 is intentionally not mapped — it is an unambiguous rate-limit rejection (request not accepted), not an ambiguous accepted-then-lost outcome, so excluding it from RESULT_UNKNOWN is the correct call.

Honest correction on my prior review

My APPROVE at 83d26d0 (now dismissed) was an under-call. All three issues (Y3/Y4/Y5) were already present in the source at 83d26d0, but I scoped that review to the 1aa847 → 83d26d delta (a single lint commit) and re-confirmed only the mail-auth boundary; I did not independently surface these three from the broader source. @yujiawei caught them correctly. This head fixes all three.

Related items also improved

P1-3 (Space-scoping): the mailbox credential is now keyed by RobotID + SpaceID + Bot-token fingerprint (internal/authstore/mail.go:38-45), so a second-Space authorization no longer silently overwrites the first. Space ambiguity returns a clear error.
Mail credential files remain 0600; no Bot/Mail token is logged; PKCE bootstrap endpoints send no credential.

Non-blocking (carried)

🟡 auth update --api-base-url still stores the value without config.NormalizeAPIBaseURL (cmd/auth.go:48-67), unlike auth login. Not a security regression: use-time overlayProfileBaseURL re-normalizes and hard-fails on any malformed/credential-bearing stored value, so no such value ever builds an outbound request. Parity fix + test still recommended; the error hint at internal/cmdutil/factory.go could also mention auth update.
🟡 Generic download writer uses 0o644 (internal/client/client.go:1045); mail bodies/attachments inherit it. Consider 0600 for mail binary output on shared hosts.

Verdict

APPROVE — Y3/Y4/Y5 are all genuinely resolved on 08a512e with file:line evidence and regression tests; remaining items are non-blocking. Thanks @yujiawei for the three catches.

yujiawei

Code Review — PR #129 (octo-cli)

Reviewed at head 08a512e, merge-base 84655c9 (35 files, +4125/−47). Built, vetted and ran the full suite locally against this head: go build ./..., go vet ./..., go test ./... -count=1 — all green. Findings below were reproduced against a binary built from this head unless marked otherwise.

Verdict: Changes requested. The Agent Mail core (binding-key design, dedicated encrypted file, fail-closed ambiguity handling, dry-run containment, test depth) is genuinely well built. The blocking issues are (1) a validation gate that exists on auth login but is missing on the new sibling auth update, and (2) the mail binding not covering the API origin, which the first issue makes reachable.

P1 — blocking

1. `auth update --api-base-url` bypasses the URL validation `auth login` enforces

cmd/auth.go:49 trims the value and cmd/auth.go:67 writes it straight to the store. The sibling path, cmd/auth.go:145, runs config.NormalizeAPIBaseURL first and rejects anything that is not a bare http(s) origin.

Reproduced with a binary from this head:

$ octo-cli auth login  --bot-id cli_other --with-token --api-base-url "https://evil.example/fleet/api?x=1"
{"error":{"code":"VALIDATION_ERROR",
 "message":"OCTO_API_BASE_URL must be an absolute base URL without credentials, query parameters, or fragments",
 "type":"validation"},"ok":false}

$ octo-cli auth update --bot-id cli_demo --api-base-url "https://evil.example/fleet/api?x=1"
{"data":{"api_base_url":"https://evil.example/fleet/api?x=1","profile":"cli_demo",...},"ok":true}

$ octo-cli auth update --bot-id cli_demo --api-base-url "not a url at all"
{"data":{"api_base_url":"not a url at all",...},"ok":true}

$ octo-cli mail me --bot-id cli_demo --dry-run
{"error":{"code":"CLI_ERROR","type":"config",
 "message":"profile \"cli_demo\" has an invalid API base URL: ... ; update it with `octo-cli auth login --profile cli_demo`"},"ok":false}

Two consequences:

Data integrity / self-inflicted DoS. The command reports ok: true, then overlayProfileBaseURL (internal/cmdutil/factory.go:387) re-validates on read and hard-fails. Every subsequent service command on that profile is bricked, and the error text points the user at auth login — the command whose validation was just bypassed.
Security. This field decides where the Bot bearer and (new in this PR) the mailbox bearer are sent. A well-formed but attacker-chosen origin (http://attacker.example) is accepted and persisted silently, with no scheme requirement and no confirmation. In an agent runtime that reads untrusted mail, a single injected command creates a redirect that outlives the turn.

Fix: call config.NormalizeAPIBaseURL in newAuthUpdateCmd exactly as login does, and store the normalized value.

This is also a test-discipline gap by the repo's own rule ("Security boundaries additionally need a test in each direction: the unsafe input is refused and the legitimate one still works"). TestAuth_UpdateAPIBaseURLPreservesCredentials and TestAuth_UpdateAPIBaseURLRequiresStoredProfile cover the happy path and the missing-profile path; neither asserts a malformed URL is refused, which is why the gap survived.

2. The mail binding key does not cover the API origin

MailBindingKey (internal/authstore/mail.go:38) binds RobotID + SpaceID + SHA-256(bot token). MailClientFunc (internal/cmdutil/factory.go:507) then builds the transport from whatever cfg.APIBaseURL currently resolves to, and FindMailCredential never consults the origin.

Reproduced by seeding a credential for robot-1/space-1 and then pointing the CLI elsewhere:

$ OCTO_API_BASE_URL=https://other-origin.example octo-cli mail me --dry-run
{"data":{"dry_run":true,
  "headers":{"Authorization":"Bearer ***"},
  "url":"https://other-origin.example/agent-mail-api/webapi/v0/identity"}, "ok":true}

The mailbox bearer authorized against one gateway is attached, unchanged, to a request against a different one, with no local signal. This repo has a staging gateway (im-test.deepminer.com.cn appears in internal/config/config_test.go), so cross-environment leakage is a realistic accident, not just a theoretical one.

To be fair to the design: OCTO_API_BASE_URL has always been able to redirect the Bot token, so on its own this is consistent with the existing model, and I would not block on it in isolation. What changes the weight is finding 1 — after it, the origin for a stored profile can be repointed persistently without possessing the token, and the credential now redirected is a separate, longer-lived, auto-attached mailbox token. The PR's own documentation (CLAUDE.md) enumerates the binding dimensions as RobotID, SpaceID and Bot-token fingerprint; origin is the missing one.

Fix options: include the normalized origin in the binding key, or record the authorizing origin alongside the credential and refuse (or at minimum warn) on mismatch.

P2 — non-blocking

internal/authstore/authstore.go:191-203 — write ordering in SaveProfile. Mail bindings are purged and flushed before saveTokens/saveProfiles. A transient failure on the later write loses mail access while leaving the profile unchanged. Fail-closed and recoverable by re-authorizing, but the ordering could be inverted.
internal/authstore/authstore.go:189 + internal/authstore/mail.go:334 — over-broad purge on a space-only change. scopeChanged triggers deleteMailBindingsForRobot, which deletes every binding for that RobotID across all Spaces. Re-running auth login --profile p --space S2 for a bot that legitimately holds bindings in S1 and S2 destroys the S2 binding the user is switching to. The key already encodes space + token fingerprint, so nothing is unlocked by keeping the other space's binding; scope the delete to the previous Space.
internal/authstore/authstore.go:260-268 — RemoveProfile ordering. Profiles and tokens are saved before mail tokens. A failure on the last two writes leaves an orphan mailbox token on disk after a logout that reported success.
cmd/mail_auth.go:248-253 — non-transactional completion. If SaveMailCredential succeeds and RemovePendingMailAuthorization fails, the caller sees an error for an authorization that actually succeeded. The next mail auth status self-heals, but the first run misreports.
internal/client/client.go:1045 — --output writes mode 0o644. Pre-existing, but this PR makes it reach mail message raw (a full private RFC822 message) and mail message attachment download. On a shared host that is world-readable mail. Consider 0o600, at least for mail.
internal/client/client.go:1276 — markResultUnknown coverage. Only transport errors and 502/503/504 become RESULT_UNKNOWN. A 500 or 408 returned after a send was committed still surfaces as an ordinary API error, so an agent may re-send. Worth widening for x-octo-retry: never operations, since those are exactly the non-idempotent ones.
internal/registry/specs/mail.json:476 and :501 vs cmd/service/mail_test.go:272 — contract disagreement. The spec declares submissionIds as array<integer>; the PR's own test stub returns ["S1"] (strings). One of them is wrong, and the spec is what octo-cli schema shows to consumers.
internal/cmdutil/factory.go:436 — ResolveBotIdentity has no caller. Consequently the --dry-run guard at factory.go:464 is unreachable: selectedMailBot passes verify=false whenever --dry-run is set, so the only entry point is VerifyBotIdentity on the non-dry-run path. (I verified mail auth login --dry-run with an environment token + OCTO_BOT_ID works correctly and never reaches it.) Either wire it up or delete it.
cmd/mail_auth.go:365 vs internal/cmdutil/factory.go:512 — inconsistent trust promotion. Both match a stored binding by the same token fingerprint, but only the factory sets botIdentityVerified. As a result mail auth status emits robot_id_claimed while mail me emits robot_id for an identical trust basis. Harmless but confusing for envelope consumers.
cmd/mail_auth.go:145 — device-flow metadata. ExpiresAt is persisted but never enforced locally (the CLI waits for the server to say authorization_expired), and device.Interval is parsed and discarded rather than surfaced in the authorization_required payload — so a polling agent has no server-suggested cadence. Also, if the server omits expiresIn, expires_at is emitted as "now".
cmd/mail_jmap.go:165 — strict method-response count. Rejecting any response where len(methodResponses) != 1 is brittle; RFC 8620 permits a server to include additional responses. Prefer selecting by name/call-id.
Scope. octo-cli auth update is a new user-facing command unrelated to "add Bot-bound Agent Mail commands", and the PR description is empty. That is not a defect by itself, but the one blocking bug in this PR is in that unrequested addition — a short PR body flagging it would have drawn the right eyes.

Please verify manually (this PR touches auth and credential storage)

The device endpoint is called unauthenticated. newMailAuthorizationClient (cmd/mail_auth.go:269) passes a nil credential, so no Authorization header is sent, and botId / botProfile / mailboxAddress come from the request body. VerifyBotIdentity is a client-side check only — a modified client can request a device code for any botId. Please confirm the server binds approval to the authenticated human approver and does not trust those body fields.
mail auth login issues POST /v1/bot/register purely as an identity probe. That operation is declared x-octo-risk: write and its response carries im_token. Please confirm registration is idempotent and side-effect-free here, and that using a write endpoint for identity verification is intended.
Local store integrity is the trust root for the robot_id promotion at internal/cmdutil/factory.go:512. CLAUDE.md states the store resists off-machine leakage but not a same-user process; worth an explicit sign-off that promoting a locally-matched binding to authenticated identity is acceptable under that model.

Coverage — what this review could not check

Backend contracts are out of reach from the diff: the device/token endpoint semantics, JMAP server behavior, X-Octo-Confirmation issuance and one-time-use, and the accuracy of the response schemas in mail.json. I also did not check for post-merge semantic conflict with other open branches in this repo (each may be individually mergeable without the merged result being green), and I did not validate skills/octo-mail/SKILL.md against real backend behavior — only against the command surface in this diff.

What is good

Worth saying explicitly, because most of this diff is strong: the binding-key design (fingerprinting the authorizing Bot token so a rotated token cannot unlock an old mailbox) is the right call; multi-space matches fail closed rather than guessing; --dry-run is correctly prevented from creating a device flow or persisting proof material, and that is tested in both directions; the robot_id / robot_id_claimed split is a genuine improvement to envelope honesty and is purely additive; mail tokens live in a dedicated encrypted file with independent lifecycle; and the test suite is unusually thorough for a feature this size, including the negative cases (token swap, unmatched fingerprint, space ambiguity). Fix the two P1s and this is in good shape.

yujiawei · 2026-08-13T11:09:00Z

One addition to my review above, spotted on a second pass — not a new blocker, just something worth folding into the same round of fixes:

README.md:38-51 — the Domains table has no mail row. The table lists 12 domains (docs, html, drive, matter, summary, group, thread, bot, message, file, event, loop) and this PR bumps CLAUDE.md to "12 active domains, 308 operations" and adds mail to the README examples and skills list — but the table itself was not updated, even though mail (24 ops) is the headline feature of the PR. One-line fix. (marketplace is also missing from that table, but that predates this PR.)

Also minor, while you are in internal/client/client.go: the status: re.status field copy added to redactError (:791) has no reader. markResultUnknown is the only consumer of re.status (:1280), and it runs inside do() (:883), which is strictly before Do() applies redactError (:773). Same category as the unused ResolveBotIdentity export — harmless, but dead.

Jerry-Xin

APPROVE — head 1c56c73d. This commit directly addresses both of yujiawei's REQUEST_CHANGES items (Z1, Z2) plus the missing-test 🟡. Verified by byte-diff 08a512ef..1c56c73d (clean linear +1 commit, not a rebase) and full source trace. Build + go test ./... green.

Z1 — `auth update` URL validation (yujiawei — legitimate catch, now FIXED)

cmd/auth.go:55 now runs config.NormalizeAPIBaseURL(apiBaseURL) and returns ErrValidation before persisting; the normalized value is stored. NormalizeAPIBaseURL (internal/config/config.go:104) rejects non-http(s) scheme, empty host, userinfo, query, ForceQuery, fragment, and any service path — exactly the illegal-URL classes raised. This brings auth update to parity with auth login.

Severity on prior head 08a512: this was a fail-fast/consistency 🟡, not a blocker — use-time overlayProfileBaseURL (factory.go:394) + config.go:93 already re-normalize and hard-error before any network, so a raw-stored bad URL could never reach the wire. My prior APPROVE correctly flagged it as non-blocking 🟡. It is now additionally fixed at write-time (defense-in-depth + fail-fast at the point of entry).

Z2 — mail credential key missing API origin (yujiawei — legitimate catch; my prior APPROVE under-called this, now FIXED)

Binding key migrated v1:robot:space:tokenFp → v2:robot:space:originFp:tokenFp (internal/authstore/mail.go:47). MailBindingKey now requires a non-empty apiOrigin, normalizes it, and fingerprints it into the key; matchingMailBindings requires BOTH origin and token fingerprints to match (mail.go:70-83). All mint/lookup paths thread cfg.APIBaseURL consistently: login mint (mail_auth.go:151), pending (198), status (253), current-connection (304), and the factory mail-request path (factory.go:171) — mint and use bind the SAME origin, so they cannot drift.

Incremental-capability assessment: On 08a512 the key omitted origin, so RobotID+Space+token on a DIFFERENT api-base-url could select a mailbox cred minted against another gateway origin. That is a real cross-origin credential-reuse exposure, not merely cosmetic. Tests now prove closure: FindMailCredential("bot-a","space-a","app_a","https://other.example") → ErrMailCredentialNotFound (mail_test.go), i.e. a cred minted against origin A is not selectable when the profile points at origin B. Origin normalization (origin vs origin/ equal keys), origin-not-leaked-in-key, and pending-flow origin isolation are all covered.
Honest note: my prior APPROVE(08a512) did NOT raise Z2 — that was an under-call. yujiawei correctly caught it; credit to them.

🟡 test coverage — now added

TestAuth_UpdateAPIBaseURLRejectsInvalidOriginWithoutChangingProfile (cmd/auth_test.go) asserts validation error AND that the stored profile is unchanged for non-URL and query-param inputs.

Migration safety (v1 → v2)

parseMailBindingKey requires exactly v2 + 5 parts (mail.go:61), so any legacy v1 key parse-fails and is never selected — fail-closed, not silently reused. deleteMailBindingsForRobot sweeps both v1/v2 prefixes on rebind. Security-positive migration.

Regression re-confirm

Mail-auth model intact (mailbox omb_ cred only; Bot token never sent in mail calls). Y1–Y5 not regressed. Cred file 0600 (authstore.go:31). No token/origin in logs. No injection. Build + go test ./... green.

Verdict: APPROVE. Both Z1 and Z2 are genuinely resolved with tests. Z1 was a 🟡 across rounds (fail-fast already prevented network use) and is now also fixed at write-time; Z2 was a real incremental-capability gap that my prior APPROVE under-called and is now closed and tested. Credit to yujiawei for both catches.

Add Bot- and Space-bound Agent Mail authorization, encrypted mailbox credentials, guarded send and Draft workflows, JMAP polling support, and agent-facing skill guidance. Keep dry-run network-free, bind mailbox credentials to the authorizing Bot token and normalized API origin, and surface ambiguous send outcomes safely.\n\nRefs Mininglamp-OSS#128

yujiawei

Code Review — PR #129 (octo-cli)

Reviewed at head SHA 5c5c57c7, diffed against merge base 84655c9. I built the branch, ran the full test suite, go vet, and gofmt, exercised the new commands against the built binary, and cross-checked the wire contract against the merged server PR Mininglamp-OSS/octo-mail#48.

Local verification: go test ./... -count=1 all packages pass, go vet ./... clean, gofmt -l . clean.

1. Spec compliance

Spec: ✅

Checked against the goal, proposed solution, out-of-scope list, and acceptance criteria of #128.

Missing: none. Every acceptance criterion has an implementation and a matching test:
- device flow for both stored-profile and OCTO_BOT_TOKEN runtimes — cmd/mail_auth_test.go:30, :189;
- claimed-RobotID rejection before any credential is stored — internal/cmdutil/factory.go:511-515 + cmd/mail_auth_test.go:277;
- secrets encrypted, RobotID-scoped, excluded from output, cleared on profile removal/rebind — internal/authstore/mail.go, internal/authstore/authstore.go:163-209 / :234-268, internal/authstore/mail_test.go:150, :233, :253;
- mailbox credential on /agent-mail-api with no Bot token and no X-Space-Id — internal/client/client.go:751-757 drops SpaceID at the transport boundary, plus x-octo-space-header: false in internal/registry/specs/mail.json; verified by cmd/service/mail_test.go:21;
- read / binary-download / send / reply / draft / delivery / RFC 8621 paths all covered;
- x-octo-retry: never on every side-effecting operation, with RESULT_UNKNOWN on ambiguous transport failure.
Out of scope: nothing added. No mailbox-token env var, no separate mail base URL, no policy logic in the CLI, no backend contract change.
Deviations: none in the harmful direction. The binding key is stronger than the issue asked for — #128 says "keyed by RobotID", the implementation keys by RobotID + SpaceID + normalized-origin fingerprint + Bot-token fingerprint (internal/authstore/mail.go:37-52). That is a deliberate tightening, and the fail-closed behaviour on origin swap and token swap is directly tested (internal/cmdutil/factory_test.go:633, :770, :808).

One scope note, not a defect: identity.robot_id_claimed and the BotKind/RobotID population in EnvProvider change the success envelope for every domain, not only mail. I confirmed this on the built binary — octo-cli group list --dry-run with OCTO_BOT_ID set now emits identity.robot_id_claimed. This is explicitly declared in the PR description and documented in CLAUDE.md, so it is intentional; flagging it only so downstream envelope consumers are aware.

2. Code quality

Quality: Approved — no P0/P1 defects found. The credential-boundary design holds up under adversarial reading, and the test suite asserts the security properties rather than merely exercising the happy path (token-swap rejection, origin rejection, space ambiguity fail-closed, at-rest encryption, dry-run network-freedom, secret redaction are each pinned by a dedicated test).

Verified as correct (points that were challenged and survived)

PKCE method is not missing. cmd/mail_auth.go:118 sends codeChallenge without a codeChallengeMethod field. Under RFC 7636 an omitted method defaults to plain, which would break the exchange — but this is not RFC 7636 over the wire, it is the proprietary agent-auth contract. The merged server (octo-mail#48) has no codeChallengeMethod field in its request struct and hard-validates the challenge as a raw base64url SHA-256 digest (codeChallenge must be a base64url SHA-256 digest, rejecting anything whose decoded length ≠ sha256.Size). Client and server agree. No change needed.
No Bot↔Mail credential crossover. client.NewMail is the only conversion, it is private to the transport boundary, and the mail client is stored in a separate factory field (internal/cmdutil/factory.go:69-70). I checked every f.Client() call site in the tree; the remaining ones are docs/drive/api leaves that never touch /agent-mail-api. routeSearchPath does not match mail paths. MaskToken returns a bare *** for the unknown omb_ prefix, so the mailbox token cannot partially leak into verbose or dry-run header output.
--bot-id remains fail-closed. AllowEmptyStoreBotIDFallback is set only when the id came from OCTO_BOT_ID (internal/cmdutil/factory.go:269-276), and even then only bypasses the file provider when the store is empty. An explicit --bot-id with an empty store still errors — pinned by internal/credential/file_provider_test.go:130-135.
mail auth login is correctly behind the auth gate. The skipValidation narrowing at cmd/root.go:137-143 is right: the skipValidation annotation is deliberately non-inherited, and the parent walk only exempts a top-level auth. I confirmed on the binary that octo-cli mail auth login with no credential fails UNAUTHORIZED.

P2 — non-blocking

RESULT_UNKNOWN skips HTTP 500 — internal/client/client.go:1279-1281
```
ambiguousGatewayStatus := errors.As(err, &re) &&
    (re.status == http.StatusBadGateway || re.status == http.StatusServiceUnavailable || re.status == http.StatusGatewayTimeout)
```
isRetryableStatus (line 1291) does not include 500, so a 500 never becomes a retryableErr and re.status is never 500. A mail backend that accepts a send and then panics while building its 202 response returns a plain api_error, and the agent-facing skill guidance ("RESULT_UNKNOWN means … inspect before a manual retry", skills/octo-mail/SKILL.md:268) does not fire. For an operation carrying x-octo-retry: never, a 500 is exactly as ambiguous as a 502. Consider treating any 5xx as unknown-outcome when UnknownOutcomeOnNetworkFailure is set.
Unknown x-octo-credential values fail open to the Bot client — internal/cmdutil/factory.go:427-436
```
if kind == "mail" { ... }
return f.Client()
```
A typo or a future value in a spec silently sends the Bot token instead of failing loudly. Given this is the security boundary the whole PR is built around, an explicit allowlist ("" | "mail", anything else → internal error) costs three lines and removes a whole class of future spec bugs.
A gateway-level 401 silently deletes a valid stored mailbox credential — cmd/mail_auth.go:334-336
```
if ee := output.AsExitError(err); ee != nil &&
    (ee.Type == "auth_error" || ee.Code == "unauthorized") {
    _ = store.RemoveMailCredential(credentialKey)
```
typeFromStatus maps any 401 to auth_error. Since the PR description states the /agent-mail-api gateway routing was not exercised end-to-end, the realistic first failure mode is the gateway itself answering 401 (e.g. it expects a Bot bearer and strips/rejects the omb_ one). In that case mail auth status wipes a perfectly good local credential and pushes the user back through the whole approval flow. Suggest gating the delete on a code the mail service actually emits for revocation, rather than on any 401.
Pending authorization expiry is never checked locally — cmd/mail_auth.go:198-222
PendingMailAuthorization.ExpiresAt is stored and echoed but never parsed. An expired or malformed pending record keeps re-transmitting the device code and verifier on every mail auth status until the server happens to answer with one of the three exact cleanup codes. A local expiry check would drop the stale proof material earlier and give a clearer error.
newProfileMayReuseLegacyName does not do what its name implies — internal/authstore/authstore.go:190-201
```
newProfileMayReuseLegacyName := !existed && name != meta.RobotID
```
In the !existed branch deleteMailBindingsForRobot is skipped, so only the legacy raw-name key is removed; every v2: binding for that RobotID survives. That is safe — the token/origin fingerprints in the key already fail closed — but the condition reads as if it purges bindings and does not. Worth either renaming or dropping, so a future reader does not rely on a purge that is not happening.
--idempotency-key has no length constraint — internal/registry/specs/mail.json:268, :301, :327
The description promises "Stable 8-200 character key", but the schema is a bare type: string and buildHeaders (cmd/service/run.go:500-515) only rejects empty values. --idempotency-key x is accepted locally. Adding minLength/maxLength would catch it before the request instead of after.
UpdateDraft.draftVersion is documented as required but is not in required — internal/registry/specs/mail.json:458 vs :467
The field's own description says "Required current version for Agent or policy drafts", yet required is ["to", "subject"]. The CLI will happily send a draft replacement with no concurrency token. If the backend enforces it, the local error is just late; if it does not, the lost-update guard is bypassable.
--verbose prints mail bodies — internal/client/client.go:968
redactDiagnosticBody masks only SensitiveJSONFields, which are derived from writeOnly schema fields. No mail content field is marked, so up to 1024 bytes of recipients, subject, body text, HTML, and base64 attachment data go to stderr under --verbose. This matches how every other domain behaves, so it is not a regression — but mail content is a different privacy class from a thread id, and agent runtimes commonly capture stderr. Worth a deliberate decision rather than an inherited default.
Factory state mutation on the shared credential — internal/cmdutil/factory.go:187-191, cmd/mail_auth.go:319 + :370-373
MailCredentialFunc and applyMailBinding write RobotID/SpaceID back onto the cached *BotCredential and set botIdentityVerified = true from a purely local store lookup. In a single-shot CLI this is harmless (and the comment at :71-73 acknowledges the design), but it means resolveBotIdentity's early return at :459 would skip the authoritative check for any caller that resolved a mail credential first. If this factory ever runs long-lived, that ordering becomes load-bearing.
PKCE test asserts only that a challenge exists — cmd/mail_auth_test.go:50
```
body["codeChallenge"] == ""
```
The test never recomputes base64url(SHA256(verifier)) from the persisted verifier. A regression that sent the verifier itself, or a truncated digest, would still pass. Given the server rejects on digest length, one recompute assertion here would pin the contract that actually matters.

Nits

showCurrentMailConnection (cmd/mail_auth.go:330-332) omits SuppressSpaceHeader: true on the identity probe, unlike the JMAP paths. Currently harmless because the mail transport credential has no SpaceID, but the asymmetry invites a future regression.
mailDeviceResponse.Interval is parsed and never used — there is no polling loop, so the server's advertised poll interval is discarded.
mailboxAddress validation (cmd/mail_auth.go:74) accepts @example.com and a@; strings.Count(s, "@") != 1 is a weak shape check.
auth update ignores --dry-run and mutates the profile on disk. This matches the rest of the auth family (login/logout behave the same, and --dry-run is documented as a request preview), so it is consistent — just noting it since auth update is new.
--idempotency-key is enforced by cobra but its help text does not carry the (required) marker that other required flags show.

3. Overall verdict

APPROVE

Spec ✅ and no P0/P1 defects. The credential-boundary work is careful, the fail-closed choices are deliberate and tested, and the one finding that would have been blocking (PKCE method) is refuted by the merged server contract. Every remaining item is a hardening or clarity improvement that does not need to gate this merge.

4. Suggested follow-ups

Roughly in value order — none of these need to happen before merge:

Treat any 5xx as unknown-outcome when UnknownOutcomeOnNetworkFailure is set (P2 #1).
Make ClientForCredential reject unknown credential kinds instead of defaulting to the Bot client (P2 #2).
Narrow the credential-deleting 401 branch to a real revocation code (P2 #3).
Add minLength/maxLength to the idempotency-key parameters and move draftVersion into UpdateDraft.required if the backend requires it (P2 #6, #7).
Strengthen the PKCE test to recompute the challenge from the stored verifier (P2 #10).

5. For a human to verify before this ships

This PR is labelled needs-human-review, and the author states live end-to-end authorization was not exercised. The items I could not verify from the code:

The gateway must admit the two bootstrap endpoints unauthenticated. /agent-mail-api/webapi/v0/agent-auth/device and .../token are called with no Authorization header by design (asserted in cmd/mail_auth_test.go:44-45, :62-63). If the /agent-mail-api route in the gateway requires a Bot bearer, or injects one, authorization cannot complete. This is the single highest-risk unverified assumption in the PR.
The gateway must forward Bearer omb_… unmodified to octo-mail for all non-bootstrap mail paths, and must not strip it as an unrecognized credential.
POST /v1/bot/register is used as an identity probe on every non-dry-run mail auth login (internal/cmdutil/factory.go:479-483). Please confirm that endpoint is genuinely idempotent and carries no registration side effect — the CLI now calls it as a read.
Server-side rate limiting on the public device endpoint. The bootstrap endpoint accepts a botId + spaceId with no proof of possession. The server PR appears to carry a limiter on this path; worth confirming it is active in the deployed configuration, since an unauthenticated caller who guesses a valid pair can generate pending-approval noise for that Bot's owner.

mochashanyao

[Octo-Q · automated review]

Verdict: Approve — no blocking findings; notes below (data-flow traced).

Code Review — PR #129 (octo-cli)

Reviewer: Octo-Q (automated review)

Summary

This PR adds Bot-bound Agent Mail to octo-cli: a new mail registry domain (24 operations over /agent-mail-api/webapi/v0/*), an OAuth-style device-flow authorization (mail auth login / mail auth status) with PKCE, a dedicated encrypted credential store keyed by RobotID + SpaceID + SHA-256 fingerprints of the API origin and the authorizing Bot token, JMAP message state/changes polling commands, a new auth update command for non-secret profile settings, and the octo-mail agent skill. The credential boundary is clean: mail operations resolve the mailbox token through ClientForCredential("mail") and the transport never mixes Bot and mailbox tokens.

No P0/P1 issues. The security model fails closed on every mismatch axis I traced (token swap, origin change, wrong Bot-id claim, multi-Space ambiguity), and the test suite drives the production wiring. Remaining items are four P2 robustness/docs gaps and one nit.

Verification

Static analysis only at head 5c5c57c7; build and tests not executed in this environment. Verified by tracing changed code and tests:

✅ Credential boundary — x-octo-credential: mail is declared only by internal/registry/specs/mail.json; every service execution path (emitOnce, runPaginated in cmd/service/run.go) resolves through ClientForCredential, and NewMail keeps the conversion private so a mail token cannot be selected by the Bot provider.
✅ Binding fail-closed — MailBindingKey fingerprints origin + Bot token, so credential lookup misses after a token rotation or origin change (TestFactory_MailCredentialRejectsDifferentAPIOrigin, ...RejectsStoredProfileTokenSwap); multi-Space matches return ErrMailCredentialAmbiguous.
✅ Device flow — PKCE S256 verifier stored only in the encrypted pending file; token exchange cross-checks botId against the Bot id verified through /v1/bot/register at login; claimed-but-unverified OCTO_BOT_ID is rejected when it does not own the token.
✅ Lifecycle parity — SaveProfile/RemoveProfile both purge legacy, raw, and robot-scoped mail bindings; logout routes through RemoveProfile.
✅ Non-idempotent writes — every send/reply/forward/draft/delete op declares x-octo-retry: never, mapped to DisableRetry + RESULT_UNKNOWN on network/502/503/504; TestMailSendIntentUsesPolicyEndpointAndDoesNotRetry pins exactly one request on 503.
✅ Secret hygiene — X-Octo-Confirmation header is x-octo-secret and collected via header flags; device code/verifier ride in SecretValues; verbose logging prints method/URL/body only, never headers.
✅ Validation gate — only top-level auth skips validation; nested mail auth login passes cfg.Validate() (pinned in TestSkipValidation).

Findings

No P0/P1 issues; four P2 items and one nit below.

P2 — Device response `expiresIn` is not validated (`cmd/mail_auth.go:143`)

The device response validation requires deviceCode and verificationUriComplete but accepts any expiresIn, including 0 or negative. The stored pending authorization is then already expired, and the user only discovers it when mail auth status round-trips to the server and reports authorization_expired. Treat expiresIn <= 0 as INVALID_AUTH_RESPONSE alongside the existing checks.

P2 — Crash window between saving the credential and removing the pending authorization (`cmd/mail_auth.go:257`)

SaveMailCredential runs before RemovePendingMailAuthorization. A process exit between the two leaves a valid stored credential plus a pending entry whose device code is already consumed; the next mail auth status attempts the exchange, gets authorization_used, deletes the pending entry, and only the run after that reports connected. Nothing is lost (the token is already persisted, which is why this order is the safer of the two), but one confusing error could be avoided by treating authorization_used as a successful-completion signal that clears pending and re-checks the stored credential.

P2 — Expired pending authorizations accumulate (`internal/authstore/mail.go:233`)

loadPendingMailAuthorizations never prunes entries whose expiresAt has passed. They are removed only when a matching status call hits a terminal server code or when the profile lifecycle purges the robot's bindings — so an abandoned login (or one orphaned by an OCTO_API_BASE_URL change, which the origin fingerprint then never matches again) stays in the encrypted store indefinitely. Drop entries past expiresAt during load.

P2 — New domain ships without a CHANGELOG entry (`README.md:362`)

The PR introduces a whole new domain (24 registry operations, device-flow auth, JMAP polling, auth update, the octo-mail skill) but CHANGELOG.md [Unreleased]/Added has no entry for it, while every prior domain addition (#127 marketplace, #123 loop, drive) records itself there and CLAUDE.md instructs keeping these docs in sync. Add an entry covering the mail domain, the Bot-bound encrypted credential store, and auth update.

Nit — Inconsistent nil-safety pattern in `mailJMAPAccountID` (`cmd/mail_jmap.go:121`)

The function dereferences f unconditionally (f.ClientForCredential) but later guards f.Globals != nil before reading DryRun. In practice both are always set by NewRootCmd; pick one contract — either drop the guard or state why Globals alone can be nil.

Human-verify

Server-side contracts are outside this checkout: the /v1/bot/register identity response shape (the CLI defensively accepts both top-level and data.robot_id), the device/token endpoint error codes (authorization_pending|expired|used|denied), and JMAP session primaryAccounts behavior. The CLI side handles each defensively; worth confirming against octo-server/agent-mail-api. Not a merge blocker for this PR.
Mailbox scoping of omb_ tokens is enforced server-side; the CLI relies on the Agent Mail service to bind each token to exactly one mailbox. Not a merge blocker for this PR.

Things I checked that are fine

Mail token never leaks into Bot API calls and vice versa: MailCredentialFunc mutates only RobotID/SpaceID of the cached Bot credential, never Token.
OCTO_BOT_ID claim semantics: an env-supplied Bot id is echoed as identity.robot_id_claimed until verified, can fall through an empty profile store, and an explicit --bot-id still fails closed (TestFactory_ExplicitBotIDAndEnvironmentClaimHaveDistinctEmptyStoreSemantics).
auth update --api-base-url touches only non-secret metadata; an origin change intentionally strands mail bindings (fingerprint mismatch → re-authorization required), which is the designed fail-closed behavior.
The raw api command has no header input surface, so header-position secrets (X-Octo-Confirmation) cannot flow through it unmasked; generated commands collect them from bound flags.
JMAP commands attach after service registration with collision guards; state/changes validate inputs and dry-run stops after the session preview without network calls.

Verdict: COMMENT

No correctness, security, or wiring defects that block merge: the credential boundary, identity verification, and retry/unknown-outcome semantics are sound and well-tested. The P2 items are robustness and documentation gaps worth addressing soon; the nit is optional.

[Octo-Q] verdict: APPROVE — 无 P0/P1（双独立审查腿合并后 4×P2 + 1×Nit，均为健壮性/文档缺口，不满足 R1 阻塞定义）；此为automated review建议，终审以 Opus 拍板为准。

yujiawei · 2026-08-13T14:03:16Z

One addition to my review above — non-blocking, but easy to fix before merge:

CHANGELOG.md has no entry for this domain. [Unreleased] / Added records every prior domain addition in full (marketplace expert/squad from #127, the loop domain from #123, the drive domain from #124), and this PR adds a comparable surface — 24 registry operations, a device-flow authorization with a dedicated encrypted credential store, the JMAP message state/changes pair, auth update, and the embedded octo-mail skill — while CHANGELOG.md is untouched by the diff.

Worth an entry covering the mail domain, the Bot-bound encrypted mailbox credential (RobotID + SpaceID + API-origin and Bot-token fingerprints), and auth update. Verdict is unchanged.

Jerry-Xin

Summary: The PR is in scope for octo-cli and the mail credential/auth changes are generally well-aligned with the existing registry-driven command architecture.

💬 Non-blocking

🟡 Warning — mail auth status can echo an unverified environment Bot claim as data.bot_id when no mail credential exists. The envelope correctly uses identity.robot_id_claimed, but the command payload at cmd/mail_auth.go:307-312 still uses bot.RobotID directly. This does not release credentials and mail auth login verifies before binding, but automation may misread the status payload as authoritative.

🔵 Suggestion — overlayProfileBaseURL still points users to auth login for fixing an invalid stored API base URL even though this PR adds auth update. See internal/cmdutil/factory.go:394-397; the hint should probably mention octo-cli auth update --profile ... --api-base-url ....

✅ Highlights

Mail operations correctly route through a separate mailbox credential boundary via ClientForCredential and registry metadata (cmd/service/run.go:70-83, internal/cmdutil/factory.go:423-436).
Mail side-effect operations disable retries and surface ambiguous outcomes as RESULT_UNKNOWN (internal/client/client.go:897-901, internal/client/client.go:1272-1289).
Bot-bound mail credentials are scoped by RobotID, SpaceID, normalized API origin, and Bot token fingerprint (internal/authstore/mail.go:40-52).
Focused and full test suites passed: go test ./cmd ./cmd/service ./internal/authstore ./internal/client ./internal/cmdutil ./internal/credential ./internal/registry ./skills -count=1 and go test ./... -count=1.

yujiawei

Code Review — PR #129 (octo-cli)

Reviewed at head 14524b0e3cb984e5ebab5d1cec4463e5b17a7355 against merge-base 84655c91 (35 files, +4334/−47). This PR is credential-handling code, so I traced every path that selects, stores, or transmits a secret rather than reading the diff top-to-bottom.

Verification I ran locally on this head:

go build ./...                          clean
go vet ./...                            clean
gofmt -l .                              clean
go test ./... -count=1                  pass
go test -race -shuffle=on -count=1 ./...  pass

1. Requirements compliance

Checked against the acceptance criteria in #128.

Criterion	Result	Evidence
Stored-profile Bot and `OCTO_BOT_TOKEN` runtime can complete the device flow	✅	`cmd/mail_auth.go:65`, tests at `cmd/mail_auth_test.go:30,189`
A claimed RobotID that does not own the token is rejected before storing	✅	`internal/cmdutil/factory.go:491-495`, test `cmd/mail_auth_test.go:277`
Secrets encrypted, RobotID-scoped, excluded from output, cleared on profile removal/rebind	✅ (one gap, P2-3)	`internal/authstore/mail.go:100-300`, `internal/authstore/authstore.go:172-200,247-272`
Mail uses the mailbox credential on `/agent-mail-api`, sends no Bot token and no `X-Space-Id`	✅ (one gap, P2-1)	`internal/registry/specs/mail.json:7-9`, `internal/client/client.go:748-757`, tests `cmd/service/mail_test.go:21`, `cmd/mail_jmap_test.go:16`
Read / binary-download / send / reply / Draft / delivery / RFC 8621 paths tested	✅	`cmd/service/mail_test.go`, `cmd/mail_jmap_test.go`
Side effects do not auto-retry; ambiguous failures → `RESULT_UNKNOWN`	✅ (two gaps, P2-4/P2-5)	`cmd/service/run.go:81-82`, `internal/client/client.go:1272-1289`
Build / vet / fmt / race tests pass	✅	reproduced above

Nothing required by #128 is missing, and I found no functionality added beyond its stated scope. Two things ride along with the feature and are worth naming explicitly, though both check out:

identity.robot_id_claimed is a cross-cutting change. internal/credential/env_provider.go:81 now populates RobotID from OCTO_BOT_ID for every command, and internal/cmdutil/factory.go:587-594 splits the echo into robot_id vs robot_id_claimed. This is additive — environment credentials previously emitted no robot_id at all — so no existing consumer of identity.robot_id changes behavior, and CLAUDE.md documents the new field. Correct, but it is a global identity-envelope change shipping inside a Mail PR.
The skipValidation exemption was narrowed (cmd/root.go:139-145) so only top-level auth stays credential-free. I traced the ancestor walk for mail auth login: auth's parent is mail, whose parent is non-nil, so it correctly falls through to the validation gate. cmd/cmd_test.go:477-482 pins both directions.

Requirements verdict: PASS.

2. Code quality

No blocking defects. The credential-boundary design is the strongest part of this change: binding keys fingerprint the normalized origin and the authorizing Bot token (internal/authstore/mail.go:38-52), so an origin swap or token rotation fails closed instead of releasing the old mailbox token, and NewMail (internal/client/client.go:748-757) constructs a transport credential with the token only — no SpaceID — so X-Space-Id cannot leak even if a spec forgot x-octo-space-header: false. Two independent mechanisms guarding the same property is the right call here.

The tests assert the actual security properties rather than restating the implementation. cmd/mail_auth_test.go:44,62 pin that the device and token bootstrap endpoints carry no Authorization header; internal/cmdutil/factory_test.go:808 pins that a stored-profile token swap refuses to release the old mail credential. Those are the two assertions I would have asked for if they were missing.

I also confirmed the transport never logs response bodies — internal/client/client.go:997 logs only ← %d (%d bytes) — so the omb_* mailbox token from the exchange cannot reach stderr under --verbose.

P2-1 — The generic `api` escape hatch bypasses all three Mail boundaries

cmd/api.go:89 resolves f.Client() unconditionally, and its client.Request sets no Credential, SuppressSpaceHeader, or DisableRetry:

cli, err := f.Client()

So octo-cli api GET /agent-mail-api/webapi/v0/messages sends Bearer app_* plus X-Space-Id, and octo-cli api POST /agent-mail-api/webapi/v0/messages retries a send. Impact is bounded — this is a documented raw passthrough, and the Bot token goes to the same OCTO_API_BASE_URL origin it already authenticates against, so there is no cross-origin or cross-tenant exposure — but the failure mode is a confusing 401 and a possible duplicate send.

Worth fixing cheaply: api.go:83 already calls apiSecretsForRequest(f.Registry(), method, path, body), so the registry lookup is right there. Resolving the matching OperationDetail and forwarding Credential / RetryMode / SpaceHeaderSet from it would close all three gaps in the same lookup.

P2-2 — Unlocked read-modify-write on the mail stores

internal/authstore/mail.go:170-175 (and RemoveMailCredential, plus the pending-file equivalents) do load → mutate map → seal → atomicWrite with no inter-process lock:

tokens, err := s.loadMailTokens()
...
tokens[botKey] = token
return s.saveMailTokens(tokens)

Two concurrent processes can lose an update — a logout writing {B} can be overwritten by a concurrent save that writes a stale {A, B, C}, resurrecting a locally revoked mail token. The atomic rename prevents a torn file, not a lost update.

For the record on severity: this is the pre-existing convention in this package — saveTokens / saveProfiles on main have the identical shape and no locking — so the PR is consistent with its surroundings rather than introducing a new pattern. It does widen the window, since a successful authorization now writes two files in sequence (mail_auth.go:252-260). I would not block on it, but it is worth a tracked follow-up to put one lock around the whole store.

Related and narrower: two concurrent authorization flows for the same Bot/Space/origin share one binding key, so a login for mailbox B can be overwritten by an in-flight status completing an earlier flow for mailbox A (cmd/mail_auth.go:198 reads pending, :260 deletes it). Both mailboxes were human-approved, so this is a wrong-outcome bug, not a boundary break. A compare-and-swap on the device code at completion would fix it.

P2-3 — Cleanup-failure handling is asymmetric between the two exit paths

On the success path a cleanup failure is fatal (cmd/mail_auth.go:260):

if err := store.RemovePendingMailAuthorization(pendingKey); err != nil {
    return failErr(f, err)
}

but on the error paths the same class of failure is swallowed (cmd/mail_auth.go:236, :336):

_ = store.RemovePendingMailAuthorization(pendingKey) //nolint:errcheck

The success-path variant produces a bad sequence: the credential is already saved and the Bot is authorized, but the command exits non-zero; the next mail auth status replays the undeleted pending record, gets authorization_used, cleans up, and errors again — the user does not see status: connected until the third invocation. The swallowed variant has the opposite problem: if the directory is unwritable, an expired verifier or a revoked mail token stays encrypted on disk while the CLI reports success.

Suggest picking one policy for both: treat post-commit cleanup as best-effort, and surface the failure as a warning on stderr rather than either failing the command or discarding it silently.

P2-4 — Pending-flow expiry is stored but never enforced locally

cmd/mail_auth.go:150,158 persists ExpiresAt, and :200-204 echoes it, but nothing ever compares it to the clock — storedPendingMailAuthorization at :198 replays whatever is on disk. The three recognized cleanup codes at :235 (authorization_expired / authorization_used / authorization_denied) are the only way a stale record is ever removed. If the server garbage-collects the device code and answers with anything else (a bare invalid_grant, say), every subsequent mail auth status replays the dead proof and never falls through to showCurrentMailConnection — so an otherwise-valid stored connection stays invisible. Recovery exists (mail auth login overwrites the record), but it is not obvious from the error.

A local time.Now().After(expiresAt) check before the exchange would drop the stale record and fall through. Related: device.ExpiresIn is used unvalidated at :150, so a zero or negative value yields an already-expired timestamp.

P2-5 — HTTP 500 after a committed side effect is not reported as `RESULT_UNKNOWN`

internal/client/client.go:1279 only treats 502/503/504 as ambiguous:

ambiguousGatewayStatus := errors.As(err, &re) &&
    (re.status == http.StatusBadGateway || re.status == http.StatusServiceUnavailable || re.status == http.StatusGatewayTimeout)

I confirmed isRetryableStatus (:1291) excludes 500, so a 500 never becomes a retryableErr and arrives here as a plain non-network ExitError — returned unchanged. If mail draft create commits and then returns 500 INTERNAL_ERROR, the caller sees an ordinary API error whose hint invites a retry, which can duplicate the draft. Automatic retry is correctly disabled; it is the partial-success signal to the human or agent that is missing.

P2-6 — Smaller items

mail.message.flag is a write without x-octo-retry: never (internal/registry/specs/mail.json, PATCH /messages/{id}). Every other write in the spec is marked. Setting a Seen flag is idempotent so this is defensible, but it is the one write that silently opts out of the stated "side effects do not retry" rule — worth either marking it or adding a one-line comment saying why it is exempt.
NormalizeAPIBaseURL (internal/config/config.go:104-121) does not lowercase the host or strip a default port. https://API.octo.com, https://api.octo.com, and https://api.octo.com:443 therefore produce three different origin fingerprints and three disjoint mail bindings. Fails closed, so it is a usability trap rather than a leak — the user sees "Agent Mail is not connected" after a cosmetic config edit.
auth update --api-base-url orphans mail bindings without warning (internal/authstore/authstore.go:218-230). UpdateProfileAPIBaseURL touches only meta.APIBaseURL, so the old origin-fingerprinted mail token becomes unreachable but stays on disk indefinitely. Read-side behavior is correct (fail-closed); the issue is silent retention of a now-unusable secret. The same holds after a Bot token rotation. A note in the auth update output would help.
MailCredentialFunc mutates the shared credential pointer (internal/cmdutil/factory.go:187-188) — bot.RobotID / bot.SpaceID are overwritten on the same struct the Bot client already holds. No live impact today, since no command mixes Mail and non-Mail requests in one invocation, but it is a latent aliasing hazard if a composite command ever does.
The ambiguity hint is sometimes wrong (cmd/mail_auth.go:375-379, internal/cmdutil/factory.go:180-182): ErrMailCredentialAmbiguous fires when bindings differ by RobotID or SpaceID, but the hint always says pass --space <space_id>, which does not help in the RobotID case.

3. Verdict

Approving. No P0 or P1. The credential-boundary work is careful and the tests pin the properties that matter; every finding above is a P2 that can land as a follow-up.

4. Please verify manually before merge

This change is credential-handling code and two items depend on backend guarantees I cannot check from this repository:

The Space binding is asserted locally but never confirmed by the server. mailTokenResponse (cmd/mail_auth.go:38-43) carries botId but no spaceId, and :250 only checks token.BotID != bot.RobotID. The credential is then stored under the locally requested Space at :254-257. If the authorization service can ever issue a same-Bot token for a Space other than the one the device flow requested, the CLI would file it under the wrong Space and later serve it for that Space. Please confirm against the octo-mail contract that a device code cannot change Space between the /device and /token calls — and if the response can carry spaceId, assert it here the way botId already is.
cmd/api_secrets_test.go:451-457 relaxes a security tripwire. The test previously rejected x-octo-secret on any non-path parameter; it now also permits header, which is what lets X-Octo-Confirmation be declared. I verified this is sound: cmd/api.go builds its client.Request with no Headers field at all, so the generic command has no caller-supplied header to leak, and generated commands do mask header secrets via collectSecrets (cmd/service/run.go:317-321), pinned by cmd/service/mail_test.go:150. The new comment states the invariant clearly. Flagging it only so a human signs off on loosening a guard rather than it passing as a routine test edit.

Also worth a human eye: the PR body notes live end-to-end authorization was never exercised, and that the external gateway must admit the PKCE bootstrap endpoints while stripping unrelated credentials. The CLI side is correct — I confirmed internal/client/client.go:953 omits Authorization entirely when the credential is nil, and newMailAuthorizationClient passes nil — but the gateway's half of that contract is untested here.

5. Review method

Findings were gathered from three independent passes over this head — my own read, plus two automated analysis passes (one line-level adversarial, one whole-repository) — then reconciled against the diff. All three converged on the cleanup-handling asymmetry (P2-3), and two of three independently found the unenforced pending expiry (P2-4). The api escape-hatch gap (P2-1) and the 500 handling (P2-5) each came from a single pass and I verified both directly before including them; I downgraded P2-1 and P2-2 from their originally proposed blocking severity after confirming that the escape hatch stays within one origin and that the unlocked read-modify-write matches the pre-existing pattern on main.

Not covered: no live end-to-end run against a real gateway; backend semantics for device-code expiry, one-time use, Space immutability, and confirmation-token idempotency are all taken on trust; and I did not evaluate the embedded skills/octo-mail/SKILL.md guidance for agent-behavioral correctness beyond checking that the command shapes it documents match the spec.

Liuxy20 requested a review from a team as a code owner August 11, 2026 16:39

yujiawei added the needs-human-review label Aug 11, 2026

github-actions Bot added the size/XL PR size: XL label Aug 11, 2026

Liuxy20 mentioned this pull request Aug 11, 2026

feat(mail): add Bot-bound Agent Mail commands and authorization #128

Closed

7 tasks

Jerry-Xin previously requested changes Aug 11, 2026

View reviewed changes

Jerry-Xin previously approved these changes Aug 11, 2026

View reviewed changes

yujiawei requested changes Aug 11, 2026

View reviewed changes

mochashanyao previously approved these changes Aug 11, 2026

View reviewed changes

Liuxy20 dismissed stale reviews from mochashanyao and Jerry-Xin via 603455b August 12, 2026 06:49

Liuxy20 force-pushed the feat/agent-mail-cli branch from 3815eec to 603455b Compare August 12, 2026 06:49

Jerry-Xin previously approved these changes Aug 12, 2026

View reviewed changes

mochashanyao previously approved these changes Aug 12, 2026

View reviewed changes

yujiawei requested changes Aug 12, 2026

View reviewed changes

Liuxy20 dismissed stale reviews from mochashanyao and Jerry-Xin via 7ad6aed August 12, 2026 07:31

Jerry-Xin previously approved these changes Aug 12, 2026

View reviewed changes

yujiawei previously approved these changes Aug 12, 2026

View reviewed changes

mochashanyao previously approved these changes Aug 12, 2026

View reviewed changes

Liuxy20 dismissed stale reviews from mochashanyao, yujiawei, and Jerry-Xin via 4ffe171 August 13, 2026 08:04

Liuxy20 force-pushed the feat/agent-mail-cli branch from 7ad6aed to 4ffe171 Compare August 13, 2026 08:04

Jerry-Xin previously approved these changes Aug 13, 2026

View reviewed changes

yujiawei requested changes Aug 13, 2026

View reviewed changes

Liuxy20 dismissed Jerry-Xin’s stale review via 1aa8470 August 13, 2026 08:32

Liuxy20 dismissed stale reviews from mochashanyao, yujiawei, and Jerry-Xin via 83d26d0 August 13, 2026 09:37

Jerry-Xin previously approved these changes Aug 13, 2026

View reviewed changes

yujiawei requested changes Aug 13, 2026

View reviewed changes

mochashanyao previously approved these changes Aug 13, 2026

View reviewed changes

Liuxy20 dismissed stale reviews from mochashanyao and Jerry-Xin via 08a512e August 13, 2026 10:28

Liuxy20 force-pushed the feat/agent-mail-cli branch from 83d26d0 to 08a512e Compare August 13, 2026 10:28

Jerry-Xin previously approved these changes Aug 13, 2026

View reviewed changes

yujiawei requested changes Aug 13, 2026

View reviewed changes

Liuxy20 dismissed Jerry-Xin’s stale review via 1c56c73 August 13, 2026 11:03

Jerry-Xin previously approved these changes Aug 13, 2026

View reviewed changes

Liuxy20 force-pushed the feat/agent-mail-cli branch from 1c56c73 to 5c5c57c Compare August 13, 2026 13:19

yujiawei previously approved these changes Aug 13, 2026

View reviewed changes

mochashanyao previously approved these changes Aug 13, 2026

View reviewed changes

fix(mail): satisfy CLI lint gates

14524b0

Liuxy20 dismissed stale reviews from mochashanyao, yujiawei, and Jerry-Xin via 14524b0 August 14, 2026 05:15

Jerry-Xin approved these changes Aug 14, 2026

View reviewed changes

yujiawei approved these changes Aug 14, 2026

View reviewed changes

Jerry-Xin self-assigned this Aug 14, 2026

Jerry-Xin merged commit e627723 into Mininglamp-OSS:main Aug 14, 2026
9 checks passed

Uh oh!

Conversation

Liuxy20 commented Aug 11, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Changes

Motivation

Testing

Checklist

Uh oh!

Jerry-Xin left a comment

Choose a reason for hiding this comment

🔴 Blocking

🟡 Non-blocking

✅ Highlights

Uh oh!

Jerry-Xin left a comment

Choose a reason for hiding this comment

🟡 Non-blocking

✅ Highlights / verified clean

Build / test / lint / CI

Uh oh!

yujiawei left a comment

Choose a reason for hiding this comment

Code Review — PR #129 (octo-cli)

1. Specification compliance

2. Code quality

P0-1 — Mailbox credential is selected by an unverified RobotID claim

P0-2 — Creating the default-named profile silently destroys a valid mailbox authorization

P1-1 — --verbose prints the PKCE verifier and device code in cleartext

P2-1 — --dry-run is unusable for the whole mail domain in a token-only runtime, with a misleading error

P2-2 — mail auth login Space hint recommends a variable that is ignored for stored profiles

P2-3 — One-time confirmation tokens are not marked x-octo-secret

P2-4 — Credential-revocation writes are ordered so a mid-way failure strands the secret

Nits

3. Overall verdict

4. Suggested direction

5. Points for manual human verification

6. Additional observations

Uh oh!

mochashanyao left a comment

Choose a reason for hiding this comment

Code Review — PR #129 (octo-cli)

Summary

Verification

Findings

P2 — Mail credential survives API base-URL change (internal/authstore/authstore.go:172)

P2 — JMAP/spec mail commands lack the 401 cleanup that mail auth status has (cmd/mail_jmap.go:115)

P2 — Confirmation token not marked x-octo-secret (internal/registry/specs/mail.json:49)

P2 — Nested-auth gate branch has no regression test (cmd/root.go:108)

P2 — CLAUDE.md command tree omits the hand-written mail leaves (CLAUDE.md:91)

P2 — Crash window between credential save and pending cleanup (cmd/mail_auth.go:200)

Human-verify

Things I checked that are fine

Verdict: COMMENT

数据流回溯（被消费数据 → 上游来源 → 是否真流到消费点）

盲点 checklist（security_sensitive 全项）

跨轮 blocker 复检（R6）

额外发现（非定级观察）

Uh oh!

yujiawei commented Aug 11, 2026

Follow-up to the review above — one correction and five additional P2 items

Correction to my P2-3

Additional P2 items

On the two blockers, restated after re-checking

Uh oh!

Jerry-Xin left a comment

Choose a reason for hiding this comment

Delta since the previously-approved head (3815eec → 603455b)

Security model — re-verified, not regressed

Non-blocking

Highlights

Uh oh!

mochashanyao left a comment

Choose a reason for hiding this comment

Code Review — PR #129 (octo-cli)

Summary

Verification

Findings

P2 — auth update skips the base-URL validation that auth login applies (cmd/auth.go:67)

Liuxy20 commented Aug 11, 2026 •

edited

Loading

P1-1 — `--verbose` prints the PKCE verifier and device code in cleartext

P2-1 — `--dry-run` is unusable for the whole `mail` domain in a token-only runtime, with a misleading error

P2-2 — `mail auth login` Space hint recommends a variable that is ignored for stored profiles

P2-3 — One-time confirmation tokens are not marked `x-octo-secret`

P2 — Mail credential survives API base-URL change (`internal/authstore/authstore.go:172`)

P2 — JMAP/spec mail commands lack the 401 cleanup that `mail auth status` has (`cmd/mail_jmap.go:115`)

P2 — Confirmation token not marked `x-octo-secret` (`internal/registry/specs/mail.json:49`)

P2 — Nested-auth gate branch has no regression test (`cmd/root.go:108`)

P2 — CLAUDE.md command tree omits the hand-written mail leaves (`CLAUDE.md:91`)

P2 — Crash window between credential save and pending cleanup (`cmd/mail_auth.go:200`)

Delta since the previously-approved head (`3815eec` → `603455b`)

P2 — `auth update` skips the base-URL validation that `auth login` applies (`cmd/auth.go:67`)

P1-1 — An unverified `OCTO_BOT_ID` is now treated as identity: it unlocks a stored mailbox token and is echoed as `identity.robot_id`

P2-1 — `--dry-run` is broken across the whole `mail` domain, and now for `mail auth` unconditionally

P2-4 — `-o` on a generated binary operation clobbers an existing file with no guard

P2-5 — The revoked-token self-heal exists only in `mail auth status`

P2-6 — `auth update --api-base-url` leaves the mailbox token bound to the old origin

P2-7 — `submissionIds` spec type still contradicts the repo's own test stub

P2-8 — `CHANGELOG.md` has no Agent Mail entry

P2-11 — `auth update --api-base-url` skips the validation its sibling `auth login` applies

P2-1 — `--dry-run` is still broken on the two JMAP leaves, with a hint that sends the operator the wrong way

P2-4 — `--dry-run` now performs a real, retryable, authenticated POST

P2 — Profile-name/RobotID key collision can silently purge another Bot's mail credential (`internal/authstore/authstore.go:188`)