Skip to content

Add code review standards: Copilot config, reviewer guide, and CI enforcement - #221

Merged
lcn2 merged 8 commits into
masterfrom
copilot/add-code-review-requirements
Oct 4, 2026
Merged

lcn2 merged 8 commits into
masterfrom
copilot/add-code-review-requirements

Conversation

Copilot AI commented Oct 4, 2026 •

Copy link
Copy Markdown
Contributor

Encodes three review standards for calc: no goto in C, tests must pass, and no typos in docs or code comments. They are written into Copilot code review config and a human-facing guide, and enforced by PR workflows. No C source is modified.

  • Copilot config
    • .github/copilot/code-review-instructions.md holds the three standards.
    • .github/copilot-instructions.md carries the repo-wide rules.
    • .github/instructions/*.instructions.md are path-scoped via applyTo: c-code and comments apply to **/*.c,**/*.h; docs applies to **/*.md,**/*.txt,help/**.
  • .github/CODE_REVIEW_GUIDE.md
    • Covers rationale and alternatives for the no-goto rule, the local test commands, and the typo-check usage.
    • Includes a reviewer checklist and how the Copilot config and workflows fit together.
  • Workflows (.github/workflows/): all trigger on pull_request (opened, synchronize, reopened) to master, with contents: read and fetch-depth: 0.
    • no-goto.yml scans only added lines in *.c/*.h and strips strings and comments before matching. It prints file:line for each offender.
    • tests.yml installs the build dependencies, then runs make clobber all, make check, and make chk.
    • typos.yml runs crate-ci/typos@v1 on changed .c, .h, .md, .txt, .1, and help/* files only.
  • _typos.toml: excludes generated and vendored paths and cal/, and allows project terms.

Review focus:

  • The _typos.toml exclusions and allow-list are a first guess. Expect to tune them against real runs.
  • The comment and string stripping in the no-goto awk script is heuristic. It won't handle multi-line block comments beyond skipping lines that start with *.
  • The existing test.yml also runs on PRs, so builds run twice. Consider retiring it in favour of tests.yml.

Follow-up (repo settings): mark these as required status checks in branch protection:

  • workflow "No goto", job no-goto
  • workflow "Tests", job tests
  • workflow "Typos", job typos

Co-authored-by: lcn2 <10429707+lcn2@users.noreply.github.com>
Copilot AI changed the title [WIP] Add code review requirements and automated enforcement Add code review standards: Copilot config, reviewer guide, and CI enforcement Oct 4, 2026
Copilot AI requested a review from lcn2 October 4, 2026 00:30
@lcn2

lcn2 commented Oct 4, 2026

Copy link
Copy Markdown
Owner

I see a number of new files under .github. I have some additional requirements and refinements to the added files. Should I attempt to directly modify PR 221, or work with you in this chat, or work with the PR 221, or ???

@lcn2 lcn2 left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

I have some additional requirements and refinements to the added files. Should I attempt to directly modify PR 221, or work with you in this chat, or work with the PR 221, or ???

Copilot AI and others added 2 commits October 4, 2026 01:01
Co-authored-by: lcn2 <10429707+lcn2@users.noreply.github.com>
Co-authored-by: lcn2 <10429707+lcn2@users.noreply.github.com>

@lcn2 lcn2 left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

The Code Review Guide needs to be extended as follows:

  1. The code must be portable to the current versions of macOS, RHEL Linux, and NetBSD
  2. The calc code must be CPU byte-order independent: make chk must work on both Little Endian (Apple Silicon arm64, and x86_64), and Big Endian (s390x) hardware.
  3. The documentation to be checked includes the files under the help/ directory: these must be included in the checks for typos and consistency.
  4. The documentation to be checked also includes BUSG, calc.man, CHANGES, CONTRIB-CODE, HOWTO.INSTALL, LIBRARY, README.FIRST, README.md, README.RELEASE, README.WINDOWS, sample.README, SECURITY.md.
  5. The ./trailblank script must run without error.
  6. The ./trailblank script must also ignore files under the .github/ sub-directory.
  7. The C code must conform to the format specified in .clang-format such that when make clang-format is run, the clang-format(1) tool does not need to modify any files.
  8. When a new version of calc is created, the CHANGES file must be updated with comments about the new version, including details of what changed.
  9. The cal/regress.cal regression test suite must NOT assume that the calc executable is in the local directory, and should not attempt to invoke the calc executable.

Copilot AI requested a review from lcn2 October 4, 2026 01:26
@lcn2
lcn2 marked this pull request as ready for review October 4, 2026 01:37
Copilot AI balanced review requested due to automatic review settings October 4, 2026 01:37

Copilot AI 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.

Copilot review overview

🟡 Changes recommended

The new enforcement misses maintained sources, includes an ignored Copilot configuration, and can reject valid multiline comments.

Review effort: Balanced
Findings: 4 Medium severity · 1 Low severity

Open (5)
What changed in this PR

Adds repository review standards, documentation, and CI enforcement for C control flow, testing, and spelling.

Changes:

  • Adds Copilot and contributor review guidance.
  • Adds PR workflows for tests, typo checks, and goto detection.
  • Configures typo exclusions and formatting-tool behavior.
File Description
trailblank Excludes .github from picky phase 1.
.github/​workflows/​typos.yml Checks changed files for typos.
.github/​workflows/​tests.yml Builds and runs regression checks.
.github/​workflows/​no-goto.yml Detects added goto statements.
.github/​instructions/​docs.instructions.md Adds documentation review guidance.
.github/​instructions/​comments.instructions.md Adds C-comment spelling guidance.
.github/​instructions/​c-code.instructions.md Adds C review requirements.
.github/​copilot/​code-review-instructions.md Adds review standards at an unsupported path.
.github/​copilot-instructions.md Defines repository-wide Copilot instructions.
.github/​CODE_REVIEW_GUIDE.md Documents standards and enforcement.
_typos.toml Configures typo exclusions and vocabulary.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment on lines +1 to +3
# Copilot code review standards for lcn2/calc

Apply these rules when reviewing pull requests.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Removed the unsupported duplicate instruction file and its guide reference in commit 02055f5.

Comment thread .github/workflows/no-goto.yml Outdated
Comment on lines +32 to +35
gsub(/\/\*.*\*\//, "", t)
sub(/\/\/.*/, "", t)
if (t ~ /^[ \t]*\*/ ) next
if (t ~ /(^|[^A-Za-z0-9_])goto([^A-Za-z0-9_]|$)/) {

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Updated the workflow to scan full-file C lexical state before checking added lines, so multiline comments are ignored in commit 02055f5.

Comment thread .github/workflows/typos.yml Outdated
run: |
files=$(git diff --name-only --diff-filter=ACMR \
"origin/${{ github.base_ref }}...HEAD" -- \
'*.c' '*.h' '*.md' '*.txt' '*.1' 'help/*' | tr '\n' ' ')

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Added *.man to the changed-file pathspec in commit 02055f5.

Comment thread _typos.toml Outdated
@@ -0,0 +1,5 @@
---
applyTo: "**/*.md,**/*.txt,help/**"

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Added **/*.man to the docs instruction scope in commit 02055f5.

@lcn2 lcn2 left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

@copilot please update the Code Review Guide, copilot instructions, code review instructions to indicate that:

  • The C code must conform clang-format options as found the .clang-format file, such that when make clang-format is run, the clang-format(1) tool does not need to modify any files.

Pease add workflows to ensure that the code confirms too clang-format options as found the .clang-format file, such that when make clang-format is run, the clang-format(1) tool does not need to modify any files.

Co-authored-by: lcn2 <10429707+lcn2@users.noreply.github.com>

Copilot AI commented Oct 4, 2026

Copy link
Copy Markdown
Contributor Author

@copilot please update the Code Review Guide, copilot instructions, code review instructions to indicate that:

  • The C code must conform clang-format options as found the .clang-format file, such ...

Added the clang-format requirement to the review guidance and Copilot instructions, plus a PR workflow that runs make clang-format and fails if any tracked files change. The change is committed as COMMIT_HASH.

Co-authored-by: lcn2 <10429707+lcn2@users.noreply.github.com>

@lcn2 lcn2 left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Please move the _typos.toml somewhere under the .github directory, if possible, or if not, rename it to .typos.toml so that the file is "hidden" for normal ls views.

Co-authored-by: lcn2 <10429707+lcn2@users.noreply.github.com>

@lcn2 lcn2 left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

@copilot please move _typos.toml under the .github/ sub-directory.

Co-authored-by: lcn2 <10429707+lcn2@users.noreply.github.com>

Copilot AI commented Oct 4, 2026

Copy link
Copy Markdown
Contributor Author

@copilot please move _typos.toml under the .github/ sub-directory.

Moved the config to .github/_typos.toml and updated its references in commit 7c2afb1.

Copilot AI requested a review from lcn2 October 4, 2026 02:27

@lcn2 lcn2 left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

🤖👍

@lcn2
lcn2 merged commit c8edd3e into master Oct 4, 2026
8 checks passed
@lcn2
lcn2 deleted the copilot/add-code-review-requirements branch October 4, 2026 02:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants