Skip to content
Open
Show file tree
Hide file tree
Changes from 2 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
2 changes: 2 additions & 0 deletions pkg/actionpins/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -92,6 +92,8 @@ reference, err := actionpins.ResolveActionPin("actions/checkout", "v4", ctx)
// 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.

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


### Container Pins

`ContainerPin` provides a pinned image reference for container images:
Expand Down
2 changes: 2 additions & 0 deletions pkg/agentdrain/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,8 @@

The `agentdrain` package implements the [Drain](https://jiemingzhu.github.io/pub/pjhe_icws2017.pdf) log template mining algorithm adapted for analyzing structured agent pipeline events. It is used for anomaly detection in agentic workflow runs.

The package also embeds default trained weights under `data/`, so callers can bootstrap a `Coordinator` with pre-trained clusters instead of starting from an empty model.

## Overview

Drain is an online log parsing algorithm that groups log lines into clusters based on token similarity. Each cluster has a *template* β€” a tokenized log pattern where variable tokens are replaced with a wildcard (`<*>`). When a new log line arrives, Drain finds the most similar existing cluster or creates a new one.
Expand Down
2 changes: 2 additions & 0 deletions pkg/cli/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,8 @@ The `cli` package implements all commands exposed through the `gh aw` CLI extens

The package is intentionally decomposed into many small files grouped by feature domain (e.g., `compile_*.go`, `audit_*.go`, `run_*.go`, `mcp_*.go`). This structure keeps individual files under 300 lines and promotes independent testing of each sub-domain.

Besides Cobra entry points, the package also exposes reusable helpers for workflow resolution, dependency analysis, PR creation, and run auditing so multiple commands can share the same business logic and tests.

All diagnostic output MUST go to `stderr` using `console` formatting helpers. Structured output (JSON, hashes, graphs) goes to `stdout`.

## Command Groups
Expand Down
1 change: 1 addition & 0 deletions pkg/constants/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -544,6 +544,7 @@ dir := constants.GetWorkflowDir() // ".github/workflows"
- 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.

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.

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.


---

Expand Down
2 changes: 1 addition & 1 deletion pkg/github/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -112,7 +112,7 @@ fmt.Println(defaults) // ObjectiveMapping{labels: 12, logic: max, priorities: 7}

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

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

- Debug output is controlled by the `DEBUG=github:*` environment variable and is only emitted when that variable is set.

---
Expand Down
6 changes: 5 additions & 1 deletion pkg/importinpututil/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -86,7 +86,11 @@ if !ok {

## Dependencies

This package has no external or internal `gh-aw` dependencies β€” it uses only the Go standard library.
**Internal**:
- `github.com/github/gh-aw/pkg/logger` β€” package-scoped debug logging for path resolution and value formatting

**External**:
- None beyond the Go standard library (`encoding/json`, `fmt`, `reflect`, `sort`, `strings`).

## Design Notes

Expand Down
3 changes: 3 additions & 0 deletions pkg/linters/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ The `linters` package namespace contains custom static analysis linters used by
This package currently provides custom Go analyzers in the following subpackages:

- `appendbytestring` β€” reports `append(b, []byte(s)...)` calls where `b` is `[]byte` and `s` is a string, which can be simplified to `append(b, s...)`.
- `bytescomparestring` β€” reports `string(a) == string(b)` and `string(a) != string(b)` comparisons where `a` and `b` are `[]byte` values that should use `bytes.Equal`.
- `contextcancelnotdeferred` β€” reports context cancel functions that are called directly instead of deferred.
- `ctxbackground` β€” reports `context.Background()` calls inside functions that already receive a `context.Context` parameter.
- `deferinloop` β€” reports `defer` statements placed directly inside `for`/`range` loop bodies, which execute when the enclosing function returns rather than each iteration and can cause resource leaks.
Expand Down Expand Up @@ -58,6 +59,7 @@ This package currently provides custom Go analyzers in the following subpackages
| 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` |

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

| `contextcancelnotdeferred` | Custom `go/analysis` analyzer that flags context cancel functions called directly instead of deferred |
| `ctxbackground` | Custom `go/analysis` analyzer that flags `context.Background()` calls inside functions that already receive a context parameter |
| `deferinloop` | Custom `go/analysis` analyzer that flags `defer` statements inside `for`/`range` loop bodies that execute when the enclosing function returns rather than each iteration |
Expand Down Expand Up @@ -169,6 +171,7 @@ _ = timesleepnocontext.Analyzer

**Internal**:
- `github.com/github/gh-aw/pkg/linters/appendbytestring` β€” append-byte-string analyzer subpackage
- `github.com/github/gh-aw/pkg/linters/bytescomparestring` β€” bytes-compare-string analyzer subpackage
- `github.com/github/gh-aw/pkg/linters/contextcancelnotdeferred` β€” context-cancel-not-deferred analyzer subpackage
- `github.com/github/gh-aw/pkg/linters/ctxbackground` β€” context-background analyzer subpackage
- `github.com/github/gh-aw/pkg/linters/deferinloop` β€” defer-in-loop analyzer subpackage
Expand Down
1 change: 1 addition & 0 deletions pkg/repoutil/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,7 @@ if err != nil {

- All debug output uses `logger.New("repoutil:repoutil")` and is only emitted when `DEBUG=repoutil:*`.
- For paths that include sub-folders (e.g. GitHub Actions `uses:` fields such as `github/codeql-action/upload-sarif`), use `gitutil.ExtractBaseRepo` first to strip the sub-path before calling `SplitRepoSlug`.
- `NormalizeRepoForAPI` only treats three-segment strings as `HOST/owner/repo`; plain `owner/repo` values are returned unchanged with an empty host.

---

Expand Down
1 change: 1 addition & 0 deletions pkg/semverutil/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -143,6 +143,7 @@ semverutil.IsCompatible("v6.0.0", "v5") // false
- All debug output uses `logger.New("semverutil:semverutil")` and is only emitted when `DEBUG=semverutil:*`.
- The package intentionally delegates to `golang.org/x/mod/semver` for canonical semver logic rather than implementing its own parsing.
- `ParseVersion` uses `semver.Canonical` before splitting into components, ensuring correct handling of short forms like `v1` (canonicalized to `v1.0.0`).
- `IsCompatible` returns `false` for invalid versions on either side before comparing majors, preventing two malformed version strings from being treated as compatible.

---

Expand Down
1 change: 1 addition & 0 deletions pkg/syncutil/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -54,6 +54,7 @@ func getCurrentRepoSlug() (string, error) {

- The internal mutex ensures that `loader` is invoked at most once, even when multiple goroutines call `Get` concurrently.
- If `loader` returns an error, the error is cached alongside the zero value of `T`; subsequent calls return the same error without re-invoking `loader`.
- `Override` marks the loader as complete and caches both the supplied result and the supplied error until `Reset` is called.
- `Reset` acquires the same mutex, making it safe to call concurrently with `Get`.
- The zero value of `OnceLoader[T]` is ready to use; no constructor is needed.

Expand Down
10 changes: 10 additions & 0 deletions pkg/workflow/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -112,6 +112,8 @@ The package is intentionally large (~320 source files) because it encodes all Gi
| `PermissionsConfig` | struct | GitHub Actions permissions (shorthand + detailed fields) |
| `GitHubActionsPermissionsConfig` | struct | Detailed permissions with all scope fields |
| `GitHubAppPermissionsConfig` | struct | GitHub App permission scopes |
| `CheckoutConfig` | struct | Parsed `checkout:` entry controlling repository/ref/path/auth/fetch behavior for generated checkout steps |
| `CheckoutManager` | struct | Merges and resolves one or more `checkout:` entries into deterministic checkout-step plans |
| `ObservabilityConfig` | struct | OTLP/observability configuration |
| `RateLimitConfig` | struct | Rate limit settings |
| `OTLPConfig` | struct | OpenTelemetry protocol configuration |
Expand All @@ -120,6 +122,14 @@ The package is intentionally large (~320 source files) because it encodes all Gi
| `NetworkPermissions` | struct | Parsed `network:` frontmatter block; controls allowed/blocked domain lists |
| `EngineNetworkConfig` | struct | Combines `*EngineConfig` and `*NetworkPermissions` for engine helpers that need both |

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

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


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

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] "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.


#### Engine Configuration Fields

`EngineConfig` is populated by `ExtractEngineConfig` from the `engine:` frontmatter key. It is stored on `EngineNetworkConfig.Engine` and forwarded to each engine's `GetExecutionSteps` / `GetInstallationSteps` implementations.
Expand Down
Loading