Skip to content
This repository was archived by the owner on May 30, 2026. It is now read-only.

feat: toolkit parity II — context, inputs and core conveniences - #127

Merged
C. Spencer Beggs (spencerbeggs) merged 3 commits into
mainfrom
ws5-toolkit-parity-context
May 21, 2026
Merged

C. Spencer Beggs (spencerbeggs) merged 3 commits into
mainfrom
ws5-toolkit-parity-context

Conversation

@spencerbeggs

Copy link
Copy Markdown
Contributor

WS5 — Toolkit parity II: context, inputs & core conveniences

Fifth workstream of the reviewer-improvements 2.0.0 effort. Brings the most-used @actions/core + @actions/github boundaries to exact documented fidelity, in Effect idioms. Bump: minor. Purely additive — base main (independent of WS2–WS4).

Items (GHA reviewer G3–G10)

  • GitHub-faithful inputs — ActionInput.boolean (YAML-1.2 "Core Schema": true|True|TRUE / false|False|FALSE, fails ConfigError.InvalidData on anything else — closes the silent divergence where Config.boolean accepts yes/on/1 and rejects True) and ActionInput.multiline.
  • Typed event payload — ActionEnvironment.payload parses GITHUB_EVENT_PATH into a schema-validated, extras-preserving WebhookPayload (degrades to {} when unset/missing, fails only on malformed JSON); .repo / .issue mirror context.repo/context.issue; .isDebug mirrors core.isDebug().
  • Core conveniences — WorkflowCommand.notice + ActionLogger.notice (::notice:: annotations, no log-level remap) with a toolkit-faithful AnnotationProperties mapper; GithubMarkdown.image/quote.
  • OIDC optional audience — OidcTokenIssuer.getToken(audience?) matches core.getIDToken(audience?) for cloud-provider federation (backward compatible; Sigstore caller unaffected). Uses encodeURIComponent (toolkit-faithful %20, not +).
  • Minor — WorkflowCommand.stopCommands/resumeCommands/setCommandEcho; PathUtils.toPosixPath/toWin32Path/toPlatformPath.

Notes

  • The new payload/repo/issue accessors require FileSystem in their R channel (provided by ActionsRuntime.Default); github/runner/get/getOptional/isDebug stay R = never.

Verification

  • pnpm run lint — exit 0 · pnpm run ci:build — passes (AnnotationProperties + WebhookPayload exported from both barrels)
  • pnpm run test — 1108 passed; the YAML-1.2 truth table asserts yes/on/1/0/no/mixed-case all FAIL, and the OIDC audience param is conditional (Sigstore path stays green)

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

Add GitHub-faithful input combinators ActionInput.boolean (YAML 1.2 Core Schema,
unlike Config.boolean which accepts yes/on/1) and ActionInput.multiline. Parse
GITHUB_EVENT_PATH into a schema-validated WebhookPayload exposed as
ActionEnvironment.payload, with repo, issue and isDebug accessors mirroring
@actions/github and @actions/core. Add WorkflowCommand.notice and
ActionLogger.notice for notice annotations, plus stopCommands, resumeCommands and
setCommandEcho. Add GithubMarkdown.image and quote, and a PathUtils namespace.
Make OidcTokenIssuer.getToken audience optional for cloud-provider federation.

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

@savvy-web-bot savvy-web-bot Bot left a comment

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.

Code Review

Current Commit: 5afde0a

Summary

Solid, focused workstream. The YAML 1.2 ActionInput.boolean truth table is exactly right and closes a real silent-divergence footgun vs Config.boolean. The payload/repo/issue chain degrades gracefully (empty {} on missing file, only fails on malformed JSON), the OIDC string-concatenation matches the toolkit's own encodeURIComponent pattern, and every new surface has comprehensive tests. 1108 passing.

Three items worth addressing:


1. GithubMarkdown.image / quote — attribute values are not HTML-escaped (diverges from @actions/core) ⚠️

@actions/core.summary.addImage runs attribute values through an internal escape() helper before writing them into the HTML string; addQuote similarly sanitises text and cite. The new GithubMarkdown.image / quote helpers pass all values through raw. Inline comment on GithubMarkdown.ts has the details. The practical risk inside GitHub's step-summary renderer is low (GitHub sanitises on the receiving end), but the PR description promises "exact @actions/core summary HTML" — this should either be fixed or disclaimed with a @remarks Attribute values are not HTML-escaped.


2. Redundant buildPayload alias in ActionEnvironmentLive.ts (nit)

const buildPayload = readPayload is a no-op alias. Inline comment with the one-line fix.


3. repoFromEnvOrPayload / issueFromPayload duplicated between Live and Test layers (note)

Identical branch logic lives in two files. The signatures differ by necessity (Live reads process.env, Test takes a param), so no immediate action needed, but worth a shared internal helper if a third consumer appears. Inline comment for reference.


What's excellent

  • ActionInput.boolean YAML 1.2 truth table: precise — rejects yes/on/1/mixed-case, accepts exactly the six canonical spellings. The error message cites the spec list, which is unusually helpful for a ConfigError.
  • WorkflowCommand additions: annotationProperties mapping (startLine→line, startColumn→col) is faithful. resumeCommands correctly emits ::{token}:: (the token itself as the command name).
  • ActionEnvironment.payload degradation logic: the file-missing path logs a debug message and returns {} — right call.
  • OIDC audience change: switching from url.searchParams.set to ${baseUrl}&audience=${encodeURIComponent(audience)} exactly matches @actions/core.getIDToken internals. The no-audience test asserting the URL is unmodified is a nice correctness anchor.
  • ActionEnvironmentTest.layer payload injection: letting test callers seed a WebhookPayload avoids file I/O entirely in tests — the FileSystem in R is satisfied by type but never exercised. Clean.
  • Test coverage: truth table for boolean rejects, OIDC URL-encode test, ActionEnvironmentTest integration, full EventPayload schema suite — comprehensive.

Comment thread src/utils/GithubMarkdown.ts
Comment thread src/layers/ActionEnvironmentLive.ts
import { GITHUB_ENV_MAP, RUNNER_ENV_MAP } from "./internal/environmentMaps.js";

const repoFromEnvOrPayload = (
repository: string | undefined,

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.

repoFromEnvOrPayload and issueFromPayload are implemented twice — here and in ActionEnvironmentLive.ts. The only difference is that Live reads process.env.GITHUB_REPOSITORY directly, while Test receives repository as a parameter (correct for isolation). The body of each branch is otherwise identical.

This is acceptable as-is, but if the logic ever needs to change (e.g., to handle edge cases in GITHUB_REPOSITORY parsing) it'll need updating in two places. Worth an internal shared helper (e.g., src/layers/internal/repoHelpers.ts) if a third consumer appears.

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.

Still present by design: Live reads process.env.GITHUB_REPOSITORY directly (no param), Test receives it as a parameter for isolation. The divergence is inherent to the Live/Test split rather than incidental duplication. Leaving open for awareness — if a third consumer appears, a shared internal helper is the right move.

- document that GithubMarkdown.image/quote interpolate values raw (not
  HTML-escaped), matching @actions/core summary which performs no escaping
- inline the no-op buildPayload alias in ActionEnvironmentLive

Signed-off-by: C. Spencer Beggs <spencer@savvyweb.systems>
@savvy-web-bot

savvy-web-bot Bot commented May 21, 2026

Copy link
Copy Markdown
Contributor

Code Review

Current Commit: 687774e | Checks: All 6 passed (PR Title, Conventional Commits, Code Quality, Markdown, Build, Tests — 1108 tests)


Summary

WS5 is a clean, well-scoped additive layer. The five feature areas each achieve exact @actions/core / @actions/github behavioral parity, and the test suite is comprehensive. No blocking issues; previous review feedback has been addressed.


Previous Comments

Thread Status
GithubMarkdown.image/quote — @remarks for raw-interpolation matching toolkit ✅ Resolved — author verified toolkit does no escaping; @remarks added to both functions
buildPayload no-op alias ✅ Resolved — payload: readPayload used directly
Duplicate repoFromEnvOrPayload/issueFromPayload in Live/Test ↗️ Open by design — Live reads process.env directly, Test receives it as a param; the divergence is inherent to the Live/Test pattern

Feature-by-Feature Notes

ActionInput.boolean / ActionInput.multiline

The YAML 1.2 Core Schema truth table is exactly right and the error message is verbatim toolkit parity ("Support boolean input list: \true | True | TRUE | false | False | FALSE`"). The whitespace-trimming before the comparison correctly matches the toolkit's default trimWhitespace` behaviour.

multiline is also correct: splitting on \n, filtering exact-empty-string, then trimming — which means \r\n line endings are handled correctly because trim() strips the trailing \r. Same logic as the toolkit.

Test coverage is excellent: the full YAML 1.2 truth table (all 6 valid + 7 invalid inputs), whitespace tolerance, Config.withDefault composition, and missing-input semantics are all asserted.

WebhookPayload schema

{ onExcessProperty: "preserve" } is applied globally to the parse context, so unknown keys in nested structs (Repository, IssueRef, etc.) survive the decode — this correctly mirrors the toolkit's [key: string]: any index signature. The explicit typed subset covers all the fields consumed by repo/issue.

ActionEnvironment.payload / .repo / .issue

The degradation chain (env unset → empty, file missing → empty, malformed JSON → fail) matches @actions/github. The R = FileSystem.FileSystem requirement is correctly expressed in the service interface without being baked into the layer type — ActionsRuntime.Default provides it transparently, and the Test layer's pure-value implementation avoids the requirement at the call site.

One observation (not a bug): buildRepo and buildIssue both call readPayload independently, so a consumer accessing both svc.repo and svc.issue will read the event file twice. This is idiomatic Effect (effects are descriptions, not memoized), and the file is tiny, so it's inconsequential in practice. Callers can wrap with Effect.cached if it ever matters.

WorkflowCommand.notice / stopCommands / resumeCommands / setCommandEcho

annotationProperties correctly maps startLine→line, startColumn→col — matching @actions/core's toCommandProperties. The notice(properties, message) internal order is aligned with the existing issue(command, props, message) pattern; the public service API ActionLogger.notice(message, properties?) correctly matches the toolkit's user-facing order.

resumeCommands(token) emitting ::{token}:: is correct per the GHA workflow command protocol.

OidcTokenIssuer.getToken(audience?)

The switch from new URL().searchParams.set() to encodeURIComponent is the right call — URLSearchParams encodes spaces as +, while the toolkit uses %20. The string-concatenation approach correctly assumes the GitHub-provided ACTIONS_ID_TOKEN_REQUEST_URL always contains ?api-version=2.0, making the &audience=... append safe. The inline comment explaining the toolkit contract is warranted here.

PathUtils

Minimal and correct. All three helpers (toPosixPath, toWin32Path, toPlatformPath) mirror the toolkit exactly.


Positive Highlights

  • The ActionInput.boolean doc comment does excellent work explaining the divergence from Config.boolean — callers upgrading from the built-in combinator will immediately understand why this one exists.
  • The payload/repo/issue error messages are specific and actionable ("context.repo requires a GITHUB_REPOSITORY environment variable like 'owner/repo' or a repository in the event payload").
  • ActionLoggerTest captures notices into typed state (notices: Array<{message, properties}>), keeping test assertions clean without stdout spying.
  • All new tests are self-contained (env setup/teardown in beforeEach/afterEach) with no test-order dependence.

Verdict

Approve. All checks green, no new blocking issues, prior feedback addressed. The FileSystem R-channel design for payload/repo/issue is the cleanest approach available without either polluting the layer type or hiding the dependency.

@savvy-web-bot savvy-web-bot Bot left a comment

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.

All previous feedback addressed. No new blocking issues. Approving.

@spencerbeggs
C. Spencer Beggs (spencerbeggs) merged commit 8b3c749 into main May 21, 2026
28 checks passed
@spencerbeggs
C. Spencer Beggs (spencerbeggs) deleted the ws5-toolkit-parity-context branch May 21, 2026 21:37
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant