-
-
Notifications
You must be signed in to change notification settings - Fork 316
fix(amp): add missing /auth/* and /api/tab/* proxy routes for AMP CLI #405
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix(amp): add missing /auth/* and /api/tab/* proxy routes for AMP CLI #405
Conversation
Summary of ChangesHello @thurstonsand, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request resolves critical 404 errors affecting the AMP CLI login process and the Amp Tab VS Code extension. It achieves this by adding specific root-level Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request correctly adds the missing proxy routes for /auth/* and /api/tab/* to fix issues with the AMP CLI login flow and VS Code extension. The changes are logical and follow the existing structure. I've provided two comments on the code changes: one suggesting a refactoring to improve maintainability, and another, more critical one, pointing out the lack of automated tests for the new routes. Please add test cases to prevent future regressions.
| ampAPI.Any("/tab", proxyHandler) | ||
| ampAPI.Any("/tab/*path", proxyHandler) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
With the addition of these routes, the list of proxy routes is getting long and repetitive. This pattern of registering a base path and a /*path variant is used for many routes in this block (lines 98-114). To improve maintainability and reduce code duplication, consider refactoring this block to register these routes in a loop. For example:
proxyPaths := []string{
"/internal",
"/user",
"/auth",
"/meta",
"/telemetry",
"/threads",
"/otel",
"/tab",
}
for _, path := range proxyPaths {
ampAPI.Any(path, proxyHandler)
ampAPI.Any(path+"/*path", proxyHandler)
}
ampAPI.Any("/ads", proxyHandler) // /ads doesn't have a /*path variant
Problem
When using CLI Proxy API with AMP CLI, two features fail with 404 errors:
amp loginflow - Returns 404 for/auth/cli-login:amp login
Logging in to localhost
...
http://localhost:8317/auth/cli-login?authToken=...
Visiting the URL returns 404.
/api/tab/llm-proxy:error getModelResponse Error calling Amp Tab Proxy API: HTTP error! status: 404
Root Cause
/auth/*routes are only registered under/api/auth/*, but AMP CLI uses root-level/auth/cli-loginfor the login flow/api/tab/*routes are not registered at allSolution
Add the missing proxy routes in
internal/api/modules/amp/routes.go:/auth/*routes for CLI login flow (with same security middleware as other management routes)/api/tab/*routes for Amp Tab functionalityAdditional Fix: Logging Panic
The new
/api/tab/*routes proxy autocomplete requests to ampcode.com. When users type in VS Code, stale autocomplete requests are canceled ("request aborted due to new request"), which causes Go's context to cancel mid-proxy. This triggers the proxy error handler → Gin's error logging → exposes a pre-existing bug inLogFormatter.LogFormatter.Format()assumedentry.Calleris always set, but log entries created through Gin's pipe writers (used forgin.DefaultErrorWriter) run in a separate goroutine without caller information. When accessingentry.Caller.Fileon a nil pointer → panic.So we just need to check for
nilcaller before dereferencing. Log entries without caller info now format without the[file:line]component.Changes
internal/api/modules/amp/routes.go: Added/auth,/auth/*path,/api/tab, and/api/tab/*pathproxy routesinternal/logging/global_logger.go: Handlenilcaller inLogFormatter.Format()Testing
Verified both fixes by running locally:
amp loginnow opens the browser and completes authentication flowAppendix: Full Panic Log