Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 12 additions & 8 deletions docs/architecture.md
Original file line number Diff line number Diff line change
Expand Up @@ -82,18 +82,22 @@ data. The intended decomposition is:

- `internal/threadcontext` normalizes `gitprovider.InlineThread`, detects
codereview-authored finding threads, detects latest human replies, strips
shared markers, collapses resolved threads to the latest sanitized comment,
and produces file-scoped reviewer context.
shared markers, collapses provider-resolved threads to the latest sanitized
comment, recognizes CR-authored settled summary replies when provider
resolution is unavailable, and produces file-scoped reviewer context.
- `internal/threadanalysis` accepts normalized thread context and returns
reusable domain decisions: thread ID, decision, reply body, summary, resolve
flag, and rationale.

Resolved inline threads should not be reprocessed as full conversations on
every review. Their durable context is the latest sanitized comment on the
resolved thread, with marker metadata retained when the comment contains a
codereview thread-summary marker. Reviewer prompts should receive compact
file-scoped summaries so agents avoid re-raising issues that have already been
discussed and resolved.
Settled inline threads should not be reprocessed as full conversations on every
review. Provider-resolved context is the latest sanitized comment on a thread
whose provider state is resolved. CR-settled context is the latest CR-authored
inline reply carrying a valid `codereview:thread-summary` marker when no newer
human reply follows it, even if the provider still reports the thread as
unresolved. Reviewer prompts should receive compact file-scoped summaries for
both sources so agents avoid re-raising issues that have already been discussed
and settled, while command output keeps provider resolution distinct from
CR-settled cache metadata.

`cr review` and `cr respond` should share the same normalization, filtering,
model resolution, LLM execution, and action-planning components. `cr respond`
Expand Down
52 changes: 40 additions & 12 deletions internal/cmd/respondcmd/respondcmd.go
Original file line number Diff line number Diff line change
Expand Up @@ -153,10 +153,13 @@ type renderedResult struct {
}

type counts struct {
Considered int `json:"considered"`
Responded int `json:"responded"`
Resolved int `json:"resolved"`
Planned int `json:"planned"`
Considered int `json:"considered"`
Responded int `json:"responded"`
Resolved int `json:"resolved"`
ResolvePlanned int `json:"resolve_planned"`
ResolveFailed int `json:"resolve_failed"`
ProviderResolved int `json:"provider_resolved"`
Planned int `json:"planned"`
}

func newResult(result threadrespond.Result) renderedResult {
Expand All @@ -169,11 +172,13 @@ func newResult(result threadrespond.Result) renderedResult {
outboxExitCode = result.ExitCode
}
responded := countThreadReplies(result.Plan.Actions)
resolved := countThreadResolves(result.Plan.Actions)
if responded == 0 && resolved == 0 && len(result.PlannedActions) > 0 {
resolvePlanned := countThreadResolves(result.Plan.Actions)
if responded == 0 && resolvePlanned == 0 && len(result.PlannedActions) > 0 {
responded = countPlannedThreadReplies(result.PlannedActions)
resolved = countPlannedThreadResolves(result.PlannedActions)
resolvePlanned = countPlannedThreadResolves(result.PlannedActions)
}
providerResolved := countPostedThreadResolves(result.PlannedActions)
resolveFailed := countFailedThreadResolves(result.PlannedActions)
return renderedResult{
Run: view.ReviewRun{
RunID: result.Run.RunID,
Expand All @@ -186,10 +191,13 @@ func newResult(result threadrespond.Result) renderedResult {
HeadSHA: result.Run.SHA,
},
Counts: counts{
Considered: len(result.EligibleThreads),
Responded: responded,
Resolved: resolved,
Planned: len(result.PlannedActions),
Considered: len(result.EligibleThreads),
Responded: responded,
Resolved: providerResolved,
ResolvePlanned: resolvePlanned,
ResolveFailed: resolveFailed,
ProviderResolved: providerResolved,
Planned: len(result.PlannedActions),
},
Outbox: view.ReviewOutbox{
Outcome: outcome,
Expand Down Expand Up @@ -221,7 +229,7 @@ func renderText(w io.Writer, rendered renderedResult) error {
if _, err := fmt.Fprintf(w, "Outcome: %s\n", rendered.Run.Outcome); err != nil {
return err
}
if _, err := fmt.Fprintf(w, "Threads: considered %d, responded %d, resolved %d\n", rendered.Counts.Considered, rendered.Counts.Responded, rendered.Counts.Resolved); err != nil {
if _, err := fmt.Fprintf(w, "Threads: considered %d, responded %d, provider resolved %d (resolve planned %d, failed %d)\n", rendered.Counts.Considered, rendered.Counts.Responded, rendered.Counts.ProviderResolved, rendered.Counts.ResolvePlanned, rendered.Counts.ResolveFailed); err != nil {
return err
}
if _, err := fmt.Fprintf(w, "Planned actions: %d\n", rendered.Counts.Planned); err != nil {
Expand Down Expand Up @@ -283,6 +291,26 @@ func countPlannedThreadResolves(actions []ledger.PlannedAction) int {
return count
}

func countPostedThreadResolves(actions []ledger.PlannedAction) int {
var count int
for _, action := range actions {
if action.Kind == ledger.PlannedActionResolveThread && action.Status == ledger.PlannedActionPosted {
count++
}
}
return count
}

func countFailedThreadResolves(actions []ledger.PlannedAction) int {
var count int
for _, action := range actions {
if action.Kind == ledger.PlannedActionResolveThread && action.Status == ledger.PlannedActionFailedTerminal {
count++
}
}
return count
}

func resultError(result threadrespond.Result) error {
if strings.TrimSpace(result.Message) != "" {
return errors.New(result.Message)
Expand Down
69 changes: 66 additions & 3 deletions internal/cmd/respondcmd/respondcmd_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -55,7 +55,7 @@ func TestRespondDryRunCallsResponderAndRendersText(t *testing.T) {
t.Fatal("runtime cleanup was not called")
}
text := out.String()
if !strings.Contains(text, "Threads: considered 1, responded 1, resolved 1") || !strings.Contains(text, "Planned actions: 2") {
if !strings.Contains(text, "Threads: considered 1, responded 1, provider resolved 0 (resolve planned 1, failed 0)") || !strings.Contains(text, "Planned actions: 2") {
t.Fatalf("stdout = %q, want respond summary", text)
}
}
Expand All @@ -82,7 +82,7 @@ func TestRespondRetryPostsFlagCallsResponder(t *testing.T) {
if responder.requests[0].DryRun {
t.Fatalf("respond retry request = %#v, want live retry", responder.requests[0])
}
if text := out.String(); !strings.Contains(text, "responded 1, resolved 1") || !strings.Contains(text, "Planned actions: 2") {
if text := out.String(); !strings.Contains(text, "responded 1, provider resolved 0 (resolve planned 1, failed 0)") || !strings.Contains(text, "Planned actions: 2") {
t.Fatalf("stdout = %q, want retry counts from planned actions", text)
}
}
Expand Down Expand Up @@ -128,7 +128,10 @@ func TestRespondJSONRendersStableShape(t *testing.T) {
decoded.Run.HeadSHA != "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa" ||
decoded.Counts.Considered != 1 ||
decoded.Counts.Responded != 1 ||
decoded.Counts.Resolved != 1 ||
decoded.Counts.Resolved != 0 ||
decoded.Counts.ProviderResolved != 0 ||
decoded.Counts.ResolvePlanned != 1 ||
decoded.Counts.ResolveFailed != 0 ||
decoded.Counts.Planned != 2 ||
decoded.Outbox.Outcome != "comment" ||
decoded.Outbox.ExitCode != 0 ||
Expand All @@ -140,6 +143,66 @@ func TestRespondJSONRendersStableShape(t *testing.T) {
}
}

func TestRespondJSONRendersPostedProviderResolve(t *testing.T) {
result := testThreadRespondResult(ledger.OutcomeComment)
result.PlannedActions[1].Status = ledger.PlannedActionPosted
responder := &fakeResponder{result: result}
cmd, out := newTestCommand(t, testConfig(), fakeFactory(responder))

if err := root.Execute(cmd, []string{"respond", "https://github.com/open-cli-collective/codereview-cli/pull/29", "--json"}); err != nil {
t.Fatalf("Execute respond json: %v", err)
}
var decoded struct {
Counts counts `json:"counts"`
}
if err := json.Unmarshal(out.Bytes(), &decoded); err != nil {
t.Fatalf("Unmarshal stdout: %v\n%s", err, out.String())
}
if decoded.Counts.Resolved != 1 ||
decoded.Counts.ProviderResolved != 1 ||
decoded.Counts.ResolvePlanned != 1 ||
decoded.Counts.ResolveFailed != 0 {
t.Fatalf("counts = %#v, want posted provider resolve plus planned resolve", decoded.Counts)
}
}

func TestRespondTextRendersPostedProviderResolve(t *testing.T) {
result := testThreadRespondResult(ledger.OutcomeComment)
result.PlannedActions[1].Status = ledger.PlannedActionPosted
responder := &fakeResponder{result: result}
cmd, out := newTestCommand(t, testConfig(), fakeFactory(responder))

if err := root.Execute(cmd, []string{"respond", "https://github.com/open-cli-collective/codereview-cli/pull/29"}); err != nil {
t.Fatalf("Execute respond: %v", err)
}
text := out.String()
if !strings.Contains(text, "provider resolved 1 (resolve planned 1, failed 0)") {
t.Fatalf("stdout = %q, want posted provider resolve count", text)
}
}

func TestRespondRendersFailedProviderResolveSeparately(t *testing.T) {
result := testThreadRespondResult(ledger.OutcomeComment)
result.PlannedActions[0].Status = ledger.PlannedActionPosted
result.PlannedActions[1].Status = ledger.PlannedActionFailedTerminal
result.Outbox = outbox.Result{Outcome: ledger.OutcomeComment, ExitCode: 1, Posted: 1, FailedTerminal: 1}
result.ExitCode = 1
result.Message = "resolveReviewThread permission denied"
responder := &fakeResponder{result: result}
cmd, out := newTestCommand(t, testConfig(), fakeFactory(responder))

err := root.Execute(cmd, []string{"respond", "https://github.com/open-cli-collective/codereview-cli/pull/29"})
if err == nil {
t.Fatal("Execute respond error = nil, want non-zero result error")
}
text := out.String()
if !strings.Contains(text, "provider resolved 0 (resolve planned 1, failed 1)") ||
!strings.Contains(text, "Outbox: posted 1, pending 0, failed 1") ||
!strings.Contains(text, "Message: resolveReviewThread permission denied") {
t.Fatalf("stdout = %q, want provider resolve failure called out", text)
}
}

func TestRespondRejectsRetryPostsWithDryRun(t *testing.T) {
responder := &fakeResponder{result: testThreadRespondResult(ledger.OutcomeComment)}
cmd, _ := newTestCommand(t, testConfig(), fakeFactory(responder))
Expand Down
Loading
Loading