Add Fireflies.ai skill for querying meeting transcripts#41
Conversation
Python CLI tool using uv inline script dependencies for zero-setup execution. Features: - Query recent, today, or date-specific meetings - Search transcripts by keyword - Get full transcript with speaker attribution - Smart .env file loading from multiple locations - Beautiful rich terminal output - Helpful setup instructions when API key missing Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
Your free trial has ended. If you'd like to continue receiving code reviews, you can add a payment method here.
Points LLMs to Context7 for GraphQL API lookups with full schema docs Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
PR Review: Fireflies.ai SkillOverviewThis PR adds a well-crafted skill for querying Fireflies.ai meeting transcripts. The implementation is clean, functional, and follows modern Python best practices. ✅ StrengthsCode Quality
Documentation
Security & Best Practices
🔍 Issues & Recommendations1. Input Validation Missing (Medium Priority)The def cmd_date(date_str: str) -> None:
"""Get meetings for a specific date."""
from_date = f"{date_str}T00:00:00.000Z"
to_date = f"{date_str}T23:59:59.999Z"Issue: Invalid dates like Recommendation: Add date validation: from datetime import datetime
def cmd_date(date_str: str) -> None:
"""Get meetings for a specific date."""
try:
datetime.strptime(date_str, "%Y-%m-%d")
except ValueError:
error("Invalid date format. Use YYYY-MM-DD (e.g., 2026-01-28)")
sys.exit(1)
from_date = f"{date_str}T00:00:00.000Z"
to_date = f"{date_str}T23:59:59.999Z"
# ...2. Integer Overflow Risk (Low Priority)In case "recent":
limit = int(args[1]) if len(args) > 1 else 5
cmd_recent(limit)Issue: No validation that the integer is reasonable. User could pass
Recommendation: Add bounds checking: case "recent":
limit = int(args[1]) if len(args) > 1 else 5
if limit < 1 or limit > 100:
error("Limit must be between 1 and 100")
sys.exit(1)
cmd_recent(limit)3. Error Handling for Invalid Integer (Low Priority)Same location - no try/except for limit = int(args[1]) if len(args) > 1 else 5Issue: Recommendation: try:
limit = int(args[1]) if len(args) > 1 else 5
except ValueError:
error('Usage: fireflies recent [N] - N must be a number')
sys.exit(1)4. GraphQL Error Handling Could Be More Granular (Low Priority)Currently all GraphQL errors are caught broadly at plugins/core/skills/fireflies/scripts/fireflies:177-186. API might return errors in the response body even with 200 status. Recommendation: Consider checking for GraphQL errors in the response: result = response.json()
if "errors" in result:
error(f"GraphQL error: {result['errors']}")
return None
return result5. Documentation: Missing Version Note (Low Priority)The SKILL.md frontmatter has 6. Test Coverage (Note)The PR description mentions:
The last item is still unchecked. While this is understandable for a PR, it would be good to validate with a real API key before merging. 🎯 Performance Considerations
🔒 Security ConsiderationsOverall: Good security posture
📝 Code Style & ConventionsMatches repository patterns well:
Minor suggestion: The description in SKILL.md:4 could be slightly more concise: # Current:
description: "Use when finding meeting transcripts, searching Fireflies recordings, getting action items from calls, or answering 'what was discussed in the meeting' questions"
# Suggestion:
description: "Use when querying meeting transcripts, searching Fireflies recordings, extracting action items, or answering 'what was discussed' questions"🎨 UX ConsiderationsExcellent UX overall:
Enhancement idea: Consider adding a progress indicator for slow API calls, though 30s timeout is reasonable for most queries. 🧪 Testing RecommendationsBefore merging:
VerdictRecommendation: Approve with minor changes This is a solid implementation that adds valuable functionality. The issues identified are minor and easy to fix: Must fix before merge:
Nice to have:
Overall score: 8.5/10 - Well-structured, modern Python with good UX. Minor input validation improvements would make this production-ready. Great work on the zero-dependency setup with uv and the helpful onboarding experience! 🎉 |
Code Review: Fireflies.ai SkillOverall, this is a well-crafted addition with clean code, good documentation, and thoughtful user experience. Here's my detailed feedback: ✅ StrengthsExcellent Documentation:
Modern Python Implementation:
Great UX:
🐛 Potential Issues1. Input Validation Missing (Medium Priority) The script doesn't validate user input, which could cause runtime errors:
2. Silent JSON Parsing Failure (Low Priority) At line 42-47, JSON parsing errors are caught but continue silently. While this works for loading config files, it might hide configuration issues. Consider logging a warning: except (json.JSONDecodeError, KeyError) as e:
# Silent failure - consider logging for debugging
console.print(f"[dim]Warning: Could not parse {env_path}: {e}[/dim]", file=sys.stderr)
pass3. Timezone Hardcoded to UTC (Low Priority) The # Current uses UTC:
today = datetime.now(timezone.utc).replace(...)
# Consider local time:
today = datetime.now().replace(hour=0, minute=0, second=0, microsecond=0)Or document in SKILL.md that dates are in UTC. 4. GraphQL Error Response Not Checked (Medium Priority) The # After line 176:
result = response.json()
if "errors" in result:
error(f"GraphQL errors: {result['errors']}")
return None
return result🔒 Security ConsiderationsAPI Key Exposure:
⚡ Performance Considerations
🧪 Test CoverageTest Plan Status:
Recommendations:
📋 Best PracticesFollows Project Conventions:
Python Style:
🎯 Suggestions for Enhancement (Optional)
📝 Documentation Updates Needed
Recommendation: Approve with minor fixes The critical issues are input validation and GraphQL error checking. These are straightforward fixes that would make the script more robust. Everything else is excellent quality. Great work on the user experience - the setup instructions and multi-location .env loading are particularly thoughtful touches! 🔥 |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 87f6b1ab25
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| try: | ||
| config = json.loads(env_path.read_text()) | ||
| env_vars = config.get("env", {}) | ||
| if "FIREFLIES_API_KEY" in env_vars: | ||
| return env_vars["FIREFLIES_API_KEY"] |
There was a problem hiding this comment.
The loader returns immediately when clawdbot.json contains FIREFLIES_API_KEY, so the later locations in ENV_LOCATIONS (e.g., project .env / .env.local) never get a chance to override that value. This contradicts the stated priority order and will pick the wrong key in the common case where a user keeps a global clawdbot config but needs a project-specific Fireflies key. Consider deferring the return until after scanning all locations, or recording the value and allowing later sources to overwrite it.
Useful? React with 👍 / 👎.
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
- Fix env loading priority (Codex P2) - don't return early from JSON config - Add GraphQL error checking for better API error messages - Add date format validation with helpful error message - Add integer validation for limit parameter - Add rate limit error handling - Add JSON parse warning for malformed config files Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
PR Review: Add Fireflies.ai skill for querying meeting transcriptsThis is a well-crafted PR that adds a valuable skill to the project. The code demonstrates excellent Python style and follows modern patterns. Below are my findings organized by category. ✅ StrengthsExcellent Code Quality
Good Security Practices
Strong SKILL.md Documentation
🔴 Security Issues1. Sensitive Data Exposure - API Error Logging (Medium Severity)Location: Error messages include full API response text which could leak sensitive information: error(f"API error: {e.response.status_code} - {e.response.text}")Recommendation: Sanitize error messages: error(f"API error: {e.response.status_code}")
# Don't include e.response.text2. Setup Instructions Missing Security Warnings (Low Severity)Location: The setup instructions teach users to store API keys in plaintext without warning about file permissions or .gitignore. Recommendation: Add security notes: [yellow]Security Note: Never commit .env files to version control. Ensure .env is in .gitignore.[/yellow]
Option B: Add to ~/.env (recommended for persistent use)
[dim]echo "FIREFLIES_API_KEY=your-api-key-here" >> ~/.env
chmod 600 ~/.env # Restrict file permissions[/dim]🟡 Logic Bugs1. GraphQL Error Array Indexing Without Check (Medium Severity)Location: Code assumes errors array has elements: if "errors" in result:
error(f"GraphQL error: {result['errors'][0].get('message', 'Unknown error')}")Fix: if "errors" in result:
if result['errors']:
error(f"GraphQL error: {result['errors'][0].get('message', 'Unknown error')}")
else:
error("GraphQL error: Unknown error (empty errors array)")
return None2. Fragile Timezone String Manipulation (Medium Severity)Location: Using "fromDate": today.isoformat().replace("+00:00", ".000Z"),Fix: "fromDate": today.strftime("%Y-%m-%dT%H:%M:%S.000Z"),3. Missing Validation for Positive Limit (Low Severity)Location:
Fix: try:
limit = int(args[1]) if len(args) > 1 else 5
if limit <= 0:
error("Limit must be a positive number")
sys.exit(1)
except ValueError:
error(f"Invalid limit: '{args[1]}'. Must be a number.")
sys.exit(1)📝 SKILL.md Improvements1. Expand Trigger PhrasesThe triggers array is good but missing natural user language: triggers:
- "fireflies"
- "meeting transcript"
- "meeting notes"
- "what was discussed"
- "action items"
- "zoom call"
- "teams meeting"
- "google meet"
# Add these:
- "call notes"
- "what happened in the meeting"
- "meeting summary"
- "recording"
- "standup"
- "sync"
- "retro"2. Remove Redundancy in
|
| Category | Status |
|---|---|
| Code Quality | ✅ Excellent |
| Security | 🟡 2 issues (1 medium, 1 low) |
| Logic | 🟡 3 bugs (2 medium, 1 low) |
| Documentation | ✅ Good with minor improvements |
| Test Coverage | 🔴 Missing |
| Style/Conventions | ✅ Excellent |
Recommendation
This PR adds valuable functionality with well-written code. The issues found are fixable and mostly low-risk:
Must fix before merge:
- GraphQL error array indexing bug (could cause crashes)
- Timezone string manipulation bug (causes incorrect API queries)
Should fix before merge:
3. Sanitize error messages to prevent data leakage
4. Validate positive limit in cmd_recent
5. Add security warnings to setup instructions
Nice to have:
6. Expand SKILL.md trigger phrases
7. Add common-pitfalls section
8. Add test coverage
Once the "must fix" items are addressed, this is ready to merge. Great work on the implementation!
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 3 potential issues.
Bugbot Autofix is OFF. To automatically fix reported issues with Cloud Agents, enable Autofix in the Cursor dashboard.
| else: | ||
| load_dotenv(env_path, override=True) | ||
|
|
||
| return os.environ.get("FIREFLIES_API_KEY") |
There was a problem hiding this comment.
Environment variable priority inverted from documented behavior
Medium Severity
The load_api_key docstring claims environment variables have "highest priority" (item 5), but the implementation using load_dotenv(env_path, override=True) causes .env files to overwrite pre-existing environment variables. If a user explicitly sets export FIREFLIES_API_KEY=xxx and ~/.env contains a different value, the .env value wins instead. The actual priority is reversed from what's documented.
| variables = args[2] if len(args) > 2 else "{}" | ||
| cmd_raw(args[1], variables) | ||
| case "help" | "-h" | "--help" | _: | ||
| show_help() |
There was a problem hiding this comment.
Unknown commands silently show help without error message
Low Severity
The match statement uses _ as a catch-all pattern combined with help commands, so any unrecognized command like fireflies tyop silently displays help instead of showing an "unknown command" error. Users won't know their command wasn't recognized and may be confused why their intended action didn't execute.
| return None | ||
| except httpx.RequestError as e: | ||
| error(f"Request failed: {e}") | ||
| return None |
There was a problem hiding this comment.
Missing JSON decode error handling causes unhandled crash
Medium Severity
The graphql() function calls response.json() without catching json.JSONDecodeError. While httpx.HTTPStatusError and httpx.RequestError are handled, if the API returns a non-JSON response (HTML error page, truncated response, or malformed JSON) with HTTP 200, the script crashes with an unhandled exception. Other JSON parsing in this file (lines 50-54, 368-369) properly catches this error, making this an oversight.
1 participant
Summary
Adds a new skill for querying Fireflies.ai meeting transcripts - recorded meetings with AI-generated summaries, action items, and searchable conversation history.
Features:
Test plan
helpcommand🤖 Generated with Claude Code