[C. Spencer Beggs (spencerbeggs)]

WS6 — Documentation

Sixth workstream of the reviewer-improvements 2.0.0 effort. Comprehensive documentation pass covering the full 2.0 release surface. Bump: minor. Single clean commit; base main.

Accuracy against the final 2.0 API

Corrects stale descriptions throughout docs/:

• GitHubClientLive.fromEnv() is now a function (not a bare layer).
• fromToken takes Redacted<string> only (WS3 tightened the signature).
• fromApp is a scoped layer — revokes its token on scope close, requires HttpClient.HttpClient.
• The "Upgrading to 2.0" migration note and @actions/* substitution map re-verified and expanded.

New services documented

Glob (glob + @actions/glob-compatible hashFiles), IoUtil (which/whichOrFail/findInPath, noting which/findInPath error channel is never), Artifact (upload/list/get/download/delete, with the "must run inside a JS action, not a run: step" constraint), ActionInput (YAML 1.2 Core Schema boolean, multiline), the typed event payload (ActionEnvironment.payload/repo/issue/isDebug + WebhookPayload), PathUtils, ActionLogger.notice + WorkflowCommand notice/stop-commands/echo, GithubMarkdown.image/quote, paginateStream.

Four new guides

• 05-best-practices.md — "Building a robust action" (layer composition, dry-run, permission checks, idempotency, secrets, error accumulation).
• 06-toolkit-parity.md — "Coming from @actions/*" (narrative walkthrough with worked examples per package; cross-references the substitution map in architecture).
• 07-logging-and-error-handling.md — ActionLogger/Step/notice/annotations/formatCause/setSecret.
• 08-resilient-github-api.md — Retry, rate-limit awareness, Retry-After/x-ratelimit-reset, ResilienceOptions, paginateStream.

Structure

docs/ renumbered to a contiguous 01–16 reading order (4 new guides at 05–08; existing guides/reference tail at 09–16). Full cross-link sweep and humanizer pass applied.

Verification

pnpm run lint:md — 0 errors (40 files) · cross-link/anchor integrity confirmed · all APIs spot-checked against src/.

Signed-off-by: C. Spencer Beggs spencer@savvyweb.systems

[savvy-web-bot]

Code Review

Current Commit: 0bf49c1

Summary

This is a high-quality documentation PR. The three new guides are accurate, well-structured and teach the right mental models. The backfill of 03-services.md, the error table, the Octokit-cast seam write-up and the Action.run type-erasure note are all genuinely useful additions that close real comprehension gaps.

There is one structural accuracy issue worth resolving before merge: the "Upgrading to 2.0" section in 10-architecture.md documents breaking changes (fromApp Scope requirement, fromEnv() function call) that live in ws2-resilient-github-client and ws3-effect-quality-observability — branches not yet merged to main. This creates a direct internal contradiction: the GitHubClientLive constructor table on line 349 correctly describes fromEnv as a property (consistent with the source), while the migration note two pages down tells consumers to call fromEnv(). See inline comments for details.

Checks

Check	Result
PR Title Validation	✅ success
Conventional Commits	✅ success
Code Quality	✅ success
Markdown	✅ success
Tests	✅ success

What's good

05-slsa-attestations.md — The layer-stack walkthrough is the right way to introduce four cooperating services. Documenting that listForSubject's predicateType filter is client-side (not a server query parameter, 404 = empty array) is exactly the kind of non-obvious contract consumers need to see written down. Path A vs. Path B for sbom is clear without being verbose.

06-publishing.md — The two-digest table at the top (digest = npm sha512/base64 for registry comparison, sha256Hex = lowercase hex for attestations) is one of the best single pieces of new content in this PR. That distinction is a silent footgun if it's not explained here, and the explanation is concise and precise.

07-step-logging.md — defaultSummary gets its own section rather than a footnote, which is the right call — it's a common pattern. The collapse unbounded-concurrency note ("meant for the small fan-out… typically two to four") sets appropriate expectations without over-constraining the API.

10-architecture.md new sections — The Octokit-cast seam write-up is unusually honest documentation: it names the runtime guard, explains why the compile-time guarantee was deliberately traded away, and says what the cost is. The Action.run type-erasure section does the same for the any cast. Both are the kind of thing that would otherwise only be discoverable by reading the source.

09-error-handling.md — The eight-entry error table with reason / operation discriminators and the retryable flag on GitHubArtifactMetadataError surfaces exactly what a caller needs for precise catchTag recovery. The SlsaError "claims" vs "env" distinction (config problem vs transient) is a nice illustration of why discriminators matter.

Issue: "Upgrading to 2.0" describes unmerged WS2/WS3 changes

The migration section has two items that don't match the current codebase (see inline comments):

1. fromEnv function call — line 271 says "is now a function — call fromEnv()", but src/layers/GitHubClientLive.ts has const fromEnv: Layer.Layer<GitHubClient, GitHubClientError> (a property). The constructor table in the same file (line 349) says "property, no arguments" — accurate per the source.

2. fromApp Scope requirement — line 256 says fromApp requires Scope; the source returns Layer.Layer<GitHubClient, GitHubAppError> with no Scope.

The Redacted signatures and HttpClient requirement items are in the same situation: commit 58c2456 (ws3-effect-quality-observability) shows those as planned changes, but they're not in main yet.

Suggested path: If WS2/WS3 are gated to land before this PR, the docs are fine as written and just need the PR ordering enforced. If not, the safest fix is to scope the note: "These changes ship in the WS2/WS3 PRs. If you're on 1.x today, fromEnv is still a property and fromApp does not require Scope." That keeps the forward guidance while preventing consumers from acting on instructions that don't apply yet.

Minor observations (no action needed)

• Step.failure is mentioned once at the end of the groupStep section and listed in the exports at the top of 07-step-logging.md, but has no dedicated section. Given it's the non-fatal path (lets the surrounding loop continue), a one-paragraph treatment could help — but given it's intentionally secondary, the current placement is defensible.
• The JSON.parse(json) as Record<string, unknown> in the Path B SBOM example (05-slsa-attestations.md line 133) is the right pattern (serialization normalises the CycloneDX structure before attesting), but a one-liner comment next to it explaining why you round-trip through string form would help readers who'd otherwise wonder why you serialize only to immediately parse back.

[savvy-web-bot]

Same pre-merge ordering concern as the fromEnv note below: the current source at src/layers/GitHubClientLive.ts shows fromApp returning Layer.Layer<GitHubClient, GitHubAppError> with no Scope in the requirements channel. The Scope requirement is a WS2/WS3 change not yet in main.

Minor additional nit: the constructor table on line 351 doesn't reflect the Scope requirement either, so when WS2 lands the table will also need updating. It might be worth a TODO comment in the table to keep them in sync.

[savvy-web-bot]

Reviewed at commit 4dae7bb.

[savvy-web-bot]

The phrase "building the layer needs a Scope" is inaccurate based on the current type signature.

fromApp returns Layer.Layer<GitHubClient, GitHubAppError, HttpClient.HttpClient> — Scope is not in the requirements channel. Layer.scoped (which fromApp uses internally) produces a type without Scope in requirements; the layer itself manages the scope lifecycle. Callers do not need to supply a Scope via the requirements channel.

Where Effect.scoped matters in practice:

• Layer.memoize — Layer.memoize(fromApp(...)) returns a Scope-requiring effect, so Effect.scoped IS needed there (as the JSDoc example shows).
• Plain Effect.provide — program.pipe(Effect.provide(fromApp(...))) already closes the scope when the provided effect completes; Effect.scoped is optional here (the token is still revoked at that boundary).

Suggest revising to something like: "fromApp is a scoped layer that manages its own scope internally — the minted token is revoked when the layer's scope closes. A plain Effect.provide handles this automatically; you only need an explicit Effect.scoped wrapper when using Layer.memoize to share one token across multiple sub-programs."

(This was flagged in the previous review on the old docs/10-architecture.md; the same inaccuracy carried over into the rewritten section.)

[savvy-web-bot]

This example calls octokit.issues.listForRepo(...) without a type cast, but the paginateStream callback receives octokit: unknown (see GitHubClient service definition). This would be a TypeScript compile error in real consumer code.

The docs/08-resilient-github-api.md guide (also added in this PR) handles this correctly with an explicit cast. For consistency, the paginateStream example here should either:

// Option A — inline structural cast (same style as the guide):
(octokit as { issues: { listForRepo: (p: unknown) => Promise<{ data: Array<{ number: number; closed_at: string | null }> }> } }).issues.listForRepo(...)

// Option B — cast to Octokit directly (@octokit/rest is a direct dep):
import { Octokit } from "@octokit/rest"
(octokit as Octokit).rest.issues.listForRepo(...)

Option B is cleaner for a docs example since @octokit/rest is a direct dependency. Either way, the uncast octokit.issues.listForRepo(...) would fail tsc --strict.

[savvy-web-bot]

Code Review

Current Commit: 4dae7bb

Summary

This is a comprehensive docs-only PR (22 files, +1845/−161) adding four new guides, backfilling service documentation, and renumbering the docs directory to a clean 01–16 reading order. The one source-code change is a comment-reference update in IoUtil.ts to match the new numbering. Overall the documentation is thorough, accurate against the current API surface, and significantly raises the discoverability of the library's flagship features.

What's well done

• New guides are accurate and example-rich. 05-best-practices, 06-toolkit-parity, 07-logging-and-error-handling, 08-resilient-github-api, 09-step-logging, 10-slsa-attestations, and 11-publishing all match the current service interfaces and exported symbols. The OIDC → Sigstore → GitHub attestation chain is precisely described. The Step API (withStep, success, failure, collapse, groupStep, defaultSummary) exactly matches the source.
• Backfill accuracy. fromEnv is now correctly documented as a function (matching source line 327 of GitHubClientLive.ts), fromToken correctly requires Redacted<string>, fromApp now correctly shows HttpClient.HttpClient in the requirements channel, and the stale tarball field is fixed to tarballPath.
• @actions/* substitution map is complete and accurate — IoUtil, Glob, and Artifact rows are filled in, and the table correctly distinguishes IoUtil from @effect/platform FileSystem for the cp/mv/rmRF/mkdirP operations.
• src/utils/IoUtil.ts comment reference updated correctly to docs/15-filesystem-io.md.

---

Issues found

1. docs/14-architecture.md line 262 — fromApp Scope description is inaccurate

The migration note states "building the layer needs a Scope" and advises wrapping every bare Effect.provide in Effect.scoped. This is factually incorrect based on the current type:

Layer.Layer<GitHubClient, GitHubAppError, HttpClient.HttpClient>

Layer.scoped (used internally by fromApp) produces a layer without Scope in the requirements channel — the scope is managed internally by Effect. A plain Effect.provide(fromApp(...)) already closes the scope and revokes the token when the effect completes; Effect.scoped is only needed in the Layer.memoize pattern. An inline comment has been left on line 262.

(This is a carry-forward of the accuracy gap flagged in the previous review on the old docs/10-architecture.md. The HttpClient.HttpClient requirement is now correctly documented — only the Scope wording needs fixing.)

2. docs/03-services.md line 302 — uncast octokit in the new paginateStream example

The newly-added streaming pagination section calls octokit.issues.listForRepo(...) without a cast. Since GitHubClient.paginateStream's callback receives octokit: unknown, this would be a TypeScript compile error in real consumer code. The docs/08-resilient-github-api.md guide (also added in this PR) handles this correctly with an explicit cast. An inline comment has been left on line 302.

---

Previous review thread status

Thread	Status
fromEnv() as function vs. property (comment 3279658228, docs/14-architecture.md)	✅ Resolved — source and constructor table now correctly describe fromEnv as a function. Thread replied to and marked resolved.
fromApp Scope requirement (comment 3279658892, old docs/10-architecture.md)	⚠️ Orphaned — file was renamed to docs/14-architecture.md in this commit. The concern partially remains as issue #1 above.

---

Merge note

The PR description calls out a filename collision with #125: docs/09-error-handling.md (this PR) vs docs/09-filesystem-io.md (WS4/PR #125). Whichever merges second needs a rename — no link rot expected since the architecture substitution map references the filesystem guide generically.