Conversation
There was a problem hiding this comment.
Pull request overview
This PR introduces a "git mode" feature that replaces GitHub API-based source retrieval with direct git operations using a local clone. The change eliminates dependency on GitHub API rate limits and tokens by performing all repository operations through git commands.
Changes:
- Replaced GitHub API calls with git command-line operations for fetching branches, commits, and files
- Introduced a local git cache (
tmp/git-cache/repo) with sparse checkout and partial clone optimizations - Updated test suite to reflect git-based implementation and removed API-mocking dependencies
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| app/download.js | Core refactoring to replace HTTP-based GitHub API requests with git commands for all repository operations |
| app/handlers.js | Removed GitHub API URL construction and rate limit handling; simplified to use git-based download functions |
| app/interpreter.js | Added support for configurable default branch and updated URL parsing to handle both 'master' and 'main' |
| app/utilities.js | Simplified branch access tracking by removing date-based update logic |
| app/filesystem.js | Added git-cache exclusion from cleanup operations and enhanced access time tracking via info.json |
| app/server.js | Added periodic git sync intervals and environment variable configuration |
| app/dashboards.js | Updated string concatenation to use template literals and added branch access tracking |
| test/filesystem.js | Modernized imports to use node: protocol and replaced forEach with for-of loops |
| test/download.js | Rewrote tests to verify git-based operations instead of GitHub API behavior |
| Dockerfile | Added git package installation |
| README.md | Documented git-only mode, environment variables, and cache behavior |
|
|
||
| await createDirectory(outputDir) | ||
|
|
||
| const archivePath = join(GIT_CACHE_DIR, `${resolvedRef}-${Date.now()}.tar`) |
There was a problem hiding this comment.
Using Date.now() in the filename prevents collision but creates temporary files that may accumulate if cleanup fails. The code does attempt cleanup at line 392, but errors are silently caught. Consider logging failures or implementing a more robust cleanup strategy for stale archive files.
app/download.js
Outdated
| const { stdout } = await execFileAsync('git', args, { | ||
| maxBuffer: GIT_MAX_BUFFER, | ||
| ...options | ||
| }) | ||
| return stdout.trim() |
There was a problem hiding this comment.
The execGit function doesn't handle stderr or provide context about which git command failed. When git operations fail, debugging will be difficult without knowing the full command and error output. Consider logging the command and stderr, or wrapping errors with more context.
| const { stdout } = await execFileAsync('git', args, { | |
| maxBuffer: GIT_MAX_BUFFER, | |
| ...options | |
| }) | |
| return stdout.trim() | |
| const command = `git ${Array.isArray(args) ? args.join(' ') : String(args)}` | |
| try { | |
| const { stdout, stderr } = await execFileAsync('git', args, { | |
| maxBuffer: GIT_MAX_BUFFER, | |
| ...options | |
| }) | |
| if (stderr && stderr.trim()) { | |
| log(`git stderr for "${command}": ${stderr.trim()}`) | |
| } | |
| return stdout.trim() | |
| } catch (error) { | |
| const stderr = error && error.stderr ? String(error.stderr).trim() : '' | |
| const messageParts = [`git command failed: ${command}`] | |
| if (options && options.cwd) { | |
| messageParts.push(`cwd: ${options.cwd}`) | |
| } | |
| if (stderr) { | |
| messageParts.push(`stderr: ${stderr}`) | |
| } | |
| const wrappedError = new Error(messageParts.join(' | ')) | |
| // Preserve the original error for callers that need more detail. | |
| wrappedError.cause = error | |
| throw wrappedError | |
| } |
| if (!branchInfo?.commit?.sha) { | ||
| return | ||
| } |
There was a problem hiding this comment.
The test silently passes if branchInfo?.commit?.sha is undefined, which could hide real failures. Consider failing the test or logging a skip reason when the condition is met, to distinguish between a legitimate skip and an unexpected missing SHA.
Closes #4