feat(configuration): [#2151] add tracker config path argument - #2178
josecelano merged 44 commits into
Conversation
Make each executable-boundary failure test fail for one reason and remove the suite's only fixed loopback ports. - Add fixture assertion helpers (assert_usage_error, assert_startup_failure, assert_diagnostic_names_source_path) that print the captured child output when they fail. - Drop the post-failure candidate-port probe: a tracker that wrongly started never exits, so the bounded wait_for_exit deadline already proves no partial startup. All configurations now use port zero. - Drop the OS-owned "Permission denied" fragment; the tracker contract is its own diagnostic plus the source path. - Add enforced_or_report_skip() so the unreadable-file test reads as plain Arrange/Act/Assert. - Move permission-restoration and drop-without-runtime checks into the fixture's unit tests; they verify fixture behaviour, not the CLI. - Repoint links broken by the tests/common fixture move (R1). - Record the R6 review outcome in the executable test plan.
Add an approved issue-local plan for issue torrust#2151 that distinguishes three verification activities: maintained Rust automatic tests, real human-oriented manual verification recorded as evidence, and disposable issue-local verification scripts that require a written rationale (and a Rust-vs-Python justification).
Implement the approved AI-harness verification plan (H1-H5, H7) so agents no longer treat scripted output as manual verification. - Add docs/templates/MANUAL-VERIFICATION-EVIDENCE.md: issue-local record of real commands, observed output, tracker logs, and conclusions; invented evidence is forbidden. - Issue template: manual scenarios must be human-oriented feature use or bug reproduction with evidence in manual-verification-evidence.md; add the disposable-script rationale, path, owner, and Rust-vs-Python justification requirements. - docs/testing.md: add "Verification Types" with concrete examples of human-style manual verification and why it catches integration and UX gaps; tests/AGENTS.md links to it. - Skills (create-issue, write-unit-test) and agents (Planner, Implementer, Task Reviewer, Committer): link to the canonical guidance instead of duplicating policy. - Issue torrust#2151: reclassify the Python verifier output as disposable- script evidence, reset M1-M5 to TODO, and extend T10 to require a real release-binary manual run (H6 stays in progress until then).
da2ce7
left a comment
There was a problem hiding this comment.
Approving at df6ccf2e.
Code — unchanged and already verified twice. git diff --stat 8b3927c1 df6ccf2e -- src packages tests Cargo.toml Cargo.lock is empty; in fact the whole delta since the reap fix is two documentation files. So the tree gated in the previous round stands: linter all, cargo clippy -p torrust-tracker --all-targets --all-features -- -D warnings, cargo test --test cli-configuration (18 passed, twice) and cargo test --test lifecycle-signals (13 passed, twice, with the renamed reaping regression green in both), no leftover tracker processes and no zombies. linter all is green again at this head (18.5 s), and lychee confirms the new Copilot-record link resolves.
Record — complete in substance. F12 is really fixed, not just marked fixed: the processing log now reads forward, and no entry postdates the commit that introduced it. F13 is really fixed too — docs/copilot-pr-reviews/pr-2178-copilot-suggestions.md exists and its two thread node IDs resolve to exactly threads 3961389301 and 3961389339, both on packages/configuration/src/lib.rs, both resolved, both matching their summaries. I checked every F1-F13 row against live thread state and every RESOLVED claim is true.
One nit, and it stays open for you to close either way. Row F11 still records its thread as 3966277176, in both the ID column and the #discussion_r3966277176 link. That comment does not exist — gh api repos/torrust/torrust-tracker/pulls/comments/3966277176 returns 404. The real thread is 3966277146, and the row proves it against itself: F11's own reply 3966391039 has in_reply_to_id = 3966277146. The substance of F11 is right and the finding it describes was genuinely fixed in aeb3c533; only the identifier and its link are wrong, which is why this is a nit rather than a blocker. Thread 3966542753 is the one unresolved thread on the PR — fix the ID or reply saying you would rather leave it, whichever you prefer.
Two smaller wording points while you are in the file. The 09:10 log line says all currently known findings are complete, but review 5152084856 (09:00:54 UTC) was already posted 51 minutes before df6ccf2e was authored and is not in the Reviews table. That is really a race — the review that checks the audit can never be inside the audit it checks — so scoping the sentence to the last processed review would read truer than an absolute claim. And Notes still says "the three Cameron reviews recorded here" where the table now lists five.
None of this blocks the merge.
da2ce7
left a comment
There was a problem hiding this comment.
Re-reviewed at 09d1de61, which is 67feff3a plus a skill-prose commit (docs(skills): harden PR feedback thread discovery, three lines in the new skill, correctly naming the GraphQL thread query as the authority). The fix I asked for is in: F11 now records 3966277146 in both the ID column and the link, gh api repos/torrust/torrust-tracker/pulls/comments/3966277146 resolves, and thread 3966542753 is resolved with an accurate reply. The code tree is untouched — git diff --stat df6ccf2e 09d1de61 -- src packages tests Cargo.toml Cargo.lock is empty, so this is still the tree already gated twice — and linter all is green at 67feff3a (18.1 s, clean checkout, no leftover processes); 09d1de61 changes only the skill's prose. On its own that is an approval.
Commenting rather than approving because the same commit adds two new record defects, both one-line fixes. First, F14 attributes thread 3966542753 to review 5151742675, but that comment was posted by review 5152084856 — gh api repos/torrust/torrust-tracker/pulls/comments/3966542753 --jq .pull_request_review_id returns 5152084856. 5151742675 closed at 08:31 and could not have raised a finding about text written at 08:48. I am flagging this rather than letting it pass as a typo because it is the traceability claim the record exists to make, and it went wrong for a reason worth fixing at the root: 5152084856 is still not a row in the Reviews table, so a finding it raised had to be filed under a review that did not raise it. Adding 5152084856 (and 5152707859) as rows fixes both. Second, the new processing-log entry is stamped 2026-09-09 10:55 UTC while its own commit is authored 10:09:12 UTC — 46 minutes in the future, which is exactly the defect F12 was filed for and 3174a6cd fixed, recurring one entry later.
Two smaller carried nits while you are in the file: the 09:10 line still claims all currently known findings are complete, and Notes still says "the three Cameron reviews recorded here" where the table lists five. Thread 3967064922 is open only because it is my round-7 restatement of the F11 nit you have now fixed — a reply or a resolve closes it.
da2ce7
left a comment
There was a problem hiding this comment.
Re-reviewed at 635ff5a2. The code is untouched again — git diff --stat 09d1de61 635ff5a2 -- src packages tests Cargo.toml Cargo.lock is empty, as is the same diff against 8b3927c1 — and linter all is green at this head (21.2 s, clean checkout, no leftover processes). The skill change in 09d1de61 is a good one: naming the GraphQL thread query as the authority over the REST per-review comments endpoint is correct and matches the existing fetch-review-threads skill.
The two round-8 record defects are not fixed. F14 still reads Review ID 5151742675, while gh api repos/torrust/torrust-tracker/pulls/comments/3966542753 --jq .pull_request_review_id returns 5152084856. F15 reuses F11's thread 3966277146, and on reflection that is legitimate: my round-5 comment on that thread is where the REST under-report explanation was first written, so one thread carrying two findings is accurate — no change needed there. The two new log entries are stamped 11:05 and 11:15 UTC while their commits are authored 10:11:45 and 10:23:06 UTC, so both are in the future — as is 10:55 against 67feff3a at 10:09:12 — which is the F12 defect a third time. One hypothesis worth checking, offered as a hypothesis only: every commit here is authored at +01:00, and reading all three stamps as +01:00 puts each one a few minutes before its own commit, so the stamps may be coming from a local clock and being labelled UTC.
Exactly three corrections: F14's Review ID to 5152084856, with a Reviews row for that review; the three stamps to true UTC no later than their commits; and the "all currently known findings are complete" sentence dropped or scoped, since threads 3967064922, 3967199424 and 3967199432 are open and unanswered as I write. One structural point so it does not become a treadmill: a row for the latest review is never something I will ask for — the review that checks the record cannot be inside the record it checks. 5152084856 is three reviews old and has a finding filed against it; the objection is to a row naming the wrong review, not to the absence of a row for the current one.
To be plain about where this stands: the code has been approved twice and has not changed since. The only thing between this PR and an ACK is the accuracy of the record it adds.
|
Review The no-runtime child-reaping fixes were already completed in The review-feedback audit now records this review and the outcome of each finding. |
|
Review The F11 identifier is corrected to the verified review-comment ID |
|
Review The audit now attributes F14 to the review that actually created its inline comment ( |
|
ACK 44f634a |
da2ce7
left a comment
There was a problem hiding this comment.
Approving at 44f634a1c8d1c27fb6347d20887784f062381167.
Code — unchanged since the twice-gated tree. git diff --stat 8b3927c1 44f634a1 -- src packages tests Cargo.toml Cargo.lock is empty, reproduced on a clean checkout at this head; the whole delta since the reap fix is three documentation files. linter all is green here (17.6 s, clean worktree, no leftover tracker processes, no zombies), and all five workflow runs for this head — Testing, OS Compatibility, Container, Docs Lint, Coverage — completed success.
The three corrections from the last round all hold. I recomputed each rather than taking the replies on trust.
-
F14 now reads Review ID
5152084856, which is exactly whatgh api repos/torrust/torrust-tracker/pulls/comments/3966542753 --jq .pull_request_review_idreturns, and5152084856now has a Reviews row — as do5152707859and5152875522. Their submitted times and reviewed commits match GitHub exactly: 09:00:54 /a33fdafc, 10:00:33 /df6ccf2e, 10:15:50 /09d1de61. -
The three stamps are true UTC and each now sits in the right window — after the commit it reports, before the commit that carries it:
10:03in67feff3a(10:09:12 UTC),10:10in09d1de61(10:11:45),10:12in635ff5a2(10:23:06). Moving10:09to10:10in44f634a1was the right call rather than a cosmetic one:67feff3ais authored 10:09:12, so10:09was ambiguous about the very ordering the entry asserts, and10:10is not. The local-clock reading offered last round as a hypothesis turned out to be the cause, which is worth knowing for the next audit. -
The absolute completeness sentence is gone. Every remaining claim is scoped to a review — "processed through review
5151742675" / "5152875522" — and the new Notes line states outright that the records are an observed snapshot and that the review list must be refreshed before declaring the PR ready. That is the structural point handled at the source instead of patched sentence by sentence.
Threads. GraphQL reviewThreads reports 19 of 19 resolved, each with a reply.
Two leftovers I am recording rather than asking for; neither is worth another round. Notes still says "the three Cameron reviews recorded here" where the table now lists eight. And F18's Validation cell still reads "F14 is timestamped 10:09 UTC" after 44f634a1 moved that entry to 10:10. Fold them in if the file is opened again for another reason; otherwise leave them.
Nothing blocks this. ACK follows.
Summary
Adds
-c/--config-toml-path <PATH>to the tracker binary. An explicit CLI file is now the selected base configuration source, ahead of complete-TOML and path environment sources, whileTORRUST_TRACKER_CONFIG_OVERRIDE_*values continue to merge above it.Explicit paths are loaded exactly, without parent-directory lookup. Missing, unreadable, directory, and malformed sources identify the supplied path and fail before listener startup. Missing and empty argument values remain Clap usage errors.
Implementation
PathBuffrom the main binary through bootstrap configuration initialization.docs/issues/open/2151-add-tracker-config-path-argument/manual-verification-evidence.md.Validation
cargo test -p torrust-tracker --test cli-configuration --test lifecycle-signals(18 and 13 passed)cargo test --package torrust-tracker-configuration --lib(129 passed)cargo clippy -p torrust-tracker --tests -qlinter allTORRUST_GIT_HOOKS_LOG_DIR=.tmp ./contrib/dev-tools/git/hooks/pre-commit.sh --format=jsonCloses #2151