|
| 1 | +# Unworked Review Issues |
| 2 | + |
| 3 | +**Run:** 2026-06-04 16:09:35 |
| 4 | +**Task:** TASK-061 |
| 5 | +**Total:** 7 (0 critical, 0 major, 7 minor) |
| 6 | + |
| 7 | +## Minor |
| 8 | + |
| 9 | +1. [ ] **code-quality-reviewer** | `src/detail/webserver_register.cpp:247` | readability |
| 10 | + The TASK-024 comment added above unregister_impl_ uses the past-tense TASK tag prefix ('TASK-024:') as a rationale anchor, which is the established project convention, but the comment text itself is already present at line 272 (the original position inside the function body as a doc block). The two comment blocks are not duplicates — the new one is a function-level doc header while the existing one was a trailing inline remark — but a reader seeing both in quick succession may find the repetition slightly noisy. |
| 11 | + *Recommendation:* Consider consolidating: keep the function-level doc header that was added (lines 247-252) and remove or abbreviate the inline reference at the original position if it now says the same thing. This is purely cosmetic and does not block approval. |
| 12 | + |
| 13 | +2. [ ] **code-quality-reviewer** | `src/detail/webserver_register.cpp:350` | readability |
| 14 | + The TASK-029 block comment (lines 350-355) is now complete and grammatically sound, but it appears between the closing brace of unregister_resource() (line 348) and the closing namespace brace (line 357), giving the impression it is a file-level orphan rather than documentation for a preceding function or for the namespace itself. Its placement is not wrong but could mislead a reader scanning top-level structure. |
| 15 | + *Recommendation:* Move the TASK-029 comment immediately above the first function it documents (block_ip at the next line after the namespace comment, or the unregister_resource group), or leave as-is and add a one-line header like '// IP-control API history:' to signal it is intentional prose. This is cosmetic only. |
| 16 | + |
| 17 | +3. [ ] **code-quality-reviewer** | `src/detail/webserver_register.cpp:355` | readability |
| 18 | + The TASK-029 block comment (lines 350-357) that was completed by appending 'they are no longer reachable from the public API.' now has a blank line between the completed sentence and the closing namespace brace. This matches the pattern in the rest of the file, but the comment sits after the last function in the file with no following declaration to anchor it, making it slightly orphaned-looking even in its repaired form. The content is correct and complete. |
| 19 | + *Recommendation:* Acceptable as-is. If a future cleanup pass targets this file, moving the TASK-029 historical note immediately above block_ip / unblock_ip (the functions it describes) would improve locality. Not required for this task. |
| 20 | + |
| 21 | +4. [ ] **code-quality-reviewer** | `test/Makefile.am:73` | readability |
| 22 | + The updated comment says 'See lines ~532-535 below for the historical XFAIL_TESTS note.' The tilde (~) and approximate line numbers will drift as the file grows. The target note is already self-contained (lines 533-536 say what happened), so the cross-reference adds maintenance burden without proportionate value. |
| 23 | + *Recommendation:* Either remove the forward-reference sentence entirely (both the comment at line 66-75 and the one at 533-536 are now self-sufficient) or replace the approximate line reference with a searchable token such as 'see the XFAIL_TESTS historical note near the TESTS= assignment below' to avoid stale numbers. |
| 24 | + |
| 25 | +5. [ ] **code-simplifier** | `src/detail/webserver_register.cpp:247` | code-structure |
| 26 | + The moved comment block for unregister_impl_ (lines 247-252) now correctly precedes the function, but it still references 'TASK-024' inline, which is a task-tracker token rather than an intent-explaining label. The rest of the codebase uses the same convention consistently, so this is a minor style note rather than a real issue. |
| 27 | + *Recommendation:* No action required; the TASK-NNN prefix is the project-wide convention and is consistent with every other block comment in these files. |
| 28 | + |
| 29 | +6. [ ] **code-simplifier** | `test/Makefile.am:74` | code-structure |
| 30 | + The updated comment says 'See lines ~532-535 below for the historical XFAIL_TESTS note' but the approximate line reference (~532-535) will drift as the file changes, making it unreliable as guidance. |
| 31 | + *Recommendation:* Either drop the cross-reference entirely (the comment stands on its own without it) or replace the line-number hint with a more stable search anchor such as 'See the XFAIL_TESTS block further below' so the reference stays valid as lines shift. |
| 32 | + |
| 33 | +7. [ ] **housekeeper** | `specs/tasks/M7-v2-cleanup/TASK-061.md:15` | documentation-stale |
| 34 | + Action item 5 ('Remove the decade-old //TODO: personalized messages at test/littletest.hpp:21 only if we have a fork policy that lets us; otherwise leave (it's vendored)') remains unchecked, which is correct per the task text — but no explicit decision was recorded about the fork policy outcome. |
| 35 | + *Recommendation:* This is acceptable as-is since the task itself says 'only if we have a fork policy that lets us; otherwise leave'. The open checkbox correctly documents the deferred decision. No change required. |
0 commit comments