diff --git a/.config/dotnet-tools.json b/.config/dotnet-tools.json index c657430..efeff64 100644 --- a/.config/dotnet-tools.json +++ b/.config/dotnet-tools.json @@ -3,7 +3,7 @@ "isRoot": true, "tools": { "csharpier": { - "version": "1.2.6", + "version": "1.3.0", "commands": [ "csharpier" ], diff --git a/.editorconfig b/.editorconfig index bfbe233..3ad5391 100644 --- a/.editorconfig +++ b/.editorconfig @@ -37,7 +37,7 @@ end_of_line = crlf indent_size = 2 # Json files -[*.json] +[*.{json,jsonc}] end_of_line = crlf # Linux scripts diff --git a/.github/copilot-instructions.md b/.github/copilot-instructions.md index 1007322..fd3bb0d 100644 --- a/.github/copilot-instructions.md +++ b/.github/copilot-instructions.md @@ -56,3 +56,126 @@ This repository builds and publishes Docker images for Network Optix VMS product - Follow the zero-warnings policy and formatting requirements in [CODESTYLE.md](../CODESTYLE.md). - Use explicit types (no `var`), Allman braces, file-scoped namespaces, and other conventions as defined in the master style guide. - Respect line endings and encoding rules from the repository configuration, including UTF-8 without BOM. + +## GitHub Copilot Review Runbook + +Use this section for provider-specific mechanics. The expected review loop *contract* (request review on every push, verify head-SHA coverage, triage findings, reply + resolve, escalate when stuck) is defined in [AGENTS.md -> PR Review Etiquette](../AGENTS.md#pr-review-etiquette). This section only describes how to make GitHub Copilot reliably execute it. + +### Triggering and Polling + +Auto-review on push is configured (via the branch ruleset's `copilot_code_review` rule with `review_on_push: true`) but fires inconsistently in practice - treat it as best-effort, not guaranteed. After every push, **re-request a review programmatically** via the GraphQL `requestReviews` mutation, passing the Copilot reviewer's bot node id in `botIds`. This now works reliably (it previously did not - a maintainer had to click "re-request review" in the UI; the agent can now drive the loop end-to-end without that hand-off). + +> **The reviewer login differs by API - this is intentional, not a typo.** In **GraphQL** (`gh api graphql` and `gh pr view --json reviews`, which is GraphQL-backed) the `Bot.login` is `copilot-pull-request-reviewer` - **no `[bot]` suffix**. In the **REST** API (`gh api repos/.../issues|pulls/...`) the same account's `user.login` is `copilot-pull-request-reviewer[bot]` - **with** the suffix. Each query below uses the correct form for its API; match the API, not a single spelling, when adapting them. + +```sh +# 1. PR node id + the Copilot reviewer's bot node id (read from any existing +# Copilot review; the reviewer login is `copilot-pull-request-reviewer`). +PR_NODE=$(gh pr view --json id --jq '.id') +BOT_ID=$(gh api graphql -f query=' +{ + repository(owner: "ptr727", name: "NxWitness") { + pullRequest(number: ) { + reviews(first: 50) { nodes { author { __typename login ... on Bot { id } } } } + } + } +}' --jq '[.data.repository.pullRequest.reviews.nodes[] + | select(.author.login == "copilot-pull-request-reviewer") + | .author.id] | first') + +# 2. Re-request a Copilot review on the current head. +gh api graphql -f query=' +mutation($pr: ID!, $bot: ID!) { + requestReviews(input: { pullRequestId: $pr, botIds: [$bot], union: true }) { + pullRequest { id } + } +}' -F pr="$PR_NODE" -F bot="$BOT_ID" +``` + +The bot node id is read from an existing Copilot review, so step 1 needs at least one prior review on the PR - the auto-review-on-open normally supplies the first one. If no Copilot review exists yet and auto-review didn't fire, request `Copilot` once through the GitHub PR UI to seed it, then use the mutation for every subsequent re-request. + +**Do NOT post `@Copilot review` as a PR comment.** That comment triggers the Copilot *coding agent* (`copilot-swe-agent[bot]`), which makes code changes rather than posting a review. + +Known non-working request paths (don't rely on them - use the `requestReviews` mutation above instead): + +- `POST /requested_reviewers` with `reviewers=[Copilot]` can return 200 but no-op. +- `copilot-pull-request-reviewer` as a requested reviewer slug returns 422. + +### Verify Review Covered Current Head + +Before merging, confirm Copilot reviewed the current PR head SHA. Copilot may respond as either a formal review (carries an exact commit SHA) or an issue comment (no SHA - use the most recent Copilot comment for manual confirmation). Check both. + +```sh +PR_HEAD=$(gh pr view --json headRefOid --jq '.headRefOid') + +# 1. Formal review - exact SHA match. +gh pr view --json reviews --jq \ + '.reviews[] | select(.author.login=="copilot-pull-request-reviewer") | .commit.oid' \ + | grep -q "$PR_HEAD" && echo "covered via formal review" + +# 2. Issue comment - show the most recent Copilot comment for manual +# confirmation. This is the REST API, so the login carries the `[bot]` suffix. +gh api repos/ptr727/NxWitness/issues//comments --jq \ + '[.[] | select(.user.login=="copilot-pull-request-reviewer[bot]")] | last | {created_at, body: .body[:200]}' +``` + +Coverage is confirmed when (1) exits 0. For issue comments (path 2), body content is the only reliable signal - `created_at` is not: `git log -1 --format=%cI` is the **commit** timestamp, not the push timestamp, so amended or rebased commits can have an earlier timestamp and an older Copilot comment could satisfy a time check even though Copilot never saw the current head. Treat path (2) as confirmed only when the comment body explicitly refers to the current changes. + +### Bounded Retry Workflow + +If a review did not run on the current head, retry: + +1. Wait briefly and check head-SHA coverage (see above). +1. Re-request the review via the `requestReviews` mutation (see "Triggering and Polling"); fall back to the GitHub PR UI only if the mutation no-ops. +1. Retry up to two more times (three total). +1. If still missing, mark review as blocked and escalate to the user/maintainer with what was attempted. + +### Reply and Thread Resolution Workflow + +List unresolved threads. Use `first: 100` with cursor-based pagination; if `hasNextPage` is true, re-run with `after: ""` to retrieve the next page: + +```sh +gh api graphql -f query=' +{ + repository(owner: "ptr727", name: "NxWitness") { + pullRequest(number: ) { + reviewThreads(first: 100) { + nodes { + id isResolved path + comments(first: 1) { nodes { author { login } body } } + } + pageInfo { hasNextPage endCursor } + } + } + } +}' | jq ' + .data.repository.pullRequest.reviewThreads | + (.pageInfo | "hasNextPage=\(.hasNextPage) endCursor=\(.endCursor)"), + (.nodes[] | select(.isResolved == false)) +' +``` + +Reply on a thread, then resolve it: + +```sh +gh api graphql -f query=' +mutation($threadId: ID!, $body: String!) { + addPullRequestReviewThreadReply(input: { pullRequestReviewThreadId: $threadId, body: $body }) { + comment { id } + } +}' -F threadId="PRRT_..." -F body="Fixed in : ." + +gh api graphql -f query=' +mutation($threadId: ID!) { + resolveReviewThread(input: { threadId: $threadId }) { thread { id isResolved } } +}' -F threadId="PRRT_..." +``` + +Issue-level Copilot comments (those in `issues//comments`) have no resolution action - GitHub provides no API or UI to resolve them. Reply if the finding warrants it; no resolution step is needed or possible. + +Reply-body conventions: + +- Accepted bug/style fix: include fixing commit SHA and a one-line summary. +- Declined style comment: cite the rule (AGENTS.md or language CODESTYLE) and the existing-tree precedent. +- Declined architecture proposal: one-sentence rationale. + +After the final push, sweep-resolve stale older threads for removed code paths. diff --git a/.github/workflows/build-docker-task.yml b/.github/workflows/build-docker-task.yml index 2a6f7be..6413f5f 100644 --- a/.github/workflows/build-docker-task.yml +++ b/.github/workflows/build-docker-task.yml @@ -15,17 +15,22 @@ on: required: false type: boolean default: false - # In smoke mode, restrict the build to this branch's rows (the PR - # base branch). Empty builds both branches' rows. Lets a PR onto - # develop smoke-test the develop images and a PR onto main the main - # images, without building the other branch. - smoke_branch: + # Logical branch whose Matrix.json rows to build/push and whose GHA + # cache scope to use. Decoupled from `ref` so `ref` can be pinned to an + # immutable commit (e.g. the publisher pins main to the versioned SHA) + # while this still selects the right branch's rows. Empty in smoke mode + # builds both branches' rows; the publisher passes main and develop so + # both branches' tags are produced from one scheduled run. Empty in a + # non-smoke build falls back to github.ref_name for the cache scope. + branch: required: false type: string default: '' - # Branch to check out and whose images to build/push. Empty uses the - # triggering ref (PR context). The publisher passes main and develop - # so both branches' tags are produced from one scheduled run. + # Immutable git ref to check out / version, decoupled from branch/tag + # selection (see `branch`). Empty uses the triggering ref (PR context). + # The publisher pins this to the exact versioned commit so the image's + # embedded version matches the GitHub release tag even if the branch + # advances mid-run. ref: required: false type: string @@ -57,27 +62,26 @@ jobs: id: getmatrix env: SMOKE: ${{ inputs.smoke }} - SMOKE_BRANCH: ${{ inputs.smoke_branch }} - REF: ${{ inputs.ref }} + BRANCH: ${{ inputs.branch }} run: | - # $ref and $sb below are jq variables (passed via --arg), not shell - # variables, so they must stay single-quoted / unexpanded. + # $b below is a jq variable (passed via --arg), not a shell + # variable, so it must stay single-quoted / unexpanded. # shellcheck disable=SC2016 if [[ "$SMOKE" == "true" ]]; then # One Ubuntu + one LSIO variant exercises the shared Dockerfile # build logic and the branch's build args; tags are irrelevant - # since smoke never pushes. Restrict to the PR base branch ($sb) + # since smoke never pushes. Restrict to the PR base branch ($b) # so a PR onto develop validates the develop rows and a PR onto - # main validates the main rows; empty $sb builds both branches. - FILTER='.Images |= (map(select((.Name == "NxMeta" or .Name == "NxMeta-LSIO") and ($sb == "" or .Branch == $sb))) | unique_by([.Name, .Branch]))' - elif [[ -n "$REF" ]]; then + # main validates the main rows; empty $b builds both branches. + FILTER='.Images |= (map(select((.Name == "NxMeta" or .Name == "NxMeta-LSIO") and ($b == "" or .Branch == $b))) | unique_by([.Name, .Branch]))' + elif [[ -n "$BRANCH" ]]; then # Publish: build only the rows targeting the branch being built # (avoids building the other branch's rows just to discard them). - FILTER='.Images |= map(select(.Branch == $ref))' + FILTER='.Images |= map(select(.Branch == $b))' else FILTER='.' fi - echo "matrix=$(jq --arg ref "$REF" --arg sb "$SMOKE_BRANCH" --compact-output "$FILTER" ./Make/Matrix.json)" >> "$GITHUB_OUTPUT" + echo "matrix=$(jq --arg b "$BRANCH" --compact-output "$FILTER" ./Make/Matrix.json)" >> "$GITHUB_OUTPUT" get-version: name: Get version information job @@ -172,5 +176,5 @@ jobs: type=gha,scope=main-${{ matrix.images.Name }} ${{ github.event.pull_request && format('type=gha,scope=pr-{0}-{1}', github.event.pull_request.number, matrix.images.Name) || '' }} cache-to: | - ${{ inputs.push && format('type=gha,mode=min,scope={0}-{1},ignore-error=true', inputs.ref != '' && inputs.ref || github.ref_name, matrix.images.Name) || '' }} + ${{ inputs.push && format('type=gha,mode=min,scope={0}-{1},ignore-error=true', inputs.branch != '' && inputs.branch || github.ref_name, matrix.images.Name) || '' }} ${{ github.event.pull_request && !github.event.pull_request.head.repo.fork && format('type=gha,mode=min,scope=pr-{0}-{1},ignore-error=true', github.event.pull_request.number, matrix.images.Name) || '' }} diff --git a/.github/workflows/publish-release.yml b/.github/workflows/publish-release.yml index 4468bcc..22d8b3e 100644 --- a/.github/workflows/publish-release.yml +++ b/.github/workflows/publish-release.yml @@ -47,12 +47,17 @@ jobs: build-main: name: Build main images job - needs: [build-base] + needs: [get-version, build-base] uses: ./.github/workflows/build-docker-task.yml secrets: inherit with: push: true - ref: main + branch: main + # Pin to the exact commit get-version computed the release version from, + # not the moving `main` ref: this closes the race where a commit landing + # on main mid-run could make the pushed image's embedded version come + # from a newer commit than the GitHub release of the same SemVer2 tag. + ref: ${{ needs.get-version.outputs.GitCommitId }} build_base: false build-develop: @@ -62,6 +67,10 @@ jobs: secrets: inherit with: push: true + # Develop only publishes the mutable :develop tag (no versioned GitHub + # release), so the moving ref can't cause a tag/release mismatch; no + # commit pin needed. + branch: develop ref: develop build_base: false diff --git a/.github/workflows/test-pull-request.yml b/.github/workflows/test-pull-request.yml index 507f72a..8c6fec9 100644 --- a/.github/workflows/test-pull-request.yml +++ b/.github/workflows/test-pull-request.yml @@ -53,8 +53,9 @@ jobs: # scheduled publish-release.yml, so a smoke pass is fast PR feedback, # not a publish gate. # - # smoke_branch targets the PR base branch so a PR onto develop validates - # the develop image rows and a PR onto main validates the main rows. + # branch targets the PR base branch so a PR onto develop validates the + # develop image rows and a PR onto main validates the main rows. ref is + # left unset so checkout uses the PR ref being tested. # # build_base is only enabled when a base Dockerfile changed: the product # smoke build pulls the published base from Docker Hub, so building the @@ -62,14 +63,18 @@ jobs: # it to validate it still compiles (e.g. a Dependabot ubuntu:noble bump). smoke-build: name: Smoke build job - needs: [changes] + # Also gate on test-release: the smoke build builds Docker images, so don't + # spend it when the test job is already failing. A failed test-release + # leaves this job skipped (needs unsatisfied) and the aggregator blocks on + # the test-release failure directly. + needs: [changes, test-release] if: ${{ needs.changes.outputs.image == 'true' }} uses: ./.github/workflows/build-docker-task.yml secrets: inherit with: push: false smoke: true - smoke_branch: ${{ github.base_ref }} + branch: ${{ github.base_ref }} build_base: ${{ needs.changes.outputs.base == 'true' }} # TODO: Workaround for GitHub Actions not supporting status checks on conditional jobs diff --git a/.markdownlint-cli2.jsonc b/.markdownlint-cli2.jsonc new file mode 100644 index 0000000..c6a5714 --- /dev/null +++ b/.markdownlint-cli2.jsonc @@ -0,0 +1,14 @@ +{ + "config": { + // Prose paragraphs and data-heavy tables/URLs are intentionally long; + // reflowing at 80 cols hurts readability and churns diffs. + "MD013": false, + // Inline HTML is used for reference-link section dividers. + "MD033": false, + // Require fenced code blocks over the legacy 4-space-indented style. + "MD046": { "style": "fenced" }, + // Wide tables are intentional where wrapping cells breaks GitHub rendering. + "MD060": false + }, + "gitignore": true +} diff --git a/AGENTS.md b/AGENTS.md index 87ba12f..24129a5 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -59,14 +59,49 @@ For comprehensive coding and formatting standards, follow: - Merges to `main`/`develop` do not build or publish images. Auto-merged Dependabot and codegen PRs simply land commits that the next scheduled publish picks up. Do not reintroduce push-triggered publishing or full-matrix PR builds. - Lint workflow edits before pushing (see [Workspace and linting](#workspace-and-linting)); there is no CI lint job. -## Pull Request Review Process - -- Open PRs against `develop` (the integration branch); `develop` is forward-only and ships to `main` via release merges. -- The repo is configured to automatically request a GitHub Copilot review when a PR is opened. Respond to every Copilot comment: either address it with a change, or justify why it does not apply. Either way, reply on the comment stating what you did, then resolve (close) the comment. -- After you push a new commit, a Copilot re-review does not reliably fire on its own. In practice the re-review can be requested via the GraphQL `requestReviews` mutation, passing the Copilot bot's node id in `botIds`: - - Get the bot id once from the existing review author: `pullRequest { reviews(first:1){ nodes { author { ... on Bot { id } } } } }` (the Copilot reviewer login is `copilot-pull-request-reviewer`). - - Trigger: `mutation { requestReviews(input: {pullRequestId: "", botIds: [""], union: true}) { pullRequest { id } } }`. - - This is observed-but-not-guaranteed; if a re-review still does not appear, ask the maintainer to start one in the GitHub UI. Repeat until both Copilot and the author are satisfied. +## PR Review Etiquette + +The repo runs a review loop on every PR: local agent iteration plus remote automated review (GitHub Copilot is the configured reviewer). Treat this as a contract regardless of which local agent authored the changes. + +### Expected Review Loop + +1. Push changes to the PR branch. +2. Re-request a review for the **current head SHA**. Auto-trigger is unreliable, so request it explicitly via the `requestReviews` GraphQL mutation (now reliable end-to-end - see the runbook); the UI is only a fallback. +3. Wait for review activity on that head. +4. Triage findings. +5. Apply fixes or write a rationale for declines. +6. Reply to each thread and resolve what was addressed. +7. Re-run the loop after every fix push until no actionable findings remain. + +`mergeStateStatus: CLEAN` only checks required statuses; it does not block on bot review comments. Drive the loop to green - review confirmed on the latest head SHA and every actionable finding closed - and then **wait for the maintainer's explicit permission to merge**. The agent does not merge on its own (consistent with "default to staging"; merging is maintainer-authorized). + +For provider-specific mechanics (how to request review, query review state, post replies, resolve threads), see the **GitHub Copilot Review Runbook** in [.github/copilot-instructions.md](./.github/copilot-instructions.md). This file owns the contract; that file owns the mechanics. + +### Triaging Review Comments + +For each comment, classify before responding: + +- **Bug** - wrong behavior, missing test coverage, or a real divergence between code and docs. Fix it. Reply with the fixing commit SHA when done. +- **Style/convention** - the comment cites a rule from this file or a language-specific style guide. Two cases: + - The cited rule matches what the existing codebase already does -> fix the offending code. + - The cited rule contradicts what's in the tree, or industry norm -> **update the rule instead of the code**. The rule is wrong, not the code. Bouncing the same code across rounds is the symptom of a wrong rule. Heuristic: three rounds on the same style category means the rule needs adjusting and the user should authorize the rule change. +- **Architectural opinion** - the comment proposes a different design ("constrain this to disabled-by-default", "move it elsewhere", "add a runtime guardrail"). This is judgment, not a bug. Surface it to the user with a recommendation; don't apply unilaterally. + +### Responding and Resolution Expectations + +Reply inline with either the fixing commit SHA (for accepted issues) or a concise rationale (for declines). Resolve review threads when addressed or intentionally declined with rationale. Issue-level comments (those at `repos/.../issues//comments` rather than tied to a specific line) have no resolution action - acknowledge with a reply if needed and move on. + +After the final push on a PR, sweep older threads from earlier rounds whose code paths no longer exist; otherwise stale unresolved markers remain in the review UI. + +### Escalating to the User + +Bring the user in when: + +- **Genuine design trade-off** surfaces (fail-open vs fail-closed, narrow vs broad refactor scope, "should we add a guardrail or trust the docstring"). Triage, recommend, ask. +- **Repeated friction** across rounds without convergence - that's the rule-needs-updating signal. Stop, summarize the pattern, and let the user authorize the rule change. +- **Architectural redesign** is requested rather than a bug fix. Surface with a recommendation; never apply unilaterally. + +Anti-pattern: don't keep flipping the code on the same style point. Flip the rule once and stick to the rule. ## Coding Conventions (Highlights)