Repository navigation
Conversation
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
… 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
…duced 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
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
|
/oc review |
LSantha
added
the
agent/failed
|
/oc review |
| 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) { |
There was a problem hiding this comment.
🟡 Missing pin for the new branches: nothing in tests/orchestrator.test.js models a lagging base. The mocks got repos.getBranch/compare defaults that make the pre-existing merge-path tests green, but no test overrides them to prove the refusals — (a) agent-approve + auto-merge eligible + stale ⇒ no mergePR call, phase HUMAN_REVIEW, comment posted (this branch), and (b) human-approval + stale ⇒ no mergePR call, phase REVIEW (orchestrator.js:809). ticket-runner.test.js got the equivalent pair; this suite is where the HUMAN_REVIEW-vs-REVIEW divergence lives, so a regression here would silently restore the old merge-on-stale behavior. Mirror the two ticket-runner tests, or link a follow-up issue.
| // 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) { |
There was a problem hiding this comment.
🟡 Override asymmetry: ticket-runner has /oc approve --force to bypass this gate; the orchestrator human-approval path refuses unconditionally — there is no way, even for an owner who has verified the branch is stale-but-safe or who is unblocking a parked task, to get the merge through this runner. The only escape hatch is the GitHub merge button, which bypasses every gate (CI, diff budget, forbidden paths) rather than just this one. If deliberate, the in-code comment should say so ("UI is the escape hatch") and the PR description should state it; if not, mirror an override or document why orchestrator tasks cannot have one. Ticket-runner tasks and orchestrator tasks go through the same approve flow, so the asymmetry will read as a bug to whoever hits it next.
| 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." |
There was a problem hiding this comment.
🟡 "Approval is recorded; it is not lost." — misleading in the common case. After this refusal the ticket leaves HUMAN_REVIEW: the branch update pushes new commits (GitHub dismisses the stale approval), a green CI must re-enter REVIEW, the agent must re-review, and for non-auto-merge tickets the human must approve again before anything merges. The original approval buys nothing. Please reword, e.g. "This approval does not merge the PR while the branch is behind its base; update the branch and approve again once it is current."
| 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."); |
There was a problem hiding this comment.
🟡 "re-request the review" is only the right instruction for the REVIEW entry points (ticket-runner and the orchestrator human-approval path, where the Java CI green wake re-requests the agent review). The other entry point — the orchestrator auto-merge path — routes the task to HUMAN_REVIEW, where an agent verdict is ignored and the CI green wake does not fire (handleJavaCICompletion requires REVIEW). The working instruction there is "merge master into the branch, wait for CI, approve again". Either parameterize the remedy line by entry point, or drop "re-request the review" from the shared text so it is correct for both.
| // 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); |
There was a problem hiding this comment.
🟡 The --force bypass skips this gate entirely — the one that exists to protect master from another agent's merged work. Note that force's original documented role was "deliberate emergency override" for red CI, i.e., risk to this PR; here the risk is to work that already landed on master. If --force must reach the merge, consider at least a loud history event (force_stale_base_merge) so the timeline shows a human deliberately overriding the master-protection gate — and confirm that is really intended versus requiring the human to update the branch first. Not a blocker, but the PR should state the choice explicitly.
| // 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"; |
There was a problem hiding this comment.
🟢 The sibling CI-red path records a history event (human_approved_ci_red via parkForCI) so the ticket's timeline shows why it bounced back to REVIEW; this refusal pushes no history entry and leaves state.retries untouched. A human_approved_base_stale event (and retries = 0, mirroring the CI-red path) would keep the timeline auditable and match the shape of the path below you called out as the template. Non-blocking.
|
/oc review You are reviewing a pull request for the automated pipeline. Your final line must be exactly one of: Post this verdict as an ordinary COMMENT on the pull request (a native GitHub review is not usable here: the PR is authored by the maintainer, so GitHub blocks self-approval). Immediately after the Verdict line, add one more line naming the model that produced this review, copied verbatim from the model your runtime reports (never invented): A reviewer makes no commit, so this comment is the only place its model can be recorded; mergePR folds it into the merge commit as Reviewed-by. Use "Verdict: request-changes" if the PR needs code changes. Use "Verdict: approve" only if the PR is correct and ready for the next phase. |
| // 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); |
There was a problem hiding this comment.
🟢 baseInfo is computed up front, so the red-CI path below pays for a checkBaseCurrency round trip it never uses (it parks on CI as before). The human-approval path in this file (orchestrator.js:808) computes it after the CI check — same ordering as ticket-runner. Moving the call below the !ciGreen branch keeps the wasted-work shape consistent. Cosmetic.
| ]; | ||
| if (info.lag && info.lag.length) { | ||
| lines.push(""); | ||
| for (var i = 0; i < info.lag.length && i < 10; i++) { |
There was a problem hiding this comment.
🟢 Detail for the lag list/count: the compare API caps commits at 250, and the response carries total_commits for the exact figure (docs: "the returned list is limited to 250 commits" without paging). lag.length is printed verbatim as the commit count in ticket-runner.js:832 — a >250-commit lag would report "250 commits" and understate it. Prefer compare.data.total_commits when present (falling back to lag.length), or cap the count display the way the list is already capped at 10. Cosmetic.
🤖 Review — PR #883 (checkBaseCurrency gate)What this is: a pre-squash refusal for branches cut before a later master merge, wired into both runners' approval paths plus the orchestrator auto-merge path. The direction is right: fail-closed, matches the isDiffSafe/isCIGreen honest-refusal pattern, helper is well-isolated, and I verified the suite is green locally (294/294, up from 292, no regressions) and that a real compare-API call against this repo accepts a head SHA as base — the merge_base === tip computation is sound and the lag list it produces is exactly the commits on base not on head. One fact-check on the record: the failure mode itself is real (multiple April 2026 incident write-ups describe a squash from a stale branch silently overwriting base-side changes, and the repo's own #872 documents #869/#877), so the gate is justified even though GitHub's public docs read the other way. Not disputing it. Findings, in priority order:
The 🟡 items are small, in-place fixes; none question the design. Verdict: request-changes — the missing orchestrator tests (1) and the misleading operator instructions (2, 3) need to land before this becomes the factory's merge gate. Happy to re-review quickly. Verdict: request-changes |
LSantha
left a comment
LSantha
left a comment
There was a problem hiding this comment.
Verdict: request-changes (submitted as a comment because GitHub refuses a request-changes review on a PR authored by the same account). Full findings and the summary are in my summary comment; inline threads are on the diff.
|
Review posted on PR #883 (7 inline comments + summary;
|
LSantha
added
agent/done
|
/oc fix Address review feedback. |
🤖 Re-review — PR #883 (checkBaseCurrency gate)Re-checking after the request for review. The branch is unchanged since the last review: head is Still blocking (the 🟡 items from the 21:45 review):
Still open (🟢, non-blocking): no The design and the fail-closed direction remain sound; this is the same list as before, unaddressed. Verdict: request-changes |
|
Posted the re-review on PR #883: the branch is unchanged since the 21:45 review (head |
|
/oc review You are reviewing a pull request for the automated pipeline. Your final line must be exactly one of: Post this verdict as an ordinary COMMENT on the pull request (a native GitHub review is not usable here: the PR is authored by the maintainer, so GitHub blocks self-approval). Immediately after the Verdict line, add one more line naming the model that produced this review, copied verbatim from the model your runtime reports (never invented): A reviewer makes no commit, so this comment is the only place its model can be recorded; mergePR folds it into the merge commit as Reviewed-by. Use "Verdict: request-changes" if the PR needs code changes. Use "Verdict: approve" only if the PR is correct and ready for the next phase. |
…ce 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
|
Fixed all review findings on #883: parameterized stale-base remedy per entry point, reworded approval line, added history/retries, |
Assisted-by: opencode/crouton-alpha-free
|
/oc fix Address review feedback. |
43fb389 to
c4dc4e2
Compare
🤖 Re-review #3 — PR #883 (checkBaseCurrency gate)Re-reviewed at head
Verified locally and live:
Nits, none blocking: "Showing first 10" lists the oldest 10 lagging commits (compare returns chronological order) — accurate as written; the compare-then-merge window has an inherent TOCTOU race (tip read before merge), as does any pre-merge check; "approve again" slightly over-generalizes for auto-merge-eligible tasks (the re-review + auto-merge would take over without human action) — acceptable shorthand, not misleading. Design remains sound: fail-closed, honest refusal, both runners wired, the divergence (HUMAN_REVIEW vs REVIEW) is pinned by tests. Ready for the next phase; the merge gate itself re-checks CI green at merge time (currently pending). Verdict: approve |
|
Review posted with a fresh re-verification at head |
|
All review findings for PR #883 are already addressed by commit
Tests: |
|
Agent review passed. Awaiting human approval via native GitHub PR Review UI, or |
Summary
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. Neither runner had any check for this.
The evidence, four times tonight
+270lines) plus four desktop files; caught by hand before merge, resolved by merging master in./octrigger; dedupe deliveries. #855 — open right now, already 3087 lines behind: they'd delete the theme, the floor, the counter, the copy-comparison and formatter-round-trip suites. Closed without review.Nothing signals the loss: the reverted files still compile, so build/test/boot stay green. It only surfaces when the feature is missed later.
The fix — a pre-squash refusal, fail-closed
checkBaseCurrency(prNumber)compares the head..base merge-base with the base branch tip and returns{current, base, lag}, wherelaglists the commits on the base but not on the head — the ones the squash would revert. Any unreadable state reads as not current, the same direction asisDiffSafe/isCIGreen, because a merge is unrecoverable while a refusal is not.Wired as the repo's existing "honest refusal" pattern:
approveAndMerge): refuses before the MERGE phase, returns to REVIEW so a later approval retries, posts the comment.--forcebypasses it exactly as it already bypasses the CI re-check.Both comments name the lagging commits and the remedy ("Merge master into the branch and re-request the review").
Tests
New: 4 helper tests (current branch / lagging branch with commits named / fail-closed on an unreadable base / fail-closed on an unreadable PR) and 2 runner tests (refusal names the commits and falls back to REVIEW;
--forcebypasses).The existing merge-path tests in both suites had to model the new dependency —
pulls.getnow returnsbase.ref, and both github mocks carryrepos.getBranch/repos.comparedefaulting to a current branch so they still reachmergePR. 294/294 in.github/scripts/tests, up from 292, no regressions (verified the failures were mine by stashing).Closes #872