Skip to content

Commit 8942532

Browse files
HemSoftHemSoft
authored
fix: reject native reviews with ambiguous base provenance (#155)
* fix: reject native reviews with ambiguous base provenance * fix: keep context check IDs bounded and update onboarding pin --------- Co-authored-by: Franz Hemmer <8227352+HemSoft@users.noreply.github.com>
1 parent 2923366 commit 8942532

9 files changed

Lines changed: 176 additions & 18 deletions

File tree

‎.github/workflows/sfl-pr-review-auto.yml‎

Lines changed: 47 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -771,6 +771,30 @@ jobs:
771771
).sort((left, right) => right.id - left.id)[0];
772772
return invalidation ? invalidation.external_id : "none";
773773
};
774+
// BEGIN TESTABLE REVIEW CONTEXT PROVENANCE
775+
function reviewContextUnambiguous(checkRuns, pullNumber, currentBase) {
776+
// Native artifacts identify a head, but neither their start time nor
777+
// originating base. Any previous context on that same head can
778+
// therefore produce a late artifact for a different diff.
779+
const contexts = checkRuns.filter(check => check.app && check.app.id === 15368 &&
780+
((check.external_id || "").startsWith("sfl-codex-review:pull-context:") ||
781+
(check.external_id || "").startsWith("sfl-codex-review:base-advance:")));
782+
const tokens = [...new Set(contexts.map(check => check.external_id))];
783+
if (tokens.length !== 1) return false;
784+
if (!/^sfl-codex-review:pull-context:at:\d+:\d+$/.test(tokens[0])) return false;
785+
// Keep the token compact: it is embedded in terminal check IDs.
786+
// Structured check output carries the base/action provenance.
787+
return contexts.every(check => {
788+
let provenance;
789+
try { provenance = JSON.parse((check.output && check.output.text) || ""); }
790+
catch { return false; }
791+
return provenance && provenance.schema === 1 &&
792+
provenance.pull_number === pullNumber &&
793+
(provenance.action === "opened" || provenance.action === "synchronize") &&
794+
provenance.base_sha === currentBase.toLowerCase();
795+
});
796+
}
797+
// END TESTABLE REVIEW CONTEXT PROVENANCE
774798
const initialChecks = await github.paginate(
775799
github.rest.checks.listForRef,
776800
{
@@ -784,6 +808,10 @@ jobs:
784808
response => response.data,
785809
);
786810
const contextToken = reviewContextToken(initialChecks);
811+
if (!reviewContextUnambiguous(initialChecks, pullNumber, currentBase)) {
812+
core.info("Native Codex artifacts cannot prove their base in this head's context history; advance to a new head before requesting review");
813+
return;
814+
}
787815
const marker = `<!-- sfl-codex-review:head=${currentHead.toLowerCase()};base=${currentBase.toLowerCase()};context=${contextToken} -->`;
788816
const conflictingBaseRequest = hasHistoricalBaseConflict(triggerComments, requestStatuses) || triggerComments.some(
789817
comment => (isOwnerRequest(comment) || historicalRequestIdsFromStatuses(requestStatuses).has(comment.id)) &&
@@ -877,7 +905,8 @@ jobs:
877905
: {state: expectedState};
878906
}
879907
// END TESTABLE REQUIRED GATE REPAIR
880-
if (reviewContextToken(checks) !== contextToken) {
908+
if (reviewContextToken(checks) !== contextToken ||
909+
!reviewContextUnambiguous(checks, pullNumber, currentBase)) {
881910
core.info("The matching Codex request is not bound to the latest pull context");
882911
return;
883912
}
@@ -901,6 +930,10 @@ jobs:
901930
},
902931
response => response.data,
903932
);
933+
if (!reviewContextUnambiguous(latestRequestChecks, pullNumber, currentBase)) {
934+
core.info("The native review base became ambiguous before publication; advance to a new head");
935+
return;
936+
}
904937
const latestRequestStatuses = await github.paginate(
905938
github.rest.repos.listCommitStatusesForRef,
906939
{owner, repo, ref: currentHead, per_page: 100},
@@ -1040,6 +1073,7 @@ jobs:
10401073
pull: currentPull.data,
10411074
openPulls,
10421075
contextToken: reviewContextToken(checkRuns),
1076+
contextUnambiguous: reviewContextUnambiguous(checkRuns, pullNumber, currentBase),
10431077
supersededRequest: hasNewerMatchingRequest(comments, checkRuns, statuses),
10441078
lifecycleChanged: lifecycleToken !== initialLifecycleToken,
10451079
invalidationRuns,
@@ -1053,7 +1087,8 @@ jobs:
10531087
state.supersededRequest ||
10541088
state.lifecycleChanged ||
10551089
state.invalidationRuns.length > 0 ||
1056-
state.contextToken !== contextToken;
1090+
state.contextToken !== contextToken ||
1091+
!state.contextUnambiguous;
10571092
const publicationChangeReason = state => state.supersededRequest
10581093
? "A newer Codex review request superseded this result while it was being published"
10591094
: state.lifecycleChanged
@@ -1234,10 +1269,12 @@ jobs:
12341269
return;
12351270
}
12361271
const reason = context.payload.action === "edited"
1237-
? `Pull request base changed to ${pull.base.ref} at ${pull.base.sha}; request a new Codex review with gh sfl review --retry.`
1272+
? `Pull request base changed to ${pull.base.ref} at ${pull.base.sha}; advance the branch to a new head, then request a Codex review.`
12381273
: context.payload.action === "synchronize"
12391274
? `Pull request head changed to ${pull.head.sha}; request a Codex review for this exact head and base.`
1240-
: `Pull request #${pull.number} opened or reopened for ${pull.base.ref} at ${pull.base.sha}; request a Codex review for this exact pull request context.`;
1275+
: context.payload.action === "reopened"
1276+
? `Pull request #${pull.number} reopened; advance the branch to a new head before requesting review because earlier native artifacts cannot prove their base.`
1277+
: `Pull request #${pull.number} opened for ${pull.base.ref} at ${pull.base.sha}; request a Codex review for this exact pull request context.`;
12411278
const finalChecks = await github.paginate(
12421279
github.rest.checks.listForRef,
12431280
{
@@ -1277,7 +1314,12 @@ jobs:
12771314
completed_at: new Date().toISOString(),
12781315
details_url: pull.html_url,
12791316
external_id: invalidationId,
1280-
output: {title: "SFL Codex review invalidated", summary: reason},
1317+
output: {
1318+
title: "SFL Codex review invalidated",
1319+
summary: reason,
1320+
text: JSON.stringify({schema: 1, pull_number: pull.number,
1321+
action: context.payload.action, base_sha: pull.base.sha.toLowerCase()}),
1322+
},
12811323
});
12821324
await github.rest.repos.createCommitStatus({
12831325
owner,

‎VERSION‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1 +1 @@
1-
2.1.0-rc.20
1+
2.1.0-rc.21

‎deployment/infrastructure/sfl-pr-review-auto.yml‎

Lines changed: 47 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -771,6 +771,30 @@ jobs:
771771
).sort((left, right) => right.id - left.id)[0];
772772
return invalidation ? invalidation.external_id : "none";
773773
};
774+
// BEGIN TESTABLE REVIEW CONTEXT PROVENANCE
775+
function reviewContextUnambiguous(checkRuns, pullNumber, currentBase) {
776+
// Native artifacts identify a head, but neither their start time nor
777+
// originating base. Any previous context on that same head can
778+
// therefore produce a late artifact for a different diff.
779+
const contexts = checkRuns.filter(check => check.app && check.app.id === 15368 &&
780+
((check.external_id || "").startsWith("sfl-codex-review:pull-context:") ||
781+
(check.external_id || "").startsWith("sfl-codex-review:base-advance:")));
782+
const tokens = [...new Set(contexts.map(check => check.external_id))];
783+
if (tokens.length !== 1) return false;
784+
if (!/^sfl-codex-review:pull-context:at:\d+:\d+$/.test(tokens[0])) return false;
785+
// Keep the token compact: it is embedded in terminal check IDs.
786+
// Structured check output carries the base/action provenance.
787+
return contexts.every(check => {
788+
let provenance;
789+
try { provenance = JSON.parse((check.output && check.output.text) || ""); }
790+
catch { return false; }
791+
return provenance && provenance.schema === 1 &&
792+
provenance.pull_number === pullNumber &&
793+
(provenance.action === "opened" || provenance.action === "synchronize") &&
794+
provenance.base_sha === currentBase.toLowerCase();
795+
});
796+
}
797+
// END TESTABLE REVIEW CONTEXT PROVENANCE
774798
const initialChecks = await github.paginate(
775799
github.rest.checks.listForRef,
776800
{
@@ -784,6 +808,10 @@ jobs:
784808
response => response.data,
785809
);
786810
const contextToken = reviewContextToken(initialChecks);
811+
if (!reviewContextUnambiguous(initialChecks, pullNumber, currentBase)) {
812+
core.info("Native Codex artifacts cannot prove their base in this head's context history; advance to a new head before requesting review");
813+
return;
814+
}
787815
const marker = `<!-- sfl-codex-review:head=${currentHead.toLowerCase()};base=${currentBase.toLowerCase()};context=${contextToken} -->`;
788816
const conflictingBaseRequest = hasHistoricalBaseConflict(triggerComments, requestStatuses) || triggerComments.some(
789817
comment => (isOwnerRequest(comment) || historicalRequestIdsFromStatuses(requestStatuses).has(comment.id)) &&
@@ -877,7 +905,8 @@ jobs:
877905
: {state: expectedState};
878906
}
879907
// END TESTABLE REQUIRED GATE REPAIR
880-
if (reviewContextToken(checks) !== contextToken) {
908+
if (reviewContextToken(checks) !== contextToken ||
909+
!reviewContextUnambiguous(checks, pullNumber, currentBase)) {
881910
core.info("The matching Codex request is not bound to the latest pull context");
882911
return;
883912
}
@@ -901,6 +930,10 @@ jobs:
901930
},
902931
response => response.data,
903932
);
933+
if (!reviewContextUnambiguous(latestRequestChecks, pullNumber, currentBase)) {
934+
core.info("The native review base became ambiguous before publication; advance to a new head");
935+
return;
936+
}
904937
const latestRequestStatuses = await github.paginate(
905938
github.rest.repos.listCommitStatusesForRef,
906939
{owner, repo, ref: currentHead, per_page: 100},
@@ -1040,6 +1073,7 @@ jobs:
10401073
pull: currentPull.data,
10411074
openPulls,
10421075
contextToken: reviewContextToken(checkRuns),
1076+
contextUnambiguous: reviewContextUnambiguous(checkRuns, pullNumber, currentBase),
10431077
supersededRequest: hasNewerMatchingRequest(comments, checkRuns, statuses),
10441078
lifecycleChanged: lifecycleToken !== initialLifecycleToken,
10451079
invalidationRuns,
@@ -1053,7 +1087,8 @@ jobs:
10531087
state.supersededRequest ||
10541088
state.lifecycleChanged ||
10551089
state.invalidationRuns.length > 0 ||
1056-
state.contextToken !== contextToken;
1090+
state.contextToken !== contextToken ||
1091+
!state.contextUnambiguous;
10571092
const publicationChangeReason = state => state.supersededRequest
10581093
? "A newer Codex review request superseded this result while it was being published"
10591094
: state.lifecycleChanged
@@ -1234,10 +1269,12 @@ jobs:
12341269
return;
12351270
}
12361271
const reason = context.payload.action === "edited"
1237-
? `Pull request base changed to ${pull.base.ref} at ${pull.base.sha}; request a new Codex review with gh sfl review --retry.`
1272+
? `Pull request base changed to ${pull.base.ref} at ${pull.base.sha}; advance the branch to a new head, then request a Codex review.`
12381273
: context.payload.action === "synchronize"
12391274
? `Pull request head changed to ${pull.head.sha}; request a Codex review for this exact head and base.`
1240-
: `Pull request #${pull.number} opened or reopened for ${pull.base.ref} at ${pull.base.sha}; request a Codex review for this exact pull request context.`;
1275+
: context.payload.action === "reopened"
1276+
? `Pull request #${pull.number} reopened; advance the branch to a new head before requesting review because earlier native artifacts cannot prove their base.`
1277+
: `Pull request #${pull.number} opened for ${pull.base.ref} at ${pull.base.sha}; request a Codex review for this exact pull request context.`;
12411278
const finalChecks = await github.paginate(
12421279
github.rest.checks.listForRef,
12431280
{
@@ -1277,7 +1314,12 @@ jobs:
12771314
completed_at: new Date().toISOString(),
12781315
details_url: pull.html_url,
12791316
external_id: invalidationId,
1280-
output: {title: "SFL Codex review invalidated", summary: reason},
1317+
output: {
1318+
title: "SFL Codex review invalidated",
1319+
summary: reason,
1320+
text: JSON.stringify({schema: 1, pull_number: pull.number,
1321+
action: context.payload.action, base_sha: pull.base.sha.toLowerCase()}),
1322+
},
12811323
});
12821324
await github.rest.repos.createCommitStatus({
12831325
owner,

‎deployment/release-metadata.json‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -3,9 +3,9 @@
33
"reviewedAt": "2026-10-08",
44
"distribution": {
55
"repository": "hemsoft-dev/set-it-free-loop",
6-
"version": "2.1.0-rc.20",
6+
"version": "2.1.0-rc.21",
77
"visibility": "private",
8-
"tag": "v2.1.0-rc.20",
8+
"tag": "v2.1.0-rc.21",
99
"prerelease": true
1010
},
1111
"reviewerRuntime": {

‎deployment/scripts/validate-org-rollout.py‎

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -34,7 +34,8 @@
3434
PILOT_SCENARIOS = {'findings':'gate_failed', 'pending_request':'gate_blocked', 'malformed_output':'gate_blocked',
3535
'revoked_permission':'gate_blocked', 'permission_lookup_failure':'gate_blocked',
3636
'forged_registration':'gate_blocked', 'edited_registration':'gate_blocked',
37-
'duplicate_delivery':'idempotent', 'new_head':'stale_gate_rejected', 'base_advance':'stale_gate_rejected'}
37+
'duplicate_delivery':'idempotent', 'new_head':'stale_gate_rejected', 'base_advance':'stale_gate_rejected',
38+
'overlapping_request':'gate_blocked', 'unregistered_base_context':'stale_gate_rejected'}
3839
# Franz's recorded October 6 decision. Expanding this set requires a new owner decision.
3940
APPROVED_RETAINED_IDS = {1162179521, 1169698740}
4041
RETENTION_RECEIPT = 'https://github.com/HemSoft/set-it-free-loop/issues/138#issuecomment-6028207635'
@@ -2570,7 +2571,7 @@ def replay_observer_fixture(workflow_json, repository_id, repository, sha, versi
25702571
canonical = (root / 'deployment/infrastructure/sfl-pr-review-auto.yml').read_text()
25712572
markers = [('// BEGIN TESTABLE ' + name, '// END TESTABLE ' + name) for name in
25722573
('CODEX OBSERVER', 'REQUESTER AUTHORIZATION', 'REQUESTER REFRESH', 'REQUEST REGISTRATION',
2573-
'REQUEST ELIGIBILITY', 'REQUIRED GATE REPAIR')]
2574+
'REQUEST ELIGIBILITY', 'REQUIRED GATE REPAIR', 'REVIEW CONTEXT PROVENANCE')]
25742575
markers.append(('const existing = latestRequestChecks.find(', 'const detailsURL = artifact.html_url'))
25752576
for begin, end in markers:
25762577
require(source.count(begin) == canonical.count(begin) > 0 and

0 commit comments

Comments
 (0)