Fix issues #1, #3, #5: misc improvements #6

Merged

jared merged 5 commits from fix/issues-1-3-5-misc-improvements into master 2026-06-04 14:04:45 -04:00

Conversation 3 Commits 5 Files Changed 9

+361 -62

kodor commented 2026-06-03 15:02:11 -04:00

Member

Copy Link

Copy Source

Changes

Issue #5 — _brand.yml unwrapped for Quarto 1.9+ compatibility

The shipped inst/quarto/_brand.yml had its meta, logo, color, and typography keys nested under a top-level brand: wrapper. Quarto 1.9+ validates strictly and rejects this when auto-discovering a standalone _brand.yml.

Fix: Removed the brand: wrapper so all keys sit at the top level, matching the Quarto brand spec.

Issue #1 — na_sum() quiet parameter

na_sum() always emitted a message(), which is noisy when called inside loops or dplyr pipelines.

Fix: Added quiet = FALSE parameter. When quiet = TRUE, the message is suppressed.

Issue #3 — beep() notification function

New beep() function for CLI progress/completion alerts. Supports four notification types:

Type	Behavior
"beep"	Terminal bell character (\007)
"notify"	Desktop notification via notify-send (Linux) or osascript (macOS)
"webhook"	POST JSON to a URL (uses curl or wget)
"all"	Beep + desktop notification (+ webhook if URL provided)

Testing

• Added test for na_sum(quiet = TRUE) in test_utils.R
• Added test_notifications.R with tests for beep()

Related Issues

Closes #1, #3, #5

## Changes ### Issue #5 — `_brand.yml` unwrapped for Quarto 1.9+ compatibility The shipped `inst/quarto/_brand.yml` had its `meta`, `logo`, `color`, and `typography` keys nested under a top-level `brand:` wrapper. Quarto 1.9+ validates strictly and rejects this when auto-discovering a standalone `_brand.yml`. **Fix:** Removed the `brand:` wrapper so all keys sit at the top level, matching the [Quarto brand spec](https://quarto.org/docs/authoring/brand.html). ### Issue #1 — `na_sum()` quiet parameter `na_sum()` always emitted a `message()`, which is noisy when called inside loops or `dplyr` pipelines. **Fix:** Added `quiet = FALSE` parameter. When `quiet = TRUE`, the message is suppressed. ### Issue #3 — `beep()` notification function New `beep()` function for CLI progress/completion alerts. Supports four notification types: | Type | Behavior | |------|----------| | `"beep"` | Terminal bell character (`\007`) | | `"notify"` | Desktop notification via `notify-send` (Linux) or `osascript` (macOS) | | `"webhook"` | POST JSON to a URL (uses `curl` or `wget`) | | `"all"` | Beep + desktop notification (+ webhook if URL provided) | ## Testing - Added test for `na_sum(quiet = TRUE)` in `test_utils.R` - Added `test_notifications.R` with tests for `beep()` ## Related Issues Closes #1, #3, #5

kodor added 1 commit 2026-06-03 15:02:12 -04:00

Fix issues #1, #3, #5: misc improvements ... 2f1cdcde52

- #5: Unwrap _brand.yml for Quarto 1.9+ compatibility (remove top-level
  brand: wrapper so meta/logo/color/typography are at the top level)
- #1: Add quiet parameter to na_sum() to suppress warnings in loops/pipelines
- #3: Add beep() function for CLI beep, desktop notifications, and webhook
  alerts (supports type=beep|notify|webhook|all)

kodor referenced this pull request 2026-06-03 15:02:30 -04:00

use_civilytics_theme() ships _brand.yml wrapped in a top-level 'brand:' key that Quarto >=1.9 rejects #5

kodor referenced this pull request 2026-06-03 15:02:32 -04:00

Issuing a message from na_sum consumes resources #1

kodor referenced this pull request 2026-06-03 15:02:33 -04:00

Create a beep/notification function that works with CLI to send notifications about progress and completion to a webservice #3

kodor added 1 commit 2026-06-03 15:08:52 -04:00

Fix CI warnings: add jsonlite dep, flush.console import, Rd docs ... 75b9a73165

- Add jsonlite to Imports (used by beep() webhook)
- Add importFrom(utils, flush.console) to NAMESPACE
- Update na_sum.Rd to include quiet parameter
- Create beep.Rd documentation

kodor added 1 commit 2026-06-03 15:11:21 -04:00

Fix beep.Rd Rd syntax errors (remove invalid \char escape) fc8bf1866b

kodor commented 2026-06-04 08:53:15 -04:00

Author

Member

Copy Link

Copy Source

Code Review: PR #6 — Fix issues #1, #3, #5: misc improvements

Accuracy

• beep() function (notifications.R): The function correctly implements all four notification types (beep, notify, webhook, all). The match.arg() call properly handles type disambiguation. The grepl() checks for "beep", "notify", and "webhook" in the type string work correctly for the "all" case.
• na_sum() function (utils.R): The new quiet parameter is correctly implemented with a default of FALSE to preserve backward compatibility. The if (!quiet) guard properly suppresses the message.
• _brand.yml: The YAML indentation fix (removing the extra brand: nesting level) is correct — this matches the expected Quarto brand config format.

Completeness

• Test coverage:
  • test_notifications.R: Tests cover invisibility, quiet mode, notify fallback, and webhook-without-URL. However, the tests are minimal — they don't test the actual notification delivery (which is expected since it depends on external tools). Consider adding a test that mocks Sys.which() to verify the correct code path is taken.
  • test_utils.R: The new test for na_sum(quiet=TRUE) is good and covers the key behavior. The existing tests for na_sum are preserved.
• Documentation: The roxygen2 documentation for beep() is thorough — covers all parameters, requirements, and examples. The @export tag is present. The NAMESPACE and man pages are auto-generated correctly.
• DESCRIPTION/NAMESPACE: The jsonlite import is correctly added to DESCRIPTION and the flush.console import is added to NAMESPACE.

Coherence

• The beep() function is a new feature that fits well with the existing utility pattern in the package.
• The na_sum() change is a backward-compatible enhancement that doesn't break existing callers.
• The _brand.yml fix is a standalone correction that doesn't affect functionality.

Security

• beep() / .send_webhook(): The webhook URL and payload are interpolated into shell commands via paste0(). This is a potential command injection risk if the URL or message contains shell metacharacters. For example, a message containing "; rm -rf /; " would be executed by the shell.
  • Recommendation: Use system2() instead of system() with shell interpolation, or use R's built-in httr2/curl package for HTTP requests. If sticking with system(), at minimum validate/sanitize the URL and message parameters.
• .send_desktop_notify(): Similar shell injection risk via paste0() for the notify-send and osascript commands. The gsub() for single quotes in notify-send is a good start but doesn't handle all shell metacharacters.

Evaluation: ⚠️ Needs security fix before merge

Blocking issue:

1. Command injection vulnerability in .send_webhook() and .send_desktop_notify(). Both functions construct shell commands via string interpolation. Use system2() with separate args parameters instead of system() with interpolated strings. For example:

   # Instead of:
   cmd <- paste0("curl -s -X POST -d '", payload, "' '", url, "'")
   system(cmd)
   
   # Use:
   system2("curl", args = c("-s", "-X", "POST", "-d", payload, url))
   

Suggestions (non-blocking):

1. Add a test that verifies na_sum() returns the correct value (not just silence) when quiet=TRUE.
2. Consider adding a test for beep() with type="all" to verify both beep and notify paths execute.
3. The flush.console() call in the beep path is good but should also be used after the terminal beep to ensure the bell character is flushed immediately.

The code quality and documentation are excellent — the security fix is the only blocker.

## Code Review: PR #6 — Fix issues #1, #3, #5: misc improvements ### Accuracy - **`beep()` function** (notifications.R): The function correctly implements all four notification types (beep, notify, webhook, all). The `match.arg()` call properly handles type disambiguation. The `grepl()` checks for "beep", "notify", and "webhook" in the type string work correctly for the "all" case. - **`na_sum()` function** (utils.R): The new `quiet` parameter is correctly implemented with a default of `FALSE` to preserve backward compatibility. The `if (!quiet)` guard properly suppresses the message. - **`_brand.yml`**: The YAML indentation fix (removing the extra `brand:` nesting level) is correct — this matches the expected Quarto brand config format. ### Completeness - **Test coverage**: - `test_notifications.R`: Tests cover invisibility, quiet mode, notify fallback, and webhook-without-URL. However, the tests are minimal — they don't test the actual notification delivery (which is expected since it depends on external tools). Consider adding a test that mocks `Sys.which()` to verify the correct code path is taken. - `test_utils.R`: The new test for `na_sum(quiet=TRUE)` is good and covers the key behavior. The existing tests for `na_sum` are preserved. - **Documentation**: The roxygen2 documentation for `beep()` is thorough — covers all parameters, requirements, and examples. The `@export` tag is present. The NAMESPACE and man pages are auto-generated correctly. - **DESCRIPTION/NAMESPACE**: The `jsonlite` import is correctly added to DESCRIPTION and the `flush.console` import is added to NAMESPACE. ### Coherence - The `beep()` function is a new feature that fits well with the existing utility pattern in the package. - The `na_sum()` change is a backward-compatible enhancement that doesn't break existing callers. - The `_brand.yml` fix is a standalone correction that doesn't affect functionality. ### Security - **`beep()` / `.send_webhook()`**: The webhook URL and payload are interpolated into shell commands via `paste0()`. This is a **potential command injection risk** if the URL or message contains shell metacharacters. For example, a message containing `"; rm -rf /; "` would be executed by the shell. - **Recommendation**: Use `system2()` instead of `system()` with shell interpolation, or use R's built-in `httr2`/`curl` package for HTTP requests. If sticking with `system()`, at minimum validate/sanitize the URL and message parameters. - **`.send_desktop_notify()`**: Similar shell injection risk via `paste0()` for the `notify-send` and `osascript` commands. The `gsub()` for single quotes in `notify-send` is a good start but doesn't handle all shell metacharacters. ### Evaluation: ⚠️ Needs security fix before merge **Blocking issue:** 1. **Command injection vulnerability** in `.send_webhook()` and `.send_desktop_notify()`. Both functions construct shell commands via string interpolation. Use `system2()` with separate `args` parameters instead of `system()` with interpolated strings. For example: ```r # Instead of: cmd <- paste0("curl -s -X POST -d '", payload, "' '", url, "'") system(cmd) # Use: system2("curl", args = c("-s", "-X", "POST", "-d", payload, url)) ``` **Suggestions (non-blocking):** 1. Add a test that verifies `na_sum()` returns the correct value (not just silence) when `quiet=TRUE`. 2. Consider adding a test for `beep()` with `type="all"` to verify both beep and notify paths execute. 3. The `flush.console()` call in the beep path is good but should also be used after the terminal beep to ensure the bell character is flushed immediately. The code quality and documentation are excellent — the security fix is the only blocker.

kodor added 1 commit 2026-06-04 09:17:20 -04:00

fix: replace system() with system2() to prevent command injection in beep notifications ... 366ac39fac

Use system2() with separate args instead of shell-interpolated system()
calls in .send_desktop_notify() and .send_webhook(). This eliminates
command injection risk when msg, status, url, or payload contain shell
metacharacters.

kodor commented 2026-06-04 09:17:54 -04:00

Author

Member

Copy Link

Copy Source

Security fix pushed — replaced system() with system2() in both .send_desktop_notify() and .send_webhook().

What changed:

• notify-send: now uses system2("notify-send", args = c(status, msg)) — no shell interpolation
• osascript: now uses system2("osascript", args = c("-e", ...)) — no shell interpolation
• curl: now uses system2("curl", args = c("-s", "-X", "POST", "-H", ..., "-d", payload, url, ...)) — no shell interpolation
• wget: now uses system2("wget", args = c("-q", "-O", "/dev/null", paste0("--post-data=", payload), ...)) — no shell interpolation

system2() passes arguments directly to the executable without going through a shell, so shell metacharacters in msg, status, url, or payload are treated as literal data, not executable commands.

The PR should now be safe to merge.

Security fix pushed — replaced `system()` with `system2()` in both `.send_desktop_notify()` and `.send_webhook()`. **What changed:** - `notify-send`: now uses `system2("notify-send", args = c(status, msg))` — no shell interpolation - `osascript`: now uses `system2("osascript", args = c("-e", ...))` — no shell interpolation - `curl`: now uses `system2("curl", args = c("-s", "-X", "POST", "-H", ..., "-d", payload, url, ...))` — no shell interpolation - `wget`: now uses `system2("wget", args = c("-q", "-O", "/dev/null", paste0("--post-data=", payload), ...))` — no shell interpolation `system2()` passes arguments directly to the executable without going through a shell, so shell metacharacters in `msg`, `status`, `url`, or `payload` are treated as literal data, not executable commands. The PR should now be safe to merge.

kodor added 1 commit 2026-06-04 09:48:34 -04:00

test: add security regression test and beep(type='all') coverage ... 2a74ec2bd3

- Add test verifying .send_webhook() and .send_desktop_notify() use
  system2() (not system()) to prevent command injection. This test
  reads the function body and asserts system2 is present and
  system("...") is absent, so the vulnerability cannot be
  reintroduced by accident.
- Add test for beep(type='all') exercising both beep and notify paths.

kodor commented 2026-06-04 09:49:01 -04:00

Author

Member

Copy Link

Copy Source

Additional test coverage pushed:

1. Security regression test — verifies .send_webhook() and .send_desktop_notify() use system2() (not system() with interpolated strings). The test reads the function body and asserts system2 is present and system("...") is absent, so the command injection vulnerability cannot be reintroduced by accident.

2. beep(type='all') test — exercises both the beep and notify code paths to ensure they execute without error.

Note: the na_sum(quiet=TRUE) value test and flush.console() were already present in the codebase.

PR #6 should now be fully ready to merge.

Additional test coverage pushed: 1. **Security regression test** — verifies `.send_webhook()` and `.send_desktop_notify()` use `system2()` (not `system()` with interpolated strings). The test reads the function body and asserts `system2` is present and `system("...")` is absent, so the command injection vulnerability cannot be reintroduced by accident. 2. **`beep(type='all')` test** — exercises both the beep and notify code paths to ensure they execute without error. Note: the `na_sum(quiet=TRUE)` value test and `flush.console()` were already present in the codebase. PR #6 should now be fully ready to merge.

jared merged commit 41162597cf into master 2026-06-04 14:04:45 -04:00

jared referenced this issue from a commit 2026-08-09 14:21:11 -04:00

Fix ggplot2 imports and polish utility functions for publication

jared referenced this issue from a commit 2026-08-09 14:35:14 -04:00

Fix ggplot2 imports and polish utility functions for publication

Sign in to join this conversation.

Reviewers

No Reviewers

Labels

Clear labels

bug

Something isn't working

documentation

Improvements or additions to documentation

duplicate

This issue or pull request already exists

enhancement

New feature or request

good first issue

Good for newcomers

help wanted

Extra attention is needed

invalid

This doesn't seem right

kodor

Kodor should process this issue

kodor/feature-proposal

Kodor has written a feature proposal

kodor/fix

Kodor should implement a fix (assigned to Kodor)

kodor/needs-review

Kodor's work or failure needs Jared's review

kodor/triaged

Kodor has already triaged this issue (skip)

question

Further information is requested

wontfix

This will not be worked on

needs

human

Cannot move without a person -- a decision, a check an agent cannot make, something outside the repo

origin

client

Came from a client ask

origin

obligation

Created by a change elsewhere

origin

review

Came from human review

origin

roborev

Promoted from a roborev finding

type

chore

Maintenance with no behaviour change

type

debt

Owed work -- docs, tests, cleanup a change obligated

type

decision

Needs a decision before work can proceed

type

defect

Something is wrong

type

feature

New capability

ws

helpers

Analysis and workflow helpers

ws

logo

Logo and branded output composition

ws

packaging

Package infrastructure and release

ws

quarto

Quarto themes and publishing templates

ws

theme

Themes, palettes, and fonts

kodor

Assign a task to kodor

kodor/feature-proposal

Kodor thinks this needs a feature.

kodor/fix

Kodor should fix this

kodor/needs-review

Kodor thinks the user is ready to review this.

kodor/triaged

Kodor is done with this issue.

No labels

Milestone

No items

No Milestone

Projects

Clear projects

No projects

Assignees

Clear assignees

hannah (Hannah Miller) jared (Jared Knowles) kodor

No Assignees

1 Participants

Notifications

Subscribe

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: Civilytics/civilyticsR#6