AI generated code suggestions

REVIEW_NOTES.md

@@ -0,0 +1,238 @@
+# Script Review Notes - Improvements & Observations
+
+**Branch:** `review-improvements`  
+**Date:** 2026-07-20
+
+---
+
+## 📋 Summary Table
+
+| File | Lines | Type | Overall Rating | Key Issues |
+|------|-------|------|----------------|------------|
+| `4-private-ip.sh` | 6 | Simple | ⭐⭐⭐⭐ | Minor - uses deprecated shell syntax |
+| `4-public-ip.sh` | 44 | IP Lookup | ⭐⭐⭐⭐ | Medium - fallback order could be optimized |
+| `6-public-ip.sh` | 47 | IPv6 Lookup | ⭐⭐⭐⭐ | Medium - similar to IPv4, duplicated code |
+| `cloudflare.sh` | 253 | API Wrapper | ⭐⭐⭐ | High - security concerns with tokens in args |
+| `discord.sh` | 242 | Discord Sender | ⭐⭐⭐⭐ | Low - well documented, minor tweaks needed |
+| `git-prompt.sh` | 672 | Git Prompt | ⭐⭐⭐ | Medium - large file, some edge cases |
+| `Dynv6.ps1` | 145 | Powershell DDNS | ⭐⭐⭐⭐ | Low - clean PowerShell code |
+| `delay.sh` | 20 | Utility | ⭐⭐ | High - uses `bc`, poor error handling |
+| `df.sh` | 13 | Wrapper | ⭐⭐⭐⭐ | Medium - relies on external script |
+| `domain-check.sh` | 49 | DNS Checker | ⭐⭐⭐ | Medium - regex fragile, missing error handling |
+| `dynv6.sh` | 114 | Bash DDNS | ⭐⭐⭐ | Medium - backticks vs `$()` inconsistency |
+| `fetch-all.sh` | 52 | Git Fetch | ⭐⭐⭐⭐ | Low - clean and functional |
+| `list-all.sh` | 52 | Git List | ⭐⭐⭐⭐ | Low - mirror of fetch-all.sh |
+| `pull-all.sh` | 52 | Git Pull | ⭐⭐⭐⭐ | Medium - could check branch upstream first |
+| `status-all.sh` | 110 | Git Status | ⭐⭐⭐ | High - complex regex matching, colors hardcoded |
+| `upgrade.sh` | 21 | System Upgrade | ⭐⭐ | High - runs as root by default, no dry-run |
+| `thermal.sh` | 48 | Thermal Monitor | ⭐⭐⭐ | Medium - assumes `/sys/thermal/*` exists |
+| `ssh-ident` | 1029 | SSH Manager | ⭐⭐⭐ | Medium - Python-heavy, dependencies |
+
+---
+
+## 📝 Detailed Notes by File
+
+### IP & Network Tools
+
+#### `4-private-ip.sh`
+```bash
+ip -4 addr list scope global | sed -n 's/.*inet \([0-9\.]\+\).*/\1/p' | head -n 1
+```
+
+**Observations:**
+- ✅ Very concise and functional
+- ⚠️ Uses `sed` with regex that may fail on some edge cases
+- ⚠️ Comments out alternative (longer) method without explanation
+
+**Suggestions:**
+1. Add a fallback using `/usr/sbin/ip6tables` or similar as backup
+2. Consider: `ip -4 addr show | awk '/inet / {print $2; exit}'`
+3. Remove commented-out code with note
+
+---
+
+#### `4-public-ip.sh` & `6-public-ip.sh`
+**Observations:**
+- ✅ Good fallback chain (dig → curl → wget)
+- ⚠️ Uses `-4` flag consistently but no graceful degradation if all fail
+- ⚠️ Error messages could be more descriptive
+
+**Suggestions:**
+1. Add timeout to curl/wget calls: `curl --connect-timeout 5 ...`
+2. Consider caching the last known IP with TTL
+3. Add metrics/counter for success rate
+4. In `6-public-ip.sh`, consider using both IPv4 and IPv6 APIs simultaneously
+
+---
+
+### API & Service Integration
+
+#### `cloudflare.sh`
+**Observations:**
+- ✅ Handles `-c`, `-f`, `-q`, `-t` flags well
+- ⚠️ **Security**: Token/zone defaults loaded from config, but overrides via positional args
+- ⚠️ Large file (253 lines) - could be modularized
+
+**Suggestions:**
+1. Add `--dry-run` flag for testing before commits
+2. Consider storing the resolved IP in a temp file to avoid repeated lookups
+3. Add retry logic with exponential backoff for failed API calls
+4. Parse JSON response more robustly using `jq --argjson ...` pattern
+
+---
+
+#### `discord.sh`
+**Observations:**
+- ✅ Excellent color palette support
+- ✅ Handles message splitting for long messages
+- ⚠️ ANSI escape codes embedded in strings may need escaping
+
+**Suggestions:**
+1. Consider creating a helper function to build the JSON payload
+2. Add optional rate-limit header handling (`--wait=true`)
+3. Log retry attempts if webhook fails
+
+---
+
+### Git Utilities
+
+#### `fetch-all.sh`, `list-all.sh`, `pull-all.sh`
+**Observations:**
+- ✅ Clean, consistent patterns
+- ⚠️ `-l` flag description says "follow symbolic links" but sets `-H` (which is actually "follow hardlinks only")
+- ⚠️ `checkHidden` uses glob pattern that may not work as expected
+
+**Suggestions:**
+1. Fix help text: `-L = follow symlinks`, `-H = follow hardlinks`
+2. Consider adding option to exclude specific paths
+3. Add progress bar or counter for large repo counts
+
+---
+
+#### `status-all.sh`
+**Observations:**
+- ✅ Very detailed status reporting with colors
+- ⚠️ Complex regex matching against git output - fragile across versions
+- ⚠️ Colors hardcoded as escape sequences
+
+**Suggestions:**
+1. Use `GIT_PS1_SHOWCOLORHINTS` env var to toggle color rendering
+2. Extract common status patterns into variables/regex constants
+3. Add option for "quiet" or "verbose" output modes
+
+---
+
+### System & Hardware Tools
+
+#### `upgrade.sh`
+**Observations:**
+- ⚠️ Runs apt commands as root immediately
+- ⚠️ No dry-run mode to preview changes
+- ⚠️ Could lock filesystem during update
+
+**Suggestions:**
+1. Add `--dry-run` flag that just updates without installing
+2. Wrap in flock: `flock -n /var/lock/apt.lock apt update ...`
+3. Create pre/post hooks directory for custom steps
+4. Report disk space usage before/after
+
+---
+
+#### `thermal.sh`
+**Observations:**
+- ✅ Clean temperature reading loop
+- ⚠️ Assumes `/sys/class/thermal/*/* -path '*/thermal_*' -name 'temp'` structure
+
+**Suggestions:**
+1. Add graceful handling for systems without thermal sensors
+2. Consider configurable alert thresholds per zone
+3. Add optional Discord/Pushover notification on alert
+
+---
+
+### Utilities
+
+#### `delay.sh`
+```bash
+for i in $(seq 1 50); do sleep "${slice}"; echo -n '.'; done
+```
+
+**Observations:**
+- ⚠️ Requires `bc` for floating-point math (not always available)
+- ⚠️ Output dots could be suppressed with flag
+- ⚠️ Default range of 1-60 seconds is arbitrary
+
+**Suggestions:**
+1. Add `--no-output` flag
+2. Consider using pure bash arithmetic: `sleep $(awk "BEGIN {printf \"%.3f\", $seconds/50}")`
+3. Add support for millisecond precision
+
+---
+
+#### `dynv6.sh`
+**Observations:**
+- ✅ Modular design with scope/device options
+- ⚠️ Uses backticks instead of `$()` (legacy style)
+- ⚠️ Error handling minimal
+
+**Suggestions:**
+1. Convert to `$()` for POSIX compliance
+2. Add `--test-mode` that validates credentials without updating
+3. Consider storing last IP in config file for trend analysis
+
+---
+
+### Large/Complex Files
+
+#### `git-prompt.sh` (672 lines)
+- ✅ Feature-rich prompt customization
+- ⚠️ Requires careful review due to length
+- ⚠e Many conditional branches with edge cases
+
+**Quick Wins:**
+1. Consider extracting sub-functions into separate files
+2. Add unit tests for key functions
+3. Document what each environment variable does
+
+---
+
+#### `ssh-ident` (1029 lines - Python)
+- ✅ Sophisticated SSH agent management
+- ⚠️ Requires Python 2.6+ (very broad compatibility)
+- ⚠️ Large monolithic file
+
+**Suggestions:**
+1. Consider packaging as standalone module
+2. Add `--config-file` override for testing
+3. Document batch mode behavior more clearly
+
+---
+
+## 🎯 Priority Improvements
+
+### High Priority (Security/Reliability)
+1. **cloudflare.sh**: Add timeout and retry logic to API calls
+2. **upgrade.sh**: Implement dry-run and file locking
+3. All IP lookup scripts: Add timeouts and fallback chains
+
+### Medium Priority (Maintainability)
+4. Convert `dynv6.sh` backticks to `$()` syntax
+5. Extract common patterns from git-* utilities
+6. Add unit tests for critical functions
+
+### Low Priority (Nice-to-Have)
+7. Create shared base classes/modules for IP lookups
+8. Add logging framework across all scripts
+9. Consider creating a "master" config file template
+
+---
+
+## 📦 Files to Investigate Further
+
+- `ssh-ident` - Check Python version requirements in production
+- `git-prompt.sh` - Verify compatibility with modern git versions
+- `cloudflare.sh` - Test error handling for rate-limited responses
+
+---
+
+*Generated on branch: review-improvements*

pull-docker.sh

@@ -46,7 +46,7 @@ function process_docker_pull () {
 
     quietDocker=$(docker compose pull --ignore-buildable --ignore-pull-failures)
 
-    if docker compose up -d --dry-run 2>&1 | grep -q "Recreate"; then
+    if docker compose up -d --dry-run | grep -q "Recreate"; then
         # Only restart services that were already running
         if ((${#runningServices[@]} > 0)); then
             echo "restarting the following services: ${runningServices[@]}"

secure-gitea.py

@@ -1,563 +0,0 @@
-#!/usr/bin/env python3
-
-import argparse
-import os
-import sys
-from urllib.parse import quote
-
-import requests
-
-
-# ---------------------------------------------------------------------------
-# Configuration
-# ---------------------------------------------------------------------------
-
-GITEA_URL = "https://" + os.environ.get("GITEA_DOMAIN", "").rstrip("/")
-GITEA_TOKEN = os.environ.get("GITEA_TOKEN", "")
-USERNAME = os.environ.get("GITEA_ADMIN", "andrew")
-
-BRANCH = "main"
-TAG_PATTERN = "v*"
-
-# Number of repositories to request per API page.
-PAGE_SIZE = 50
-
-
-# ---------------------------------------------------------------------------
-# Desired branch protection configuration
-#
-# Only fields listed here are managed by this script.
-# Other Gitea branch-protection settings are left untouched.
-# ---------------------------------------------------------------------------
-
-BRANCH_DESIRED = {
-    "rule_name": BRANCH,
-
-    # Direct pushes
-    "enable_push": True,
-    "enable_push_whitelist": True,
-    "push_whitelist_usernames": [USERNAME],
-    "push_whitelist_teams": [],
-    "push_whitelist_deploy_keys": False,
-
-    # Force pushes -- explicitly disabled
-    "enable_force_push": False,
-    "enable_force_push_whitelist": False,
-    "force_push_whitelist_usernames": [],
-    "force_push_whitelist_teams": [],
-    "force_push_whitelist_deploy_keys": False,
-
-    # Pull request approvals
-    "required_approvals": 1,
-    "enable_approvals_whitelist": True,
-    "approvals_whitelist_username": [USERNAME],
-    "approvals_whitelist_teams": [],
-
-    # Pull request merging
-    "enable_merge_whitelist": True,
-    "merge_whitelist_usernames": [USERNAME],
-    "merge_whitelist_teams": [],
-
-    # Status checks
-    "enable_status_check": False,
-    "status_check_contexts": [],
-}
-
-
-# ---------------------------------------------------------------------------
-# Gitea nullable branch-protection fields
-#
-# Gitea returns None for these fields when the corresponding whitelist
-# functionality is disabled. Treat those values as equivalent to the
-# explicit values above when comparing configurations.
-# ---------------------------------------------------------------------------
-
-BRANCH_NULL_EQUIVALENTS = {
-    "enable_force_push_whitelist": False,
-    "force_push_whitelist_usernames": [],
-    "force_push_whitelist_teams": [],
-    "force_push_whitelist_deploy_keys": False,
-}
-
-
-# ---------------------------------------------------------------------------
-# Desired tag protection configuration
-#
-# Tags matching TAG_PATTERN are protected. Only USERNAME may create/delete
-# those tags.
-# ---------------------------------------------------------------------------
-
-TAG_DESIRED = {
-    "name_pattern": TAG_PATTERN,
-    "whitelist_usernames": [USERNAME],
-    "whitelist_teams": [],
-}
-
-
-# ---------------------------------------------------------------------------
-# API session
-# ---------------------------------------------------------------------------
-
-session = requests.Session()
-session.headers.update({
-    "Authorization": f"token {GITEA_TOKEN}",
-    "Accept": "application/json",
-    "Content-Type": "application/json",
-})
-
-
-def api(method, path, **kwargs):
-    """Make a request to the Gitea API."""
-
-    url = f"{GITEA_URL}/api/v1{path}"
-
-    response = session.request(method, url, **kwargs)
-
-    if not response.ok:
-        print(
-            f"ERROR {method} {path}: "
-            f"{response.status_code} {response.text}",
-            file=sys.stderr,
-        )
-        response.raise_for_status()
-
-    if response.status_code == 204:
-        return None
-
-    return response.json()
-
-
-# ---------------------------------------------------------------------------
-# Value comparison
-# ---------------------------------------------------------------------------
-
-def values_equal(key, current, desired):
-    """
-    Compare a value returned by Gitea against the desired value.
-
-    Some Gitea branch-protection fields are returned as None when their
-    associated feature is disabled. Those fields are explicitly handled
-    above so that None and their configured disabled value are equivalent.
-    """
-
-    if current is None and key in BRANCH_NULL_EQUIVALENTS:
-        return desired == BRANCH_NULL_EQUIVALENTS[key]
-
-    return current == desired
-
-
-def describe_changes(current, desired, ignored=()):
-    """
-    Return human-readable descriptions of managed fields that differ.
-    """
-
-    changes = []
-
-    for key, wanted in desired.items():
-        if key in ignored:
-            continue
-
-        actual = current.get(key)
-
-        if not values_equal(key, actual, wanted):
-            changes.append(
-                f"{key}: {actual!r} -> {wanted!r}"
-            )
-
-    return changes
-
-
-# ---------------------------------------------------------------------------
-# Repository enumeration
-# ---------------------------------------------------------------------------
-
-def get_all_repositories():
-    """Enumerate every repository the authenticated user can administer."""
-
-    repos = []
-    page = 1
-
-    while True:
-        batch = api(
-            "GET",
-            "/user/repos",
-            params={
-                "limit": PAGE_SIZE,
-                "page": page,
-            },
-        )
-
-        if not batch:
-            break
-
-        repos.extend(batch)
-
-        if len(batch) < PAGE_SIZE:
-            break
-
-        page += 1
-
-    return repos
-
-
-# ---------------------------------------------------------------------------
-# Branch protection
-# ---------------------------------------------------------------------------
-
-def get_branch_protection(owner, repo):
-    """Return the existing protection for BRANCH, or None."""
-
-    protections = api(
-        "GET",
-        f"/repos/{quote(owner)}/{quote(repo)}/branch_protections",
-    )
-
-    for protection in protections:
-        if protection.get("rule_name") == BRANCH:
-            return protection
-
-    return None
-
-
-def describe_branch_protection(protection):
-    """Return a concise description of the current branch protection."""
-
-    if protection is None:
-        return "NO PROTECTION"
-
-    return (
-        f"push={protection.get('enable_push')} "
-        f"push_allowlist={protection.get('push_whitelist_usernames')} "
-        f"force_push={protection.get('enable_force_push')} "
-        f"approvals={protection.get('required_approvals')} "
-        f"approval_allowlist="
-        f"{protection.get('approvals_whitelist_username')} "
-        f"merge_allowlist="
-        f"{protection.get('merge_whitelist_usernames')}"
-    )
-
-
-def apply_branch_protection(owner, repo, existing):
-    """Create or update the branch protection."""
-
-    encoded_owner = quote(owner)
-    encoded_repo = quote(repo)
-
-    if existing is None:
-        api(
-            "POST",
-            f"/repos/{encoded_owner}/{encoded_repo}/branch_protections",
-            json=BRANCH_DESIRED,
-        )
-
-        return "CREATED"
-
-    # PATCH only the fields explicitly managed by this script.
-    payload = {
-        key: value
-        for key, value in BRANCH_DESIRED.items()
-        if key != "rule_name"
-    }
-
-    api(
-        "PATCH",
-        f"/repos/{encoded_owner}/{encoded_repo}/branch_protections/"
-        f"{quote(BRANCH)}",
-        json=payload,
-    )
-
-    return "UPDATED"
-
-
-# ---------------------------------------------------------------------------
-# Tag protection
-# ---------------------------------------------------------------------------
-
-def get_tag_protection(owner, repo):
-    """Return the existing protection for TAG_PATTERN, or None."""
-
-    protections = api(
-        "GET",
-        f"/repos/{quote(owner)}/{quote(repo)}/tag_protections",
-    )
-
-    for protection in protections:
-        if protection.get("name_pattern") == TAG_PATTERN:
-            return protection
-
-    return None
-
-
-def describe_tag_protection(protection):
-    """Return a concise description of the current tag protection."""
-
-    if protection is None:
-        return "NO PROTECTION"
-
-    return (
-        f"users={protection.get('whitelist_usernames')} "
-        f"teams={protection.get('whitelist_teams')}"
-    )
-
-
-def apply_tag_protection(owner, repo, existing):
-    """Create or update the tag protection."""
-
-    encoded_owner = quote(owner)
-    encoded_repo = quote(repo)
-
-    if existing is None:
-        api(
-            "POST",
-            f"/repos/{encoded_owner}/{encoded_repo}/tag_protections",
-            json=TAG_DESIRED,
-        )
-
-        return "CREATED"
-
-    # PATCH only the fields explicitly managed by this script.
-    payload = {
-        key: value
-        for key, value in TAG_DESIRED.items()
-        if key != "name_pattern"
-    }
-
-    api(
-        "PATCH",
-        f"/repos/{encoded_owner}/{encoded_repo}/tag_protections/"
-        f"{quote(str(existing['id']))}",
-        json=payload,
-    )
-
-    return "UPDATED"
-
-
-# ---------------------------------------------------------------------------
-# Main
-# ---------------------------------------------------------------------------
-
-def main():
-    parser = argparse.ArgumentParser(
-        description=(
-            "Apply standardized Gitea branch and tag protection."
-        )
-    )
-
-    parser.add_argument(
-        "--apply",
-        action="store_true",
-        help=(
-            "Actually modify repositories. Without this, only show "
-            "what would happen."
-        ),
-    )
-
-    args = parser.parse_args()
-
-    # -----------------------------------------------------------------------
-    # Validate configuration
-    # -----------------------------------------------------------------------
-
-    if not GITEA_URL or GITEA_URL == "https://":
-        print("GITEA_DOMAIN is not set.", file=sys.stderr)
-        sys.exit(1)
-
-    if not GITEA_TOKEN:
-        print("GITEA_TOKEN is not set.", file=sys.stderr)
-        sys.exit(1)
-
-    # -----------------------------------------------------------------------
-    # Header
-    # -----------------------------------------------------------------------
-
-    print(f"Gitea:       {GITEA_URL}")
-    print(f"Branch:      {BRANCH}")
-    print(f"Tag pattern: {TAG_PATTERN}")
-    print(f"User:        {USERNAME}")
-    print()
-
-    if not args.apply:
-        print("*** DRY RUN ***")
-        print("Use --apply to actually make changes.")
-        print()
-
-    # -----------------------------------------------------------------------
-    # Enumerate repositories
-    # -----------------------------------------------------------------------
-
-    print("Enumerating repositories...")
-
-    repositories = get_all_repositories()
-
-    print(f"Found {len(repositories)} repositories.")
-    print()
-
-    # -----------------------------------------------------------------------
-    # Counters
-    # -----------------------------------------------------------------------
-
-    branch_changed = 0
-    branch_unchanged = 0
-
-    tag_changed = 0
-    tag_unchanged = 0
-
-    skipped = 0
-    failed = 0
-
-    # -----------------------------------------------------------------------
-    # Process repositories
-    # -----------------------------------------------------------------------
-
-    for repo in repositories:
-        owner = repo["owner"]["login"]
-        name = repo["name"]
-
-        print(f"[{owner}/{name}]")
-
-        # Archived repositories cannot have their protection modified.
-        if repo.get("archived", False):
-            print("  SKIP: archived")
-            skipped += 1
-            print()
-            continue
-
-        try:
-            # ---------------------------------------------------------------
-            # Branch protection
-            # ---------------------------------------------------------------
-
-            protection = get_branch_protection(owner, name)
-
-            print(
-                f"  Branch: {describe_branch_protection(protection)}"
-            )
-
-            if protection is None:
-                print(
-                    f"    Would CREATE protection for {BRANCH}"
-                )
-
-                if args.apply:
-                    result = apply_branch_protection(
-                        owner,
-                        name,
-                        protection,
-                    )
-                    print(f"    {result}")
-
-                branch_changed += 1
-
-            else:
-                branch_changes = describe_changes(
-                    protection,
-                    BRANCH_DESIRED,
-                    ignored=("rule_name",),
-                )
-
-                if not branch_changes:
-                    print("    OK: already matches")
-                    branch_unchanged += 1
-
-                else:
-                    print(
-                        f"    Would UPDATE protection for {BRANCH}"
-                    )
-
-                    for change in branch_changes:
-                        print(f"      {change}")
-
-                    if args.apply:
-                        result = apply_branch_protection(
-                            owner,
-                            name,
-                            protection,
-                        )
-                        print(f"    {result}")
-
-                    branch_changed += 1
-
-            # ---------------------------------------------------------------
-            # Tag protection
-            # ---------------------------------------------------------------
-
-            tag_protection = get_tag_protection(owner, name)
-
-            print(
-                f"  Tags:   {describe_tag_protection(tag_protection)}"
-            )
-
-            if tag_protection is None:
-                print(
-                    f"    Would CREATE protection for {TAG_PATTERN}"
-                )
-
-                if args.apply:
-                    result = apply_tag_protection(
-                        owner,
-                        name,
-                        tag_protection,
-                    )
-                    print(f"    {result}")
-
-                tag_changed += 1
-
-            else:
-                tag_changes = describe_changes(
-                    tag_protection,
-                    TAG_DESIRED,
-                    ignored=("name_pattern",),
-                )
-
-                if not tag_changes:
-                    print("    OK: already matches")
-                    tag_unchanged += 1
-
-                else:
-                    print(
-                        f"    Would UPDATE protection for {TAG_PATTERN}"
-                    )
-
-                    for change in tag_changes:
-                        print(f"      {change}")
-
-                    if args.apply:
-                        result = apply_tag_protection(
-                            owner,
-                            name,
-                            tag_protection,
-                        )
-                        print(f"    {result}")
-
-                    tag_changed += 1
-
-        except requests.HTTPError:
-            print("  FAILED")
-            failed += 1
-
-        print()
-
-    # -----------------------------------------------------------------------
-    # Summary
-    # -----------------------------------------------------------------------
-
-    print("----------------------------------------")
-    print(f"Repositories:      {len(repositories)}")
-    print()
-    print(f"Branch changed:    {branch_changed}")
-    print(f"Branch unchanged:  {branch_unchanged}")
-    print()
-    print(f"Tags changed:      {tag_changed}")
-    print(f"Tags unchanged:    {tag_unchanged}")
-    print()
-    print(f"Skipped:           {skipped}")
-    print(f"Failed:            {failed}")
-
-    if not args.apply:
-        print()
-        print("Dry run complete. Nothing was changed.")
-
-
-if __name__ == "__main__":
-    main()
-