Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Refresh package specs for spec-audit mismatches and stale README coverage #44564
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. Weβll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Uh oh!
There was an error while loading. Please reload this page.
Refresh package specs for spec-audit mismatches and stale README coverage #44564
Changes from 2 commits
58e037dd5792fbcd1b9bcb85e6faFile filter
Filter by extension
Conversations
Uh oh!
There was an error while loading. Please reload this page.
Jump to
Uh oh!
There was an error while loading. Please reload this page.
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[/grill-with-docs] The SHOULD recommendation here is slightly misleading β callers that pass
nilforctxare safe becauseResolveActionPinallocates a freshPinContextin that case, andinitWarningsauto-initialises the map on first use. The real risk is only for callers who pass a non-nilPinContextwith a nilWarningsfield and then reuse it across calls that exercise the Mappings path.π‘ Suggested rewording
@copilot please address this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Misleading claim: "split into local and remote variants" implies behavioral divergence, but
DefaultGitHubToolsLocalandDefaultGitHubToolsRemotecurrently both aliasDefaultReadOnlyGitHubToolsβ they are identical in value today.π‘ Suggested correction
The constants source explicitly comments that they are "currently identical" with potential to diverge. The README should match this reality:
The current wording implies the split already carries meaning, which could lead callers to assume they get different tool sets per mode when they do not.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[/grill-with-docs] The new design note describes iteration order as "walks the issue's labels in their existing order" β this accurately reflects the code, but the fallback condition is slightly imprecise: it returns the first label from
matchingValues(positional in the matched list), not the first matching label in issue-label order.π‘ Suggested rewording
See
computeValueFirstinlabel_objective_mapping.goβ the fallback ismatchingValues[0], which is the first element collected during the matching scan, not necessarily the first label in issue order.@copilot please address this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Inaccurate fix guidance for
!=comparisons: the description says both==and!=comparisons "should usebytes.Equal", but!=requires!bytes.Equal(a, b)βbytes.Equalalone inverts the semantics.π‘ Suggested correction
Current text:
Suggested:
A developer consulting this README and applying the
bytes.Equalsuggestion to a!=comparison would silently negate the condition, introducing a logic bug. The same inaccuracy appears in both the bullet list and table entry β both should be corrected.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[/grill-with-docs] The phrase "supports authentication via either
github-token,github-app, orsafe-outputs-github-app" implies all three options are mutually exclusive, but the code shows they are not βsafe-outputs-github-appis independent and can coexist withgithub-tokenorgithub-app.π‘ Suggested rewording
From
checkout_manager.go,github-tokenandgithub-appblock each other (first-seen wins), whilesafe-outputs-github-apphas its own independent first-seen rule and can co-exist in the same entry.@copilot please address this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[/grill-with-docs] "tracks whether any checkout requires GitHub App token minting" is accurate but undersells what
CheckoutManagerdoes β it also resolves and deduplicates overlapping checkout requests and generates deterministic step plans.π‘ Suggested addition
See
checkout_manager.goβ theMergelogic handles deduplication and the public API surface goes beyond just tracking the app flag.@copilot please address this.
Uh oh!
There was an error while loading. Please reload this page.