Skip to content

[CR-6][P1] Add native AI review, PR questions and scoped review instructions #325

New issue
New issue
Open
Open
[CR-6][P1] Add native AI review, PR questions and scoped review instructions#325
Labels
enhancementNew feature or request

Description

@BunsDev
BunsDev
opened on Oct 19, 2026

Outcome

Make the screenshot's Review action, R3 instructions popover and Ask about this pull request composer useful through Modex's existing native coding backend. Reviews produce inspectable, revision-bound findings with explicit coverage, not automatic approvals, published comments or merges.

Parent: #319. Blocked by accepted artifacts: #321's authorized read service, #323's overview/context host and #324's revision/line evidence. Reuse the delivered lifecycle, approval, evidence and recovery contracts of #273/#276–#280 where relevant; open tracker status is not itself a blocker. CR-7 is independent: this issue produces optional drafts/findings, not GitHub write authority.

Context / entry points

Read current AGENTS.md, docs/status.md, #320's accepted source map and the current native runner/backends, event journal, approvals and thread-context implementation. Audit baseline is cb33db5df00e235f68d36b5fac556dc30e0851f2; revalidate current main/open PRs. Repository coding turns use native Claude/Codex CLIs; this issue does not replace them with an inference SDK or a proprietary cloud-review endpoint. Respect the repository rule that the bounded Jev judge must not receive file/turn content.

Proposed focused additions, only if existing equivalents do not suffice: apps/desktop/src/main/engine/code-review/review-context.ts, apps/desktop/src/renderer/components/code-review/ReviewInstructions.tsx, PrConversation.tsx, and corresponding Node/Playwright tests. Integrate existing runner/journal/approval paths instead of creating parallel systems. Assign one owner for shared runner/IPC changes.

Items to do

  • Write failing tests for main-owned review context: account/repository/revision identity, selected files/hunks, coverage limits, provenance, explicit user instructions and stale-head behavior. Use inert synthetic PR data and the existing native-CLI fixture mechanism.
  • Recreate R3's keyboard-accessible popover using existing menu/dialog primitives. The screenshot describes instructions applying across repositories: make that scope visible, use explicit user-entered settings and record the instruction version used for each run. Do not silently reinterpret them as repository-local or automatically copy private project text into global instructions. Changes affect future runs, not an in-flight snapshot. Any additional scope/override UX requires a recorded [CR-1][P1] Establish the Code Review reference contract and deterministic fixtures #320 decision.
  • Build bounded context in main only after validating the requesting account and PR. Include exact base/head revision, selected evidence and omitted-file/byte limits. Treat PR text, diffs, comments, imported repository instructions and URLs as untrusted evidence, separate from the owner's explicit instructions. Do not send unrelated threads, secrets, private Notes or files outside the authorized selection.
  • Feature-detect the installed native backend and verify its actual protocol/safety controls. Enforce read-only review in the process/tool environment: no local-worktree edits, arbitrary shell execution, hooks, dependency installs, inherited GitHub credentials, unrestricted MCP tools or external writes. A prompt saying read-only is insufficient. Use the existing isolation/capability mechanism or a narrowly reviewed snapshot-only path; when enforcement cannot be established, keep review unavailable with a clear reason.
  • Require an explicit user start with visible backend, context scope and applicable cost/availability controls. Reuse existing concurrency/resource limits and stop/lifecycle handling. Selecting a PR or editing instructions must not start an inference request. Do not automatically retry a provider request whose outcome is uncertain or start unattended recurring reviews.
  • Render findings through the existing conversation/evidence components. Bind citations to PR revision, path, side and line; show coverage omissions and outdated results. A completed native turn is not proof that tests ran, the diff is correct or GitHub checks passed. Unsupported/malformed result references remain unverified rather than receiving invented anchors.
  • Scope PR questions and drafts to the correct account/PR conversation; selecting another PR must not leak previous context or send a draft. Reuse existing transcript/recovery semantics rather than a new chat store. Restoring a session replays evidence, not the provider action.
  • Provide an explicit handoff of selected findings as a draft to CR-7 when available. Never post, approve, request changes or merge from model output. Keep AI conclusions, human intent and server authorization as separate facts.

Acceptance criteria

  • AC1: R3 popover opens/closes accessibly, clearly states instruction scope, persists only the intended user settings and records the actual instruction version for each review.
  • AC2: the context builder excludes wrong-account/repository data and binds findings to the selected immutable revision; new heads mark old results outdated without silently re-reviewing.
  • AC3: adversarial PR content attempting tool execution, secret access or GitHub writes is rejected by the actual capability/isolation boundary, not merely ignored in model prose. An unsupported boundary disables the feature.
  • AC4: explicit start, streaming, stop, failure, unknown completion and restart use the existing native lifecycle correctly, with no duplicate inference after reload or uncertain failure.
  • AC5: omitted files and unsupported references are visible; no AI statement is promoted to test evidence, GitHub approval or merge authority.
  • AC6: native-CLI fixture tests through the real app pass. Separately record one explicitly authorized live native-provider smoke test with backend/version, revision and bounded scope, without secrets; an unavailable live run remains an open gate.

Validation / handoff

Add main-context/isolation tests, native-CLI fixtures and Electron tests for instructions, PR switching, stale findings, stop and restart. Run pnpm build && pnpm test and pnpm test:e2e; do not equate fixture output with live provider evidence. A live canary needs the caller's existing or newly explicit authorization and budget. Record actual safety controls, denied-action evidence, SHA, commands and residual limits in docs/reviews//docs/status.md. Do not weaken policy to make the demo work. Sources: #319's primary-source review and current repository-native backend documentation.

Long-running goal prompt

Implement CR-6 for #319 in TypeSafeAI/modex. Read this issue, #320/#321/#323/#324, the relevant accepted #273 contracts, current source and scoped AGENTS.md. Verify main and owned-worktree state. Build the smallest native review and PR-question path, beginning with failing context/isolation/lifecycle tests. Reuse the actual runner, journal, evidence UI and approval machinery; do not add an inference SDK or a second chat runtime. Make instructions' scope explicit and snapshot the selected account/PR/revision/context for each user-started run. Enforce read-only authority in code and the real process/tool environment; refuse operation when that cannot be established. Keep secrets, unrelated context and file content for the Jev judge out. Do not execute foreign PR code, publish model findings, merge, or replay uncertain paid requests. Inspect R3 and test switching, stop and restart. Work within authorized resource limits, checkpoint full SHA, tests/evidence, remaining gates and one next action, and hand off a small signed PR with mock versus live evidence clearly separated.

Follow-up / resume prompt

Resume CR-6 by reading the live issues/PR, current native-backend capabilities and checkpoint. Verify worktree ownership and the actual diff, then rerun the context/isolation smoke tests. Reconcile run receipts before doing anything that could restart inference; an uncertain prior outcome is not permission to resend. Check whether the account, PR head, instruction version or producer contract changed. Continue one missing acceptance item, retaining fail-closed tool authority and coverage labels. Record unsupported native/manual gates rather than simulating them. Do not post, merge or expand permissions.

Independent review prompt

Review actual context assembly and process/tool enforcement, not only prompts or screenshots. Attempt cross-PR/account disclosure, injected instructions, credential access, unintended tool calls, changed-head citations and duplicate execution after restart. Verify findings never become approval/merge authority and fixture evidence is not labeled live. Exercise R3 keyboard focus and visible instruction scope. Report security and acceptance blockers with reproducible evidence.

Activity

  1. BunsDev
    added
    enhancementNew feature or request
    on Oct 19, 2026

  2. BunsDev commented on Oct 20, 2026

    @BunsDev
    BunsDev
    on Oct 20, 2026
    ContributorAuthor

    The CR-1 contract is on main as 43d5b9c (#334). Bind to these files at that commit:

    • apps/desktop/src/shared/code-review.ts (the contract)
    • apps/desktop/test/fixtures/code-review.ts (synthetic fixtures)
    • docs/design/code-review.md (source map, references, inventory, open decisions)

    For CR-6:

    • capabilities keeps review, ask and instructions unavailable, with this issue as the owner, until this gate delivers them.

    Coven-Harness: Claude Code / claude-opus-5-5

  3. mariscate commented on Oct 22, 2026

    @mariscate
    mariscate
    on Oct 22, 2026

    Refreshed goal prompts — 2026-10-22 (main at 0e674e7)

    Where it stands: not started. It depends on #323's overview context and #324's revision/line anchors, and neither has started. Until both land, capabilities() keeps review, ask and instructions unavailable, owned here. Already on main and reusable: the typed conversation items, evidence rows, human-request broker and turn lifecycle from the CF work (#273, shipped in v0.3.0). The rule that Jev never sees file or turn content still applies. Waiting on Val: whether the PR composer shows an access mode at all (C82).

    Can start now, before #323/#324: a spike that proves the read-only boundary. For each installed claude and codex CLI version, find out which flags or sandbox settings actually block edits, shell, hooks, MCP and network writes.

    Long-running goal prompt

    Implement CR-6 (#325) in TypeSafeAI/modex. Read AGENTS.md (coding turns run only through the claude/codex CLIs; no API mode), #319, this issue, docs/design/code-review.md and docs/status.md, then verify main's SHA. Create your own worktree. Phase 1, possible now: for the installed claude and codex CLIs, prove with real denial tests which process, sandbox and tool settings enforce a read-only review: no worktree edits, no arbitrary shell, no hooks, no inherited gh credentials, no unrestricted MCP, no external writes. Write the results to docs/reviews. If neither CLI can be confined, keep review unavailable and say why. Phase 2, after #323/#324 land: build the review context in main (src/main/engine/code-review/) from validated account/repo/revision data, with bounded files and bytes and the omissions listed. Keep PR content separate from the owner's instructions as untrusted evidence. Build the R3 instructions popover with existing menu primitives, scoped as the screenshot shows, with the version recorded on every run. Start a review only on an explicit click, through the existing runner and lifecycle; never auto-retry an uncertain request. Show findings with the existing evidence components, anchored to CR-5's anchors and marked outdated when the head moves. Offer findings to #326 as drafts only. Fixture tests through the real app first. A live run needs Val's explicit authorization and budget. Open one signed PR and don't merge.

    Follow-up / resume prompt

    Resume CR-6. Confirm you own the worktree and fetch main. Check whether #323/#324 have landed; until both have, work only on Phase 1 isolation proofs. Re-run the denial tests against the CLI versions installed now, because they change often, and record each version. Continue with the weakest AC: isolation, stale-head findings, stop and restart without duplicate inference, or instruction versioning. Never relax policy to make a demo work. Post a checkpoint: SHA, CLI versions, denial evidence, fixture vs live results, and one next action.
  4. BunsDev
    added a commit that references this issue on Oct 22, 2026

    Add the CR-8 acceptance matrix, Electron journeys and cross-slice reg…

    ac5fc58

  5. BunsDev commented on Oct 23, 2026

    @BunsDev
    BunsDev
    on Oct 23, 2026
    ContributorAuthor

    Refreshed goal prompts — 2026-10-23 (main at 203a350)

    On main already:

    Still open:

    • Nothing started. There is no branch, worktree or PR for CR-6.
    • Dependencies: CR-6 needs CR-4's overview context and CR-5's revision and line anchors.
    • Waiting on Val: whether the PR composer shows an access mode at all (C82).
    • Can start now: the Phase 1 spike. For each installed claude and codex CLI version, find which flags or sandbox settings actually block edits, shell, hooks, MCP and network writes.

    Long-running goal prompt

    Implement CR-6 (#325) in TypeSafeAI/modex.

Read:
- AGENTS.md. Coding turns run only through the claude and codex CLIs; there is no API mode.
- #319, this issue, docs/design/code-review.md and docs/status.md.
Then verify main's SHA, and create your own worktree with scripts/worktree.sh new.

Phase 1 (possible now): for the installed claude and codex CLIs, prove with real denial tests which process, sandbox and tool settings enforce a read-only review:
- no worktree edits;
- no arbitrary shell;
- no hooks;
- no inherited gh credentials;
- no unrestricted MCP;
- no external writes.
Record each CLI version and write the results to docs/reviews. If neither CLI can be confined, keep review unavailable and say why.

Phase 2 (after #393 and CR-5 land):
- Build the review context in main (src/main/engine/code-review/) from validated account, repository and revision data. Bound files and bytes, and list what was left out.
- Keep PR content separate from the owner's instructions, as untrusted evidence.
- Build the R3 instructions popover with the existing menu primitives, scoped as the screenshot shows, and record the instructions' version on every run.
- Start a review only on an explicit click, through the existing runner and lifecycle. Never auto-retry an uncertain request.
- Show findings with the existing evidence components, anchored to CR-5's anchors and marked outdated when the head moves. Offer findings to #326 as drafts only.

Write red tests first. Run the fixture tests through the real app with pnpm build && pnpm test && pnpm test:e2e. A live run needs Val's explicit authorization and budget. Update docs/status.md and docs/design/code-review.md. Open one signed PR and don't merge without Val's go-ahead.

    Follow-up / resume prompt

    Resume CR-6.
1. Confirm you own the worktree and fetch main.
2. Check whether #393 (CR-4) and the CR-5 PR have landed. Until both have, work only on the Phase 1 isolation proofs.
3. Re-run the denial tests against the CLI versions installed now, because they change often, and record each version.
4. Continue with the weakest AC: isolation, stale-head findings, stop and restart without duplicate inference, or instruction versioning.
Never relax policy to make a demo work. Post a checkpoint: SHA, CLI versions, denial evidence, fixture vs live results, and one next action.

  6. BunsDev commented on Nov 1, 2026

    @BunsDev
    BunsDev
    on Nov 1, 2026
    ContributorAuthor

    CR-6 checkpoint, 2026-11-01 (main at 0a8c513)

    PR: #472. Docs and evidence only.

    Denial tests re-run on the installed CLIs (Claude Code 2.1.316, codex-cli 0.166.0; the earlier evidence was taken on 2.1.312):

    • code-review-*, claude-backend and desktop-service-drain unit tests: 224 tests, 223 passed, 1 skipped (Windows only).

    • MODEX_CR6_NATIVE=1 node --test test/code-review-isolation.native.test.mjs: 6 of 6 passed, 0 skipped. The production REVIEW_CONFINEMENT_ARGS case:

      • advertises only Read;
      • refuses Bash, Write, WebFetch and mcp__sentinel__write with "No such tool available";
      • refuses the outside file and the symlink under --restricted;
      • reads the selected file;
      • leaves no write, hook or MCP marker.

      The positive controls still fire. Codex's read-only profile still allows shell and outside reads, so it stays out of reviews.

    • e2e/code-review-agent.spec.ts: 6 of 6 passed (part of a focused 51/51 run).

    Evidence by kind:

    • Fixture: unit tests and the Electron spec, with a synthetic gh and a synthetic claude.
    • Native fixture: the real 2.1.316 CLI with a scripted loopback provider and a synthetic key.
    • Live: none.

    Live-smoke procedure written into the evidence doc. It has not run.

    • It uses a disposable Modex home, plus pass-through gh/claude wrappers that log argv only.
    • It captures the exact argv, the snapshot's file list, the reply, the bound findings, the run record and the instruction version.
    • Closing checks: no POST/PUT/PATCH/DELETE, the gh login unchanged, and the token absent from everything Modex wrote.

    Waiting on Val:

    • a repository and PR for the smoke, and a go;
    • decision 7 (C82);
    • VoiceOver for R3;
    • the R3 references.

    Next action: run the smoke exactly as written once a repository is named.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    enhancementNew feature or request

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions