Skip to content

Resolve owned rebase conflicts in the sandbox - #144

Merged
mchwang merged 2 commits into
mainfrom
codex/issue22-owned-conflicts
Oct 7, 2026
Merged

mchwang merged 2 commits into
mainfrom
codex/issue22-owned-conflicts

Conversation

@mchwang

@mchwang mchwang commented Oct 7, 2026 •

Copy link
Copy Markdown
Contributor

Summary

  • route trusted-ledger owned commit conflicts through the existing sandboxed conflict resolver
  • bind owned conflicts to the exact plan item from one captured invocation context
  • reject context drift immediately before launch and after final export, before copy-back
  • preserve owned commit attribution and trailers without emitting foreign-conflict provenance
  • keep compatibility with the existing foreign-conflict resolver assembly

This is part of issue #22. It supplies the owned-conflict resolution slice; the separate F5 orchestration work remains responsible for guarded admission and recovery integration.

Done when

An owned conflicting commit is resolved only in the sandbox with its exact plan item and conflict paths, while its owner, author identity, and trailers survive the rewrite; stale plan context cannot launch or apply a child result; the affected suites and typecheck pass on the pushed head.

Failing-before evidence

The new regressions failed against the prior behavior:

  • owned conflict stopped with An owned commit conflict needs its plan-item resolver
  • sandbox prompt/input had no current plan-item context
  • both controlled plan-revision races resolved successfully instead of refusing stale launch/copy-back

Validation

Validated head: bb33705503a7688110e2a47c059f7b4a480eb2c3

  • focused context-race regressions: 2/2 passed after failing before
  • changed rebase suites: 92/92 passed
  • affected suites: 238/238 passed across rebase, conflict resolver, production, recovery, and lifecycle/store tests
  • focused Store provenance/lifecycle: 9/9 passed, 108 skipped
  • typecheck: passed
  • diff check: passed
  • independent full-diff review: clean for cb69e50d6f7d92f6e1d2e897c45db3f8152bb36c...bb33705503a7688110e2a47c059f7b4a480eb2c3

Current-head CI passed on both push and pull-request runs. Real-Docker validation passed 2/2 on the exact head, covering the writable fix profile and sealed read-only schema input. Copilot round 2 returned zero open findings.

Review record

Self-review

The initial full-diff pass found and fixed:

  • moved the full plan item out of the prompt argument and into captured read-only input
  • preserved the single schema.json input contract by using an envelope instead of a second mounted file
  • guarded synchronous owned-item serialization with the operation deadline before claiming resources
  • retained compatibility aliases to avoid unnecessary overlap with the active F5 assembly work

After Copilot's first round, another full-diff pass added durable-state assertions to both controlled races and found no further issues.

Independent review

Round 1, base/head cb69e50d6f7d92f6e1d2e897c45db3f8152bb36c...1530e5f55f2569dec6455bb073f30f884a524f3c: no findings in this diff.

Round 2, base/head cb69e50d6f7d92f6e1d2e897c45db3f8152bb36c...bb33705503a7688110e2a47c059f7b4a480eb2c3: clean after the context-drift fix. The reviewer reran 92 changed rebase tests, 9 focused Store tests, typecheck, and diff check.

The reviewer found an integration requirement in the separate active F5 worktree: its orchestration must not call abort() when RebaseResourcesUnsettled retains durable ownership. That task needs a regression proving the workspace remains available for recovery.

Copilot

Round 1 found one valid high-severity race: the owned plan item could come from revision N while launch/result context came from revision N+1. Fixed in bb33705 by capturing one exact context, reading its immutable plan revision, checking before launch, and checking after final export before copy-back. The thread was replied to and resolved after targeted failing-before/passing-after regressions.

Round 2, head bb33705503a7688110e2a47c059f7b4a480eb2c3: zero open findings; the prior high-severity finding is recorded as resolved.

Review lessons

  • The context-drift finding is covered by the existing async rule that an in-flight action owns every version counter and must recheck all of them before applying a response.
  • Plan-item transport and bounded lifecycle work are covered by the guarded-external-action and async lifecycle rules.
  • Resource retention after an unsettled resolver is covered by the owned-resource cleanup rules; the required F5 integration regression is recorded above.
  • Compatibility with the parallel F5 assembly is a one-off coordination decision, not a new reusable rule.
  • No new AGENTS.md/CLAUDE.md rule is required.

Copilot AI 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.

🟡 Changes recommended

Plan-item capture is not guarded against concurrent plan revisions, allowing stale instructions to be applied to the rebase workspace.

1 open finding
What changed in this PR

Extends sandboxed rebase conflict resolution to owned commits while preserving attribution and provenance.

Changes:

  • Passes trusted ownership and exact plan-item data to the sandbox.
  • Preserves owned commit authors, trailers, and ledger attribution.
  • Adds owned-conflict and deadline regression coverage.
File Description
runner/​rebase.ts Routes owned and foreign conflicts through the sandbox resolver.
runner/​rebase-conflict.ts Supplies owned plan-item context to conflict-resolution agents.
test/​runner-rebase.test.ts Tests owned conflict routing and provenance.
test/​runner-rebase-conflict.test.ts Tests plan-item input and deadline behavior.
docs/​implementation/​pre-merge-rebase.md Documents the expanded conflict boundary.

🧠 Review effort: Balanced


💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread runner/rebase-conflict.ts Outdated

Copilot AI 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.

🔵 Needs a closer look

The sandbox and lifecycle changes are security-sensitive, while current-head CI and real-Docker validation remain pending.

0 open findings

1 resolved since last review

🧠 Review effort: Balanced

@mchwang
mchwang merged commit 396758f into main Oct 7, 2026
3 checks passed
@mchwang
mchwang deleted the codex/issue22-owned-conflicts branch October 7, 2026 20:27
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.

2 participants