diff --git a/.github/workflows/ci-maintenance.yml b/.github/workflows/ci-maintenance.yml index 8f55fe5cef..a5ad52bb75 100644 --- a/.github/workflows/ci-maintenance.yml +++ b/.github/workflows/ci-maintenance.yml @@ -3,7 +3,7 @@ name: CI Maintenance # Trusted base-branch code only; never checks out or executes PR content. on: pull_request_target: - types: [closed, synchronize] + types: [closed, synchronize, opened, reopened, edited] permissions: contents: read @@ -13,6 +13,51 @@ concurrency: cancel-in-progress: true jobs: + refresh-pr-diff: + if: github.event_name == 'pull_request_target' && github.event.action != 'closed' + runs-on: ubuntu-latest + timeout-minutes: 2 + permissions: + contents: read + pull-requests: write + steps: + - uses: actions/github-script@v8 + with: + script: | + const repo = context.repo; + const pull_number = context.payload.number; + const expected = context.payload.pull_request; + const readPR = async () => (await github.rest.pulls.get({...repo, pull_number})).data; + const pr = await readPR(); + const sameIdentity = current => current.state === 'open' && + current.head.sha === expected.head.sha && current.head.ref === expected.head.ref && + current.head.repo?.id === expected.head.repo?.id && + current.base.ref === expected.base.ref && current.base.repo?.id === expected.base.repo?.id; + if (!sameIdentity(pr) || !pr.head.repo?.id || !pr.base.repo?.id) { + return core.info('PR identity changed; stop diff refresh.'); + } + const readTarget = async () => (await github.rest.repos.getBranch( + {...repo, branch: pr.base.ref})).data.commit.sha; + const target = await readTarget(); + if (pr.base.sha === target) return core.info('PR base snapshot is current.'); + // GitHub can retain the original base snapshot after main is merged + // into the source branch, counting those unrelated changes again. + // Re-select the same base; never rewrite the source or retarget it. + const before = await readPR(); + if (!sameIdentity(before) || before.base.sha !== pr.base.sha) { + return core.info('PR changed before update; stop diff refresh.'); + } + await github.rest.pulls.update({...repo, pull_number, base: pr.base.ref}); + const after = await readPR(); + if (!sameIdentity(after)) return core.info('PR changed during refresh; inspect the newer revision.'); + // A concurrent target push may have been included by the update. + if (after.base.sha !== target && after.base.sha !== await readTarget()) { + throw new Error('PR base snapshot did not refresh; inspect its diff.'); + } + core.info(`Refreshed #${pull_number} at unchanged head ${after.head.sha}: ` + + `${pr.changed_files} -> ${after.changed_files} files, ` + + `+${pr.additions}/-${pr.deletions} -> +${after.additions}/-${after.deletions}.`); + cancel-obsolete-pr: if: github.event_name == 'pull_request_target' && (github.event.action == 'closed' || github.event.action == 'synchronize') runs-on: ubuntu-latest diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index c1c14b31ac..4583251af2 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -190,7 +190,7 @@ touch, mention it in the PR instead of fixing it there. | --- | --- | --- | --- | | `python-tests.yml` | every PR | yes (`merge-gate`) | lint, mypy, sharded pytest, TypeScript core and coverage, minimum Node.js, Windows PowerShell, dashboard presentation | | `dco.yml` | every PR | yes (`Sign-off`) | `Signed-off-by` on contribution commits; verified GitHub-generated two-parent merges are exempt | -| `ci-maintenance.yml` | closed or updated PR (`pull_request_target`, trusted base code only) | no | cancels obsolete Python qualification runs after rechecking the PR and source branch | +| `ci-maintenance.yml` | opened, reopened, edited, updated or closed PR (`pull_request_target`, trusted base code only) | no | refreshes stale PR diff bases without changing the head; cancels obsolete Python qualification runs after rechecking the PR and source branch | | `dependency-review.yml` | every PR | no | dependency changes introduced by the PR | | `postgresql-integration.yml` | control-plane or npm lockfile paths | no | PostgreSQL authority store and service on a temporary instance | | `package-smoke.yml` | extension package paths | no | extension packages install, entrypoints, and example schemas | @@ -380,6 +380,37 @@ Maintainers may request consolidation when a useful outcome was unnecessarily split, or a smaller PR when unrelated concerns were mixed. Review evaluates the verified goal delta and evidence, not minimum size, model identity or PR count. +### Verify the published diff + +After opening or updating a PR, fetch its actual target branch and source head, +then compare GitHub's changed-file inventory and line totals with +`git diff --stat TARGET...HEAD` and `git diff --name-status TARGET...HEAD` at those +exact revisions. Use the actual PR target, including a non-main target; a +two-dot comparison with an advancing target also includes target-only changes. + +GitHub's [PR and compare views can use different merge bases](https://docs.github.com/en/pull-requests/reference/pull-requests#differences-between-commits-on-compare-and-pull-request-pages). +The PR can retain an older base snapshot after the source branch incorporates +target commits, inflating both files and line totals. CI Maintenance refreshes +that snapshot on PR lifecycle events, using trusted inline code without checking +out the contributor's branch. It re-selects the same base, verifies the source +identity before writing and reads back the unchanged head and refreshed base. +It does not repair unrelated code that is genuinely present in the Git diff. + +If published totals still disagree, inspect the PR's exact repository, head and +base, and compare the pinned base with the current target. A maintainer can +refresh the same base with the native API: + +```bash +gh api --method PATCH repos/OWNER/REPO/pulls/NUMBER -f base=VERIFIED_BASE_BRANCH +``` + +Read back the unchanged head, unchanged target branch and corrected file/line +inventory before declaring the repair complete. Recheck moved revisions rather +than reusing stale evidence. A refresh needs no empty commit, history rewrite, +temporary retarget or close/reopen cycle. Ordinary target advancement alone is +not evidence of an incorrect diff. If the Git diff itself contains unrelated +work, isolate the intended changes on the verified target before publishing. + ### Validation disclosure Use the [PR template](.github/PULL_REQUEST_TEMPLATE.md) to report facts, not a diff --git a/skills/loopx-self-repair/references/repair-patterns.md b/skills/loopx-self-repair/references/repair-patterns.md index 02a65b952e..06e6948739 100644 --- a/skills/loopx-self-repair/references/repair-patterns.md +++ b/skills/loopx-self-repair/references/repair-patterns.md @@ -247,6 +247,7 @@ teaches a reusable control-plane lesson. | `todo_succession_gap` | A non-trivial feature todo is marked done after PR/smoke success, but rollout, product-path audit, docs, telemetry, benchmark proof, or operator-decision follow-up is only mentioned in prose or not recorded at all. | completed todo note/evidence, recent PR/run history, active `Agent Todo`, active `Next Action`, related design docs. | Agent treated an implementation slice as feature completion, or LoopX lacked a reminder to create the successor todo. | Keep the lifecycle simple: do not add many feature states. Complete the old todo and create the next concrete agent/user todo with `--next-agent-todo`/`todo add`, or record a compact no-follow-up rationale. | | `todo_parent_successor_frontier_gap` | An open parent remains quota-runnable after its work was split into explicit successors, while every unfinished successor is deferred or owned by another peer and the parent has no independent immediate action. | parent status and `successor_todo_ids`, successor ownership/resume conditions, `todo update` response `parent_successor_advisory`, current quota candidate. | Successor links were mistaken for suspension or aggregate lifecycle state even though they intentionally encode lineage only. | Do not infer aggregation from successor links. At the authoring boundary, require the agent to decide whether the parent retains an independent immediate action; if not, explicitly defer it with a supported `resume_when` or complete it. Keep the reminder advisory and machine-readable rather than adding persisted aggregate state. | | `integration_remote_head_observation_gap` | `integration-branch status/sync --refresh-remotes` reports `in_sync`, but a normal push is rejected because another agent advanced the published integration branch. | integration plan, configured branch upstream, local and remote integration SHAs, merge-base relation, normal push result. | Remote refresh observed only base/source inputs, while tree-equivalent local candidates were treated as sufficient even when they omitted the published integration ancestry. | Narrow-fetch the configured integration upstream with base/source refs, surface remote-ahead or diverged ancestry as typed drift, and require automatic or supplied candidates to contain that exact remote head before recording an in-sync receipt. Never force-push or prune unrelated refs. | +| `github_pr_stale_diff_base` | GitHub PR files, line totals or commit count include target-branch changes absent from the current three-dot Git diff, often after merging the target into the source branch. | Exact source repository/head, actual target branch/tip, PR pinned base, both merge bases, complete changed-file inventories and line totals before/after. | GitHub retained an older PR base snapshot; checking only the source head or comparing two tips missed the stale comparison. | Prove the discrepancy against the current target merge base, then re-select the same verified base through the native PR API under existing write authority. Read back unchanged head/target and corrected inventory; do not force-push, add an empty commit or temporarily retarget. Check all requested open PRs, preserve genuine source changes, and cover lifecycle refresh in trusted CI Maintenance plus published-diff readback in CONTRIBUTING.md. | | `plan_todo_writeback_gap` | The agent explains a route change, top-20 todo list, P0/P1 plan, or deprecation policy to the user, but the active LoopX todos / Next Action / refresh-state payload do not contain the same commitments. | latest user-facing plan, quota `todo_write_hint`, active `Agent Todo`, `User Todo`, `Next Action`, recent refresh-state runs. | Agent used chat as memory after understanding the plan; LoopX prompt/skill did not make plan publication a writeback trigger. | Treat user-facing LoopX plans as writeback triggers: add/update concrete todos or refresh state before final, or explicitly mark the plan speculative/no-writeback with rationale. Add skill/prompt wording when this recurs. | | `gated_unmerged_change_continuity_gap` | Work is prepared, staged, or committed locally while a repository change window blocks commit or push, but the durable state records only a future reminder; work that never attempts a Git operation is especially easy to miss. After restart or handoff, no global surface can identify the producing repository, checkout/head, worktree, changed-path inventory, validation, or terminal outcome. | change-window decision, pending-change global projection, private locator health and path count, repository checkout/head and worktree fingerprint, linked Todo, validation and PR refs, plus an explicit linked-worktree reconciliation preview when broad recovery is requested. | Schedule enforcement and work continuity were modeled as separate prose conventions, and hook denial was the only enrollment seam, so correctly paused work could silently orphan the code needed after the gate reopened. | On a denied repository hook, append or idempotently refresh a public-safe pending-change event. At bounded writeback or handoff during a blocked window, reconcile the current dirty checkout even when no Git operation was attempted; keep a broad linked-worktree sweep explicit and treat unattributed historical dirt as candidates rather than proof of current-gate causality. Store repository identity, branch or detached checkout/head, content-free fingerprint, next eligible time, and counts in the shared projection; retain the worktree and repository-relative changed paths only in a mode-0600 private locator. Allow explicit goal/Todo/write-scope/validation enrichment, expose cross-project list/verify, and require typed merged/superseded/abandoned resolution. Never copy code, diff bodies, credentials, file names, or personal paths into the shared projection. | | `capability_route_hidden_in_prompt` | A caller explicitly selects a registered capability, but `start-goal` exposes the route only in planning prose; or the first turn qualifies correctly while a later heartbeat loses the route and writes a plausible generic implementation Todo. | exact start-goal route switch, full and compact packets, guided `ordered_steps`, capability admission state, projected successor Todo, later quota `selected_todo`. | Capability selection remained optional prose, or a bootstrap-only route was mistaken for durable later-turn authority. | Project the explicit switch as a typed bootstrap route and pre-implementation guard, persist admission in capability-owned state, then write its exact generic successor with `action_kind` plus a stable public `target_key`. Preserve those scheduling fields in later quota selection; do not copy domain facts into Todo or infer admission from stale prompt context. Cover switch-off/on, compact/full parity, exact successor validation, and later-turn quota re-entry. | diff --git a/tests/test_ci_resource_policy.py b/tests/test_ci_resource_policy.py index b3e7329da1..e667c46b8e 100644 --- a/tests/test_ci_resource_policy.py +++ b/tests/test_ci_resource_policy.py @@ -129,6 +129,110 @@ def test_cleanup_concurrency_cannot_replace_current_qualification() -> None: assert qualification != cleanup +@pytest.mark.parametrize("action", ["opened", "reopened", "edited", "synchronize", "closed"]) +def test_diff_refresh_uses_trusted_code_and_its_own_minimal_write_scope(action: str) -> None: + policy = workflow("ci-maintenance") + job = policy["jobs"]["refresh-pr-diff"] + context = event("pull_request_target") + context["github"].event.action = action + assert action in policy[True]["pull_request_target"]["types"] + assert expression(job["if"], context) is (action != "closed") + assert policy["permissions"] == {"contents": "read"} + assert job["permissions"] == {"contents": "read", "pull-requests": "write"} + assert job["timeout-minutes"] <= 2 + assert all(step["uses"].startswith("actions/github-script@") for step in job["steps"]) + + +def _run_diff_refresh(mode: str, branch: str = "main") -> dict: + script = workflow("ci-maintenance")["jobs"]["refresh-pr-diff"]["steps"][0]["with"]["script"] + harness = r''' +let input = ''; for await (const chunk of process.stdin) input += chunk; +const {script, mode, branch} = JSON.parse(input); +const oldBase = 'a'.repeat(40), head = 'b'.repeat(40), target = 'c'.repeat(40), newer = 'd'.repeat(40); +const pr = {state:'open', head:{sha:head,ref:'work',repo:{id:2}}, + base:{sha:oldBase,ref:branch,repo:{id:1}}, changed_files:9,additions:30,deletions:12}; +const expected = structuredClone(pr); +if (mode === 'closed') pr.state = 'closed'; +if (mode === 'fresh') pr.base.sha = target; +if (mode === 'old_event_head') expected.head.sha = newer; +if (mode === 'old_event_source') expected.head.repo.id = 3; +if (mode === 'old_event_base') expected.base.ref = 'other'; +if (mode === 'missing_source') pr.head.repo = null; +if (mode === 'same_repo') {pr.head.repo.id = 1; expected.head.repo.id = 1;} +let reads = 0, branchReads = 0; +const updates = [], notes = []; +const context = {repo:{owner:'owner',repo:'fixture'}, payload:{number:17,pull_request:expected}}; +const github = {rest:{pulls:{get:async()=>{ + reads++; + if (reads === 2) { + if (mode === 'head_before_write') pr.head.sha = newer; + if (mode === 'source_before_write') pr.head.repo.id = 3; + if (mode === 'base_before_write') pr.base.ref = 'other'; + if (mode === 'refreshed_before_write') pr.base.sha = target; + } + return {data:structuredClone(pr)}; +}, update:async params=>{ + updates.push(params); + if (mode === 'failed_api') throw new Error('provider rejected update'); + if (mode !== 'not_applied') pr.base.sha = mode === 'target_moves' ? newer : target; + if (mode === 'head_during_write') pr.head.sha = newer; + if (mode === 'base_during_write') pr.base.ref = 'other'; + pr.changed_files = 2; pr.additions = 3; pr.deletions = 1; +}}, repos:{getBranch:async params=>{ + if (params.branch !== branch) throw new Error('unverified target'); + branchReads++; + return {data:{commit:{sha:mode === 'target_moves' && branchReads > 1 ? newer : target}}}; +}}}}; +let error = null; +try {await new (Object.getPrototypeOf(async function(){}).constructor)('github','context','core',script) + (github,context,{info:message=>notes.push(message)});} catch(e) {error=e.message;} +process.stdout.write(JSON.stringify({updates,notes,error,pr})); +''' + result = subprocess.run( + ["node", "--input-type=module", "-e", harness], + input=json.dumps({"script": script, "mode": mode, "branch": branch}), + text=True, capture_output=True, check=True, timeout=10, + ) + return json.loads(result.stdout) + + +@pytest.mark.parametrize("branch", ["main", "release/stable"]) +@pytest.mark.parametrize("mode", ["stale", "same_repo", "target_moves"]) +def test_stale_diff_refresh_preserves_the_exact_source_and_actual_target(mode: str, branch: str) -> None: + observed = _run_diff_refresh(mode, branch) + assert observed["error"] is None + assert observed["updates"] == [{"owner": "owner", "repo": "fixture", "pull_number": 17, "base": branch}] + assert observed["pr"]["head"] == { + "sha": "b" * 40, "ref": "work", "repo": {"id": 1 if mode == "same_repo" else 2}, + } + assert observed["pr"]["base"]["ref"] == branch + assert any("9 -> 2 files" in note and "+30/-12 -> +3/-1" in note for note in observed["notes"]) + + +@pytest.mark.parametrize("mode", [ + "fresh", "closed", "old_event_head", "old_event_source", "old_event_base", "missing_source", + "head_before_write", "source_before_write", "base_before_write", "refreshed_before_write", +]) +def test_diff_refresh_does_not_write_after_identity_or_snapshot_drift(mode: str) -> None: + observed = _run_diff_refresh(mode) + assert observed["updates"] == [] + assert observed["error"] is None + + +@pytest.mark.parametrize("mode", ["head_during_write", "base_during_write", "not_applied", "failed_api"]) +def test_diff_refresh_requires_readback_and_never_retries_a_moved_revision(mode: str) -> None: + observed = _run_diff_refresh(mode) + assert len(observed["updates"]) == 1 + assert not any("Refreshed #" in note for note in observed["notes"]) + if mode == "not_applied": + assert "did not refresh" in observed["error"] + elif mode == "failed_api": + assert "provider rejected update" in observed["error"] + else: + assert observed["error"] is None + assert any("newer revision" in note for note in observed["notes"]) + + @pytest.mark.parametrize("mode", ["delayed", "never", "changed"]) def test_obsolete_run_cancellation_waits_for_verified_completion(mode: str) -> None: script = workflow("ci-maintenance")["jobs"]["cancel-obsolete-pr"]["steps"][0]["with"]["script"] diff --git a/tests/test_sonarcloud_workflow.py b/tests/test_sonarcloud_workflow.py index fd89d8dec8..3d4a1e23d2 100644 --- a/tests/test_sonarcloud_workflow.py +++ b/tests/test_sonarcloud_workflow.py @@ -126,7 +126,9 @@ def test_separate_trusted_cleanup_does_not_activate_analysis() -> None: caller = yaml.load(WORKFLOW.with_name("python-tests.yml").read_text(), Loader=yaml.BaseLoader) maintenance = yaml.load(WORKFLOW.with_name("ci-maintenance.yml").read_text(), Loader=yaml.BaseLoader) assert "pull_request_target" not in caller["on"] - assert maintenance["on"]["pull_request_target"]["types"] == ["closed", "synchronize"] + assert set(maintenance["on"]["pull_request_target"]["types"]) == { + "closed", "synchronize", "opened", "reopened", "edited", + } for job in maintenance["jobs"].values(): assert "uses" not in job and "secrets" not in job assert all(not step.get("uses", "").startswith("actions/checkout@") for step in job["steps"])