Skip to content

fix(change-review): derive imported edits from native history - #1777

Closed
beruro wants to merge 3 commits into
junyu/audit-mobile-request-schedulerfrom
junyu/audit-imported-change-review
Closed

beruro wants to merge 3 commits into
junyu/audit-mobile-request-schedulerfrom
junyu/audit-imported-change-review

Conversation

@beruro

@beruro beruro commented Sep 14, 2026 •

Copy link
Copy Markdown
Collaborator

Problem

Imported Codex edits can disappear from desktop review because native runtime artifacts are not the imported rollout authority. Wrapped exec patches and empty successful tool results also lose completion attribution. Historical review must not substitute today's worktree or label partial patches as full-file/net changes.

Solution

Share the non-evaluating wrapped-patch scanner between metadata and transcript projection, attribute completed results, and invalidate old metadata parser caches. Add bounded read-only imported edit projection and exact-call historical reconstruction, backed by original patches or verified pre-tool captures. Route desktop diff lists and the mobile session/changes API through these authorities. Explicit workspace scope reads current Git state and is labeled separately; absent/ambiguous historical content stays unavailable.

Potential risks

Stacked on #1767. The optional changeReview capability/API has no new mobile UI in this PR. Parser version advances 14 to 16, triggering metadata rebuild on subsequent scans; raw transcripts are unchanged and no historical cleanup runs. Review rejects histories beyond 64 MiB, bounds context lookup to eight prior turns/32 MiB, and limits projected artifacts to 500 chunks/2 MiB; parse intermediates can be larger than retained output. Two single-entry caches use source path/size/mtime and serialize reads; external edits preserving metadata can evade invalidation. Incomplete, ambiguous or unanchored patches may intentionally lack full-file reconstruction; multiple patches are not summed as net diff. Workspace commands have five-second/512 KiB limits and do not write. Revert to old readers and allow metadata cache rebuild; no database schema migration. Live Codex, phone/hosted relay UI, Windows and sustained CPU/RSS remain unverified.

Verification

  • CI failure follow-up: changed resolve_string_expression(input, &expression) to pass expression directly. cargo clippy --manifest-path src-tauri/Cargo.toml -p orgtrack_core --all-targets --locked -- -D warnings: passed; the commit hook also passed scoped orgtrack_core clippy. A subsequent full-workspace run exposed seven additional needless-borrow and style findings in the same change set; those were corrected. cargo clippy --manifest-path src-tauri/Cargo.toml --workspace --all-targets --locked -- -D warnings: passed; the follow-up commit hook passed scoped agent_core and org2 clippy.

  • cargo test --manifest-path src-tauri/Cargo.toml -p orgtrack_core --lib sources::codex --locked --target-dir <existing-local-target>: 103 passed, three existing optional tests ignored. Covers actual temporary rollout parsing, wrapped patches and completion attribution.

  • cargo test --manifest-path src-tauri/Cargo.toml --lib orgtrack:: --locked --target-dir <existing-local-target>: 60 passed, two existing optional tests ignored; includes original rollout -> normalized events -> artifacts and historical replay boundaries.

  • cargo test --manifest-path src-tauri/Cargo.toml --lib api::mobile_bridge --locked --target-dir <existing-local-target>: 133 passed, including read-only initialize gating, owner/round selection and a real temporary Git workspace with external-symlink rejection.

  • cargo test --manifest-path src-tauri/Cargo.toml -p agent_core --lib core::tools::file_history::review --locked --target-dir <existing-local-target>: one passed, exercising the real capture/write/read path, exact owner/tool lookup and corrupted-backup rejection.

  • Standard commit hooks passed scoped clippy for agent_core/org2/orgtrack_core. Final 25-file diff contains only this review data path. Native visuals and production resource measurements were not run.

Audit

Architecture L1 actual source-boundary tests/clippy; L2-L4 one imported transcript authority; L5 pending/completed/failed edit attribution and cache invalidation; L6-L7 bounded readers and exact reconstruction; L8 parser-version and optional RPC compatibility; L9 metadata, desktop, mobile and captured-file readers; L10 original transcript/capture, never current files for historical scope. Resource verdict: on-demand bounded readers/caches with no polling; real connected workload measurements remain open.

@Harry19081 Harry19081 added bug Something isn't working workstation Workstation, editor, source control, LSP, or status bar labels Sep 15, 2026
@Harry19081

Copy link
Copy Markdown
Member

Closing as superseded: #1826 (f46920c) already landed this change set on develop, and the stacked base #1767 has merged.

Merging current develop into this branch and resolving the 5 conflicts leaves a zero diff against develop, apart from one line not carried forward: routing session/changes with a params.query into the Search lane in request_scheduler.rs. session/changes has no query param (its Params is deny_unknown_fields), so such a request always fails — yet in the Search lane it could still supersede a genuine pending session/list search. develop already schedules session/changes in the Read lane.

The one piece unique to this PR, the change_review_requires_initialize_and_is_readonly test in rpc.rs, can land in a follow-up.

@Harry19081 Harry19081 closed this Sep 24, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working workstation Workstation, editor, source control, LSP, or status bar

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants