fix: address Kimi review findings on PR #594

- nightly-staleness.yml: compare against the `nightly` tag's target commit date, not the release object's published_at/created_at. release.yml updates the release in place (gh release edit --clobber), so those timestamps are stamped once and never move -- the watchdog would have started crying wolf on every scheduled run ~24h after this merged. The tag ref IS force-moved on every publish, so it's the correct signal. - README.md: Team Tap and Pro Controller were still marked "Not emulated" in the peripheral table, contradicting this release. Fixed both, added the 6D flight stick and paddle-ADC rows the table was missing entirely, and named all three in the peripherals intro paragraph. - docs/RELEASE_NOTES_v3.4.0.md: added a "(Corrected 2026-08-19: ...)" marker where a prior docs-only fix (f2d8457) silently changed the notes' claimed $F17C00 return value from 0x0000 to 0xFF. Verified the shipped v3.4.0 code always returned 0xFF (the fixing commit predates the v3.4.0 tag) -- the correction was accurate, it just lacked a transparency marker on a tagged release's record. - jaguar-toolchain-image.yml: workflow_dispatch had no branch guard, so a manual dispatch from any branch could publish :latest -- contradicting the file's own stated invariant that only develop ever does. Added `if: github.ref == 'refs/heads/develop'` on the job. - kimi_review.py: large PRs were truncated by raw character count on a `git diff` whose file order is roughly path-alphabetical, so a PR with many docs/.github/Makefile changes (which sort before src/) could lose every src/ change to the budget before the model ever saw one -- exactly what happened reviewing this PR. Diff is now built file-by-file in priority order (src/**/*.c and *.h first, then other sources, then tests, then docs/CI/build last), grouped by directory so a changed .c lands next to its changed .h and next to its other changed same-folder siblings, and accumulated against the budget per-file so truncation always falls on a clean file boundary instead of mid-hunk. The PR note and the prompt sent to the model both now name which files got cut instead of just saying "large PR".
2026-11-07 04:36:00 -10:00 · 2026-08-25 15:21:52 -04:00
1 parent eaa77f1fe2
commit b466c0372f
@@ -81,14 +81,75 @@ def sh(args):
    return subprocess.run(args, capture_output=True, text=True).stdout


+def _source_priority(path):
+    """Lower sorts first. Actual C source/headers are what the system prompt's
+    house rules (C89, endianness, savestate layout, audio pair) are about --
+    on a PR too large to send whole, those are what must survive truncation,
+    not whatever a plain `git diff --name-only` (roughly path-alphabetical)
+    happens to list first. docs/.github/Makefile/dist sort ahead of src/ and
+    test/ purely alphabetically, which is exactly backwards for review value.
+    """
+    if path.startswith("src/") and (path.endswith(".c") or path.endswith(".h")):
+        return 0
+    if path.endswith(".c") or path.endswith(".h"):
+        return 1  # top-level sources, e.g. libretro.c
+    if path.startswith("test/"):
+        return 2
+    return 3  # docs, CI, build metadata, everything else
+
+
+def _order_files(names):
+    """Priority tier first; within a tier, directory then basename, so a
+    changed foo.c lands next to a changed foo.h (same basename) and next to
+    its other changed same-directory siblings -- a truncation cutoff drops
+    whole trailing directories instead of splitting one file's diff from its
+    header or scattering a module's changes across the cut line.
+    """
+    def key(n):
+        d, base = n.rsplit("/", 1) if "/" in n else ("", n)
+        stem = base.rsplit(".", 1)[0]
+        return (_source_priority(n), d, stem, base)
+    return sorted(names, key=key)
+
+
 def filtered_diff(base, head):
-    """Diff with generated/vendored paths dropped, so the model reads signal."""
+    """Diff with generated/vendored paths dropped, so the model reads signal.
+
+    Built one file at a time in priority order (source > tests > docs/CI) and
+    accumulated against MAX_DIFF_CHARS, rather than one `git diff` call
+    truncated by raw character count -- that used to cut wherever git's
+    path-alphabetical file order happened to land, which for a large PR meant
+    dropping every src/ change in favor of whatever docs/.github/Makefile
+    changes sorted first alphabetically. A file's diff is included whole or
+    not at all -- never sliced mid-hunk -- so a truncated review always ends
+    on a clean file boundary.
+
+    Returns (diff_text, skipped, omitted): `skipped` is generated/vendored
+    paths excluded on principle (SKIP_PREFIXES); `omitted` is files that would
+    have been reviewed but didn't fit the budget. Kept separate so the PR note
+    can say "N generated files excluded" and "budget cut before file X" as two
+    different, both useful, facts instead of one merged count.
+    """
    names = sh(["git", "diff", "--name-only", f"{base}...{head}"]).split()
    keep = [n for n in names if not n.startswith(SKIP_PREFIXES)]
-    dropped = len(names) - len(keep)
+    skipped = [n for n in names if n.startswith(SKIP_PREFIXES)]
    if not keep:
-        return "", dropped
-    return sh(["git", "diff", f"{base}...{head}", "--"] + keep), dropped
+        return "", skipped, []
+
+    budget = MAX_DIFF_CHARS
+    parts = []
+    omitted = []
+    for name in _order_files(keep):
+        file_diff = sh(["git", "diff", f"{base}...{head}", "--", name])
+        if not file_diff:
+            continue
+        if len(file_diff) > budget:
+            omitted.append(name)
+            continue
+        parts.append(file_diff)
+        budget -= len(file_diff)
+
+    return "".join(parts), skipped, omitted


 def call_kimi(base_url, key, key_id, model, system, user):
@@ -212,24 +273,26 @@ def main():
        return 0

    base, head = meta["baseRefOid"], meta["headRefOid"]
-    diff, dropped = filtered_diff(base, head)
+    diff, skipped, omitted = filtered_diff(base, head)
    if not diff.strip():
        print("nothing reviewable after filtering generated paths")
        return 0

-    truncated = False
-    if len(diff) > MAX_DIFF_CHARS:
-        diff = diff[:MAX_DIFF_CHARS]
-        truncated = True
-
    user = (
        f"Pull request: {meta['title']}\n"
        f"Target branch: {meta['baseRefName']}\n\n"
        f"Description:\n{(meta.get('body') or '(none)')[:4000]}\n\n"
        f"Diff:\n```diff\n{diff}\n```"
    )
-    if truncated:
-        user += "\n\n(The diff was truncated; review what is shown and say so.)"
+    if omitted:
+        shown = ", ".join(omitted[:10])
+        more = f" and {len(omitted) - 10} more" if len(omitted) > 10 else ""
+        user += (
+            f"\n\n(The diff was truncated to fit context: {len(omitted)} "
+            f"changed file(s) did not fit and are NOT shown above -- "
+            f"{shown}{more}. Review what is shown and say so; do not imply "
+            "coverage of the omitted files.)"
+        )

    try:
        review = call_kimi(
@@ -326,10 +389,15 @@ def main():
        return 0

    note = ""
-    if dropped:
-        note += f"\n\n<sub>{dropped} generated/vendored file(s) excluded from review.</sub>"
-    if truncated:
-        note += "\n\n<sub>Diff truncated before review — large PR.</sub>"
+    if skipped:
+        note += f"\n\n<sub>{len(skipped)} generated/vendored file(s) excluded from review.</sub>"
+    if omitted:
+        names = ", ".join(f"`{n}`" for n in omitted[:10])
+        more = f" +{len(omitted) - 10} more" if len(omitted) > 10 else ""
+        note += (
+            f"\n\n<sub>Diff truncated before review — large PR. "
+            f"{len(omitted)} changed file(s) not reviewed: {names}{more}.</sub>"
+        )

    comment = f"## 🌙 Kimi review\n\n{review}{note}"
    p = "/tmp/kimi-review.md"
@@ -27,6 +27,12 @@ on:
 jobs:
  publish:
    name: Build + push toolchain image
+    # workflow_dispatch has no branch restriction of its own -- without this,
+    # a manual dispatch from any branch would push :latest, contradicting the
+    # push-trigger's develop-only guard above and the invariant this file
+    # claims ("a tagged image from an unmerged branch would be
+    # indistinguishable from a real one at :latest").
+    if: github.ref == 'refs/heads/develop'
    runs-on: ubuntu-latest
    timeout-minutes: 20
    permissions:
@@ -50,13 +50,28 @@ jobs:

          # A missing nightly release is itself the fault condition -- that is
          # what "the nightly stopped publishing" looks like from outside.
-          if ! REL=$(gh api "repos/${REPO}/releases/tags/nightly" 2>/dev/null); then
+          if ! gh api "repos/${REPO}/releases/tags/nightly" >/dev/null 2>&1; then
            echo "::error::No 'nightly' release exists. The nightly publish in release.yml is not running at all. See #508."
            exit 1
          fi

-          NIGHTLY_DATE=$(echo "$REL" | jq -r '.published_at // .created_at')
-          echo "nightly     : published ${NIGHTLY_DATE}"
+          # NOT the release object's published_at/created_at: release.yml's
+          # "Publish rolling prerelease" step updates the SAME release in
+          # place (gh release edit --clobber) rather than recreating it, so
+          # those timestamps are stamped once at first publish and never
+          # move again -- comparing against them would make LAG_HOURS grow
+          # forever and fail this check on every run after the first
+          # GRACE_HOURS, regardless of how current the nightly actually is.
+          # The signal that DOES move every run is the `nightly` tag itself:
+          # the preceding "Move the rolling tag" step force-updates
+          # refs/tags/nightly to point at GITHUB_SHA on every publish, so the
+          # tag's target commit date is the true "last published" time.
+          if ! TAG_SHA=$(gh api "repos/${REPO}/git/refs/tags/nightly" --jq '.object.sha' 2>/dev/null) || [ -z "$TAG_SHA" ]; then
+            echo "::error::'nightly' release exists but its tag ref is missing. See #508."
+            exit 1
+          fi
+          NIGHTLY_DATE=$(gh api "repos/${REPO}/commits/${TAG_SHA}" --jq '.commit.committer.date')
+          echo "nightly     : tag points at ${TAG_SHA:0:7}, committed ${NIGHTLY_DATE}"

          DEV_EPOCH=$(date -u -d "$DEV_DATE" +%s)
          NIGHTLY_EPOCH=$(date -u -d "$NIGHTLY_DATE" +%s)
@@ -34,7 +34,7 @@ This is a libretro core, so the whole frontend feature set applies — and the c
 - **Jaguar CD, two ways.** CUE/BIN and CDI images boot through a high-level-emulated CD BIOS *or* through the real CD BIOS driving emulated BUTCH/DSA/FIFO hardware. Audio CDs play and the Virtual Light Machine visualizer works. ([v3.0.0 notes](docs/RELEASE_NOTES_v3.0.0.md) · [v3.1.0 notes](docs/RELEASE_NOTES_v3.1.0.md))
 - **Damaged-rip repair.** CDI images from the old V2 ripper that lost each track's leading bytes used to be rejected outright. The loader now classifies the boot track and repairs slides or head-loss at read time; when bytes are provably gone it logs a precise diagnosis instead of a black screen. ([PR #342](https://github.com/libretro/virtualjaguar-libretro/pull/342))
 - **JagLink / netlink link-cable networking.** The Jaguar's link-port serial hardware is emulated at the register level with a TCP transport and a multi-peer hub — Doom deathmatch over LAN validated end to end. ([user guide](docs/netlink-user-guide.md) · [design doc](docs/netlink-design.md))
- **Controller-port peripherals, not just the pad.** The ST/Amiga mouse adapter sold by AtariAge and The Brewing Academy (and its PS/2 variant), emulated from a sourced pin-level mapping including the fact that the real adapter is *row-blind* — all three wiring cases ship as selectable devices. Plus the Tempest 2000 rotary, Atari's TR10 bank-switching analog and driving controllers, the port-1 light gun, and the early-board motherboard ADC at `$F17C00` that BattleSphere's analog stick actually reads — with per-axis dead zone, offset and response curves on every analog source. Every one is opt-in: with none selected the controller registers are bit-identical to before, proven by a test. ([user guide](docs/input-devices-user-guide.md) · [mouse mapping](docs/jaguar-mouse-adapter-mapping.md) · [light gun design](docs/lightgun-design.md) · [epic #428](https://github.com/libretro/virtualjaguar-libretro/issues/428))
+- **Controller-port peripherals, not just the pad.** The ST/Amiga mouse adapter sold by AtariAge and The Brewing Academy (and its PS/2 variant), emulated from a sourced pin-level mapping including the fact that the real adapter is *row-blind* — all three wiring cases ship as selectable devices. Plus the Tempest 2000 rotary, Atari's TR10 bank-switching analog and driving controllers, the port-1 light gun, and the early-board motherboard ADC at `$F17C00` that BattleSphere's analog stick actually reads — with per-axis dead zone, offset and response curves on every analog source. Every Jaguar controller Atari shipped is now emulated too: the **Team Tap** 4-player adapter (validated through the real bus against AtariJaguarPadtest), the **Pro Controller** six-button pad, and — best-effort, unvalidated against any known software — the **6D flight stick**'s six degrees of freedom. Every one is opt-in: with none selected the controller registers are bit-identical to before, proven by a test. ([user guide](docs/input-devices-user-guide.md) · [mouse mapping](docs/jaguar-mouse-adapter-mapping.md) · [light gun design](docs/lightgun-design.md) · [epic #428](https://github.com/libretro/virtualjaguar-libretro/issues/428))
 - **The Jaguar Voice Modem.** Ultra Vortek's modem netplay works — dial, answer and play a match — with the modem's command set derived from the game's own ROM rather than guessed. ([`voice-modem.md`](docs/voice-modem.md) · [#481](https://github.com/libretro/virtualjaguar-libretro/issues/481))
 - **Jaguar GameDrive (JagGD).** Detection plus full 16 MB bank switching (6×1 MB pages over 16 banks), implemented from RetroHQ's published GDBIOS bindings, so GD-locked homebrew boots. ([interface notes](docs/jgd-interface-notes.md) · v3.1.0 #312, #320)
 - **Memory Track saves.** The Jaguar CD's Memory Track cartridge is emulated — a flash window at `$900000` plus an HLE'd NVM BIOS module — so CD titles keep their saves, round-tripping through the frontend's `.srm`. ([`memory-track.md`](docs/memory-track.md) · [PR #259](https://github.com/libretro/virtualjaguar-libretro/pull/259))
@@ -303,8 +303,10 @@ Plus the peripherals that hang off it:
 | Jaguar Voice Modem | Emulated, over the netlink transport |
 | JagLink / CatBox link cable | Emulated, TCP or RetroArch netplay |
 | Memory Track cartridge · Jaguar GameDrive | Emulated |
-| **Team Tap** (4-player adapter) | **Not emulated** — [#513](https://github.com/libretro/virtualjaguar-libretro/issues/513) |
-| **Pro Controller** (six-button pad) | **Not emulated** — [#514](https://github.com/libretro/virtualjaguar-libretro/issues/514) |
+| Team Tap (4-player adapter) | Emulated, validated through the real bus against AtariJaguarPadtest — [#513](https://github.com/libretro/virtualjaguar-libretro/issues/513) |
+| Pro Controller (six-button pad) | Emulated as a core-option preset (no published detection method exists to test against) — [#514](https://github.com/libretro/virtualjaguar-libretro/issues/514) |
+| 6D flight stick (six degrees of freedom) | Emulated from Technical Reference V10; unvalidated against any known real or homebrew software — [#538](https://github.com/libretro/virtualjaguar-libretro/issues/538) |
+| Motherboard paddle ADC (`$F17C00`, BattleSphere analog stick) | Emulated, either port — [#505](https://github.com/libretro/virtualjaguar-libretro/issues/505) |

 System clock 26.590906 MHz NTSC / 26.593900 MHz PAL. Full source map: [`docs/source-layout.md`](docs/source-layout.md).

@@ -197,7 +197,10 @@ The campaign itself lands in **v4.0.0**.
  same channel-select without ever reading it back. That interface is **not
  emulated**: this release reads `$F17C00` as `0xFF`, matching a production
  console with no ADC fitted (A/B-verified inert on BattleSphere across 900
-  frames and Club Drive across 700). Emulating the ADC itself, and with it
+  frames and Club Drive across 700). *(Corrected 2026-08-19: this note
+  originally said `0x0000`; the shipped v3.4.0 code always returned `0xFF`
+  — the tagged behavior is unchanged, only this description was wrong.
+  See f2d8457.)* Emulating the ADC itself, and with it
  BattleSphere's analog stick, is #505.

 ## Downloads