Skip to content

Speed up PR validation for ordinary documentation - #806

Merged
justin808 merged 4 commits into
mainfrom
jg-codex/speed-issue-747
Sep 10, 2026
Merged

justin808 merged 4 commits into
mainfrom
jg-codex/speed-issue-747

Conversation

@justin808

@justin808 justin808 commented Sep 9, 2026 •

Copy link
Copy Markdown
Member

Why

Ordinary documentation edits currently wait for the same long validation job as installer and workflow changes. This gives recognized prose changes a focused gate while retaining full checks for code, instructions, unknown paths, and main pushes. Part of #747.

What changed

  • Select documentation checks with policy extracted from the trusted PR base; candidate policy edits cannot reduce their own checks.
  • Preserve existing draft-head/current-base job names and report selected and omitted coverage.
  • Preserve embedded-code line positions so moving unchanged commands retains full validation.
  • Pin the full job's JSON dependency to avoid the reproduced JSON 3 diagnostic incompatibility, and reinstall executable wrappers after gem-cache restoration.

This is over 500 changed lines because the workflow, trusted selector, selected runner, and regression fixtures land together. Broader documentation eligibility is deferred.

How to review and verify

Inspect the workflow's trusted-base fallback and the selector's conservative handling of code and incomplete Git evidence. This introducing PR still runs full validation because its base does not contain the selector.

Test plan

  • Clean committed local bin/validate, including installer and stack suites: PASS.
  • Current-head full hosted validation: PASS.
  • Current-head review findings assessed; published dispositions below.
  • Repository lint passed, with changed Ruby/workflow checks repeated after edits.
  • Selector tests: 17 tests, 120 assertions; actual broken-navigation and unstaged-deletion replays passed.
  • Changelog: deferred_to_update_changelog.
Agent details

Current candidate and evidence

Candidate 943c3768cf6ddeea79e99192cbc53403f0047e23 integrates actual main 9c6234ba60bfcf7c844d8e151933185af461f9de, including the merged installer batching change from #807. The merge was clean. All seven feature files and Git modes match the approved 218bd113ca52c9e9459398d542f84d2b4c6e4ce4 implementation, except that bin/validate also retains the newly merged installer-copy test entry. Both directions of that test-entry integration were checked against the approved feature and actual base. Files outside the feature diff match main, including CHANGELOG.md.

Integration precommit validation passed with the documented partial coverage: installer and stack suites were skipped because incoming skills files were uncommitted. Clean committed local full validation passed on 943c3768cf6ddeea79e99192cbc53403f0047e23. Current-head full hosted validation passed. The validation step took 28m27s. All 23 review threads are resolved.

The prior independent integration and corrective reviews remain applicable to the unchanged feature. The approved 218bd113ca52c9e9459398d542f84d2b4c6e4ce4 candidate passed full hosted validation, including a real gem-cache hit and successful RuboCop execution. Its Claude job completed with permission-limited execution and no buffered comments; it was not counted as a clean review attestation. The independent local reviews and completed Codex review supplied the recorded review evidence. Current-head Codex completed. Claude published five inline suggestions and its job completed with 28 permission denials; the launcher success is not a clean-review attestation. The findings were checked against source and prior dispositions, with independent assessment of the new consequential claims. No additional required code correction was established; see the decision log.

The earlier hosted documentation exercise took 24 seconds for Validate, including 6 seconds for selected checks; separate lint took 80 seconds. This is historical evidence from the earlier candidate, not a timing claim for this head or the whole PR cycle. The post-merge exercise against the actual merged selector is tracked in #747.

QA Evidence

  • QA lane: independent feature reviews and behavioral CLI replays.
  • Scope checked: selector, selected runner, workflow trust boundary, and local validation compatibility.
  • Tested at: 943c3768cf6ddeea79e99192cbc53403f0047e23; complete local validation passed.
  • Automated checks: selector 17 tests/120 assertions and integration precommit checks passed; clean committed local full passed; current-head hosted validation passed.
  • Manual checks: actual CLI selection and broken-navigation/unstaged-deletion replays on the unchanged feature.
  • User-visible UI change: no.
  • Visual evidence: not applicable: command-line and CI change.
  • Interaction change: no application interaction change.
  • Interaction evidence: not applicable: command-line and CI change.
  • Visual fix: no.
  • Negative control: not applicable: no visual fix.
  • Performance evidence: historical selected-job timing above; no end-to-end PR-cycle saving claimed.
  • Findings: independently confirmed selection and fixture findings fixed; approved feature unchanged.
  • QA required: yes.
  • QA required rationale: changed CI selection must preserve required coverage and fail closed.
  • QA lane status: satisfied.
  • Release-blocking status: clear for reviewed findings; hosted validation passed.
  • Process-gap disposition: script.

Corrective verification

Moving a fenced, inline, or indented command selects full coverage for both draft and ready integration cases. Regression tests preserve positions in the code projection; ordinary prose remains eligible. The earlier cache failure came from restored gem data without the RuboCop executable. The approved correction reinstalls executable wrappers; its hosted full run passed after a real cache hit. The earlier isolated dependency probe was not a full hosted-environment reproduction.

Decision log

The current Claude suggestions were assessed without source changes. Selected coverage names describe subchecks; omitted names describe full groups not run in their entirety. The omitted RuboCop label retains a literal version placeholder, a display limitation. The conditional empty-base/index-read mechanism is real, but no supported missing-field trigger was demonstrated. A missing validator already fails the required full command; no reporting-only availability regression was demonstrated. Complete-history checkout and separate trusted selector/link-scanner responsibilities remain as previously reviewed.

Current dispositions: 3974257011, 3974257550, 3974258158, 3974258569, 3974259058.

Heading-only prose can change how a reader interprets an unchanged command. The selector protects code bytes and positions; it does not certify surrounding prose meaning. The new same-position heading example was reproduced and independently assessed against the documented boundary; broader section-context capture was declined. The real README heading-swap fixture selected docs and passed the selected checks. Full validation was not rerun for that fixture. See the review disposition.

The lint requirements are already active. This PR keeps local validation full and makes no branch-protection change. The real post-merge documentation exercise is tracked in #747; making Validate required remains a separate action. The allowlist remains deliberately narrow.

@github-actions github-actions Bot added the coderabbit:first-pass Triggers CodeRabbit's automatic first-pass pull-request review. label Sep 9, 2026
@chatgpt-codex-connector

chatgpt-codex-connector Bot commented Sep 9, 2026 •

Copy link
Copy Markdown

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review Status Commit Review trigger
📝 Code Review ✅ Completed 2026-09-10T00:16:50.055745Z 943c376 New commits
ℹ️ 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" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

@coderabbitai

coderabbitai Bot commented Sep 9, 2026

Copy link
Copy Markdown

Important

  • 🔍 Trigger review

This repository does not receive automatic reviews because it has fewer than 10 stars.

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Advanced

Run ID: 3b2e41ca-7e2c-4b27-8054-7da32b81ead6


Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

Comment thread bin/pr-validation-scope Resolved
Comment thread .github/workflows/validate.yml Resolved
Comment thread bin/pr-validation-scope Resolved
Comment thread bin/pr-validation-scope Resolved
Comment thread .github/workflows/validate.yml Resolved
@justin808

Copy link
Copy Markdown
Member Author

Reviewed the complete current-head feedback at e4b6ad5e508a25f31ca505bcf6c715b2f5790d3f. Five conversations are resolved with verified rationale; no code change was needed. Full CI passed. Current-base integration and merge authority remain separate gates.

Review decisions and evidence

Mattered

No confirmed must-fix defect remains from this review wave. The proposed tab-fence example renders its changed text as prose and fails the existing MD010 documentation check.

Optional and skipped

  • 3967101688: SKIPPED, declined and resolved. Replayed the exact example through markdown-it and the repository Markdown lint configuration. A leading tab expands to four columns, so this is not a CommonMark fenced block: the changed install ... line renders as paragraph text, while the tab-prefixed marker is indented code. CommonMark fence rules permit at most three spaces of indentation. More directly, the selected documentation gate rejects both tabs with MD010, so this example cannot produce a green reduced-validation result. Keeping the existing selector; no code change is needed for this report.
  • 3967101989: OPTIONAL, declined and resolved. Declined for this PR: complete PR trees are required by the trusted selector, while the measured delay is in validation suites. The proposed expression also uses a falsy numeric zero in && ... || ..., so it would select depth 1 for PRs and lose required history. A separate event-specific checkout optimization needs measured setup savings; this PR retains the straightforward complete-history checkout.
  • 3967102496: SKIPPED, declined and resolved. Declined: file contents are converted to binary before code projection, known Git/missing-object failures select full, and unknown paths already select full. An unexpected internal exception fails Validate rather than allowing reduced checks to report success. This report does not reproduce an unhandled repository input, so a blanket exception fallback would hide implementation faults without addressing an observed failure.
  • 3967102809: SKIPPED, declined and resolved. Declined: these are coverage labels, not identifiers matched against console headings. The combined documentation command checks both routing links and ordinary documentation links; listing those two checks separately does not omit either or affect selection. Deriving another manifest from shell output adds machinery without a demonstrated coverage error.
  • 3967103084: OPTIONAL, declined and resolved. Declined for this PR: the first real hosted documentation run is still needed to measure setup cost, as recorded in Require Lint on main and make Validate change-aware before requiring it #747. The local selected runner takes about 11 seconds, but that does not measure npm installation. Keep the pinned installation until hosted evidence shows whether caching is worth its invalidation and workflow complexity.

The Codex summary is a completed status artifact; no separate action was requested there. CodeRabbit explicitly skipped review and is not counted as a completed clean review. The scan used the default cutoff; no earlier checkpoint existed. No follow-up issue was created. Optional changes are not being tracked.

Future full-PR scans should start after this comment unless check all reviews is requested.

@justin808

Copy link
Copy Markdown
Member Author

The selected documentation path passed in real hosted CI in both PR states, using #810 as a temporary exercise against this implementation branch:

Both runs selected policy from base e4b6ad5e508a25f31ca505bcf6c715b2f5790d3f and exercised the one-word ordinary README edit at head 3b6a05a1d512071c7ac6425f4f2334e186ec8c71. The ready run tested GitHub integration commit 9d057c7fbeb6e10e9a3a39242f71e158b5855aca. Logs explicitly list selected and omitted suites; full pack validation and its gem setup were skipped. This is successful selected coverage, not full pack evidence.

A separate local negative control changed the README description and was rejected by the existing native-plugin manifest contract. This demonstrates that selected checks still enforce that cross-file requirement.

The temporary exercise PR is being closed without merging. This qualifies the implemented selection route; #806 still needs its normal current-main integration qualification and merge authorization. These two small exercises are observations, not a repeated full-suite performance benchmark.

@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: 6293ff7da6

ℹ️ 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 bin/pr-validation-scope Resolved
Comment thread bin/pr-validation-scope Resolved
Comment thread bin/pr-validation-scope Resolved
Comment thread bin/pr-validation-scope Resolved
Comment thread bin/pr-validation-scope Resolved
Comment thread bin/pr-validation-scope Resolved
Comment thread bin/pr-validation-scope Resolved
Comment thread .github/workflows/validate.yml Resolved
@claude

claude Bot commented Sep 9, 2026

Copy link
Copy Markdown

Review summary

Left 7 inline comments. Overview:

Correctness

  • bin/pr-validation-scope:109 — the initial git rev-parse HEAD call isn't wrapped by the rescue that the rest of the file relies on for its fail-closed design, so a git hiccup here crashes the CI step instead of falling back to full.
  • bin/pr-validation-scope:112 — full_suites is scraped from bin/validate's raw source text via regex, so rubocop ${RUBOCOP_VERSION} gets captured unexpanded and leaks into the job summary's "Omitted suites" line on docs-scope PRs.

Simplification / dead logic

  • bin/pr-validation-scope:49 — re-derives git rev-parse HEAD and compares it to target, which was computed via the identical call moments earlier in run and passed straight through. In the real call path this can never be false — it's a no-op check.

Reuse

  • bin/pr-validation-scope:23 (git wrapper) and :77 (code_lines fence tracking) each duplicate logic that already exists elsewhere in bin/ (git_capture/git_output in the delivery-state and status scripts; fence tracking in bin/validate-doc-links, which is loaded into the same process as this file via bin/validate-docs). Worth reusing rather than maintaining parallel implementations that can drift.

Scope/simplicity

  • bin/pr-validation-scope:9 — the fast path is a hardcoded 4-file allowlist (two of which are dated postmortem filenames) rather than a pattern. Since the policy is read from the PR's base commit, a new postmortem can never get fast-path treatment in the same PR that adds it, and the ~130 lines of new Ruby (including a hand-rolled partial Markdown parser in code_lines) buy a speedup for exactly 4 files rather than a general "docs-only" rule.
  • .github/workflows/validate.yml:26 — fetch-depth: 0 now unshallow-fetches full history on every run, paid before scope is even known, even though most PRs won't hit the narrow allowlist and will select full anyway.

No security issues found — SHAs are validated as strict 40-hex before being interpolated into git argv arrays (passed as argv, not shell strings), so there's no injection vector even for fork PRs.

Comment thread .github/workflows/validate.yml Resolved
Comment thread bin/pr-validation-scope Resolved
Comment thread bin/pr-validation-scope Resolved
Comment thread .github/workflows/validate.yml Resolved

@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: 943c3768cf

ℹ️ 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 bin/pr-validation-scope Resolved
Comment thread .github/workflows/validate.yml Resolved
Comment thread bin/pr-validation-scope Resolved
Comment thread bin/pr-validation-scope Resolved
Comment thread .github/workflows/validate.yml Resolved
Comment thread bin/pr-validation-scope Resolved
@justin808
justin808 merged commit ad8175a into main Sep 10, 2026
28 checks passed
@justin808
justin808 deleted the jg-codex/speed-issue-747 branch September 10, 2026 00:40
justin808 added a commit that referenced this pull request Sep 10, 2026
…data-trust-boundary

* origin/main:
  Speed up PR validation for ordinary documentation (#806)
  Batch installer copies to reduce validation overhead (#807)
justin808 added a commit that referenced this pull request Sep 10, 2026
…address-review

* origin/main:
  Speed up PR validation for ordinary documentation (#806)
justin808 added a commit that referenced this pull request Sep 10, 2026
…usted-base-provenance

* origin/main:
  Speed up PR validation for ordinary documentation (#806)
justin808 added a commit that referenced this pull request Sep 10, 2026
…ical-token-budgets

* origin/main:
  Separate integration checks from full release verification (#808)
  Make address-review summaries human-ready (#795)
  Speed up PR validation for ordinary documentation (#806)
  Batch installer copies to reduce validation overhead (#807)
  Report oversized PR diffs as blocked preflight coverage (#748)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

coderabbit:first-pass Triggers CodeRabbit's automatic first-pass pull-request review.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant