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/.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..73eedfe --- /dev/null +++ b/.github/scripts/publication-guards_test.py @@ -0,0 +1,227 @@ +#!/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_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 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) + + 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() + 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_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) + + 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 + '`'}, + ] + 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 [{ + '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..0984dfe 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 @@ -133,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: | @@ -149,7 +163,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}" @@ -186,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 @@ -198,7 +224,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 @@ -212,14 +238,23 @@ 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 - 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 \ @@ -243,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" } @@ -292,6 +329,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 +356,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 0f02326..8e73440 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 @@ -148,14 +149,31 @@ 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 +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 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 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/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{ 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)