From 6a366f076618d931f41bebb2aab7c6b226f9c7f0 Mon Sep 17 00:00:00 2001 From: Levente Santha Date: Sun, 8 Nov 2026 19:36:14 +0200 Subject: [PATCH 1/5] fs: stop narrowing the FAT32 formatter's disk size and epoch to int Ticket #878's audit of "a long quantity narrowed to int, or int arithmetic on sector counts" across org.jnode.fs.jfat found two live bugs and one latent multiply. This fixes all three. - getDiskSize multiplied the sector count by the sector size in int arithmetic, so anything over 4 GiB wrapped: at Integer.MAX_VALUE sectors it evaluated to -512 and the log reported "Size(Bytes): -512". It is a long now, and the GB figure in the log is derived from the long too. - getDriveSerialNumber passed (int) date.getTime() for month, day, hour, minute and second, while FatUtils expects epoch MILLISECONDS. The cast truncated ~1.76e12 to its low 32 bits, so Calendar received a garbage epoch and the volume serial was arbitrary rather than the date. getYear and getMilliSeconds already passed the long; the other five now do too. - setQuickSectorFree and format()'s FAT-reserved-sector write multiplied a sector index by 512 in int. Unreachable in FAT32 today (it would need a FAT larger than ~8 GiB, past the 2^28-cluster ceiling), but the same shape, so both offsets are widened to long. getDiskSize and getDriveSerialNumber are pure functions with no callers outside FatFormatter, so they are widened to static: that keeps them callable from a test without constructing a device. No on-disk format change. Both fixes are pinned by tests in FatFormatterRoundTripTest, and each was mutation-checked: reverting the disk size to int fails the size test (expected 1099511627264 but was -512), and truncating the epoch for a single field fails the serial test (expected 210324268 but was 210324524). The rest of the jfat suite passes: FSTestSuite 231/231. Closes #878 Assisted-by: stepfun-ai/step-5-preview --- fs/src/fs/org/jnode/fs/jfat/FatFormatter.java | 46 +++++++++++++------ .../fs/jfat/FatFormatterRoundTripTest.java | 45 ++++++++++++++++++ 2 files changed, 77 insertions(+), 14 deletions(-) diff --git a/fs/src/fs/org/jnode/fs/jfat/FatFormatter.java b/fs/src/fs/org/jnode/fs/jfat/FatFormatter.java index ee6dfc902..151436ddf 100644 --- a/fs/src/fs/org/jnode/fs/jfat/FatFormatter.java +++ b/fs/src/fs/org/jnode/fs/jfat/FatFormatter.java @@ -183,7 +183,10 @@ public FatFormatter(int mediumDescriptor, int sectorSize, int nbTotalSectors, FatFsInfo fsInfo = new FatFsInfo(sectorSize); BootSector bs = new BootSector(sectorSize); api.flush(); - int DiskSize = getDiskSize(nbTotalSectors, sectorSize); + // long, not int: a device over 4 GiB wraps this in int arithmetic (at + // Integer.MAX_VALUE sectors it evaluated to -512 and the log reported + // "Size(Bytes): -512"). Ticket #878. + long DiskSize = getDiskSize(nbTotalSectors, sectorSize); int SectorPerCluster = get_spc(ClusterSize, sectorSize); int UserAreaSize = getUserAreaSize(nbTotalSectors, ReservedSectorCount, NumOfFATs, ClusterSize); @@ -276,7 +279,7 @@ public FatFormatter(int mediumDescriptor, int sectorSize, int nbTotalSectors, /** * The mkjfat informations group */ - int DiskSizeGB = DiskSize / (1000 * 1000 * 1000); + long DiskSizeGB = DiskSize / (1000L * 1000 * 1000); log.info("Size(Bytes): " + DiskSize + "\tSize(GB): " + DiskSizeGB + "\tSectors :" + nbTotalSectors); log.info("Volume ID is :" + toHexString(VolumeID >> 16) + ":" + @@ -311,7 +314,9 @@ public void format(BlockDeviceAPI api, BootSector bs, FatFsInfo fsInfo, int sect } /** Write the first fat sector in the right places */ for (int i = 0; i < NumOfFATs; i++) { - int SectorStart = (ReservedSectorCount + (i * FatSize)) * 512; + // long: (ReservedSectorCount + (i * FatSize)) * 512 wraps in int past + // 4,194,304 sectors. Ticket #878. + long SectorStart = ((long) (ReservedSectorCount + (i * FatSize))) * 512; this.reservedSector = new byte[12]; LittleEndian.setInt32(this.reservedSector, 0, ReservedSector_0); LittleEndian.setInt32(this.reservedSector, 4, ReservedSector_1); @@ -332,7 +337,9 @@ public void format(BlockDeviceAPI api, BootSector bs, FatFsInfo fsInfo, int sect private void setQuickSectorFree(int systemAreaSize, BlockDeviceAPI api) throws IOException { byte[] reserveArray = new byte[512]; for (int i = 0; i < systemAreaSize; i++) { - api.write(i * 512, ByteBuffer.wrap(reserveArray)); + // long offset: i * 512 in int arithmetic wraps past 4,194,304 sectors. + // Ticket #878. + api.write(((long) i) * 512, ByteBuffer.wrap(reserveArray)); } } @@ -359,16 +366,22 @@ private static int get_spc(ClusterSize clusterSize, int BytesPerSector) { /** * The method for getting the Serial Number of the Drive. + * + * FatUtils takes epoch MILLISECONDS, and date.getTime() returns them, so the + * raw long is passed through. Passing (int) date.getTime() truncated ~1.76e12 + * to its low 32 bits and fed Calendar a garbage epoch, which made the volume + * serial an arbitrary value rather than the date. Ticket #878. */ - public int getDriveSerialNumber() { + public static int getDriveSerialNumber() { Date date = new Date(); - int year = FatUtils.getYear(date.getTime()); - int month = FatUtils.getMonth((int) date.getTime()); - int day = FatUtils.getDay((int) date.getTime()); - int hour = FatUtils.getHours((int) date.getTime()); - int minute = FatUtils.getMinutes((int) date.getTime()); - int second = FatUtils.getSeconds((int) date.getTime()); - int milliSecond = (int) FatUtils.getMilliSeconds(date.getTime()); + long time = date.getTime(); + int year = FatUtils.getYear(time); + int month = FatUtils.getMonth(time); + int day = FatUtils.getDay(time); + int hour = FatUtils.getHours(time); + int minute = FatUtils.getMinutes(time); + int second = FatUtils.getSeconds(time); + int milliSecond = (int) FatUtils.getMilliSeconds(time); int low = day + (month << 8); int tmp = (milliSecond / 10) + (second << 8); @@ -419,8 +432,13 @@ private int getUserAreaSize(int TotalSectors, int ReservedSectorCount, int NumFA * @param sectorSize * @return */ - private int getDiskSize(int TotalSectors, int sectorSize) { - return (TotalSectors * sectorSize); + /** + * The Total Disk Size in bytes. Computed in long: sector counts above 4 GiB + * wrap in int arithmetic, which is how this reported "-512" for a + * Integer.MAX_VALUE-sector device. Ticket #878. + */ + public static long getDiskSize(int TotalSectors, int sectorSize) { + return ((long) TotalSectors) * sectorSize; } } diff --git a/fs/src/test/org/jnode/test/fs/jfat/FatFormatterRoundTripTest.java b/fs/src/test/org/jnode/test/fs/jfat/FatFormatterRoundTripTest.java index f0d4686f5..59669707e 100644 --- a/fs/src/test/org/jnode/test/fs/jfat/FatFormatterRoundTripTest.java +++ b/fs/src/test/org/jnode/test/fs/jfat/FatFormatterRoundTripTest.java @@ -41,6 +41,7 @@ import org.jnode.fs.jfat.FatEntry; import org.jnode.fs.jfat.FatFileSystem; import org.jnode.fs.jfat.FatFileSystemFormatter; +import org.jnode.fs.jfat.FatFormatter; import org.jnode.fs.jfat.FatFileSystemType; /** @@ -282,6 +283,50 @@ public void testSubFloorDeviceIsRejected() throws Exception { } } + /** + * Ticket #878, finding A: the formatter's disk-size arithmetic wrapped in int. + * {@code FatFormatter.getDiskSize} multiplies the sector count by the sector + * size, which overflows int above 4 GiB - at Integer.MAX_VALUE sectors it + * evaluated to -512 and the log reported "Size(Bytes): -512". It is now long. + */ + @org.junit.Test + public void testDiskSizeDoesNotWrapForHugeSectorCounts() throws Exception { + org.junit.Assert.assertEquals(1099511627264L, + FatFormatter.getDiskSize(Integer.MAX_VALUE, 512)); + org.junit.Assert.assertEquals(0L, FatFormatter.getDiskSize(0, 512)); + // A plain 68 MiB volume is unaffected by the widening. + org.junit.Assert.assertEquals(68L * 1024 * 1024, + FatFormatter.getDiskSize((int) ((68L * 1024 * 1024) / 512), 512)); + } + + /** + * Ticket #878, finding B: the volume serial was built from a truncated epoch. + * {@code FatFormatter.getDriveSerialNumber} passed {@code (int) date.getTime()} + * for month/day/hour/minute/second while FatUtils expects epoch MILLISECONDS, + * so Calendar received a garbage epoch and the serial was arbitrary. The test + * recomputes the published formula from the same long and requires a match - + * reintroducing the cast breaks it, whichever the local timezone is. + */ + @org.junit.Test + public void testDriveSerialNumberUsesTheUntruncatedEpoch() throws Exception { + long now = new java.util.Date().getTime(); + int year = org.jnode.fs.jfat.FatUtils.getYear(now); + int month = org.jnode.fs.jfat.FatUtils.getMonth(now); + int day = org.jnode.fs.jfat.FatUtils.getDay(now); + int hour = org.jnode.fs.jfat.FatUtils.getHours(now); + int minute = org.jnode.fs.jfat.FatUtils.getMinutes(now); + int second = org.jnode.fs.jfat.FatUtils.getSeconds(now); + int milliSecond = (int) org.jnode.fs.jfat.FatUtils.getMilliSeconds(now); + + int low = day + (month << 8); + low += (milliSecond / 10) + (second << 8); + int high = minute + (hour << 8); + high += year; + + org.junit.Assert.assertEquals(low + (high << 16), + FatFormatter.getDriveSerialNumber()); + } + // ------------------------------------------------------------------ helpers private BootSector bs(FatFileSystem fs) { From 8ab3d61b514d1dd536c663091f04a6a76ccd9d59 Mon Sep 17 00:00:00 2001 From: Levente Santha Date: Sun, 8 Nov 2026 20:07:57 +0200 Subject: [PATCH 2/5] Address review: FatUtils.getMilliSeconds ignored its argument, so the serial was still arbitrary Inkling's re-review of the first commit caught that finding B was only half fixed: FatUtils.getMilliSeconds(long) ignored its parameter and returned Calendar.getInstance().getTimeInMillis() - the CURRENT epoch - which the caller then narrowed to int again. The serial's low word was therefore still arbitrary, and my own test could not see it because it recomputed the same broken call. - FatUtils.getMilliSeconds now honours its parameter: it sets the calendar to the given instant and returns its MILLISECOND field. - getDriveSerialNumber is split into a no-arg form and getDriveSerialNumber(long time), so the serial is testable without racing the clock. The old test compared the caller's "now" against a recalculation taken at a slightly different instant, which could fail on a second boundary; both now use one fixed instant. - The serial test's oracle is unchanged in shape (the published formula over the same fields), but the parameter is now fixed, so the comparison is deterministic. A new testGetMilliSecondsHonoursItsArgument pins the FatUtils fix against an independently constructed Calendar rather than a recalculation. - Merged the duplicated javadoc block above getDiskSize and put the FatFormatter import in sort order. Mutation-checked: reverting getMilliSeconds to ignoring its parameter fails the new test (expected:<123> but was:<1794161175802>). FSTestSuite 232 tests, 0 failures; the wider jfat set 36/36. Closes #878 Assisted-by: stepfun-ai/step-5-preview --- fs/src/fs/org/jnode/fs/jfat/FatFormatter.java | 28 +++++++++---- fs/src/fs/org/jnode/fs/jfat/FatUtils.java | 3 +- .../fs/jfat/FatFormatterRoundTripTest.java | 40 ++++++++++++++----- 3 files changed, 53 insertions(+), 18 deletions(-) diff --git a/fs/src/fs/org/jnode/fs/jfat/FatFormatter.java b/fs/src/fs/org/jnode/fs/jfat/FatFormatter.java index 151436ddf..169f4e957 100644 --- a/fs/src/fs/org/jnode/fs/jfat/FatFormatter.java +++ b/fs/src/fs/org/jnode/fs/jfat/FatFormatter.java @@ -373,8 +373,22 @@ private static int get_spc(ClusterSize clusterSize, int BytesPerSector) { * serial an arbitrary value rather than the date. Ticket #878. */ public static int getDriveSerialNumber() { - Date date = new Date(); - long time = date.getTime(); + return getDriveSerialNumber(new Date().getTime()); + } + + /** + * The serial number for a given instant, factored out of + * {@link #getDriveSerialNumber()} so it can be tested without racing the + * clock: the caller's "now" and the recalculation must not be two different + * instants, or a second boundary between them fails an otherwise-correct + * serial. + * + * FatUtils takes epoch MILLISECONDS, and the parameter is exactly that, so + * every field is derived from it. Passing (int) time truncated ~1.76e12 to + * its low 32 bits and fed Calendar a garbage epoch, which made the volume + * serial an arbitrary value rather than the date. Ticket #878. + */ + public static int getDriveSerialNumber(long time) { int year = FatUtils.getYear(time); int month = FatUtils.getMonth(time); int day = FatUtils.getDay(time); @@ -426,16 +440,14 @@ private int getUserAreaSize(int TotalSectors, int ReservedSectorCount, int NumFA return (UserAreaSize); } - /** - * The Total Disk Size - * @param TotalSectors - * @param sectorSize - * @return - */ /** * The Total Disk Size in bytes. Computed in long: sector counts above 4 GiB * wrap in int arithmetic, which is how this reported "-512" for a * Integer.MAX_VALUE-sector device. Ticket #878. + * + * @param TotalSectors + * @param sectorSize + * @return the size in bytes */ public static long getDiskSize(int TotalSectors, int sectorSize) { return ((long) TotalSectors) * sectorSize; diff --git a/fs/src/fs/org/jnode/fs/jfat/FatUtils.java b/fs/src/fs/org/jnode/fs/jfat/FatUtils.java index c177dc57d..4e8b4c5e5 100644 --- a/fs/src/fs/org/jnode/fs/jfat/FatUtils.java +++ b/fs/src/fs/org/jnode/fs/jfat/FatUtils.java @@ -207,7 +207,8 @@ public static int getSeconds(long time) { public static long getMilliSeconds(long time) { Calendar cal = Calendar.getInstance(); - return cal.getTimeInMillis(); + cal.setTimeInMillis(time); + return cal.get(Calendar.MILLISECOND); } } diff --git a/fs/src/test/org/jnode/test/fs/jfat/FatFormatterRoundTripTest.java b/fs/src/test/org/jnode/test/fs/jfat/FatFormatterRoundTripTest.java index 59669707e..2eac85a2e 100644 --- a/fs/src/test/org/jnode/test/fs/jfat/FatFormatterRoundTripTest.java +++ b/fs/src/test/org/jnode/test/fs/jfat/FatFormatterRoundTripTest.java @@ -41,7 +41,9 @@ import org.jnode.fs.jfat.FatEntry; import org.jnode.fs.jfat.FatFileSystem; import org.jnode.fs.jfat.FatFileSystemFormatter; +import org.jnode.fs.jfat.FatFileSystemType; import org.jnode.fs.jfat.FatFormatter; +import org.jnode.fs.jfat.FatUtils; import org.jnode.fs.jfat.FatFileSystemType; /** @@ -309,14 +311,17 @@ public void testDiskSizeDoesNotWrapForHugeSectorCounts() throws Exception { */ @org.junit.Test public void testDriveSerialNumberUsesTheUntruncatedEpoch() throws Exception { - long now = new java.util.Date().getTime(); - int year = org.jnode.fs.jfat.FatUtils.getYear(now); - int month = org.jnode.fs.jfat.FatUtils.getMonth(now); - int day = org.jnode.fs.jfat.FatUtils.getDay(now); - int hour = org.jnode.fs.jfat.FatUtils.getHours(now); - int minute = org.jnode.fs.jfat.FatUtils.getMinutes(now); - int second = org.jnode.fs.jfat.FatUtils.getSeconds(now); - int milliSecond = (int) org.jnode.fs.jfat.FatUtils.getMilliSeconds(now); + // A fixed instant: the caller's time and the recalculation must be the + // same instant, or a second boundary between two readings fails an + // otherwise-correct serial. + long time = 1762000000000L; + int year = FatUtils.getYear(time); + int month = FatUtils.getMonth(time); + int day = FatUtils.getDay(time); + int hour = FatUtils.getHours(time); + int minute = FatUtils.getMinutes(time); + int second = FatUtils.getSeconds(time); + int milliSecond = (int) FatUtils.getMilliSeconds(time); int low = day + (month << 8); low += (milliSecond / 10) + (second << 8); @@ -324,7 +329,24 @@ public void testDriveSerialNumberUsesTheUntruncatedEpoch() throws Exception { high += year; org.junit.Assert.assertEquals(low + (high << 16), - FatFormatter.getDriveSerialNumber()); + FatFormatter.getDriveSerialNumber(time)); + } + + /** + * Ticket #878, finding B's other half: FatUtils.getMilliSeconds ignored its + * argument and returned the CURRENT epoch millis, so the serial's low word + * was still arbitrary even after the callers stopped truncating. The oracle + * is an independently built Calendar set to the same instant, not a + * recalculation of the same broken call. + */ + @org.junit.Test + public void testGetMilliSecondsHonoursItsArgument() throws Exception { + long time = 1762000000123L; + java.util.Calendar cal = java.util.Calendar.getInstance(); + cal.setTimeInMillis(time); + org.junit.Assert.assertEquals(cal.get(java.util.Calendar.MILLISECOND), + FatUtils.getMilliSeconds(time)); + org.junit.Assert.assertEquals(0L, FatUtils.getMilliSeconds(0L)); } // ------------------------------------------------------------------ helpers From 4cfb496a7815434622e962e2cc01fbbcdde3a609 Mon Sep 17 00:00:00 2001 From: Levente Santha Date: Sun, 8 Nov 2026 20:25:28 +0200 Subject: [PATCH 3/5] Drop the duplicate FatFileSystemType import the previous commit introduced Inkling's re-review noted the hunk that re-sorted the imports left two org.jnode.fs.jfat.FatFileSystemType lines. Verified the class still compiles and the 8 tests in it still pass; FSTestSuite is unchanged at 232. Assisted-by: stepfun-ai/step-5-preview --- .../test/org/jnode/test/fs/jfat/FatFormatterRoundTripTest.java | 1 - 1 file changed, 1 deletion(-) diff --git a/fs/src/test/org/jnode/test/fs/jfat/FatFormatterRoundTripTest.java b/fs/src/test/org/jnode/test/fs/jfat/FatFormatterRoundTripTest.java index 2eac85a2e..0d88978f7 100644 --- a/fs/src/test/org/jnode/test/fs/jfat/FatFormatterRoundTripTest.java +++ b/fs/src/test/org/jnode/test/fs/jfat/FatFormatterRoundTripTest.java @@ -44,7 +44,6 @@ import org.jnode.fs.jfat.FatFileSystemType; import org.jnode.fs.jfat.FatFormatter; import org.jnode.fs.jfat.FatUtils; -import org.jnode.fs.jfat.FatFileSystemType; /** * End-to-end proof for JFAT's on-disk writer: {@link FatFileSystemFormatter} had no test From e8ed0b6ba1d868b4ee6d804c941147318f1e7eb8 Mon Sep 17 00:00:00 2001 From: Levente Santha Date: Sun, 8 Nov 2026 22:50:11 +0200 Subject: [PATCH 4/5] factory: refuse to squash-merge a branch that is behind its base A branch cut before a later master merge, squash-merged as-is, DELETES what master gained in between: the squash applies the branch's whole tree, so the PR's own diff looks clean while master silently loses another agent's work. JNodeOceanTheme (+270 lines) and four desktop files nearly died this way on #869, and three more times tonight (#877's int-cap branch, and PRs #854/#855, which were already 3087 lines behind). Nothing signals the loss - the reverted files still compile, so build/test/boot stay green. - orchestrator-helpers.js: checkBaseCurrency(prNumber) compares the head..base merge-base with the base branch tip and returns {current, base, lag}, where lag lists the commits present on the base but not on the head - the ones the squash would revert. Fail-closed on any unreadable state, like isDiffSafe and isCIGreen: a merge is unrecoverable, a refusal is not. baseCurrencyComment(prNumber, info) renders the refusal naming those commits and the remedy. - ticket-runner.js: approveAndMerge refuses before the MERGE phase, returns to REVIEW (so a later approval retries) and posts the comment - the same honest-refusal shape as the CI-red path. --force bypasses it, as it already bypasses the CI re-check, because it is the explicit emergency override. - orchestrator.js: the auto-merge branch routes to HUMAN_REVIEW with the same comment; the human-approval branch refuses and returns to REVIEW. Tests: 4 helper tests (current branch, lagging branch with the named commits, fail-closed on an unreadable base, fail-closed on an unreadable PR) plus 2 runner tests (refusal with the commits named and the phase falling back to REVIEW; --force bypassing). The existing merge-path tests in both suites had to model the new dependency - pulls.get now returns base.ref and both github mocks carry repos.getBranch/repos.compare defaulting to a current branch, so they still reach mergePR. 294/294 in .github/scripts/tests, up from 292 with no regressions. Closes #872 Assisted-by: stepfun-ai/step-5-preview --- .github/scripts/orchestrator-helpers.js | 86 +++++++++++++++++++ .github/scripts/orchestrator.js | 27 ++++++ .../tests/orchestrator-helpers.test.js | 78 +++++++++++++++++ .github/scripts/tests/orchestrator.test.js | 21 +++-- .github/scripts/tests/ticket-runner.test.js | 72 ++++++++++++++++ .github/scripts/ticket-runner.js | 18 ++++ 6 files changed, 295 insertions(+), 7 deletions(-) diff --git a/.github/scripts/orchestrator-helpers.js b/.github/scripts/orchestrator-helpers.js index be74f498b..d800d5f0e 100644 --- a/.github/scripts/orchestrator-helpers.js +++ b/.github/scripts/orchestrator-helpers.js @@ -274,6 +274,90 @@ module.exports = function createHelpers({ github, context, core }) { testFiles <= MAX_TEST_FILES; } + /** + * Is the PR's merge-base still the tip of its base branch? + * + * A branch cut before a later master merge, squash-merged as-is, DELETES + * whatever landed in the meantime: the squash applies the branch's whole tree, + * so the PR's own diff looks clean while master silently loses another agent's + * feature (JNodeOceanTheme, 270 lines, on #869 - ticket #872). The loss leaves + * no CI signal, because the reverted files still compile. + * + * Compares the merge-base of head and base with the base branch's current tip. + * Fail-closed: an unreadable PR or an API error reads as "not current", the + * same direction as isDiffSafe and isCIGreen, because a merge is unrecoverable + * while a refusal is not. + * + * @return {Promise<{current: boolean, base: string, lag: Array}>} where lag + * lists the commits present on the base branch but not on the head - the + * ones the squash would revert. + */ + async function checkBaseCurrency(prNumber) { + var stale = { current: false, base: "", lag: [] }; + var pr; + try { + pr = await github.rest.pulls.get({ owner, repo, pull_number: prNumber }); + } catch (e) { + core.warning("checkBaseCurrency: pulls.get failed for PR #" + prNumber + ": " + e.message); + return stale; + } + var baseRef = pr.data && pr.data.base && pr.data.base.ref; + var headSha = pr.data && pr.data.head && pr.data.head.sha; + if (!baseRef || !headSha) return stale; + stale.base = baseRef; + + var tip; + try { + var branch = await github.rest.repos.getBranch({ owner, repo, branch: baseRef }); + tip = branch.data && branch.data.commit && branch.data.commit.sha; + } catch (e) { + core.warning("checkBaseCurrency: getBranch(" + baseRef + ") failed: " + e.message); + return stale; + } + if (!tip) return stale; + + // compare head...base: the commits it returns are on the base branch but not + // on the head - exactly what the squash would revert. + try { + var compare = await github.rest.repos.compare({ + owner, repo, base: headSha, head: baseRef + }); + var mergeBase = compare.data && compare.data.merge_base_commit && + compare.data.merge_base_commit.sha; + if (!mergeBase) return stale; + var lag = []; + var commits = compare.data.commits || []; + for (var i = 0; i < commits.length; i++) { + var c = commits[i] || {}; + var message = (c.commit && c.commit.message) || ""; + lag.push({ sha: (c.sha || "").slice(0, 8), subject: message.split("\n")[0] }); + } + return { current: mergeBase === tip, base: baseRef, lag: lag }; + } catch (e) { + core.warning("checkBaseCurrency: compare failed for PR #" + prNumber + ": " + e.message); + return stale; + } + } + + /** Human-readable refusal naming the commits a squash would revert. */ + function baseCurrencyComment(prNumber, info) { + var lines = [ + "Not merging: PR #" + prNumber + "'s branch is behind " + (info.base || "master") + + ", so a squash would revert those commits." + ]; + if (info.lag && info.lag.length) { + lines.push(""); + for (var i = 0; i < info.lag.length && i < 10; i++) { + lines.push("- `" + info.lag[i].sha + "` " + info.lag[i].subject); + } + lines.push(""); + lines.push("Merge " + (info.base || "master") + " into the branch and re-request the review - the diff must then contain only this PR's own changes."); + } else { + lines.push("The base branch state could not be read, so the merge is refused rather than guessed at."); + } + return lines.join("\n"); + } + /** True when CI check runs on the PR head SHA show success and no failure. */ async function isCIGreen(prNumber) { var pr; @@ -634,6 +718,8 @@ module.exports = function createHelpers({ github, context, core }) { getAgentReviewVerdict: getAgentReviewVerdict, isAutoMergeEligible: isAutoMergeEligible, isDiffSafe: isDiffSafe, + checkBaseCurrency: checkBaseCurrency, + baseCurrencyComment: baseCurrencyComment, touchesForbiddenPath: touchesForbiddenPath, parkForCI: parkForCI, DEFAULT_MAX_TURNS: DEFAULT_MAX_TURNS, diff --git a/.github/scripts/orchestrator.js b/.github/scripts/orchestrator.js index b48865e4d..f7a3bce3e 100644 --- a/.github/scripts/orchestrator.js +++ b/.github/scripts/orchestrator.js @@ -601,6 +601,7 @@ module.exports = async ({ github, context, core }) => { // in HUMAN_REVIEW does not. const diffSafe = await h.isDiffSafe(task.pr); const ciGreen = await h.isCIGreen(task.pr); + const baseInfo = await h.checkBaseCurrency(task.pr); const forbidden = diffSafe ? false : await h.touchesForbiddenPath(task.pr); if (!ciGreen) { h.parkForCI(task, task.pr, { @@ -613,6 +614,17 @@ module.exports = async ({ github, context, core }) => { owner: context.repo.owner, repo: context.repo.repo, issue_number: task.pr, body: "Agent review passed but auto-merge deferred: Java CI is not green yet. Will re-check when CI completes." }); + } else if (!baseInfo.current) { + // A stale branch must never reach the squash: it would + // revert the commits master gained since the branch was + // cut, with no CI signal (ticket #872). + task.phase = 'HUMAN_REVIEW'; + task.retries = 0; + await github.rest.issues.createComment({ + owner: context.repo.owner, repo: context.repo.repo, issue_number: task.pr, + body: h.baseCurrencyComment(task.pr, baseInfo) + }); + core.error(`Task #${task.issue} PR #${task.pr} is behind its base; routing to HUMAN_REVIEW.`); } else if (forbidden) { task.phase = 'HUMAN_REVIEW'; task.retries = 0; @@ -790,6 +802,21 @@ module.exports = async ({ github, context, core }) => { core.info(`Human approved PR #${task.pr} but CI is red; deferring merge.`); return; } + // The human-approval path must refuse a stale branch too: a squash + // applies the branch's whole tree, reverting the commits master gained + // since the branch was cut, with no CI signal (ticket #872). + const baseInfo = await h.checkBaseCurrency(task.pr); + if (!baseInfo.current) { + task.phase = 'REVIEW'; + task.retries = 0; + await updateMasterIssue(masterIssueNumber, state); + await github.rest.issues.createComment({ + owner: context.repo.owner, repo: context.repo.repo, issue_number: task.pr, + body: h.baseCurrencyComment(task.pr, baseInfo) + }); + core.error(`Human approved PR #${task.pr} but its branch is behind its base; refusing to merge.`); + return; + } task.phase = 'MERGE'; await mergePR(task.pr); state.completed.push(task.issue); diff --git a/.github/scripts/tests/orchestrator-helpers.test.js b/.github/scripts/tests/orchestrator-helpers.test.js index 3b05ce16b..f62e40ae7 100644 --- a/.github/scripts/tests/orchestrator-helpers.test.js +++ b/.github/scripts/tests/orchestrator-helpers.test.js @@ -320,3 +320,81 @@ test("parkForCI appends to a separate history array for the orchestrator shape", assert.strictEqual(history[0].event, "review_pending_ci"); assert.strictEqual(history[0].pr, 99); }); + +// --------------------------------------------------------------------------- +// #872: a PR branch cut before a later master merge must not be squash-merged, +// because applying the branch's whole tree reverts what master gained. +// --------------------------------------------------------------------------- + +function githubWithBase({ tip, mergeBase, lagCommits = [], failOn = null, head = "headsha" } = {}) { + const { github, calls } = createGithub({}); + github.rest.pulls.get = async () => ({ + data: { number: 99, head: { ref: "opencode/issue99-x", sha: head }, base: { ref: "master" } } + }); + github.rest.repos = { + getBranch: async (args) => { + calls.getBranch = args; + if (failOn === "getBranch") throw new Error("getBranch unavailable"); + return { data: { commit: { sha: tip || "tipsha" } } }; + }, + compare: async (args) => { + calls.compare = args; + if (failOn === "compare") throw new Error("compare unavailable"); + return { + data: { + merge_base_commit: { sha: mergeBase || "tipsha" }, + commits: lagCommits.map((c, i) => ({ sha: c.sha, commit: { message: c.subject } })) + } + }; + } + }; + const context = { repo: { owner: "LSantha", repo: "jnode_ai" } }; + return { h: createHelpers({ github, context, core: createCore() }), calls }; +} + +test("checkBaseCurrency: a branch containing the base tip is current", async () => { + const { h, calls } = githubWithBase({ tip: "tipsha", mergeBase: "tipsha", lagCommits: [] }); + const info = await h.checkBaseCurrency(99); + assert.strictEqual(info.current, true, "the branch contains the base tip"); + assert.strictEqual(info.base, "master"); + assert.strictEqual(info.lag.length, 0, "nothing would be reverted"); + assert.ok(calls.compare, "the merge-base is read from the compare API"); +}); + +test("checkBaseCurrency: a lagging branch is not current and names what it reverts", async () => { + const { h } = githubWithBase({ + tip: "tipsha", + mergeBase: "oldsha", + lagCommits: [ + { sha: "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa", subject: "Add JNodeOceanTheme" }, + { sha: "bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb", subject: "Fix FAT32 floor" } + ] + }); + const info = await h.checkBaseCurrency(99); + assert.strictEqual(info.current, false, "the branch is behind master"); + assert.strictEqual(info.lag.length, 2, "both commits would be reverted"); + assert.strictEqual(info.lag[0].sha, "aaaaaaaa", "the sha is shortened for the comment"); + assert.strictEqual(info.lag[0].subject, "Add JNodeOceanTheme", "the subject is the first line"); + const comment = h.baseCurrencyComment(99, info); + assert.ok(comment.includes("tipsha") === false && comment.includes("Add JNodeOceanTheme"), + "the refusal names the commits that would be reverted"); + assert.ok(comment.includes("Merge master into the branch"), "the refusal states the remedy"); +}); + +test("checkBaseCurrency: fail-closed on an unreadable base", async () => { + const { h } = githubWithBase({ failOn: "getBranch" }); + const info = await h.checkBaseCurrency(99); + assert.strictEqual(info.current, false, "an unreadable base must block the merge"); + assert.ok(h.baseCurrencyComment(99, info).includes("could not be read"), + "the refusal says why rather than naming nothing"); +}); + +test("checkBaseCurrency: fail-closed on an unreadable PR", async () => { + const { github } = createGithub({}); + github.rest.pulls.get = async () => { throw new Error("no PR"); }; + const context = { repo: { owner: "LSantha", repo: "jnode_ai" } }; + const h = createHelpers({ github, context, core: createCore() }); + const info = await h.checkBaseCurrency(99); + assert.strictEqual(info.current, false); + assert.strictEqual(info.lag.length, 0); +}); diff --git a/.github/scripts/tests/orchestrator.test.js b/.github/scripts/tests/orchestrator.test.js index 53a845dfa..231023960 100644 --- a/.github/scripts/tests/orchestrator.test.js +++ b/.github/scripts/tests/orchestrator.test.js @@ -21,7 +21,7 @@ function createMocks(eventName = 'issue_comment', commentBody = '/orchestrate') const updateIssueDetails = []; let masterIssueBody = `- [ ] #2\n- [ ] #3`; let currentTaskData = { labels: [], state: 'open' }; - let prData = { labels: [], state: 'open', merged: true, head: { ref: 'opencode/issue2-fix', sha: 'abc' } }; + let prData = { labels: [], state: 'open', merged: true, head: { ref: 'opencode/issue2-fix', sha: 'abc' }, base: { ref: 'master' } }; const github = { rest: { @@ -51,6 +51,13 @@ function createMocks(eventName = 'issue_comment', commentBody = '/orchestrate') return { data: [{ number: 1, labels: [{name: 'kind/orchestrator'}], state: 'open', body: masterIssueBody }] }; } }, + repos: { + // #872's merge gate reads the base tip and the head..base merge-base; + // the default models an up-to-date branch so the merge-path tests still + // reach mergePR. Tests exercising the stale-base gate override these. + getBranch: async () => ({ data: { commit: { sha: 'tipsha' } } }), + compare: async () => ({ data: { merge_base_commit: { sha: 'tipsha' }, commits: [] } }) + }, pulls: { list: async () => { return { data: [] }; // No PRs by default @@ -237,7 +244,7 @@ test('orchestrator.js test suite', async (t) => { setMasterBody(``); setTaskData({ labels: [{name: 'auto-merge'}], state: 'open' }); github.rest.issues.listComments = async () => ({ data: [{ body: 'Verdict: approve' }] }); - github.rest.pulls.get = async () => ({ data: { head: { ref: 'opencode/issue2-fix', sha: 'abc' }, merged: true } }); + github.rest.pulls.get = async () => ({ data: { head: { ref: 'opencode/issue2-fix', sha: 'abc' }, base: { ref: 'master' }, merged: true } }); github.rest.pulls.listFiles = async () => ({ data: [{ filename: 'fs/a.java', additions: 5 }] }); github.rest.checks = { listForRef: async () => ({ data: { check_runs: javaCiChecks('success') } }) }; @@ -256,7 +263,7 @@ test('orchestrator.js test suite', async (t) => { github.rest.issues.listComments = async () => ({ data: [{ body: 'Verdict: approve' }] }); // The merge call resolves with an empty body; the verification read still // reports an unmerged PR. - github.rest.pulls.get = async () => ({ data: { head: { ref: 'opencode/issue2-fix', sha: 'abc' }, state: 'open', merged: false } }); + github.rest.pulls.get = async () => ({ data: { head: { ref: 'opencode/issue2-fix', sha: 'abc' }, base: { ref: 'master' }, state: 'open', merged: false } }); github.rest.pulls.listFiles = async () => ({ data: [{ filename: 'fs/a.java', additions: 5 }] }); github.rest.checks = { listForRef: async () => ({ data: { check_runs: javaCiChecks('success') } }) }; @@ -283,7 +290,7 @@ test('orchestrator.js test suite', async (t) => { const { core, github, context, calls, setMasterBody } = createMocks('pull_request_review'); setMasterBody(``); - github.rest.pulls.get = async () => ({ data: { head: { ref: 'opencode/issue2-fix', sha: 'abc' }, state: 'open', merged: false } }); + github.rest.pulls.get = async () => ({ data: { head: { ref: 'opencode/issue2-fix', sha: 'abc' }, base: { ref: 'master' }, state: 'open', merged: false } }); await assert.rejects( runOrchestrator({ github, context, core }), @@ -305,7 +312,7 @@ test('orchestrator.js test suite', async (t) => { setMasterBody(``); setTaskData({ labels: [{name: 'auto-merge'}], state: 'open' }); github.rest.issues.listComments = async () => ({ data: [{ body: 'Verdict: approve' }] }); - github.rest.pulls.get = async () => ({ data: { head: { ref: 'opencode/issue2-fix', sha: 'abc' }, merged: true } }); + github.rest.pulls.get = async () => ({ data: { head: { ref: 'opencode/issue2-fix', sha: 'abc' }, base: { ref: 'master' }, merged: true } }); github.rest.pulls.listFiles = async () => ({ data: [{ filename: 'fs/a.java', additions: 5 }] }); github.rest.checks = { listForRef: async () => ({ data: { check_runs: javaCiChecks('failure') } }) }; @@ -321,7 +328,7 @@ test('orchestrator.js test suite', async (t) => { setMasterBody(``); setTaskData({ labels: [{name: 'auto-merge'}], state: 'open' }); github.rest.issues.listComments = async () => ({ data: [{ body: 'Verdict: approve' }] }); - github.rest.pulls.get = async () => ({ data: { head: { ref: 'opencode/issue2-fix', sha: 'abc' }, merged: true } }); + github.rest.pulls.get = async () => ({ data: { head: { ref: 'opencode/issue2-fix', sha: 'abc' }, base: { ref: 'master' }, merged: true } }); github.rest.pulls.listFiles = async () => ({ data: [{ filename: 'core/src/native/x86/kernel.asm', additions: 3 }] }); // CI defaults to green, so this isolates the diff gate. @@ -422,7 +429,7 @@ test('orchestrator.js test suite', async (t) => { setMasterBody(``); setTaskData({ labels: [{name: 'auto-merge'}], state: 'open' }); github.rest.issues.listComments = async () => ({ data: [{ body: 'Verdict: approve' }] }); - github.rest.pulls.get = async () => ({ data: { head: { ref: 'opencode/issue2-fix', sha: 'abc' }, merged: true } }); + github.rest.pulls.get = async () => ({ data: { head: { ref: 'opencode/issue2-fix', sha: 'abc' }, base: { ref: 'master' }, merged: true } }); github.rest.pulls.listFiles = async () => ({ data: [{ filename: 'fs/a.java', additions: 5 }] }); github.rest.checks = { listForRef: async () => ({ data: { check_runs: javaCiChecks('failure') } }) }; diff --git a/.github/scripts/tests/ticket-runner.test.js b/.github/scripts/tests/ticket-runner.test.js index d5f0d36a1..a0752c8ec 100644 --- a/.github/scripts/tests/ticket-runner.test.js +++ b/.github/scripts/tests/ticket-runner.test.js @@ -160,6 +160,15 @@ function createMocks(eventName, { }; } }, + repos: { + // #872's merge gate reads the base branch tip and the head..base + // merge-base; the default here models an up-to-date branch so existing + // merge-path tests still reach mergePR. + getBranch: async () => ({ data: { commit: { sha: 'tipsha' } } }), + compare: async () => ({ + data: { merge_base_commit: { sha: 'tipsha' }, commits: [] } + }) + }, pulls: { listReviews: async () => ({ data: [] }), list: async () => { @@ -180,6 +189,7 @@ function createMocks(eventName, { data: { number: pull_number, head: currentPRHead, + base: { ref: "master" }, body: currentPRBody, state: currentPRMerged ? "closed" : "open", merged: currentPRMerged @@ -1424,6 +1434,68 @@ test("ticket-runner.js event handling suite", async (t) => { assert.ok(mocks.calls.createComment.some(c => c.issue_number === 99 && c.body.includes("/oc fix Address human review feedback."))); }); + await t.test("/oc approve is refused when the PR branch is behind master (#872)", async () => { + const initialBody = _replaceOrAppendStatus("Task", { + phase: "HUMAN_REVIEW", + pr: 99, + turn: 1, + max_turns: 3, + retries: 0, + started: new Date().toISOString(), + history: [] + }, 42); + + const mocks = createMocks("issue_comment", { + commentBody: "/oc approve", + commentOnPR: true, + issueBody: initialBody + }); + // A branch that does not contain master's tip: the squash would revert it. + mocks.github.rest.repos.getBranch = async () => ({ data: { commit: { sha: "tipsha" } } }); + mocks.github.rest.repos.compare = async () => ({ + data: { + merge_base_commit: { sha: "oldsha" }, + commits: [{ sha: "cafebabecafebabecafebabecafebabecafebabe", commit: { message: "Add JNodeOceanTheme" } }] + } + }); + + await runTicketRunner(mocks); + + assert.strictEqual(mocks.calls.mergePR.length, 0, "a stale branch must not be merged"); + assert.ok(mocks.calls.createComment.some(c => c.issue_number === 99 && + (c.body || "").includes("Add JNodeOceanTheme") && (c.body || "").includes("Merge master into the branch")), + "the refusal names the commits that would be reverted and the remedy"); + assert.ok(mocks.getIssueBody().includes('"phase": "REVIEW"'), + "the ticket falls back to REVIEW so a later approval can retry"); + }); + + await t.test("/oc approve --force bypasses the stale-base check (#872)", async () => { + const initialBody = _replaceOrAppendStatus("Task", { + phase: "HUMAN_REVIEW", + pr: 99, + turn: 1, + max_turns: 3, + retries: 0, + started: new Date().toISOString(), + history: [] + }, 42); + + const mocks = createMocks("issue_comment", { + commentBody: "/oc approve --force", + commentOnPR: true, + issueBody: initialBody + }); + mocks.github.rest.repos.getBranch = async () => ({ data: { commit: { sha: "tipsha" } } }); + mocks.github.rest.repos.compare = async () => ({ + data: { merge_base_commit: { sha: "oldsha" }, commits: [] } + }); + + await runTicketRunner(mocks); + + assert.strictEqual(mocks.calls.mergePR.length, 1, + "--force is the deliberate emergency override, as it is for the CI check"); + }); + await t.test("/oc approve on a HUMAN_REVIEW PR merges and closes the issue", async () => { const initialBody = _replaceOrAppendStatus("Task", { phase: "HUMAN_REVIEW", diff --git a/.github/scripts/ticket-runner.js b/.github/scripts/ticket-runner.js index b7bb19d67..03fd00a37 100644 --- a/.github/scripts/ticket-runner.js +++ b/.github/scripts/ticket-runner.js @@ -814,6 +814,24 @@ module.exports = async ({ github, context, core }) => { core.info("Ticket runner: approval on PR #" + prNumber + " but CI is red; deferring merge."); return; } + // A stale branch must not reach the squash: applying the branch's whole tree + // would revert the commits master gained since the branch was cut, with no + // CI signal (ticket #872). --force is the deliberate emergency override, so + // it skips this check exactly as it skips the CI re-check. Same + // honest-refusal shape as the CI check above. + var baseInfo = force ? { current: true, base: "", lag: [] } : await h.checkBaseCurrency(prNumber); + if (!baseInfo.current) { + state.phase = "REVIEW"; + await updateIssueState(issueNumber, state); + await github.rest.issues.createComment({ + owner, repo, issue_number: prNumber, + body: h.baseCurrencyComment(prNumber, baseInfo) + + "\n\nApproval is recorded; it is not lost. Re-request it once the branch is current." + }); + core.info("Ticket runner: PR #" + prNumber + " is behind its base (" + + baseInfo.lag.length + " commits); refusing to merge."); + return; + } state.phase = "MERGE"; state.history.push({ event: force ? "human_approved_forced" : "human_approved", From ded4aa34ac36cd0fe4d61ebd129485aafb059635 Mon Sep 17 00:00:00 2001 From: Levente Santha Date: Sun, 8 Nov 2026 23:53:28 +0200 Subject: [PATCH 5/5] Address review on #872: orchestrator tests, honest wording, and --force semantics Inkling's round-1 request-changes found four real gaps, all fixed here. 1. The two new orchestrator.js branches had no tests - they were also where the HUMAN_REVIEW-vs-REVIEW divergence lives, the highest regression risk in the diff. Both are now pinned: auto-merge on a stale base routes to HUMAN_REVIEW and merges nothing; a human approval on a stale base refuses and falls back to REVIEW. 2. "Approval is recorded; it is not lost" was false in the common case: GitHub normally dismisses the approval when the branch is updated, so the ticket needs a fresh one. The line is gone. 3. The shared remedy line said "re-request the review", which does nothing for the HUMAN_REVIEW entry point (no CI wakeup re-reviews it). baseCurrencyComment now takes the entry point and says "approve again on the PR Review UI" for the human path, "re-request the review" for the review path. 4. Override asymmetry, and the deeper question of whether --force should bypass this gate at all: it should not. --force exists for a red-CI PR, where the risk is local to the PR; a stale base is a master-side data loss, and the ticket's acceptance criteria say "cannot be merged by either runner". The /oc approve --force test is inverted to assert the refusal, and the escape hatch is documented as GitHub's UI merge button - a human's explicit act, which already bypasses every gate. Also from the review: the stale path now records a merge_refused_base_stale history event and resets retries (matching the CI-red path it models itself on); the orchestrator computes the base check only once CI is green, so a red-CI PR is parked before spending the calls; and the printed lag count uses the compare API's total_commits, since compare paginates at 250. Suite: 296/296 in .github/scripts/tests (was 294), no regressions. Closes #872 Assisted-by: stepfun-ai/step-5-preview --- .github/scripts/orchestrator-helpers.js | 34 +++++++++-- .github/scripts/orchestrator.js | 13 ++++- .../tests/orchestrator-helpers.test.js | 12 ++-- .github/scripts/tests/orchestrator.test.js | 56 +++++++++++++++++++ .github/scripts/tests/ticket-runner.test.js | 14 ++++- .github/scripts/ticket-runner.js | 24 +++++--- 6 files changed, 129 insertions(+), 24 deletions(-) diff --git a/.github/scripts/orchestrator-helpers.js b/.github/scripts/orchestrator-helpers.js index d800d5f0e..6a8ab5330 100644 --- a/.github/scripts/orchestrator-helpers.js +++ b/.github/scripts/orchestrator-helpers.js @@ -332,6 +332,13 @@ module.exports = function createHelpers({ github, context, core }) { var message = (c.commit && c.commit.message) || ""; lag.push({ sha: (c.sha || "").slice(0, 8), subject: message.split("\n")[0] }); } + // compare paginates at 250 commits; total_commits is the exact count, so + // the caller's "N commits" is honest even on a very lagging branch. + if (typeof compare.data.total_commits === "number" && + compare.data.total_commits !== lag.length) { + lag.push({ sha: "", subject: "... and " + + (compare.data.total_commits - (lag.length - 1)) + " more" }); + } return { current: mergeBase === tip, base: baseRef, lag: lag }; } catch (e) { core.warning("checkBaseCurrency: compare failed for PR #" + prNumber + ": " + e.message); @@ -339,19 +346,34 @@ module.exports = function createHelpers({ github, context, core }) { } } - /** Human-readable refusal naming the commits a squash would revert. */ - function baseCurrencyComment(prNumber, info) { + /** + * Human-readable refusal naming the commits a squash would revert. + * + * @param prNumber the PR + * @param info the checkBaseCurrency result + * @param entry how the merge was requested - "review" (an agent review the + * ticket-runner re-posts) or "human" (a native GitHub approval). The remedy + * line differs: re-requesting a review does nothing for a human approval, + * which must be given again on the updated branch. + */ + function baseCurrencyComment(prNumber, info, entry) { + var base = info.base || "master"; var lines = [ - "Not merging: PR #" + prNumber + "'s branch is behind " + (info.base || "master") + + "Not merging: PR #" + prNumber + "'s branch is behind " + base + ", so a squash would revert those commits." ]; if (info.lag && info.lag.length) { lines.push(""); - for (var i = 0; i < info.lag.length && i < 10; i++) { - lines.push("- `" + info.lag[i].sha + "` " + info.lag[i].subject); + for (var i = 0; i < info.lag.length && i < 11; i++) { + var item = info.lag[i]; + lines.push(item.sha ? "- `" + item.sha + "` " + item.subject : "- " + item.subject); } lines.push(""); - lines.push("Merge " + (info.base || "master") + " into the branch and re-request the review - the diff must then contain only this PR's own changes."); + lines.push("Merge " + base + " into the branch (or re-push it on a current base), then " + + (entry === "human" + ? "approve again on the GitHub PR Review UI - the approval above does not carry over." + : "re-request the review.") + + " The diff must then contain only this PR's own changes."); } else { lines.push("The base branch state could not be read, so the merge is refused rather than guessed at."); } diff --git a/.github/scripts/orchestrator.js b/.github/scripts/orchestrator.js index f7a3bce3e..7f3be87d1 100644 --- a/.github/scripts/orchestrator.js +++ b/.github/scripts/orchestrator.js @@ -601,8 +601,11 @@ module.exports = async ({ github, context, core }) => { // in HUMAN_REVIEW does not. const diffSafe = await h.isDiffSafe(task.pr); const ciGreen = await h.isCIGreen(task.pr); - const baseInfo = await h.checkBaseCurrency(task.pr); const forbidden = diffSafe ? false : await h.touchesForbiddenPath(task.pr); + // Only once CI is green: the base check costs API calls and + // a red-CI PR must be parked before anything else. + const baseInfo = ciGreen ? await h.checkBaseCurrency(task.pr) + : { current: true, base: "", lag: [] }; if (!ciGreen) { h.parkForCI(task, task.pr, { event: 'merge_deferred', @@ -620,9 +623,11 @@ module.exports = async ({ github, context, core }) => { // cut, with no CI signal (ticket #872). task.phase = 'HUMAN_REVIEW'; task.retries = 0; + state.history.push({ event: 'merge_refused_base_stale', task: task.issue, + pr: task.pr, commits: baseInfo.lag.length, timestamp: new Date().toISOString() }); await github.rest.issues.createComment({ owner: context.repo.owner, repo: context.repo.repo, issue_number: task.pr, - body: h.baseCurrencyComment(task.pr, baseInfo) + body: h.baseCurrencyComment(task.pr, baseInfo, "human") }); core.error(`Task #${task.issue} PR #${task.pr} is behind its base; routing to HUMAN_REVIEW.`); } else if (forbidden) { @@ -809,10 +814,12 @@ module.exports = async ({ github, context, core }) => { if (!baseInfo.current) { task.phase = 'REVIEW'; task.retries = 0; + state.history.push({ event: 'merge_refused_base_stale', task: task.issue, + pr: task.pr, commits: baseInfo.lag.length, timestamp: new Date().toISOString() }); await updateMasterIssue(masterIssueNumber, state); await github.rest.issues.createComment({ owner: context.repo.owner, repo: context.repo.repo, issue_number: task.pr, - body: h.baseCurrencyComment(task.pr, baseInfo) + body: h.baseCurrencyComment(task.pr, baseInfo, "human") }); core.error(`Human approved PR #${task.pr} but its branch is behind its base; refusing to merge.`); return; diff --git a/.github/scripts/tests/orchestrator-helpers.test.js b/.github/scripts/tests/orchestrator-helpers.test.js index f62e40ae7..f63c29697 100644 --- a/.github/scripts/tests/orchestrator-helpers.test.js +++ b/.github/scripts/tests/orchestrator-helpers.test.js @@ -375,17 +375,21 @@ test("checkBaseCurrency: a lagging branch is not current and names what it rever assert.strictEqual(info.lag.length, 2, "both commits would be reverted"); assert.strictEqual(info.lag[0].sha, "aaaaaaaa", "the sha is shortened for the comment"); assert.strictEqual(info.lag[0].subject, "Add JNodeOceanTheme", "the subject is the first line"); - const comment = h.baseCurrencyComment(99, info); - assert.ok(comment.includes("tipsha") === false && comment.includes("Add JNodeOceanTheme"), + const comment = h.baseCurrencyComment(99, info, "review"); + assert.ok(comment.includes("Add JNodeOceanTheme"), "the refusal names the commits that would be reverted"); - assert.ok(comment.includes("Merge master into the branch"), "the refusal states the remedy"); + assert.ok(comment.includes("Merge master into the branch") && comment.includes("re-request the review"), + "the review-entry remedy says to update and re-review"); + const human = h.baseCurrencyComment(99, info, "human"); + assert.ok(human.includes("approve again") && !human.includes("re-request the review"), + "the human-entry remedy says to approve again, not re-review"); }); test("checkBaseCurrency: fail-closed on an unreadable base", async () => { const { h } = githubWithBase({ failOn: "getBranch" }); const info = await h.checkBaseCurrency(99); assert.strictEqual(info.current, false, "an unreadable base must block the merge"); - assert.ok(h.baseCurrencyComment(99, info).includes("could not be read"), + assert.ok(h.baseCurrencyComment(99, info, "review").includes("could not be read"), "the refusal says why rather than naming nothing"); }); diff --git a/.github/scripts/tests/orchestrator.test.js b/.github/scripts/tests/orchestrator.test.js index 231023960..8b3979b77 100644 --- a/.github/scripts/tests/orchestrator.test.js +++ b/.github/scripts/tests/orchestrator.test.js @@ -238,6 +238,62 @@ test('orchestrator.js test suite', async (t) => { assert.ok(calls.updateIssue.includes(1), 'Master issue was updated'); }); + await t.test('Review approve with auto-merge refuses a branch behind master (#872)', async () => { + const { core, github, context, calls, updateIssueDetails, setMasterBody, setTaskData } = createMocks('workflow_run'); + + setMasterBody(``); + setTaskData({ labels: [{name: 'auto-merge'}], state: 'open' }); + github.rest.issues.listComments = async () => ({ data: [{ body: 'Verdict: approve' }] }); + github.rest.pulls.get = async () => ({ data: { head: { ref: 'opencode/issue2-fix', sha: 'abc' }, base: { ref: 'master' }, merged: true } }); + github.rest.pulls.listFiles = async () => ({ data: [{ filename: 'fs/a.java', additions: 5 }] }); + github.rest.checks = { listForRef: async () => ({ data: { check_runs: javaCiChecks('success') } }) }; + // A branch that does not contain the base tip. + github.rest.repos.getBranch = async () => ({ data: { commit: { sha: 'tipsha' } } }); + github.rest.repos.compare = async () => ({ + data: { + merge_base_commit: { sha: 'oldsha' }, + total_commits: 3, + commits: [{ sha: 'cafebabecafebabecafebabecafebabecafebabe', commit: { message: 'Add JNodeOceanTheme' } }] + } + }); + + await runOrchestrator({ github, context, core }); + + assert.strictEqual(calls.mergePR.length, 0, "a stale branch must not be squash-merged"); + assert.ok(calls.createComment.some(c => c.issue_number === 99 && + c.body.includes('Add JNodeOceanTheme') && c.body.includes('approve again')), + "the refusal names the would-be-reverted commits and the human-approval remedy"); + assert.ok(updateIssueDetails.some(u => u.issue_number === 1 && + u.body.includes('"phase": "HUMAN_REVIEW"')), "the task routes to HUMAN_REVIEW"); + assert.ok(updateIssueDetails.some(u => u.issue_number === 1 && + u.body.includes('merge_refused_base_stale')), "the refusal is in the master history"); + }); + + await t.test('Human approval refuses a branch behind master (#872)', async () => { + const { core, github, context, calls, updateIssueDetails, setMasterBody, setTaskData } = createMocks('pull_request_review'); + + setMasterBody(``); + setTaskData({ labels: [], state: 'open' }); + github.rest.pulls.get = async () => ({ data: { head: { ref: 'opencode/issue2-fix', sha: 'abc' }, base: { ref: 'master' }, merged: true } }); + github.rest.pulls.listFiles = async () => ({ data: [{ filename: 'fs/a.java', additions: 5 }] }); + github.rest.checks = { listForRef: async () => ({ data: { check_runs: javaCiChecks('success') } }) }; + github.rest.repos.getBranch = async () => ({ data: { commit: { sha: 'tipsha' } } }); + github.rest.repos.compare = async () => ({ + data: { + merge_base_commit: { sha: 'oldsha' }, + commits: [{ sha: 'cafebabecafebabecafebabecafebabecafebabe', commit: { message: 'Add JNodeOceanTheme' } }] + } + }); + + await runOrchestrator({ github, context, core }); + + assert.strictEqual(calls.mergePR.length, 0, "a human approval must not merge a stale branch"); + assert.ok(calls.createComment.some(c => c.issue_number === 99 && + c.body.includes('approve again')), "the refusal says the approval does not carry over"); + assert.ok(updateIssueDetails.some(u => u.issue_number === 1 && + u.body.includes('"phase": "REVIEW"')), "the task falls back to REVIEW"); + }); + await t.test('Review approve with auto-merge and green CI merges and advances', async () => { const { core, github, context, calls, updateIssueDetails, setMasterBody, setTaskData } = createMocks('workflow_run'); diff --git a/.github/scripts/tests/ticket-runner.test.js b/.github/scripts/tests/ticket-runner.test.js index a0752c8ec..b0cac912b 100644 --- a/.github/scripts/tests/ticket-runner.test.js +++ b/.github/scripts/tests/ticket-runner.test.js @@ -1467,9 +1467,15 @@ test("ticket-runner.js event handling suite", async (t) => { "the refusal names the commits that would be reverted and the remedy"); assert.ok(mocks.getIssueBody().includes('"phase": "REVIEW"'), "the ticket falls back to REVIEW so a later approval can retry"); + assert.ok(mocks.getIssueBody().includes("merge_refused_base_stale"), + "the refusal is recorded in the ticket history"); }); - await t.test("/oc approve --force bypasses the stale-base check (#872)", async () => { + await t.test("/oc approve --force still refuses a stale base (#872)", async () => { + // --force exists for a red-CI PR, where the risk is local to the PR. A + // stale base is a master-side data loss, so the ticket's acceptance + // criteria ("cannot be merged by either runner") win: the escape hatch is + // the GitHub UI merge button, a human's explicit act. const initialBody = _replaceOrAppendStatus("Task", { phase: "HUMAN_REVIEW", pr: 99, @@ -1492,8 +1498,10 @@ test("ticket-runner.js event handling suite", async (t) => { await runTicketRunner(mocks); - assert.strictEqual(mocks.calls.mergePR.length, 1, - "--force is the deliberate emergency override, as it is for the CI check"); + assert.strictEqual(mocks.calls.mergePR.length, 0, + "--force must not merge a branch that would revert master commits"); + assert.ok(mocks.getIssueBody().includes('"phase": "REVIEW"'), + "the ticket falls back to REVIEW"); }); await t.test("/oc approve on a HUMAN_REVIEW PR merges and closes the issue", async () => { diff --git a/.github/scripts/ticket-runner.js b/.github/scripts/ticket-runner.js index 03fd00a37..1ef699d8a 100644 --- a/.github/scripts/ticket-runner.js +++ b/.github/scripts/ticket-runner.js @@ -814,19 +814,27 @@ module.exports = async ({ github, context, core }) => { core.info("Ticket runner: approval on PR #" + prNumber + " but CI is red; deferring merge."); return; } - // A stale branch must not reach the squash: applying the branch's whole tree - // would revert the commits master gained since the branch was cut, with no - // CI signal (ticket #872). --force is the deliberate emergency override, so - // it skips this check exactly as it skips the CI re-check. Same - // honest-refusal shape as the CI check above. - var baseInfo = force ? { current: true, base: "", lag: [] } : await h.checkBaseCurrency(prNumber); + // A stale branch must not reach the squash, even under --force: the squash + // applies the branch's whole tree, so it would revert the commits master + // gained since the branch was cut - a master-side data loss, unlike the + // red-CI case --force exists for, where the risk is local to the PR. The + // ticket's acceptance criteria are "cannot be merged by either runner", so + // the escape hatch is GitHub's UI merge button (which bypasses every gate + // and is a human's explicit act), not this command. + var baseInfo = await h.checkBaseCurrency(prNumber); if (!baseInfo.current) { state.phase = "REVIEW"; + state.retries = 0; + state.history.push({ + event: "merge_refused_base_stale", + pr: prNumber, + commits: baseInfo.lag.length, + timestamp: new Date().toISOString() + }); await updateIssueState(issueNumber, state); await github.rest.issues.createComment({ owner, repo, issue_number: prNumber, - body: h.baseCurrencyComment(prNumber, baseInfo) + - "\n\nApproval is recorded; it is not lost. Re-request it once the branch is current." + body: h.baseCurrencyComment(prNumber, baseInfo, "human") }); core.info("Ticket runner: PR #" + prNumber + " is behind its base (" + baseInfo.lag.length + " commits); refusing to merge.");