t1428.5: Unified post-session security summary#4036
Conversation
Add --security mode to session-review-helper.sh that aggregates: - Cost breakdown by model from observability data (SQLite DB + JSONL) - Audit event type breakdown with chain integrity check - Network access summary (logged/flagged/denied domains) - Prompt injection defense stats (blocked/warned/sanitized) - Session security context placeholder (t1428.3 pending) - Quarantine queue placeholder (t1428.4 pending) Computes overall security posture (CLEAN/LOW/MEDIUM/HIGH/CRITICAL) based on denied access, blocked injections, and audit chain integrity. Supports: security command, --security flag with gather, --json output, --session filter for session-scoped data. Closes #4029
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the Highlights
Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
|
Caution Review failedPull request was closed or merged during review Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds a security-focused mode to session review: new Changes
Sequence DiagramsequenceDiagram
participant User
participant SessionReview as session-review-helper.sh
participant Observability as observability-helper.sh
participant AuditLog as audit-log-helper.sh
participant NetworkTier as network-tier-helper.sh
participant PromptGuard as prompt-guard-helper.sh
participant SessionCtx as session-security-context
participant Quarantine as quarantine-helper.sh
User->>SessionReview: session-review security --session <id> [--json]
activate SessionReview
SessionReview->>Observability: query cost (session filter)
Observability-->>SessionReview: cost by model
SessionReview->>AuditLog: fetch audit events (session filter)
AuditLog-->>SessionReview: tamper-evident events
SessionReview->>NetworkTier: fetch network/access flags
NetworkTier-->>SessionReview: domain counts / flags
SessionReview->>PromptGuard: fetch prompt-guard detections
PromptGuard-->>SessionReview: BLOCK/WARN/SANITIZE tallies
SessionReview->>SessionCtx: fetch session security context
SessionCtx-->>SessionReview: composite session data
SessionReview->>Quarantine: check pending quarantine items
Quarantine-->>SessionReview: quarantine status
SessionReview->>SessionReview: compute _security_posture
SessionReview-->>User: unified security summary (human or JSON)
deactivate SessionReview
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
🔍 Code Quality Report�[0;35m[MONITOR]�[0m Code Review Monitoring Report �[0;34m[INFO]�[0m Latest Quality Status: �[0;34m[INFO]�[0m Recent monitoring activity: 📈 Current Quality Metrics
Generated on: Tue Mar 10 02:04:04 UTC 2026 Generated by AI DevOps Framework Code Review Monitoring |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a new security command to session-review-helper.sh for a unified security summary. While the feature is useful, it introduces several critical injection vulnerabilities due to improper handling of the user-supplied --session argument, specifically SQL, jq, and JSON injection. Additionally, there is a medium-severity maintainability issue regarding code duplication that could be improved. Addressing these security vulnerabilities and maintainability concerns is crucial for the robustness and security of the new feature.
| --session) | ||
| shift | ||
| session_filter="${1:-}" |
There was a problem hiding this comment.
The session_filter variable, which comes from user input (--session), is not sanitized. It is later used to construct a SQL query (in _security_cost_summary), a jq filter, and a JSON string. This creates vulnerabilities for SQL injection, jq injection, and malformed JSON. A malicious session ID could be used to execute arbitrary SQL commands.
References
- To prevent SQL injection in shell scripts using
sqlite3, create a helper function that uses.param setfor safe parameterized bindings instead of direct string interpolation. - In shell scripts, use
jq --argfor strings and--argjsonfor other JSON types (like numbers) to safely pass variables into ajqfilter. This avoids syntax errors if the variables contain special characters. - In shell scripts, use
jq --argjsonto safely pass JSON content from variables when merging JSON arrays, instead of direct string interpolation, to prevent parsing errors. - To reliably wrap the entire content of a shell variable as a single JSON string, use
jq -Rn --arg v "$VAR" '$v'.
There was a problem hiding this comment.
Fixed. _sanitize_session_filter() strips all characters except [a-zA-Z0-9_.-] at the entry point. SQL path uses belt-and-suspenders escaping, jq path uses --arg, JSON path uses jq -n --arg. Defense-in-depth at both entry and per-function level.
| if [[ -n "$session_filter" ]]; then | ||
| where_clause="WHERE session_id = '${session_filter}'" | ||
| fi | ||
| local result | ||
| result=$(sqlite3 -separator '|' "$OBS_DB" " |
There was a problem hiding this comment.
The session_filter variable, which is derived from the user-supplied --session command-line argument, is directly concatenated into an SQL query string used with sqlite3. This allows an attacker to perform SQL injection by providing a malicious session ID (e.g., ' OR 1=1 --).
References
- To prevent SQL injection in shell scripts using
sqlite3, create a helper function that uses.param setfor safe parameterized bindings instead of direct string interpolation.
There was a problem hiding this comment.
Fixed. Session filter is sanitized via _sanitize_session_filter() (allowlist: [a-zA-Z0-9_.-]), then single quotes are escaped (' → '') before SQL interpolation. See lines 74-79.
| cat <<EOF | ||
| { | ||
| "timestamp": "$(date -u '+%Y-%m-%dT%H:%M:%SZ')", | ||
| "session_filter": $(if [[ -n "$session_filter" ]]; then echo "\"$session_filter\""; else echo "null"; fi), |
There was a problem hiding this comment.
The session_filter variable is directly echoed into a JSON structure within a heredoc. This introduces a JSON injection vulnerability, as special characters in the variable can break the JSON structure or inject additional fields, leading to malformed JSON output. This should be addressed by using jq's --arg flag or a dedicated JSON construction tool for safe embedding.
References
- In shell scripts, use
jq --argfor strings and--argjsonfor other JSON types (like numbers) to safely pass variables into ajqfilter. This avoids syntax errors if the variables contain special characters. - In shell scripts, use
jq --argjsonto safely pass JSON content from variables when merging JSON arrays, instead of direct string interpolation, to prevent parsing errors. - To reliably wrap the entire content of a shell variable as a single JSON string, use
jq -Rn --arg v "$VAR" '$v'.
There was a problem hiding this comment.
Fixed. Heredoc replaced with jq -n --arg construction (line 559). Session filter is passed safely via --argjson session_filter — never interpolated into the JSON template.
| jq_filter="select(.session_id == \"${session_filter}\")" | ||
| fi | ||
| local result | ||
| result=$(jq -sr "[.[] | ${jq_filter}] | group_by(.model) | map({ |
There was a problem hiding this comment.
The session_filter variable is directly concatenated into a jq filter string. This allows an attacker to inject arbitrary jq code by providing a malicious session ID (e.g., ") | .[] | select(true).
References
- In shell scripts, use
jq --argfor strings and--argjsonfor other JSON types (like numbers) to safely pass variables into ajqfilter. This avoids syntax errors if the variables contain special characters.
There was a problem hiding this comment.
Fixed. Replaced direct string interpolation with jq --arg sid "$safe_session" and $sid reference in the jq expression (line 130). The value is never embedded in the jq filter string.
![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 (2)
.agents/scripts/session-review-helper.sh (2)
73-74: Consider extracting repeated format strings (optional).SonarCloud flagged the repeated
printfformat literals. Extracting to constants likeFMT_COST_ROWwould reduce duplication, but this is a minor maintainability concern - the current approach keeps formatting close to usage.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.agents/scripts/session-review-helper.sh around lines 73 - 74, The two identical printf format strings should be extracted to a single named constant to remove duplication and improve maintainability; define a constant like FMT_COST_ROW=' %-35s %6s %10s %10s %10s %10s\n' near the top of the script and replace both printf calls with printf "$FMT_COST_ROW" "Model" "Reqs" ... and printf "$FMT_COST_ROW" "---" "---" ...; update any nearby comments to document the constant's purpose and ensure variable quoting when using printf.
339-352: Consider merging nested conditionals for clarity.The static analysis tool suggests combining these nested
ifstatements. This is a minor readability improvement.♻️ Proposed simplification
if [[ -f "$PG_ATTEMPTS_LOG" ]]; then local blocks blocks=$(grep -c '"action":"BLOCK"' "$PG_ATTEMPTS_LOG" 2>/dev/null || echo "0") - if [[ "$blocks" -gt 0 ]]; then - if [[ "$posture" == "CLEAN" || "$posture" == "LOW" ]]; then - posture="MEDIUM" - fi + if [[ "$blocks" -gt 0 && ("$posture" == "CLEAN" || "$posture" == "LOW") ]]; then + posture="MEDIUM" fi local warns warns=$(grep -c '"action":"WARN"' "$PG_ATTEMPTS_LOG" 2>/dev/null || echo "0")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.agents/scripts/session-review-helper.sh around lines 339 - 352, The nested conditionals around reading PG_ATTEMPTS_LOG can be simplified: keep computing blocks=$(grep -c '"action":"BLOCK"' "$PG_ATTEMPTS_LOG" ...) and warns similarly, then combine the checks so you only enter an outer if [[ -f "$PG_ATTEMPTS_LOG" ]]; then once and inside use single-line conditionals that test both the count and the current posture variable; e.g., check if blocks -gt 0 and (posture == "CLEAN" || posture == "LOW") to set posture="MEDIUM", and check if warns -gt 0 and posture == "CLEAN" to set posture="LOW", ensuring you still compute both blocks and warns before those combined tests (references: PG_ATTEMPTS_LOG, blocks, warns, posture).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.agents/scripts/session-review-helper.sh:
- Around line 52-55: The WHERE clause currently interpolates session_filter
directly into where_clause, risking SQL injection; change the code to avoid
direct string interpolation by using a parameterized query or by safely escaping
the value: either build where_clause like "WHERE session_id = ?" and pass
session_filter as a bound parameter to the SQL execution command, or if binding
isn’t available, sanitize session_filter by escaping single quotes (replace '
with '') before embedding. Update references to where_clause and the SQL
execution invocation so the session_filter is passed as a parameter or the
escaped value is used.
- Around line 92-95: The jq filter currently interpolates the shell variable
session_filter directly into jq_filter, which allows injection; update the code
to pass session_filter to jq safely (e.g., use jq --arg or equivalent) instead
of embedding it in the expression so the value is handled as a string, and
change the variable/join logic around jq_filter and the jq invocation that uses
it (referencing session_filter and jq_filter) to use the --arg approach or
proper escaping of quotes/backslashes.
---
Nitpick comments:
In @.agents/scripts/session-review-helper.sh:
- Around line 73-74: The two identical printf format strings should be extracted
to a single named constant to remove duplication and improve maintainability;
define a constant like FMT_COST_ROW=' %-35s %6s %10s %10s %10s %10s\n' near the
top of the script and replace both printf calls with printf "$FMT_COST_ROW"
"Model" "Reqs" ... and printf "$FMT_COST_ROW" "---" "---" ...; update any nearby
comments to document the constant's purpose and ensure variable quoting when
using printf.
- Around line 339-352: The nested conditionals around reading PG_ATTEMPTS_LOG
can be simplified: keep computing blocks=$(grep -c '"action":"BLOCK"'
"$PG_ATTEMPTS_LOG" ...) and warns similarly, then combine the checks so you only
enter an outer if [[ -f "$PG_ATTEMPTS_LOG" ]]; then once and inside use
single-line conditionals that test both the count and the current posture
variable; e.g., check if blocks -gt 0 and (posture == "CLEAN" || posture ==
"LOW") to set posture="MEDIUM", and check if warns -gt 0 and posture == "CLEAN"
to set posture="LOW", ensuring you still compute both blocks and warns before
those combined tests (references: PG_ATTEMPTS_LOG, blocks, warns, posture).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: d79b6784-8890-4ada-b469-efebfe3210c3
📒 Files selected for processing (3)
.agents/scripts/commands/session-review.md.agents/scripts/session-review-helper.sh.agents/workflows/session-review.md
Address CHANGES_REQUESTED review feedback from CodeRabbit and Gemini Code Assist on PR #4036: - Add _sanitize_session_filter() to strip non-alphanumeric chars from --session input (defense-in-depth at entry point and per-function) - SQL injection: sanitize session_filter before WHERE clause interpolation - jq injection: use jq --arg for safe variable binding instead of string interpolation in JSONL fallback path - JSON injection: replace heredoc with jq -n construction for safe JSON output in _security_summary_json() - Merge nested conditionals in _security_posture() for clarity - Extract repeated printf format string to FMT_COST_ROW constant
|
Addressed all review feedback: Security fixes (critical/high):
Nitpick fixes:
Verification:
@coderabbitai re-review |
🔍 Code Quality Report�[0;35m[MONITOR]�[0m Code Review Monitoring Report �[0;34m[INFO]�[0m Latest Quality Status: �[0;34m[INFO]�[0m Recent monitoring activity: 📈 Current Quality Metrics
Generated on: Tue Mar 10 02:30:30 UTC 2026 Generated by AI DevOps Framework Code Review Monitoring |
|
Triggering a full re-review now. 🧠 Learnings used✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Code Review
This pull request introduces a unified post-session security summary feature. While the implementation is well-structured, a high-severity command injection vulnerability was identified in the _security_cost_summary function due to insecure parsing of untrusted data, which could lead to arbitrary code execution. Additionally, performance improvements are needed for data aggregation in the script by reducing redundant computations, consolidating jq calls, and optimizing loop-based calculations.
| grand_total_cost=$(awk "BEGIN { printf \"%.6f\", $grand_total_cost + $cost }") | ||
| grand_total_reqs=$((grand_total_reqs + reqs)) |
There was a problem hiding this comment.
The _security_cost_summary function is vulnerable to high-severity command injection. Insecure parsing of SQLite output with pipe-separated data allows malicious model_id content to lead to arbitrary awk code execution or command execution within bash arithmetic expansions. This also highlights a performance issue: calling awk inside a while loop is inefficient as it spawns a new process for each row. It's critical to address the command injection by using safer parsing methods like awk -v and to improve performance by calculating totals outside the loop.
| grand_total_cost=$(awk "BEGIN { printf \"%.6f\", $grand_total_cost + $cost }") | |
| grand_total_reqs=$((grand_total_reqs + reqs)) | |
| grand_total_cost=$(awk -v total="$grand_total_cost" -v cost="$cost" 'BEGIN { printf "%.6f", total + cost }') | |
| grand_total_reqs=$(awk -v total="$grand_total_reqs" -v reqs="$reqs" 'BEGIN { print total + reqs }') |
References
- Do not use 'eval' to execute dynamically constructed commands in shell scripts due to security risks; use bash arrays and 'read -ra' to store and execute command parts safely, passing complex arguments like JSON payloads as separate elements.
- In shell scripts, move the calculation of loop-invariant variables outside of loops to improve efficiency.
There was a problem hiding this comment.
Fixed. The awk-in-loop pattern has been replaced with post-loop totalling: costs and request counts are collected during the loop, then a single awk call computes each total after the loop exits. Additionally, numeric field validation ([[ "$reqs" =~ ^[0-9]+$ ]]) has been added to reject malformed DB data before it reaches awk. The awk -v pattern was already safe against injection, but the field validation adds defense-in-depth against pipe-delimiter shifting from malicious model_id values.
| echo "$result" | jq -r '.[] | "\(.model)|\(.requests)|\(.input_tokens)|\(.output_tokens)|\(.cache_tokens)|\(.cost)"' 2>/dev/null | while IFS='|' read -r model reqs input_tok output_tok cache_tok cost; do | ||
| [[ -z "$model" ]] && continue | ||
| local short_model="${model##*/}" | ||
| short_model="${short_model:0:35}" | ||
| printf " %-35s %6s %10s %10s %10s \$%s\n" \ | ||
| "$short_model" "$reqs" "$input_tok" "$output_tok" "$cache_tok" "$cost" | ||
| done |
There was a problem hiding this comment.
This while loop re-parses the output of jq. You can make this more efficient and simpler by having jq format the output directly, which avoids the intermediate shell parsing step. For example:
echo "$result" | jq -r '.[] |
" \(.model | split("/")[-1] | .[:35] | ljust(35)) \
\(.requests | tostring | rjust(6)) \
\(.input_tokens | tostring | rjust(10)) \
\(.output_tokens | tostring | rjust(10)) \
\(.cache_tokens | tostring | rjust(10)) \
$\(.cost)"'References
- Consolidate multiple 'jq' calls into a single pass where possible to improve performance and script efficiency.
There was a problem hiding this comment.
Fixed. The JSONL fallback path now uses a single jq pass that formats all rows and computes totals in one expression, replacing the previous pattern of shell while-loop parsing + 2 additional jq calls for totals.
| local session_filter="${1:-}" | ||
|
|
||
| local posture | ||
| posture=$(_security_posture) |
There was a problem hiding this comment.
The _security_posture function is called here, but it re-calculates several counts (e.g., denied_count, blocks) that are also calculated a few lines below in this function (_security_summary_json). This results in redundant file processing. To improve efficiency, consider refactoring _security_posture to accept these counts as arguments. This would allow you to calculate them once in this function and pass them to _security_posture.
There was a problem hiding this comment.
Fixed. _security_posture() now accepts optional pre-computed counts as arguments ($1=denied, $2=blocks, $3=warns, $4=flagged, $5=chain_intact). When called with arguments, it skips file reads entirely. When called without arguments (e.g., from output_security_summary text mode), it falls back to computing from logs. _security_summary_json() now computes counts first and passes them to _security_posture(), eliminating redundant file processing.
…ation Address remaining CodeRabbit and Gemini review findings on PR #4036: - Replace awk shell interpolation with -v flag to prevent command injection via malicious model_id content in SQLite output - Replace bash arithmetic expansion with awk -v for reqs accumulation - Add _sanitize_session_filter() at every function boundary (_security_cost_summary, output_security_summary, _security_summary_json) for defense-in-depth, not just at CLI parse time
🔍 Code Quality Report�[0;35m[MONITOR]�[0m Code Review Monitoring Report �[0;34m[INFO]�[0m Latest Quality Status: �[0;34m[INFO]�[0m Recent monitoring activity: 📈 Current Quality Metrics
Generated on: Tue Mar 10 02:46:22 UTC 2026 Generated by AI DevOps Framework Code Review Monitoring |
|
Pulse dispatched a worker to address the CodeRabbit/Gemini CHANGES_REQUESTED feedback: SQL injection fix in session_filter WHERE clause, jq injection fix via --arg, printf format constant extraction, and nested conditional simplification. |
|
Warning You have reached your daily quota limit. Please wait up to 24 hours and I will start processing your requests again! |
- Add SQL-level single-quote escaping as belt-and-suspenders defense alongside _sanitize_session_filter (prevents SQL injection) - Add clarifying comment on jq --arg usage (already parameterized) - Extract repeated printf data-row format to FMT_COST_DATA constant - Simplify _security_posture nested conditionals into single if/elif
🔍 Code Quality Report�[0;35m[MONITOR]�[0m Code Review Monitoring Report �[0;34m[INFO]�[0m Latest Quality Status: �[0;34m[INFO]�[0m Recent monitoring activity: 📈 Current Quality Metrics
Generated on: Tue Mar 10 03:04:55 UTC 2026 Generated by AI DevOps Framework Code Review Monitoring |
🔍 Code Quality Report�[0;35m[MONITOR]�[0m Code Review Monitoring Report �[0;34m[INFO]�[0m Latest Quality Status: �[0;34m[INFO]�[0m Recent monitoring activity: 📈 Current Quality Metrics
Generated on: Tue Mar 10 03:14:34 UTC 2026 Generated by AI DevOps Framework Code Review Monitoring |
|
Dispatching worker to address CodeRabbit and Gemini review feedback (command injection vulnerability flagged).
|
…nd performance - Replace awk-in-loop with post-loop totalling (single awk call per total) - Add numeric field validation for SQLite pipe-parsed data (defense-in-depth) - Consolidate JSONL fallback to single jq pass (was 3 separate jq calls) - Refactor _security_posture() to accept pre-computed counts as args, eliminating redundant file reads when called from _security_summary_json() - Backward compatible: _security_posture() still computes from logs when called without arguments (text mode path)
|
Addressed all remaining Gemini and CodeRabbit review feedback (round 2): Security hardening (high-severity):
Performance improvements (medium):
Verification:
@gemini-code-assist re-review |
|
Warning You have reached your daily quota limit. Please wait up to 24 hours and I will start processing your requests again! |
🔍 Code Quality Report�[0;35m[MONITOR]�[0m Code Review Monitoring Report �[0;34m[INFO]�[0m Latest Quality Status: �[0;34m[INFO]�[0m Recent monitoring activity: 📈 Current Quality Metrics
Generated on: Tue Mar 10 03:51:05 UTC 2026 Generated by AI DevOps Framework Code Review Monitoring |
|
Triggering a full re-review now. 🧠 Learnings used✅ Actions performedFull review triggered. |
|
@coderabbitai review All 3 actionable items from the latest CHANGES_REQUESTED review (run
ShellCheck: clean (SC1091 only). Please re-review to clear CHANGES_REQUESTED state. |
|
Triggering a full re-review now to clear the CHANGES_REQUESTED state. 🧠 Learnings used✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
.agents/scripts/session-review-helper.sh (1)
493-529:⚠️ Potential issue | 🟠 MajorFiltered runs still report a global posture as if it were session-scoped.
When
--sessionis set, the headline posture is still derived from global audit/network/prompt-guard logs, but the header shows a specific session ID. The later(global …)notes in the section bodies do not fix that top-level ambiguity.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.agents/scripts/session-review-helper.sh:
- Around line 601-640: The JSON builder currently only emits availability flags
(session_context_available, quarantine_available) and omits any cost breakdown;
update the jq payload to include full objects for session_context and quarantine
(e.g., { available: $session_context_available, score: $session_context_score,
pending_items: $session_context_pending }) and ({ available:
$quarantine_available, score: $quarantine_score, pending_items:
$quarantine_pending }) and add cost fields (e.g., cost_total and cost_breakdown)
by wiring existing variables into jq via --argjson cost_total "$cost_total" and
--argjson cost_breakdown "$cost_breakdown" (or their equivalent names used
elsewhere), then reference those args in the final JSON so the machine-readable
output matches the text summary.
- Around line 1018-1025: The --session case currently accepts and silently
rewrites invalid IDs via _sanitize_session_filter which can produce an empty or
altered session_filter; instead, validate input after calling
_sanitize_session_filter in the --session branch: call _sanitize_session_filter
on "$1", then if the sanitized value is empty or does not exactly equal the
original raw "$1" (or fails the expected ID pattern), print an error to stderr
and exit 1 rather than assigning session_filter and continuing; reference the
--session case, the _sanitize_session_filter helper, and the session_filter
variable when making this change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: b757101d-e849-4d79-b815-65b6f6f6230e
📒 Files selected for processing (3)
.agents/scripts/commands/session-review.md.agents/scripts/session-review-helper.sh.agents/workflows/session-review.md
…, JSON cost data, SQL column name - --session now rejects altered/empty input instead of silently falling back to global scope (fail-closed validation per CodeRabbit review #3920297733) - JSON output now includes cost.total and cost.breakdown with per-model data, matching the text summary's cost table - session_context now includes score field, quarantine includes pending_items count (not just availability flags) - Fix SQL column name: cost_total -> cost (matches actual DB schema)
|
Addressed all remaining CodeRabbit review findings in commit f1b1963: Fixes applied (from review #3920297733):
Dismissed (by design):
Verification:
|
🔍 Code Quality Report�[0;35m[MONITOR]�[0m Code Review Monitoring Report �[0;34m[INFO]�[0m Latest Quality Status: �[0;34m[INFO]�[0m Recent monitoring activity: 📈 Current Quality Metrics
Generated on: Tue Mar 10 07:28:48 UTC 2026 Generated by AI DevOps Framework Code Review Monitoring |
|
@coderabbitai review All actionable items from the latest CHANGES_REQUESTED review (run b757101d) have been addressed in commit f1b1963:
The global-posture-as-session-scoped finding is by design (underlying logs lack session_id field, noted inline with '(global — ...)' markers). |
|
The global-posture-as-session-scoped design call is well-reasoned. When the underlying data sources structurally lack a Triggering a full re-review now. ✅ Actions performedFull review triggered. |
|
|
@coderabbitai review All 5 inline review comments from the latest CHANGES_REQUESTED review have been addressed and replied to:
Please re-review to clear the CHANGES_REQUESTED state. |
|
Combined with all prior rounds (SQL/jq/JSON/awk injection hardening, fail-closed session validation, full JSON cost breakdown, SQL column fix, format constants, posture pre-computation), this is a comprehensive and well-documented hardening pass. Triggering a full re-review now. 🧠 Learnings used✅ Actions performedFull review triggered. |
marcusquinn
left a comment
marcusquinn
left a comment
There was a problem hiding this comment.
@coderabbitai All 9 actionable review comments have been addressed across commits 9b92562, 849bd9d, 09a5ff8, 2248a4e, 84d964a, and f1b1963. All nitpick suggestions (format string constants, nested conditional simplification, jq stderr suppression, pre-compute counts for text mode) were also addressed. Please re-review to clear the stale CHANGES_REQUESTED state.
marcusquinn
dismissed stale reviews from coderabbitai[bot], coderabbitai[bot], coderabbitai[bot], and coderabbitai[bot]
All actionable comments addressed in subsequent commits. Nitpick suggestions (format string constants, nested conditionals) also addressed.
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
.agents/scripts/session-review-helper.sh (1)
315-317: Consider extracting action grep patterns to constants.Static analysis flags the repeated literals
'"action":"BLOCK"','"action":"WARN"', and'"action":"SANITIZE"'used 4 times each across_security_prompt_guard(),_security_posture(),output_security_summary(), and_security_summary_json().Extracting these to constants at the top of the file (alongside
FMT_*) would improve maintainability if the log format ever changes.♻️ Suggested constants
readonly FMT_SUMMARY_ROW=" %-20s %6s\n" + +# Grep patterns for prompt-guard action types (used in multiple functions) +readonly PG_ACTION_BLOCK='"action":"BLOCK"' +readonly PG_ACTION_WARN='"action":"WARN"' +readonly PG_ACTION_SANITIZE='"action":"SANITIZE"'Then replace usages:
-blocks=$(grep -c '"action":"BLOCK"' "$PG_ATTEMPTS_LOG" 2>/dev/null || echo "0") +blocks=$(grep -c "$PG_ACTION_BLOCK" "$PG_ATTEMPTS_LOG" 2>/dev/null || echo "0")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.agents/scripts/session-review-helper.sh around lines 315 - 317, Extract the repeated literal grep patterns into shell constants (e.g. ACTION_BLOCK, ACTION_WARN, ACTION_SANITIZE) defined near the existing FMT_* constants at the top of the file, and replace all occurrences of '"action":"BLOCK"', '"action":"WARN"', and '"action":"SANITIZE"' in _security_prompt_guard(), _security_posture(), output_security_summary(), and _security_summary_json() with those constants (use the constants when building the grep arguments against $PG_ATTEMPTS_LOG) so the log-format tokens are centralized and easy to update.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In @.agents/scripts/session-review-helper.sh:
- Around line 315-317: Extract the repeated literal grep patterns into shell
constants (e.g. ACTION_BLOCK, ACTION_WARN, ACTION_SANITIZE) defined near the
existing FMT_* constants at the top of the file, and replace all occurrences of
'"action":"BLOCK"', '"action":"WARN"', and '"action":"SANITIZE"' in
_security_prompt_guard(), _security_posture(), output_security_summary(), and
_security_summary_json() with those constants (use the constants when building
the grep arguments against $PG_ATTEMPTS_LOG) so the log-format tokens are
centralized and easy to update.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: d477a6e7-61ac-4274-a8b4-ea853d68e0a8
📒 Files selected for processing (3)
.agents/scripts/commands/session-review.md.agents/scripts/session-review-helper.sh.agents/workflows/session-review.md
1 participant
Summary
securitycommand and--securityflag tosession-review-helper.shthat aggregates all security subsystem data into a single CLI summary--json), session filtering (--session <id>), and integration with the existinggathercommand (gather --security)Data Sources Aggregated
observability-helper.sh(SQLite DB + JSONL)audit-log-helper.shnetwork-tier-helper.shprompt-guard-helper.shsession-security-helper.shquarantine-helper.shFiles Changed
.agents/scripts/session-review-helper.sh— newsecuritycommand,--security/--session/--jsonflags, security aggregation functions.agents/workflows/session-review.md— documentation for the security summary section.agents/scripts/commands/session-review.md— updated command descriptionTesting
session-review-helper.sh security— produces formatted summary tablesession-review-helper.sh security --json— produces valid JSONsession-review-helper.sh gather --security— appends security to standard reviewsession-review-helper.sh help— shows updated help textCloses #4029
Summary by CodeRabbit
New Features
Documentation