✅ [main] add fuzz targets and fix negative-index panic in extractTimePart, fixes #47

[chicks-net]

Done

• ✅ [main] add fuzz targets and fix negative-index panic in extractTimePart, fixes Close OpenSSF Scorecard gaps: SAST, Fuzzing, Branch-Protection (7.7 → ~8.6) #47
• 🛡️ [gha] add CodeQL SAST workflow for Go analysis, fixes Close OpenSSF Scorecard gaps: SAST, Fuzzing, Branch-Protection (7.7 → ~8.6) #47
• ✅ [main] strengthen FuzzParseColorSpec two-color invariant per review, fixes Close OpenSSF Scorecard gaps: SAST, Fuzzing, Branch-Protection (7.7 → ~8.6) #47

Meta

(Automated in .just/gh-process.just.)

[github-advanced-security]

You are seeing this message because GitHub Code Scanning has recently been set up for this repository, or this pull request contains the workflow file for the Code Scanning tool.

What Enabling Code Scanning Means:

• The 'Security' tab will display more code scanning analysis results (e.g., for the default branch).
• Depending on your configuration and choice of analysis tool, future pull requests will be annotated with code scanning analysis results.
• You will be able to see the analysis results for the pull request's branch on this overview once the scans have completed and the checks have passed.

For more information about GitHub Code Scanning, check out the documentation.

[claude]

Review

Solid, well-scoped change - the fuzz targets are genuinely useful (they exercise the wire-decode and CLI-parsing paths with untrusted-shaped input) and the CodeQL workflow mirrors the existing go-ci.yml conventions (pinned SHAs, permissions: {} plus a minimal job-level grant, harden-runner). A few notes:

Code quality / correctness

• extractTimePart fix is correct, but the panic it closes isn't reachable today: every call site (main.go:430-446) passes a compile-time constant 0-4, so part can never be negative in production. The fix is still worth keeping - it makes the function correct for any future caller and for the fuzz target itself - but the PR title/body ('fix negative-index panic') could be read as implying a live user-facing bug. Might be worth a one-line clarification that this was found and closed via the new fuzz target rather than being exploitable via the CLI today.
• The refactored early-return structure in extractTimePart (main.go:333-347) is a nice simplification over the original nested if.

CodeQL workflow

• .github/workflows/codeql.yml is missing a trailing newline at end of file. Minor, but worth fixing since other workflow files in the repo end cleanly and some YAML linters flag this.
• Manual 'go build ./...' instead of CodeQL's autobuild is a good call for consistency with the pinned Go version - matches the stated rationale in the comment.

Test coverage

• FuzzStatusDecode's invariant (binary.Read must not error/panic for exactly api1PacketSize/api2PacketSize-length packets) is well-chosen and matches the documented 'trailing byte is padding' behavior for the 35-byte v1.x packet.
• FuzzParseColorSpec's single invariant (single color applies to both digit groups) is good but light - it never checks that a valid two-color spec (rrggbb:rrggbb) actually decodes the two halves independently (i.e., mmss != hh when the two colors differ and parsing succeeds). Not a blocker since table tests presumably already cover that in the non-fuzz suite, just flagging it as an easy follow-up invariant.
• FuzzDisplayModeString's only invariant is 'doesn't panic,' which is true today since the function is pure bitmask/switch logic with no indexing - low risk, but also low value beyond satisfying Scorecard's fuzzing check (which the comment candidly acknowledges as one of the two motivations). Reasonable trade-off given the goal.

Security

• No concerns - CodeQL workflow permissions are least-privilege (contents: read, security-events: write only where needed), actions are pinned by SHA, and harden-runner is present. persist-credentials: false on checkout is good practice.

Nice work overall - this is a clean, low-risk hardening PR that does what it says.

[claude]

Review

Overall this is a solid, well-scoped hardening PR: a real (if narrow) bug fix in extractTimePart, four fuzz targets with sensible invariants, and a CodeQL workflow that matches the existing go-ci.yml conventions (pinned SHAs, harden-runner, minimal permissions: {} + per-job grants). Nice touch documenting the fuzzing rationale directly in the test-file comment, including the "partly for Scorecard" motivation — that kind of honesty is good for future maintainers.

Code quality / correctness

• main.go:333-349 — the fix itself is correct: parts[part] with a negative part panics in Go (negative slice index), and the old len(parts) > part guard never caught part < 0. Adding the part < 0 || part >= len(parts) guard is the right call.
• Worth noting in the PR description (or just for your own awareness) that this path is not reachable from the CLI today — every call site (main.go:430-446) passes a compile-time constant 0–4. So this isn't a fix for a user-triggerable panic, it's a defensive fix for extractTimePart's exported-shaped API being safe to call with any int, which matters once you fuzz it directly. That's a fine reason to fix it, just a slightly different framing than "fixes a panic" might suggest to someone skimming the PR title later.

Fuzz targets (main_test.go)

• FuzzExtractTimePart: good seed corpus (negative index, out-of-range index, non-numeric, out-of-range numeric). The invariant (no panic, err ⇒ zero value, else 0-255) is meaningful.
• FuzzParseColorSpec: the two-color branch re-derives the expected halves via strings.Cut + parseHexColor, which is largely re-implementing parseColorSpec's own logic rather than checking an independent property — it'll catch a regression that only breaks one of the two calls, but won't catch a bug shared by both parseColorSpec and this duplicated check path. Not wrong, just weaker than it looks; consider whether a property like "re-splitting spec on : and hex-decoding independently must always agree" is worth the duplication vs. just asserting non-panic + valid-range for the RGB bytes.
• FuzzDisplayModeString / FuzzStatusDecode: appropriately minimal (panic-freedom is really the only invariant available here). FuzzStatusDecode's seeds cover both packet sizes plus a couple of malformed lengths — reasonable given binary.Read against a fixed-size struct can't actually fail for a correctly-sized buffer, so there isn't much more to assert.
• Since these are described as running "as ordinary tests" via their seed corpora under plain go test ./..., double check that go vet/golangci-lint (if configured) doesn't flag the unused t in cases where the fuzz function doesn't call t.Fatalf on every path — not an issue here since all four do use t, just flagging as something to watch if more targets are added later.

CI / security

• .github/workflows/codeql.yml looks correct: manual go build ./... instead of autobuild is a good call for reproducibility with the pinned go.mod toolchain, security-events: write is scoped to the job not the workflow, and harden-runner + persist-credentials: false match the repo's existing security posture.
• No secrets or credential handling introduced. pull_request trigger with contents: read only is safe for a public repo with no privileged operations in the job.

Test coverage

• Fuzz corpora only run as seeded unit tests in CI (per the CLAUDE.md note), which is reasonable for keeping CI fast, but means the "real" fuzzing (-fuzz -fuzztime) is opt-in/manual and won't run automatically — consider a periodic scheduled workflow (e.g. weekly) that runs each target with a short -fuzztime if you want continuous coverage-guided fuzzing rather than just corpus replay, though that's a follow-up, not a blocker for this PR.

Nothing here blocks merging — the extractTimePart fix is correct and the fuzz/CodeQL additions are net positive for the project's security posture.