Tests and GH Actions #2
Conversation
This comprehensive upgrade implements Deno 2.5+ features including: - Permission sets for granular security control - Modern test APIs with beforeAll/afterAll hooks - Enhanced lint rules (no-unversioned-import, no-import-prefix) - TCP backlog optimization for better performance - Unicode hostname support in validation - Structured logging with Deno version info - Improved error handling and security headers - GitHub Actions workflow updates for Deno 2.5 - Comprehensive documentation of all changes The upgrade maintains backward compatibility while providing significant improvements to security, performance, and developer experience.
WalkthroughAdds Deno 2.5+ support and CI: new GitHub Actions workflows (security & proxy tests), IDE files, expanded deno.json, a refactored proxy server (main.ts), extensive integration tests and runners, CI/CD/docs, and helper scripts for local testing and GitHub setup. Changes
Sequence Diagram(s)sequenceDiagram
actor Dev as Developer
participant GHA as GitHub Actions
participant Runner as Workflow Runner
participant Deno as Deno Runtime
participant Proxy as Local Proxy
participant OpenAI as OpenAI API
Dev->>GHA: push / PR / manual dispatch
GHA->>Runner: start workflow
Runner->>Deno: setup Deno 2.5.x
rect rgb(245,247,255)
note right of Deno: Security & Quality Scan
Runner->>Deno: run security audit, secrets scan, code checks
Deno-->>Runner: produce report
end
rect rgb(245,255,245)
note right of Proxy: Test OpenAI Proxy
Runner->>Proxy: spawn server (proxy-server profile)
Proxy->>OpenAI: forward test requests
OpenAI-->>Proxy: return responses
Proxy-->>Runner: validate responses & emit summary
Runner->>Proxy: teardown
end
Runner->>GHA: post workflow summary/status
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Areas needing extra attention:
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (3)
.idea/.gitignore (1)
1-8: Redundant if.idea/is excluded at root level.This gitignore is only effective if
.idea/files are committed. As noted in the previous review comment, IDE-specific directories should typically be excluded at the root.gitignorelevel, making this file unnecessary..idea/modules.xml (1)
1-8: IDE configuration file should be excluded.As with other
.idea/files, this module configuration is IDE-specific and should not be committed to version control..idea/deno-proxy.iml (1)
1-9: IDE module file should be excluded.This IDE module definition file should not be committed to version control.
🧹 Nitpick comments (13)
.idea/vcs.xml (1)
1-6: Consider excluding IDE configuration files from version control.IDE-specific files under
.idea/are typically machine and user-specific. Committing them can lead to conflicts between developers with different IDE setups and clutters the repository with non-project files.Add the
.idea/directory to your root.gitignore:+# IDE files +.idea/If certain IDE configurations must be shared, consider using
.idea/templates or documenting setup instructions instead.TEST_RESULTS_SUMMARY.md (2)
40-55: Add language identifier to code block.The fenced code block showing test output should specify a language (e.g.,
textorshell) for proper syntax highlighting and accessibility.Apply this diff:
### Final Working Test Run -``` +```text running 1 test from ./test_openai_responses_simple.ts
140-140: Use heading instead of emphasis for status.Using bold emphasis for "Status: ✅ COMPLETE AND WORKING" treats it as emphasized text rather than a semantic heading. Consider using a proper heading level.
Apply this diff:
-**Status: ✅ COMPLETE AND WORKING** +### Status: ✅ COMPLETE AND WORKINGdeno.json (1)
38-44: Test permissions duplicate the testing profile.The test configuration (lines 38-44) duplicates permissions already defined in the
testingprofile (lines 23-27). While not harmful, this is redundant since tasks using-P=testingwill inherit those permissions.Consider removing the redundant test block if the testing profile is sufficient:
- "test": { - "permissions": { - "net": true, - "env": true, - "run": ["deno"] - } - },Or keep it for explicit documentation of test requirements.
TEST_README.md (2)
128-148: Add language identifier to code block.The expected test output code block should specify a language (e.g.,
text) for proper rendering.Apply this diff:
### Successful Test Run -``` +```text OpenAI Responses API Proxy Tests
154-167: Use consistent heading format for error messages.The error messages in the troubleshooting section use bold emphasis where headings or code formatting would be more appropriate and semantically correct.
Apply this diff to format error messages consistently:
-**"OPENAI_API_KEY environment variable is required"** +#### "OPENAI_API_KEY environment variable is required" - Solution: Set your OpenAI API key: `export OPENAI_API_KEY=sk-your-key` -**"Connection refused" or "Server not starting"** +#### "Connection refused" or "Server not starting" - Solution: Ensure port 8001 is available, or modify `PROXY_PORT` in test file -**"403 Forbidden" from OpenAI** +#### "403 Forbidden" from OpenAI - Solution: Verify your API key is valid and has necessary permissions -**"Rate limit exceeded"** +#### "Rate limit exceeded" - Solution: Wait a moment and retry, or check your OpenAI account usage -**"Tests timing out"** +#### "Tests timing out" - Solution: Increase `PROXY_TIMEOUT_MS` or check network connectivitytest_server_startup.ts (2)
35-37: Consider using a retry loop instead of fixed sleep.The fixed 3-second wait could cause flakiness (too short) or waste time (too long). The test suite in
test_openai_responses_simple.ts(lines 37-60) uses a retry loop with exponential backoff, which is more robust.Apply this pattern:
- // Wait and test connectivity - console.log("⏳ Waiting for server to start..."); - await new Promise(resolve => setTimeout(resolve, 3000)); + // Wait for server to start with retry + console.log("⏳ Waiting for server to start..."); + let retries = 15; + let serverReady = false; + + while (retries > 0 && !serverReady) { + try { + const healthCheck = await fetch("http://localhost:8000/test-host.com/test", { + signal: AbortSignal.timeout(1000) + }); + await healthCheck.text(); // Consume body + if (healthCheck.status === 403) { + serverReady = true; + break; + } + } catch { + // Server not ready, will retry + } + await new Promise(resolve => setTimeout(resolve, 200)); + retries--; + } + + if (!serverReady) { + throw new Error("Server failed to start within timeout"); + }
59-61: Status code check is too permissive.The condition
response.status >= 200 && response.status < 500treats client errors (400-499) as success, which could mask issues like 404 (not found) or 400 (bad request).Apply this diff:
- if (response.status >= 200 && response.status < 500) { + if (response.status === 200 || response.status === 401) { console.log("🎉 Proxy is forwarding requests correctly"); }example_test_run.sh (1)
51-58: Consider removing--no-checkflag for better type safety.The
--no-checkflag bypasses TypeScript type checking, which could hide type errors during local testing. While the CI workflow runs type checking separately, including it here would provide earlier feedback.deno test \ --allow-net \ --allow-env \ --allow-run \ - --no-check \ --reporter=pretty \ test_openai_responses_simple.tsCI_CD_SETUP.md (1)
105-105: Add language specifiers to fenced code blocks.Several code blocks are missing language identifiers, which affects rendering and syntax highlighting. Consider adding appropriate language tags (e.g.,
text,console, orbash).Example for line 105:
-``` +```text ✅ All Tests Passed! ...Also applies to: 121-121, 127-127, 133-133, 139-139, 145-145 </blockquote></details> <details> <summary>.github/workflows/security-scan.yml (2)</summary><blockquote> `261-287`: **Warning: `deno bundle` is deprecated.** The `deno bundle` command was deprecated and may be removed in future Deno versions. Consider alternatives for bundle size analysis. Options: 1. Use `deno info --json` to analyze dependency tree size instead 2. Remove bundle size check temporarily 3. Use a third-party bundler like esbuild Example alternative using `deno info`: ```diff - name: Bundle size analysis run: | - echo "📦 Analyzing bundle size..." - - # Create a bundle to check size - deno bundle main.ts bundle.js - - bundle_size=$(stat -f%z bundle.js 2>/dev/null || stat -c%s bundle.js) - echo "Bundle size: $bundle_size bytes" + echo "📦 Analyzing dependency size..." + + # Analyze dependency tree instead + deno info --json main.ts > deps.json + + dep_count=$(jq '.modules | length' deps.json) + echo "Total dependencies: $dep_count" - # Warn if bundle is too large (>1MB for a proxy) - if [ $bundle_size -gt 1048576 ]; then - echo "⚠️ WARNING: Bundle size is large (>1MB). Consider optimizing." + if [ $dep_count -gt 50 ]; then + echo "⚠️ WARNING: High dependency count (>50). Consider optimization."
305-338: Consider removingDENO_FUTURE=1flag.The
DENO_FUTURE=1environment variable enables experimental features that may cause instability or break in future releases. Since the project targets Deno 2.5.x (stable), this flag may not be necessary.- DENO_FUTURE=1 deno run --allow-net --allow-env main.ts & + deno run --allow-net --allow-env main.ts & server_pid=$!README.md (1)
155-162: Add a fenced code language for lint compliance.markdownlint is flagging this block (MD040) because it lacks a language hint. Please tag it (e.g., ```text) to keep the docs lint-clean.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (3)
deno.lockis excluded by!**/*.lockpublic/images/deno.svgis excluded by!**/*.svgpublic/images/deploy.svgis excluded by!**/*.svg
📒 Files selected for processing (20)
.github/workflows/security-scan.yml(1 hunks).github/workflows/test-proxy.yml(1 hunks).idea/.gitignore(1 hunks).idea/deno-proxy.iml(1 hunks).idea/modules.xml(1 hunks).idea/vcs.xml(1 hunks)CI_CD_SETUP.md(1 hunks)DENO_2_5_UPGRADE_SUMMARY.md(1 hunks)IMPLEMENTATION_SUMMARY.md(1 hunks)README.md(6 hunks)TEST_README.md(1 hunks)TEST_RESULTS_SUMMARY.md(1 hunks)deno.json(1 hunks)example_test_run.sh(1 hunks)main.ts(1 hunks)run_tests.ts(1 hunks)setup-github-ci.sh(1 hunks)test_openai_responses.ts(1 hunks)test_openai_responses_simple.ts(1 hunks)test_server_startup.ts(1 hunks)
🧰 Additional context used
🪛 actionlint (1.7.8)
.github/workflows/security-scan.yml
219-219: shellcheck reported issue in this script: SC2086:info:1:38: Double quote to prevent globbing and word splitting
(shellcheck)
219-219: shellcheck reported issue in this script: SC2086:info:2:12: Double quote to prevent globbing and word splitting
(shellcheck)
219-219: shellcheck reported issue in this script: SC2129:style:5:3: Consider using { cmd1; cmd2; } >> file instead of individual redirects
(shellcheck)
219-219: shellcheck reported issue in this script: SC2086:info:5:41: Double quote to prevent globbing and word splitting
(shellcheck)
219-219: shellcheck reported issue in this script: SC2086:info:6:14: Double quote to prevent globbing and word splitting
(shellcheck)
219-219: shellcheck reported issue in this script: SC2086:info:7:72: Double quote to prevent globbing and word splitting
(shellcheck)
219-219: shellcheck reported issue in this script: SC2086:info:8:14: Double quote to prevent globbing and word splitting
(shellcheck)
219-219: shellcheck reported issue in this script: SC2086:info:9:47: Double quote to prevent globbing and word splitting
(shellcheck)
219-219: shellcheck reported issue in this script: SC2086:info:10:47: Double quote to prevent globbing and word splitting
(shellcheck)
219-219: shellcheck reported issue in this script: SC2086:info:11:42: Double quote to prevent globbing and word splitting
(shellcheck)
219-219: shellcheck reported issue in this script: SC2086:info:12:49: Double quote to prevent globbing and word splitting
(shellcheck)
219-219: shellcheck reported issue in this script: SC2086:info:13:51: Double quote to prevent globbing and word splitting
(shellcheck)
219-219: shellcheck reported issue in this script: SC2086:info:14:14: Double quote to prevent globbing and word splitting
(shellcheck)
219-219: shellcheck reported issue in this script: SC2086:info:15:47: Double quote to prevent globbing and word splitting
(shellcheck)
219-219: shellcheck reported issue in this script: SC2129:style:17:3: Consider using { cmd1; cmd2; } >> file instead of individual redirects
(shellcheck)
219-219: shellcheck reported issue in this script: SC2086:info:17:41: Double quote to prevent globbing and word splitting
(shellcheck)
219-219: shellcheck reported issue in this script: SC2086:info:18:14: Double quote to prevent globbing and word splitting
(shellcheck)
219-219: shellcheck reported issue in this script: SC2086:info:19:82: Double quote to prevent globbing and word splitting
(shellcheck)
219-219: shellcheck reported issue in this script: SC2086:info:20:14: Double quote to prevent globbing and word splitting
(shellcheck)
219-219: shellcheck reported issue in this script: SC2086:info:21:36: Double quote to prevent globbing and word splitting
(shellcheck)
219-219: shellcheck reported issue in this script: SC2086:info:22:44: Double quote to prevent globbing and word splitting
(shellcheck)
219-219: shellcheck reported issue in this script: SC2086:info:23:50: Double quote to prevent globbing and word splitting
(shellcheck)
219-219: shellcheck reported issue in this script: SC2086:info:24:44: Double quote to prevent globbing and word splitting
(shellcheck)
219-219: shellcheck reported issue in this script: SC2086:info:25:54: Double quote to prevent globbing and word splitting
(shellcheck)
262-262: shellcheck reported issue in this script: SC2086:info:10:6: Double quote to prevent globbing and word splitting
(shellcheck)
262-262: shellcheck reported issue in this script: SC2086:info:12:8: Double quote to prevent globbing and word splitting
(shellcheck)
.github/workflows/test-proxy.yml
155-155: shellcheck reported issue in this script: SC2086:info:1:35: Double quote to prevent globbing and word splitting
(shellcheck)
155-155: shellcheck reported issue in this script: SC2086:info:2:12: Double quote to prevent globbing and word splitting
(shellcheck)
155-155: shellcheck reported issue in this script: SC2129:style:5:3: Consider using { cmd1; cmd2; } >> file instead of individual redirects
(shellcheck)
155-155: shellcheck reported issue in this script: SC2086:info:5:37: Double quote to prevent globbing and word splitting
(shellcheck)
155-155: shellcheck reported issue in this script: SC2086:info:6:14: Double quote to prevent globbing and word splitting
(shellcheck)
155-155: shellcheck reported issue in this script: SC2086:info:7:76: Double quote to prevent globbing and word splitting
(shellcheck)
155-155: shellcheck reported issue in this script: SC2086:info:8:14: Double quote to prevent globbing and word splitting
(shellcheck)
155-155: shellcheck reported issue in this script: SC2086:info:9:46: Double quote to prevent globbing and word splitting
(shellcheck)
155-155: shellcheck reported issue in this script: SC2086:info:10:44: Double quote to prevent globbing and word splitting
(shellcheck)
155-155: shellcheck reported issue in this script: SC2086:info:11:49: Double quote to prevent globbing and word splitting
(shellcheck)
155-155: shellcheck reported issue in this script: SC2086:info:12:42: Double quote to prevent globbing and word splitting
(shellcheck)
155-155: shellcheck reported issue in this script: SC2086:info:13:50: Double quote to prevent globbing and word splitting
(shellcheck)
155-155: shellcheck reported issue in this script: SC2086:info:14:14: Double quote to prevent globbing and word splitting
(shellcheck)
155-155: shellcheck reported issue in this script: SC2086:info:15:52: Double quote to prevent globbing and word splitting
(shellcheck)
155-155: shellcheck reported issue in this script: SC2129:style:17:3: Consider using { cmd1; cmd2; } >> file instead of individual redirects
(shellcheck)
155-155: shellcheck reported issue in this script: SC2086:info:17:32: Double quote to prevent globbing and word splitting
(shellcheck)
155-155: shellcheck reported issue in this script: SC2086:info:18:14: Double quote to prevent globbing and word splitting
(shellcheck)
155-155: shellcheck reported issue in this script: SC2086:info:19:69: Double quote to prevent globbing and word splitting
(shellcheck)
155-155: shellcheck reported issue in this script: SC2086:info:20:14: Double quote to prevent globbing and word splitting
(shellcheck)
155-155: shellcheck reported issue in this script: SC2086:info:21:28: Double quote to prevent globbing and word splitting
(shellcheck)
155-155: shellcheck reported issue in this script: SC2086:info:22:56: Double quote to prevent globbing and word splitting
(shellcheck)
155-155: shellcheck reported issue in this script: SC2086:info:23:43: Double quote to prevent globbing and word splitting
(shellcheck)
155-155: shellcheck reported issue in this script: SC2086:info:24:45: Double quote to prevent globbing and word splitting
(shellcheck)
155-155: shellcheck reported issue in this script: SC2086:info:25:48: Double quote to prevent globbing and word splitting
(shellcheck)
🪛 ast-grep (0.39.7)
main.ts
[warning] 37-37: Regular expression constructed from variable input detected. This can lead to Regular Expression Denial of Service (ReDoS) attacks if the variable contains malicious patterns. Use libraries like 'recheck' to validate regex safety or use static patterns.
Context: new RegExp(^${regexString}$, "i")
Note: [CWE-1333] Inefficient Regular Expression Complexity [REFERENCES]
- https://owasp.org/www-community/attacks/Regular_expression_Denial_of_Service_-_ReDoS
- https://cwe.mitre.org/data/definitions/1333.html
(regexp-from-variable)
🪛 GitHub Actions: Security & Quality Scan
.github/workflows/security-scan.yml
[error] 1-1: grep: unrecognized option '-----BEGIN.*PRIVATE KEY-----'
[error] 1-1: Potential database URL found!
[error] 1-1: Hardcoded secrets detected! Remove them and use environment variables or secrets.
🪛 LanguageTool
IMPLEMENTATION_SUMMARY.md
[uncategorized] ~63-~63: If this is a compound adjective that modifies the following noun, use a hyphen.
Context: ...##
(EN_COMPOUND_ADJECTIVE_INTERNAL)
CI_CD_SETUP.md
[uncategorized] ~74-~74: If this is a compound adjective that modifies the following noun, use a hyphen.
Context: ... 🛡️ Input validation verification - 🚦 Rate limiting implementation - 📦 Dependency security...
(EN_COMPOUND_ADJECTIVE_INTERNAL)
[uncategorized] ~185-~185: The official name of this software platform is spelled with a capital “H”.
Context: ... CI ### Modifying Security Checks Edit .github/workflows/security-scan.yml: ```yaml ...
(GITHUB)
README.md
[uncategorized] ~28-~28: The official name of this software platform is spelled with a capital “H”.
Context: ...xible whitelisting of subdomains (e.g., *.github.com). - Path Traversal Prevention:...
(GITHUB)
🪛 markdownlint-cli2 (0.18.1)
CI_CD_SETUP.md
105-105: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
121-121: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
127-127: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
133-133: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
139-139: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
145-145: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
TEST_README.md
128-128: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
154-154: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
157-157: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
160-160: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
163-163: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
166-166: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
TEST_RESULTS_SUMMARY.md
40-40: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
140-140: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
README.md
156-156: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🔇 Additional comments (10)
.idea/deno-proxy.iml (1)
3-3: Go component enabled for a Deno project?The Go component is enabled in this module file for what appears to be a Deno/TypeScript project. This seems like an IDE configuration error.
If this project does not use Go, this component should be disabled or removed from the IDE configuration.
run_tests.ts (1)
10-68: LGTM! Well-structured test runner.The test runner properly validates environment requirements, configures test settings, and provides clear feedback. Good use of exit codes and informative console output.
deno.json (2)
6-28: Excellent use of permission profiles.The permission profiles approach is a great security practice for Deno 2.5+. The
proxy-serverprofile explicitly lists allowed environment variables (lines 14-20), and thetestingprofile appropriately restrictsrunpermissions to onlydeno(line 26).
2-64: Well-structured Deno 2.5+ configuration.The configuration properly uses compiler options, permission profiles, task definitions, and tooling configurations. The exclude patterns and import mappings are appropriate.
TEST_README.md (1)
1-201: Comprehensive and well-organized test documentation.The documentation provides clear setup instructions, test coverage details, troubleshooting guidance, and cost considerations. This will be very helpful for contributors.
DENO_2_5_UPGRADE_SUMMARY.md (1)
1-283: Excellent upgrade documentation!This comprehensive guide clearly explains Deno 2.5+ features, migration steps, and benefits. The examples and migration checklist will help developers understand and adopt the new features.
test_openai_responses_simple.ts (1)
1-286: Well-structured test suite with excellent practices!This test implementation demonstrates:
- Proper use of Deno 2.5+
beforeAll/afterAllhooks for lifecycle management- Robust server startup with retry logic and timeout handling
- Correct response body consumption to prevent resource leaks (lines 46, 141, 155, etc.)
- Comprehensive test coverage including security, authentication, and error handling
IMPLEMENTATION_SUMMARY.md (1)
1-189: Thorough implementation analysis.This document provides valuable technical assessment and recommendations. The structured review of security, testing, and architecture decisions helps understand the implementation quality.
.github/workflows/test-proxy.yml (2)
30-36: The review comment is incorrect—deno.lockexists in the repository.The verification confirms that
deno.lockexists (815 bytes, modified Nov 7). The cache key correctly references this file, so there is no cache miss concern or configuration issue.Likely an incorrect or invalid review comment.
136-141: The grep pattern in test-proxy.yml (lines 136-141) is correct and functional.The pattern uses a literal string search
"sk-"with valid--excludeoptions and executes successfully without errors. The review comment incorrectly flags this pattern based on concerns about "similar patterns" insecurity-scan.yml, which is a separate file. Whilesecurity-scan.ymlmay have regex patterns that lack the-Eflag (lines 46, 52, 66, 72, etc.), the current file and lines under review contain no such issues.Likely an incorrect or invalid review comment.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
main.ts (1)
141-143: Remove redundant type annotation.The explicit type annotation for the
regexparameter is unnecessary since TypeScript already infers the type fromALLOWED_HOST_REGEXPS.Apply this diff to simplify:
- const isAllowed = ALLOWED_HOST_REGEXPS.some( - (regex: { test: (arg0: string) => any }) => regex.test(targetHost), - ); + const isAllowed = ALLOWED_HOST_REGEXPS.some((regex) => + regex.test(targetHost) + );
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
.github/workflows/security-scan.yml(1 hunks)main.ts(1 hunks)run_tests.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- run_tests.ts
🧰 Additional context used
🪛 ast-grep (0.39.7)
main.ts
[warning] 38-38: Regular expression constructed from variable input detected. This can lead to Regular Expression Denial of Service (ReDoS) attacks if the variable contains malicious patterns. Use libraries like 'recheck' to validate regex safety or use static patterns.
Context: new RegExp(^${regexString}$, "i")
Note: [CWE-1333] Inefficient Regular Expression Complexity [REFERENCES]
- https://owasp.org/www-community/attacks/Regular_expression_Denial_of_Service_-_ReDoS
- https://cwe.mitre.org/data/definitions/1333.html
(regexp-from-variable)
🪛 GitHub Actions: Security & Quality Scan
main.ts
[error] 1-1: Command failed: deno bundle main.ts bundle.js. Error: No such file or directory (os error 2).
🔇 Additional comments (3)
main.ts (1)
9-12: LGTM! Host pattern trimming fixed.The past review comment has been addressed correctly. The code now trims whitespace from each host pattern before filtering, ensuring patterns like
"api.openai.com, *.deno.land"are properly parsed..github/workflows/security-scan.yml (2)
83-87: LGTM! Grep syntax fixed.The
--separator has been correctly added to prevent the pattern starting with-from being interpreted as an option. This addresses the previous pipeline failure.
89-93: LGTM! Documentation examples filtered correctly.The additional
grep -v "example\|Example"filter and--exclude-dir=docsexclusion properly prevent false positives from documentation while still detecting actual hardcoded database URLs.
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (6)
TEST_README.md (2)
134-154: Add language specification to code block.The expected output code block is missing a language specifier, which reduces syntax highlighting and clarity.
Apply this diff:
-``` +```text OpenAI Responses API Proxy Tests
160-176: Consider using proper headings for troubleshooting sections.The troubleshooting subsections use bold text instead of proper markdown headings, which affects document structure and navigation.
Apply this diff:
-**"OPENAI_API_KEY environment variable is required"** +#### "OPENAI_API_KEY environment variable is required" -**"Connection refused" or "Server not starting"** +#### "Connection refused" or "Server not starting" -**"403 Forbidden" from OpenAI** +#### "403 Forbidden" from OpenAI -**"Rate limit exceeded"** +#### "Rate limit exceeded" -**"Tests timing out"** +#### "Tests timing out"BUNDLE_FIX_SUMMARY.md (1)
7-9: Add language specifications to code blocks for consistency.Three code blocks are missing language specifiers, which reduces clarity and syntax highlighting.
Apply these diffs:
-``` +```text error: `deno bundle` was removed in Deno 2.-
+text
❌ error:deno bundlewas removed in Deno 2.
Bundle size analysis FAILED-``` +```text ✅ Source size analysis:Also applies to: 87-90, 93-104
test_openai_responses_simple.ts (1)
77-100: Consider extracting shared test utilities to reduce duplication.The
makeProxyRequesthelper andstartProxyServerfunction appear in bothtest_openai_responses_simple.tsandtest_openai_responses.tswith slight variations. While test isolation is valuable, extracting these to a shared test utilities module could improve maintainability.Consider creating a
test_utils.tsfile:// test_utils.ts export async function startProxyServer(port: number, usePermissionProfile: boolean = true): Promise<Deno.ChildProcess> { // Shared implementation } export async function makeProxyRequest( proxyHost: string, apiKey: string, endpoint: string, method: string = "GET", body?: any, ): Promise<Response> { // Shared implementation }Then import in both test files:
import { startProxyServer, makeProxyRequest } from "./test_utils.ts";Also applies to: 19-74
TEST_RESULTS_SUMMARY.md (1)
44-59: Add language specification to test output code block.The test output code block should specify
textas the language for better rendering.Apply this diff:
-``` +```text running 1 test from ./test_openai_responses_simple.tstest_openai_responses.ts (1)
446-451: Stream cancellation in cleanup may fail if streams were never read.The cleanup code attempts to cancel stdout/stderr streams, but since they were set to "piped" and never consumed, this could potentially cause resource leaks or errors. With the earlier suggested change to use "null" instead of "piped", this cleanup would no longer be necessary.
If you keep "piped" streams, consider consuming them asynchronously to prevent blocking:
// Start consuming streams immediately after spawn const stdoutReader = proxyProcess.stdout.getReader(); const stderrReader = proxyProcess.stderr.getReader(); // Consume in background (async () => { while (!(await stdoutReader.read()).done); })(); (async () => { while (!(await stderrReader.read()).done); })();Otherwise, prefer "null" streams as suggested in the previous comment.
Also applies to: 557-562
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
public/images/deno.svgis excluded by!**/*.svgpublic/images/deploy.svgis excluded by!**/*.svg
📒 Files selected for processing (13)
.github/workflows/security-scan.yml(1 hunks)BUNDLE_FIX_SUMMARY.md(1 hunks)CI_CD_SETUP.md(1 hunks)DENO_2_5_UPGRADE_SUMMARY.md(1 hunks)IMPLEMENTATION_SUMMARY.md(1 hunks)README.md(2 hunks)TEST_README.md(1 hunks)TEST_RESULTS_SUMMARY.md(1 hunks)main.ts(1 hunks)run_tests.ts(1 hunks)test_openai_responses.ts(1 hunks)test_openai_responses_simple.ts(1 hunks)test_server_startup.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- run_tests.ts
- test_server_startup.ts
🧰 Additional context used
🪛 ast-grep (0.39.7)
main.ts
[warning] 43-43: Regular expression constructed from variable input detected. This can lead to Regular Expression Denial of Service (ReDoS) attacks if the variable contains malicious patterns. Use libraries like 'recheck' to validate regex safety or use static patterns.
Context: new RegExp(^${regexString}$, "i")
Note: [CWE-1333] Inefficient Regular Expression Complexity [REFERENCES]
- https://owasp.org/www-community/attacks/Regular_expression_Denial_of_Service_-_ReDoS
- https://cwe.mitre.org/data/definitions/1333.html
(regexp-from-variable)
🪛 LanguageTool
README.md
[uncategorized] ~28-~28: The official name of this software platform is spelled with a capital “H”.
Context: ...xible whitelisting of subdomains (e.g., *.github.com). - Path Traversal Prevention:...
(GITHUB)
[style] ~47-~47: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...nt in the ALLOWED_HOSTS whitelist. 4. It reconstructs the target URL: `https://m...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[style] ~48-~48: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...tps://mcp.exa.ai/some/path?query=1`. 5. It forwards the original request (includin...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
BUNDLE_FIX_SUMMARY.md
[style] ~199-~199: Consider using a different verb to strengthen your wording.
Context: ...balances simplicity with functionality, giving developers the information they need to...
(GIVE_INFORMATION)
CI_CD_SETUP.md
[uncategorized] ~83-~83: If this is a compound adjective that modifies the following noun, use a hyphen.
Context: ... 🛡️ Input validation verification - 🚦 Rate limiting implementation - 📦 Dependency security...
(EN_COMPOUND_ADJECTIVE_INTERNAL)
[uncategorized] ~218-~218: The official name of this software platform is spelled with a capital “H”.
Context: ...CI ### Modifying Security Checks Edit .github/workflows/security-scan.yml: ```yaml ...
(GITHUB)
IMPLEMENTATION_SUMMARY.md
[uncategorized] ~67-~67: If this is a compound adjective that modifies the following noun, use a hyphen.
Context: ...##
(EN_COMPOUND_ADJECTIVE_INTERNAL)
🪛 markdownlint-cli2 (0.18.1)
README.md
158-158: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
BUNDLE_FIX_SUMMARY.md
7-7: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
87-87: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
93-93: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
TEST_README.md
134-134: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
160-160: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
164-164: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
168-168: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
172-172: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
176-176: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
TEST_RESULTS_SUMMARY.md
158-158: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🔇 Additional comments (4)
test_openai_responses_simple.ts (3)
1-16: LGTM! Clean environment setup with proper validation.The environment setup properly validates the required
OPENAI_API_KEYand provides clear error messages. The use ofparseIntwith a default value forPROXY_PORTprovides good flexibility.
19-74: Well-implemented server startup with robust readiness checks.The proxy server startup function includes:
- Proper use of Deno 2.5+ permission profiles (
-P=proxy-server)- Robust retry logic with 30 attempts
- Clever readiness detection using 403 response for non-whitelisted host
- Clean resource management
229-260: No action needed — the model name is correct.The API model ID is "gpt-4o-mini", which matches the test code exactly. The review comment's concern is unfounded.
test_openai_responses.ts (1)
203-203: No action needed — "gpt-5-nano" is now a valid OpenAI model.As of Nov 7, 2025, "gpt-5-nano" is a recognized variant in the GPT-5 series available for chat completions API. The model name in the test file is correct and does not require changes. Tests using this model should execute successfully with a valid API key.
| if grep -r "sk-[a-zA-Z0-9]{48}" --exclude-dir=.git --exclude-dir=node_modules . ; then | ||
| echo "❌ Potential OpenAI API key found!" | ||
| secrets_found=true | ||
| fi | ||
| # Generic API patterns | ||
| if grep -ri "api[_-]key.*=.*['\"][a-zA-Z0-9]{20,}" --exclude-dir=.git --exclude-dir=node_modules --exclude="*.yml" --exclude="*.yaml" --exclude="*.md" . ; then | ||
| echo "❌ Potential API key pattern found!" | ||
| secrets_found=true | ||
| fi | ||
| # Bearer tokens | ||
| if grep -r "Bearer [a-zA-Z0-9]{20,}" --exclude-dir=.git --exclude-dir=node_modules --exclude="*.yml" --exclude="*.yaml" --exclude="*.md" . ; then | ||
| echo "❌ Potential bearer token found!" | ||
| secrets_found=true | ||
| fi | ||
| # Private keys | ||
| if grep -r -- "-----BEGIN.*PRIVATE KEY-----" --exclude-dir=.git --exclude-dir=node_modules . ; then | ||
| echo "❌ Private key found!" | ||
| secrets_found=true | ||
| fi | ||
| # Database URLs | ||
| if grep -r "postgres://\|mysql://\|mongodb://" --exclude-dir=.git --exclude-dir=node_modules --exclude="*.md" --exclude-dir=docs . | grep -v "example\|Example" ; then | ||
| echo "❌ Potential database URL found!" | ||
| secrets_found=true | ||
| fi |
There was a problem hiding this comment.
Fix secret scan regex to actually match secrets.
Every grep here relies on {…} quantifiers, but plain grep defaults to basic regex where {} are literals. As written, none of these patterns will ever match, so real secrets slip through unnoticed—defeating the entire scan. Please switch to extended regex (or escape the braces) so the checks work.
Apply this diff:
- if grep -r "sk-[a-zA-Z0-9]{48}" --exclude-dir=.git --exclude-dir=node_modules . ; then
+ if grep -rE "sk-[A-Za-z0-9]{48}" --exclude-dir=.git --exclude-dir=node_modules . ; then
echo "❌ Potential OpenAI API key found!"
secrets_found=true
fi
@@
- if grep -ri "api[_-]key.*=.*['\"][a-zA-Z0-9]{20,}" --exclude-dir=.git --exclude-dir=node_modules --exclude="*.yml" --exclude="*.yaml" --exclude="*.md" . ; then
+ if grep -riE "api[-_]key.*=.*['\"][A-Za-z0-9]{20,}" --exclude-dir=.git --exclude-dir=node_modules --exclude="*.yml" --exclude="*.yaml" --exclude="*.md" . ; then
echo "❌ Potential API key pattern found!"
secrets_found=true
fi
@@
- if grep -r "Bearer [a-zA-Z0-9]{20,}" --exclude-dir=.git --exclude-dir=node_modules --exclude="*.yml" --exclude="*.yaml" --exclude="*.md" . ; then
+ if grep -rE "Bearer [A-Za-z0-9]{20,}" --exclude-dir=.git --exclude-dir=node_modules --exclude="*.yml" --exclude="*.yaml" --exclude="*.md" . ; then
echo "❌ Potential bearer token found!"
secrets_found=true
fi🤖 Prompt for AI Agents
.github/workflows/security-scan.yml lines 66-93: the grep patterns use `{n}`
quantifiers which are treated as literal braces by basic grep, so none of the
secret regexes will match; fix by enabling extended regex (add -E to each grep,
e.g. grep -rE or grep -riE as appropriate) or alternatively escape the braces as
`\{...\}`; ensure you preserve the existing flags and excludes while updating
each grep invocation so the intended quantifiers (e.g. {48}, {20,}) are honored.
| 1. Fork this repository. | ||
| 2. Create a new project on Deno Deploy and link it to your forked repository. | ||
| 3. Choose the `proxy.ts` file as the entry point. | ||
| 4. In the project settings on Deno Deploy, go to **Settings -> Environment Variables** and add your configuration. At a minimum, you must set `ALLOWED_HOSTS`. | ||
|
|
There was a problem hiding this comment.
Correct the Deno Deploy entrypoint.
Step 3 still points to proxy.ts, but the project now ships main.ts. Deno Deploy will fail to start if users follow this. Please update the instruction to reference main.ts.
Apply this diff:
-3. Choose the `proxy.ts` file as the entry point.
+3. Choose the `main.ts` file as the entry point.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| 1. Fork this repository. | |
| 2. Create a new project on Deno Deploy and link it to your forked repository. | |
| 3. Choose the `proxy.ts` file as the entry point. | |
| 4. In the project settings on Deno Deploy, go to **Settings -> Environment Variables** and add your configuration. At a minimum, you must set `ALLOWED_HOSTS`. | |
| 1. Fork this repository. | |
| 2. Create a new project on Deno Deploy and link it to your forked repository. | |
| 3. Choose the `main.ts` file as the entry point. | |
| 4. In the project settings on Deno Deploy, go to **Settings -> Environment Variables** and add your configuration. At a minimum, you must set `ALLOWED_HOSTS`. |
🤖 Prompt for AI Agents
In README.md around lines 119 to 123, the entrypoint instruction still
references proxy.ts; update step 3 to instruct users to choose main.ts as the
Deno Deploy entry point so deployments start correctly.
| import { assert, assertEquals, assertExists } from "@std/assert"; | ||
|
|
||
| // Test configuration | ||
| const PROXY_PORT = 8000; |
There was a problem hiding this comment.
Inconsistent PROXY_PORT configuration between test files.
This file hardcodes PROXY_PORT = 8000, while test_openai_responses_simple.ts reads it from the environment with a fallback (parseInt(Deno.env.get("PROXY_PORT") ?? "8000", 10)). This inconsistency could cause issues if multiple test files run concurrently or if users want to configure the port.
Apply this diff to align with the simple test file:
-const PROXY_PORT = 8000;
+const PROXY_PORT = parseInt(Deno.env.get("PROXY_PORT") ?? "8000", 10);🤖 Prompt for AI Agents
In test_openai_responses.ts around line 4, PROXY_PORT is hardcoded to 8000 which
is inconsistent with test_openai_responses_simple.ts; replace the hardcoded
value with an environment-aware parse like: read Deno.env.get("PROXY_PORT") and
fall back to "8000", then parseInt(..., 10) so the test uses the same
configurable port behavior as the simple test file.
| const process = new Deno.Command("deno", { | ||
| args: ["run", "--allow-net", "--allow-env", "main.ts"], | ||
| env, | ||
| stdout: "piped", | ||
| stderr: "piped", | ||
| }).spawn(); |
There was a problem hiding this comment.
Inconsistent permission model and unused piped streams.
This test file uses explicit permission flags (--allow-net, --allow-env) while test_openai_responses_simple.ts uses the Deno 2.5+ permission profile (-P=proxy-server). Additionally, stdout/stderr are set to "piped" but are never read—only cancelled during cleanup.
Consider aligning with the simple version for consistency:
const process = new Deno.Command("deno", {
- args: ["run", "--allow-net", "--allow-env", "main.ts"],
+ args: ["run", "-P=proxy-server", "main.ts"],
env,
- stdout: "piped",
- stderr: "piped",
+ stdout: "null",
+ stderr: "null",
}).spawn();This change:
- Uses the modern permission profile approach
- Avoids allocating unused pipe streams
- Aligns with
test_openai_responses_simple.ts
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const process = new Deno.Command("deno", { | |
| args: ["run", "--allow-net", "--allow-env", "main.ts"], | |
| env, | |
| stdout: "piped", | |
| stderr: "piped", | |
| }).spawn(); | |
| const process = new Deno.Command("deno", { | |
| args: ["run", "-P=proxy-server", "main.ts"], | |
| env, | |
| stdout: "null", | |
| stderr: "null", | |
| }).spawn(); |
🤖 Prompt for AI Agents
In test_openai_responses.ts around lines 24 to 29, the command is started with
explicit permission flags and creates piped stdout/stderr that are never used;
change the args to use the permission profile like "-P=proxy-server" (e.g.
["run", "-P=proxy-server", "main.ts"]) to match test_openai_responses_simple.ts,
remove the stdout/stderr: "piped" properties so no unused pipe streams are
allocated, and update any cleanup code so it no longer attempts to cancel those
nonexistent piped streams.
|
|
||
| ## Prerequisites | ||
|
|
||
| 1. **Deno Runtime**: Ensure you have Deno installed (v1.x or later) |
There was a problem hiding this comment.
Update Deno version requirement to match PR scope.
The documentation states "Deno Runtime: Ensure you have Deno installed (v1.x or later)", but this PR specifically adds Deno 2.5+ support. The test files use Deno 2.5+ features like permission profiles (-P=proxy-server).
Apply this diff to correct the version requirement:
-1. **Deno Runtime**: Ensure you have Deno installed (v1.x or later)
+1. **Deno Runtime**: Ensure you have Deno installed (v2.5 or later)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| 1. **Deno Runtime**: Ensure you have Deno installed (v1.x or later) | |
| 1. **Deno Runtime**: Ensure you have Deno installed (v2.5 or later) |
🤖 Prompt for AI Agents
In TEST_README.md around line 18, the Deno version requirement is too broad
("v1.x or later") but the PR introduces and tests Deno 2.5+ features; update the
line to require Deno 2.5 or later (e.g., "Ensure you have Deno installed (v2.5
or later)") so the docs match the codebase and tests that use permission
profiles like -P=proxy-server.
Summary by CodeRabbit
New Features
Documentation
Tests
Chores