diff --git a/.github/prompts/code-review.prompt.md b/.github/prompts/code-review.prompt.md new file mode 100644 index 0000000..e3a2714 --- /dev/null +++ b/.github/prompts/code-review.prompt.md @@ -0,0 +1,204 @@ +--- +agent: agent +description: 'Perform comprehensive code review as a senior technical lead, evaluating correctness, quality, maintainability, and adherence to standards with actionable feedback' +--- + +# Task: Code Review as Senior Technical Lead + +You are a senior technical software engineer lead performing a thorough code review. Your goal is to reduce risk and improve code quality through objective, actionable pragmatic feedback. + +## Review Scope + +1. **Identify code to review**: + - **Uncommitted changes** (default): Review all uncommitted changes in the workspace using `git diff` + - Include staged and unstaged changes + - Exclude untracked files unless explicitly requested + - **Line-specific review**: If specific lines are selected/provided, **focus the review exclusively on those lines** + - Use surrounding context for understanding, but only provide feedback on the selected lines + - Do not expand the review to other parts of the file unless directly related to the selected lines + - **Full file review**: If no uncommitted changes exist and no lines are selected, review the currently open file + - **Custom scope**: If user specifies files/paths/commits, review those instead + +2. **Detect file types and load applicable standards**: + - Identify programming language(s) and frameworks in the code + - Read and apply relevant instruction files from `.github/instructions/` + - Apply language-specific best practices automatically + - Override with project-specific guidelines when available in config files, ie. linter configs, style guides, compiler options, code sniffers, etc. + +3. **Detect testing configuration and standards**: + - Identify test framework(s), check for testing config files (ie. jest.config.js, pytest.ini, .coveragerc), review project-level testing documentation, and analyze existing test patterns to understand coverage thresholds and mocking philosophy. + - Apply project's established testing conventions when generating or reviewing tests; prompt for guidance if testing standards are not documented. + +## Reviewer Persona & Guidelines + +**Approach**: +- Be polite, but pragmatic +- Make best-intent assumptions that the author has done their homework +- Provide objective, evidence-based feedback +- Avoid boasting about programming knowledge +- Remember: "Perfect is the enemy of good" - pragmatism over perfection +- Focus on reducing risk, not producing perfect code + +**Reviewer Conduct**: +- **Address the code, not the developer**: Comments should be about the code itself, never evaluative of the person +- **Write clear and specific comments**: Be precise about what needs to change and why +- **Include positive feedback**: Always highlight strengths and good decisions, not just issues +- **Be generous with code examples**: Show the suggested improvement in code when possible +- **Frame feedback as requests, not commands**: "Consider refactoring..." vs. "Refactor this..." +- **Tie notes to principles, not opinions**: Reference standards, best practices, official guidance, or project guidelines rather than personal preference +- **Start high-level, then work down**: Review architecture and design first, then dive into implementation details + +**Feedback Priority** (Scale): +- **[l: low]** - Non-blocking. Author may address but doesn't have to +- **[m: medium]** - Normal comment. Worth addressing and fixing +- **[h: high]** - Critical. Must not merge without addressing this issue + +## Comprehensive Review Checklist + +**Important**: When reviewing specific lines only, apply these checks exclusively to the selected lines. Use surrounding code for context, but do not provide feedback on unselected portions of the file. + +### 1. Necessity & Purpose +- **Is this code required?** Does it solve a real problem or is it speculative/premature? +- Does this change align with project goals and priorities? +- Could the desired outcome be achieved with less code or by leveraging existing functionality? + +### 2. Correctness & Functionality +- Does the code accomplish its intended purpose? +- Are there any logical errors or edge cases not handled? +- Are error conditions properly handled? +- Does the code match any requirements or specifications? + +### 3. Code Quality & Maintainability +- **System design**: How well do the various pieces interact together? Is the integration with the overall system well-designed? +- **Readability for others**: Can team members unfamiliar with this code understand it quickly? Is it written for humans first? +- **Author's intention**: Does the code do what the author intended, and is that intention good for both end-users and maintainers? +- **Complexity**: Is this no more complex than needed? Can it be understood quickly? Are developers likely to introduce bugs when modifying it? +- **Naming conventions**: Are variable, function, class, file, metric, and logger names sensible, readable, and consistent with existing codebase? +- **Code clarity**: Is the code self-documenting? Is complex logic explained? +- **Single Responsibility**: Do functions/classes have clear, focused purposes? +- **DRY principle**: Is there unnecessary code duplication? +- **Magic values**: Are there hardcoded values that should be constants/configs? + +### 4. Documentation & Type Safety +- **Function documentation**: Are function docs complete and accurate? +- **Type hints/annotations**: Are types properly defined and comprehensive? +- **Comments**: Are they necessary, accurate, and add value? +- **Outdated docs**: Have docs been updated to match code changes? + +### 5. Efficiency & Performance +- Are there more efficient approaches to achieve the same result? +- Are there unnecessary computations or redundant operations? +- Are data structures and algorithms appropriate for the use case? +- Are there potential memory leaks or resource management issues? + +### 6. Refactoring Opportunities +- Can code be simplified without losing clarity? +- Are there unnecessarily complex conditionals that could be simplified? +- Are we implementing something that already exists elsewhere in the codebase? +- Can nested loops/conditionals be flattened or extracted? +- Is there dead code or unused imports/variables? +- Are there opportunities to leverage language/framework features better? + +### 7. Testing & Coverage +- Are there gaps in unit test scenarios for this code? +- Are edge cases and error conditions tested? +- Are tests readable and maintainable? +- Is test coverage adequate for the risk level? +- Are integration points properly tested? + +### 8. Security Review +- Apply all checks from `security.instructions.md`: + +### 9. Accessibility Review (UI/Frontend Code) +- Apply all checks from `a11y.instructions.md`: + +### 10. Logging & Observability +- Are log statements accurate in describing the logic state? +- Is the log level appropriate (debug, info, warn, error)? +- Are sensitive data (passwords, tokens, PII) excluded from logs? +- Are important state changes and errors logged? + +## Contextual Questions for Author + +Before completing the review, ask the author: + +1. **Change rationale**: "Can you explain the primary reason for these changes? What problem are we solving?" + +2. **Alternative approaches**: "Were there other approaches you considered but decided against? What led you to this solution?" + - This provides context and prevents reviewers from suggesting already-explored alternatives + +3. **Risk assessment**: "Are there any areas of this code you're uncertain about or would like specific feedback on?" + +4. **Deployment considerations**: "Are there any special deployment steps, feature flags, or rollback plans needed for these changes?" + + +## Deliverables + +Provide your review in this format: + +### Summary +[Brief 2-3 sentence overview of the changes and overall assessment. For line-specific reviews, explicitly mention you're reviewing only the selected lines (e.g., "lines 7-26")] + +### Strengths +[Highlight 2-3 positive aspects - be genuinely complimentary] + +### Changes +- order by severity and priority +- limit to 3-5 critical issues to avoid overwhelming the author +- Use conventional comments (https://conventionalcomments.org/) to format each feedback item (e.g., **issue (security):** for security issues, **suggestion:** for improvements) + +### Required Changes [h: high] +[List critical issues that must be fixed before merging] +- Include specific line numbers/code references +- Provide suggested code changes in fenced code blocks when possible +- Explain the risk or impact of not addressing + +### Recommended Improvements [m: medium] +[List important improvements worth making] +- Include specific line numbers/code references +- Provide suggested code changes when possible +- Explain the benefit of making the change + +### Non-blocking [l: low] +[List minor improvements that are optional] +- Keep these brief and only if they add value + +### Questions for Author +[List additional contextual questions relevant to this review] + +### Testing Recommendations +[Suggest specific test scenarios that should be added or verified] + +### Overall Assessment +[Final recommendation: "Approved", "Approved with suggestions", or "Requires changes"] + + +## Validation Steps + +1. Ensure all feedback is: + - **Objective**: Based on standards, not personal preference + - **Actionable**: Clear what needs to be done + - **Specific**: References exact locations and provides examples + - **Constructive**: Focuses on improvement, not criticism + - **Respectful**: About the code, never about the developer's skill or judgment + +2. Double-check that suggested code: + - Is syntactically correct for the language + - Follows the project's existing patterns + - Actually solves the identified issue + +3. Verify severity assignments are appropriate: + - Don't over-escalate minor issues + - Don't under-escalate security/correctness issues + +4. Confirm your review includes: + - **Positive feedback**: Genuine compliments on what was done well + - **Principle-based rationale**: Each comment ties to a standard, best practice, or documented guideline (not personal opinion) + - **Non-condescending tone**: Questions are framed respectfully and show genuine curiosity + +## Remember + +- It's okay to ship code in stages and commit to improving later +- Consider the cost of each incremental change request +- Focus on risk reduction, not perfection +- Be a thoughtful colleague, not a gatekeeper diff --git a/test-code/.gitignore b/test-code/.gitignore new file mode 100644 index 0000000..c2658d7 --- /dev/null +++ b/test-code/.gitignore @@ -0,0 +1 @@ +node_modules/ diff --git a/test-code/alertButton.html b/test-code/alertButton.html new file mode 100644 index 0000000..98f27f2 --- /dev/null +++ b/test-code/alertButton.html @@ -0,0 +1,35 @@ + + + + Alert Demo + + + + +
+

Alert System

+ +
+

Send Alert

+
+ + + Maximum 500 characters +
+ +
+ +
+

Alert History

+
+ +
+
+
+ + + + + diff --git a/test-code/alertButton.js b/test-code/alertButton.js new file mode 100644 index 0000000..aee018b --- /dev/null +++ b/test-code/alertButton.js @@ -0,0 +1,42 @@ +// Alert Button Component +// TODO: Add better docs + +var alertButton = document.getElementById('alert-btn'); +var userInput = document.getElementById('user-message'); + +// Show alert when clicked +function showAlert() { + var message = userInput.value; + + // Send to API + fetch('https://api.example.com/alerts', { + method: 'POST', + body: JSON.stringify({msg: message}) + }).then(response => response.json()) + .then(data => { + console.log('API Response:', data); + }); + + // Display alert + if (message) { + alert(message) + } else { + alert('Please enter a message!') + } +} + +alertButton.addEventListener('click', showAlert); + +// Helper function +function validateMessage(msg) { + if (msg.length > 0 && msg.length < 500) { + return true; + } + return false +} + +// Log user activity +function logActivity(user, action) { + console.log(`User ${user.email} performed ${action} at ${new Date()}`); + console.log('User details:', user); +} diff --git a/test-code/alertButton.test.js b/test-code/alertButton.test.js new file mode 100644 index 0000000..dca2bab --- /dev/null +++ b/test-code/alertButton.test.js @@ -0,0 +1,75 @@ +// Test file for alert button + +describe('Alert Button', () => { + test('shows alert when button clicked', () => { + // TODO: implement test + }); + + test('validates message length', () => { + const result = validateMessage('test'); + expect(result).toBe(true); + }); + + test('sanitizes input to prevent XSS', () => { + const maliciousInput = ''; + const sanitized = sanitizeInput(maliciousInput); + expect(sanitized).toBe('<script>alert("XSS")</script>'); + expect(sanitized).not.toContain(' + + + + +
+ + + ')).to.equal('alert(1)'); + }); + }); + + describe('getUserData', () => { + it('should parse user data from localStorage', () => { + localStorageStub.getItem.returns('{"email":"test@example.com"}'); + expect(getUserData()).to.deep.equal({ email: 'test@example.com' }); + }); + }); + + describe('saveAlert', () => { + it('should save alert to localStorage', () => { + localStorageStub.getItem.returns('[]'); + localStorageStub.getItem.withArgs('user').returns('{"email":"test@example.com"}'); + saveAlert('Test alert'); + expect(localStorageStub.setItem.calledWith('alerts', sinon.match.string)).to.be.true; + }); + }); + + describe('calculatePriority', () => { + it('should return 1 for urgent messages', () => { + expect(calculatePriority('urgent issue')).to.equal(1); + }); + + it('should return 2 for important messages', () => { + expect(calculatePriority('important')).to.equal(2); + }); + + it('should return 3 for normal messages', () => { + expect(calculatePriority('normal')).to.equal(3); + }); + }); + + describe('sendNotification', () => { + it('should send POST request for notification', async () => { + const fetchStub = sinon.stub(window, 'fetch'); + fetchStub.resolves({ json: () => Promise.resolve({ sent: true }) }); + const result = await sendNotification('Test', 123); + expect(fetchStub.calledWith('/api/notify', sinon.match({ method: 'POST' }))).to.be.true; + expect(result.sent).to.be.true; + fetchStub.restore(); + }); + }); + }); + + mocha.run(); + + + diff --git a/test-code/utils.js b/test-code/utils.js new file mode 100644 index 0000000..4537b81 --- /dev/null +++ b/test-code/utils.js @@ -0,0 +1,52 @@ +// Utility functions for alert system + +function formatDate(timestamp) { + return timestamp.toLocaleDateString() +} + +function sanitizeInput(input) { + // Basic sanitization + return input.replace('', '') +} + +function getUserData() { + let userData = localStorage.getItem('user'); + return JSON.parse(userData); +} + +function saveAlert(message) { + var alerts = JSON.parse(localStorage.getItem('alerts')) || []; + alerts.push({ + message: message, + timestamp: Date.now(), + user: getUserData() + }); + localStorage.setItem('alerts', JSON.stringify(alerts)); +} + +// Calculate alert priority +function calculatePriority(msg) { + if (msg.includes('urgent') || msg.includes('critical')) { + return 1; + } else if (msg.includes('important')) { + return 2; + } + return 3; +} + +// Send notification +async function sendNotification(message, userId) { + const response = await fetch('/api/notify', { + method: 'POST', + body: JSON.stringify({ + message: message, + userId: userId + }) + }); + return response.json(); +} + +// Unused function +function oldAlertMethod() { + window.alert('This is deprecated'); +} diff --git a/test-code/utils.test.js b/test-code/utils.test.js new file mode 100644 index 0000000..ab004cf --- /dev/null +++ b/test-code/utils.test.js @@ -0,0 +1,74 @@ +// Test file for utility functions + +describe('Utility Functions', () => { + beforeEach(() => { + // Clear localStorage mocks + localStorage.getItem.mockClear(); + localStorage.setItem.mockClear(); + localStorage.removeItem.mockClear(); + }); + + test('sanitizes input to prevent XSS', () => { + const maliciousInput = ''; + const sanitized = sanitizeInput(maliciousInput); + expect(sanitized).toBe('<script>alert("XSS")</script>'); + expect(sanitized).not.toContain('