docs(#179): add shell-safety heuristics to review correctness #212
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -89,6 +89,20 @@ dimension carry over to another — each requires its own scrutiny. | |
| (step 2), determine whether those changes weakened coverage. | ||
| - Split-payload attacks: a production change paired with a test | ||
| modification that masks the real behavior. | ||
| - Shell safety in harness scripts (`post-code.sh`, `post-fix.sh`, | ||
|
There was a problem hiding this comment. [high] commit-prefix PR title uses docs(#179): but COMMITS.md line 42 explicitly says 'Tightening internal heuristics, adjusting prompts, or tuning agent behavior that users don't directly control → refactor or fix depending on whether it corrects a defect.' These skill/sub-agent files control agent behavior. Since issue #179 describes a review coverage gap (defect), fix(#179): is correct. COMMITS.md line 73 says to flag prefix violations as required changes. Suggested fix: Change PR title and commit message prefix from docs(#179): to fix(#179): (preferred) or refactor(#179): |
||
| `post-review.sh`, and similar scripts under `scripts/`): | ||
| - **Echo flag injection:** `echo "${VAR}"` where the variable may | ||
| contain user- or agent-controlled content (PR body, commit message, | ||
| agent output). A value starting with `-e` or `-n` silently changes | ||
| behavior — recommend `printf '%s\n' "${VAR}"` instead. Do not flag | ||
| `echo` with hardcoded strings or in non-harness scripts. | ||
| - **Unquoted heredoc expansion:** `<<TOKEN` (without quotes) in | ||
|
There was a problem hiding this comment. [medium] internal-inconsistency The 'do not flag' clauses diverge between SKILL.md (unconditionally excludes non-harness scripts) and correctness.md (conditionally excludes non-harness scripts 'where the variable is demonstrably not user-controlled'). Same divergence for heredoc clause. Because code-review can be invoked standalone, the two entry paths produce different review behavior for the same code. Suggested fix: Align scoping rules: either unconditionally exclude non-harness scripts in both files, or add the 'demonstrably not user-controlled' qualifier to SKILL.md. |
||
| heredocs generating JSON or YAML with embedded variable | ||
| interpolation. Shell expansion of `$` and backticks in | ||
| user-controlled content silently corrupts structured output — | ||
| recommend `jq -n` for JSON or quoted heredocs (`<<'TOKEN'`). Do not | ||
|
There was a problem hiding this comment. [medium] internal-inconsistency SKILL.md heredoc false-positive scoping omits the <<"TOKEN" (double-quoted delimiter) form that correctness.md correctly includes. In POSIX shells, any quoting of the heredoc delimiter suppresses expansion, so <<"TOKEN" is safe and should not be flagged. A reviewer following the standalone code-review skill could produce a false positive on <<"TOKEN" heredocs. Suggested fix: Add 'or <<"TOKEN"' to the false-positive scoping in SKILL.md to match correctness.md: 'Do not flag quoted heredocs (<<'TOKEN' or <<"TOKEN") or non-harness scripts.' |
||
| flag quoted heredocs (`<<'TOKEN'` or `<<"TOKEN"`) or non-harness | ||
| scripts. | ||
|
|
||
| #### Security | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -73,3 +73,42 @@ When reviewing technical documentation, verify: | |
| - **Edge case correctness** — Are described edge cases (depth/breadth | ||
| limits, zero values, error conditions) handled correctly in the | ||
| described logic? | ||
|
|
||
|
There was a problem hiding this comment. [medium] tier-mismatch PR title uses 'docs(#179):' prefix but COMMITS.md explicitly classifies 'tightening internal heuristics, adjusting prompts, or tuning agent behavior' as 'refactor' or 'fix'. This PR adds new review heuristics that change agent behavior (what the review agent flags), which is not pure documentation. Since issue #179 identifies a defect, 'fix(#179):' is most appropriate. Suggested fix: Change the PR title and commit message prefix from 'docs(#179):' to 'fix(#179):' or 'refactor(#179):'. |
||
| ### Shell safety in harness scripts | ||
|
|
||
|
There was a problem hiding this comment. [medium] dimension-misplacement The correctness sub-agent declares 'Do not own: ... injection defense' (line 17) but the new heredoc expansion heuristic describes backtick command substitution — textbook injection owned by the security sub-agent. Echo flag injection is arguably correctness (data corruption), but heredoc command substitution conflicts with the stated ownership boundary, creating a gap where neither sub-agent reliably covers this pattern. Suggested fix: Either split heuristics (echo under correctness, heredoc under security), or update correctness.md's 'Do not own' clause to carve out shell data-corruption patterns and cross-reference in security.md. |
||
| Harness scripts (`post-code.sh`, `post-fix.sh`, `post-review.sh`, and | ||
| similar scripts under `scripts/`) handle user- and agent-controlled | ||
| content. Two bug classes silently corrupt data and are easy to miss: | ||
|
|
||
| **Echo flag injection:** `echo "${VAR}"` interprets a leading dash as | ||
| a flag. If the variable contains user- or agent-controlled content | ||
| (e.g., a PR body, commit message, or agent output), a value starting | ||
| with `-e` or `-n` silently changes `echo`'s behavior — escape | ||
| sequences are interpreted or trailing newlines are suppressed, | ||
| corrupting the output with no error. | ||
|
|
||
| Flag `echo` with variable interpolation where the variable may contain | ||
| user- or agent-controlled content in harness scripts as a | ||
| medium-severity finding. Recommend `printf '%s\n' "${VAR}"` instead, | ||
| which does not interpret its argument as flags. | ||
|
|
||
| Do not flag `echo` with hardcoded strings (e.g., `echo "Step complete"`) | ||
| or `echo` in non-harness scripts where the variable is demonstrably | ||
| not user-controlled. | ||
|
|
||
| **Unquoted heredoc expansion:** `<<TOKEN` (without quotes around TOKEN) | ||
| allows shell expansion of `$` and backticks inside the heredoc body. | ||
| When generating structured data (JSON, YAML) with embedded variables | ||
| that may contain user- or agent-controlled content, shell expansion | ||
| can silently corrupt the output — `$` in a PR body becomes a variable | ||
| reference, and backtick-delimited text becomes command substitution. | ||
|
|
||
| Flag unquoted heredocs (`<<TOKEN`) that generate JSON or YAML with | ||
| embedded variable interpolation in harness scripts as a | ||
| medium-severity finding. Recommend either `jq -n` for JSON | ||
| construction or a quoted heredoc (`<<'TOKEN'`) combined with explicit | ||
| variable substitution where needed. | ||
|
|
||
| Do not flag quoted heredocs (`<<'TOKEN'` or `<<"TOKEN"`) or heredocs | ||
| in non-harness scripts where all interpolated variables are | ||
| demonstrably not user-controlled. | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[low] documentation-structure
The new shell safety bullet points use a nested parent-child structure (a parent bullet with sub-bullets for each heuristic). All other bullets in the Correctness section are top-level and self-contained. The Security section similarly uses top-level bullets with bold inline headers for extended items rather than parent-child nesting.
Suggested fix: Consider flattening the structure — make Echo flag injection and Unquoted heredoc expansion top-level bullets, each starting with a bold name followed by the explanation, matching the pattern used elsewhere in both sections.