mirror of
https://github.com/GoodOlClint/PSProxmoxVE.git
synced 2026-09-04 03:05:32 +00:00
goodolclint-claude[bot]
08ee3ae249
The local dev environment had drifted badly from CI. Remove the parts that no longer describe anything real, and make the rest match how CI actually runs. Delete tests/dev.ps1. It wrapped run-integration.sh, which CI calls directly, and duplicated the module build that script already performs internally. As a second entry point it drifted: it still offered the PVE 8 leg retired in #88, mounted the Docker socket for storage containers replaced by the storage VM in #87, and pointed its remote-host examples at a runner decommissioned in the ARC migration. All four documents describing it used a positional syntax that bound the bare word to -Tests and then fell through to -Shell, so every documented command silently opened a container shell. Recorded as D019. Delete tests/infrastructure/runner/, a self-hosted-runner-in-Docker superseded by Actions Runner Controller. Make disk_storage and iso_storage required. Their defaults named a NAS that the lab replaced with Ceph, and CI overrides both from repository variables, so the defaults only ever misled local runs. require_env now fails at the top of a run rather than at terraform apply, and the descriptions point at tests/.env.test because cmd_provision deletes terraform.tfvars before applying. preflight-cleanup.sh no longer falls back to the literal "local" storage. An unset TF_VAR_iso_storage now skips only the ISO branch, leaving VM destroy and state cleanup intact, and emits a workflow annotation: force-cleanup is the only cleanup CI runs and it wipes Terraform state, so a silent skip strands the uploaded ISO with nothing left to reclaim it. Drop docker-ce-cli and the /var/run/docker.sock mount. Nothing in the container has called docker since #87 moved storage into a VM; the remaining docker calls run inside that VM over SSH. The CI job image is built from the same target, so this also removes a third-party apt repository from its supply chain. Rewrite tests/.env.test.example against what the code now requires, and fix the documented commands in CLAUDE.md, README.md, copilot-instructions.md and the integration README.
165 lines
7.4 KiB
Markdown
165 lines
7.4 KiB
Markdown
# PSProxmoxVE — Claude Code Instructions
|
||
|
||
## Project Overview
|
||
|
||
C# binary PowerShell module for managing Proxmox VE (PVE) infrastructure. Two projects:
|
||
- `src/PSProxmoxVE/` — Cmdlets and module surface (targets netstandard2.0)
|
||
- `src/PSProxmoxVE.Core/` — Services, models, HTTP client (targets netstandard2.0)
|
||
|
||
Tests: xUnit (`tests/PSProxmoxVE.Core.Tests/`) and Pester 5 (`tests/PSProxmoxVE.Tests/`).
|
||
|
||
## Development Workflow
|
||
|
||
**All changes go through pull requests.** The `main` branch has branch protection enabled
|
||
(required build checks, required review, admin enforced). Never push directly to main.
|
||
|
||
```bash
|
||
# Create a feature branch
|
||
git checkout -b feat/my-feature
|
||
|
||
# ... make changes ...
|
||
|
||
# Commit using conventional commits
|
||
git commit -m "feat: add new cmdlet"
|
||
|
||
# Push and create PR
|
||
git push -u origin feat/my-feature
|
||
gh pr create
|
||
```
|
||
|
||
### Review before pushing, not after
|
||
|
||
**Write the code, then review it, then commit and push.** Not the other way round.
|
||
Before `git add`/`git commit` on any non-trivial change, spawn reviews of the working
|
||
tree and act on what they find:
|
||
|
||
- **Codex** (`codex:codex-rescue`) — a second opinion from a different model.
|
||
- **Subagent reviewers** — pick for the change: `correctness-reviewer`,
|
||
`security-reviewer`, `test-reviewer`, `architecture-reviewer`, `api-compat-reviewer`,
|
||
`performance-reviewer`, `ai-smell-reviewer`.
|
||
|
||
Run them in parallel in one message; they are read-only and independent.
|
||
|
||
The point is to cut review churn. A defect found before the push costs one edit; the same
|
||
defect found by the PR reviewer costs a review cycle, a force-push and a re-review, and on
|
||
CI-infrastructure changes a runner iteration is roughly 45 minutes. Reviewers are not
|
||
infallible — verify a finding against the code before acting on it, and say so when you
|
||
judge one wrong rather than silently ignoring it.
|
||
|
||
When a reviewer criticises a test, mutation-test it: break the behaviour the test claims to
|
||
cover and confirm the test fails. A suite that passes against a deliberately broken
|
||
implementation is not evidence of anything.
|
||
|
||
Trivial edits skip this: a typo, a version bump, a one-line doc change.
|
||
|
||
### Agent pushes and commit identity
|
||
|
||
**Default: the `github` MCP tools.** Agent-authored branches go up with `create_branch` +
|
||
`push_files`, which commits as the `goodolclint-claude` App and produces a **verified**
|
||
commit. `push_files` re-uploads full file contents, so byte-verify before opening the PR:
|
||
commit the identical change locally, `git fetch`, and `git diff <local-commit>
|
||
origin/<branch> --` must be empty. `push_files` cannot express a removal — use
|
||
`delete_file` for deletions, and a rename is `push_files` of the new path plus
|
||
`delete_file` of the old.
|
||
|
||
**Fallback: local `git push`, for large pushes only.** Re-uploading full contents inline
|
||
is impractical past a certain size. `.claude/settings.json` sets `GIT_AUTHOR_*` /
|
||
`GIT_COMMITTER_*` to `goodolclint-claude[bot]` so those commits are still attributed to the
|
||
App — but they are **not verified**, because the signature comes from committing through
|
||
the API, not from the author name. Use this path when needed, not by default.
|
||
|
||
Claude Code picks the env block up immediately — the session that adds it already commits
|
||
as the bot, no restart needed. A `Co-Authored-By` trailer is redundant once it is in
|
||
effect, since the App is the commit author.
|
||
|
||
### Local dev environment
|
||
|
||
`tests/infrastructure/scripts/run-integration.sh` is the single source of truth for the
|
||
provision → test → cleanup lifecycle. CI calls it directly, and so should you — there is
|
||
no wrapper script.
|
||
|
||
Build and unit tests run natively, no container needed:
|
||
|
||
```bash
|
||
dotnet build PSProxmoxVE.sln
|
||
dotnet test tests/PSProxmoxVE.Core.Tests/
|
||
pwsh -Command "Invoke-Pester tests/PSProxmoxVE.Tests/ -ExcludeTagFilter Integration -Output Detailed"
|
||
```
|
||
|
||
An installed `PSProxmoxVE` in `~/.local/share/powershell/Modules/` shadows the local build,
|
||
because `_TestHelper.ps1` tries `Import-Module PSProxmoxVE` by name first. Force the local
|
||
build with `Import-Module ./src/PSProxmoxVE/bin/Debug/netstandard2.0/PSProxmoxVE.psd1 -Force`,
|
||
or delete the installed copy. (`dotnet build` writes there; only `dotnet publish -o
|
||
./publish/netstandard2.0`, which CI runs, creates `publish/`.)
|
||
|
||
The integration flow needs the `dev-infra` container — the same image CI runs its jobs in
|
||
(`tests/Dockerfile.test`, target `dev-infra`). x86 only:
|
||
|
||
```bash
|
||
pve() {
|
||
docker compose -f tests/docker-compose.test.yml --profile infra run --rm dev-infra \
|
||
bash tests/infrastructure/scripts/run-integration.sh "$@"
|
||
}
|
||
|
||
pve provision 9
|
||
pve test 9 Cluster,VMs # the area filter is optional
|
||
pve force-cleanup
|
||
```
|
||
|
||
Copy `tests/.env.test.example` to `tests/.env.test` first — it lists every required
|
||
variable, including the Terraform storage pools CI supplies from repository variables.
|
||
|
||
## Key Conventions
|
||
|
||
- All cmdlets use `Pve` noun prefix
|
||
- All cmdlet classes must be `sealed`
|
||
- All cmdlets must have `[OutputType]` attribute
|
||
- Destructive cmdlets must set `ConfirmImpact = ConfirmImpact.High`
|
||
- VmId parameters: `[ValidateRange(100, 999999999)]`, nullable when optional
|
||
- JSON: Newtonsoft.Json only (`[JsonProperty]`), no System.Text.Json attributes
|
||
- Task polling: always use `TaskService.WaitForTask`, never inline loops
|
||
- Passwords: `SecureString` type, never plain `string`
|
||
- URL paths: `Uri.EscapeDataString()` on all dynamic path segments
|
||
- No bare `catch {}` blocks — use specific or filtered exceptions
|
||
- Verb class constants required (`VerbsCommon.Get`, not `"Get"`)
|
||
|
||
## Review System
|
||
|
||
This repo uses a structured review system to track findings and prevent regressions.
|
||
|
||
### Key files
|
||
- `docs/review/findings.json` — stable findings database. IDs are permanent (F001, F002...).
|
||
Never renumber. Read this before any coding session to understand open issues.
|
||
- `docs/review/REVIEW_REPORT.md` — latest full review report (scan-9, 2026-03-26, F001–F085)
|
||
- `DECISIONS.md` — architectural decisions and anti-patterns. **Read this before writing
|
||
any new code.** It documents patterns that were deliberately chosen or changed and must
|
||
not be reintroduced.
|
||
|
||
### Before starting a coding session
|
||
1. Read `DECISIONS.md` to understand established patterns
|
||
2. Check `docs/review/findings.json` for open findings relevant to the area you're working in
|
||
3. Do not introduce patterns listed as anti-patterns in DECISIONS.md
|
||
|
||
### Finding ID stability
|
||
Finding IDs (F001, F002...) are permanent. A resolved finding is never deleted from
|
||
findings.json — it is marked `resolved` with evidence of the fix. If a finding reappears,
|
||
it is marked `regressed` and retains its original ID.
|
||
|
||
## Releasing to PSGallery
|
||
|
||
Tag-driven: pushing a `v*` tag to `main` triggers `.github/workflows/publish.yml` (build →
|
||
PS 5.1 smoke test → publish to PSGallery → create GitHub Release with auto-generated notes).
|
||
|
||
Each release PR must update **three** things in lockstep before the tag is cut:
|
||
|
||
1. `ModuleVersion` in `src/PSProxmoxVE/PSProxmoxVE.psd1` (semver patch for bug-fix-only;
|
||
minor for new features; major for breaking changes).
|
||
2. `ReleaseNotes` in the same psd1 — this is what PSGallery surfaces on the version page.
|
||
Replace the previous version's notes; do not append.
|
||
3. `CHANGELOG.md` — cut the `[Unreleased]` section into a new `[X.Y.Z] - YYYY-MM-DD`
|
||
block and reset `[Unreleased]` to empty.
|
||
|
||
After merge, tag `main` with `vX.Y.Z` and push the tag. The publish workflow rewrites
|
||
the psd1 `ModuleVersion` in the build artifact from the tag, so the tag and the source
|
||
version must match.
|