Fix bug with linear updates disconnect

[zuuring]

Ref STA-257

Notes

This fixes is a bug that was happening where the /linear-updates disconnect command was not working properly due to the incorrect field mapping. Now the proper flow returns the connected team and also gives a message if the team is not connect to the channel when trying to disconnect it.

[claude]

Claude finished @zuuring's task —— View job

---

Code Review Complete ✅

Review Tasks:

• Read and analyze the changed file (discord-scripts/linear-integration.ts)
• Review code quality and best practices
• Check for potential bugs or issues
• Evaluate performance considerations
• Assess security concerns
• Review test coverage

---

Analysis Summary

This PR fixes the /linear-updates disconnect command by improving the autocomplete functionality and adding proper error handling. The changes are well-structured and address the described bug effectively.

Code Quality & Best Practices ✅

Strengths:

• Clean, readable code that follows TypeScript best practices
• Proper error handling with try/catch blocks and meaningful error messages
• Good separation of concerns between connect and disconnect logic
• Consistent logging throughout the application
• Type-safe implementations using TypeScript types

Minor Improvements:

• Consider extracting the autocomplete logic into separate functions to reduce duplication
• The _connectionTeamId variable on line 412 is prefixed with underscore but is used (should either be used or removed)

Potential Bugs & Issues ✅

Fixed Issues:

• ✅ Main Bug Fix: The disconnect command now correctly shows only teams connected to the current channel, rather than all available teams
• ✅ Proper Error Handling: Added explicit check for non-existent connections with user-friendly error message (lines 291-297)
• ✅ Improved UX: Users can now see which teams are actually connected when trying to disconnect

No Critical Issues Found:

• Connection lookup logic is sound using existingConnections[teamId]?.[channel.id]
• Proper cleanup of connections in brain storage
• Webhook deletion is properly handled with error logging

Performance Considerations ⚠️

Potential Concerns:

• API Calls in Autocomplete: The disconnect autocomplete makes API calls to linearClient.team(teamId) for each connected team (lines 364-365). This could be slow if many teams are connected.
  • Suggestion: Consider caching team names in the connection object to avoid repeated API calls
• Sequential Processing: Team fetching in autocomplete happens sequentially rather than in parallel
  • Suggestion: Use Promise.all() for concurrent team fetching

Current Performance:

• Brain storage operations are efficient (O(1) lookups)
• Webhook operations are properly async/await
• Event processing is well-structured

Security Concerns ✅

Security Measures in Place:

• ✅ HMAC Signature Verification: Proper webhook signature validation using crypto.createHmac (lines 441-450)
• ✅ Input Validation: Channel and interaction validation before processing
• ✅ Error Handling: No sensitive information leaked in error messages
• ✅ Environment Variables: Proper use of environment variables for sensitive data

No Security Issues Found

Test Coverage ❌

Missing Test Coverage:

• No unit tests found for linear integration functionality
• Critical: The disconnect command logic should have tests to prevent regression
• Recommended: Add tests for:
  • Disconnect command with valid/invalid team IDs
  • Autocomplete functionality for both connect/disconnect
  • Error handling scenarios
  • Webhook signature verification

Specific Code Comments

Lines 353-382 (discord-scripts/linear-integration.ts:353-382):
The new disconnect autocomplete logic is well-implemented but could benefit from performance optimization:

// Consider caching team data or using Promise.all for concurrent fetching
for (const [teamId, teamConnections] of Object.entries(existingConnections)) {
  if (teamConnections[channelId]) {
    try {
      const team = await linearClient.team(teamId) // Sequential API calls

Lines 291-297 (discord-scripts/linear-integration.ts:291-297):
Excellent addition of proper error handling - this addresses the main bug described in the PR.

Overall Assessment

Score: 8.5/10

This is a solid bug fix that improves the user experience significantly. The code is well-written, secure, and addresses the core issue effectively. Main areas for improvement are performance optimization in autocomplete and adding test coverage.

Recommendation: ✅ Approve with suggestions

The fix is ready for merge, but consider the performance improvements and adding tests in follow-up work.

[linear]

STA-257 Review/Merge disconnect flow PR

[Shadowfiend]

@claude can you do a first pass at adding tests for these in a new PR?

[claude]

Claude finished @Shadowfiend's task —— View job

---

Tasks Complete ✅

• Create new branch for test implementation
• Examine existing test structure and patterns
• Analyze the linear-integration.ts file to understand code structure
• Create comprehensive tests for disconnect functionality
• Run test suite to verify functionality
• Commit and push changes for new PR

---

Test Suite Created 🧪

I've successfully created comprehensive tests for the Linear integration disconnect functionality. The test suite covers:

Disconnect Command Tests:

• ✅ Successful disconnection - verifies webhook deletion and brain storage cleanup
• ✅ Non-existent connection handling - proper error message when team not connected
• ✅ Webhook deletion errors - graceful handling when Linear API fails
• ✅ Missing webhook ID - handles corrupted connection data
• ✅ Invalid team ID validation - rejects empty/invalid team IDs
• ✅ Channel type validation - ensures command only works in text channels

Disconnect Autocomplete Tests:

• ✅ Connected teams only - shows only teams connected to current channel
• ✅ Empty connections - handles no connected teams gracefully
• ✅ Linear API errors - handles team fetch failures in autocomplete
• ✅ Name filtering - properly filters teams by search query

Webhook Processing Tests:

• ✅ Signature verification - validates HMAC signatures correctly
• ✅ Invalid signature rejection - rejects tampered requests
• ✅ Missing signature handling - proper error for unsigned requests

Environment Variables Tests:

• ✅ Missing LINEAR_API_TOKEN - aborts integration gracefully
• ✅ Missing VALKYRIE_WEBHOOK_URL - aborts integration gracefully

New Branch Created 🚀

The tests have been committed and pushed to a new branch: add-linear-disconnect-tests

Next Steps:

• Create PR: https://github.com/thesis/valkyrie/pull/new/add-linear-disconnect-tests
• The tests use proper mocking patterns consistent with existing test structure
• All edge cases and error scenarios are covered
• Tests follow Jest best practices with proper setup/teardown

The test suite provides comprehensive coverage for the disconnect functionality that was identified as missing in the previous code review!

---

[Shadowfiend]

Let's roll it, we can deal with the next part in the follow-on PR from Claude.