feat: integrate upstream v1.11 and add fork release assets - #9
Conversation
* feat(mail): add bounded page collector * fix(mail): validate semantic continuation scopes * feat(mail): collect bounded message pages * fix(mail): guard empty message page responses * feat(mail): add list order and JSON projection * fix(mail): preserve selected JSON safety * fix(mail): reject explicit empty select * docs(mail): document bounded list traversal * fix(mail): reject ordered classification filters * fix(mail): keep list options immutable * fix(mail): preserve list top option * feat(mail): verify provider body format * fix(mail): hydrate typed thread bodies * feat(mail): expose protected provider contracts * fix(cli): emit structured JSON errors * feat(mail): add complete thread traversal * fix(mail): preserve complete recipient evidence * feat(mail): support immutable provider IDs * fix(mail): accept combined preference-applied headers * feat(mail): expose stable message observations * fix(mail): preserve non-json select output * fix(mail): satisfy lint for provider contracts --------- Co-authored-by: rlrghb <roshin@roshin-macmini.local>
Amp-Thread-ID: https://ampcode.com/threads/T-019fec28-bf40-76a7-a4de-aa47fdf3c610 # Conflicts: # internal/cmd/root.go
Amp-Thread-ID: https://ampcode.com/threads/T-019fec28-bf40-76a7-a4de-aa47fdf3c610 Co-authored-by: Dustin Lancaster <dustin@planmonster.com>
Amp-Thread-ID: https://ampcode.com/threads/T-019fec28-bf40-76a7-a4de-aa47fdf3c610 Co-authored-by: Dustin Lancaster <dustin@planmonster.com>
📝 WalkthroughWalkthroughChangesFork release automation
CLI and mail capabilities
Sequence Diagram(s)sequenceDiagram
participant MailCommand
participant GraphClient
participant MicrosoftGraph
participant BodyContract
MailCommand->>GraphClient: request message or thread with body preference
GraphClient->>MicrosoftGraph: send paged or batch request
MicrosoftGraph-->>GraphClient: return messages, continuation links, and headers
GraphClient->>BodyContract: validate preference, identity, conversation, and continuation
BodyContract-->>GraphClient: return validated messages
GraphClient-->>MailCommand: return bounded or complete results
✨ Finishing Touches📝 Generate docstrings
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (5)
internal/graphapi/mail_pages_test.go (1)
39-42: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick win
t.Fatalfandt.Fatalrun inside HTTP responders in three test files. The shared root cause is one pattern: aFailNow-family call inside anhttp.RoundTripperresponder.t.Fatalfcallsruntime.Goexit, and the Go testing package restricts that to the goroutine running the test function. When the HTTP client invokes the responder on another goroutine, the responder returns no response and the test hangs instead of reporting the failure. Several sites alsoreturn nilafter the call, which yields a nil*http.Responseif execution continues. Replace each call witht.Errorfand return a valid synthetic response.
internal/graphapi/mail_pages_test.go#L39-L42: change thedefaultbranch tot.Errorfand returngraphJSONResponse(req,+ "" +{"value":[]}+ "" +)instead ofnil; apply the same change to the seconddefaultbranch at Lines 175-178.internal/cmd/mail_list_test.go#L252-L257: change the batch-decode and request-countt.Fatalfcalls tot.Errorfand return a well-formed batch response; apply the same change at Lines 343-348 and Lines 384-389.internal/cmd/mail_provider_contract_test.go#L60-L76: in the "no Graph request expected" responder, changet.Fatalftot.Errorfand return a valid response instead ofnil.Note that
t.Fatalcalls inside thecollectMessagePagespage callbacks ininternal/graphapi/mail_pages_test.goare correct, becausecollectMessagePagesinvokes those callbacks synchronously on the test goroutine.internal/cmd/mail_folders.go (1)
53-75: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueHoist the loop-invariant
WellKnownbranch out of the row loop.
c.WellKnown != ""does not change between iterations. The loop builds the four-column row, then discards it and rebuilds a five-column row. The header is then set after the loop. Decide the shape once before the loop.♻️ Proposed refactor
- headers := []string{"ID", "NAME", "TOTAL", "UNREAD"} + wellKnown := c.WellKnown != "" + headers := []string{"ID", "NAME", "TOTAL", "UNREAD"} + if wellKnown { + headers = []string{"ID", "NAME", "WELL-KNOWN", "TOTAL", "UNREAD"} + } rows := make([][]string, 0, len(folders)) for _, f := range folders { - row := []string{ - f.ID, - f.DisplayName, - fmt.Sprintf("%d", f.TotalCount), - fmt.Sprintf("%d", f.UnreadCount), - } - if c.WellKnown != "" { - row = []string{ - f.ID, - f.DisplayName, - f.WellKnownName, - fmt.Sprintf("%d", f.TotalCount), - fmt.Sprintf("%d", f.UnreadCount), - } - } - rows = append(rows, row) - } - if c.WellKnown != "" { - headers = []string{"ID", "NAME", "WELL-KNOWN", "TOTAL", "UNREAD"} + row := []string{f.ID, f.DisplayName} + if wellKnown { + row = append(row, f.WellKnownName) + } + row = append(row, + fmt.Sprintf("%d", f.TotalCount), + fmt.Sprintf("%d", f.UnreadCount), + ) + rows = append(rows, row) }internal/cmd/mail_list.go (1)
27-41: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winGuard against drift between the selector registry and the projection switch.
Three declarations must stay in sync:
mailListSelectableFields, the fields ofmailListJSONMessage, and theswitchinprojectMailMessage. Theswitchhas nodefaultbranch. If a future change adds an entry tomailListSelectableFieldswithout a matchingcase,parseMailSelectaccepts the selector, the Graph query requests the field, and the JSON output omits it silently. That is the exact failure mode the comment on Line 24 warns about.Make the omission loud. One option is a
defaultbranch that returns an error. A stronger option is to derivemailListSelectableFieldsfrom a single table of setter functions, so one entry defines both admission and projection.♻️ Minimal guard: single source of truth for admission and projection
-var mailListSelectableFields = map[string]bool{ - "id": true, - "subject": true, - "from": true, - "toRecipients": true, - "ccRecipients": true, - "bccRecipients": true, - "replyTo": true, - "receivedDateTime": true, - "isRead": true, - "hasAttachments": true, - "bodyPreview": true, - "categories": true, - "conversationId": true, -} +var mailListSelectableFields = map[string]func(*mailListJSONMessage, *graphapi.MailMessage){ + "id": func(p *mailListJSONMessage, m *graphapi.MailMessage) { p.ID = &m.ID }, + "subject": func(p *mailListJSONMessage, m *graphapi.MailMessage) { p.Subject = &m.Subject }, + "from": func(p *mailListJSONMessage, m *graphapi.MailMessage) { p.From = &m.From }, + "toRecipients": func(p *mailListJSONMessage, m *graphapi.MailMessage) { p.To = &m.To }, + "ccRecipients": func(p *mailListJSONMessage, m *graphapi.MailMessage) { p.Cc = &m.Cc }, + "bccRecipients": func(p *mailListJSONMessage, m *graphapi.MailMessage) { p.Bcc = &m.Bcc }, + "replyTo": func(p *mailListJSONMessage, m *graphapi.MailMessage) { p.ReplyTo = &m.ReplyTo }, + "receivedDateTime": func(p *mailListJSONMessage, m *graphapi.MailMessage) { p.ReceivedAt = &m.ReceivedAt }, + "isRead": func(p *mailListJSONMessage, m *graphapi.MailMessage) { p.IsRead = &m.IsRead }, + "hasAttachments": func(p *mailListJSONMessage, m *graphapi.MailMessage) { p.HasAttachments = &m.HasAttachments }, + "bodyPreview": func(p *mailListJSONMessage, m *graphapi.MailMessage) { p.BodyPreview = &m.BodyPreview }, + "categories": func(p *mailListJSONMessage, m *graphapi.MailMessage) { p.Categories = &m.Categories }, + "conversationId": func(p *mailListJSONMessage, m *graphapi.MailMessage) { p.ConversationID = &m.ConversationID }, +}
parseMailSelectalready uses the two-value map lookup at Line 174, so it needs no change.projectMailMessagecollapses to:func projectMailMessage(message *graphapi.MailMessage, selected []string) mailListJSONMessage { projected := mailListJSONMessage{} for _, field := range selected { - switch field { - case "id": - projected.ID = &message.ID - // ... eleven more cases ... - case "conversationId": - projected.ConversationID = &message.ConversationID - } + if apply, ok := mailListSelectableFields[field]; ok { + apply(&projected, message) + } } return projected }Also applies to: 225-258
internal/graphapi/mail.go (1)
766-771: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove the dead slice initialization.
Lines 790-793 overwrite
To,Cc,Bcc, andReplyTounconditionally withrecipientAddresses, which always returns a non-nil slice. The initializer values are never observed.♻️ Proposed simplification
- m := MailMessage{ - To: []string{}, - Cc: []string{}, - Bcc: []string{}, - ReplyTo: []string{}, - } + m := MailMessage{}internal/graphapi/mail_provider_contract_test.go (1)
12-47: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert the requested
$selectso the projection contract is covered.The stub ignores query parameters, so this test passes even though
messageDetailSelectininternal/graphapi/mail.goomitsreplyTo. Real Graph omits unselected properties. Add an assertion onreq.URL.Query().Get("$select")that requires every recipient class the test verifies.♻️ Proposed assertion
client := testGraphClient(t, func(req *http.Request) *http.Response { + selected := req.URL.Query().Get("$select") + for _, field := range []string{"toRecipients", "ccRecipients", "bccRecipients", "replyTo"} { + if !strings.Contains(selected, field) { + t.Errorf("$select = %q, want %s requested", selected, field) + } + } return graphJSONResponse(req, `{
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 63a5288b-e8bd-4887-95ff-57ef733f4fd4
📒 Files selected for processing (36)
.github/workflows/release-planmonster.ymlAGENTS.mdREADME.mdSKILL.mddocs/github-releases.mddocs/npm-publishing.mdinternal/cmd/config.gointernal/cmd/mail_batch.gointernal/cmd/mail_folders.gointernal/cmd/mail_get.gointernal/cmd/mail_list.gointernal/cmd/mail_list_test.gointernal/cmd/mail_move.gointernal/cmd/mail_provider_contract_test.gointernal/cmd/mail_thread.gointernal/cmd/root.gointernal/cmd/root_error_test.gointernal/cmd/token.gointernal/cmd/token_test.gointernal/cmd/version.gointernal/cmd/version_test.gointernal/config/config_test.gointernal/graphapi/client.gointernal/graphapi/client_guards_test.gointernal/graphapi/delta.gointernal/graphapi/delta_test.gointernal/graphapi/mail.gointernal/graphapi/mail_batch.gointernal/graphapi/mail_batch_test.gointernal/graphapi/mail_body_preference.gointernal/graphapi/mail_body_preference_test.gointernal/graphapi/mail_immutable_id.gointernal/graphapi/mail_pages.gointernal/graphapi/mail_pages_test.gointernal/graphapi/mail_provider_contract_test.gointernal/graphapi/validate.go
| func writeCommandError( | ||
| jsonMode bool, | ||
| err error, | ||
| stdout io.Writer, | ||
| stderr io.Writer, | ||
| ) { | ||
| if jsonMode { | ||
| code, status := graphapi.ErrorMetadata(err) | ||
| value := struct { | ||
| Error struct { | ||
| Code string `json:"code"` | ||
| Status int `json:"status"` | ||
| } `json:"error"` | ||
| }{} | ||
| value.Error.Code = code | ||
| value.Error.Status = status | ||
| if encodeErr := json.NewEncoder(stdout).Encode(value); encodeErr != nil { | ||
| fmt.Fprintln(stderr, "Error: JSON error output failed") | ||
| } | ||
| return | ||
| } | ||
| fmt.Fprintf( | ||
| stderr, | ||
| "Error: %s\n", | ||
| outfmt.SanitizeMultiline(err.Error()), | ||
| ) | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
JSON mode discards the error message.
ErrorMetadata returns ("CommandFailed", 0) for any non-Graph error. In JSON mode the emitted object then carries only that code and status: 0. The original message text is written nowhere. A validation failure such as --select field "importance" is not available in mail list output becomes undiagnosable for the operator.
Write the sanitized message to stderr in addition to the structured object on stdout. Stdout stays parseable, and stderr keeps the human-readable hint.
🛠️ Proposed fix to retain diagnostics in JSON mode
if jsonMode {
code, status := graphapi.ErrorMetadata(err)
value := struct {
Error struct {
Code string `json:"code"`
Status int `json:"status"`
} `json:"error"`
}{}
value.Error.Code = code
value.Error.Status = status
if encodeErr := json.NewEncoder(stdout).Encode(value); encodeErr != nil {
fmt.Fprintln(stderr, "Error: JSON error output failed")
}
+ fmt.Fprintf(stderr, "Error: %s\n", outfmt.SanitizeMultiline(err.Error()))
return
}As per coding guidelines: "Keep stdout parseable for command output (--json or --plain); send human-readable hints and progress messages to stderr."
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func writeCommandError( | |
| jsonMode bool, | |
| err error, | |
| stdout io.Writer, | |
| stderr io.Writer, | |
| ) { | |
| if jsonMode { | |
| code, status := graphapi.ErrorMetadata(err) | |
| value := struct { | |
| Error struct { | |
| Code string `json:"code"` | |
| Status int `json:"status"` | |
| } `json:"error"` | |
| }{} | |
| value.Error.Code = code | |
| value.Error.Status = status | |
| if encodeErr := json.NewEncoder(stdout).Encode(value); encodeErr != nil { | |
| fmt.Fprintln(stderr, "Error: JSON error output failed") | |
| } | |
| return | |
| } | |
| fmt.Fprintf( | |
| stderr, | |
| "Error: %s\n", | |
| outfmt.SanitizeMultiline(err.Error()), | |
| ) | |
| } | |
| func writeCommandError( | |
| jsonMode bool, | |
| err error, | |
| stdout io.Writer, | |
| stderr io.Writer, | |
| ) { | |
| if jsonMode { | |
| code, status := graphapi.ErrorMetadata(err) | |
| value := struct { | |
| Error struct { | |
| Code string `json:"code"` | |
| Status int `json:"status"` | |
| } `json:"error"` | |
| }{} | |
| value.Error.Code = code | |
| value.Error.Status = status | |
| if encodeErr := json.NewEncoder(stdout).Encode(value); encodeErr != nil { | |
| fmt.Fprintln(stderr, "Error: JSON error output failed") | |
| } | |
| fmt.Fprintf(stderr, "Error: %s\n", outfmt.SanitizeMultiline(err.Error())) | |
| return | |
| } | |
| fmt.Fprintf( | |
| stderr, | |
| "Error: %s\n", | |
| outfmt.SanitizeMultiline(err.Error()), | |
| ) | |
| } |
Source: Coding guidelines
| if preference != MessageBodyDefault { | ||
| item := resp.GetResponseById(stepID) | ||
| if item == nil || item.GetStatus() == nil || *item.GetStatus() >= 400 { | ||
| return nil, fmt.Errorf("batch message %q did not return a successful provider body response", stepID) | ||
| } | ||
| if err := verifyPreferenceApplied(batchResponseHeader(item.GetHeaders(), preferenceAppliedHeader), preference); err != nil { | ||
| return nil, fmt.Errorf("batch message %q: %w", stepID, err) | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== locate file =="
git ls-files | rg '(^|/)mail_batch\.go$' || true
echo "== relevant code =="
if [ -f internal/graphapi/mail_batch.go ]; then
sed -n '1,130p' internal/graphapi/mail_batch.go | cat -n
fi
echo "== search batch status helper / GetResponseById usage =="
rg -n 'GetResponseById|status|GetHeaders|verifyPreferenceApplied|batch message|provider body response' internal/graphapi -S || trueRepository: PlanMonster/olkcli
Length of output: 11208
🌐 Web query:
Microsoft Graph batch request response operations status code successful response 2xx
💡 Result:
When using Microsoft Graph batch requests, it is critical to distinguish between the HTTP status code of the outer batch request and the individual status codes returned for each sub-request [1][2][3]. Outer Batch Response The outer HTTP status code for a batch request confirms only that the batch envelope itself was successfully received and processed by the server [1][3]. - If the batch request is well-formed and parseable, the outer response will typically return a 200 OK status [1][2]. - If the batch request is malformed, the outer response will return a 400 Bad Request status [1][4]. - A 200 OK status on the outer response does not guarantee that any of the individual requests inside the batch were successful [1][2][3]. Individual Request Responses Because individual requests within the batch are processed independently (unless dependencies are specified), you must iterate through the responses array in the batch response body to verify the outcome of each operation [1][2][3]. - Each entry in the responses array contains a status property, which provides the HTTP status code for that specific sub-request [1][4]. - You must check this individual status code for any value in the 2xx range to confirm a successful operation [1][5][2]. - If an individual request fails, it will contain a 4xx or 5xx status code [1][3]. Additionally, if a request has a dependency on another request that fails, the dependent request will typically return a 424 Failed Dependency status [5][3]. It is also important to note that individual responses may not appear in the same order as the requests in the original batch; you should use the id property to correlate responses with their corresponding requests [1][4][3].
Citations:
- 1: https://learn.microsoft.com/en-us/graph/json-batching
- 2: https://cli.nylas.com/guides/graph-api-batch-requests-explained
- 3: https://imrizwan.com/blog/microsoft-graph-batch-requests-combine-api-calls-2026
- 4: https://github.com/microsoftgraph/microsoft-graph-docs-contrib/blob/main/concepts/json-batching.md
- 5: https://learn.microsoft.com/en-us/graph/sdks/batch-requests
Reject non-2xx batch subresponses.
Line 74 accepts batch subresponse statuses from 300 through 399 as successful. Reject statuses below 200 or above 299 before checking Preference-Applied or deserializing the body.
Proposed fix
- if item == nil || item.GetStatus() == nil || *item.GetStatus() >= 400 {
+ if item == nil || item.GetStatus() == nil ||
+ *item.GetStatus() < 200 || *item.GetStatus() >= 300 {
return nil, fmt.Errorf("batch message %q did not return a successful provider body response", stepID)
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if preference != MessageBodyDefault { | |
| item := resp.GetResponseById(stepID) | |
| if item == nil || item.GetStatus() == nil || *item.GetStatus() >= 400 { | |
| return nil, fmt.Errorf("batch message %q did not return a successful provider body response", stepID) | |
| } | |
| if err := verifyPreferenceApplied(batchResponseHeader(item.GetHeaders(), preferenceAppliedHeader), preference); err != nil { | |
| return nil, fmt.Errorf("batch message %q: %w", stepID, err) | |
| } | |
| if preference != MessageBodyDefault { | |
| item := resp.GetResponseById(stepID) | |
| if item == nil || item.GetStatus() == nil || | |
| *item.GetStatus() < 200 || *item.GetStatus() >= 300 { | |
| return nil, fmt.Errorf("batch message %q did not return a successful provider body response", stepID) | |
| } | |
| if err := verifyPreferenceApplied(batchResponseHeader(item.GetHeaders(), preferenceAppliedHeader), preference); err != nil { | |
| return nil, fmt.Errorf("batch message %q: %w", stepID, err) | |
| } |
| func mailMessagesDeltaScope(target, folderID string) graphContinuationScope { | ||
| return graphContinuationScope{ | ||
| host: defaultGraphAPIHost, | ||
| collectionPath: graphUserCollectionPath(target, "mailFolders/"+url.PathEscape(folderID)+"/messages/delta"), | ||
| } | ||
| } | ||
|
|
||
| func calendarViewDeltaScope(target string) graphContinuationScope { | ||
| return graphContinuationScope{host: defaultGraphAPIHost, collectionPath: graphUserCollectionPath(target, "calendarView/delta")} | ||
| } | ||
|
|
||
| func contactsDeltaScope(target string) graphContinuationScope { | ||
| return graphContinuationScope{host: defaultGraphAPIHost, collectionPath: graphUserCollectionPath(target, "contacts/delta")} | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
Continuation validation pins the global Graph host, so sovereign-cloud paging and delta break. Every scope constructor sets host: defaultGraphAPIHost, and validateGraphContinuation requires an exact host match, while graphAPIHosts and internal/graphapi/delta_test.go still treat regional endpoints as valid.
internal/graphapi/mail_pages.go#L332-L345: derive the expected host from the configured Graph base URL instead ofdefaultGraphAPIHostinmailMessagesDeltaScope,calendarViewDeltaScope, andcontactsDeltaScope.internal/graphapi/mail_pages.go#L157-L165: make the scope host an input rather than a fixed constant, and keep thegraphAPIHostsallowlist check.internal/graphapi/delta.go#L105-L105: pass the resolved host throughmailMessagesDeltaScopeso mail delta continuations work on regional clouds.internal/graphapi/delta.go#L134-L134: pass the resolved host throughcalendarViewDeltaScope.internal/graphapi/delta.go#L159-L159: pass the resolved host throughcontactsDeltaScope.
📍 Affects 2 files
internal/graphapi/mail_pages.go#L332-L345(this comment)internal/graphapi/mail_pages.go#L157-L165internal/graphapi/delta.go#L105-L105internal/graphapi/delta.go#L134-L134internal/graphapi/delta.go#L159-L159
| // messageDetailSelect is the $select field set for a full single message (used by | ||
| // GetMessage and the batch fetch) — includes the body and conversation id. | ||
| var messageDetailSelect = []string{ | ||
| "id", "subject", "from", "toRecipients", "ccRecipients", "bccRecipients", | ||
| "receivedDateTime", "isRead", "hasAttachments", "body", "bodyPreview", "conversationId", | ||
| "parentFolderId", "changeKey", "flag", | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
messageDetailSelect omits replyTo, so GetMessage and the batch fetch always return an empty replyTo.
MailMessage now exposes ReplyTo, and convertMessage fills it from msg.GetReplyTo(). The list default selection at Lines 171-175 includes replyTo, but the single-message and batch selection does not. Graph omits unselected properties, so replyTo is empty for mail get and mail batch. The contract test passes only because the test transport ignores $select.
🐛 Proposed fix
var messageDetailSelect = []string{
- "id", "subject", "from", "toRecipients", "ccRecipients", "bccRecipients",
+ "id", "subject", "from", "toRecipients", "ccRecipients", "bccRecipients", "replyTo",
"receivedDateTime", "isRead", "hasAttachments", "body", "bodyPreview", "conversationId",
"parentFolderId", "changeKey", "flag",
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // messageDetailSelect is the $select field set for a full single message (used by | |
| // GetMessage and the batch fetch) — includes the body and conversation id. | |
| var messageDetailSelect = []string{ | |
| "id", "subject", "from", "toRecipients", "ccRecipients", "bccRecipients", | |
| "receivedDateTime", "isRead", "hasAttachments", "body", "bodyPreview", "conversationId", | |
| "parentFolderId", "changeKey", "flag", | |
| } | |
| // messageDetailSelect is the $select field set for a full single message (used by | |
| // GetMessage and the batch fetch) — includes the body and conversation id. | |
| var messageDetailSelect = []string{ | |
| "id", "subject", "from", "toRecipients", "ccRecipients", "bccRecipients", "replyTo", | |
| "receivedDateTime", "isRead", "hasAttachments", "body", "bodyPreview", "conversationId", | |
| "parentFolderId", "changeKey", "flag", | |
| } |
| msg, err := c.targetUser(target).Messages().ByMessageId(messageID).Get(ctx, &users.ItemMessagesMessageItemRequestBuilderGetRequestConfiguration{ | ||
| Headers: c.messageIDHeaders(headers), | ||
| Options: options, | ||
| QueryParameters: &users.ItemMessagesMessageItemRequestBuilderGetQueryParameters{ | ||
| Select: messageDetailSelect, | ||
| }, | ||
| }) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("getting message: %w", err) | ||
| } | ||
| if err := contract.verify(); err != nil { | ||
| return nil, fmt.Errorf("getting message: %w", err) | ||
| } | ||
| m := convertMessage(msg) | ||
| fillBody(&m, msg) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🔴 Critical | ⚡ Quick win
The new read path dereferences Graph SDK values without nil checks. Both sites assume the SDK returns non-nil values, which violates the repository nil-check rule and can panic at runtime.
internal/graphapi/mail.go#L260-L274: returnerrNilMessageResponsewhenmsgis nil, beforeconvertMessageandfillBody.internal/graphapi/mail.go#L818-L830: skip nil elements inrecipientsbefore callingGetEmailAddress().
📍 Affects 1 file
internal/graphapi/mail.go#L260-L274(this comment)internal/graphapi/mail.go#L818-L830
Source: Coding guidelines
| func recipientAddresses(recipients []models.Recipientable) []string { | ||
| result := make([]string, 0, len(recipients)) | ||
| for _, recipient := range recipients { | ||
| if recipient.GetEmailAddress() != nil && | ||
| recipient.GetEmailAddress().GetAddress() != nil { | ||
| result = append( | ||
| result, | ||
| *recipient.GetEmailAddress().GetAddress(), | ||
| ) | ||
| } | ||
| } | ||
| return result | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Nil-check each recipient element.
Graph deserialization can yield a nil element in a []models.Recipientable. recipient.GetEmailAddress() then panics. Add an element guard.
🛡️ Proposed guard
for _, recipient := range recipients {
- if recipient.GetEmailAddress() != nil &&
+ if recipient == nil {
+ continue
+ }
+ if recipient.GetEmailAddress() != nil &&
recipient.GetEmailAddress().GetAddress() != nil {As per coding guidelines "Always nil-check pointer values returned by the Microsoft Graph SDK before dereferencing them."
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func recipientAddresses(recipients []models.Recipientable) []string { | |
| result := make([]string, 0, len(recipients)) | |
| for _, recipient := range recipients { | |
| if recipient.GetEmailAddress() != nil && | |
| recipient.GetEmailAddress().GetAddress() != nil { | |
| result = append( | |
| result, | |
| *recipient.GetEmailAddress().GetAddress(), | |
| ) | |
| } | |
| } | |
| return result | |
| } | |
| func recipientAddresses(recipients []models.Recipientable) []string { | |
| result := make([]string, 0, len(recipients)) | |
| for _, recipient := range recipients { | |
| if recipient == nil { | |
| continue | |
| } | |
| if recipient.GetEmailAddress() != nil && | |
| recipient.GetEmailAddress().GetAddress() != nil { | |
| result = append( | |
| result, | |
| *recipient.GetEmailAddress().GetAddress(), | |
| ) | |
| } | |
| } | |
| return result | |
| } |
Source: Coding guidelines
3 participants
Summary
b2e5ed84f24fbecffbf9899092497c2aeb4bb14awith an explicit two-parent merge commitOLK_ACCESS_TOKENenvironment-only (no--access-token, Kong help/schema field, argv propagation, persistence, or credential-store fallback)olk-pm-vX.Y.Z.NnamespaceMerge and conflict resolution
The only content conflict was
internal/cmd/root.go. The resolution keeps upstream's structured command-error writer and immutable-ID setup, while preserving PlanMonster's token-mode branch beforeStore()/Config()and exit-code 77 mapping.Post-merge commits:
5610545027d25d7b34d7dfe4001ec66f67824309— environment-only token hardening5c9f4c2fae4747fd142d0b4a9434bede32371ab9— fork-only GitHub release assetsToken-mode behavior
OLK_ACCESS_TOKENis read directly from the environment and has noRootFlags/Kong representationOLK_ACCESS_TOKEN_EXPIRES_ATfails before Graph/client/store/config access; expiry retains exit 77config get/setrefuses before disk accessOLK_ACCOUNT_EMAILremains a backward-compatible, non-authoritative display hint and is never used for authorizationOLK_NO_WRITE,OLK_NO_SEND,OLK_NO_INPUT,OLK_WRAP_UNTRUSTED,OLK_ENABLE_COMMANDS_EXACT, mailbox delegation, and MCP composition remain intactFork GitHub release design
.github/workflows/release-planmonster.ymltriggers only onolk-pm-vX.Y.Z.N(example:olk-pm-v1.11.0.1). It builds six archives, verifies the expected set, emits and verifieschecksums.txt, smoke-tests Linux AMD64, and creates a GitHub Release only.For the example tag, NanoClaw consumes
olk_1.11.0.1_linux_amd64.tar.gzand verifies it againstchecksums.txt. The workflow cannot match upstreamv*or fork npmnpm-v*, and does not invoke GoReleaser, Homebrew, npm, or the MCP Registry. No release tag was created.Validation
Passed locally:
go mod tidy+ cleango.mod/go.sumdrift checkgo vet ./...go build ./...go test -race -count=1 ./...golangci-lint v2.11.4 run ./...(0 issues)actionlintacross all workflows (custom Blacksmith label allowlisted)git diff --checkscripts/test-npm-package.shwas started but intentionally stopped before completion at the request to stop further optional validation.scripts/test-bootstrap-npm.shwas not run locally. Existing PR CI remains the authority for the package job.Release / merge safety
This PR does not move any existing tag, rewrite
main, create a release tag, publish an artifact, or merge itself. After review and green CI, merge normally; create the firstolk-pm-v*tag only as a separate release action followingdocs/github-releases.md.Need help on this PR? Tag
@codesmith-botwith what you need. Autofix is disabled.