docs: resolve spec audit — pkg/intent spec, actionpins Mappings field, linters 4 new subpackages#41723
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
…linters 4 new subpackages - Add pkg/intent/README.md specification covering all exported types, constants, and methods - Add pkg/intent/spec_test.go validating the public API - Update pkg/actionpins/README.md: document PinContext.Mappings field and add usage example - Update pkg/linters/README.md: add lenstringsplit, sprintferrorsnew, sprintferrdot, stringreplaceminusone to overview, subpackages table, and dependencies - Update pkg/linters/spec_test.go: add the 4 new analyzer subpackages Closes #41722 Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
|
✅ PR Code Quality Reviewer completed the code quality review. |
|
✅ Test Quality Sentinel completed test quality analysis. |
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. |
There was a problem hiding this comment.
Pull request overview
This PR addresses documentation/spec audit gaps by adding specs and spec-enforcing tests for pkg/intent, documenting the PinContext.Mappings behavior in pkg/actionpins, and updating pkg/linters documentation/tests to include newly added analyzer subpackages. It also updates several compiled workflow .lock.yml files (strict-mode policy enforcement condition/message).
Changes:
- Add
pkg/intentpackage spec (README.md) and spec tests covering constants and resolver behavior. - Document
PinContext.Mappingsinpkg/actionpinsand add the missing analyzer entries topkg/lintersdocs and spec tests. - Recompile/update multiple workflow
.lock.ymlfiles affecting strict-mode policy enforcement gating.
Show a summary per file
| File | Description |
|---|---|
| pkg/linters/spec_test.go | Adds the 4 missing analyzer subpackages to the “documented analyzers” spec test list. |
| pkg/linters/README.md | Documents lenstringsplit, sprintferrdot, sprintferrorsnew, and stringreplaceminusone across the overview/table/dependencies sections. |
| pkg/intent/spec_test.go | Introduces spec tests to enforce pkg/intent’s documented public API and resolution behavior. |
| pkg/intent/README.md | Adds a new public spec for pkg/intent types/constants/methods, including examples and notes. |
| pkg/actionpins/README.md | Documents PinContext.Mappings and adds a dedicated “Action Pin Mappings” usage section. |
| .github/workflows/mcp-inspector.lock.yml | Updates compiled workflow strict-mode policy enforcement condition/message. |
| .github/workflows/gpclean.lock.yml | Updates compiled workflow strict-mode policy enforcement condition/message. |
| .github/workflows/example-permissions-warning.lock.yml | Updates compiled workflow strict-mode policy enforcement condition/message. |
| .github/workflows/dev.lock.yml | Updates compiled workflow strict-mode policy enforcement condition/message. |
| .github/workflows/daily-team-evolution-insights.lock.yml | Updates compiled workflow strict-mode policy enforcement condition/message. |
| .github/workflows/copilot-pr-merged-report.lock.yml | Updates compiled workflow strict-mode policy enforcement condition/message. |
| .github/workflows/cli-version-checker.lock.yml | Updates compiled workflow strict-mode policy enforcement condition/message. |
| .github/workflows/cli-consistency-checker.lock.yml | Updates compiled workflow strict-mode policy enforcement condition/message. |
| .github/workflows/blog-auditor.lock.yml | Updates compiled workflow strict-mode policy enforcement condition/message. |
| .github/workflows/ace-editor.lock.yml | Updates compiled workflow strict-mode policy enforcement condition/message. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 15/15 changed files
- Comments generated: 4
- Review effort level: Low
|
|
||
| Resolution is performed by a `Resolver`, which holds a label matcher function and a version string. The resolver applies a priority chain: | ||
|
|
||
| 1. Explicit intent metadata (`PullRequestData.ExplicitIntent`) — used as-is. |
|
|
||
| ## Thread Safety | ||
|
|
||
| `Resolver` holds no mutable state and is safe for concurrent use. `IntentRecord` values are returned by value and do not share mutable state with the caller. |
| - `sprintferrorsnew` — reports `errors.New(fmt.Sprintf(...))` calls that should use `fmt.Errorf` instead. | ||
| - `ssljson` — validates `ssl.json` skill artifacts found in `.github/skills/` against the SSL spec (enum membership, graph integrity, transition targets, entry pointer validity). | ||
| - `strconvparseignorederror` — reports `strconv` parsing calls (`Atoi`, `ParseInt`, etc.) where the error return is discarded with `_`. | ||
| - `stringreplaceminusone` — reports `strings.Replace` calls whose `n` argument is `-1`, which should use the more readable `strings.ReplaceAll`. |
| - name: Enforce strict mode policy | ||
| if: ${{ contains(toJSON(vars), '"GH_AW_POLICY_STRICT":') }} | ||
| if: ${{ vars.GH_AW_POLICY_STRICT == 'true' }} | ||
| run: | | ||
| echo "::error::GH_AW_POLICY_STRICT is set but this workflow was not compiled in strict mode. Recompile with --strict or strict: true." | ||
| echo "::error::GH_AW_POLICY_STRICT=true but this workflow was not compiled in strict mode. Recompile with --strict or strict: true." |
🏗️ Design Decision Gate — ADR RequiredThis PR makes significant changes to core business logic (353 new lines in 📄 Draft ADR committed:
📋 What to do next
Once an ADR is linked in the PR body, this gate will re-run and verify the implementation matches the decision. ❓ Why ADRs MatterADRs create a searchable, permanent record of why the codebase looks the way it does. Future contributors (and your future self) will thank you. 📋 Michael Nygard ADR Format ReferenceAn ADR must contain these four sections to be considered complete:
All ADRs are stored in
|
🧪 Test Quality Sentinel Report
📊 Metrics & Test Classification (10 tests analyzed)
Go: 10 ( Verdict
|
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /tdd and /grill-with-docs — approving with suggestions on test coverage and documentation completeness. No blocking issues found.
📋 Key Themes & Highlights
Key Themes
/tdd: Three spec-test gaps inpkg/intent/spec_test.go: swappedassert.Equalarg order, missingRuleassertion in the explicit-intent path, and no coverage forAttributionUnmapped(labels present but unmatched)./grill-with-docs:IntentRecordfields are not enumerated in the README — callers need that field table. The thread-safety claim needs aMatchLabels-goroutine-safety qualifier. TheactionpinsMappings section leaves the resolved-value lifecycle ambiguous.
Positive Highlights
- ✅ Comprehensive
pkg/intent/README.md— types, 15 constants, resolution priority chain, usage examples, and thread-safety note all in one place. - ✅
spec_test.gopattern is well-structured: tests derive explicitly from the README and use meaningful assertion messages. - ✅ 4 new linter subpackages added consistently across overview, table, dependencies, and
spec_test.goin alphabetical order. - ✅
pkg/actionpinsMappings section provides a concrete, runnable code example. - ✅ Lock-file strict-mode fix (
contains(toJSON(vars))→vars.GH_AW_POLICY_STRICT == 'true') simplifies the condition and aligns with explicit boolean semantics.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · 104.3 AIC · ⌖ 9.9 AIC · ⊞ 6.5K
| // AttributionMapped="mapped", AttributionUnmapped="unmapped", AttributionUnlinked="unlinked", | ||
| // AttributionAmbiguous="ambiguous", AttributionSuggested="suggested" | ||
| func TestSpec_PublicAPI_AttributionStatusConstants(t *testing.T) { | ||
| assert.Equal(t, intent.AttributionMapped, intent.AttributionStatus("mapped"), |
There was a problem hiding this comment.
[/tdd] Assert args are swapped here: the constant is in the expected position and the string literal is in the actual position — opposite of testify convention (assert.Equal(t, expected, actual)). If a constant value drifts, the failure reads "expected: AttributionMapped, got: mapped" instead of the clearer "expected: 'mapped', got: (new_value)".
💡 Suggested fix
The sibling test TestSpec_PublicAPI_AttributionSourceConstants uses the correct order (assert.Equal(t, c.expected, c.got)). Apply the same convention:
assert.Equal(t, intent.AttributionStatus("mapped"), intent.AttributionMapped,
"AttributionMapped should equal \"mapped\" per the spec")Apply the same swap to all 5 constant assertions in this function.
| assert.Equal(t, intent.SourceExplicitMetadata, record.Source, | ||
| "ResolvePullRequest should honour explicit intent source") | ||
| assert.Equal(t, "v1", record.ResolverVersion, | ||
| "ResolvePullRequest should stamp ResolverVersion when absent from explicit intent") |
There was a problem hiding this comment.
[/tdd] The test sets Rule: "explicit" in the input but never asserts it's preserved in the output. The spec says explicit intent is "used as-is" — a missing assertion lets a regression silently slip through if the resolver ever clears or overwrites Rule.
💡 Suggested addition
Add one more assertion after line 81:
assert.Equal(t, "explicit", record.Rule,
"ResolvePullRequest should preserve Rule from explicit intent")This makes the "used as-is" spec claim fully verifiable.
| assert.Equal(t, intent.SourceNone, record.Source, | ||
| "no sources should produce none source") | ||
| assert.Equal(t, "no_supported_intent_source", record.Rule, | ||
| "no sources should produce no_supported_intent_source rule") |
There was a problem hiding this comment.
[/tdd] The AttributionUnmapped status ("Labels were present but matched no category") documented in the README has no test coverage. The path is exercised when MatchLabels returns an empty slice for a non-empty label set — a realistic scenario for any production Resolver.
💡 Suggested test to add
// TestSpec_PublicAPI_ResolvePullRequest_UnmappedLabels validates that PR labels
// that match no category produce an unmapped attribution.
// Spec: "Labels were present but matched no category → unmapped."
func TestSpec_PublicAPI_ResolvePullRequest_UnmappedLabels(t *testing.T) {
resolver := intent.Resolver{
MatchLabels: func(labels []string) []string { return nil }, // no match
}
record := resolver.ResolvePullRequest(intent.PullRequestData{
Labels: []string{"unknown-label"},
})
assert.Equal(t, intent.AttributionUnmapped, record.Status,
"labels that match nothing should produce unmapped status")
assert.Equal(t, intent.SourceArtifactLabels, record.Source,
"unmapped PR labels should still report artifact_labels source")
}| |------|------|-------------| | ||
| | `AttributionStatus` | string | Classifies the outcome of intent attribution | | ||
| | `AttributionSource` | string | Identifies the data source used for attribution | | ||
| | `IntentRecord` | struct | Holds the attribution result for a pull request or issue | |
There was a problem hiding this comment.
[/grill-with-docs] IntentRecord is described only as "Holds the attribution result" — its exported fields (Status, Source, RootNodeID, RootType, RootURL, Labels, Rule, ResolverVersion) aren't listed anywhere in the spec. The tests and usage example access several of these fields, implying callers need them, but a reader of the README alone wouldn't know what data to read.
💡 Suggested addition
Add a ### IntentRecord fields subsection (or expand the types table) documenting the fields:
| Field | Type | Description |
|---|---|---|
Status |
AttributionStatus |
Classification outcome |
Source |
AttributionSource |
Data source used |
Rule |
string |
Internal rule identifier for the match path |
RootNodeID |
string |
Node ID of the resolved root (issue/PR) |
RootType |
string |
Type of the resolved root ("issue", "artifact") |
RootURL |
string |
URL of the resolved root |
Labels |
[]string |
Labels that drove the attribution |
ResolverVersion |
string |
Version of the resolver that produced this record |
This is especially important since spec_test.go asserts on record.Rule, record.RootType, and record.ResolverVersion — those fields are part of the public contract.
|
|
||
| ## Thread Safety | ||
|
|
||
| `Resolver` holds no mutable state and is safe for concurrent use. `IntentRecord` values are returned by value and do not share mutable state with the caller. |
There was a problem hiding this comment.
[/grill-with-docs] The thread-safety claim "Resolver holds no mutable state and is safe for concurrent use" is only half the story. If MatchLabels closes over mutable state (e.g., a cache or counter), concurrent calls to ResolvePullRequest can race. The guarantee should be qualified.
💡 Suggested wording
`Resolver` holds no mutable state of its own and is safe for concurrent use,
provided the supplied `MatchLabels` function is goroutine-safe.
`IntentRecord` values are returned by value and do not share mutable state with the caller.This mirrors how the Go standard library phrases similar caveats (e.g., http.Transport).
|
|
||
| `PinContext.Mappings` redirects `owner/repo@ref` references to replacement references before pin resolution. This is used to substitute private or mirror repositories for well-known public actions (set from `aw.json` `action_pins`). | ||
|
|
||
| Keys and values use the format `"owner/repo@ref"`. When a key matches the incoming `actionRepo@version`, resolution proceeds against the mapped value instead. An informational message is emitted once per mapping via `PinContext.Warnings`. |
There was a problem hiding this comment.
[/grill-with-docs] The section says keys and values use "owner/repo@ref" format, but it's unclear whether the value undergoes full SHA pin resolution. The comment // reference resolves against acme-corp/checkout@v4 pins implies pinning still happens, but a reader may wonder: does the mapped target need a pre-existing pin entry, or does the resolver look it up dynamically?
💡 Suggested clarification
Add a sentence clarifying the lifecycle, e.g.:
The mapped value is treated as a new
owner/repo@refand proceeds through the normal pin resolution path — the replacement repository must have a corresponding entry in the pin database or be resolvable by the configuredSHAResolver.
This prevents confusion when users map to a mirror that has no existing pin.
There was a problem hiding this comment.
REQUEST_CHANGES — three spec accuracy issues in plus minor gaps in the spec test and actionpins doc.
Blocking issues (must fix before merge)
1. "used as-is" contradicts the implementation ()
The priority chain describes explicit intent as "used as-is", but the resolver stamps when the field is absent. The spec test comment even cites the spec and then asserts the stamping — evidence that the README description is wrong, not the implementation.
2. Priority chain omits the multiple-closing-issues → ambiguous path ()
The switch in ResolvePullRequest has a default branch that returns AttributionAmbiguous for two or more closing issues. The four-item priority chain never mentions this. Any consumer who reads only the spec and then encounters a PR with two closing issues will be surprised.
3. IntentRecord struct fields undocumented, but spec tests enforce specific Rule string values ()
The types table lists IntentRecord with a one-liner description but documents none of its fields. The spec tests then assert five specific Rule values that appear in the serialized JSON output. These are contractual strings — either document them (and the other output fields) or don't test them in a spec test.
Non-blocking issues (fix recommended)
4. Missing Source assertion in ResolveIssue_NoLabelsUnlinked ()
The parallel ResolvePullRequest_NoSources test asserts Status, Source, and Rule. This test only asserts two of the three, leaving Source = SourceNone untested for ResolveIssue.
5. "via PinContext.Warnings" implies Warnings is the output channel ()
Notifications go to stderr directly; Warnings is only the dedup map. The current wording misleads readers into expecting the notifications to be accessible via the map.
🔎 Code quality review by PR Code Quality Reviewer · 176.3 AIC · ⌖ 11 AIC · ⊞ 5.2K
|
|
||
| Resolution is performed by a `Resolver`, which holds a label matcher function and a version string. The resolver applies a priority chain: | ||
|
|
||
| 1. Explicit intent metadata (`PullRequestData.ExplicitIntent`) — used as-is. |
There was a problem hiding this comment.
Inaccurate spec — "used as-is" does not match the implementation: the resolver modifies the explicit intent record by stamping ResolverVersion when it is absent, so it is not truly used as-is.
💡 Suggested fix
The implementation in resolver.go does this:
if pr.ExplicitIntent != nil {
intent := *pr.ExplicitIntent
if intent.ResolverVersion == "" {
intent.ResolverVersion = r.ResolverVersion
}
return intent
}The spec test even contradicts this README line — its doc comment says "Spec: ... used as-is" but then asserts record.ResolverVersion == "v1" to confirm the stamping. Fix the README:
1. Explicit intent metadata (`PullRequestData.ExplicitIntent`) — returned directly, with `ResolverVersion` stamped from the resolver when the field is absent.
Otherwise downstream implementers may incorrectly assume the record passes through without mutation.
| 1. Explicit intent metadata (`PullRequestData.ExplicitIntent`) — used as-is. | ||
| 2. A single closing issue — resolved from the issue's labels. | ||
| 3. PR labels — used as an artifact fallback when no closing issues are present. | ||
| 4. No supported sources — returns an `AttributionUnlinked` record. |
There was a problem hiding this comment.
Priority chain is incomplete — the multiple-closing-issues → ambiguous path is undocumented: the chain lists 4 outcomes but the implementation handles a 5th case (two or more closing issues) that produces AttributionAmbiguous, which is entirely absent here.
💡 Suggested fix
Add the missing case between items 2 and 3:
1. Explicit intent metadata (`PullRequestData.ExplicitIntent`) — returned with `ResolverVersion` stamped when absent.
2. A single closing issue — resolved from the issue's labels.
3. Multiple closing issues — returns an `AttributionAmbiguous` record (`SourceClosingIssue`, rule `multiple_closing_issues`).
4. PR labels — used as an artifact fallback when no closing issues are present.
5. No supported sources — returns an `AttributionUnlinked` record.
A reader building on this package who only reads the priority chain will be blindsided by AttributionAmbiguous appearing in outputs for PRs with two or more closing issues.
| |------|------|-------------| | ||
| | `AttributionStatus` | string | Classifies the outcome of intent attribution | | ||
| | `AttributionSource` | string | Identifies the data source used for attribution | | ||
| | `IntentRecord` | struct | Holds the attribution result for a pull request or issue | |
There was a problem hiding this comment.
IntentRecord struct fields are absent from the spec, yet the tests enforce specific field values: the types table lists IntentRecord but documents none of its fields — not Rule, ResolverVersion, RootType, RootURL, Labels, or RootNodeID. The spec tests then assert specific Rule string values ("single_closing_issue", "pull_request_label_fallback", "issue_label_fallback", "no_supported_intent_source", "multiple_closing_issues") that consumers may rely on (the field is in the JSON output), but those values have no documented contract to break against.
💡 Suggested fix
Add a ### IntentRecord fields section (or expand the types table) documenting the exported fields and their semantics:
| Field | Type | Description |
|---|---|---|
Status |
AttributionStatus |
Attribution outcome |
Source |
AttributionSource |
Data source used for attribution |
Rule |
string |
Internal rule identifier that produced this record (e.g. "single_closing_issue", "pull_request_label_fallback") |
ResolverVersion |
string |
Version of the resolver that produced this record |
RootNodeID |
string |
GraphQL node ID of the root reference, if applicable |
RootType |
string |
Type of the root reference ("issue" or "artifact") |
RootURL |
string |
URL of the root reference |
Labels |
[]string |
Labels that drove the attribution |
Without this, the spec tests are enforcing undocumented behaviour, and the "rule" JSON field is serialized into downstream storage with no contract.
| assert.Equal(t, intent.AttributionUnlinked, record.Status, | ||
| "issue with no labels should produce unlinked status") | ||
| assert.Equal(t, "no_supported_intent_source", record.Rule, | ||
| "issue with no labels should produce no_supported_intent_source rule") |
There was a problem hiding this comment.
Source field not asserted in ResolveIssue_NoLabelsUnlinked, inconsistent with the parallel ResolvePullRequest_NoSources test: the no-labels path calls the same unlinked() helper which sets Source = SourceNone, but this test only checks Status and Rule, leaving the source contract untested.
💡 Suggested fix
Add the missing assertion:
assert.Equal(t, intent.SourceNone, record.Source,
"issue with no labels should produce none source")ResolvePullRequest_NoSources asserts all three fields (Status, Source, Rule) for the same unlinked() return path. The omission here creates an asymmetry in spec coverage that makes it easier to accidentally change ResolveIssue's zero-label source without a test catching it.
|
|
||
| `PinContext.Mappings` redirects `owner/repo@ref` references to replacement references before pin resolution. This is used to substitute private or mirror repositories for well-known public actions (set from `aw.json` `action_pins`). | ||
|
|
||
| Keys and values use the format `"owner/repo@ref"`. When a key matches the incoming `actionRepo@version`, resolution proceeds against the mapped value instead. An informational message is emitted once per mapping via `PinContext.Warnings`. |
There was a problem hiding this comment.
"via PinContext.Warnings" is misleading about how the notification is delivered: the phrase implies PinContext.Warnings is the output channel, but the implementation writes directly to stderr via fmt.Fprintln(os.Stderr, ...). PinContext.Warnings is only a deduplication map (to avoid printing the same message twice).
💡 Suggested fix
Replace:
An informational message is emitted once per mapping via
PinContext.Warnings.
With:
An informational message is written to stderr once per unique mapping;
PinContext.Warningsis used as a deduplication map to suppress repeats.
As written, a reader may expect that querying ctx.Warnings after resolution will expose the mapping notifications — it wont.
| - `httpnoctx` — reports HTTP client and package-level HTTP calls that do not accept a `context.Context`. | ||
| - `jsonmarshalignoredeerror` — reports `json.Marshal` and `json.Unmarshal` calls where the error return is discarded. | ||
| - `largefunc` — reports function bodies that exceed a configurable line-count threshold. | ||
| - `lenstringsplit` — reports `len(strings.Split(s, sep))` expressions with a non-empty separator that should use `strings.Count(s, sep)+1` to avoid an intermediate slice allocation. |
There was a problem hiding this comment.
lenstringsplit description omits "provably" — misrepresents when the analyzer fires: the README says "non-empty separator" but the analyzer only fires on statically-evaluable constant separators (it calls constant.StringVal(tv.Value) to check the value at analysis time). Variable separators that are always non-empty at runtime are not flagged.
💡 Suggested fix
Both the overview list and the subpackages table should say "provably non-empty separator" (matching the analyzer's own Doc string):
`lenstringsplit` — reports `len(strings.Split(s, sep))` expressions with a **provably** non-empty separator that should use `strings.Count(s, sep)+1` to avoid an intermediate slice allocation.
Without "provably", users will file false-negative bug reports when the analyzer doesn't fire on variable separators, and they'll misunderstand the analyzer's scope.
Addresses 3 issues from the 2026-06-26 spec-librarian audit: missing spec for⚠️ ), and 4 new analyzer subpackages absent from ⚠️ ).
pkg/intent(❌ critical), undocumentedPinContext.Mappingsinpkg/actionpins(pkg/lintersdocs (pkg/intent— new README.md + spec_test.goPackage had no specification at all. Added:
README.mddocumenting all exported types (AttributionStatus,AttributionSource,IntentRecord,RootReference,PullRequestData,Resolver), 15 constants with string values, both public methods, resolution priority chain, and thread-safety notespec_test.gowith 8 tests covering constant values and all resolution pathspkg/actionpins— documentPinContext.MappingsThe
Mappings map[string]stringfield added toPinContextforaw.jsonaction-pin redirects was missing from the spec. Updated the type description and added a dedicated usage section:pkg/linters— add 4 new analyzer subpackageslenstringsplit,sprintferrorsnew,sprintferrdot, andstringreplaceminusonewere absent from the README overview, subpackages table, and dependencies section. Updated all three locations and added the 4 analyzers tospec_test.go'sdocumentedAnalyzers()(count: 31 → 35).