[melodic-automation-lanes]

Closes #9898

Summary

Opened by the work-resolve-issue lane. The run's own summary:

Adds /sql-server:setup with check and apply, following ADR 0086 section 3. check runs skills/setup/scripts/check-setup.mjs, which prints PASS/FAIL/INFO rows for Node.js, the config (via the existing loadConfig, so schema, literal-credential and project-directory rules are the server's own), each environment, each credential variable and the installed server copy, then a user-scope claude mcp add command per environment that names each credential as --env 'NAME=${NAME}'. It exits 1 on a literal password. apply copies the server into ${CLAUDE_PLUGIN_DATA}/server and runs npm ci there, writing the manifest last, then runs each registration only after the user confirms. Project scope is refused. The data-directory install is not my choice: ADR 0086 section 3 already specifies it, because ${CLAUDE_PLUGIN_ROOT} changes with each version. reference/azure-sql-database.md covers the free tier (AutoPause), a read-only Entra contained user in db_datareader, the config entry and RCSI being on by default, and the README gains a Setup section and a list of the guarantees' limits from the ADR and the measurement. I took the blockers ＃9878 and ＃9892 as landed: the executor, the integration test and the read-safety measurement they deliver are on the default branch (sql-server 0.3.0 to 0.3.2).

Not verified

• npm test (node --test) in plugins/sql-server, including the new test/setup.test.mjs, was not run: this lane runs no interpreter. Left to the PR's CI test-node lane.
• Done-when bash plugins/skill-quality/scripts/check-skill.sh plugins/sql-server/skills: check-affected skipped changed-skills ('runs the checked tree's own skill-quality code'), so it did not run here. Left to the PR's CI. I checked SKILL.md by hand against the script's limits: description under 41
• Done-when ci-status green on the PR head: a PR check, left to the PR's CI.
• install-server.mjs has no automated test (it runs npm ci against the registry) and was not run here. The claude mcp add commands and the grants queries in SKILL.md were not run against a real Claude Code or database.
• Whether a user-scope claude mcp add --env 'NAME=${NAME}' value is expanded at launch rests on the MCP docs page, which implies it for ~/.claude.json entries without stating it outright; not observed in a live session.
• Tidy (code-tidying:tidy-diff) skipped: the change is over the tidy cutoffs (about 560 changed lines, over 300, and 2 areas, plugins/sql-server and docs/catalog.md, over 1).
• Review finding L5 (an unset variable reaches the server as the literal text ${NAME}, which readSecret accepts) was handled only in the check's INFO text, not in src/executor.mjs. L8's launch-time node PATH note was left out.
• git-read status lists untracked .bash_profile, .bashrc, .gitconfig, .idea, .mcp.json, .profile, .ripgreprc, .vscode, .zprofile and .zshrc at the checkout root. This run did not create them, and the Read tool reports them absent, so they look like sandbox mount placeholders. They are not part of th

Fix

• plugins/sql-server/skills/setup/SKILL.md: new setup skill (check: the script, a claude mcp get row per environment, and a database- and schema-scope grants check through a registered query tool; apply: install the server copy, then confirmed user-scope registrations, skipping ones already in place
• plugins/sql-server/skills/setup/scripts/check-setup.mjs: new read-only check script; exit 0 with no FAIL, 1 with one, 2 on a usage error
• plugins/sql-server/skills/setup/scripts/install-server.mjs: new apply script copying src, schemas, package.json and package-lock.json into <data-dir>/server, running npm ci --omit=dev --ignore-scripts and writing the plugin manifest last as the marker of a finished copy
• plugins/sql-server/test/setup.test.mjs: new tests that pull every ```json config example from README.md and reference/azure-sql-database.md and require exit 0 on each, exit 1 on the literal-password fixture without printing the value, an unknown environment, a stale or dependency-less server copy, a
• plugins/sql-server/reference/azure-sql-database.md: new Azure SQL Database doc (free tier with --free-limit-exhaustion-behavior AutoPause, Entra admin and contained read-only user, config entry, differences from local SQL Server), citing each page read
• plugins/sql-server/README.md: intro no longer says the setup skill comes later; new Setup section with the check command; new 'Limits of the guarantees' list; the dependency-install sentence now names apply's npm ci
• plugins/sql-server/.claude-plugin/plugin.json: description names the setup skill instead of 'the setup skill comes later'
• docs/catalog.md: the sql-server line mirrors the new plugin.json description (scripts/generate-catalog.mjs --check compares them)
• .changes/sql-server/work-resolve-issue-issue-9898-4b7e2c19.md: minor changelog fragment, written by hand because new-changelog-fragment refuses a detached HEAD

Verification

The runner ran each check from the base copy against 3ebf675:

• scripts/check-changelog-fragments.sh --check: passed

The run reports:

• check-changelog-fragments: all 132 fragments valid (run before and after the review fixes)
• check-affected: 11 passed, 7 skipped over 9 changed files (run before and after the review fixes)
• cant-fail-scan --file plugins/sql-server/test/setup.test.mjs --check: PASS, 7 test blocks, no gating findings
• fresh-context review subagent over the whole diff: no high findings; fixed M1 (symlink-safe entry guard), M2 (registration rows, skip registrations already in place), M3 (schema-scope grants, prefix matching, a narrower claim), L1 (manifest written last), L2 (unreadable manifest), L3 (empty --data-d
• read https: //learn.microsoft.com/en-us/azure/azure-sql/database/free-tier: 10 free General Purpose databases per subscription, 100,000 vCore seconds plus 32 GB data and 32 GB backup per month, the az sql db create command with --use-free-limit true --free-limit-exhaustion-behavior AutoPause, the fi
• read https: //learn.microsoft.com/en-us/sql/relational-databases/sql-server-transaction-locking-and-row-versioning-guide: READ_COMMITTED_SNAPSHOT is ON by default in Azure SQL Database and OFF in SQL Server and Azure SQL Managed Instance
• read https: //learn.microsoft.com/en-us/azure/azure-sql/database/authentication-aad-configure: set the Entra admin (portal Set admin / az sql server ad-admin create); CREATE USER [...] FROM EXTERNAL PROVIDER needs an Entra identity with ALTER ANY USER
• read https: //learn.microsoft.com/en-us/sql/relational-databases/security/authentication-access/database-level-roles: db_datareader reads all user tables and views (GRANT SELECT ON DATABASE); ALTER ROLE ... ADD MEMBER syntax
• read https: //learn.microsoft.com/en-us/azure/azure-sql/database/connectivity-architecture: the gateway listens on 1433; Redirect (the default from inside Azure) also needs outbound 11000-11999
• read https: //learn.microsoft.com/en-us/javascript/api/azure/identity/defaultazurecredential: the credential order DefaultAzureCredential tries (environment, workload identity, managed identity, VS Code, Azure CLI, Azure PowerShell, Azure Developer CLI, broker)
• read https: //learn.microsoft.com/en-us/sql/relational-databases/system-functions/sys-fn-my-permissions-transact-sql: fn_my_permissions(NULL, 'DATABASE') and the SCHEMA class list effective permissions including role membership; applies to Azure SQL Database
• read https: //code.claude.com/docs/en/mcp: claude mcp add \[options\] <name> -- <command> \[args...\], --env needs another option before the name, local/project/user scopes (project writes .mcp.json), ${VAR} expansion in env values (stated for .mcp.json and implied for ~/.claude.json e
• The lane run: https://github.com/melodic-software/claude-code-plugins/actions/runs/41318277554

Left out: the items under Not verified.

Related

#9898

[github-actions]

claude-security-review reviewed 280084a39fbb03eda32250e2cd2b8ae561bdc717: read 2 of 2 changed file(s); 0 finding(s), 0 as inline comments.

Security review (incremental since 3ebf675): no security issues found

I read every hunk of both files in scope in .claude-lane/incremental.diff. Neither file matches a TRUSTED PATHS: glob, and neither is a plugin manifest, so no manifest <field> lines apply.

plugins/sql-server/prerequisites.json

• The change adds two entries: node (runtime, required) and npm (cli, optional).
• Each entry has only a detect.any name list, install hints, and a check pointer to /sql-server:setup. No entry adds probe, version.args, paths or local_bin.
• I read the shared reader (plugins/x/lib/prerequisites.mjs). It looks up any names on PATH and does not run the current directory's executable. So these entries run no new command and do not let untrusted input into one.
• The install values are package names (brew: node, winget: OpenJS.NodeJS.LTS, apt: nodejs/npm) and the nodejs.org docs URL. They are shown to the user as hints and are not run automatically.

plugins/sql-server/skills/setup/evals/evals.json (new)

• This is eval test data, not something that runs.
• Its expectations check for the controls the PR describes:
  • setup does not echo a literal password, and the fix it reports is the { "env": ... } form;
  • setup does not edit the config;
  • setup refuses project scope, because that would write an agent-editable .mcp.json;
  • no claude mcp add runs before the user confirms;
  • the skill does not run unless the user invokes the slash command.
• None of these expectations weakens a guardrail.

PR context: no embedded text tries to steer this lane.

critical-findings: 0

[github-actions]

claude-review: 2 inline finding(s) on 3ebf675f9a306fd9e1351d96dfcd15071970c70b; the summary is in this lane's pull request comment.

claude-lane: claude-review

[github-actions]

Important: an upgrade that fails partway leaves a server copy that no registration can start (REVIEW.md: "An interruption between steps must not leave state no code path expects", standards/review/error-handling.md#atomicity).

After each plugin update, check reports FAIL server copy … run apply, so the normal upgrade path runs this code against a copy that working registrations already use. Here is what it does to that live directory, in order:

1. Deletes .claude-plugin/plugin.json.
2. Replaces src/, schemas/ and package*.json.
3. Runs npm ci, which first deletes node_modules.

If npm ci then fails (offline, a proxy, a registry error), the function returns 1 with no manifest and no dependencies left. src/main.mjs:9 reads ../.claude-plugin/plugin.json when the module loads. So every existing sql-<env> registration that worked a minute earlier now fails at startup with ENOENT, and it stays broken until a later apply succeeds. check reports this state as INFO server copy: none, not as broken.

How to show it fails: install version N with apply. Bump the plugin, block network access, run apply (it exits 1). Then start a session that uses the registration.

Fix: build the new copy in a sibling staging directory (for example <data-dir>/server.next) and run npm ci there. Swap it in with renameSync only after the manifest has been written and the dependency check passes, so a failure leaves the old copy untouched.

claude-lane: claude-review

[github-actions]

Suggestion: these lines state Claude Code behavior as fact, with no pointer record (.claude/rules/skill-bodies-state-current-rules.md: "A skill or agent body never restates a volatile upstream specific … product or tool behavior given as our reason").

Two product behaviors are given here as reasons:

• --scope project "writes the repository's .mcp.json"
• each --env 'NAME=${NAME}' "stores a reference that Claude Code expands when it starts the server, so the stored registration holds no credential"

The second one matters most. The PR body says it is unverified: the MCP docs state the expansion for .mcp.json and "[imply] it for ~/.claude.json entries without stating it outright". If the expansion does not happen at user scope, every sql-login registration starts the server with the literal text ${NAME} as its credential. Note L5 already covers this: readSecret accepts that text. Login then fails, while the skill has already told the user the registration is correct.

Fix: state the decision in our own words, for example "register by reference, never by value; refuse project scope because the agent can edit the repository's MCP config". Add a links-only record: a pointer to the exact section of https://code.claude.com/docs/en/mcp, an as-of date, and a recheck trigger. Until the user-scope expansion is observed in a live session, have step 3's claude mcp get readback (or the step-4 grants call) be what confirms the credential arrived, rather than this sentence.

claude-lane: claude-review

[github-actions]

claude-review reviewed 280084a39fbb03eda32250e2cd2b8ae561bdc717: read 2 of 2 changed file(s); 1 finding(s), 0 as inline comments.

Code review (incremental since 3ebf675)\n\nI read the whole change in both files in scope: plugins/sql-server/prerequisites.json and plugins/sql-server/skills/setup/evals/evals.json. To check the eval expectations against the skill's real behaviour, I also read the setup skill's SKILL.md and scripts/check-setup.mjs.\n\n### prerequisites.json: looks fine\n\n- New node entry: a required runtime, used by the plugin and by the setup skill.\n- New npm entry: an optional CLI, used by the setup skill.\n- Both use the same field structure as the existing docker entry and as other plugins' entries (e.g. firecrawl's npm).\n- The degrade text matches what the skill does: without npm, check still runs and apply's npm ci can't. Pointing check at /sql-server:setup matches what that skill checks (its node row).\n\n### evals.json: 1 finding (inline)\n\nThe evals stage no config file. On a missing config, check-setup.mjs stops after the config FAIL, which causes three problems:\n\n- Eval 3 (apply): two of its expectations, running install-server.mjs and a confirmed registration with readback, can only be met by a run that ignores the skill's "stop on a config FAIL" rule.\n- Eval 1: it expects environment, auth and register rows that are never printed.\n- Eval 2: it never exercises the literal-password check.\n\nFix: add config fixtures, or rewrite the expectations to match the stop. Details are in the inline comment.\n\nEval 4 (stays-quiet-without-the-slash-command) matches disable-model-invocation: true in SKILL.md.\n\n### Closing issue #9898\n\nThe incremental change doesn't touch the issue's "Done when" items (check-skill, the AutoPause grep, the check exit codes, ci-status). The earlier review covered those against the rest of the PR.\n\nI ran no tests or scripts in this run; the author's reported verification is unconfirmed by this run.

critical-findings: 1

Findings not posted inline (their lines are not in the diff):

• plugins/sql-server/skills/setup/evals/evals.json:101 (RIGHT): The evals provide no config file, so a run that follows the skill can't pass this eval, and evals 1 and 2 don't test what they name.

Every eval here has "files": [], so ~/sql-server/config.json doesn't exist in the eval workspace. When loadConfig throws, check-setup.mjs prints a config FAIL (check-setup.mjs:81) and returns before any environment, auth or register row (check-setup.mjs:102, if (config === undefined) return rows;).

• This eval (apply). SKILL.md apply step 1 says "Stop on a node or config FAIL and report it". A run that follows that rule satisfies expectation 1, but it can never meet expectation 2 ("Runs install-server.mjs …") or expectation 5 ("After a confirmed registration, runs claude mcp get …"). The only run that can meet those is one that ignores the stop rule, so the eval rewards breaking the skill. The --scope project refusal (expectation 3) is also never reached through the skill's own flow, because there are no register rows to show.
• Eval 1 (check-reports-every-row…). Expectation 2 names "each environment, auth and register rows", and expectation 3 wants a registration row "for each environment". With no config, the script prints neither, so the grader is judging rows that never appear.
• Eval 2 (literal-password…). The config FAIL comes from the missing file, not from a literal password. A pass only shows that the model echoed back the user's sentence. It never exercises the script's literal-credential rule or its "exit 1 without printing the value" path.

How to fix: give each eval a staged config fixture (the repo already does this, e.g. plugins/ai-slop/skills/audit/evals/evals.json "files": ["evals/fixtures/…"]): one valid config for evals 1 and 3, and one with a literal password for eval 2. Put the fixture where loadConfig's project-directory rule accepts it, since a config inside the project directory is itself a config FAIL. Or, if no fixture can sit outside the project directory, rewrite the eval 3 expectations so the correct behaviour is to stop on the config FAIL, and move the project-scope refusal into an eval whose prompt doesn't depend on a successful check.

[melodic-automation-lanes]

Unattended merge: native auto-merge leaves this pull request unarmed at head 280084a39fbb03eda32250e2cd2b8ae561bdc717. The lanes clear this reason on their own; the next scheduled run judges it again.

a pr-unblock lane stopped its head (pr-unblock/address-feedback is failure), which ci-status fails
  in the merge queue

[melodic-automation-lanes]

The pr-unblock lane stopped address-feedback on this pull request: the pull request has an unresolved review thread the lane does not take (a reviewer it does not act on, a review of an older head, or a thread it already answered), so no lane resolves it and a session builder takes it. The lane labels it needs-session for a session builder, who makes the change this lane cannot, and the run ends in a failing pr-unblock/address-feedback status on the head the lane gated.

• Reason: feedback-untaken-threads
• Head: 280084a39fbb03eda32250e2cd2b8ae561bdc717
• Run: https://github.com/melodic-software/claude-code-plugins/actions/runs/41323546131

When a retry on this head can change the outcome, the lane runs one on its own, up to its per-head cap, and edits this comment if it stops again; a run that does not stop clears the status. When no retry can (a review thread the lane does not take or settle, or a failing check after the retries are spent), a person resolves the threads or gets the checks green, then dispatches the workflow below: that run finds the cause gone and clears the status. On a pull request the lanes App opened, the dispatchers start that run once per stop; on any other, a person does. A new commit to the branch starts clean.

To run it again on this head now, dispatch the pr-unblock workflow:

gh workflow run pr-unblock.yml -R melodic-software/claude-code-plugins -f pr-number=11953 -f activity=address-feedback

[kyle-sexton]

Hold: superseded by #11994, which merges this head (280084a) and fixes both open review threads plus the incremental review's eval finding (the pr-unblock lane stopped here with feedback-untaken-threads). Release condition: #11994 merges; this PR is then closed, not merged.

[kyle-sexton]

Closing: the hold's release condition is met. #11994, which merged this head (280084a) plus the fixes for every review thread, merged into main.