amadan

carlos / platform Public

changelog: Astra's code review, all three findings

- CheckPublic decodes HTML entities, markdown escapes and percent
  encoding before looking for addresses and private links.
- The gate requires notes in exactly the two forms the page renders;
  a release published with an empty body (v0.18.0) has no notes.
- Credit keeps each identity's pairing: an agent account credits its
  operator, a lone agent trailer the commit's author (da866a92 no
  longer credits Tony Wood with claude).
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
claude d10798d0f96da7eb4d88f2a158d4018bb0ac047f parent f1076f6
8 files changed, +268 −38
  • docs/site/changelog.json +2 −7
  • docs/superpowers/specs/CODEX-FINDINGS-CHANGELOG-CODE-1.md +18 −0
  • docs/superpowers/specs/CODEX-REVIEW-BRIEF-CHANGELOG-CODE.md +57 −0
  • internal/changelog/build.go +73 −3
  • internal/changelog/build_test.go +17 −0
  • internal/changelog/committed_test.go +21 −1
  • internal/changelog/credit.go +48 −27
  • internal/changelog/credit_test.go +32 −0

CI passed — run details

diff --git a/docs/site/changelog.json b/docs/site/changelog.json
index 91976b35a..5732ae2d0 100644
--- a/docs/site/changelog.json
+++ b/docs/site/changelog.json
@@ -1120,10 +1120,7 @@
]
},
{
- "person": "Tony Wood",
- "agents": [
- "claude"
- ]
+ "person": "Tony Wood"
}
]
}
@@ -2483,9 +2480,7 @@
"sha": "16593de5eb27ba96f74423520e4728008059f990",
"published": true,
"notes_url": "https://github.com/carlosframework/releases/releases/tag/v0.18.0",
- "notes": {
- "form": "markdown"
- },
+ "notes": null,
"changes": [
{
"sha": "16593de5eb27ba96f74423520e4728008059f990",
diff --git a/docs/superpowers/specs/CODEX-FINDINGS-CHANGELOG-CODE-1.md b/docs/superpowers/specs/CODEX-FINDINGS-CHANGELOG-CODE-1.md
new file mode 100644
index 000000000..1b46b8a62
--- /dev/null
+++ b/docs/superpowers/specs/CODEX-FINDINGS-CHANGELOG-CODE-1.md
@@ -0,0 +1,18 @@
+Reviewed platform `f1076f62` and website `59b9cf9`. No files edited. All 28 release ranges and own-commit counts match Git. Go tests could not start in the read-only sandbox; full `npm run check` requires writes. Existing website checks and in-memory rendering passed.
+
+1. **P1 — Encoded addresses and private links bypass public-output validation.**
+ [internal/changelog/build.go:559](/home/paulca/amadan.net/carlos/platform-changelog/internal/changelog/build.go:559)
+ `CheckPublic` scans serialized source text, but Markdown decodes entities afterward. For example, `[contact](mailto:ops&#64;example.org)` and `[source](https://github.com/carlosframework/plat&#102;orm)` evade its patterns. In-memory rendering produced the actual email address and private platform URL, and `check-docs.mjs` still passed.
+ **Fix:** Validate decoded Markdown text and normalized link destinations, with a corresponding rendered-output privacy check. Add regression cases for entities and percent-encoded URLs.
+
+2. **P1 — An invalid notes form silently removes release notes while passing the gates.**
+ [internal/changelog/committed_test.go:48](/home/paulca/amadan.net/carlos/platform-changelog/internal/changelog/committed_test.go:48), [src/changelog.njk:160](/home/paulca/github.com/carlosframework/website-changelog/src/changelog.njk:160)
+ Changing v0.24.2’s `notes.form` from `"bullets"` to `"bullet"` in memory removed its notes from the rendered page; the website gate passed. The Go checks never validate `form`, and the bullet-mapping checks still inspect the retained sections, so they cannot detect this loss.
+ **Fix:** Require exactly `"bullets"` or `"markdown"` with the matching payload. Reject unknown forms during website validation, and test this mutation.
+
+3. **P2 — Credit assigns every agent to every person on a commit.**
+ [internal/changelog/credit.go:136](/home/paulca/amadan.net/carlos/platform-changelog/internal/changelog/credit.go:136)
+ In v0.23.0, landing `da866a92` credits Tony Wood “with claude.” Across its 24 own commits, Tony appears only as the human co-author of `9fe9ec28`; that commit’s agent identity belongs to Paul. `Tally` discards those associations and attaches the combined agents to both people. This contradicts the specified per-person attribution.
+ **Fix:** Preserve each resolved person–agent pair. Attach agent-only trailers to the author’s person, rather than every co-author, and add this historical case as a regression.
+
+**Accept with fixes.**
\ No newline at end of file
diff --git a/docs/superpowers/specs/CODEX-REVIEW-BRIEF-CHANGELOG-CODE.md b/docs/superpowers/specs/CODEX-REVIEW-BRIEF-CHANGELOG-CODE.md
new file mode 100644
index 000000000..8a23a6ea0
--- /dev/null
+++ b/docs/superpowers/specs/CODEX-REVIEW-BRIEF-CHANGELOG-CODE.md
@@ -0,0 +1,57 @@
+# 🤖 Review brief: the platform changelog, code
+
+You are the independent reviewer of a finished build. The author is
+Claude. Do not edit any file.
+
+## What to read
+
+1. The spec, rev 2: `docs/superpowers/specs/2026-10-06-platform-changelog-design.md`,
+ and the earlier findings `CODEX-FINDINGS-CHANGELOG-1.md` (all four were
+ taken; check the fixes hold).
+2. The platform branch: `git diff origin/main...platform-changelog` in
+ `/home/paulca/amadan.net/carlos/platform-changelog`, excluding the
+ generated `docs/site/changelog.json` (read it as output, not code).
+ Code is `internal/changelog/`, `cmd/changelog/`, the `Line` and
+ `Parse` additions in `internal/docsite/docsite.go`, `docs/site/people.json`,
+ `docs/site/changelog-links.json`, and README step 10.
+3. The website: commit `59b9cf9` on branch `platform-changelog` in
+ `/home/paulca/github.com/carlosframework/website-changelog`
+ (`git show 59b9cf9 -- . ':!src/_data/docschangelog.json' ':!src/docs/cli.md'`).
+ `src/changelog.njk` renders the page; `hack/sync-docs.mjs` vendors
+ the data; `hack/check-docs.mjs` gates the build.
+
+## Fixed: do not reopen
+
+The spec's four rulings; that change lines are verbatim records; that the
+copy is in a separate human review (do not review wording).
+
+## What to look for
+
+1. **Wrong output on real history.** The generator has been run over all
+ 28 tags. Pick changes in `docs/site/changelog.json` and check them
+ against `git log` and `git for-each-ref refs/amadan/closed` (fetch
+ with `git fetch origin '+refs/amadan/closed/*:refs/amadan/closed/*'`
+ if missing): the range a release claims, the own commits a change
+ counts, credit, discussions, docs links. Name the commit.
+2. **Public safety.** Anything that could put an email address, a
+ private repo link or unescaped markup on the public page: the
+ generator, `CheckPublic`/`Redact`, the gate, and the template
+ (Nunjucks autoescape, the `notesMd` filters, `| safe`).
+3. **Gate holes.** A committed file that is wrong and still passes
+ `go test ./internal/changelog/` and `npm run check`.
+4. **Robustness at release time.** What breaks when the next release is
+ cut and step 10 is followed: a new tag, a new contributor, a bullet
+ with no bold lead, a release with no first-parent commits, gh or
+ amadan failing.
+5. **Code quality** where it would mislead the next person: dead code,
+ names that say the wrong thing, tests that pass for the wrong reason.
+
+Run `go test ./internal/changelog/ ./internal/docsite/` (set
+`GOCACHE` to a writable directory) if you can; say if you could not.
+
+## Output
+
+A numbered list, each with severity (P1 publishes something wrong or
+breaks the page or gate; P2 misleading or fragile; P3 nit), file:line,
+evidence, and a proposed fix. Then one line: accept, accept with fixes,
+or reject.
diff --git a/internal/changelog/build.go b/internal/changelog/build.go
index 9e15a9e92..d6351e696 100644
--- a/internal/changelog/build.go
+++ b/internal/changelog/build.go
@@ -3,6 +3,7 @@ package changelog
import (
"encoding/json"
"fmt"
+ "html"
"net/url"
"regexp"
"slices"
@@ -212,8 +213,12 @@ func (b *builder) release(t Tag, earlier []Release) (*Release, error) {
if ok {
r.Published = true
r.NotesURL = NotesBase + t.Name
- r.Notes = ParseNotes(body)
- for si := range r.Notes.Sections {
+ // A release published with an empty body (v0.18.0) has a notes
+ // page to link and nothing to show here.
+ if n := ParseNotes(body); n.MD != "" || len(n.Sections) > 0 {
+ r.Notes = n
+ }
+ for si := range notesSections(r.Notes) {
for bi := range r.Notes.Sections[si].Bullets {
bl := &r.Notes.Sections[si].Bullets[bi]
bl.Docs = capped(CLIDocLinks(bl.MD, b.cliHeadings))
@@ -239,6 +244,13 @@ func (b *builder) release(t Tag, earlier []Release) (*Release, error) {
return r, nil
}
+func notesSections(n *Notes) []NoteSection {
+ if n == nil {
+ return nil
+ }
+ return n.Sections
+}
+
func (b *builder) change(c Commit) (*Change, error) {
if ch, ok := b.built[c.SHA]; ok {
return ch, nil
@@ -561,7 +573,65 @@ func checkPublicValue(v any, people People) error {
if err != nil {
return err
}
- s := strings.ToLower(string(raw))
+ var tree any
+ if err := json.Unmarshal(raw, &tree); err != nil {
+ return err
+ }
+ var texts []string
+ collectStrings(tree, &texts)
+ for _, t := range texts {
+ for _, form := range decodedForms(t) {
+ if err := checkPublicText(strings.ToLower(form), people); err != nil {
+ return err
+ }
+ }
+ }
+ return nil
+}
+
+func collectStrings(v any, out *[]string) {
+ switch x := v.(type) {
+ case string:
+ *out = append(*out, x)
+ case []any:
+ for _, e := range x {
+ collectStrings(e, out)
+ }
+ case map[string]any:
+ for _, e := range x {
+ collectStrings(e, out)
+ }
+ }
+}
+
+var (
+ mdEscapeRe = regexp.MustCompile(`\\([!-/:-@\[-` + "`" + `{-~])`)
+ percentRe = regexp.MustCompile(`%[0-9a-fA-F]{2}`)
+)
+
+// decodedForms is a string as stored and as a reader would see it once
+// markdown and the browser have decoded it: HTML entities, markdown
+// backslash escapes and percent-encoding undone, each alone and all
+// together. "ops&#64;example.org" is an address on the page.
+func decodedForms(s string) []string {
+ ent := html.UnescapeString(s)
+ md := mdEscapeRe.ReplaceAllString(s, "$1")
+ pct := percentRe.ReplaceAllStringFunc(s, func(m string) string {
+ if b, err := strconv.ParseUint(m[1:], 16, 8); err == nil {
+ return string(rune(b))
+ }
+ return m
+ })
+ all := percentRe.ReplaceAllStringFunc(mdEscapeRe.ReplaceAllString(html.UnescapeString(html.UnescapeString(s)), "$1"), func(m string) string {
+ if b, err := strconv.ParseUint(m[1:], 16, 8); err == nil {
+ return string(rune(b))
+ }
+ return m
+ })
+ return []string{s, ent, md, pct, all}
+}
+
+func checkPublicText(s string, people People) error {
for addr := range people {
if strings.Contains(addr, "@") && strings.Contains(s, strings.ToLower(addr)) {
return fmt.Errorf("would publish the address %s", addr)
diff --git a/internal/changelog/build_test.go b/internal/changelog/build_test.go
index 06ea4abb2..05d333c2d 100644
--- a/internal/changelog/build_test.go
+++ b/internal/changelog/build_test.go
@@ -238,6 +238,11 @@ func TestCheckPublic(t *testing.T) {
"write to ops@example.org",
"see https://github.com/carlosframework/platform/pull/409",
"in github.com/carlosframework/carloku-infrastructure",
+ "[contact](mailto:ops&#64;example.org)",
+ "[source](https://github.com/carlosframework/plat&#102;orm)",
+ "[source](https://github.com/carlosframework/plat%66orm)",
+ "write to ops\\@example.org",
+ "ops&amp;#64;example.org",
} {
bad := &Changelog{Releases: []Release{{Tag: "v1.0.0", Changes: []Change{{Subject: s}}}}}
if err := CheckPublic(bad, testPeople); err == nil {
@@ -372,3 +377,15 @@ func TestDescribe(t *testing.T) {
}
}
}
+
+func TestBuildPublishedWithEmptyNotes(t *testing.T) {
+ g, rel, disc := fixture()
+ rel["v0.10.0"] = "\n"
+ c, err := Build(Sources{g, rel, disc}, testPeople, nil, testSite(t))
+ if err != nil {
+ t.Fatal(err)
+ }
+ if r := c.Releases[0]; !r.Published || r.NotesURL == "" || r.Notes != nil {
+ t.Fatalf("v0.10.0 = %+v", r)
+ }
+}
diff --git a/internal/changelog/committed_test.go b/internal/changelog/committed_test.go
index d18ddc7d7..c3a0abd9c 100644
--- a/internal/changelog/committed_test.go
+++ b/internal/changelog/committed_test.go
@@ -45,9 +45,29 @@ func TestCommittedShape(t *testing.T) {
if !dayRe.MatchString(r.Date) || r.SHA == "" || r.Changes == nil {
t.Errorf("%s: date %q, sha %q, changes nil=%v", r.Tag, r.Date, r.SHA, r.Changes == nil)
}
- if r.Published != (r.Notes != nil) || r.Published != (r.NotesURL == NotesBase+r.Tag) {
+ if (r.Notes != nil && !r.Published) || r.Published != (r.NotesURL == NotesBase+r.Tag) {
t.Errorf("%s: published=%v but notes=%v notes_url=%q", r.Tag, r.Published, r.Notes != nil, r.NotesURL)
}
+ // The page renders exactly these two forms; anything else would
+ // drop the release's notes without a word.
+ if n := r.Notes; n != nil {
+ switch {
+ case n.Form == "bullets" && n.MD == "" && len(n.Sections) > 0:
+ for _, s := range n.Sections {
+ if len(s.Bullets) == 0 {
+ t.Errorf("%s: notes section %q has no bullets", r.Tag, s.Title)
+ }
+ for _, b := range s.Bullets {
+ if strings.TrimSpace(b.MD) == "" {
+ t.Errorf("%s: an empty bullet in %q", r.Tag, s.Title)
+ }
+ }
+ }
+ case n.Form == "markdown" && strings.TrimSpace(n.MD) != "" && len(n.Sections) == 0:
+ default:
+ t.Errorf("%s: notes form %q with %d sections and %d bytes of markdown is not one the page renders", r.Tag, n.Form, len(n.Sections), len(n.MD))
+ }
+ }
for _, ch := range r.Changes {
if len(ch.SHA) != 40 || !dayRe.MatchString(ch.Date) || ch.Subject == "" || len(ch.Credit) == 0 {
t.Errorf("%s: change %+v is missing a field", r.Tag, ch)
diff --git a/internal/changelog/credit.go b/internal/changelog/credit.go
index 4057dc620..138167498 100644
--- a/internal/changelog/credit.go
+++ b/internal/changelog/credit.go
@@ -96,45 +96,66 @@ func Tally(commits []CommitIdent, people People) ([]Credit, error) {
}
byPerson := map[string]*tally{}
loose := map[string]bool{}
+ credit := func(p string, agents ...string) {
+ t := byPerson[p]
+ if t == nil {
+ t = &tally{agents: map[string]bool{}}
+ byPerson[p] = t
+ }
+ for _, a := range agents {
+ if a != "" {
+ t.agents[a] = true
+ }
+ }
+ }
for _, c := range commits {
- ids := append([][2]string{{c.AuthorName, c.AuthorEmail}}, CoAuthors(c.Message)...)
- var persons []string
- var agents []string
- for _, id := range ids {
+ type ident struct{ person, agent string }
+ var ids []ident
+ for _, id := range append([][2]string{{c.AuthorName, c.AuthorEmail}}, CoAuthors(c.Message)...) {
p, a, skip, err := ParseIdentity(id[0], id[1], people)
if err != nil {
return nil, err
}
- if skip {
- continue
- }
- if p != "" {
- persons = append(persons, p)
- }
- if a != "" {
- agents = append(agents, a)
+ if !skip {
+ ids = append(ids, ident{p, a})
}
}
- if len(persons) == 0 {
- for _, a := range agents {
- loose[a] = true
- }
+ if len(ids) == 0 {
continue
}
- seen := map[string]bool{}
- for _, p := range persons {
- if seen[p] {
+ // Each identity keeps its own pairing: an agent account names its
+ // operator, a person is just a person. An agent with no person of
+ // its own (a Co-Authored-By: Claude trailer) worked with the
+ // commit's author, or failing that the first co-author who is a
+ // person; it is never spread over every person on the commit.
+ var loneAgents []string
+ owner := ""
+ for _, id := range ids {
+ if id.person == "" {
+ loneAgents = append(loneAgents, id.agent)
+ } else if owner == "" {
+ owner = id.person
+ }
+ }
+ if ids[0].person != "" {
+ owner = ids[0].person
+ }
+ counted := map[string]bool{}
+ for _, id := range ids {
+ if id.person == "" {
continue
}
- seen[p] = true
- t := byPerson[p]
- if t == nil {
- t = &tally{agents: map[string]bool{}}
- byPerson[p] = t
+ credit(id.person, id.agent)
+ if !counted[id.person] {
+ counted[id.person] = true
+ byPerson[id.person].count++
}
- t.count++
- for _, a := range agents {
- t.agents[a] = true
+ }
+ if owner != "" {
+ credit(owner, loneAgents...)
+ } else {
+ for _, a := range loneAgents {
+ loose[a] = true
}
}
}
diff --git a/internal/changelog/credit_test.go b/internal/changelog/credit_test.go
index fb1b558ec..f6f289dee 100644
--- a/internal/changelog/credit_test.go
+++ b/internal/changelog/credit_test.go
@@ -12,6 +12,7 @@ var testPeople = People{
"bill@keymail.dev": "Bill Horsman",
"james@lazyatom.com": "James Adam",
"tonywood@keymail.dev": "Tony Wood",
+ "tony@tctl.co.uk": "Tony Wood",
}
func TestParseIdentity(t *testing.T) {
@@ -102,3 +103,34 @@ func TestTallyPersonOnly(t *testing.T) {
t.Fatalf("Tally = %+v, want %+v", got, want)
}
}
+
+// da866a92 (v0.23.0): an agent commit with a human co-author credits the
+// agent to its own operator, not to the co-author.
+func TestTallyKeepsEachIdentitysPairing(t *testing.T) {
+ commits := []CommitIdent{
+ {"claude", "claude~paul@keymail.dev", "x\n\nCo-Authored-By: Tony Wood <tony@tctl.co.uk>"},
+ {"claude", "claude~paul@keymail.dev", "y"},
+ }
+ got, err := Tally(commits, testPeople)
+ if err != nil {
+ t.Fatal(err)
+ }
+ want := []Credit{{Person: "Paul Campbell", Agents: []string{"claude"}}, {Person: "Tony Wood"}}
+ if !reflect.DeepEqual(got, want) {
+ t.Fatalf("Tally = %+v, want %+v", got, want)
+ }
+}
+
+// A person's commit with a Claude trailer and a human co-author: Claude
+// worked with the author.
+func TestTallyLoneAgentGoesToTheAuthor(t *testing.T) {
+ msg := "x\n\nCo-Authored-By: James Adam <james@lazyatom.com>\nCo-Authored-By: Claude <noreply@anthropic.com>"
+ got, err := Tally([]CommitIdent{{"Paul Campbell", "paul@tito.io", msg}}, testPeople)
+ if err != nil {
+ t.Fatal(err)
+ }
+ want := []Credit{{Person: "James Adam"}, {Person: "Paul Campbell", Agents: []string{"claude"}}}
+ if !reflect.DeepEqual(got, want) {
+ t.Fatalf("Tally = %+v, want %+v", got, want)
+ }
+}