Skip to content

feat(mail): manage the Gmail vacation responder - #41

Merged
piekstra merged 4 commits into
mainfrom
piekstra/mail-vacation
Nov 4, 2026
Merged

piekstra merged 4 commits into
mainfrom
piekstra/mail-vacation

Conversation

@piekstra

@piekstra piekstra commented Nov 4, 2026 •

Copy link
Copy Markdown
Member

Adds an out-of-office reply to grw, which until now had no way to read or change it.

  • grw mail vacation get prints the current settings.
  • grw mail vacation update changes only the flags given: --subject, --body or --file, --start, --end, --contacts-only, --domain-only. It leaves the on/off state alone.
  • grw mail vacation enable and disable turn the reply on and off and keep its text and dates.

No scope change: updateVacation needs gmail.settings.basic, which grw already requests. The scope descriptions, admin guide and help text now mention this use. gro is unchanged.

--end takes the last day to reply. Gmail replies only to messages received before the end time, so a bare date resolves to the next local midnight, while an RFC3339 value is sent as given. Gmail clears fields left out of an update, so clearing a date or body needs no extra handling. The on/off and restriction flags are always sent explicitly, and tests assert them on the request.

Evidence

make check and make test-cover-check pass. Against a real Workspace account, on a reply whose start date was still in the future so nothing was sent:

  • disable returned Enabled: false and kept the text and dates.

  • enable returned Enabled: true, and a following get confirmed the same text and dates.

  • update --end "" --subject "" returned and then read back both fields as empty, which confirms Gmail clears fields left out of the update. A second update restored them.

The HTML body case relies on that same behavior; the test account has no HTML body to clear.

Model: Claude Opus 5.5 (1M context, medium effort) in Claude Code.

Add grw mail vacation show, set and off, using the
gmail.settings.basic scope grw already requests.

@monit-reviewer monit-reviewer left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Automated PR Review

Reviewed commit: 87d52212facc
Profile: claude-monit-reviewer - Posting as: monit-reviewer

Summary

Reviewer Findings
go:implementation-tests 4
policies:conventions 4
harness-engineering:repo-health 3
security:code-auditor 0
go:implementation-tests (4 findings)

Minor - internal/rwcmd/mail/vacation.go:180

printVacation prefers ResponseBodyPlainText and falls back to ResponseBodyHtml only when plain text is empty. The generated client documents the opposite precedence for Gmail: if both are specified, the HTML body is used. For a responder holding both bodies, for example one configured by another client, show can display text that differs from what Gmail actually sends. Because off keeps the existing fields, the same mismatch can persist across later reads. Fix: select ResponseBodyHtml first and fall back to plain text, and cover the both-set case in the show test.

Minor - internal/rwcmd/mail/vacation.go:160

The RFC3339 branch of parseVacationTime is never executed by the tests, although --start and --end both advertise RFC3339. The contract at risk is that only a bare date moves to the next midnight, while an explicit --end timestamp is sent exactly as given. If the endOfDay adjustment were moved above this branch, or the order of the two parses changed, an RFC3339 end time would silently shift by a day and the responder would keep replying 24 hours too long, with every current test still passing. Fix: add a table test for parseVacationTime covering RFC3339 with both endOfDay values (unchanged millis, including a non-local offset), a bare date with both values, and the empty value returning 0.

Minor - internal/rwcmd/mail/vacation.go:25

grw mail vacation show has no handler test. Coverage for newVacationShowCommand is 11.1%, and that comes only from the architecture test walking the command tree, so RunE never executes. Nothing proves show calls GetVacation, returns its error, or prints the fields a user relies on. The ResponseBodyHtml fallback in printVacation is also never executed, and sanitize.Output on subject and body is not asserted for this new output path, even though the values can come from outside this CLI. docs/adding-a-write-surface.md asks for command success and error tests, and TestFilter_List is the equivalent precedent. Fix: add TestVacationShow using GetVacationFunc and testutil.CaptureStdout. Assert the enabled state, formatted dates, (not set), an HTML-only body, an escape sequence being stripped from subject and body, and propagation of a GetVacation error.

Minor - internal/rwcmd/mail/vacation_test.go:68

The both bodies case would pass even if the mutual-exclusion guard were deleted. It passes --body-file b, which does not exist, so removing the guard makes os.ReadFile fail and the test still sees an error, because the table only checks err != nil. The other cases have the same weakness in that they never check which validation fired, whereas TestFilter_CreateRequiresCriterion matches on the message. The success path of --body-file is also unexercised, so nothing proves the file contents reach ResponseBodyPlainText. Fix: make the table map each case to an expected error substring (for example mutually exclusive), point --body-file at a real file from t.TempDir() in that case, and add a success test asserting the file contents are sent.

policies:conventions (4 findings)

Major - internal/rwcmd/mail/vacation.go:50

The new leaf verbs drift from the family verb language in cli-common command-surface.md §1, which says that doc wins for new surface inside existing CLIs. set is explicitly excluded for resource operations ("Use update"; the only exception is config set). off (line 123) is an invented synonym for disable, defined as toggling active state without modifying the definition, which is exactly what this leaf does. show (line 27) is used elsewhere in this repo only for the control-plane config show; resource reads use get. Renaming gets harder after release because feat: merges ship automatically and scripts will bind to these names. Suggested fix: rename to grw mail vacation get, update and disable before merge, and update the PR description. TestWriteLeavesHaveDryRun exempts get, so the dry-run rule is still met.

Minor - internal/rwcmd/mail/vacation.go:145

off discards the UpdateVacation response and prints only a confirmation line. output-and-rendering.md §4.6 says mutation success output mirrors the get output so callers see the post-state without a follow-up fetch; confirmation-only output is reserved for delete, archive and remove. set already does this. Showing the post-state here also lets the caller confirm the retained text and dates, which is this command's stated behavior. Suggested fix: keep the returned settings and call printVacation(updated) after a Vacation responder turned off: line.

Minor - internal/rwcmd/mail/vacation.go:111

--body-file is a new synonym for an existing flag. mail draft already reads a body from a file with --file/-f (internal/cmd/mail/draft.go:340), and command-surface.md §8.1 requires reusing the existing name before inventing one. An agent that knows mail draft --file will hit an unknown-flag error here. Suggested fix: rename to --file, keeping draft's -f binding so the two commands match, and update the error strings and example.

Minor - internal/rwcmd/mail/vacation.go:191

The date rendering does not follow output-and-rendering.md §4.3, which requires ISO-8601 dates and - for missing values; relative or locale-style text is not the family format. This leaf prints Mon, Jan 2, 2006 3:04 PM MST and (not set) (line 189). mail draft has a similar human format at internal/cmd/mail/draft.go:488, but the standard wins for new surface, and the abbreviated zone name is ambiguous for agents parsing the value. Date-only output would hide the end instant, so suggested fix: format with time.RFC3339 as refresh and profiles already do, and return - when unset.

harness-engineering:repo-health (3 findings)

Minor - internal/rw/gmail/client.go:154

The docs that explain what gmail.settings.basic is for still say filters only, though this PR makes it the scope behind changing the auto-reply. The PR is right that no scope is added, but the purpose text is now incomplete in places where people decide whether to grant it. internal/app/grw/identity.go:25 and the ScopeDescriptions entry at line 46 say "create, list, and delete filters", and that description is shown by the init wizard and the re-auth prompt. WORKSPACE_ADMINS.md:56, the admin-facing scope justification, says "Gmail settings access supports filters". The grw root Long text in internal/app/grw/root.go also omits the responder, although the README was updated. A user or Workspace admin reading these would not learn that the grant also lets grw turn on replies sent automatically to incoming senders. Future agents would also get a wrong picture of what the scope is used for. Fix: mention the vacation responder in the identity comment, ScopeDescriptions, WORKSPACE_ADMINS.md and the root help text, in this PR.

Minor - internal/rw/gmail/client.go:155

The new client methods have no API-client tests, so the off request has no wire-level check. docs/adding-a-write-surface.md step 6 requires API-client tests for a write surface, and internal/rw/gmail/drafts_test.go already has an httptest harness (testDraftClient) for this. TestVacationOff_KeepsSettingsAndSendsFalse only calls json.Marshal on the struct handed to the mock, so it never exercises the real Users.Settings.UpdateVacation call. That gap matters here because the PR says off has only run against mocks and in --dry-run, so nothing has ever checked the actual request it sends. Fix: add GetVacation and UpdateVacation tests to internal/rw/gmail using that harness. Assert the method and the <path> path, assert the captured body contains "enableAutoReply":false, and assert that API errors are wrapped with %w, as TestDraftAPIErrorsWrap does.

Minor - internal/rwcmd/mail/mail.go:21

Nothing checks that the vacation tree is reachable from grw mail, and show has no test. vacation_test.go builds the leaves directly with newVacationSetCommand() and newVacationOffCommand(). TestNewCommandComposition and TestFolderAndFilterSubcommands in internal/rwcmd/mail/mail_test.go list folder and filter by name but were not extended. TestWriteCommandsExtendReadCommands only requires at least one added leaf, and TestWriteLeavesHaveDryRun only checks leaves that are registered. Dropping this AddCommand line would therefore remove the feature with all tests still passing, and set and off would also escape the dry-run check. newVacationShowCommand is never executed in tests either, unlike its sibling TestFilter_List, and the new handlers have no API-error or client-creation-error cases, which step 6 of docs/adding-a-write-surface.md asks for. Fix: add vacation to the composition list, assert its show, set and off subcommands, and add a show output test plus one *_APIError and one *_ClientCreationError case.

Reviewer Coverage

Complete coverage details are retained in the local coverage.json artifact.

  • go:implementation-tests — complete (broad); scope: 6 files; examples: internal/rw/gmail/client.go, internal/rwcmd/mail/mail.go, internal/rwcmd/mail/mock_test.go, internal/rwcmd/mail/output.go, internal/rwcmd/mail/vacation.go; 1 omitted; body-inspected: 6 files; examples: internal/rw/gmail/client.go, internal/rwcmd/mail/mail.go, internal/rwcmd/mail/mock_test.go, internal/rwcmd/mail/output.go, internal/rwcmd/mail/vacation.go; 1 omitted; skipped: none; context (not coverage): 4 files; examples: docs/adding-a-write-surface.md, internal/architecture/architecture_test.go, internal/rwcmd/mail/filter_test.go, internal/rwcmd/mail/mail_test.go; constraints: Ran go test with coverage for internal/rwcmd/mail, internal/rw/gmail and internal/architecture; all passed. No live Gmail API calls were made, so API behavior is taken from the generated client's field docs.
  • policies:conventions — complete (broad); scope: 4 files; examples: README.md, internal/rwcmd/mail/mail.go, internal/rwcmd/mail/output.go, internal/rwcmd/mail/vacation.go; body-inspected: 4 files; examples: README.md, internal/rwcmd/mail/mail.go, internal/rwcmd/mail/output.go, internal/rwcmd/mail/vacation.go; skipped: none; context (not coverage): 6 files; examples: STANDARDS.md, docs/adding-a-write-surface.md, docs/golden-principles.md, internal/cmd/mail/draft.go, internal/rwcmd/calendar/update.go; 1 omitted; constraints: Shared standards were read from cli-common main via the GitHub API (command-surface.md, output-and-rendering.md); no local ../cli-common copy was present. internal/rw/gmail/client.go, mock_test.go and vacation_test.go are in the diff but outside assignment scope; they were read as context only and not reviewed for findings.
  • harness-engineering:repo-health — complete (constrained); scope: 6 files; examples: internal/rw/gmail/client.go, internal/rwcmd/mail/mail.go, internal/rwcmd/mail/mock_test.go, internal/rwcmd/mail/output.go, internal/rwcmd/mail/vacation.go; 1 omitted; body-inspected: 6 files; examples: internal/rw/gmail/client.go, internal/rwcmd/mail/mail.go, internal/rwcmd/mail/mock_test.go, internal/rwcmd/mail/output.go, internal/rwcmd/mail/vacation.go; 1 omitted; skipped: none; context (not coverage): 10 files; examples: WORKSPACE_ADMINS.md, docs/adding-a-write-surface.md, docs/architecture.md, docs/golden-principles.md, internal/app/grw/identity.go; 5 omitted; constraints: Static review only: tests and the CLI were not run, so the claim that removing the vacation registration leaves tests green comes from reading the tests, not from a mutation run.
  • security:code-auditor — complete (constrained); scope: 2 files; examples: internal/rw/gmail/client.go, internal/rwcmd/mail/vacation.go; body-inspected: 2 files; examples: internal/rw/gmail/client.go, internal/rwcmd/mail/vacation.go; skipped: none; context (not coverage): 5 files; examples: internal/app/grw/identity.go, internal/cmd/mail/draft.go, internal/rwcmd/mail/mail.go, internal/rwcmd/mail/output.go, internal/sanitize/sanitize.go; constraints: Outside assigned scope, non-exploitable: internal/app/grw/identity.go lines 25 and 46 still describe gmail.settings.basic as filters-only, so the scope text now understates that grw can also change the auto-reply. Static review only: no code was run and no Gmail API calls were made, so server-side PUT behavior for omitted fields was not verified.

0 PR discussion threads considered. 0 summarized; 0 resolved.


Completed in 2m 35s | ~$2.33 (est.) | claude-opus-5-5 | cr 0.10.367
Field Value
Model claude-opus-5-5
Reviewers go:implementation-tests, policies:conventions, harness-engineering:repo-health, security:code-auditor
Engine claude_cli · claude-opus-5-5
Reviewed by cr · monit-reviewer
Duration 2m 35s wall · 6m 16s compute
Cost ~$2.33 (est.)
Pricing basis baseline-2026-10-02/pricing
Tokens 70 in / 31.9k out

Per-workstream usage

  • orchestrator-selection — claude-opus-5-5
    • In: 4
    • Out: 1.7k
    • Cache read: 9.6k
    • Cache create: 11.6k
    • Cost: ~$0.13 (est.)
    • Duration: 20s
  • go:implementation-tests — claude-opus-5-5
    • In: 12
    • Out: 8.3k
    • Cache read: 128.5k
    • Cache create: 39.3k
    • Cost: ~$0.51 (est.)
    • Duration: 1m 38s
  • policies:conventions — claude-opus-5-5
    • In: 20
    • Out: 8.8k
    • Cache read: 335.1k
    • Cache create: 58.1k
    • Cost: ~$0.71 (est.)
    • Duration: 1m 41s
  • harness-engineering:repo-health — claude-opus-5-5
    • In: 16
    • Out: 7.6k
    • Cache read: 216.9k
    • Cache create: 47.3k
    • Cost: ~$0.57 (est.)
    • Duration: 1m 25s
  • security:code-auditor — claude-opus-5-5
    • In: 12
    • Out: 4.5k
    • Cache read: 100.9k
    • Cache create: 26.5k
    • Cost: ~$0.32 (est.)
    • Duration: 53s
  • orchestrator-rollup — claude-opus-5-5
    • In: 6
    • Out: 1.1k
    • Cache read: 57.3k
    • Cache create: 8.1k
    • Cost: ~$0.10 (est.)
    • Duration: 18s

Comment thread internal/rwcmd/mail/vacation.go Resolved
Comment thread internal/rwcmd/mail/vacation_test.go Resolved
Comment thread internal/rwcmd/mail/mail.go Resolved
Comment thread internal/rwcmd/mail/vacation.go Resolved
Comment thread internal/rw/gmail/client.go Resolved
Comment thread internal/rwcmd/mail/vacation.go Resolved
Comment thread internal/rwcmd/mail/vacation.go Resolved
Comment thread internal/rwcmd/mail/vacation.go Resolved
Comment thread internal/rw/gmail/client.go Resolved
Comment thread internal/rwcmd/mail/vacation.go Resolved
Use the get, update, enable and disable verbs, change only the flags
given on update, print post-state and ISO-8601 dates, show the HTML body
Gmail actually sends, and document the scope's new use.

@monit-reviewer monit-reviewer left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Automated PR Review

Reviewed commit: bb4b75143c2a
Profile: claude-monit-reviewer - Posting as: monit-reviewer

Summary

Reviewer Findings
go:implementation-tests 3
policies:conventions 0
harness-engineering:repo-health 2
security:code-auditor 0
go:implementation-tests (3 findings)

Minor - internal/rwcmd/mail/vacation.go:185

No test proves writeVacation forces RestrictToContacts and RestrictToDomain into the request. The PR states these flags are forced because the API client omits false values, but neither test file asserts restrictToContacts or restrictToDomain on the wire. TestUpdateVacation_SendsForcedFalse builds its own ForceSendFields with only EnableAutoReply, so it exercises the client and not this list, and TestVacationToggle_KeepsSettingsAndShowsResult checks only enableAutoReply. Removing both restriction names from line 185 would leave every test green. The user-visible case at risk is update --contacts-only=false on a responder with the restriction on, where false is the value that must reach Gmail; that path is also never run. Fix: add a handler test that starts from RestrictToContacts: true and RestrictToDomain: true, runs update --contacts-only=false --domain-only=false, marshals the captured settings, and asserts "restrictToContacts":false and "restrictToDomain":false.

Minor - internal/rwcmd/mail/vacation.go:110

Several of update's flag-to-field mappings are unproven. Coverage shows the --subject assignment (line 111) and the --contacts-only assignment (line 125) never run. --start (line 119) runs only in the reversed dates case, which returns an error before any value is checked. A copy-paste slip such as settings.StartTime = endMillis would still pass, because equal times also trigger must be before. Since update promises that only the given flags change, these mappings are its main contract, and a wrong one would silently write the wrong setting to a live responder. Fix: add a success test (or extend TestVacationUpdate_ChangesOnlyGivenFlags) passing --subject, --start and --contacts-only against existing values, and assert each resulting field exactly, including a --start value distinct from any --end.

Nits - internal/rwcmd/mail/vacation.go:93

The --file branch of the dry-run preview never runs in tests, and it renders differently from --body: file prints body: followed by a newline and the text, while --body falls to the default case and prints body: <text> on one line. TestVacationUpdate_DryRunDoesNotMutate only covers the inline form. This branch is also the only place file contents reach the preview, so a regression there would go unnoticed. Fix: print both sources in one format and add a dry-run case using a temp file that asserts the file's contents appear in the preview.

harness-engineering:repo-health (2 findings)

Minor - internal/rwcmd/mail/vacation.go:185

update promises to clear values, but the request omits them, and nothing checks that Gmail treats an omitted field as cleared. writeVacation force-sends only the three booleans. In the generated VacationSettings, ResponseBodyHtml, StartTime and EndTime are all omitempty, so setting them to empty or 0 at lines 116, 119 and 122 drops them from the request body. The help text says "An empty value clears a date", and the comment at line 114 says a stale HTML body "must go". Both rely on the comment at line 183 that "the update replaces every field". The code does not rely on that for the booleans, which it forces. The handler tests only inspect the struct passed to the mock, and TestUpdateVacation_SendsForcedFalse only asserts enableAutoReply. The PR notes that clearing a date is unchecked, but it does not mention the HTML case. If Gmail keeps omitted fields, then per the code's own comment --body on a reply with an HTML body would report success while Gmail keeps sending the old text. Fix: add ResponseBodyHtml, StartTime, EndTime and the other clearable text fields to ForceSendFields, if a live check confirms Gmail accepts the empty values, and assert them in a test on the serialized request. Otherwise verify the omission behavior against Gmail and record the result in the writeVacation comment.

Minor - internal/rwcmd/mail/vacation.go:87

update --dry-run can approve a command that the real run rejects. docs/golden-principles.md principle 4 makes --dry-run the review step for mutations, but the dry-run branch returns at line 98, before the start/end check at line 130. update --start 2026-03-08 --end 2026-03-02 --dry-run therefore prints a normal preview and exits 0, and the same command fails without the flag. The previous revision checked this before previewing, so this is a regression. TestVacationUpdate_RejectsInvalidInput has no dry-run case to catch it. The preview also prints the raw flag value through f.Value.String(), so --end 2026-03-08 hides the resolved next-midnight value the command would send, which is the behavior the PR calls out as non-obvious. Fix: when both --start and --end are given, compare them before the dry-run branch and add a dry-run case to the invalid-input table. Keep the merged-settings check at line 130 for values read from Gmail. Print startMillis and endMillis through formatVacationTime in the preview.

Reviewer Coverage

Complete coverage details are retained in the local coverage.json artifact.

  • go:implementation-tests — complete (constrained); scope: 11 files; examples: go.mod, internal/app/grw/identity.go, internal/app/grw/root.go, internal/rw/gmail/client.go, internal/rw/gmail/vacation_test.go; 6 omitted; body-inspected: 11 files; examples: go.mod, internal/app/grw/identity.go, internal/app/grw/root.go, internal/rw/gmail/client.go, internal/rw/gmail/vacation_test.go; 6 omitted; skipped: none; context (not coverage): 1 file; examples: internal/rwcmd/mail/filter_test.go; constraints: Ran go test with coverage for internal/rwcmd/mail, internal/rw/gmail and internal/architecture; all passed. No live Gmail API calls were made. Whether Gmail clears a date or HTML body when the field is omitted could not be checked here. The PR already discloses this for dates, so it is not raised as a finding.
  • policies:conventions — complete (constrained); scope: 4 files; examples: README.md, internal/rwcmd/mail/mail.go, internal/rwcmd/mail/output.go, internal/rwcmd/mail/vacation.go; body-inspected: 4 files; examples: README.md, internal/rwcmd/mail/mail.go, internal/rwcmd/mail/output.go, internal/rwcmd/mail/vacation.go; skipped: none; constraints: Other changed files, including tests, go.mod and WORKSPACE_ADMINS.md, are outside assignment scope and were not reviewed. Architecture tests and make check were not run; the PR reports make check passing. Shared and repo standards (cli-common command-surface.md and output-and-rendering.md; docs/golden-principles.md; docs/adding-a-write-surface.md) were read in the review of the prior head 87d5221 and not re-fetched for this head.
  • harness-engineering:repo-health — complete (constrained); scope: 11 files; examples: go.mod, internal/app/grw/identity.go, internal/app/grw/root.go, internal/rw/gmail/client.go, internal/rw/gmail/vacation_test.go; 6 omitted; body-inspected: 11 files; examples: go.mod, internal/app/grw/identity.go, internal/app/grw/root.go, internal/rw/gmail/client.go, internal/rw/gmail/vacation_test.go; 6 omitted; skipped: none; context (not coverage): 4 files; examples: README.md, WORKSPACE_ADMINS.md, docs/adding-a-write-surface.md, docs/golden-principles.md; constraints: Static review only: tests and the CLI were not run, and Gmail's handling of omitted fields on updateVacation was not checked against the live API or its docs, so that finding describes an unverified behavior, not a confirmed failure.
  • security:code-auditor — complete (constrained); scope: 3 files; examples: WORKSPACE_ADMINS.md, internal/rw/gmail/client.go, internal/rwcmd/mail/vacation.go; body-inspected: 3 files; examples: WORKSPACE_ADMINS.md, internal/rw/gmail/client.go, internal/rwcmd/mail/vacation.go; skipped: none; context (not coverage): 5 files; examples: go.mod, internal/app/grw/identity.go, internal/app/grw/root.go, internal/rwcmd/mail/mail.go, internal/sanitize/sanitize.go; constraints: Static review only: no code was run and no Gmail API calls were made, so Gmail's handling of fields omitted from the update request, including cleared dates, was not verified.

0 PR discussion threads considered. 0 summarized; 0 resolved.


Completed in 2m 11s | ~$2.87 (est.) | claude-opus-5-5 | cr 0.10.367
Field Value
Model claude-opus-5-5
Reviewers go:implementation-tests, policies:conventions, harness-engineering:repo-health, security:code-auditor
Engine claude_cli · claude-opus-5-5
Reviewed by cr · monit-reviewer
Duration 2m 11s wall · 3m 35s compute
Cost ~$2.87 (est.)
Pricing basis baseline-2026-10-02/pricing
Tokens 38 in / 16.6k out

Per-workstream usage

  • go:implementation-tests — claude-opus-5-5
    • In: 10
    • Out: 7.2k
    • Cache read: 230.3k
    • Cache create: 77.9k
    • Cost: ~$0.81 (est.)
    • Duration: 1m 32s
  • policies:conventions — claude-opus-5-5
    • In: 4
    • Out: 1.8k
    • Cache read: 68.2k
    • Cache create: 71.5k
    • Cost: ~$0.62 (est.)
    • Duration: 25s
  • harness-engineering:repo-health — claude-opus-5-5
    • In: 10
    • Out: 4.8k
    • Cache read: 259.7k
    • Cache create: 77.6k
    • Cost: ~$0.77 (est.)
    • Duration: 56s
  • security:code-auditor — claude-opus-5-5
    • In: 8
    • Out: 2.0k
    • Cache read: 131.7k
    • Cache create: 44.5k
    • Cost: ~$0.42 (est.)
    • Duration: 27s
  • orchestrator-rollup — claude-opus-5-5
    • In: 6
    • Out: 788
    • Cache read: 58.5k
    • Cache create: 27.4k
    • Cost: ~$0.25 (est.)
    • Duration: 12s

Comment thread internal/rwcmd/mail/vacation.go Resolved
Comment thread internal/rwcmd/mail/vacation.go Resolved
Comment thread internal/rwcmd/mail/vacation.go Resolved
Comment thread internal/rwcmd/mail/vacation.go Resolved
Comment thread internal/rwcmd/mail/vacation.go Resolved
A dry run now rejects reversed dates and shows the body and times the
command would send. Tests cover each update flag and the forced false
restrictions.

@monit-reviewer monit-reviewer left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Automated PR Review

Reviewed commit: b857fe8a72ac
Profile: claude-monit-reviewer - Posting as: monit-reviewer

Summary

Reviewer Findings
go:implementation-tests 1
policies:conventions 0
harness-engineering:repo-health 0
security:code-auditor 0
go:implementation-tests (1 finding)

Minor - internal/rwcmd/mail/vacation.go:140

The check of merged dates no longer runs in any test. The new flag-level check at line 89 now catches the reversed dates table cases first, so coverage shows this return errVacationDateOrder is never reached. This is the only guard for a reversal formed against saved settings, such as update --start 2026-03-08 when the account's end time is earlier, or update --end before a saved start. Deleting lines 139-141 would leave every test green, and the command would then send the invalid pair to Gmail instead of giving the local error. Fix: add a table test using vacationMock with an existing EndTime and --start after it, plus an existing StartTime and --end before it. Assert that the error is errVacationDateOrder with errors.Is, and that UpdateVacation is not called.

Reviewer Coverage

Complete coverage details are retained in the local coverage.json artifact.

  • go:implementation-tests — complete (constrained); scope: 11 files; examples: go.mod, internal/app/grw/identity.go, internal/app/grw/root.go, internal/rw/gmail/client.go, internal/rw/gmail/vacation_test.go; 6 omitted; body-inspected: 11 files; examples: go.mod, internal/app/grw/identity.go, internal/app/grw/root.go, internal/rw/gmail/client.go, internal/rw/gmail/vacation_test.go; 6 omitted; skipped: none; constraints: Ran go test for internal/rwcmd/mail, internal/rw/gmail and internal/architecture; all passed. No live Gmail API calls were made, so Gmail clearing omitted fields rests on the PR's reported live check. Only vacation.go and vacation_test.go changed since bb4b751.
  • policies:conventions — complete (constrained); scope: 4 files; examples: README.md, internal/rwcmd/mail/mail.go, internal/rwcmd/mail/output.go, internal/rwcmd/mail/vacation.go; body-inspected: 4 files; examples: README.md, internal/rwcmd/mail/mail.go, internal/rwcmd/mail/output.go, internal/rwcmd/mail/vacation.go; skipped: none; constraints: Other changed files, including tests, go.mod and WORKSPACE_ADMINS.md, are outside assignment scope and were not reviewed. Architecture tests and make check were not run; the PR reports make check passing. Shared and repo standards (cli-common command-surface.md and output-and-rendering.md; docs/golden-principles.md; docs/adding-a-write-surface.md) were read in the review of an earlier head, 87d5221, and not re-fetched for this head.
  • harness-engineering:repo-health — complete (constrained); scope: 11 files; examples: go.mod, internal/app/grw/identity.go, internal/app/grw/root.go, internal/rw/gmail/client.go, internal/rw/gmail/vacation_test.go; 6 omitted; body-inspected: 11 files; examples: go.mod, internal/app/grw/identity.go, internal/app/grw/root.go, internal/rw/gmail/client.go, internal/rw/gmail/vacation_test.go; 6 omitted; skipped: none; constraints: Static review only: tests and the CLI were not run. Only vacation.go and vacation_test.go changed since bb4b751, so the other files were inspected at that revision and not re-read at this one. The claim that Gmail clears omitted fields, including the HTML body case, comes from the PR's reported live check and was not independently verified.
  • security:code-auditor — complete (constrained); scope: 3 files; examples: WORKSPACE_ADMINS.md, internal/rw/gmail/client.go, internal/rwcmd/mail/vacation.go; body-inspected: 3 files; examples: WORKSPACE_ADMINS.md, internal/rw/gmail/client.go, internal/rwcmd/mail/vacation.go; skipped: none; context (not coverage): 1 file; examples: go.mod; constraints: Static review only: no code was run and no Gmail API calls were made. Gmail's clearing of omitted fields is taken from the PR's stated live check, and the PR notes the HTML body case was not exercised live.

0 PR discussion threads considered. 0 summarized; 0 resolved.


Completed in 1m 28s | ~$3.32 (est.) | claude-opus-5-5 | cr 0.10.367
Field Value
Model claude-opus-5-5
Reviewers go:implementation-tests, policies:conventions, harness-engineering:repo-health, security:code-auditor
Engine claude_cli · claude-opus-5-5
Reviewed by cr · monit-reviewer
Duration 1m 28s wall · 2m 05s compute
Cost ~$3.32 (est.)
Pricing basis baseline-2026-10-02/pricing
Tokens 28 in / 7.2k out

Per-workstream usage

  • go:implementation-tests — claude-opus-5-5
    • In: 4
    • Out: 3.0k
    • Cache read: 86.1k
    • Cache create: 98.6k
    • Cost: ~$0.86 (est.)
    • Duration: 48s
  • policies:conventions — claude-opus-5-5
    • In: 6
    • Out: 1.2k
    • Cache read: 171.5k
    • Cache create: 93.6k
    • Cost: ~$0.81 (est.)
    • Duration: 20s
  • harness-engineering:repo-health — claude-opus-5-5
    • In: 6
    • Out: 1.4k
    • Cache read: 166.0k
    • Cache create: 92.1k
    • Cost: ~$0.80 (est.)
    • Duration: 26s
  • security:code-auditor — claude-opus-5-5
    • In: 6
    • Out: 1.2k
    • Cache read: 115.9k
    • Cache create: 66.3k
    • Cost: ~$0.58 (est.)
    • Duration: 21s
  • orchestrator-rollup — claude-opus-5-5
    • In: 6
    • Out: 434
    • Cache read: 68.3k
    • Cache create: 30.7k
    • Cost: ~$0.27 (est.)
    • Duration: 8s

Comment thread internal/rwcmd/mail/vacation.go Resolved

@monit-reviewer monit-reviewer left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Automated PR Review

Reviewed commit: f71604df2e88
Profile: claude-monit-reviewer - Posting as: monit-reviewer

Summary

Reviewer Findings
go:implementation-tests 0
policies:conventions 0
harness-engineering:repo-health 0
security:code-auditor 0

Reviewer Coverage

Complete coverage details are retained in the local coverage.json artifact.

  • go:implementation-tests — complete (constrained); scope: 11 files; examples: go.mod, internal/app/grw/identity.go, internal/app/grw/root.go, internal/rw/gmail/client.go, internal/rw/gmail/vacation_test.go; 6 omitted; body-inspected: 11 files; examples: go.mod, internal/app/grw/identity.go, internal/app/grw/root.go, internal/rw/gmail/client.go, internal/rw/gmail/vacation_test.go; 6 omitted; skipped: none; constraints: Coverage still shows the dry-run preview lines for --start and for --subject and the restriction flags unexecuted (vacation.go:100, 104). These are simple display lines, so they are not raised as a finding. Only vacation_test.go changed since b857fe8; the other files were read at earlier revisions of this PR and verified unchanged by diff. Tests passed for internal/rwcmd/mail, internal/rw/gmail and internal/architecture. No live Gmail API calls were made.
  • policies:conventions — complete (constrained); scope: 4 files; examples: README.md, internal/rwcmd/mail/mail.go, internal/rwcmd/mail/output.go, internal/rwcmd/mail/vacation.go; body-inspected: 4 files; examples: README.md, internal/rwcmd/mail/mail.go, internal/rwcmd/mail/output.go, internal/rwcmd/mail/vacation.go; skipped: none; constraints: Other changed files, including tests, go.mod and WORKSPACE_ADMINS.md, are outside assignment scope and were not reviewed. Architecture tests and make check were not run; the PR reports make check passing. Shared and repo standards (cli-common command-surface.md and output-and-rendering.md; docs/golden-principles.md; docs/adding-a-write-surface.md) were read in the review of an earlier head, 87d5221, and not re-fetched for this head. vacation.go was read in full at b857fe8, not re-read…
  • harness-engineering:repo-health — complete (constrained); scope: 11 files; examples: go.mod, internal/app/grw/identity.go, internal/app/grw/root.go, internal/rw/gmail/client.go, internal/rw/gmail/vacation_test.go; 6 omitted; body-inspected: 11 files; examples: go.mod, internal/app/grw/identity.go, internal/app/grw/root.go, internal/rw/gmail/client.go, internal/rw/gmail/vacation_test.go; 6 omitted; skipped: none; constraints: Static review only: tests and the CLI were not run. Only vacation_test.go changed since b857fe8, so the other files were inspected at earlier revisions of this PR and not re-read at this one. The claim that Gmail clears omitted fields, including the HTML body case, comes from the PR's reported live check and was not independently verified.
  • security:code-auditor — complete (constrained); scope: 3 files; examples: WORKSPACE_ADMINS.md, internal/rw/gmail/client.go, internal/rwcmd/mail/vacation.go; body-inspected: 3 files; examples: WORKSPACE_ADMINS.md, internal/rw/gmail/client.go, internal/rwcmd/mail/vacation.go; skipped: none; constraints: Static review only: no code was run and no Gmail API calls were made. Gmail's clearing of omitted fields is taken from the PR's stated live check, and the PR notes the HTML body case was not exercised live.

0 PR discussion threads considered. 0 summarized; 0 resolved.


Completed in 1m 10s | ~$0.78 (est.) | claude-opus-5-5 | cr 0.10.367
Field Value
Model claude-opus-5-5
Reviewers go:implementation-tests, policies:conventions, harness-engineering:repo-health, security:code-auditor
Engine claude_cli · claude-opus-5-5
Reviewed by cr · monit-reviewer
Duration 1m 10s wall · 1m 39s compute
Cost ~$0.78 (est.)
Pricing basis baseline-2026-10-02/pricing
Tokens 30 in / 5.7k out

Per-workstream usage

  • go:implementation-tests — claude-opus-5-5
    • In: 6
    • Out: 1.6k
    • Cache read: 315.9k
    • Cache create: 12.1k
    • Cost: ~$0.19 (est.)
    • Duration: 30s
  • policies:conventions — claude-opus-5-5
    • In: 6
    • Out: 1.3k
    • Cache read: 291.1k
    • Cache create: 10.0k
    • Cost: ~$0.16 (est.)
    • Duration: 20s
  • harness-engineering:repo-health — claude-opus-5-5
    • In: 6
    • Out: 1.2k
    • Cache read: 291.7k
    • Cache create: 11.7k
    • Cost: ~$0.18 (est.)
    • Duration: 19s
  • security:code-auditor — claude-opus-5-5
    • In: 6
    • Out: 1.1k
    • Cache read: 215.4k
    • Cache create: 15.8k
    • Cost: ~$0.19 (est.)
    • Duration: 20s
  • orchestrator-rollup — claude-opus-5-5
    • In: 6
    • Out: 514
    • Cache read: 105.4k
    • Cache create: 3.2k
    • Cost: ~$0.06 (est.)
    • Duration: 9s

@piekstra
piekstra marked this pull request as ready for review November 4, 2026 22:10
@piekstra
piekstra merged commit c8931ae into main Nov 4, 2026
11 checks passed
@piekstra
piekstra deleted the piekstra/mail-vacation branch November 4, 2026 22:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants