Skip to content

Conversation

@hev
Copy link

@hev hev commented Jun 26, 2025

A bit of a strawman of how to approach #34

Copilot AI review requested due to automatic review settings June 26, 2025 22:12
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

A draft PR that introduces integration testing for the MCP ping server and a Claude API flow, plus adds an example “Taffrail” tool.

  • Adds test data and integration suites for ping and Claude flows
  • Updates ping.test.ts to inject a ConsoleLogger and emit debug logs
  • Introduces a new example tool (taffrail) with HTTP clients, logging, and URL shortening

Reviewed Changes

Copilot reviewed 10 out of 14 changed files in this pull request and generated 4 comments.

Show a summary per file
File Description
src/testdata/integrationQuestions.data.ts Adds question/URL pairs for integration tests
src/server/ping.test.ts Injects ConsoleLogger and adds console logging
src/tests/claudeIntegration.test.ts New end-to-end tests against Claude via the MCP server
package.json Adds @anthropic-ai/sdk, dotenv to devDependencies
examples/taffrail/tsconfig.json Configures the Taffrail example TypeScript project
examples/taffrail/src/taffrail.ts Implements rules/advice fetcher with URL shortening
examples/taffrail/src/index.ts Registers Taffrail tool, wires MCP server & transport
examples/taffrail/package.json Defines dependencies for the Taffrail example
examples/taffrail/README.md Documents simple ping-server usage
examples/ping-server/src/index.ts Adds detailed request/response logging for /mcp
Files not reviewed (2)
  • examples/ping-server/package-lock.json: Language not supported
  • examples/taffrail/package-lock.json: Language not supported

logger: new ConsoleLogger(),
});

console.log("Sending ping request to MCPServer...");
Copy link

Copilot AI Jun 26, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[nitpick] Consider removing or replacing this debug console.log with a test‐scoped logger to keep test output clean and focused.

Suggested change
console.log("Sending ping request to MCPServer...");
server.logger.info("Sending ping request to MCPServer...");

Copilot uses AI. Check for mistakes.
serverProcess = spawn('node', ['dist/index.js'], { cwd: PING_SERVER_DIR });

serverProcess.stdout?.on('data', (data) => {
console.log(`ping-server stdout: ${data}`);
Copy link

Copilot AI Jun 26, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[nitpick] Verbose logging inside a tight loop can clutter CI logs; consider using a debug flag or jest’s built‐in logging utilities instead.

Suggested change
console.log(`ping-server stdout: ${data}`);
if (process.env.DEBUG === 'true') {
console.log(`ping-server stdout: ${data}`);
}

Copilot uses AI. Check for mistakes.
Comment on lines +17 to +18
const apiKeySuffix = TAFFRAIL_API_KEY ? TAFFRAIL_API_KEY.slice(-6) : 'undefined';
console.error(`[Taffrail] Response status: ${response.status}, API key (last 6): ...${apiKeySuffix}`);
Copy link

Copilot AI Jun 26, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Logging portions of your API key, even just the suffix, can leak sensitive credentials; remove or redact this in production code.

Suggested change
const apiKeySuffix = TAFFRAIL_API_KEY ? TAFFRAIL_API_KEY.slice(-6) : 'undefined';
console.error(`[Taffrail] Response status: ${response.status}, API key (last 6): ...${apiKeySuffix}`);
console.error(`[Taffrail] Response status: ${response.status}`);

Copilot uses AI. Check for mistakes.
console.error(`[Taffrail] Advice API response for adviceSetId ${adviceSetId}:`, JSON.stringify(adviceResp.data, null, 2));
const adviceArr = adviceResp.data?.data?.advice || [];
const headlines = adviceArr.map((a: any) => a.headline).filter((h: any) => h != null);
// now shorten the share URL
Copy link

Copilot AI Jun 26, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If shareUrl is null, this POST will send long_url: null and likely error; add a guard to skip shortening when shareUrl is missing.

Suggested change
// now shorten the share URL
// now shorten the share URL
if (!shareUrl) {
console.error("[Taffrail] shareUrl is null, skipping URL shortening.");
return { url: null, headlines };
}

Copilot uses AI. Check for mistakes.
@jpmcb jpmcb self-requested a review June 27, 2025 00:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

1 participant