Record the v0.2.0 implementation review findings
This commit is contained in:
@@ -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
|
Reviewed committed implementation `f53354b`, covering the eight foundation
|
||||||
commits after `0c7129a`, against the [implementation plan](implementation.md),
|
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;
|
The worktree was clean when review began. This document records required fixes;
|
||||||
production code was unchanged during the review.
|
production code was unchanged during the review.
|
||||||
|
|
||||||
## Architecture assessment
|
## Foundation architecture assessment
|
||||||
|
|
||||||
The root orchestration facade and independent `buildinfo` runtime facade retain
|
The root orchestration facade and independent `buildinfo` runtime facade retain
|
||||||
their intended dependency direction. Both candidate operations share request
|
their intended dependency direction. Both candidate operations share request
|
||||||
@@ -234,7 +243,7 @@ land.
|
|||||||
links. Preserve the distinction between foundation completion and completed
|
links. Preserve the distinction between foundation completion and completed
|
||||||
optional features.
|
optional features.
|
||||||
|
|
||||||
## Review evidence and limits
|
## Foundation review evidence and limits
|
||||||
|
|
||||||
- `go test ./...` passed; unchanged packages used cached results.
|
- `go test ./...` passed; unchanged packages used cached results.
|
||||||
- `go run ./internal/apicheck` passed.
|
- `go run ./internal/apicheck` passed.
|
||||||
@@ -253,3 +262,147 @@ optional features.
|
|||||||
This review evaluates the completed foundation. It does not establish that the
|
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
|
remaining v0.2.0 features have been implemented or that their eventual checker
|
||||||
semantics are verified.
|
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
|
||||||
|
# <https://example.invalid>
|
||||||
|
[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
|
||||||
|
`# <maintainer@example.invalid>` 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
|
||||||
|
](image.png)
|
||||||
|
```
|
||||||
|
|
||||||
|
Goldmark's own renderer produces `<img src="image.png" alt="inner">`.
|
||||||
|
Similarly, `](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:
|
||||||
|
`[](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.
|
||||||
|
|||||||
Reference in New Issue
Block a user