From ef21d1cf3776ecc91eb4b3c8789ee6e9fd1b54b6 Mon Sep 17 00:00:00 2001 From: Jon Olson Date: Sun, 4 Oct 2026 21:59:31 -0700 Subject: [PATCH 1/5] Route review-policy inputs to the maintainer. The gate scripts, lint and format settings, and agent override files could change review behavior without a policy warning. Treat these inputs as review policy and align CODEOWNERS with that scope so the maintainer sees changes to the checks and their guidance. --- .github/CODEOWNERS | 8 ++++++-- CONTRIBUTING.md | 4 ++++ internal/ci/checkpr/changes.go | 13 +++++++------ internal/ci/checkpr/changes_test.go | 24 ++++++++++++++++++++++++ 4 files changed, 41 insertions(+), 8 deletions(-) diff --git a/.github/CODEOWNERS b/.github/CODEOWNERS index 9a6f5fd..2a26137 100644 --- a/.github/CODEOWNERS +++ b/.github/CODEOWNERS @@ -1,7 +1,11 @@ # Review policy and public contribution guidance. /.github/ @jon -/AGENTS.md @jon -**/AGENTS.md @jon +/AGENTS*.md @jon +**/AGENTS*.md @jon +/internal/ci/ @jon +/.golangci*.yml @jon +/.markdownlint-cli2.yaml @jon +/.prettierrc.json @jon /CODE_OF_CONDUCT.md @jon /CONTRIBUTING.md @jon /SECURITY.md @jon diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 0f02326..72add81 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -157,6 +157,10 @@ approval. Resolve its actionable conversations or explain the disposition before merging. If Codex completes after the gate's polling window, rerun the failed `codex-reviewed` job to evaluate the completed result. +Changes to policy tooling, lint and format configuration, files under +`.github/`, and `AGENTS*.md` instruction files receive a review-policy warning +and are assigned to the maintainer in CODEOWNERS. + Untrusted pull-request workflows receive no secrets or write-capable checkout credentials and never run HIL. CodeQL uses the ordinary `pull_request` event and GitHub's built-in token; it does not use `pull_request_target` or a maintainer diff --git a/internal/ci/checkpr/changes.go b/internal/ci/checkpr/changes.go index e49f50d..0e80312 100644 --- a/internal/ci/checkpr/changes.go +++ b/internal/ci/checkpr/changes.go @@ -168,12 +168,13 @@ func isTestFile(path string) bool { } func isReviewPolicy(path string) bool { - return path == "CONTRIBUTING.md" || - path == ".github/CODEOWNERS" || - path == ".github/pull_request_template.md" || - path == "AGENTS.md" || - strings.HasSuffix(path, "/AGENTS.md") || - strings.HasPrefix(path, ".github/workflows/") || + name := filepath.Base(path) + return path == "CONTRIBUTING.md" || path == "SECURITY.md" || + path == "CODE_OF_CONDUCT.md" || + (strings.HasPrefix(name, "AGENTS") && strings.HasSuffix(name, ".md")) || + (strings.HasPrefix(path, ".golangci") && strings.HasSuffix(path, ".yml")) || + path == ".markdownlint-cli2.yaml" || path == ".prettierrc.json" || + strings.HasPrefix(path, ".github/") || strings.HasPrefix(path, "internal/ci/") } diff --git a/internal/ci/checkpr/changes_test.go b/internal/ci/checkpr/changes_test.go index f8fa12c..79fb591 100644 --- a/internal/ci/checkpr/changes_test.go +++ b/internal/ci/checkpr/changes_test.go @@ -42,6 +42,30 @@ func TestCheckCommitChangesWarnsAboutPolicyFiles(t *testing.T) { } } +func TestReviewPolicyIncludesGateAndInstructionInputs(t *testing.T) { + for _, path := range []string{ + ".github/scripts/codex-review-signal.jq", + ".github/scripts/codex-review-signal_test.sh", + ".github/actionlint.yaml", + ".github/ISSUE_TEMPLATE/bug-report.yml", + ".golangci.yml", ".golangci.tests.yml", + ".markdownlint-cli2.yaml", ".prettierrc.json", + "SECURITY.md", "CODE_OF_CONDUCT.md", + "AGENTS.override.md", "usb/AGENTS.override.md", + } { + t.Run(path, func(t *testing.T) { + if !isReviewPolicy(path) { + t.Fatalf("%s is missing from review policy", path) + } + }) + } + for _, path := range []string{"usb/device.go", "docs/usb.md", "internal/ci-example.go"} { + if isReviewPolicy(path) { + t.Errorf("%s is not a review-policy input", path) + } + } +} + func TestCheckCommitChangesWarnsAboutRenamedPolicyFiles(t *testing.T) { repo := newRepository(t) commitPaths(t, repo, "Establish the base.\n", map[string]string{ From d41e577bc1913aa36d4b752bfdb5b5ca2a8fd263 Mon Sep 17 00:00:00 2001 From: Jon Olson Date: Sun, 4 Oct 2026 22:11:05 -0700 Subject: [PATCH 2/5] Evaluate pull requests with the base revision's policy. A proposed head could run its own weakened checker or review evaluator. Build the checker from the base revision and apply it to head data; load the review evaluator from the base as well. Require a current ready head before accepting a review, retry failed API reads within the polling window, and attribute fork runs by their head repository. Reject failed or empty commit enumeration so the per-commit job cannot succeed without testing a commit. --- .github/scripts/codex-review-signal.jq | 5 +- .github/scripts/codex-review-signal_test.sh | 4 + .github/scripts/publication-guards_test.py | 199 ++++++++++++++++++++ .github/workflows/test.yml | 57 ++++-- CONTRIBUTING.md | 6 + 5 files changed, 257 insertions(+), 14 deletions(-) create mode 100644 .github/scripts/publication-guards_test.py diff --git a/.github/scripts/codex-review-signal.jq b/.github/scripts/codex-review-signal.jq index a6ec408..96cd5fc 100644 --- a/.github/scripts/codex-review-signal.jq +++ b/.github/scripts/codex-review-signal.jq @@ -5,7 +5,10 @@ def reviewed_commit: ) catch empty; ([$runs[0][] - | select(.head_sha == $head and any(.pull_requests[]?; .number == $pr)) + | select(.head_sha == $head and ( + any(.pull_requests[]?; .number == $pr) + or ((.pull_requests | length) == 0 and .head_repository.id == $head_repository) + )) | .created_at ] | min) as $head_first_seen_at | select(($head_first_seen_at | type) == "string") diff --git a/.github/scripts/codex-review-signal_test.sh b/.github/scripts/codex-review-signal_test.sh index 35e1e2e..d9e0fd6 100644 --- a/.github/scripts/codex-review-signal_test.sh +++ b/.github/scripts/codex-review-signal_test.sh @@ -57,6 +57,7 @@ evaluate_files() { --slurpfile runs "$runs_file" \ --arg head "$head_sha" \ --argjson pr "$pr_number" \ + --argjson head_repository 1234 \ -f "$filter" >/dev/null } @@ -108,6 +109,9 @@ reject malformed-summary "$(jq '.comments[0].body = "not metadata"' <<< "$matchi reject wrong-run-head "$(jq '.runs[0].head_sha = "ffffffffffffffffffffffffffffffffffffffff"' <<< "$matching")" reject wrong-run-pr "$(jq '.runs[0].pull_requests[0].number = 54' <<< "$matching")" reject missing-run "$(jq '.runs = []' <<< "$matching")" +accept fork-run "$(jq '.runs[0] |= (.pull_requests = [] | .head_repository = {id: 1234})' <<< "$matching")" +reject wrong-fork-repository "$(jq '.runs[0] |= (.pull_requests = [] | .head_repository = {id: 5678})' <<< "$matching")" +reject unattributed-run "$(jq '.runs[0].pull_requests = []' <<< "$matching")" argument_limit=$(getconf ARG_MAX) if [[ ! "$argument_limit" =~ ^[0-9]+$ ]]; then diff --git a/.github/scripts/publication-guards_test.py b/.github/scripts/publication-guards_test.py new file mode 100644 index 0000000..6665489 --- /dev/null +++ b/.github/scripts/publication-guards_test.py @@ -0,0 +1,199 @@ +#!/usr/bin/env python3 +"""Exercise workflow commands with local repositories and a fake GitHub API.""" + +import os +from pathlib import Path +import subprocess +import tempfile +import unittest + +ROOT = Path(__file__).resolve().parents[2] + + +def step_script(name): + lines = (ROOT / '.github/workflows/test.yml').read_text().splitlines() + start = lines.index(' - name: ' + name) + for index in range(start + 1, len(lines)): + if lines[index] in (' run: |', ' run: >-'): + body = [] + for line in lines[index + 1:]: + if line.strip() and not line.startswith(' '): + break + body.append(line[10:]) + separator = ' ' if lines[index].endswith('>-') else '\n' + return separator.join(body) + raise ValueError('missing multiline run block: ' + name) + + +class PublicationGuards(unittest.TestCase): + def setUp(self): + self.temporary = tempfile.TemporaryDirectory() + self.addCleanup(self.temporary.cleanup) + self.root = Path(self.temporary.name) + self.repo = self.root / 'repo' + self.repo.mkdir() + self.git('init', '-q') + self.git('config', 'user.name', 'Test') + self.git('config', 'user.email', 'test@example.invalid') + self.env = os.environ | { + 'GITHUB_WORKSPACE': str(self.repo), + 'RUNNER_TEMP': str(self.root), + 'GITHUB_EVENT_PATH': str(self.root / 'event.json'), + } + + def git(self, *args): + return subprocess.check_output(['git', *args], cwd=self.repo, text=True).strip() + + def commit(self): + self.git('add', '.') + self.git('commit', '-qm', 'Establish the fixture.') + return self.git('rev-parse', 'HEAD') + + def run_step(self, name): + return subprocess.run( + ['bash', '-eo', 'pipefail', '-c', step_script(name)], + cwd=self.repo, env=self.env, text=True, capture_output=True, + ) + + def test_invalid_commit_range_fails(self): + self.env |= {'BASE_SHA': 'not-a-revision', 'HEAD_SHA': 'HEAD'} + result = self.run_step('Test every pull-request commit') + self.assertNotEqual(result.returncode, 0, result.stdout + result.stderr) + + def test_review_evaluator_uses_base_checkout(self): + workflow = (ROOT / '.github/workflows/test.yml').read_text() + review = workflow.split(' codex-reviewed:', 1)[1] + checkout = review.split(' - name: Check out the review policy', 1)[1] + checkout = checkout.split(' - name: Wait for Codex', 1)[0] + self.assertIn('ref: ${{ github.event.pull_request.base.sha }}', checkout) + self.assertNotIn('github.event.pull_request.draft', review) + + def test_empty_commit_range_fails(self): + (self.repo / 'fixture').write_text('base') + sha = self.commit() + self.env |= {'BASE_SHA': sha, 'HEAD_SHA': sha} + result = self.run_step('Test every pull-request commit') + self.assertNotEqual(result.returncode, 0, result.stdout + result.stderr) + + def test_checker_uses_base_code_and_head_data(self): + (self.repo / 'go.mod').write_text('module fixture\n\ngo 1.25\n') + checker = self.repo / 'internal/ci/checkpr/main.go' + checker.parent.mkdir(parents=True) + checker.write_text('''package main +import ("flag"; "os"; "path/filepath") +func main() { + repo := flag.String("repo", ".", "") + flag.String("base", "", ""); flag.String("head", "", "") + flag.String("event", "", ""); flag.Parse() + data, err := os.ReadFile(filepath.Join(*repo, "proposal")) + if err != nil || string(data) != "head data" { os.Exit(2) } + os.Exit(1) +} +''') + (self.repo / 'proposal').write_text('base data') + base = self.commit() + checker.write_text('package main\nfunc main() {}\n') + (self.repo / 'proposal').write_text('head data') + head = self.commit() + self.git('worktree', 'add', '--detach', str(self.root / 'policy-base'), base) + self.env |= {'BASE_SHA': base, 'HEAD_SHA': head} + result = self.run_step('Check commits and pull-request metadata') + self.assertEqual(result.returncode, 1, result.stdout + result.stderr) + + + def run_review(self, mode): + scripts = self.repo / '.github/scripts' + scripts.mkdir(parents=True) + (scripts / 'codex-review-signal.jq').write_text( + (ROOT / '.github/scripts/codex-review-signal.jq').read_text() + ) + tools = self.root / 'tools' + tools.mkdir() + fake = tools / 'gh' + fake.write_text("#!/usr/bin/env python3\n" + FAKE_GH) + fake.chmod(0o755) + sleeper = tools / 'sleep' + sleeper.write_text('#!/bin/sh\nexit 0\n') + sleeper.chmod(0o755) + self.env |= { + 'PATH': str(tools) + os.pathsep + self.env['PATH'], + 'GH_REPO': 'owner/repo', 'PR_NUMBER': '53', + 'HEAD_SHA': '0123456789abcdef0123456789abcdef01234567', + 'GUARD_TEST_MODE': mode, 'GUARD_TEST_STATE': str(self.root / 'state'), + } + return self.run_step('Wait for Codex to review the current head') + + def test_draft_cannot_pass_review(self): + result = self.run_review('draft') + self.assertNotEqual(result.returncode, 0, result.stdout + result.stderr) + + def test_superseded_head_cannot_pass_review(self): + result = self.run_review('superseded') + self.assertNotEqual(result.returncode, 0, result.stdout + result.stderr) + + def test_transient_timeline_failure_retries(self): + result = self.run_review('transient') + self.assertEqual(result.returncode, 0, result.stdout + result.stderr) + + def test_fork_run_is_attributed(self): + result = self.run_review('fork') + self.assertEqual(result.returncode, 0, result.stdout + result.stderr) + + def test_wrong_fork_repository_cannot_pass(self): + result = self.run_review('wrong-fork') + self.assertNotEqual(result.returncode, 0, result.stdout + result.stderr) + + +FAKE_GH = r""" +import json +import os +from pathlib import Path +import sys + +endpoint = next(arg for arg in sys.argv[1:] if arg.startswith('repos/')) +mode = os.environ['GUARD_TEST_MODE'] +head = os.environ['HEAD_SHA'] +actor = 'chatgpt-codex-connector[bot]' +if '/pulls/' in endpoint: + print(json.dumps({'draft': mode == 'draft', 'head': { + 'sha': 'f' * 40 if mode == 'superseded' else head, + 'repo': {'id': 1234}, + }})) +elif '/commits/' in endpoint: + print(head) +elif '/timeline?' in endpoint: + if mode == 'transient': + state = Path(os.environ['GUARD_TEST_STATE']) + if not state.exists(): + state.touch() + sys.exit(1) + entries = [] if 'fork' in mode else [ + {'event': 'committed', 'sha': head}, + {'event': 'commented', 'user': {'login': actor}, + 'body': "Codex Review: Didn't find any major issues.\n**Reviewed commit:** `" + head + '`'}, + ] + print(json.dumps([entries])) +elif '/comments?' in endpoint: + rows = [] if 'fork' not in mode else [{ + 'user': {'login': actor}, + 'body': '\n' + '| Review | Status | Commit |\n' + '| 📝 **Code Review** | ✅ **Completed** | `' + head[:7] + '` |', + }] + print(json.dumps([rows])) +elif '/reactions?' in endpoint: + print(json.dumps([[{'user': {'login': actor}, 'content': '+1', + 'created_at': '2026-08-29T20:29:46Z'}]])) +elif '/actions/runs?' in endpoint: + print(json.dumps([{'workflow_runs': [{ + 'head_sha': head, 'created_at': '2026-08-29T20:29:30Z', + 'pull_requests': [], + 'head_repository': {'id': 5678 if mode == 'wrong-fork' else 1234}, + }]}])) +else: + sys.exit('unexpected endpoint: ' + endpoint) +""" + + +if __name__ == '__main__': + unittest.main() diff --git a/.github/workflows/test.yml b/.github/workflows/test.yml index e1664af..ab3d534 100644 --- a/.github/workflows/test.yml +++ b/.github/workflows/test.yml @@ -37,17 +37,22 @@ jobs: fetch-depth: 0 persist-credentials: false ref: ${{ github.event.pull_request.head.sha }} + - name: Prepare the base policy source + run: git worktree add --detach "$RUNNER_TEMP/policy-base" "$BASE_SHA" - name: Set up Go uses: actions/setup-go@b7ad1dad31e06c5925ef5d2fc7ad053ef454303e # v7 with: - go-version-file: go.mod + go-version-file: ${{ runner.temp }}/policy-base/go.mod cache: false - name: Check commits and pull-request metadata - run: >- - go run ./internal/ci/checkpr - -base "$BASE_SHA" - -head "$HEAD_SHA" - -event "$GITHUB_EVENT_PATH" + shell: bash + run: | + ( + cd "$RUNNER_TEMP/policy-base" + GOWORK=off go build -o "$RUNNER_TEMP/checkpr" ./internal/ci/checkpr + ) + "$RUNNER_TEMP/checkpr" -repo "$GITHUB_WORKSPACE" \ + -base "$BASE_SHA" -head "$HEAD_SHA" -event "$GITHUB_EVENT_PATH" commits: if: ${{ github.event_name == 'pull_request' }} @@ -81,6 +86,11 @@ jobs: } trap cleanup EXIT + commits=$(git rev-list --reverse "${BASE_SHA}..${HEAD_SHA}") + if [[ -z "$commits" ]]; then + echo "The pull-request commit range is empty." + exit 1 + fi while read -r commit; do echo "Testing ${commit}" git worktree add --detach "$worktree" "$commit" @@ -97,7 +107,7 @@ jobs: go test -race ./... ) git worktree remove --force "$worktree" - done < <(git rev-list --reverse "${BASE_SHA}..${HEAD_SHA}") + done <<< "$commits" quality: name: quality @@ -149,7 +159,9 @@ jobs: npx --yes "prettier@${PRETTIER_VERSION}" --check "${markdown_files[@]}" npx --yes "markdownlint-cli2@${MARKDOWNLINT_VERSION}" -- "${markdown_files[@]}" - name: Test Codex review signal - run: bash .github/scripts/codex-review-signal_test.sh + run: | + bash .github/scripts/codex-review-signal_test.sh + python3 .github/scripts/publication-guards_test.py - name: Lint workflows run: | go install "github.com/rhysd/actionlint/cmd/actionlint@${ACTIONLINT_VERSION}" @@ -198,7 +210,7 @@ jobs: usb/iokit_darwin.c codex-reviewed: - if: ${{ github.event_name == 'pull_request' && !github.event.pull_request.draft }} + if: ${{ github.event_name == 'pull_request' }} name: codex-reviewed runs-on: ubuntu-latest timeout-minutes: 25 @@ -216,10 +228,19 @@ jobs: uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7 with: persist-credentials: false - ref: ${{ github.event.pull_request.head.sha }} + ref: ${{ github.event.pull_request.base.sha }} - name: Wait for Codex to review the current head shell: bash run: | + current_pull_request() { + gh api "repos/${GH_REPO}/pulls/${PR_NUMBER}" + } + + current_reviewable_head() { + current_pull_request | jq -e --arg head "$HEAD_SHA" \ + '.draft == false and .head.sha == $head' >/dev/null + } + post_head_timeline() { gh api \ --paginate \ @@ -292,6 +313,7 @@ jobs: --slurpfile runs "$review_runs_file" \ --arg head "$HEAD_SHA" \ --argjson pr "$PR_NUMBER" \ + --argjson head_repository "$head_repository" \ -f .github/scripts/codex-review-signal.jq > "$review_prefixes_file" || return 1 reviewed_head < "$review_prefixes_file" } @@ -318,13 +340,22 @@ jobs: trap cleanup EXIT for attempt in {1..60}; do - if clean_reaction_review; then + pull_request=$(current_pull_request) || pull_request='{}' + head_repository=$(jq -er --arg head "$HEAD_SHA" ' + select(.draft == false and .head.sha == $head) | .head.repo.id + ' <<< "$pull_request") || head_repository='null' + if [[ "$head_repository" == "null" ]]; then + echo "Waiting for a current ready pull-request head (${attempt}/60)." + sleep 20 + continue + fi + if clean_reaction_review && current_reviewable_head; then echo "Codex completed a clean review of ${HEAD_SHA}." finish 0 fi - post_head_entries=$(post_head_timeline) - if clean_comment_review; then + post_head_entries=$(post_head_timeline) || post_head_entries='[]' + if clean_comment_review && current_reviewable_head; then echo "Codex completed a clean review of ${HEAD_SHA}." finish 0 fi diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 72add81..2b3003f 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -161,6 +161,12 @@ Changes to policy tooling, lint and format configuration, files under `.github/`, and `AGENTS*.md` instruction files receive a review-policy warning and are assigned to the maintainer in CODEOWNERS. +The policy job builds its checker from the pull request's base revision and +checks the proposed head's commits and Markdown. The Codex gate also loads its +review evaluator from the base revision. It checks the pull request's current +head and draft state before accepting a review, including on reruns of older +workflow events. Failed or empty commit enumeration fails the per-commit job. + Untrusted pull-request workflows receive no secrets or write-capable checkout credentials and never run HIL. CodeQL uses the ordinary `pull_request` event and GitHub's built-in token; it does not use `pull_request_target` or a maintainer From 9cf3af0d4272f57bf06bd1dd9c6e73c206863b33 Mon Sep 17 00:00:00 2001 From: Jon Olson Date: Sun, 4 Oct 2026 22:15:32 -0700 Subject: [PATCH 3/5] Lint integration tests and native Go on both hosts. CI compiled integration tests but never applied the lint profiles to them, and macOS implementation files were outside the Linux lint jobs. Run both profiles and Staticcheck on ordinary and integration-tagged Go on Linux and macOS. Split bench identity checks, cleanup, and scratch recovery into smaller helpers so the integration tests meet the existing limits. Keep their protocol calls and restoration order intact. --- .github/scripts/publication-guards_test.py | 12 +++ .github/workflows/test.yml | 16 ++- CONTRIBUTING.md | 7 +- cmsisdap/session_integration_test.go | 50 +++++---- dap/jtag_integration_test.go | 111 +++++++++++--------- dap/memap_block_integration_test.go | 10 +- dap/memap_write_integration_test.go | 91 ++++++++++------ jlink/session_integration_test.go | 50 +++++---- target/cortexm/register_integration_test.go | 5 +- 9 files changed, 216 insertions(+), 136 deletions(-) diff --git a/.github/scripts/publication-guards_test.py b/.github/scripts/publication-guards_test.py index 6665489..28579b3 100644 --- a/.github/scripts/publication-guards_test.py +++ b/.github/scripts/publication-guards_test.py @@ -68,6 +68,18 @@ def test_review_evaluator_uses_base_checkout(self): self.assertIn('ref: ${{ github.event.pull_request.base.sha }}', checkout) self.assertNotIn('github.event.pull_request.draft', review) + def test_integration_lint_on_both_hosts(self): + workflow = (ROOT / '.github/workflows/test.yml').read_text() + for job, following in [('quality', 'macos'), ('macos', 'codex-reviewed')]: + commands = workflow.split(' ' + job + ':', 1)[1] + commands = commands.split(' ' + following + ':', 1)[0] + for command in [ + 'staticcheck -tags integration ./...', + 'golangci-lint run --build-tags integration --config .golangci.yml ./...', + 'golangci-lint run --build-tags integration --config .golangci.tests.yml ./...', + ]: + self.assertIn(command, commands, job + ' is missing ' + command) + def test_empty_commit_range_fails(self): (self.repo / 'fixture').write_text('base') sha = self.commit() diff --git a/.github/workflows/test.yml b/.github/workflows/test.yml index ab3d534..74b557e 100644 --- a/.github/workflows/test.yml +++ b/.github/workflows/test.yml @@ -143,12 +143,16 @@ jobs: run: | go install "honnef.co/go/tools/cmd/staticcheck@${STATICCHECK_VERSION}" staticcheck ./... + staticcheck -tags integration ./... - name: Run production-code lint run: | go install "github.com/golangci/golangci-lint/v2/cmd/golangci-lint@${GOLANGCI_LINT_VERSION}" golangci-lint run --config .golangci.yml ./... + golangci-lint run --build-tags integration --config .golangci.yml ./... - name: Run test-code lint - run: golangci-lint run --config .golangci.tests.yml ./... + run: | + golangci-lint run --config .golangci.tests.yml ./... + golangci-lint run --build-tags integration --config .golangci.tests.yml ./... - name: Check tracked Markdown formatting and lint shell: bash run: | @@ -198,6 +202,16 @@ jobs: run: go vet ./... - name: Run race tests run: go test -race ./... + - name: Lint Go on macOS + run: | + go install "honnef.co/go/tools/cmd/staticcheck@${STATICCHECK_VERSION}" + go install "github.com/golangci/golangci-lint/v2/cmd/golangci-lint@${GOLANGCI_LINT_VERSION}" + staticcheck ./... + staticcheck -tags integration ./... + golangci-lint run --config .golangci.yml ./... + golangci-lint run --config .golangci.tests.yml ./... + golangci-lint run --build-tags integration --config .golangci.yml ./... + golangci-lint run --build-tags integration --config .golangci.tests.yml ./... - name: Compile integration tests run: go test -tags=integration -run '^$' ./... - name: Check native C formatting diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 2b3003f..22457bf 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -127,9 +127,10 @@ pull-request metadata, and broken local Markdown links or anchors. The `commits` check runs formatting, build, vet, and race tests independently at every commit. The `quality` and macOS checks validate the final tip with the additional linters, vulnerability scan, integration-tag compilation, and native C checks -applicable to their hosts. The `CodeQL` workflow analyzes Go and native C/C++ -with the security-extended query suite on every pull request, including pull -requests from forks. +applicable to their hosts. Linux and macOS lint both ordinary and +integration-tagged Go, including the native macOS implementation. The `CodeQL` +workflow analyzes Go and native C/C++ with the security-extended query suite on +every pull request, including pull requests from forks. Policy annotations remain advisory when judgment is required. These include 73- through 120-column subjects, ambiguous imperative mood, weak bodies, commits diff --git a/cmsisdap/session_integration_test.go b/cmsisdap/session_integration_test.go index 24a9568..c65e1b6 100644 --- a/cmsisdap/session_integration_test.go +++ b/cmsisdap/session_integration_test.go @@ -122,28 +122,7 @@ func observeCMSISDAPTarget(t *testing.T, ctx context.Context, readyOnOpen bool) } t.Fatal(err) } - dpidr, ok := identity.DPIDR() - if !ok { - t.Fatal("SW-DP identity has no DPIDR") - } - transaction := debugPort.NewTxn() - dpidrResult := transaction.ReadDP(dap.DPIDR) - apidrResult := transaction.ReadAPIDR(dap.NewAPSel(0)) - if err := transaction.Commit(ctx); err != nil { - t.Fatal(err) - } - confirmedDPIDR, err := dpidrResult.Value() - if err != nil { - t.Fatal(err) - } - apidrRaw, err := apidrResult.Value() - if err != nil { - t.Fatal(err) - } - apidr := dap.DecodeAPIDR(apidrRaw) - if confirmedDPIDR != dpidr.Raw || apidr.Raw == 0 || apidr.Class != 8 { - t.Fatalf("debug identities = DPIDR %#08x/%#08x AP0 %+v", dpidr.Raw, confirmedDPIDR, apidr) - } + dpidr, apidr := confirmCMSISDAPTargetIdentities(t, ctx, debugPort, identity) savedCSW, err := debugPort.ReadRawAP(ctx, dap.NewAPSel(0).Address(0x00)) if err != nil { t.Fatal(err) @@ -189,6 +168,33 @@ func observeCMSISDAPTarget(t *testing.T, ctx context.Context, readyOnOpen bool) } } +func confirmCMSISDAPTargetIdentities(t *testing.T, ctx context.Context, debugPort *dap.DebugPort, identity dap.Identity) (dap.DPIDRInfo, dap.APIDRInfo) { + t.Helper() + dpidr, ok := identity.DPIDR() + if !ok { + t.Fatal("SW-DP identity has no DPIDR") + } + transaction := debugPort.NewTxn() + dpidrResult := transaction.ReadDP(dap.DPIDR) + apidrResult := transaction.ReadAPIDR(dap.NewAPSel(0)) + if err := transaction.Commit(ctx); err != nil { + t.Fatal(err) + } + confirmedDPIDR, err := dpidrResult.Value() + if err != nil { + t.Fatal(err) + } + apidrRaw, err := apidrResult.Value() + if err != nil { + t.Fatal(err) + } + apidr := dap.DecodeAPIDR(apidrRaw) + if confirmedDPIDR != dpidr.Raw || apidr.Raw == 0 || apidr.Class != 8 { + t.Fatalf("debug identities = DPIDR %#08x/%#08x AP0 %+v", dpidr.Raw, confirmedDPIDR, apidr) + } + return dpidr, apidr +} + func assertCMSISDAPAPRegister(t *testing.T, ctx context.Context, debugPort *dap.DebugPort, address uint8, want uint32) { t.Helper() got, err := debugPort.ReadRawAP(ctx, dap.NewAPSel(0).Address(uint16(address))) diff --git a/dap/jtag_integration_test.go b/dap/jtag_integration_test.go index a5b78a5..07503a6 100644 --- a/dap/jtag_integration_test.go +++ b/dap/jtag_integration_test.go @@ -58,32 +58,7 @@ func exerciseJTAGDPMemory(t *testing.T, ctx context.Context, discovered bool) (u } dp := NewDebugPort(JTAGDP(chain, 0), WithMaxWaits(100)) var mem *MemAP - defer func() { - for range 3 { - cleanup, cancel := context.WithTimeout(context.Background(), 30*time.Second) - err = mem.Release(cleanup) - if err == nil { - err = dp.Release(cleanup) - } - cancel() - if err == nil { - break - } - if errors.Is(err, ftdi.ErrChannelPoisoned) { - break - } - } - if err != nil { - t.Errorf("stopping without closing the probe after failed DAP cleanup: %v", err) - return - } - for range 3 { - if err = closeOwner(); err == nil { - return - } - } - t.Errorf("close probe: %v", err) - }() + defer closeJTAGDPBench(t, dp, &mem, closeOwner) identity, err := dp.Connect(ctx) if err != nil { t.Fatal(err) @@ -132,36 +107,39 @@ func exerciseJTAGDPMemory(t *testing.T, ctx context.Context, discovered bool) (u return power, overrun } -func openJTAGDPBench(t *testing.T, ctx context.Context, discovered bool) (jtag.Wire, func() error) { +func closeJTAGDPBench(t *testing.T, dp *DebugPort, mem **MemAP, closeOwner func() error) { t.Helper() - if discovered { - var registry discover.Registry - if err := ftdidiscovery.Register(®istry); err != nil { - t.Fatal(err) + var err error + for range 3 { + cleanup, cancel := context.WithTimeout(context.Background(), 30*time.Second) + err = (*mem).Release(cleanup) + if err == nil { + err = dp.Release(cleanup) } - inventory, err := registry.Probes(ctx) - if err != nil { - t.Fatal(err) - } - candidate, err := inventory.Select(discover.Selection{Provider: ftdidiscovery.ID, Serial: "01691", Function: "A"}) - if errors.Is(err, discover.ErrCandidateNotFound) || errors.Is(err, discover.ErrCandidateAmbiguous) { - t.Skip(err) - } - if err != nil { - t.Fatal(err) + cancel() + if err == nil { + break } - owner, err := candidate.Open(ctx) - if err != nil { - if owner != nil { - err = errors.Join(err, owner.Close()) - } - t.Fatal(err) + if errors.Is(err, ftdi.ErrChannelPoisoned) { + break } - wire, err := owner.JTAG(ctx, probe.JTAGConfig{MaxClockHz: 100_000}) - if err != nil { - t.Fatal(errors.Join(err, owner.Close())) + } + if err != nil { + t.Errorf("stopping without closing the probe after failed DAP cleanup: %v", err) + return + } + for range 3 { + if err = closeOwner(); err == nil { + return } - return wire, owner.Close + } + t.Errorf("close probe: %v", err) +} + +func openJTAGDPBench(t *testing.T, ctx context.Context, discovered bool) (jtag.Wire, func() error) { + t.Helper() + if discovered { + return openDiscoveredJTAGDPBench(t, ctx) } bus := usb.New() devices, err := bus.List(ctx, []usb.DeviceFilter{usb.ExactDevice(ftdi.VID, ftdi.PIDFT4232H)}) @@ -191,3 +169,34 @@ func openJTAGDPBench(t *testing.T, ctx context.Context, discovered bool) (jtag.W } return channel, channel.Close } + +func openDiscoveredJTAGDPBench(t *testing.T, ctx context.Context) (jtag.Wire, func() error) { + t.Helper() + var registry discover.Registry + if err := ftdidiscovery.Register(®istry); err != nil { + t.Fatal(err) + } + inventory, err := registry.Probes(ctx) + if err != nil { + t.Fatal(err) + } + candidate, err := inventory.Select(discover.Selection{Provider: ftdidiscovery.ID, Serial: "01691", Function: "A"}) + if errors.Is(err, discover.ErrCandidateNotFound) || errors.Is(err, discover.ErrCandidateAmbiguous) { + t.Skip(err) + } + if err != nil { + t.Fatal(err) + } + owner, err := candidate.Open(ctx) + if err != nil { + if owner != nil { + err = errors.Join(err, owner.Close()) + } + t.Fatal(err) + } + wire, err := owner.JTAG(ctx, probe.JTAGConfig{MaxClockHz: 100_000}) + if err != nil { + t.Fatal(errors.Join(err, owner.Close())) + } + return wire, owner.Close +} diff --git a/dap/memap_block_integration_test.go b/dap/memap_block_integration_test.go index 2baadc5..d3d2c14 100644 --- a/dap/memap_block_integration_test.go +++ b/dap/memap_block_integration_test.go @@ -111,6 +111,13 @@ func TestWriteMEMAPBlocksOverFTDI(t *testing.T) { t.Fatal(err) } + aligned, unaligned := exerciseHardwareBlockWrites(t, ctx, mem, addr, original) + restored = true + t.Logf("block writes: scratch=%#x aligned=%d unaligned=%d neighbors=unchanged restored=true", addr, aligned, unaligned) +} + +func exerciseHardwareBlockWrites(t *testing.T, ctx context.Context, mem *dap.MemAP, addr uint64, original []byte) (int, int) { + t.Helper() aligned := make([]byte, hardwareScratchSize) for i := range aligned { aligned[i] = byte(i*29 + 3) @@ -138,8 +145,7 @@ func TestWriteMEMAPBlocksOverFTDI(t *testing.T) { if err := writeAndVerifyHardwareBlock(ctx, mem, addr, original, original); err != nil { t.Fatal(err) } - restored = true - t.Logf("block writes: scratch=%#x aligned=%d unaligned=%d neighbors=unchanged restored=true", addr, len(aligned), len(unaligned)) + return len(aligned), len(unaligned) } func writeAndVerifyHardwareBlock(ctx context.Context, mem *dap.MemAP, addr uint64, data, want []byte) error { diff --git a/dap/memap_write_integration_test.go b/dap/memap_write_integration_test.go index 1e0d67d..d18c5fe 100644 --- a/dap/memap_write_integration_test.go +++ b/dap/memap_write_integration_test.go @@ -71,6 +71,19 @@ func TestWriteMEMAPScalarsOverFTDI(t *testing.T) { t.Fatal(err) } + size64 := exerciseHardwareScalarWrites(t, ctx, mem, addr, original) + if err := writeHardwareBytes(ctx, mem, addr, original); err != nil { + t.Fatal(err) + } + if err := assertHardwareBytes(ctx, mem, addr, original); err != nil { + t.Fatal(err) + } + restored = true + t.Logf("scalar writes: scratch=%#x bytes=%d sizes=8,16,32 size64=%s neighboring_lanes=unchanged restored=true", addr, hardwareScratchSize, size64) +} + +func exerciseHardwareScalarWrites(t *testing.T, ctx context.Context, mem *dap.MemAP, addr uint64, original []byte) string { + t.Helper() expected := slices.Clone(original) accesses := []struct { offset int @@ -108,14 +121,7 @@ func TestWriteMEMAPScalarsOverFTDI(t *testing.T) { } else if !strings.Contains(err.Error(), "CFG.LD") { t.Fatalf("Size64 write error = %v, want CFG.LD rejection", err) } - if err := writeHardwareBytes(ctx, mem, addr, original); err != nil { - t.Fatal(err) - } - if err := assertHardwareBytes(ctx, mem, addr, original); err != nil { - t.Fatal(err) - } - restored = true - t.Logf("scalar writes: scratch=%#x bytes=%d sizes=8,16,32 size64=%s neighboring_lanes=unchanged restored=true", addr, hardwareScratchSize, size64) + return size64 } func restoreHardwareScratch(ctx context.Context, dp *dap.DebugPort, mem **dap.MemAP, addr uint64, original []byte, expectedDPIDR, expectedAPIDR uint32) error { @@ -123,44 +129,27 @@ func restoreHardwareScratch(ctx context.Context, dp *dap.DebugPort, mem **dap.Me identityVerified := true for { if *mem != nil { - writeErr := writeHardwareBytes(ctx, *mem, addr, original) - verifyErr := error(nil) - if writeErr == nil { - verifyErr = assertHardwareBytes(ctx, *mem, addr, original) - } - if writeErr == nil && verifyErr == nil { + restored, err := restoreHardwareScratchAttempt(ctx, mem, addr, original) + if restored { return nil } - attemptErr = errors.Join(attemptErr, writeErr, verifyErr) - if err := releaseHardwareMemAPWithin(ctx, *mem); err != nil { - return errors.Join(attemptErr, err) + attemptErr = errors.Join(attemptErr, err) + if *mem != nil { + return attemptErr } - *mem = nil } if !identityVerified { - identity, err := dp.Connect(ctx) + retry, err := verifyHardwareScratchIdentity(ctx, dp, expectedDPIDR, expectedAPIDR) if err != nil { attemptErr = errors.Join(attemptErr, err) - if err := releaseHardwareDebugPortWithin(ctx, dp); err != nil { - return errors.Join(attemptErr, err) + if !retry { + return attemptErr } - continue - } - dpidr, ok := identity.DPIDR() - if !ok || dpidr.Raw != expectedDPIDR { - return errors.Join(attemptErr, fmt.Errorf("reconnected DPIDR = %#08x, want %#08x", dpidr.Raw, expectedDPIDR)) - } - apIdentity, err := dp.ReadAPIDR(ctx, hardwareAP) - if err != nil { - attemptErr = errors.Join(attemptErr, err) if err := releaseHardwareDebugPortWithin(ctx, dp); err != nil { return errors.Join(attemptErr, err) } continue } - if apIdentity.Raw != expectedAPIDR { - return errors.Join(attemptErr, fmt.Errorf("reconnected APIDR = %#08x, want %#08x", apIdentity.Raw, expectedAPIDR)) - } identityVerified = true } reopened, err := dap.OpenMemAP(ctx, dp, hardwareAP) @@ -179,6 +168,42 @@ func restoreHardwareScratch(ctx context.Context, dp *dap.DebugPort, mem **dap.Me } } +func restoreHardwareScratchAttempt(ctx context.Context, mem **dap.MemAP, addr uint64, original []byte) (bool, error) { + writeErr := writeHardwareBytes(ctx, *mem, addr, original) + var verifyErr error + if writeErr == nil { + verifyErr = assertHardwareBytes(ctx, *mem, addr, original) + } + if writeErr == nil && verifyErr == nil { + return true, nil + } + attemptErr := errors.Join(writeErr, verifyErr) + if err := releaseHardwareMemAPWithin(ctx, *mem); err != nil { + return false, errors.Join(attemptErr, err) + } + *mem = nil + return false, attemptErr +} + +func verifyHardwareScratchIdentity(ctx context.Context, dp *dap.DebugPort, expectedDPIDR, expectedAPIDR uint32) (bool, error) { + identity, err := dp.Connect(ctx) + if err != nil { + return true, err + } + dpidr, ok := identity.DPIDR() + if !ok || dpidr.Raw != expectedDPIDR { + return false, fmt.Errorf("reconnected DPIDR = %#08x, want %#08x", dpidr.Raw, expectedDPIDR) + } + apIdentity, err := dp.ReadAPIDR(ctx, hardwareAP) + if err != nil { + return true, err + } + if apIdentity.Raw != expectedAPIDR { + return false, fmt.Errorf("reconnected APIDR = %#08x, want %#08x", apIdentity.Raw, expectedAPIDR) + } + return false, nil +} + func readHardwareBytes(ctx context.Context, mem *dap.MemAP, addr uint64, size int) ([]byte, error) { data := make([]byte, size) for i := range data { diff --git a/jlink/session_integration_test.go b/jlink/session_integration_test.go index a8e8f74..be1be9d 100644 --- a/jlink/session_integration_test.go +++ b/jlink/session_integration_test.go @@ -142,28 +142,7 @@ func observeJLinkTarget(t *testing.T, ctx context.Context, readyOnOpen bool) tar if err != nil { t.Fatal(err) } - dpidr, ok := identity.DPIDR() - if !ok { - t.Fatal("SW-DP identity has no DPIDR") - } - transaction := debugPort.NewTxn() - dpidrResult := transaction.ReadDP(dap.DPIDR) - apidrResult := transaction.ReadAPIDR(dap.NewAPSel(0)) - if err := transaction.Commit(ctx); err != nil { - t.Fatal(err) - } - confirmedDPIDR, err := dpidrResult.Value() - if err != nil { - t.Fatal(err) - } - apidrRaw, err := apidrResult.Value() - if err != nil { - t.Fatal(err) - } - apidr := dap.DecodeAPIDR(apidrRaw) - if confirmedDPIDR != dpidr.Raw || apidr.Raw == 0 || apidr.Class != 8 { - t.Fatalf("debug identities = DPIDR %#08x/%#08x AP0 %+v", dpidr.Raw, confirmedDPIDR, apidr) - } + dpidr, apidr := confirmJLinkTargetIdentities(t, ctx, debugPort, identity) savedCSW, err := debugPort.ReadRawAP(ctx, dap.NewAPSel(0).Address(0x00)) if err != nil { t.Fatal(err) @@ -210,6 +189,33 @@ func observeJLinkTarget(t *testing.T, ctx context.Context, readyOnOpen bool) tar } } +func confirmJLinkTargetIdentities(t *testing.T, ctx context.Context, debugPort *dap.DebugPort, identity dap.Identity) (dap.DPIDRInfo, dap.APIDRInfo) { + t.Helper() + dpidr, ok := identity.DPIDR() + if !ok { + t.Fatal("SW-DP identity has no DPIDR") + } + transaction := debugPort.NewTxn() + dpidrResult := transaction.ReadDP(dap.DPIDR) + apidrResult := transaction.ReadAPIDR(dap.NewAPSel(0)) + if err := transaction.Commit(ctx); err != nil { + t.Fatal(err) + } + confirmedDPIDR, err := dpidrResult.Value() + if err != nil { + t.Fatal(err) + } + apidrRaw, err := apidrResult.Value() + if err != nil { + t.Fatal(err) + } + apidr := dap.DecodeAPIDR(apidrRaw) + if confirmedDPIDR != dpidr.Raw || apidr.Raw == 0 || apidr.Class != 8 { + t.Fatalf("debug identities = DPIDR %#08x/%#08x AP0 %+v", dpidr.Raw, confirmedDPIDR, apidr) + } + return dpidr, apidr +} + func assertAPRegister(t *testing.T, ctx context.Context, debugPort *dap.DebugPort, address uint8, want uint32) { t.Helper() got, err := debugPort.ReadRawAP(ctx, dap.NewAPSel(0).Address(uint16(address))) diff --git a/target/cortexm/register_integration_test.go b/target/cortexm/register_integration_test.go index 15d9934..526ec23 100644 --- a/target/cortexm/register_integration_test.go +++ b/target/cortexm/register_integration_test.go @@ -93,9 +93,10 @@ func checkRegisterFirmware(t *testing.T, ctx context.Context, memory *dap.MemAP) // Match the linked vectors and instructions before changing processor state. for addr := uint32(0); addr < 0xc0; addr += 4 { want := uint32(0xcd) - if addr == 0 { + switch addr { + case 0: want = 0x20004000 - } else if addr == 4 { + case 4: want = 0xc1 } got, err := memory.ReadWord(ctx, addr) From c8ee3e98200c5ca8395e4a1692753da10bb04d3b Mon Sep 17 00:00:00 2001 From: Jon Olson Date: Sun, 18 Oct 2026 17:22:17 -0700 Subject: [PATCH 4/5] Describe the review gate's actual trust boundary. The base checkout supplies the jq signal parser, while polling and acceptance remain in the pull-request workflow. Calling the whole gate a base evaluator implies protection against changes to the workflow itself. Name the parser in the workflow and its test, and document that workflow changes still require maintainer review. CODEOWNERS routes review but does not enforce approval or prevent a same-name check from passing. --- .github/scripts/publication-guards_test.py | 4 ++-- .github/workflows/test.yml | 2 +- CONTRIBUTING.md | 15 +++++++++++---- 3 files changed, 14 insertions(+), 7 deletions(-) diff --git a/.github/scripts/publication-guards_test.py b/.github/scripts/publication-guards_test.py index 28579b3..63a1c67 100644 --- a/.github/scripts/publication-guards_test.py +++ b/.github/scripts/publication-guards_test.py @@ -60,10 +60,10 @@ def test_invalid_commit_range_fails(self): result = self.run_step('Test every pull-request commit') self.assertNotEqual(result.returncode, 0, result.stdout + result.stderr) - def test_review_evaluator_uses_base_checkout(self): + def test_review_signal_parser_uses_base_checkout(self): workflow = (ROOT / '.github/workflows/test.yml').read_text() review = workflow.split(' codex-reviewed:', 1)[1] - checkout = review.split(' - name: Check out the review policy', 1)[1] + checkout = review.split(' - name: Check out the base signal parser', 1)[1] checkout = checkout.split(' - name: Wait for Codex', 1)[0] self.assertIn('ref: ${{ github.event.pull_request.base.sha }}', checkout) self.assertNotIn('github.event.pull_request.draft', review) diff --git a/.github/workflows/test.yml b/.github/workflows/test.yml index 74b557e..dc09259 100644 --- a/.github/workflows/test.yml +++ b/.github/workflows/test.yml @@ -238,7 +238,7 @@ jobs: HEAD_SHA: ${{ github.event.pull_request.head.sha }} PR_NUMBER: ${{ github.event.pull_request.number }} steps: - - name: Check out the review policy + - name: Check out the base signal parser uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7 with: persist-credentials: false diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 22457bf..3b7dd08 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -163,10 +163,17 @@ Changes to policy tooling, lint and format configuration, files under and are assigned to the maintainer in CODEOWNERS. The policy job builds its checker from the pull request's base revision and -checks the proposed head's commits and Markdown. The Codex gate also loads its -review evaluator from the base revision. It checks the pull request's current -head and draft state before accepting a review, including on reruns of older -workflow events. Failed or empty commit enumeration fails the per-commit job. +checks the proposed head's commits and Markdown. The Codex gate loads its jq +signal parser from the base revision; its polling and acceptance logic remains +in the pull-request workflow. It checks the pull request's current head and +draft state before accepting a review, including on reruns of older workflow +events. Failed or empty commit enumeration fails the per-commit job. + +The workflow runs the base versions of the checker and parser even when a pull +request changes those tools. It does not prevent a pull request from changing +the workflow itself or producing a check with the same name. Maintainers must +review workflow and review-policy changes before merging; CODEOWNERS identifies +the maintainer, but does not by itself enforce approval. Untrusted pull-request workflows receive no secrets or write-capable checkout credentials and never run HIL. CodeQL uses the ordinary `pull_request` event and From 849c4a3efbdbda69ba5ea9bd1ccb0df2bda3df70 Mon Sep 17 00:00:00 2001 From: Jon Olson Date: Sun, 18 Oct 2026 17:31:07 -0700 Subject: [PATCH 5/5] Recognize Codex review receipt comments. Clean Codex comments now start with a receipt marker, so the gate ignores their result even when the reviewed commit matches the current head. Strip the known marker before matching the existing clean-result prefix; retain the bot identity, commit resolution and current-head checks. Keep plain result comments supported and reject unknown marker versions or quoted receipts rather than treating arbitrary body text as a result. --- .github/scripts/publication-guards_test.py | 16 ++++++++++++++++ .github/workflows/test.yml | 4 +++- CONTRIBUTING.md | 16 ++++++++-------- 3 files changed, 27 insertions(+), 9 deletions(-) diff --git a/.github/scripts/publication-guards_test.py b/.github/scripts/publication-guards_test.py index 63a1c67..73eedfe 100644 --- a/.github/scripts/publication-guards_test.py +++ b/.github/scripts/publication-guards_test.py @@ -147,6 +147,18 @@ def test_transient_timeline_failure_retries(self): result = self.run_review('transient') self.assertEqual(result.returncode, 0, result.stdout + result.stderr) + def test_clean_receipt_comment_is_attributed(self): + result = self.run_review('receipt') + self.assertEqual(result.returncode, 0, result.stdout + result.stderr) + + def test_unknown_receipt_version_cannot_pass(self): + result = self.run_review('unknown-receipt') + self.assertNotEqual(result.returncode, 0, result.stdout + result.stderr) + + def test_quoted_receipt_cannot_pass(self): + result = self.run_review('quoted-receipt') + self.assertNotEqual(result.returncode, 0, result.stdout + result.stderr) + def test_fork_run_is_attributed(self): result = self.run_review('fork') self.assertEqual(result.returncode, 0, result.stdout + result.stderr) @@ -184,6 +196,10 @@ def test_wrong_fork_repository_cannot_pass(self): {'event': 'commented', 'user': {'login': actor}, 'body': "Codex Review: Didn't find any major issues.\n**Reviewed commit:** `" + head + '`'}, ] + if entries and 'receipt' in mode: + marker = '' if mode == 'unknown-receipt' else '' + prefix = 'Quoted review:\n' if mode == 'quoted-receipt' else '' + entries[-1]['body'] = prefix + marker + '\n\n' + entries[-1]['body'] print(json.dumps([entries])) elif '/comments?' in endpoint: rows = [] if 'fork' not in mode else [{ diff --git a/.github/workflows/test.yml b/.github/workflows/test.yml index dc09259..0984dfe 100644 --- a/.github/workflows/test.yml +++ b/.github/workflows/test.yml @@ -278,7 +278,9 @@ jobs: jq -r '.[] | select( .event == "commented" and .user.login == "chatgpt-codex-connector[bot]" - and (.body // "" | startswith("Codex Review: Didn\u0027t find any major issues.")) + and (.body // "" + | ltrimstr("\n\n") + | startswith("Codex Review: Didn\u0027t find any major issues.")) ) | try (.body | capture("\\*\\*Reviewed commit:\\*\\* `(?\u003cprefix\u003e[0-9a-f]{7,40})`").prefix) catch empty' \ <<< "$post_head_entries" } diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 3b7dd08..8e73440 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -149,14 +149,14 @@ in the Markdown source. On the final head, comment `@codex review`; a clean review may instead complete with a SHA-labeled Codex result comment. Automatic review begins when a pull request becomes ready, and the connector reacts to the pull request with a thumbs-up after a clean review. The gate accepts a -SHA-labeled clean result, or a completed Code Review row whose displayed commit -resolves to the exact head together with the connector's current thumbs-up. -After a head change, the thumbs-up must also be created after the earliest -GitHub Actions run for that pull request and head. A formal review with -suggestions does not satisfy the gate. A Codex opinion does not count as an -approval. Resolve its actionable conversations or explain the disposition before -merging. If Codex completes after the gate's polling window, rerun the failed -`codex-reviewed` job to evaluate the completed result. +SHA-labeled clean result, including Codex receipt comments, or a completed Code +Review row whose displayed commit resolves to the exact head together with the +connector's current thumbs-up. After a head change, the thumbs-up must also be +created after the earliest GitHub Actions run for that pull request and head. A +formal review with suggestions does not satisfy the gate. A Codex opinion does +not count as an approval. Resolve its actionable conversations or explain the +disposition before merging. If Codex completes after the gate's polling window, +rerun the failed `codex-reviewed` job to evaluate the completed result. Changes to policy tooling, lint and format configuration, files under `.github/`, and `AGENTS*.md` instruction files receive a review-policy warning