From ec6d9edfb7ff8339edb23dc076ffeab5a242fdbc Mon Sep 17 00:00:00 2001 From: Vivswan Shah <58091053+Vivswan@users.noreply.github.com> Date: Tue, 22 Sep 2026 05:26:27 -0400 Subject: [PATCH 1/2] refactor: replace hand-rolled helpers with the library calls that already ship zod issue paths render through z.core.toDotPath at the four sites that each spelled their own join, so a key with a non-word character reads as ["my-key"] instead of .my-key. The secret scanning pattern comparison uses isDeepStrictEqual from node:util instead of JSON.stringify equality. The two set-equality helpers use Set.prototype.isSubsetOf, and the tsconfig lib moves from ES2022 to ES2025 for its types while the target stays ES2022. The REST query string is built by URLSearchParams, which writes a space as + where encodeURIComponent wrote %20; GitHub decodes both. Central-mode file names go through extname and parse from node:path instead of a YAML extension regex. valueAt has one home in the shared list-section module, the two environments modules share one unreconcilable message builder, and the generator scripts call RegExp.escape instead of a local escapeRe. --- .github/scripts/gen-action-docs.ts | 10 ++++---- .github/scripts/gen-docs.ts | 25 ++++++++----------- .github/scripts/lib/generated-regions.ts | 5 ---- src/discovery/central.ts | 12 ++++----- src/engine/validate.ts | 24 ++++-------------- src/sections/contract/endpoints.ts | 5 +--- src/sections/contract/live.ts | 7 ++---- src/sections/contract/permissions.ts | 6 +---- src/sections/custom_properties/index.ts | 2 +- src/sections/environments/branch-policies.ts | 19 +++++++------- src/sections/environments/endpoints.ts | 13 ++++++++++ src/sections/environments/protection-rules.ts | 13 +++------- src/sections/rulesets/schema.ts | 12 +++------ .../secret_scanning_custom_patterns/index.ts | 3 ++- .../secret_scanning_custom_patterns/schema.ts | 3 +-- src/sections/shared/list-section.ts | 5 ++-- test/docs/markdown.ts | 4 +-- test/e2e/foundation.test.ts | 9 ++++--- test/flows/snapshot.test.ts | 7 +++--- test/scripts/release-pipeline.test.ts | 7 +++--- test/sections/docs-registry.test.ts | 11 ++++---- tsconfig.json | 2 +- 22 files changed, 85 insertions(+), 119 deletions(-) diff --git a/.github/scripts/gen-action-docs.ts b/.github/scripts/gen-action-docs.ts index 24fdc12cf..630808a5e 100644 --- a/.github/scripts/gen-action-docs.ts +++ b/.github/scripts/gen-action-docs.ts @@ -20,7 +20,7 @@ import { import { SECTIONS } from "../../src/sections/registry.js"; import { agree } from "../../src/text.js"; import { countWord } from "./lib/count-word.js"; -import { escapeRe, type GeneratedRegion, regenerateRegions } from "./lib/generated-regions.js"; +import { type GeneratedRegion, regenerateRegions } from "./lib/generated-regions.js"; const ROOT = join(import.meta.dir, "..", ".."); @@ -338,7 +338,7 @@ function blockShape(lines: string): RegExp { } function tableShape(header: string, cells: string): RegExp { - return blockShape(String.raw`${escapeRe(header)}\n(?:\| ${cells} \|\n)*`); + return blockShape(String.raw`${RegExp.escape(header)}\n(?:\| ${cells} \|\n)*`); } /** A JSON string literal as JSON.stringify() emits it: its own escapes only, bare quotes never. */ @@ -398,7 +398,7 @@ export const GENERATED_REGIONS: Readonly renderPolicyCountSentence(knobbedSections())), }, { @@ -415,7 +415,7 @@ export const GENERATED_REGIONS: Readonly renderGrantSentence(SECTIONS)), }, { @@ -433,7 +433,7 @@ export const GENERATED_REGIONS: Readonly renderCheckModeGatedReads(SECTIONS)), }, diff --git a/.github/scripts/gen-docs.ts b/.github/scripts/gen-docs.ts index 9d35ce6bc..df6c19984 100644 --- a/.github/scripts/gen-docs.ts +++ b/.github/scripts/gen-docs.ts @@ -15,12 +15,7 @@ import type { UndeclaredPolicy } from "../../src/types.js"; import { readArchitecture, renderArchitectureMermaid } from "./arch-lint.js"; import { COVERAGE_DATA, type CoverageData } from "./coverage-data.js"; import { ENDPOINT_ANCHORS, type EndpointAnchors } from "./endpoint-docs.js"; -import { - escapeRe, - type GeneratedRegion, - regenerateRegions, - regionBounds, -} from "./lib/generated-regions.js"; +import { type GeneratedRegion, regenerateRegions, regionBounds } from "./lib/generated-regions.js"; const ROOT = join(import.meta.dir, "..", ".."); export const COVERAGE_PATH = "docs/reference/coverage.md"; @@ -428,7 +423,7 @@ function sectionsTableRegion(name: string, heading: string): GeneratedRegion { name, placement: { kind: "under-heading", heading }, body: new RegExp( - String.raw`^\n(?:${escapeRe(TABLE_HEADER)}\n(?:\| \x60[a-z_]+\x60 \| [^\n]* \|\n)*)?$`, + String.raw`^\n(?:${RegExp.escape(TABLE_HEADER)}\n(?:\| \x60[a-z_]+\x60 \| [^\n]* \|\n)*)?$`, ), render: () => `\n${renderSectionsTable(SECTIONS, DOCS)}\n`, }; @@ -439,7 +434,7 @@ function outputsListRegion(name: string, heading: string): GeneratedRegion { name, placement: { kind: "under-heading", heading }, body: new RegExp( - String.raw`^(?:\x60[a-z]+\x60(?: / \x60[a-z]+\x60)*${escapeRe(RESULT_TAIL)})?$`, + String.raw`^(?:\x60[a-z]+\x60(?: / \x60[a-z]+\x60)*${RegExp.escape(RESULT_TAIL)})?$`, ), render: () => renderOutputsList(RUN_RESULTS), }; @@ -450,7 +445,7 @@ function patUrlRegion(name: string): GeneratedRegion { return { name, placement: { kind: "tail" }, - body: new RegExp(String.raw`^\n(?:\[${escapeRe(PAT_FORM_LABEL)}\]: \S+\n)?$`), + body: new RegExp(String.raw`^\n(?:\[${RegExp.escape(PAT_FORM_LABEL)}\]: \S+\n)?$`), render: () => `\n[${PAT_FORM_LABEL}]: ${patFormUrl()}\n`, }; } @@ -505,10 +500,10 @@ const nonBlank = (excluded: string): string => String.raw`[ \t]*[^${excluded}\s][^${excluded}\r\n]*`; const PROSE_LINE = `${nonBlank("")}\n`; const CELL = nonBlank("|"); -const KEY_CELL = String.raw`\[\x60[a-z_]+\x60\]\(${escapeRe(SECTIONS_PAGE)}\)(?: \(\x60${nonBlank("|\x60")}\x60\))?`; +const KEY_CELL = String.raw`\[\x60[a-z_]+\x60\]\(${RegExp.escape(SECTIONS_PAGE)}\)(?: \(\x60${nonBlank("|\x60")}\x60\))?`; const SUPPORTED_ROWS = String.raw`(?:\| ${CELL} \| ${KEY_CELL} \| ${CELL} \|\n)+`; const GAP_ROWS = String.raw`(?:\| ${CELL} \| ${CELL} \| ${CELL} \|\n)+`; -const GAPS_BODY = String.raw`(?:${PROSE_LINE}\n${escapeRe(GAPS_HEADER)}\n|${escapeRe(GAPS_HEADER)}\n${GAP_ROWS})`; +const GAPS_BODY = String.raw`(?:${PROSE_LINE}\n${RegExp.escape(GAPS_HEADER)}\n|${RegExp.escape(GAPS_HEADER)}\n${GAP_ROWS})`; const BULLETS = `(?:- ${PROSE_LINE})+`; const NOTE_GROUPS = String.raw`(?:\*\*${nonBlank("*")}\*\* \(\x60[a-z_]+\x60\)\n\n${BULLETS}\n)+`; @@ -519,10 +514,10 @@ const COVERAGE_REGIONS: readonly GeneratedRegion[] = [ name: "coverage", placement: { kind: "tail" }, body: new RegExp( - String.raw`^\n(?:(?:${PROSE_LINE}\n)+${escapeRe(SUPPORTED_HEADING)}\n\n${escapeRe(SUPPORTED_HEADER)}\n` + - String.raw`${SUPPORTED_ROWS}\n${escapeRe(NOTES_HEADING)}\n\n${NOTE_GROUPS}` + - String.raw`${escapeRe(GAPS_HEADING)}\n\n${GAPS_BODY}\n${escapeRe(NO_API_HEADING)}\n\n` + - String.raw`${PROSE_LINE}\n${BULLETS}\n${escapeRe(OUT_OF_SCOPE_HEADING)}\n\n${BULLETS})?$`, + String.raw`^\n(?:(?:${PROSE_LINE}\n)+${RegExp.escape(SUPPORTED_HEADING)}\n\n${RegExp.escape(SUPPORTED_HEADER)}\n` + + String.raw`${SUPPORTED_ROWS}\n${RegExp.escape(NOTES_HEADING)}\n\n${NOTE_GROUPS}` + + String.raw`${RegExp.escape(GAPS_HEADING)}\n\n${GAPS_BODY}\n${RegExp.escape(NO_API_HEADING)}\n\n` + + String.raw`${PROSE_LINE}\n${BULLETS}\n${RegExp.escape(OUT_OF_SCOPE_HEADING)}\n\n${BULLETS})?$`, ), render: () => `\n${renderCoverage(SECTIONS, DOCS, COVERAGE_DATA, ENDPOINT_ANCHORS)}\n`, }, diff --git a/.github/scripts/lib/generated-regions.ts b/.github/scripts/lib/generated-regions.ts index fc67162a2..8e9f1605d 100644 --- a/.github/scripts/lib/generated-regions.ts +++ b/.github/scripts/lib/generated-regions.ts @@ -8,11 +8,6 @@ import { Parser } from "yaml"; /** Which comment syntax a file's markers use: a complete `` comment, or a whole-line YAML `#` comment. */ export type MarkerSyntax = "html" | "yaml"; -/** `text` as a regex source matching itself literally; body shapes splice renderer constants through it. */ -export function escapeRe(text: string): string { - return text.replace(/[.*+?^${}()|[\]\\]/g, "\\$&"); -} - /** Character offsets of one marker: `[start, end)`. */ export type MarkerSpan = readonly [start: number, end: number]; diff --git a/src/discovery/central.ts b/src/discovery/central.ts index 14e62e49f..d7f6293d5 100644 --- a/src/discovery/central.ts +++ b/src/discovery/central.ts @@ -4,13 +4,13 @@ */ import { existsSync, readdirSync, statSync } from "node:fs"; -import { join } from "node:path"; +import { extname, join, parse } from "node:path"; import { err, ok, type Result } from "neverthrow"; import { type SlugKey, slugKey } from "../github/slug.js"; import type { CentralFileProblem, ProblemOf } from "../problem.js"; import { type CentralTarget, SLUG_RE } from "./targets.js"; -const YAML_EXT = /\.ya?ml$/; +const YAML_EXTENSIONS = new Set([".yml", ".yaml"]); export function resolveCentralTargets( reposDir: string, @@ -52,13 +52,13 @@ export function resolveCentralTargets( ); continue; } - if (!YAML_EXT.test(inner)) { + if (!YAML_EXTENSIONS.has(extname(inner))) { warnings.push( `ignoring ${innerPath}: not a .yml/.yaml file, so it defines no target repository`, ); continue; } - addTarget(`${owner}/${inner.replace(YAML_EXT, "")}`, innerPath); + addTarget(`${owner}/${parse(inner).name}`, innerPath); } }; @@ -71,7 +71,7 @@ export function resolveCentralTargets( scanOwnerDir(entryPath, entry); continue; } - if (!YAML_EXT.test(entry)) { + if (!YAML_EXTENSIONS.has(extname(entry))) { warnings.push( `ignoring ${entryPath}: not a .yml/.yaml file, so it defines no target repository`, ); @@ -81,7 +81,7 @@ export function resolveCentralTargets( ownerlessFiles.push(entryPath); continue; } - addTarget(`${adminOwner}/${entry.replace(YAML_EXT, "")}`, entryPath); + addTarget(`${adminOwner}/${parse(entry).name}`, entryPath); } if (ownerlessFiles.length > 0) { errors.push({ kind: "ownerless", files: ownerlessFiles }); diff --git a/src/engine/validate.ts b/src/engine/validate.ts index 46ff02d34..ffe62af3e 100644 --- a/src/engine/validate.ts +++ b/src/engine/validate.ts @@ -1,6 +1,7 @@ /** Shape validation against each section's loose zod shape; the parsed output, not the input, is what the engine applies. */ import { err, ok, type Result } from "neverthrow"; +import { z } from "zod"; import { nonPlainKind } from "../plain-data.js"; import type { ProblemOf } from "../problem.js"; import { LIST_SECTIONS, type ListSection, SECTION_KEYS, type SettingsFile } from "../schema.js"; @@ -10,6 +11,7 @@ import { type DeclaredSecretValue, } from "../sections/contract/module.js"; import { listLayering, sectionModule, sectionShape } from "../sections/registry.js"; +import { valueAt } from "../sections/shared/list-section.js"; import { agree, countNoun } from "../text.js"; import { type SettingsSource, validateSecretRef } from "./secret-refs.js"; @@ -19,18 +21,6 @@ function isListSection(key: string): key is ListSection { return LIST_KEYS.has(key); } -/** The value at an issue's path, so a null the author wrote can be told from a type the author got wrong. */ -function valueAt(root: unknown, path: readonly PropertyKey[]): unknown { - let node: unknown = root; - for (const step of path) { - if (typeof node !== "object" || node === null) { - return undefined; - } - node = (node as Record)[step]; - } - return node; -} - /** zod's issue fields this module reads; `legal` is this action's own, set where a shape refuses null itself. */ interface NullIssue { code: string; @@ -230,18 +220,14 @@ export function validateSectionShapes( if (!parsed.success) { const issues = parsed.error.issues; for (const issue of issues.slice(0, 5)) { - const path = issue.path - .map((p) => (typeof p === "number" ? `[${p}]` : `.${String(p)}`)) - .join(""); + const path = z.core.toDotPath([key, ...issue.path]); // A null the shape refused is the author saying "empty" where GitHub has no empty state: name the values that // exist. A shape's own diagnostic (a custom issue) already names the fix, unless it supplies the legal values itself. if (valueAt(declared, issue.path) === null && rewritesForNull(issue as NullIssue)) { - problems.push( - `${key}${path} has no empty state; write ${legalValues(issue as NullIssue)}`, - ); + problems.push(`${path} has no empty state; write ${legalValues(issue as NullIssue)}`); continue; } - problems.push(`${key}${path}: ${issue.message}`); + problems.push(`${path}: ${issue.message}`); } if (issues.length > 5) { // A silently truncated list costs one fix-and-rerun cycle per hidden offender. diff --git a/src/sections/contract/endpoints.ts b/src/sections/contract/endpoints.ts index 7055856e3..90f00624a 100644 --- a/src/sections/contract/endpoints.ts +++ b/src/sections/contract/endpoints.ts @@ -243,10 +243,7 @@ export function expand( ); } if (query && Object.keys(query).length > 0) { - const qs = Object.entries(query) - .map(([key, value]) => `${encodeURIComponent(key)}=${encodeURIComponent(value)}`) - .join("&"); - return `${path}?${qs}`; + return `${path}?${new URLSearchParams(query)}`; } return path; } diff --git a/src/sections/contract/live.ts b/src/sections/contract/live.ts index e733fb854..d334049e2 100644 --- a/src/sections/contract/live.ts +++ b/src/sections/contract/live.ts @@ -5,7 +5,7 @@ */ import { err, ok, type Result } from "neverthrow"; -import type { z } from "zod"; +import { z } from "zod"; import { countNoun } from "../../text.js"; import { endpointMethod, endpointPath } from "./endpoints.js"; import type { SectionFailure } from "./errors.js"; @@ -85,10 +85,7 @@ export function parseLive( } const issues = parsed.error.issues; const shown = issues.slice(0, 3).map((issue) => { - const path = issue.path - .map((part) => (typeof part === "number" ? `[${part}]` : `.${String(part)}`)) - .join(""); - return `${path.replace(/^\./, "") || "(body)"}: ${issue.message}`; + return `${z.core.toDotPath(issue.path) || "(body)"}: ${issue.message}`; }); const more = issues.length > 3 ? `; and ${countNoun(issues.length - 3, "more issue", "more issues")}` : ""; diff --git a/src/sections/contract/permissions.ts b/src/sections/contract/permissions.ts index 8f66964a9..32e1789de 100644 --- a/src/sections/contract/permissions.ts +++ b/src/sections/contract/permissions.ts @@ -35,11 +35,7 @@ export function samePermission( } const resources = new Set(a.repo); const others = new Set(b.repo); - return ( - a.org === b.org && - resources.size === others.size && - [...resources].every((resource) => others.has(resource)) - ); + return a.org === b.org && resources.size === others.size && resources.isSubsetOf(others); } /** Human-facing label for each PAT resource, as shown in the token UI. */ diff --git a/src/sections/custom_properties/index.ts b/src/sections/custom_properties/index.ts index 212d648ea..002f91a47 100644 --- a/src/sections/custom_properties/index.ts +++ b/src/sections/custom_properties/index.ts @@ -54,7 +54,7 @@ function sameValue(a: WireValue, b: WireValue): boolean { if (Array.isArray(a) && Array.isArray(b)) { const setA = new Set(a); const setB = new Set(b); - return setA.size === setB.size && [...setA].every((element) => setB.has(element)); + return setA.size === setB.size && setA.isSubsetOf(setB); } return a === b; } diff --git a/src/sections/environments/branch-policies.ts b/src/sections/environments/branch-policies.ts index 4ba2b90cd..9765d041e 100644 --- a/src/sections/environments/branch-policies.ts +++ b/src/sections/environments/branch-policies.ts @@ -7,7 +7,7 @@ import { err, ok, Result, safeTry } from "neverthrow"; import { z } from "zod"; import { subsetDiff } from "../../engine/diff.js"; import type { UndeclaredPolicy } from "../../types.js"; -import { type SectionFailure, sectionFailure } from "../contract/errors.js"; +import type { SectionFailure } from "../contract/errors.js"; import { liveByIdentity, liveIdentity } from "../contract/live.js"; import { type DeclaredIssue, @@ -18,7 +18,11 @@ import { undeclaredNote, } from "../contract/module.js"; import { hasDrift, plainData, type Read } from "../contract/plan.js"; -import type { EnvironmentRestOp, EnvironmentsRestContext } from "./endpoints.js"; +import { + type EnvironmentRestOp, + type EnvironmentsRestContext, + unreconcilable, +} from "./endpoints.js"; import type { LiveEnvironmentBody } from "./index.js"; import type { NestedPlan } from "./nested.js"; import type { DeploymentBranchPolicyConfig } from "./schema.js"; @@ -46,23 +50,18 @@ function livePolicyType(policy: LiveBranchPolicy): string { return typeof policy.type === "string" ? policy.type : "branch"; } -function unreconcilable(envName: string, what: string): SectionFailure { - return sectionFailure( - "live-shape", - `environments: the deployment branch-policy list for environment "${envName}" returned a policy without ${what}, so it cannot be reconciled. Check the "api-version" input against the GitHub REST docs for this endpoint`, - ); -} +const POLICY = { list: "deployment branch-policy", entry: "policy" }; function livePolicyId(policy: LiveBranchPolicy, envName: string): Result { if (policy.id === undefined) { - return err(unreconcilable(envName, "an id")); + return err(unreconcilable(POLICY, envName, "an id")); } return ok(String(policy.id)); } function livePolicyName(policy: LiveBranchPolicy, envName: string): Result { if (typeof policy.name !== "string") { - return err(unreconcilable(envName, "a name")); + return err(unreconcilable(POLICY, envName, "a name")); } return ok(policy.name); } diff --git a/src/sections/environments/endpoints.ts b/src/sections/environments/endpoints.ts index 9316ecfc0..2edf0e638 100644 --- a/src/sections/environments/endpoints.ts +++ b/src/sections/environments/endpoints.ts @@ -4,6 +4,7 @@ */ import type { EndpointDecl } from "../contract/endpoints.js"; +import { type SectionFailure, sectionFailure } from "../contract/errors.js"; import type { PlanContext, PlannedOp } from "../contract/plan.js"; const BRANCH_POLICIES_DENIAL_HINT = @@ -133,3 +134,15 @@ export const ENDPOINTS = { export type EnvironmentsRestContext = PlanContext; export type EnvironmentRestOp = PlannedOp; + +/** A live entry missing the field its reconcile keys on has no identity to match; `noun` names the list and one entry. */ +export function unreconcilable( + noun: { list: string; entry: string }, + envName: string, + what: string, +): SectionFailure { + return sectionFailure( + "live-shape", + `environments: the ${noun.list} list for environment "${envName}" returned a ${noun.entry} without ${what}, so it cannot be reconciled. Check the "api-version" input against the GitHub REST docs for this endpoint`, + ); +} diff --git a/src/sections/environments/protection-rules.ts b/src/sections/environments/protection-rules.ts index 230c816b8..b262d228c 100644 --- a/src/sections/environments/protection-rules.ts +++ b/src/sections/environments/protection-rules.ts @@ -12,7 +12,7 @@ import { undeclaredNote, } from "../contract/module.js"; import type { Read } from "../contract/plan.js"; -import type { EnvironmentsRestContext } from "./endpoints.js"; +import { type EnvironmentsRestContext, unreconcilable } from "./endpoints.js"; import type { NestedPlan } from "./nested.js"; import type { DeploymentProtectionRuleConfig } from "./schema.js"; @@ -32,17 +32,12 @@ const LiveProtectionRule = z.looseObject({ }); type LiveProtectionRule = z.infer; -function unreconcilable(envName: string, what: string): SectionFailure { - return sectionFailure( - "live-shape", - `environments: the deployment protection rule list for environment "${envName}" returned a rule without ${what}, so it cannot be reconciled. Check the "api-version" input against the GitHub REST docs for this endpoint`, - ); -} +const RULE = { list: "deployment protection rule", entry: "rule" }; function liveRuleSlug(rule: LiveProtectionRule, envName: string): Result { const slug = rule.app?.slug; if (typeof slug !== "string") { - return err(unreconcilable(envName, "an app slug")); + return err(unreconcilable(RULE, envName, "an app slug")); } return ok(slug); } @@ -50,7 +45,7 @@ function liveRuleSlug(rule: LiveProtectionRule, envName: string): Result { // A null or string id would serialize into the DELETE path (".../deployment_protection_rules/null"). if (typeof rule.id !== "number") { - return err(unreconcilable(envName, "a numeric id")); + return err(unreconcilable(RULE, envName, "a numeric id")); } return ok(String(rule.id)); } diff --git a/src/sections/rulesets/schema.ts b/src/sections/rulesets/schema.ts index 09cbc3cc9..7065bcc55 100644 --- a/src/sections/rulesets/schema.ts +++ b/src/sections/rulesets/schema.ts @@ -276,14 +276,6 @@ const UnknownRule = z }) .meta({ id: "UnknownRule" }); -function renderPath(path: readonly PropertyKey[]): string { - return path - .map((step, i) => - typeof step === "number" ? `[${step}]` : `${i === 0 ? "" : "."}${String(step)}`, - ) - .join(""); -} - /** * zod reports a failed union as "Invalid input" unless exactly one branch failed on non-aborting checks alone, and * here every branch aborts, so the report is built from the branch the rule's type selects. @@ -296,7 +288,9 @@ function ruleUnionError(issue: z.core.$ZodRawIssue): string | undefined { const type = (issue.input as { type?: unknown } | null)?.type; const own = typeof type === "string" && KNOWN_RULE_TYPES.includes(type) ? known : unknown; return own - .map((sub) => (sub.path.length === 0 ? sub.message : `${renderPath(sub.path)}: ${sub.message}`)) + .map((sub) => + sub.path.length === 0 ? sub.message : `${z.core.toDotPath(sub.path)}: ${sub.message}`, + ) .join("; "); } diff --git a/src/sections/secret_scanning_custom_patterns/index.ts b/src/sections/secret_scanning_custom_patterns/index.ts index dbec68224..9c1da5ed0 100644 --- a/src/sections/secret_scanning_custom_patterns/index.ts +++ b/src/sections/secret_scanning_custom_patterns/index.ts @@ -8,6 +8,7 @@ * PATCH and DELETE -> carry custom_pattern_version when GitHub supplies one; a pattern edited between read and write answers 412 */ +import { isDeepStrictEqual } from "node:util"; import { ok, type Result } from "neverthrow"; import { z } from "zod"; import { agree } from "../../text.js"; @@ -141,7 +142,7 @@ function matches(declaredValue: string | string[], liveValue: unknown): boolean Array.isArray(declaredValue) && (liveValue === undefined || liveValue === null) ? [] : liveValue; - return JSON.stringify(liveComparable) === JSON.stringify(declaredValue); + return isDeepStrictEqual(liveComparable, declaredValue); } function patternsByName( diff --git a/src/sections/secret_scanning_custom_patterns/schema.ts b/src/sections/secret_scanning_custom_patterns/schema.ts index 1e9ee3e34..cfdc1d2ff 100644 --- a/src/sections/secret_scanning_custom_patterns/schema.ts +++ b/src/sections/secret_scanning_custom_patterns/schema.ts @@ -48,8 +48,7 @@ export function unverifiableRegexFields(entry: unknown): string[] { if (reason === undefined) { return []; } - const path = issue.path.map((step) => (typeof step === "number" ? `[${step}]` : step)).join(""); - labels.push(`${path} (${reason})`); + labels.push(`${z.core.toDotPath(issue.path)} (${reason})`); } return labels; } diff --git a/src/sections/shared/list-section.ts b/src/sections/shared/list-section.ts index bf2397ea5..012b693c4 100644 --- a/src/sections/shared/list-section.ts +++ b/src/sections/shared/list-section.ts @@ -517,13 +517,14 @@ function pathOf(field: string): string[] { return field.split("."); } -function valueAt(record: unknown, path: readonly string[]): unknown { +/** The own value at `path`; undefined once a step is missing, so a null the author wrote reads as null, not as absent. */ +export function valueAt(record: unknown, path: readonly PropertyKey[]): unknown { let node: unknown = record; for (const step of path) { if (typeof node !== "object" || node === null || !Object.hasOwn(node, step)) { return undefined; } - node = (node as Fields)[step]; + node = (node as Record)[step]; } return node; } diff --git a/test/docs/markdown.ts b/test/docs/markdown.ts index 3b763d5f6..f71f000b1 100644 --- a/test/docs/markdown.ts +++ b/test/docs/markdown.ts @@ -1,9 +1,7 @@ -import { escapeRe } from "../../.github/scripts/lib/generated-regions.js"; - /** The contents of every fenced code block whose info string is exactly `info`. */ export function fencedBlocks(markdown: string, info: string): string[] { const blocks: string[] = []; - const escaped = escapeRe(info); + const escaped = RegExp.escape(info); // Leading whitespace is stripped (the README nests fences inside list items) and longer fences close per CommonMark; the guides additionally pin // column-zero triple backticks, so this extractor cannot miss a docs/ block. const re = new RegExp( diff --git a/test/e2e/foundation.test.ts b/test/e2e/foundation.test.ts index 5055cdced..e6d80e37a 100644 --- a/test/e2e/foundation.test.ts +++ b/test/e2e/foundation.test.ts @@ -2,7 +2,6 @@ import { describe, expect, test } from "bun:test"; import { chmodSync, mkdirSync, readFileSync, writeFileSync } from "node:fs"; import { basename, join } from "node:path"; import { parse as parseYaml } from "yaml"; -import { escapeRe } from "../../.github/scripts/lib/generated-regions.js"; import { MARKER_LABEL, MARKER_LABEL_CONFIG } from "../../src/report/issue-report.js"; import { SECTION_KEYS } from "../../src/schema.js"; import { ROOT } from "../root.js"; @@ -293,7 +292,9 @@ describe("scenario corpus loader (collectYmlFiles)", () => { // the read silently succeeding. writeFileSync(join(root, "one.yml"), "name: one\n"); chmodSync(root, 0o000); - const named = new RegExp(`^cannot read the scenario directory ${escapeRe(root)}: .*EACCES`); + const named = new RegExp( + `^cannot read the scenario directory ${RegExp.escape(root)}: .*EACCES`, + ); expect(() => collectYmlFiles(root)).toThrow(named); // loadScenarios is what run.ts and the coverage tripwire call, so the // failure must reach them through it. @@ -318,7 +319,9 @@ describe("scenario corpus loader (collectYmlFiles)", () => { expect(roots[0]).toBe(join(import.meta.dir, "scenarios")); expect(roots).toContain(unreadable); expect(() => loadScenarios(roots.slice(1))).toThrow( - new RegExp(`^cannot read the scenario directory ${escapeRe(unreadable)}: .*EACCES`), + new RegExp( + `^cannot read the scenario directory ${RegExp.escape(unreadable)}: .*EACCES`, + ), ); } finally { // withTempRoot restores only the top of the tree; this nested diff --git a/test/flows/snapshot.test.ts b/test/flows/snapshot.test.ts index 7d5f7f2a4..5a5554732 100644 --- a/test/flows/snapshot.test.ts +++ b/test/flows/snapshot.test.ts @@ -22,7 +22,6 @@ import { import { tmpdir } from "node:os"; import { basename, dirname, join, sep } from "node:path"; import { parse as parseYaml } from "yaml"; -import { escapeRe } from "../../.github/scripts/lib/generated-regions.js"; import { parseRepoSlug } from "../../src/discovery/targets.js"; import { SectionSelection } from "../../src/engine/section-selection.js"; import { failRun } from "../../src/flows/deliver.js"; @@ -370,13 +369,13 @@ describe("runSnapshot writes through a staging file", () => { ); // The staging name carries the pid and random bytes, so the line is matched with that piece wild. const [head = "", tail = ""] = line.split(""); - const stagingRe = `${escapeRe(join(cfg.snapshotDir, "o"))}/\\.gsac-\\d+-[0-9a-f]{8}\\.tmp`; + const stagingRe = `${RegExp.escape(join(cfg.snapshotDir, "o"))}/\\.gsac-\\d+-[0-9a-f]{8}\\.tmp`; expect(collected.lines).toEqual([ TAKEN, { level, line: expect.stringMatching( - new RegExp(`^o/a: ${escapeRe(head)}${stagingRe}${escapeRe(tail)}$`), + new RegExp(`^o/a: ${RegExp.escape(head)}${stagingRe}${RegExp.escape(tail)}$`), ), }, { line: `o/b: snapshot written to ${fileB}` }, @@ -907,7 +906,7 @@ describe("runSnapshot, dir form", () => { level: "error", line: expect.stringMatching( new RegExp( - `^${escapeRe(second)}: cannot write the snapshot to .*: the filesystem carries it to the file this run already claimed for ${escapeRe(first)}\\. `, + `^${RegExp.escape(second)}: cannot write the snapshot to .*: the filesystem carries it to the file this run already claimed for ${RegExp.escape(first)}\\. `, ), ), }); diff --git a/test/scripts/release-pipeline.test.ts b/test/scripts/release-pipeline.test.ts index 994a4c524..b0b8cb63c 100644 --- a/test/scripts/release-pipeline.test.ts +++ b/test/scripts/release-pipeline.test.ts @@ -7,7 +7,6 @@ import { describe, expect, setDefaultTimeout, test } from "bun:test"; import { execFileSync } from "node:child_process"; import { mkdirSync, readFileSync, realpathSync, rmSync } from "node:fs"; import { basename, join } from "node:path"; -import { escapeRe } from "../../.github/scripts/lib/generated-regions.js"; import { anchorCheck, anchorReleasePr, @@ -343,7 +342,7 @@ describe("packageRelease", () => { drift(rerun); expect(() => packageRelease({ cwd: rerun, tag: "v2.1.0", sourceSha: fx.mergeSha })).toThrow( new RegExp( - `^${TAG} \\(${before}\\) packages ${fx.mergeSha}, but its tree [0-9a-f]{40} is not the tree [0-9a-f]{40} this checkout's build packages, .*Diff the two trees by hand; ${escapeRe(FROZEN)}$`, + `^${TAG} \\(${before}\\) packages ${fx.mergeSha}, but its tree [0-9a-f]{40} is not the tree [0-9a-f]{40} this checkout's build packages, .*Diff the two trees by hand; ${RegExp.escape(FROZEN)}$`, ), ); expect(git(fx.origin, "rev-parse", `${TAG}^{}`)).toBe(before); @@ -355,7 +354,7 @@ describe("packageRelease", () => { const fx = seedFixture(); const { from, sha, error } = plant(fx); git(from, "push", "--quiet", "origin", `${sha}:${TAG}`); - const frozen = new RegExp(`${escapeRe(FROZEN)}$`); + const frozen = new RegExp(`${RegExp.escape(FROZEN)}$`); const pushes = withPushPlans(fx, [], () => { for (const path of [ () => packageRelease({ cwd: fx.work, tag: "v2.1.0", sourceSha: fx.mergeSha }), @@ -437,7 +436,7 @@ describe("packageRelease", () => { const fx = seedFixture(); spoil(fx.work); const message = new RegExp( - `does not carry a non-empty regular-file ${escapeRe(file)} \\(${entry}\\); refusing to point a consumable ref at an unpackaged commit; run the build before packaging\\.$`, + `does not carry a non-empty regular-file ${RegExp.escape(file)} \\(${entry}\\); refusing to point a consumable ref at an unpackaged commit; run the build before packaging\\.$`, ); expect(() => packageCommit({ cwd: fx.work, sourceSha: fx.mergeSha })).toThrow(message); expect(() => packageRelease({ cwd: fx.work, tag: "v2.1.0", sourceSha: fx.mergeSha })).toThrow( diff --git a/test/sections/docs-registry.test.ts b/test/sections/docs-registry.test.ts index 79144b873..3d388c207 100644 --- a/test/sections/docs-registry.test.ts +++ b/test/sections/docs-registry.test.ts @@ -2,7 +2,6 @@ import { describe, expect, test } from "bun:test"; import { existsSync, readdirSync, readFileSync, statSync, writeFileSync } from "node:fs"; import { dirname, join, relative } from "node:path"; import { ok } from "neverthrow"; -import { escapeRe } from "../../.github/scripts/lib/generated-regions.js"; import { SECTION_KEYS, type SectionKey } from "../../src/schema.js"; import { readDocsYaml, SectionDocs } from "../../src/sections/contract/docs.js"; import { endpointPath, type Route } from "../../src/sections/contract/endpoints.js"; @@ -14,7 +13,7 @@ import { withTempDir } from "../temp-dir.js"; // Whole-identifier, case-insensitive: "the pinEnvironment mutation" names PinEnvironment, "DocumentPinEnvironmentAudit" does not. function namesOperation(prose: string, name: string): boolean { - return new RegExp(`(? { "at coverage[0]", ]) { expect(readDocsYaml(malformed, SectionDocs)._unsafeUnwrapErr()).toMatch( - new RegExp(escapeRe(issue)), + new RegExp(RegExp.escape(issue)), ); } // The tail of a missing-file error is the runtime's ENOENT prose, so only our prefix is pinned. const absent = join(dir, "absent.yml"); expect(readDocsYaml(absent, SectionDocs)._unsafeUnwrapErr()).toMatch( - new RegExp(`^${escapeRe(`${absent} is not valid YAML: `)}`), + new RegExp(`^${RegExp.escape(`${absent} is not valid YAML: `)}`), ); // YAML that does not even parse (a duplicated key, which the loader refuses) names the file too. writeFileSync(malformed, ["sections_table:", " endpoints: a", " endpoints: b"].join("\n")); expect(readDocsYaml(malformed, SectionDocs)._unsafeUnwrapErr()).toMatch( - new RegExp(`${escapeRe(malformed)} is not valid YAML: .*unique`), + new RegExp(`${RegExp.escape(malformed)} is not valid YAML: .*unique`), ); // Control: the same reader accepts a well-formed document. writeFileSync(malformed, WELL_FORMED_DOCS.join("\n")); @@ -264,7 +263,7 @@ describe("Endpoints cells vs declared operations", () => { ]; const cell = normalize(DOCS[endpoint.section].sections_table.endpoints); expect( - variants.some((variant) => new RegExp(`\\b${escapeRe(variant)}\\b`).test(cell)), + variants.some((variant) => new RegExp(`\\b${RegExp.escape(variant)}\\b`).test(cell)), `the ${endpoint.section} Endpoints cell never mentions "${needle}" from endpoint ${endpoint.route}`, ).toBe(true); } diff --git a/tsconfig.json b/tsconfig.json index 2c25838e0..39b30b0d7 100644 --- a/tsconfig.json +++ b/tsconfig.json @@ -3,7 +3,7 @@ "target": "ES2022", "module": "ESNext", "moduleResolution": "bundler", - "lib": ["ES2022"], + "lib": ["ES2025"], "types": ["node", "bun"], "strict": true, "noUncheckedIndexedAccess": true, From b8020b6d9c3dd889446f9ba09b0fcb2c317f719d Mon Sep 17 00:00:00 2001 From: Vivswan Shah <58091053+Vivswan@users.noreply.github.com> Date: Tue, 22 Sep 2026 05:45:21 -0400 Subject: [PATCH 2/2] refactor: return neverthrow results instead of hand-rolled ok/err unions validateSecretRef returns Result and resolveSecretRefs returns Result, replacing the SecretRefCheck and SecretRefsResolution unions. redactSecretPayloadSafe returns Result, the undefined error being the case that had no reason field: a non-JSON primitive or a hostile throw. parsePublicKey and parseStoredKey return Result, replacing the PublicKeyParse union. Every consumer reads isErr(), value, and error; every message is unchanged. --- src/engine/orchestrate.ts | 8 +-- src/engine/secret-refs.ts | 59 ++++++++----------- src/engine/secrets.ts | 2 +- src/engine/validate.ts | 2 +- src/github/api.ts | 20 +++---- src/github/secret-scan.ts | 34 ++++++----- src/sections/deploy_keys/index.ts | 12 ++-- src/sections/deploy_keys/schema.ts | 24 ++++---- test/engine/secret-refs.test.ts | 34 ++++------- test/engine/secrets.test.ts | 8 +-- test/sections/deploy_keys/deploy_keys.test.ts | 46 ++++++--------- 11 files changed, 117 insertions(+), 132 deletions(-) diff --git a/src/engine/orchestrate.ts b/src/engine/orchestrate.ts index 6c87e8462..3d309f59c 100644 --- a/src/engine/orchestrate.ts +++ b/src/engine/orchestrate.ts @@ -308,12 +308,12 @@ export async function runForRepo( const mask = new Set(); for (const [key, names] of bySection) { const resolution = resolveSecretRefs(names, env); - if (!resolution.ok) { - resolutionErrors.set(key, resolution.errors); + if (resolution.isErr()) { + resolutionErrors.set(key, resolution.error); continue; } - Object.assign(resolved, resolution.values); - for (const plaintext of resolution.mask) { + Object.assign(resolved, resolution.value.values); + for (const plaintext of resolution.value.mask) { mask.add(plaintext); } } diff --git a/src/engine/secret-refs.ts b/src/engine/secret-refs.ts index f9245ea94..231c82d70 100644 --- a/src/engine/secret-refs.ts +++ b/src/engine/secret-refs.ts @@ -9,16 +9,16 @@ * a reference in a target's file -> refused: a target must not route the operator's environment into itself */ +import { err, ok, type Result } from "neverthrow"; + /** Who authored the source: a `target` document (fetched from the target repository) has its references refused; flows/multi.ts decides. */ export type SettingsSource = "operator" | "target"; /** A syntactically valid reference: the env var name, without the `$`. */ -interface SecretRef { +export interface SecretRef { readonly name: string; } -export type SecretRefCheck = { ok: true; ref: SecretRef } | { ok: false; error: string }; - const REFERENCE_RE = /^\$[A-Z_][A-Z0-9_]*$/; const EMBEDDED_REFERENCE_RE = /\$[A-Z_][A-Z0-9_]*/; @@ -38,46 +38,39 @@ export function validateSecretRef( value: string, source: SettingsSource, label: string, -): SecretRefCheck { +): Result { if (REFERENCE_RE.test(value)) { const name = value.slice(1); if (source === "target") { - return { - ok: false, - error: `${label} uses the secret reference ${value} in a target-fetched settings file; references are honored only in operator-owned settings sources, so a target repository cannot read the operator's environment`, - }; + return err( + `${label} uses the secret reference ${value} in a target-fetched settings file; references are honored only in operator-owned settings sources, so a target repository cannot read the operator's environment`, + ); } const reserved = RESERVED_REF_PREFIXES.find((prefix) => name.startsWith(prefix)); if (reserved !== undefined) { - return { - ok: false, - error: `${label} references the reserved runner variable ${value} (${reserved}* is refused): workflow inputs and GitHub/runner context cannot be routed into settings values`, - }; + return err( + `${label} references the reserved runner variable ${value} (${reserved}* is refused): workflow inputs and GitHub/runner context cannot be routed into settings values`, + ); } - return { ok: true, ref: { name } }; + return ok({ name }); } const embedded = value.match(EMBEDDED_REFERENCE_RE)?.[0]; if (embedded !== undefined) { - return { - ok: false, - error: `${label} embeds ${embedded} without being a whole-value reference; it would otherwise ship verbatim as the secret. Make the entire value a single $NAME reference`, - }; + return err( + `${label} embeds ${embedded} without being a whole-value reference; it would otherwise ship verbatim as the secret. Make the entire value a single $NAME reference`, + ); } - return { - ok: false, - error: `${label} carries a literal value, but settings files are committed plaintext - exactly what secret references exist to prevent. Set it to a whole-value $NAME reference and define NAME in the step's env block`, - }; + return err( + `${label} carries a literal value, but settings files are committed plaintext - exactly what secret references exist to prevent. Set it to a whole-value $NAME reference and define NAME in the step's env block`, + ); } -export type SecretRefsResolution = - | { - ok: true; - /** Resolved plaintext, keyed by env var name (two fields may share one). */ - values: Record; - /** Every distinct plaintext value, for the caller to register with masking. */ - mask: string[]; - } - | { ok: false; errors: string[] }; +export interface ResolvedSecretRefs { + /** Resolved plaintext, keyed by env var name (two fields may share one). */ + values: Record; + /** Every distinct plaintext value, for the caller to register with masking. */ + mask: string[]; +} /** The variable a validated whole-value reference names: REFERENCE_RE admits `$NAME`, so the name follows the `$`. */ export function referenceName(reference: string): string { @@ -94,7 +87,7 @@ export function referenceName(reference: string): string { export function resolveSecretRefs( names: readonly string[], env: Record = process.env, -): SecretRefsResolution { +): Result { const errors: string[] = []; const resolved: Record = {}; const mask = new Set(); @@ -116,7 +109,7 @@ export function resolveSecretRefs( mask.add(plaintext); } if (errors.length > 0) { - return { ok: false, errors }; + return err(errors); } - return { ok: true, values: resolved, mask: [...mask] }; + return ok({ values: resolved, mask: [...mask] }); } diff --git a/src/engine/secrets.ts b/src/engine/secrets.ts index 87b36587f..13a056fcf 100644 --- a/src/engine/secrets.ts +++ b/src/engine/secrets.ts @@ -50,7 +50,7 @@ export interface MintedSecretReference { function mintSecretReference(variable: string, label: string): MintedSecretReference { const reference = `$${variable}`; const checked = validateSecretRef(reference, "operator", label); - if (!checked.ok) { + if (checked.isErr()) { throw new Error( `BUG: the snapshot minted a reference the settings file refuses: ${checked.error}`, ); diff --git a/src/engine/validate.ts b/src/engine/validate.ts index ffe62af3e..34a919ce0 100644 --- a/src/engine/validate.ts +++ b/src/engine/validate.ts @@ -281,7 +281,7 @@ function secretReferenceProblems( const problems: string[] = []; for (const { label, value } of module.secretValues?.(parsed) ?? []) { const checked = validateSecretRef(value, source, label); - if (!checked.ok) { + if (checked.isErr()) { problems.push(`${key}: ${checked.error}`); } } diff --git a/src/github/api.ts b/src/github/api.ts index 86092e27a..fd693bb13 100644 --- a/src/github/api.ts +++ b/src/github/api.ts @@ -480,9 +480,9 @@ export class GitHubApi implements GitHubClient { // cannot be normalized is never sent; sending what the scan could not inspect would let a stateful object show the // scan one thing and the wire another. const secretScan = redactSecretPayloadSafe(payload); - if (!secretScan.ok) { + if (secretScan.isErr()) { const reason = - secretScan.reason ?? + secretScan.error ?? "its payload is not plain JSON data (a value carrying a function or exotic prototype)"; return { failed: `${method} ${path} was not sent: ${reason}, so it could not be safely inspected for secret fields. Replace that value with a plain string in the settings file`, @@ -490,7 +490,7 @@ export class GitHubApi implements GitHubClient { } // Either signal withholds: the caller's mark knows the value's origin, the scan knows the wire's field names. const marked = options?.carriesSecret === true; - const carriesSecret = marked || secretScan.carriesSecret; + const carriesSecret = marked || secretScan.value.carriesSecret; const trace = (status: number): void => { const safe = this.trace.path(path); this.trace.debug( @@ -499,7 +499,7 @@ export class GitHubApi implements GitHubClient { ? "" : marked ? ` payload: ${MARKED_PAYLOAD_TRACE}` - : ` payload: ${JSON.stringify(secretScan.traced)}`), + : ` payload: ${JSON.stringify(secretScan.value.traced)}`), ); }; try { @@ -511,7 +511,7 @@ export class GitHubApi implements GitHubClient { "x-github-api-version": this.apiVersion, }, // The body is the tree the scan inspected, so octokit never reshapes the payload and the wire carries exactly what was scanned. - ...(payload === undefined ? {} : { data: secretScan.payload }), + ...(payload === undefined ? {} : { data: secretScan.value.payload }), } as unknown as Parameters["request"]>[0]); trace(response.status); const data = response.data as unknown; @@ -552,21 +552,21 @@ export class GitHubApi implements GitHubClient { const started = Date.now(); // The same one-serialization contract as tryRequest, so a future secret-bearing variable is masked and withheld like a REST payload field. const scan = redactSecretPayloadSafe(variables); - if (!scan.ok) { + if (scan.isErr()) { const reason = - scan.reason ?? + scan.error ?? "its variables are not plain JSON data (a value carrying a function or exotic prototype)"; return { failed: `GRAPHQL ${op.name} was not sent: ${reason}, so they could not be safely inspected for secret fields. Replace that value with a plain string in the settings file`, }; } const marked = options?.carriesSecret === true; - const carriesSecret = marked || scan.carriesSecret; + const carriesSecret = marked || scan.value.carriesSecret; // Read live at every emission, never snapshotted at request start: a mask registered mid-flight must redact what follows. const redacted = (): boolean => this.trace.isRedacted(slug); // The operation addresses its repository in the BODY, which the path redactor never sees: a redacted slug collapses // the ENTIRE line, since the variables carry the repository's live state. - const tracedVariables = marked ? MARKED_PAYLOAD_TRACE : JSON.stringify(scan.traced); + const tracedVariables = marked ? MARKED_PAYLOAD_TRACE : JSON.stringify(scan.value.traced); const trace = (status: number, suffix = ""): void => { this.trace.debug( redacted() @@ -591,7 +591,7 @@ export class GitHubApi implements GitHubClient { "x-github-api-version": this.apiVersion, }, // operationName makes the request self-describing on the wire (the mock dispatches on it). - data: { query: op.query, operationName: op.name, variables: scan.payload }, + data: { query: op.query, operationName: op.name, variables: scan.value.payload }, } as unknown as Parameters["request"]>[0])) as { status: number; data: unknown; diff --git a/src/github/secret-scan.ts b/src/github/secret-scan.ts index c5a49fcf1..847bbfdbd 100644 --- a/src/github/secret-scan.ts +++ b/src/github/secret-scan.ts @@ -1,9 +1,10 @@ /** - * Plain-data normalization and secret-field scanning for outgoing payloads, dependency-free on purpose: the guarantees + * Plain-data normalization and secret-field scanning for outgoing payloads, transport-free on purpose: the guarantees * (no payload-supplied method or accessor is ever invoked, the scanned tree IS the sent tree, secret fields are masked * in traces) must hold independent of any transport. */ +import { err, ok, type Result } from "neverthrow"; import { isPlainObject, nonPlainKind } from "../plain-data.js"; const SECRET_FIELD_PLACEHOLDER = "***"; @@ -128,21 +129,26 @@ function normalizePlainData( return out; } +/** The normalized tree the request sends, the masked copy the trace prints, and whether a secret field was seen. */ +export interface SafePayload { + payload: unknown; + traced: unknown; + carriesSecret: boolean; +} + /** * One read, one truth: normalizePlainData reads the input once into a plain-data tree; the scan walks it, the trace * prints it masked, and the request SENDS it, so no exotic object can make the scan, the trace, and the wire disagree. * * undefined (no body), or a JSON primitive -> passes through: no named fields, and a bare-value secret is unsupported by design * plain objects and arrays throughout -> normalized, scanned, sent - * a non-plain value, at any depth -> `ok: false`, never sent: normalizing a non-plain container would change what reaches fetch + * a non-plain value, at any depth -> an error, never sent: normalizing a non-plain container would change what reaches fetch + * + * The error is the rejection's prose, or undefined where no safe prose exists (a non-JSON primitive, a hostile throw). */ -export function redactSecretPayloadSafe( - payload: unknown, -): - | { ok: true; payload: unknown; traced: unknown; carriesSecret: boolean } - | { ok: false; reason?: string } { +export function redactSecretPayloadSafe(payload: unknown): Result { if (payload === undefined) { - return { ok: true, payload: undefined, traced: undefined, carriesSecret: false }; + return ok({ payload: undefined, traced: undefined, carriesSecret: false }); } // Everything reflective happens INSIDE the try: even Array.isArray can throw on a hostile proxy, and an error thrown // before the guard could carry a secret in its message. @@ -155,22 +161,22 @@ export function redactSecretPayloadSafe( typeof payload === "boolean" || (typeof payload === "number" && Number.isFinite(payload)); return jsonPrimitive - ? { ok: true, payload, traced: payload, carriesSecret: false } - : { ok: false }; + ? ok({ payload, traced: payload, carriesSecret: false }) + : err(undefined); } if (!isPlainJsonContainer(payload)) { - return { ok: false, reason: describeNotPlain(new NotPlainData([], nonPlainKind(payload))) }; + return err(describeNotPlain(new NotPlainData([], nonPlainKind(payload)))); } const normalized = normalizePlainData(payload); if (normalized instanceof NotPlainData) { // Only our own rejection contributes prose: it carries key PATHS and a value-class word, never a value. - return { ok: false, reason: describeNotPlain(normalized) }; + return err(describeNotPlain(normalized)); } const scanned = redactSecretPayload(normalized); - return { ok: true, payload: normalized, ...scanned }; + return ok({ payload: normalized, ...scanned }); } catch { // A hostile object threw from a reflective read; its message never leaks. - return { ok: false }; + return err(undefined); } } diff --git a/src/sections/deploy_keys/index.ts b/src/sections/deploy_keys/index.ts index 9671ceff9..9321d7e73 100644 --- a/src/sections/deploy_keys/index.ts +++ b/src/sections/deploy_keys/index.ts @@ -29,15 +29,15 @@ const LiveDeployKey = z }) .transform((live, refineCtx) => { const parsed = parseStoredKey(live.key); - if (!parsed.ok) { + if (parsed.isErr()) { refineCtx.addIssue({ code: "custom", path: ["key"], - message: `key id ${String(live.id)} ("${live.title}") holds material that is not " ": ${parsed.reason}`, + message: `key id ${String(live.id)} ("${live.title}") holds material that is not " ": ${parsed.error}`, }); return z.NEVER; } - return { ...live, key: parsed.material, algorithm: parsed.algorithm }; + return { ...live, key: parsed.value.material, algorithm: parsed.value.algorithm }; }); const ENDPOINTS = { @@ -62,12 +62,12 @@ const ENDPOINTS = { /** The shape refused malformed material through the same parsePublicKey before plan() ran, so reaching one here is a bug. */ function declaredMaterial(title: string, key: string): string { const parsed = parsePublicKey(key); - if (!parsed.ok) { + if (parsed.isErr()) { throw new Error( - `BUG: deploy_keys[${title}] reached plan() with material the shape refuses (${parsed.reason}); the shape refuses it first`, + `BUG: deploy_keys[${title}] reached plan() with material the shape refuses (${parsed.error}); the shape refuses it first`, ); } - return parsed.material; + return parsed.value.material; } export const deployKeysSection = listSection({ diff --git a/src/sections/deploy_keys/schema.ts b/src/sections/deploy_keys/schema.ts index 270f2b3a9..ad55fbe93 100644 --- a/src/sections/deploy_keys/schema.ts +++ b/src/sections/deploy_keys/schema.ts @@ -1,5 +1,6 @@ /** The `deploy_keys:` section's schema slice; root src/schema.ts composes the SettingsFile property from it. */ +import { err, ok, type Result } from "neverthrow"; import { z } from "zod"; /** @@ -73,10 +74,11 @@ const BLOB_NOT_BASE64 = */ const PRIVATE_KEY_FRAMING = ["PRIVATE", "KEY-----"].join(" "); -export type PublicKeyParse = - /** `material` is the comparable form, algorithm + blob: GitHub may strip or rewrite the comment on storage. */ - | { readonly ok: true; readonly algorithm: string; readonly material: string } - | { readonly ok: false; readonly reason: string }; +/** `material` is the comparable form, algorithm + blob: GitHub may strip or rewrite the comment on storage. */ +export interface PublicKeyMaterial { + readonly algorithm: string; + readonly material: string; +} /** * Names what PUBLIC_KEY_PATTERN refused. The reason never quotes the input: it may be a pasted PRIVATE key, and a @@ -133,20 +135,20 @@ function parseMaterial( pattern: RegExp, raw: string, rejection: (raw: string) => string, -): PublicKeyParse { +): Result { const match = pattern.exec(raw); return match === null - ? { ok: false, reason: rejection(raw) } - : { ok: true, algorithm: match[1] ?? "", material: `${match[1]} ${match[2]}` }; + ? err(rejection(raw)) + : ok({ algorithm: match[1] ?? "", material: `${match[1]} ${match[2]}` }); } /** The one reading of DECLARED deploy key material, shared by the schema and the planner so the two cannot disagree. */ -export function parsePublicKey(raw: string): PublicKeyParse { +export function parsePublicKey(raw: string): Result { return parseMaterial(PUBLIC_KEY_PATTERN, raw, declaredRejection); } /** The reading of material GitHub returned: the same comparable form, with GitHub the authority on the algorithm. */ -export function parseStoredKey(raw: string): PublicKeyParse { +export function parseStoredKey(raw: string): Result { return parseMaterial(STORED_KEY_PATTERN, raw, storedRejection); } @@ -165,12 +167,12 @@ export const DeployKeyConfig = z return; } const parsed = parsePublicKey(key); - if (!parsed.ok) { + if (parsed.isErr()) { const who = typeof entry.title === "string" ? `entry "${entry.title}"` : "this entry"; refineCtx.addIssue({ code: "custom", path: ["key"], - message: `${who}: ${parsed.reason}`, + message: `${who}: ${parsed.error}`, }); } }) diff --git a/test/engine/secret-refs.test.ts b/test/engine/secret-refs.test.ts index 47e7df5dc..f7a784558 100644 --- a/test/engine/secret-refs.test.ts +++ b/test/engine/secret-refs.test.ts @@ -1,4 +1,5 @@ import { describe, expect, test } from "bun:test"; +import { ok } from "neverthrow"; import { RESERVED_REF_PREFIXES, resolveSecretRefs, @@ -86,11 +87,10 @@ describe("validateSecretRef (syntax phase, never reads the environment)", () => ])("%s", (_what, value, source, verdict) => { const result = validateSecretRef(value, source, LABEL); if ("name" in verdict) { - expect(result).toEqual({ ok: true, ref: { name: verdict.name } }); + expect(result).toEqual(ok({ name: verdict.name })); return; } - expect(result.ok).toBe(false); - if (result.ok) { + if (result.isOk()) { throw new Error("expected rejection"); } for (const word of verdict.says) { @@ -104,8 +104,7 @@ describe("validateSecretRef (syntax phase, never reads the environment)", () => test("every reserved prefix is refused, naming the prefix", () => { for (const prefix of RESERVED_REF_PREFIXES) { const result = validateSecretRef(`$${prefix}SOMETHING`, "operator", LABEL); - expect(result.ok).toBe(false); - if (result.ok) { + if (result.isOk()) { throw new Error("expected rejection"); } expect(result.error).toContain(`${prefix}*`); @@ -118,12 +117,9 @@ describe("resolveSecretRefs (resolution phase over validated names, injected env const result = resolveSecretRefs(["WEBHOOK_SECRET"], { WEBHOOK_SECRET: "s3cret-value", }); - expect(result.ok).toBe(true); - if (!result.ok) { - throw new Error(result.errors.join("; ")); - } - expect(result.values.WEBHOOK_SECRET).toBe("s3cret-value"); - expect(result.mask).toEqual(["s3cret-value"]); + expect(result).toEqual( + ok({ values: { WEBHOOK_SECRET: "s3cret-value" }, mask: ["s3cret-value"] }), + ); }); test("two variables holding the same plaintext mask it once", () => { @@ -131,12 +127,9 @@ describe("resolveSecretRefs (resolution phase over validated names, injected env FIRST_NAME: "identical", SECOND_NAME: "identical", }); - expect(result.ok).toBe(true); - if (!result.ok) { - throw new Error(result.errors.join("; ")); - } - expect(result.values).toEqual({ FIRST_NAME: "identical", SECOND_NAME: "identical" }); - expect(result.mask).toEqual(["identical"]); + expect(result).toEqual( + ok({ values: { FIRST_NAME: "identical", SECOND_NAME: "identical" }, mask: ["identical"] }), + ); }); // Each row: the names to resolve, the environment, and per failed reference the words its error must say. @@ -164,14 +157,13 @@ describe("resolveSecretRefs (resolution phase over validated names, injected env ], ])("%s", (_what, names, env, errors) => { const result = resolveSecretRefs(names, env); - expect(result.ok).toBe(false); - if (result.ok) { + if (result.isOk()) { throw new Error("expected failure"); } - expect(result.errors).toHaveLength(errors.length); + expect(result.error).toHaveLength(errors.length); errors.forEach((words, index) => { for (const word of words) { - expect(result.errors[index]).toContain(word); + expect(result.error[index]).toContain(word); } }); }); diff --git a/test/engine/secrets.test.ts b/test/engine/secrets.test.ts index 6d22217e7..92cf75878 100644 --- a/test/engine/secrets.test.ts +++ b/test/engine/secrets.test.ts @@ -1,4 +1,5 @@ import { describe, expect, test } from "bun:test"; +import { ok } from "neverthrow"; import { type ValidatedSettings, validateSettingsDoc } from "../../src/engine/orchestrate.js"; import { type SettingsSource, validateSecretRef } from "../../src/engine/secret-refs.js"; import { collectSecretReferences, snapshotSecretReference } from "../../src/engine/secrets.js"; @@ -127,10 +128,9 @@ describe("snapshotSecretReference", () => { ); // Every minted reference passes the grammar the settings file enforces. for (const { reference } of inStore) { - expect(validateSecretRef(reference, "operator", "x")).toEqual({ - ok: true, - ref: { name: reference.slice(1) }, - }); + expect(validateSecretRef(reference, "operator", "x")).toEqual( + ok({ name: reference.slice(1) }), + ); } }); }); diff --git a/test/sections/deploy_keys/deploy_keys.test.ts b/test/sections/deploy_keys/deploy_keys.test.ts index 3fbc2a39a..53e148ee2 100644 --- a/test/sections/deploy_keys/deploy_keys.test.ts +++ b/test/sections/deploy_keys/deploy_keys.test.ts @@ -1,4 +1,5 @@ import { describe, expect, test } from "bun:test"; +import { ok } from "neverthrow"; import type { SectionInput } from "../../../src/sections/contract/module.js"; import { planContext } from "../../../src/sections/contract/plan.js"; import { deployKeysSection } from "../../../src/sections/deploy_keys/index.js"; @@ -49,13 +50,11 @@ const plan = async (api: MockApi, desired: SectionInput<"deploy_keys">) => describe("parsePublicKey", () => { test("the comparable material is algorithm + blob: GitHub strips the comment on storage, so a raw compare would recreate on every apply", () => { - const bot = { ok: true, algorithm: "ssh-ed25519", material: BOT_KEY } as const; + const bot = ok({ algorithm: "ssh-ed25519", material: BOT_KEY }); expect(parsePublicKey(`${BOT_KEY} deploy@host`)).toEqual(bot); - expect(parsePublicKey(` ${RETIRED_KEY} a b c `)).toEqual({ - ok: true, - algorithm: "ssh-rsa", - material: RETIRED_KEY, - }); + expect(parsePublicKey(` ${RETIRED_KEY} a b c `)).toEqual( + ok({ algorithm: "ssh-rsa", material: RETIRED_KEY }), + ); expect(parsePublicKey(BOT_KEY)).toEqual(bot); }); @@ -70,7 +69,7 @@ describe("parsePublicKey", () => { `ssh-ed25519\tAAAAC3NzaC1lZDI1NTE5AAAAIBotBotBotBotBotBotBotBotBotBotBotBotBotBotB\tdeploy@host`, ], ])("%s is a public key GitHub accepts, so the parse keeps it", (_label, raw) => { - expect(parsePublicKey(raw).ok).toBe(true); + expect(parsePublicKey(raw).isOk()).toBe(true); }); // Every refusal is checked for the ABSENCE of the input: a pasted private key must not surface in a log. @@ -148,34 +147,27 @@ describe("parsePublicKey", () => { ], ])("%s is refused with a reason that never quotes the input", (_label, raw, reason) => { const parsed = parsePublicKey(raw); - expect(parsed.ok).toBe(false); - if (parsed.ok) { - return; + if (parsed.isOk()) { + throw new Error("expected a refusal"); } - expect(parsed.reason).toMatch(reason); + expect(parsed.error).toMatch(reason); // The reason may name the accepted algorithms; every other field of the input stays out of it. const algorithms = new Set(PUBLIC_KEY_ALGORITHMS); for (const field of raw .split(/\s+/) .filter((field) => field !== "" && !algorithms.has(field))) { - expect(parsed.reason).not.toContain(field); + expect(parsed.error).not.toContain(field); } }); }); describe("parseStoredKey", () => { test("material GitHub stored is read as algorithm + blob under ANY algorithm: GitHub is the authority on what it accepted", () => { - expect(parseStoredKey(ED448_KEY)).toEqual({ - ok: true, - algorithm: "ssh-ed448", - material: ED448_KEY, - }); - expect(parseStoredKey(`${BOT_KEY} deploy@host`)).toEqual({ - ok: true, - algorithm: "ssh-ed25519", - material: BOT_KEY, - }); - expect(parsePublicKey(ED448_KEY).ok).toBe(false); + expect(parseStoredKey(ED448_KEY)).toEqual(ok({ algorithm: "ssh-ed448", material: ED448_KEY })); + expect(parseStoredKey(`${BOT_KEY} deploy@host`)).toEqual( + ok({ algorithm: "ssh-ed25519", material: BOT_KEY }), + ); + expect(parsePublicKey(ED448_KEY).isErr()).toBe(true); }); test.each<[label: string, raw: string, reason: RegExp]>([ @@ -194,11 +186,11 @@ describe("parseStoredKey", () => { ], ])("%s is refused as a shape violation, never over the algorithm", (_label, raw, reason) => { const parsed = parseStoredKey(raw); - expect(parsed.ok).toBe(false); - if (!parsed.ok) { - expect(parsed.reason).toMatch(reason); - expect(parsed.reason).not.toContain("algorithm GitHub accepts"); + if (parsed.isOk()) { + throw new Error("expected a refusal"); } + expect(parsed.error).toMatch(reason); + expect(parsed.error).not.toContain("algorithm GitHub accepts"); }); });