Skip to content

fix: CLI build/update/watch now run post-processing (signatures, FTS, flows, communities)#98

Open
igagankalra wants to merge 1 commit intotirth8205:mainfrom
igagankalra:fix/cli-post-processing
Open

fix: CLI build/update/watch now run post-processing (signatures, FTS, flows, communities)#98
igagankalra wants to merge 1 commit intotirth8205:mainfrom
igagankalra:fix/cli-post-processing

Conversation

@igagankalra
Copy link
Copy Markdown

Extract the 4-step post-processing pipeline from tools/build.py into a shared postprocessing.py module and wire it into all CLI entry points so that build, update, and watch produce the same complete graph as the MCP tool.

  • Add code_review_graph/postprocessing.py with run_post_processing()
  • tools/build.py now delegates to run_post_processing() (no duplication)
  • cli.py calls _cli_post_process() after build and update commands
  • watch() accepts on_files_updated callback, invoked after each flush
  • 18 new tests covering all steps, isolation, idempotency, and watch

Closes #93

@igagankalra
Copy link
Copy Markdown
Author

I've put up a focused fix for this issue on my fork: igagankalra:fix/cli-post-processing (main...igagankalra:code-review-graph:fix/cli-post-processing)
Why a separate PR instead of relying on #95
PR #95 incidentally patches build and update as part of a much larger feature PR (10 new CLI subcommands + CommonJS require() parsing — 633 lines). The #93 fix is ~65 lines buried in that diff, and it has a few gaps:

  1. Code duplication — feat: CLI-first expansion — 10 subcommands + CommonJS require() parsing #95 copy-pastes the post-processing logic into cli.py as _run_post_processing(), leaving the original copy in tools/build.py untouched. Two copies that can drift.
  2. Watch mode still broken — feat: CLI-first expansion — 10 subcommands + CommonJS require() parsing #95 doesn't touch watch(), which is the third affected entry point listed in this issue.
  3. No tests for the post-processing path itself.
    What the focused fix does differently

Copy link
Copy Markdown
Owner

@tirth8205 tirth8205 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Clean approach to the post-processing pipeline — extracting into a shared module and wiring into build/update/watch is the right design. 18 new tests.

Please rebase on latest main. This should merge before #95 (which overlaps on the post-processing piece).

… flows, communities)

Extract the 4-step post-processing pipeline from tools/build.py into a
shared postprocessing.py module and wire it into all CLI entry points so
that `build`, `update`, and `watch` produce the same complete graph
as the MCP tool.

- Add code_review_graph/postprocessing.py with run_post_processing()
- tools/build.py now delegates to run_post_processing() (no duplication)
- cli.py calls _cli_post_process() after build and update commands
- watch() accepts on_files_updated callback, invoked after each flush
- 18 new tests covering all steps, isolation, idempotency, and watch

Closes tirth8205#93
@igagankalra igagankalra force-pushed the fix/cli-post-processing branch from 6880bb5 to d66309f Compare April 8, 2026 21:47
@igagankalra
Copy link
Copy Markdown
Author

@tirth8205 , Rebased the branch with main, should be good to be merged.

@tirth8205
Copy link
Copy Markdown
Owner

Review: PR #98 — fix: CLI build/update/watch now run post-processing

This is a clean, focused fix. Extracting the 4-step post-processing pipeline into postprocessing.py is the right architectural move, and wiring it into build, update, and watch via an on_files_updated callback is correct.

The author rebased on April 8 per the comments.

Code review findings:

  1. _cli_post_process wrapper in cli.py: The function is clean and the print-per-step approach matches the existing build output style.

  2. 18 new tests: Coverage of signatures, FTS, flows, communities, step isolation (each failure does not block the next step), idempotency, and watch callback are all appropriate.

  3. One concern: The diff shows run_post_processing(store) # type: ignore[arg-type] in the wrapper. This type ignore suggests a typing mismatch — if store is typed differently in cli.py vs postprocessing.py, this should be resolved rather than suppressed with a comment.

  4. No overlap with feat: parser refactoring, Jedi call resolution, and performance optimizations #158: This PR's scope (post-processing extraction) is distinct from feat: parser refactoring, Jedi call resolution, and performance optimizations #158's scope (parser refactoring, Jedi, performance). feat: parser refactoring, Jedi call resolution, and performance optimizations #158 will need to adapt its post-processing references after this merges, but there is no semantic conflict.

Merge order: Owner confirmed this should merge before #95. Both #94 (transaction fixes) and #98 (post-processing) are independently mergeable — however, #94's transaction fixes should ideally land first since they correct bugs in the code that post-processing calls (flows.py, communities.py, search.py).

Verdict: Ready to merge after #94 lands and CI passes. Please confirm CI is green on the rebased branch.

@tirth8205
Copy link
Copy Markdown
Owner

Update: Merge conflict detected. Despite the author's note that the branch was rebased on April 8, GitHub reports this PR has merge conflicts with main (mergeStateStatus: DIRTY). Please rebase again on the current main branch — there have likely been additional merges since April 8. Once conflicts are resolved and CI passes, this is ready to merge.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

CLI build missing post-processing steps that build_or_update_graph_tool MCP tool includes

2 participants