Add hunk session review --json for full live-session exports#160
Add hunk session review --json for full live-session exports#160benvinegar merged 5 commits intomainfrom
Conversation
Greptile SummaryThis PR adds a Key changes:
Findings:
Confidence Score: 5/5Safe to merge — all remaining findings are P2 style/architecture suggestions with no runtime impact. The new command is correctly plumbed end-to-end with no logic errors. Both open findings (duplicated hunk-header helper, parallel files/reviewFiles arrays) are maintainability concerns rather than defects, and they do not affect correctness or reliability of the feature.
Important Files Changed
Sequence DiagramsequenceDiagram
participant Agent
participant CLI as hunk session review
participant Client as HttpHunkDaemonCliClient
participant Server as MCP Daemon Server
participant State as HunkDaemonState
participant Reg as HunkSessionRegistration
Agent->>CLI: hunk session review --repo . --json
CLI->>Client: getSessionReview(selector)
Client->>Server: POST /session-api { action: "review", selector }
Server->>State: getSessionReview(selector)
State->>State: getSessionEntry(selector)
State->>Reg: registration.reviewFiles
State->>State: findSelectedReviewFile(reviewFiles, snapshot)
State-->>Server: SessionReview { files, selectedFile, selectedHunk, patch, hunks... }
Server-->>Client: { review: SessionReview }
Client-->>CLI: SessionReview
CLI-->>Agent: JSON output
Reviews (1): Last reviewed commit: "Add session review export command" | Re-trigger Greptile |
| function formatHunkHeader(file: AppBootstrap["changeset"]["files"][number], hunkIndex: number) { | ||
| const hunk = file.metadata.hunks[hunkIndex]!; | ||
| const specs = | ||
| hunk.hunkSpecs ?? | ||
| `@@ -${hunk.deletionStart},${hunk.deletionLines} +${hunk.additionStart},${hunk.additionLines} @@`; | ||
| return hunk.hunkContext ? `${specs} ${hunk.hunkContext}` : specs; | ||
| } |
There was a problem hiding this comment.
Duplicated hunk-header logic from
pierre.ts
formatHunkHeader is an exact copy of the private hunkHeader function in src/ui/diff/pierre.ts (lines 317–322). Both functions use the same hunkSpecs ?? "@@ ... @@" fallback and hunkContext suffix pattern. Per the project's architecture rule — "make that structure explicit in helper modules and reuse it across rendering and interaction code instead of re-deriving it ad hoc in multiple places" — this should be extracted to a shared helper (e.g. src/core/hunkHeader.ts) and imported from both call sites instead of being duplicated.
// src/core/hunkHeader.ts (example extraction)
import type { Hunk } from "@pierre/diffs";
/** Format a unified-diff hunk header string (specs + optional context). */
export function formatHunkHeader(hunk: Hunk): string {
const specs =
hunk.hunkSpecs ??
`@@ -${hunk.deletionStart},${hunk.deletionLines} +${hunk.additionStart},${hunk.additionLines} @@`;
return hunk.hunkContext ? `${specs} ${hunk.hunkContext}` : specs;
}Then sessionRegistration.ts and pierre.ts both import from there.
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
There was a problem hiding this comment.
Good catch. I extracted the shared hunk-header formatter into src/core/hunkHeader.ts and switched both src/ui/diff/pierre.ts and src/mcp/sessionRegistration.ts to reuse it.
This comment was generated by Pi using OpenAI o3
| terminal?: SessionTerminalMetadata; | ||
| files: SessionFileSummary[]; | ||
| reviewFiles: SessionReviewFile[]; | ||
| } |
There was a problem hiding this comment.
Redundant parallel arrays in
HunkSessionRegistration
HunkSessionRegistration now carries both files: SessionFileSummary[] and reviewFiles: SessionReviewFile[], where SessionReviewFile extends SessionFileSummary. Every entry in files is also present (with additional fields) in reviewFiles, so the two arrays are always kept in sync by both createSessionRegistration and updateSessionRegistration. The files array is used downstream only by listSessions() → ListedSession.files.
Consider deriving files from reviewFiles at the listSessions() call site (e.g. registration.reviewFiles.map(({ patch: _, hunks: __, ...summary }) => summary)) and dropping the redundant files field, or at minimum documenting why the two arrays must stay in sync so the invariant is visible to future maintainers.
There was a problem hiding this comment.
Agreed. I removed the redundant files field from HunkSessionRegistration and now derive the lightweight file summaries from reviewFiles inside listSessions(), so there is a single source of truth for the review export.
This comment was generated by Pi using OpenAI o3
e9f8084 to
f92a75f
Compare
f92a75f to
85b8d4b
Compare
Summary
hunk session reviewcommand and daemon API action for exporting the full loaded live-session review modelskills/hunk-review/SKILL.mdso agents know to use the new commandTesting
bun run typecheckbun testPart of #158.
This PR description was generated by Pi using OpenAI o3