Skip to content
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

Update dependencies, add API key support, and enhance code execution #716

Merged
merged 11 commits into from
Sep 18, 2024

Conversation

pelikhan
Copy link
Member

@pelikhan pelikhan commented Sep 18, 2024

This pull request includes updates to dependencies, such as the zx package, and adds API key support for server authentication in CLI commands and server logic. It also enhances code execution by simplifying the logic and adjusting array checking. Additionally, it includes instructions for building custom Docker images and improves error handling in the WebSocket server.

  • Upgraded Dependencies and Packages: Various dependencies and packages were upgraded across multiple package.json files, most notably the upgrade of @astrojs/starlight from 0.27.0 to 0.28.1, and the astro package from 4.15.4 to 4.15.7 for improved functionality and compatibility. 🚀
  • Changes to genaiscript: Tweaks were made to genaiscript files like optillm.genai.mts, gcm.genai.mts, and some others, mainly adjusting how different commands are called, and updates in authentication processes.
  • Refactoring in DockerManager: The DockerManager class received a revamp in how it handles command executions with the introduction of a new exec function and other associated changes.
  • Code Refactoring: A new shell.ts file was introduced to handle shell related operations, leading to a removal of the quoteify function. Furthermore, changes were made to how commands are executed and handled in the cli, nodehost, and TaskProvider classes. 🔄
  • API Changes: The exec() function now supported a new command format - where instead of a string representing the command and an array of string arguments, it could take in a complete command line as a string and an options object. This change was reflected directly in the public API. 💡
  • Server Security Update: In the module handling server start ups, authentication was introduced, where an API key is checked before proceeding to service connections. This allowed for more controlled and secure interactions. 🔐
  • Content Alterations: Some markdown and content files also saw restructuring in terms of content organization and presentation.

generated by pr-describe

@@ -34,7 +34,28 @@ const container = await host.container()
By default, the container uses the [python:alpine](https://hub.docker.com/_/python/) image, which provides a minimal python environment. You can change the image using the `image` option.

```js 'image: "python:3"'
const container = await host.container({ image: "python:3" })
const container = await host.container({ image: "node:20" })

Choose a reason for hiding this comment

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

The image option in the container instantiation should be consistent with the default mentioned in the text.

generated by pr-docs-review-commit image_inconsistency

Copy link

The pull request covers the following changes:

  1. Addition of an API key authentication mechanism to the server, with a new CLI option -k, --api-key <string> added to provide the key at startup. 🔒
  2. Refactoring of the shell command execution (exec function) in a few places. The command and arguments are now parsed and quoted more robustly. 🛠️
  3. Deletion of the quoteify function from util.ts and moving its functionality to a new shell.ts file, with the addition of a shellParse function.
  4. The exec method in ShellHost interface has been overloaded to accept a single string argument for command with arguments, and an options argument.

It seems like a good effort has been made to improve the way the shell commands are handled to be more robust and flexible. The addition of the API key authentication adds an extra layer of security to the server.

However, there are potential concerns:

  1. In server.ts, it appears that if a client does not provide the correct API key, the WebSocket connection is simply closed with a 401 error, but there doesn't seem to be a mechanism to inform the client of why the connection was closed. A more informative error message might be helpful to the client.

Here are some suggested changes:

- ws.close(401, "Unauthorized")
+ ws.close(401, "Unauthorized: Invalid API Key provided.")
  1. The refactoring of shell command execution (exec function) in a few places might potentially introduce some unforeseen bugs, especially as the command parsing and quoting change quite drastically compared to before. Therefore, thorough testing will be required to ensure that the changes work as expected across different platforms and scenarios.

Other than these concerns, the code changes look good.

LGTM 🚀

generated by pr-review

@pelikhan pelikhan merged commit a54fe98 into main Sep 18, 2024
9 of 10 checks passed
@pelikhan pelikhan deleted the shellquote branch September 18, 2024 18:38
"--abbrev=0",
"HEAD^",
])
const { stdout: tag } = await host.exec("git describe --tags --abbrev=0 HEAD^")

Choose a reason for hiding this comment

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

Inline code should be enclosed within backticks, not a code block.

generated by pr-docs-review-commit code_fence

"--no-merges",
`HEAD...${tag}`,
])
const { stdout: commits } = await host.exec(`git log --grep='skip ci' --invert-grep --no-merges HEAD...${tag}`)

Choose a reason for hiding this comment

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

Inline code should be enclosed within backticks, not a code block.

generated by pr-docs-review-commit code_fence

":!*yarn.lock",
":!*THIRD_PARTY_NOTICES.md",
])
const { stdout: diff } = await host.exec(`git diff ${tag}..HEAD --no-merges -- ':!**/package.json' ':!**/genaiscript.d.ts' ':!**/jsconfig.json' ':!docs/**' ':!.github/*' ':!.vscode/*' ':!*yarn.lock' ':!*THIRD_PARTY_NOTICES.md'`)

Choose a reason for hiding this comment

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

Inline code should be enclosed within backticks, not a code block.

generated by pr-docs-review-commit code_fence


const { stdout: commits } = await host.exec(`git log HEAD...${tag}`)

const { stdout: diff } = await host.exec(`git diff ${tag}..HEAD`)

Choose a reason for hiding this comment

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

Inline code should be enclosed within backticks, not a code block.

generated by pr-docs-review-commit code_fence

@@ -34,7 +34,28 @@ const container = await host.container()
By default, the container uses the [python:alpine](https://hub.docker.com/_/python/) image, which provides a minimal python environment. You can change the image using the `image` option.

```js 'image: "python:3"'
const container = await host.container({ image: "python:3" })
const container = await host.container({ image: "node:20" })

Choose a reason for hiding this comment

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

The example should use the same image as the description, which is 'python:3', not 'node:20'.

generated by pr-docs-review-commit image_example

const dir = "."
await host.exec(
`docker build -t ${repo} https://github.com/${repo}.git#${branch}:${dir}`
)

Choose a reason for hiding this comment

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

Direct execution of build commands in documentation should be replaced with a more generic description or wrapped in a code block without direct execution instructions.

generated by pr-docs-review-commit code_execution

then use repo as your image name

```js
const container = await host.container({ image: repo, ... })

Choose a reason for hiding this comment

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

The variable 'repo' is used without being defined in the current context. It should be defined or the example should be adjusted to avoid confusion.

generated by pr-docs-review-commit variable_usage

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.

1 participant