Skip to content

Repository navigation

Port the pull request description validator to Go #718

New issue
New issue
Open
Feature
Open
Port the pull request description validator to Go#718
Feature
Labels
refinement:readyPassed the Definition of Ready; may be picked up

Description

@tbhb-agent
tbhb-agent
opened on Oct 25, 2026

Problem

packages/agents/source/skills/pr/shared/scripts/validate-description.sh (506 lines on main, the second largest maintained skill script) checks a pull request description against the template before a PR is created or edited. It is called by pr-agent.sh, by review.sh and by the write-pr-description skill in both its Claude and Codex forms, and it sits in front of every agent-created pull request (feature 12 of #588). A shell implementation of this size is hard to test and slow in Bats, while its verdicts decide whether a description is accepted. Its verdict depends on more than the description text: it reads the repository configuration, the template, environment variables, the Git history and the base branch.

Acceptance criteria

  1. AC1: Given a description that meets the template, when it is validated in a repository, then nothing is printed and the exit status is 0.
  2. AC2: Given a description with a missing, misordered or empty required section, then the validator prints a finding that names the section and rule, with the same rule name, line number and message as today, and exits 1.
  3. AC3: Given any other rule violation that the validator reports today (frontmatter missing, unterminated or missing a key, empty labels, wrong owner-review label, unknown base, base equal to head, a path that does not exist, and the rest of the rule set), then the same finding is reported with the same rule name, line number and message.
  4. AC4: Given the recorded corpus of descriptions and contexts, then no description that the shell validator rejects is accepted by the Go validator, and every verdict equals the shell validator's.
  5. AC5: Given a draft that is missing or empty, or a template that is missing or has no sections, then the validator fails with the same fatal rule as today and never accepts; given a draft that exists and is non-empty but cannot be read, then it exits with the same non-zero status and the same standard output and standard error as the shell validator, and never accepts.
  6. AC6: Given the callers in the pr and write-pr-description skills of both the Claude and Codex packages, then each keeps its arguments, exit statuses and output streams.
  7. AC7: After regeneration, the validator entry points of both skill packages and both local deployments are generated from the canonical sources and agree with them.
  8. AC8: Non-functional: validating a 1,000-line description, measured from the start of the registered script path to exit in a fixture repository, takes under 300 ms at the median over 50 runs on the CI runner.
  9. AC9: Given a template chosen through the PR_TEMPLATE environment variable, through the repository configuration's pr_template, or by default, and title bounds taken from PR_TITLE_MIN_LENGTH and PR_TITLE_MAX_LENGTH, then the template and bounds used are the ones the shell validator picks, including its precedence, its refusal of an absolute path or a .. path, and its reading of null, ~ and an empty string as absent.
  10. AC10: Given a repository where the base is known by a remote-tracking branch, only by a local branch, or not at all, and where the commits since the base carry different landing types, then the base chosen and the title type required are the ones the shell validator picks.
  11. AC11: Given any set of findings, then they are printed in the shell validator's order, one per line as <draft>:<line> [error] <rule> <message> on the same stream, followed by the TOTAL: <n> finding(s) in <draft> line.
  12. AC12: Given a failing Git subprocess (not a repository, an unresolvable base, an unreadable history), then the validator's verdict and finding are the ones the shell validator gives.

Scope and non-goals

In scope: the description validator, its forwarding shim, the Claude and Codex entry points and their tests.
Non-goal: pull request creation, review signature and property resolution (create-pr.sh, a later wave) and the guard-gh hook (#719).
Non-goal: changing the template or any rule, or fixing a quirk of the shell validator; a quirk is recorded and preserved, and a separate issue may change it.
Non-goal: the shared readers in description.sh beyond what the validator calls.
Out of scope: the Markdown prose linters.

Approach

  1. Build the characterization matrix first, from the shell validator at a pinned commit of main, as committed fixtures of draft text, repository state and environment together with the exact standard output, standard error and exit status. The matrix covers every rule the validator can report at least once, and these contexts: template selection (environment, repository configuration, default, absolute and .. paths), title bounds, the scalar forms (null, ~, empty), a remote-tracking base, a local-only base, an unknown base, the head equal to the base, commits with each landing type, a missing draft, an empty draft, an unreadable draft (recorded with its exit status and streams, because the read failure leaves through shell error handling and not through the fatal rule), a missing template, a template without sections, a failing Git subprocess, and the owner-review label (the repository configuration's owner_review_label and its default, case-insensitive matching against the draft's labels, an empty configured value, and a failing configuration read). Run the shell validator over it to confirm the recording.
  2. Port the rules to Go one group at a time, each with a table test over its slice of the matrix, then require identical results over the whole matrix.
  3. Keep the script path as a forwarding shim of at most 10 lines that execs the Go command and fails with the usual fatal output if the binary is missing, so the existing callers keep their arguments. Delete the shell validator. Regenerate both skill variants from the canonical sources.
  4. The validator script is not matched by the golden or owner path patterns, so this change adds no protection change. The callers are switched only after the epic's rollout hold below.
  5. Deliver in the documented order: mise run generate-skills, then mise run skill-tokens, then apm install, committing what they rewrite. Add to docs/agent-skills-migration.md that the pr skills need the release carrying the validator installed before the skills are deployed, or state "No consumer impact" with the reason.

Dependencies

Native blocked-by links: #333 (the Go binaries ship as release assets) and #544 (consumers install those assets, because the shim runs in consumer repositories). The change follows the sequence of #588: the binary is installed before the callers are switched, and features 3 to 8 (gates and the merge path) come first.
Blocked on: #720 (feature 3, the check judge) and #713 (feature 4, gate execution), held as prose lines and not native links while #720 is being split. Features 5 to 8 have no issues yet, so the rollout hold is: the shim is not switched until features 3 to 8 have shipped, and their issues are added as native blockers when they exist.
Independent of those features, and startable once #333 and #544 allow: the characterization matrix, the Go port and its tests. Only the switch of the callers waits.

Size

S. One PR: the Go validator, the recorded matrix, the shim and the generated outputs. The matrix is committed fixtures and not hand-written tests, which keeps the review size within S. If the port turns out to need more than one PR, this issue becomes an epic and is split into sub-issues by rule group before coding, each refined on its own.

Verify plan

  • AC1: Go test over a clean description in the fixture repository, asserting empty output and exit 0, plus the accepted-description cases of tests/skills/pr/scripts/pr-workflow.bats.
  • AC2: Go table tests for missing, misordered and empty sections, comparing rule, line and message with the recorded results.
  • AC3: golden diagnostics over every other rule in the matrix, compared with the recorded shell results.
  • AC4: differential test over the matrix asserting equal verdicts, and a check that no recorded rejected case becomes accepted. The claim covers the recorded matrix only; each new quirk found later is added to it.
  • AC5: Go tests with a missing and an empty draft, a missing template and a template without sections, asserting the same fatal rule and a non-zero exit; and an unreadable draft, asserting the recorded exit status and streams.
  • AC6: the pr-workflow Bats suite for the Claude package, plus the same cases run against the Codex package's copy of the entry point, asserting identical arguments, exit statuses and streams; the harness helper currently selects only the Claude package, so the Codex run is added explicitly.
  • AC7: mise run generate-skills, mise run skill-tokens and apm install in that order leave no diff, mise run check-generated-skills passes, the shim has at most 10 lines, a search shows the shell validator is gone from the source, and the Claude and Codex outputs are byte-identical where the generator makes them so.
  • AC8: a process-level timing script that runs 50 invocations of the registered script path in the fixture repository with a 1,000-line description and reports the median, run in CI.
  • AC9: matrix cases for each template source and each precedence combination, the refused paths, and the scalar forms.
  • AC3 (owner-review label): matrix cases for the default label, a configured label, a differing case, an empty configured value and a failing configuration read, compared with the recorded results.
  • AC10: matrix cases with a remote-tracking base, a local-only base, an unknown base, and each landing type among the commits.
  • AC11: matrix cases with several findings, comparing the order, line numbers, streams and the TOTAL line, and a clean case asserting silence.
  • AC12: matrix cases that run the validator outside a repository, with an unresolvable base and with an unreadable history.
  • Delivery: mise run check is green on the exact head, and so are mise run cover-go-check and mise run cover-py-check; the pull request lists every AC ID with its evidence.

Risks

Accepting a description the old validator would reject lets a malformed PR through; AC4 is the differential check for that, bounded by the matrix. A context that the matrix misses could change a verdict; AC9 to AC12 list the contexts the validator reads. The size could overrun one PR, handled by the split rule. The undo is reverting the PR, which restores the shell validator and its generated copies.

Operator authority

None

Activity

  1. github-project-automation
    added this to devtoolson Oct 25, 2026
  2. github-project-automation
    moved this to Backlog in devtoolson Oct 25, 2026
  3. tbhb-agent
    changed the title [-]Port the draft prose linter to Go and drop its duplicate[/-] [+]Port the pull request description validator to Go[/+] on Oct 25, 2026
  4. tbhb-agent
    added the
    Feature
    issue type on Oct 25, 2026
  5. tbhb-agent
    updated Priority Standard and Work type Planned on Oct 25, 2026
  6. tbhb-agent
    added a parent issue on Oct 25, 2026
  7. tbhb-agent
    added
    refinement:activeA refinement round is open; not ready for implementation
    on Oct 25, 2026

  8. tbhb-agent commented on Oct 25, 2026

    @tbhb-agent
    tbhb-agent
    on Oct 25, 2026
    ContributorAuthor

    Refinement: NOT READY — compatibility evidence and rollout prerequisites need refinement

    Checked

    Gaps

    1. Acceptance criteria: AC6 combines compatibility, shim layout, line count, and implementation deletion; AC7 combines generation and shared implementation. Move implementation details into Approach and give each AC one observable outcome. AC4’s universal “never accepted” claim exceeds what a finite corpus can establish; define its compatibility boundary.

    2. Approach and compatibility evidence: “Full fixture corpus” is undefined. The current validator depends on repository configuration, template precedence, scalar parsing, remote-versus-local base selection, Git history, and filesystem state—not description text alone. See packages/agents/source/skills/pr/shared/scripts/description.sh:23, packages/agents/source/skills/pr/shared/scripts/description.sh:93, and packages/agents/source/skills/pr/shared/scripts/description.sh:102. Specify a characterization matrix covering every rule and these contextual inputs, including subprocess failures. Preserve diagnostic order, line numbers, streams, totals, and silence on success, rather than wording and verdict alone.

    3. Dependencies: Native blocked-by links are empty, although switching the shim requires binary availability through still-open Ship the Go binaries as release assets installed through mise's github backend #333. Add the corresponding native blocker or explicitly separate preparatory work from a blocked switch. Also account for Replace complex shell with tested Go, starting with gates and merging #588’s owner-approved protection of replacement Go enforcement paths before rollout; Port the pull request description validator to Go #718 currently omits that prerequisite.

    4. Size: M is an allowed size, but Replace complex shell with tested Go, starting with gates and merging #588 explicitly requires larger Features to be divided into XS/S implementation PRs before coding. Port the pull request description validator to Go #718 instead defers splitting until the corpus comparison overruns. Resolve that conflict before implementation.

    5. Verify plan: Every AC ID appears, but several checks do not prove their claims:

      • AC3/AC4 need the defined corpus and repository/configuration matrix above.
      • AC6’s line count and grep do not prove argument, stream, or exit-status compatibility. The cited PR suite selects the Claude package at tests/skills/pr/test_helper.bash:9; name how the Codex entry point is exercised.
      • AC8 needs a timing boundary, environment, and statistic. An in-process Go benchmark may omit shim startup, configuration reads, and Git operations.
      • Add the generation sequence and committed outputs required by Replace complex shell with tested Go, starting with gates and merging #588, plus consumer migration guidance for the new binary requirement.

    Split

    Resolve the epic’s sizing requirement now. A characterization/Go implementation slice followed by the compatibility switch and shell removal is a possible split. Intermediate work must keep deployed callers operational and avoid changing the shared publisher readers outside scope.

    Definition of done

    Unchecked items below are unmet by the current AC/Verify plan or pending implementation evidence.

    • D1 Pending: AC IDs are mapped, but merged-head verification and PR evidence requirements are absent.
    • D2 Pending: exact-head independent approval and finding disposition are not covered.
    • D3 Pending: required checks and owner approval for loosened gates are not covered.
    • D4 Unmet: no explicit gitleaks, private-string scan, or post leak-review evidence.
    • D5 Unmet: commit and squash signing are not covered.
    • D6 Unmet: documentation updates or a justified “No doc impact” statement are absent.
    • D7 Unmet: Assisted-by trailers and the provenance label are not covered.
    • D8 Pending: the size warning supplies neither a concrete compliant split nor an approval requirement.
    • D9 Unmet: deferred findings and follow-up issue requirements are absent.
    • D10 Pending: closing reference, board completion, and merge-commit status evidence are absent.
    • devtools-1 Unmet: the plan omits mise run check, mise run cover-go-check, and mise run cover-py-check.
    • devtools-2 Partially covered: AC7 requires the generation check, but omits the ordered generation, token measurement, deployment, and committed results.
    • devtools-3 Unmet: the consumer-visible binary dependency needs adoption guidance or an explicit, justified consumer-impact statement.

    Round: 1 of 3

  9. tbhb-agent
    marked this as blocked by 2 issues on Oct 25, 2026

  10. tbhb-agent commented on Oct 25, 2026

    @tbhb-agent
    tbhb-agent
    on Oct 25, 2026
    ContributorAuthor

    Refinement: NOT READY — unreadable-input contract and contextual coverage still need correction

    Checked

    Gaps

    1. AC5 requires a fatal rule that unreadable drafts do not produce. The shell checks existence and size, but not readability, at packages/agents/source/skills/pr/shared/scripts/validate-description.sh:76. Reading occurs through the redirection at packages/agents/source/skills/pr/shared/scripts/validate-description.sh:242; a read failure exits through shell error handling, without invoking fatal. Consequently, the AC5 verification cannot assert “the same fatal rule” for this case while preserving behavior. Require the recorded exit status and streams for unreadable drafts, reserving fatal-rule assertions for cases that actually produce them.

    2. The characterization matrix still omits configuration inputs that change the result. It covers pr_template, but not the configurable reserved owner-review label or configuration-read failures. The validator reads and compares that label case-insensitively at packages/agents/source/skills/pr/shared/scripts/validate-description.sh:280. Its reader supplies a default and rejects an explicitly empty value at packages/agents/source/lib/agent.sh:40; configuration parsing can also reject invalid or duplicate settings at packages/agents/source/lib/agent.sh:15. Add matrix cases for the default and overridden label, case-insensitive matching, an empty configured label and a failing configuration read, recording both streams and status. Otherwise, the stated matrix can pass while these contextual verdicts change.

    3. Existing sequencing prerequisites are missing from native blockers. Dependencies explicitly requires Replace complex shell with tested Go, starting with gates and merging #588 features 3–8 first, but links only Ship the Go binaries as release assets installed through mise's github backend #333 and feat(distribution): migrate consumer tool pins to release assets #544. Feature 3 already has Settle the check judge contract and divergence corpus before porting it #720, and feature 4 already has Port the gate runner and pass cache from Python to Go #713. Add those native blockers, or explicitly distinguish preparatory work that may proceed independently from the blocked switch. The hold for features 5–8 whose issues do not yet exist is clear.

    Split

    S is an acceptable declared size for one PR. The explicit conversion to an epic and XS/S sub-issues before coding if it exceeds one PR addresses #588’s split rule. No additional split is required at refinement.

    Definition of done

    The definition of done for #718 (Feature). Ticks show what the issue's plan already covers, not delivery evidence.

    • D1 Pending: every AC has a check and PR evidence is required; merged-head verification is not explicit.
    • D2 Pending: exact-head independent approval and finding disposition.
    • D3 Pending: exact-head checks are planned; owner approval for any loosened gate is not covered.
    • D4 Unmet in the plan: gitleaks, private-string scanning and leak review of posts.
    • D5 Unmet in the plan: signed commits and squash.
    • D6 Approach requires migration documentation explaining the new installation requirement.
    • D7 Unmet in the plan: Assisted-by trailers and provenance:agent-assisted.
    • D8 Partially covered: S and the split rule are explicit; actual PR size compliance remains pending.
    • D9 Partially covered: quirks may receive separate issues; required follow-up structure and PR Related links are absent.
    • D10 Pending: closing reference, board completion and merge-commit Status: comment.
    • devtools-1 mise run check is green, and so are the coverage floors mise run cover-go-check and mise run cover-py-check, which check doesn't run.
    • devtools-2 A change to skill sources under packages/agents/source/ commits what mise run generate-skills, then mise run skill-tokens, then apm install rewrite, in that order, and check-generated-skills (part of lint) passes.
    • devtools-3 A consumer-visible change (the vendored payload, a skill, a reusable workflow, a task or a pin) has its consumer steps in docs/agent-skills-migration.md or the adoption docs, or the PR's Verification says "No consumer impact" and why.

    Round: 2 of 3

  11. tbhb-agent commented on Oct 25, 2026

    @tbhb-agent
    tbhb-agent
    on Oct 25, 2026
    ContributorAuthor

    Refinement: READY — compatibility contract and rollout boundaries are sufficiently defined

    Checked

    Gaps

    None that require an implementer to guess or prevent the planned checks from proving their ACs. All three round-2 gaps are resolved.

    Split

    S is acceptable for the declared single PR. The explicit conversion to an epic and refined sub-issues before coding if it exceeds one PR satisfies the split boundary. No additional refinement split is required.

    Definition of done

    Ticks indicate coverage in the issue’s plan, not completed delivery evidence.

    • D1 Pending: every AC has a check and PR evidence is required; verification on the merged head remains pending.
    • D2 Pending: independent approval of the exact merged head and disposition of findings.
    • D3 Pending: exact-head checks are planned; any applicable gate-loosening approval remains pending.
    • D4 Unmet in the plan: explicit gitleaks, private-string scanning and leak review of posts.
    • D5 Unmet in the plan: signing every commit and the squash.
    • D6 Approach requires migration documentation explaining the new binary installation requirement.
    • D7 Unmet in the plan: Assisted-by trailers and provenance:agent-assisted.
    • D8 Pending: S and a split boundary are explicit; actual PR size compliance remains to be established.
    • D9 Partially covered: separate quirk issues are contemplated; required follow-up structure, Related links and code references remain pending.
    • D10 Pending: closing reference, board completion and final merge-commit Status: comment.
    • devtools-1 Delivery verification names mise run check, mise run cover-go-check and mise run cover-py-check.
    • devtools-2 Approach requires committing the ordered generation, token and deployment outputs; AC7 verifies generation consistency.
    • devtools-3 Approach requires consumer migration guidance or a justified “No consumer impact” statement.

    Round: 3 of 3

  12. tbhb-agent
    added
    refinement:readyPassed the Definition of Ready; may be picked up
    and removed
    refinement:activeA refinement round is open; not ready for implementation
    on Oct 25, 2026

  13. tbhb-agent commented on Oct 26, 2026

    @tbhb-agent
    tbhb-agent
    on Oct 26, 2026
    ContributorAuthor

    Refinement amendment for the shell-to-Go epic (#588), covering compatibility and protection. The acceptance criteria stay as written, except where noted.

    1. The validator shim forwards to the Go command when present and runs the existing shell validator otherwise. The differential corpus runs the shell validator and the Go validator against the same cases, which keeps AC4 true on both paths.
    2. Fallback rule from docs: state the N-2 compatibility policy #661, applied here: the caller probes for the Go command without writes. When the command is absent (an older pinned binary), the existing shell implementation runs unchanged. When the command is present and fails, prints partial output or returns an invalid result, the caller refuses and never falls back to repeat a protected action. Configuration, flags, streams, statuses and state formats stay as they are, and this change does not raise the binary floor. A separate XS issue removes the fallback after every supported release pin ships the command. Any acceptance criterion or step that demands deleting the shell logic, or a missing-binary refusal, moves to that issue.
    3. No new complex shell: the fallback is the frozen existing code, not a translation of it.
    4. The replacement Go package and its dispatcher registration are covered by the owner-approval protection patterns before any caller switches to them. That protection is its own Wave 1 issue and is a native dependency of this one.
    5. Generated skill output is regenerated in the same PR, in the documented order.
  14. tbhb-agent
    marked this as blocking on Oct 27, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    refinement:readyPassed the Definition of Ready; may be picked up

    Type

    Feature

    Fields

    Priority

    Standard

    Work type

    Planned

    Projects

    • devtools
      Status
      Backlog

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions