-
Notifications
You must be signed in to change notification settings - Fork 411
feat: add ai command for cli to start the project #7352
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
base: main
Are you sure you want to change the base?
feat: add ai command for cli to start the project #7352
Conversation
@@ -0,0 +1,447 @@ | |||
import { resolve } from 'node:path' |
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.
part of code here is duplication, but we still not setelled the command and want to see how it feels so then we can extract stuff into common utils after we work on making command fully public
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.
I'm not sure why trialing the new command should mean more code duplication? If you move the generic methods you're reusing to a separate file (which I would argue should already be the case), you're making it easier for this PR to be reviewed, you're going to have less code to maintain, and you reduce the number of places where you might need to adjust things if an issue or an improvement come up.
If you then decide that you want to keep the new command in its current shape, great — you already have the code in the structure you want and no further changes are needed; if you decide you want to bin it, you can just delete it and the code you abstracted away into a separate file still makes sense on its own.
|
||
// Step 4: Save AI instructions to file | ||
if (projectInfo.prompt) { | ||
const ntlContext = await fetch( |
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.
in future we don't wanna fetch context if we installing MCP
@@ -0,0 +1,447 @@ | |||
import { resolve } from 'node:path' |
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.
I'm not sure why trialing the new command should mean more code duplication? If you move the generic methods you're reusing to a separate file (which I would argue should already be the case), you're making it easier for this PR to be reviewed, you're going to have less code to maintain, and you reduce the number of places where you might need to adjust things if an issue or an improvement come up.
If you then decide that you want to keep the new command in its current shape, great — you already have the code in the structure you want and no further changes are needed; if you decide you want to bin it, you can just delete it and the code you abstracted away into a separate file still makes sense on its own.
src/commands/init/ai-rules.ts
Outdated
} | ||
|
||
// Generate MCP configuration for the detected IDE | ||
const generateMcpConfig = (ide: ConsumerConfig): string => { |
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.
Why are you returning the config as a string here only to then re-parse it into an object in configureMcpForVSCode
?
src/commands/init/ai-rules.ts
Outdated
await fs.writeFile(configPath, JSON.stringify(updatedConfig, null, 2), 'utf-8') | ||
log(`${chalk.green('✅')} VS Code MCP configuration saved to ${chalk.cyan('.vscode/mcp.json')}`) | ||
} catch (error) { | ||
throw new Error(`Failed to configure VS Code MCP: ${error instanceof Error ? error.message : 'Unknown error'}`) |
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.
When will error
not be an instance of Error
?
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.
it's always unknown, but here yeah it will be Error, it's more following code pattern across repo
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.
I see a couple of instances of that, which doesn't feel like enough of a reason to perpetuate this and just duplicate this code every time we handle an error, especially when we know for a fact that the type will be Error
.
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.
This is a normal idiom--TypeScript can't guarantee caught values are errors.
src/commands/init/ai-rules.ts
Outdated
|
||
// VS Code specific MCP configuration | ||
const configureMcpForVSCode = async (config: string, projectPath: string): Promise<void> => { | ||
const configPath = resolve(projectPath, '.vscode', 'mcp.json') |
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.
nit: Save the path to the .vscode
directory in a variable to avoid resolving the path multiple times.
src/commands/init/ai-rules.ts
Outdated
|
||
// Windsurf specific MCP configuration | ||
const configureMcpForWindsurf = async (config: string, _projectPath: string): Promise<void> => { | ||
const { homedir } = await import('node:os') |
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.
Why are you using a dynamic import here?
src/commands/init/ai-rules.ts
Outdated
const { api } = command.netlify | ||
|
||
log(`${chalk.blue('🤖 Initializing AI project')} with rules...`) | ||
log(`${chalk.gray('Hash:')} ${hash}`) |
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.
Do we have to print the hash? Why do people care?
src/commands/init/ai-rules.ts
Outdated
let mcpConfigured = false | ||
|
||
if (detectedIDE) { | ||
log(`${chalk.green('✅')} Detected IDE: ${chalk.cyan(detectedIDE.presentedName)}`) |
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.
Can we settle on either "development environment" or "IDE"? Using both next to each other feels unnecessarily confusing.
src/commands/init/ai-rules.ts
Outdated
} else { | ||
log(chalk.yellowBright(`🔧 Step 2: Manual MCP Configuration`)) | ||
log(` ${chalk.cyan(detectedIDE.key)} detected - MCP setup was skipped`) | ||
log(` ${chalk.gray('You can configure MCP manually later for enhanced AI capabilities')}`) |
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.
How?
Co-authored-by: Eduardo Bouças <mail@eduardoboucas.com>
Co-authored-by: Eduardo Bouças <mail@eduardoboucas.com>
Co-authored-by: Eduardo Bouças <mail@eduardoboucas.com>
Co-authored-by: Eduardo Bouças <mail@eduardoboucas.com>
Co-authored-by: Eduardo Bouças <mail@eduardoboucas.com>
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.
Approving so you can get unblocked, but I left some notes and questions that I think we should be addressed.
log(`${chalk.green('✅')} AI instructions saved to ${chalk.cyan('AI-instructions.md')}`) | ||
} catch (error) { | ||
const errorMessage = error instanceof Error ? error.message : 'Unknown error' | ||
log(`${chalk.yellow('⚠️')} Warning: Failed to save AI instructions: ${errorMessage}`) |
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.
The CLI has a warn
method precisely for this purpose:
cli/src/utils/command-helpers.ts
Lines 185 to 188 in f449ad8
export const warn = (message = '') => { | |
const bang = chalk.yellow(BANG) | |
log(` ${bang} Warning: ${message}`) | |
} |
* Generate MCP configuration for the detected IDE or development environment | ||
*/ | ||
export const generateMcpConfig = (ide: ConsumerConfig): Record<string, unknown> => { | ||
const configs: Record<string, Record<string, unknown>> = { |
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.
It would be great to properly type this.
} | ||
} | ||
|
||
const fetchProjectInfo = async (url: string): Promise<ProjectInfo> => { |
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.
It would be great to have a comment explaining what is the URL we're hitting — is it the Netlify API, something else?
|
||
const getRepoUrlFromProjectId = async (api: NetlifyAPI, projectId: string): Promise<string> => { | ||
try { | ||
const siteInfo = (await api.getSite({ siteId: projectId })) as SiteInfo |
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.
Do we not already have the site information populated by the base command? Or is this a different site? If we could use that, we'd avoid making an extra API call.
{ | ||
method: 'GET', | ||
headers: { | ||
'Content-Type': 'text/plain', |
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.
Why are you setting Content-Type
on a GET request? Did you mean to use Accept
?
} | ||
|
||
// Update working directory to cloned repo | ||
process.chdir(targetDir) |
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.
What is the purpose of this?
Awesome 🙌 thank you |
…i-to-start-the-project
…i-to-start-the-project
…5-add-ai-command-for-cli-to-start-the-project
…mmand-for-cli-to-start-the-project' into artemdenysov/wrfl-2615-add-ai-command-for-cli-to-start-the-project
response.statusCode = 500; | ||
if (acceptsHtml) { | ||
response.setHeader('Content-Type', 'text/html'); | ||
response.end(await renderErrorTemplate(errorString, '../../src/lib/templates/function-error.html', 'function')); |
Check warning
Code scanning / CodeQL
Information exposure through a stack trace Medium
stack trace information
response.end(await renderErrorTemplate(errorString, '../../src/lib/templates/function-error.html', 'function')); | ||
} | ||
else { | ||
response.end(errorString); |
Check warning
Code scanning / CodeQL
Information exposure through a stack trace Medium
stack trace information
res.end(`${"<html><head><title>Logged in</title><script>if(history.replaceState){history.replaceState({},'','/')}</script><style>html{font-family:system-ui, -apple-system, BlinkMacSystemFont, 'Segoe UI', Roboto, Helvetica, Arial, sans-serif, 'Apple Color Emoji', 'Segoe UI Emoji', 'Segoe UI Symbol';line-height:1.5;background:rgb(18 24 31)}body{overflow:hidden;position:relative;display:flex;flex-direction:column;align-items:center;justify-content:center;height:100vh;width:100vw;}h3{margin:0}p{margin: 1rem 0 0.5rem}.card{position:relative;display:flex;flex-direction:column;width:75%;max-width:364px;padding:24px;background:white;color:rgb(18 24 31);border-radius:8px;box-shadow:rgb(6 11 16 / 20%) 0px 16px 24px, rgb(6 11 16 / 30%) 0px 6px 30px, rgb(6 11 16 / 40%) 0px 8px 10px;}</style></head>" + | ||
"<body><div class=card><h3>Logged in</h3><p>You're now logged into Netlify CLI with your "}${parameters.get('provider') ?? ''} credentials. Please close this window.</p></div>`); |
Check failure
Code scanning / CodeQL
Reflected cross-site scripting High
user-provided value
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 5 days ago
To fix the reflected cross-site scripting vulnerability, the user-controlled input (parameters.get('provider')
) must be sanitized before being incorporated into the HTML response. The best approach is to use a library like escape-html
to escape special characters in the input, ensuring that it cannot be interpreted as executable code by the browser.
Steps to fix:
- Import the
escape-html
library at the top of the file. - Use
escape-html
to sanitize the value ofparameters.get('provider')
before embedding it in the HTML response.
This fix ensures that the functionality remains unchanged while eliminating the XSS vulnerability.
-
Copy modified line R10 -
Copy modified line R39
@@ -9,2 +9,3 @@ | ||
import openBrowser from './open-browser.js'; | ||
import escapeHtml from 'escape-html'; | ||
const SERVER_PORT = 3000; | ||
@@ -37,3 +38,3 @@ | ||
res.end(`${"<html><head><title>Logged in</title><script>if(history.replaceState){history.replaceState({},'','/')}</script><style>html{font-family:system-ui, -apple-system, BlinkMacSystemFont, 'Segoe UI', Roboto, Helvetica, Arial, sans-serif, 'Apple Color Emoji', 'Segoe UI Emoji', 'Segoe UI Symbol';line-height:1.5;background:rgb(18 24 31)}body{overflow:hidden;position:relative;display:flex;flex-direction:column;align-items:center;justify-content:center;height:100vh;width:100vw;}h3{margin:0}p{margin: 1rem 0 0.5rem}.card{position:relative;display:flex;flex-direction:column;width:75%;max-width:364px;padding:24px;background:white;color:rgb(18 24 31);border-radius:8px;box-shadow:rgb(6 11 16 / 20%) 0px 16px 24px, rgb(6 11 16 / 30%) 0px 6px 30px, rgb(6 11 16 / 40%) 0px 8px 10px;}</style></head>" + | ||
"<body><div class=card><h3>Logged in</h3><p>You're now logged into Netlify CLI with your "}${parameters.get('provider') ?? ''} credentials. Please close this window.</p></div>`); | ||
"<body><div class=card><h3>Logged in</h3><p>You're now logged into Netlify CLI with your "}${escapeHtml(parameters.get('provider') ?? '')} credentials. Please close this window.</p></div>`); | ||
server.close(); |
-
Copy modified lines R153-R154
@@ -152,3 +152,4 @@ | ||
"write-file-atomic": "5.0.1", | ||
"ws": "8.18.2" | ||
"ws": "8.18.2", | ||
"escape-html": "^1.0.3" | ||
}, |
Package | Version | Security advisories |
escape-html (npm) | 1.0.3 | None |
const isEndpointExists = async function (endpoint, origin) { | ||
const url = new URL(endpoint, origin); | ||
try { | ||
const res = await fetch(url, { method: 'HEAD' }); |
Check failure
Code scanning / CodeQL
Server-side request forgery Critical
URL
user-provided value
The
URL
user-provided value
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 5 days ago
To fix the issue, we need to ensure that user-controlled input (req.url
) does not directly influence the hostname or path of the outgoing request. This can be achieved by implementing an allow-list of valid endpoints or paths and validating the endpoint
against this allow-list before constructing the url
object.
Steps to fix:
- Introduce an allow-list of valid endpoints or paths that the server is allowed to query.
- Validate the
endpoint
against this allow-list before constructing theurl
object. - Reject or sanitize any
endpoint
value that does not match the allow-list.
-
Copy modified lines R116-R120
@@ -115,2 +115,7 @@ | ||
const isEndpointExists = async function (endpoint, origin) { | ||
const allowedEndpoints = ['/valid-path-1', '/valid-path-2']; // Define allow-list of valid paths | ||
if (!allowedEndpoints.includes(endpoint)) { | ||
console.warn(NETLIFYDEVWARN, `Blocked attempt to access invalid endpoint: ${endpoint}`); | ||
return false; | ||
} | ||
const url = new URL(endpoint, origin); |
🎉 Thanks for submitting a pull request! 🎉
Summary
Fixes #<replace_with_issue_number>
For us to review and ship your PR efficiently, please perform the following steps:
passes our tests.
A picture of a cute animal (not mandatory, but encouraged)