Skip to content

fix: reuse CR-settled thread summaries#395

Merged
rianjs merged 4 commits into
mainfrom
fix/394-cr-settled-thread-summary-cache
Jun 27, 2026
Merged

fix: reuse CR-settled thread summaries#395
rianjs merged 4 commits into
mainfrom
fix/394-cr-settled-thread-summary-cache

Conversation

@rianjs

@rianjs rianjs commented Jun 27, 2026

Copy link
Copy Markdown
Contributor

Summary

  • add CR-settled inline thread summaries keyed by thread_id when GitHub provider resolution is unavailable
  • reuse cached settled summaries in PR dossier discussion artifacts and reviewer selection context instead of re-summarizing full threads
  • keep provider-resolved counts distinct from planned/failed resolve actions in cr respond output

Validation

  • make tidy
  • make build
  • make lint
  • make test
  • go test ./internal/threadcontext ./internal/pipeline ./internal/cmd/respondcmd
  • go test ./...
  • built /tmp/cr-394 and dry-ran PR feat: add reusable thread lifecycle pipeline #359 with codex-rianjs-bot; verified a CR-settled cached summary from thread PRRT_kwDOSrNqjc6MjnWL was reused in dossier summary/final artifacts without leaking codereview:thread-summary marker metadata

Closes #394

@rianjs rianjs changed the title Reuse CR-settled thread summaries fix: reuse CR-settled thread summaries Jun 27, 2026
@rianjs

rianjs commented Jun 27, 2026

Copy link
Copy Markdown
Contributor Author

TDD review

STATUS: blockers=0 majors=0 minors=0 nits=0

The test coverage is acceptable to proceed.

Follow-up coverage added after the initial TDD pass:

  • thread_id wins over mismatched anchor when decoding dossier discussion summaries.
  • raw dossier cached_summary.source distinguishes provider_resolved and cr_settled.
  • successful provider resolve counters are asserted in both text and JSON cr respond output.

Validation after coverage updates:

  • rtk go test ./internal/pipeline ./internal/cmd/respondcmd ./internal/threadcontext
  • rtk go test ./...
  • rtk make lint
  • rtk make build

@rianjs

rianjs commented Jun 27, 2026

Copy link
Copy Markdown
Contributor Author

Final architect pass

STATUS: blockers=0 majors=0 minors=0 nits=0

Remaining merge-blocking concerns from the architecture/code pass: none identified. Draft status and the pending cr review loop remain procedural gates.

Focused verification run from the final architect pass:

  • rtk go test ./internal/threadcontext ./internal/pipeline ./internal/cmd/respondcmd

@rianjs-bot

rianjs-bot Bot commented Jun 27, 2026

Copy link
Copy Markdown

Automated PR Review

Reviewed commit: ba495fa15c20
Profile: codex-rianjs-bot - Posting as: rianjs-bot[bot]

Summary

Reviewer Findings
go:implementation-tests 1
go:implementation-tests (1 finding)

Major - internal/pipeline/pipeline.go:1024

When ThreadContext is present, this path rebuilds the prompt from normalized thread context but then clears knownThreadIDs before decoding the model response. buildSelectionPrompt still includes thread_id values in the selection input, and llm.DecodeSelection rejects any thread_actions whose IDs are not in KnownThreads, so a valid model response that selects a cached CR-settled thread will now fail as llm: unknown thread .... Keep the allowlist populated from req.ThreadContext (or derive it from the prompt threads) and add a focused selection-phase test that exercises a thread_actions response while ThreadContext is used.

Reviewer Coverage

Reviewer Status Inspected Skipped Constraints
go:implementation-tests complete_broad internal/cmd/respondcmd/respondcmd.go, internal/cmd/respondcmd/respondcmd_test.go, internal/pipeline/pipeline.go, internal/pipeline/pipeline_test.go, internal/threadcontext/threadcontext.go, internal/threadcontext/threadcontext_test.go unavailable Review limited to static inspection of the assigned Go files and nearby repo guidance in a read-only sandbox; I did not execute the test suite.
unassigned incomplete_unassigned unavailable docs/architecture.md changed files were not assigned to a selected reviewer

0 PR discussion threads considered. 0 summarized; 0 resolved.


Completed in 3m 52s | unavailable | gpt-5.4 | cr dev
Field Value
Model gpt-5.4
Reviewers go:implementation-tests
Engine codex_cli · gpt-5.4
Reviewed by cr · rianjs-bot[bot]
Duration 3m 52s wall · 3m 32s compute
Cost unavailable
Tokens unavailable

Per-workstream usage

Workstream Model In Out Cache read Cache create Cost Duration
orchestrator-selection gpt-5.4 unavailable unavailable unavailable unavailable unavailable 17s
go:implementation-tests gpt-5.4 unavailable unavailable unavailable unavailable unavailable 3m 05s
orchestrator-rollup gpt-5.4 unavailable unavailable unavailable unavailable unavailable 9s

@rianjs-bot rianjs-bot Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Automated PR review completed with outcome: request_changes.

Comment thread internal/pipeline/pipeline.go
@rianjs rianjs marked this pull request as ready for review June 27, 2026 17:25
@rianjs-bot

rianjs-bot Bot commented Jun 27, 2026

Copy link
Copy Markdown

Automated PR Review

Reviewed commit: de1425765c45
Profile: codex-rianjs-bot - Posting as: rianjs-bot[bot]

Summary

Reviewer Findings
go:implementation-tests 0

Reviewer Coverage

Reviewer Status Inspected Skipped Constraints
go:implementation-tests complete_constrained internal/cmd/respondcmd/respondcmd.go, internal/cmd/respondcmd/respondcmd_test.go, internal/pipeline/pipeline.go, internal/pipeline/pipeline_test.go, internal/threadcontext/threadcontext.go, internal/threadcontext/threadcontext_test.go unavailable Review limited to the six assigned Go files and nearby repo-local context; docs/development.md was not present in this review scope.
unassigned incomplete_unassigned unavailable docs/architecture.md changed files were not assigned to a selected reviewer

0 PR discussion threads considered. 0 summarized; 0 resolved.


Completed in 3m 38s | unavailable | gpt-5.4 | cr dev
Field Value
Model gpt-5.4
Reviewers go:implementation-tests
Engine codex_cli · gpt-5.4
Reviewed by cr · rianjs-bot[bot]
Duration 3m 38s wall · 3m 17s compute
Cost unavailable
Tokens unavailable

Per-workstream usage

Workstream Model In Out Cache read Cache create Cost Duration
orchestrator-selection gpt-5.4 unavailable unavailable unavailable unavailable unavailable 13s
go:implementation-tests gpt-5.4 unavailable unavailable unavailable unavailable unavailable 2m 55s
orchestrator-rollup gpt-5.4 unavailable unavailable unavailable unavailable unavailable 7s

@rianjs-bot rianjs-bot Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Automated PR review completed with outcome: comment.

@rianjs rianjs merged commit e155a71 into main Jun 27, 2026
10 checks passed
@rianjs rianjs deleted the fix/394-cr-settled-thread-summary-cache branch June 27, 2026 18:00
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.

Treat CR thread-summary replies as durable settled context when provider resolution is unavailable

1 participant