Repository navigation
feat(mail): manage the Gmail vacation responder - #41
Conversation
Add grw mail vacation show, set and off, using the gmail.settings.basic scope grw already requests.
monit-reviewer
left a comment
There was a problem hiding this comment.
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
printVacationprefersResponseBodyPlainTextand falls back toResponseBodyHtmlonly 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,showcan display text that differs from what Gmail actually sends. Becauseoffkeeps the existing fields, the same mismatch can persist across later reads. Fix: selectResponseBodyHtmlfirst and fall back to plain text, and cover the both-set case in theshowtest.
Minor - internal/rwcmd/mail/vacation.go:160
The RFC3339 branch of
parseVacationTimeis never executed by the tests, although--startand--endboth advertise RFC3339. The contract at risk is that only a bare date moves to the next midnight, while an explicit--endtimestamp is sent exactly as given. If theendOfDayadjustment 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 forparseVacationTimecovering RFC3339 with bothendOfDayvalues (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 showhas no handler test. Coverage fornewVacationShowCommandis 11.1%, and that comes only from the architecture test walking the command tree, so RunE never executes. Nothing provesshowcalls GetVacation, returns its error, or prints the fields a user relies on. TheResponseBodyHtmlfallback inprintVacationis also never executed, andsanitize.Outputon 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, andTestFilter_Listis the equivalent precedent. Fix: addTestVacationShowusingGetVacationFuncandtestutil.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 bodiescase would pass even if the mutual-exclusion guard were deleted. It passes--body-file b, which does not exist, so removing the guard makesos.ReadFilefail and the test still sees an error, because the table only checkserr != nil. The other cases have the same weakness in that they never check which validation fired, whereasTestFilter_CreateRequiresCriterionmatches on the message. The success path of--body-fileis also unexercised, so nothing proves the file contents reachResponseBodyPlainText. Fix: make the table map each case to an expected error substring (for examplemutually exclusive), point--body-fileat a real file fromt.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.setis explicitly excluded for resource operations ("Useupdate"; the only exception isconfig set).off(line 123) is an invented synonym fordisable, 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-planeconfig show; resource reads useget. Renaming gets harder after release becausefeat:merges ship automatically and scripts will bind to these names. Suggested fix: rename togrw mail vacation get,updateanddisablebefore merge, and update the PR description.TestWriteLeavesHaveDryRunexemptsget, so the dry-run rule is still met.
Minor - internal/rwcmd/mail/vacation.go:145
offdiscards theUpdateVacationresponse and prints only a confirmation line.output-and-rendering.md§4.6 says mutation success output mirrors thegetoutput so callers see the post-state without a follow-up fetch; confirmation-only output is reserved for delete, archive and remove.setalready 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 callprintVacation(updated)after aVacation responder turned off:line.
Minor - internal/rwcmd/mail/vacation.go:111
--body-fileis a new synonym for an existing flag.mail draftalready reads a body from a file with--file/-f(internal/cmd/mail/draft.go:340), andcommand-surface.md§8.1 requires reusing the existing name before inventing one. An agent that knowsmail draft --filewill hit an unknown-flag error here. Suggested fix: rename to--file, keeping draft's-fbinding 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 printsMon, Jan 2, 2006 3:04 PM MSTand(not set)(line 189).mail drafthas a similar human format atinternal/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 withtime.RFC3339asrefreshandprofilesalready 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.basicis 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:25and theScopeDescriptionsentry 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". ThegrwrootLongtext ininternal/app/grw/root.goalso omits the responder, although the README was updated. A user or Workspace admin reading these would not learn that the grant also letsgrwturn 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.mdand 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
offrequest has no wire-level check.docs/adding-a-write-surface.mdstep 6 requires API-client tests for a write surface, andinternal/rw/gmail/drafts_test.goalready has an httptest harness (testDraftClient) for this.TestVacationOff_KeepsSettingsAndSendsFalseonly callsjson.Marshalon the struct handed to the mock, so it never exercises the realUsers.Settings.UpdateVacationcall. That gap matters here because the PR saysoffhas only run against mocks and in--dry-run, so nothing has ever checked the actual request it sends. Fix: addGetVacationandUpdateVacationtests tointernal/rw/gmailusing 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, asTestDraftAPIErrorsWrapdoes.
Minor - internal/rwcmd/mail/mail.go:21
Nothing checks that the
vacationtree is reachable fromgrw mail, andshowhas no test.vacation_test.gobuilds the leaves directly withnewVacationSetCommand()andnewVacationOffCommand().TestNewCommandCompositionandTestFolderAndFilterSubcommandsininternal/rwcmd/mail/mail_test.golistfolderandfilterby name but were not extended.TestWriteCommandsExtendReadCommandsonly requires at least one added leaf, andTestWriteLeavesHaveDryRunonly checks leaves that are registered. Dropping thisAddCommandline would therefore remove the feature with all tests still passing, andsetandoffwould also escape the dry-run check.newVacationShowCommandis never executed in tests either, unlike its siblingTestFilter_List, and the new handlers have no API-error or client-creation-error cases, which step 6 ofdocs/adding-a-write-surface.mdasks for. Fix: addvacationto the composition list, assert itsshow,setandoffsubcommands, and add ashowoutput test plus one*_APIErrorand one*_ClientCreationErrorcase.
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
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
left a comment
There was a problem hiding this comment.
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
writeVacationforcesRestrictToContactsandRestrictToDomaininto the request. The PR states these flags are forced because the API client omits false values, but neither test file assertsrestrictToContactsorrestrictToDomainon the wire.TestUpdateVacation_SendsForcedFalsebuilds its ownForceSendFieldswith onlyEnableAutoReply, so it exercises the client and not this list, andTestVacationToggle_KeepsSettingsAndShowsResultchecks onlyenableAutoReply. Removing both restriction names from line 185 would leave every test green. The user-visible case at risk isupdate --contacts-only=falseon 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 fromRestrictToContacts: trueandRestrictToDomain: true, runsupdate --contacts-only=false --domain-only=false, marshals the captured settings, and asserts"restrictToContacts":falseand"restrictToDomain":false.
Minor - internal/rwcmd/mail/vacation.go:110
Several of
update's flag-to-field mappings are unproven. Coverage shows the--subjectassignment (line 111) and the--contacts-onlyassignment (line 125) never run.--start(line 119) runs only in thereversed datescase, which returns an error before any value is checked. A copy-paste slip such assettings.StartTime = endMilliswould still pass, because equal times also triggermust be before. Sinceupdatepromises 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 extendTestVacationUpdate_ChangesOnlyGivenFlags) passing--subject,--startand--contacts-onlyagainst existing values, and assert each resulting field exactly, including a--startvalue distinct from any--end.
Nits - internal/rwcmd/mail/vacation.go:93
The
--filebranch of the dry-run preview never runs in tests, and it renders differently from--body:fileprintsbody:followed by a newline and the text, while--bodyfalls to the default case and printsbody: <text>on one line.TestVacationUpdate_DryRunDoesNotMutateonly 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
updatepromises to clear values, but the request omits them, and nothing checks that Gmail treats an omitted field as cleared.writeVacationforce-sends only the three booleans. In the generatedVacationSettings,ResponseBodyHtml,StartTimeandEndTimeare allomitempty, 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, andTestUpdateVacation_SendsForcedFalseonly assertsenableAutoReply. 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--bodyon a reply with an HTML body would report success while Gmail keeps sending the old text. Fix: addResponseBodyHtml,StartTime,EndTimeand the other clearable text fields toForceSendFields, 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 thewriteVacationcomment.
Minor - internal/rwcmd/mail/vacation.go:87
update --dry-runcan approve a command that the real run rejects.docs/golden-principles.mdprinciple 4 makes--dry-runthe 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-runtherefore 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_RejectsInvalidInputhas no dry-run case to catch it. The preview also prints the raw flag value throughf.Value.String(), so--end 2026-03-08hides the resolved next-midnight value the command would send, which is the behavior the PR calls out as non-obvious. Fix: when both--startand--endare 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. PrintstartMillisandendMillisthroughformatVacationTimein 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
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
left a comment
There was a problem hiding this comment.
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 datestable cases first, so coverage shows thisreturn errVacationDateOrderis never reached. This is the only guard for a reversal formed against saved settings, such asupdate --start 2026-03-08when the account's end time is earlier, orupdate --endbefore 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 usingvacationMockwith an existingEndTimeand--startafter it, plus an existingStartTimeand--endbefore it. Assert that the error iserrVacationDateOrderwitherrors.Is, and thatUpdateVacationis 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
monit-reviewer
left a comment
There was a problem hiding this comment.
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
Adds an out-of-office reply to
grw, which until now had no way to read or change it.grw mail vacation getprints the current settings.grw mail vacation updatechanges only the flags given:--subject,--bodyor--file,--start,--end,--contacts-only,--domain-only. It leaves the on/off state alone.grw mail vacation enableanddisableturn the reply on and off and keep its text and dates.No scope change:
updateVacationneedsgmail.settings.basic, whichgrwalready requests. The scope descriptions, admin guide and help text now mention this use.grois unchanged.--endtakes 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 checkandmake test-cover-checkpass. Against a real Workspace account, on a reply whose start date was still in the future so nothing was sent:disablereturnedEnabled: falseand kept the text and dates.enablereturnedEnabled: true, and a followinggetconfirmed 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 secondupdaterestored 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.