Dev - #24
Merged
Merged
Conversation
Fix CI module naming and deterministic task ordering
…limit Harden upload lifecycle controls
Add CSRF mutation guard
Align runtime operational limits
Add visible author credit
Document terminal release downloads
Add product website surface
📝 WalkthroughWalkthroughThis PR adds server-side CSRF protection, mutation rate limiting, upload validation and cleanup, task store validation, graceful shutdown handling, installer and release updates, and a new deployment website. ChangesServer security and store hardening
Release, install, and website tooling
Estimated code review effort: 4 (Complex) | ~60 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Owner
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (7)
store.go (2)
176-192: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicated validation logic with
validateTaskText.The title/notes length checks here duplicate the exact logic and error messages already implemented in
validateTaskText/tooLong(Lines 373-381). Consider reusingvalidateTaskTextby passing the unchanged field fromtaskfor the field not being patched.♻️ Proposed refactor
if patch.Title != nil { title := strings.TrimSpace(*patch.Title) if title == "" { return Snapshot{}, Task{}, fmt.Errorf("%w: title is required", errBadInput) } - if tooLong(title, maxTitleLength) { - return Snapshot{}, Task{}, fmt.Errorf("%w: title must be at most %d characters", errBadInput, maxTitleLength) - } + if err := validateTaskText(title, task.Notes); err != nil { + return Snapshot{}, Task{}, err + } task.Title = title } if patch.Notes != nil { notes := strings.TrimSpace(*patch.Notes) - if tooLong(notes, maxNotesLength) { - return Snapshot{}, Task{}, fmt.Errorf("%w: notes must be at most %d characters", errBadInput, maxNotesLength) - } + if err := validateTaskText(task.Title, notes); err != nil { + return Snapshot{}, Task{}, err + } task.Notes = notes }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@store.go` around lines 176 - 192, The title and notes validation in the patch update path duplicates the existing `validateTaskText` logic and error messages. Refactor the `patch` handling in `store.go` to call `validateTaskText` for `task.Title` and `task.Notes`, reusing the current `task` values for any field not present in `patch` so the same validation path is used consistently.
383-396: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueManual UTF-16 code-unit counting reimplements stdlib
utf16.RuneLen.
tooLonghand-rolls the "2 units for supplementary-plane runes, else 1" logic thatunicode/utf16.RuneLenalready provides. Using the stdlib function would be more idiomatic and self-documenting about the intended semantics (matching JS.length).♻️ Proposed refactor
+import "unicode/utf16" + func tooLong(value string, max int) bool { units := 0 for _, r := range value { - if r > 0xFFFF { - units += 2 - } else { - units++ - } + units += utf16.RuneLen(r) if units > max { return true } } return false }Separately, please confirm this UTF-16-unit-based limit is intentional and consistent with any client-side character-count validation (e.g., in
static/app.js), since it differs from a plain rune/codepoint count for titles/notes containing emoji or other supplementary-plane characters.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@store.go` around lines 383 - 396, The manual UTF-16 counting in tooLong is reimplementing stdlib behavior; refactor tooLong to use unicode/utf16.RuneLen for each rune instead of hand-rolling the surrogate-pair logic. Keep the max-unit check semantics unchanged, and review any callers of tooLong to ensure the UTF-16 unit limit is intentional and still matches client-side validation such as static/app.js for titles/notes with emoji or other supplementary-plane characters..github/workflows/ci.yml (1)
23-27: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueConsider consolidating the redundant test runs.
go test -v ./...andgo test -race ./...both execute the full suite;go test -race -v ./...alone would cover both concerns in a single step.♻️ Proposed consolidation
- name: Test - run: go test -v ./... - - - name: Race test - run: go test -race ./... + run: go test -race -v ./...🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/ci.yml around lines 23 - 27, The CI workflow runs the full Go test suite twice in separate steps, which is redundant. Consolidate the existing Test and Race test steps in the workflow by using a single go test invocation with both verbose and race flags, and update the job definition around the test commands so the suite still runs once while preserving the same coverage.scripts/install.sh (1)
47-51: 🔒 Security & Privacy | 🔵 Trivial | 🏗️ Heavy liftNo integrity verification of downloaded release artifact.
The script downloads and installs a binary via
curl | sh-style flow with no checksum or signature verification. If the release archive is tampered with or served from a compromised mirror, this installs an unverified binary directly to the user's PATH.🔒 Suggested approach
Publish a
checksums.txt(e.g. via goreleaser) alongside releases and verify before install:download "$base_url/$archive" "$tmp_dir/$archive" +download "$base_url/checksums.txt" "$tmp_dir/checksums.txt" +(cd "$tmp_dir" && grep " $archive\$" checksums.txt | sha256sum -c -) tar -xzf "$tmp_dir/$archive" -C "$tmp_dir"🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/install.sh` around lines 47 - 51, The install flow in install.sh downloads and extracts the release archive without verifying integrity. Update the download/install path around download, tar extraction, and install to validate the artifact first, ideally by fetching a release checksums file and checking the archive hash before installing doit into install_dir. Keep the existing install logic but only proceed after verification succeeds, using the same archive and install_dir variables to locate the affected code.website/index.html (2)
93-98: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winFallback "select" label doesn't actually select the command text.
When
navigator.clipboard.writeTextfails (e.g. insecure context or denied permission), the button text changes to "select" but no selection is performed, leaving the user without an easy way to copy the command manually.♻️ Suggested fix: select the adjacent `
` text on fallback} catch { button.textContent = "select"; + const codeEl = button.previousElementSibling?.querySelector("code") ?? button.previousElementSibling; + if (codeEl) { + const range = document.createRange(); + range.selectNodeContents(codeEl); + window.getSelection()?.removeAllRanges(); + window.getSelection()?.addRange(range); + } window.setTimeout(() => {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@website/index.html` around lines 93 - 98, The fallback in the clipboard copy handler only changes the button label to “select” but does not actually select any text; update the catch path in the click handler around `navigator.clipboard.writeText` so it selects the adjacent command text from the related `<pre><code>` block. Use the existing button/label logic in that handler, and implement a real text selection fallback so the user can manually copy when clipboard access fails.
69-82: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueUnauthenticated GitHub API call from every page load.
Each visitor's browser hits the unauthenticated GitHub REST API directly with no caching, which is subject to GitHub's low anonymous rate limit; heavy traffic behind shared NATs/proxies could see frequent failures (silently falling back to "latest on GitHub", so degradation is graceful).
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@website/index.html` around lines 69 - 82, The release lookup in the page load script is making an unauthenticated GitHub API request on every visit, so update the logic around the fetch/releaseLink handling to avoid direct anonymous calls from each browser session. Use a cached or precomputed release URL/tag source, or route the lookup through a backend/proxy layer with caching, and keep the existing fallback behavior in the catch path.server.go (1)
746-776: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winFull client map scan on every
Allow()call.Every mutation request triggers a linear scan of up to
maxRateLimitClients(4096) entries to prune expired windows, all inside the single global mutex. This serializes every mutation request behind an O(n) critical section, which will add latency and lock contention as the client map grows.Consider pruning lazily (e.g., only sweep periodically via a background ticker, or amortize by evicting a bounded number of stale entries per call) rather than scanning the full map on every request.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@server.go` around lines 746 - 776, The rate limiter’s Allow method is doing a full l.clients scan on every call, which creates an O(n) critical section under l.mu and increases contention. Update rateLimiter.Allow to avoid sweeping all expired entries per request by pruning lazily, such as with a periodic background cleanup or by evicting only a bounded number of stale clients per invocation, while preserving the existing limit and window behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@server.go`:
- Around line 648-673: Keep the HTTP method checks in sync between isMutation
and requiresCSRF in server.go: requiresCSRF currently omits http.MethodPut while
isMutation includes it, so update requiresCSRF to treat PUT the same way as the
other mutating API methods. Use the existing isMutation/requiresCSRF switch
patterns as the source of truth so future API method support cannot bypass CSRF
protection.
In `@website/index.html`:
- Around line 84-100: The copy-button handler in the data-copy click listener
can capture “copied” as the restore label on rapid repeated clicks, leaving the
button stuck. Update the click logic so it preserves the true original label for
each button in a stable place (outside the async handler, keyed by the button or
via a dataset field) and reuse it on every restore. Also clear any pending
timeout before scheduling a new one in the same handler so only the latest click
controls the label reset.
---
Nitpick comments:
In @.github/workflows/ci.yml:
- Around line 23-27: The CI workflow runs the full Go test suite twice in
separate steps, which is redundant. Consolidate the existing Test and Race test
steps in the workflow by using a single go test invocation with both verbose and
race flags, and update the job definition around the test commands so the suite
still runs once while preserving the same coverage.
In `@scripts/install.sh`:
- Around line 47-51: The install flow in install.sh downloads and extracts the
release archive without verifying integrity. Update the download/install path
around download, tar extraction, and install to validate the artifact first,
ideally by fetching a release checksums file and checking the archive hash
before installing doit into install_dir. Keep the existing install logic but
only proceed after verification succeeds, using the same archive and install_dir
variables to locate the affected code.
In `@server.go`:
- Around line 746-776: The rate limiter’s Allow method is doing a full l.clients
scan on every call, which creates an O(n) critical section under l.mu and
increases contention. Update rateLimiter.Allow to avoid sweeping all expired
entries per request by pruning lazily, such as with a periodic background
cleanup or by evicting only a bounded number of stale clients per invocation,
while preserving the existing limit and window behavior.
In `@store.go`:
- Around line 176-192: The title and notes validation in the patch update path
duplicates the existing `validateTaskText` logic and error messages. Refactor
the `patch` handling in `store.go` to call `validateTaskText` for `task.Title`
and `task.Notes`, reusing the current `task` values for any field not present in
`patch` so the same validation path is used consistently.
- Around line 383-396: The manual UTF-16 counting in tooLong is reimplementing
stdlib behavior; refactor tooLong to use unicode/utf16.RuneLen for each rune
instead of hand-rolling the surrogate-pair logic. Keep the max-unit check
semantics unchanged, and review any callers of tooLong to ensure the UTF-16 unit
limit is intentional and still matches client-side validation such as
static/app.js for titles/notes with emoji or other supplementary-plane
characters.
In `@website/index.html`:
- Around line 93-98: The fallback in the clipboard copy handler only changes the
button label to “select” but does not actually select any text; update the catch
path in the click handler around `navigator.clipboard.writeText` so it selects
the adjacent command text from the related `<pre><code>` block. Use the existing
button/label logic in that handler, and implement a real text selection fallback
so the user can manually copy when clipboard access fails.
- Around line 69-82: The release lookup in the page load script is making an
unauthenticated GitHub API request on every visit, so update the logic around
the fetch/releaseLink handling to avoid direct anonymous calls from each browser
session. Use a cached or precomputed release URL/tag source, or route the lookup
through a backend/proxy layer with caching, and keep the existing fallback
behavior in the catch path.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 52783bc3-6451-42f8-8f4d-a8774024c3f6
📥 Commits
Reviewing files that changed from the base of the PR and between 6a51e0e65a6ce0052d181b6a3ea177c106c6ef51 and 7fb5a60fcba21e9c45b6435131c232606a929e3c.
⛔ Files ignored due to path filters (1)
website/assets/product-preview.svg is excluded by !**/*.svg
📒 Files selected for processing (18)
.github/workflows/ci.yml.gitignore.vercelignoreREADME.mddocs/ARCHITECTURE.mdgo.modmain.goscripts/install.shserver.goserver_test.gostatic/app.cssstatic/app.jsstatic/index.htmlstore.gostore_test.govercel.jsonwebsite/index.htmlwebsite/styles.css
There was a problem hiding this comment.
🧹 Nitpick comments (1)
website/index.html (1)
82-82: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCopy status changes aren't announced to screen readers.
Button text flips to "copied"/"select" but there's no
aria-live/role="status"region, so assistive tech users may not perceive the state change.♻️ Suggested fix: add a visually-hidden live region
- button.addEventListener("click", async () => { + const status = button.querySelector(".sr-status") ?? (() => { + const el = document.createElement("span"); + el.className = "sr-status visually-hidden"; + el.setAttribute("aria-live", "polite"); + button.appendChild(el); + return el; + })(); + button.addEventListener("click", async () => { clearTimeout(revertTimer); try { await navigator.clipboard.writeText(button.dataset.copy); button.textContent = "copied"; + status.textContent = "Copied to clipboard"; restoreLabel();Also applies to: 92-92
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@website/index.html` at line 82, The copy button state changes in the inline script are not announced to assistive technologies. Update the copy/select flow around the button text updates in the relevant handlers so that state changes are also sent to a visually hidden live region using an aria-live/role=status element. Use the existing button update logic in the code that sets “copied” and “select” to keep the live region in sync with those changes.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@website/index.html`:
- Line 82: The copy button state changes in the inline script are not announced
to assistive technologies. Update the copy/select flow around the button text
updates in the relevant handlers so that state changes are also sent to a
visually hidden live region using an aria-live/role=status element. Use the
existing button update logic in the code that sets “copied” and “select” to keep
the live region in sync with those changes.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: ad17952f-d1b7-400e-a848-c8910d592315
📒 Files selected for processing (7)
.github/workflows/ci.yml.goreleaser.yamlscripts/install.shserver.goserver_test.gostore.gowebsite/index.html
🚧 Files skipped from review as they are similar to previous changes (3)
- scripts/install.sh
- store.go
- server.go
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@scripts/install.sh`:
- Around line 88-92: The checksum download fallback in the install flow is too
permissive and skips integrity verification on fetch failures. Update the logic
around download_optional in the install script so the checksums.txt path fails
closed for any transport or fetch error, and only allows the legacy no-checksum
path when the response is a real 404. Use the existing verify_checksum and
download_optional flow to distinguish not-found from transient/network failures,
and abort installation when checksums.txt cannot be retrieved for any other
reason.
- Around line 47-60: The download_optional helper currently treats any curl/wget
failure as a missing checksum, so update its callers and error handling to
distinguish a real 404 from transport/TLS/DNS failures. In download_optional and
the checksum-fetch logic it feeds, inspect the fetch result so only a 404
triggers the “checksums unavailable” fallback; for any other nonzero exit, fail
closed and stop the install. Use the download_optional function and the checksum
download path in scripts/install.sh to locate the change.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
Comment on lines
+47
to
+60
| download_optional() { | ||
| url="$1" | ||
| output="$2" | ||
| if command -v curl >/dev/null 2>&1; then | ||
| curl -fsSL "$url" -o "$output" >/dev/null 2>&1 && return 0 | ||
| return 1 | ||
| fi | ||
| if command -v wget >/dev/null 2>&1; then | ||
| wget -qO "$output" "$url" >/dev/null 2>&1 && return 0 | ||
| return 1 | ||
| fi | ||
| echo "curl or wget is required" >&2 | ||
| exit 1 | ||
| } |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the target file around the referenced lines.
if [ -f scripts/install.sh ]; then
wc -l scripts/install.sh
sed -n '1,180p' scripts/install.sh | cat -n
fi
# Find where download_optional is used.
rg -n "download_optional|download\(" scripts/install.shRepository: blackdragoon26/Do-It
Length of output: 3633
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Check whether releases always publish checksums.txt or if it is genuinely optional.
rg -n "checksums\.txt|sha256|release" .github scripts README* .Repository: blackdragoon26/Do-It
Length of output: 4034
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the release workflow and installer docs to see whether checksums.txt is expected.
sed -n '1,220p' .github/workflows/release.yml | cat -n
printf '\n---- README excerpt ----\n'
sed -n '45,90p' README.md | cat -nRepository: blackdragoon26/Do-It
Length of output: 2869
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Check for any GoReleaser config that could disable checksums or rename assets.
fd -a -H -t f '(.goreleaser.*|goreleaser.*)' .Repository: blackdragoon26/Do-It
Length of output: 194
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect GoReleaser config to see whether checksums.txt is always emitted or can be missing.
wc -l .goreleaser.yaml
sed -n '1,260p' .goreleaser.yaml | cat -nRepository: blackdragoon26/Do-It
Length of output: 1963
download_optional treats every fetch error as “checksums missing.” scripts/install.sh:47-52,88-91
A transient curl/wget failure now falls back to an unverified install; only skip verification for a real 404 and fail closed on transport/TLS/DNS errors.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@scripts/install.sh` around lines 47 - 60, The download_optional helper
currently treats any curl/wget failure as a missing checksum, so update its
callers and error handling to distinguish a real 404 from transport/TLS/DNS
failures. In download_optional and the checksum-fetch logic it feeds, inspect
the fetch result so only a 404 triggers the “checksums unavailable” fallback;
for any other nonzero exit, fail closed and stop the install. Use the
download_optional function and the checksum download path in scripts/install.sh
to locate the change.
Comment on lines
+88
to
+92
| if download_optional "$base_url/checksums.txt" "$tmp_dir/checksums.txt"; then | ||
| verify_checksum "$tmp_dir/$archive" "$tmp_dir/checksums.txt" | ||
| else | ||
| echo "checksums.txt not found for this release; installing without checksum verification" >&2 | ||
| fi |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== scripts/install.sh ==\n'
ast-grep outline scripts/install.sh --view expanded || true
printf '\n\n-- relevant lines --\n'
sed -n '1,140p' scripts/install.sh
printf '\n== release config candidates ==\n'
git ls-files | rg '(^|/)(\.goreleaser(\.ya?ml)?|goreleaser(\.ya?ml)?|release).*'
printf '\n== search for checksums.txt and DOIT_VERSION ==\n'
rg -n 'checksums\.txt|DOIT_VERSION|download_optional|verify_checksum' -S .Repository: blackdragoon26/Do-It
Length of output: 3552
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== .goreleaser.yaml ==\n'
wc -l .goreleaser.yaml
sed -n '1,220p' .goreleaser.yaml
printf '\n== release workflow ==\n'
wc -l .github/workflows/release.yml
sed -n '1,220p' .github/workflows/release.yml
printf '\n== docs mention install script ==\n'
rg -n 'scripts/install\.sh|DOIT_VERSION|checksums\.txt|goreleaser' README.md .github . -g '!scripts/install.sh'Repository: blackdragoon26/Do-It
Length of output: 3185
Fail closed when checksums.txt can’t be fetched.
.goreleaser.yaml always emits checksums.txt, so any fetch failure here should not silently skip integrity checking. Treat only a real 404 as the legacy fallback; transport errors should abort the install instead of proceeding unverified.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@scripts/install.sh` around lines 88 - 92, The checksum download fallback in
the install flow is too permissive and skips integrity verification on fetch
failures. Update the logic around download_optional in the install script so the
checksums.txt path fails closed for any transport or fetch error, and only
allows the legacy no-checksum path when the response is a real 404. Use the
existing verify_checksum and download_optional flow to distinguish not-found
from transient/network failures, and abort installation when checksums.txt
cannot be retrieved for any other reason.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit