Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions .github/workflows/ace-editor.lock.yml

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

4 changes: 2 additions & 2 deletions .github/workflows/blog-auditor.lock.yml

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

4 changes: 2 additions & 2 deletions .github/workflows/cli-consistency-checker.lock.yml

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

4 changes: 2 additions & 2 deletions .github/workflows/cli-version-checker.lock.yml

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

4 changes: 2 additions & 2 deletions .github/workflows/copilot-pr-merged-report.lock.yml

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

4 changes: 2 additions & 2 deletions .github/workflows/daily-team-evolution-insights.lock.yml

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

4 changes: 2 additions & 2 deletions .github/workflows/dev.lock.yml

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

4 changes: 2 additions & 2 deletions .github/workflows/example-permissions-warning.lock.yml

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

4 changes: 2 additions & 2 deletions .github/workflows/gpclean.lock.yml

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

4 changes: 2 additions & 2 deletions .github/workflows/mcp-inspector.lock.yml

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

19 changes: 18 additions & 1 deletion pkg/actionpins/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@ Resolution supports two modes:
| `SHAResolver` | interface | Resolves a SHA for `repo@version` dynamically |
| `ResolutionErrorType` | string | Classifies unresolved action-ref pinning outcomes for auditing |
| `ResolutionFailure` | struct | Captures an unresolved action-ref pinning event (repo, ref, error type) |
| `PinContext` | struct | Runtime context for resolution (resolver, strict mode, warning dedupe map) |
| `PinContext` | struct | Runtime context for resolution (resolver, strict mode, warning dedupe map, action-pin mappings) |

### Functions

Expand Down Expand Up @@ -75,6 +75,23 @@ actionpins.ResolveActionPin("unknown/action", "v1", ctx)
// failures[0].ErrorType == actionpins.ResolutionErrorTypePinNotFound
```

### Action Pin Mappings

`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`.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[/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@ref and proceeds through the normal pin resolution path — the replacement repository must have a corresponding entry in the pin database or be resolvable by the configured SHAResolver.

This prevents confusion when users map to a mirror that has no existing pin.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"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.Warnings is 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.


```go
ctx := &actionpins.PinContext{
Warnings: make(map[string]bool),
Mappings: map[string]string{
"actions/checkout@v4": "acme-corp/checkout@v4",
},
}
reference, err := actionpins.ResolveActionPin("actions/checkout", "v4", ctx)
// reference resolves against acme-corp/checkout@v4 pins
```

### Container Pins

`ContainerPin` provides a pinned image reference for container images:
Expand Down
105 changes: 105 additions & 0 deletions pkg/intent/README.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,105 @@
# intent Package

> Intent attribution and resolution utilities for mapping pull requests and issues to labelled intent records.

## Overview

The `intent` package resolves the intent behind a pull request or issue by inspecting labels, closing issues, and explicit metadata. It produces `IntentRecord` values that classify attribution status and source, allowing downstream consumers to route work items or report on coverage.

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.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

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.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


## Public API

### Types

| Type | Kind | Description |
|------|------|-------------|
| `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 |

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[/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.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

| `RootReference` | struct | Represents a referenced issue or artifact root (node ID, type, URL, labels) |
| `PullRequestData` | struct | Input data for pull request resolution (node ID, URL, labels, explicit intent, closing issues) |
| `Resolver` | struct | Stateless resolver that maps labels to intent records |

### AttributionStatus constants

| Constant | Value | Description |
|----------|-------|-------------|
| `AttributionMapped` | `"mapped"` | Labels matched a known intent category |
| `AttributionUnmapped` | `"unmapped"` | Labels were present but matched no category |
| `AttributionUnlinked` | `"unlinked"` | No supported intent source was found |
| `AttributionAmbiguous` | `"ambiguous"` | Multiple competing sources were found |
| `AttributionSuggested` | `"suggested"` | Attribution was inferred by suggestion (not confirmed) |

### AttributionSource constants

| Constant | Value | Description |
|----------|-------|-------------|
| `SourceExplicitMetadata` | `"explicit_metadata"` | Explicitly provided intent metadata |
| `SourceClosingIssue` | `"closing_issue"` | Derived from a closing issue |
| `SourceParentIssue` | `"parent_issue"` | Derived from a parent issue |
| `SourceReferencedIssue` | `"referenced_issue"` | Derived from a referenced issue |
| `SourceProject` | `"project"` | Derived from a project assignment |
| `SourceMilestone` | `"milestone"` | Derived from a milestone assignment |
| `SourceIssueLabels` | `"issue_labels"` | Derived from labels on an issue |
| `SourceArtifactLabels` | `"artifact_labels"` | Derived from labels on the artifact (pull request) |
| `SourceSuggestion` | `"suggestion"` | Derived from a suggestion |
| `SourceNone` | `"none"` | No source was used |

### Resolver methods

| Method | Signature | Description |
|--------|-----------|-------------|
| `ResolvePullRequest` | `func (r Resolver) ResolvePullRequest(pr PullRequestData) IntentRecord` | Resolves intent for a pull request using explicit intent, closing issues, or PR labels |
| `ResolveIssue` | `func (r Resolver) ResolveIssue(nodeID, url string, labels []string) IntentRecord` | Resolves intent for an issue using its labels |

## Usage Examples

```go
resolver := intent.Resolver{
ResolverVersion: "v1",
MatchLabels: func(labels []string) []string {
for _, l := range labels {
if l == "security" {
return []string{l}
}
}
return nil
},
}

record := resolver.ResolvePullRequest(intent.PullRequestData{
NodeID: "PR_kwDOAAABCD4",
URL: "https://github.com/owner/repo/pull/42",
Labels: []string{"security"},
})

fmt.Println(record.Status) // "mapped"
fmt.Println(record.Source) // "artifact_labels"
```

### Resolving an issue

```go
record := resolver.ResolveIssue(
"I_kwDOAAABCQ4",
"https://github.com/owner/repo/issues/1",
[]string{"security"},
)

fmt.Println(record.Status) // "mapped"
fmt.Println(record.Source) // "issue_labels"
```

## 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.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[/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).


---

*This specification is automatically maintained by the [spec-extractor](../../.github/workflows/spec-extractor.md) workflow.*
Loading
Loading