From 8deecfddc319fb4784f57cb0c2cbcc961bf34b51 Mon Sep 17 00:00:00 2001 From: Jose Celano Date: Tue, 21 Jul 2026 11:34:39 +0100 Subject: [PATCH 01/11] fix(build): remove fat LTO from development profile --- Cargo.toml | 1 - Containerfile | 1 - .../1875-review-lto-fat-in-dev-profile.md | 141 ------------------ .../ISSUE.md | 134 +++++++++++++++++ .../research.md | 74 +++++++++ 5 files changed, 208 insertions(+), 143 deletions(-) delete mode 100644 docs/issues/open/1875-review-lto-fat-in-dev-profile.md create mode 100644 docs/issues/open/1875-review-lto-fat-in-dev-profile/ISSUE.md create mode 100644 docs/issues/open/1875-review-lto-fat-in-dev-profile/research.md diff --git a/Cargo.toml b/Cargo.toml index e19d1c8c7..c9252ef8d 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -118,7 +118,6 @@ suspicious = { level = "deny", priority = -1 } [profile.dev] debug = 1 -lto = "fat" opt-level = 1 [profile.release] diff --git a/Containerfile b/Containerfile index d335807e0..d9ac05d24 100644 --- a/Containerfile +++ b/Containerfile @@ -179,7 +179,6 @@ RUN mkdir -p \ packages/rest-api-application/src/lib.rs \ packages/rest-api-protocol/src/lib.rs \ packages/rest-api-runtime-adapter/src/lib.rs \ - packages/swarm-coordination-registry/src/lib.rs \ packages/test-helpers/src/lib.rs \ packages/torrent-repository-benchmarking/src/lib.rs \ diff --git a/docs/issues/open/1875-review-lto-fat-in-dev-profile.md b/docs/issues/open/1875-review-lto-fat-in-dev-profile.md deleted file mode 100644 index f03aa78d4..000000000 --- a/docs/issues/open/1875-review-lto-fat-in-dev-profile.md +++ /dev/null @@ -1,141 +0,0 @@ ---- -doc-type: issue -issue-type: task -status: planned -priority: p2 -github-issue: 1875 -spec-path: docs/issues/open/1875-review-lto-fat-in-dev-profile.md -branch: "1875-review-lto-fat-in-dev-profile" -related-pr: null -last-updated-utc: 2026-06-03 00:00 -semantic-links: - skill-links: - - create-issue - - create-adr - related-artifacts: - - Cargo.toml - - docs/adrs/ - - docs/skills/semantic-skill-link-convention.md ---- - - -# Issue #1875 - Review and fix `lto = "fat"` in `[profile.dev]` - -## Goal - -Determine whether `lto = "fat"` in `[profile.dev]` is still necessary, and remove or replace it with an appropriate setting that does not unnecessarily slow down development builds. - -## Background - -Commit `3c715fbb` (fix: [#898] docker build error: failed to load bitcode of module criterion) changed `lto = "thin"` to `lto = "fat"` in `[profile.dev]` as a workaround for an LLVM bitcode compatibility error that occurred when building benchmarks (criterion) inside a Docker container with Rust 1.79/1.81-nightly (mid-2024): - -```text -error: failed to load bitcode of module "criterion-...": failed to load bitcode -``` - -The root cause was an LLVM cross-module bitcode version mismatch triggered by `lto = "thin"` when mixing crates compiled with different LLVM/rustc versions inside a container build. Switching to `"fat"` forced all bitcode into a single monolithic unit, eliminating the cross-module issue. - -This was a legitimate workaround at the time but carries a significant cost: `lto = "fat"` in `[profile.dev]` applies full-program LTO to every incremental development build, substantially increasing compile times with no benefit for day-to-day development iteration. - -The project now targets MSRV 1.88 (as of 2026-06). The LLVM version bundled with Rust 1.88 is well past the version where this bug was observed, and the `Containerfile` now builds with the stable toolchain. The original triggering conditions may no longer exist. - -## Scope - -### In Scope - -- Investigate whether removing `lto = "fat"` from `[profile.dev]` still causes the Docker build to fail with current Rust/LLVM versions -- If the bug is gone: remove `lto = "fat"` from `[profile.dev]` (restore `lto = "thin"` or remove the key to use the Cargo default of `false`) -- If the bug persists: document exactly why, pin the minimum fix to the narrowest possible scope (e.g. only the benchmark crate, only the release profile, or via a per-crate override), and open a follow-up tracking upstream resolution -- Keep `lto = "fat"` in `[profile.release]` — it is appropriate there for production binary optimization - -### Out of Scope - -- Changing `[profile.release]` LTO settings -- Restructuring the Containerfile beyond what is required to verify the fix - -## Implementation Plan - -Status values: `TODO`, `IN_PROGRESS`, `BLOCKED`, `DONE`. - -| ID | Status | Task | Notes / Expected Output | -| --- | ------ | ----------------------------------------------------------------------------------- | ------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | -| T1 | TODO | Reproduce the original failure (optional, low priority) | Confirm what Rust/LLVM version combination triggers the bitcode error, if reproducible at all | -| T2 | TODO | Remove `lto = "fat"` from `[profile.dev]` (or restore `lto = "thin"`) | `Cargo.toml` `[profile.dev]` no longer carries fat LTO | -| T2a | TODO | If the final LTO choice is non-obvious, create an ADR and link it from `Cargo.toml` | ADR created in `docs/adrs/` (see `.github/skills/dev/planning/create-adr/SKILL.md`). A `# adr-link: ` comment added to `Cargo.toml` near `[profile.dev]` following the semantic-link convention in `docs/skills/semantic-skill-link-convention.md`. Skip if the change is straightforward (e.g. removing an obsolete workaround). | -| T3 | TODO | Run the full local test suite with the updated dev profile | `cargo test --tests --benches --examples --workspace --all-targets --all-features` exits with code 0 | -| T4 | TODO | Run the Docker build with the updated dev profile to verify no bitcode error | `docker build --target release ...` completes without `failed to load bitcode` error | -| T5 | TODO | If T4 fails: scope the workaround narrowly and document the upstream tracking issue | Narrowest fix applied; comment in `Cargo.toml` explains why with a link | -| T6 | TODO | Run pre-commit checks | `./contrib/dev-tools/git/hooks/pre-commit.sh` exits with code 0 | - -## Progress Tracking - -### Workflow Checkpoints - -- [ ] Spec drafted in `docs/issues/drafts/` -- [ ] Spec reviewed and approved by user/maintainer -- [ ] GitHub issue created and issue number added to this spec -- [ ] Implementation completed -- [ ] Automatic verification completed (`linter all`, relevant tests, and any pre-push checks) -- [ ] Manual verification scenarios executed and recorded (status + evidence) -- [ ] Acceptance criteria reviewed after implementation and updated with evidence -- [ ] Reviewer validated acceptance criteria and updated checkboxes -- [ ] Committer verified spec progress is up to date before commit -- [ ] Issue closed and spec moved from `docs/issues/open/` to `docs/issues/closed/` - -### Progress Log - -- 2026-06-03 00:00 UTC - GitHub Copilot - Spec drafted after investigating git history for `lto = "fat"` in `[profile.dev]`; root cause traced to commit `3c715fbb` - -## Acceptance Criteria - -- [ ] AC1: `[profile.dev]` in `Cargo.toml` does not use `lto = "fat"` (unless the Docker build failure is confirmed to still require it, in which case a comment linking to a tracking issue is present) -- [ ] AC1a: If the final LTO choice constitutes a non-obvious design decision, an ADR exists in `docs/adrs/` documenting the choice and rationale, and `Cargo.toml` carries a `# adr-link: ` comment near `[profile.dev]` following `docs/skills/semantic-skill-link-convention.md` -- [ ] AC2: `cargo test --tests --benches --examples --workspace --all-targets --all-features` exits with code 0 -- [ ] AC3: Docker build (`docker build --target release`) completes without a `failed to load bitcode` error -- [ ] AC4: `linter all` exits with code 0 -- [ ] AC5: Manual verification scenarios are executed and documented (status + evidence) -- [ ] AC6: Acceptance criteria are re-reviewed after implementation and reflect actual behavior - -## Verification Plan - -### Automatic Checks - -- `linter all` -- `cargo test --tests --benches --examples --workspace --all-targets --all-features` -- `./contrib/dev-tools/git/hooks/pre-commit.sh` - -### Manual Verification Scenarios - -Status values: `TODO`, `IN_PROGRESS`, `DONE`, `FAILED`, `BLOCKED`. - -| ID | Scenario | Command/Steps | Expected Result | Status | Evidence | -| --- | ---------------------------------------------------- | ------------------------------------------------------------------------------------ | -------------------------------------------------- | ------ | -------- | -| M1 | Local test suite passes without fat LTO in dev | `cargo test --tests --benches --examples --workspace --all-targets --all-features` | All tests pass, no bitcode errors | TODO | | -| M2 | Docker release build succeeds without fat LTO in dev | `docker build --target release --tag torrust-tracker:release --file Containerfile .` | Build completes; no `failed to load bitcode` error | TODO | | - -Notes: - -- M2 is the key regression guard for the original bug fix. -- If M2 fails, T5 applies: scope the workaround narrowly and document why. - -### Acceptance Verification - -| AC ID | Status (`TODO`/`DONE`) | Evidence | -| ----- | ---------------------- | -------- | -| AC1 | TODO | | -| AC2 | TODO | | -| AC3 | TODO | | -| AC4 | TODO | | -| AC5 | TODO | | -| AC6 | TODO | | - -## Risks and Trade-offs - -- If the bitcode LLVM bug is still present in some container environments, removing `lto = "fat"` from `[profile.dev]` could break Docker CI builds. Mitigation: verify M2 before merging; scope any required workaround to the narrowest target (e.g. a per-crate `[profile.dev.package.criterion]` override or a Containerfile-level `CARGO_PROFILE_DEV_LTO` env var). -- `lto = "fat"` in `[profile.dev]` has been present since mid-2024; removing it will improve local incremental build times noticeably for all contributors. - -## References - -- Commit `3c715fbb` — original workaround: "fix: [#898] docker build error: failed to load bitcode of module criterion" -- [Cargo reference — profiles](https://doc.rust-lang.org/cargo/reference/profiles.html#lto) -- [Rust issue tracker — LTO bitcode compatibility](https://github.com/rust-lang/rust/issues) (search "failed to load bitcode") diff --git a/docs/issues/open/1875-review-lto-fat-in-dev-profile/ISSUE.md b/docs/issues/open/1875-review-lto-fat-in-dev-profile/ISSUE.md new file mode 100644 index 000000000..4613a8cf8 --- /dev/null +++ b/docs/issues/open/1875-review-lto-fat-in-dev-profile/ISSUE.md @@ -0,0 +1,134 @@ +--- +doc-type: issue +issue-type: task +status: in-review +priority: p2 +github-issue: 1875 +spec-path: docs/issues/open/1875-review-lto-fat-in-dev-profile/ISSUE.md +branch: "1875-review-lto-fat-in-dev-profile" +related-pr: null +last-updated-utc: 2026-07-21 10:18 +semantic-links: + skill-links: + - create-issue + related-artifacts: + - Cargo.toml + - Containerfile + - docs/issues/open/1875-review-lto-fat-in-dev-profile/research.md + - docs/skills/semantic-skill-link-convention.md +--- + +# Issue #1875 - Review and fix `lto = "fat"` in `[profile.dev]` + +## Goal + +Optimize development builds for compilation speed and production builds for execution speed. +Remove the obsolete `lto = "fat"` setting from `[profile.dev]`, allowing Cargo's development-profile default (`lto = false`) to apply. Keep `lto = "fat"` in `[profile.release]` for production binary optimization. + +## Background + +Commit `3c715fbb` changed `[profile.dev]` from `lto = "thin"` to `lto = "fat"` as a workaround for a `failed to load bitcode` error involving Criterion in a Docker build with Rust 1.79/1.81-nightly in mid-2024. + +The investigation is recorded in [research.md](research.md). It found an important discrepancy: the recorded failing command used `--release`, which selects `[profile.release]`; changing `[profile.dev]` could not have directly affected that invocation. The release profile already used fat LTO before the workaround. Therefore, this issue removes the unsupported development-profile workaround while retaining the independently appropriate release setting. + +## Scope + +### In Scope + +- Remove `lto = "fat"` from `[profile.dev]` in `Cargo.toml`. +- Preserve `lto = "fat"` in `[profile.release]`. +- Verify development-profile tests and the Docker debug image build. +- Verify the Docker release image build continues to succeed. +- Record evidence in this issue spec and its research document. + +### Out of Scope + +- Changing `[profile.release]` LTO settings. +- Introducing a non-default development LTO setting such as `"thin"` or `"off"`. +- Restructuring the `Containerfile` beyond what is necessary to verify the change. +- Reproducing the historic Rust 1.79/1.81-nightly failure. + +## Implementation Plan + +Status values: `TODO`, `IN_PROGRESS`, `BLOCKED`, `DONE`. + +| ID | Status | Task | Notes / Expected Output | +| --- | ------ | ------------------------------------------------------- | ------------------------------------------------------------------------------------------------------------------------------------------------ | +| T1 | DONE | Collect user decision and research current LTO behavior | User selected Cargo's default development LTO setting. Findings are in [research.md](research.md). | +| T2 | DONE | Remove `lto = "fat"` from `[profile.dev]` | Removed the key; Cargo uses its default `lto = false` development-profile behavior. | +| T3 | DONE | Run the full local test suite | Passed: `cargo test --tests --benches --examples --workspace --all-targets --all-features`. | +| T4 | DONE | Build the Docker debug image | Passed: `docker build --target debug --tag torrust-tracker:debug --file Containerfile .` completed in 120.9 seconds without a bitcode error. | +| T5 | DONE | Build the Docker release image | Passed: `docker build --target release --tag torrust-tracker:release --file Containerfile .` completed in 214.4 seconds without a bitcode error. | +| T6 | DONE | Run pre-commit checks | Passed: `TORRUST_GIT_HOOKS_LOG_DIR=.tmp ./contrib/dev-tools/git/hooks/pre-commit.sh` exited 0. | + +## Progress Tracking + +### Workflow Checkpoints + +- [x] Spec drafted in `docs/issues/drafts/` +- [x] Spec reviewed and approved by user/maintainer +- [x] GitHub issue created and issue number added to this spec +- [x] Local implementation branch created +- [x] Implementation completed +- [x] Automatic verification completed (`linter all`, relevant tests, and any pre-push checks) +- [x] Manual verification scenarios executed and recorded (status + evidence) +- [x] Acceptance criteria reviewed after implementation and updated with evidence +- [ ] Reviewer validated acceptance criteria and updated checkboxes +- [x] Committer verified spec progress is up to date before commit +- [ ] Issue closed and spec moved from `docs/issues/open/` to `docs/issues/closed/` + +### Progress Log + +- 2026-06-03 00:00 UTC - GitHub Copilot - Spec drafted after investigating git history for `lto = "fat"` in `[profile.dev]`; root cause traced to commit `3c715fbb`. +- 2026-07-21 10:18 UTC - User - Confirmed the policy: prioritize development compilation speed and production execution speed. Approved the folder format with `ISSUE.md` as the normal-issue specification file. +- 2026-07-21 10:18 UTC - GitHub Copilot - Created branch `1875-review-lto-fat-in-dev-profile`, converted the specification to folder format, and recorded research findings. +- 2026-07-21 10:18 UTC - GitHub Copilot - Removed development-profile fat LTO. The full local test suite, Docker debug and release image builds, and pre-commit checks all passed. +- 2026-07-21 10:18 UTC - GitHub Copilot - Removed the empty continued line in `Containerfile` that produced Docker's `NoEmptyContinuation` warning. `docker build --target recipe --file Containerfile .` passed without the warning. + +## Acceptance Criteria + +- [x] AC1: `[profile.dev]` in `Cargo.toml` has no explicit `lto` setting and therefore uses Cargo's default `lto = false` behavior. +- [x] AC2: `[profile.release]` retains `lto = "fat"`. +- [x] AC3: `cargo test --tests --benches --examples --workspace --all-targets --all-features` exits with code 0. +- [x] AC4: Docker debug build (`docker build --target debug`) completes without a `failed to load bitcode` error. +- [x] AC5: Docker release build (`docker build --target release`) completes without a `failed to load bitcode` error. +- [x] AC6: `linter all` exits with code 0. +- [x] AC7: Manual verification scenarios are executed and documented (status + evidence). +- [x] AC8: Acceptance criteria are re-reviewed after implementation and reflect actual behavior. + +## Verification Plan + +### Automatic Checks + +- `linter all` +- `cargo test --tests --benches --examples --workspace --all-targets --all-features` +- `./contrib/dev-tools/git/hooks/pre-commit.sh` + +### Manual Verification Scenarios + +Status values: `TODO`, `IN_PROGRESS`, `DONE`, `FAILED`, `BLOCKED`. + +| ID | Scenario | Command/Steps | Expected Result | Status | Evidence | +| --- | ------------------------------- | ------------------------------------------------------------------------------------ | -------------------------------------- | ------ | -------------------------------------------------------------------------------------------- | +| M1 | Local development-profile tests | `cargo test --tests --benches --examples --workspace --all-targets --all-features` | All tests pass with no bitcode error. | DONE | Passed. | +| M2 | Docker debug image | `docker build --target debug --tag torrust-tracker:debug --file Containerfile .` | Build completes with no bitcode error. | DONE | Passed in 120.9 seconds. The unrelated `NoEmptyContinuation` warning was subsequently fixed. | +| M3 | Docker release image | `docker build --target release --tag torrust-tracker:release --file Containerfile .` | Build completes with no bitcode error. | DONE | Passed in 214.4 seconds. The unrelated `NoEmptyContinuation` warning was subsequently fixed. | + +## Acceptance Verification + +| AC ID | Status (`TODO`/`DONE`) | Evidence | +| ----- | ---------------------- | ------------------------------------------------------------------------------------ | +| AC1 | DONE | `[profile.dev]` contains only `debug = 1` and `opt-level = 1`; no `lto` key remains. | +| AC2 | DONE | `[profile.release]` still contains `lto = "fat"`. | +| AC3 | DONE | Full command passed. | +| AC4 | DONE | Docker debug image build passed. | +| AC5 | DONE | Docker release image build passed. | +| AC6 | DONE | Pre-commit's `linter all` step passed. | +| AC7 | DONE | M1 through M3 passed and are recorded above. | +| AC8 | DONE | This table was reviewed and updated after all verification completed. | + +## References + +- Commit `3c715fbb` — original workaround: "fix: [#898] docker build error: failed to load bitcode of module criterion" +- [Cargo reference — profiles](https://doc.rust-lang.org/cargo/reference/profiles.html#lto) +- [Rustc codegen option — LTO](https://doc.rust-lang.org/rustc/codegen-options/index.html#lto) diff --git a/docs/issues/open/1875-review-lto-fat-in-dev-profile/research.md b/docs/issues/open/1875-review-lto-fat-in-dev-profile/research.md new file mode 100644 index 000000000..1f183b8fd --- /dev/null +++ b/docs/issues/open/1875-review-lto-fat-in-dev-profile/research.md @@ -0,0 +1,74 @@ +--- +semantic-links: + related-artifacts: + - docs/issues/open/1875-review-lto-fat-in-dev-profile/ISSUE.md + - Cargo.toml + - Containerfile + - commit 3c715fbb +--- + +# Research: Development-profile LTO + +## Question + +Should the tracker retain `lto = "fat"` in `[profile.dev]`? + +## Decision + +No. Remove the explicit development-profile LTO setting and use Cargo's default `lto = false` behavior. This follows the maintainer-approved policy: + +1. Optimize development builds for compilation speed. +2. Optimize production builds for execution speed. + +`[profile.release]` continues to use `lto = "fat"` with `opt-level = 3` because it produces the production artifact. + +## Evidence + +### Cargo and rustc documentation + +Cargo documents the default development profile as `opt-level = 0`, `incremental = true`, `codegen-units = 256`, and `lto = false`. This profile is intended for normal development and debugging. The project overrides `opt-level` to `1`, but the default LTO setting remains appropriate for fast iteration. + +Cargo documents `lto = "fat"` as whole-program LTO across the dependency graph, and `lto = "thin"` as a less expensive alternative. Both make linking slower in exchange for better optimized code. The rustc documentation likewise describes fat LTO as whole-program analysis at the cost of longer linking time. + +Cargo further documents that `lto = false` permits thin local LTO across a crate's codegen units, while `lto = "off"` fully disables LTO. Removing the key restores Cargo's documented default rather than selecting a non-standard, project-specific optimization policy. + +Sources: + +- [Cargo profiles: LTO](https://doc.rust-lang.org/cargo/reference/profiles.html#lto) +- [Cargo profiles: default development profile](https://doc.rust-lang.org/cargo/reference/profiles.html#dev) +- [Rustc codegen options: LTO](https://doc.rust-lang.org/rustc/codegen-options/index.html#lto) + +### Historic workaround analysis + +Commit `3c715fbb` on 2024-06-17 changed `[profile.dev]` from `lto = "thin"` to `lto = "fat"`. Its commit message records a failure while running: + +```text +docker build --target release --tag torrust-tracker:release --file Containerfile . +``` + +The failure occurred in a release invocation using Rust 1.79 stable in a container, while the host default was Rust 1.81 nightly. The error reported an invalid LLVM bitcode producer/reader value for Criterion. + +However, `--target release` reaches the `release` Docker target, whose build stages pass `--release` to Cargo. Cargo's `--release` selects `[profile.release]`, not `[profile.dev]`. At that parent revision, `[profile.release]` already used `lto = "fat"`. Thus, the documented failing command cannot have been directly corrected by changing `[profile.dev]`; the causal connection is not supported by the retained evidence. + +The current `Containerfile` retains separate debug and release pipelines. The debug pipeline has no `--release` flag and is the relevant regression check for `[profile.dev]`; the release pipeline continues to test the production setting independently. + +### Current environment + +Collected on 2026-07-21: + +- Host rustc: `1.99.0-nightly`, LLVM `22.1.8`. +- Host cargo: `1.99.0-nightly`. +- Docker: `28.3.3`. +- Container base image: `docker.io/library/rust:slim-trixie`. + +The repository MSRV is Rust 1.88. The production-container verification is authoritative because it uses the toolchain provided by the `Containerfile` base image. + +## Verification implications + +- Build `--target debug` after removing `[profile.dev].lto`; this is the meaningful Docker regression test for the changed setting. +- Build `--target release`; it does not validate `[profile.dev]`, but confirms the retained production fat-LTO configuration remains healthy. +- Do not add a per-package LTO override: Cargo does not allow profile overrides to set `lto`. + +## Limitations + +The original Rust 1.79/1.81-nightly container environment is not reproduced. Reproduction is unnecessary to make the current configuration correct because the recorded command used the release profile, whereas this change only removes an explicit development-profile setting. From 71abb9d8eed040a4e21d69ba877c3c12bd08add7 Mon Sep 17 00:00:00 2001 From: Jose Celano Date: Tue, 21 Jul 2026 11:41:51 +0100 Subject: [PATCH 02/11] docs(issues): fix issue 1875 spec references --- .../EPIC.md | 6 +-- docs/issues/open/AGENTS.md | 33 ++++++------- .../pr-reviews/pr-2013-copilot-suggestions.md | 46 +++++++++++++++++++ 3 files changed, 66 insertions(+), 19 deletions(-) create mode 100644 docs/pr-reviews/pr-2013-copilot-suggestions.md diff --git a/docs/issues/open/1840-improve-pr-workflow-performance-epic/EPIC.md b/docs/issues/open/1840-improve-pr-workflow-performance-epic/EPIC.md index 028d32af9..96c44bab1 100644 --- a/docs/issues/open/1840-improve-pr-workflow-performance-epic/EPIC.md +++ b/docs/issues/open/1840-improve-pr-workflow-performance-epic/EPIC.md @@ -16,7 +16,6 @@ semantic-links: - .github/skills/dev/planning/create-issue/SKILL.md --- - # EPIC #1840 - Improve PR Workflow Performance ## Goal @@ -81,7 +80,7 @@ Ordering policy: | 11 | #[To be assigned] - Publish stable base stages as pre-built Docker Hub images (p3, deferred) | `docs/issues/drafts/1840-workflow-performance-prebuilt-base-images/ISSUE.md` | TODO | Low priority. Base stages (`chef`, `tester`, `gcc`) are fast (3–7 min cold). Compile dominates (35+ min). Revisit if base stages grow or if CI runner cold-cache frequency increases. | | 12 | #[To be assigned] - Pass Cargo registry/git caches into BuildKit cook stages | `docs/issues/drafts/1840-workflow-performance-buildkit-cargo-cache-mounts/ISSUE.md` | TODO | Adds `--mount=type=cache` for registry/git to cook stages. Local benefit: saves ~7 s download per cook rebuild (cold fetch 6.9 s → warm 0.16 s; registry 823 MB). CI benefit: none with ephemeral GitHub Actions runners (`type=gha` layer cache does not persist cache mount volumes). Evaluate target-dir cache mount variant as T5. | | 13 | #[To be assigned] - Apply Profile-Guided Optimization (PGO) to the tracker release binary | `docs/issues/drafts/1840-workflow-performance-pgo-optimization.md` | TODO | Deferred. Instrumentation PGO requires a double-compile pass which adds CI time — a direct cost against this EPIC's goals. Must measure CI overhead (T4/T5 in spec) and weigh against binary performance gains before enabling. Prerequisites: LTO already enabled in `[profile.release]`. Tooling: `cargo-pgo`. Training workload to be defined against realistic announce/scrape traffic. | -| 14 | #1875 - Review and fix `lto = "fat"` in `[profile.dev]` | `docs/issues/open/1875-review-lto-fat-in-dev-profile.md` | TODO | `lto = "fat"` in `[profile.dev]` was added in 2024 as a Docker/LLVM bitcode workaround (commit `3c715fbb`). With MSRV 1.88 and a stable toolchain in the Containerfile, the workaround may no longer be needed. Removing it should reduce CI test compile time (testing.yaml runs without `--release`) and Docker cook step time. | +| 14 | #1875 - Review and fix `lto = "fat"` in `[profile.dev]` | `docs/issues/open/1875-review-lto-fat-in-dev-profile/ISSUE.md` | IN_REVIEW | `lto = "fat"` in `[profile.dev]` was added in 2024 as a Docker/LLVM bitcode workaround (commit `3c715fbb`). The issue removes the development-profile override and retains release fat LTO; PR #2013 is under review. | ## Delivery Strategy @@ -146,7 +145,8 @@ Append one line per meaningful update. - 2026-06-03 00:00 UTC - GitHub Copilot - Marked #1853 DONE (merged PR #1867); added follow-up subissue #1868 (row 4.1) for `--exclude` fix based on post-merge CI analysis showing `workspace-coupling` still compiled (~840s gap, 19m03s total build step) - 2026-06-03 00:00 UTC - GitHub Copilot - Created GitHub issue #1869 and promoted dependency-layer-cache-reuse spec to `docs/issues/open/` (row 7) - 2026-06-03 00:00 UTC - GitHub Copilot - Added deferred subissue row 13 for PGO optimization of the release binary; draft spec at `docs/issues/drafts/1840-workflow-performance-pgo-optimization.md` -- 2026-06-03 00:00 UTC - GitHub Copilot - Created GitHub issue #1875 and added subissue row 14 for reviewing `lto = "fat"` in `[profile.dev]`; spec at `docs/issues/open/1875-review-lto-fat-in-dev-profile.md` +- 2026-06-03 00:00 UTC - GitHub Copilot - Created GitHub issue #1875 and added subissue row 14 for reviewing `lto = "fat"` in `[profile.dev]`; spec at `docs/issues/open/1875-review-lto-fat-in-dev-profile/ISSUE.md` +- 2026-07-21 00:00 UTC - GitHub Copilot - Updated subissue #1875 to IN_REVIEW; its folder-format spec is at `docs/issues/open/1875-review-lto-fat-in-dev-profile/ISSUE.md` and implementation PR #2013 is open. - 2026-06-09 00:00 UTC - GitHub Copilot - Updated row 10 (split-external-dep-cache-layer draft) to SUPERSEDED: the `--external-only` cargo-chef flag was implemented in a `torrust-cargo-chef` fork during #1869 investigation, directly addressing T3; row 7 (#1869) now covers implementation of the three-layer cook pattern - 2026-06-09 00:00 UTC - GitHub Copilot - Marked row 7 (#1869) as DONE: three-layer cook pattern implemented, verified locally (release + debug builds pass, third-party layer CACHED on app-code-only changes) diff --git a/docs/issues/open/AGENTS.md b/docs/issues/open/AGENTS.md index 2ad5a15af..94bdd6296 100644 --- a/docs/issues/open/AGENTS.md +++ b/docs/issues/open/AGENTS.md @@ -1,37 +1,38 @@ # Agents Instructions — `docs/issues/open/` -## File Naming Conventions +## Spec Folder Naming Conventions -Spec files in this folder follow one of these naming patterns: +Issue and EPIC specs use a folder named after the GitHub issue. The primary specification file +inside the folder is `ISSUE.md` for issues or `EPIC.md` for EPICs. ### Standalone issue (not part of an EPIC) ```text -{ISSUE_NUMBER}-{short-description}.md +{ISSUE_NUMBER}-{short-description}/ISSUE.md ``` Example: ```text -1875-review-lto-fat-in-dev-profile.md +1875-review-lto-fat-in-dev-profile/ISSUE.md ``` ### EPIC spec ```text -{EPIC_ISSUE_NUMBER}-{short-description}.md +{EPIC_ISSUE_NUMBER}-{short-description}/EPIC.md ``` Example: ```text -1978-configuration-overhaul-epic.md +1978-configuration-overhaul-epic/EPIC.md ``` ### Subissue (part of an EPIC) ```text -{SUB_ISSUE_NUMBER}-{EPIC_ISSUE_NUMBER}-{short-description}.md +{SUB_ISSUE_NUMBER}-{EPIC_ISSUE_NUMBER}-{short-description}/ISSUE.md ``` Where: @@ -42,7 +43,7 @@ Where: Example: ```text -1979-1978-copy-configuration-schema-v2-to-v3-baseline.md +1979-1978-copy-configuration-schema-v2-to-v3-baseline/ISSUE.md ``` ### Subissue with explicit implementation order @@ -51,7 +52,7 @@ An optional `si-{N}` segment can be added between the EPIC number and the descri the implementation order within the EPIC is significant and worth surfacing in the filename: ```text -{SUB_ISSUE_NUMBER}-{EPIC_ISSUE_NUMBER}-si-{ORDER}-{short-description}.md +{SUB_ISSUE_NUMBER}-{EPIC_ISSUE_NUMBER}-si-{ORDER}-{short-description}/ISSUE.md ``` Where: @@ -61,7 +62,7 @@ Where: Example: ```text -1969-1938-si-8-eliminate-unwraps-from-rest-api-client.md +1969-1938-si-8-eliminate-unwraps-from-rest-api-client/ISSUE.md ``` ## Key Rule @@ -72,9 +73,9 @@ number. ## Summary Table -| Pattern | Example | -| ------------------- | ---------------------------------------------------------- | -| Standalone | `1875-review-lto-fat-in-dev-profile.md` | -| EPIC | `1978-configuration-overhaul-epic.md` | -| Subissue | `1979-1978-copy-configuration-schema-v2-to-v3-baseline.md` | -| Subissue with order | `1969-1938-si-8-eliminate-unwraps-from-rest-api-client.md` | +| Pattern | Example | +| ------------------- | ---------------------------------------------------------------- | +| Standalone | `1875-review-lto-fat-in-dev-profile/ISSUE.md` | +| EPIC | `1978-configuration-overhaul-epic/EPIC.md` | +| Subissue | `1979-1978-copy-configuration-schema-v2-to-v3-baseline/ISSUE.md` | +| Subissue with order | `1969-1938-si-8-eliminate-unwraps-from-rest-api-client/ISSUE.md` | diff --git a/docs/pr-reviews/pr-2013-copilot-suggestions.md b/docs/pr-reviews/pr-2013-copilot-suggestions.md new file mode 100644 index 000000000..3ea1b8aa5 --- /dev/null +++ b/docs/pr-reviews/pr-2013-copilot-suggestions.md @@ -0,0 +1,46 @@ +--- +semantic-links: + skill-links: + - process-copilot-suggestions + related-artifacts: + - .github/skills/dev/pr-reviews/process-copilot-suggestions/SKILL.md +--- + + + +# PR #2013 Copilot Suggestions Tracking + +Source: Copilot PR review threads for https://github.com/torrust/torrust-tracker/pull/2013 + +Status legend: + +- `action`: code/docs change applied +- `no-action`: suggestion reviewed; no code change needed +- `resolved`: thread resolved in PR + +## Workflow + +1. Download all review threads (including resolved/outdated state and thread IDs). +2. Add one row per thread in the Suggestions table. +3. Process suggestions one by one: + - decide `action` or `no-action` + - if `action`, apply change and validate + - if needed, commit changes + - resolve the PR thread +4. Set `Thread State` to `resolved` once resolved in PR. + +## Processing Log + +- 2026-07-21: Started processing Copilot suggestions. + +## Suggestions + +| # | Thread ID | Path | URL | Suggestion Summary | Decision | Status | Thread State | +| --- | ----------------------- | -------------------------------------------------------------- | -------------------------------------------------------------------------------------- | ---------------------------------------------------------- | ----------------------------------------------------------------------------------------------------------- | ------ | ------------ | +| 1 | `PRRT_kwDOGp2yqc6SitOP` | `docs/issues/open/1875-review-lto-fat-in-dev-profile/ISSUE.md` | [comment](https://github.com/torrust/torrust-tracker/pull/2013#discussion_r3621393047) | Update stale references to the standalone issue-spec path. | action — update the EPIC's direct references and migrate the open-issues naming convention to folder specs. | DONE | OPEN | + +## Notes + +- Keep this file as an audit log of review handling for the PR. +- Prefer concise decisions with explicit rationale. +- If no code changes are needed, explain why in `Decision`. From 6a8e8cf1aa85bd1b5217982afa0f3704dc2c3385 Mon Sep 17 00:00:00 2001 From: Jose Celano Date: Tue, 21 Jul 2026 11:43:36 +0100 Subject: [PATCH 03/11] docs(review): record PR 2013 Copilot audit --- docs/pr-reviews/pr-2013-copilot-suggestions.md | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/docs/pr-reviews/pr-2013-copilot-suggestions.md b/docs/pr-reviews/pr-2013-copilot-suggestions.md index 3ea1b8aa5..86109fe46 100644 --- a/docs/pr-reviews/pr-2013-copilot-suggestions.md +++ b/docs/pr-reviews/pr-2013-copilot-suggestions.md @@ -32,12 +32,14 @@ Status legend: ## Processing Log - 2026-07-21: Started processing Copilot suggestions. +- 2026-07-21: Updated stale issue-spec references, validated the documentation change, pushed commit `01a4843d`, and resolved the Copilot thread. +- 2026-07-21: Completed processing suggestions. ## Suggestions | # | Thread ID | Path | URL | Suggestion Summary | Decision | Status | Thread State | | --- | ----------------------- | -------------------------------------------------------------- | -------------------------------------------------------------------------------------- | ---------------------------------------------------------- | ----------------------------------------------------------------------------------------------------------- | ------ | ------------ | -| 1 | `PRRT_kwDOGp2yqc6SitOP` | `docs/issues/open/1875-review-lto-fat-in-dev-profile/ISSUE.md` | [comment](https://github.com/torrust/torrust-tracker/pull/2013#discussion_r3621393047) | Update stale references to the standalone issue-spec path. | action — update the EPIC's direct references and migrate the open-issues naming convention to folder specs. | DONE | OPEN | +| 1 | `PRRT_kwDOGp2yqc6SitOP` | `docs/issues/open/1875-review-lto-fat-in-dev-profile/ISSUE.md` | [comment](https://github.com/torrust/torrust-tracker/pull/2013#discussion_r3621393047) | Update stale references to the standalone issue-spec path. | action — updated the EPIC's direct references and migrated the open-issues naming convention to folder specs. | DONE | RESOLVED | ## Notes From 1453693005a659cdc6ffcf104c672bf35be0720d Mon Sep 17 00:00:00 2001 From: Jose Celano Date: Tue, 21 Jul 2026 11:55:03 +0100 Subject: [PATCH 04/11] docs(review): require replies to Copilot suggestions --- .../process-copilot-suggestions/SKILL.md | 20 +++++++++++++++++-- .../pr-reviews/pr-2013-copilot-suggestions.md | 8 ++++---- .../templates/COPILOT-SUGGESTIONS-TEMPLATE.md | 9 ++++++--- 3 files changed, 28 insertions(+), 9 deletions(-) diff --git a/.github/skills/dev/pr-reviews/process-copilot-suggestions/SKILL.md b/.github/skills/dev/pr-reviews/process-copilot-suggestions/SKILL.md index 1983e2839..298024530 100644 --- a/.github/skills/dev/pr-reviews/process-copilot-suggestions/SKILL.md +++ b/.github/skills/dev/pr-reviews/process-copilot-suggestions/SKILL.md @@ -122,7 +122,20 @@ For each `action` item: ### 6. Batch Resolve All Threads -After all decisions are made and `action` items are committed: +After all decisions are made and `action` items are committed, reply to every suggestion thread +before resolving it. The reply is required even when the decision is `no-action`; it makes the +decision visible in the PR itself rather than only in the repository tracker. + +For an `action` item, state: + +- the commit that contains the fix, +- the files or behavior changed, and +- the validation performed when it is useful to establish correctness. + +For a `no-action` item, state the reason it was declined (for example, it was already addressed, +is outdated, or is a verified false positive). + +Then refresh the threads and resolve only those with a completed reply and a verified decision: ```bash bash ../fetch-review-threads/scripts/get-pr-review-threads.sh \ @@ -133,7 +146,9 @@ bash ../resolve-review-threads/scripts/resolve-all-unresolved-threads.sh \ --threads-file /tmp/pr_threads_.json ``` -This resolves all unresolved threads (both `action` and `no-action` categories). +This resolves all unresolved threads (both `action` and `no-action` categories). Do not use the +batch resolver before posting the required replies, because it would hide the outcome from the +review conversation. ### 7. Final Documentation @@ -183,6 +198,7 @@ with all 26 Copilot suggestions processed, decided, and resolved. - [ ] All review threads fetched and added to tracker table - [ ] Each thread categorized as `action` or `no-action` with rationale - [ ] All `action` items implemented, validated, and committed +- [ ] Each thread has a PR reply explaining the applied fix or no-action rationale - [ ] All threads resolved in GitHub (via batch script or one-by-one) - [ ] Tracker file updated with Processing Log and Thread State column - [ ] Tracker and helper scripts committed as documentation diff --git a/docs/pr-reviews/pr-2013-copilot-suggestions.md b/docs/pr-reviews/pr-2013-copilot-suggestions.md index 86109fe46..d76e56afe 100644 --- a/docs/pr-reviews/pr-2013-copilot-suggestions.md +++ b/docs/pr-reviews/pr-2013-copilot-suggestions.md @@ -32,14 +32,14 @@ Status legend: ## Processing Log - 2026-07-21: Started processing Copilot suggestions. -- 2026-07-21: Updated stale issue-spec references, validated the documentation change, pushed commit `01a4843d`, and resolved the Copilot thread. +- 2026-07-21: Updated stale issue-spec references, validated the documentation change, pushed commit `01a4843d`, replied with the fix summary, and resolved the Copilot thread. - 2026-07-21: Completed processing suggestions. ## Suggestions -| # | Thread ID | Path | URL | Suggestion Summary | Decision | Status | Thread State | -| --- | ----------------------- | -------------------------------------------------------------- | -------------------------------------------------------------------------------------- | ---------------------------------------------------------- | ----------------------------------------------------------------------------------------------------------- | ------ | ------------ | -| 1 | `PRRT_kwDOGp2yqc6SitOP` | `docs/issues/open/1875-review-lto-fat-in-dev-profile/ISSUE.md` | [comment](https://github.com/torrust/torrust-tracker/pull/2013#discussion_r3621393047) | Update stale references to the standalone issue-spec path. | action — updated the EPIC's direct references and migrated the open-issues naming convention to folder specs. | DONE | RESOLVED | +| # | Thread ID | Path | URL | Suggestion Summary | Decision | Reply URL | Status | Thread State | +| --- | ----------------------- | -------------------------------------------------------------- | -------------------------------------------------------------------------------------- | ---------------------------------------------------------- | ------------------------------------------------------------------------------------------------------------- | ------------------------------------------------------------------------------------ | ------ | ------------ | +| 1 | `PRRT_kwDOGp2yqc6SitOP` | `docs/issues/open/1875-review-lto-fat-in-dev-profile/ISSUE.md` | [comment](https://github.com/torrust/torrust-tracker/pull/2013#discussion_r3621393047) | Update stale references to the standalone issue-spec path. | action — updated the EPIC's direct references and migrated the open-issues naming convention to folder specs. | [reply](https://github.com/torrust/torrust-tracker/pull/2013#discussion_r3621442150) | DONE | RESOLVED | ## Notes diff --git a/docs/templates/COPILOT-SUGGESTIONS-TEMPLATE.md b/docs/templates/COPILOT-SUGGESTIONS-TEMPLATE.md index 11d793063..9f81bf951 100644 --- a/docs/templates/COPILOT-SUGGESTIONS-TEMPLATE.md +++ b/docs/templates/COPILOT-SUGGESTIONS-TEMPLATE.md @@ -26,7 +26,9 @@ Status legend: - decide `action` or `no-action` - if `action`, apply change and validate - if needed, commit changes + - reply on the PR thread with the fix commit and outcome, or the no-action rationale - resolve the PR thread + 4. Set `Thread State` to `resolved` once resolved in PR. ## Processing Log @@ -36,12 +38,13 @@ Status legend: ## Suggestions -| # | Thread ID | Path | URL | Suggestion Summary | Decision | Status | Thread State | -| --- | ----------- | ----------- | ------------- | ------------------ | --------------------- | -------------- | ------------------ | -| 1 | | | | | | | | +| # | Thread ID | Path | URL | Suggestion Summary | Decision | Reply URL | Status | Thread State | +| --- | ----------- | ----------- | ------------- | ------------------ | --------------------- | ----------- | -------------- | ------------------ | +| 1 | | | | | | | | | ## Notes - Keep this file as an audit log of review handling for the PR. - Prefer concise decisions with explicit rationale. - If no code changes are needed, explain why in `Decision`. +- Reply on every PR suggestion thread before resolving it so the decision is visible to reviewers. From 4dd3dc8554228d2c4abff3b9fb5491f53fb58419 Mon Sep 17 00:00:00 2001 From: Jose Celano Date: Tue, 21 Jul 2026 11:59:03 +0100 Subject: [PATCH 05/11] docs(review): add cspell:disable to PR suggestion trackers --- docs/pr-reviews/pr-2013-copilot-suggestions.md | 13 +++++++++---- docs/templates/COPILOT-SUGGESTIONS-TEMPLATE.md | 2 ++ 2 files changed, 11 insertions(+), 4 deletions(-) diff --git a/docs/pr-reviews/pr-2013-copilot-suggestions.md b/docs/pr-reviews/pr-2013-copilot-suggestions.md index d76e56afe..f66188228 100644 --- a/docs/pr-reviews/pr-2013-copilot-suggestions.md +++ b/docs/pr-reviews/pr-2013-copilot-suggestions.md @@ -6,6 +6,8 @@ semantic-links: - .github/skills/dev/pr-reviews/process-copilot-suggestions/SKILL.md --- + + # PR #2013 Copilot Suggestions Tracking @@ -33,13 +35,16 @@ Status legend: - 2026-07-21: Started processing Copilot suggestions. - 2026-07-21: Updated stale issue-spec references, validated the documentation change, pushed commit `01a4843d`, replied with the fix summary, and resolved the Copilot thread. -- 2026-07-21: Completed processing suggestions. +- 2026-07-21: Completed processing first suggestion. +- 2026-07-21: Added `` to tracker and template, committed in `2410d52d`, replied and resolved thread `PRRT_kwDOGp2yqc6Si2c6`. +- 2026-07-21: All suggestions processed. ## Suggestions -| # | Thread ID | Path | URL | Suggestion Summary | Decision | Reply URL | Status | Thread State | -| --- | ----------------------- | -------------------------------------------------------------- | -------------------------------------------------------------------------------------- | ---------------------------------------------------------- | ------------------------------------------------------------------------------------------------------------- | ------------------------------------------------------------------------------------ | ------ | ------------ | -| 1 | `PRRT_kwDOGp2yqc6SitOP` | `docs/issues/open/1875-review-lto-fat-in-dev-profile/ISSUE.md` | [comment](https://github.com/torrust/torrust-tracker/pull/2013#discussion_r3621393047) | Update stale references to the standalone issue-spec path. | action — updated the EPIC's direct references and migrated the open-issues naming convention to folder specs. | [reply](https://github.com/torrust/torrust-tracker/pull/2013#discussion_r3621442150) | DONE | RESOLVED | +| # | Thread ID | Path | URL | Suggestion Summary | Decision | Reply URL | Status | Thread State | +| --- | ----------------------- | -------------------------------------------------------------- | -------------------------------------------------------------------------------------- | --------------------------------------------------------------------------------- | -------------------------------------------------------------------------------------------------------------------------------- | ------------------------------------------------------------------------------------ | ------ | ------------ | +| 1 | `PRRT_kwDOGp2yqc6SitOP` | `docs/issues/open/1875-review-lto-fat-in-dev-profile/ISSUE.md` | [comment](https://github.com/torrust/torrust-tracker/pull/2013#discussion_r3621393047) | Update stale references to the standalone issue-spec path. | action — updated the EPIC's direct references and migrated the open-issues naming convention to folder specs. | [reply](https://github.com/torrust/torrust-tracker/pull/2013#discussion_r3621442150) | DONE | RESOLVED | +| 2 | `PRRT_kwDOGp2yqc6Si2c6` | `docs/pr-reviews/pr-2013-copilot-suggestions.md` | [comment](https://github.com/torrust/torrust-tracker/pull/2013#discussion_r3621444815) | Add `` to avoid spell-check failures on opaque thread IDs. | action — added `` to the tracker file and to the template so future PR trackers include it automatically. | [reply](https://github.com/torrust/torrust-tracker/pull/2013#discussion_r3621514178) | DONE | RESOLVED | ## Notes diff --git a/docs/templates/COPILOT-SUGGESTIONS-TEMPLATE.md b/docs/templates/COPILOT-SUGGESTIONS-TEMPLATE.md index 9f81bf951..0797c76c8 100644 --- a/docs/templates/COPILOT-SUGGESTIONS-TEMPLATE.md +++ b/docs/templates/COPILOT-SUGGESTIONS-TEMPLATE.md @@ -6,6 +6,8 @@ semantic-links: - .github/skills/dev/pr-reviews/process-copilot-suggestions/SKILL.md --- + + # PR # Copilot Suggestions Tracking From 266ab8543bbace09ef3461eaf32a842b90d5f503 Mon Sep 17 00:00:00 2001 From: Jose Celano Date: Tue, 21 Jul 2026 12:10:26 +0100 Subject: [PATCH 06/11] docs(review): add reply step to PR 2013 suggestion tracker workflow --- docs/pr-reviews/pr-2013-copilot-suggestions.md | 2 ++ 1 file changed, 2 insertions(+) diff --git a/docs/pr-reviews/pr-2013-copilot-suggestions.md b/docs/pr-reviews/pr-2013-copilot-suggestions.md index f66188228..8da332a92 100644 --- a/docs/pr-reviews/pr-2013-copilot-suggestions.md +++ b/docs/pr-reviews/pr-2013-copilot-suggestions.md @@ -28,6 +28,7 @@ Status legend: - decide `action` or `no-action` - if `action`, apply change and validate - if needed, commit changes + - reply on the PR thread with the fix commit and outcome, or the no-action rationale - resolve the PR thread 4. Set `Thread State` to `resolved` once resolved in PR. @@ -45,6 +46,7 @@ Status legend: | --- | ----------------------- | -------------------------------------------------------------- | -------------------------------------------------------------------------------------- | --------------------------------------------------------------------------------- | -------------------------------------------------------------------------------------------------------------------------------- | ------------------------------------------------------------------------------------ | ------ | ------------ | | 1 | `PRRT_kwDOGp2yqc6SitOP` | `docs/issues/open/1875-review-lto-fat-in-dev-profile/ISSUE.md` | [comment](https://github.com/torrust/torrust-tracker/pull/2013#discussion_r3621393047) | Update stale references to the standalone issue-spec path. | action — updated the EPIC's direct references and migrated the open-issues naming convention to folder specs. | [reply](https://github.com/torrust/torrust-tracker/pull/2013#discussion_r3621442150) | DONE | RESOLVED | | 2 | `PRRT_kwDOGp2yqc6Si2c6` | `docs/pr-reviews/pr-2013-copilot-suggestions.md` | [comment](https://github.com/torrust/torrust-tracker/pull/2013#discussion_r3621444815) | Add `` to avoid spell-check failures on opaque thread IDs. | action — added `` to the tracker file and to the template so future PR trackers include it automatically. | [reply](https://github.com/torrust/torrust-tracker/pull/2013#discussion_r3621514178) | DONE | RESOLVED | +| 3 | `PRRT_kwDOGp2yqc6SjBkm` | `docs/pr-reviews/pr-2013-copilot-suggestions.md` | [comment](https://github.com/torrust/torrust-tracker/pull/2013#discussion_r3621507603) | Workflow section missing the explicit reply-before-resolve step. | action — added the reply step to the workflow list in this file to match the template and skill. | [reply](https://github.com/torrust/torrust-tracker/pull/2013#discussion_r3621586425) | DONE | RESOLVED | ## Notes From 80924cac6539e1a895f47fa11a8b74298cd18c2d Mon Sep 17 00:00:00 2001 From: Jose Celano Date: Tue, 21 Jul 2026 12:23:42 +0100 Subject: [PATCH 07/11] docs(review): add blank line before step 4 in tracker workflow --- docs/pr-reviews/pr-2013-copilot-suggestions.md | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/docs/pr-reviews/pr-2013-copilot-suggestions.md b/docs/pr-reviews/pr-2013-copilot-suggestions.md index 8da332a92..65532ac03 100644 --- a/docs/pr-reviews/pr-2013-copilot-suggestions.md +++ b/docs/pr-reviews/pr-2013-copilot-suggestions.md @@ -30,6 +30,7 @@ Status legend: - if needed, commit changes - reply on the PR thread with the fix commit and outcome, or the no-action rationale - resolve the PR thread + 4. Set `Thread State` to `resolved` once resolved in PR. ## Processing Log @@ -46,7 +47,8 @@ Status legend: | --- | ----------------------- | -------------------------------------------------------------- | -------------------------------------------------------------------------------------- | --------------------------------------------------------------------------------- | -------------------------------------------------------------------------------------------------------------------------------- | ------------------------------------------------------------------------------------ | ------ | ------------ | | 1 | `PRRT_kwDOGp2yqc6SitOP` | `docs/issues/open/1875-review-lto-fat-in-dev-profile/ISSUE.md` | [comment](https://github.com/torrust/torrust-tracker/pull/2013#discussion_r3621393047) | Update stale references to the standalone issue-spec path. | action — updated the EPIC's direct references and migrated the open-issues naming convention to folder specs. | [reply](https://github.com/torrust/torrust-tracker/pull/2013#discussion_r3621442150) | DONE | RESOLVED | | 2 | `PRRT_kwDOGp2yqc6Si2c6` | `docs/pr-reviews/pr-2013-copilot-suggestions.md` | [comment](https://github.com/torrust/torrust-tracker/pull/2013#discussion_r3621444815) | Add `` to avoid spell-check failures on opaque thread IDs. | action — added `` to the tracker file and to the template so future PR trackers include it automatically. | [reply](https://github.com/torrust/torrust-tracker/pull/2013#discussion_r3621514178) | DONE | RESOLVED | -| 3 | `PRRT_kwDOGp2yqc6SjBkm` | `docs/pr-reviews/pr-2013-copilot-suggestions.md` | [comment](https://github.com/torrust/torrust-tracker/pull/2013#discussion_r3621507603) | Workflow section missing the explicit reply-before-resolve step. | action — added the reply step to the workflow list in this file to match the template and skill. | [reply](https://github.com/torrust/torrust-tracker/pull/2013#discussion_r3621586425) | DONE | RESOLVED | +| 3 | `PRRT_kwDOGp2yqc6SjBkm` | `docs/pr-reviews/pr-2013-copilot-suggestions.md` | [comment](https://github.com/torrust/torrust-tracker/pull/2013#discussion_r3621507603) | Workflow section missing the explicit reply-before-resolve step. | action — added the reply step to the workflow list in this file to match the template and skill. | [reply](https://github.com/torrust/torrust-tracker/pull/2013#discussion_r3621586425) | DONE | RESOLVED | +| 4 | `PRRT_kwDOGp2yqc6Sjapq` | `docs/pr-reviews/pr-2013-copilot-suggestions.md` | [comment](https://github.com/torrust/torrust-tracker/pull/2013#discussion_r3621648952) | Missing blank line before step 4 in Workflow list causes unreliable rendering. | action — added blank line before step 4. | [reply](https://github.com/torrust/torrust-tracker/pull/2013#discussion_r3621692387) | DONE | RESOLVED | ## Notes From 993ec09eab2ed5bfd2cd259dfcf7dc00d61c152b Mon Sep 17 00:00:00 2001 From: Jose Celano Date: Tue, 21 Jul 2026 12:39:02 +0100 Subject: [PATCH 08/11] docs(review): enforce reply-then-resolve per thread in copilot workflow --- .../scripts/check-thread-reply-status.sh | 105 ++++++++++++++ .../process-copilot-suggestions/SKILL.md | 135 ++++++++++++------ .../scripts/reply-and-resolve-thread.sh | 117 +++++++++++++++ .../scripts/reply-to-thread.sh | 91 ++++++++++++ 4 files changed, 407 insertions(+), 41 deletions(-) create mode 100755 .github/skills/dev/pr-reviews/fetch-review-threads/scripts/check-thread-reply-status.sh create mode 100755 .github/skills/dev/pr-reviews/resolve-review-threads/scripts/reply-and-resolve-thread.sh create mode 100755 .github/skills/dev/pr-reviews/resolve-review-threads/scripts/reply-to-thread.sh diff --git a/.github/skills/dev/pr-reviews/fetch-review-threads/scripts/check-thread-reply-status.sh b/.github/skills/dev/pr-reviews/fetch-review-threads/scripts/check-thread-reply-status.sh new file mode 100755 index 000000000..4d30ba930 --- /dev/null +++ b/.github/skills/dev/pr-reviews/fetch-review-threads/scripts/check-thread-reply-status.sh @@ -0,0 +1,105 @@ +#!/usr/bin/env bash + +set -euo pipefail + +usage() { + cat <<'EOF' +Usage: check-thread-reply-status.sh --threads-file [--login ] + +For each unresolved review thread, report whether the given user (or the current +authenticated GitHub user) has already posted a reply. + +Use this before running resolve-all-unresolved-threads.sh to confirm that every +thread has a reply. Threads without a reply should be handled with +reply-and-resolve-thread.sh instead of the bulk resolver. + +Options: + --threads-file Path to review threads JSON file (required) + --login GitHub login to check for replies (default: current gh user) + -h, --help Show this help + +Output: + - JSON lines to stdout, one per unresolved thread: + {"thread_id":"...","path":"...","url":"...","has_reply":true|false} + - Summary line at the end: + {"summary":true,"total":N,"with_reply":N,"without_reply":N} + - Diagnostics to stderr +EOF +} + +THREADS_FILE="" +LOGIN="" + +while [[ $# -gt 0 ]]; do + case "$1" in + --threads-file) + THREADS_FILE=${2:-} + shift 2 + ;; + --login) + LOGIN=${2:-} + shift 2 + ;; + -h|--help) + usage + exit 0 + ;; + *) + echo "Error: unknown argument '$1'." >&2 + usage >&2 + exit 2 + ;; + esac +done + +if [[ -z "${THREADS_FILE}" ]]; then + echo "Error: --threads-file is required." >&2 + usage >&2 + exit 2 +fi + +if [[ -z "${LOGIN}" ]]; then + LOGIN=$(gh api /user --jq .login) + echo "Using current GitHub user: ${LOGIN}" >&2 +fi + +total=0 +with_reply=0 +without_reply=0 + +while IFS= read -r thread_json; do + thread_id=$(echo "${thread_json}" | jq -r '.id') + path=$(echo "${thread_json}" | jq -r '.path') + url=$(echo "${thread_json}" | jq -r '.url') + has_reply=$(echo "${thread_json}" | jq --arg login "${LOGIN}" ' + .comments.nodes + | map(select(.author.login == $login)) + | length > 0 + ') + + printf '{"thread_id":"%s","path":"%s","url":"%s","has_reply":%s}\n' \ + "${thread_id}" "${path}" "${url}" "${has_reply}" + + total=$((total + 1)) + if [[ "${has_reply}" == "true" ]]; then + with_reply=$((with_reply + 1)) + else + without_reply=$((without_reply + 1)) + echo " ⚠ No reply yet on thread ${thread_id} (${path})" >&2 + fi +done < <(jq -c '.data.repository.pullRequest.reviewThreads.nodes[] + | select(.isResolved == false) + | { + id, + path, + url: (.comments.nodes[0].url // null), + comments + }' "${THREADS_FILE}") + +printf '{"summary":true,"total":%d,"with_reply":%d,"without_reply":%d}\n' \ + "${total}" "${with_reply}" "${without_reply}" + +if [[ "${without_reply}" -gt 0 ]]; then + echo "Error: ${without_reply} thread(s) have no reply. Use reply-and-resolve-thread.sh before bulk-resolving." >&2 + exit 1 +fi diff --git a/.github/skills/dev/pr-reviews/process-copilot-suggestions/SKILL.md b/.github/skills/dev/pr-reviews/process-copilot-suggestions/SKILL.md index 298024530..3a99de2c9 100644 --- a/.github/skills/dev/pr-reviews/process-copilot-suggestions/SKILL.md +++ b/.github/skills/dev/pr-reviews/process-copilot-suggestions/SKILL.md @@ -10,6 +10,9 @@ metadata: - .github/skills/dev/pr-reviews/fetch-review-threads/scripts/get-pr-review-threads.sh - .github/skills/dev/pr-reviews/fetch-review-threads/scripts/list-unresolved-threads.sh - .github/skills/dev/pr-reviews/fetch-review-threads/scripts/show-unresolved-thread-bodies.sh + - .github/skills/dev/pr-reviews/fetch-review-threads/scripts/check-thread-reply-status.sh + - .github/skills/dev/pr-reviews/resolve-review-threads/scripts/reply-to-thread.sh + - .github/skills/dev/pr-reviews/resolve-review-threads/scripts/reply-and-resolve-thread.sh - .github/skills/dev/pr-reviews/resolve-review-threads/scripts/resolve-all-unresolved-threads.sh --- @@ -25,6 +28,18 @@ Copilot generates suggestions that fall into two categories: - **action** — Code or documentation changes needed; implement, validate, commit - **no-action** — Already handled, false positive, or intentionally declined; explain reasoning and mark resolved +## Two Absolute Rules + +**Rule 1 — Always reply before resolving.** +Every thread must have a comment explaining what was done (or why nothing was done) before it +is marked resolved. Resolving a thread without a reply makes the decision invisible to reviewers +and future contributors reading the PR. + +**Rule 2 — Resolve promptly, one thread at a time.** +Copilot re-reviews the PR on every push and opens new suggestion threads. If old threads are +left unresolved, they become indistinguishable from the newly opened ones. Resolve each thread +immediately after posting the reply — do not accumulate a backlog of open threads. + ## Prerequisites - Target PR number @@ -83,81 +98,110 @@ Add one row per thread to your tracker file with: - Comment URL - Brief summary of the suggestion -### 4. Analyze and Decide +### 4. Process Each Thread (Decide → Implement → Reply → Resolve) + +Handle suggestions **one at a time**, completing each thread fully before moving to the next. +**Post a reply and resolve the thread before touching the next one.** This keeps already-addressed +threads visibly separated from new suggestions Copilot may open on the next push. -For each suggestion, decide: +For each unresolved thread: -- **action** — The suggestion identifies a real fix needed: - - Apply the code/doc change - - Run `linter all` and targeted tests - - Commit with clear message - - Update tracker with `action` status -- **no-action** — The suggestion is already handled or not needed: - - Document the reason (e.g., "outdated after later commits", "false positive verified by tests") - - Update tracker with `no-action` status and rationale +#### Step A — Decide -**Key principle**: Do not resolve a thread just because a suggestion exists. Only resolve when the concern is genuinely addressed or explicitly declined with documented reasoning. +- **`action`** — The suggestion identifies a real fix needed. Apply it. +- **`no-action`** — Already handled, false positive, or intentionally declined. Document the reason. -### 5. Implement Fixes +**Key principle**: Do not resolve a thread just because a suggestion exists. Only resolve when +the concern is genuinely addressed or explicitly declined with documented reasoning. -For each `action` item: +#### Step B — Implement (action only) -1. Read the suggestion carefully -2. Apply the minimal fix -3. Validate: +1. Apply the minimal fix. +2. Validate: ```bash linter all # Full lint gate cargo test -p # Targeted tests ``` -4. Commit with GPG signature: +3. Commit with GPG signature: ```bash git add - git commit -S -m "chore(review): " + git commit -S -m "fix(review): " ``` -5. Update tracker with `action` status +#### Step C — Reply and resolve -### 6. Batch Resolve All Threads +Use the `reply-and-resolve-thread.sh` script to post a reply **and** resolve in one operation: -After all decisions are made and `action` items are committed, reply to every suggestion thread -before resolving it. The reply is required even when the decision is `no-action`; it makes the -decision visible in the PR itself rather than only in the repository tracker. +```bash +bash ../resolve-review-threads/scripts/reply-and-resolve-thread.sh \ + --thread-id \ + --body "" +``` -For an `action` item, state: +For an `action` reply, include: - the commit that contains the fix, -- the files or behavior changed, and -- the validation performed when it is useful to establish correctness. +- the files or behaviour changed, and +- the validation performed (when useful to establish correctness). + +For a `no-action` reply, state the reason it was declined (for example, it was already +addressed, is outdated, or is a verified false positive). -For a `no-action` item, state the reason it was declined (for example, it was already addressed, -is outdated, or is a verified false positive). +The script outputs `{"reply_url": "...", "resolved": true}`. Copy the `reply_url` into the +tracker row. -Then refresh the threads and resolve only those with a completed reply and a verified decision: +#### Step D — Update tracker + +- Set `Reply URL` to the reply URL from the script output. +- Set `Status` to `DONE`. +- Set `Thread State` to `RESOLVED`. + +Repeat steps A–D for every thread before moving on. + +### 5. Verify All Threads Are Resolved + +After processing all threads, refresh and verify no unresolved threads remain: ```bash bash ../fetch-review-threads/scripts/get-pr-review-threads.sh \ --pr-number \ --output-file /tmp/pr_threads_.json -bash ../resolve-review-threads/scripts/resolve-all-unresolved-threads.sh \ +bash ../fetch-review-threads/scripts/list-unresolved-threads.sh \ + --threads-file /tmp/pr_threads_.json +``` + +If any threads remain (Copilot may post new suggestions as you push commits), process them +using the same per-thread loop (Step 4). + +#### Batch resolver — emergency cleanup only + +If some threads need bulk-resolving, first confirm every thread already has a user reply: + +```bash +bash ../fetch-review-threads/scripts/check-thread-reply-status.sh \ --threads-file /tmp/pr_threads_.json ``` -This resolves all unresolved threads (both `action` and `no-action` categories). Do not use the -batch resolver before posting the required replies, because it would hide the outcome from the -review conversation. +This script exits with code 1 if any thread lacks a reply. Only proceed with the batch resolver +once it exits 0: + +```bash +bash ../resolve-review-threads/scripts/resolve-all-unresolved-threads.sh \ + --threads-file /tmp/pr_threads_.json +``` -### 7. Final Documentation +### 6. Final Documentation Update the tracker file with completion notes: -- Add timestamps to the Processing Log -- Mark all threads as `resolved` in the Thread State column +- Add timestamps to the Processing Log. +- Confirm all rows have `Status = DONE` and `Thread State = RESOLVED`. -Commit the tracker and related review docs as final documentation: +Commit the tracker as final documentation: ```bash git add docs/pr-reviews/pr--copilot-suggestions.md @@ -176,9 +220,18 @@ git commit -S -m "docs(review): document PR # copilot suggestions aud ## Helper Scripts Reference +### Fetch & inspect threads + - `../fetch-review-threads/scripts/get-pr-review-threads.sh` — Fetch all threads for a PR - `../fetch-review-threads/scripts/list-unresolved-threads.sh` — Filter to unresolved threads only -- `../resolve-review-threads/scripts/resolve-all-unresolved-threads.sh` — Resolve all unresolved threads via GraphQL +- `../fetch-review-threads/scripts/show-unresolved-thread-bodies.sh` — Show full body of each unresolved thread +- `../fetch-review-threads/scripts/check-thread-reply-status.sh` — Report which unresolved threads are missing a reply (exits 1 if any are missing) + +### Reply & resolve threads + +- `../resolve-review-threads/scripts/reply-and-resolve-thread.sh` — Post a reply then resolve a single thread (preferred per-thread operation) +- `../resolve-review-threads/scripts/reply-to-thread.sh` — Post a reply on a thread without resolving it +- `../resolve-review-threads/scripts/resolve-all-unresolved-threads.sh` — Bulk-resolve all unresolved threads (use only after `check-thread-reply-status.sh` exits 0) ## Related Skills @@ -198,8 +251,8 @@ with all 26 Copilot suggestions processed, decided, and resolved. - [ ] All review threads fetched and added to tracker table - [ ] Each thread categorized as `action` or `no-action` with rationale - [ ] All `action` items implemented, validated, and committed -- [ ] Each thread has a PR reply explaining the applied fix or no-action rationale -- [ ] All threads resolved in GitHub (via batch script or one-by-one) +- [ ] Every thread replied to with `reply-and-resolve-thread.sh` (reply URL recorded in tracker) +- [ ] All threads resolved in GitHub (`list-unresolved-threads.sh` returns no output) - [ ] Tracker file updated with Processing Log and Thread State column -- [ ] Tracker and helper scripts committed as documentation +- [ ] Tracker committed as documentation - [ ] No uncommitted changes remain diff --git a/.github/skills/dev/pr-reviews/resolve-review-threads/scripts/reply-and-resolve-thread.sh b/.github/skills/dev/pr-reviews/resolve-review-threads/scripts/reply-and-resolve-thread.sh new file mode 100755 index 000000000..9bde4ab57 --- /dev/null +++ b/.github/skills/dev/pr-reviews/resolve-review-threads/scripts/reply-and-resolve-thread.sh @@ -0,0 +1,117 @@ +#!/usr/bin/env bash + +set -euo pipefail + +usage() { + cat <<'EOF' +Usage: reply-and-resolve-thread.sh --thread-id (--body | --body-file ) [--dry-run] + +Post a reply on a pull-request review thread and then resolve it. +The reply is always posted before the thread is resolved. + +Options: + --thread-id Node ID of the review thread (e.g. PRRT_kwDOxxx) (required) + --body Reply body text (required unless --body-file is given) + --body-file Read reply body from file instead of --body + --dry-run Print what would happen without posting or resolving + -h, --help Show this help + +Output: + - JSON line to stdout: {"status":"ok","thread_id":"...","reply_url":"...","resolved":true} + - Diagnostics to stderr +EOF +} + +THREAD_ID="" +BODY="" +BODY_FILE="" +DRY_RUN="false" + +while [[ $# -gt 0 ]]; do + case "$1" in + --thread-id) + THREAD_ID=${2:-} + shift 2 + ;; + --body) + BODY=${2:-} + shift 2 + ;; + --body-file) + BODY_FILE=${2:-} + shift 2 + ;; + --dry-run) + DRY_RUN="true" + shift + ;; + -h|--help) + usage + exit 0 + ;; + *) + echo "Error: unknown argument '$1'." >&2 + usage >&2 + exit 2 + ;; + esac +done + +if [[ -z "${THREAD_ID}" ]]; then + echo "Error: --thread-id is required." >&2 + usage >&2 + exit 2 +fi + +if [[ -n "${BODY_FILE}" ]]; then + if [[ ! -f "${BODY_FILE}" ]]; then + echo "Error: --body-file '${BODY_FILE}' does not exist." >&2 + exit 2 + fi + BODY=$(cat "${BODY_FILE}") +fi + +if [[ -z "${BODY}" ]]; then + echo "Error: --body or --body-file is required." >&2 + usage >&2 + exit 2 +fi + +if [[ "${DRY_RUN}" == "true" ]]; then + printf '{"status":"dry-run","thread_id":"%s","body_length":%d}\n' "${THREAD_ID}" "${#BODY}" + exit 0 +fi + +echo "Posting reply to thread ${THREAD_ID}..." >&2 + +# shellcheck disable=SC2016 +REPLY_URL=$(gh api graphql \ + -F threadId="${THREAD_ID}" \ + -F body="${BODY}" \ + -f query='mutation($threadId: ID!, $body: String!) { + addPullRequestReviewThreadReply(input: { + pullRequestReviewThreadId: $threadId + body: $body + }) { + comment { + url + } + } + }' \ + --jq '.data.addPullRequestReviewThreadReply.comment.url') + +echo "Resolving thread ${THREAD_ID}..." >&2 + +# shellcheck disable=SC2016 +gh api graphql \ + -F threadId="${THREAD_ID}" \ + -f query='mutation($threadId: ID!) { + resolveReviewThread(input: { threadId: $threadId }) { + thread { + id + isResolved + } + } + }' >/dev/null + +printf '{"status":"ok","thread_id":"%s","reply_url":"%s","resolved":true}\n' "${THREAD_ID}" "${REPLY_URL}" diff --git a/.github/skills/dev/pr-reviews/resolve-review-threads/scripts/reply-to-thread.sh b/.github/skills/dev/pr-reviews/resolve-review-threads/scripts/reply-to-thread.sh new file mode 100755 index 000000000..5c63cea62 --- /dev/null +++ b/.github/skills/dev/pr-reviews/resolve-review-threads/scripts/reply-to-thread.sh @@ -0,0 +1,91 @@ +#!/usr/bin/env bash + +set -euo pipefail + +usage() { + cat <<'EOF' +Usage: reply-to-thread.sh --thread-id (--body | --body-file ) + +Post a reply comment on a pull-request review thread. + +Options: + --thread-id Node ID of the review thread (e.g. PRRT_kwDOxxx) (required) + --body Reply body text (required unless --body-file is given) + --body-file Read reply body from file instead of --body + -h, --help Show this help + +Output: + - JSON line to stdout: {"status":"ok","thread_id":"...","reply_url":"..."} + - Diagnostics to stderr +EOF +} + +THREAD_ID="" +BODY="" +BODY_FILE="" + +while [[ $# -gt 0 ]]; do + case "$1" in + --thread-id) + THREAD_ID=${2:-} + shift 2 + ;; + --body) + BODY=${2:-} + shift 2 + ;; + --body-file) + BODY_FILE=${2:-} + shift 2 + ;; + -h|--help) + usage + exit 0 + ;; + *) + echo "Error: unknown argument '$1'." >&2 + usage >&2 + exit 2 + ;; + esac +done + +if [[ -z "${THREAD_ID}" ]]; then + echo "Error: --thread-id is required." >&2 + usage >&2 + exit 2 +fi + +if [[ -n "${BODY_FILE}" ]]; then + if [[ ! -f "${BODY_FILE}" ]]; then + echo "Error: --body-file '${BODY_FILE}' does not exist." >&2 + exit 2 + fi + BODY=$(cat "${BODY_FILE}") +fi + +if [[ -z "${BODY}" ]]; then + echo "Error: --body or --body-file is required." >&2 + usage >&2 + exit 2 +fi + +echo "Posting reply to thread ${THREAD_ID}..." >&2 + +# shellcheck disable=SC2016 +REPLY_URL=$(gh api graphql \ + -F threadId="${THREAD_ID}" \ + -F body="${BODY}" \ + -f query='mutation($threadId: ID!, $body: String!) { + addPullRequestReviewThreadReply(input: { + pullRequestReviewThreadId: $threadId + body: $body + }) { + comment { + url + } + } + }' \ + --jq '.data.addPullRequestReviewThreadReply.comment.url') + +printf '{"status":"ok","thread_id":"%s","reply_url":"%s"}\n' "${THREAD_ID}" "${REPLY_URL}" From d999ce8c24f3331700169f2c7386e85d3dc7322b Mon Sep 17 00:00:00 2001 From: Jose Celano Date: Tue, 21 Jul 2026 12:46:51 +0100 Subject: [PATCH 09/11] docs(agents): add Copilot Suggestions Handler agent --- .../copilot-suggestions-handler.agent.md | 123 ++++++++++++++++++ 1 file changed, 123 insertions(+) create mode 100644 .github/agents/copilot-suggestions-handler.agent.md diff --git a/.github/agents/copilot-suggestions-handler.agent.md b/.github/agents/copilot-suggestions-handler.agent.md new file mode 100644 index 000000000..e8b1e72db --- /dev/null +++ b/.github/agents/copilot-suggestions-handler.agent.md @@ -0,0 +1,123 @@ +--- +name: Copilot Suggestions Handler +description: Processes all Copilot review suggestion threads on a pull request. For each thread it decides action or no-action, applies the fix, posts a reply explaining the outcome, and resolves the thread immediately. Use when asked to handle Copilot suggestions, process PR review feedback, reply and resolve Copilot threads, or clear open suggestion threads on a PR. +argument-hint: Provide the PR number. Optionally specify threads to skip or a path to an existing tracker file. +tools: [execute, read, search, edit, todo, agent] +user-invocable: true +disable-model-invocation: false +--- + +You are the repository's Copilot suggestion handler. + +Your job is to process every open Copilot review thread on a pull request: decide whether to act, +apply and commit any needed fix, post a reply explaining the outcome, and immediately resolve the +thread. Then repeat for the next thread until none remain. + +## Two Absolute Rules + +**Rule 1 — Always reply before resolving.** +Every thread must have a comment that explains what was done (or why nothing was done) before it +is marked resolved. Resolving a thread without a reply makes the decision invisible to reviewers. + +**Rule 2 — Resolve each thread immediately after replying.** +Copilot opens new suggestion threads on every push. If old threads stay open they become +indistinguishable from new ones. Resolve each thread right after posting the reply — do not +accumulate a backlog. + +## Repository Rules + +- Follow `AGENTS.md` for repository-wide standards. +- Use `.github/skills/dev/pr-reviews/process-copilot-suggestions/SKILL.md` as the primary + reference for the full workflow, decision matrix, and helper script commands. +- Use the **Committer** agent for all GPG-signed commits. + +## Required Workflow + +### 1. Setup + +If no tracker file exists for this PR, create one from the template: + +```bash +cp docs/templates/COPILOT-SUGGESTIONS-TEMPLATE.md \ + docs/pr-reviews/pr--copilot-suggestions.md +``` + +Fill in `` and ``. + +### 2. Fetch Unresolved Threads + +```bash +bash .github/skills/dev/pr-reviews/fetch-review-threads/scripts/get-pr-review-threads.sh \ + --pr-number \ + --output-file /tmp/pr_threads_.json + +bash .github/skills/dev/pr-reviews/fetch-review-threads/scripts/show-unresolved-thread-bodies.sh \ + --threads-file /tmp/pr_threads_.json +``` + +Add one row per unresolved thread to the tracker table. + +### 3. Per-Thread Loop + +For **each** unresolved thread — complete all steps before moving to the next: + +#### Step A — Decide + +- `action`: suggestion identifies a real fix. Apply it. +- `no-action`: already handled, false positive, or intentionally declined. Document the reason. + +#### Step B — Implement (action only) + +1. Apply the minimal fix. +2. Validate: `linter all` and targeted `cargo test -p `. +3. Ask the **Committer** agent to create a GPG-signed commit. + +#### Step C — Reply and resolve (always) + +Use the atomic script — it posts the reply first and then resolves. It requires `--body`, so +resolving without a reply is not possible: + +```bash +bash .github/skills/dev/pr-reviews/resolve-review-threads/scripts/reply-and-resolve-thread.sh \ + --thread-id \ + --body "" +``` + +For an `action` reply include: the commit SHA, files changed, and validation performed. +For a `no-action` reply state the reason it was declined. + +Copy the `reply_url` from the script output into the tracker row. + +#### Step D — Update tracker + +Set `Reply URL`, `Status = DONE`, `Thread State = RESOLVED` in the suggestions table. + +### 4. Re-check After Each Push + +After any new commits are pushed, re-run Steps 2–3. Copilot may have opened new threads. +Stop only when `list-unresolved-threads.sh` returns no output. + +```bash +bash .github/skills/dev/pr-reviews/fetch-review-threads/scripts/list-unresolved-threads.sh \ + --threads-file /tmp/pr_threads_.json +``` + +### 5. Finalize + +Update the tracker Processing Log with timestamps and commit: + +```bash +git add docs/pr-reviews/pr--copilot-suggestions.md +# then ask the Committer agent to commit +``` + +## Constraints + +- Do not resolve a thread before posting a reply. Use `reply-and-resolve-thread.sh` — never call + the resolver directly. +- Do not use the batch resolver (`resolve-all-unresolved-threads.sh`) unless every thread already + has a reply. Run `check-thread-reply-status.sh` first to confirm. +- Do not implement large features or refactors in response to a Copilot suggestion. Prefer + `no-action` with a documented explanation and a follow-up issue. +- Do not push commits without running the pre-commit gate first. +- Do not modify threads from human reviewers — this agent handles Copilot threads only. From 8c2cf5de2d5c334f0d9af64e01bc83bee53f758a Mon Sep 17 00:00:00 2001 From: Jose Celano Date: Tue, 21 Jul 2026 12:52:46 +0100 Subject: [PATCH 10/11] fix(pr-review-scripts): validate reply URL and fix JSON type handling - reply-to-thread.sh: guard against empty/null REPLY_URL from GraphQL; exit 1 if the mutation returns no comment URL so callers detect failure. - reply-and-resolve-thread.sh: same guard before proceeding to resolve, preventing a silent resolve when the reply was never posted. - check-thread-reply-status.sh: replace printf JSON construction with jq -n / --argjson so url:null is emitted as JSON null instead of the string "null". Addresses Copilot review threads PRRT_kwDOGp2yqc6Sj3Ce, PRRT_kwDOGp2yqc6Sj3Cy, PRRT_kwDOGp2yqc6Sj3DE on PR #2013. --- .../scripts/check-thread-reply-status.sh | 10 +++++++--- .../scripts/reply-and-resolve-thread.sh | 5 +++++ .../scripts/reply-to-thread.sh | 5 +++++ docs/pr-reviews/pr-2013-copilot-suggestions.md | 16 ++++++++++------ 4 files changed, 27 insertions(+), 9 deletions(-) diff --git a/.github/skills/dev/pr-reviews/fetch-review-threads/scripts/check-thread-reply-status.sh b/.github/skills/dev/pr-reviews/fetch-review-threads/scripts/check-thread-reply-status.sh index 4d30ba930..36f56d4bb 100755 --- a/.github/skills/dev/pr-reviews/fetch-review-threads/scripts/check-thread-reply-status.sh +++ b/.github/skills/dev/pr-reviews/fetch-review-threads/scripts/check-thread-reply-status.sh @@ -70,15 +70,19 @@ without_reply=0 while IFS= read -r thread_json; do thread_id=$(echo "${thread_json}" | jq -r '.id') path=$(echo "${thread_json}" | jq -r '.path') - url=$(echo "${thread_json}" | jq -r '.url') has_reply=$(echo "${thread_json}" | jq --arg login "${LOGIN}" ' .comments.nodes | map(select(.author.login == $login)) | length > 0 ') - printf '{"thread_id":"%s","path":"%s","url":"%s","has_reply":%s}\n' \ - "${thread_id}" "${path}" "${url}" "${has_reply}" + url_json=$(echo "${thread_json}" | jq '.url') + jq -n \ + --arg thread_id "${thread_id}" \ + --arg path "${path}" \ + --argjson url "${url_json}" \ + --argjson has_reply "${has_reply}" \ + '{"thread_id":$thread_id,"path":$path,"url":$url,"has_reply":$has_reply}' total=$((total + 1)) if [[ "${has_reply}" == "true" ]]; then diff --git a/.github/skills/dev/pr-reviews/resolve-review-threads/scripts/reply-and-resolve-thread.sh b/.github/skills/dev/pr-reviews/resolve-review-threads/scripts/reply-and-resolve-thread.sh index 9bde4ab57..a0203c7c7 100755 --- a/.github/skills/dev/pr-reviews/resolve-review-threads/scripts/reply-and-resolve-thread.sh +++ b/.github/skills/dev/pr-reviews/resolve-review-threads/scripts/reply-and-resolve-thread.sh @@ -100,6 +100,11 @@ REPLY_URL=$(gh api graphql \ }' \ --jq '.data.addPullRequestReviewThreadReply.comment.url') +if [[ -z "${REPLY_URL}" || "${REPLY_URL}" == "null" ]]; then + echo "Error: GraphQL mutation returned no reply URL; aborting resolve." >&2 + exit 1 +fi + echo "Resolving thread ${THREAD_ID}..." >&2 # shellcheck disable=SC2016 diff --git a/.github/skills/dev/pr-reviews/resolve-review-threads/scripts/reply-to-thread.sh b/.github/skills/dev/pr-reviews/resolve-review-threads/scripts/reply-to-thread.sh index 5c63cea62..98c2a53e1 100755 --- a/.github/skills/dev/pr-reviews/resolve-review-threads/scripts/reply-to-thread.sh +++ b/.github/skills/dev/pr-reviews/resolve-review-threads/scripts/reply-to-thread.sh @@ -88,4 +88,9 @@ REPLY_URL=$(gh api graphql \ }' \ --jq '.data.addPullRequestReviewThreadReply.comment.url') +if [[ -z "${REPLY_URL}" || "${REPLY_URL}" == "null" ]]; then + echo "Error: GraphQL mutation returned no reply URL; the comment may not have been posted." >&2 + exit 1 +fi + printf '{"status":"ok","thread_id":"%s","reply_url":"%s"}\n' "${THREAD_ID}" "${REPLY_URL}" diff --git a/docs/pr-reviews/pr-2013-copilot-suggestions.md b/docs/pr-reviews/pr-2013-copilot-suggestions.md index 65532ac03..0eee3240c 100644 --- a/docs/pr-reviews/pr-2013-copilot-suggestions.md +++ b/docs/pr-reviews/pr-2013-copilot-suggestions.md @@ -40,15 +40,19 @@ Status legend: - 2026-07-21: Completed processing first suggestion. - 2026-07-21: Added `` to tracker and template, committed in `2410d52d`, replied and resolved thread `PRRT_kwDOGp2yqc6Si2c6`. - 2026-07-21: All suggestions processed. +- 2026-07-21: Three new threads opened by Copilot after last push. Fixed reply URL validation in `reply-to-thread.sh` and `reply-and-resolve-thread.sh`, and replaced `printf` JSON construction with `jq -n`/`--argjson` in `check-thread-reply-status.sh`. ## Suggestions -| # | Thread ID | Path | URL | Suggestion Summary | Decision | Reply URL | Status | Thread State | -| --- | ----------------------- | -------------------------------------------------------------- | -------------------------------------------------------------------------------------- | --------------------------------------------------------------------------------- | -------------------------------------------------------------------------------------------------------------------------------- | ------------------------------------------------------------------------------------ | ------ | ------------ | -| 1 | `PRRT_kwDOGp2yqc6SitOP` | `docs/issues/open/1875-review-lto-fat-in-dev-profile/ISSUE.md` | [comment](https://github.com/torrust/torrust-tracker/pull/2013#discussion_r3621393047) | Update stale references to the standalone issue-spec path. | action — updated the EPIC's direct references and migrated the open-issues naming convention to folder specs. | [reply](https://github.com/torrust/torrust-tracker/pull/2013#discussion_r3621442150) | DONE | RESOLVED | -| 2 | `PRRT_kwDOGp2yqc6Si2c6` | `docs/pr-reviews/pr-2013-copilot-suggestions.md` | [comment](https://github.com/torrust/torrust-tracker/pull/2013#discussion_r3621444815) | Add `` to avoid spell-check failures on opaque thread IDs. | action — added `` to the tracker file and to the template so future PR trackers include it automatically. | [reply](https://github.com/torrust/torrust-tracker/pull/2013#discussion_r3621514178) | DONE | RESOLVED | -| 3 | `PRRT_kwDOGp2yqc6SjBkm` | `docs/pr-reviews/pr-2013-copilot-suggestions.md` | [comment](https://github.com/torrust/torrust-tracker/pull/2013#discussion_r3621507603) | Workflow section missing the explicit reply-before-resolve step. | action — added the reply step to the workflow list in this file to match the template and skill. | [reply](https://github.com/torrust/torrust-tracker/pull/2013#discussion_r3621586425) | DONE | RESOLVED | -| 4 | `PRRT_kwDOGp2yqc6Sjapq` | `docs/pr-reviews/pr-2013-copilot-suggestions.md` | [comment](https://github.com/torrust/torrust-tracker/pull/2013#discussion_r3621648952) | Missing blank line before step 4 in Workflow list causes unreliable rendering. | action — added blank line before step 4. | [reply](https://github.com/torrust/torrust-tracker/pull/2013#discussion_r3621692387) | DONE | RESOLVED | +| # | Thread ID | Path | URL | Suggestion Summary | Decision | Reply URL | Status | Thread State | +| --- | ----------------------- | ------------------------------------------------------------------------------------------ | -------------------------------------------------------------------------------------- | ------------------------------------------------------------------------------------------------------------- | -------------------------------------------------------------------------------------------------------------------------------- | ------------------------------------------------------------------------------------ | ----------- | ------------ | +| 1 | `PRRT_kwDOGp2yqc6SitOP` | `docs/issues/open/1875-review-lto-fat-in-dev-profile/ISSUE.md` | [comment](https://github.com/torrust/torrust-tracker/pull/2013#discussion_r3621393047) | Update stale references to the standalone issue-spec path. | action — updated the EPIC's direct references and migrated the open-issues naming convention to folder specs. | [reply](https://github.com/torrust/torrust-tracker/pull/2013#discussion_r3621442150) | DONE | RESOLVED | +| 2 | `PRRT_kwDOGp2yqc6Si2c6` | `docs/pr-reviews/pr-2013-copilot-suggestions.md` | [comment](https://github.com/torrust/torrust-tracker/pull/2013#discussion_r3621444815) | Add `` to avoid spell-check failures on opaque thread IDs. | action — added `` to the tracker file and to the template so future PR trackers include it automatically. | [reply](https://github.com/torrust/torrust-tracker/pull/2013#discussion_r3621514178) | DONE | RESOLVED | +| 3 | `PRRT_kwDOGp2yqc6SjBkm` | `docs/pr-reviews/pr-2013-copilot-suggestions.md` | [comment](https://github.com/torrust/torrust-tracker/pull/2013#discussion_r3621507603) | Workflow section missing the explicit reply-before-resolve step. | action — added the reply step to the workflow list in this file to match the template and skill. | [reply](https://github.com/torrust/torrust-tracker/pull/2013#discussion_r3621586425) | DONE | RESOLVED | +| 4 | `PRRT_kwDOGp2yqc6Sjapq` | `docs/pr-reviews/pr-2013-copilot-suggestions.md` | [comment](https://github.com/torrust/torrust-tracker/pull/2013#discussion_r3621648952) | Missing blank line before step 4 in Workflow list causes unreliable rendering. | action — added blank line before step 4. | [reply](https://github.com/torrust/torrust-tracker/pull/2013#discussion_r3621692387) | DONE | RESOLVED | +| 5 | `PRRT_kwDOGp2yqc6Sj3Ce` | `.github/skills/dev/pr-reviews/resolve-review-threads/scripts/reply-to-thread.sh` | [comment](https://github.com/torrust/torrust-tracker/pull/2013#discussion_r3621808021) | `gh api graphql --jq` can return empty/`null` while exiting 0; validate `REPLY_URL` before reporting success. | action — added guard after GraphQL call: exit 1 if `REPLY_URL` is empty or `null`. | | IN-PROGRESS | OPEN | +| 6 | `PRRT_kwDOGp2yqc6Sj3Cy` | `.github/skills/dev/pr-reviews/fetch-review-threads/scripts/check-thread-reply-status.sh` | [comment](https://github.com/torrust/torrust-tracker/pull/2013#discussion_r3621808055) | `printf` with `%s` emits `"null"` string when url is JSON null; use `jq -n`/`--argjson` for correct types. | action — replaced `printf` with `jq -n --argjson` to preserve JSON types. | | IN-PROGRESS | OPEN | +| 7 | `PRRT_kwDOGp2yqc6Sj3DE` | `.github/skills/dev/pr-reviews/resolve-review-threads/scripts/reply-and-resolve-thread.sh` | [comment](https://github.com/torrust/torrust-tracker/pull/2013#discussion_r3621808078) | GraphQL mutation can return empty/`null` URL while exiting 0; validate before proceeding to resolve. | action — added guard after GraphQL call: exit 1 if `REPLY_URL` is empty or `null`. | | IN-PROGRESS | OPEN | ## Notes From f375d34d342a53cab64685cac1daf3cfcddc2eef Mon Sep 17 00:00:00 2001 From: Jose Celano Date: Tue, 21 Jul 2026 12:54:55 +0100 Subject: [PATCH 11/11] docs(pr-2013): update Copilot suggestions tracker with threads 5-7 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Added rows for PRRT_kwDOGp2yqc6Sj3Ce, PRRT_kwDOGp2yqc6Sj3Cy, PRRT_kwDOGp2yqc6Sj3DE — all resolved in commit 3039b382. --- .../pr-reviews/pr-2013-copilot-suggestions.md | 20 +++++++++---------- 1 file changed, 10 insertions(+), 10 deletions(-) diff --git a/docs/pr-reviews/pr-2013-copilot-suggestions.md b/docs/pr-reviews/pr-2013-copilot-suggestions.md index 0eee3240c..a53db5b6e 100644 --- a/docs/pr-reviews/pr-2013-copilot-suggestions.md +++ b/docs/pr-reviews/pr-2013-copilot-suggestions.md @@ -40,19 +40,19 @@ Status legend: - 2026-07-21: Completed processing first suggestion. - 2026-07-21: Added `` to tracker and template, committed in `2410d52d`, replied and resolved thread `PRRT_kwDOGp2yqc6Si2c6`. - 2026-07-21: All suggestions processed. -- 2026-07-21: Three new threads opened by Copilot after last push. Fixed reply URL validation in `reply-to-thread.sh` and `reply-and-resolve-thread.sh`, and replaced `printf` JSON construction with `jq -n`/`--argjson` in `check-thread-reply-status.sh`. +- 2026-07-21: Three new threads opened by Copilot after last push. Fixed reply URL validation in `reply-to-thread.sh` and `reply-and-resolve-thread.sh`, and replaced `printf` JSON construction with `jq -n`/`--argjson` in `check-thread-reply-status.sh`. Committed `3039b382`, replied and resolved threads `PRRT_kwDOGp2yqc6Sj3Ce`, `PRRT_kwDOGp2yqc6Sj3Cy`, `PRRT_kwDOGp2yqc6Sj3DE`. All threads resolved. ## Suggestions -| # | Thread ID | Path | URL | Suggestion Summary | Decision | Reply URL | Status | Thread State | -| --- | ----------------------- | ------------------------------------------------------------------------------------------ | -------------------------------------------------------------------------------------- | ------------------------------------------------------------------------------------------------------------- | -------------------------------------------------------------------------------------------------------------------------------- | ------------------------------------------------------------------------------------ | ----------- | ------------ | -| 1 | `PRRT_kwDOGp2yqc6SitOP` | `docs/issues/open/1875-review-lto-fat-in-dev-profile/ISSUE.md` | [comment](https://github.com/torrust/torrust-tracker/pull/2013#discussion_r3621393047) | Update stale references to the standalone issue-spec path. | action — updated the EPIC's direct references and migrated the open-issues naming convention to folder specs. | [reply](https://github.com/torrust/torrust-tracker/pull/2013#discussion_r3621442150) | DONE | RESOLVED | -| 2 | `PRRT_kwDOGp2yqc6Si2c6` | `docs/pr-reviews/pr-2013-copilot-suggestions.md` | [comment](https://github.com/torrust/torrust-tracker/pull/2013#discussion_r3621444815) | Add `` to avoid spell-check failures on opaque thread IDs. | action — added `` to the tracker file and to the template so future PR trackers include it automatically. | [reply](https://github.com/torrust/torrust-tracker/pull/2013#discussion_r3621514178) | DONE | RESOLVED | -| 3 | `PRRT_kwDOGp2yqc6SjBkm` | `docs/pr-reviews/pr-2013-copilot-suggestions.md` | [comment](https://github.com/torrust/torrust-tracker/pull/2013#discussion_r3621507603) | Workflow section missing the explicit reply-before-resolve step. | action — added the reply step to the workflow list in this file to match the template and skill. | [reply](https://github.com/torrust/torrust-tracker/pull/2013#discussion_r3621586425) | DONE | RESOLVED | -| 4 | `PRRT_kwDOGp2yqc6Sjapq` | `docs/pr-reviews/pr-2013-copilot-suggestions.md` | [comment](https://github.com/torrust/torrust-tracker/pull/2013#discussion_r3621648952) | Missing blank line before step 4 in Workflow list causes unreliable rendering. | action — added blank line before step 4. | [reply](https://github.com/torrust/torrust-tracker/pull/2013#discussion_r3621692387) | DONE | RESOLVED | -| 5 | `PRRT_kwDOGp2yqc6Sj3Ce` | `.github/skills/dev/pr-reviews/resolve-review-threads/scripts/reply-to-thread.sh` | [comment](https://github.com/torrust/torrust-tracker/pull/2013#discussion_r3621808021) | `gh api graphql --jq` can return empty/`null` while exiting 0; validate `REPLY_URL` before reporting success. | action — added guard after GraphQL call: exit 1 if `REPLY_URL` is empty or `null`. | | IN-PROGRESS | OPEN | -| 6 | `PRRT_kwDOGp2yqc6Sj3Cy` | `.github/skills/dev/pr-reviews/fetch-review-threads/scripts/check-thread-reply-status.sh` | [comment](https://github.com/torrust/torrust-tracker/pull/2013#discussion_r3621808055) | `printf` with `%s` emits `"null"` string when url is JSON null; use `jq -n`/`--argjson` for correct types. | action — replaced `printf` with `jq -n --argjson` to preserve JSON types. | | IN-PROGRESS | OPEN | -| 7 | `PRRT_kwDOGp2yqc6Sj3DE` | `.github/skills/dev/pr-reviews/resolve-review-threads/scripts/reply-and-resolve-thread.sh` | [comment](https://github.com/torrust/torrust-tracker/pull/2013#discussion_r3621808078) | GraphQL mutation can return empty/`null` URL while exiting 0; validate before proceeding to resolve. | action — added guard after GraphQL call: exit 1 if `REPLY_URL` is empty or `null`. | | IN-PROGRESS | OPEN | +| # | Thread ID | Path | URL | Suggestion Summary | Decision | Reply URL | Status | Thread State | +| --- | ----------------------- | ------------------------------------------------------------------------------------------ | -------------------------------------------------------------------------------------- | ------------------------------------------------------------------------------------------------------------- | -------------------------------------------------------------------------------------------------------------------------------- | ------------------------------------------------------------------------------------ | ------ | ------------ | +| 1 | `PRRT_kwDOGp2yqc6SitOP` | `docs/issues/open/1875-review-lto-fat-in-dev-profile/ISSUE.md` | [comment](https://github.com/torrust/torrust-tracker/pull/2013#discussion_r3621393047) | Update stale references to the standalone issue-spec path. | action — updated the EPIC's direct references and migrated the open-issues naming convention to folder specs. | [reply](https://github.com/torrust/torrust-tracker/pull/2013#discussion_r3621442150) | DONE | RESOLVED | +| 2 | `PRRT_kwDOGp2yqc6Si2c6` | `docs/pr-reviews/pr-2013-copilot-suggestions.md` | [comment](https://github.com/torrust/torrust-tracker/pull/2013#discussion_r3621444815) | Add `` to avoid spell-check failures on opaque thread IDs. | action — added `` to the tracker file and to the template so future PR trackers include it automatically. | [reply](https://github.com/torrust/torrust-tracker/pull/2013#discussion_r3621514178) | DONE | RESOLVED | +| 3 | `PRRT_kwDOGp2yqc6SjBkm` | `docs/pr-reviews/pr-2013-copilot-suggestions.md` | [comment](https://github.com/torrust/torrust-tracker/pull/2013#discussion_r3621507603) | Workflow section missing the explicit reply-before-resolve step. | action — added the reply step to the workflow list in this file to match the template and skill. | [reply](https://github.com/torrust/torrust-tracker/pull/2013#discussion_r3621586425) | DONE | RESOLVED | +| 4 | `PRRT_kwDOGp2yqc6Sjapq` | `docs/pr-reviews/pr-2013-copilot-suggestions.md` | [comment](https://github.com/torrust/torrust-tracker/pull/2013#discussion_r3621648952) | Missing blank line before step 4 in Workflow list causes unreliable rendering. | action — added blank line before step 4. | [reply](https://github.com/torrust/torrust-tracker/pull/2013#discussion_r3621692387) | DONE | RESOLVED | +| 5 | `PRRT_kwDOGp2yqc6Sj3Ce` | `.github/skills/dev/pr-reviews/resolve-review-threads/scripts/reply-to-thread.sh` | [comment](https://github.com/torrust/torrust-tracker/pull/2013#discussion_r3621808021) | `gh api graphql --jq` can return empty/`null` while exiting 0; validate `REPLY_URL` before reporting success. | action — added guard after GraphQL call: exit 1 if `REPLY_URL` is empty or `null`. | [reply](https://github.com/torrust/torrust-tracker/pull/2013#discussion_r3621845797) | DONE | RESOLVED | +| 6 | `PRRT_kwDOGp2yqc6Sj3Cy` | `.github/skills/dev/pr-reviews/fetch-review-threads/scripts/check-thread-reply-status.sh` | [comment](https://github.com/torrust/torrust-tracker/pull/2013#discussion_r3621808055) | `printf` with `%s` emits `"null"` string when url is JSON null; use `jq -n`/`--argjson` for correct types. | action — replaced `printf` with `jq -n --argjson` to preserve JSON types. | [reply](https://github.com/torrust/torrust-tracker/pull/2013#discussion_r3621847390) | DONE | RESOLVED | +| 7 | `PRRT_kwDOGp2yqc6Sj3DE` | `.github/skills/dev/pr-reviews/resolve-review-threads/scripts/reply-and-resolve-thread.sh` | [comment](https://github.com/torrust/torrust-tracker/pull/2013#discussion_r3621808078) | GraphQL mutation can return empty/`null` URL while exiting 0; validate before proceeding to resolve. | action — added guard after GraphQL call: exit 1 if `REPLY_URL` is empty or `null`. | [reply](https://github.com/torrust/torrust-tracker/pull/2013#discussion_r3621849094) | DONE | RESOLVED | ## Notes