Repository navigation
feat: toolkit parity II — context, inputs and core conveniences - #127
Conversation
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>
There was a problem hiding this comment.
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.booleanYAML 1.2 truth table: precise — rejectsyes/on/1/mixed-case, accepts exactly the six canonical spellings. The error message cites the spec list, which is unusually helpful for aConfigError.WorkflowCommandadditions:annotationPropertiesmapping (startLine→line,startColumn→col) is faithful.resumeCommandscorrectly emits::{token}::(the token itself as the command name).ActionEnvironment.payloaddegradation logic: the file-missing path logs a debug message and returns{}— right call.- OIDC audience change: switching from
url.searchParams.setto${baseUrl}&audience=${encodeURIComponent(audience)}exactly matches@actions/core.getIDTokeninternals. The no-audience test asserting the URL is unmodified is a nice correctness anchor. ActionEnvironmentTest.layerpayload injection: letting test callers seed aWebhookPayloadavoids file I/O entirely in tests — theFileSysteminRis satisfied by type but never exercised. Clean.- Test coverage: truth table for boolean rejects, OIDC URL-encode test,
ActionEnvironmentTestintegration, fullEventPayloadschema suite — comprehensive.
| import { GITHUB_ENV_MAP, RUNNER_ENV_MAP } from "./internal/environmentMaps.js"; | ||
|
|
||
| const repoFromEnvOrPayload = ( | ||
| repository: string | undefined, |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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>
Code ReviewCurrent Commit: 687774e | Checks: All 6 passed (PR Title, Conventional Commits, Code Quality, Markdown, Build, Tests — 1108 tests) SummaryWS5 is a clean, well-scoped additive layer. The five feature areas each achieve exact Previous Comments
Feature-by-Feature Notes
|
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/githubboundaries to exact documented fidelity, in Effect idioms. Bump: minor. Purely additive — basemain(independent of WS2–WS4).Items (GHA reviewer G3–G10)
ActionInput.boolean(YAML-1.2 "Core Schema":true|True|TRUE/false|False|FALSE, failsConfigError.InvalidDataon anything else — closes the silent divergence whereConfig.booleanacceptsyes/on/1and rejectsTrue) andActionInput.multiline.ActionEnvironment.payloadparsesGITHUB_EVENT_PATHinto a schema-validated, extras-preservingWebhookPayload(degrades to{}when unset/missing, fails only on malformed JSON);.repo/.issuemirrorcontext.repo/context.issue;.isDebugmirrorscore.isDebug().WorkflowCommand.notice+ActionLogger.notice(::notice::annotations, no log-level remap) with a toolkit-faithfulAnnotationPropertiesmapper;GithubMarkdown.image/quote.OidcTokenIssuer.getToken(audience?)matchescore.getIDToken(audience?)for cloud-provider federation (backward compatible; Sigstore caller unaffected). UsesencodeURIComponent(toolkit-faithful%20, not+).WorkflowCommand.stopCommands/resumeCommands/setCommandEcho;PathUtils.toPosixPath/toWin32Path/toPlatformPath.Notes
payload/repo/issueaccessors requireFileSystemin theirRchannel (provided byActionsRuntime.Default);github/runner/get/getOptional/isDebugstayR = never.Verification
pnpm run lint— exit 0 ·pnpm run ci:build— passes (AnnotationProperties+WebhookPayloadexported from both barrels)pnpm run test— 1108 passed; the YAML-1.2 truth table assertsyes/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