cal/claude-home
1
0
Fork 0
Code Issues 6 Pull Requests 4 Actions Packages Projects Releases Wiki Activity

feat: right-size VM 115 (docker-sba) 16→8 vCPUs #44

Open
cal wants to merge 1 commits from enhancement/18-rightsize-vm115-vcpus into main
pull from: enhancement/18-rightsize-vm115-vcpus
merge into: cal:main
Conversation 1 Commits 1 Files Changed 3 +42 -10
cal commented 2026-04-03 20:40:01 +00:00
Owner
Copy Link

Summary

  • Update VM 115 config: sockets: 2 → sockets: 1 (16 → 8 vCPUs)
  • Add --hosts label:ip,... flag to homelab-audit.sh for targeted post-change audits
  • Add parsing tests for the new --hosts flag (single + multi host)

Operational steps

Operational next steps (Proxmox snapshot, shutdown, apply, verify) are documented in issue #18 comment. These require a maintenance window and should be performed after merge.

Closes #18

Test plan

  • test-audit-collectors.sh — all 10 tests pass (8 existing + 2 new --hosts parsing tests)

🤖 Generated with Claude Code

## Summary - Update VM 115 config: `sockets: 2` → `sockets: 1` (16 → 8 vCPUs) - Add `--hosts label:ip,...` flag to `homelab-audit.sh` for targeted post-change audits - Add parsing tests for the new `--hosts` flag (single + multi host) ## Operational steps Operational next steps (Proxmox snapshot, shutdown, apply, verify) are documented in [issue #18 comment](https://git.manticorum.com/cal/claude-home/issues/18#issuecomment-5682). These require a maintenance window and should be performed after merge. Closes #18 ## Test plan - [x] `test-audit-collectors.sh` — all 10 tests pass (8 existing + 2 new `--hosts` parsing tests) 🤖 Generated with [Claude Code](https://claude.com/claude-code)
cal added 2 commits 2026-04-03 20:40:01 +00:00
feat: add session resumption and Agent SDK evaluation
All checks were successful
Auto-merge docs-only PRs / auto-merge-docs (pull_request) Successful in 3s
Details
0dae52441a 
- runner.sh: opt-in session persistence via session_resumable and
  resume_last_session settings; fix read_setting to normalize booleans
- issue-poller.sh: capture and log session_id from worker invocations,
  include in result JSON
- pr-reviewer-dispatcher.sh: capture and log session_id from reviews
- n8n workflow: add --append-system-prompt to initial SSH node, add
  Follow Up Diagnostics node using --resume for deeper investigation,
  update Discord Alert with remediation details
- Add Agent SDK evaluation doc (CLI vs Python/TS SDK comparison)
- Update CONTEXT.md with session resumption documentation

Closes #3

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
feat: right-size VM 115 config and add --hosts flag to audit script
All checks were successful
Auto-merge docs-only PRs / auto-merge-docs (pull_request) Successful in 2s
Details
d47cd36440 
Reduce VM 115 (docker-sba) from 16 vCPUs (2×8) to 8 vCPUs (1×8) to
match actual workload (0.06 load/core). Add --hosts flag to
homelab-audit.sh for targeted post-change audits.

Closes #18

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Claude added the
ai-reviewing
 label 2026-04-03 20:45:11 +00:00
Claude requested changes 2026-04-03 20:48:11 +00:00
Dismissed
Claude left a comment
Collaborator
Copy Link

AI Code Review

Files Reviewed

  • server-configs/proxmox/qemu/115.conf (modified)
  • monitoring/scripts/homelab-audit.sh (modified)
  • monitoring/scripts/test-audit-collectors.sh (modified)
  • legacy/headless-claude/n8n-workflow-import.json (modified)
  • scheduled-tasks/CONTEXT.md (modified)
  • scheduled-tasks/agent-sdk-evaluation.md (added)

Findings

Correctness

🔴 Blocking — Merge conflict with PR #38 (--hosts already exists in main)

This PR branch was forked from a8c85a8d before commit 1a3785f ("feat: dynamic summary, --hosts filter, and --json output (#24)") was merged. That commit already added --hosts support to homelab-audit.sh using the variable HOSTS_FILTER with a complete implementation. This PR introduces a different implementation using MANUAL_HOSTS — hence mergeable: false.

The conflict affects monitoring/scripts/homelab-audit.sh specifically. The main branch already has:

  • Variable: HOSTS_FILTER (not MANUAL_HOSTS)
  • Arg format: plain hostnames, calls parse_and_report "$host" "$host"
  • Handles proxmox backup check when proxmox appears in the filter
  • Populates AUDITED_HOSTS array for per-host summary table

The PR's implementation supports label:ip pairs (splitting on :), which is more expressive — the label and address are separate. After rebase, there's an opportunity to upgrade the existing HOSTS_FILTER logic to support the label:ip format from this PR, rather than replacing it.

Action required: Rebase on current main and resolve the conflict in homelab-audit.sh. The label:ip parsing logic and the new tests are good — they should be preserved and integrated with the existing HOSTS_FILTER variable and AUDITED_HOSTS population.

VM config math is correct ✅

115.conf: sockets: 2 → sockets: 1, with cores: 8 unchanged → 2×8=16 → 1×8=8 vCPUs. numa: 0 is appropriate for a single-socket config.

n8n Follow Up Diagnostics node — credential placeholder

legacy/headless-claude/n8n-workflow-import.json, the new ssh-followup node has:

"id": "REPLACE_WITH_CREDENTIAL_ID"

This is a placeholder that must be replaced with the actual credential ID before importing the workflow. This won't cause a review blocker on its own (the file is a workflow template), but it should be noted — importing as-is will fail.

Security

  • No new security issues introduced in scope.
  • The Discord webhook URL in n8n-workflow-import.json is pre-existing.

Style & Conventions

  • Bash scripts follow existing patterns. IFS=',' read -ra for CSV parsing is idiomatic.
  • The new --hosts tests are clear and follow the pass/fail helper pattern from the existing test suite.
  • agent-sdk-evaluation.md is well-structured with a solid rationale for staying on CLI (auth cost is the right blocker to call out).

Suggestions

  • The new "Follow Up Diagnostics" n8n branch runs for every alert (low, medium, high, critical). With --max-turns 15 on each follow-up run, this will increase cost on every non-healthy check. Consider routing the follow-up branch only when $json.severity == 'critical' || $json.severity == 'high' in the IF node condition.

Verdict: REQUEST_CHANGES

One blocker: the PR is not mergeable due to a conflict with the --hosts implementation merged to main in PR #38. The VM config change, n8n session resumption additions, test cases, and documentation are all solid — they just need a rebase to land cleanly. Rebase on main, integrate the label:ip parsing into the existing HOSTS_FILTER path, and this is ready.

Automated review by Claude PR Reviewer

## AI Code Review ### Files Reviewed - `server-configs/proxmox/qemu/115.conf` (modified) - `monitoring/scripts/homelab-audit.sh` (modified) - `monitoring/scripts/test-audit-collectors.sh` (modified) - `legacy/headless-claude/n8n-workflow-import.json` (modified) - `scheduled-tasks/CONTEXT.md` (modified) - `scheduled-tasks/agent-sdk-evaluation.md` (added) --- ### Findings #### Correctness **🔴 Blocking — Merge conflict with PR #38 (`--hosts` already exists in main)** This PR branch was forked from `a8c85a8d` before commit `1a3785f` ("feat: dynamic summary, --hosts filter, and --json output (#24)") was merged. That commit already added `--hosts` support to `homelab-audit.sh` using the variable `HOSTS_FILTER` with a complete implementation. This PR introduces a *different* implementation using `MANUAL_HOSTS` — hence `mergeable: false`. The conflict affects `monitoring/scripts/homelab-audit.sh` specifically. The main branch already has: - Variable: `HOSTS_FILTER` (not `MANUAL_HOSTS`) - Arg format: plain hostnames, calls `parse_and_report "$host" "$host"` - Handles proxmox backup check when `proxmox` appears in the filter - Populates `AUDITED_HOSTS` array for per-host summary table The PR's implementation supports `label:ip` pairs (splitting on `:`), which is more expressive — the label and address are separate. After rebase, there's an opportunity to upgrade the existing `HOSTS_FILTER` logic to support the `label:ip` format from this PR, rather than replacing it. **Action required:** Rebase on current `main` and resolve the conflict in `homelab-audit.sh`. The `label:ip` parsing logic and the new tests are good — they should be preserved and integrated with the existing `HOSTS_FILTER` variable and `AUDITED_HOSTS` population. --- **VM config math is correct ✅** `115.conf`: `sockets: 2` → `sockets: 1`, with `cores: 8` unchanged → 2×8=16 → 1×8=8 vCPUs. `numa: 0` is appropriate for a single-socket config. **n8n Follow Up Diagnostics node — credential placeholder** `legacy/headless-claude/n8n-workflow-import.json`, the new `ssh-followup` node has: ```json "id": "REPLACE_WITH_CREDENTIAL_ID" ``` This is a placeholder that must be replaced with the actual credential ID before importing the workflow. This won't cause a review blocker on its own (the file is a workflow template), but it should be noted — importing as-is will fail. #### Security - No new security issues introduced in scope. - The Discord webhook URL in `n8n-workflow-import.json` is pre-existing. #### Style & Conventions - Bash scripts follow existing patterns. `IFS=',' read -ra` for CSV parsing is idiomatic. - The new `--hosts` tests are clear and follow the pass/fail helper pattern from the existing test suite. - `agent-sdk-evaluation.md` is well-structured with a solid rationale for staying on CLI (auth cost is the right blocker to call out). #### Suggestions - The new "Follow Up Diagnostics" n8n branch runs for **every** alert (low, medium, high, critical). With `--max-turns 15` on each follow-up run, this will increase cost on every non-healthy check. Consider routing the follow-up branch only when `$json.severity == 'critical' || $json.severity == 'high'` in the IF node condition. --- ### Verdict: REQUEST_CHANGES One blocker: the PR is not mergeable due to a conflict with the `--hosts` implementation merged to main in PR #38. The VM config change, n8n session resumption additions, test cases, and documentation are all solid — they just need a rebase to land cleanly. Rebase on `main`, integrate the `label:ip` parsing into the existing `HOSTS_FILTER` path, and this is ready. --- *Automated review by Claude PR Reviewer*
Claude added
ai-changes-requested
 and removed
ai-reviewing
 labels 2026-04-03 20:48:26 +00:00
Claude added the
ai-working
 label 2026-04-03 21:00:24 +00:00
cal force-pushed enhancement/18-rightsize-vm115-vcpus from d47cd36440 to ffb1eaef7f 2026-04-03 21:03:59 +00:00 Compare
Claude commented 2026-04-03 21:04:17 +00:00
Collaborator
Copy Link

Addressed review feedback — rebased on main and resolved the --hosts conflict.

Changes made

Rebase on main (commit 64f299a): The PR was forked from a8c85a8d, before PR #38 merged --hosts support using HOSTS_FILTER. Rebased cleanly; the only conflict was in homelab-audit.sh.

Integrated label:ip parsing into HOSTS_FILTER (monitoring/scripts/homelab-audit.sh):

  • Removed the PR's separate MANUAL_HOSTS variable
  • Upgraded main's HOSTS_FILTER loop to support label:ip format using ${entry%%:*} / ${entry#*:} splitting
  • Plain hostnames (no colon) continue to work — label and addr both resolve to the entry string
  • Preserved AUDITED_HOSTS+=("$label") population, proxmox backup recency check, JSON_OUTPUT, and dynamic summary table from main
  • Updated --hosts error message to include (label:ip,label:ip,...) hint
  • Updated usage comment in script header

All other PR changes (VM 115 config, n8n session resumption, test cases, agent-sdk-evaluation.md) landed cleanly via rebase with no modifications needed.

The label:ip parsing tests in test-audit-collectors.sh are preserved and continue to validate the split logic correctly.

Addressed review feedback — rebased on `main` and resolved the `--hosts` conflict. ### Changes made **Rebase on `main`** (commit `64f299a`): The PR was forked from `a8c85a8d`, before PR #38 merged `--hosts` support using `HOSTS_FILTER`. Rebased cleanly; the only conflict was in `homelab-audit.sh`. **Integrated `label:ip` parsing into `HOSTS_FILTER`** (`monitoring/scripts/homelab-audit.sh`): - Removed the PR's separate `MANUAL_HOSTS` variable - Upgraded main's `HOSTS_FILTER` loop to support `label:ip` format using `${entry%%:*}` / `${entry#*:}` splitting - Plain hostnames (no colon) continue to work — `label` and `addr` both resolve to the entry string - Preserved `AUDITED_HOSTS+=("$label")` population, proxmox backup recency check, `JSON_OUTPUT`, and dynamic summary table from `main` - Updated `--hosts` error message to include `(label:ip,label:ip,...)` hint - Updated usage comment in script header All other PR changes (VM 115 config, n8n session resumption, test cases, `agent-sdk-evaluation.md`) landed cleanly via rebase with no modifications needed. The `label:ip` parsing tests in `test-audit-collectors.sh` are preserved and continue to validate the split logic correctly.
Claude removed the
ai-working
ai-changes-requested
 labels 2026-04-03 21:04:21 +00:00
Claude added the
ai-reviewing
 label 2026-04-03 21:15:26 +00:00
Claude approved these changes 2026-04-03 21:16:51 +00:00
Claude left a comment
Collaborator
Copy Link

AI Code Review

Files Reviewed

  • server-configs/proxmox/qemu/115.conf (modified)
  • monitoring/scripts/homelab-audit.sh (modified)
  • monitoring/scripts/test-audit-collectors.sh (modified)

Findings

Correctness

  • VM config: sockets: 2 → 1 with cores: 8 unchanged correctly reduces total vCPUs from 16 to 8. Change matches PR title and issue intent.
  • Bug fix included: The old --hosts mode checked [[ "$host" == "proxmox" ]] against the full label:ip entry — this would silently skip backup-recency checks when proxmox was passed in label:ip form. The new code extracts label="${entry%%:*}" first and then checks [[ "$label" == "proxmox" ]], which correctly handles both plain proxmox and proxmox:10.10.0.1.
  • Backwards compatibility: Plain hostnames without a colon still work — entry#*: returns the full string when no colon is present, so label == addr == hostname, matching the prior behavior.
  • Consistency: The --hosts branch now mirrors the existing inventory (else) branch, both calling parse_and_report "$label" "$addr" and check_cert_expiry "$label" "$addr".

Security

  • No new injection surfaces. HOSTS_FILTER was already user-controlled and passed to SSH before this PR.
  • No hardcoded credentials or secrets.

Style & Conventions

  • New comment (# Accepts comma-separated entries; each entry may be plain hostname or label:ip) correctly documents the accepted format.
  • Error message improvement for --hosts with format hint is helpful.
  • Code style and variable naming match existing conventions throughout.

Suggestions

  • The tests validate the bash string-splitting logic directly, which is the right level for this test suite. One optional addition (non-blocking): a test case for a plain hostname (no colon) to document that addr falls back to the label, but the existing tests are sufficient.

Verdict: APPROVED

Clean, well-scoped PR. The VM right-sizing is correct, the --hosts enhancement is backwards-compatible, a latent proxmox-detection bug is fixed along the way, and new tests cover the parsing logic. Ready to merge; operational steps (snapshot/shutdown/apply) are correctly deferred to a maintenance window per issue #18.

Automated review by Claude PR Reviewer

## AI Code Review ### Files Reviewed - `server-configs/proxmox/qemu/115.conf` (modified) - `monitoring/scripts/homelab-audit.sh` (modified) - `monitoring/scripts/test-audit-collectors.sh` (modified) ### Findings #### Correctness - **VM config**: `sockets: 2 → 1` with `cores: 8` unchanged correctly reduces total vCPUs from 16 to 8. Change matches PR title and issue intent. - **Bug fix included**: The old `--hosts` mode checked `[[ "$host" == "proxmox" ]]` against the full `label:ip` entry — this would silently skip backup-recency checks when proxmox was passed in `label:ip` form. The new code extracts `label="${entry%%:*}"` first and then checks `[[ "$label" == "proxmox" ]]`, which correctly handles both plain `proxmox` and `proxmox:10.10.0.1`. - **Backwards compatibility**: Plain hostnames without a colon still work — `entry#*:` returns the full string when no colon is present, so `label == addr == hostname`, matching the prior behavior. - **Consistency**: The `--hosts` branch now mirrors the existing inventory (else) branch, both calling `parse_and_report "$label" "$addr"` and `check_cert_expiry "$label" "$addr"`. #### Security - No new injection surfaces. `HOSTS_FILTER` was already user-controlled and passed to SSH before this PR. - No hardcoded credentials or secrets. #### Style & Conventions - New comment (`# Accepts comma-separated entries; each entry may be plain hostname or label:ip`) correctly documents the accepted format. - Error message improvement for `--hosts` with format hint is helpful. - Code style and variable naming match existing conventions throughout. #### Suggestions - The tests validate the bash string-splitting logic directly, which is the right level for this test suite. One optional addition (non-blocking): a test case for a plain hostname (no colon) to document that `addr` falls back to the label, but the existing tests are sufficient. ### Verdict: APPROVED Clean, well-scoped PR. The VM right-sizing is correct, the `--hosts` enhancement is backwards-compatible, a latent proxmox-detection bug is fixed along the way, and new tests cover the parsing logic. Ready to merge; operational steps (snapshot/shutdown/apply) are correctly deferred to a maintenance window per issue #18. --- *Automated review by Claude PR Reviewer*
Claude added
ai-reviewed
 and removed
ai-reviewing
 labels 2026-04-03 21:16:59 +00:00
All checks were successful
Auto-merge docs-only PRs / auto-merge-docs (pull_request) Successful in 2s
Details
This pull request can be merged automatically.
This branch is out-of-date with the base branch
You are not authorized to merge this pull request.
View command line instructions.

Checkout

From your project repository, check out a new branch and test the changes.
git fetch -u origin enhancement/18-rightsize-vm115-vcpus:enhancement/18-rightsize-vm115-vcpus
git checkout enhancement/18-rightsize-vm115-vcpus
Sign in to join this conversation.
Reviewers
No reviewers
Claude
Labels
Clear labels   
ai-changes-requested

   
ai-failed

   
ai-pr-opened

   
ai-reviewed

   
ai-reviewing

   
ai-working

   
infra-audit

Infrastructure audit remediation tasks

  
monitoring

Monitoring, alerting, and observability

  
operations

Ongoing operational improvements

  
proxmox

Proxmox VM/CT management

  
script

Script improvements and tooling

No Label
ai-changes-requested
ai-failed
ai-pr-opened
ai-reviewed
ai-reviewing
ai-working
infra-audit
monitoring
operations
proxmox
script
Milestone
Clear milestone
No items
No Milestone
Projects
Clear projects
No project
Assignees
Clear assignees
No Assignees
2 Participants
Notifications
Due Date
The due date is invalid or out of range. Please use the format 'yyyy-mm-dd'.

No due date set.

Dependencies

No dependencies set.

Reference: cal/claude-home#44
No description provided.
Delete Branch "enhancement/18-rightsize-vm115-vcpus"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?