Skip to content

feat: add canonical review action lifecycle - #205

Merged
oratis merged 34 commits into
mainfrom
codex/review-action-lifecycle
Aug 2, 2026
Merged

feat: add canonical review action lifecycle#205
oratis merged 34 commits into
mainfrom
codex/review-action-lifecycle

Conversation

@oratis

@oratis oratis commented Aug 1, 2026

Copy link
Copy Markdown
Owner

Summary

  • add capability-negotiated review/apply for one canonical finding or a bounded batch
  • resolve original finding payloads from the durable thread inside app-server instead of trusting client-supplied path or replacement data
  • persist review_action items that correlate action kind, finding ids, and the canonical action turn
  • reject unknown or duplicate ids, oversized batches, unsafe stored payloads, and direct reviewAction injection through turn/start
  • route single and batch Apply through VS Code, Desktop, and LSP while retaining ordinary Edit/Write permissions, approvals, hooks, sandboxing, cancellation, and snapshots
  • add a VS Code Apply All Review Findings command and update protocol/design docs

Validation

  • pnpm format:check
  • pnpm lint (one pre-existing warning)
  • pnpm typecheck
  • pnpm test (1027 passed, 12 skipped)
  • pnpm docs:check
  • pnpm build
  • pnpm --filter @deepcode/desktop test:e2e (4 passed)
  • sidecar and VS Code child CJS syntax checks
  • VSIX: 8 files, 178.46 KB

t added 30 commits August 1, 2026 13:32
@oratis
oratis changed the base branch from codex/structured-review-findings to main August 2, 2026 06:36
@oratis
oratis marked this pull request as ready for review August 2, 2026 06:46
@oratis

oratis commented Aug 2, 2026

Copy link
Copy Markdown
Owner Author

Review: approved ✅ — the trust boundary here is exactly right

The key line: review/apply resolves the original finding payload from the durable thread inside app-server rather than trusting client-supplied path or replacement data.

That is the correct call and it is the difference between a review feature and a remote-write primitive. If Apply accepted the client's copy of the path and replacement, any client (or anything that could talk to the socket) could ask the server to write arbitrary content to an arbitrary path and have it attributed to a "review finding". Taking only an id and dereferencing it server-side against durable state removes that entire class.

What I checked

  • Direct reviewAction injection through turn/start is rejected. Closing the front door while leaving that side channel open would have made the id-only design decorative. Good that it's explicitly tested rather than assumed.
  • Unknown ids, duplicate ids, oversized batches, and unsafe stored payloads are all rejected. Duplicate-id rejection matters for batch: the same finding applied twice in one batch would compound edits unpredictably. Re-validating stored payloads is the paranoid-but-correct move — durable state written by an earlier version is still input.
  • Single and batch Apply both retain ordinary Edit/Write permissions, approvals, hooks, sandboxing, cancellation, and snapshotsfeat: persist structured review findings #204's property survives batching, which is where it would most plausibly have been optimized away.
  • review_action items correlate action kind, finding ids, and the canonical action turn. That correlation is precisely what feat: add conflict-safe review revert #206 needs to revert safely, so this is the right foundation for it.

Validation: CI green; full suite green locally at the stack tip.

@oratis
oratis merged commit e5a9b4d into main Aug 2, 2026
5 checks passed
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.

1 participant