From 1eb1cc6491da35158d6305b12b89abb919caffcd Mon Sep 17 00:00:00 2001 From: Eric Rakestraw Date: Thu, 8 Oct 2026 01:00:42 +0000 Subject: [PATCH] Record the v0.2.0 implementation review findings --- docs/roadmap/remediation.md | 161 +++++++++++++++++++++++++++++++++++- 1 file changed, 157 insertions(+), 4 deletions(-) diff --git a/docs/roadmap/remediation.md b/docs/roadmap/remediation.md index 59e48f0..2f465a0 100644 --- a/docs/roadmap/remediation.md +++ b/docs/roadmap/remediation.md @@ -1,6 +1,15 @@ -# Shared validation foundation remediation +# ReleaseKit implementation remediation -Status: All six findings addressed. +Status: Three open v0.2.0 findings (7–9). The six foundation findings (1–6) +are addressed. + +The v0.2.0 review covers committed state `edd670e` and the implementation +commits after `89981ab`, against the [implementation plan](implementation.md), +[feature roadmap](v0.2.0.md), [model roadmap](model.md), accepted ADRs, and +applicable repository policies. Both ReleaseKit and Audita had clean worktrees +when inspected. This review changes only this document. + +## Foundation review history Reviewed committed implementation `f53354b`, covering the eight foundation commits after `0c7129a`, against the [implementation plan](implementation.md), @@ -8,7 +17,7 @@ commits after `0c7129a`, against the [implementation plan](implementation.md), The worktree was clean when review began. This document records required fixes; production code was unchanged during the review. -## Architecture assessment +## Foundation architecture assessment The root orchestration facade and independent `buildinfo` runtime facade retain their intended dependency direction. Both candidate operations share request @@ -234,7 +243,7 @@ land. links. Preserve the distinction between foundation completion and completed optional features. -## Review evidence and limits +## Foundation review evidence and limits - `go test ./...` passed; unchanged packages used cached results. - `go run ./internal/apicheck` passed. @@ -253,3 +262,147 @@ optional features. This review evaluates the completed foundation. It does not establish that the remaining v0.2.0 features have been implemented or that their eventual checker semantics are verified. + +## v0.2.0 architecture assessment + +The intended package and ownership boundaries are implemented. The root +`releasekit` facade owns requests, validation selections, diagnostics, and +publication/delivery outcomes. The separate `buildinfo` facade depends on the +standard library and `internal/runtimeinfo`; its graph remains independent of +orchestration and the new parsing dependencies. + +`Check` and `Publish` use the same request conversion and +`internal/candidate.Validate`. `internal/validation` coordinates one native +test suite, selected full vet and static package compilation, and selected +committed-file checks. Formatting, distribution, file-size, and Markdown checks +share candidate inventory and lazy blob access. Goldmark and `x/mod/modfile` +remain internal dependencies. Product callbacks, artifact building and version +injection, publication guards/recovery, and hosted delivery retain their owners. +The expected push URL is compared exactly in publication preflight; local +checking ignores it. The CLI owns profile conversion, presentation, and exits. + +The consumer Woodpecker template verifies tagged source before selecting the +tool through temporary module metadata. Audita's local wrapper and workflow +adopt that approach in commit `17421bc`; the inspected declarations preserve +its existing dependency version, image, secret name, and release profile. + +All ten feature areas are present. The three findings below identify incomplete +behavior within Markdown validation and self-bootstrap. Finding 8 imposes +unrequested existence/boundary checks on image alternative text. Finding 9 +effectively requires a prepopulated tool module cache. The review found no +additional public package, validator-registration framework, universal optional +check, arbitrary inspection limit, or change to source-publication completion. + +## 7. P2: Heading anchors omit autolink display text + +**Owner:** [Markdown heading extraction](../../internal/validation/markdown.go), +`markdownHeadingText`, lines 115–139. + +The agreed convention derives anchors from visible heading text. The extractor +handles text and code spans and recurses through formatting nodes, but Goldmark's +`ast.AutoLink` stores its visible text in `Label` and has no text children. +URL and email autolinks therefore disappear from the heading before slugging. + +**Evidence:** A temporary overlay probe against the current checker used: + +```markdown +# +[section](#httpsexampleinvalid) +``` + +The computed anchor was the empty string, and the section link failed with +`heading anchor does not exist`. The same failure occurred for a heading +`# ` and fragment `#maintainerexampleinvalid`. +Existing anchor tests cover text, code, formatting, entities, Unicode, and +duplicates, while existing autolink coverage only checks URL extraction. + +**Required remediation:** Include an autolink's display label in heading text +before applying the existing normalization and duplicate-anchor algorithm. +Preserve the current HTML, code-span, and ordinary link-label handling. + +**Verification:** Add parser coverage for URL and email autolinks inside headings, +including surrounding text, and a representative committed-fragment check. +Keep external destination availability outside this offline capability. + +## 8. P2: Image alternative text creates additional release requirements + +**Owner:** [Markdown link extraction](../../internal/validation/markdown.go), +`parseMarkdown`, especially the image branch and AST traversal at lines 65–110. + +After recording an image destination, the walker continues through the image's +label children and records nested link/image syntax as independent destinations. +Those children contribute plain alternative text to the rendered image. The +checker consequently requires files and anchors that the document does not +navigate to or load, and boundary enforcement can also reject those labels. + +**Evidence:** With only `image.png` committed, this valid Markdown is rejected +for missing `missing.png`: + +```markdown +![![inner](missing.png)](image.png) +``` + +Goldmark's own renderer produces `inner`. +Similarly, `![[label](missing.md)](image.png)` renders an image with alternative +text `label`, while ReleaseKit records and validates `missing.md` as a link. +Temporary overlay probes confirmed both extraction and rendering; the first +case also reproduced the checker failure against committed Git contents. + +**Required remediation:** Record the outer image destination and treat its label +as alternative text during destination extraction. Preserve real linked images: +`[![logo](badge.png)](guide.md)` has an image source and an enclosing link target +that both require validation. This change removes the unintended requirement +without narrowing supported Markdown syntax. + +**Verification:** Cover nested image/link syntax in alternative text, including +an outside-checkout label destination under boundary enforcement. Verify that +real linked images still report missing image sources and missing link targets. + +## 9. P1: ReleaseKit's source-delivery bootstrap fails with an empty module cache + +**Owners:** [source-delivery workflow](../../examples/woodpecker/releasekit-source-delivery.yml), +lines 27–31; [workflow verification](../../cmd/releasekit/woodpecker_test.go), +`TestWoodpeckerSourceDeliveryTemplateSelfBootstraps`; and the +[delivery guide](../consumers/delivery.md#woodpecker-tag-workflows). + +The source template sets `GOPROXY=off` and `GOSUMDB=off` before building the tool +and provides no tool dependency preparation. ReleaseKit now imports Goldmark and +`x/mod` through validation, so the standard Go image needs those modules to build +`./cmd/releasekit`. On a fresh runner the workflow stops before `ci-deliver` and +the hosted note page is never delivered. The previous dependency-free tool did +not expose this problem. The guide describes cache mounts as optional, while +the current implementation effectively requires a prepopulated module cache. + +**Evidence:** Running the template's build command against `edd670e`, with a +temporary empty `GOMODCACHE` and its workspace/toolchain/proxy settings, returned +exit 1. Both dependencies reported `module lookup disabled by GOPROXY=off`. +The existing self-bootstrap test builds a synthetic command that imports only +`fmt` and `os`, so it cannot detect missing tool dependencies. + +**Required remediation:** Allow ordinary tool dependency preparation after tag +verification. A network-capable readonly tool build or an explicit preparation +step can populate the module cache while preserving tagged source. Honor normal +Go proxy/private-module/cache settings. Keep offline candidate validation and +application artifact compilation in their existing owners. Update the template +and guidance together; a prewarmed cache remains an optional deployment choice. + +**Verification:** Exercise the maintained self-bootstrap commands with an empty +module cache and a local module proxy serving a real external tool dependency. +Verify preparation follows tag/commit verification, the executable runs, tagged +module files remain unchanged, and temporary output is cleaned up. Existing +delivery tests retain ownership of hosted page behavior. + +## v0.2.0 review evidence and limits + +- Reviewed recent production, test, template, and documentation changes and + their shared request, execution, and dependency boundaries. +- `GOPROXY=off GOSUMDB=off GOTOOLCHAIN=local go test -count=1 ./...` passed. +- `go run ./internal/apicheck` passed. Direct dependency inspection of + `buildinfo` confirmed its standard-library/runtime-only graph. +- Inspected Audita commit `17421bc` and its actual local and CI bootstrap files. +- Temporary overlay tests outside the worktree reproduced findings 7 and 8. + Goldmark's renderer confirmed the alternative-text semantics. A separate + temporary empty module cache reproduced finding 9. These probes are review + evidence; production code and the committed test suite remain unchanged. +- Existing tests pass but omit these cases. The required fixes remain open; + their remediation and regression coverage are subsequent work.