Repository navigation
refactor(errors): neverthrow Results with typed problems across validation, config, fold, flows, and the library - #145
Merged
Merged
Conversation
Vivswan
force-pushed
the
refactor/neverthrow
branch
from
September 11, 2026 23:20
0b2f6b7 to
c9dfe69
Compare
Contributor
File size check0 over a hard cap (fails), 90 warning(s).
Split the file, wrap the line, shorten or exempt the comment, or list the path in 5 managed file(s) skipped; repo-platform owns them. |
Vivswan
force-pushed
the
refactor/flows-library
branch
from
September 12, 2026 01:11
649afb3 to
801c7ec
Compare
Vivswan
force-pushed
the
refactor/neverthrow
branch
from
September 12, 2026 01:41
8c0e190 to
d44b8cb
Compare
added 6 commits
September 11, 2026 21:42
The Result library the run flows, validation, config parsing, and the layer fold return their problems through.
…validation, config, fold, flows, and the library
Every failure outside the sections' API-call channel is now a member of one `Problem` union (src/problem.ts, discriminated on `code`) returned as a neverthrow `Result`/`ResultAsync` instead of a thrown domain error or a `{error: string}` shape. `describeProblem` is the one renderer; the action edge (`failRun`, `run()`) renders there, so every line the action prints, every output, summary, and exit code is unchanged (the e2e corpus is green as it was).
Converted: `validateSectionShapes`, `validateSettingsDoc`, `mergeLayers` (the admission walk returns Results; the `LayerRefusal` throw and `describeRefusal` are gone, their prose lives in `describeProblem` as the `layer-*` cases under the same value-free invariant), `parseSettingsDoc`, `readSettingsFile` (now takes the file's role, which selects the advice), `readLayerFiles`, `foldLayers`, `runSingle`, `runMulti` (a `safeTry` async generator; `orTee` flushes the buffered central warnings on the fatal paths), `runMerge` (returns the finished merge; `concludeMerge` writes the summary), `parseConfig` (`safeTry`), `parseReposInput`, `parseRepoSlug`, `resolveCentralTargets`, `discoverRepos` (`ResultAsync.fromPromise`), `parseRecipient`, and the library's `validateSettings`.
`isPlainObject` moves to src/plain-data.ts so the renderer and the fold share it; `quoteList` moves to src/problem.ts. `ARTIFACT_NEEDS_UPLOADER` is gone: `requireUploader` returns the `artifact-uploader-missing` problem and `openReportChannel`'s backstop throw renders it. `architecture.yml` gains the `problem` layer (edges from engine, flows, discovery, report, library; problem reads github and plain-data).
Tests assert on `code` and the structured payload; test/problem.test.ts renders one specimen per member, typed over every code so a new member fails compilation until it has both a case and a specimen.
The fleet's file-size check caps every source and test line at 256 code points. The over-long string and template literals this branch owns (the problem renderer and its specimens, the input declarations, two test expectations) are split into `+` concatenations that reproduce the same characters. One specimen's deliberately misspelled section name is renamed from `tpyo` to `nope`, which the fleet's typos check does not read as a misspelling.
…put declarations into the library
`sectionSelection({only, required})` in src/engine/section-selection.ts is the one construction site of the branded `SectionSelection` that `RepoRunOptions` and `RunFlowConfig` now carry in place of two loose sets. A required section outside a non-empty allowlist is refused there, so the action's parser and a library caller of `checkRepository`/`applyRepository` can no longer disagree: the engine marks an excluded section without attempting it, and the old pair let a library call pass green having proven nothing. The problem is `required-sections-excluded`, rendered to the same line as before.
The input declarations (INPUT_DECLS, InputDecl, InputName, MODES, MERGE_INPUTS, MERGE_REJECTED_INPUTS, FILTER_INPUTS, RunConfig) and `parseConfig` move from src/action/inputs.ts to src/flows/inputs.ts. `parseConfig(read, env)` reads every input through a `(name: InputName) => string` port and the four GITHUB_* keys from an environment record, so a CLI reads the same declarations the action does; the action keeps `parseActionConfig()`, the adapter over `@actions/core` and process.env. INPUT_DECLS is unchanged byte for byte. The docs generator and the tests that import the declarations follow the move.
Tests drive `parseConfig` through a record-backed port instead of the process environment; the adapter keeps one env-driven test; the library test shows the contradictory selection refused before a check can be given one.
…and read layers sequentially RERUN_ADVICE moves from src/github/api.ts, which only defined it, into src/problem.ts: the renderer is the one place that words it, and importing it from the client module made the leaf problem module load Octokit. multi.ts and the public entry read it from problem.ts; architecture.yml drops the problem -> github edge. readLayerFiles folds the layer paths sequentially so the first unreadable layer stops the read, as the base did, instead of reading every layer before returning the first problem.
…n's nominal typing parseConfig trims each value once, at the read port, so an action port (whose getInput trims already) and a CLI port read identically: a whitespace-only token is unset and the environment token applies, instead of a blank token beating it and failing as missing. test/engine/section-selection.test.ts gains two @ts-expect-error guards asserting that a spread of SectionSelection.ALL and a plain literal are not SectionSelections. The runtime test alone would pass silently if the fields became public; the directives then report unused and fail the build.
There was a problem hiding this comment.
🟡 Changes recommended
Managed guidance and generated documentation contain updates that will be overwritten or direct contributors to the wrong source file.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
Refactors error handling to typed neverthrow results, introduces a public library surface, and separates reusable flows from the GitHub Actions adapter.
Changes:
- Adds typed
Problemresults and centralized rendering. - Moves reusable execution, input, redaction, and delivery logic into
src/flows/. - Adds
SectionSelection, library APIs, and expanded tests.
File summaries
| File | Description |
|---|---|
src/index.ts |
Defines the public API. |
src/problem.ts |
Defines and renders typed problems. |
src/types.ts |
Adds distributive omit utility. |
src/plain-data.ts |
Centralizes plain-object detection. |
src/io.ts |
Adds collecting and silent IO implementations. |
src/github/api.ts |
Moves shared retry advice. |
src/engine/validate.ts |
Returns typed validation results. |
src/engine/orchestrate.ts |
Uses typed problems and section selections. |
src/engine/layers.ts |
Returns typed layer failures. |
src/engine/section-selection.ts |
Adds validated section selection. |
src/engine/secrets.ts |
Updates secret-reference ownership. |
src/engine/secret-refs.ts |
Moves secret reference logic into the engine. |
src/discovery/targets.ts |
Returns typed slug parsing results. |
src/discovery/repos-input.ts |
Returns structured input failures. |
src/discovery/discover.ts |
Converts discovery to ResultAsync. |
src/discovery/central.ts |
Structures central-target failures. |
src/flows/inputs.ts |
Houses reusable configuration parsing. |
src/flows/settings-read.ts |
Returns typed file and YAML failures. |
src/flows/layers.ts |
Composes layer reads, merges, and validation. |
src/flows/merge.ts |
Implements the reusable merge flow. |
src/flows/single.ts |
Implements the reusable single-repo flow. |
src/flows/multi.ts |
Implements multi-repo orchestration. |
src/flows/library.ts |
Adds library-friendly engine APIs. |
src/flows/redact.ts |
Moves shared redaction behavior. |
src/flows/deliver.ts |
Centralizes flow completion and delivery. |
src/flows/summary.ts |
Moves summary rendering into flows. |
src/report/artifact-report.ts |
Makes artifact upload an explicit port. |
src/report/delivery.ts |
Owns report channel and detail types. |
src/action/artifact.ts |
Implements the Actions artifact adapter. |
src/action/inputs.ts |
Adapts Actions inputs to parseConfig. |
src/action/run.ts |
Delegates execution to reusable flows. |
src/action/io.ts |
Imports IO contracts through the public entry. |
src/action/settings-read.ts |
Removes the old action-local reader. |
src/action/merge-mode.ts |
Removes the old action-local merge flow. |
src/sections/actions/actions.test.ts |
Updates validation-result assertions. |
src/sections/actions_secrets/actions_secrets.test.ts |
Uses typed validation and selections. |
src/sections/repository/repository.test.ts |
Uses centralized problem rendering. |
src/sections/secret_scanning_custom_patterns/secret_scanning_custom_patterns.test.ts |
Updates validation-result handling. |
src/sections/webhooks/webhooks.test.ts |
Uses centralized problem rendering. |
test/library/public-surface.test.ts |
Pins public exports and architecture. |
test/io/collecting.test.ts |
Covers reusable IO implementations. |
test/io-fake.ts |
Removes the superseded test helper. |
test/flows/inputs.test.ts |
Covers reusable configuration parsing. |
test/flows/settings-read.test.ts |
Covers typed YAML parsing. |
test/flows/layers.test.ts |
Covers typed layer reads. |
test/flows/merge.test.ts |
Covers merge results and collisions. |
test/flows/single.test.ts |
Covers single-flow results. |
test/flows/library.test.ts |
Covers public library operations. |
test/flows/redact.test.ts |
Updates moved redaction imports. |
test/flows/deliver.test.ts |
Updates delivery and problem assertions. |
test/engine/validate.test.ts |
Asserts structured validation problems. |
test/engine/section-selection.test.ts |
Covers selection invariants. |
test/engine/orchestrate.test.ts |
Updates engine result assertions. |
test/engine/layers.test.ts |
Pins typed layer failures. |
test/engine/secrets.test.ts |
Uses validated section selections. |
test/engine/secret-refs.test.ts |
Updates moved secret-reference imports. |
test/engine/check-purity.test.ts |
Updates validation and selection usage. |
test/discovery/repos-input.test.ts |
Covers structured repository-input failures. |
test/discovery/discover.test.ts |
Covers typed discovery failures. |
test/discovery/central.test.ts |
Covers structured central-target failures. |
test/report/artifact-report.test.ts |
Updates explicit uploader behavior. |
test/report/delivery.test.ts |
Updates moved delivery types and results. |
test/report/issue-report.test.ts |
Uses centralized problem rendering. |
test/action/inputs.test.ts |
Narrows coverage to the Actions adapter. |
test/action/artifact.test.ts |
Covers the production uploader adapter. |
test/action/io.test.ts |
Updates summary import location. |
test/action/action-yml.test.ts |
Updates declaration imports. |
test/published-schema.test.ts |
Updates validation expectations. |
test/private.test.ts |
Updates private-data importer allowlists. |
test/scripts/gen-action-docs.test.ts |
Updates input declaration import. |
test/e2e/schema.ts |
Updates moved imports. |
test/e2e/runner.test.ts |
Updates recipient-result assertion. |
test/e2e/oracle.ts |
Renders typed validation failures. |
test/e2e/oracle.test.ts |
Renders typed merge failures. |
test/e2e/mock/core-paths.ts |
Updates report-channel import. |
test/e2e/generators.ts |
Uses shared silent IO. |
test/e2e/generators.test.ts |
Handles typed validation failures. |
test/e2e/apply-idempotence-proof.ts |
Updates issue-channel import. |
test/docs/settings-examples.ts |
Uses shared silent IO. |
test/docs/readme.test.ts |
Updates moved constants. |
test/docs/guides.test.ts |
Updates flow imports and result assertions. |
package.json |
Adds neverthrow. |
bun.lock |
Locks the new dependency. |
knip.json |
Registers the public entry. |
biome.json |
Updates the redaction-file override. |
architecture.yml |
Defines library, flows, and problem layers. |
docs/reference/architecture.md |
Regenerates the architecture documentation. |
AGENTS.md |
Updates architecture guidance. |
.github/scripts/gen-docs.ts |
Updates delivery imports. |
.github/scripts/gen-action-docs.ts |
Updates declaration imports. |
.github/scripts/changed-sections.ts |
Makes new cross-cutting paths select all sections. |
Review details
- Files reviewed: 92/94 changed files
- Comments generated: 3
- Review effort level: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Vivswan
force-pushed
the
refactor/neverthrow
branch
from
September 12, 2026 01:52
d44b8cb to
6c438f2
Compare
There was a problem hiding this comment.
🟡 Changes recommended
The exported SettingsProblem type unintentionally includes settings-file-unreadable, overstating the validation APIs' possible errors.
Get a fresh assessment by requesting another Copilot review.
Review details
- Files reviewed: 69/70 changed files
- Comments generated: 1
- Review effort level: Balanced
…nt the input hints at src/flows/inputs.ts The `settings-` prefix match also caught `settings-file-unreadable`, a read failure validateSettingsDoc never returns, so callers had to handle a code they could never see; the four validation codes are now listed. The generated-region hints in action.yml and docs/reference/inputs.md and a comment in multi.ts still named src/action/inputs.ts as the home of the input declarations, which moved to src/flows/inputs.ts.
There was a problem hiding this comment.
🔵 Needs a closer look
The public API break lacks a breaking-release signal, and mutable selection sets can bypass their validated invariant.
Review details
Suppressed comments (2)
Previously missed (2) — in code that hasn't changed since the last review.
src/engine/section-selection.ts:49
- The returned sets expose mutable internal state, so a JavaScript caller can invalidate the selection after
ofhas accepted it.
const s = SectionSelection.ALL;
s.only.add("repository");
runForRepo(..., { sections: SectionSelection.ALL })
That shared default now silently excludes every other section for this and later runs. Return defensive copies (or expose membership methods without returning the sets) so callers cannot mutate onlyKeys or requiredKeys.
src/flows/merge.ts:54
- The changed public signatures will be released without a breaking-version signal under the current PR title.
Existing callers that expect runMerge(...) to return an exit code now receive a Result, while refactor(errors): ... produces a non-breaking release when the squash title drives release-please (AGENTS.md:14). Mark the PR title as breaking, for example refactor(errors)!: ..., before merge.
- Files reviewed: 71/72 changed files
- Comments generated: 0 new
- Review effort level: Balanced
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
One call site, before and after:
Problem(src/problem.ts, discriminated oncode, one member per distinct failure) and one renderer,describeProblem. A library caller branches on the code; the action renders at its edge (failRun), so every printed line, output, summary, and exit code is unchanged. The e2e corpus (217 scenarios) is green with no expectation changed.Result/ResultAsync:validateSectionShapes,validateSettingsDoc,mergeLayers(the admission walk returns Results; theLayerRefusalthrow is gone and its value-free prose lives indescribeProblemas thelayer-*cases),parseSettingsDoc,readSettingsFile,readLayerFiles,foldLayers,runSingle,runMulti,runMerge,parseConfig,parseReposInput,parseRepoSlug,resolveCentralTargets,discoverRepos,parseRecipient,validateSettings.ResultAsync:runMultiandparseConfigaresafeTrybodies,discoverReposisResultAsync.fromPromisewith a typed mapper,runSingleis anandThenchain ending infromSafePromise.{error: string}shapes andARTIFACT_NEEDS_UPLOADERare gone;requireUploaderreturns the problem andopenReportChannel's backstop throw renders it.codeand the structured payload;test/problem.test.tsrenders one specimen per member, typed over every code, so a new member fails compilation until it has both adescribeProblemcase and a specimen.SectionSelection(src/engine/section-selection.ts) replaces the two loose setsrequiredSections/onlySectionsonRepoRunOptionsandRunFlowConfig. Its private constructor is reached only throughSectionSelection.of({only, required}), which refuses a required section outside a non-empty allowlist asrequired-sections-excluded, so a library call can no longer pass green on a pair the action's parser would reject. A spread or a literal does not typecheck as one (private fields).parseConfigmove fromsrc/action/inputs.tstosrc/flows/inputs.ts.parseConfig(read, env)reads through a(name: InputName) => stringport and an environment record, so a CLI reuses the same declarations; the action keepsparseActionConfig(), the adapter over@actions/coreandprocess.env.INPUT_DECLSis unchanged byte for byte.call()/throwFor()exception channel (the permission policy classifies it),runForRepo'sBUG:invariant throws, andcheckRepository/applyRepository/renderMergedYaml(no failure path, so a Result would carry an uninhabited error type).Technical details
Problem members (the
codeliteral, its payload, and where it is minted):input-unsupported-valueinput, value, noun, allowed, fallbackparseConfiginput-unknown-sectionsunknown: [{input, names}], knownparseConfiginput-report-key-unusedchannelparseConfiginput-report-key-missingparseConfiginput-report-key-invalidreasonparseConfiginput-rejected-in-mergeinputsparseConfiginput-merged-file-missingparseConfiginput-settings-file-emptyvalueparseConfiginput-merge-onlyinputs, modeparseConfiginput-token-missingparseConfiginput-report-without-redactionparseConfiginput-affiliation-unsupportedentry, allowedparseConfiginput-exclude-pattern-invalidpatternparseConfiginput-repository-with-multiparseConfiginput-settings-file-with-multiparseConfigdiscovery-filters-without-wildcardfilters, targets: single-repo / explicit-repos / repos-dirparseConfig,runMultiinput-defaults-file-without-multiparseConfiginput-settings-file-is-listvalue, modeparseConfiginput-repository-not-slugvalueparseConfigsettings-not-mappingsource, shapevalidateSettingsDocsettings-not-plain-mappingsourcevalidateSettingsDocsettings-unknown-sectionssource, unknown, knownvalidateSettingsDocsettings-malformed-sectionssource, issuesvalidateSectionShapesyaml-invalidreasonparseSettingsDocsettings-file-unreadablerole, path, reasonreadSettingsFilelayer-cyclelayer, sitemergeLayerslayer-wrong-shapelayer, site, expected, actual, detail?mergeLayerslayer-bad-directivelayer, site, actualmergeLayerslayer-no-layering-keylayer, sitemergeLayerslayer-no-keylayer, site, keyFieldmergeLayerslayer-duplicate-keylayer, site, keyField, first, secondmergeLayersrequired-sections-excludedexcluded: SectionKey[]SectionSelection.of(viaparseConfig)artifact-uploader-missingrequireUploadermerged-file-is-layermergedFile, index, layerrunMerge(the guard beside the write, where the base put it)merged-file-unwritablepath, reasonrunMergeno-targetsfilteredOutrunMultirepo-slug-invalidvalueparseRepoSlugrepos-input-wildcard-mixedparseReposInputrepos-input-invalid-entriesinvalid, duplicatedparseReposInputrepos-dir-missingreposDirresolveCentralTargetsrepos-dir-unreadablereposDir, reasonresolveCentralTargetsrepos-dir-invalid-filesreposDir, files: CentralFileProblem[]resolveCentralTargetsdiscovery-request-failedpath, status, message, denieddiscoverReposdiscovery-transport-failedreasondiscoverReposdiscovery-response-not-a-listpathdiscoverReposage-recipient-invalidreasonparseRecipientChanged public signatures (name: old return, new return):
validateSectionShapes(settings, sourceLabel){settings} | {error}Result<SettingsFile, ProblemOf<"settings-malformed-sections">>validateSettingsDoc(doc, sourceLabel, onlySections, io){settings} | {error}Result<ValidatedSettings, SettingsProblem>mergeLayers(layers, {layering}){settings, notices} | {error}Result<{settings, notices}, LayerProblem>parseSettingsDoc(raw){doc} | {error}Result<unknown, ProblemOf<"yaml-invalid">>readSettingsFile(path){doc} | {error}readSettingsFile(path, role): Result<unknown, ProblemOf<"settings-file-unreadable">>readLayerFiles(paths){layers} | {error}Result<Layer[], ProblemOf<"settings-file-unreadable">>foldLayers(layers, sourceLabel, layering, io){settings, notices} | {error}Result<{settings, notices}, SettingsProblem | LayerProblem>runSingle(api, cfg, io, uploader?)Promise<{fatal} | {target}>ResultAsync<SingleOutcome, Problem>runMulti(api, cfg, io, uploader?)Promise<{fatal: string | null, targets}>ResultAsync<TargetOutcome[], Problem>runMerge(cfg, io)number(exit code; wrote the summary and outputs itself)Result<FinishedMerge, Problem>; the caller passes the value toconcludeMergeconcludeMerge(io, run)run: {layers, mergedFile}, outputs onlyrun: FinishedMerge, writes the merge summary toofailRun(io, message: string)failRun(io, problem: Problem)renders throughdescribeProblemparseConfig()(was insrc/action/inputs.ts, read@actions/coreandprocess.env){config} | {error}parseConfig(read: InputReader, env: ConfigEnv): Result<RunConfig, Problem>insrc/flows/inputs.ts; the action callsparseActionConfig()RepoRunOptions/RunFlowConfigrequiredSections: Set<SectionKey>; onlySections: Set<SectionKey>sections: SectionSelection(SectionSelection.of({only?, required?}),SectionSelection.ALL)parseReposInput(raw){slugs, discover} | {error}Result<{slugs, discover}, ProblemOf<"repos-input-wildcard-mixed" | "repos-input-invalid-entries">>parseRepoSlug(raw)RepoRef | nullResult<RepoRef, ProblemOf<"repo-slug-invalid">>resolveCentralTargets(reposDir, adminOwner){targets, warnings} | {error}Result<{targets, warnings}, ProblemOf<"repos-dir-missing" | "repos-dir-unreadable" | "repos-dir-invalid-files">>discoverRepos(api, filters)Promise<DiscoveryResult | {error}>ResultAsync<DiscoveryResult, DiscoveryProblem>parseRecipient(recipient){ok: true} | {ok: false, error}Result<void, ProblemOf<"age-recipient-invalid">>validateSettings(doc, options?){ok: true, settings, warnings} | {ok: false, error}Result<{settings, warnings}, SettingsProblem>missingUploaderProblem(cfg, uploader)string | nullrequireUploader:Result<void, ProblemOf<"artifact-uploader-missing">>quoteListsrc/flows/multi.tssrc/problem.tsARTIFACT_NEEDS_UPLOADERdescribeProblem({code: "artifact-uploader-missing"})New public names:
SectionSelection,INPUT_DECLS,InputDecl,InputName,InputReader,ConfigEnv,RunConfig,MODES,Mode,MERGE_INPUTS,MERGE_REJECTED_INPUTS,FILTER_INPUTS,DEFAULT_PRIVATE_REPOS,parseConfig,Problem,ProblemOf,LayerProblem,SettingsProblem,DiscoveryProblem,CentralFileProblem,SettingsFileRole,TopLevelShape,describeProblem,FinishedMerge,SingleOutcome,concludeMerge.neverthrowitself is not re-exported; consumers importok/err/Resultfrom it directly.Remaining
throw newin the converted layers, each an invariant and not a run outcome:src/engine/orchestrate.ts(threeBUG:throws and the rethrow of a section's classified API failure into the section loop),src/engine/execute.tsandsrc/engine/diff.ts(the section execution channel'sBUG:throws),src/action/artifact.ts(the uploader port rejects without a runtime token;deliverArtifactReportturns it into a warning),src/report/delivery.ts(the backstop for a caller that skippedrequireUploader).Architecture:
problemis a new layer inarchitecture.yml(readsgithubforRERUN_ADVICEandplain-dataforisPlainObject);engine,flows,discovery,report, andlibrarygain the edge;flowsalso readsplain-data.docs/reference/architecture.mdis regenerated.Reviewer note (Copilot): report a finding only with a concrete input and the wrong output it produces.