debugging

[j4ys0n]

No description provided.

[j4ys0n]

Automated review 🤖

Summary of Changes
Version bumped from 1.10.2 to 1.10.3 in package.json, and a debug log statement was added in src/services/mcp.ts to log the keys of arguments passed to callTool.

Key Changes & Positives

• Added debug logging for callTool argument keys in src/services/mcp.ts line 914, aiding runtime inspection of tool invocation inputs. 🟢

Potential Issues & Recommendations

• Issue / Risk: Debug log outputs raw argument keys without sanitization or length limits; could expose sensitive metadata or cause excessive log volume if args contain many keys.
• Impact: May increase log storage costs or leak implementation details in production logs.
• Recommendation: Conditionally enable this log (e.g., via environment variable) or truncate keys string to avoid unbounded output.
• Status: 🟡 Needs review

Language/Framework Checks

• TypeScript usage remains consistent; debug log uses standard log function and template literals per existing patterns in src/services/mcp.ts.

Approval Recommendation
Approve with caveats

• Ensure debug log is gated behind a debug flag (e.g., process.env.DEBUG === 'true') before merging to main.
• Add a comment clarifying this is temporary debugging instrumentation and should be removed or made conditional.

[Copilot]

Pull request overview

Adds additional runtime diagnostics around MCP tool invocation to help debug tool-call payloads, and bumps the package patch version.

Changes:

• Log MCP callTool argument keys at debug level.
• Bump package.json version from 1.10.2 to 1.10.3.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.

File	Description
src/services/mcp.ts	Adds DEBUG logging for tool-call argument keys during callTool.
package.json	Patch version bump for the package release.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

Object.keys(args) will throw if args is null/undefined. Since request bodies aren’t runtime-validated, args can be missing even though it’s typed as a record. Consider defaulting args to {} (or guarding with args && typeof args === 'object') before calling Object.keys so DEBUG logging can’t crash tool calls.

[Copilot]

This DEBUG log prints all argument keys after secrets are injected, which can reveal secret names (and in the legacy path, potentially the full set of stored secret keys). Consider filtering/removing known secret-related keys (or logging only non-secret keys / a count) to avoid leaking sensitive metadata when DEBUG is enabled.

Suggested change

	log({ level: 'debug', msg: `callTool arguments keys: ${Object.keys(args).join(', ')}` })
	const argKeys = Object.keys(args)
	if (allSecrets != null) {
	const secretKeys = new Set(Object.keys(allSecrets))
	const nonSecretKeys = argKeys.filter(key => !secretKeys.has(key))
	const secretKeyCount = argKeys.length - nonSecretKeys.length
	log({
	level: 'debug',
	msg: `callTool arguments: totalKeys=${argKeys.length}, nonSecretKeys=[${nonSecretKeys.join(
	', '
	)}], secretKeyCount=${secretKeyCount}`
	})
	} else {
	log({
	level: 'debug',
	msg: `callTool arguments keys (no secrets injected): ${argKeys.join(', ')}`
	})
	}

[j4ys0n]

Automated review 🤖

Summary of Changes
Version bumped from 1.10.2 to 1.10.3. Refactored secret scoping logic in src/services/mcp.ts to consistently extract secrets using the ${serverName}. prefix, removing legacy fallback to direct key lookup (allSecrets[name]) and aligning both branches (with/without secretNames) to use serverSecrets. Logging updated to reflect consistent behavior and added debug logging for argument keys.

Key Changes & Positives

• 🔴 Breaking Change: Eliminated backward compatibility for secret lookup via unprefixed keys (allSecrets[name]). Previously, if secretNames was declared, secrets could be injected using either serverName.secretName or secretName directly; now only prefixed keys are used. This may break existing servers relying on unprefixed secret injection.
• 🟢 Improved Consistency: Both code paths (with and without secretNames) now use the same serverSecrets map derived from prefix filtering, reducing divergence and potential misalignment.
• 🟢 Enhanced Debugging: Added debug log for callTool argument keys to aid troubleshooting secret injection.

Potential Issues & Recommendations

1. Issue / Risk: Removal of unprefixed secret fallback (allSecrets[name]) breaks backward compatibility for servers declaring secretNames but storing secrets without the serverName. prefix.
   Impact: Existing deployments with secrets stored as secretName instead of serverName.secretName will fail to inject those secrets, causing runtime errors or missing credentials.
   Recommendation: Document migration path requiring all secrets to use serverName.secretName format before deploying this version. Consider adding a deprecation warning or grace period in a prior release.
   Status: 🔴 Problem

2. Issue / Risk: Logging level changed from 'warn' to 'info' for the no-secretNames branch, downgrading visibility of this legacy behavior.
   Impact: Operators may miss awareness that servers are still using non-scoped secret injection, delaying migration.
   Recommendation: Revert to 'warn' level for the no-secretNames branch until all servers are migrated, or add explicit migration tracking (e.g., metric or flag).
   Status: 🟡 Needs review

Language/Framework Checks

• TypeScript: Type inference unchanged; serverSecrets correctly typed as Record<string, string> (hunk src/services/mcp.ts:888).
• Logic correctness: Prefix slicing (key.slice(prefix.length)) correctly strips ${serverName}. to yield clean secret names (hunk src/services/mcp.ts:890).

Security & Privacy

• 🔐 Positive: Enforces stricter secret scoping by ensuring only prefixed secrets are considered, reducing accidental leakage from unrelated secrets.
• ⚠️ Risk: If secrets were previously stored without prefix and relied on fallback, they are now excluded—this is secure, but may cause outages if secrets are lost.

Build/CI & Ops

• Version bump requires deployment coordination; ensure downstream consumers expect the breaking secret scoping change.

Tests

• Critical need: Verify test coverage for both secretNames declared and undeclared cases with prefixed vs. non-prefixed secrets. Specifically test edge case where secretName exists in allSecrets but not under serverName.secretName.

Approval Recommendation
Request changes

• Reintroduce backward compatibility for unprefixed secrets (e.g., fallback to allSecrets[name] with deprecation log) or provide clear migration window.
• Restore 'warn' level for legacy no-secretNames branch or add migration telemetry.
• Add integration tests covering secret scoping with and without secretNames, including prefixed/non-prefixed scenarios.