Add project skills for build, duckdb, maintenance, and release - #2853
Conversation
Move four skills out of the personal ~/.claude/skills directory into .claude/skills so they travel with the repo to other machines and to cloud sessions, which do not read the local personal skills directory. .gitignore now un-ignores .claude/skills only. settings.local.json, commands/, and worktrees/ stay ignored. Machine-specific content was stripped rather than committed: - release-checklist no longer contains any credentials. Cloud test instances take a per-run throwaway password via $SQL_TEST_PASSWORD. The local SQL instance inventory, and the rule that sql2022 and sql2025 hold real data, moved to the untracked local CLAUDE.md. - Region guidance is now "close to the machine running the collector" rather than hardcoded to US East for one developer's timezone. - Absolute C:\Users and C:\GitHub paths are gone. duckdb resolves the store through %LOCALAPPDATA%, build uses repo-relative paths. - maintenance was named skill.md; renamed to SKILL.md so it resolves on case-sensitive filesystems. build was also stale. It pointed at Dashboard/Dashboard.csproj, which has been bin/obj leftovers since the v3.3.0 move to deprecated/. The argument table is rebuilt from git ls-files with correct paths and target frameworks, and now covers the Darling service and Viewer. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01EbYty5EYQwd4YGTDcJfEkE
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| 4. Firewall churn: DELETE the allow rule, wait 3–5 minutes, re-add it. Collection must resume on its own without restarting the service. (CAVEAT, verified v3.2.0: a bare rule-deletion often does NOT sever an actively-collecting client — connection pooling keeps the open connection alive and Azure gates only NEW connections — so treat a no-outage result as inconclusive rather than a pass; the recovery logic's authoritative validation is the unit suites.) | ||
| 5. Clean up: `az group delete --name rg-release-test --yes --no-wait` | ||
|
|
||
| **AWS RDS:** |
There was a problem hiding this comment.
Portability drift: the PR description says region guidance was generalized to "close to the machine running the collector" instead of being hardcoded for one developer's timezone, and the Azure section above (line ~78) was updated that way ("Use a region geographically close to the machine running the collector ... From US East that is eastus or eastus2"). This AWS RDS section wasn't updated the same way — it still hardcodes us-east-1 with reasoning specific to a US-East-based operator ("cross-country latency to us-west-2"). Someone running this checklist from a different region will get steered to the wrong AWS region, same class of bug the PR set out to fix for Azure.
ReviewThis PR is docs/config-only (four Verified clean:
One finding posted inline: the AWS RDS section of |
The Azure block was changed to "close to the machine running the collector" but the AWS RDS block kept "Use us-east-1, cross-country latency to us-west-2", which is the same single-developer-timezone assumption this branch set out to remove. Both halves now read the same way and keep their concrete US East example region. Also notes that the AWS block uses the same $SQL_TEST_PASSWORD throwaway as the Azure block, since the two can be run independently. Caught in review on #2853. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01EbYty5EYQwd4YGTDcJfEkE
|
Good catch on the AWS half. Fixed in the follow-up commit: the RDS block now reads "close to the machine running the collector" exactly like the Azure block above it, keeping |
|
|
||
| ## A. Dependencies & Security | ||
|
|
||
| > **Scope — scan every project, not just the solution.** A repo's `.sln` may not list every project. PerformanceStudio's `PlanViewer.sln` omits `server/PlanShare.csproj` and the SSMS VSIX (`PlanViewer.Ssms`, `PlanViewer.Ssms.Installer`), so the `dotnet list <Solution>.sln …` scans and the solution build all silently skip them. Run the checks against those projects too. The net472 VSIX is old-style, so `dotnet list` is unreliable on it — read its `<PackageReference>`s by hand (its `Microsoft.VSSDK.BuildTools` is intentionally held on the 17.x line; 18.x is un-restorable from nuget.org and targets VS 18, not VS 2022). Where the `.sln` covers all projects, the solution is enough. |
There was a problem hiding this comment.
This "Scope" callout (and several others below — line 24's .github/workflows/release.yml for PerformanceStudio, line 26's lock-file comparison, the cross-platform publish step) hardcodes facts about PerformanceStudio, a separate/sibling repo that doesn't exist anywhere in this codebase. PlanViewer.sln, server/PlanShare.csproj, and the PlanViewer.Ssms* VSIX projects aren't in this repo — I checked git ls-files "*.csproj" and there's no PlanViewer.sln at all (this repo's solution is PerformanceMonitor.sln, and "PlanViewer" here only refers to the plan-viewer feature inside Lite/Darling/deprecated Dashboard, e.g. Lite/Windows/PlanViewerWindow.xaml).
This looks like a personal skill written to cover two of the author's repos at once, moved into .claude/skills here as-is. The PR's stated goal is removing single-machine/single-developer assumptions so skills work for anyone who clones this repo — but a skill that spends a third of its "Dependencies & Security" section on another private repo's solution layout is exactly that kind of leftover context, just not the flavor (paths/credentials) the cleanup targeted. Worth stripping the PerformanceStudio-specific detail down to a one-line pointer ("apply the same process to sibling repos you maintain") rather than embedding another codebase's file layout into this one.
| 2. **Vulnerable packages (security).** `dotnet list <Solution>.sln package --vulnerable --include-transitive` | ||
| - Must be **zero**. Any hit (incl. transitive) is urgent — bump or pin to a fixed version. This catches CVEs the `--outdated` check does not. | ||
| - **Code-level pass, not just packages:** run the `security-review` skill/agent on the diff since the last maintenance pass. These apps have real attack surface beyond their dependencies — PerformanceStudio's MCP server opens a local network listener, both store DB credentials (Windows Credential Manager), and both parse untrusted input (e.g. execution-plan XML). Triage anything it flags. | ||
| - **Calibrate severity to the deployment.** PerformanceStudio runs on a single-user personal laptop, and its MCP tools are strictly read-only (no arbitrary SQL, no writes/config changes). So loopback-bound / opt-in / local-IPC findings — the MCP listener, the named-pipe single-instance server — are **Low/informational here, not High**: there's no other local user or attacker to exploit them, and read-only tools can at most leak data they already return. Reserve High for *remotely reachable* vectors (e.g. a missing `Host`/`Origin` check that allows DNS rebinding) or credential disclosure. This calibration would change only if Studio shipped the MCP server enabled-by-default or ran on a shared/multi-user host. Don't re-raise the same local-IPC findings at High each pass. |
There was a problem hiding this comment.
This severity calibration is written for PerformanceStudio's threat model only ("single-user personal laptop," "MCP tools are strictly read-only"), but the paragraph above it (line 30) frames the finding-triage instruction as applying to both apps ("These apps have real attack surface... both store DB credentials... both parse untrusted input"). That's a real mismatch for this repo:
- Darling's MCP surface is explicitly not read-only-only — README.md calls out "write-capable tools (Darling's view authoring, alert tuning, and server onboarding) change the monitor's own configuration."
- Darling is explicitly not a single-user personal-laptop deployment — README.md describes it as "the always-on edition for teams," a headless service with an opt-in network web dashboard (
--enable-web, port 5153) and MCP endpoint (--enable-mcp, port 5152) serving a fleet of monitored servers, viewable from "any seat."
If someone runs /maintenance against this repo and pattern-matches the reasoning here ("no other local user or attacker... reserve High for remotely reachable vectors") onto Darling's --enable-mcp/--enable-web surfaces or its write-capable MCP tools, they'd downgrade findings that don't actually fit the "no other user, read-only" justification. Worth adding a PerformanceMonitor-specific calibration line (or explicitly noting Darling's write-capable tools and multi-seat/team deployment as the higher-severity case) rather than leaving this repo to fall through on PerformanceStudio's reasoning by default.
|
Reviewed. This is a docs/config-only PR (four
Given the PR's own stated goal is stripping single-machine/single-developer assumptions so skills work for any clone, this is worth cleaning up before merge, but it's not a blocker for the mechanical parts of the migration (gitignore change, credential removal, path fixes), which all check out. |
1 participant
What
Moves four skills out of the personal
~/.claude/skillsdirectory into.claude/skillsso they travel with the repo. Personal skills are machine-local, and cloud sessions do not read them at all, so a skill living only in~/.claude/skillsis unavailable to every session outside the one machine it was written on..gitignoregoes from.claude/to.claude/*plus!.claude/skills/, which exposes skills and nothing else.settings.local.json,commands/, andworktrees/stay ignored.release-checklistbuildduckdbmaintenanceCredentials removed
release-checklistcontained the labsapassword and a reused Azure/AWS admin password in plaintext. This repo is public, so both are gone:"$SQL_TEST_PASSWORD", a throwaway generated per run.CLAUDE.md. Those VMs exist on a single machine, so the committed procedure stays machine-neutral.Portability
%LOCALAPPDATA%replaces absoluteC:\Users\...paths.duckdbresolves the store throughos.environ['LOCALAPPDATA'].builduses repo-relative paths instead ofC:\GitHub\PerformanceMonitor\.maintenance/skill.mdrenamed toSKILL.md. It resolved on Windows but would not have on the case-sensitive filesystem a cloud session runs on.Drive-by fix
buildwas stale. It pointed atDashboard/Dashboard.csproj, which has beenbin/objleftovers since v3.3.0 moved Dashboard and Installer todeprecated/, and it did not know Darling exists. The argument table is rebuilt fromgit ls-files "*.csproj"with correct paths and target frameworks, now covering the Darling service and Viewer, with the deprecated projects labeled.Test plan
git check-ignoreconfirms.claude/skillsis tracked whilesettings.local.json,commands/, andworktrees/stay ignorednameparses on everySKILL.md/skillsDocs and config only. No product code changes.
🤖 Generated with Claude Code
https://claude.ai/code/session_01EbYty5EYQwd4YGTDcJfEkE