Refresh package specs for spec-audit mismatches and stale README coverage#44564
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. No ADR enforcement needed: PR #44564 does not have the 'implementation' label and has only 29 new lines of code in business logic directories (threshold: 100). |
|
✅ Test Quality Sentinel completed test quality analysis. No test files were added or modified in this PR. All changed files are README.md documentation updates. Test Quality Sentinel skipped. |
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
|
|
There was a problem hiding this comment.
Pull request overview
This pull request refreshes package-level README specifications to resolve spec-audit mismatches and bring documentation back in sync with current gh-aw behavior (no implementation changes).
Changes:
- Document
pkg/workflowcheckout configuration types and clarifysafe-outputs-github-appauth behavior forsafe_outputs. - Fix spec mismatches (e.g.,
importinpututilinternal dependency onpkg/logger, andlinterscoverage forbytescomparestring). - Refresh multiple package READMEs with behavior notes that now match current implementations.
Show a summary per file
| File | Description |
|---|---|
| pkg/workflow/README.md | Adds documented CheckoutConfig / CheckoutManager API surface and clarifies safe_outputs-only GitHub App auth behavior. |
| pkg/syncutil/README.md | Documents OnceLoader.Override caching semantics. |
| pkg/semverutil/README.md | Specifies invalid-version handling behavior for IsCompatible. |
| pkg/repoutil/README.md | Documents NormalizeRepoForAPI host parsing behavior. |
| pkg/linters/README.md | Adds bytescomparestring analyzer to package overview/table/dependencies. |
| pkg/importinpututil/README.md | Corrects internal dependency contract to include pkg/logger. |
| pkg/github/README.md | Clarifies PriorityLabels behavior for "first" multi-label logic. |
| pkg/constants/README.md | Notes local/remote default GitHub tool list split and DefaultGitHubTools deprecation alias. |
| pkg/cli/README.md | Describes non-Cobra reusable helpers exposed by the CLI package. |
| pkg/agentdrain/README.md | Documents embedded default weights under data/ for bootstrapping. |
| pkg/actionpins/README.md | Adds note about PinContext.Warnings usage for mapping/warning deduplication. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 11/11 changed files
- Comments generated: 1
- Review effort level: Low
| // reference resolves against acme-corp/checkout@v4 pins | ||
| ``` | ||
|
|
||
| Because `PinContext.Warnings` is used for one-time mapping diagnostics, callers that reuse a `PinContext` SHOULD initialize it with a non-nil map when they want warning deduplication. |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Documentation-only PR. All claims verified against implementation:
- pkg/importinpututil — critical fix: removed false 'no dependencies' claim; pkg/logger import confirmed in source.
- pkg/linters — bytescomparestring subpackage exists; bullet list, API table, and dependency list all consistently updated.
- pkg/github — PriorityLabels behaviour matches computeValueFirst exactly (iterates issue labels in order, falls back to first matching value).
- pkg/workflow — CheckoutConfig/CheckoutManager types exist; safe-outputs-github-app description matches SafeOutputGitHubApp field semantics; sparse-pattern union and HasAppAuth confirmed.
- All other stale-spec additions (syncutil, repoutil, semverutil, agentdrain, cli, constants, actionpins) are accurate.
No blocking issues found.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · 54.1 AIC · ⌖ 6.18 AIC · ⊞ 4.8K
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /grill-with-docs — requesting changes on three precision issues found in the new spec text.
📋 Key Themes & Highlights
Key Themes
- Auth model description:
pkg/workflow/README.mdline 127 impliesgithub-token,github-app, andsafe-outputs-github-appare mutually exclusive, but only the first two are; the third is orthogonal and composable. - Fallback precision:
pkg/github/README.mdline 115 describes the"first"fallback as "first matching label" but the code returnsmatchingValues[0]— the first collected match, which may differ from first-in-issue-order. - nil-ctx safety:
pkg/actionpins/README.mdline 95 recommends callers initialiseWarningswithout noting thatnilctx is already safe (auto-allocated internally), which could mislead callers.
Positive Highlights
- ✅ Critical dependency correction in
pkg/importinpututil/README.md— the old "no dependencies" claim was clearly wrong and this fix aligns spec with reality. - ✅
bytescomparestringadditions topkg/linters/README.mdare thorough: overview list, API table, and dependencies all updated in lock-step. - ✅ All other stale spec refreshes (
syncutil,semverutil,repoutil,agentdrain,constants,cli) are accurate and verify cleanly against the implementation. - ✅ Good discipline throughout: purely additive changes, no behavior-altering edits.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · 125.1 AIC · ⌖ 9.15 AIC · ⊞ 6.6K
Comment /matt to run again
|
|
||
| #### Checkout configuration | ||
|
|
||
| The `checkout:` frontmatter key is parsed by `ParseCheckoutConfigs` into one or more `CheckoutConfig` values. Each entry may target the current repository or a cross-repository checkout, and supports authentication via either `github-token`, `github-app`, or `safe-outputs-github-app`. |
There was a problem hiding this comment.
[/grill-with-docs] The phrase "supports authentication via either github-token, github-app, or safe-outputs-github-app" implies all three options are mutually exclusive, but the code shows they are not — safe-outputs-github-app is independent and can coexist with github-token or github-app.
💡 Suggested rewording
supports authentication via `github-token` or `github-app` (mutually exclusive) and optionally `safe-outputs-github-app` for safe_outputs git operations.From checkout_manager.go, github-token and github-app block each other (first-seen wins), while safe-outputs-github-app has its own independent first-seen rule and can co-exist in the same entry.
@copilot please address this.
| - All label comparisons are case-insensitive: labels are normalised with `strings.ToLower(strings.TrimSpace(...))` before lookup. | ||
| - The default `MultiLabelLogic` is `"max"`. Callers that do not set this field get max-value semantics automatically. | ||
| - `PriorityLabels` is only consulted when `MultiLabelLogic` is `"first"`; it establishes evaluation precedence among matching labels. | ||
| - `PriorityLabels` is only consulted when `MultiLabelLogic` is `"first"`; the implementation walks the issue's labels in their existing order and returns the first label that also appears in `PriorityLabels`, falling back to the first matching label if no priority entry matches. |
There was a problem hiding this comment.
[/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
- `PriorityLabels` is only consulted when `MultiLabelLogic` is `"first"`; the implementation walks the issue's labels in their existing order and returns the value for the first label that also appears in `PriorityLabels`. If no priority entry matches, it falls back to the value of the first label from the `LabelToValue` map that matched any issue label.See computeValueFirst in label_objective_mapping.go — the fallback is matchingValues[0], which is the first element collected during the matching scan, not necessarily the first label in issue order.
@copilot please address this.
| // reference resolves against acme-corp/checkout@v4 pins | ||
| ``` | ||
|
|
||
| Because `PinContext.Warnings` is used for one-time mapping diagnostics, callers that reuse a `PinContext` SHOULD initialize it with a non-nil map when they want warning deduplication. |
There was a problem hiding this comment.
[/grill-with-docs] The SHOULD recommendation here is slightly misleading — callers that pass nil for ctx are safe because ResolveActionPin allocates a fresh PinContext in that case, and initWarnings auto-initialises the map on first use. The real risk is only for callers who pass a non-nil PinContext with a nil Warnings field and then reuse it across calls that exercise the Mappings path.
💡 Suggested rewording
Because `PinContext.Warnings` is used for one-time mapping diagnostics, callers that supply a non-nil `PinContext` and reuse it across multiple `ResolveActionPin` calls that exercise `Mappings` SHOULD initialise `Warnings` with a non-nil map. Passing `nil` as `ctx` is always safe: the function allocates a fresh context internally.@copilot please address this.
|
|
||
| - `github-app` changes the authentication used by the generated `actions/checkout` step itself. | ||
| - `safe-outputs-github-app` mints a GitHub App token only for later `safe_outputs` git operations (fetch/push) against the checkout target; it does **not** change activation or agent-job checkout authentication. | ||
| - `CheckoutManager` merges compatible checkout requests, unions sparse-checkout patterns, and tracks whether any checkout requires GitHub App token minting. |
There was a problem hiding this comment.
[/grill-with-docs] "tracks whether any checkout requires GitHub App token minting" is accurate but undersells what CheckoutManager does — it also resolves and deduplicates overlapping checkout requests and generates deterministic step plans.
💡 Suggested addition
- `CheckoutManager` merges compatible checkout requests, unions sparse-checkout patterns, deduplicates overlapping repo/ref pairs, and tracks whether any checkout requires GitHub App token minting for later safe_outputs operations.See checkout_manager.go — the Merge logic handles deduplication and the public API surface goes beyond just tracking the app flag.
@copilot please address this.
There was a problem hiding this comment.
Two documentation accuracy issues found
This PR refreshes 11 package READMEs — the majority of changes are accurate and well-matched to the current source. Two issues need attention.
Verified accurate: pkg/github, pkg/syncutil, pkg/semverutil, pkg/repoutil, pkg/workflow, pkg/actionpins, pkg/agentdrain, pkg/importinpututil, pkg/cli — all checked against source.
See inline comments for the two issues.
🔎 Code quality review by PR Code Quality Reviewer · 227.1 AIC · ⌖ 6.68 AIC · ⊞ 5.4K
Comment /review to run again
| | Subpackage | Description | | ||
| |------------|-------------| | ||
| | `appendbytestring` | Custom `go/analysis` analyzer that flags `append(b, []byte(s)...)` calls where `s` is a string that can be simplified to `append(b, s...)` | | ||
| | `bytescomparestring` | Custom `go/analysis` analyzer that flags `string(a) == string(b)` / `!=` comparisons on `[]byte` values that should use `bytes.Equal` | |
There was a problem hiding this comment.
Inaccurate fix guidance for != comparisons: the description says both == and != comparisons "should use bytes.Equal", but != requires !bytes.Equal(a, b) — bytes.Equal alone inverts the semantics.
💡 Suggested correction
Current text:
`bytescomparestring` — reports `string(a) == string(b)` and `string(a) != string(b)` comparisons where `a` and `b` are `[]byte` values that should use `bytes.Equal`.
Suggested:
`bytescomparestring` — reports `string(a) == string(b)` and `string(a) != string(b)` comparisons where `a` and `b` are `[]byte` values; use `bytes.Equal(a, b)` for `==` and `!bytes.Equal(a, b)` for `!=`.
A developer consulting this README and applying the bytes.Equal suggestion 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.
| - Version constants are intentionally plain string literals (not derived from build tags or embedded files) so that individual upgrades can be made as targeted one-line changes. | ||
| - `GetWorkflowDir()` reads `GH_AW_WORKFLOWS_DIR` from the environment at call time, allowing the directory to be overridden in tests and CI. | ||
| - `AgenticEngines` is deprecated in favour of `workflow.NewEngineCatalog(workflow.NewEngineRegistry()).IDs()` but is kept for backward compatibility. | ||
| - Default GitHub tool lists are split into local and remote variants; `DefaultGitHubTools` remains as a deprecated compatibility alias for callers that have not yet switched to the explicit local/remote names. |
There was a problem hiding this comment.
Misleading claim: "split into local and remote variants" implies behavioral divergence, but DefaultGitHubToolsLocal and DefaultGitHubToolsRemote currently both alias DefaultReadOnlyGitHubTools — 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:
- `DefaultGitHubTools` is a deprecated compatibility alias for `DefaultGitHubToolsLocal` (which is itself currently identical to `DefaultGitHubToolsRemote`); callers should switch to the explicit local/remote names to be ready for future divergence.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.
|
@copilot please run the
|
This addresses the spec-audit findings for package README drift:
importinpututilhad an incorrect dependency contract,linterswas missing thebytescomparestringanalyzer, and several package specs were stale relative to current code. The update refreshes those package-level specifications without changing implementation behavior.Critical spec fixes
pkg/importinpututildependencies to includepkg/loggerbytescomparestringto thepkg/lintersoverview, subpackage API table, and dependency listingStale spec refreshes
pkg/syncutilpkg/githubpkg/actionpinspkg/agentdrainpkg/repoutilpkg/semverutilpkg/clipkg/constantsWorkflow spec alignment
pkg/workflow/README.mdto document checkout configuration types and thecheckout.safe-outputs-github-appauth path used only forsafe_outputsgit operationsRepresentative spec correction