From 63b4b9bf17e52e56f18627482b49e291a897ee38 Mon Sep 17 00:00:00 2001 From: Graham Watts <34165628+GingerGraham@users.noreply.github.com> Date: Mon, 20 Apr 2026 09:33:27 +0100 Subject: [PATCH 1/3] docs: add claude skills for linting, pre-PR checks, running tests, security review, writing commits, and writing tests --- .claude/skills/linting-code/SKILL.md | 91 +++++++++ .claude/skills/pre-pr-checks/SKILL.md | 98 ++++++++++ .claude/skills/running-tests/SKILL.md | 110 +++++++++++ .claude/skills/security-review/SKILL.md | 240 ++++++++++++++++++++++++ .claude/skills/writing-commits/SKILL.md | 155 +++++++++++++++ .claude/skills/writing-tests/SKILL.md | 133 +++++++++++++ 6 files changed, 827 insertions(+) create mode 100644 .claude/skills/linting-code/SKILL.md create mode 100644 .claude/skills/pre-pr-checks/SKILL.md create mode 100644 .claude/skills/running-tests/SKILL.md create mode 100644 .claude/skills/security-review/SKILL.md create mode 100644 .claude/skills/writing-commits/SKILL.md create mode 100644 .claude/skills/writing-tests/SKILL.md diff --git a/.claude/skills/linting-code/SKILL.md b/.claude/skills/linting-code/SKILL.md new file mode 100644 index 0000000..e68a67a --- /dev/null +++ b/.claude/skills/linting-code/SKILL.md @@ -0,0 +1,91 @@ +--- +name: linting-code +description: Lints shell scripts and Markdown files in the bash-logger repository using the project's configured toolchain. Use this skill whenever checking code quality, fixing linting errors, or running lint as part of a pre-commit or pre-PR workflow. +--- + +# Linting Code + +## Tools and where configuration lives + +| Tool | Checks | Config | +|---|---|---| +| ShellCheck | Shell syntax, quoting, portability, common bugs | `.pre-commit-config.yaml` (args: `--severity=warning --external-sources`) | +| MarkdownLint | Markdown formatting, list style, line length | `.markdownlint.yaml` | + +Both run through `pre-commit` so versions are pinned. Always use the Make targets or `pre-commit` directly — do not invoke `shellcheck` or `markdownlint` as standalone commands. + +## Commands + +### Lint everything (shell + Markdown) + +```bash +make lint +``` + +### Shell scripts only + +```bash +make lint-shell +``` + +### Markdown only + +```bash +make lint-markdown +``` + +### Run all pre-commit hooks including tests + +```bash +make pre-commit +``` + +### Run a single hook by name + +```bash +pre-commit run shellcheck --all-files +pre-commit run markdownlint --all-files +``` + +## Key rules — write clean code from the start + +### Shell scripts + +* 4-space indentation, no tabs +* Lines ≤ 100 characters +* Quote all variable expansions: `"$var"` not `$var` +* `[[ ]]` for bash conditionals; `[ ]` only when POSIX portability is needed +* `$(...)` not backticks +* `UPPERCASE` for constants/exported vars; `lowercase` for locals and functions +* When suppressing a ShellCheck warning, add an explanation comment: + ```bash + # shellcheck disable=SC3010 -- bash-specific syntax required for performance here + ``` + +### Markdown + +* Unordered list markers: `*` only — **never** `-` +* 2-space list indentation +* Maximum line length: 200 characters (code blocks and tables are exempt) +* Blank line above and below every heading +* Specify a language on all fenced code blocks where possible + +## Interpreting failures + +### ShellCheck + +Format: `file.sh:LINE:COL: severity: message [SCXXX]` + +Look up unknown codes at `https://www.shellcheck.net/wiki/SCXXX`. + +Most common codes in this project: + +| Code | Cause | Fix | +|---|---|---| +| SC2086 | Unquoted variable | Wrap in `"$var"` | +| SC2155 | Combined `local`/assign | Split: `local x; x=$(...)` | +| SC2181 | Check `$?` instead of direct `if` | Use `if command; then` directly | + +### MarkdownLint + +MarkdownLint runs with `--fix` — most formatting issues auto-correct and the file is re-staged. Violations that cannot be auto-fixed (long lines, wrong list markers) require manual edits. diff --git a/.claude/skills/pre-pr-checks/SKILL.md b/.claude/skills/pre-pr-checks/SKILL.md new file mode 100644 index 0000000..3908269 --- /dev/null +++ b/.claude/skills/pre-pr-checks/SKILL.md @@ -0,0 +1,98 @@ +--- +name: pre-pr-checks +description: Runs the full gate sequence before raising a pull request on bash-logger. Covers linting, targeted tests, full test suite, code review checklist, commit message audit, PR description requirements, and branch hygiene. Use this skill before pushing to a shared branch or opening a PR. +--- + +# Pre-PR Checks + +Work through these steps in order. Each builds on the last. + +## 1. Lint + +```bash +make lint +``` + +Fix all ShellCheck and MarkdownLint failures. MarkdownLint auto-fixes most issues and re-stages files — review them before continuing. + +## 2. Targeted tests + +Identify relevant suites (see `running-tests` skill) and run them: + +```bash +./tests/run_tests.sh +``` + +All must pass before continuing. + +## 3. Full test suite + +```bash +./tests/run_tests.sh +``` + +Zero failures required. Investigate all regressions before pushing. + +## 4. Code review checklist + +### Shell scripts (`logging.sh`, `install.sh`, scripts) + +* [ ] Every exit path assigns the correct return/exit code — passing tests do not guarantee all exit paths are correct +* [ ] Variables quoted throughout: `"$var"` +* [ ] No hardcoded paths that break portability +* [ ] New functions have comments explaining purpose and parameters +* [ ] Shell-agnostic where possible; bash-specific constructs commented with ShellCheck suppressions +* [ ] Lines ≤ 100 characters +* [ ] No leftover debug `echo` or `set -x` calls + +### Test files + +* [ ] Every new test function called at the bottom of its suite file +* [ ] `|| return` follows every assertion +* [ ] All I/O uses `$TEST_DIR` + +### Documentation + +* [ ] New flags, functions, or behaviour documented in the relevant `.md` file +* [ ] If `logging.sh` public API changed, `README.md` and `docs/` updated +* [ ] Markdown uses `*` list markers, 2-space indent, ≤ 200 char lines + +## 5. Commit message audit + +```bash +git log origin/main..HEAD --oneline +``` + +Every commit must follow `(): ` format. Amend any that do not: + +```bash +git rebase -i origin/main # reword the offending commits +``` + +If a commit addresses a tracked issue, confirm `Fixes #NNN` or `Refs #NNN` is in the footer. See the `writing-commits` skill for the full specification. + +## 6. PR description + +* Title: `(): ` matching the primary intent of the PR +* Body must include: summary, list of changes, testing steps taken, and issue links +* All related issues linked — at minimum the issue that prompted the work + +## 7. Branch hygiene + +```bash +git fetch origin +git rebase origin/main +``` + +Resolve conflicts, then re-run the full test suite. + +## Quick-reference: common pre-PR failures + +| Symptom | Cause | Fix | +|---|---|---| +| ShellCheck SC2086 | Unquoted variable | `"$var"` | +| ShellCheck SC2155 | Combined declare/assign | `local x; x=$(...)` | +| MarkdownLint MD004 | List marker is `-` | Change to `*` | +| Test not run | Call missing at bottom of suite | Add the call | +| Assertion not reached | Missing `\|\| return` on prior assertion | Add `\|\| return` | +| No release triggered | Commit type not release-triggering | Check `.releaserc.json` | diff --git a/.claude/skills/running-tests/SKILL.md b/.claude/skills/running-tests/SKILL.md new file mode 100644 index 0000000..3a47fdd --- /dev/null +++ b/.claude/skills/running-tests/SKILL.md @@ -0,0 +1,110 @@ +--- +name: running-tests +description: Runs the bash-logger test suite — either the full suite or a targeted subset. Use this skill whenever running, selecting, or interpreting tests in this repository. Covers discovering available suites, choosing which to run after a change, correct invocation syntax, and reading test output. +--- + +# Running Tests + +## Discover available suites first + +Never assume which suites exist. Always discover them at runtime: + +```bash +ls tests/test_*.sh | grep -v test_example.sh | grep -v test_helpers.sh | sed 's|tests/||; s|\.sh||' +``` + +This lists every runnable suite name. `test_example.sh` is a contributor template and `test_helpers.sh` is a shared library — neither is a runnable suite. + +## Choosing: targeted vs full suite + +Default to targeted. Only run the full suite for broad changes or as a pre-PR gate. + +| What changed | Run | +|---|---| +| A specific functional area (e.g. format handling, log levels) | The matching suite(s) only | +| `logging.sh` core logic (init, output routing, sanitization) | Full suite | +| `install.sh` | `test_install` only | +| Config file parsing | `test_config` and `test_config_security` | +| Security-related code | Relevant security suites + full suite | +| New test file added | New suite only, then full suite | +| Pre-PR / CI gate | Full suite | + +### Mapping changed code to suites + +After discovering available suites, match by name: + +* `logging.sh` format/template changes → `test_format` +* `logging.sh` level logic → `test_log_levels` +* `logging.sh` initialisation → `test_initialization` +* `logging.sh` output/stream routing → `test_output` +* Config file support → `test_config`, `test_config_security`, `test_runtime_config` +* Journal integration → `test_journal_logging` +* Sanitisation (ANSI, newlines, paths) → `test_ansi_injection`, `test_unsafe_newlines`, `test_path_traversal`, `test_script_name_sanitization` +* Security hardening → `test_environment_security`, `test_sensitive_data`, `test_config_security`, `test_toctou_protection` +* `install.sh` → `test_install` +* JUnit/CI reporting → `test_junit_output` + +When in doubt about scope, run the full suite. + +## Commands + +### Full suite + +```bash +./tests/run_tests.sh +``` + +Or via Make (quieter output): + +```bash +make test +``` + +### Single suite + +```bash +./tests/run_tests.sh +``` + +`` is the filename without the `test_` prefix **and** without `.sh`: + +```bash +# Correct +./tests/run_tests.sh log_levels +./tests/run_tests.sh config +./tests/run_tests.sh initialization + +# Also accepted — runner strips the extension +./tests/run_tests.sh test_log_levels.sh +``` + +### Multiple suites in one invocation + +```bash +./tests/run_tests.sh config config_security runtime_config +``` + +### JUnit XML output (for CI artefacts) + +```bash +make test-junit +``` + +## Reading output + +``` +✓ (green) — passed +✗ (red) — failed; expected/actual values printed below +⊘ (yellow) — skipped (e.g. a zsh-only test running under bash) +``` + +Summary line: `Total Tests: N Passed: N Failed: N Skipped: N` + +Exit code `0` = all passed. Exit code `1` = at least one failure. Fix all failures before committing. + +## Mistakes to avoid + +* Do **not** run from inside the `tests/` directory — always run from repo root. +* Do **not** pass the full path (`tests/test_config.sh`) — pass the suite name only. +* Do **not** skip the discovery step and guess suite names — the list changes as the project grows. +* `test_helpers.sh` and `test_example.sh` are never valid suite arguments. diff --git a/.claude/skills/security-review/SKILL.md b/.claude/skills/security-review/SKILL.md new file mode 100644 index 0000000..c007091 --- /dev/null +++ b/.claude/skills/security-review/SKILL.md @@ -0,0 +1,240 @@ +--- +name: security-review +description: Reviews code changes in bash-logger for security issues across all protected domains. Use this skill whenever adding new features that handle external input, changing sanitization behaviour, adding config options, or reviewing a PR for security. Covers input sanitization, file system safety, config injection, environment variable attacks, journal command validation, sensitive data handling, and unsafe flag governance. +--- + +# Security Review + +## When to use this skill + +Apply to any change that touches: + +* Message processing or formatting (`_sanitize_log_message`, `_strip_ansi_codes`, format strings) +* File path handling (`init_logger`, `_parse_config_file`, `_validate_config_file_path`) +* Configuration parsing (new config keys, value parsing, `_parse_config_file`) +* Journal/`logger` integration (`_find_and_validate_logger`, `_write_to_journal`) +* Runtime setters (`set_*` functions, `--unsafe-*` flags) +* Any new public API function that accepts external input + +--- + +## Domain 1: Input sanitization (log injection) + +Every message written to file or journal passes through `_sanitize_log_message`, which calls +both `_strip_ansi_codes` (strips CSI, OSC, DCS, PM, APC sequences) and newline replacement. + +**What to check:** + +* New `_log_message` callers pass user-supplied content as the `$message` argument — not + as part of `$level_name`, which is never sanitized (it comes from internal constants only). +* No new code path writes directly to `$LOG_FILE` without first calling `_sanitize_log_message`. +* If you add a new ANSI sequence type, update `_strip_ansi_codes` **and** add a test in + `test_ansi_injection.sh`. +* Format string changes: `LOG_FORMAT` uses `%d`, `%l`, `%s`, `%m`, `%z` only. Never + interpolate raw user data into the format string itself. + +**Unsafe flags:** + +`LOG_UNSAFE_ALLOW_ANSI_CODES` and `LOG_UNSAFE_ALLOW_NEWLINES` bypass sanitization. Their +setter functions (`set_unsafe_allow_*`, `--unsafe-allow-*` CLI flags) should: + +* Always emit a WARNING to console when enabling +* Accept and validate boolean input — reject unrecognised values +* Write the mode change to the log file and journal so the audit trail shows the unsafe window + +Never read `LOG_UNSAFE_*` inside `_format_log_message` or any library-generated output path +— only the user-message path should consult them. + +--- + +## Domain 2: File system security (TOCTOU, symlinks, path traversal) + +Log file creation in `init_logger` uses noclobber (`set -C; : > "$LOG_FILE"`) followed by +immediate validation to minimize the TOCTOU window. + +**What to check:** + +* No existence check (`-f`, `-e`) before file creation — this re-opens the TOCTOU window. + Always create first, then validate. +* After any file creation: check `-L` (reject symlinks), `-f` (must be regular file), `-w` + (must be writable). These three checks are mandatory in that order. +* Path traversal: file paths from config must pass `_validate_config_file_path`, which + enforces absolute paths and rejects injection patterns. CLI paths are not validated by + `_validate_config_file_path` — if you add a new path-accepting CLI flag, apply the same + absolute-path and injection-pattern checks. +* Never use `eval` or unquoted variable expansion in path construction. + +**Test suites for this domain:** + +```bash +./tests/run_tests.sh toctou_protection +./tests/run_tests.sh path_traversal +``` + +--- + +## Domain 3: Configuration file parsing (injection, length limits) + +`_parse_config_file` reads untrusted INI content. All values pass through +`_validate_config_value_length` (max `CONFIG_MAX_VALUE_LENGTH` = 4096) before any +type-specific validation. + +**What to check when adding a new config key:** + +1. Add the key to the `case` block in `_parse_config_file`. +2. Apply the appropriate validator: + * Free-form string → `_validate_string` with a sensible max length and `check_control_chars=true` + * File path → `_validate_config_file_path` + * Boolean → `_parse_bool_value` + * Enum (e.g. log level, facility) → existing typed validator or a new allowlist `case` + * Journal tag → `_validate_config_journal_tag` +3. Never `eval` a config value. Never use `source` on config content. +4. Verify shell metacharacters (`$`, `` ` ``, `;`, `|`, `&`, `<`, `>`) cannot reach a + command execution context via the new key's handling code. +5. Add the new key name to the "Valid keys" hint in the `*` fallback branch. + +**Test suites for this domain:** + +```bash +./tests/run_tests.sh config_security +./tests/run_tests.sh config +``` + +--- + +## Domain 4: Environment variable security + +On source, `logging.sh` unsets known environment variables before setting them as `readonly` +constants (log level integers, color codes). This prevents a pre-set environment from +poisoning constants. + +**What to check:** + +* New global constants that should not be user-overridable: unset before assignment, then + declare `readonly`. Follow the existing guard pattern: + + ```bash + if ! readonly -p 2>/dev/null | grep -q "declare -[^ ]*r[^ ]* MY_CONST="; then + unset MY_CONST 2>/dev/null || true + readonly MY_CONST="value" + fi + ``` + +* New mutable state variables (like `LOG_FILE`): these must not be `readonly`. Ensure they + are reset to a safe default on each `source` so pre-set environment values cannot persist. +* `LOGGER_FILE_ERROR_REPORTED` and `LOGGER_JOURNAL_ERROR_REPORTED`: explicitly unset on + source so a pre-set env value cannot permanently suppress error reporting. Any new + "reported once" deduplication flag needs the same treatment. +* The `IFS`, `PATH`, and other critical shell variables are not modified by the library. + Do not add code that sets or relies on a modified `IFS`. + +**Test suites for this domain:** + +```bash +./tests/run_tests.sh environment_security +``` + +--- + +## Domain 5: Journal / logger command validation + +`_find_and_validate_logger` resolves the `logger` binary via `command -v`, then resolves +symlinks via `readlink -f`, then validates the real path against an allowlist: +`/bin/logger`, `/usr/bin/logger`, `/usr/local/bin/logger`, `/sbin/logger`, `/usr/sbin/logger`. + +Once validated, `LOGGER_PATH` is set `readonly`. A changed path after lock triggers a +warning and disables journal logging rather than accepting the new path. + +**What to check:** + +* All journal writes use `"$LOGGER_PATH"` (the validated path), never `logger` or + `$(command -v logger)` directly. +* New code that invokes external commands for any purpose must apply the same + pattern: resolve → validate location → lock as readonly. +* The tag passed to `_write_to_journal` is always validated by `_validate_journal_tag` + before use. Never pass raw user input as the tag argument. +* Syslog facility is validated by `_validate_syslog_facility` against a strict allowlist + before being used in a `logger -p` invocation. + +--- + +## Domain 6: Sensitive data + +`log_sensitive` routes messages to console only (`skip_file=true`, `skip_journal=true`). +It bypasses the log-level filter (`force_show=true`) so sensitive output is always visible +during interactive sessions regardless of level configuration. + +**What to check:** + +* `log_sensitive` must never write to `$LOG_FILE`. Verify `skip_file` is `"true"` and not + accidentally cleared by a new parameter reordering in `_log_message`. +* `log_sensitive` must never write to the journal. Verify `skip_journal` is `"true"`. +* No new feature should add a `--log-sensitive-to-file` option or equivalent — this + would defeat the purpose of the function. +* Documentation: any new example in `docs/` or `demo-scripts/` that shows credentials, + tokens, or passwords must use `log_sensitive`, not `log_info` or `log_debug`. + +**Test suites for this domain:** + +```bash +./tests/run_tests.sh sensitive_data +``` + +--- + +## Domain 7: Script name sanitization + +`_sanitize_script_name` strips everything outside `[a-zA-Z0-9._-]`. It is applied to +the auto-detected caller script name, any `--name` CLI argument, any `script_name` config +value, and `SCRIPT_NAME` at the end of `init_logger` regardless of source. + +**What to check:** + +* Any new place that sets `SCRIPT_NAME` must call `_sanitize_script_name` on the value. +* The sanitized name is used in the INIT log message and every subsequent log entry; an + unsanitized name would inject arbitrary content into every log line. + +--- + +## Security test suite map + +| Area | Test suite(s) | +|---|---| +| ANSI injection | `test_ansi_injection` | +| Log injection (newlines) | `test_unsafe_newlines` | +| Path traversal | `test_path_traversal` | +| TOCTOU / symlink attacks | `test_toctou_protection` | +| Config file injection | `test_config_security` | +| Environment variable attacks | `test_environment_security` | +| Sensitive data isolation | `test_sensitive_data` | +| Script name sanitization | `test_script_name_sanitization` | + +Run all security suites together: + +```bash +./tests/run_tests.sh ansi_injection unsafe_newlines path_traversal toctou_protection \ + config_security environment_security sensitive_data script_name_sanitization +``` + +After any security-related change, always follow with the full suite: + +```bash +./tests/run_tests.sh +``` + +--- + +## Security checklist before committing + +* [ ] New external input is sanitized before being written to file, journal, or console +* [ ] No new code path bypasses `_sanitize_log_message` +* [ ] No new constant is set without unset + readonly guard against env override +* [ ] New mutable state is reset to safe default on source +* [ ] New file path handling uses noclobber → symlink check → regular-file check → writable check +* [ ] New config key uses an appropriate typed validator; metacharacter injection is not possible +* [ ] Journal command is always invoked via `$LOGGER_PATH`, not `logger` directly +* [ ] `log_sensitive` still skips file and journal (parameters not reordered) +* [ ] Any new public setter validates its input and rejects unrecognised values +* [ ] Unsafe flag changes still emit WARNING log entries +* [ ] All relevant security test suites pass +* [ ] Full test suite passes diff --git a/.claude/skills/writing-commits/SKILL.md b/.claude/skills/writing-commits/SKILL.md new file mode 100644 index 0000000..2c31b94 --- /dev/null +++ b/.claude/skills/writing-commits/SKILL.md @@ -0,0 +1,155 @@ +--- +name: writing-commits +description: Generates correctly formatted commit messages and pull request descriptions for this repository. Uses Conventional Commits / semantic-release format with issue references. Use this skill whenever writing, reviewing, or amending a commit message or drafting a PR title and body. +--- + +# Writing Commits and Pull Requests + +## Commit message format + +``` +(): + + + +