Conversation
…DMEs Agent-Logs-Url: https://github.com/github/gh-aw/sessions/b1746852-7337-4d93-b7eb-29f54fb19c3e Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Adds missing package-level specifications and expands existing docs so utility packages and key UI/logging helpers have complete, searchable README coverage.
Changes:
- Added new
README.mdspecifications for multiplepkg/*utility packages (type conversions, semver, strings, slices, file/env helpers, etc.). - Documented
agentdrain’s Drain-based mining/anomaly detection components and persistence/weights workflow. - Expanded existing docs for
console(full formatting/rendering + accessibility API surface) andlogger(slog adapter bridge).
Show a summary per file
| File | Description |
|---|---|
| pkg/agentdrain/README.md | New spec describing Drain algorithm usage, types, and coordinator/miner APIs. |
| pkg/constants/README.md | New spec enumerating semantic types/constants, version pins, runtime config, and tool lists. |
| pkg/console/README.md | Expanded spec for message formatting, rendering helpers, interactive prompts, and accessibility. |
| pkg/envutil/README.md | New spec for bounded integer env var parsing helper. |
| pkg/fileutil/README.md | New spec for path validation, boundary enforcement, copy, and safe tar extraction. |
| pkg/gitutil/README.md | New spec for git helpers and GitHub error classification utilities. |
| pkg/logger/README.md | Expanded spec documenting slog integration and adapter behavior. |
| pkg/repoutil/README.md | New spec for parsing owner/repo repository slugs. |
| pkg/semverutil/README.md | New spec for semver parsing/normalization/comparison helpers. |
| pkg/sliceutil/README.md | New spec for generic slice/map helpers like Filter/Map/Any/Deduplicate. |
| pkg/stringutil/README.md | New spec covering ANSI stripping, normalization, sanitization, URL helpers, and PAT validation. |
| pkg/styles/README.md | New spec describing palette, borders, preconfigured styles, and Huh theme. |
| pkg/testutil/README.md | New spec for test temp dirs, stderr capture, and YAML header stripping. |
| pkg/timeutil/README.md | New spec for human-readable duration formatting helpers. |
| pkg/tty/README.md | New spec for terminal detection helpers including WASM behavior. |
| pkg/types/README.md | New spec documenting shared config/types used across packages. |
| pkg/typeutil/README.md | New spec describing strict/lenient conversions and overflow-safe helpers. |
Copilot's findings
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 17/17 changed files
- Comments generated: 6
| // Step IDs | ||
| constants.CheckMembershipStepID // "check_membership" | ||
| constants.CheckRateLimitStepID // "check_rate_limit" | ||
| constants.CheckMembershipStepID // "check_membership" |
There was a problem hiding this comment.
The Step ID example list includes constants.CheckMembershipStepID twice; this looks like an accidental duplicate and makes the snippet harder to follow. Remove the duplicate or replace it with the intended additional StepID constant.
| constants.CheckMembershipStepID // "check_membership" |
|
|
||
| ## Design Notes | ||
|
|
||
| - All semantic types implement `String()` and `IsValid()` to allow consistent validation across the codebase. |
There was a problem hiding this comment.
The Design Notes claim that all semantic types implement String() and IsValid(), but several types in pkg/constants (e.g. EngineName, FeatureFlag) do not define these methods. Either add the methods for the remaining semantic types, or adjust this statement to reflect the current implementation.
| - All semantic types implement `String()` and `IsValid()` to allow consistent validation across the codebase. | |
| - Some semantic types implement `String()` and `IsValid()` helpers where needed, but this is not currently guaranteed for every type in `pkg/constants`. |
|
|
||
| #### `IsRateLimitError(errMsg string) bool` | ||
|
|
||
| Returns `true` when `errMsg` indicates a GitHub API rate-limit error (HTTP 403 "API rate limit exceeded" or HTTP 429). |
There was a problem hiding this comment.
IsRateLimitError is documented as detecting HTTP 429, but the implementation currently only does substring matching for "rate limit exceeded" / "secondary rate limit" and does not explicitly match "429". Either update the docs to describe the actual matching, or extend the function to recognize common 429 error strings.
| Returns `true` when `errMsg` indicates a GitHub API rate-limit error (HTTP 403 "API rate limit exceeded" or HTTP 429). | |
| Returns `true` when `errMsg` case-insensitively contains GitHub API rate-limit phrases such as `API rate limit exceeded` or `secondary rate limit`. |
| ## Design Notes | ||
|
|
||
| - This package has no dependencies on other `gh-aw` packages, making it safe to import from anywhere. | ||
| - All struct fields use both `json` and `yaml` struct tags so they can be round-tripped through both serialization formats. |
There was a problem hiding this comment.
The Design Notes say all struct fields have both json and yaml tags, but TokenWeights/TokenClassWeights currently only have json tags (see pkg/types/token_weights.go). Update this note (or add the missing yaml tags in code if round-tripping through YAML is required).
| - All struct fields use both `json` and `yaml` struct tags so they can be round-tripped through both serialization formats. | |
| - Shared configuration structs commonly use both `json` and `yaml` struct tags when they need to be round-tripped through both serialization formats, but this is not a universal requirement for every type in this package. |
| Prints multiple rendered sections to `os.Stderr`, separated by blank lines. | ||
|
|
||
| ```go | ||
| lines := append( | ||
| console.RenderTitleBox("Audit Report", 60), |
There was a problem hiding this comment.
RenderComposedSections does not itself insert blank lines between sections; callers currently add empty-string elements to the sections slice when they want spacing. Consider rewording this line to avoid implying automatic separation (or change the helper to insert spacing explicitly).
| Prints multiple rendered sections to `os.Stderr`, separated by blank lines. | |
| ```go | |
| lines := append( | |
| console.RenderTitleBox("Audit Report", 60), | |
| Prints the provided rendered lines to `os.Stderr` in order. To add blank lines between sections, include empty-string elements in `sections`. | |
| ```go | |
| lines := append( | |
| append(console.RenderTitleBox("Audit Report", 60), ""), |
| } | ||
| ``` | ||
|
|
||
| > **Note**: `ConfirmAction` is only available in non-WASM builds. In WASM environments the function is unavailable. |
There was a problem hiding this comment.
The note about ConfirmAction being unavailable in WASM builds is inaccurate: there is a WASM implementation (confirm_wasm.go) that exists but returns an error ("interactive confirmation not available in Wasm"). Update the note to reflect that behavior so callers know they can compile but must handle the error in WASM.
| > **Note**: `ConfirmAction` is only available in non-WASM builds. In WASM environments the function is unavailable. | |
| > **Note**: `ConfirmAction` is available in WASM builds, but interactive confirmation is not supported there. In WASM environments, the function returns an error (for example, `"interactive confirmation not available in Wasm"`), so callers should handle that error. |
The spec audit identified 18 packages with no README and 2 with incomplete specs. This PR addresses all utility packages (15 new READMEs) and both incomplete specs.
New READMEs
agentdrainconstantsEngineName,FeatureFlag,JobName,StepID, etc.), version pins, tool listsstringutilstylesHuhThemefileutilMustBeWithinsymlink-safe boundary checks, tar extraction with security notessemverutilSemanticVersiontype, comparison/compatibility helpers,IsPreciseVersion/IsNewermethodstypeutilParseIntValue,ConvertToInt,SafeUint64ToIntsliceutilFilter,Map,MapToSlice,FilterMapKeys,Any,DeduplicatetypesBaseMCPServerConfigfield table,MCPAuthConfig,TokenWeights/TokenClassWeightstestutilTempDir,CaptureStderr,StripYAMLCommentHeader, isolation rationaletimeutilFormatDuration,FormatDurationMs,FormatDurationNsenvutilGetIntFromEnvbounds validation and fallback behaviorgitutilFindGitRoot,ReadFileFromHEADWithRootpath-traversal protectionsrepoutilSplitRepoSlugwith all error casesttyIsStdoutTerminal/IsStderrTerminal, WASM stub behaviorUpdated READMEs
logger/README.md— addsSlogHandlertype andNewSlogHandler/NewSlogLoggerWithHandlerconstructors with full behavior notes (WithAttrs/WithGroupare intentionally no-ops, enabled-check prevents attribute collection for disabled loggers).console/README.md— adds the completeFormat*/Render*API surface, all type definitions (CompilerError,TableConfig,FormField,ListItem, etc.),ConfirmAction,IsAccessibleMode, andFormatFileSize.Not included
cli,workflow, andparser— too large for manual authoring; the issue recommends running thespec-extractorworkflow for these.