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

fix missing globals #611

Merged
merged 7 commits into from
Aug 8, 2024
Merged

fix missing globals #611

merged 7 commits into from
Aug 8, 2024

Conversation

pelikhan
Copy link
Member

@pelikhan pelikhan commented Aug 8, 2024

  • In the 'crypto.genai.test.ts' file, the structure of the 'randomHex function' tests has been reshuffled and enhanced:
    • Test cases have been added specifically for sizes 1, 16, and 32.
    • Each test now also checks whether the result matches a hexadecimal pattern. 🧪🔍
  • In 'crypto.ts', a minor change happened that doesn't affect the function itself. Just a small formatting adjustment. 🎨
  • The 'importprompt.ts' file now has a 'leakables' array that lists several strings. It seems to serve as a filter when restoring the global context, possibly to prevent memory leaks or unwanted data propagation. 🧐🔒
  • In 'types/prompt_type.d.ts', the deprecated 'fs' variable has been removed in favor of 'workspace'. This is user-facing, simplifying the variable use for developers working with the workspace filesystem. 👥💻
  • Moving onto the sample package, 'exec.genai.js' has been deleted and replaced with 'exec.genai.mjs' - a similar file but with minor tweaks:
    • The AI model has been specified more clearly.
    • There's a new console debug statement when the 'python_version' tool is executed. 🐍🔧
  • Overall, these changes focus on strengthening the test coverage, potentially preventing memory leaks, simplifying workspace filesystem interaction, and improving debugging visibility. 👍🔧

generated by pr-describe

import { randomHex } from './crypto';
import { describe, test, beforeEach } from 'node:test';
Copy link

Choose a reason for hiding this comment

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

The import order seems to be incorrect. It's a good practice to keep the external and internal imports separate.

generated by pr-review-commit import_order

"AICI",
"fetchText",
"cancel",
]
Copy link

Choose a reason for hiding this comment

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

The array 'leakables' seems to be hard-coded. Consider making it a constant or configurable if these values are used elsewhere or could change.

generated by pr-review-commit hard_coded

@@ -111,12 +111,6 @@ declare var retrieval: Retrieval
*/
declare var workspace: WorkspaceFileSystem
Copy link

Choose a reason for hiding this comment

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

The 'fs' variable is deprecated. Please make sure to replace all its usages with 'workspace'.

generated by pr-review-commit deprecated_code

Copy link

github-actions bot commented Aug 8, 2024

The changes in the pull request mostly seem to be about code styling, refactoring and adding a security check to avoid leakage of sensitive information. Here's a brief overview:

  1. The crypto.genai.test.ts file has been modified, with the test cases for the randomHex function getting refactored into individual test cases for different sizes. The checks for hexadecimal string length and valid hexadecimal characters have been retained and look good. 👍

  2. An end of file newline has been removed in crypto.ts. This is a minor change, but it's generally a good practice to end files with a newline. ✏️

  3. Security enhancements in the importprompt.ts file: a leakables array has been introduced, which appears to contain variables that shouldn't leak into the global context. This is a great security improvement. 🛡️

  4. In prompt_type.d.ts, the deprecated fs variable is removed which is good as it encourages usage of the updated workspace variable.

Overall, the changes look good with an improvement in the test case structure and a potential security enhancement. Just one minor issue: the removal of the end of file newline in crypto.ts.

Proposed changes:

diff --git a/packages/core/src/crypto.ts b/packages/core/src/crypto.ts
index 492409cb..25621bf1 100644
--- a/packages/core/src/crypto.ts
+++ b/packages/core/src/crypto.ts
@@ -4,4 +4,4 @@ export function randomHex(size: number) {
     const bytes = new Uint8Array(size)
     crypto.getRandomValues(bytes)
     return toHex(bytes)
-}
+}
\ No newline at end of file

Back to:

diff --git a/packages/core/src/crypto.ts b/packages/core/src/crypto.ts
index 492409cb..25621bf1 100644
--- a/packages/core/src/crypto.ts
+++ b/packages/core/src/crypto.ts
@@ -4,4 +4,4 @@ export function randomHex(size: number) {
     const bytes = new Uint8Array(size)
     crypto.getRandomValues(bytes)
     return toHex(bytes)
+}

With these changes, it would be "LGTM 🚀".

generated by pr-review

@@ -68,6 +85,7 @@ export async function importPrompt(
} finally {
// restore global context
for (const field of Object.keys(oldGlb)) {
if (leakables.includes(field)) continue
Copy link

Choose a reason for hiding this comment

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

The array leakables contains global variables that could potentially be leaked into the global scope. This could lead to unexpected behavior.

generated by pr-review-commit leakable_globals

@@ -297,6 +298,14 @@ export class NodeHost implements RuntimeHost {
if (stdout) trace?.detailsFenced(`📩 stdout`, stdout)
if (stderr) trace?.detailsFenced(`📩 stderr`, stderr)
return { stdout, stderr, exitCode, failed }
} catch (err) {
trace?.error("exec failed", error)
Copy link

Choose a reason for hiding this comment

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

The variable error is not defined in the current scope. It seems like you meant to use err instead.

generated by pr-review-commit undefined_variable

@@ -111,12 +111,6 @@ declare var retrieval: Retrieval
*/
declare var workspace: WorkspaceFileSystem
Copy link

Choose a reason for hiding this comment

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

The global variable fs has been deprecated and removed, but it is still referenced in the code. Please replace it with the recommended workspace variable.

generated by pr-review-commit deprecated_global

@pelikhan pelikhan merged commit d161866 into main Aug 8, 2024
7 of 8 checks passed
@pelikhan pelikhan deleted the hostdeffix branch August 8, 2024 19:36
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