skills/CODE-REVIEW-niri-window-capture.md

Code Review: niri-window-capture Skill

Review Date: 2025-11-09
Reviewer: OpenCode Agent
Scope: Complete skill review including SKILL.md, SECURITY.md, and all scripts

Overall Assessment

Status: ✅ APPROVED with minor recommendations

Summary: High-quality, security-conscious skill with comprehensive documentation. Scripts follow bash best practices. Security considerations are well-documented and appropriate audit logging is implemented.

Strengths:

• Excellent security documentation
• Comprehensive audit logging
• Proper error handling
• Good use of set -euo pipefail
• Safe jq usage with --arg for variable passing
• Clear separation of concerns across scripts

Areas for Improvement (minor):

• Some opportunities for additional input validation
• Could benefit from more defensive programming around filesystem assumptions
• Consider adding timeout handling for niri commands

---

Documentation Review

SKILL.md (185 lines)

✅ Strengths:

• Clear security warning at the top
• Comprehensive "When to Use" section with concrete examples
• Well-documented helper scripts with usage examples
• Direct niri commands provided for advanced use
• Common workflows documented
• Clear guidelines section
• Requirements clearly stated

Recommendations:

1. Consider adding version compatibility note (currently says "niri 25.08")
2. Could add troubleshooting section for common issues
3. Examples directory referenced but should verify contents match docs

Score: 9/10

---

SECURITY.md (196 lines)

✅ Strengths:

• Excellent threat model section
• Clear explanation of privacy implications
• Concrete protection mechanisms with code examples
• Audit trail documentation
• Window blocking configuration examples
• Attack scenarios documented
• Recommendations prioritized and actionable

✅ Outstanding:

• The security awareness demonstrated here is exemplary
• Cross-workspace invisibility clearly explained
• Clipboard pollution documented (important edge case)
• Upstream feature request for --no-clipboard shows proactive thinking

Recommendations:

1. Consider adding emergency response section ("If you suspect unauthorized captures...")
2. Could document log retention policy

Score: 10/10

---

Script Reviews

1. capture-focused.sh (32 lines)

Purpose: Capture currently focused window

✅ Bash Best Practices:

• ✅ Proper shebang: #!/usr/bin/env bash
• ✅ Error handling: set -euo pipefail
• ✅ Variables: Uppercase for constants (LOG_TAG, WINDOW_ID)
• ✅ Error messages to stderr: >&2
• ✅ Exit codes: exit 1 on error
• ✅ JSON parsing: Safe with jq -r
• ✅ Audit logging: All captures logged

✅ Security:

• ✅ Logs before and after capture
• ✅ Includes window metadata in logs
• ✅ No shell injection vulnerabilities
• ✅ jq properly handles untrusted input

✅ Logic:

• ✅ Checks if window exists before capture
• ✅ Handles null responses from jq
• ✅ Uses sleep 0.1 for filesystem sync (good practice)
• ✅ Returns screenshot path to stdout

⚠️ Minor Issues:

1. Hardcoded screenshot directory:

   SCREENSHOT_PATH=$(ls -t ~/Pictures/Screenshots/*.png | head -1)
   

   • Could fail if directory doesn't exist or no PNGs present
   • Recommend: Check directory exists first or query niri config

2. Potential race condition:

   • What if another process creates a screenshot in that 0.1s window?
   • Unlikely but possible
   • Recommendation: Capture timestamp before screenshot, filter by time

3. Silent failure handling:

   niri msg action screenshot-window ... >/dev/null 2>&1
   

   • Error output suppressed completely
   • Recommendation: Check exit code or at least log errors

Suggested Improvements:

# Check screenshot directory exists
SCREENSHOT_DIR="${XDG_PICTURES_DIR:-$HOME/Pictures}/Screenshots"
if [[ ! -d "$SCREENSHOT_DIR" ]]; then
    logger -t "$LOG_TAG" "ERROR: Screenshot directory does not exist: $SCREENSHOT_DIR"
    echo "Error: Screenshot directory not found" >&2
    exit 1
fi

# Capture timestamp before screenshot for safer lookup
BEFORE_TIME=$(date +%s)

# Capture with error checking
if ! niri msg action screenshot-window --id "$WINDOW_ID" --write-to-disk true 2>&1 | logger -t "$LOG_TAG"; then
    logger -t "$LOG_TAG" "ERROR: Screenshot command failed for window $WINDOW_ID"
    echo "Error: Screenshot capture failed" >&2
    exit 1
fi

# More robust screenshot finding
sleep 0.15  # Slightly longer for filesystem
SCREENSHOT_PATH=$(find "$SCREENSHOT_DIR" -name "*.png" -newer <(date -d "@$BEFORE_TIME" +%Y-%m-%d) | sort -r | head -1)

if [[ -z "$SCREENSHOT_PATH" ]]; then
    logger -t "$LOG_TAG" "ERROR: Screenshot file not found after capture"
    echo "Error: Screenshot file not created" >&2
    exit 1
fi

Score: 8.5/10

---

2. capture-by-title.sh (41 lines)

Purpose: Find and capture window by partial title match

✅ Bash Best Practices:

• ✅ Proper shebang and error handling
• ✅ Input validation: [[ $# -lt 1 ]]
• ✅ Safe jq usage: jq --arg search "$SEARCH"
• ✅ Case-insensitive matching
• ✅ Comprehensive logging

✅ Security:

• ✅ No shell injection via search term (jq handles it safely)
• ✅ Logs matched term for audit trail
• ✅ All captures logged with context

✅ Logic:

• ✅ Takes first match (.[0])
• ✅ Clear error if no match found
• ✅ Extracts full metadata before capture

⚠️ Minor Issues:

1. Empty check insufficient:

   if [[ -z "$WINDOW_META" ]]; then
   

   • Should also check for "null" result from jq
   • Recommendation: if [[ -z "$WINDOW_META" || "$WINDOW_META" == "null" ]]; then

2. Ambiguous matches:

   • Takes first match silently
   • If multiple windows match, user might not know which one
   • Recommendation: Log how many matches found, or make it explicit in error message

3. Same filesystem issues as capture-focused.sh:

   • Hardcoded path
   • No verification screenshot was created

Suggested Improvements:

# Find ALL matches first
ALL_MATCHES=$(niri msg --json windows | jq --arg search "$SEARCH" \
    '[.[] | select(.title | ascii_downcase | contains($search | ascii_downcase))]')

MATCH_COUNT=$(echo "$ALL_MATCHES" | jq 'length')

if [[ "$MATCH_COUNT" -eq 0 ]]; then
    logger -t "$LOG_TAG" "ERROR: No window found matching title '$SEARCH'"
    echo "Error: No window found with title matching '$SEARCH'" >&2
    exit 1
fi

if [[ "$MATCH_COUNT" -gt 1 ]]; then
    logger -t "$LOG_TAG" "WARNING: Multiple windows match '$SEARCH' ($MATCH_COUNT found), using first"
    echo "Warning: Found $MATCH_COUNT matches, using first" >&2
fi

WINDOW_META=$(echo "$ALL_MATCHES" | jq '.[0]')

Score: 8/10

---

3. capture-all-windows.sh (37 lines)

Purpose: Capture all windows to a directory with JSON output

✅ Bash Best Practices:

• ✅ Proper error handling
• ✅ Configurable output directory with default
• ✅ Creates directory if needed
• ✅ Sanitizes title for filename

✅ Security:

• ✅ Title sanitization prevents path traversal: tr '/' '-'
• ✅ Limits filename length: cut -c1-50
• ✅ No shell injection vulnerabilities

✅ Logic:

• ✅ Moves screenshots to organized directory
• ✅ Outputs structured JSON at the end
• ✅ Uses jq streaming for clean JSON array output

⚠️ Minor Issues:

1. No audit logging:

   • Unlike other scripts, this one doesn't use logger
   • High-risk since it captures EVERYTHING
   • Recommendation: Add logging for each capture

2. Filename collision handling:

   OUTPUT_PATH="$OUTPUT_DIR/window-${id}-${SAFE_TITLE}.png"
   

   • What if two windows have same title?
   • IDs should make it unique but worth adding timestamp
   • Recommendation: Add timestamp or counter

3. Error handling in loop:

   • If one capture fails, loop continues silently
   • mv could fail if file doesn't exist
   • Recommendation: Add error checking in loop

4. Metadata lookup inefficiency:

   METADATA=$(niri msg --json windows | jq --arg id "$id" '.[] | select(.id == ($id | tonumber))')
   

   • Queries all windows for each ID
   • Could query once at start and reference
   • Minor performance issue

Suggested Improvements:

#!/usr/bin/env bash
# Capture all windows and output JSON mapping window metadata to screenshot paths

set -euo pipefail

LOG_TAG="niri-capture"
OUTPUT_DIR="${1:-/tmp/niri-window-captures}"
mkdir -p "$OUTPUT_DIR"

echo "Capturing all windows to $OUTPUT_DIR..." >&2
logger -t "$LOG_TAG" "Starting capture of all windows to $OUTPUT_DIR"

# Get all windows metadata once
ALL_WINDOWS=$(niri msg --json windows)
WINDOW_COUNT=$(echo "$ALL_WINDOWS" | jq 'length')

logger -t "$LOG_TAG" "Found $WINDOW_COUNT windows to capture"

# Capture each window
echo "$ALL_WINDOWS" | jq -c '.[]' | while IFS= read -r metadata; do
    ID=$(echo "$metadata" | jq -r '.id')
    TITLE=$(echo "$metadata" | jq -r '.title')
    APP_ID=$(echo "$metadata" | jq -r '.app_id')
    WORKSPACE=$(echo "$metadata" | jq -r '.workspace_id')
    
    # Sanitize title for filename
    SAFE_TITLE=$(echo "$TITLE" | tr '/' '-' | tr ' ' '_' | cut -c1-50)
    TIMESTAMP=$(date +%s)
    OUTPUT_PATH="$OUTPUT_DIR/window-${ID}-${TIMESTAMP}-${SAFE_TITLE}.png"
    
    logger -t "$LOG_TAG" "Capturing window $ID: '$TITLE' (workspace: $WORKSPACE)"
    
    # Capture window
    if niri msg action screenshot-window --id "$ID" --write-to-disk true >/dev/null 2>&1; then
        sleep 0.1
        
        # Move from Screenshots to our output dir
        LATEST=$(ls -t ~/Pictures/Screenshots/*.png 2>/dev/null | head -1)
        if [[ -n "$LATEST" ]] && mv "$LATEST" "$OUTPUT_PATH" 2>/dev/null; then
            logger -t "$LOG_TAG" "Screenshot saved: $OUTPUT_PATH"
            # Output JSON for this window
            echo "$metadata" | jq --arg path "$OUTPUT_PATH" '. + {screenshot_path: $path}'
        else
            logger -t "$LOG_TAG" "ERROR: Failed to move screenshot for window $ID"
            echo "$metadata" | jq --arg path "" '. + {screenshot_path: $path, error: "file_move_failed"}'
        fi
    else
        logger -t "$LOG_TAG" "ERROR: Failed to capture window $ID"
        echo "$metadata" | jq --arg path "" '. + {screenshot_path: $path, error: "capture_failed"}'
    fi
done | jq -s '.'

logger -t "$LOG_TAG" "Completed capture of all windows"

Score: 7.5/10

---

Security Analysis

Threat Coverage

✅ Well Handled:

1. Audit Trail: Comprehensive logging to systemd journal
2. Privacy Awareness: Clear documentation of invisible capture capability
3. Protection Mechanisms: Window blocking configuration documented
4. Attack Scenarios: Documented in SECURITY.md
5. No Shell Injection: Safe use of jq --arg everywhere

⚠️ Could Improve:

1. Rate Limiting: No protection against rapid-fire captures
2. Privilege Escalation: No check if running as expected user
3. IPC Socket Security: Assumes niri socket is properly protected (document this assumption)
4. Screenshot Directory Permissions: Should verify/enforce 700 permissions

Recommendations

1. Add rate limiting check:

# At start of scripts
RECENT_CAPTURES=$(journalctl --user -t niri-capture --since "10 seconds ago" | wc -l)
if [[ "$RECENT_CAPTURES" -gt 10 ]]; then
    logger -t "$LOG_TAG" "ERROR: Rate limit exceeded ($RECENT_CAPTURES captures in 10s)"
    echo "Error: Too many captures in short time, potential abuse" >&2
    exit 1
fi

2. Verify screenshot directory permissions:

# In each script's initialization
SCREENSHOT_DIR="${HOME}/Pictures/Screenshots"
if [[ -d "$SCREENSHOT_DIR" ]]; then
    PERMS=$(stat -c %a "$SCREENSHOT_DIR")
    if [[ "$PERMS" != "700" ]]; then
        logger -t "$LOG_TAG" "WARNING: Screenshot directory has weak permissions: $PERMS (should be 700)"
    fi
fi

---

Compliance with AGENTS.md Guidelines

Checking against /home/dan/proj/skills/AGENTS.md:

✅ Bash Script Requirements:

• ✅ Shebang: #!/usr/bin/env bash - PASS
• ✅ Error handling: set -euo pipefail - PASS
• ✅ Audit logging: logger -t <tag> - PASS
• ✅ Variables: UPPER_CASE constants, lower_case locals - PASS (mostly, some could be local)
• ✅ Error output: echo "Error: ..." >&2 - PASS
• ✅ Dependency checks: Missing! - FAIL
• ✅ JSON parsing: jq with --arg - PASS

Missing: Dependency Checks

None of the scripts check if required commands exist. Should add:

# At start of each script
for cmd in niri jq logger; do
    if ! command -v "$cmd" >/dev/null 2>&1; then
        echo "Error: Required command not found: $cmd" >&2
        exit 1
    fi
done

✅ Skill Structure Requirements:

• ✅ SKILL.md with YAML frontmatter - PASS
• ✅ Required sections: When to Use, Process, Requirements - PASS
• ✅ Security-sensitive: SECURITY.md with threat model - PASS
• ✅ README.md with installation instructions - PASS

---

Testing Recommendations

Unit Tests Needed

1. capture-focused.sh:

   • Test with no focused window
   • Test with focused window
   • Test screenshot directory doesn't exist
   • Test niri command failure

2. capture-by-title.sh:

   • Test no match found
   • Test single match
   • Test multiple matches
   • Test special characters in title
   • Test empty/whitespace search term

3. capture-all-windows.sh:

   • Test empty window list
   • Test single window
   • Test multiple windows
   • Test filename collisions
   • Test move failure

Integration Tests Needed

1. Verify audit logs are written correctly
2. Verify screenshots created with correct permissions
3. Verify cross-workspace capture works
4. Verify blocked windows are not captured (test block-out-from)

---

Recommendations Summary

High Priority

1. Add dependency checks to all scripts
2. Add error checking for niri command failures (don't silent discard)
3. Verify screenshot directory exists before attempting capture
4. Check screenshot file was created after capture command

Medium Priority

5. Add rate limiting to prevent abuse
6. Improve error messages for multiple window matches
7. Use timestamp-based screenshot finding instead of ls -t
8. Add audit logging to capture-all-windows.sh
9. Verify/warn about screenshot directory permissions

Low Priority

10. Query niri config for screenshot-path instead of hardcoding
11. Add timeout handling for niri commands
12. Consider adding a "dry-run" mode
13. Add unit tests for all scripts

---

Example Improvements

Improved Script Template

#!/usr/bin/env bash
# Script purpose

set -euo pipefail

LOG_TAG="niri-capture"

# Dependency checks
for cmd in niri jq logger; do
    if ! command -v "$cmd" >/dev/null 2>&1; then
        echo "Error: Required command not found: $cmd" >&2
        exit 1
    fi
done

# Rate limiting
RECENT_CAPTURES=$(journalctl --user -t "$LOG_TAG" --since "10 seconds ago" 2>/dev/null | wc -l)
if [[ "$RECENT_CAPTURES" -gt 10 ]]; then
    logger -t "$LOG_TAG" "ERROR: Rate limit exceeded"
    echo "Error: Too many recent captures, potential abuse" >&2
    exit 1
fi

# Verify screenshot directory
SCREENSHOT_DIR="${XDG_PICTURES_DIR:-$HOME/Pictures}/Screenshots"
if [[ ! -d "$SCREENSHOT_DIR" ]]; then
    logger -t "$LOG_TAG" "ERROR: Screenshot directory not found: $SCREENSHOT_DIR"
    echo "Error: Screenshot directory does not exist" >&2
    exit 1
fi

# Main script logic here...

---

Final Verdict

Overall Score: 8.5/10

Strengths:

• Excellent security awareness and documentation
• Good bash practices (error handling, safe variable usage)
• Comprehensive audit logging
• Clear, well-organized code
• Security-first design

Critical Issues: None

Non-Critical Issues:

• Missing dependency checks (easily fixed)
• Some error handling could be more robust
• Minor efficiency improvements possible

Recommendation: ✅ APPROVED FOR DEPLOYMENT with suggestion to implement high-priority improvements in next iteration.

This is a well-crafted, security-conscious skill that demonstrates mature understanding of both bash scripting and security considerations. The comprehensive SECURITY.md and thorough audit logging show excellent judgment for a capability with privacy implications.

---

Action Items

For the next version, consider:

1. Add dependency checks to all scripts
2. Improve error handling for niri command failures
3. Add screenshot directory validation
4. Implement rate limiting
5. Add unit tests
6. Consider adding a CHANGELOG.md to track improvements

---

Review Completed: 2025-11-09
Status: APPROVED with recommendations
Next Review: After implementing high-priority improvements