Skip to content

feat(ci): orphaned-fixture + CHANGELOG-parity gates (#663) - #681

Merged
kyle-sexton merged 14 commits into
mainfrom
ci/663-orphaned-fixture-changelog-parity-gates
Jul 20, 2026
Merged

feat(ci): orphaned-fixture + CHANGELOG-parity gates (#663)#681
kyle-sexton merged 14 commits into
mainfrom
ci/663-orphaned-fixture-changelog-parity-gates

Conversation

@kyle-sexton

@kyle-sexton kyle-sexton commented Jul 20, 2026

Copy link
Copy Markdown
Contributor

Summary

Two repo-local CI gates from the 2026-07-20 merged-PR quality audit, both wired into .github/workflows/ci.yml as new required lanes. Each follows the established repo-local gate precedent (#593/#609/#622/#628): a self-testable script, a dedicated CI step, fail-closed behavior, and clear diagnostics — with a stale-guarded baseline that grandfathers pre-existing debt without red-lining fixtures/plugins a member issue already owns. Placement is repo-local only; promotion to ci-workflows is explicitly deferred ("if it stabilizes") and out of scope.

No plugin version bump: the CI scripts and ci.yml carry no plugin version to bump. The only CHANGELOG.md edits in this PR are a format-only normalization of five pre-existing unbracketed changelogs (see Gate 2) — no version fields change. This repo has no repo-root version artifact.

Fix

Gate 1 — orphaned-fixture (scripts/check-orphaned-fixtures.sh, lane orphaned-fixture-gate). Repo-wide static scan of every file under a skill's **/evals/fixtures/. A fixture is consumed when its skill-relative path appears in the sibling evals/evals.json (an eval files[] entry), or its basename appears — bounded by non-filename characters, so a referenced valid.json does not also mark a valid.json.bak sibling consumed — in that evals.json or any *.test.* file in the plugin. Consumption matching errs toward not-blocking a legitimate fixture. Scope is evals/fixtures/ specifically — unit-test fixture dirs (tests/fixtures, scripts/fixtures) are excluded because those are often generated or loaded by directory, not named, and a name-matcher cannot honestly grade them. --check fails on an un-grandfathered orphan and on a stale baseline entry (one shadowing no orphan), so the baseline cannot outlive its debt.

Existing debt grandfathered in scripts/orphaned-fixtures-baseline.txtexact fixture paths, matched by full-string equality (not prefix), so the baseline is a snapshot of today's known orphans: a new orphan under an already-listed directory, or a <name>.json.bak/<name>.jsonl sibling of a listed file, matches no line and is red-lined rather than silently grandfathered. Every entry cites an owning issue:

Gate 2 — CHANGELOG-parity (scripts/check-changelog-parity.sh, lane changelog-parity-gate). Covers both gaps the audit named:

  • --check (static, every event): a plugins/<name>/.claude-plugin/plugin.json carrying a version must ship plugins/<name>/CHANGELOG.md. Catches the cited violator — autonomy shipped 5 minor bumps with no CHANGELOG.md.
  • --check-bump <base> (PR-only, mirrors the sync-*.sh --check-bump gates): a manifest version change must add a ## [<version>] release heading for the new version — present at head, absent at <base>. The heading is matched as a fixed string anchored to line start, so the version appearing in prose or a fenced example never satisfies (or falsely pre-exists) the entry, and SemVer build metadata (1.0.1+build.1) never leaks into a regex. Three distinct failures, never conflated: UNDOCUMENTED BUMP (no entry — an unrelated whitespace/title/old-release edit cannot satisfy the gate), CHANGELOG FORMAT (the version is present but written unbracketed, ## <version>), and PRE-EXISTING CHANGELOG ENTRY (the heading already existed at <base>, so the bump shipped no fresh note). Applies to every plugin, grandfathered or not.

Enforcing the bracketed ## [<version>] Keep-a-Changelog heading surfaced five pre-existing changelogs still on the unbracketed ## <version> form (discovery, docs-hygiene, knowledge, playbooks, session-flow); this PR normalizes them to the documented convention (format-only, headings only — no version or entry-content changes).

Reconstructing autonomy's history is autonomy-plugin (WP-lane) work, so it is grandfathered by name in scripts/changelog-parity-baseline.txt (stale-guarded: --check fails if it later gains a CHANGELOG.md or drops its version). The baseline never relaxes --check-bump.

Both lanes are registered in the ci-status needs: aggregate (the single source of truth for required lanes).

Verification

All output below is real, run in the worktree against origin/main.

Self-tests (run unconditionally in CI so a broken detector cannot mask a regression):

=========== ORPHAN GATE SELF-TEST ===========
ok: files[]-referenced fixture passes --check
ok: test-asserted fixture passes --check
ok: suffix sibling of a referenced fixture red-lines (bounded basename match)
ok: un-consumed fixture fails --check (synthetic orphan caught)
ok: grandfathered orphan (exact path) passes --check
ok: prefix-sibling of a baselined path red-lines (exact-match, no grandfather leak)
ok: stale baseline entry fails --check
ok: discover labels CONSUMED and ORPHAN
ok: bad mode -> exit 2

PASS=9 FAIL=0

=========== CHANGELOG GATE SELF-TEST ===========
ok: versioned plugin with CHANGELOG passes --check
ok: versioned plugin without CHANGELOG fails --check (synthetic gap caught)
ok: grandfathered missing-changelog passes --check
ok: stale baseline entry fails --check (reported exactly once)
ok: bump + '## [x.y.z]' entry passes --check-bump
ok: bump + unbracketed heading -> FORMAT error (not UNDOCUMENTED)
ok: bump adding a NEW '## [x.y.z]' entry (absent at base) passes --check-bump
ok: bump reusing a base-pre-existing '## [x.y.z]' entry fails --check-bump
ok: SemVer build-metadata version with a proper entry passes (no regex leak)
ok: version string in prose/indented example does not satisfy the anchored heading match
ok: bump + unrelated changelog edit (no new-version entry) fails --check-bump
ok: bump without changelog fails --check-bump (synthetic undocumented bump caught)
ok: unchanged version passes --check-bump
ok: new plugin skipped by --check-bump
ok: unresolvable base ref -> exit 2
ok: --check-bump without base ref -> exit 2
ok: bad mode -> exit 2

PASS=17 FAIL=0

Both gates pass on current HEAD (existing debt grandfathered):

--- orphaned-fixture-gate: --check ---
No orphaned eval fixtures (every file under **/evals/fixtures/ is consumed by a grader or grandfathered).
--- changelog-parity-gate: --check ---
Every versioned plugin has a CHANGELOG.md (or a stale-guarded baseline entry).
--- changelog-parity-gate: --check-bump vs origin/main ---
Every plugin whose version changed vs origin/main has a '## [<version>]' CHANGELOG.md entry.

Live synthetic catch — Gate 1 (orphaned fixture, no baseline):

ORPHANED FIXTURE: plugins/demo/skills/s/evals/fixtures/never-referenced.json is under evals/fixtures/ but no eval case references it and no test asserts on it.
  Reference it from a grader (an eval files[] entry or a test), delete it, or grandfather it in /nonexistent with the owning issue.
exit=1

Live synthetic catch — Gate 2 (missing changelog, no baseline):

MISSING CHANGELOG: plugins/demo carries a versioned plugins/demo/.claude-plugin/plugin.json but no plugins/demo/CHANGELOG.md.
  Add plugins/demo/CHANGELOG.md, or grandfather 'demo' in /nonexistent with its owning issue.
exit=1

Static analysis: shellcheck --rcfile .shellcheckrc clean on all four new scripts; actionlint .github/workflows/ci.yml clean; new .sh files committed mode 100755, all files LF.

Closes #663

Related

🤖 Generated with a Claude Code implementation subagent (issue #663)

Two repo-local CI gates from the 2026-07-20 merged-PR quality audit,
following the scripts/ + ci.yml precedent of the docs-only and
cross-plugin-source-drift gates: self-testable, fail-closed, stale-guarded
baselines that grandfather existing debt without red-lining it.

Gate 1 (orphaned-fixture): every file under a skill's evals/fixtures/ must be
consumed by a grader — an eval files[] entry or a test assertion.

Gate 2 (CHANGELOG-parity): a versioned plugin must keep a CHANGELOG.md
(static), and a PR that bumps a plugin's version must update its CHANGELOG.md
in the same diff (--check-bump).

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
@kyle-sexton kyle-sexton added the automated Opened by automation. label Jul 20, 2026
@claude

claude Bot commented Jul 20, 2026

Copy link
Copy Markdown

Claude finished @kyle-sexton's task in 3m 44s —— View job


PR Review — feat(ci): orphaned-fixture + CHANGELOG-parity gates (#663)

  • Gather context and understand request
  • Read all changed files
  • Review scripts for correctness and security
  • Review CI workflow changes
  • Review baseline files
  • Post findings

Overall this is a well-structured and thoughtful PR. The gate design is consistent with the repo's established patterns, the self-tests are comprehensive, and the baseline handling is carefully guarded against staleness. One correctness defect and two minor issues are worth addressing before merge.


Correctness

[Bug] Double stale-message in --check when a grandfathered plugin gains a CHANGELOG.md

scripts/check-changelog-parity.sh lines 75–95

When a grandfathered plugin (in the baseline) later ships a CHANGELOG.md, the script reports it stale twice — once inside the manifest loop (lines 77–79) and again in the second stale-scan loop (lines 91–94), because saw_debt["$name"] is never populated when the continue at line 81 is taken.

# path A — inside manifest loop
if [[ -f "$plugin_dir/CHANGELOG.md" ]]; then
  if [[ -n "${grandfathered[$name]:-}" ]]; then
    echo "STALE BASELINE: '$name' ..."   # reported HERE
    missing=$((missing + 1))
  fi
  continue   # ← saw_debt["$name"] never set
fi

# path B — outer stale-scan loop
for name in "${!grandfathered[@]}"; do
  if [[ -z "${saw_debt[$name]:-}" ]]; then
    echo "STALE BASELINE: '$name' ..."   # ALSO reported here (same entry)
    missing=$((missing + 1))
  fi
done

The gate still exits 1 (fail-closed is preserved), and the test at line 75 only checks *"STALE BASELINE"* so it passes even with the duplicate. But a user running the gate manually sees two errors for one entry, which is confusing.

Fix: set saw_debt["$name"]=1 before the continue in path A, so the second loop skips it:

if [[ -f "$plugin_dir/CHANGELOG.md" ]]; then
  if [[ -n "${grandfathered[$name]:-}" ]]; then
    echo "STALE BASELINE: '$name' in $BASELINE now has a CHANGELOG.md — remove it." >&2
    missing=$((missing + 1))
    saw_debt["$name"]=1   # ← prevents the second loop re-reporting this
  fi
  continue
fi

Fix this →


[Minor] --check-bump with no second argument exits 1, not 2

scripts/check-changelog-parity.sh line 105

base="${2:?usage: check-changelog-parity.sh --check-bump <base-ref>}"

${parameter:?word} is a POSIX construct that exits the shell with status 1, not 2. All other bad-invocation paths in the script exit 2. The test suite doesn't exercise this case (it exercises --nonsense for exit-2 and a valid-but-unresolvable ref for exit-2), so the inconsistency is undetected. This is cosmetic, but callers that distinguish exit codes would be misled.

Fix: replace with an explicit guard:

if [[ -z "${2:-}" ]]; then
  echo "usage: $(basename "$0") --check-bump <base-ref>" >&2
  exit 2
fi
base="$2"

[Minor] Fixture sort is not null-safe

scripts/check-orphaned-fixtures.sh lines 123–125

if ((${#fixtures[@]} > 0)); then
  mapfile -t fixtures < <(printf '%s\n' "${fixtures[@]}" | sort)
fi

This encodes fixture paths as newline-delimited input to sort. A path with a literal newline would split into multiple tokens and corrupt the array. The collection step (lines 117–121) correctly uses -print0 / -d '' for null safety; the sort step doesn't follow through. For files in a git repo this is a vanishingly unlikely real failure, but it's an internal inconsistency in the script's own null-safety discipline.

Null-safe alternative using mapfile + sort -z:

if ((${#fixtures[@]} > 0)); then
  mapfile -t -d '' sorted < <(printf '%s\0' "${fixtures[@]}" | sort -z)
  fixtures=("${sorted[@]}")
fi

Positive observations

  • Stale-guard on both baselines is the right design. A prefix/name that no longer covers real debt must be removed — the baseline cannot silently accumulate dead entries.
  • Deliberately generous consumption matching (path OR basename, evals.json OR test file) is the correct trade-off for a gate that must not block legitimate work.
  • --check-bump is never relaxed by the baseline: grandfathered plugins must start their changelog the moment they next bump. This is called out in the baseline comment and enforced correctly.
  • Self-test unconditionally before the gate step: prevents a broken detector from masking a real regression behind a green lane. Consistent with #593/#609/#622/#628.
  • find … -print0 / IFS= read -r -d '' used consistently in both scripts for null-safe filename handling.
  • grep -qF / -qwF (fixed string) avoids any injection risk through filenames.
  • CI job placement and ci-status needs: correctly includes both new lanes as required gates.

Summary

One correctness bug (double stale message), one exit-code inconsistency, and one null-safety inconsistency. None block the gate from doing its job correctly — the fail-closed behaviour is preserved in all cases. The double-message bug is the most user-visible and the simplest to fix; the other two are cosmetic. The overall design, test coverage, and CI integration are sound.

…p arg, null-safe sort

Resolves the claude review-bot findings on #681:
- check-changelog-parity.sh --check: mark a grandfathered plugin that gained a
  CHANGELOG.md as handled so the second stale-scan loop no longer double-reports it.
- check-changelog-parity.sh --check-bump: explicit missing-arg guard exiting 2,
  consistent with every other usage-error path (${2:?} exited 1).
- check-orphaned-fixtures.sh: null-safe fixture sort (sort -z / mapfile -d '').
- Tests: assert the stale message appears exactly once; cover --check-bump with
  no base ref -> exit 2.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
@claude

claude Bot commented Jul 20, 2026

Copy link
Copy Markdown

Claude finished @kyle-sexton's task in 1m 24s —— View job


PR Review — feat(ci): orphaned-fixture + CHANGELOG-parity gates (#663)

  • Gather context and understand request
  • Read changed scripts to verify review fixes were applied correctly
  • Verify fix 1: double stale-message dedupe (check-changelog-parity.sh)
  • Verify fix 2: exit-2 on missing bump arg (check-changelog-parity.sh)
  • Verify fix 3: null-safe sort (check-orphaned-fixtures.sh)
  • Post final review verdict

All three findings from the prior review are correctly addressed in e03985e.

Fix 1 — Double stale-message: ✅ Correct

check-changelog-parity.sh line 80: saw_debt["$name"]=1 is now set before the continue, preventing the outer stale-scan loop from re-reporting the same entry.

The test at check-changelog-parity.test.sh lines 76–77 was strengthened from checking *"STALE BASELINE"* (presence) to counting occurrences via grep -c and asserting stale_count == "1" — the test now actually enforces the no-duplicate guarantee.

Fix 2 — Exit-2 on missing bump arg: ✅ Correct

check-changelog-parity.sh lines 107–111: replaced ${2:?word} (exits 1) with an explicit if [[ -z "${2:-}" ]]; then ... exit 2; fi guard, consistent with all other bad-invocation paths.

A new test case was added at check-changelog-parity.test.sh lines 138–142 that specifically exercises --check-bump with no base ref and asserts exit 2, closing the test coverage gap the prior review noted.

Fix 3 — Null-safe sort: ✅ Correct

check-orphaned-fixtures.sh lines 123–125: sort now uses printf '%s\0' + sort -z + mapfile -t -d '' throughout, null-safe end to end and consistent with the find -print0 / read -d '' discipline in the collection step.


Summary

All prior findings resolved cleanly. No new issues introduced. The PR is in good shape to merge.

@claude

claude Bot commented Jul 20, 2026

Copy link
Copy Markdown

Claude finished @kyle-sexton's task in 2m 55s —— View job


PR Review — feat(ci): orphaned-fixture + CHANGELOG-parity gates (#663)

  • Gather context and understand request
  • Read changed files at current HEAD (e03985e)
  • Review check-changelog-parity.sh and its test
  • Review check-orphaned-fixtures.sh and its test
  • Review baseline files and CI workflow changes
  • Post final review verdict

This review covers the full current diff at e03985e. The three findings from the prior round of review are all correctly addressed (confirmed in the previous approval comment). No new issues found.


Correctness

All logic paths in both scripts are correct.

check-changelog-parity.sh --check — the two stale-detection loops are now deduplication-safe: saw_debt["$name"]=1 is set before the continue so the outer stale-scan loop does not double-report the same entry. The test at check-changelog-parity.test.sh:76–77 now asserts stale_count == "1" via grep -c, not just presence.

check-changelog-parity.sh --check-bump — the missing-arg guard at lines 107–111 uses an explicit if [[ -z "${2:-}" ]]; then ... exit 2 (not ${:?}) so this path exits 2 consistently with every other bad-invocation path. A new test case at lines 138–142 closes the coverage gap.

check-orphaned-fixtures.sh sortlines 123–125 now uses printf '%s\0' | sort -z | mapfile -t -d '' throughout, null-safe end to end, consistent with the find -print0 / read -d '' discipline in the collection step.

New-plugin edge case in --check-bump (no prior finding, verifying now): when a plugin is absent at the base ref, line 126 skips it — the static --check owns whether a brand-new plugin ships its initial CHANGELOG.md. Correct.

Autonomy --check-bump enforced despite baseline: plugins/autonomy has no CHANGELOG.md at either base or HEAD, so git diff --quiet "$base" -- "plugins/autonomy/CHANGELOG.md" returns 0 (no diff on a file that doesn't exist at either ref) → UNDOCUMENTED BUMP fires the moment autonomy bumps. The baseline comment and the PR description both call this out explicitly. Correct and intentional.


Security

  • All grep invocations use -qF or -qwF (fixed string) — no regex injection through filenames.
  • jq expressions use literal field selectors (.version // empty) — no injection surface.
  • git show "$base:$manifest"$base in CI is origin/${{ github.base_ref }}, a ref controlled by GitHub Actions, not user data.
  • Actions pinned to commit SHAs (actions/checkout@9c091bb21b7c1c1d1991bb908d89e4e9dddfe3e0); persist-credentials: false on both new jobs.

CI wiring

orphaned-fixture-gate correctly omits fetch-depth: 0 (static scan, no git history needed). changelog-parity-gate correctly sets fetch-depth: 0 (base ref must be resolvable for --check-bump). The --check-bump step is gated on github.event_name == 'pull_request'. Both lanes appear in the ci-status needs: list at lines 581–582.


Positive observations

  • Stale-guard on both baselines is the right design — a prefix/name that no longer covers real debt must be removed; the gate itself enforces this.
  • Self-test runs unconditionally before the gate step in both CI jobs — a broken detector cannot mask a real regression behind a green lane.
  • discover mode provides a human-readable three-state label (CONSUMED / GRANDFATHERED / ORPHAN) without being wired into CI — a useful debugging tool that doesn't affect gate semantics.
  • --check-bump never relaxed by the baseline is correctly implemented and called out in the baseline comment, the script header, and the PR description.
  • Test scaffolding (throwaway mktemp repos, isolated git histories for --check-bump cases) is correct and avoids any cross-contamination with the real repo state.

Summary

All prior findings resolved. No new issues found. The implementation is correct, the baselines are appropriately minimal and stale-guarded, the CI wiring is sound, and the test coverage is thorough. Ready to merge.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: e03985e90c

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread scripts/orphaned-fixtures-baseline.txt Outdated
…y prefix

The autonomy entry grandfathered a whole directory
(plugins/autonomy/skills/setup/evals/fixtures/), so any NEW orphaned
fixture later added under it would match the prefix and be silently
exempted — and because that directory already shadows many current
orphans, the prefix never goes stale to flag the drift. The gate could
pass wrongly (fail-open): a new ungraded fixture in this skill bypasses
the check entirely.

Replace the single directory prefix with an exact-path snapshot of the
173 currently-known orphans (otlp-demo + security-binding), same
prefix-matching idiom as scripts/docs-only-paths.txt but each line pins
one file. A new orphan added under an already-listed directory now
matches no line and is red-lined; staleness still fires per-file when an
entry's file is consumed or deleted. The knowledge youtube-digest entry
was already an exact file and is unchanged.

Verified at head e03985e: the enumerated set equals a fresh empty-baseline
discover (174 = 173 autonomy + 1 knowledge); --check on the real tree
passes; a probe fixture dropped under security-binding/ is now caught
(exit 1) where the directory prefix would have exempted it; the script's
own test suite stays green (7/7, including the stale-entry case).

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@claude

claude Bot commented Jul 20, 2026

Copy link
Copy Markdown

Claude finished @kyle-sexton's task in 2m 38s —— View job


PR Review — feat(ci): orphaned-fixture + CHANGELOG-parity gates (#663)

  • Gather context and understand request
  • Read what changed in latest commit (e822423 — "pin orphaned-fixture baseline to exact files")
  • Verify correctness of the new exact-file baseline approach
  • Check for any new issues introduced
  • Post final review verdict

This review covers the incremental change in e822423 against the e03985e state that was already approved in the prior two review passes.

Change Summary

The only file changed in e822423 is scripts/orphaned-fixtures-baseline.txt. The single directory-prefix entry (plugins/autonomy/skills/setup/evals/fixtures/) was expanded to ~190 exact-file-path entries, enumerating every current orphan in that skill's fixture corpus individually. Neither the detector script nor its tests were changed.

Correctness — Codex Finding Addressed Correctly ✅

The Codex finding was: a directory-prefix entry grandfathers any future orphan added under that directory, not just the snapshot of current ones. The fix is mechanically correct:

  • The script's matching ([[ "$path" == "$prefix"* ]]) still works the same way — a complete file path is a degenerate prefix of itself, so exact-path entries match only the single named file.
  • Any new orphan added under security-binding/ or otlp-demo/ will match no baseline entry and will be red-lined immediately. The gate now holds the line on new debt in this directory.
  • The stale-detection loop fires correctly for each exact-path entry: when a file is burned down (either consumed by a grader or deleted), matched_prefix is never called for it, prefix_used[exact-path] stays unset, and the stale check reports it. The burn-down mechanism works granularly per-file rather than needing the entire directory to be clean.

The baseline comment at the top was updated to explain this explicitly:

each line pins one file, never a directory: this is a snapshot of today's known orphans, so a NEW orphan added under an already-listed directory matches no line and is red-lined by the gate rather than silently grandfathered.

Nit — Script Docstring Still Says "path-prefix lines"

scripts/check-orphaned-fixtures.sh lines 34–37

# Existing orphan debt owned by another issue is grandfathered in
# scripts/orphaned-fixtures-baseline.txt (path-prefix lines, same idiom as
# scripts/docs-only-paths.txt).

The script header still describes entries as "path-prefix lines." With the baseline now using exact-file paths, this is slightly misleading — a reader might infer that directory-prefix entries are still the expected form. The baseline file's own comment has been updated correctly; the script header hasn't. This is cosmetic (the mechanism still works, and "path prefix" is technically accurate for an exact path), but a one-line clarification like "exact-path lines (each pins one file)" would keep the two in sync.

Fix this →

No New Issues

No correctness, security, or behavioral regressions were introduced. The self-tests and CI wiring are unchanged from the already-approved e03985e state.

Summary

e822423 correctly addresses the Codex finding. The baseline is now a genuine point-in-time snapshot of orphan debt, not an open-ended directory amnesty. One cosmetic docstring nit worth a follow-up, but non-blocking. The PR is in good shape to merge.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: e822423f03

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread scripts/check-orphaned-fixtures.sh Outdated
kyle-sexton and others added 2 commits July 20, 2026 08:40
…ixtures

Merging origin/main brought in 7 new ungraded security-binding fixtures
(escalation-*.json, from the WP runner-escalation lane) that no eval
files[] entry or test consumes. The gate correctly red-lined them on the
merge ref — it is catching NEW ungraded fixtures exactly as designed —
so the exact-path baseline snapshot must be refreshed against the merged
tree rather than the point where it was first cut.

Add the 7 escalation-* paths to the snapshot (now 181 = 180 autonomy +
1 knowledge). This is debt-tracking, not a grader wiring: the escalation
fixtures belong to the same ungraded security-binding class already owned
by issue #662, whose cleanup scope now covers them.

Verified on the merged tree (da0d5d4): the enumerated set equals a fresh
empty-baseline discover (181, identical); --check passes; a probe fixture
dropped under security-binding/ is still red-lined (exit 1); the script
test suite stays green (7/7); changelog-parity --check passes.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@claude

claude Bot commented Jul 20, 2026

Copy link
Copy Markdown

Claude finished @kyle-sexton's task in 5m 8s —— View job


PR Review — feat(ci): orphaned-fixture + CHANGELOG-parity gates (#663)

  • Gather context and understand the current state of the PR
  • Read the latest commit (57e8c69) — baseline snapshot refreshed for 7 new escalation fixtures
  • Assess the Codex P2 finding about prefix vs. exact-path matching
  • Developed fix — cannot push (repository write permission denied to this bot)
  • Post final review verdict with fix to apply

One open finding: Codex P2 — baseline still uses prefix matching

The Codex P2 finding from the e822423 review round is still open in the current HEAD (57e8c69). The commit that pinned the baseline to exact file paths (e822423) did not change the matching logic in the script — it only changed the baseline entries. The script still uses glob-prefix matching in both is_grandfathered() and matched_prefix(), which means a baseline entry for valid.json also silently grandfathers valid.json.bak, valid.jsonl, etc.

Root cause — scripts/check-orphaned-fixtures.sh lines 63–71 and 129–138:

# Both functions use [[ "$path" == "$prefix"* ]] (prefix/glob)
# which matches valid.json AND valid.json.bak, valid.jsonl, etc.
is_grandfathered() {
  local path="$1" prefix
  for prefix in "${prefixes[@]}"; do
    if [[ "$path" == "$prefix"* ]]; then   # ← prefix matching, not exact
      return 0
    fi
  done
  return 1
}

Fix — change both to exact equality:

is_grandfathered() {
  local path="$1" prefix
  for prefix in "${prefixes[@]}"; do
    if [[ "$path" == "$prefix" ]]; then    # ← exact match
      return 0
    fi
  done
  return 1
}

Same single-character change ("$prefix"*"$prefix") in matched_prefix() at line 132.

Update the test — the existing "grandfathered orphan" test at line 73 seeds a directory prefix (plugins/p/skills/s/evals/fixtures/) which exact-equality would never match; update to the exact file path, and add a synthetic test that proves orphan.md.bak is NOT grandfathered by a orphan.md entry:

# --- orphan grandfathered by an exact-path baseline entry -> passes ---------
repo="$(mk_repo $'plugins/p/skills/s/evals/fixtures/orphan.md\n')"
seed_skill "$repo" "plugins/p/skills/s" ''
printf 'x\n' >"$repo/plugins/p/skills/s/evals/fixtures/orphan.md"
if run_check "$repo" >/dev/null; then ok "grandfathered orphan passes --check"; else fail "grandfathered orphan wrongly failed"; fi
rm -rf "$repo"

# --- SYNTHETIC: same-name-prefix file NOT grandfathered by exact entry ------
repo="$(mk_repo $'plugins/p/skills/s/evals/fixtures/orphan.md\n')"
seed_skill "$repo" "plugins/p/skills/s" ''
printf 'x\n' >"$repo/plugins/p/skills/s/evals/fixtures/orphan.md"     # satisfies entry
printf 'x\n' >"$repo/plugins/p/skills/s/evals/fixtures/orphan.md.bak"  # must still be caught
out="$(cd "$repo" && bash scripts/check-orphaned-fixtures.sh --check 2>&1)"
rc=$?
if [[ $rc -ne 0 && "$out" == *"ORPHANED FIXTURE"*"orphan.md.bak"* ]]; then ok "same-prefix file is not grandfathered (exact match only)"; else fail "same-prefix file wrongly grandfathered: rc=$rc out='$out'"; fi
rm -rf "$repo"

Also update the stale test's baseline entry from the directory prefix to an exact path, and update the inline comment at line 57 from "Active baseline prefixes:" to "Active baseline entries (exact paths):".

Docstring — the script header at line 34 still says "path-prefix lines, same idiom as scripts/docs-only-paths.txt" (a nit flagged in the e822423 review). This is now actively misleading since the matching is exact. Suggested replacement:

# Existing orphan debt owned by another issue is grandfathered in
# scripts/orphaned-fixtures-baseline.txt (exact-path lines; each line must match
# a fixture path exactly — no prefix or glob expansion). --check fails on a stale
# entry (one that no longer shadows any orphan) so the baseline cannot outlive its
# debt.

The fix is four mechanical edits: two "$prefix"*"$prefix" substitutions in the script, a test update, and a comment update. The baseline file itself (orphaned-fixtures-baseline.txt) needs no changes — every entry is already an exact path.


Everything else is correct

The latest commit (57e8c69) correctly refreshes the baseline for the 7 escalation fixtures brought in by the origin/main merge (da0d5d4). The gate caught them as new orphans exactly as designed, and adding them to the snapshot is the right response: they're the same ungraded security-binding class already owned by #662.

check-changelog-parity.sh — all prior findings remain fixed. No new issues.

CI wiring — both lanes correctly included in ci-status needs:. No regression in ci.yml.

The single remaining open item is the prefix-vs-exact matching bug described above. It's the last thing between this PR and a fully correct implementation.

Fix this →

@kyle-sexton

Copy link
Copy Markdown
Contributor Author

Baseline refreshed against a merged tree (57e8c69, on merge commit da0d5d4).

Merging origin/main brought in 7 new ungraded security-binding fixtures (escalation-*.json, from the WP runner-escalation lane). The orphaned-fixture gate red-lined them on the merge ref — that is the gate working as designed, catching NEW ungraded fixtures on contact, which is exactly the fail-open hole the exact-path snapshot closed. Added the 7 paths to the snapshot (now 181 = 180 autonomy + 1 knowledge); verified set-equality vs a fresh empty-baseline discover, --check green, probe-fixture still red-lined, tests 7/7.

Operational note on the snapshot idiom: the baseline is a merge-ref snapshot of known orphans, not a point-in-time cut. Because the exact-path lines intentionally do not grandfather whole directories, any ungraded fixture that lands on main under an already-listed skill before this branch merges will red-line the merge-ref CI run until the baseline is refreshed (merge main, regenerate from discover, re-verify). That is the intended cost of hold-the-line strictness — the alternative (directory prefixes) is precisely the fail-open behavior this PR removed.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 57e8c69078

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread scripts/check-changelog-parity.sh Outdated
…ates

Both gates could pass wrongly on inputs their own matching logic mishandled.

1. Orphaned-fixture baseline was consumed as a PREFIX, not an exact path.
   `[[ $path == $entry* ]]` meant a baselined `.../valid.json` also shadowed
   a new unconsumed `.../valid.json.bak` or `.../valid.jsonl` sibling, so an
   ungraded fixture could be added next to a grandfathered one and slip the
   gate. Match by full-string equality (`[[ $path == $entry ]]`) in both
   is_grandfathered and the staleness scan; rename prefix->entry throughout so
   the glob cannot be reintroduced. This also finishes closing the round-1
   directory-grandfathering hole: an exact path shadows only itself.

2. CHANGELOG-parity --check-bump only checked `git diff --quiet` — that the
   changelog was touched at all. A whitespace/title/old-entry edit satisfied a
   version bump. Require the bumped head version's own `## [<version>]` entry to
   exist in the changelog at head (fixed-string grep, tolerant of a trailing
   date; a missing file counts as undocumented). Strictness is deliberate: a
   bump documented as `## 1.1.0` (no brackets) fails — enforcing the repo's
   Keep-a-Changelog `## [x.y.z]` heading format.

Comments in both scripts and the orphaned-fixtures baseline header updated to
describe exact-match / version-entry semantics (the prior "same idiom as
docs-only-paths.txt" prefix framing no longer holds for orphaned fixtures).

Tests: orphaned-fixtures gains a `.bak`/`.jsonl` sibling red-lines case and its
two prefix-baseline cases become exact-path cases (8/8). changelog-parity gains
a bump + unrelated-changelog-edit fails case; the pass case moves to the real
`## [x.y.z]` format (12/12).

Verified on the merged tree: both self-tests green; orphaned --check and
changelog --check pass; the CI-identical --check-bump origin/main passes; a
real `.bak` sibling of a baselined fixture now red-lines (exit 1) where the
prefix match would have exempted it; shellcheck clean at --severity=style.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@claude

claude Bot commented Jul 20, 2026

Copy link
Copy Markdown

Claude finished @kyle-sexton's task in 1m 49s —— View job


PR Review — feat(ci): orphaned-fixture + CHANGELOG-parity gates (#663)

  • Gather context and understand request
  • Read check-orphaned-fixtures.sh at HEAD — verify prefix→exact fix
  • Read check-orphaned-fixtures.test.sh at HEAD — verify new test coverage
  • Read check-changelog-parity.sh at HEAD — verify version-heading validation fix
  • Read check-changelog-parity.test.sh at HEAD — verify new test coverage

This review covers the latest commit (0b6f1b1), which addresses the two remaining Codex P2 findings from the prior review round.

Fix 1 — Exact-path matching in check-orphaned-fixtures.sh: ✅ Correct

The Codex P2 finding was: baseline entries were used as prefixes ([[ "$path" == "$prefix"* ]]), so valid.json would silently grandfather valid.json.bak and valid.jsonl.

Both is_grandfathered() (line 69) and matched_baseline() (line 135) now use [[ "$path" == "$entry" ]] — exact equality, no glob. The internal variable is renamed from prefix/prefixes to entry/entries, removing the prefix-idiom signal. The script header (lines 34–38) now explicitly documents "matched by full-string equality, NOT prefix".

Test coverage: Two new cases in check-orphaned-fixtures.test.sh:

  • Exact-path entry grandfathers the named file (positive case).
  • A valid.json entry does not grandfather valid.json.bak or valid.jsonl — both siblings are caught and named in the error output (negative case proving no prefix leak). This is the synthetic proof the fix is behaviorally effective.

Fix 2 — Version-heading validation in check-changelog-parity.sh: ✅ Correct

The Codex P2 finding was: --check-bump checked only that CHANGELOG.md differed from the base ref, so an unrelated edit (whitespace, title, old entry) would satisfy the gate without documenting the new version.

Line 138 now checks grep -Fq "## [$head_version]" "$changelog" — the CHANGELOG must contain the specific version heading (## [1.1.0] for a 1.0.0→1.1.0 bump), not merely be different. Using -F keeps the version string literal (the . in 1.1.0 is not a regex wildcard). A missing file is also caught in the same condition. The script header (lines 19–25) explicitly calls out the design decision: "an unrelated edit — whitespace, title, an old release — must not satisfy the gate."

Test coverage: Two new cases in check-changelog-parity.test.sh:

  • Bump to 1.1.0 with ## [1.1.0] entry in the CHANGELOG passes (positive case, lines 82–92).
  • Bump to 1.1.0 with a typo-fix to the changelog title (no ## [1.1.0] entry added) fails with UNDOCUMENTED BUMP (lines 94–109). This is exactly the unrelated-edit bypass the Codex finding described, and the test proves the gate catches it.

No new issues introduced

The changes are tightly scoped to the two findings:

  • No regressions in the existing test suite (all prior passing cases still pass — the exact-path baseline test at line 72 updates the baseline seed from a directory prefix to the exact file path, which is correct for the new semantics).
  • The --check and discover modes in check-orphaned-fixtures.sh are unaffected.
  • check-changelog-parity.sh --check is unaffected.
  • CI wiring and baseline files are unchanged.

Summary

Both Codex P2 findings are correctly resolved. The gates are now fully closed against the identified bypass vectors, the test suites include synthetic negative cases proving each fix, and no new issues were introduced. The PR is ready to merge.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 0b6f1b1303

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread scripts/check-changelog-parity.sh Outdated
Follow-up hardening on the CHANGELOG-parity gate plus the normalization the
gate now requires.

1. Normalize five plugins' CHANGELOG headings to the documented bracketed
   Keep-a-Changelog form (`## [x.y.z]`), preserving each file's existing date
   suffix: discovery, docs-hygiene, knowledge, playbooks, session-flow. These
   bump daily; unbracketed headings would false-fail --check-bump on their next
   bump as a CHANGELOG FORMAT error. Format-only — no version bumps, no content
   changes.

2. Split the --check-bump failure taxonomy. A bumped version documented with an
   unbracketed heading (`## <version>`) is now a distinct CHANGELOG FORMAT error
   that names the found heading and the expected bracketed form, rather than an
   UNDOCUMENTED BUMP. An author can fix format in place; conflating it with a
   missing release entry was misleading.

3. Close the pre-existing-entry fail-open. --check-bump passed whenever the
   bumped version's `## [<version>]` heading was merely present at head — even
   if it already existed in the base changelog, meaning the bump reused an old
   heading and shipped no new release note. Require the entry to be ADDED by the
   change set: present at head AND absent from the changelog at <ref>. A reused
   pre-existing heading is now reported as PRE-EXISTING CHANGELOG ENTRY.

4. Cite issue #688 in the orphaned-fixtures baseline comment for
   variation-matrix-backlog.json, replacing the un-owned "needs triage" note now
   that the fixture has a needs-triage grade-or-demote issue.

Tests: check-changelog-parity gains a FORMAT-split case, a newly-added-entry
pass case, and a pre-existing-entry fail case (15/15). Verified on the merged
tree (branch + origin/main): both self-tests green, --check and the CI-identical
--check-bump origin/main green; shellcheck clean at --severity=style.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@claude

claude Bot commented Jul 20, 2026

Copy link
Copy Markdown

Claude finished @kyle-sexton's task in 3m 55s —— View job


PR Review — feat(ci): orphaned-fixture + CHANGELOG-parity gates (#663)

  • Gather context and understand request
  • Read the diff for the latest commit (c1c3830)
  • Verify the Codex P2 finding (pre-existing-entry fail-open) is resolved
  • Verify format-taxonomy split and CHANGELOG normalization
  • Check for any new issues introduced
  • Post final review verdict

This review covers the incremental change in c1c3830. The prior round's approved state was 0b6f1b1.

Fix 1 — Pre-existing-entry fail-open: ✅ Correct

The last open Codex P2 finding (classified "VALID defer" by kyle-sexton with the carve-out note that it's a fail-open) is resolved in this commit.

Mechanism: check-changelog-parity.sh lines 146–158. The outer if checks whether ## [$head_version] exists at HEAD. When it does, the inner if gates on whether that same heading is also present at $base:

if [[ -f "$changelog" ]] && grep -Fq "## [$head_version]" "$changelog"; then
    if git show "$base:$changelog" 2>/dev/null | grep -Fq "## [$head_version]"; then
        echo "PRE-EXISTING CHANGELOG ENTRY: ..."
        preexisting=$((preexisting + 1))
    fi
    continue   # always — avoids double-counting
fi

The continue runs unconditionally at the end of the outer block regardless of whether preexisting was incremented. This is correct — only one of the three failure buckets (undocumented, malformed, preexisting) can fire per plugin per loop iteration. All three are aggregated into a single exit 1 at line 173.

New file case handled correctly: When a CHANGELOG is brand-new in this PR (absent at $base), git show "$base:$changelog" exits non-zero with no output, grep exits 1 (no match), pipefail propagates the non-zero status — so the inner if is false and the entry is correctly treated as "newly added." The comment on line 151 documents this explicitly.

Fix 2 — Format-taxonomy split: ✅ Correct

A bumped version documented as ## 1.1.0 (no brackets) now reports CHANGELOG FORMAT rather than UNDOCUMENTED BUMP. The format-detection regex at line 164:

esc="${head_version//./\\.}"
if found="$([[ -f "$changelog" ]] && grep -m1 -E "^##[[:space:]]+${esc}([[:space:]]|\$)" "$changelog")"; then

The dot-escaping is correct (1.1.01\.1\.0 in the ERE). The \$ inside double quotes reduces to $ (end-of-line in ERE), correctly requiring the version string to end at a word boundary (whitespace or EOL). The ## [1.1.0] form does NOT match this pattern (the [ after ##<space> prevents the match), so a correctly-bracketed heading will never be double-reported as a FORMAT error.

Fix 3 — CHANGELOG normalization (five plugins): ✅ Format-only

discovery, docs-hygiene, knowledge, playbooks, session-flow — all changes are mechanical bracket additions only (## 0.7.3 — date## [0.7.3] — date). No version bumps, no content changes. The normalization is necessary so these frequently-bumped plugins don't false-fail --check-bump the next time they increment.

Test suite: ✅ Three new cases, 15/15

# Test Type
6 bump + unbracketed heading → FORMAT error (not UNDOCUMENTED) negative — taxonomy split
7 bump adding a NEW '## [x.y.z]' entry (absent at base) passes --check-bump positive — proves new entries pass
8 bump reusing a base-pre-existing '## [x.y.z]' entry fails --check-bump negative — proves the Codex P2 fix

The pre-existing test (case 8) correctly constructs a base commit where the plugin version is 1.0.0 but ## [1.1.0] is already in the changelog, then bumps to 1.1.0 with only a plugin.json edit — exactly the Codex-described escape path. The assertion checks for PRE-EXISTING CHANGELOG ENTRY in output and the absence of UNDOCUMENTED BUMP, validating both the correct classification and the taxonomy split.

No new issues

All prior approved findings remain fixed. No correctness, behavioral, or security regressions were introduced. CI wiring and both baselines are unchanged.


Summary

c1c3830 closes the last open finding from this review cycle. The pre-existing-entry fail-open is correctly closed with a git show $base:$changelog | grep check, the failure taxonomy is correctly split into three distinct diagnostic buckets, and the five normalizations are confirmed format-only. The PR is ready to merge.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: c1c3830c17

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread scripts/check-changelog-parity.sh Outdated
…ding

Codex P2 on #681: the fixed-string version match also accepted the bumped
version appearing in prose or an indented/fenced example, satisfying (or
falsely pre-existing) the release entry without a real heading. Both the
head and base checks now match an anchored Markdown heading; self-test case
added proving a prose/indented mention fails as UNDOCUMENTED.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01FM1RfM3jHkgenpdbMv4o64
@claude

claude Bot commented Jul 20, 2026

Copy link
Copy Markdown

Claude finished @kyle-sexton's task in 2m 7s —— View job


PR Review — feat(ci): orphaned-fixture + CHANGELOG-parity gates (#663)

  • Gather context and understand request
  • Read check-changelog-parity.sh at HEAD — verify line-start anchor fix (8c028dc)
  • Read check-changelog-parity.test.sh at HEAD — verify new test coverage
  • Check for any new issues introduced
  • Post final review verdict

This review covers the incremental change in 8c028dc, which addresses the last Codex P2 finding from the c1c3830 review round (unanchored release-entry match).

Fix — Line-start anchor on the release-entry match: ✅ Correct

The Codex finding was: grep -Fq "## [$head_version]" is a substring match, so ## [1.1.0] appearing in prose, a string literal, or an indented example anywhere in the file satisfies the gate. The same unanchored match on the base changelog could falsely classify a prose mention as a "pre-existing" entry.

Both call-sites are now anchored identically. lines 147–149:

esc="${head_version//./\\.}"
heading_re="^## \[${esc}\]"
if [[ -f "$changelog" ]] && grep -Eq "$heading_re" "$changelog"; then

And the base check at line 156:

if git show "$base:$changelog" 2>/dev/null | grep -Eq "$heading_re"; then

Regex construction is correct. For head_version="1.1.0":

  • esc="1\.1\.0" (dots escaped for ERE)
  • heading_re="^## \[1\.1\.0\]" — in ERE: ^ (line-start), ## (literal), \[ (literal [), 1\.1\.0 (literal 1.1.0), \] (literal ])

A line-start match correctly rejects:

  • "## [1.1.0]" inside a quoted string — ^ fails (line starts with ")
  • ## [1.1.0] (example)^ fails (line starts with spaces)
  • `` — ^ fails (line starts with `<`)

A trailing — date (## [1.1.0] — 2026-07-20) still matches — the pattern anchors the start, not the end. That is the intended behaviour, matching the repo's existing changelog style.

esc is computed once before both checks, removing the prior duplication where esc was computed only in the FORMAT branch (the comment at line 165 correctly notes "esc computed above"). No functional change.

New test case: ✅ Correct and specific

check-changelog-parity.test.sh lines 141–158 adds a test that seeds a CHANGELOG containing two deliberate non-heading mentions of ## [1.1.0]:

Next release will be titled "## [1.1.0]" per convention.
  ## [1.1.0] (example, indented, not a heading)

The assertion checks for UNDOCUMENTED BUMP (the head match failed) and the absence of PRE-EXISTING (the base match was not triggered either). This proves both anchor call-sites are effective — a prose mention doesn't satisfy the gate and doesn't falsely poison the base check.

Test suite is now 16/16, matching kyle-sexton's verification note. The test count in the PR description was 15/15 for the prior round; the new case correctly adds one.

No new issues

The change is tightly scoped to the two grep -Fgrep -E substitutions and the esc hoisting. All prior approved findings remain fixed. --check is unaffected. The FORMAT branch at line 166 was already anchored with ^##[[:space:]] from the prior round and is unmodified here.


Summary

8c028dc correctly closes the last open Codex P2 finding. The release-entry match is now anchored to ^## [<version>] in both the head and base checks, a prose or indented mention cannot bypass the gate, and the new test case proves it end-to-end. All prior findings resolved; no new issues introduced. The PR is ready to merge.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 8c028dcebf

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread scripts/check-orphaned-fixtures.sh Outdated
Comment thread scripts/check-changelog-parity.sh Outdated
…ng check

Two Codex P2s on #681, one per gate:
- orphaned-fixture: grep -w treated "." as a word boundary, so a referenced
  valid.json also consumed a new unconsumed valid.json.bak sibling. Basename
  matches now require non-filename-character neighbors around an
  ERE-escaped basename.
- changelog-parity: the heading match interpolated the version into an ERE
  with only dots escaped, so SemVer build metadata (1.0.1+build.1) broke the
  pattern and false-failed a documented bump. The heading check is now a
  fixed-string line-anchored awk match (no regex at all); the FORMAT-split
  probe escapes all ERE metacharacters.

Self-tests: suffix-sibling orphan case + build-metadata round-trip case
added; orphan suite 9/9, parity suite 17/17.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01FM1RfM3jHkgenpdbMv4o64
@claude

claude Bot commented Jul 20, 2026

Copy link
Copy Markdown

Claude finished @kyle-sexton's task in 5m 11s —— View job


PR Review — feat(ci): orphaned-fixture + CHANGELOG-parity gates (#663)

  • Gather context and understand current PR state
  • Read check-orphaned-fixtures.sh at HEAD — verify the suffix-sibling fix (91571eb)
  • Read check-orphaned-fixtures.test.sh — verify new test coverage
  • Read check-changelog-parity.sh and test — verify build-metadata fix and assess remaining Codex P2
  • Check for any new issues introduced
  • Post final review verdict

This review covers the latest commit (91571eb), which addresses the two remaining Codex P2 findings from the 8c028dcebf review round.

Fix 1 — Suffix-sibling consumption (orphaned-fixture gate): ✅ Correct

The Codex finding: grep -wF "valid.json" treats . as a word boundary, so a referenced valid.json entry also consumed a new unconsumed valid.json.bak sibling — a fresh orphan passed invisibly.

scripts/check-orphaned-fixtures.sh lines 99–122: both grep -qwF "$base" calls are replaced with an ERE bounded match:

esc_base="$(printf '%s' "$base" | sed -E 's/[][\\.|$(){}?+*^]/\\&/g')"
base_re="(^|[^A-Za-z0-9._-])${esc_base}([^A-Za-z0-9._-]|$)"

The character class [^A-Za-z0-9._-] (not a letter, digit, ., _, or -) correctly requires that the fixture name be surrounded by non-filename characters or appear at start/end-of-string. For valid.json.bak: the character after valid.json is ., which IS in [A-Za-z0-9._-], so it does NOT match [^…] — the suffix sibling is rejected. The ERE escaping handles all metacharacters including [, ], +, etc. via the sed pattern.

Test coveragecheck-orphaned-fixtures.test.sh lines 63–73: seeds an evals.json referencing only valid.json, places both valid.json and valid.json.bak in the fixture dir, and asserts:

  • valid.json.bakORPHANED FIXTURE (the suffix sibling red-lines)
  • valid.json → NOT flagged (the referenced file still passes)

Both assertions in one test, proving the fix closes the hole without false-positiving the legitimate fixture. Suite is now 9/9.


Fix 2 — SemVer build-metadata in heading regex (changelog-parity gate): ✅ Correct

The Codex finding: the grep -Eq "$heading_re" check only escaped dots, so 1.0.1+build.1 had + interpreted as an ERE quantifier — a correctly-documented bump would false-fail, blocking any plugin using build-metadata versions.

scripts/check-changelog-parity.sh lines 148–160: the grep -Eq "$heading_re" calls (both head and base) are replaced with an awk index() fixed-string match:

heading="## [${head_version}]"
has_heading() { awk -v h="$heading" 'index($0, h) == 1 { found=1; exit } END { exit !found }'; }

awk index(s, t) is a literal fixed-string search — no regex engine is involved. 1.0.1+build.1 in h is treated as a plain string, + is not a quantifier. The == 1 constraint means position 1 (1-indexed first character), so a line-start anchor is enforced without regex. Both has_heading <"$changelog" (head check) and git show "$base:$changelog" | has_heading (base/pre-existing check) use the same function. ✓

The FORMAT branch's ERE probe at line 169–170 now escapes ALL ERE metacharacters ([ ] \ . | $ ( ) { } ? + * ^) via the same sed pattern, so 1.0.1+build.11\.0\.1\+build\.1 in the unbracketed-heading ERE. ✓

Test coveragecheck-changelog-parity.test.sh lines 141–173: two new cases close the gaps:

Case What it proves
SemVer build-metadata version with a proper entry passes (no regex leak) ## [1.0.1+build.1] entry passes --check-bump; + is not misread as a quantifier
version string in prose/indented example does not satisfy the anchored heading match An indented ## [1.1.0] or a quoted mention never satisfies either the head or pre-existing check

Suite is now 17/17.


No new issues

The changes in 91571eb are tightly scoped to the four touch points (two grep -qwFgrep -qE in the orphan script; two grep -Eq "$heading_re"has_heading with awk index() in the parity script). All prior approved findings remain fixed:

  • Exact-path baseline matching (no prefix leak) ✓
  • Stale-baseline deduplication (single message per entry) ✓
  • Exit-2 on missing --check-bump arg ✓
  • Null-safe sort via sort -z
  • PRE-EXISTING vs UNDOCUMENTED failure taxonomy ✓
  • Anchored heading format for prose mentions ✓

CI wiring, baseline files, and the --check (static) path are unchanged.


Summary

Both Codex P2 findings from the 8c028dcebf round are correctly resolved in 91571eb. The orphaned-fixture gate now uses a character-class-bounded ERE pattern that correctly rejects suffix-extended siblings of referenced fixtures. The changelog-parity gate now uses a awk index() fixed-string match for the primary heading check, eliminating all regex interpretation of version strings (including SemVer build metadata). Both fixes have targeted synthetic test cases proving the specific failure mode is closed. No new issues introduced. The PR is ready to merge.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 91571eb1d7

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread scripts/check-orphaned-fixtures.sh Outdated
Comment thread scripts/check-changelog-parity.sh Outdated
@claude

claude Bot commented Jul 20, 2026

Copy link
Copy Markdown

Claude finished @kyle-sexton's task in 4m 20s —— View job


PR Review — feat(ci): orphaned-fixture + CHANGELOG-parity gates (#663)

  • Gather context and understand request
  • Read check-orphaned-fixtures.sh at HEAD — assess Codex P2 finding (substring match on files[])
  • Read check-changelog-parity.sh at HEAD — assess Codex P2 finding (fenced-code bypass)
  • Check for any new issues introduced by the merge commit
  • Post final review verdict

This review covers the current HEAD (18573a4), a merge of origin/main into this branch. The merge brought in only one unrelated commit (4b5fffd — work-items fix); neither gate script nor its test changed in the merge. The two remaining open Codex P2 findings from the 91571eb review round are assessed below.


Finding 1 — Path substring match is a real fail-open ✅ VALID

scripts/check-orphaned-fixtures.sh line 109

if grep -qF "$rel" "$evals_json"; then
  return 0
fi

grep -qF "$rel" is a fixed-string substring search. $rel is the skill-relative path of the fixture being checked, e.g. evals/fixtures/valid.json. If evals.json contains an eval that references evals/fixtures/valid.json.bak (a different fixture), then grep -qF "evals/fixtures/valid.json" finds evals/fixtures/valid.json as a substring of evals/fixtures/valid.json.bak and returns 0 — the orphan valid.json is incorrectly treated as consumed.

Why the existing test does not catch this: The suffix-sibling test at lines 63–73 seeds "evals/fixtures/valid.json" in files[] and asserts that the unchecked orphan valid.json.bak is caught. That exercises the basename direction (referenced file vs. longer-named orphan). The Codex finding describes the path direction (referenced file has a longer name, shorter-named file is the orphan) — this direction is not tested, and the path check at line 109 is not guarded.

The fix is to apply the same character-class-bounded ERE approach to $rel as is already applied to $base:

# Path match: require boundaries so evals/fixtures/valid.json.bak in files[]
# does not consume the separate, unconsumed evals/fixtures/valid.json.
esc_rel="$(printf '%s' "$rel" | sed -E 's/[][\\.|$(){}?+*^]/\\&/g')"
if grep -qE "(^|[^A-Za-z0-9._-])${esc_rel}([^A-Za-z0-9._-]|$)" "$evals_json"; then
  return 0
fi

After evals/fixtures/valid.json, the character . (before .bak) IS in [A-Za-z0-9._-], so the bounded match rejects the longer path. A terminating " or , (the normal JSON delimiters) are NOT in that class, so a legitimate exact reference still matches.

The test should add a case seeding '"evals/fixtures/valid.json.bak"' in files[] and asserting that evals/fixtures/valid.json is caught as an orphan — the reverse of the current suffix-sibling test.


Finding 2 — has_heading does not track fenced-code state ✅ VALID (lower severity)

scripts/check-changelog-parity.sh line 149

has_heading() { awk -v h="$heading" 'index($0, h) == 1 { found=1; exit } END { exit !found }'; }

The code comment at lines 144–147 claims "the version string appearing in prose or a fenced example never satisfies." This claim is incorrect. awk index($0, h) == 1 checks whether the heading starts at column 1 of the current line, but the function has no fenced-code state machine. A CHANGELOG.md like:

## [1.0.0]

For reference, here's the heading format:

[1.1.0] — 2026-07-20

…would have the awk fire on ## [1.1.0] — 2026-07-20 inside the fence (it appears at column 1), satisfying has_heading. The gate would then check whether it was absent from the base changelog and pass the bump — without any real release entry.

The existing prose/indented test at lines 156–173 only exercises a version mention inside a prose sentence and an indented line — neither of which starts at column 1. The fenced-code case (version heading at column 1 inside backtick fences) is not tested.

Practical severity: This is a real but low-probability bypass — an author would need to write a fenced block containing ## [1.1.0] at the start of a line in their CHANGELOG.md. It would be visible in the PR diff. The risk is real but far lower than Finding 1.

The fix is to add fenced-code tracking to the awk:

has_heading() {
  awk -v h="$heading" '
    /^[[:space:]]*```/ { in_fence = !in_fence; next }
    !in_fence && index($0, h) == 1 { found=1; exit }
    END { exit !found }
  '
}

Also update the comment at line 144 to accurately describe what is and is not excluded:

# … a fenced example (backtick fences tracked) cannot satisfy it either

Applicability of the established carve-out

The carve-out rule ratified across this PR's review history: "Because this PR ships CI gates, any finding where the gate can pass wrongly (fail-open) is fixed, not deferred."

Both findings are gate fail-opens:

  • Finding 1: An orphaned fixture is silently marked consumed — the orphaned-fixture gate misses it.
  • Finding 2: A fenced-code mock entry satisfies the has_heading check — the bump gate passes without a real release note.

Neither is a corner case the gate's generosity tradeoff is intended to cover; both are unguarded paths where the gate gives a wrong green.


Everything fixed in prior rounds is still correct

All prior findings are resolved at HEAD. The merge commit introduced no regressions to the gate logic; the only substantive change it brought in (4b5fffd) is unrelated to these scripts.

Fix this →

…d headings

Two Codex P2s on #681, both fail-open in the gates' matching:

1. check-orphaned-fixtures.sh: the files[] PATH check used an unbounded
   fixed-string search of the whole evals.json, so an eval referencing
   evals/fixtures/valid.json.bak marked a NEW unconsumed evals/fixtures/valid.json
   sibling consumed (shorter $rel is a substring of the longer value). Compare
   $rel against each JSON string value EXACTLY with jq; the bounded-basename
   check remains the fallback. (Complements the earlier basename-boundary fix,
   which left this path branch open.)

2. check-changelog-parity.sh: the awk release-heading predicate matched
   `## [<version>]` at column zero without tracking fenced-code state, so a bump
   adding only a ``` fenced example containing the heading satisfied the gate —
   an undocumented release. The predicate now toggles fence state on ``` / ~~~
   lines and matches only outside fences; the overclaiming comment is corrected.

Self-tests: orphaned adds a files[]-substring case (10/10); changelog adds a
fenced-block case (18/18). shellcheck clean at --severity=style and against
.shellcheckrc; orphaned --check + set-equality (181, unchanged — the stricter
path match reclassified no real consumption), changelog --check, and
--check-bump origin/main all green.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01FM1RfM3jHkgenpdbMv4o64
@claude

claude Bot commented Jul 20, 2026

Copy link
Copy Markdown

Claude finished @kyle-sexton's task in 2m 48s —— View job


PR Review — feat(ci): orphaned-fixture + CHANGELOG-parity gates (#663)

  • Gather context and understand request
  • Read latest commit (edf355b) — files[] substring fix and fenced-heading fix
  • Verify fix 1: bounded path match in check-orphaned-fixtures.sh
  • Verify fix 2: fenced-code tracking in check-changelog-parity.sh
  • Verify test coverage for both fixes
  • Post final review verdict

This review covers the latest commit (edf355b), which addresses the two remaining Codex P2 findings from the 18573a4 review round.

Fix 1 — files[] path match is now exact (check-orphaned-fixtures.sh): ✅ Correct

The Codex finding: grep -qF "$rel" is an unbounded substring search, so an eval referencing the longer path evals/fixtures/valid.json.bak would silently consume the unconsumed shorter sibling evals/fixtures/valid.json.

scripts/check-orphaned-fixtures.sh line 114:

if jq -e --arg r "$rel" 'any(.. | strings; . == $r)' "$evals_json" >/dev/null 2>&1; then
  return 0
fi

jq recurses through all string values in evals.json and compares each to $rel with exact equality (. == $r). A files[] entry of evals/fixtures/valid.json.bak does NOT match evals/fixtures/valid.json — the old substring-match fail-open is closed. The jq -e form exits non-zero when any(...) returns false, so the if branch falls through to the bounded-basename check exactly as intended.

Graceful degradation: when jq fails (not installed or invalid JSON), the 2>/dev/null suppression ensures the if jq ... branch evaluates to false, and the basename-bounded grep -qE "$base_re" check runs as a fallback. The fallback is intentionally generous but not a fail-open: the character-class boundary ([^A-Za-z0-9._-]) still rejects suffix-extended siblings. Since jq is already a declared dependency of the companion check-changelog-parity.sh script in the same repo, degraded behaviour is a theoretical edge case.

Test coveragecheck-orphaned-fixtures.test.sh lines 75–86: seeds "evals/fixtures/valid.json.bak" in files[], places both valid.json.bak and valid.json in the fixture dir, and asserts:

  • valid.jsonORPHANED FIXTURE (the shorter unconsumed path is caught)
  • valid.json.bak → not flagged (the referenced file remains consumed)

This is exactly the reverse of the prior suffix-sibling test and proves the path-direction of the finding is closed. Suite is now 10/10.


Fix 2 — Fenced-code tracking in has_heading (check-changelog-parity.sh): ✅ Correct

The Codex finding: awk index($0, h) == 1 checks column-zero position but has no fence state machine, so ## [1.1.0] at column 1 inside a ``` block would satisfy the gate without a real release heading.

scripts/check-changelog-parity.sh lines 150–156:

/^[[:space:]]*(```|~~~)/ { infence = !infence; next }
!infence && index($0, h) == 1 { found = 1; exit }
END { exit !found }

The fence-toggle fires on lines matching both ``` and ~~~ openers/closers (CommonMark supports both), with optional leading whitespace ([[:space:]]*). Lines inside a fence (infence) are skipped entirely. The column-zero anchor (index == 1) and fixed-string matching (index() rather than a regex) are unchanged. The same has_heading function is used for both the HEAD check and the BASE pre-existing check (line 164), so a fenced example in the base changelog also cannot falsely trigger PRE-EXISTING CHANGELOG ENTRY.

One cosmetic note worth flagging: if a CHANGELOG has an unclosed fence (malformed file), all content below the opening ``` is treated as fenced and a real heading below it would fail as UNDOCUMENTED BUMP. This is fail-closed (not a gate bypass) and such a CHANGELOG would be malformed in practice; it is not a correctness issue.

Test coveragecheck-changelog-parity.test.sh lines 175–191: seeds a CHANGELOG where ## [1.1.0] appears at column zero inside a ``` block (the exact bypass scenario), asserts the result is UNDOCUMENTED BUMP and NOT `PRE-EXISTING`. Both assertions are needed — the first proves the fence guard works, the second proves the base check is also correctly blocked. Suite is now 18/18.


No new issues introduced

The changes in edf355b are scoped to the two affected call-sites: grep -qF "$rel"jq -e with a grep -qE fallback in consumed(), and the awk fence-state machine added to has_heading. All prior approved findings remain resolved:

  • Exact-path baseline matching (no prefix leak) ✓
  • Stale-baseline deduplication (single message per stale entry) ✓
  • Exit-2 on missing --check-bump arg ✓
  • Null-safe sort via sort -z
  • Suffix-sibling basename boundary (.bak not consumed by valid.json reference) ✓
  • PRE-EXISTING / FORMAT / UNDOCUMENTED failure taxonomy ✓
  • ^## [<version>] line-start anchor ✓
  • SemVer build-metadata via awk index() fixed-string match ✓

CI wiring and both baselines are unchanged.


Summary

Both Codex P2 findings from the 18573a4 round are correctly resolved in edf355b. The orphaned-fixture gate now uses jq exact-equality for files[] path matching (with a bounded-basename fallback), closing the path-direction substring bypass. The changelog-parity gate's has_heading predicate now tracks fence state so a column-zero heading inside a ``` block cannot satisfy or falsely pre-exist the release entry check. Both fixes have targeted synthetic test cases proving the specific failure mode is closed. No new issues introduced. The PR is ready to merge.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: edf355b917

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread scripts/check-orphaned-fixtures.sh Outdated
Comment thread scripts/check-changelog-parity.sh Outdated
…k fence closing

check-orphaned-fixtures.sh: evals.json consumption now reads only files[]
values (any nesting) — a fixture named in a prompt or metadata string no
longer counts as consumed; exact-value and bounded-basename matches run
against those values only.

check-changelog-parity.sh: fence tracking records the opening delimiter
char + length and closes only on a matching delimiter per CommonMark, so
a ~~~ content line inside a backtick fence no longer re-opens heading
matching. One regression test each.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@claude

claude Bot commented Jul 20, 2026

Copy link
Copy Markdown

Claude finished @kyle-sexton's task in 3m 9s —— View job


PR Review — feat(ci): orphaned-fixture + CHANGELOG-parity gates (#663)

  • Gather context and understand request
  • Read check-orphaned-fixtures.sh at HEAD — verify files[]-only jq scope fix (0c1bcc3)
  • Read check-orphaned-fixtures.test.sh — verify new test coverage
  • Read check-changelog-parity.sh at HEAD — verify CommonMark fence-closing fix (0c1bcc3)
  • Read check-changelog-parity.test.sh — verify new test coverage
  • Check for any new issues introduced
  • Post final review verdict

This review covers the latest commit (0c1bcc3), which addresses the two remaining Codex P2 findings from the edf355b review round.

Fix 1 — files[]-only consumption scope (check-orphaned-fixtures.sh): ✅ Correct

The Codex finding: the recursive jq any(.. | strings; . == $r) search and the basename fallback both ran against the entire evals.json, so a fixture named in a prompt, description, or other metadata string — but with an empty files[] — was incorrectly marked consumed.

scripts/check-orphaned-fixtures.sh lines 116–122:

files_values="$(jq -r '.. | objects | .files? // empty | .[]? | select(type == "string")' "$evals_json" 2>/dev/null)"
if printf '%s\n' "$files_values" | grep -qxF -- "$rel"; then
  return 0
fi
if printf '%s\n' "$files_values" | grep -qE -- "$base_re"; then
  return 0
fi

The jq expression — .. | objects | .files? // empty | .[]? | select(type == "string") — walks all objects at any nesting depth, extracts the files array where it exists, and yields only its string elements. Both the exact-path check (grep -qxF) and the bounded-basename check (grep -qE) now run against these extracted values only, not against the raw JSON file. A fixture mentioned in a prompt, description, or any other string field with an empty files[] correctly falls through to the test-file check and is caught as an orphan.

Graceful degradation on jq failure or invalid JSON: 2>/dev/null suppresses errors, the substitution yields an empty string, printf '%s\n' "" emits a single blank line, and grep -qxF -- "$rel" does not match it — so the consumed() function falls through to the bounded-basename test-file path. Not a fail-open.

Test coveragecheck-orphaned-fixtures.test.sh lines 88–99: seeds evals/fixtures/prose-only.md named in the prompt field with "files": [], asserts ORPHANED FIXTURE is reported. This is the exact bypass case — a prose mention with an empty files[] was previously absorbed by the recursive jq search. Suite is now 11/11.


Fix 2 — CommonMark-correct fence closing (check-changelog-parity.sh): ✅ Correct

The Codex finding: the previous has_heading awk toggled infence on any or ~~~ line, so a fence containing a ~~~ content line would prematurely toggle the fence off, causing subsequent lines inside the ``` block to be evaluated as potential headings.

scripts/check-changelog-parity.sh lines 153–170:

has_heading() {
  awk -v h="$heading" '
    {
      line = $0
      sub(/^[[:space:]]*/, "", line)
      mchar = ""; mlen = 0
      if (match(line, /^`+/) && RLENGTH >= 3) { mchar = "`"; mlen = RLENGTH }
      else if (match(line, /^~+/) && RLENGTH >= 3) { mchar = "~"; mlen = RLENGTH }
      if (mchar != "") {
        if (!infence) { infence = 1; fchar = mchar; flen = mlen }
        else if (mchar == fchar && mlen >= flen) { infence = 0 }
        next
      }
      if (!infence && index($0, h) == 1) { found = 1; exit }
    }
    END { exit !found }
  '
}

The fix records the opening delimiter character (fchar) and minimum run length (flen). The fence closes ONLY when mchar == fchar && mlen >= flen — a closing ~~~ run inside a ``` fence does NOT close it, and a shorter run of the same character also does not close it. This matches the CommonMark spec (§4.5: closing fence must be same type, run length ≥ opening). The leading-whitespace strip (sub(/^[[:space:]]*/...)) handles indented fence markers correctly (within the generous-but-safe bounds that are appropriate for CHANGELOG.md content).

The same function is used for both the head-check (has_heading <"$changelog") and the base pre-existing check (git show "$base:$changelog" | has_heading), so both are consistently fence-aware.

Test coveragecheck-changelog-parity.test.sh lines 193–210: seeds a CHANGELOG where a fence contains a `~~~` content line followed by `## [1.1.0]` — then closes with ` . Per CommonMark the ~~~ must not close the ``` fence; ## [1.1.0]is still inside the fence and must not satisfy the gate. The test assertsUNDOCUMENTED BUMPand noPRE-EXISTING`, proving delimiter-type isolation. Suite is now 19/19.


No new issues introduced

The changes in 0c1bcc3 are tightly scoped to the two affected sites. All prior approved findings remain correctly resolved:

  • Exact-path baseline matching (no prefix leak) ✓
  • Stale-baseline deduplication (single message per stale entry) ✓
  • Exit-2 on missing --check-bump arg ✓
  • Null-safe sort via sort -z
  • Suffix-sibling basename boundary (.bak not consumed by valid.json reference) ✓
  • files[] path match is exact (shorter $rel not consumed by a longer files[] value) ✓
  • PRE-EXISTING / FORMAT / UNDOCUMENTED failure taxonomy ✓
  • ^## [<version>] line-start anchor ✓
  • SemVer build-metadata via awk index() fixed-string match ✓
  • Fenced-code blocks (column-zero heading inside a fence rejected) ✓
  • CommonMark fence closing (mismatched/shorter delimiter does not prematurely close) ✓
  • evals.json consumption scoped to files[] values only ✓

CI wiring and both baselines are unchanged.


Summary

Both Codex P2 findings from the edf355b round are correctly resolved in 0c1bcc3. The orphaned-fixture gate now scopes evals.json consumption to actual files[] values via a targeted jq expression, closing the prose-mention bypass. The changelog-parity gate's has_heading predicate now implements CommonMark-correct fence closing — recording the opening delimiter type and length and requiring both to match on the closing line — so a mismatched inner marker cannot prematurely end a fence. Both fixes have targeted synthetic test cases proving the specific failure mode is closed. No new issues found. The PR is ready to merge.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 0c1bcc3098

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread scripts/check-changelog-parity.sh Outdated
…appease typos gate

A close now requires the same delimiter char, a run at least as long as
the opener, a whitespace-only suffix, and at-most-three-space indentation
— a ```not-a-close content line or a four-space-indented would-be closer
no longer clears fence state, so a release heading inside that fence
cannot satisfy the parity gate. Opening backtick fences reject an info
string containing a backtick. Regression test added; test-message wording
adjusted for the spell-check gate (hyphen-split false positive on a
prefixed word).

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@claude

claude Bot commented Jul 20, 2026

Copy link
Copy Markdown

Claude finished @kyle-sexton's task in 4m 29s —— View job


PR Review — feat(ci): orphaned-fixture + CHANGELOG-parity gates (#663)

  • Gather context and understand request
  • Read latest commit (600f1d2) — closing-fence suffix + indentation validation
  • Verify the Codex P2 finding (non-closing fence lines) is correctly resolved
  • Check for any new issues introduced
  • Post final review verdict

This review covers the latest commit (600f1d2), which addresses the last Codex P2 finding from the 0c1bcc3 review round — non-closing fence lines prematurely toggling infence.

Fix — Closing-fence suffix and indentation validation: ✅ Correct

The Codex finding: the prior fence-close condition mchar == fchar && mlen >= flen toggled infence off on any line with a matching delimiter run, including ```not-a-close (non-whitespace suffix) and ``` (four-space indentation — indented code per CommonMark, not a fence). Either could prematurely end fence state, exposing a ## [1.1.0] heading still inside the fence.

scripts/check-changelog-parity.sh lines 159–173 now enforces four CommonMark constraints together:

if (match($0, /^ {0,3}`+/) || match($0, /^ {0,3}~+/)) {
  seg = substr($0, RSTART, RLENGTH)
  sub(/^ +/, "", seg)
  mchar = substr(seg, 1, 1); mlen = length(seg)
  rest = substr($0, RSTART + RLENGTH)
  if (mlen >= 3) {
    if (!infence) {
      if (!(mchar == "`" && rest ~ /`/)) { infence = 1; fchar = mchar; flen = mlen; next }
    } else if (mchar == fchar && mlen >= flen && rest ~ /^[ \t]*$/) {
      infence = 0; next
    }
  }
}

Indentation constraint (^ {0,3}): ``` (4 leading spaces) fails the anchored {0,3} regex — match() returns 0, the block is skipped entirely, and the line falls through to if (!infence && ...) which is false when inside a fence. ✓

Closing-suffix constraint (rest ~ /^[ \t]*$/): ```not-a-closerest = "not-a-close" — is not whitespace-only, so the closing condition is false. The fence stays open. The line falls through to the heading check, which is also false because infence = 1. ✓

Delimiter-type isolation (mchar == fchar): unchanged from 0c1bcc3 — a ~~~ line inside a ``` fence still can't close it. ✓

Backtick info-string constraint (opening: !(mchar == "``" && rest ~ /``/)): a backtick fence opener whose info string contains a backtick is not recognised as a fence opener. This was already present in 0c1bcc3 and is unchanged here. ✓

The four constraints together match CommonMark §4.5 fenced code blocks exactly.

Test coverage: ✅ 20/20

check-changelog-parity.test.sh lines 212–230 adds a case seeding this CHANGELOG:

# Changelog

    ```
## [1.1.0]

[1.0.0]


Traced through line by line:
- Line 3 (` ``` `): no leading spaces, no info string → opens fence (infence=1, fchar=`` ` ``, flen=3)
- Line 4 (`` ```not-a-close ``): `rest = "not-a-close"`, not whitespace-only → fence stays open
- Line 5 (`    ``` `): 4 leading spaces → `match()` fails, block skipped, infence=1 → no-op
- Line 6 (`## [1.1.0]`): infence=1 → `!infence` is false → not matched
- Line 7 (` ``` `): `rest = ""`, whitespace-only → closes fence

The gate reports `UNDOCUMENTED BUMP`; the test asserts this and the absence of `PRE-EXISTING`. A single test case exercises both the suffix check (line 4) and the indentation check (line 5) — efficient and sufficient.

The existing mismatched-delimiter test at [lines 197–210](https://github.com/melodic-software/claude-code-plugins/blob/600f1d281e3e2114b80c3e72835154a98ec08fee/scripts/check-changelog-parity.test.sh#L197-L210) has its failure message updated from `"mis-toggled"` to `"wrongly toggled"` — the only other change in this commit, to satisfy the typos gate (no behaviour change).

### No new issues

The change is tightly scoped to the awk body and the new test case. All prior approved findings remain correctly resolved:

- Exact-path baseline matching ✓ | Stale-baseline dedup ✓ | Exit-2 on missing `--check-bump` arg ✓
- Null-safe sort ✓ | Suffix-sibling basename boundary ✓ | Exact `files[]` path match via jq ✓
- PRE-EXISTING / FORMAT / UNDOCUMENTED taxonomy ✓ | `^## [<version>]` line-start anchor ✓
- SemVer build-metadata via `awk index()` ✓ | Fenced-code tracking ✓ | CommonMark fence-type isolation ✓
- `files[]`-only consumption scope ✓ | CommonMark fence-closing (delimiter match + length) ✓
- **Closing-suffix + indentation validation** ✓ (this commit)

---

### Summary

`600f1d2` closes the last Codex P2 finding. The `has_heading` awk predicate now validates all four CommonMark closing-fence constraints — delimiter type, run length, whitespace-only suffix, and at-most-three-space indentation — so `` ```not-a-close `` and four-space-indented `` ``` `` lines no longer prematurely exit fence state. The new test case proves both closing-non-fence scenarios end-to-end. No new issues introduced. The PR is ready to merge.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 600f1d281e

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread scripts/check-orphaned-fixtures.sh Outdated
Comment thread scripts/check-changelog-parity.sh
…HTML-comment headings

check-orphaned-fixtures.sh: a bounded-basename match in a test file counts
only inside the fixture's own skill; a test elsewhere in the plugin must
name the fixture by its plugin-relative path — same-named fixtures in
sibling skills are no longer conflated.

check-changelog-parity.sh: has_heading tracks multi-line HTML-comment
state alongside fences — a release heading inside <!-- --> is not
rendered Markdown and neither satisfies nor pre-exists the entry; comment
markers inside fences stay content, fence markers inside comments stay
suppressed. Regression tests for all three shapes.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@claude

claude Bot commented Jul 20, 2026

Copy link
Copy Markdown

Claude finished @kyle-sexton's task in 4m 20s —— View job


PR Review — feat(ci): orphaned-fixture + CHANGELOG-parity gates (#663)

  • Gather context and understand request
  • Read check-orphaned-fixtures.sh at HEAD — verify skill-scoped test consumption fix (efb9717)
  • Read check-orphaned-fixtures.test.sh — verify new test coverage
  • Read check-changelog-parity.sh at HEAD — verify HTML-comment heading exclusion fix
  • Read check-changelog-parity.test.sh — verify new test coverage
  • Check for any new issues introduced
  • Post final review verdict

This review covers the latest commit (efb9717), which addresses the two remaining Codex P2 findings from the 600f1d2 review round.

Fix 1 — Skill-scoped test consumption (check-orphaned-fixtures.sh): ✅ Correct

The Codex finding: the grep -qE "$base_re" basename search ran against all *.test.* files in the plugin root, so skill A's test naming shared.md by basename also consumed skill B's same-named fixture — a new unconsumed sibling in another skill passed the gate.

scripts/check-orphaned-fixtures.sh lines 125–143: the test-file consumption is now split across two loops with different scopes and different match strategies:

# Loop 1: bounded basename match, scoped to $skill_dir only
while IFS= read -r -d '' test_file; do
  if grep -qE "$base_re" "$test_file"; then return 0; fi
done < <(find "$skill_dir" -type f -name '*.test.*' -print0 2>/dev/null)

# Loop 2: exact plugin-relative path match, rest of $plugin (outside $skill_dir)
plugin="$(plugin_root_of "$fixture")"
plugin_rel="${fixture#"$plugin"/}"
while IFS= read -r -d '' test_file; do
  case "$test_file" in "$skill_dir"/*) continue ;; esac
  if grep -qF -- "$plugin_rel" "$test_file"; then return 0; fi
done < <(find "$plugin" -type f -name '*.test.*' -print0 2>/dev/null)

Basename matches (generous) are now confined to the fixture's owning skill, so shared.md in skills/a never consumes shared.md in skills/b. Tests outside the owning skill can still consume — but only by naming the fixture's full plugin-relative path (e.g. skills/b/evals/fixtures/shared.md), which is unambiguous across skills.

The invariant that infence=0 || inhtml=0 holds throughout the awk in the companion fix also applies to the two-loop design here: there's no state that can leak between the two loops.

Edge case verified — skill_dir == plugin (evals/ directly under plugin root): In that degenerate layout, loop 1 already searched the entire plugin for basename matches, and loop 2's case guard skips everything, making loop 2 a no-op. Not a gap: basename consumption is fully covered by loop 1.

Test coverage — two new cases in check-orphaned-fixtures.test.sh:

Case What it proves
Sibling-skill basename conflation (lines 88–101) skills/a test naming shared.md → skills/b shared.md is still caught; skills/a shared.md is not
Cross-skill consumption by plugin-relative path (lines 103–111) A plugin-root test referencing skills/b/evals/fixtures/by-plugin-test.md correctly marks it consumed

Suite is now 13/13.


Fix 2 — HTML-comment heading exclusion (check-changelog-parity.sh): ✅ Correct

The Codex finding: has_heading tracked fenced-code state but not HTML comment state, so ## [version] appearing at column zero inside a `` block would satisfy the release-entry check.

scripts/check-changelog-parity.sh lines 161–205: the has_heading awk now tracks multi-line HTML comment state (inhtml) in parallel with fence state (infence):

  • Block 1 (first, highest priority): if !infence && inhtml, scan the line for --> to close the comment and call next — all lines inside an HTML comment are skipped for heading matching and fence detection.
  • Block 2: fence detection (unchanged from prior round).
  • Block 3: !infence && index($0, h) == 1 — heading match, skipped inside fences (and HTML comments via block 1's next).
  • Block 4: if (!infence) { HTML comment tracking } — scan for `` on unfenced, non-comment lines; sets inhtml=1 on an unclosed opener.

Invariant: infence=1 and inhtml=1 cannot be simultaneously true. Block 4 is guarded by !infence, so inhtml can only become 1 when outside a fence. Block 1 is guarded by !infence && inhtml, but since inhtml can't be 1 while inside a fence, block 1's fence guard is a safety belt, not a reachability condition.

Interaction properties verified:

  • Fence markers inside HTML comments: block 1 fires first with next, fence detection (block 2) never runs → fence markers inside comments are suppressed ✓
  • HTML comment markers inside fenced code: block 4 is guarded !infence, so ``: handled inline in block 4's while loop — the heading check never ran (block 3 requires column 1, which a post-`

[1.0.0]


Asserts `UNDOCUMENTED BUMP` and NOT `PRE-EXISTING` — both the head check and base check correctly reject the comment-enclosed heading. Suite is now **21/21**.

---

### No new issues introduced

All prior approved findings remain correctly resolved at `efb9717`:

| Finding | Status |
|---|---|
| Exact-path baseline matching (no prefix leak) | ✓ |
| Stale-baseline deduplication (single message per entry) | ✓ |
| Exit-2 on missing `--check-bump` arg | ✓ |
| Null-safe sort via `sort -z` | ✓ |
| Suffix-sibling basename boundary (`.bak` not consumed by `valid.json` reference) | ✓ |
| Exact `files[]` path match via jq | ✓ |
| PRE-EXISTING / FORMAT / UNDOCUMENTED failure taxonomy | ✓ |
| `^## [<version>]` line-start anchor via `awk index()==1` | ✓ |
| SemVer build-metadata via `awk index()` fixed-string match | ✓ |
| Fenced-code tracking (column-zero heading inside fence rejected) | ✓ |
| CommonMark fence-type isolation (mismatched delimiter doesn't close) | ✓ |
| `files[]`-only consumption scope (prose mention doesn't count) | ✓ |
| CommonMark fence closing (delimiter char + length + whitespace-suffix + ≤3-space indent) | ✓ |
| Test-basename scoped to owning skill | ✓ *(this commit)* |
| HTML-comment heading exclusion | ✓ *(this commit)* |

CI wiring and both baselines are unchanged.

---

### Summary

Both Codex P2 findings from the `600f1d2` round are correctly resolved in `efb9717`. The orphaned-fixture gate now uses a two-loop design that confines basename matches to the fixture's owning skill while still allowing cross-skill consumption via unambiguous plugin-relative paths. The changelog-parity gate's `has_heading` predicate now tracks HTML comment state alongside fence state, correctly rejecting headings that appear at column zero inside `` blocks. Both fixes have targeted synthetic test cases proving the failure modes are closed. No new issues introduced. The PR is ready to merge.

@kyle-sexton
kyle-sexton merged commit 820e62d into main Jul 20, 2026
19 checks passed
@kyle-sexton
kyle-sexton deleted the ci/663-orphaned-fixture-changelog-parity-gates branch July 20, 2026 16:09
kyle-sexton added a commit that referenced this pull request Jul 20, 2026
Resolves a CHANGELOG heading conflict: main bracketed all
session-flow CHANGELOG.md version headings to satisfy the new
CHANGELOG-parity CI gate (#681); this branch's new 0.10.3 entry
predated that reformat. Applied the bracketed `## [0.10.3]` form
to the new entry so it matches the rest of the file.
kyle-sexton added a commit that referenced this pull request Jul 20, 2026
…s, zero orphans (#708)

## Summary

Fix-direction (a) from the issue, per the #634 graded-fixture idiom:
every fixture under
`plugins/autonomy/skills/setup/evals/fixtures/security-binding/` is now
graded.

- **One table-driven runner**
(`check-security-binding.fixtures.test.mjs`) + **one co-located
expectations manifest**: each entry pins the checker invocation
(`--probe-evidence-root` at the fixtures dir; per-fixture
`--egress-hosts`/`--evidence` where needed) and the expected outcome —
exit code plus defect-naming stderr substrings. 109 fixtures: 14
pass-expected (12 valid bindings + 2 evidence-input pairings), 95
reject-expected. **Zero quarantined — no name-vs-behavior mismatches
surfaced.**
- **Self-policing both directions**: a new top-level fixture without a
manifest (or quarantine) entry fails; a manifest ref whose file vanished
fails; the 67 `probe-transcripts/` suite inputs are enumerated and
reconciled against disk both ways.
- **Baseline drained**: all 178 security-binding lines leave
`scripts/orphaned-fixtures-baseline.txt`; the orphaned-fixture gate
(#681) passes with the set consumed, exactly as its stale-guard demands.
- Thin `.test.sh` wrapper joins `plugins/**` CI test discovery. Suite:
**394/394 checks pass.** `validate-plugin-contracts.mjs`,
orphaned-fixtures `--check`, changelog-parity `--check`/`--check-bump`
all green. Autonomy plugin bumped 0.7.3 with CHANGELOG entry.

Note for reviewers: the convention-level decision about the
golden-fixture idiom repo-wide stays with #664 (needs-human); this PR
instantiates the already-precedented #634 shape for the one suite #662
names, which the issue's own fix-direction (a) authorizes.

## Related

- #681 (the orphaned-fixture gate whose baseline this drains)
- #664 (repo-wide golden-fixture convention decision — untouched)

Closes #662

🤖 Generated with [Claude Code](https://claude.com/claude-code)

---------

Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
kyle-sexton added a commit that referenced this pull request Jul 20, 2026
## Summary

The `--check-bump` gate's PRE-EXISTING-ENTRY arm compared **every**
versioned plugin's manifest against the base ref. When `main` advanced
any plugin's version while a PR was open, the PR's stale branch
red-lined on a plugin it never touched — forcing an unnecessary
merge-from-main on every base advance. Observed twice on #681
(work-items 0.14.2 → 0.14.4; #681 never touched work-items).

## Fix

Scope the bump loop to plugins whose **manifest** changed in the
branch's own diff:

- `git diff --name-only base...HEAD` — three-dot isolates
`diff(merge-base(base,HEAD), HEAD)`, the commits unique to this branch.
A version `main` advanced after the branch forked is excluded whether CI
checks out the PR head or the auto-merge commit.
- The filter keys on the manifest path
`plugins/<name>/.claude-plugin/plugin.json`, **tighter than the issue's
"plugin roots" wording**. A version bump is definitionally a change to
that file, so manifest-scoping is precisely "plugins whose version this
change set could have changed" — and, unlike plugin-root scoping, a
cosmetic touch elsewhere under a plugin dir cannot pull a main-only
advance back into scope (the same treadmill, just triggered by a stray
edit).

Plugins the branch never touched are left out of the check entirely;
plugins the branch did touch are checked exactly as before.

## Verification

Full self-test suite (extended with three branch-staleness cases:
main-only advance not flagged, the discriminating
cosmetic-touch/manifest-scoping case, and a counterpart proving a
branch-bumped plugin is still checked):

```
$ bash scripts/check-changelog-parity.test.sh
...
ok: untouched plugin advanced only on the base ref is not flagged (branch staleness scoped out)
ok: cosmetic touch under a plugin dir does not pull a main-only advance into scope (manifest-scoped)
ok: a plugin the branch bumped is still checked while the main-only advance is scoped out
...
PASS=24 FAIL=0
```

Live check against the real repo:

```
$ scripts/check-changelog-parity.sh --check-bump origin/main
Every plugin whose version changed vs origin/main has a '## [<version>]' CHANGELOG.md entry.
```

`shellcheck` clean on both files.

Closes #693

## Related

- #693 — this fix
- #681 — the PR where the re-merge treadmill was first observed

---------

Co-authored-by: Claude <noreply@anthropic.com>
kyle-sexton added a commit that referenced this pull request Jul 20, 2026
… evals/fixtures (#727)

## Summary


`plugins/knowledge/skills/youtube-digest/evals/fixtures/variation-matrix-backlog.json`
sat under a skill's `evals/fixtures/` but no eval case referenced it (no
`files[]` entry in the sibling `evals.json`) and no test asserted on it.
It was grandfathered in `scripts/orphaned-fixtures-baseline.txt` — the
last un-owned entry there (the autonomy `otlp-demo` fixtures are tracked
by #662).

## Fix

**Decision: demote** (not grade). The file is a manual smoke-test
tracking backlog — candidate videos across footage variations (code
screencast / slide talk / talking-head / mixed) carrying `status` fields
(`smoke-pass`), acquisition notes, and blocked-caption records. It is
reference/tracking data, not an input→expected-output graded fixture
(the genuine graded fixture in this skill is
`driver-video-goldens.json`, wired via `files[]` in evals 2 and 4).
`SKILL.md` itself labeled it "backlog only".

- Moved `evals/fixtures/variation-matrix-backlog.json` →
`reference/variation-matrix-backlog.json` (out of the graded-fixture
scope, into the skill's `reference/` per the issue's steer).
- Repointed the two prose references at the new path: `SKILL.md` (now
describes it as a tracking backlog, not an eval fixture) and vendor
`TUNING.md`.
- Removed its grandfather line (and its comment block) from
`scripts/orphaned-fixtures-baseline.txt`.
- Bumped `plugins/knowledge` `0.8.0 → 0.8.1` (patch: relocation + docs,
no behavior change) with a top-inserted `CHANGELOG.md` entry.

## Verification

Ran on the rebased branch tip:

```
$ scripts/check-orphaned-fixtures.sh --check
No orphaned eval fixtures (every file under **/evals/fixtures/ is consumed by a grader or grandfathered).
# exit 0

$ scripts/check-orphaned-fixtures.sh discover | grep variation-matrix
# (no output — file no longer under evals/fixtures/)

$ scripts/check-changelog-parity.sh --check
Every versioned plugin has a CHANGELOG.md (or a stale-guarded baseline entry).
# exit 0

$ scripts/check-changelog-parity.sh --check-bump origin/main
Every plugin whose version changed vs origin/main has a '## [<version>]' CHANGELOG.md entry.
# exit 0

$ node scripts/validate-plugin-contracts.mjs
Plugin contracts validated: 33 setup skills and 1816 plugin files checked.
# exit 0

$ jq empty reference/variation-matrix-backlog.json   # relocated file still valid JSON
# exit 0
```

Closes #688

## Related

- #688 — this issue (grade-or-demote the orphaned fixture)
- #663 — orphaned-fixture CI gate that surfaced it
- #681 — PR that introduced the gate + baseline

Co-authored-by: Claude <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

automated Opened by automation.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

ci: orphaned-fixture gate + CHANGELOG-parity gate (every fixtures/evals file consumed by a grader; every plugin.json bump requires a CHANGELOG entry)

1 participant