fix(mcp): judge run_command on the command and flags cobra resolves

[ktn-jamf]

Why

jamf-cli mcp serve lets a connected model run CLI commands through run_command. The operator pins the profile, instance, credentials and local destinations. The model is only meant to choose which command to run. Five findings showed that the model could get past that boundary:

• Deny-list bypass (F7). The deny-list compared raw argv tokens. The cfg alias, a leading flag (--no-color multi …) or a flag inside the path (pro -q backup) reached multi, the config writers and both backups. Verified end to end: cfg set-report-dir and --no-color config set-default ran.
• Another tenant's data (F2). pro diff --target <other-profile> used that profile's stored credentials and returned its script bodies to the model.
• Local file read (F5). --script-file, --from-file and the other input-path flags read any local file. With -n, the body came back to the model with no login at all.
• Local file write (F6). --save-to, -O and the Protect --output flags wrote to any local path.
• Local file delete (F8). pro jcds sync --dir X --delete deleted every file in X that JCDS did not list, with no prompt.

What changed

• run_command resolves the argv with cobra's Find, on the tree mcp serve is already running and under one lock. It collects every flag occurrence with pflag ParseAll. Refusals are judged on the resolved command path and the resolved flag name, not on the raw tokens.
• Refused commands. multi, mcp, completion, the config writers, config validate, doctor, every setup, both backups, and both mounts of jcds sync. Also refused are the commands that print a live access token: auth token under platform, pro and protect, and pro api-authentication token, oauth-token and keep-alive.
• Refused flags.
  • The existing credential and target prefixes.
  • Every write-side path flag: --save-to, a command's own --output, --report-dir, and --dir where it writes.
  • --password-file, always.
• Read-side path flags (--from-file, --file, --script-file, --input, and the others) are refused unless the operator starts mcp serve --input-dir <dir>. With --input-dir set, a path is allowed only if it resolves inside that directory, after EvalSymlinks. The directory-valued readers (pro diff, the Protect imports and protect restore) apply the same check to each file they open.
• pro diff. Each side must be the pinned profile, or a directory inside --input-dir.
• config show. In an MCP child it prints <redacted> for the token, client ID and client secret. config list --status probes only the pinned profile.
• The child checks again. When JAMF_CLI_MCP=1, the child runs the same refusal in root PersistentPreRunE, before any auth-skip return. It compares against JAMF_CLI_MCP_PROFILE and JAMF_CLI_MCP_INPUT_DIR. childEnv strips both from the inherited environment.
• protect plans config-profile (outside MCP too). It now refuses a plan name that contains / or \, or is empty, . or ... Such a name would place the file outside the working directory, and the error tells the user to pass -O. Ordinary names are unchanged.

Breaking for MCP users

• File-input flags now fail over MCP unless the operator sets --input-dir. This includes creating a script from a file.
• pro diff with a directory side now needs --input-dir.
• doctor, config validate, completion and the token-printing commands are no longer available through run_command.

CHANGELOG.md records all of this under ## Unreleased, with --input-dir as the migration.

Verification

go test ./internal/commands/ -count=1
go test -race ./internal/commands/ -run 'MCP|BuildChildArgs|RunChild|Refuse|Diff|InputDir|Resolve|ConfigList|CollectProtect' -count=1

The first commit adds the repro tests alone, one or more per finding. On that commit they fail, with 8 failing tests. The later commits make them pass. No test that exists on main was changed.

Review ran in two rounds. The lanes were security (twice), silent failures and test quality, with mutation checks on each fix. The security lane reproduced its findings through a real mcp serve with fake credentials.

Residual gap. The child opens a path again after the check passes. So a symlink swapped in between the check and the open is not caught.

🤖 Generated with Claude Code

[neilmartin83]

Warning

⚠️ Needs changes. Moves the run_command checks onto the command and flags cobra resolves, adds --input-dir, and now refuses token printers and redacts Protect secrets. Both round 1 findings are fixed.
Blocking: (1), (2). See the collapsed section for 6 nice-to-have suggestions.

Rating: 3/5

• Would be a 5 with the remaining credential-printing commands refused (1) and Classic secret fields redacted or refused in the MCP child (2).

Findings

⚠️ IMPORTANT (1) (security, pre-existing, via security-reviewer+devil-advocate) — internal/commands/mcp.go:552: three generated commands still print a live credential

The latest commit refuses pro api-integrations client-credentials because it prints a credential that works outside the server. The same rule applies to three commands that are not on the list:

• pro jamf-cloud-distribution-service renew-credentials (POST /v1/jcds/renew-credentials)
• pro jamf-cloud-distribution-service-files create (POST /v1/jcds/files)
• pro sso-oauth-session-tokens list (GET /v1/oauth2/session-tokens)

The first two return the Credentials schema (specs/JamfProAPI.yaml:6745), which holds accessKeyID, secretAccessKey and sessionToken for the tenant's JCDS S3 bucket. The third returns accessToken and idToken. Nothing sweeps the tree for this class. TestMCPRefusedCommands_EveryEntryNamesACommandInTheTree catches a stale entry and not a missing one, so the next make generate adds new holes without a failing test.

Failure scenario: run_command ["pro","jamf-cloud-distribution-service","renew-credentials"] resolves to a path not in mcpRefusedCommands. The child prints AWS STS keys into the model's context, and they keep working against the JCDS bucket after mcp serve exits.

Suggested fix:

 	{"jamf-cli pro api-integrations client-credentials", refusedMintsClientSecret},
+	{"jamf-cli pro jamf-cloud-distribution-service renew-credentials", refusedMintsUploadCredentials},
+	{"jamf-cli pro jamf-cloud-distribution-service-files create", refusedMintsUploadCredentials},
+	{"jamf-cli pro sso-oauth-session-tokens", refusedPrintsToken},

Add a tree-walk test that fails on any leaf whose name, Short or Long matches token|secret|credential|password|private.?key. A matching leaf must be refused or sit in an exemptions map with a reason, the same shape as notALocalPathFlags.

Fixed when: the three paths refuse over MCP with a reason that names the credential. A new generated leaf matching that pattern fails go test until someone classifies it.

⚠️ IMPORTANT (2) (security, pre-existing, via security-reviewer) — internal/commands/mcp.go:536: Classic get prints secret fields to the model unredacted

The PR redacts three Protect third-party secrets in the MCP child. The Classic resources get no such treatment. specs/classic/schemas.json declares string secrets in read schemas: vpp_account.service_token (the Apple sToken), smtp_server.password, ldap_server.connection.account.password, webhook.password and directory_binding.password. internal/client/client.go:520 already records that a Classic read of a distribution point or SMTP server returns its password field. The Classic credential policy covers --set writes only, and no read path masks these fields.

Failure scenario: run_command ["pro","classic-vpp-accounts","get","1","-o","json"] is not refused. The child prints service_token, and the model can then use the VPP sToken against Apple outside the server.

Suggested fix: in an MCP child, pass every Classic get and list response through the existing credentialFieldNames / credentialFieldPaths matcher at the one shared Classic print path, and print <redacted> for each match. Do not redact resource by resource.

Fixed when: a table-driven test walks every string-typed secret field in specs/classic/schemas.json, reusing the sweep behind TestEverySecretBearingFieldIsRefusedForSet. It shows that an MCP-child get prints the redaction marker for each field.

This covers all findings — addressing the above gets this PR to merge-ready.

Nice-to-have suggestions (6 items)

💡 NICE-TO-HAVE (3) (usability, via usability-reviewer) — internal/commands/protect_plans.go:234: the plan-name refusal tells the model to pass -O, and MCP refuses -O. Over MCP, drop the remedy or say the plan cannot be saved through run_command.

💡 NICE-TO-HAVE (4) (security, via security-reviewer+silent-failure-hunter) — internal/commands/protect_action_configs.go:207: redactReportClientHeaders masks header values but leaves Params.URL. A Slack, Teams or HEC webhook often carries its secret in the URL path or query. Mask the userinfo and query, or say in the help that the URL is shown.

💡 NICE-TO-HAVE (5) (security, via security-reviewer) — internal/commands/mcp.go:536: device secrets are still readable. These include the LAPS password (pro local-admin-password password), the FileVault personal recovery key, the recovery lock password and the mobile device unlockToken. Refuse them or allow them, and record which in the mcpRefusedCommands comment.

Promote to IMPORTANT when: the threat model includes prompt injection that steers the model into device-credential exfiltration.

💡 NICE-TO-HAVE (6) (security, via security-reviewer) — internal/commands/protect_org.go:403: protect downloads websocket-auth and csr write a .p12 into the server's working directory with no path flag, so the model can trigger a key-material write and overwrite a same-named file. Refuse them in the MCP child.

💡 NICE-TO-HAVE (7) (silent-failure, via silent-failure-hunter) — internal/commands/mcp.go:283: mcp serve --input-dir "$DIR" with DIR unset starts as if no input dir were given. The failure is closed, but the operator gets no signal. Error on an empty value when the flag is Changed.

💡 NICE-TO-HAVE (8) (usability, via usability-reviewer) — internal/commands/mcp.go:799: the --output refusal does not say that -o <format> is still fine. A relative read path resolves against the server's cwd, not --input-dir, so the "outside the input directory" refusal should tell the model to pass an absolute path.

PR decomposition

Keep as one PR. All 8 commits harden the run_command boundary in mcp.go, and each layer depends on the one before it: the resolver, then --input-dir, then the pro diff sides, then the token refusals and the secret redaction. Optional carve-out: the protect plans config-profile plan-name refusal (about 40 lines) changes non-MCP behaviour and could merge first on its own. The CHANGELOG already gives it its own entry, so keeping it here is acceptable.

Review coverage and scope

• Correctness: resolver, child re-check, pro diff sides, input-dir containment, Protect redaction helpers
• Security: credential-printing commands (1), Classic reads (2), Protect redaction paths across every -o format
• Test coverage: both suites pass; 6/6 guard mutations killed
• Silent failures: no fail-open; Find and ParseAll errors fall to the child's own parse
• Documentation: CHANGELOG, agent_context.md, mcp serve --help and the tool description match mcpRefusedCommands
• Project rules: credentials-and-auth.md, coding-style.md, classic-api.md
• [na] Performance, frontend, dependencies, migrations

Diff: 20 files, +2206/−131, head 41f1f70, incremental +353/−11 since 7111d7e. High risk (security boundary, size). Lanes run: security-reviewer, silent-failure-hunter, test-quality-reviewer, devil-advocate, usability-reviewer, scope-reviewer. Never run: performance-reviewer (inapplicable: no query, cache or hot-path change), fidelity-reviewer (inapplicable: no linked spec). Conventions: root CLAUDE.md and 3 .claude/rules/*.md, from the session load at an unchanged revision; the PR changes none of them.

Prior review status

#	Location	Rounds	State	Notes
(1)	internal/commands/mcp.go:518	1	✅ Fixed	auth token ×3 and api-authentication token/oauth-token/keep-alive refused in f77cc32
(2)	CHANGELOG.md:12	1	✅ Fixed	## Unreleased carries the MCP breaking entry with --input-dir as the migration, plus the plan-name line

What's done well

✅ The child repeats the refusal in root PersistentPreRunE from its own env, so a server-side miss is still caught. Mutation C showed that seven child-spawning tests depend on it.

_{🤖 Generated by the pr-review:review skill v1.39.0 · reviewed head 41f1f70}

[ktn-jamf]

Thanks. Round 2 is addressed in 9ca8c830..9f23d678.

(1) Credential-printing commands. pro sso-oauth-session-tokens is now refused. A new guard, TestMCPSecretNamingLeaves_AreClassified, walks the tree. It fails on any leaf whose name or help names a token, secret, credential, password or private key, unless that leaf is refused or carries an exemption with a reason. It also fails on a stale exemption. Some of these commands stay available on purpose, and each one is an exemption entry with its reason:

• pro jamf-cloud-distribution-service renew-credentials and pro jamf-cloud-distribution-service-files create stay allowed. That is the maintainer's decision: they return upload credentials for the pinned tenant's own JCDS bucket.
• Pinned-tenant device secrets stay readable on purpose. These are the LAPS password / password-by-guid, view-recovery-lock-password and the FileVault readers. The mcpRefusedCommands comment records that policy.

Two decisions the maintainer made while going through the classification:

• pro cloud-distribution-point list runs, and the CloudFront privateKey and the CDN password print as <redacted> in every output format. create and patch stay refused.
• pro accounts create / update / apply and pro jamf-pro-user-account-settings change-password are refused. They would let the model choose a working Jamf Pro login password.

(2) Classic reads. Every generated Classic get and list passes through one shared redaction helper in an MCP child, using the Classic credential matcher. pro diff masks the same fields. A test walks every string secret field in specs/classic/schemas.json. Outside MCP, the output is byte-identical.

(3)–(8).

• (3) The plan-name refusal now says, over MCP, that the plan cannot be saved through run_command.
• (4) Report-client URLs have their userinfo and query masked. The help notes that a secret in the URL path, as with Slack webhooks, still shows.
• (5) The device-secret policy is recorded, as described above.
• (6) protect downloads csr and websocket-auth are refused.
• (7) An empty --input-dir is an error.
• (8) The --output refusal says that -o <format> still works. A relative path outside the input directory is told to use an absolute path.

[neilmartin83]

Warning

⚠️ Needs changes. All 8 round-2 findings are fixed. Wire check on pro-nmartin: the new Classic redaction prints <redacted> in every output format. One new gap is open: Wi-Fi, VPN and certificate secrets inside a configuration profile's <payloads> still reach the model, and neither the help nor the policy comment says they are shown.
Blocking: (1). See the collapsed section for 1 nice-to-have suggestion.

Rating: 3/5

• Would be a 5 once profile-payload secrets are either redacted, or recorded as shown in the policy comment, mcp serve --help and the tool description (1).

Findings

⚠️ IMPORTANT (1) (security, pre-existing) — generator/classic/generator.go:1709: configuration-profile payload secrets print to the model, and nothing says they do

redactClassicReadInMCPChild redacts only elements named after a schema credential field. A profile's secrets are not elements. They sit inside the escaped plist in <payloads>, so the element walk never sees them. I checked this on the wire: I created a prreview-402-wifi macOS profile on pro-nmartin with a Wi-Fi payload whose Password was a sentinel, then deleted it. An MCP child (JAMF_CLI_MCP=1, pinned profile) printed the sentinel verbatim with -o raw and with -o json.

A Wi-Fi PSK, an 802.1X or VPN password, a VPN shared secret, a SCEP challenge, or a PKCS#12 blob with its password works outside this server. By the rule in the mcpRefusedCommands comment (internal/commands/mcp.go:572), such a secret is refused or redacted. It is not a device secret, so the device-secret exemption does not cover it. The new help text (mcp.go:140) lists what is shown, and payloads are not on that list. An operator who reads "every Classic credential field prints as <redacted>" will assume a Wi-Fi password is covered. The same payloads reach the model through classic-mobile-config-profiles get and through pro diff on the profiles filter. Blueprint configuration components carry them too.

Failure scenario: run_command ["pro","classic-macos-config-profiles","get","<id>","-o","json"] on a tenant with an enterprise Wi-Fi profile. The child prints <key>Password</key><string>…</string> into the model's context.

Suggested fix: pick one of these and state it in the policy comment.

• Record it (the smallest fix). Add profile payloads to the "shown" sentence in mcp serve --help, in the tool description, and in the mcpRefusedCommands comment, beside the LAPS password.
• Redact it. In an MCP child, pass <payloads> through profileconvert's plist decode and replace the string or data value of the known secret keys (Password, SharedSecret, XAuthPassword, Challenge, PayloadCertificatePassword, a com.apple.security.pkcs12 PayloadContent) with the marker.

Fixed when: either the three policy texts name profile payloads as shown, or an MCP-child get of a Wi-Fi-payload fixture prints the redaction marker in place of the PSK in every -o format, and pro diff on profiles prints it too.

This covers all findings — addressing the above gets this PR to merge-ready.

Nice-to-have suggestions (1 item)

💡 NICE-TO-HAVE (2) (test-coverage) — internal/commands/mcp_secret_leaves_test.go:130: TestMCPSecretNamingLeaves_AreClassified decides on help text, not on what the response carries. pro cloud-distribution-point list is the proof. It returns the CloudFront privateKey, it passes the guard, and it is not in notACredentialPrinter, so its help never names the key. The guard would not have caught it. A sweep of specs/JamfProAPI.yaml and the Platform and Security specs finds no other unclassified response that carries a readable secret today, so nothing is open now. The next spec ingest can still add one with no failing test. A second guard that walks 2xx response schemas for a string property matching the same pattern, and not marked writeOnly, covers what the help-text walk misses.

Prior findings status

#	Location	Rounds	State	Notes
(1)	internal/commands/mcp.go:552	2	✅ Fixed	sso-oauth-session-tokens refused. Both JCDS leaves are allowed by the maintainer's decision, and that decision is recorded with its reason. The tree-walk guard now exists and fails on a stale exemption
(2)	internal/commands/mcp.go:536	2	✅ Fixed	Every generated Classic get/list redacts at one shared helper (53 files), and pro diff masks the same leaves. Wire-checked on classic-ldap-servers get 31: <redacted> in json, raw, xml, table and yaml
(3)	internal/commands/protect_plans.go:234	2	✅ Fixed	Over MCP the error says the profile cannot be saved through run_command
(4)	internal/commands/protect_action_configs.go:207	2	✅ Fixed	The userinfo and query are masked, and a malformed URL fails closed. The help says a path secret still shows
(5)	internal/commands/mcp.go:536	2	✅ Fixed	Device-secret policy recorded in the mcpRefusedCommands comment, the help and the tool description
(6)	internal/commands/protect_org.go:403	2	✅ Fixed	protect downloads csr and websocket-auth refused
(7)	internal/commands/mcp.go:283	2	✅ Fixed	An empty --input-dir is an error
(8)	internal/commands/mcp.go:799	2	✅ Fixed	The refusal says -o <format> still works, and a relative path gets the absolute-path hint

Review coverage and scope

• Correctness: Classic span redaction (nested, self-closing and CDATA cases), the CDP client decorator, pro diff masking, the URL redaction, the empty --input-dir check
• Security: Classic reads wire-checked; profile payloads wire-checked (1); response-schema sweep of the Pro, Platform and Security specs (2)
• Test coverage: internal/commands, pro/generated, registry and generator/... pass at 8b7ab94
• Silent failures: Classic redaction fails closed on a non-XML or unparsable body. redactCloudDistributionPoint returns the raw bytes when decode fails, which is acceptable for an endpoint that only answers with an object
• Documentation: CHANGELOG, agent_context.md, mcp serve --help and the tool description match mcpRefusedCommands, except for (1)
• Project rules: root CLAUDE.md and three .claude/rules/*.md, from the session load. The PR changes none of them
• [na] Performance: one XML token pass per Classic read, MCP child only. Fidelity: no linked spec

Grading: this round graded only the author's six commits since 41f1f70 (9ca8c830, 9aef814a, 87f58e01, c5337dfe, 9f23d678, 8b7ab942). Both merges of main resolved only CHANGELOG.md, and both resolutions are clean. git merge-tree against current main, which now includes #404 and #407, reports no conflict. Specialist lanes were not re-dispatched this round. Security and silent-failure on the delta were covered inline with wire probes. Findings were scored by the orchestrator: (1) is wire-verified, and (2) is proven by the passing guard.

_{🤖 Generated by the pr-review:review skill v1.39.0 · reviewed head 8b7ab94}

[neilmartin83]

Warning

⚠️ Needs changes. Both round-3 findings are fixed, and on pro-nmartin the Wi-Fi PSK now prints as <redacted> from get. Two gaps remain. pro blueprints components configuration-profile --id prints the same PSK verbatim. Secrets under token-named or key-named payload keys are not redacted, although the help says "each secret" is.
Blocking: (1), (2).

Rating: 3/5

• Would be a 5 once the blueprint converter's --id/--name download is redacted in an MCP child (1), and the payload key matcher covers what the help text says it covers (2).

Findings

⚠️ IMPORTANT (1) (security, pre-existing) — internal/commands/pro_blueprints.go:912: pro blueprints components configuration-profile --id <id> prints a Classic profile's payload secrets to the model

The new redaction is attached to the generated Classic get/list and to pro diff. A third reader fetches the same <payloads> and gets no redaction. components configuration-profile --id/--name calls downloadClassicProfile → fetchClassicProfile (:1051), converts the plist, and fmt.Printlns the component JSON. The command needs no path flag, so it runs over MCP without --input-dir.

Wire-checked through a real mcp serve -p pro-nmartin over stdio, against a disposable macOS profile (id 7620, since deleted) that held a Wi-Fi Password and a Chrome custom payload:

run_command args	Wi-Fi PSK in the result
pro classic-macos-config-profiles get 7620 -o json	no, <redacted>
pro classic-macos-config-profiles get 7620 -o raw	no, <redacted>
pro blueprints components configuration-profile --id 7620	yes, verbatim

The help text names "blueprint configuration" as shown. That describes the Platform API's blueprint reads. This command reads a Classic profile, which is the resource this PR now says is redacted. --type mobile takes the same path.

Failure scenario: run_command ["pro","blueprints","components","configuration-profile","--id","<enterprise Wi-Fi profile id>"]. The component JSON carries "Password": "<PSK>" into the model's context.

Suggested fix: in an MCP child, redact the downloaded mobileconfig at the print site, before ConvertMobileconfig:

if registry.InMCPChild() && (profileName != "" || profileID != "") {
    if data, err = profileconvert.RedactPayloadSecrets(data); err != nil {
        return fmt.Errorf("redacting the profile's payload secrets, so it is not printed over MCP: %w", err)
    }
}

Do not put this in fetchClassicProfile. import-profile also calls it, and there the plist is written into a new blueprint, so redacting it would store <redacted> as the real Wi-Fi password.

Fixed when: an MCP-child run of components configuration-profile --id on a Wi-Fi-payload fixture prints the marker instead of the PSK, import-profile still copies the real value, and a test holds both.

⚠️ IMPORTANT (2) (security) — internal/profileconvert/payload_secrets.go:22: isSecretPayloadKey misses token-named and key-named secrets, but the help says "each secret" in a payload is redacted

The matcher redacts a key that ends in password or secret, or is Challenge. Custom-settings payloads often carry credentials under other names. The same probe profile carried two of them, and both printed verbatim through get -o json and -o raw:

• CloudManagementEnrollmentToken (Chrome Browser Cloud Management). This token enrolls any browser into the organization.
• TailscaleAuthKey, a pre-auth key.

mcp serve --help (internal/commands/mcp.go:137) says "Each secret inside a configuration profile's payloads … is redacted too". The CHANGELOG and agent_context.md use the same words. The PR's own guard regex, namesASecret (mcp_secret_leaves_test.go:13), counts token and private.?key as secret names, so the two matchers in this PR disagree about what a secret is called.

Failure scenario: a tenant deploys Chrome CBCM through a Jamf custom-settings profile. get <id> -o json over MCP returns the enrollment token.

Suggested fix: pick one of these.

• Widen the matcher. Also match a key that ends in token, authkey, apikey, accesskey or privatekey. A false positive costs one redacted value in an MCP child, and a miss leaks a credential, so err wide.
• Narrow the claim. Change the help, the tool description, the CHANGELOG and agent_context.md to say which keys are redacted ("keys named *Password, *Secret, Challenge, and PKCS#12 content"). Also say that other custom-payload values are shown.

Fixed when: either a payload fixture with CloudManagementEnrollmentToken and AuthKey prints the marker for both, with a profileconvert test, or the four policy texts name the matched keys and say what is not matched.

This covers all findings — addressing the above gets this PR to merge-ready.

Prior findings status

#	Location	Rounds	State	Notes
(1)	generator/classic/generator.go:1709	2	✅ Fixed	get/list on both profile resources and pro diff on profiles redact payload secrets (2b4bd96e). Wire-checked: the Wi-Fi PSK prints as <redacted> in -o json and -o raw. Policy texts updated (2b9f2de6). The gaps found next to it are new findings (1) and (2)
(2)	internal/commands/mcp_secret_leaves_test.go:130	2	✅ Fixed	TestMCPResponseSecrets_AreClassified (11e1092b) walks the 2xx response schemas of the Pro, Platform and Security specs. It classifies the CloudFront privateKey and fails on stale verdicts. A synthetic test confirms it fires

Review coverage and scope

• Correctness: RedactClassicProfilePayloads span rewrite (nested, CDATA, undecodable payloads fail closed, depth check on EOF), plist round trip of the redacted payload, pro diff's shallow-field path through redactPayloadsKeys
• Security: wire-checked through a real mcp serve session. Covered get -o json, get -o raw and the blueprint converter, then grepped every other Classic-profile reader (pro_blueprints.go, pro_group_tools.go, pro_report_mdm.go, dashboards). Only the converter prints payload content, which is (1)
• Test coverage: internal/profileconvert, internal/commands/pro/generated, internal/commands (MCP|Diff|Payload|Redact) and generator/... pass at 11e1092
• Silent failures: an undecodable payload becomes the marker, and a body that is not XML is refused. redactDiffPayloadValue leaves a value that is not JSON alone, which is safe here because diffObjects is shallow and a nested general arrives as JSON
• Documentation: CHANGELOG, agent_context.md, mcp serve --help, the tool description and the mcpRefusedCommands comment agree with one another. They overclaim against the matcher, which is (2)
• Project rules: root CLAUDE.md and the three .claude/rules/*.md, from the session load. The PR changes none of them. The generated files changed through generator/classic/generator.go, as the generated-code rule requires
• [na] Performance: one plist decode per profile get in an MCP child only. Fidelity: no linked spec

Grading: this round graded the author's four commits since 8b7ab94 (a7612bfe, 2b4bd96e, 2b9f2de6, 11e1092b). Merge f919e4e9, which brought in #404, matches a clean git merge-tree of its parents. git merge-tree against current main reports no conflict. Specialist lanes were not re-dispatched, because this run could not spawn subagents. The new redaction predicate is a shared guard, which would normally re-run security and silent-failure, so both were covered inline with wire probes. The orchestrator scored the findings itself: both are wire-verified, and (2) is at c=75 because its severity rests on how the help text reads.

_{🤖 Generated by the pr-review:review skill v1.39.0 · reviewed head 11e1092}

[ktn-jamf]

Round 4 is addressed in c17fad5 (failing tests), 5070faa and b70aed3.

1. Blueprint converter --id/--name. In an MCP child, pro blueprints components configuration-profile now runs the downloaded mobileconfig through profileconvert.RedactPayloadSecrets before ConvertMobileconfig. If the payload does not decode, the command fails closed with an error. The redaction is at the print site only, not in fetchClassicProfile, so import-profile still sends the real values. A test asserts that the create request carries the real PSK. The MCP-child test covers --type computer and --type mobile.

Caller sweep of the Classic profile fetchers:

• Prints payloads, now redacted: components configuration-profile.
• Writes, left alone: import-profile. It prints only the blueprint it created, read back from the Platform API.
• Reads scope or category only, never <payloads>: the dashboard passes and group-tools.
• List only: pro report mdm and overview.
• Refused over MCP: pro backup.

2. Key matcher coverage. isSecretPayloadKey now matches a whole, case-insensitive suffix from the exported SecretPayloadKeySuffixes: password, secret, token, authkey, apikey, accesskey, privatekey, secretkey, passcode, credential. The fixture includes CloudManagementEnrollmentToken and TailscaleAuthKey. TokenURL, TokenEndpoint, PIN, KeyID, SSID_STR and PayloadIdentifier stay visible. The help text, the run_command description, the CHANGELOG and agent_context.md name these suffixes and say that every other payload value is shown. A new test holds all four texts to the list.

Gates: no test failures, make lint printed "0 issues." and make verify-generated printed "Generated code is up to date.".

[neilmartin83]

Warning

⚠️ Needs changes. Both round-4 findings are fixed: the blueprint converter redacts its --id/--name download in an MCP child, and the payload key matcher now covers token-named and key-named keys. The four policy texts also name the suffixes. One new gap is open. -vvv is not refused over MCP, and it writes the Classic response body to stderr, where the <payloads> plist is not redacted. run_command returns stderr to the model.
Blocking: (1).

Rating: 4/5

• Would be a 5 once a run_command with -vvv can no longer print a body that the PR's redaction removes from stdout (1).

Findings

⚠️ IMPORTANT (1) (security, pre-existing) — internal/commands/root.go:923: -vvv logs the unredacted Classic profile body, and run_command returns that log to the model

--verbose is not in blockedChildFlagPrefixes (internal/commands/mcp.go:567), and refuseOverMCP has no rule for it. In the child, client.WithVerbose(verboseLevel) goes into client.New. The body is logged inside the client, before any MCP-child decorator (cdnKeyRedactingClient) and before the print-site redaction (redactClassicProfilePayloadsInMCPChild, the blueprint converter's RedactPayloadSecrets). runChild uses CombinedOutput() (mcp.go:497), so stderr reaches the model.

The log goes through redactBodyForLog → RedactCredentialBody. That function matches a credential by name in three forms: a JSON key, a form field, and a leaf XML element (credentialXMLElementRe, internal/client/client.go:504). A profile's secret is in none of those forms. It is a plist inside <payloads>, escaped as element text, and the plist puts the value in a <string> beside a <key>Password</key>. I ran the redactor's regex against such a body. The output was byte-identical to the input:

<payloads>&lt;dict&gt;&lt;key&gt;Password&lt;/key&gt;&lt;string&gt;HUNTER2PSK&lt;/string&gt;&lt;/dict&gt;</payloads>

So the redaction from rounds 2 to 4 holds on stdout and can be skipped with one flag. The JSON and leaf-element credentials (service_token, distribution point and SMTP passwords, privateKey) are matched in the log, so the payload plist is the gap. The token-named payload keys this round added (CloudManagementEnrollmentToken, TailscaleAuthKey) are not in credentialNamePattern either.

Failure scenario: run_command ["pro","classic-macos-config-profiles","get","<Wi-Fi profile id>","-vvv"]. stdout shows <redacted>, and the stderr body log in the same tool result carries the PSK. The same happens with pro blueprints components configuration-profile --id <id> -vvv.

Suggested fix: in an MCP child, cap or refuse body logging. Do not try to teach the log redactor about plists:

• In refuseOverMCP, refuse a resolved --verbose whose count is 3 or more, with a reason ("bodies are not logged over MCP"). This fits the PR's model: the parent refuses and the child checks again.
• Or, in the child's PersistentPreRunE, clamp verboseLevel to 2 when registry.InMCPChild(), and say so on stderr.

Refusing is better. A clamp leaves -vvv advertised in the tool and silently changes what it does. Also check what -vv headers show. Authorization is redacted, but Set-Cookie/Cookie (the session-affinity cookies the jar carries) are logged in full.

Fixed when: a buildChildArgs test refuses -vvv, -v -v -v and --verbose=3 (and -vvv placed after the command path), the child-side check refuses the same argv with JAMF_CLI_MCP=1, and the policy texts say body logging is not available over MCP.

This covers all findings — addressing the above gets this PR to merge-ready.

Prior findings status

#	Location	Rounds	State	Notes
(1)	internal/commands/pro_blueprints.go:912	2	✅ Fixed	5070faa1 redacts the download in an MCP child, before ConvertMobileconfig, and fails closed when the payload does not decode. It is placed at the print site, so import-profile still sends the real values, and a test holds that. Both --type computer and --type mobile are covered
(2)	internal/profileconvert/payload_secrets.go:22	2	✅ Fixed	b70aed36 uses the exported SecretPayloadKeySuffixes (adds token, authkey, apikey, accesskey, privatekey, secretkey, passcode, credential). isScalarSecret limits it to string and data values, so boolean policy keys such as RequirePasscode are not touched. TestMCPPolicyTexts_NameEveryPayloadSecretSuffix holds all four policy texts to the list

Review coverage and scope

• Correctness: the new suffix matcher, scalar-only replacement, and the redaction placed only on the converter's download branch (--from-file input is the operator's file inside --input-dir)
• Security: swept other routes by which a fetched profile body can reach the child's combined output. Found -vvv (1). Checked credentialNamePattern against the Classic schema's token field names: the VPP token is service_token, which is matched
• Test coverage: go test ./internal/profileconvert/ and go test ./internal/commands/ -run 'MCP|Blueprint|Payload|Redact|PolicyText' pass at b70aed3
• Silent failures: an undecodable download is refused, not printed
• Documentation: help, tool description, CHANGELOG and agent_context.md agree with the matcher and are held to it by a test. They do not mention -vvv, which is part of (1)
• Project rules: root CLAUDE.md and the three .claude/rules/*.md, from the session load. The PR changes none of them. No generated files changed this round
• [na] Performance, fidelity (no linked spec)

Grading: this round graded the three commits since 11e1092 (c17fad57, 5070faa1, b70aed36). No specialist lanes were dispatched. The delta is 292 lines across two redaction call sites already covered by the security and silent-failure lanes in earlier rounds. (1) was found inline and checked by running the log redactor's regex on a synthetic escaped-payload body. It was not wire-checked through a live mcp serve, which is why it is scored c=75 and not 100.

_{🤖 Generated by the pr-review:review skill v1.39.0 · reviewed head b70aed3}

[neilmartin83]

Round 5, finding (1): wire-checked. Confidence is now 100.

I ran the PR binary at b70aed3 as a real mcp serve -p pro-nmartin over stdio. The probe was a disposable macOS profile (id 7621, since deleted) with a Wi-Fi payload whose Password was the sentinel PROBE402PSKSENTINEL.

run_command args	sentinel in the tool result
pro classic-macos-config-profiles get 7621 -o json	no
pro classic-macos-config-profiles get 7621 -o json -vvv	yes, in the -vvv response-body log
-vvv pro classic-macos-config-profiles get 7621 -o raw	yes
pro blueprints components configuration-profile --id 7621	no, <redacted>
pro blueprints components configuration-profile --id 7621 -vvv	yes. The <redacted> component JSON and the raw PSK are in the same result

The flag is accepted both before and after the command path. Nothing refuses it in the parent or in the child.

Correction to (1): please ignore my aside about cookies at -vv. On the same tenant, -vv logged Authorization: [redacted] and no Cookie or Set-Cookie header. The fix stays the same: refuse --verbose at 3 or above over MCP, in refuseOverMCP and again in the child's check.

[ktn-jamf]

Round 5 is addressed in ad18fdb (failing tests), 7ddd0a0, ca06b80 and 87b521c.

-vvv over MCP. refuseOverMCP now refuses a resolved --verbose level of 3 or more with "use -vv or less". The parent check and the child re-check under JAMF_CLI_MCP=1 both use it. The level is resolved the way pflag's count flag does it, so these forms are refused through buildChildArgs and through the child check: -vvv, -v -v -v, --verbose=3, --verbose=4, -vv -v, -v -vv, and -vvv after the command path. -v, -vv, --verbose=2 and -vvv --verbose=2 stay allowed. childEnv drops JAMF_CLI_ARGS, and the child judges the argv it parsed. The help, the run_command description, the CHANGELOG and agent_context.md say that body logging is not available over MCP.

Cookies at -vv. The header log now shows Cookie and Set-Cookie values as [redacted] in every mode, not only over MCP.

Test change. TestBuildChildArgs_KeepsDryRunVerboseHelpAndFormat, which this branch added, listed -vvv as allowed. That entry is now -vv.

With the refusal removed, all 14 refusal checks fail. With the cookie redaction removed, both cookie checks fail. Gates: no test failures, make lint printed "0 issues." and make verify-generated printed "Generated code is up to date.".

[ktn-jamf]

Thanks for the wire check. 87b521c6, pushed after this comment, covers every row in your table. refuseOverMCP refuses a resolved --verbose of 3 or more in the parent and again in the child, both before and after the command path. The tests cover -vvv, -v -v -v, --verbose=3, -vv -v and a trailing -vvv. Details are in the round-5 reply.

On cookies: noted that your tenant logged no Cookie or Set-Cookie header at -vv. I kept 7ddd0a04, which shows those values as [redacted] in the header log. It is a small change that protects against a load balancer that sets a session cookie. If you would rather keep the PR narrower, I can revert it.

[neilmartin83]

Tip

✅ Merge-ready. Round 5's finding (1) is fixed, and I checked the fix through a real mcp serve -p pro-nmartin. run_command now refuses --verbose at level 3 or more in both the parent and the child. The -vv header log also redacts Cookie/Set-Cookie values, in every mode. One nice-to-have below, not blocking: the child already catches what it describes.

Rating: 5/5

• The last open gap is closed and wire-verified. The one remaining item is defence-in-depth parity, and the child's check already covers it.

Findings

1 nice-to-have suggestion

💡 NICE-TO-HAVE (1) (correctness) — internal/commands/mcp.go:865: verboseCount parses with strconv.Atoi, while pflag's count flag uses strconv.ParseInt(s, 0, 0)

pflag reads --verbose=0x3 and --verbose=0b11 as 3. Atoi fails on both, so the parent leaves n at 0 and spawns the child. The child then refuses, because it reads the final count, so nothing leaks. I confirmed this on the wire: both spellings came back as the child's refusal (command failed: exit status 1 around the refusal envelope), with no request body in the result. The cost is that the parent check is not the boundary its comment claims ("resolves --verbose the way pflag's count flag does"), and the refusal comes back in a different shape.

Suggested fix: strconv.ParseInt(s.value, 0, 0) instead of Atoi, and add --verbose=0x3 to verboseBodyForms.

Fixed when: TestMCP_RefusesVerboseBodyLogging refuses --verbose=0x3 at buildChildArgs.

Prior findings status

#	Location	Rounds	State	Notes
(1)	internal/commands/root.go:923	1	✅ Fixed	ca06b80d refuses --verbose ≥ 3 in refuseOverMCP, which the child also runs. Wire-checked against a disposable profile (id 7622, since deleted) whose Wi-Fi Password is a sentinel. -vvv before or after the path, -v -v -v, -vv -v, --verbose=3, --verbose=2 -v, --verbose=+1 -vv, -v=3 and the blueprint converter with -vvv are all refused, with no sentinel. -vv runs and carries no sentinel. Plain get and the converter still run and are still redacted. 7ddd0a04 covers my withdrawn cookie aside anyway, which is harmless

Review coverage and scope

• Correctness: verboseCount against pflag's countValue.Set (+1 increments, an explicit value sets it, so order is respected). Every body log (internal/client/client.go and the Platform SDK transport in pro_platform_helpers.go) is gated on the same global verboseLevel ≥ 3. No env var turns on body logging. JAMF_CLI_ARGS is stripped by childEnv
• Security: wire-checked through a real mcp serve as above, including the 0x3/0b11 spellings that only the child catches, which is (1)
• Test coverage: go test ./internal/client/ ./internal/profileconvert/ ./internal/commands/ passes at 87b521c. The new tests cover the parent and the child for 7 refused and 4 allowed spellings
• Documentation: mcp serve --help, the tool description, the CHANGELOG and agent_context.md all name the -vvv refusal and the cookie redaction
• Project rules: root CLAUDE.md and the three .claude/rules/*.md, from the session load. The PR changes none of them
• [na] Performance, fidelity (no linked spec)

Grading: this round graded the four commits since b70aed3 (ad18fdb8, 7ddd0a04, ca06b80d, 87b521c6), 134 lines. No specialist lanes were dispatched. The delta is one predicate and one header-log condition, and both were checked inline and on the wire.

_{🤖 Generated by the pr-review:review skill v1.39.0 · reviewed head 87b521c}