Phase 3 implementation complete#5
Conversation
There was a problem hiding this comment.
Pull request overview
Implements “Phase 3” features across the CodeGraph backend + client, including GitHub PR webhook ingestion, PR impact comments, shareable graphs, function-level graph expansion, multi-language scanning/parsing, and observability/test tooling.
Changes:
- Add GitHub webhook handling + PR diff parsing + automated PR impact comment posting.
- Add graph sharing via tokens and function-level nodes stored/queryable from the API/UI.
- Add Sentry wiring, test infrastructure (Vitest + CI), and supporting docs/migrations.
Reviewed changes
Copilot reviewed 69 out of 86 changed files in this pull request and generated 10 comments.
Show a summary per file
| File | Description |
|---|---|
| skills-lock.json | Adds a new skill source entry. |
| server/vitest.config.js | Adds Vitest config + coverage thresholds. |
| server/test/pr-comment.test.js | Adds Node test coverage for PR comment routes/service. |
| server/test/parser.multilang.test.js | Adds Node test for Python/Go parsing via workers. |
| server/test/github.webhook.test.js | Adds Node test coverage for GitHub webhook endpoint. |
| server/test/ai.queries.test.js | Adds integration test for /api/ai/queries. |
| server/src/services/ImpactAnalysisService.js | Adds impact analysis logic over stored graph edges. |
| server/src/services/GitHubPRService.js | Adds GitHub API client + diff parsing + comment formatting/posting. |
| server/src/middleware/planGuard.middleware.js | Adds plan/auth guard + user resolution helper. |
| server/src/infrastructure/migrations/004_analysis_jobs_metadata.sql | Adds analysis_jobs.metadata + audit_logs table. |
| server/src/infrastructure/migrations/003_share_tokens.sql | Adds graph_shares table for share links. |
| server/src/infrastructure/migrations/002_function_nodes.sql | Adds function_nodes table for function-level info. |
| server/src/infrastructure/migrations/001_initial.sql | Updates users.plan comment; (schema also contains repos is_starred). |
| server/src/api/webhooks/pr-comment.routes.js | Adds route to post/update PR impact comments after analysis. |
| server/src/api/webhooks/github.webhook.js | Adds signed GitHub webhook handler that enqueues analysis jobs. |
| server/src/api/share/routes/share.routes.js | Adds API to resolve a share token to a graph payload. |
| server/src/api/share/index.js | Exports share router. |
| server/src/api/repositories/routes/repositories.routes.js | Adds repo “star” field and toggle endpoint; sorts starred first. |
| server/src/api/graph/services/graphPayload.service.js | Extracts shared graph payload loading/caching logic. |
| server/src/api/graph/routes/graph.routes.js | Adds share creation endpoint + function-node endpoint + refactors graph GET. |
| server/src/api/ai/routes/ai.routes.js | Adds query history endpoint and OpenAI streaming explain endpoint; improves rate-limit keying. |
| server/src/agents/scanner/ScannerAgent.js | Expands scan extensions to include Python/Go. |
| server/src/agents/persistence/PersistenceAgent.js | Persists function-level nodes to DB. |
| server/src/agents/parser/pythonWorker.js | Adds Python parser worker (imports/declarations). |
| server/src/agents/parser/parseWorker.js | Enhances JS/TS parser worker with call-collection + functionNodes output. |
| server/src/agents/parser/goWorker.js | Adds Go parser worker (imports/declarations). |
| server/src/agents/parser/tests/ParserAgent.test.js | Adds Vitest unit test for parser routing to language workers. |
| server/src/agents/parser/ParserAgent.js | Routes parsing to JS/Python/Go workers; tweaks worker exec args. |
| server/src/agents/graph/tests/GraphBuilderAgent.test.js | Adds Vitest test ensuring functionNodes are preserved in graph output. |
| server/src/agents/graph/GraphBuilderAgent.js | Adds Python/Go resolve extensions + surfaces functionNodes in result payload. |
| server/src/agents/core/tests/confidence.test.js | Adds Vitest tests for confidence scoring helpers. |
| server/src/agents/core/tests/SupervisorAgent.test.js | Adds Vitest tests for supervision retry/abort behavior. |
| server/src/agents/core/SupervisorAgent.js | Persists functionNodes; attempts PR comment posting after completion. |
| server/package.json | Adds deps/scripts for Vitest, supertest, axios, Sentry; updates migrate/test commands. |
| server/index.js | Initializes Sentry if configured. |
| server/coverage/lcov-report/sorter.js | Adds committed coverage artifact. |
| server/coverage/lcov-report/sort-arrow-sprite.png | Adds committed coverage artifact (binary). |
| server/coverage/lcov-report/prettify.css | Adds committed coverage artifact. |
| server/coverage/lcov-report/parser/index.html | Adds committed coverage artifact. |
| server/coverage/lcov-report/index.html | Adds committed coverage artifact. |
| server/coverage/lcov-report/graph/index.html | Adds committed coverage artifact. |
| server/coverage/lcov-report/favicon.png | Adds committed coverage artifact (binary). |
| server/coverage/lcov-report/core/index.html | Adds committed coverage artifact. |
| server/coverage/lcov-report/block-navigation.js | Adds committed coverage artifact. |
| server/coverage/lcov-report/base.css | Adds committed coverage artifact. |
| server/app.js | Registers new routers; adds raw webhook middleware; adds Sentry error handler. |
| server/Dockerfile | Updates exposed port to 5000. |
| server/.env.example | Documents webhook/token/Sentry env vars. |
| docs/SECTION_8_2_SUMMARY.md | Adds executive summary doc for PR impact comments. |
| docs/SECTION_8_2_INTEGRATION.md | Adds integration checklist doc for PR comments. |
| docs/Phase3/PHASE2_AUDIT.md | Adds Phase 2 audit doc. |
| docs/GITHUB_WEBHOOK_SETUP.md | Adds webhook setup guide doc. |
| docker-compose.yml | Adds npm install before migrate/dev on startup. |
| client/src/main.jsx | Adds Sentry init + ErrorBoundary wrapper. |
| client/src/features/graph/slices/graphSlice.js | Adds thunk to load shared graphs. |
| client/src/features/graph/services/graphService.js | Adds shared-graph fetch, function-nodes fetch, share link creation. |
| client/src/features/graph/pages/GraphPage.jsx | Adds shared graph loading and query history panel. |
| client/src/features/graph/components/GraphView.jsx | Adds function-node expansion on double-click and safer node click handling. |
| client/src/features/graph/components/GraphToolbar.jsx | Adds “Share” button and clipboard copy helper. |
| client/src/features/graph/components/AnalyzeForm.jsx | Adds “reanalyze” pre-fill behavior via router state. |
| client/src/features/dashboard/slices/dashboardSlice.js | Adds thunk for starring repositories with optimistic UI. |
| client/src/features/dashboard/services/dashboardService.js | Aligns API base URL usage; adds toggle star API call. |
| client/src/features/dashboard/pages/DashboardPage.jsx | Adds star/re-analyze controls and starred-first sorting. |
| client/src/features/dashboard/index.js | Re-exports toggleRepositoryStar. |
| client/src/features/ai/services/aiService.js | Adds query history fetch + streaming explain over SSE-style fetch. |
| client/src/features/ai/index.js | Exports QueryHistory. |
| client/src/features/ai/components/QueryHistory.jsx | Adds UI for recent AI queries per job. |
| client/src/features/ai/components/QueryBar.jsx | Fixes loading/status mapping to match slice state. |
| client/src/features/ai/components/AiPanel.jsx | Adds streaming explanation + impact simulation controls + summary section. |
| client/package.json | Adds Sentry client deps. |
| client/package-lock.json | Locks Sentry deps. |
| client/.env.example | Updates defaults + adds Sentry env vars. |
| .github/workflows/ci.yml | Adds CI workflow (server tests+coverage + client build). |
| .claude/skills/nodejs-best-practices/SKILL.md | Adds Node best practices skill doc. |
| .agents/skills/nodejs-best-practices/SKILL.md | Adds Node best practices skill doc (duplicate location). |
Files not reviewed (1)
- client/package-lock.json: Language not supported
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if (data?.jobId && !String(data.jobId).startsWith('shared:')) { | ||
| // Avoid replacing an existing private graph without explicit confirmation. | ||
| if (!window.confirm('Load shared graph? This will replace your current view.')) return; | ||
| } | ||
| dispatch(loadSharedGraph({ token: shareToken })); | ||
| }, [data?.jobId, dispatch, shareToken]); |
There was a problem hiding this comment.
This useEffect will keep prompting and re-dispatching when a shared graph finishes loading: data.jobId becomes a UUID (doesn't start with shared:), so the confirmation branch stays true and the effect re-runs due to the data?.jobId dependency. Track whether the currently loaded graph is already the shared one (e.g., compare rootDir to shared:${shareToken} or store a loadedShareToken flag) and remove data?.jobId from the dependency/guard accordingly.
| if (data?.jobId && !String(data.jobId).startsWith('shared:')) { | |
| // Avoid replacing an existing private graph without explicit confirmation. | |
| if (!window.confirm('Load shared graph? This will replace your current view.')) return; | |
| } | |
| dispatch(loadSharedGraph({ token: shareToken })); | |
| }, [data?.jobId, dispatch, shareToken]); | |
| const isCurrentGraphShared = data?.rootDir === `shared:${shareToken}`; | |
| if (isCurrentGraphShared) return; | |
| if (data?.jobId && !String(data.jobId).startsWith('shared:')) { | |
| // Avoid replacing an existing private graph without explicit confirmation. | |
| if (!window.confirm('Load shared graph? This will replace your current view.')) return; | |
| } | |
| dispatch(loadSharedGraph({ token: shareToken })); | |
| }, [data?.rootDir, dispatch, shareToken]); |
| // Mock dependencies | ||
| const mockPgPool = { | ||
| query: async (sql, params) => { | ||
| // Mock job lookup | ||
| if (sql.includes('FROM analysis_jobs')) { | ||
| if (params[0] === 'valid-job-id') { | ||
| return { | ||
| rowCount: 1, | ||
| rows: [ | ||
| { | ||
| id: 'valid-job-id', | ||
| status: 'complete', | ||
| branch: 'feature/new-ui', | ||
| repositoryId: 'repo-123', | ||
| github_owner: 'myorg', | ||
| github_repo: 'myrepo', | ||
| prNumber: '42', | ||
| prTitle: 'Add new UI', | ||
| }, | ||
| ], | ||
| }; | ||
| } | ||
| return { rowCount: 0, rows: [] }; | ||
| } | ||
|
|
||
| // Mock audit log insertion | ||
| if (sql.includes('INSERT INTO audit_logs')) { | ||
| return { rowCount: 1, rows: [{ id: 'log-id' }] }; | ||
| } | ||
|
|
||
| return { rowCount: 0, rows: [] }; | ||
| }, |
There was a problem hiding this comment.
This test file defines mocks (e.g., mockPgPool) but never injects them into prCommentRouter, which imports the real pgPool singleton. As written, these tests will hit the real database (or fail if not configured), and assertions like the "valid job" path won’t work. Refactor the router/service to accept dependencies (pgPool/GitHubPRService) for testing, or use the test runner’s module-mocking facilities to replace the imported singletons.
| "migrate": "psql \"$DATABASE_URL\" -v ON_ERROR_STOP=1 -f ./src/infrastructure/migrations/001_initial.sql || true; psql \"$DATABASE_URL\" -v ON_ERROR_STOP=1 -f ./src/infrastructure/migrations/002_function_nodes.sql || true; psql \"$DATABASE_URL\" -v ON_ERROR_STOP=1 -f ./src/infrastructure/migrations/003_share_tokens.sql || true; psql \"$DATABASE_URL\" -v ON_ERROR_STOP=1 -f ./src/infrastructure/migrations/004_analysis_jobs_metadata.sql || true", | ||
| "db:migrate": "npm run migrate", |
There was a problem hiding this comment.
The migration script chains multiple psql ... || true commands. Because of || true, any failure (even with ON_ERROR_STOP=1) is ignored and the container can start with a partially migrated schema. Consider removing || true and failing fast (or using && / a small Node script) so schema errors are surfaced immediately.
| parseDiff(diff) { | ||
| const changedFiles = []; | ||
| const lines = diff.split('\n'); | ||
|
|
||
| for (const line of lines) { | ||
| // Matches: "diff --git a/path/file.js b/path/file.js" | ||
| const match = line.match(/^diff --git a\/(.*?) b\/(.*?)$/); | ||
| if (!match) continue; | ||
|
|
||
| const filePath = match[2]; | ||
|
|
||
| // Determine status from next lines | ||
| let status = 'modified'; | ||
| if (diff.includes(`new file mode`) && diff.lastIndexOf(`new file mode`) > diff.indexOf(line)) { | ||
| status = 'added'; | ||
| } else if (diff.includes(`deleted file mode`) && diff.lastIndexOf(`deleted file mode`) > diff.indexOf(line)) { | ||
| status = 'deleted'; | ||
| } |
There was a problem hiding this comment.
parseDiff() determines file status by searching the entire diff for new file mode / deleted file mode after the current diff --git line. With multi-file diffs this can mislabel unrelated files (e.g., one added file causes later modified files to be marked as added). Parse status within the current file’s diff hunk only (scan forward until the next diff --git header).
| router.get('/:jobId/functions/*filePath', async (req, res, next) => { | ||
| const { jobId } = req.params; | ||
| const wildcardPath = req.params?.filePath; | ||
| const rawFilePath = Array.isArray(wildcardPath) | ||
| ? wildcardPath.join('/') | ||
| : String(wildcardPath || '').trim(); | ||
|
|
There was a problem hiding this comment.
The route pattern /:jobId/functions/*filePath is not a valid Express (path-to-regexp) named wildcard; req.params.filePath will be undefined and this handler will return 400 for all requests. Use /:jobId/functions/* and read req.params[0], or use a regex param like /:jobId/functions/:filePath(*) so the full path is captured reliably.
|
|
||
| describe('configuration', () => { | ||
| it('detects when GitHub token is configured', () => { | ||
| const oldToken = process.env.GITHUB_TOKEN; | ||
| process.env.GITHUB_TOKEN = 'test-token'; | ||
|
|
||
| const service = require('../src/services/GitHubPRService.js').default; | ||
| assert.equal(service.isConfigured(), true); | ||
|
|
||
| process.env.GITHUB_TOKEN = oldToken; | ||
| }); | ||
|
|
||
| it('detects when GitHub token is missing', () => { | ||
| const oldToken = process.env.GITHUB_TOKEN; | ||
| delete process.env.GITHUB_TOKEN; | ||
|
|
||
| const service = require('../src/services/GitHubPRService.js').default; |
There was a problem hiding this comment.
This file uses require('../src/services/GitHubPRService.js') to load an ES module (server/package.json sets "type": "module" and the service uses import). Node will throw ERR_REQUIRE_ESM here. Use await import() instead (or avoid re-importing by exposing a factory that can be instantiated with an env-provided token).
| - name: Run tests | ||
| run: npm run test:coverage | ||
| working-directory: server |
There was a problem hiding this comment.
CI only runs npm run test:coverage (Vitest unit tests) and never runs the Node test suite under server/test/** (npm test). This means the newly added webhook/PR-comment tests won’t execute in CI. Add a separate step to run npm test (or migrate those tests to Vitest) so regressions in these endpoints are caught.
| - /app/node_modules | ||
| command: sh -c " | ||
| until pg_isready -h postgres -p 5432; do sleep 1; done; | ||
| npm install; |
There was a problem hiding this comment.
Running npm install on every container start will slow down startup and introduces nondeterminism (depends on registry availability), especially since dependencies should already be installed via the image build or cached volumes. Prefer installing during the Docker image build (or only when node_modules is missing) and keep docker-compose startup focused on running migrations + the app.
| npm install; | |
| if [ ! -d node_modules ]; then npm install; fi; |
| app.use(cookieParser()); | ||
| app.use('/api/webhooks/github', express.raw({ type: 'application/json' })); | ||
| app.use(express.json()); | ||
| app.use(express.urlencoded({ extended: true })); |
There was a problem hiding this comment.
express.raw() is mounted on /api/webhooks/github for all sub-routes, which will consume the request body and leave req.body as a Buffer for /api/webhooks/github/pr-comment. That route expects JSON ({ jobId }) and will start returning 400 in production. Scope the raw-body middleware to the GitHub webhook endpoint only (e.g., only /api/webhooks/github POST handler) or mount prCommentRouter under a different base path not covered by express.raw().
| router.post('/:jobId/share', async (req, res, next) => { | ||
| const { jobId } = req.params; | ||
| const visibility = String(req.body?.visibility || 'unlisted').trim().toLowerCase(); | ||
| const expiresAtInput = req.body?.expiresAt; | ||
|
|
||
| const deadCodeCandidates = []; | ||
| const graph = {}; | ||
| if (!jobId) { | ||
| return res.status(400).json({ error: 'jobId is required.' }); | ||
| } | ||
|
|
||
| for (const node of nodesResult.rows) { | ||
| if (node.is_dead_code) deadCodeCandidates.push(node.file_path); | ||
| if (!SHARE_VISIBILITY.has(visibility)) { | ||
| return res.status(400).json({ error: 'visibility must be either unlisted or public.' }); | ||
| } | ||
|
|
||
| graph[node.file_path] = { | ||
| deps: depsBySource.get(node.file_path) || [], | ||
| type: node.file_type, | ||
| declarations: node.declarations || [], | ||
| metrics: node.metrics || {}, | ||
| summary: node.summary || null, | ||
| }; | ||
| let expiresAt = null; | ||
| if (expiresAtInput !== undefined && expiresAtInput !== null && String(expiresAtInput).trim() !== '') { | ||
| const parsed = new Date(expiresAtInput); | ||
| if (Number.isNaN(parsed.getTime())) { | ||
| return res.status(400).json({ error: 'expiresAt must be a valid ISO date string.' }); | ||
| } | ||
| expiresAt = parsed.toISOString(); | ||
| } | ||
|
|
||
| const responsePayload = { | ||
| graph, | ||
| edges, | ||
| topology: { | ||
| nodeCount: nodesResult.rowCount, | ||
| edgeCount: edgesResult.rowCount, | ||
| deadCodeCandidates, | ||
| }, | ||
| }; | ||
|
|
||
| await writeJsonCache( | ||
| redisClient, | ||
| graphCacheKey, | ||
| responsePayload, | ||
| cacheTtl.graphSeconds, | ||
| const token = crypto.randomBytes(24).toString('base64url'); | ||
|
|
||
| try { | ||
| const inserted = await pgPool.query( | ||
| ` | ||
| INSERT INTO graph_shares (job_id, token, visibility, expires_at) | ||
| VALUES ($1, $2, $3, $4) | ||
| RETURNING token, visibility, expires_at | ||
| `, | ||
| [jobId, token, visibility, expiresAt], | ||
| ); |
There was a problem hiding this comment.
POST /api/graph/:jobId/share allows creating share tokens for any jobId without any authentication/authorization check. If job IDs can be guessed/leaked, this enables unauthorized sharing of private graphs. Require authentication and verify the analysis job belongs to the requesting user before inserting into graph_shares.
|
@copilot Apply all the suggestions and comments you have generated |
…, body parsing scope, CI, Docker Agent-Logs-Url: https://github.com/SamanPandey-in/codegraph-ai/sessions/e644ac39-fbd1-4ebc-8735-041f6eb88851 Co-authored-by: SamanPandey-in <171229634+SamanPandey-in@users.noreply.github.com>
Agent-Logs-Url: https://github.com/SamanPandey-in/codegraph-ai/sessions/e644ac39-fbd1-4ebc-8735-041f6eb88851 Co-authored-by: SamanPandey-in <171229634+SamanPandey-in@users.noreply.github.com>
All 10 suggestions have been applied in commits cc85ccd and d863a22:
Security Summary: CodeQL flagged missing rate-limiting on the modified routes (now resolved — added |
refer docs