feat: add deterministic local DiffGraph v2 extraction - #24
Conversation
Resolve staged and unstaged snapshots with exact Git identities, then extract Python structure and import evidence locally. Add a schema-validated opt-in JSON CLI path while preserving the legacy AI/HTML default and explicitly warning on unsupported or unparseable files.
|
Warning Review limit reached
Next review available in: 44 seconds You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (12)
WalkthroughChangesLocal structural JSON pipeline
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related issues
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 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: 13
🧹 Nitpick comments (14)
diffgraph/structural.py (8)
76-76: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueReplace the assigned lambda with a
def.Ruff reports E731 at line 76. A named function is also clearer about what identity means here.
♻️ Proposed change
- identity = lambda value: (value.st_dev, value.st_ino, value.st_size, value.st_mtime_ns) + def identity(value): + return (value.st_dev, value.st_ino, value.st_size, value.st_mtime_ns) +🤖 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 `@diffgraph/structural.py` at line 76, Replace the identity lambda assignment with a named def function in the surrounding structural comparison logic, preserving the existing tuple of st_dev, st_ino, st_size, and st_mtime_ns values it returns.Source: Linters/SAST tools
258-260: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winParenthesize the mixed
and/orcondition.Python binds
andtighter thanor, so the current behavior matches the intent. Ruff reports RUF021 twice on this line because the precedence is not explicit to a reader. This guard decides whether a file is skipped, so clarity matters.♻️ Proposed change
- if old is None and entry.old_oid is not None or new is None and entry.new_oid is not None: + if (old is None and entry.old_oid is not None) or ( + new is None and entry.new_oid is not None + ): skipped += 1 continue🤖 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 `@diffgraph/structural.py` around lines 258 - 260, Parenthesize the mixed boolean expression in the skip guard so the two `and` clauses are explicitly grouped under the `or`, preserving the current behavior while satisfying Ruff RUF021. Update only the condition controlling `skipped` and `continue`.Source: Linters/SAST tools
172-176: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueSplit the single-line conditional bodies.
Ruff reports E701 at lines 173, 174, 175, and 202. The repository runs Ruff, so these fail lint.
♻️ Proposed change
def _change_kind(status: str, old_oid: Optional[str], new_oid: Optional[str]) -> str: - if status == "A": return "added" - if status == "D": return "deleted" - if status == "R": return "renamed" if old_oid == new_oid else "renamed_modified" + if status == "A": + return "added" + if status == "D": + return "deleted" + if status == "R": + return "renamed" if old_oid == new_oid else "renamed_modified" return "modified"def _warning(code: str, path: Optional[str], detail: str) -> Dict[str, str]: result = {"code": code, "detail": detail} - if path is not None: result["file"] = path + if path is not None: + result["file"] = path return resultAlso applies to: 200-203
🤖 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 `@diffgraph/structural.py` around lines 172 - 176, Expand each single-line conditional body in _change_kind and the corresponding conditionals around lines 200–203 into standard multiline blocks, preserving their existing return values and control flow so Ruff no longer reports E701.Source: Linters/SAST tools
190-197: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winCache the parser version lookup.
_parser_provenancecallsimportlib.metadata.versionon every call._evidencecalls it once per symbol at line 217, and the import loop calls it again at line 345 for every import. Each call reads package metadata from disk. The value cannot change during one run.♻️ Proposed change
+@lru_cache(maxsize=1) +def _parser_version() -> str: + try: + return version("tree-sitter-language-pack") + except PackageNotFoundError: + return "unknown" + + def _parser_provenance(oid: Optional[str]) -> str: - try: - parser_version = version("tree-sitter-language-pack") - except PackageNotFoundError: - parser_version = "unknown" return "analyzer={};parser=tree-sitter-language-pack@{};query={};blob={}".format( - ANALYZER, parser_version, QUERY_VERSION, oid or "absent" + ANALYZER, _parser_version(), QUERY_VERSION, oid or "absent" )🤖 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 `@diffgraph/structural.py` around lines 190 - 197, Cache the tree-sitter-language-pack version lookup used by _parser_provenance so importlib.metadata.version is evaluated at most once per process. Reuse the cached value for all provenance calls, preserving "unknown" when PackageNotFoundError occurs and the existing provenance format.
91-97: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winCache the parser instead of rebuilding it for every blob.
_parse_pythonline 115 calls_parser()for every parsed blob.analyze_local_diffparses two blobs per changed Python file, so a diff with N Python files performs 2N language lookups and 2NParserconstructions.♻️ Proposed change
+from functools import lru_cache + + +@lru_cache(maxsize=1) def _parser(): import tree_sitter import tree_sitter_language_pack # Construct the official parser directly so byte offsets and byte input # remain exact across language-pack releases. return tree_sitter.Parser(tree_sitter_language_pack.get_language("python"))A
tree_sitter.Parseris not thread-safe. Ifanalyze_local_diffbecomes concurrent, use a thread-local parser instead of a shared cached instance.🤖 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 `@diffgraph/structural.py` around lines 91 - 97, Cache parser instances for reuse instead of constructing one in every _parser call, while keeping them isolated per thread because tree_sitter.Parser is not thread-safe. Update _parser and its use from _parse_python so repeated blob parsing reuses the current thread’s parser and avoids repeated language lookups and constructions.
50-58: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy lift
structural.pyreimplements the Git access layer thatgit_snapshot.pyalready owns. The two modules now hold two independent Git invocation layers with divergent failure handling, and the working-tree hashing logic exists twice with different mode handling. The duplication is the root cause of the symlink defect:diffgraph/git_snapshot.pyline 293 handles mode120000, and the copy instructural.pydoes not.
diffgraph/structural.py#L50-L58: reuse a shared Git runner instead of the private_runand_root, or move both intogit_snapshot.pyand expose a variant that raises instead of collecting warnings.diffgraph/structural.py#L67-L78: fold the working-tree read and verification intogit_snapshot._working_tree_oid, so the mode allowlist and the stat-based race checks exist in one place and return the content alongside the object ID.🤖 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 `@diffgraph/structural.py` around lines 50 - 58, The Git access and working-tree logic is duplicated in diffgraph/structural.py, causing divergent behavior and missing symlink handling. In diffgraph/structural.py lines 50-58, replace private _run and _root usage with the shared Git runner from git_snapshot.py, exposing an error-raising variant if needed; in diffgraph/structural.py lines 67-78, reuse git_snapshot._working_tree_oid for reading and verifying working-tree content, including its mode allowlist and stat-based race checks, and consume its content-plus-object-ID result.
317-324: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMove
keyed_importsto module scope.
keyed_importsis defined inside the per-entry loop, so Python rebuilds the function object for every changed file. The function closes over nothing from the loop.♻️ Proposed change
Define it next to the other module-level helpers and delete the nested definition:
def _keyed_imports(items: List[_Import]) -> Dict[Tuple[str, int], _Import]: occurrences: Dict[str, int] = {} result: Dict[Tuple[str, int], _Import] = {} for import_item in items: occurrence = occurrences.get(import_item.module, 0) occurrences[import_item.module] = occurrence + 1 result[(import_item.module, occurrence)] = import_item return result🤖 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 `@diffgraph/structural.py` around lines 317 - 324, Move the keyed_imports helper out of the per-entry loop and define it at module scope alongside the other helpers, renaming it to _keyed_imports as proposed. Remove the nested definition and update its call sites to use the module-level helper, preserving the existing occurrence-keying behavior.
236-243: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low valueReplace the invariant
assertstatements with explicit errors.Lines 237, 284, and 332 use
assertto guard values that later feed string concatenation, for example"file::" + pathat line 246.python -Oremovesassertstatements. The invariant then fails later as aTypeErrorinstead of at the check.Raise an explicit error, or restructure so the type checker proves the value is not
None.🤖 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 `@diffgraph/structural.py` around lines 236 - 243, Replace the invariant assert checks in the affected analysis paths, including the one near path assignment and the corresponding checks around lines 284 and 332, with explicit errors or control flow that establishes non-None values before later string operations. Preserve the existing behavior for valid paths while ensuring optimized Python execution cannot bypass validation and produce a later TypeError..github/workflows/test.yml (1)
18-21: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winTest the Python versions the package claims to support.
setup.pyline 23 setspython_requires=">=3.10"and lines 33-35 declare 3.10, 3.11, and 3.12. This job runs 3.10 only. Thenetworkx>=3.5; python_version >= "3.11"branch inrequirements.txtline 7 is therefore never installed or exercised.♻️ Proposed matrix
test: runs-on: ubuntu-latest + strategy: + fail-fast: false + matrix: + python-version: ["3.10", "3.11", "3.12"] steps: - uses: actions/checkout@v4 with: persist-credentials: false - uses: actions/setup-python@v5 with: - python-version: "3.10" + python-version: ${{ matrix.python-version }} cache: pip🤖 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/test.yml around lines 18 - 21, Update the test job’s setup-python configuration to use a version matrix covering Python 3.10, 3.11, and 3.12, and reference the matrix value in python-version so each supported version runs the existing tests and dependency resolution.tests/test_git_snapshot.py (2)
206-211: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueGuard the "not a repository" test against a repository-hosted temporary directory.
The test asserts
not_a_git_repositoryfortmp_path. IfTMPDIRresolves inside a Git working tree,git rev-parse --show-toplevelsucceeds and this assertion fails. SetGIT_CEILING_DIRECTORIESto the parent oftmp_path, or create a marker so discovery stops.🤖 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 `@tests/test_git_snapshot.py` around lines 206 - 211, Update test_git_failures_are_warnings_not_changes to isolate Git discovery by setting GIT_CEILING_DIRECTORIES to tmp_path’s parent (or equivalent marker) before calling resolve_staged, ensuring the temporary path is treated as outside any repository and the existing warning assertions remain valid.
39-45: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winBoth test modules build repositories that inherit global Git configuration. Each module defines its own
githelper and repository factory, and each sets onlyuser.nameanduser.email.git initstill reads global and system configuration.commit.gpgsign=truemakes the commit helpers fail undercheck=True, andcore.autocrlfchanges the stored bytes, which breaks the byte-exact object ID assertions. Test results then depend on the developer machine.
tests/test_git_snapshot.py#L39-L45: setGIT_CONFIG_GLOBAL,GIT_CONFIG_SYSTEM, andGIT_CONFIG_NOSYSTEMin the environment passed to thegithelper at lines 7-16, so the assertions at lines 123-126 and line 147 no longer depend on global config.tests/test_structural.py#L23-L29: apply the same isolation in thegithelper at lines 13-14, so the object ID assertion at lines 110-111 becomes hermetic.Extract one shared helper into a
conftest.pyfixture rather than duplicating the isolation logic in both modules.🤖 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 `@tests/test_git_snapshot.py` around lines 39 - 45, Both test modules inherit global Git configuration when initializing repositories, causing test failures when developers have settings like commit.gpgsign=true or core.autocrlf configured. Create a shared fixture in conftest.py that sets GIT_CONFIG_GLOBAL, GIT_CONFIG_SYSTEM, and GIT_CONFIG_NOSYSTEM environment variables to empty or /dev/null to isolate Git from system and global config. Then update the git helper functions in tests/test_git_snapshot.py at lines 7-16 and tests/test_structural.py at lines 13-14 to pass these environment variables when executing git commands, and update the make_repo factory in test_git_snapshot.py at lines 39-45 and the make_repo_factory function in test_structural.py at lines 23-29 to use the shared fixture. This ensures the repository initialization at both sites inherits only the user.name and user.email configuration, making the object ID assertions at test_git_snapshot.py lines 123-126 and 147, and test_structural.py lines 110-111 hermetic.tests/test_structural.py (2)
93-96: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winThe
UPDATE_GOLDENbranch makes the assertion unconditionally pass.Lines 93-95 rewrite the fixture and then set
expected = actual. The assertion at line 96 then compares a value with itself. IfUPDATE_GOLDENis ever set in an automated environment, this test reports success and overwrites the golden fixture.Skip the test in regeneration mode so a passing result never depends on the environment variable.
💚 Proposed change
if os.environ.get("UPDATE_GOLDEN"): golden_path.write_text(json.dumps(actual, indent=2) + "\n") - expected = actual - assert actual == expected + pytest.skip("golden fixture regenerated") + assert actual == expectedAdd
import pytestat the top of the file.🤖 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 `@tests/test_structural.py` around lines 93 - 96, Update the UPDATE_GOLDEN branch in the affected structural test to skip the test via pytest instead of assigning expected = actual and continuing to the assertion; add the pytest import required for this behavior, while preserving golden-file regeneration.
49-96: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd coverage for
renamed_modifiedandPARTIAL_ANALYSIS.
diffgraph/structural.pyline 175 returnsrenamed_modifiedwhen a rename also changes content. No test reaches that branch: the rename at line 59 is a puregit mv, so_change_kindreturnsrenamed. ThePARTIAL_ANALYSISwarning atdiffgraph/structural.pyline 243 has no coverage either, and no test assertsmetadata["files_analyzed"].A renamed-and-modified Python file is the case most likely to produce wrong symbol change kinds, because the old and new symbols come from different paths while
output_pathuses only the new path.🤖 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 `@tests/test_structural.py` around lines 49 - 96, Extend test_staged_add_modify_delete_rename_import_is_schema_valid_and_golden to modify renamed.py after git mv, then assert its file change_kind is renamed_modified and verify the affected symbol change kinds use the new output path correctly. Add coverage for the PARTIAL_ANALYSIS warning path in the relevant structural analysis test and assert metadata["files_analyzed"] contains the expected analyzed-file count; update the golden fixture through the existing UPDATE_GOLDEN mechanism.README.md (1)
91-101: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMove the new section into "💻 Usage" and list the option.
The
###heading at line 92 follows "🙏 Acknowledgments" at line 85, so it nests under Acknowledgments and appears after "📝 License". Readers looking for usage will not find it.The "Command-line Options" list at lines 58-61 also omits
--structural-json.Move lines 92-114 to just after line 66, and add the option to the list:
📝 Proposed addition to the options list
- `--no-open`: Don't automatically open the HTML report in browser +- `--structural-json`: Write a local Python structural DiffGraph v2 artifact to the given path (`-` for stdout). Applies to `wild diff` only. - `--version`: Show version information🤖 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 `@README.md` around lines 91 - 101, Relocate the "### Local structural JSON (experimental)" section from its current position after "🙏 Acknowledgments" to immediately after the main "💻 Usage" section content. Additionally, add the `--structural-json` option to the Command-line Options list that appears in the Usage section, including a brief description of what this option does based on the examples provided in the moved section.
🤖 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 `@diffgraph/cli.py`:
- Around line 175-179: Handle --structural-json when the selected command is not
diff instead of silently discarding it: either reject the combination with
Click’s UsageError in main before the pass-through path, or clearly state the
diff-only restriction in the option’s help text. Keep structural artifact
handling unchanged for diff commands.
- Around line 197-201: Update the structural diff flow around the
analyze_local_diff call in diffgraph/cli.py so user-supplied pathspecs are
resolved relative to the caller’s current directory before Git evaluates them
from the repository root, preserving expected paths such as src/app.py when
invoked inside src/. Use an explicit root-relative conversion or Git pathspec
semantics consistently, and document the chosen behavior in README.md.
- Around line 206-208: Update the structural_json output handling around the
structural_json.write_text call to validate that the destination’s parent
directory exists and is writable before writing, using the Click path
configuration or explicit validation. Ensure a missing or unwritable parent
produces a user-facing Click error instead of an uncaught FileNotFoundError
traceback.
- Around line 211-215: Move the lazy imports for spinner, CodeAnalysisAgent, and
report helpers into the existing protected command flow beginning around the try
block, or add a narrowly scoped try/except around them, and convert any
ImportError into the command’s Click error handling instead of allowing a
traceback. Keep these imports lazy so structural output does not load AI
dependencies.
- Around line 118-122: Add jsonschema.SchemaError to the except clause in the
schema validation try-block. The jsonschema.validate function raises SchemaError
when the schema itself is malformed, not just ValidationError when the artifact
fails validation. Update the exception tuple to include SchemaError alongside
the existing OSError, json.JSONDecodeError, and jsonschema.ValidationError
handlers to ensure schema parsing errors are caught and converted to Click
errors instead of tracebacks.
In `@diffgraph/git_snapshot.py`:
- Around line 293-294: Update the symlink handling in the snapshot attribute
path around the mode == "120000" branch so it does not hash the raw link target
with a path-dependent Git operation. Use stdin-only hashing for symlink targets,
or compare against the raw target hash, while preserving the existing encoded
readlink content.
In `@diffgraph/structural.py`:
- Around line 50-58: Update _root to catch failures from _run when resolving the
repository top level and raise a dedicated typed error with the underlying git
failure details. In the CLI path that invokes analyze_local_diff, including the
call site around _root, catch that typed error and convert it to
click.ClickException so bare, missing, or invalid repositories produce a
user-facing Click error instead of an unhandled traceback.
- Around line 122-151: The _Symbol creation in the visit function uses only
qname as the unique identifier, causing duplicate symbols with the same
qualified name to be silently dropped when building old_map and new_map
dictionaries. Add an occurrence index to _Symbol (similar to the occurrence
scheme used for imports in lines 317-324) so that each symbol instance gets a
unique identifier even when sharing the same qualified_name, parent, and kind.
Ensure the _Symbol constructor and the qname assignment both incorporate this
occurrence counter to preserve all symbol definitions.
- Around line 348-350: The import classification logic should report edits as
modified when the before and after entries exist but their snippets differ.
Update the import_kind assignment in the surrounding structural comparison to
keep added/deleted handling, use unchanged only when before.snippet equals
after.snippet, and otherwise select the supported modified kind.
- Around line 262-276: Remove ImportError from the per-file parser_errors
handling in the structural diff flow so missing parser dependencies do not
become PARSE_FAILURE warnings or increment skipped. Let the dependency error
from _parser() propagate, or convert it into a distinct run-level configuration
error and handle it as a click.ClickException in the CLI entry point.
- Around line 67-78: Update _worktree_bytes to detect symlink entries with
stat.S_ISLNK and avoid comparing the lstat-style link metadata in before/after
against the opened target metadata. Keep the existing identity-based change
detection for regular files, while allowing reachable symlinked files to be read
without raising the spurious OSError.
- Around line 155-158: Update the import processing loop in the relevant
structural parsing function so aliased_import items use
item.child_by_field_name("name") and extract that child node’s text, avoiding
raw-text splitting. Preserve _node_text(item) for non-aliased dotted_name items
and keep the existing _Import creation behavior.
- Around line 206-207: Update _resolution_warning to map each
ResolutionWarning.code to its corresponding warning schema enum value,
preserving distinct codes such as not_a_git_repository, git_diff_failed,
missing_object_id, worktree_read_failed, hash_object_failed,
malformed_hash_object_output, and malformed_git_output; return "UNKNOWN" only
when no mapping exists, while keeping the existing path and detail formatting.
---
Nitpick comments:
In @.github/workflows/test.yml:
- Around line 18-21: Update the test job’s setup-python configuration to use a
version matrix covering Python 3.10, 3.11, and 3.12, and reference the matrix
value in python-version so each supported version runs the existing tests and
dependency resolution.
In `@diffgraph/structural.py`:
- Line 76: Replace the identity lambda assignment with a named def function in
the surrounding structural comparison logic, preserving the existing tuple of
st_dev, st_ino, st_size, and st_mtime_ns values it returns.
- Around line 258-260: Parenthesize the mixed boolean expression in the skip
guard so the two `and` clauses are explicitly grouped under the `or`, preserving
the current behavior while satisfying Ruff RUF021. Update only the condition
controlling `skipped` and `continue`.
- Around line 172-176: Expand each single-line conditional body in _change_kind
and the corresponding conditionals around lines 200–203 into standard multiline
blocks, preserving their existing return values and control flow so Ruff no
longer reports E701.
- Around line 190-197: Cache the tree-sitter-language-pack version lookup used
by _parser_provenance so importlib.metadata.version is evaluated at most once
per process. Reuse the cached value for all provenance calls, preserving
"unknown" when PackageNotFoundError occurs and the existing provenance format.
- Around line 91-97: Cache parser instances for reuse instead of constructing
one in every _parser call, while keeping them isolated per thread because
tree_sitter.Parser is not thread-safe. Update _parser and its use from
_parse_python so repeated blob parsing reuses the current thread’s parser and
avoids repeated language lookups and constructions.
- Around line 50-58: The Git access and working-tree logic is duplicated in
diffgraph/structural.py, causing divergent behavior and missing symlink
handling. In diffgraph/structural.py lines 50-58, replace private _run and _root
usage with the shared Git runner from git_snapshot.py, exposing an error-raising
variant if needed; in diffgraph/structural.py lines 67-78, reuse
git_snapshot._working_tree_oid for reading and verifying working-tree content,
including its mode allowlist and stat-based race checks, and consume its
content-plus-object-ID result.
- Around line 317-324: Move the keyed_imports helper out of the per-entry loop
and define it at module scope alongside the other helpers, renaming it to
_keyed_imports as proposed. Remove the nested definition and update its call
sites to use the module-level helper, preserving the existing occurrence-keying
behavior.
- Around line 236-243: Replace the invariant assert checks in the affected
analysis paths, including the one near path assignment and the corresponding
checks around lines 284 and 332, with explicit errors or control flow that
establishes non-None values before later string operations. Preserve the
existing behavior for valid paths while ensuring optimized Python execution
cannot bypass validation and produce a later TypeError.
In `@README.md`:
- Around line 91-101: Relocate the "### Local structural JSON (experimental)"
section from its current position after "🙏 Acknowledgments" to immediately
after the main "💻 Usage" section content. Additionally, add the
`--structural-json` option to the Command-line Options list that appears in the
Usage section, including a brief description of what this option does based on
the examples provided in the moved section.
In `@tests/test_git_snapshot.py`:
- Around line 206-211: Update test_git_failures_are_warnings_not_changes to
isolate Git discovery by setting GIT_CEILING_DIRECTORIES to tmp_path’s parent
(or equivalent marker) before calling resolve_staged, ensuring the temporary
path is treated as outside any repository and the existing warning assertions
remain valid.
- Around line 39-45: Both test modules inherit global Git configuration when
initializing repositories, causing test failures when developers have settings
like commit.gpgsign=true or core.autocrlf configured. Create a shared fixture in
conftest.py that sets GIT_CONFIG_GLOBAL, GIT_CONFIG_SYSTEM, and
GIT_CONFIG_NOSYSTEM environment variables to empty or /dev/null to isolate Git
from system and global config. Then update the git helper functions in
tests/test_git_snapshot.py at lines 7-16 and tests/test_structural.py at lines
13-14 to pass these environment variables when executing git commands, and
update the make_repo factory in test_git_snapshot.py at lines 39-45 and the
make_repo_factory function in test_structural.py at lines 23-29 to use the
shared fixture. This ensures the repository initialization at both sites
inherits only the user.name and user.email configuration, making the object ID
assertions at test_git_snapshot.py lines 123-126 and 147, and test_structural.py
lines 110-111 hermetic.
In `@tests/test_structural.py`:
- Around line 93-96: Update the UPDATE_GOLDEN branch in the affected structural
test to skip the test via pytest instead of assigning expected = actual and
continuing to the assertion; add the pytest import required for this behavior,
while preserving golden-file regeneration.
- Around line 49-96: Extend
test_staged_add_modify_delete_rename_import_is_schema_valid_and_golden to modify
renamed.py after git mv, then assert its file change_kind is renamed_modified
and verify the affected symbol change kinds use the new output path correctly.
Add coverage for the PARTIAL_ANALYSIS warning path in the relevant structural
analysis test and assert metadata["files_analyzed"] contains the expected
analyzed-file count; update the golden fixture through the existing
UPDATE_GOLDEN mechanism.
🪄 Autofix
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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 234fceb7-2ec9-4644-ba53-068c2f893a95
📒 Files selected for processing (11)
.github/workflows/test.ymlREADME.mddiffgraph/__init__.pydiffgraph/cli.pydiffgraph/git_snapshot.pydiffgraph/structural.pyrequirements.txtsetup.pytests/fixtures/python_topology.jsontests/test_git_snapshot.pytests/test_structural.py
Preserve exact Git/pathspec and symlink semantics while surfacing dependency and CLI failures cleanly. Retain duplicate symbols and warning fidelity, classify edited imports, and expand hermetic regression coverage and supported-version CI.
2 participants
Why
This is the clean
main-based replacement for the useful deterministic work in #13. The original PR is stacked onmerged-foundation-prsand includes unrelated legacy exporters, GraphManager/Mermaid canonicalization, an AI-default processor registry, a divergent schema copy, and stale release churn. Those pieces are intentionally not carried forward.This PR keeps the product-aligned core: exact Git snapshots, local Tree-sitter extraction, schema-v2 output, stable structural topology, evidence, warnings, and an additive CLI path.
What changed
HEAD → indexindex → working treewild --structural-json artifact.json diffwild --structural-json - diff--staged/--cachedand pathspecs after--Explicit boundaries
files[]withUNSUPPORTED_LANGUAGEwarnings.Verification
python3 -m pytest -q— 17 passedpython3 -m compileall -q diffgraph tests— passedpython3 setup.py check— passed (setuptools license-classifier deprecation warning only)git diff origin/main...HEAD --check— passedRoadmap relationship
This reconciles the useful part of #13 with the current direction in #21, #22, and #23. It is intentionally an incremental baseline; commit-range resolution, broader language support, and downstream canonical consumers remain follow-up work.
Summary by CodeRabbit
New Features
Documentation
Chores