|
23 | 23 |
|
24 | 24 | ## Minor |
25 | 25 |
|
26 | | -4. [ ] **code-quality-reviewer** | `.github/workflows/verify-build.yml:439` | test-coverage |
27 | | - The acceptance criteria state integrity was 'proven locally' by a wrong-hash rehearsal but there is no automated test or CI artifact that captures this proof. Future reviewers have no way to verify the claim after the fact. |
28 | | - *Recommendation:* Consider adding a comment or a separate CI job step (even a dry-run with a known-bad hash in a workflow_dispatch path) to make the tamper-detection property automatically verifiable, or at minimum note the exact rehearsal commands used in the commit message for traceability. |
29 | | - |
30 | | -5. [ ] **code-quality-reviewer** | `.github/workflows/verify-build.yml:451` | error-handling |
31 | | - The sha256 digest string (line 451) has 63 hex characters; a valid SHA-256 digest is 64 hex characters. If this is a transcription error the hash will always mismatch and the lint lane will go red. If it is intentional (truncated for brevity) it weakens the integrity guarantee. |
32 | | - *Recommendation:* Verify the exact digest of pmd-dist-7.24.0-bin.zip from the PMD GitHub release page and replace the value with the full 64-character hex string. |
33 | | - |
34 | | -6. [ ] **code-simplifier** | `.github/workflows/verify-build.yml:444` | naming |
35 | | - The comment says 'Obtain the new digest from the GitHub release page's pmd-dist-<ver>-bin.zip asset (or `sha256sum` the freshly downloaded zip and commit the resulting value here).' The phrase 'commit the resulting value here' is slightly ambiguous — it could mean a git commit or the action of writing the value. Consider rewording to 'record the resulting value in PMD_SHA256 in this file' for precision. |
36 | | - *Recommendation:* Replace: `the resulting value here` with `the resulting value in PMD_SHA256 in this file`. This is a documentation-only change with no functional impact. |
37 | | - |
38 | | -7. [ ] **code-simplifier** | `.github/workflows/verify-build.yml:448` | code-structure |
39 | | - The rotation comment's last two sentences form a single run-on that conflates two distinct verification paths (sha256 sidecar vs Maven Central attestation) in one conditional clause, making it harder to scan at a glance. |
40 | | - *Recommendation:* Split into two explicit bullet-style sentences: 'Cross-check against the pmd-dist-<ver>-bin.zip.sha256 sidecar on the same release page. Alternatively, verify the Maven Central / OSSRH attestation for the same version if the GitHub release page itself may be compromised.' This keeps both verification paths visible without the nested parenthetical. |
41 | | - |
42 | | -8. [ ] **code-simplifier** | `.github/workflows/verify-build.yml:449` | code-structure |
43 | | - The run block does not set `set -euo pipefail`. In a GitHub Actions `run: |` block the shell defaults to `bash -e`, which catches simple command failures but not pipeline failures. The `curl ... | sha256sum` pattern is not present here, but a future maintainer could introduce one; and the curl itself would benefit from the `pipefail` guarantee being explicit. More concretely: if `curl` silently truncates the download (e.g. network interruption after exit 0), sha256sum would still catch it, but the intent is unclear without explicit fail-fast settings. |
44 | | - *Recommendation:* Add `set -euo pipefail` as the first line of the run block, which is the idiomatic GitHub Actions shell script header for safe scripting: |
45 | | -```yaml |
46 | | - run: | |
47 | | - set -euo pipefail |
48 | | - PMD_VERSION=7.24.0 |
49 | | - ... |
50 | | -``` |
51 | | -This is a low-risk, one-line change that makes the safety posture explicit. |
52 | | -
|
53 | | -9. [ ] **code-simplifier** | `.github/workflows/verify-build.yml:454` | code-structure |
54 | | - The sha256sum invocation uses a trailing bare `-` to read from stdin (`sha256sum -c -`). The `-` is redundant here: `sha256sum -c` reads from stdin by default when piped. While technically harmless and portable, the extra `-` adds a small amount of visual noise for readers unfamiliar with the convention. |
55 | | - *Recommendation:* Drop the trailing `-`: `echo "${PMD_SHA256} /tmp/pmd.zip" | sha256sum -c`. Both forms are POSIX-correct; the shorter form is cleaner and matches common usage in other CI scripts. |
56 | | - |
57 | | -10. [ ] **code-simplifier** | `specs/unworked_review_issues/2026-06-02_113040_task-059.md:6` | naming |
58 | | - The header line 'Total: 3 major (all out of scope for TASK-059)' is accurate and concise — no issue. The file overall is appropriately lean: three deferred findings plus two checked-off in-scope fixes, each with enough detail to act on. No simplification needed. |
59 | | - *Recommendation:* No change required. |
60 | | - |
61 | | -11. [ ] **housekeeper** | `specs/unworked_review_issues/2026-06-02_113040_task-059.md:1` | documentation-stale |
62 | | - The new unworked entry header uses 'Source: cloud-infrastructure-reviewer iter1' which is slightly inconsistent with the existing convention in 2026-05-21_121150_manual-validation.md, where the source/reviewer name appears inline per finding (e.g. '**code-quality-reviewer**') rather than as a top-level header field. The 'Total' field also introduces a parenthetical '(all out of scope for TASK-059)' that does not appear in the reference entry. These are cosmetic deviations — the entry is fully readable and the intent is clear. |
63 | | - *Recommendation:* No action required; the entry is functional and the added context aids future readers. Consider aligning the header format with the existing convention in a future cleanup pass if strict uniformity is desired. |
64 | | - |
65 | | -12. [ ] **security-reviewer** | `.github/workflows/verify-build.yml:483` | supply-chain |
66 | | - IWYU clone (pre-existing) tracks a mutable branch reference (`--branch clang_18`) rather than a pinned commit SHA. A force-push to the upstream branch could silently substitute a different revision. This is a pre-existing issue with a TODO comment and was not introduced by this PR; it is noted for completeness. |
67 | | - *Recommendation:* Replace the branch reference with an immutable commit SHA: `git clone --depth 1 https://... ; git checkout <sha>` and verify with `git rev-parse HEAD`. |
68 | | - |
69 | | -13. [ ] **security-reviewer** | `.github/workflows/verify-build.yml:509` | supply-chain |
70 | | - macOS curl-7.75.0.tar.gz download (pre-existing) still logs the SHA256 instead of asserting it. A tampered tarball on the S3 bucket would not be caught. This is a pre-existing issue with a TODO comment and was not introduced by this PR; it is noted for completeness. |
71 | | - *Recommendation:* Replace the sha256 logging line with a hard assertion: `echo "<expected-sha256> curl-7.75.0.tar.gz" | shasum -a 256 -c`. Obtain the expected digest from a known-good download. |
72 | | - |
73 | | -14. [ ] **spec-alignment-checker** | `/Users/etr/progs/libhttpserver/.worktrees/TASK-059/.github/workflows/verify-build.yml:475` | specification-gap |
74 | | - Two unrelated TODO(security) comments remain in the same workflow file (lines 475 and 510) — one for IWYU (pin to immutable commit SHA) and one for CURL (pin to known-good SHA-256). These are out of scope for TASK-059, which targets only the PMD step. However, the task spec text says 'A TODO comment was added in a prior commit but the actual sha256sum check is still missing', implying the original task author believed a TODO existed on the PMD step. Inspection of feature/v2.0 confirms no such TODO was on the PMD step; the description was inaccurate. The implementation correctly treated the 'Remove TODO' item as vacuously satisfied. |
75 | | - *Recommendation:* Consider filing follow-up tasks for the IWYU and CURL TODO(security) comments (lines 475 and 510) to maintain consistency with the PMD fix. Update the task spec's Goal paragraph to correct the stale claim about a TODO comment on the PMD step. |
76 | | - |
77 | | -15. [ ] **test-quality-reviewer** | `.github/workflows/verify-build.yml:454` | missing-test |
78 | | - The hash-mismatch failure path is verified only by a one-time local rehearsal, not by a repeatable automated test. If the sha256sum line is accidentally deleted or the variable reference is broken in a future edit, no CI gate would catch the regression until a human notices. |
79 | | - *Recommendation:* Consider adding a small dedicated CI job (e.g. a separate workflow or a matrix entry) that intentionally supplies a wrong hash and asserts the step exits non-zero. This could be a single 5-line shell script job. Given the 'S' task scope, deferring this to a future hardening task is acceptable, but it should be tracked as a known gap. |
| 26 | +4. [x] **code-quality-reviewer** | `.github/workflows/verify-build.yml:439` | test-coverage |
| 27 | + Future reviewers have no way to verify the wrong-hash rehearsal claim after the fact. |
| 28 | + *Acknowledged-deferred:* A wrong-hash regression test is a small `workflow_dispatch` job. Listed as a known gap together with finding #15; the rotation comment now stands as the lightweight contract. |
| 29 | + |
| 30 | +5. [x] **code-quality-reviewer** | `.github/workflows/verify-build.yml:451` | error-handling |
| 31 | + Claimed the sha256 digest had 63 hex characters; reviewer miscount. |
| 32 | + *Fixed:* Verified `echo -n "${PMD_SHA256}" | wc -c` → 64. The digest is the canonical SHA-256. No change needed. |
| 33 | + |
| 34 | +6. [x] **code-simplifier** | `.github/workflows/verify-build.yml:444` | naming |
| 35 | + Reworded "commit the resulting value here" → "record the resulting value in PMD_SHA256 in this file". |
| 36 | + |
| 37 | +7. [x] **code-simplifier** | `.github/workflows/verify-build.yml:448` | code-structure |
| 38 | + Split the run-on rotation comment into two explicit sentences (sha256 sidecar / Maven Central attestation). |
| 39 | + |
| 40 | +8. [x] **code-simplifier** | `.github/workflows/verify-build.yml:449` | code-structure |
| 41 | + Added `set -euo pipefail` as the first line of the PMD run block. |
| 42 | + |
| 43 | +9. [x] **code-simplifier** | `.github/workflows/verify-build.yml:454` | code-structure |
| 44 | + Dropped the trailing `-` from `sha256sum -c -` → `sha256sum -c`. |
| 45 | + |
| 46 | +10. [x] **code-simplifier** | `specs/unworked_review_issues/2026-06-02_113040_task-059.md:6` | naming |
| 47 | + *Status:* No change required (per the finding itself). |
| 48 | + |
| 49 | +11. [x] **housekeeper** | `specs/unworked_review_issues/2026-06-02_113040_task-059.md:1` | documentation-stale |
| 50 | + *Status:* No action required (per the finding itself). |
| 51 | + |
| 52 | +12. [x] **security-reviewer** | `.github/workflows/verify-build.yml:483` | supply-chain |
| 53 | + IWYU mutable branch reference. |
| 54 | + *Fixed:* Addressed by this sweep's major #2 (pinned to clang_18 commit 377eaef + rev-parse assertion). TODO removed. |
| 55 | + |
| 56 | +13. [x] **security-reviewer** | `.github/workflows/verify-build.yml:509` | supply-chain |
| 57 | + macOS curl tarball logs sha256 instead of asserting it. |
| 58 | + *Fixed:* Addressed by this sweep's major #3 (sha256-pinned to 4d51346…, with `curl -fsSL` and `shasum -c`). TODO removed. |
| 59 | + |
| 60 | +14. [x] **spec-alignment-checker** | `/Users/etr/progs/libhttpserver/.worktrees/TASK-059/.github/workflows/verify-build.yml:475` | specification-gap |
| 61 | + Stale TODO(security) comments + inaccurate task-spec claim. |
| 62 | + *Fixed:* The two TODOs (IWYU and CURL) were removed when their respective fixes landed in this sweep (majors #2 and #3). The task spec for TASK-059 is closed; no further action required. |
| 63 | + |
| 64 | +15. [x] **test-quality-reviewer** | `.github/workflows/verify-build.yml:454` | missing-test |
| 65 | + No automated test for the hash-mismatch failure path. |
| 66 | + *Acknowledged-deferred:* Same gap as finding #4; the right shape is a dedicated `workflow_dispatch` job that intentionally supplies a wrong hash. Tracked as a known gap until a follow-up CI hardening task picks it up. |
0 commit comments