diff --git a/.github/skills/dev/planning/cleanup-completed-issues/SKILL.md b/.github/skills/dev/planning/cleanup-completed-issues/SKILL.md index c92a3ff6d..25d660906 100644 --- a/.github/skills/dev/planning/cleanup-completed-issues/SKILL.md +++ b/.github/skills/dev/planning/cleanup-completed-issues/SKILL.md @@ -3,7 +3,7 @@ name: cleanup-completed-issues description: Guide for archiving closed issue specification files from docs/issues/open/ to docs/issues/closed/. Covers verifying closure on GitHub, moving files, updating frontmatter, auditing and repairing affected documentation links, creating a branch, and opening a PR. Permanent deletion of closed specs is not automated — the user must explicitly request it. Use when cleaning up closed issue specs, archiving issue docs, or maintaining the docs/issues/ folder. Triggers on "cleanup issue", "archive issue", "move closed issue", "clean completed issues", or "maintain issue docs". metadata: author: torrust - version: "1.10" + version: "1.11" --- # Cleaning Up Completed Issues @@ -25,6 +25,8 @@ Related lifecycle docs: - Open issue specs: [`docs/issues/open/README.md`](../../../../../docs/issues/open/README.md) - Closed issue buffer: [`docs/issues/closed/README.md`](../../../../../docs/issues/closed/README.md) +- Reopening an archived issue or EPIC: [reopen-issue](../reopen-issue/SKILL.md) + ## When to Archive diff --git a/.github/skills/dev/planning/reopen-issue/SKILL.md b/.github/skills/dev/planning/reopen-issue/SKILL.md new file mode 100644 index 000000000..0b6a54084 --- /dev/null +++ b/.github/skills/dev/planning/reopen-issue/SKILL.md @@ -0,0 +1,149 @@ +--- +name: reopen-issue +description: Guide for reopening a closed GitHub issue or EPIC whose specification was archived in docs/issues/closed/. Covers deciding whether to reopen or file new work, reopening on GitHub, linking a new subissue, moving the spec back to docs/issues/open/, migrating legacy frontmatter, resetting closure checkpoints, and repairing live references. Use when new work belongs to a closed EPIC or a closed issue regressed or was incomplete. Triggers on "reopen issue", "reopen EPIC", "add subissue to closed EPIC", "unarchive issue spec", or "move spec back to open". +metadata: + author: torrust + version: "1.1" +--- + + + +# Reopening a Closed Issue or EPIC + +This is the inverse of [cleanup-completed-issues](../cleanup-completed-issues/SKILL.md). It was +first used to reopen configuration EPIC #1978 for subissue #2466 (2026-10-07). The EPIC path is +exercised; the single-issue path is derived from it and should be refined on first use. + +## Step 1: Decide Whether to Reopen + +Reopen only with explicit maintainer approval; reopening changes shared GitHub state. + +Reopen when the new work belongs to the original goal and its deliverable is not yet released (for +example, #1978's configuration schema v3.0.0 was active but not published on crates.io). Otherwise +create a new issue or EPIC and reference the closed one. + +For a closed single issue, reopen when the closing PR did not meet its acceptance criteria or the +fix regressed before release. A regression after release is a new bug issue. + +## Step 2: Prepare the Branch + +Update `develop` first, check that it is not behind, and only then create the branch. Run the +behind count as its own step and read it: a skipped or failed `git pull --ff-only` leaves `develop` +behind, and only the count shows it. Chained with `&&`, `git checkout -b` runs even when the count +is non-zero. + +```bash +UPSTREAM_REMOTE="${UPSTREAM_REMOTE:-torrust}" +git checkout develop +git pull --ff-only "$UPSTREAM_REMOTE" develop +git rev-list --count HEAD.."$UPSTREAM_REMOTE"/develop # must print 0 +git checkout -b +``` + +When reopening an EPIC to add a subissue, reuse that subissue's specification branch +(`---spec`), so the reopen, the subissue spec, and any ADR land in one PR. + +## Step 3: Update GitHub + +1. When adding a subissue, create it first (see [create-issue](../create-issue/SKILL.md)), so the + reopen comment can cite it. +2. Reopen with the reason: + + ```bash + gh issue reopen --repo torrust/torrust-tracker --comment "Reopened to ." + ``` + +3. Link a new subissue natively; the REST endpoint needs the child's database `id`, not its number: + + ```bash + CHILD_ID=$(gh api repos/torrust/torrust-tracker/issues/ --jq .id) + gh api -X POST repos/torrust/torrust-tracker/issues//sub_issues -F sub_issue_id="$CHILD_ID" + ``` + +4. Check the reopened issue's body for a `Specification` link and point it at the `open/` path. + Repository searches do not see issue bodies, and older bodies may still name a legacy + single-file path: + + ```bash + gh issue view --repo torrust/torrust-tracker --json body --jq .body > .tmp/issue--body.md + # edit the link, then: + gh issue edit --repo torrust/torrust-tracker --body-file .tmp/issue--body.md + ``` + +## Step 4: Move the Specification Back + +```bash +git mv docs/issues/closed/ docs/issues/open/ +``` + +Then update the moved spec: + +- **Frontmatter**: set `status` to `planned` or `in-progress`, `spec-path` to the `open/` path, and + `last-updated-utc` to the current time. Specs archived before the v1 schema must be migrated to + v1 in `open/` (`schema-version: 1`, `epic: null` where applicable, quoted `last-updated-utc`, no + trailing slash in `related-artifacts`). Follow the migration checklist in + [`contrib/dev-tools/checks/frontmatter-validator/README.md`](../../../../../contrib/dev-tools/checks/frontmatter-validator/README.md). +- **Archive-time edits**: revert body paths the archive rewrote to `closed/`, such as the + "spec drafted in" checkpoint. +- **Checkpoints**: clear the "issue closed and spec moved" and "acceptance criteria reviewed" boxes. +- **Acceptance Verification**: set rows that depended on all work being done back to `TODO`, clear + the matching Acceptance Criteria checkboxes (for example, "Epic status reflects actual state of + linked subissues"), and correct stale counts (for example, the number of linked subissues). +- **Subissues table** (EPIC): add the new row with `TODO`. A `related-artifacts` entry must name a + tracked file, so add the new child spec path in the commit that adds the spec. +- **Progress Log**: add an entry with the reason and the remaining work. Date each entry at the + action it records: the maintainer's decision, the GitHub reopen, and the move are separate + events. +- **References**: add the new subissue, the issue it came from, and any new ADR. +- **Parent EPIC** (single issue): set its row back to `IN_PROGRESS` and point it at `open/`. + +The frontmatter validator checks **staged** content. Stage the edits, not only the rename, before +running it: + +```bash +git add -A docs/issues/open/ +cargo run --quiet --package frontmatter-validator --bin frontmatter-validator -- --staged +``` + +## Step 5: Repair Live References + +```bash +rg '' --glob '!target/**' --glob '!storage/**' +``` + +- Update live navigational references outside `docs/issues/closed/` (for example, package docs) to + the `open/` path. +- Leave links inside other closed specs and historical PR review records unchanged. They are + archived records, the [local link checker](../../../../../lychee.toml) excludes + `docs/issues/closed/`, and the spec returns + there on closure. + +When the reopened issue closes again, archive it with +[cleanup-completed-issues](../cleanup-completed-issues/SKILL.md); its reference search flips the +live references back. + +## Step 6: Commit and Open the PR + +Commit the reopen separately from the new subissue spec. When the reopened spec references a new +ADR or spec in `related-artifacts`, commit that artifact first, because the validator requires +tracked paths. + +```bash +git commit -S -m "docs(issues): [#] reopen to " +``` + +Use `Related to #` in the PR body, never a closing keyword. + +## Skill Links + +Artifacts this skill depends on; the first three carry a `skill-link: reopen-issue` marker: + +- [`cleanup-completed-issues`](../cleanup-completed-issues/SKILL.md): the inverse workflow; a + change to how specs are archived changes what a reopen must undo. +- [`frontmatter-validator/README.md`](../../../../../contrib/dev-tools/checks/frontmatter-validator/README.md): + the v1 migration checklist used in Step 4. +- [`lychee.toml`](../../../../../lychee.toml): the `docs/issues/closed/` exclusion that Step 5 + relies on. +- [`docs/templates/EPIC.md`](../../../../../docs/templates/EPIC.md) and + [`docs/templates/ISSUE.md`](../../../../../docs/templates/ISSUE.md): the checkpoint wording reset + in Step 4. They carry no marker, because specs copy template markers. diff --git a/contrib/dev-tools/checks/frontmatter-validator/README.md b/contrib/dev-tools/checks/frontmatter-validator/README.md index 1cdfc6ac7..c69b2aae9 100644 --- a/contrib/dev-tools/checks/frontmatter-validator/README.md +++ b/contrib/dev-tools/checks/frontmatter-validator/README.md @@ -84,6 +84,8 @@ A staged edit to a draft or open `ISSUE.md` or `EPIC.md` without v1 frontmatter Re-run the validator on the file until it exits `0`. + + ## Temporary Placement This crate is approved early work under EPIC #2003, which owns the long-term automation diff --git a/docs/adrs/20261007082938_bound_protocol_agnostic_values_by_the_tightest_delivery_protocol.md b/docs/adrs/20261007082938_bound_protocol_agnostic_values_by_the_tightest_delivery_protocol.md new file mode 100644 index 000000000..ff9153b0b --- /dev/null +++ b/docs/adrs/20261007082938_bound_protocol_agnostic_values_by_the_tightest_delivery_protocol.md @@ -0,0 +1,124 @@ +--- +semantic-links: + skill-links: + - create-adr + related-artifacts: + - "issue #1978" + - "issue #2245" + - packages/primitives/src/announce.rs + - docs/adrs/20260723184019_separate_configuration_value_invariants_from_consistency_validation.md + - docs/adrs/20260721100000_use_newtypes_for_constrained_configuration_field_types.md +--- + + + +# Bound Protocol-Agnostic Values by the Tightest Delivery Protocol + +## Scope + +Repository-level. The decision spans the configuration schema, the domain types in `primitives`, +and every delivery-protocol package (`udp-server`, `http-protocol`, `axum-http-server`), so it +belongs in `docs/adrs/`. + +## Description + +The tracker serves the same swarm through several delivery protocols: UDP (BEP 15), HTTP (BEP 3 +and BEP 23), and possibly others later, such as WebTorrent. Some configured values are sent to +clients by every protocol, but each protocol encodes them differently. The announce interval is the +first case: BEP 15 encodes it as a signed 32-bit integer, while HTTP encodes it as a bencode +integer (`i64` in `http-protocol`). + +The configuration and domain types held the interval as a plain `u32`, so a value above +`i32::MAX` was accepted. Before #2245, UDP sent such a value as a negative number. #2245 clamped +it on the UDP wire, which hid the problem: UDP and HTTP clients then received different intervals +for the same configuration. + +The [value-invariant ADR](20260723184019_separate_configuration_value_invariants_from_consistency_validation.md) +says *how* to reject a single-value bound: a typed newtype, rejected during deserialization. It +does not say *which* bound to choose when the delivery protocols disagree, or where such a type +lives. + +## Agreement + +A configuration value that every delivery protocol sends to clients is **one protocol-agnostic +value**, not one value per protocol. + +Its bound is the **tightest limit among the supported delivery protocols**. For the announce +interval that is `i32::MAX` seconds (about 68 years), from BEP 15; realistic values are minutes to +hours. + +The bound is a domain rule (the value must be representable on every supported protocol), so it is +enforced by a validated newtype in `primitives`. The configuration uses that domain type directly, +as it already uses `AnnouncePolicy`. Following the value-invariant ADR, construction and +`Deserialize` reject out-of-range values, so an invalid configuration fails at load. + +Delivery adapters then convert the value to their wire type without clamping or failing. Clamping +remains appropriate only for values that no configuration bounds, such as peer counts. + +When a new delivery protocol has a tighter limit, the shared bound is lowered. That is a breaking +configuration change and must be released as one. + +This rule covers values the operator configures once and the tracker sends through every protocol. +It does not cover limits on what clients send, which differ per protocol for their own reasons; see +[Cap scrape info hashes per protocol](20261005124222_cap_scrape_info_hashes_per_protocol.md). + +## Alternatives Considered + +### One configuration value per delivery protocol + +Rejected. It duplicates a single concept across protocols and lets operators configure different +intervals for the same swarm by accident. + +### Validate the shared value per delivery protocol + +Rejected. A value could be valid for HTTP and invalid for UDP, so enabling a protocol would turn a +working configuration into an invalid one. It also needs consistency rules that depend on which +protocols are enabled. + +### Clamp on the wire only + +Rejected. This was the state after #2245. It silently changes the configured value for some +clients, and the protocols then disagree. + +### An arbitrary "sensible" cap + +Rejected. A cap such as one day has no source to derive it from, and choosing it is a separate +policy decision. The protocol limit is objective. + +### A bounded schema type only in `configuration` + +Rejected. It keeps `primitives` unaware of the bound, but the domain type stays unbounded, so the +delivery adapters still need a fallible conversion or a clamp. + +## Consequences + +- **Positive**: one unambiguous value per concept; all delivery protocols report the same value. +- **Positive**: invalid configurations fail at load with a clear error instead of being clamped. +- **Positive**: delivery adapters convert infallibly, with no clamp to keep in sync. +- **Negative**: the bound is lower than some protocols need. For the announce interval this costs + nothing in practice. +- **Negative**: adding a protocol with a tighter limit is a breaking configuration change. +- **Negative**: changing a `primitives` field type is a breaking public API change for its + consumers. + +## Affected Code + +- [`packages/primitives/src/announce.rs`](../../packages/primitives/src/announce.rs): + `AnnouncePolicy::interval` and `interval_min`, which #2466 changes to the bounded type. +- [`packages/udp-server/src/handlers/announce.rs`](../../packages/udp-server/src/handlers/announce.rs): + the UDP response encodes the interval through `saturating_wire_i32` until #2466 converts it + without clamping. + +Issue #2466 adds module-level doc comments in both places that link back to this ADR. + +## Date + +2026-10-07 + +## References + +- [Configuration Overhaul EPIC #1978](https://github.com/torrust/torrust-tracker/issues/1978) +- [Issue #2245](https://github.com/torrust/torrust-tracker/issues/2245) — review numeric protocol wire conversions (introduced the UDP clamp) +- [BEP 15: UDP Tracker Protocol](https://www.bittorrent.org/beps/bep_0015.html) +- [Separate Configuration Value Invariants from Consistency Validation](20260723184019_separate_configuration_value_invariants_from_consistency_validation.md) +- [Use Newtypes for Domain-Constrained Configuration Field Types](20260721100000_use_newtypes_for_constrained_configuration_field_types.md) diff --git a/docs/adrs/index.md b/docs/adrs/index.md index fdec42a34..5e348d725 100644 --- a/docs/adrs/index.md +++ b/docs/adrs/index.md @@ -48,6 +48,7 @@ supersession rules. | [20261002173716](20261002173716_load_persisted_scrape_downloads_with_a_batched_uncached_lookup.md) | 2026-10-02 | Load persisted scrape downloads with a batched, uncached lookup | Announce and scrape share one persisted-downloads lookup; scrape loads all torrents absent from memory with one deduplicated `IN (...)` query, bounded at 100 info-hashes independently of protocol request limits, with no cache until metrics show a need. | | [20261005124222](20261005124222_cap_scrape_info_hashes_per_protocol.md) | 2026-10-05 | Cap scrape info hashes per protocol, each for its own reason | UDP and HTTP parsers each keep the first N scrape info hashes and ignore the rest: UDP 74, computed from the packet size; HTTP 100, an abuse-mitigation policy value. Core has no cap, and a per-request cap is not a substitute for rate limiting. | | [20261005145329](20261005145329_return_result_only_for_concretely_fallible_public_apis.md) | 2026-10-05 | Return `Result` only for concretely fallible public APIs | A public API returns `Result` only when it can fail today, crosses an I/O boundary, or is a trait or port with an existing or planned backend that can fail; otherwise it returns a plain value and a real failure later is a semver-signalled breaking change. No `Infallible` or empty error enums as placeholders; existing public error enums get `#[non_exhaustive]` before first publish. | +| [20261007082938](20261007082938_bound_protocol_agnostic_values_by_the_tightest_delivery_protocol.md) | 2026-10-07 | Bound protocol-agnostic values by the tightest delivery protocol | A configured value that every delivery protocol sends (such as the announce interval) is one value, bounded by the tightest supported protocol (`i32::MAX` from BEP 15) through a validated `primitives` newtype rejected at configuration load; adapters convert without clamping. Per-protocol values, per-protocol validation, and wire-only clamping are rejected. | ## ADR Lifecycle diff --git a/docs/issues/README.md b/docs/issues/README.md index 12d90092e..667f38132 100644 --- a/docs/issues/README.md +++ b/docs/issues/README.md @@ -30,3 +30,4 @@ Use these skills as the authoritative process definitions: - Create and maintain issue specs: [`.github/skills/dev/planning/create-issue/SKILL.md`](../../.github/skills/dev/planning/create-issue/SKILL.md) - Close and archive completed specs: [`.github/skills/dev/planning/cleanup-completed-issues/SKILL.md`](../../.github/skills/dev/planning/cleanup-completed-issues/SKILL.md) +- Reopen an archived issue or EPIC: [`.github/skills/dev/planning/reopen-issue/SKILL.md`](../../.github/skills/dev/planning/reopen-issue/SKILL.md) diff --git a/docs/issues/closed/README.md b/docs/issues/closed/README.md index 05fe7ff9d..63e9e6403 100644 --- a/docs/issues/closed/README.md +++ b/docs/issues/closed/README.md @@ -34,3 +34,4 @@ The authoritative procedure is the cleanup workflow skill below. - Issues index: [../README.md](../README.md) - Cleanup workflow source of truth: [`.github/skills/dev/planning/cleanup-completed-issues/SKILL.md`](../../../.github/skills/dev/planning/cleanup-completed-issues/SKILL.md) +- Reopen workflow (moves a spec back to `open/`): [`.github/skills/dev/planning/reopen-issue/SKILL.md`](../../../.github/skills/dev/planning/reopen-issue/SKILL.md) diff --git a/docs/issues/closed/1978-configuration-overhaul-epic/EPIC.md b/docs/issues/open/1978-configuration-overhaul-epic/EPIC.md similarity index 94% rename from docs/issues/closed/1978-configuration-overhaul-epic/EPIC.md rename to docs/issues/open/1978-configuration-overhaul-epic/EPIC.md index 9d4e29362..01a574b94 100644 --- a/docs/issues/closed/1978-configuration-overhaul-epic/EPIC.md +++ b/docs/issues/open/1978-configuration-overhaul-epic/EPIC.md @@ -1,15 +1,17 @@ --- +schema-version: 1 doc-type: epic -status: done +status: in-progress +epic: null github-issue: 1978 -spec-path: docs/issues/closed/1978-configuration-overhaul-epic/EPIC.md +spec-path: docs/issues/open/1978-configuration-overhaul-epic/EPIC.md epic-owner: josecelano -last-updated-utc: 2026-09-01 10:27 +last-updated-utc: "2026-10-07 10:24" semantic-links: skill-links: - create-issue related-artifacts: - - packages/configuration/src/v2_0_0/ + - packages/configuration/src/v2_0_0 - packages/configuration/src/lib.rs - packages/configuration/docs/migrate-v2-to-v3.md - docs/issues/closed/1417-1978-add-public-service-url-to-configuration/ISSUE.md @@ -24,6 +26,8 @@ semantic-links: - docs/issues/closed/2107-1978-activate-persistence-free-v3-runtime-composition/ISSUE.md - docs/issues/closed/2079-adopt-secrecy-for-sensitive-configuration/ISSUE.md - docs/adrs/20260617093046_reject_wildcard_external_ip.md + - docs/adrs/20261007082938_bound_protocol_agnostic_values_by_the_tightest_delivery_protocol.md + - docs/issues/open/2466-1978-announce-interval-upper-bound/ISSUE.md --- # EPIC #1978 - Configuration Overhaul (schema v3.0.0) @@ -108,6 +112,7 @@ Status values: `TODO`, `IN_PROGRESS`, `IN_REVIEW`, `BLOCKED`, `DONE`. | 13 | [#2107](https://github.com/torrust/torrust-tracker/issues/2107) — Activate persistence-free v3 runtime composition | `docs/issues/closed/2107-1978-activate-persistence-free-v3-runtime-composition/ISSUE.md` | DONE | Active v3 composition honors an omitted database while preserving capability-aware REST API routes and configured database-driver startup. | | 14 | [#2023](https://github.com/torrust/torrust-tracker/issues/2023) — Expose configured public URLs in runtime observability | `docs/issues/closed/2023-1978-expose-configured-public-urls-in-runtime-observability/ISSUE.md` | DONE | Implemented with automated and reproducible local runtime verification; evidence is recorded in the issue folder. | | 15 | [#2067](https://github.com/torrust/torrust-tracker/issues/2067) — Analyze a flat heterogeneous service configuration | `docs/issues/closed/2067-1978-analyze-flat-service-configuration/ISSUE.md` | DONE | Analysis rejected a successor flat schema; its confirmed configuration-model bug was resolved by #2083. | +| 16 | [#2466](https://github.com/torrust/torrust-tracker/issues/2466) — Bound announce intervals to what every delivery protocol can encode | `docs/issues/open/2466-1978-announce-interval-upper-bound/ISSUE.md` | TODO | Added after closure (EPIC reopened). Found by #2245: reject `interval` and `interval_min` above `i32::MAX` at configuration load. | ### Release-gated prerequisite @@ -231,7 +236,7 @@ For each subissue implementation in this EPIC, the default completion policy is: ### Workflow Checkpoints -- [x] Epic spec drafted in `docs/issues/closed/1978-configuration-overhaul-epic/EPIC.md` +- [x] Epic spec drafted in `docs/issues/open/1978-configuration-overhaul-epic/EPIC.md` - [x] Epic spec reviewed and approved by user/maintainer - [x] GitHub epic issue created: #1978 - [x] Subissues created and linked in this spec @@ -239,8 +244,8 @@ For each subissue implementation in this EPIC, the default completion policy is: - [x] For each implemented subissue: automatic checks completed and recorded - [x] For each implemented subissue: manual verification completed and recorded - [x] For each implemented subissue: acceptance criteria reviewed post-implementation -- [x] Epic acceptance criteria reviewed and checked off -- [x] Epic issue closed and spec moved from `docs/issues/open/` to `docs/issues/closed/` +- [ ] Epic acceptance criteria reviewed and checked off +- [ ] Epic issue closed and spec moved from `docs/issues/open/` to `docs/issues/closed/` ### Progress Log @@ -318,13 +323,15 @@ For each subissue implementation in this EPIC, the default completion policy is: contract is separately drafted under API EPIC #144. - 2026-08-26 16:45 UTC - GitHub Copilot/User - #1980 runtime activation is in review in draft PR #2103. It activates v3 consumers, all shipped templates, shared UDP policy, logging style, and HTTP query-IP wiring. Automatic checks and deferred #889/#1987 local manual evidence are recorded; the persistence-free activation follow-up remains deferred. - 2026-09-01 10:25 UTC - GitHub Copilot - Verified GitHub's native hierarchy has 16 of 16 subissues complete, confirmed the recorded verification evidence, closed #1978 as completed, and archived this EPIC specification. +- 2026-10-07 08:29 UTC - josecelano - Decided to reopen #1978 for the #2245 follow-up instead of a standalone issue: configuration schema v3.0.0 is not yet published on crates.io, so the change still belongs to this EPIC - Chat decision +- 2026-10-07 08:50 UTC - GitHub Copilot - Created subissue #2466 (bound the announce interval to what every delivery protocol can encode), reopened #1978 on GitHub, and linked #2466 natively; the specification PR then moves this specification back to `docs/issues/open/` - 16 of 17 subissues done ## Acceptance Criteria - [x] All required subissues are created and linked. - [x] Implementation order is explicit and justified. - [x] Dependencies and blockers are documented and current. -- [x] Epic status reflects actual state of linked subissues. +- [ ] Epic status reflects actual state of linked subissues. - [x] Every completed subissue includes automated verification evidence. - [x] Every completed subissue includes manual verification evidence. - [x] Every completed subissue includes post-implementation acceptance criteria review. @@ -334,10 +341,10 @@ For each subissue implementation in this EPIC, the default completion policy is: | AC ID | Status (`TODO`/`DONE`) | Evidence | | ----- | ---------------------- | ----------------------------------------------------------------------------------------------------------------- | -| AC1 | DONE | GitHub EPIC #1978 reports 13 linked subissues in the documented order. | +| AC1 | DONE | GitHub EPIC #1978 reports 17 linked native subissues, including #2466 added after reopening. | | AC2 | DONE | The dependency graph, critical paths, phases, and conflict hotspot table document ordering and rationale. | | AC3 | DONE | The EPIC table, dependency graph, and release-gated #2079 prerequisite record current prerequisites and blockers. | -| AC4 | DONE | The `Subissues` table and progress log record the current status for each linked issue. | +| AC4 | TODO | Reopened for #2466; recheck when it closes. | | AC5 | DONE | The completed subissue specifications record their relevant automated-check evidence. | | AC6 | DONE | The completed subissue specifications record their applicable manual-verification evidence. | | AC7 | DONE | The completed subissue specifications record post-implementation acceptance reviews. | @@ -357,7 +364,7 @@ For each subissue implementation in this EPIC, the default completion policy is: ## References -- Related issues: #1417, #1640, #1490, #999, #1453, #1415, #1136, #889, #1987 +- Related issues: #1417, #1640, #1490, #999, #1453, #1415, #1136, #889, #1987, #2466, #2245 - Related PRs: #1937 (spec for #1640) -- Related ADRs: `docs/adrs/20260617093046_reject_wildcard_external_ip.md` +- Related ADRs: `docs/adrs/20260617093046_reject_wildcard_external_ip.md`, `docs/adrs/20261007082938_bound_protocol_agnostic_values_by_the_tightest_delivery_protocol.md` - Related EPICs: #1669 (package overhaul) diff --git a/docs/issues/open/2243-review-numeric-conversion-boundaries/EPIC.md b/docs/issues/open/2243-review-numeric-conversion-boundaries/EPIC.md index e7a729759..f1e81c198 100644 --- a/docs/issues/open/2243-review-numeric-conversion-boundaries/EPIC.md +++ b/docs/issues/open/2243-review-numeric-conversion-boundaries/EPIC.md @@ -6,7 +6,7 @@ epic: null github-issue: 2243 spec-path: docs/issues/open/2243-review-numeric-conversion-boundaries/EPIC.md epic-owner: josecelano -last-updated-utc: "2026-10-06 16:03" +last-updated-utc: "2026-10-07 08:59" semantic-links: skill-links: - create-issue @@ -71,7 +71,7 @@ Status values: `TODO`, `IN_PROGRESS`, `BLOCKED`, `DONE`. | Order | Issue | Local Spec | Status | Notes | | ----- | ----- | ---------- | ------ | ----- | | 1 | #2244 - Review metric aggregate conversions | `docs/issues/open/2244-2243-review-metric-aggregate-conversions/ISSUE.md` | TODO | Owns 72 metric entries: A080-A087, A113-A114, A143-A154, A170, A178-A222, A224-A227. | -| 2 | #2245 - Review numeric protocol wire conversions | `docs/issues/closed/2245-2243-review-numeric-protocol-wire-conversions/ISSUE.md` | DONE | Owns A156 and A171 only. Follow-up: reject an announce `interval` above `i32::MAX` at configuration load with a typed newtype (see the #2245 Decisions); its specification is drafted next as a separate issue. | +| 2 | #2245 - Review numeric protocol wire conversions | `docs/issues/closed/2245-2243-review-numeric-protocol-wire-conversions/ISSUE.md` | DONE | Owns A156 and A171 only. Follow-up: #2466 (under configuration EPIC #1978) rejects an announce `interval` above `i32::MAX` at configuration load with a typed value. | | 3 | #2246 - Review domain numeric conversions | `docs/issues/closed/2246-2243-review-domain-numeric-conversions/ISSUE.md` | DONE | Owns A099, A123, and A129. | ## Delivery Strategy @@ -127,6 +127,7 @@ For each subissue implementation in this EPIC, the default completion policy is: - 2026-09-16 12:20 UTC - josecelano - Reframed as a review: undocumented exceptions may be legitimate; each conversion is judged on semantics, alternatives, and then documented with a native reason if retained - Chat decision - 2026-10-06 09:10 UTC - GitHub Copilot - #2246 done: PR #2440 merged with all three domain allowances (A099, A123, A129) removed; spec archived to `docs/issues/closed/` - 1 of 3 subissues done; #2244 and #2245 remain TODO - 2026-10-06 16:03 UTC - GitHub Copilot - #2245 done: PR #2452 merged with A156 replaced by a checked conversion mapped to a parse error and A171 clamped to the BEP 15 field width; spec archived to `docs/issues/closed/`. The configuration-load follow-up is not yet specified - 2 of 3 subissues done; #2244 remains TODO +- 2026-10-07 08:59 UTC - GitHub Copilot - The #2245 configuration-load follow-up is #2466, a subissue of the reopened configuration EPIC #1978 rather than of this EPIC - No change to this EPIC's scope ## Acceptance Criteria @@ -159,6 +160,6 @@ For each subissue implementation in this EPIC, the default completion policy is: ## References -- Related issues: #2158 +- Related issues: #2158, #2466 (follow-up of #2245) - Related PRs: #2247 (specification bundle), #2440 (#2246), #2452 (#2245) - Related ADRs: None diff --git a/docs/issues/open/2466-1978-announce-interval-upper-bound/ISSUE.md b/docs/issues/open/2466-1978-announce-interval-upper-bound/ISSUE.md new file mode 100644 index 000000000..73232d648 --- /dev/null +++ b/docs/issues/open/2466-1978-announce-interval-upper-bound/ISSUE.md @@ -0,0 +1,274 @@ +--- +schema-version: 1 +doc-type: issue +issue-type: bug +status: planned +priority: p2 +epic: 1978 +github-issue: 2466 +spec-path: docs/issues/open/2466-1978-announce-interval-upper-bound/ISSUE.md +branch: "2466-1978-announce-interval-upper-bound-spec" +related-pr: null +last-updated-utc: "2026-10-07 13:12" +semantic-links: + skill-links: + - create-issue + related-artifacts: + - docs/adrs/20261007082938_bound_protocol_agnostic_values_by_the_tightest_delivery_protocol.md + - docs/adrs/20260723184019_separate_configuration_value_invariants_from_consistency_validation.md + - docs/adrs/20260721100000_use_newtypes_for_constrained_configuration_field_types.md + - "issue #2245" + - packages/primitives/src/announce.rs + - packages/configuration/docs/migrate-v2-to-v3.md +--- + + + +# Issue #2466 - Bound Announce Intervals to What Every Delivery Protocol Can Encode + +**Parent EPIC:** #1978 - Configuration Overhaul (schema v3.0.0) + +## Goal + +Make `core.announce_policy.interval` and `core.announce_policy.interval_min` typed values that +reject anything above `i32::MAX` during deserialization, so the tracker fails fast at configuration +load instead of silently clamping the interval on the UDP wire. + +## Background + +BEP 15 encodes the UDP announce interval as a signed 32-bit integer. HTTP encodes `interval` and +`min interval` as bencode integers (`i64` in `http-protocol`). The domain fields +`primitives::announce::AnnouncePolicy::interval` and `interval_min` are plain `u32` values with no +bound, and the v3 configuration uses `AnnouncePolicy` directly as `Core::announce_policy`. + +Before #2245, a configured `interval = 2147483648` was sent over UDP as `-2147483648` (reproduced +against a local tracker). #2245 (PR #2452) fixed the wire defect by clamping the value to +`i32::MAX` through `saturating_wire_i32` in `udp-server`. The configuration is still accepted, so +UDP clients receive `2147483647` while HTTP announces report the configured `2147483648` +(reproduced on 2026-10-07; see `manual-verification-evidence.md` section V0). + +The #2245 plan first rejected the value in `Core::validate`. That conflicts with the configuration +validation ADR, which classifies a single-value bound as a **value invariant**: it must be a typed +newtype rejected during deserialization, and one-field rules must not be added to `Validator`. See +the [#2245 issue](https://github.com/torrust/torrust-tracker/issues/2245), its specification, and +its implementation retrospective. + +## Decisions + +Recorded with the maintainer on 2026-10-07: + +1. **One protocol-agnostic value, bounded by the tightest delivery protocol.** The configuration + keeps a single interval value for every delivery protocol (HTTP, UDP, and future ones such as + WebTorrent). Its bound is the tightest limit among the supported protocols: `i32::MAX` seconds + (about 68 years), from BEP 15. Realistic intervals are minutes to hours, so only nonsensical + values are rejected. Per-protocol configuration values and per-protocol validation were rejected + because a value could then be valid for one protocol and invalid for another. +2. **The bounded type lives in `primitives`.** Because the bound is a domain rule (an interval + must be representable on every supported protocol), the type belongs to the domain and is used + directly by the configuration, as `AnnouncePolicy` already is. `interval_min` uses the same type + because it measures the same real-world quantity as `interval`, a re-announce interval in + seconds. No protocol imposes the bound on it: BEP 15 has no minimum-interval field, and only + HTTP sends `min interval`. +3. **Parent EPIC.** Configuration EPIC #1978 is reopened and this issue is added as its subissue. + Configuration schema v3.0.0 is active in the code but not yet published on crates.io. +4. **Breaking change accepted.** Changing the `AnnouncePolicy` field types is acceptable: the + tracker binary and library are heading for the 4.0.0 major release, and the other workspace + packages version independently. +5. **Error message.** The error names the field, the configured value, and the limit + (`2147483647`). +6. **ADR.** The rule in decision 1 is recorded as a repository-wide ADR, because it will apply to + other numeric configuration values and future delivery protocols. + +## Scope + +### In Scope + +- A bounded interval type in `primitives` whose construction and `Deserialize` reject values above + `i32::MAX`, following the value-invariant ADR and the constrained-field newtype ADR. +- Changing `AnnouncePolicy::interval` and `AnnouncePolicy::interval_min` to that type and updating + their consumers (`tracker-core`, `http-protocol`, `axum-http-server`, `udp-server`, + `configuration`, and test or console consumers). +- Converting the UDP interval through the type without clamping. `saturating_wire_i32` remains for + the seeder and leecher counts, which no configuration bounds. +- Module-level doc comments on the bounded type and the UDP interval conversion that link back to + the ADR. +- Updating `packages/configuration/docs/migrate-v2-to-v3.md`, as the parent EPIC requires for a + configuration public API change: a quick-reference row and the new bound on + `[core.announce_policy]`. + +### Out of Scope + +- An `interval_min <= interval` consistency rule; that would be a separate `Validator` rule. +- Other changes to the v2 configuration schema. v2 shares `AnnouncePolicy` with v3, so it inherits + the bound; this reaches only library users of `v2_0_0`, because the tracker no longer loads v2 + at runtime. +- Seeder and leecher count clamping, which #2245 owns. +- Applying the ADR rule to other configuration values; later issues do that when they touch them. + +## Architectural Decisions + +- Related ADRs: + - [Separate configuration value invariants from consistency validation](../../../adrs/20260723184019_separate_configuration_value_invariants_from_consistency_validation.md) + - [Use newtypes for constrained configuration field types](../../../adrs/20260721100000_use_newtypes_for_constrained_configuration_field_types.md) +- ADRs to create: [Bound protocol-agnostic values by the tightest delivery protocol](../../../adrs/20261007082938_bound_protocol_agnostic_values_by_the_tightest_delivery_protocol.md), + added in the specification PR so the rule is reviewed before implementation. + +## Design and Ownership Review + +Not applicable: a value type change with no child processes, asynchronous I/O, network readiness, +resource cleanup, or reusable test fixtures. + +## Bug-Fix Process + +Follows [fix-bug](../../../../.github/skills/dev/debugging/fix-bug/SKILL.md): + +- Analysis: the configuration accepts an interval that cannot be represented on the UDP wire. + After #2245 the UDP reply is clamped, so the remaining defect is silent acceptance and the + divergence between UDP and HTTP intervals. +- Reproduction: **Reproduced** before review (2026-10-07). With `interval = 2147483648` the + tracker starts, the UDP announce returns `2147483647` (clamped), and the HTTP announce returns + `2147483648`. See `manual-verification-evidence.md` section V0. +- Regression test boundary: configuration-load tests, written first and recorded red against the + current `u32` fields (T2); then construction and deserialization of the bounded type (unit tests + at `i32::MAX` and `i32::MAX + 1`). + +## Regression Test Strategy + +Configuration-load tests are the regression tests: loading a TOML with `interval = 2147483648` or +`interval_min = 2147483648` must fail with an error naming the field, the value (`2147483648`), +and the limit (`2147483647`). They are written first and fail against the current `u32` fields; +the red run is recorded in the evidence file before the fix (T2), and the green run after it (T7). +Unit tests on the bounded type cover `2147483647` accepted and `2147483648` rejected, both through +construction and through `Deserialize`. + +## Implementation Plan + +Status values: `TODO`, `IN_PROGRESS`, `BLOCKED`, `DONE`. + +| ID | Status | Task | Notes / Expected Output | +| -- | ------ | ---- | ----------------------- | +| T1 | DONE | Resolve the open questions | Decisions section; ADR added in the specification PR. | +| T2 | TODO | Write the configuration-load regression tests | Assert field, value, and limit for `interval` and `interval_min`. Expected output: the recorded red run against the `u32` fields. | +| T3 | TODO | Add the bounded type to `primitives` | Construction and `Deserialize` reject values above `i32::MAX`; boundary unit tests; module doc links the ADR. | +| T4 | TODO | Fix: adopt it for `interval` and `interval_min` | Consumers updated; the T2 tests pass. | +| T5 | TODO | Convert the UDP interval through the type | No clamp for the interval; `saturating_wire_i32` kept for peer counts; doc comment links the ADR. | +| T6 | TODO | Update the migration guide | Quick-reference row and the bound on `[core.announce_policy]` in `migrate-v2-to-v3.md`. | +| T7 | TODO | Green run and recheck | T2 tests green; M1 "after", M2, and M3 recorded in the evidence file. | + +## Commit Points + +| Task | Coherent change set | Commit policy | +| ---- | ------------------- | ------------- | +| T3 | Bounded type and its boundary unit tests | Commit after focused validation and test-design review. | +| T2 + T4 | Regression tests, field type change, consumer updates | One commit: the red tests cannot pass the pre-push gate alone. The red run stays in the evidence file. | +| T5 | UDP interval conversion | Commit after focused validation; may merge into T2 + T4 if the type change forces it. | +| T6 | Migration guide update | Separate documentation commit. | +| T7 | Evidence and spec progress | Separate documentation commit. | + +Tests follow the `write-unit-test` skill, with the prose-first Arrange-Act-Assert review after each +passing increment and before maintainer review and commit. Commits are signed Conventional Commits +with the narrow affected scope. + +## Progress Tracking + +### Workflow Checkpoints + +- [x] Folder-style spec drafted in `docs/issues/drafts/1978-announce-interval-upper-bound/ISSUE.md` +- [x] Spec reviewed and approved by user/maintainer +- [x] GitHub issue created and issue number added to this spec +- [ ] Spec-only PR merged into `develop` before implementation +- [ ] Implementation completed +- [ ] Automatic verification completed (`linter all`, relevant tests, and any pre-push checks) +- [ ] Manual verification scenarios executed and recorded in issue-local `manual-verification-evidence.md` +- [ ] Acceptance criteria reviewed after implementation and updated with evidence +- [ ] Evidence-based implementation completion review recorded +- [ ] Reviewer validated acceptance criteria and updated checkboxes +- [ ] Independent reviewer reports recorded in issue-local `agent-review-reports.md` when reviewers received this folder-style specification +- [ ] 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-10-06 12:29 UTC - GitHub Copilot - Local draft written from the #2245 follow-up decision - Parked until PR #2452 merged +- 2026-10-06 16:41 UTC - GitHub Copilot - Moved to `docs/issues/drafts/` after #2245 was archived (PR #2455); refreshed facts against `develop`; added Commit Points and Acceptance Verification - Awaiting maintainer review +- 2026-10-07 08:29 UTC - josecelano - Answered the open questions: one protocol-agnostic value bounded by the tightest delivery protocol, type in `primitives` (also for `interval_min`), subissue of reopened #1978, breaking change accepted for 4.0.0, ADR in the specification PR - Chat decision +- 2026-10-07 08:59 UTC - GitHub Copilot - Maintainer approved the specification; created #2466, reopened #1978 and linked #2466 as its subissue; moved this spec to `docs/issues/open/` - Specification PR +- 2026-10-07 10:24 UTC - GitHub Copilot - Applied PR #2468 round-1 review fixes: reproduced the defect before implementation (V0, Reproduced), added the red-run regression task and the migration-guide task, asserted field, value, and limit in tests and scenarios, restored the verification notes and two template checkpoints, scoped the v2 inheritance, and cited #2245 by issue number - In review +- 2026-10-07 13:12 UTC - josecelano - Kept one type for `interval` and `interval_min` because both measure the same quantity, although no protocol bounds `interval_min` (Copilot's review overview had noted the missing protocol justification) - Chat decision + +## Acceptance Criteria + +- [ ] AC1: A configuration with `core.announce_policy.interval` or `interval_min` above + `2147483647` fails to load with an error naming the field, the value, and the limit. +- [ ] AC2: `2147483647` is accepted, and UDP and HTTP announces report the same interval. +- [ ] AC3: The bound is enforced by a type in `primitives`, not by `Validator`, as the ADRs require. +- [ ] AC4: The UDP interval is converted without clamping. +- [ ] AC5: `linter all` exits with code `0` and relevant tests pass. +- [ ] AC6: Manual verification scenarios are executed and documented in issue-local + `manual-verification-evidence.md`. +- [ ] AC7: `packages/configuration/docs/migrate-v2-to-v3.md` documents the new bound. + +## Verification Plan + +### Automatic Checks + +- Bounded-type unit tests and configuration-load tests (stable Rust toolchain). +- Focused Clippy for changed packages, `linter all`, and pre-push checks. + +### Manual Verification Scenarios + +Status values: `TODO`, `IN_PROGRESS`, `DONE`, `FAILED`, `BLOCKED`. + +| ID | Scenario | Human-oriented command/steps | Expected Result | Status | Evidence | +| -- | -------- | ---------------------------- | --------------- | ------ | -------- | +| M1 | Reproduce, then reject an out-of-range interval | Start a local tracker with `interval = 2147483648`, before and after the fix. | Before: starts; UDP reports `2147483647`, HTTP `2147483648`. After: startup fails with an error naming `interval`, the value `2147483648`, and the limit `2147483647`. | IN_PROGRESS (before: DONE) | `manual-verification-evidence.md` sections V0 and V1 | +| M2 | Reject an out-of-range minimum interval | Start with `interval_min = 2147483648`. | Startup fails with an error naming `interval_min`, the value `2147483648`, and the limit `2147483647`. | TODO | `manual-verification-evidence.md` section V2 | +| M3 | Accept the boundary | Start with `interval = 2147483647` and announce over UDP and HTTP. | Both report `2147483647`. | TODO | `manual-verification-evidence.md` section V3 | + +Notes: + +- Manual verification is mandatory even when automated tests pass. It is a real human-oriented + use of the feature or reproduction of the bug fix, not a simulated result and not merely running + automated tests. +- Every recorded validation command result must identify the toolchain or runtime that produced + it when one can affect behavior; M1-M3 record the Rust toolchain that built the tracker and + `tracker_client`. +- `manual-verification-evidence.md` follows `docs/templates/MANUAL-VERIFICATION-EVIDENCE.md`. + Record actual prerequisites, actions, commands, program output, relevant tracker logs, and + outcomes there. +- If a scenario fails, record the failure and diagnosis in the progress log before proceeding. + +### Disposable Verification Scripts + +None planned. The scenarios use the tracker binary and `tracker_client`. + +### Acceptance Verification + +| AC ID | Status (`TODO`/`DONE`) | Evidence | +| ----- | ---------------------- | -------- | +| AC1 | TODO | Configuration-load tests, M1, and M2. | +| AC2 | TODO | Bounded-type unit test and M3. | +| AC3 | TODO | Type definition and absence of a `Validator` rule. | +| AC4 | TODO | UDP response code and M3. | +| AC5 | TODO | `linter all` and test output. | +| AC6 | TODO | `manual-verification-evidence.md`. | +| AC7 | TODO | Migration guide diff. | + +## Risks and Trade-offs + +- A future delivery protocol with a tighter limit lowers the bound for every protocol, which is a + breaking configuration change. The ADR accepts this in exchange for one unambiguous value. +- Rejecting a previously accepted configuration breaks deployments with an absurd interval; the + error message must make the fix obvious. + +## Implementation Completion Review + +- Retrospective: `Not yet assessed`. Create `implementation-retrospective.md` if the type design + changes during implementation or yields a reusable lesson; otherwise record why none is needed + in the Progress Log. + +## References + +- Related issues: #2245 (origin), #2243 (numeric conversion EPIC), #1978 (parent EPIC) +- Related PRs: #2452 (wire clamp), #2455 (#2245 archive) +- Related ADRs: see Architectural Decisions diff --git a/docs/issues/open/2466-1978-announce-interval-upper-bound/manual-verification-evidence.md b/docs/issues/open/2466-1978-announce-interval-upper-bound/manual-verification-evidence.md new file mode 100644 index 000000000..07c543ac6 --- /dev/null +++ b/docs/issues/open/2466-1978-announce-interval-upper-bound/manual-verification-evidence.md @@ -0,0 +1,120 @@ +--- +doc-type: manual-verification-evidence +issue-spec: docs/issues/open/2466-1978-announce-interval-upper-bound/ISSUE.md +last-updated-utc: "2026-10-07 10:18" +--- + +# Manual Verification Evidence + +## Environment and Prerequisites + +- Date and time (UTC): 2026-10-07 10:18 (pre-review defect reproduction). +- Artifact under test: `target/debug/torrust-tracker` and `target/debug/tracker_client`, built + with the stable Rust toolchain `rustc 1.99.0 (b940084d7 2026-09-28)` from `develop` at + `7836471b3` plus this specification PR's documentation-only commits (no code change). +- Operating system / environment: Linux; repository root as current working directory. +- Setup: the configuration below, saved as `.tmp/repro-2466/tracker-over.toml`. + +```toml +[metadata] +app = "torrust-tracker" +purpose = "configuration" +schema_version = "3.0.0" + +[logging] +trace_filter = "info" +trace_style = "full" + +[core] +listed = false +private = false +tracker_usage_statistics = false + +[core.announce_policy] +interval = 2147483648 +interval_min = 120 +max_peers_per_announce = 2 + +[core.database] +driver = "sqlite3" +path = "./.tmp/repro-2466/sqlite3.db" + +[core.tracker_policy] +persistent_torrent_completed_stat = false +remove_peerless_torrents = false + +[udp_tracker_server] +ip_bans_reset_interval_in_secs = 86400 +max_connection_id_errors_per_ip = 10 +connection_id_validation = "strict" + +[[udp_trackers]] +bind_address = "127.0.0.1:16971" + +[[http_trackers]] +bind_address = "127.0.0.1:17071" +``` + +## Verification Processes + +### V0 - Pre-Review Reproduction (M1 "Before") + +- Goal: Observe that the tracker accepts `interval = 2147483648` and that UDP and HTTP announces + report different intervals. +- Initial state: a fresh tracker with an empty SQLite database; one announce per protocol on + info-hash `9c38422213e30bff212b30c360d26f9a02136422`. +- Status: `DONE` +- Outcome: **Reproduced**. + +#### Steps Performed + +1. Started the tracker: + + ```sh + TORRUST_TRACKER_CONFIG_TOML_PATH=.tmp/repro-2466/tracker-over.toml ./target/debug/torrust-tracker \ + > .tmp/repro-2466/tracker-over.log 2>&1 & + ``` + +2. Announced over UDP: + + ```sh + ./target/debug/tracker_client udp announce 127.0.0.1:16971 9c38422213e30bff212b30c360d26f9a02136422 \ + --peer-id=-qB00000000000000001 --port 7001 --event started --left 100 + ``` + +3. Announced over HTTP: + + ```sh + ./target/debug/tracker_client http announce http://127.0.0.1:17071 9c38422213e30bff212b30c360d26f9a02136422 + ``` + +4. Stopped the tracker. + +#### Observed Result + +The tracker started and logged the configured value: + +```text +"interval": 2147483648 +``` + +UDP announce (clamped by #2245): + +```text +{"AnnounceIpv4":{"transaction_id":-888840697,"announce_interval":2147483647,"leechers":1,"seeders":0,"peers":[]}} +``` + +HTTP announce (configured value): + +```text +{"complete":1,"incomplete":1,"interval":2147483648,"min interval":120,"peers":[{"ip":"127.0.0.1","peer id":[45,113,66,48,48,48,48,48,48,48,48,48,48,48,48,48,48,48,48,49],"port":7001}]} +``` + +#### Conclusion + +Reproduced: the configuration is accepted, UDP clients receive `2147483647`, and HTTP clients +receive `2147483648` for the same tracker. M1's "after" half and M2-M3 run after the fix. + +## Failures and Follow-up + +None. diff --git a/docs/pr-reviews/pr-2468-review/PR-REVIEW.md b/docs/pr-reviews/pr-2468-review/PR-REVIEW.md new file mode 100644 index 000000000..eda956bb9 --- /dev/null +++ b/docs/pr-reviews/pr-2468-review/PR-REVIEW.md @@ -0,0 +1,322 @@ +--- +semantic-links: + skill-links: + - process-pr-review + related-artifacts: + - .github/skills/dev/pr-reviews/process-pr-review/SKILL.md +--- + + + +# PR #2468 Review Audit + +Source: pull-request reviews and inline review threads for . + +## Ownership + +The PR author owns this tracked audit record. Reviewers, including repository review agents, +deliver findings through GitHub and have no repository-artifact obligation. + +- Post-merge workflow approval: N/A + +## Status Values + +- Relationship: `ORIGINAL`, `RE_RAISE_OF:` +- Disposition: `FIXED`, `NO_ACTION`, `SUPERSEDED`, `FOLLOW_UP` +- Thread state: `OPEN`, `RESOLVED`, `NON_RESOLVABLE`, `SUPERSEDED`. `OPEN` applies prospectively + to audits created or updated for approved follow-up work; historical audits remain valid without + bulk migration. +- Severity: `Blocker`, `Major`, `Minor`, `Nit`, `Suggestion`; append `(inferred)` when derived + from free prose. +- Author class: `Copilot`, `Human`, `Unknown` +- Category: `link-integrity`, `formatting`, `metadata`, `testing`, `correctness`, + `documentation`, `maintainability`, `security`, `other` +- An outdated thread whose concern was fixed is `FIXED`/`RESOLVED`, even when GitHub marks the + original thread outdated after the push. For in-PR feedback, use `NO_ACTION`/`SUPERSEDED` only + for a duplicate, superseded, or no-change concern. A post-merge `NO_ACTION` requires maintainer + approval to decline the follow-up work. + +## Findings + +Copilot review 5440515841 numbered its four inline findings F5, F6, F9, and F10; they keep those +IDs. Its overview also says the `interval_min` bound lacks protocol justification, but no inline +finding or retrievable suppressed comment carries that assertion, so it has no row (see the +Processing Log). + +Human review 5440619059 (da2ce7, round 1) numbered its findings F1-F11. Its F1-F4, F7, F8, and F11 +keep their IDs; its F5, F6, F9, and F10 collide with Copilot's and are recorded as F12, F13, F14, +and F15, with the reviewer's IDs in each detail entry. + +Human review 5441434837 (da2ce7, round 2) numbered its findings F12 and F13, continuing its own +series; they collide with this audit's F12 and F13 and are recorded as F16 and F17. + +| Finding ID | Review finding reference | Author class | Severity | Category | Relationship | Disposition | Thread state | +| ---------- | ------------------------ | ------------ | -------- | -------- | ------------ | ----------- | ------------ | +| F5 | `review-finding:pr-2468-f5` | Copilot | Major | link-integrity | ORIGINAL | FIXED | RESOLVED | +| F6 | `review-finding:pr-2468-f6` | Copilot | Minor | metadata | ORIGINAL | FIXED | RESOLVED | +| F10 | `review-finding:pr-2468-f10` | Copilot | Minor | maintainability | ORIGINAL | FIXED | RESOLVED | +| F9 | `review-finding:pr-2468-f9` | Copilot | Minor | metadata | ORIGINAL | FIXED | RESOLVED | +| F1 | `review-finding:pr-2468-f1` | Human | Major | testing | ORIGINAL | FIXED | RESOLVED | +| F2 | `review-finding:pr-2468-f2` | Human | Minor | testing | ORIGINAL | FIXED | RESOLVED | +| F3 | `review-finding:pr-2468-f3` | Human | Minor | documentation | ORIGINAL | FIXED | RESOLVED | +| F4 | `review-finding:pr-2468-f4` | Human | Minor | documentation | ORIGINAL | FIXED | RESOLVED | +| F12 | `review-finding:pr-2468-f12` | Human | Minor | metadata | RE_RAISE_OF:F6 | NO_ACTION | SUPERSEDED | +| F13 | `review-finding:pr-2468-f13` | Human | Minor | correctness | ORIGINAL | FIXED | RESOLVED | +| F7 | `review-finding:pr-2468-f7` | Human | Minor | documentation | ORIGINAL | FIXED | RESOLVED | +| F8 | `review-finding:pr-2468-f8` | Human | Suggestion | maintainability | ORIGINAL | FIXED | RESOLVED | +| F14 | `review-finding:pr-2468-f14` | Human | Suggestion | documentation | ORIGINAL | FIXED | RESOLVED | +| F15 | `review-finding:pr-2468-f15` | Human | Suggestion | documentation | ORIGINAL | FIXED | RESOLVED | +| F11 | `review-finding:pr-2468-f11` | Human | Nit | documentation | ORIGINAL | FIXED | RESOLVED | +| F16 | `review-finding:pr-2468-f16` | Human | Minor | formatting | ORIGINAL | FIXED | RESOLVED | +| F17 | `review-finding:pr-2468-f17` | Human | Nit | correctness | ORIGINAL | FIXED | RESOLVED | + +## Finding Details + +### F5 - The reopened issue's Specification link is stale + +- PR number: 2468 +- Source review ID: 5440515841 +- Reviewer finding ID: N/A +- Source URL: +- Concern: The skill's reference search covers repository files only, so it misses the `Specification` link in the #1978 issue body, which named the legacy single file `docs/issues/open/1978-configuration-overhaul-epic.md`. +- Solution: Edited the #1978 issue body to link `docs/issues/open/1978-configuration-overhaul-epic/EPIC.md`, and added a Step 3 item to the skill that checks and edits the reopened issue's `Specification` link. +- Current-tree verification: `gh issue view 1978 --json body` shows the folder path on line 7; `reopen-issue/SKILL.md` Step 3 item 4 contains the `gh issue edit --body-file` command. +- Resolution reference: `docs(skills): [#2466] couple, register, and complete the reopen-issue skill` +- Follow-up PR URL: N/A +- Reply URL: + +### F6 - Unquoted issue markers in the ADR frontmatter + +- PR number: 2468 +- Source review ID: 5440515841 +- Reviewer finding ID: N/A +- Source URL: +- Concern: YAML parses the unquoted `issue #1978` and `issue #2245` values as `issue` followed by a comment. +- Solution: Quoted both values. +- Current-tree verification: The ADR frontmatter lists `"issue #1978"` and `"issue #2245"`. +- Resolution reference: `docs(adrs): [#2466] quote issue markers and add the affected code` +- Follow-up PR URL: N/A +- Reply URL: + +### F10 - The new skill lacks semantic Skill Links coupling + +- PR number: 2468 +- Source review ID: 5440515841 +- Reviewer finding ID: N/A +- Source URL: +- Concern: The skill depends on `cleanup-completed-issues` but has no `## Skill Links` section and no `skill-link: reopen-issue` marker in the dependent artifact. +- Solution: Added a `## Skill Links` section and the skill's own marker. Placed `skill-link: reopen-issue` markers in the cleanup skill, the frontmatter-validator README, and `lychee.toml`. The templates are listed without a marker, because specs copy template markers. +- Current-tree verification: `rg --hidden 'skill-link: reopen-issue'` matches the four marked files (the skill, the cleanup skill, the validator README, and `lychee.toml`), plus this audit, which quotes the marker; the skill ends with the Skill Links section. +- Resolution reference: `docs(skills): [#2466] couple, register, and complete the reopen-issue skill` +- Follow-up PR URL: N/A +- Reply URL: + +### F9 - The spec references #2245 by its lifecycle path + +- PR number: 2468 +- Source review ID: 5440515841 +- Reviewer finding ID: N/A +- Source URL: +- Concern: The spec cited the #2245 spec by its `docs/issues/closed/` path, which goes stale if #2245 moves again; the semantic-link convention asks for `issue #NNNN`. +- Solution: `related-artifacts` lists `"issue #2245"`, and the Background links the #2245 GitHub issue. +- Current-tree verification: `grep 'closed/2245' docs/issues/open/2466-1978-announce-interval-upper-bound/*.md` finds nothing. +- Resolution reference: `docs(issues): [#2466] address round-1 spec review findings` +- Follow-up PR URL: N/A +- Reply URL: + +### F1 - Bug spec reviewed without the pre-review reproduction + +- PR number: 2468 +- Source review ID: 5440619059 +- Reviewer finding ID: N/A +- Source URL: +- Concern: `fix-bug` requires the reproduction before maintainer review, recorded and classified in `manual-verification-evidence.md`; the spec deferred it, and the #2245 evidence shows only the UDP side. +- Solution: Reproduced on 2026-10-07 10:18 UTC against `develop` `7836471b3` plus this PR's documentation commits (stable Rust 1.99.0): the tracker starts, UDP returns `2147483647`, and HTTP returns `2147483648`. Recorded as Reproduced in the new evidence file, section V0, and linked from the Background, the Bug-Fix Process, and M1. +- Current-tree verification: `manual-verification-evidence.md` section V0 holds the configuration and both responses verbatim. +- Resolution reference: `docs(issues): [#2466] address round-1 spec review findings` +- Follow-up PR URL: N/A +- Reply URL: + +### F2 - No red-run regression task; AC1 not matched by the planned tests + +- PR number: 2468 +- Source review ID: 5440619059 +- Reviewer finding ID: N/A +- Source URL: +- Concern: `fix-bug` requires a regression-test task whose expected output is the red run, then separate fix and green-plus-recheck tasks; the tests checked only the field, while AC1 requires the field, value, and limit. +- Solution: T2 writes the configuration-load tests first with the red run as its expected output; T4 is the fix and T7 the green run plus recheck. The tests, M1, and M2 assert the field, the value, and the limit. Commit Points pair T2 with T4, because red tests cannot pass the pre-push gate alone. +- Current-tree verification: The Implementation Plan has T1-T7 and the Regression Test Strategy names the field, value, and limit. +- Resolution reference: `docs(issues): [#2466] address round-1 spec review findings` +- Follow-up PR URL: N/A +- Reply URL: + +### F3 - No migration-guide deliverable + +- PR number: 2468 +- Source review ID: 5440619059 +- Reviewer finding ID: N/A +- Source URL: +- Concern: The parent EPIC requires a migration-guide update for configuration public API changes, but the spec planned none. +- Solution: Added the guide update to In Scope, task T6, AC7, and its Acceptance Verification row, and listed the guide in `related-artifacts`. +- Current-tree verification: The spec mentions `migrate-v2-to-v3.md` in In Scope, T6, AC7, and the frontmatter. +- Resolution reference: `docs(issues): [#2466] address round-1 spec review findings` +- Follow-up PR URL: N/A +- Reply URL: + +### F4 - Verification policy omits toolchain qualification; checkpoints dropped + +- PR number: 2468 +- Source review ID: 5440619059 +- Reviewer finding ID: N/A +- Source URL: +- Concern: The spec dropped the template's manual-verification Notes, including toolchain qualification, and two workflow checkpoints. +- Solution: Restored the Notes block and status values, and the reviewer-validation and `agent-review-reports.md` checkpoints. +- Current-tree verification: The Manual Verification Scenarios section ends with the Notes block; the Workflow Checkpoints include both restored items. +- Resolution reference: `docs(issues): [#2466] address round-1 spec review findings` +- Follow-up PR URL: N/A +- Reply URL: + +### F12 - Unquoted issue markers parse as the bare string `issue` + +- PR number: 2468 +- Source review ID: 5440619059 +- Reviewer finding ID: F5 +- Source URL: +- Concern: Same as F6: quote the ADR's `issue #1978` and `issue #2245` values. +- Solution: Superseded by F6, which quoted both values. +- Current-tree verification: As for F6. +- Resolution reference: +- Follow-up PR URL: N/A +- Reply URL: + +### F13 - The EPIC log dates the reopen at the decision time + +- PR number: 2468 +- Source review ID: 5440619059 +- Reviewer finding ID: F6 +- Source URL: +- Concern: The #1978 Progress Log dated the reopen and the move at 08:29 UTC, the time of the maintainer's decision; GitHub shows the reopen comment at 08:50:40Z and the move came later in the PR. +- Solution: Split the entry into the 08:29 decision and an 08:50 entry for the GitHub actions, saying the specification PR moves the spec. The skill now says to date each entry at the action it records. +- Current-tree verification: `EPIC.md` has the two entries; `gh issue view 1978 --json comments` reports the reopen comment at 2026-10-07T08:50:40Z. +- Resolution reference: `docs(issues): [#1978] date the reopen at the GitHub reopen, not the decision` +- Follow-up PR URL: N/A +- Reply URL: + +### F7 - Skill Step 4 omits two edits the founding reopen made + +- PR number: 2468 +- Source review ID: 5440619059 +- Reviewer finding ID: N/A +- Source URL: +- Concern: Step 4 did not mention clearing the Acceptance Criteria checkbox that mirrors a reset row, or updating References. +- Solution: Extended the Acceptance Verification bullet to the matching checkbox and added a References bullet. +- Current-tree verification: `reopen-issue/SKILL.md` Step 4 contains both bullets. +- Resolution reference: `docs(skills): [#2466] couple, register, and complete the reopen-issue skill` +- Follow-up PR URL: N/A +- Reply URL: + +### F8 - Register the new lifecycle skill and name its dependencies + +- PR number: 2468 +- Source review ID: 5440619059 +- Reviewer finding ID: N/A +- Source URL: +- Concern: The skill was registered only through a cross-link, not in `docs/issues/README.md` or `docs/issues/closed/README.md`, and did not name the artifacts it restates. +- Solution: Registered the skill in both READMEs; the Skill Links section names the validator README, `lychee.toml`, and the EPIC and ISSUE templates. +- Current-tree verification: Both READMEs link `.github/skills/dev/planning/reopen-issue/SKILL.md`. +- Resolution reference: `docs(skills): [#2466] couple, register, and complete the reopen-issue skill` +- Follow-up PR URL: N/A +- Reply URL: + +### F14 - ADR lacks Affected Code and planned code back-links + +- PR number: 2468 +- Source review ID: 5440619059 +- Reviewer finding ID: F9 +- Source URL: +- Concern: `create-adr` Step 3.5 asks for an Affected Code section and module-level back-links from the code. +- Solution: Added the Affected Code section to the ADR, and planned the back-links in the spec's In Scope, T3, and T5 (spec commit `docs(issues): [#2466] address round-1 spec review findings`). +- Current-tree verification: The ADR has `## Affected Code` naming `packages/primitives/src/announce.rs` and `packages/udp-server/src/handlers/announce.rs`. +- Resolution reference: `docs(adrs): [#2466] quote issue markers and add the affected code` +- Follow-up PR URL: N/A +- Reply URL: + +### F15 - v2 inherits the bound through the shared type + +- PR number: 2468 +- Source review ID: 5440619059 +- Reviewer finding ID: F10 +- Source URL: +- Concern: v2 declares `announce_policy: AnnouncePolicy` too, so "v2 out of scope" read as "unchanged" although v2 inherits the bound. +- Solution: Out of Scope now says v2 inherits the bound through the shared type, reaching only library users of `v2_0_0`. +- Current-tree verification: `packages/configuration/src/v2_0_0/core.rs:17` declares `pub announce_policy: AnnouncePolicy`; the spec's Out of Scope states the inheritance. +- Resolution reference: `docs(issues): [#2466] address round-1 spec review findings` +- Follow-up PR URL: N/A +- Reply URL: + +### F11 - Step 2 prose names `git fetch`; the block runs `git pull --ff-only` + +- PR number: 2468 +- Source review ID: 5440619059 +- Reviewer finding ID: N/A +- Source URL: +- Concern: The caution named a command the block does not run. +- Solution: The prose now names `git pull --ff-only` and says the behind count catches a skipped or failed pull. +- Current-tree verification: `reopen-issue/SKILL.md` Step 2 prose matches its block. +- Resolution reference: `docs(skills): [#2466] couple, register, and complete the reopen-issue skill` +- Follow-up PR URL: N/A +- Reply URL: + +### F16 - Template blocks not copied verbatim + +- PR number: 2468 +- Source review ID: 5441434837 +- Reviewer finding ID: F12 +- Source URL: +- Concern: The template marks Status Values and Completion Rules as copied verbatim; this record rewrapped the Category line and dropped the last Completion Rules bullet. +- Solution: Copied both blocks from `docs/templates/PR-REVIEW-TEMPLATE.md` instead of the #2452 audit they had been copied from. +- Current-tree verification: `diff` of both sections against the template shows no difference apart from the template's guidance comment. +- Resolution reference: `docs(pr-reviews): [#2466] copy template blocks verbatim and log the second rebase on #2468` +- Follow-up PR URL: N/A +- Reply URL: + +### F17 - Two record statements no longer held at the round-2 head + +- PR number: 2468 +- Source review ID: 5441434837 +- Reviewer finding ID: F13 +- Source URL: +- Concern: The log recorded only the first rebase, and F10's verification counted four marker matches where the audit itself made a fifth. +- Solution: Added a 10:53 correction entry for the second rebase and push, and scoped F10's count to the four marked files plus this audit. +- Current-tree verification: The Processing Log has the 10:53 entry; F10's verification names the four files. +- Resolution reference: `docs(pr-reviews): [#2466] copy template blocks verbatim and log the second rebase on #2468` +- Follow-up PR URL: N/A +- Reply URL: + +## Processing Log + +- 2026-10-07 09:46 UTC - Copilot review 5440515841 submitted findings F5, F6, F9, and F10. +- 2026-10-07 09:56 UTC - Human review 5440619059 (da2ce7, round 1) requested changes with eleven findings, recorded as F1-F4, F7, F8, F11, and F12-F15. +- 2026-10-07 10:49 UTC - Reproduced the defect for F1, fixed the other findings in four commits, edited the #1978 issue body for F5, rebased onto `develop` (9 commits behind), pushed after the pre-push suite passed, and replied to all fifteen threads. Copilot's overview remark that the `interval_min` bound lacks protocol justification has no inline thread; it was raised with the maintainer as an open specification question instead of being recorded as a finding. +- 2026-10-07 10:53 UTC - Correction to the 10:49 entry, which was committed before this step: `develop` had moved 10 more commits, so the branch was rebased again onto `24bf4746a` and force-pushed with this record, then all fifteen threads were resolved. +- 2026-10-07 11:12 UTC - Human review 5441434837 (da2ce7, round 2) verified the fifteen round-1 rows and requested changes with two findings, recorded as F16 and F17. +- 2026-10-07 13:12 UTC - The maintainer kept one type for `interval` and `interval_min` because both measure the same quantity, although no protocol bounds `interval_min`. Decision 2 states that reason in `docs(issues): [#2466] justify the shared interval_min type by meaning, not protocol`; the #2466 issue body was updated to match. +- 2026-10-07 13:26 UTC - Fixed F16 and F17, rebased onto `develop` (19 commits behind), pushed after the pre-push suite passed, and replied to both threads. +- 2026-10-07 14:37 UTC - Human review 5443287949 (da2ce7, round 3, 13:48 UTC, at `docs(pr-reviews): [#2466] record round-2 review findings on #2468`) approved with no new findings, verified F16 and F17 fixed, and confirmed the `interval_min` rationale and its record. No unresolved thread remains. At the maintainer's request, the branch was then rebased onto the latest `develop` with this entry, which dismisses that approval. + +## Completion Rules + +- Re-derive the reply claim against the current tree before replying or resolving a thread. +- Reply on every resolvable thread before resolving it. +- For an outdated thread whose concern was fixed, record `Disposition=FIXED` and + `Thread state=RESOLVED`, even if GitHub marks the thread outdated after the push. For a + duplicate, superseded, or no-change in-PR thread, reply exactly + `Superseded by : .`, record `Disposition=NO_ACTION` and + `Thread state=SUPERSEDED`, then resolve it. A post-merge `NO_ACTION` requires maintainer + approval to decline the follow-up work. +- A consolidated PR conversation response may cover multiple review rounds only when it names + every review ID and every finding ID with its disposition and resolution reference. Record its + durable URL in each related row. +- Cite a fix by its unique Conventional Commit subject or durable reply URL, never by a branch SHA + that can change after a rebase. +- Refresh review threads using GraphQL and confirm that no unresolved actionable thread remains. diff --git a/lychee.toml b/lychee.toml index 05dc28804..3b7bc6a29 100644 --- a/lychee.toml +++ b/lychee.toml @@ -4,6 +4,7 @@ offline = true # Verify both Markdown heading anchors and text fragments in local links. include_fragments = "full" +# skill-link: reopen-issue exclude_path = [ "(^|/)docs/external-snapshots/.*/(LICENSE|SPEC)\\.md$", # Upstream files use upstream link semantics. "(^|/)docs/issues/closed/", # Immutable historical records with known stale relative links. diff --git a/packages/configuration/docs/migrate-v2-to-v3.md b/packages/configuration/docs/migrate-v2-to-v3.md index 69d679bf0..97914dc26 100644 --- a/packages/configuration/docs/migrate-v2-to-v3.md +++ b/packages/configuration/docs/migrate-v2-to-v3.md @@ -457,7 +457,7 @@ Use this checklist to verify your configuration is ready for v3: ## References -- [EPIC #1978 — Configuration Overhaul](../../../docs/issues/closed/1978-configuration-overhaul-epic/EPIC.md) +- [EPIC #1978 — Configuration Overhaul](../../../docs/issues/open/1978-configuration-overhaul-epic/EPIC.md) - [Issue #1980 — Runtime activation and final cleanup](../../../docs/issues/closed/1980-1978-configuration-overhaul-final-cleanup/ISSUE.md) - [Issue #1987 — HTTP announce query-IP policy](../../../docs/issues/closed/1987-add-config-option-to-use-ip-from-announce-query-string/ISSUE.md) - [ADRs](../../../docs/adrs/README.md)