Skip to content

Fix: Add shorthand syntax support for notify send (#853) - #857

Merged
tbrandenburg merged 3 commits into
mainfrom
dev
Feb 1, 2026
Merged

Fix: Add shorthand syntax support for notify send (#853)#857
tbrandenburg merged 3 commits into
mainfrom
dev

Conversation

@tbrandenburg

Copy link
Copy Markdown
Owner

Summary

Documentation showed shorthand syntax work notify send TASK-001 to alerts but the implementation only accepted the full syntax work notify send where <query> to <target>. This PR adds support for both syntaxes.

Root Cause

The CLI command had fixed argument definitions that required exactly 4 arguments in the specific order: where, <query>, to, <target>. This made the shorthand syntax impossible.

Changes

File Change
src/cli/commands/notify/send.ts Removed fixed args, enabled strict=false, added argv parsing to support both syntaxes
tests/e2e/notify-workflow.test.ts Added test case for shorthand syntax

Implementation Details

  • Used strict=false to allow flexible argument parsing
  • Parse argv directly to detect syntax type
  • Shorthand: TASK-001 to alerts → converts to query id=TASK-001
  • Full syntax: where id=TASK-001 to alerts → parses query normally
  • Supports multi-word queries in full syntax

Testing

  • Type check passes
  • Unit tests pass (282 tests)
  • E2E notify workflow tests pass (5 tests)
  • New test for shorthand syntax added
  • Lint passes
  • Full CI pipeline passes

Validation

npm run type-check && npm test && npm run lint

All tests passing ✅

Issue

Fixes #853


Implementation completed following project standards and testing requirements

Tom Brandenburg added 2 commits February 1, 2026 17:36
Documentation showed shorthand syntax 'work notify send TASK-001 to alerts'
but implementation only accepted full syntax 'work notify send where <query> to <target>'.

Changes:
- Modified src/cli/commands/notify/send.ts to support both syntaxes
- Removed fixed arg definitions, using strict=false for flexible parsing
- Added argv parsing to detect shorthand vs full syntax
- Shorthand 'TASK-001 to alerts' is converted to 'id=TASK-001'
- Added test case for shorthand syntax in notify-workflow.test.ts
- Updated examples to show shorthand first

Fixes #853
Prevent malformed query 'id=' when task ID is empty.
Addresses code review feedback.
@tbrandenburg

Copy link
Copy Markdown
Owner Author

🔍 Automated Code Review

Summary

The implementation successfully adds shorthand syntax support for work notify send while maintaining backward compatibility. Code is clear, well-structured, and properly handles edge cases after the validation fix.

Findings

✅ Strengths

  • Backward compatibility preserved: Full syntax where <query> to <target> continues to work
  • Clear logic flow: Easy to understand shorthand vs full syntax detection
  • Comprehensive test coverage: New E2E test validates shorthand syntax works correctly
  • Good error messages: Helpful syntax hints provided when validation fails
  • Robust parsing: Handles multi-word queries and targets correctly by using .join(' ')

⚠️ Addressed Issues

  • Empty task ID validation: Added validation to prevent malformed query id= when task ID is empty (fixed in commit 641c138)

✅ Security

  • No security concerns identified
  • Input validation is appropriate
  • No injection risks with the query construction

✅ Code Quality

  • Follows project TypeScript standards
  • Clear comments explaining both syntax paths
  • Appropriate use of strict=false for flexible parsing

Checklist


Self-reviewed by Claude • Ready for human review

@tbrandenburg
tbrandenburg merged commit 5286d8f into main Feb 1, 2026
4 checks passed
@tbrandenburg
tbrandenburg deleted the dev branch February 1, 2026 18:29
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.

Check notification sending for one particular task

1 participant