mirror of
https://github.com/QwenLM/qwen-code.git
synced 2026-08-21 22:55:16 +00:00
fix(review): make the incremental cache work for Aone AGit-Flow CRs (#9630)
* fix(review): make the incremental cache work for Aone AGit-Flow CRs * docs(review): qualify the Aone no-ancestry claims in comments and docs The D7 comments described the ancestry gate as unconditional and both ancestry tests as failing for every AGit-Flow update; the head test alone fails for every amend (the clamp fires only on amend-plus-rebase), and the narrowing join never lets a drift byte reach the published scope. Qualify the ledger.ts SHA_RE block, the resolveIncrementalAnchor docstring, the clamp-skip and call-site comments, the test-block comments, and the design/user docs accordingly. --------- Co-authored-by: qwen-code-dev-bot <qwen-code-dev@service.alibaba.com>
This commit is contained in:
Shaojin Wen
GitHub
parent
0dd518f950
commit
04886c4354
5 changed files with 307 additions and 39 deletions
|
|
@ -387,6 +387,31 @@ Enterprise paragraph.
|
|||
- **Phase 4 — semantic gaps.** Incremental-cache ancestry fallback, build-test
|
||||
repo-config escape hatch, publish-assets gating polish, generic-GitLab
|
||||
(glab) evaluation.
|
||||
- **Landed (2026-08-21): the incremental-cache ancestry fallback (D7,
|
||||
#9618).** `resolveIncrementalAnchor` gained a `noAncestry` mode that
|
||||
`fetch-pr` selects when the platform is Aone: an AGit-Flow update
|
||||
AMENDS the single CR commit in place, orphaning the cached head, so
|
||||
the anchor-behind-head test failed for EVERY update and an
|
||||
amend-and-re-review never scoped. Both ancestry tests — the
|
||||
anchor-behind-head test and the behind-merge-base clamp — are
|
||||
skipped (the clamp only ever fired when the update ALSO rebased onto
|
||||
newer master, moving the merge base past the cached head; a pure
|
||||
amend passed it); after the fetch both heads are local, so
|
||||
`anchor..head` IS the update's delta, and the narrowing step
|
||||
assembles the published scope from the CR's own diff exactly as it
|
||||
does for an ancestrally valid GitHub anchor (an amended-and-rebased
|
||||
update's delta carries the rebase drift, but the join reads it only
|
||||
for which files changed — no drift byte reaches the published scope
|
||||
— and drift touching a file outside the CR's diff falls back to the
|
||||
full range there).
|
||||
The existence checks and the `base-untrusted` refusal stay — they
|
||||
guard presence and the base-derived capture, not the lineage. The
|
||||
head-drift checks Aone has were confirmed to compare the live
|
||||
`sourceBranch` SHA against the reviewed SHA the same way D7 names —
|
||||
submit's pre-write gate and mid-batch re-read, and fetch-pr's resume
|
||||
probe; none consults a platform compare API or an ancestry test.
|
||||
GitHub keeps the tests: there an ancestor-less anchor is a
|
||||
force-push, and the tests are the detection.
|
||||
|
||||
## Testing strategy
|
||||
|
||||
|
|
|
|||
|
|
@ -363,7 +363,7 @@ If you switch models (via `/model`) and re-review the same PR, `/review` detects
|
|||
|
||||
The model match also gates incremental scoping, not just the skip: "clean up to the cached commit" is the previous model's verdict, so when new commits have landed since the cached review, a model mismatch never scopes to `lastCommitSha..HEAD` — the range is the full diff, noting "Previous round was reviewed by qwen3-coder. Running full review with gpt-4o." — unless an anchor certified by the model now running is recovered from the last posted review (below), which scopes the range instead. The previous round's findings still carry over to be re-ruled; only the anchor does not. The same gate binds the anchor recovered from the last posted review's machine-ledger marker when the cache is absent or its anchor is unusable (CI, another clone): it scopes the incremental range only if the model now running certified it — a marker certified by a different model, or carrying no model (a review posted with `review.attribution` off, or one from before the field), falls back to the full diff.
|
||||
|
||||
Cache is stored in `.qwen/review-cache/` and tracks both the commit SHA and model ID. Make sure this directory is in your `.gitignore` (a broader rule like `.qwen/*` also works). If the cached commit was rebased away, it falls back to a full review. Only high-effort reviews consult or write the cache — a `--effort low|medium` quick pass never counts as "already reviewed".
|
||||
Cache is stored in `.qwen/review-cache/` and tracks both the commit SHA and model ID. Make sure this directory is in your `.gitignore` (a broader rule like `.qwen/*` also works). On GitHub, if the cached commit was rebased or force-pushed away, it falls back to a full review; Aone rules the cached anchor differently — see its paragraph below. Only high-effort reviews consult or write the cache — a `--effort low|medium` quick pass never counts as "already reviewed".
|
||||
|
||||
## Review Reports
|
||||
|
||||
|
|
@ -382,7 +382,7 @@ The deterministic halves of the pipeline — argument parsing (`qwen review pars
|
|||
|
||||
**GitHub Enterprise:** reviewing a PR URL on a non-`github.com` host routes every GitHub call at that host — the review subcommands (`match-remote`, `meta`, `fetch-pr`, `pr-context`, `comment-status`, `issue-context`, `fetch-diff`, `comment-body`, `plan-diff`, `test-plan`, `presubmit`, `compose-review`, `submit`, `publish-assets`) accept `--host` and set it in code, so a forgotten host cannot silently retarget the review at `github.com`.
|
||||
|
||||
**Aone Code:** for a clone whose origin is on `gitlab.alibaba-inc.com`, run `/review` from inside that clone — the platform is detected from the remote and the subcommands work, backed by the `a1` CLI — the target number is the global MR id. `fetch-pr` fetches `refs/merge-requests/<id>/head` and builds the worktree + diff, so the agent review of the worktree is unchanged. Every Aone run is context-unavailable and several flows are skipped (rather than hitting github.com's same-named repo): `pr-context`/`comment-status`/`presubmit` have no Aone backing (verdict caps at `COMMENT`), `test-plan` is unbacked, Agent 0 is skipped, and the `publish-assets` write is skipped. `--comment` **posts** the review through the `a1` CLI: one comment per inline finding, then the summary comment. Aone has no native request-changes state — on that verdict the summary comment carries a blocking header, and any inline Criticals that were actually posted block the merge through the discussion gate while their discussions stay unresolved (when no inline Critical posted, the header is advisory and nothing mechanically blocks the merge). The posted comments carry no AI-comment flag — `a1` cannot set one — so a repo's dedicated `ai_comment` merge gate does not track them. The native `a1 repo mr approve` is wired for an Approve verdict but does not fire this phase: the context-unavailable cap keeps every Aone verdict at Comment. Two caveats for repeat rounds: there is no dedup backing yet, so a second `--comment` round re-posts every still-valid finding as a new comment, and self-PR detection has no Aone backing. See `docs/design/2026-08-15-review-aone-provider.md`.
|
||||
**Aone Code:** for a clone whose origin is on `gitlab.alibaba-inc.com`, run `/review` from inside that clone — the platform is detected from the remote and the subcommands work, backed by the `a1` CLI — the target number is the global MR id. `fetch-pr` fetches `refs/merge-requests/<id>/head` and builds the worktree + diff, so the agent review of the worktree is unchanged. Every Aone run is context-unavailable and several flows are skipped (rather than hitting github.com's same-named repo): `pr-context`/`comment-status`/`presubmit` have no Aone backing (verdict caps at `COMMENT`), `test-plan` is unbacked, Agent 0 is skipped, and the `publish-assets` write is skipped. `--comment` **posts** the review through the `a1` CLI: one comment per inline finding, then the summary comment. Aone has no native request-changes state — on that verdict the summary comment carries a blocking header, and any inline Criticals that were actually posted block the merge through the discussion gate while their discussions stay unresolved (when no inline Critical posted, the header is advisory and nothing mechanically blocks the merge). The posted comments carry no AI-comment flag — `a1` cannot set one — so a repo's dedicated `ai_comment` merge gate does not track them. The native `a1 repo mr approve` is wired for an Approve verdict but does not fire this phase: the context-unavailable cap keeps every Aone verdict at Comment. Incremental re-review follows the AGit-Flow update model: an update AMENDS the single CR commit in place, orphaning the head the previous round reviewed — so the cached anchor is ruled WITHOUT ancestry (the anchor-behind-head test would fail for every update), and the re-review scopes the PR's own diff to the files the update touched instead of falling back to a full review; an update that also rebased onto newer master keeps that scope only while the rebase's drift stays within the CR's files — drift touching any other file falls back to the full review, and no drift byte enters the published scope either way. Two caveats for repeat rounds: there is no dedup backing yet, so a second `--comment` round re-posts every still-valid finding as a new comment, and self-PR detection has no Aone backing. See `docs/design/2026-08-15-review-aone-provider.md`.
|
||||
|
||||
Every run ends with one machine-readable line (`Review complete: <target> — <disposition>`), so scripts and CI wrappers can detect completion and outcome with a single `^Review complete: ` match.
|
||||
|
||||
|
|
|
|||
|
|
@ -1261,6 +1261,24 @@ describe('fetch-pr report assembly', () => {
|
|||
);
|
||||
}
|
||||
|
||||
/**
|
||||
* The ORPHANED-anchor shape, the complement of `anchorIsValid`: the
|
||||
* anchor EXISTS and resolves to a commit, but every ancestry question
|
||||
* answers null (exit 1) — a rebased-away GitHub anchor, or an Aone head
|
||||
* orphaned by the AGit-Flow amend. One shape, so the tests that refuse
|
||||
* it on GitHub and the tests that scope it on Aone cannot drift apart.
|
||||
*/
|
||||
function serveOrphanShape(): void {
|
||||
producerMocks.gitOpt.mockImplementation((...args: string[]) =>
|
||||
args[0] === 'cat-file' ? '' : args[0] === 'rev-parse' ? ANCHOR : null,
|
||||
);
|
||||
producerMocks.resolveMergeBase.mockReturnValue({
|
||||
sha: BASE,
|
||||
baseFetchFailed: false,
|
||||
});
|
||||
servesBothRanges();
|
||||
}
|
||||
|
||||
it('pulls a still-clean importer of a changed file back into the scope', async () => {
|
||||
// The narrowing is sound in one direction only. `b.ts` has not changed
|
||||
// since the anchor, so the delta capture cannot show it and the narrowed
|
||||
|
|
@ -2181,15 +2199,7 @@ describe('fetch-pr report assembly', () => {
|
|||
// The `effective` clause in the partition guard: without it a round
|
||||
// whose anchor was refused for a deterministic reason gets relabelled
|
||||
// `partition-failed`, which invites re-running a dead anchor.
|
||||
producerMocks.gitOpt.mockImplementation(
|
||||
(...args: string[]) =>
|
||||
args[0] === 'cat-file' ? '' : args[0] === 'rev-parse' ? ANCHOR : null, // not an ancestor
|
||||
);
|
||||
producerMocks.resolveMergeBase.mockReturnValue({
|
||||
sha: BASE,
|
||||
baseFetchFailed: false,
|
||||
});
|
||||
servesBothRanges();
|
||||
serveOrphanShape(); // not an ancestor
|
||||
producerMocks.buildDiffPlan.mockImplementation((text: unknown) => {
|
||||
if (typeof text === 'string' && text.trim() !== '') {
|
||||
throw new Error('chunks do not tile the diff');
|
||||
|
|
@ -2274,14 +2284,8 @@ describe('fetch-pr report assembly', () => {
|
|||
});
|
||||
|
||||
it('refuses a rebased-away anchor end to end, on a full-range plan', async () => {
|
||||
producerMocks.gitOpt.mockImplementation(
|
||||
(...args: string[]) =>
|
||||
args[0] === 'cat-file' ? '' : args[0] === 'rev-parse' ? ANCHOR : null, // every merge-base probe fails → not an ancestor
|
||||
);
|
||||
producerMocks.resolveMergeBase.mockReturnValue({
|
||||
sha: BASE,
|
||||
baseFetchFailed: false,
|
||||
});
|
||||
// Every merge-base probe fails → not an ancestor.
|
||||
serveOrphanShape();
|
||||
producerMocks.gitRaw.mockImplementation((...args: string[]) =>
|
||||
args.includes(`${BASE}..f00df00df00d`)
|
||||
? Buffer.from(DELTA_DIFF)
|
||||
|
|
@ -2454,15 +2458,8 @@ describe('fetch-pr report assembly', () => {
|
|||
// reason announced "no diff could be captured" moments after the capture
|
||||
// succeeded and the partitioner warned, sending whoever diagnoses the
|
||||
// round at git and the network instead of at the partitioner.
|
||||
producerMocks.gitOpt.mockImplementation((...args: string[]) =>
|
||||
// `merge-base` answers null → exit 1 → the predicate's NO.
|
||||
args[0] === 'cat-file' ? '' : args[0] === 'rev-parse' ? ANCHOR : null,
|
||||
);
|
||||
producerMocks.resolveMergeBase.mockReturnValue({
|
||||
sha: BASE,
|
||||
baseFetchFailed: false,
|
||||
});
|
||||
servesBothRanges();
|
||||
// `merge-base` answers null → exit 1 → the predicate's NO.
|
||||
serveOrphanShape();
|
||||
producerMocks.buildDiffPlan.mockImplementation((text: unknown) => {
|
||||
if (typeof text === 'string' && text.trim() !== '') {
|
||||
throw new Error('chunks do not tile the diff');
|
||||
|
|
@ -2697,6 +2694,91 @@ describe('fetch-pr report assembly', () => {
|
|||
expect(report.effort).toBeUndefined();
|
||||
});
|
||||
});
|
||||
|
||||
describe('the Aone incremental rule (AGit-Flow, D7, #9618)', () => {
|
||||
// An AGit-Flow update AMENDS the single CR commit in place: the new
|
||||
// head has the cached head's parent, never the cached head itself, so
|
||||
// the head test answers "no" for the cached anchor on EVERY update —
|
||||
// and the clamp too once the update also rebased onto newer master.
|
||||
// The Aone rule must not ask either — after the fetch both heads are
|
||||
// local, and `anchor..head` is the update's delta. Driven through the
|
||||
// real handler with the Aone reader: an explicit Aone `--host` selects
|
||||
// it, a mocked `a1` serves auth + MR view, and the git probes answer
|
||||
// the orphan shape (existence yes, ancestry exit 1).
|
||||
|
||||
function serveAone(): void {
|
||||
producerMocks.execFileSync.mockImplementation(
|
||||
(cmd: string, args: string[]) => {
|
||||
if (cmd !== 'a1') return '';
|
||||
if (args[0] === 'repo' && args[1] === 'mr' && args[2] === 'view') {
|
||||
return JSON.stringify({
|
||||
mergeRequest: {
|
||||
sourceBranch: 'f00df00df00d',
|
||||
targetBranch: 'main',
|
||||
detailUrl:
|
||||
'https://code.alibaba-inc.com/acme/widgets/codereview/42',
|
||||
description: '',
|
||||
},
|
||||
});
|
||||
}
|
||||
return ''; // `auth whoami`
|
||||
},
|
||||
);
|
||||
serveOrphanShape();
|
||||
}
|
||||
|
||||
it('scopes an amend-orphaned anchor instead of refusing it', async () => {
|
||||
serveAone();
|
||||
const report = await reportFor({
|
||||
since: ANCHOR,
|
||||
host: 'gitlab.alibaba-inc.com',
|
||||
});
|
||||
expect(report.incremental).toEqual({
|
||||
since: ANCHOR,
|
||||
effective: true,
|
||||
scope: SCOPE_A,
|
||||
diffBase: BASE,
|
||||
});
|
||||
expect(report.diffPath).not.toBeNull();
|
||||
// The published scope is the PR's own diff narrowed to the amend's
|
||||
// delta — the untouched file is dropped, exactly as the GitHub
|
||||
// incremental path narrows.
|
||||
expect(writtenDiff()).toBe(NARROWED);
|
||||
// ...and the diff came from the Aone ref namespace, not GitHub's.
|
||||
expect(producerMocks.git.mock.calls).toContainEqual([
|
||||
'fetch',
|
||||
'origin',
|
||||
'refs/merge-requests/42/head:qwen-review/pr-42',
|
||||
]);
|
||||
});
|
||||
|
||||
it('never asks an ancestry question on the Aone platform', async () => {
|
||||
serveAone();
|
||||
await reportFor({ since: ANCHOR, host: 'gitlab.alibaba-inc.com' });
|
||||
const ancestryCalls = producerMocks.gitOpt.mock.calls.filter(
|
||||
(args) => args[0] === 'merge-base' && args[1] === '--is-ancestor',
|
||||
);
|
||||
// Neither the head test nor the clamp — a mutant re-asking either
|
||||
// would refuse this anchor (every answer is "no") but could survive
|
||||
// an outcome-only assertion, so pin the silence itself.
|
||||
expect(ancestryCalls).toEqual([]);
|
||||
});
|
||||
|
||||
it('keeps the ancestry tests on GitHub — an orphaned anchor there is a force-push', async () => {
|
||||
// Same orphan shape, GitHub platform (no Aone host): the refusal
|
||||
// stands, because on a push-based platform an anchor the head does
|
||||
// not descend from is rewritten history, not an amend.
|
||||
serveOrphanShape();
|
||||
const report = await reportFor({ since: ANCHOR });
|
||||
expect(report.incremental).toEqual({
|
||||
since: ANCHOR,
|
||||
effective: false,
|
||||
reason: 'not-an-ancestor',
|
||||
});
|
||||
// The full range is the fallback.
|
||||
expect(writtenDiff()).toBe(FULL_DIFF);
|
||||
});
|
||||
});
|
||||
});
|
||||
|
||||
describe('resolveIncrementalAnchor', () => {
|
||||
|
|
@ -3031,6 +3113,112 @@ describe('resolveIncrementalAnchor', () => {
|
|||
});
|
||||
expect(r.diffBase).toBeNull();
|
||||
});
|
||||
|
||||
// ---- The AGit-Flow rule (design D7, #9618) ----------------------------
|
||||
// Under AGit-Flow an update AMENDS the single CR commit in place, so the
|
||||
// amended head has the cached head's parent, never the cached head itself
|
||||
// — the anchor-behind-head test refuses EVERY update's anchor (the clamp
|
||||
// fires only when the update also rebased). The Aone rule rules without
|
||||
// ancestry: after the fetch both heads are local, and their diff is the
|
||||
// update's delta.
|
||||
|
||||
it('noAncestry scopes an orphaned anchor the ancestry test refuses', () => {
|
||||
// isAncestor answers "no" for everything — the exact amend shape. The
|
||||
// constant-false probe also kills the mutant that keeps asking: were
|
||||
// the test still consulted, its answer would refuse this anchor.
|
||||
const r = resolveIncrementalAnchor(
|
||||
ANCHOR,
|
||||
HEAD,
|
||||
probe({ isAncestor: () => false }),
|
||||
null,
|
||||
{ noAncestry: true },
|
||||
);
|
||||
expect(r.incremental).toEqual({ since: ANCHOR, effective: true });
|
||||
expect(r.diffBase).toBe(ANCHOR);
|
||||
});
|
||||
|
||||
it('noAncestry asks NEITHER ancestry question — not the head test, not the clamp', () => {
|
||||
// A constant-false isAncestor passes with a dropped guard on either
|
||||
// check, so pin the ruling to silence: the merge base is present and
|
||||
// "older" than nothing an amend can reach — the clamp's exact trigger
|
||||
// after a rebase onto newer master.
|
||||
const asked: Array<[string, string]> = [];
|
||||
const r = resolveIncrementalAnchor(
|
||||
ANCHOR,
|
||||
HEAD,
|
||||
probe({
|
||||
isAncestor: (a, b) => {
|
||||
asked.push([a, b]);
|
||||
return false;
|
||||
},
|
||||
}),
|
||||
{ sha: 'c'.repeat(40), fetchFailed: false },
|
||||
{ noAncestry: true },
|
||||
);
|
||||
expect(r.incremental).toEqual({ since: ANCHOR, effective: true });
|
||||
expect(r.diffBase).toBe(ANCHOR);
|
||||
expect(asked).toEqual([]);
|
||||
});
|
||||
|
||||
it('noAncestry keeps the existence refusals — a fresh clone cannot diff the orphan', () => {
|
||||
// The ancestry skip is about LINEAGE, not presence: an anchor the
|
||||
// object store does not hold (the round-1 fetch happened elsewhere)
|
||||
// has no delta to capture, and says so with the deterministic reason.
|
||||
const absent = resolveIncrementalAnchor(
|
||||
ANCHOR,
|
||||
HEAD,
|
||||
probe({ commitExists: () => false, isAncestor: () => false }),
|
||||
null,
|
||||
{ noAncestry: true },
|
||||
);
|
||||
expect(absent.incremental).toEqual({
|
||||
since: ANCHOR,
|
||||
effective: false,
|
||||
reason: 'unknown-commit',
|
||||
});
|
||||
expect(absent.diffBase).toBeNull();
|
||||
const unresolvable = resolveIncrementalAnchor(
|
||||
ANCHOR,
|
||||
HEAD,
|
||||
probe({ resolveCommit: () => null, isAncestor: () => false }),
|
||||
null,
|
||||
{ noAncestry: true },
|
||||
);
|
||||
expect(unresolvable.incremental.reason).toBe('unknown-commit');
|
||||
});
|
||||
|
||||
it('noAncestry rules upToDate when the anchor IS the head', () => {
|
||||
// A re-run with no amend in between: the identity comparison is not an
|
||||
// ancestry test, so it rules exactly as on GitHub.
|
||||
const r = resolveIncrementalAnchor(HEAD, HEAD, probe(), null, {
|
||||
noAncestry: true,
|
||||
});
|
||||
expect(r.incremental).toEqual({
|
||||
since: HEAD,
|
||||
effective: true,
|
||||
upToDate: true,
|
||||
});
|
||||
expect(r.diffBase).toBeNull();
|
||||
});
|
||||
|
||||
it('noAncestry still refuses a possibly-stale base — base-untrusted is not an ancestry test', () => {
|
||||
// The published scope is assembled from the base-derived full capture;
|
||||
// a base the run flagged possibly-stale is a capture no sibling guard
|
||||
// rules on, ancestry skip or not.
|
||||
const r = resolveIncrementalAnchor(
|
||||
ANCHOR,
|
||||
HEAD,
|
||||
probe({ isAncestor: () => false }),
|
||||
{ sha: 'c'.repeat(40), fetchFailed: true },
|
||||
{ noAncestry: true },
|
||||
);
|
||||
expect(r.incremental).toEqual({
|
||||
since: ANCHOR,
|
||||
effective: false,
|
||||
reason: 'base-untrusted',
|
||||
});
|
||||
expect(r.diffBase).toBeNull();
|
||||
});
|
||||
});
|
||||
|
||||
describe('isEmptyDiff', () => {
|
||||
|
|
|
|||
|
|
@ -279,6 +279,11 @@ type FetchPrResult = PlanReport & {
|
|||
* the same change differently (a path or a rename git resolves differently
|
||||
* across the two ranges), so narrowing would drop a change the PR's diff
|
||||
* displays. Every shape keeps the full range: wider, never wrong.
|
||||
* On Aone the two ancestry-based reasons (`not-an-ancestor`,
|
||||
* `behind-merge-base`) never occur: an AGit-Flow update amends the head
|
||||
* in place and orphans the cached sha, so the anchor is ruled WITHOUT
|
||||
* ancestry (design D7) and the two heads' diff is read as the update's
|
||||
* delta.
|
||||
*
|
||||
* Whether a PLAN exists is a separate fact, and it is `diffPath`: null
|
||||
* means this round has no diff to review, whatever refused the anchor. A
|
||||
|
|
@ -367,12 +372,33 @@ export interface AnchorProbe {
|
|||
* `isCollapsedFromUpstream`) declines to rule in that state rather than
|
||||
* ruling on it. `{fetchFailed: true, sha: null}` is not that state — there
|
||||
* is no clamp to rule at all, and the delta range needs no base.
|
||||
*
|
||||
* `noAncestry` is the AGit-Flow rule (Aone; design D7). Under AGit-Flow,
|
||||
* updating a CR AMENDS the single commit in place: the amended H2 has H1's
|
||||
* parent, never H1 itself, so the old head is orphaned and the
|
||||
* anchor-behind-head test fails for EVERY update — the amended head never
|
||||
* descends from the cached one. (The clamp additionally fails whenever the
|
||||
* update also rebased onto newer master, since the merge base then moves
|
||||
* past the cached head; a pure amend passes it.) Neither is asked: after
|
||||
* the fetch both heads are local, so `anchor..head` IS the update's delta
|
||||
* (for a pure amend, exactly the amended lines; if the author also rebased
|
||||
* onto newer master, the range additionally carries the rebase drift; the
|
||||
* narrowing join reads it only for which files changed and never lets a
|
||||
* drift byte reach the published scope, falling back to the full range via
|
||||
* `nothing-to-narrow` when the drift touched files outside the CR's diff).
|
||||
* The published scope is still assembled
|
||||
* from the PR's own diff by the narrowing step, so it cannot carry a hunk
|
||||
* the platform does not display, and the `base-untrusted` refusal stays —
|
||||
* it guards a capture against a stale base, not a lineage. The existence
|
||||
* checks also stay: an anchor the object store does not hold (a fresh
|
||||
* clone) cannot be diffed against.
|
||||
*/
|
||||
export function resolveIncrementalAnchor(
|
||||
rawSince: string,
|
||||
fetchedSha: string,
|
||||
probe: AnchorProbe,
|
||||
mergeBase: { sha: string | null; fetchFailed: boolean } | null = null,
|
||||
options: { noAncestry?: boolean } = {},
|
||||
): { incremental: IncrementalDecision; diffBase: string | null } {
|
||||
// git resolves hex case-insensitively, and an operator pasting an
|
||||
// uppercase sha (some UIs render them that way) was refused before any
|
||||
|
|
@ -410,8 +436,11 @@ export function resolveIncrementalAnchor(
|
|||
};
|
||||
}
|
||||
// Ancestry is asked about the RESOLVED commit, so a non-commit can no
|
||||
// longer reach it and an error here really is the git surface.
|
||||
if (!probe.isAncestor(resolved, fetchedSha)) {
|
||||
// longer reach it and an error here really is the git surface. Not asked
|
||||
// at all under `noAncestry` — see the docstring's AGit-Flow paragraph:
|
||||
// an amend orphans the cached head, so the test would fail for every
|
||||
// update, and the two heads' diff is the update's delta anyway.
|
||||
if (!options.noAncestry && !probe.isAncestor(resolved, fetchedSha)) {
|
||||
return {
|
||||
incremental: { since, effective: false, reason: 'not-an-ancestor' },
|
||||
diffBase: null,
|
||||
|
|
@ -421,13 +450,28 @@ export function resolveIncrementalAnchor(
|
|||
// clamp to rule, stale or otherwise, and the docstring's "a null `sha`
|
||||
// skips the clamp" holds — the delta range needs no base at all, so a
|
||||
// deleted or renamed base branch must not cost a valid anchor its scope.
|
||||
// NOT an ancestry test — it fires under `noAncestry` too: the narrowing
|
||||
// below assembles the published scope from the base-derived full capture,
|
||||
// and a base the run flagged possibly-stale is one every sibling guard
|
||||
// declines to rule on.
|
||||
if (mergeBase?.fetchFailed && mergeBase.sha != null) {
|
||||
return {
|
||||
incremental: { since, effective: false, reason: 'base-untrusted' },
|
||||
diffBase: null,
|
||||
};
|
||||
}
|
||||
if (mergeBase?.sha != null && !probe.isAncestor(mergeBase.sha, resolved)) {
|
||||
// The clamp. Skipped under `noAncestry` with its sibling: on a rebase
|
||||
// onto newer master the merge base moves PAST the cached head, so the
|
||||
// clamp fires for every amended-and-rebased update — retiring the anchor
|
||||
// there would cost the full range even when the drift stays inside the
|
||||
// CR's files and the narrowing join could still scope (it reads the
|
||||
// delta for its file list only, so no drift byte is published; drift
|
||||
// beyond the CR's files falls back to the full range there anyway).
|
||||
if (
|
||||
!options.noAncestry &&
|
||||
mergeBase?.sha != null &&
|
||||
!probe.isAncestor(mergeBase.sha, resolved)
|
||||
) {
|
||||
return {
|
||||
incremental: { since, effective: false, reason: 'behind-merge-base' },
|
||||
diffBase: null,
|
||||
|
|
@ -1153,6 +1197,13 @@ async function runFetchPr(args: FetchPrArgs): Promise<void> {
|
|||
},
|
||||
},
|
||||
{ sha: mergeBaseSha, fetchFailed: baseFetchFailed },
|
||||
// The AGit-Flow rule (design D7): an Aone update AMENDS the single
|
||||
// CR commit in place and orphans the cached head, so the head test
|
||||
// refuses every update's anchor (the clamp fires only when the
|
||||
// update also rebased) — rule it without ancestry; the two heads'
|
||||
// diff is the update's delta. A force-pushed GitHub history keeps
|
||||
// the tests: there they are the detection.
|
||||
{ noAncestry: platform.kind === 'aone' },
|
||||
);
|
||||
} catch (err) {
|
||||
if (!(err instanceof GitUnavailable)) throw err;
|
||||
|
|
@ -1850,10 +1901,12 @@ export const fetchPrCommand: CommandModule = {
|
|||
'Incremental anchor: the head sha the last clean review round ' +
|
||||
'covered (from the review cache, or the posted ledger marker). ' +
|
||||
'Validated against the fetched history here — an anchor that is ' +
|
||||
'unknown or not an ancestor of the head falls back to the full ' +
|
||||
'diff with the reason in the report; a valid one scopes the diff ' +
|
||||
"and the chunk plan to since..head. The decision is the report's " +
|
||||
'`incremental` field.',
|
||||
'unknown, or not an ancestor of the head, falls back to the full ' +
|
||||
'diff with the reason in the report. Ancestry is skipped on ' +
|
||||
'Aone, where an update AMENDS the single CR commit and orphans ' +
|
||||
"the cached head, so the two heads' diff is the update itself; " +
|
||||
'a valid anchor scopes the diff and the chunk plan to ' +
|
||||
"since..head. The decision is the report's `incremental` field.",
|
||||
})
|
||||
.option('since-model', {
|
||||
type: 'string',
|
||||
|
|
|
|||
|
|
@ -172,10 +172,12 @@ export interface Ledger {
|
|||
* A usable anchor: abbreviated-to-full hex, matching what `git rev-parse`
|
||||
* emits. The parser drops a field that fails this rather than the ledger —
|
||||
* the findings are still a work list even when the anchor is garbage — and
|
||||
* `fetch-pr --since` additionally verifies the anchor is an ancestor of the
|
||||
* fetched head before scoping to it (in the CLI; the orchestrator never runs
|
||||
* git against an anchor), so a tampered sha costs a full-range review, never
|
||||
* a mis-scoped one.
|
||||
* `fetch-pr --since` additionally validates the anchor against the fetched
|
||||
* history — existence always; ancestry except on Aone, where AGit-Flow
|
||||
* amends orphan the cached head (design D7) — before scoping to it (in the
|
||||
* CLI; the orchestrator never runs git against an anchor). The published
|
||||
* scope is joined against the CR's own diff either way, so a tampered sha
|
||||
* costs a full-range review, never a mis-scoped one.
|
||||
*
|
||||
* Exported because `fetch-pr --since` gates on the SAME shape: an anchor the
|
||||
* marker will not carry must not be one the fetch accepts, or a
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue