Skip to content

Migration logistic-sentry#147

Closed
KrishnaAgarwal7531 wants to merge 3 commits intotinyfish-io:mainfrom
KrishnaAgarwal7531:mig-logistic-sentry
Closed

Migration logistic-sentry#147
KrishnaAgarwal7531 wants to merge 3 commits intotinyfish-io:mainfrom
KrishnaAgarwal7531:mig-logistic-sentry

Conversation

@KrishnaAgarwal7531
Copy link
Copy Markdown
Contributor

No description provided.

@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented Apr 8, 2026

Important

Review skipped

Auto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro

Run ID: e56bcf75-6355-4366-8cbb-f0c38dea1c61

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • 🔍 Trigger review
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Copy Markdown
Contributor

@simantak-dabhade simantak-dabhade left a comment

Choose a reason for hiding this comment

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

🚨 Security: same leaked key as #132 — merge blocked

Second occurrence of the same TinyFish API key in as many PRs. logistics-sentry/.env.local was committed in a95557e (2026-04-09) with the identical key string we already flagged on PR #132. Treat the key as compromised (if it wasn't already) — both fork branches expose it publicly.

This is now a pattern, not a one-off. Your local workflow is creating .env.local with the real key and letting it get staged because neither the recipe nor the repo has a .gitignore covering it. Before any further PRs, please confirm with @simantak-dabhade that the key was rotated (he has the earlier screenshot request from #132), and adjust your local flow.

Security steps before we can continue review

  1. Confirm rotation — verify with Simantak out-of-band that the key is dead (the one shared across #132 and #147).
  2. Delete logistics-sentry/.env.local from this branch.
  3. Add logistics-sentry/.gitignore (and the repo-wide one is coming separately) covering .env, .env.local, .env.*.local, node_modules/, .next/.
  4. Force-push to rewrite history so the key isn't in the branch log (git reset --soft <parent> + recommit without .env.local, then git push --force-with-lease).
  5. Fix your local dev flow so future PRs don't keep doing this. A one-line check before pushing: git diff --cached --name-only | grep -E '\.env($|\.)' catches this. Or set up a personal pre-commit hook.

Also noted (not blocking — flagged for when security lifts)

Since I'm here, other things I saw while checking:

  • env.example should be .env.example. Currently named env.example (no leading dot). Playbook requires the dotfile form. Rename.
  • .env.example has no get-your-key comment — just TINYFISH_API_KEY=your_tinyfish_api_key_here. Match the format used in #129 tutor-finder / #134 wing-command.
  • PR body is empty — describe what was migrated.
  • .js files instead of .ts — unusual for the cookbook but it's how the recipe was originally; not a blocker.

Full migration review of the code deferred until the security block lifts. README exists, SDK import is in place (src/lib/logistics/agent.js), Supabase not present — code side will probably review fast once the secret is handled.

@simantak-dabhade
Copy link
Copy Markdown
Contributor

Status update: @simantak-dabhade has confirmed the key was rotated (same one was on #132). The exposed sk-mino-t0y8BUE216q0n4T7Jsm7MAXdNNbIGvYv is now dead.

Still need to complete the cleanup before full review can continue:

  1. Rotate the key (done by @simantak-dabhade)
  2. Delete logistics-sentry/.env.local from this branch.
  3. Add logistics-sentry/.gitignore covering at least:
    .env
    .env.local
    .env.*.local
    node_modules/
    .next/
    
  4. Force-push to rewrite history:
    git reset --soft HEAD~1
    git restore --staged logistics-sentry/.env.local
    rm logistics-sentry/.env.local
    git commit -m "Migration logistic-sentry"
    git push --force-with-lease

And please adjust your local flow so this doesn't happen a third time — git diff --cached --name-only | grep -E '\.env($|\.)' before every push would have caught both occurrences.

(Scanned the rest of your open PRs — #129, #131, #134#140, #144 are all clean. Only #132 and this one carried the leaked key.)

Once the file is gone from history and the .gitignore is in, I'll proceed with the full migration review. The other items from my earlier review still stand — env.example.env.example rename, add get-your-key URL comment, empty PR body.

@simantak-dabhade
Copy link
Copy Markdown
Contributor

Full migration review (in addition to the security block above)

Now that the key is rotated, I went through the rest of the PR. Good news: the code-side migration is actually really clean. Build passes — second PR in the batch where that's true (after #136 viet-bike-scout).

✅ What's working well

  • All three API routes use the SDK idiomatically:
    • src/lib/tinyfish.js — typed EventType.COMPLETE + RunStatus.COMPLETED, explicit signal passthrough for cancellation, direct SSE passthrough.
    • src/lib/pricing-intelligence.js — concurrency-limited parallel agents (MAX_CONCURRENCY = 5), per-agent AbortController with 35s timeout, custom SSE-to-SSE proxy, upfront input validation (MAX_URLS = 10, URL parsing).
    • src/lib/logistics/agent.jsSOURCE_KNOWLEDGE_BASE fallback pattern for port/carrier URL resolution before falling back to Search API.
  • Educational comments. tinyfish.js:80-82// COMPLETED only means the browser ran without crashing — always validate result content, not just the status. That's the kind of nuance we want in cookbook recipes.
  • No Mino refs. The earlier grep hits (Minor Congestion, Minor disruptions) were all false positives in content strings.
  • No Supabase, no env leak (other than the already-flagged .env.local).
  • README structure (the part that exists) is well-written with proper mermaid diagrams for the intelligence lifecycle + risk decision logic.

🔴 Additional blockers beyond the security one

  1. README is truncated mid-sentence. Ends at line 118 with:

    2. Create a `.env.local` file:
    

    Followed by an empty code block, then nothing. Steps 3-5 (env values, npm run dev, open browser) are missing. Please complete it.

  2. env.example should be .env.example. Missing the leading dot. Playbook requires the dotfile form. All other krishna migrations got this right; this one regressed. Once renamed, also add the get-your-key URL comment:

    # TinyFish Web Agent API key (server-side only)
    # Get yours at: https://agent.tinyfish.ai/api-keys
    TINYFISH_API_KEY=
    

    Match the format in Migration wing-command to Tinyfish SDK  #134 wing-command.

  3. Package name mismatch. package.json has "name": "inventory-risk-agent" but the folder is logistics-sentry. Rename the package to logistics-sentry for consistency with the repo's migration state table (and with the recipe's folder).

🟡 Nits (not blocking)

  • PR body is empty.
  • Next 14.2.3 — outdated vs pack, repo-wide sweep will handle.
  • .js not .ts — pre-existing, not a regression. Not going to push for a TS conversion in this PR.

Summary

Order of operations from here:

  1. Security cleanup (rotation done; still need .env.local deletion + .gitignore + force-push).
  2. Finish README.
  3. Rename env.example.env.example, add key URL comment.
  4. Update package.json name.

Once those land I'll run the local test pass and approve.

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.

3 participants