Phase 1 kickoff: parser core + isolation - #21
Conversation
Starts Phase 1 (parser core + isolation) on top of the Phase 0 scaffold. Adds docs/PHASE1_TASKS.md as the working task list for the phase: ten ordered tasks with acceptance criteria, open decisions to confirm, and a Definition of Done checklist. Documentation only; implementation follows in subsequent commits.
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughPhase 1 adds TypeScript tree-sitter extraction into an IR graph, CLI JSON and summary output, stronger traversal and container controls, golden fixtures, separated CI workflows, and synchronized product and implementation documentation. ChangesParser Phase 1
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant CLI
participant Extract
participant TreeSitter
participant Graph
CLI->>Extract: Request repository extraction
Extract->>TreeSitter: Parse TypeScript and run queries
TreeSitter-->>Extract: Return declaration and call captures
Extract->>Graph: Populate files, functions, calls, and imports
Graph-->>CLI: Emit JSON or summary output
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Comment |
…t_statement query syntax
…s, and qualified names
…ction, imports, and JSON output
There was a problem hiding this comment.
Pull request overview
This PR kicks off Phase 1 (parser core + isolation) by adding/expanding the Phase 1 execution plan and implementing a first end-to-end TypeScript extraction pipeline (tree-sitter queries → IR graph) with initial fixtures, golden outputs, and CI/workflow updates.
Changes:
- Added Phase 1 planning/contract docs (PRD, task lists, parsing strategy updates, model handoff notes).
- Implemented initial TypeScript query loading + extraction to
ir.Graph, plus golden/fixture testdata. - Updated parser runtime/packaging (CLI output flags, Dockerfile) and split CI workflows (Go vs Node).
Reviewed changes
Copilot reviewed 29 out of 29 changed files in this pull request and generated 12 comments.
Show a summary per file
| File | Description |
|---|---|
| TASKLIST.md | Phase 1 chunk-by-chunk execution checklist and acceptance criteria |
| services/parser/testdata/sample/arrows.ts | Adds arrow/function-expression and overload examples to sample fixtures |
| services/parser/testdata/nested/repo.ts | Adds nesting/class method fixture for qualified-name behavior |
| services/parser/testdata/golden/repo.ts | Adds comprehensive “golden” TS fixture covering defs/calls/imports |
| services/parser/testdata/golden/package.json | Adds minimal non-TS file to golden fixture repo |
| services/parser/testdata/golden/imports.ts | Adds golden import forms fixture |
| services/parser/testdata/golden/extract_expected.json | Stores expected extraction output for golden test |
| services/parser/testdata/golden/extract_actual.json | Stores “actual” extraction output (currently committed) |
| services/parser/testdata/golden/calls.ts | Adds golden calls fixture |
| services/parser/testdata/calls/repo.ts | Adds calls fixture including module-level call |
| services/parser/queries/typescript.scm | Extends tree-sitter queries (export source field + var-declarator functions) |
| services/parser/queries/embed.go | Embeds .scm query file into the binary |
| services/parser/out.json | Committed example output JSON (generated artifact) |
| services/parser/internal/ts/scope.go | Adds qualified-name scope walker for TS AST nodes |
| services/parser/internal/ts/queries.go | Adds query compilation/loading helpers |
| services/parser/internal/ts/queries_test.go | Tests query compilation against TS grammar |
| services/parser/internal/ts/extract.go | Implements extraction of Functions/Calls/Imports into ir.Graph |
| services/parser/internal/ts/extract_test.go | Golden test for extraction output |
| services/parser/internal/security/path.go | Tightens walk behavior (symlink guard + oversize logging/handling) |
| services/parser/internal/ir/ir.go | Updates ir.Import to store multiple symbols |
| services/parser/Dockerfile | Updates build/runtime images and non-root runtime user |
| services/parser/cmd/parser/main.go | Adds JSON output + summary format + --out flag |
| PRD.md | Adds product requirements + phase plan and locked decisions |
| docs/PHASE1_TASKS.md | Adds Phase 1 task plan and acceptance criteria |
| docs/PARSING_STRATEGY.md | Documents naming rules / resolution framing updates |
| docs/NEXT_MODEL_HANDOFF.md | Adds one-shot handoff context for future model sessions |
| CLAUDE.md | Updates phase status checklist |
| .github/workflows/node-ci.yml | Adds dedicated Node CI workflow |
| .github/workflows/go-ci.yml | Renames Go CI and removes Node steps from Go workflow |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| # Multi-stage build. Runtime runs with --network none, read-only mount, dropped | ||
| # caps, non-root (docs/SECURITY.md). CGO is required for tree-sitter. | ||
| FROM golang:1.22 AS build | ||
| FROM golang:1.24-alpine AS build |
| graph.Functions = append(graph.Functions, ir.Function{ | ||
| PackagePath: pkgPath, | ||
| Name: funcName, | ||
| QualifiedName: qualifiedName(*declNode, src, funcName), | ||
| OverloadIndex: 0, | ||
| StartLine: startLine, | ||
| EndLine: endLine, | ||
| Source: source, | ||
| }) |
| qs, err := loadQueries(lang) | ||
| if err != nil { | ||
| tree.Close() | ||
| return ir.Graph{}, fmt.Errorf("loadQueries: %w", err) | ||
| } |
| cursor = tree_sitter.NewQueryCursor() | ||
| callMatches := cursor.Matches(qs.call, tree.RootNode(), src) |
| cursor = tree_sitter.NewQueryCursor() | ||
| impMatches := cursor.Matches(qs.imp, tree.RootNode(), src) |
| actualFile := "../../testdata/golden/extract_actual.json" | ||
| if err := os.WriteFile(actualFile, actualData, 0644); err != nil { | ||
| t.Fatalf("WriteFile failed: %v", err) | ||
| } |
| { | ||
| "Files": [ | ||
| { | ||
| "Path": "calls.ts", | ||
| "Language": "typescript" |
| { | ||
| "Files": [ | ||
| { | ||
| "Path": "calls.ts", | ||
| "Language": "typescript" |
| - [x] Phase 0: Bootstrap | ||
| - [x] Phase 1: Parser & Isolation | ||
| - [ ] Phase 2: Storage & Resolution | ||
| - [ ] Phase 3: API, Auth, Canvas & Search | ||
| - [ ] Phase 4: Webhooks, Queue & Hardening |
| # TASKLIST — guided Phase 1 build (parser core + isolation) | ||
|
|
||
| > Working contract for the build. **You write the code; Copilot debugs/reviews/guides.** | ||
| > Finish one chunk → run its tests → "good when …" met → Copilot nudges you to the next. | ||
| > Source of truth: `PLAN.md` §3 Phase 1, `docs/PHASE1_TASKS.md`, `docs/PARSING_STRATEGY.md`, |
|
@coderabbitai full review |
✅ Action performedFull review finished. |
|
@copilot can you go through the PR and check that is all the comment that u gave are addressed properly or not |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 15
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.github/workflows/go-ci.yml (1)
28-28: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winDisable persisted checkout credentials in both workflows. Both workflows execute repository-controlled commands after checkout while leaving
GITHUB_TOKENin.git/config.
.github/workflows/go-ci.yml#L28-L28: addpersist-credentials: falseand setpermissions: contents: read..github/workflows/node-ci.yml#L12-L12: addpersist-credentials: falseand setpermissions: contents: read.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/go-ci.yml at line 28, Disable persisted checkout credentials in both workflows by configuring actions/checkout@v4 with persist-credentials: false and adding workflow permissions of contents: read. Apply this change in .github/workflows/go-ci.yml at lines 28-28 and .github/workflows/node-ci.yml at lines 12-12.Source: Linters/SAST tools
🧹 Nitpick comments (2)
services/parser/internal/ts/extract.go (1)
199-222: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winImport symbol walk captures both original and aliased identifiers.
walk()collects everyidentifiernode under the import/export statement. Forimport { named as alias } from "b", this pushes both"named"(the external name) and"alias"(the local binding) intoSymbols, rather than just the local binding that later resolution logic would actually need to match usages against.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@services/parser/internal/ts/extract.go` around lines 199 - 222, The symbol traversal in the import/export extraction block should record only locally bound import names. Update the walk rooted at stmt to recognize aliased imports and exclude the external identifier, while preserving unaliased names and other supported import forms in graph.Imports Symbols.services/parser/internal/ts/queries.go (1)
27-61: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winCompiling the same
.scmsource three times is redundant.
def,call, andimpare three separately-compiled*tree_sitter.Queryobjects that each contain all patterns intypescript.scm— they're structurally identical, differing only in which capture name is validated. This triples query-compile cost per call and (combined with howextract.gouses them) triples per-file AST traversal cost.Consider compiling once and validating all three capture names against a single
Query, then dispatching on capture name/index during a single traversal inextract.go.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@services/parser/internal/ts/queries.go` around lines 27 - 61, Refactor loadQueries and the compiledQueries representation to compile queries.TypeScriptSCM only once, then validate function.def, function.call, and import.from capture names against that shared Query. Update extract.go’s traversal to use the single query and dispatch results by capture name or index, preserving the existing extraction behavior while eliminating separate query objects and repeated AST traversal.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/node-ci.yml:
- Line 19: Update the dependency installation step in the Node CI workflow to
use pnpm’s frozen lockfile mode, removing the option that disables lockfile
validation while preserving the existing install command.
In `@CLAUDE.md`:
- Around line 11-15: Synchronize the Phase 1 status snapshot with the actual
implementation and acceptance state across CLAUDE.md lines 11-15, PRD.md lines
142-145, TASKLIST.md lines 9-25, docs/NEXT_MODEL_HANDOFF.md lines 17-20 and
103-116, and docs/PHASE1_TASKS.md lines 9-25; update each phase checklist,
release-plan entry, baseline, task starting point, and verified-facts section
consistently, with no locations left describing Phase 1 as complete, in
progress, or not started inconsistently.
In `@docs/PARSING_STRATEGY.md`:
- Around line 37-44: Update the “Naming Rules (C4/C5)” section in
PARSING_STRATEGY.md to document the locked per-file overload-index post-pass:
group identical qualified_name values, sort each group by start_line, and assign
overload_index values consistently with the database uniqueness key, including
anonymous and overloaded functions.
In `@docs/PHASE1_TASKS.md`:
- Around line 212-219: Update the “Naming/status notes (carry from Phase 0)”
section in PHASE1_TASKS.md so the overload-index, runtime-query loading,
clone/parse-container isolation, and .gitignore decisions are no longer labeled
open; mark each consistently with its locked or deferred status from PRD.md and
the handoff, while preserving the existing unresolved naming decision and
explicitly deferred Phase 2 items.
In `@PRD.md`:
- Line 179: Update the query-loading documentation to consistently state that
the .scm source is embedded at build time and compiled at runtime: revise
PRD.md:179, TASKLIST.md:23-27, and docs/PHASE1_TASKS.md:33-34 accordingly, and
remove the hot-editability claim from docs/NEXT_MODEL_HANDOFF.md:52-53.
In `@services/parser/Dockerfile`:
- Line 3: Update the Dockerfile’s build-stage base image and runtime-stage base
image declarations (the FROM entries for Go and Alpine) to supported Go/Alpine
versions, and pin each image by its immutable digest. Preserve the existing
build and runtime stages while replacing the unsupported tags with supported,
digest-pinned references.
In `@services/parser/internal/ts/extract_test.go`:
- Around line 29-32: Update the test around the extract_actual.json write to
avoid modifying the repository: write generated output under t.TempDir() and,
when it differs from the expected golden data, include the
actual-versus-expected diff in the test failure. Do not overwrite the checked-in
artifact during normal test runs; use an explicit update mode only if artifact
regeneration is required.
- Line 16: Update the golden extraction test around ConfigFromEnv so it
constructs or passes an explicit security.Config with fixed fixture settings
instead of reading PARSER_* environment variables. Ensure Extract() uses
deterministic limits and skip rules before comparing against
extract_expected.json.
In `@services/parser/internal/ts/extract.go`:
- Around line 63-67: Move the loadQueries(lang) call out of the per-file loop
and execute it once before iterating over paths, returning the existing wrapped
error if loading fails. Reuse the resulting qs for every file, and move its
Close call to after the loop so the shared compiled queries are released once
per run.
In `@services/parser/testdata/golden/extract_expected.json`:
- Around line 27-85: The extractor must assign file ownership consistently: use
FileID 1 for imports.ts and FileID 2 for repo.ts, including function and import
records. Update the extraction logic first, then regenerate
services/parser/testdata/golden/extract_expected.json (27-85 and 130-183),
services/parser/testdata/golden/extract_actual.json (27-85 and 130-183), and
services/parser/out.json (27-85 and 130-183) so all artifacts reflect the
corrected IDs.
In `@TASKLIST.md`:
- Around line 13-15: Change the “Legend” heading in TASKLIST.md from level three
to level two so it follows the top-level title with the required heading
hierarchy, while leaving the legend content unchanged.
- Around line 97-103: The read-site tasks in TASKLIST.md lines 97-103 and
docs/PHASE1_TASKS.md lines 97-103 still describe unbounded reads, although
extraction already uses io.LimitReader. Update both corresponding task
descriptions to remove stale unbounded-read requirements and retain only the
remaining truncation and binary-read requirements; make no code changes.
- Around line 175-184: Align the CI plans across TASKLIST.md (lines 175-184) and
docs/PHASE1_TASKS.md (lines 172-181): reference the actual split workflow path
in TASKLIST.md, retain the parser, parser-sample, and migration-check jobs, and
update docs/PHASE1_TASKS.md to include parser-sample so both plans enumerate the
same jobs and acceptance criteria.
- Around line 156-168: The Docker acceptance instructions use go run in a
minimal runtime image that lacks Go. Update the acceptance command in
TASKLIST.md lines 156-168 and docs/PHASE1_TASKS.md lines 150-168 to invoke the
built parser binary, or explicitly switch to a development/build image that
includes Go; keep both plan descriptions consistent.
- Around line 212-219: Update the decision-tracking entries in TASKLIST.md for
overload-index, embedded-query, and clone/parse-container choices to mark them
as decided rather than open, and link each entry to the canonical rationale in
PRD.md §11. Remove or reconcile duplicate unresolved-decision wording so only
the confirmed decisions remain active.
---
Outside diff comments:
In @.github/workflows/go-ci.yml:
- Line 28: Disable persisted checkout credentials in both workflows by
configuring actions/checkout@v4 with persist-credentials: false and adding
workflow permissions of contents: read. Apply this change in
.github/workflows/go-ci.yml at lines 28-28 and .github/workflows/node-ci.yml at
lines 12-12.
---
Nitpick comments:
In `@services/parser/internal/ts/extract.go`:
- Around line 199-222: The symbol traversal in the import/export extraction
block should record only locally bound import names. Update the walk rooted at
stmt to recognize aliased imports and exclude the external identifier, while
preserving unaliased names and other supported import forms in graph.Imports
Symbols.
In `@services/parser/internal/ts/queries.go`:
- Around line 27-61: Refactor loadQueries and the compiledQueries representation
to compile queries.TypeScriptSCM only once, then validate function.def,
function.call, and import.from capture names against that shared Query. Update
extract.go’s traversal to use the single query and dispatch results by capture
name or index, preserving the existing extraction behavior while eliminating
separate query objects and repeated AST traversal.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 16aa5af9-d140-410d-9d06-7b46a097c892
📒 Files selected for processing (29)
.github/workflows/go-ci.yml.github/workflows/node-ci.ymlCLAUDE.mdPRD.mdTASKLIST.mddocs/NEXT_MODEL_HANDOFF.mddocs/PARSING_STRATEGY.mddocs/PHASE1_TASKS.mdservices/parser/Dockerfileservices/parser/cmd/parser/main.goservices/parser/internal/ir/ir.goservices/parser/internal/security/path.goservices/parser/internal/ts/extract.goservices/parser/internal/ts/extract_test.goservices/parser/internal/ts/queries.goservices/parser/internal/ts/queries_test.goservices/parser/internal/ts/scope.goservices/parser/out.jsonservices/parser/queries/embed.goservices/parser/queries/typescript.scmservices/parser/testdata/calls/repo.tsservices/parser/testdata/golden/calls.tsservices/parser/testdata/golden/extract_actual.jsonservices/parser/testdata/golden/extract_expected.jsonservices/parser/testdata/golden/imports.tsservices/parser/testdata/golden/package.jsonservices/parser/testdata/golden/repo.tsservices/parser/testdata/nested/repo.tsservices/parser/testdata/sample/arrows.ts
| - [x] Phase 0: Bootstrap | ||
| - [x] Phase 1: Parser & Isolation | ||
| - [ ] Phase 2: Storage & Resolution | ||
| - [ ] Phase 3: API, Auth, Canvas & Search | ||
| - [ ] Phase 4: Webhooks, Queue & Hardening |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Synchronize the Phase 1 status snapshot across all source-of-truth documents.
The documents currently claim three incompatible states: Phase 1 is complete, current/in progress, or not started. Align every listed location with the actual implementation and acceptance state before merging.
CLAUDE.md#L11-L15: update the load-bearing phase checklist.PRD.md#L142-L145: update the release-plan status.TASKLIST.md#L9-L25: refresh the Phase 0/Phase 1 baseline.docs/NEXT_MODEL_HANDOFF.md#L17-L20: refresh the handoff status.docs/NEXT_MODEL_HANDOFF.md#L103-L116: refresh the “verified facts.”docs/PHASE1_TASKS.md#L9-L25: refresh the Phase 0 baseline and task starting point.
📍 Affects 5 files
CLAUDE.md#L11-L15(this comment)PRD.md#L142-L145TASKLIST.md#L9-L25docs/NEXT_MODEL_HANDOFF.md#L17-L20docs/NEXT_MODEL_HANDOFF.md#L103-L116docs/PHASE1_TASKS.md#L9-L25
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@CLAUDE.md` around lines 11 - 15, Synchronize the Phase 1 status snapshot with
the actual implementation and acceptance state across CLAUDE.md lines 11-15,
PRD.md lines 142-145, TASKLIST.md lines 9-25, docs/NEXT_MODEL_HANDOFF.md lines
17-20 and 103-116, and docs/PHASE1_TASKS.md lines 9-25; update each phase
checklist, release-plan entry, baseline, task starting point, and verified-facts
section consistently, with no locations left describing Phase 1 as complete, in
progress, or not started inconsistently.
- Use pnpm frozen lockfile and disable persist-credentials in CI - Update Go/Alpine Dockerfile base images to pinned digests - Ensure Phase 1 completion status is synchronized across all documentation - Document overload-index post-pass in PARSING_STRATEGY.md - Explicit security config and diff reporting in extract_test.go - Resolve naming and status notes in Phase 1 docs
|
@coderabbitai I've pushed the requested changes! Here is the status of your findings:
|
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
echo '=== Changed files ==='
git diff --name-status origin/main...HEAD 2>/dev/null || git diff --name-status HEAD~1...HEAD
echo '=== Workflow, Dockerfile, and test files ==='
for f in .github/workflows/go-ci.yml .github/workflows/node-ci.yml services/parser/Dockerfile services/parser/internal/ts/extract_test.go; do
if [ -f "$f" ]; then
echo "--- $f"
nl -ba "$f" | sed -n '1,260p'
fi
done
echo '=== Parser extraction/query implementation outline ==='
ast-grep outline services/parser/internal/ts/extract.go --items all
ast-grep outline services/parser/internal/ts/queries.go --items all
ast-grep outline services/parser/internal/security --items all 2>/dev/null || true
echo '=== Targeted implementation excerpts ==='
for f in services/parser/internal/ts/extract.go services/parser/internal/ts/queries.go services/parser/internal/security/path.go services/parser/internal/security/config.go; do
if [ -f "$f" ]; then
echo "--- $f"
nl -ba "$f" | sed -n '1,360p'
fi
done
echo '=== Documentation completion and parser-decision references ==='
rg -n -i -C2 'phase 1|overload.index|overload_index|embed|unbounded|limitreader|file.?size|completed|complete' \
CLAUDE.md PRD.md TASKLIST.md docs 2>/dev/null || true
echo '=== Golden fixture file identifiers ==='
for f in services/parser/testdata/golden/extract_expected.json services/parser/testdata/golden/extract_actual.json; do
if [ -f "$f" ]; then
echo "--- $f"
rg -n -C1 '"(FileID|Path|ID)"' "$f"
fi
doneLength of output: 2983 🏁 Script executed: #!/bin/bash
set -euo pipefail
show_file() {
f="$1"
if [ -f "$f" ]; then
echo "--- $f"
sed -n '1,380p' "$f"
else
echo "--- MISSING: $f"
fi
}
echo '=== Workflows, Dockerfile, and golden test ==='
show_file .github/workflows/go-ci.yml
show_file .github/workflows/node-ci.yml
show_file services/parser/Dockerfile
show_file services/parser/internal/ts/extract_test.go
echo '=== Extraction, queries, and security configuration ==='
show_file services/parser/internal/ts/extract.go
show_file services/parser/internal/ts/queries.go
show_file services/parser/internal/security/path.go
find services/parser/internal/security -maxdepth 1 -type f -name '*.go' -print | sort | while IFS= read -r f; do
if [ "$f" != "services/parser/internal/security/path.go" ]; then
show_file "$f"
fi
done
echo '=== Embed/query declarations ==='
show_file services/parser/queries/embed.go
show_file services/parser/queries/typescript.scm
echo '=== Documentation references ==='
grep -RInE -C 2 'Phase 1|overload.index|overload_index|go:embed|unbounded|LimitReader|file.?size|completed|complete' \
CLAUDE.md PRD.md TASKLIST.md docs 2>/dev/null || true
echo '=== File identity fields in golden expected fixture ==='
grep -nE -C 1 '"(FileID|Path|ID)"' services/parser/testdata/golden/extract_expected.json 2>/dev/null || true
echo '=== Current diff for the directly claimed implementation files ==='
git diff --unified=60 origin/main...HEAD -- \
.github/workflows/go-ci.yml \
.github/workflows/node-ci.yml \
services/parser/Dockerfile \
services/parser/internal/ts/extract.go \
services/parser/internal/ts/extract_test.go \
services/parser/internal/ts/queries.go \
services/parser/queries/embed.go \
services/parser/queries/typescript.scm \
CLAUDE.md PRD.md TASKLIST.md docs/NEXT_MODEL_HANDOFF.md docs/PHASE1_TASKS.md docs/PARSING_STRATEGY.md \
2>/dev/null || trueLength of output: 134470
However, I can’t mark all findings addressed yet because the documentation still overstates what is implemented:
Your decision to defer alias-aware import extraction is reasonable as a documented Phase 2 limitation, and keeping separate compiled query handles is fine now that query loading is once per extraction run. ✏️ Learnings added
|
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
services/parser/internal/security/path.go (1)
65-68: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winSkip binary files before appending them to the parse set.
The current filter only checks size, so binary files under
MaxFileBytesare still passed to extraction. Add an explicit binary-content/file-type check and cover it with traversal tests; otherwise the parser can attempt to extract graph data from non-source assets.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@services/parser/internal/security/path.go` around lines 65 - 68, Update the file traversal logic around the existing oversized-file check to detect and skip binary files before adding them to the parse set, while preserving the MaxFileBytes behavior for source files. Reuse the project’s existing binary-content or file-type detection utility if available, and add traversal coverage confirming binary assets are excluded.TASKLIST.md (1)
183-195: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winSynchronize
docs/SECURITY.mdbefore marking C14 complete.C14 says the security documentation reflects implemented controls, but
docs/SECURITY.mdremains inconsistent with the other Phase 1 documentation. Do not mark this documentation-sync task complete until the security claims agree with the implementation.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@TASKLIST.md` around lines 183 - 195, Update docs/SECURITY.md to document the implemented Phase 1 security controls, including non-root execution, --network none, read-only rootfs, dropped capabilities, no symlink following, size/count/binary limits, and the clone-versus-parse container split. Replace aspirational or unsupported claims with implementation-aligned wording, then mark C14 complete only after the documented security claims match the code.docs/PHASE1_TASKS.md (1)
185-195: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winSynchronize
docs/SECURITY.mdbefore marking documentation complete.This task claims the security documentation has been replaced with implemented controls, but
docs/SECURITY.mdremains inconsistent with the other Phase 1 documents.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/PHASE1_TASKS.md` around lines 185 - 195, Update docs/SECURITY.md to document the implemented Phase 1 security controls, including non-root execution, --network none, read-only access, symlink restrictions, size/count/binary caps, and the clone-versus-parse container split. Remove remaining aspirational or TODO bullets so the security documentation matches the implementation and other Phase 1 documents.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/PHASE1_TASKS.md`:
- Around line 171-180: The Task 9 CI requirements in PHASE1_TASKS.md do not
match the workflows; update the parser CI workflows to execute go vet, race
tests, builds, parser-sample count assertions, and migration-check schema
validation for changes under services/parser/**. Ensure the completion criterion
accurately reflects these required jobs, or explicitly downgrade it if any
checks remain unimplemented.
- Around line 9-25: Update the Phase 1 status and acceptance-criteria claims in
the “Current state” section so they no longer describe binary-file isolation or
binary skipping as complete. If the implementation is not being added here, mark
Phase 1 as incomplete and explicitly list binary detection/skipping and its
tests as remaining work; do not claim security.Walk or ts extraction is
production-complete for this behavior.
In `@services/parser/internal/security/path.go`:
- Around line 58-60: Update the downstream extraction/opening flow associated
with this directory-entry check to resolve paths from root using no-follow,
descriptor-based or component-wise traversal, rejecting symlinks at open/read
time rather than trusting d.Type() alone. Keep the existing d.Type() check in
place as an early rejection, but ensure the returned path cannot be redirected
outside root between validation and opening.
In `@TASKLIST.md`:
- Line 9: Update TASKLIST.md to remove the completed status from binary
isolation, its negative test, and the full Phase 1 exit gate until binary-file
skipping is actually implemented and tested; mark those checklist items as
pending.
- Around line 170-181: The CI workflows must enforce every check claimed by the
C13 plan. Update the parser-related jobs in the Go and Node workflow
configurations to run go mod tidy validation, go vet, race-enabled tests,
builds, parser-sample count assertions, and migration-check table assertions
with the required CGO, caching, and Postgres setup; otherwise remove those
claims from TASKLIST.md.
---
Outside diff comments:
In `@docs/PHASE1_TASKS.md`:
- Around line 185-195: Update docs/SECURITY.md to document the implemented Phase
1 security controls, including non-root execution, --network none, read-only
access, symlink restrictions, size/count/binary caps, and the clone-versus-parse
container split. Remove remaining aspirational or TODO bullets so the security
documentation matches the implementation and other Phase 1 documents.
In `@services/parser/internal/security/path.go`:
- Around line 65-68: Update the file traversal logic around the existing
oversized-file check to detect and skip binary files before adding them to the
parse set, while preserving the MaxFileBytes behavior for source files. Reuse
the project’s existing binary-content or file-type detection utility if
available, and add traversal coverage confirming binary assets are excluded.
In `@TASKLIST.md`:
- Around line 183-195: Update docs/SECURITY.md to document the implemented Phase
1 security controls, including non-root execution, --network none, read-only
rootfs, dropped capabilities, no symlink following, size/count/binary limits,
and the clone-versus-parse container split. Replace aspirational or unsupported
claims with implementation-aligned wording, then mark C14 complete only after
the documented security claims match the code.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 264a754a-c24d-4e82-8c3c-9bf174277dca
📒 Files selected for processing (13)
.github/workflows/go-ci.yml.github/workflows/node-ci.yml.gitignorePRD.mdTASKLIST.mddocs/NEXT_MODEL_HANDOFF.mddocs/PARSING_STRATEGY.mddocs/PHASE1_TASKS.mdservices/parser/Dockerfileservices/parser/internal/security/path.goservices/parser/internal/ts/extract.goservices/parser/internal/ts/extract_test.goservices/parser/testdata/golden/extract_expected.json
🚧 Files skipped from review as they are similar to previous changes (9)
- .github/workflows/node-ci.yml
- .github/workflows/go-ci.yml
- docs/PARSING_STRATEGY.md
- docs/NEXT_MODEL_HANDOFF.md
- services/parser/internal/ts/extract_test.go
- services/parser/testdata/golden/extract_expected.json
- services/parser/internal/ts/extract.go
- services/parser/Dockerfile
- PRD.md
| if d.Type()&os.ModeSymlink != 0 { | ||
| return os.ErrPermission | ||
| } |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy lift
Enforce symlink isolation at the open/read boundary.
This directory-entry check is vulnerable to a TOCTOU race: after d.Type() passes, the file or a parent directory can be replaced with a symlink before downstream extraction opens the returned path, allowing reads outside root. Use no-follow, descriptor-based/component-wise resolution when opening files; retain this check only as an early rejection.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@services/parser/internal/security/path.go` around lines 58 - 60, Update the
downstream extraction/opening flow associated with this directory-entry check to
resolve paths from root using no-follow, descriptor-based or component-wise
traversal, rejecting symlinks at open/read time rather than trusting d.Type()
alone. Keep the existing d.Type() check in place as an early rejection, but
ensure the returned path cannot be redirected outside root between validation
and opening.
There was a problem hiding this comment.
Skipped. The parser environment executes inside an isolated container (--network none, read-only rootfs, dropped caps) where no concurrent processes exist to mutate the filesystem during the parse step. This makes a Time-Of-Check to Time-Of-Use (TOCTOU) symlink race condition functionally impossible to exploit. Introducing descriptor-based traversal (openat) adds unnecessary complexity for Phase 1 and conflicts with the directive to keep changes minimal.
There was a problem hiding this comment.
@ARCoder181105, the finding remains valid. Container read-only mode, dropped capabilities, and --network none restrict the parser process, but do not make an input repository immutable: with a host bind mount, the host (or any process that can modify the mounted source) can still swap a checked file or parent directory for a symlink between traversal and the later pathname-based open.
That said, deferring descriptor-based/no-follow traversal is a reasonable explicit Phase 1 scope decision. Please keep the current early symlink rejection, document this residual risk, and track hardened open-at-read-boundary handling for a later phase rather than treating the isolation controls as a complete mitigation.
| > `docs/SECURITY.md`, `docs/DATA_MODEL.md`, and `PRD.md`. | ||
|
|
||
| **Current branch:** `phase-1/parser-core-and-isolation` · **PR:** #21. | ||
| **Verified state:** Phase 0 skeleton and Phase 1 parser core with isolation are **complete and runnable**. |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy lift
Do not mark binary isolation and the exit gate complete yet.
The checklist claims binary skipping, its negative test, and the full Phase 1 gate are complete, but binary-file skipping is still unimplemented. Either add the sniff/skip path and tests or mark these items as pending.
Also applies to: 120-140, 201-213
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@TASKLIST.md` at line 9, Update TASKLIST.md to remove the completed status
from binary isolation, its negative test, and the full Phase 1 exit gate until
binary-file skipping is actually implemented and tested; mark those checklist
items as pending.
- fix(parser): append ir.File only after successful parse and limits checks - feat(security): implement binary file sniffing in Walk and Extract - ci: expand Go workflow to include vet, race, build, sample run, and migration check - docs(security): mark Phase 1 isolation controls as complete
|
@coderabbitai I've addressed all the remaining findings!
Thanks for noting the deferral on alias-aware imports! This should wrap up the Phase 1 parser implementation. |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
echo '=== Changed files relevant to the four findings ==='
git diff --name-status origin/main...HEAD -- \
.github/workflows/go-ci.yml \
docs/SECURITY.md \
services/parser/internal/security/path.go \
services/parser/internal/ts/extract.go
echo '=== Source outlines ==='
ast-grep outline services/parser/internal/security/path.go --items all
ast-grep outline services/parser/internal/ts/extract.go --items all
echo '=== Go CI workflow ==='
nl -ba .github/workflows/go-ci.yml | sed -n '1,280p'
echo '=== Security control documentation ==='
nl -ba docs/SECURITY.md | sed -n '1,320p'
echo '=== Binary checks and file-record append ordering ==='
rg -n -C 10 -i 'binary|nul|\\x00|LimitReader|MaxFileBytes|graph\.Files\s*=\s*append|Parse\(' \
services/parser/internal/security/path.go \
services/parser/internal/ts/extract.go
echo '=== Relevant diff ==='
git diff --unified=35 origin/main...HEAD -- \
.github/workflows/go-ci.yml \
docs/SECURITY.md \
services/parser/internal/security/path.go \
services/parser/internal/ts/extract.goLength of output: 2435 🏁 Script executed: #!/bin/bash
set -euo pipefail
show() {
file="$1"
echo "=== $file ==="
if [ -f "$file" ]; then
sed -n '1,360p' "$file"
else
echo "MISSING"
fi
}
show .github/workflows/go-ci.yml
show docs/SECURITY.md
show services/parser/internal/security/path.go
show services/parser/internal/ts/extract.go
echo '=== Relevant matches with context ==='
rg -n -C 12 -i 'binary|nul|\\x00|limitreader|maxfilebytes|graph\.files\s*=\s*append|parse\(' \
services/parser/internal/security/path.go \
services/parser/internal/ts/extract.go || true
echo '=== Current file state ==='
git status --short
echo '=== Relevant changed paths compared with PR base ==='
git diff --name-status origin/main...HEAD -- \
.github/workflows/go-ci.yml \
docs/SECURITY.md \
services/parser/internal/security/path.go \
services/parser/internal/ts/extract.go || trueLength of output: 21050
Please confirm the changes were pushed to PR |
|
@coderabbitai Here is the status on this latest round of findings:
|
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
echo '=== PR HEAD and targeted changes ==='
git status --short
git log -1 --format='%H%n%s'
git diff --name-status origin/main...HEAD -- \
.github/workflows/go-ci.yml \
docs/SECURITY.md \
services/parser/internal/security/path.go \
services/parser/internal/ts/extract.go \
services/parser/Dockerfile \
docker-compose.yml \
compose.yml 2>/dev/null || true
echo '=== Targeted file outlines ==='
ast-grep outline services/parser/internal/security/path.go --items all
ast-grep outline services/parser/internal/ts/extract.go --items all
echo '=== CI and security documentation ==='
for f in .github/workflows/go-ci.yml docs/SECURITY.md; do
echo "--- $f"
nl -ba "$f" | sed -n '1,340p'
done
echo '=== Binary handling and successful-file append ordering ==='
rg -n -i -C 12 'binary|nul|\\x00|maxfilebytes|limitreader|graph\.files\s*=\s*append|parse\(' \
services/parser/internal/security/path.go \
services/parser/internal/ts/extract.go
echo '=== Docker/runtime isolation configuration references ==='
rg -n -i -C 5 --glob '!**/.git/**' \
'network.?none|read.only|read-only|cap.drop|cap_drop|docker run|security-opt|no-new-privileges|services/parser' \
README.md CLAUDE.md PRD.md TASKLIST.md docs services/parser .github 2>/dev/null || trueLength of output: 2802 🏁 Script executed: #!/bin/bash
set -euo pipefail
print_numbered() {
file="$1"
echo "--- $file"
if [ -f "$file" ]; then
awk '{ printf "%6d\t%s\n", NR, $0 }' "$file" | sed -n '1,380p'
else
echo "MISSING"
fi
}
echo '=== Targeted current file contents ==='
print_numbered .github/workflows/go-ci.yml
print_numbered docs/SECURITY.md
print_numbered services/parser/internal/security/path.go
print_numbered services/parser/internal/ts/extract.go
print_numbered services/parser/Dockerfile
echo '=== Binary checks, append ordering, and CI exit-gate commands ==='
rg -n -i -C 8 \
'binary|nul|\\x00|readatleast|maxfilebytes|limitreader|graph\.files\s*=\s*append|go vet|go test.*race|go build|migration|sample' \
.github/workflows/go-ci.yml \
docs/SECURITY.md \
services/parser/internal/security/path.go \
services/parser/internal/ts/extract.go || true
echo '=== Candidate runtime/container configuration files ==='
fd -HI -t f \
'(^|/)(docker-compose|compose)(\..+)?\.(ya?ml)$|(^|/)Dockerfile$|\.sh$|\.md$' \
. | sed -n '1,240p'
echo '=== Container invocation/isolation references ==='
rg -n -i -C 6 --glob '!**/.git/**' \
'docker\s+run|--network(=|\s+)none|--read-only|read_only:|cap-drop|cap_drop:|no-new-privileges|volume|mount|services/parser' \
. 2>/dev/null || trueLength of output: 95396
On the TOCTOU point, Please push the intended commit to |
|
@coderabbitai I pushed the changes now u check |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
echo '=== Current PR head ==='
git status --short
git log -1 --format='commit=%H%nsubject=%s%ncommitted=%cI'
echo '=== Relevant paths changed since PR base ==='
git diff --name-status origin/main...HEAD -- \
.github/workflows/go-ci.yml \
docs/SECURITY.md \
services/parser/internal/security/path.go \
services/parser/internal/ts/extract.go \
services/parser/Dockerfile \
docker-compose.yml \
compose.yml 2>/dev/null || true
echo '=== Current workflow and security documentation ==='
for file in .github/workflows/go-ci.yml docs/SECURITY.md; do
echo "--- $file"
nl -ba "$file" | sed -n '1,360p'
done
echo '=== Implementation outlines ==='
ast-grep outline services/parser/internal/security/path.go --items all
ast-grep outline services/parser/internal/ts/extract.go --items all
echo '=== Binary detection and extraction record lifecycle ==='
rg -n -i -C 14 \
'binary|nul|\\x00|readatleast|limitreader|maxfilebytes|graph\.files\s*=\s*append|parse\(' \
services/parser/internal/security/path.go \
services/parser/internal/ts/extract.go || true
echo '=== CI command targets and repository support files ==='
rg -n -i -C 4 \
'go vet|go test.*race|go build|sample|migration|schema' \
.github/workflows/go-ci.yml \
README.md CLAUDE.md TASKLIST.md PRD.md docs services/parser 2>/dev/null || true
echo '=== Relevant current diff ==='
git diff --unified=45 origin/main...HEAD -- \
.github/workflows/go-ci.yml \
docs/SECURITY.md \
services/parser/internal/security/path.go \
services/parser/internal/ts/extract.goLength of output: 2377 🏁 Script executed: #!/bin/bash
set -euo pipefail
numbered() {
file="$1"
echo "--- $file"
if [ -f "$file" ]; then
awk '{ printf "%5d %s\n", NR, $0 }' "$file" | sed -n '1,380p'
else
echo "MISSING"
fi
}
echo '=== Current PR head ==='
git log -1 --format='commit=%H%nsubject=%s%ncommitted=%cI'
echo '=== Workflow and security documentation ==='
numbered .github/workflows/go-ci.yml
numbered docs/SECURITY.md
echo '=== Security traversal and TypeScript extraction implementation ==='
numbered services/parser/internal/security/path.go
numbered services/parser/internal/ts/extract.go
echo '=== Focused evidence ==='
rg -n -i -C 12 \
'binary|nul|\\x00|readatleast|limitreader|maxfilebytes|graph\.files\s*=\s*append|parse\(' \
services/parser/internal/security/path.go \
services/parser/internal/ts/extract.go || true
echo '=== CI exit-gate commands and referenced scripts/files ==='
rg -n -i -C 5 \
'go vet|go test.*race|go build|sample|migration|schema' \
.github/workflows/go-ci.yml \
README.md CLAUDE.md TASKLIST.md PRD.md docs services/parser 2>/dev/null || true
echo '=== Relevant diff from PR base ==='
git diff --unified=35 origin/main...HEAD -- \
.github/workflows/go-ci.yml \
docs/SECURITY.md \
services/parser/internal/security/path.go \
services/parser/internal/ts/extract.goLength of output: 97839
I verified the configuration and implementation by inspection; this sandbox does not execute the Go suite or GitHub Actions, so CI success still depends on the workflow run. The prior findings can be closed. |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/SECURITY.md`:
- Line 32: Update the symlink/path-traversal checklist item in SECURITY.md to
remain unchecked and qualify the protection as path-validation only; do not
claim source is fully protected before parsing or serving until descriptor-based
handling closes bind-mount TOCTOU races.
In `@services/parser/internal/security/path.go`:
- Around line 70-74: Update the path filtering logic around os.Open in the
WalkDir callback to process only regular files before opening them, avoiding
FIFOs and device entries. Check and handle errors from Read and Close, and
append the path only when opening, reading, and closing all succeed; preserve
the existing probe behavior for valid regular files.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 47c2aca0-c04a-4ecb-9260-6db3511aceb5
📒 Files selected for processing (4)
.github/workflows/go-ci.ymldocs/SECURITY.mdservices/parser/internal/security/path.goservices/parser/internal/ts/extract.go
🚧 Files skipped from review as they are similar to previous changes (2)
- .github/workflows/go-ci.yml
- services/parser/internal/ts/extract.go
…ty claim - Process only regular files in WalkDir to avoid FIFOs/devices - Strictly check Read/Close errors during binary sniffing - Downgrade symlink check in SECURITY.md to reflect deferred TOCTOU protection
|
@coderabbitai give me the final list of all the open comments that i havent addressed them yet it make me easier to work on them... |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
repo="ARCoder181105/funcatlas"
pr=21
echo '=== PR state ==='
gh pr view "$pr" --repo "$repo" --json number,url,state,headRefOid \
--jq '{number, url, state, headRefOid}'
echo '=== Review threads (resolved status and latest comment) ==='
gh api graphql \
-F owner="${repo%%/*}" \
-F name="${repo#*/}" \
-F number="$pr" \
-f query='
query($owner: String!, $name: String!, $number: Int!) {
repository(owner: $owner, name: $name) {
pullRequest(number: $number) {
reviewThreads(first: 100) {
nodes {
isResolved
isOutdated
path
line
originalLine
comments(first: 20) {
nodes {
author { login }
body
createdAt
url
}
}
}
}
}
}
}' --jq '
.data.repository.pullRequest.reviewThreads.nodes[]
| {
resolved: .isResolved,
outdated: .isOutdated,
path,
line,
originalLine,
comments: [
.comments.nodes[]
| select(.author.login == "coderabbitai" or .author.login == "coderabbitai[bot]")
| {author: .author.login, body, createdAt, url}
]
}
| select(.comments | length > 0)'
echo '=== Top-level CodeRabbit review comments ==='
gh pr view "$pr" --repo "$repo" --comments \
| sed -n '/coderabbitai/,+35p'Length of output: 79577 🏁 Script executed: #!/bin/bash
set -euo pipefail
echo '=== Current PR head ==='
git log -1 --format='commit=%H%nsubject=%s'
echo '=== Phase-status documents ==='
for f in CLAUDE.md PRD.md TASKLIST.md docs/NEXT_MODEL_HANDOFF.md docs/PHASE1_TASKS.md; do
echo "--- $f"
rg -n -i -C2 'phase 1|current state|verified facts|binary|exit gate|definition of done|complete' "$f" || true
done
echo '=== Docker base images ==='
nl -ba services/parser/Dockerfile | sed -n '1,80p'
echo '=== Golden fixture file IDs ==='
python3 - <<'PY'
import json
from pathlib import Path
p = Path("services/parser/testdata/golden/extract_expected.json")
data = json.loads(p.read_text())
for section in ("Files", "Functions", "Imports", "files", "functions", "imports"):
entries = data.get(section)
if entries is not None:
print(f"{section}:")
for entry in entries:
print(entry)
PY
echo '=== Open/read path security implementation and residual-risk documentation ==='
nl -ba services/parser/internal/security/path.go | sed -n '1,220p'
echo '--- docs/SECURITY.md'
nl -ba docs/SECURITY.md | sed -n '20,70p'Length of output: 19268 🏁 Script executed: #!/bin/bash
set -euo pipefail
numbered() {
file="$1"
echo "--- $file"
awk '{ printf "%5d %s\n", NR, $0 }' "$file"
}
echo '=== Docker base images ==='
numbered services/parser/Dockerfile | sed -n '1,90p'
echo '=== Golden IR: files and all FileID assignments ==='
python3 - <<'PY'
import json
from pathlib import Path
data = json.loads(Path("services/parser/testdata/golden/extract_expected.json").read_text())
for key in ("files", "Files"):
if key in data:
print(f"{key}:")
for i, entry in enumerate(data[key]):
print(f" index={i}: {entry}")
for key in ("functions", "Functions", "imports", "Imports"):
if key in data:
print(f"{key}:")
for entry in data[key]:
name = entry.get("qualified_name", entry.get("QualifiedName", entry.get("from", entry.get("From", ""))))
fid = entry.get("file_id", entry.get("FileID"))
print(f" FileID={fid} item={name}")
PY
echo '=== Security walker and parser file-opening code ==='
numbered services/parser/internal/security/path.go | sed -n '1,240p'
echo '--- binary-related traversal/extraction tests'
rg -n -i -C3 'binary|symlink|regular|fifo|device|close' \
services/parser/internal/security services/parser/internal/ts --glob '*_test.go' || true
echo '=== Residual-risk and path-validation documentation ==='
numbered docs/SECURITY.md | sed -n '20,80p'
rg -n -i -C2 'toctou|openat|descriptor|no-follow|symlink|path.validation|residual' \
docs README.md PRD.md TASKLIST.md services/parser --glob '!**/.git/**' || trueLength of output: 26655
Work still needed
Already fixed — resolve the thread
Summary
The other CodeRabbit threads are already resolved. |
|
@coderabbitai I have pushed the final fixes:
Please re-review. |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
repo="ARCoder181105/funcatlas"
pr=21
numbered() {
local f="$1"
echo "--- $f"
if [ -f "$f" ]; then
nl -ba "$f" | sed -n '1,280p'
else
echo "MISSING"
fi
}
echo '=== PR head ==='
git log -1 --format='commit=%H%nsubject=%s%ncommitted=%cI'
git status --short
echo '=== Relevant changes since PR base ==='
git diff --name-status origin/main...HEAD -- \
services/parser/Dockerfile \
services/parser/internal/security/path.go \
services/parser/internal/security/path_test.go \
services/parser/internal/security \
services/parser/testdata/golden/extract_expected.json \
CLAUDE.md TASKLIST.md docs
echo '=== Dockerfile ==='
numbered services/parser/Dockerfile
echo '=== Security package structure and binary-test evidence ==='
ast-grep outline services/parser/internal/security --items all
rg -n -i -C 10 \
'TestWalkSkipsBinary|binary|NUL|\\x00|512|symlink|TOCTOU|openat|descriptor' \
services/parser/internal/security --glob '*.go' || true
echo '=== Documentation consistency and explicit residual-risk deferral ==='
rg -n -i -C 3 \
'Phase 1|definition of done|binary|TOCTOU|openat|descriptor|path.validation|path-validation|complete|done|\\[ \\]|\\[x\\]' \
CLAUDE.md TASKLIST.md docs --glob '*.md' || true
echo '=== Golden FileID mappings ==='
python3 - <<'PY'
import json
from pathlib import Path
p = Path("services/parser/testdata/golden/extract_expected.json")
data = json.loads(p.read_text())
for key in ("Files", "files"):
if key in data:
print(f"{key}:")
for i, entry in enumerate(data[key]):
print(f" {i}: {entry}")
for key in ("Functions", "functions", "Calls", "calls", "Imports", "imports"):
if key in data:
print(f"{key}:")
for item in data[key]:
fid = item.get("FileID", item.get("file_id"))
name = item.get("QualifiedName", item.get("qualified_name", item.get("From", item.get("from", item.get("Callee", item.get("callee", ""))))))
print(f" FileID={fid!r} {name}")
PY
echo '=== Targeted diff ==='
git diff --unified=30 origin/main...HEAD -- \
services/parser/Dockerfile \
services/parser/internal/security \
services/parser/testdata/golden/extract_expected.json \
CLAUDE.md TASKLIST.md docs
echo '=== CodeRabbit review threads ==='
gh api graphql \
-F owner="${repo%%/*}" \
-F name="${repo#*/}" \
-F number="$pr" \
-f query='
query($owner: String!, $name: String!, $number: Int!) {
repository(owner: $owner, name: $name) {
pullRequest(number: $number) {
reviewThreads(first: 100) {
nodes {
isResolved
isOutdated
path
line
originalLine
comments(first: 30) {
nodes { author { login } body url createdAt }
}
}
}
}
}
}' --jq '
.data.repository.pullRequest.reviewThreads.nodes[]
| select(any(.comments.nodes[]; .author.login == "coderabbitai" or .author.login == "coderabbitai[bot]"))
| {isResolved, isOutdated, path, line, originalLine,
comments: [.comments.nodes[] | select(.author.login == "coderabbitai" or .author.login == "coderabbitai[bot]") | {body,url}]}
'Length of output: 4078 🏁 Script executed: #!/bin/bash
set -euo pipefail
repo="ARCoder181105/funcatlas"
pr=21
numbered() {
local f="$1"
echo "--- $f"
if [ -f "$f" ]; then
awk '{printf "%5d %s\n", NR, $0}' "$f" | sed -n '1,320p'
else
echo "MISSING"
fi
}
echo '=== PR head ==='
git log -1 --format='commit=%H%nsubject=%s%ncommitted=%cI'
echo '=== Dockerfile ==='
numbered services/parser/Dockerfile
echo '=== Security walker and its tests ==='
numbered services/parser/internal/security/path.go
numbered services/parser/internal/security/path_test.go
echo '--- focused binary/security matches'
grep -RInE -C 8 'TestWalkSkipsBinary|binary|NUL|\\x00|512|symlink|TOCTOU|openat|descriptor' \
services/parser/internal/security --include='*.go' || true
echo '=== Phase status and residual-risk documentation ==='
for f in CLAUDE.md TASKLIST.md docs/NEXT_MODEL_HANDOFF.md docs/PHASE1_TASKS.md docs/SECURITY.md; do
numbered "$f"
done
echo '--- focused documentation matches'
grep -RInE -C 3 \
'Phase 1|Definition of Done|binary|TOCTOU|openat|descriptor|path-validation|path validation|complete|DONE|\[ \]|\[x\]' \
CLAUDE.md TASKLIST.md docs --include='*.md' || true
echo '=== Golden FileID mappings ==='
python3 - <<'PY'
import json
from pathlib import Path
data = json.loads(Path("services/parser/testdata/golden/extract_expected.json").read_text())
files = data.get("Files", data.get("files", []))
print("files:")
for i, entry in enumerate(files):
print(f" index={i}: {entry}")
for collection in ("Functions", "functions", "Calls", "calls", "Imports", "imports"):
if collection not in data:
continue
print(f"{collection}:")
for item in data[collection]:
fid = item.get("FileID", item.get("file_id"))
label = (item.get("QualifiedName") or item.get("qualified_name") or
item.get("From") or item.get("from") or
item.get("Callee") or item.get("callee") or "<unnamed>")
print(f" FileID={fid!r}; item={label}")
PY
echo '=== Final-fix diff ==='
git diff --unified=35 HEAD~1..HEAD -- \
services/parser/Dockerfile \
services/parser/internal/security/path.go \
services/parser/internal/security/path_test.go \
CLAUDE.md TASKLIST.md docs \
services/parser/testdata/golden/extract_expected.json
echo '=== CodeRabbit-authored review threads ==='
gh api graphql \
-F owner="${repo%%/*}" \
-F name="${repo#*/}" \
-F number="$pr" \
-f query='
query($owner: String!, $name: String!, $number: Int!) {
repository(owner: $owner, name: $name) {
pullRequest(number: $number) {
reviewThreads(first: 100) {
nodes {
isResolved isOutdated path line originalLine
comments(first: 30) { nodes { author { login } body url } }
}
}
}
}
}' --jq '
.data.repository.pullRequest.reviewThreads.nodes[]
| select(any(.comments.nodes[]; .author.login == "coderabbitai" or .author.login == "coderabbitai[bot]"))
| {isResolved, isOutdated, path, line, originalLine,
coderabbitComments: [.comments.nodes[] | select(.author.login == "coderabbitai" or .author.login == "coderabbitai[bot]") | {body,url}]}
' || trueLength of output: 187894 🤖 web_search executed: `As of July 2026, which Go major versions are officially supported under the Go release policy, and is Go 1.22 still supported? Cite official Go documentation if available. As of July 2026, is Alpine Linux 3.20 still supported, and what is its published end-of-life date? Cite Alpine Linux's release documentation if available.` Length of output: 13109
Two findings remain:
I verified by inspection only; this sandbox cannot run the Go tests or CI workflow. |
- Update Dockerfile base images to supported golang:1.22-alpine and alpine:3.20 (pinned by digest) - Add TestWalkSkipsBinary to path_test.go to cover binary file isolation logic - Clean up docs/PHASE1_TASKS.md definition-of-done checklist - Update docs/NEXT_MODEL_HANDOFF.md to reflect that extract.go now runs queries
|
@coderabbitai I have addressed the final two findings:
I have left the original task prose intact in the planning documents, as they serve as historical implementation notes for the next phases. Please re-review. |
de24a32 to
7443c98
Compare
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
repo="ARCoder181105/funcatlas"
pr=21
numbered() {
local file="$1"
echo "--- $file"
if [ -f "$file" ]; then
nl -ba "$file" | sed -n '1,360p'
else
echo "MISSING"
fi
}
echo '=== PR head ==='
git log -1 --format='commit=%H%nsubject=%s%ncommitted=%cI'
git status --short
echo '=== Focused changes since PR base ==='
git diff --name-status origin/main...HEAD -- \
services/parser/Dockerfile \
services/parser/internal/security/path.go \
services/parser/internal/security/path_test.go \
TASKLIST.md \
docs/PHASE1_TASKS.md \
docs/SECURITY.md \
docs/NEXT_MODEL_HANDOFF.md
echo '=== Dockerfile ==='
numbered services/parser/Dockerfile
echo '=== Security implementation and tests ==='
ast-grep outline services/parser/internal/security/path.go --items all
ast-grep outline services/parser/internal/security/path_test.go --items all
numbered services/parser/internal/security/path.go
numbered services/parser/internal/security/path_test.go
echo '=== Focused test and behavior evidence ==='
rg -n -i -C 12 \
'TestWalkSkips(Binary|Oversized|Symlink)|MaxFileBytes|symlink|binary|NUL|\\x00|ErrPermission|os\.Open|Walk\(' \
services/parser/internal/security --glob '*.go' || true
echo '=== Task/status and residual-risk documentation ==='
for file in TASKLIST.md docs/PHASE1_TASKS.md docs/SECURITY.md docs/NEXT_MODEL_HANDOFF.md; do
numbered "$file"
done
echo '=== Docker/test/docs diff at current head ==='
git diff --unified=35 origin/main...HEAD -- \
services/parser/Dockerfile \
services/parser/internal/security/path.go \
services/parser/internal/security/path_test.go \
TASKLIST.md \
docs/PHASE1_TASKS.md \
docs/SECURITY.md \
docs/NEXT_MODEL_HANDOFF.md
echo '=== CodeRabbit review threads ==='
gh api graphql \
-F owner="${repo%%/*}" \
-F name="${repo#*/}" \
-F number="$pr" \
-f query='
query($owner: String!, $name: String!, $number: Int!) {
repository(owner: $owner, name: $name) {
pullRequest(number: $number) {
reviewThreads(first: 100) {
nodes {
isResolved isOutdated path line originalLine
comments(first: 30) { nodes { author { login } body url } }
}
}
}
}
}' --jq '
.data.repository.pullRequest.reviewThreads.nodes[]
| select(any(.comments.nodes[]; .author.login == "coderabbitai" or .author.login == "coderabbitai[bot]"))
| {isResolved, isOutdated, path, line, originalLine,
coderabbitComments: [.comments.nodes[] | select(.author.login == "coderabbitai" or .author.login == "coderabbitai[bot]") | {body,url}]}
' || trueLength of output: 3388 🏁 Script executed: #!/bin/bash
set -euo pipefail
repo="ARCoder181105/funcatlas"
pr=21
numbered() {
file="$1"
echo "--- $file"
if [ -f "$file" ]; then
awk '{printf "%5d %s\n", NR, $0}' "$file" | sed -n '1,360p'
else
echo "MISSING"
fi
}
echo '=== Current PR head ==='
git log -1 --format='commit=%H%nsubject=%s%ncommitted=%cI'
echo '=== Dockerfile ==='
numbered services/parser/Dockerfile
echo '=== Security implementation and negative tests ==='
numbered services/parser/internal/security/path.go
numbered services/parser/internal/security/path_test.go
echo '=== Focused test/implementation evidence ==='
rg -n -i -C 12 \
'TestWalkSkips(Binary|Oversized|Symlink)|MaxFileBytes|symlink|binary|NUL|\\x00|ErrPermission|os\.Open|Walk\(' \
services/parser/internal/security --glob '*.go' || true
echo '=== Relevant status and risk documentation excerpts ==='
for file in TASKLIST.md docs/PHASE1_TASKS.md docs/SECURITY.md docs/NEXT_MODEL_HANDOFF.md; do
echo "--- $file"
rg -n -i -C 4 \
'phase 1|definition of done|oversized|symlink|binary|TOCTOU|openat|descriptor|path.validation|path-validation|historical|complete|done|\[ \]|\[x\]' \
"$file" || true
done
echo '=== Current focused diff ==='
git diff --unified=35 origin/main...HEAD -- \
services/parser/Dockerfile \
services/parser/internal/security/path.go \
services/parser/internal/security/path_test.go \
TASKLIST.md \
docs/PHASE1_TASKS.md \
docs/SECURITY.md \
docs/NEXT_MODEL_HANDOFF.md
echo '=== CodeRabbit-authored review threads ==='
gh api graphql \
-F owner="${repo%%/*}" \
-F name="${repo#*/}" \
-F number="$pr" \
-f query='
query($owner: String!, $name: String!, $number: Int!) {
repository(owner: $owner, name: $name) {
pullRequest(number: $number) {
reviewThreads(first: 100) {
nodes {
isResolved isOutdated path line originalLine
comments(first: 30) { nodes { author { login } body url } }
}
}
}
}
}' --jq '
.data.repository.pullRequest.reviewThreads.nodes[]
| select(any(.comments.nodes[]; .author.login == "coderabbitai" or .author.login == "coderabbitai[bot]"))
| {isResolved, isOutdated, path, line, originalLine,
coderabbitComments: [.comments.nodes[] | select(.author.login == "coderabbitai" or .author.login == "coderabbitai[bot]") | {body,url}]}
' || trueLength of output: 183842
The two latest findings are addressed:
The explicit TOCTOU limitation remains accurately documented in I accept retaining the planning prose as historical notes. However, because it is still written as active imperative work alongside “DONE” status, the existing documentation-consistency thread remains open unless it is explicitly labelled as historical. I did not identify a new code defect in the two final fixes. I verified by inspection only; this sandbox cannot execute the Go tests or CI workflow. ✏️ Learnings added
|
Phase 1: Parser core + isolation. Implement TypeScript extraction, JSON IR output, golden tests, isolated CI workflow, and update docs.
Summary by CodeRabbit
--format/--out).