Skip to content

Conversation

Copy link
Contributor

Copilot AI commented Jan 15, 2026

Addresses review feedback on PR #7 regarding code duplication, a typo, and insufficient test validation.

Changes

  • Restored scripts/utils.js - Centralizes getMajorVersion utility instead of duplicating across validate-frontmatter.js and functional-tests.js
  • Fixed markdown heading typo - o## in 001-accessibility-expert-agent/spec.md
  • Strengthened test assertions - Validates exactly 12 tools are registered by:
    • Asserting test array length equals 12
    • Counting actual vscode.lm.registerTool calls in extension.js
    • Previously only checked that registration calls existed, not the count
// Before: only checked presence
for (const tool of allTools) {
  assert(content.includes(`vscode.lm.registerTool('${tool}'`));
}

// After: validates exact count
assert.strictEqual(allTools.length, 12);
const registrations = content.match(/vscode\.lm\.registerTool\(/g) || [];
assert.strictEqual(registrations.length, 12);

✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.

… test assertions

Co-authored-by: mdaashir <85205726+mdaashir@users.noreply.github.com>
Copilot AI changed the title [WIP] Add Specification-Driven Development framework and SpecKit integration Fix PR review comments: remove code duplication and improve test assertions Jan 15, 2026
Copilot AI requested a review from mdaashir January 15, 2026 09:56
@mdaashir mdaashir marked this pull request as ready for review January 15, 2026 09:57
Copilot AI review requested due to automatic review settings January 15, 2026 09:57
@mdaashir mdaashir merged commit b69e184 into feature/spec-kit-integration Jan 15, 2026
@mdaashir mdaashir deleted the copilot/sub-pr-7 branch January 15, 2026 09:57
Copy link
Contributor

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

This PR addresses feedback from PR #7 by eliminating code duplication, fixing a typo, and strengthening test assertions to validate exact tool counts.

Changes:

  • Centralized getMajorVersion utility function into a shared scripts/utils.js module
  • Fixed markdown heading typo in accessibility expert agent specification
  • Enhanced test assertions to validate exactly 12 tool registrations

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.

File Description
scripts/utils.js New utility module containing shared getMajorVersion function with documentation
scripts/validate-frontmatter.js Removed duplicate getMajorVersion function, now imports from utils
scripts/functional-tests.js Removed duplicate getMajorVersion function and added assertions to verify exact count of 12 tool registrations
resources/specs/001-accessibility-expert-agent/spec.md Fixed markdown heading from o# to #

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

const match = version.match(/^[~^>=<]*(\d+)/);
return match ? parseInt(match[1], 10) : null;
}
const { getMajorVersion } = require('./utils');
Copy link

Copilot AI Jan 15, 2026

Choose a reason for hiding this comment

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

Unused variable getMajorVersion.

Suggested change
const { getMajorVersion } = require('./utils');

Copilot uses AI. Check for mistakes.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants