-
Notifications
You must be signed in to change notification settings - Fork 0
feat: add reusable thread lifecycle pipeline #359
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
a376644
57959a7
e000c98
bbb4e15
010b195
c63e75b
cd78dd8
b822314
3fb8254
e6399bf
bac48c1
b006629
66e98b3
994639f
331c2c5
34d7a6d
432cb9a
4d73a26
8da289a
05ba883
3826a3b
5d8f3e2
853bab4
36e7acc
54c74b0
7e1a922
82bb27b
ad443c6
5c22e83
43ea71b
3f1a284
a68aabe
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,118 @@ | ||
| # Architecture Decisions | ||
|
|
||
| This document records review-pipeline boundaries that are intended to stay | ||
| stable as the implementation evolves. | ||
|
|
||
| ## Durable LLM Execution Boundary | ||
|
|
||
| All production structured LLM actions must flow through | ||
| `internal/llmlifecycle`. Callers describe the task ID, phase, prompt input, | ||
| structured output contract, model/effort, artifact paths, and run/session | ||
| scope. The lifecycle runner owns provider invocation, structured-output retry, | ||
| provider-session resume, pre-flight reuse, task metadata, accepted-output | ||
| artifacts, session persistence, progress breadcrumbs, and failure | ||
| classification. | ||
|
|
||
| The final commit marker for a task is | ||
| `llm-tasks/<encoded-task-id>/metadata.json`. Writers must publish validated | ||
| output or failed-attempt payloads first, persist the ledger session row when | ||
| the task is run-owned, and write metadata last. Resume code must trust only the | ||
| final metadata path, never temporary files or partial payloads. | ||
|
|
||
| New LLM-backed components must not call `internal/llm` structured helpers | ||
| directly. They should call `llmlifecycle` through explicit, fakeable | ||
| dependencies in unit tests and should return domain results rather than | ||
| posting comments or mutating provider state. | ||
| `internal/architecture/llm_lifecycle_test.go` enforces that direct structured | ||
| helper calls stay inside `internal/llm` and `internal/llmlifecycle`. Direct | ||
| provider-adapter calls should also stay behind `llmlifecycle` for production | ||
| structured tasks; code review owns that broader boundary until a stronger | ||
| static guardrail exists. | ||
|
|
||
| Most lifecycle tasks are run-owned and must have a matching ledger session row | ||
| when a provider session is available. Caller-owned no-run tasks are allowed only | ||
| where no review run exists yet, such as `SelectionOnly` and the pre-run approval | ||
| override classifier. Those tasks may reuse artifact metadata without a ledger | ||
| session row, but they still use the same metadata schema and lifecycle runner. | ||
|
|
||
| ## Stage Model Resolution | ||
|
|
||
| Runtime model choice must be resolved through `internal/stagemodel`. Code that | ||
| executes an LLM stage must not hard-code model IDs and must not call | ||
| `config.ResolveModelTier` directly. | ||
|
|
||
| `stagemodel.ResolveStageModel` is the single runtime path from profile | ||
| preferences and command overrides to a concrete model and effort. The request | ||
| must include the named stage, requested tier, default effort, and any explicit | ||
| operator override. The resolver applies user profile `llm.model_map` values, | ||
| built-in provider defaults, and configured tier floors before returning the | ||
| concrete runtime choice. | ||
|
|
||
| This boundary exists so model catalog data, provider capabilities, token costs, | ||
| and profile-level tier floors can be added without touching individual review | ||
| stages. Runtime hard-coding bypasses user preference and is a bug. | ||
|
|
||
| Reviewer `agent.model_id` is an exact provider-specific model override. It must | ||
| still enter runtime execution through `stagemodel.ResolveStageModel` as a model | ||
| override rather than bypassing the resolver, but it intentionally bypasses the | ||
| tier map because the agent author selected a concrete model. | ||
|
|
||
| The direct `config.ResolveModelTier` exception is config inspection and the | ||
| resolver implementation itself. | ||
| `internal/architecture/model_resolution_test.go` enforces that direct | ||
| `config.ResolveModelTier` calls stay inside approved packages. Hard-coded | ||
| runtime model IDs remain a code-review concern until model-catalog guardrails | ||
| exist. | ||
|
|
||
| ## Git Provider Writes | ||
|
|
||
| Provider writes have one durable path: planned actions in the ledger followed | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This section states provider writes have one durable path and that commands should not mutate provider state directly, but the new architecture guardrail test does not actually enforce that repo-wide rule: Reply inline to this comment. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This section states provider writes have one durable path and that commands should not mutate provider state directly, but the new architecture guardrail test does not actually enforce that repo-wide rule: Reply inline to this comment. |
||
| by outbox execution. Commands and domain analyzers should not post comments, | ||
| reply to review threads, resolve threads, submit reviews, or mutate provider | ||
| state directly. | ||
|
|
||
| This keeps markers, retries, reconciliation, idempotency, and resume behavior in | ||
| one place. New commands such as `cr respond` should produce planned thread | ||
| actions and let the reviewplan/ledger/outbox flow perform provider writes. | ||
|
|
||
| ## Inline Thread Lifecycle | ||
|
|
||
| Inline PR discussion threads are domain input, not provider-specific prompt | ||
| 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. | ||
| - `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. | ||
|
|
||
| `cr review` and `cr respond` should share the same normalization, filtering, | ||
| model resolution, LLM execution, and action-planning components. `cr respond` | ||
| is a command-shaped reuse of the thread-response portion of the review | ||
| pipeline, not a separate posting system. | ||
|
|
||
| ## Retention And Cleanup | ||
|
|
||
| Durable run-owned LLM tasks, thread-analysis results, and artifacts must be | ||
| owned by a run and must be safe to delete through the existing data lifecycle | ||
| commands. Database rows should reference `runs(run_id)` with cascade delete | ||
| semantics, and large artifacts should live under the run artifact directory. | ||
|
|
||
| When the retention window elapses, normal prune/GC commands should remove these | ||
| results along with the rest of the run. If a user deletes retained state, a | ||
| future review may need to spend time and tokens recreating it; that is the | ||
| expected tradeoff for user-controlled local data retention. | ||
|
|
||
| Caller-owned no-run task artifacts must live under the configured data root or | ||
| an explicit caller artifact root. If no run is eventually allocated for that | ||
| artifact root, the directory is treated as orphaned local data and must be safe | ||
| for `cr data purge` to remove. | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -7,14 +7,20 @@ Collective standards and automation remain canonical in their own repositories. | |
|
|
||
| codereview-cli is the Open CLI Collective code-review CLI and ships the `cr` | ||
| binary. It provides configuration and credential commands, trusted-agent | ||
| inspection, dry-run and live pull-request review orchestration, named LLM | ||
| session management, and local data lifecycle commands. | ||
| inspection, dry-run and live pull-request review orchestration, inline thread | ||
| response handling through `cr respond`, named LLM session management, and local | ||
| data lifecycle commands. | ||
|
|
||
| The current Go code is a Cobra command tree in `internal/cmd/*` with a thin | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. File-level note: docs/development.md The repo-local project overview still summarizes the current CLI surface without mentioning the new Reply inline to this comment. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. File-level note: docs/development.md The repo-local project overview still summarizes the current CLI surface without mentioning the new Reply inline to this comment. |
||
| `cmd/cr` entrypoint, shared exit-code mapping in `internal/cmd/exitcode`, and | ||
| version plumbing in `internal/version`. Review orchestration is split across | ||
| `internal/pipeline`, `internal/reviewrun`, `internal/reviewplan`, | ||
| `internal/outbox`, `internal/gate`, and `internal/gateio`. | ||
| `internal/pipeline`, `internal/reviewrun`, `internal/threadrespond`, | ||
| `internal/reviewplan`, `internal/outbox`, `internal/gate`, and | ||
| `internal/gateio`. | ||
|
|
||
| Architecture guardrails for LLM execution, model resolution, Git provider | ||
| writes, inline thread lifecycle, and retention live in | ||
| [`docs/architecture.md`](architecture.md). | ||
|
|
||
| Within `internal/pipeline`, the public entry points are `DryRun`, `Live`, and | ||
| `SelectionOnly`. `DryRun` and `Live` execute the full review pipeline, while | ||
|
|
@@ -80,7 +86,8 @@ make clean # remove build artifacts | |
| state/config adapters in `internal/config`, `internal/ledger`, and | ||
| `internal/statepaths`, provider/LLM adapters in their owning packages, and | ||
| review posting/gating in `internal/outbox`, `internal/gate`, and | ||
| `internal/gateio`. | ||
| `internal/gateio`, and response-only inline discussion handling in | ||
| `internal/threadrespond`. | ||
|
|
||
| ## Interactive Init Notes | ||
|
|
||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This PR documents
Git Provider WritesandInline Thread Lifecycleas stable architecture boundaries, but unlike the LLM lifecycle and stage-model sections there is no corresponding enforceable guardrail in the diff. Right now those two invariants live only in prose, so future commands can quietly reintroduce direct provider writes or bypassthreadcontext/threadanalysiswithout any test catching it. Add architecture tests similar tointernal/architecture/llm_lifecycle_test.goandinternal/architecture/model_resolution_test.gothat restrict provider-write calls to the posting path and verify thread-lifecycle packages stay on the intended domain/lifecycle seams.Reply inline to this comment.