skill: prrev gets posted PR responses as examples, and new tone rules

The responses posted on PR #39 and PR #40, copied from GitHub, as
references for the shape of a draft. Both predate the current tone
rules, so the skill says the rules win where an example differs.

The tone rules change with them:
- The first response thanks the author for the work itself, not
  only its testing.
- Memory improvements are requested changes before merging, each its
  own bullet (the lever, what it saves, why it's safe).
- The hardware-testing heading carries the framing: for one of us to
  possibly get to before release, not gates, with anything unlisted
  covered or fine. The "everything else" line and the offer of a
  hand are gone.
- When one author gets responses on several related PRs at once, the
  shared lines go only in the first.

Author: SensoriumEmbedded

.claude/skills/prrev/SKILL.md

@@ -252,7 +252,8 @@ each RAM1 change. Then look for improvements to suggest:
 - **New newlib routines.** The build already refuses `sscanf`, `pow`, `strtod` and similar.
   Note smaller additions such as `snprintf` or `strlcpy`.
 
-Frame memory points as improvements, not blockers, unless a floor is at risk.
+Memory improvements are requested changes, like the recommended items. Ask for each one before
+merging, whether or not a floor is at risk, and say plainly when one is.
 
 ## 5. Hardware testing
 
@@ -313,23 +314,35 @@ as a direct message to the author.
 - **Contents.**
   - The recommended changes, renumbered 1..n. For each: a heading saying what's wrong (with
     `file:line`), why it matters (a repro if there is one) and the suggested fix.
-  - Memory: one short paragraph with the measured impact. Raise concerns as cleanups, naming
-    the specific lever.
+  - Memory: one short paragraph with the measured impact, then each improvement as its own
+    bullet: the lever, what it saves (bytes, and which RAM), and why it's safe. Don't fold a
+    saving into a passing sentence. These count as changes before merging: include them in
+    the "N things" lead-in (for example "Besides the memory item above, one thing…"), and
+    never say "nothing to change" or "ready as it stands" when a response has any.
   - Hardware testing still to do, after acknowledging what the author covered.
   - Leave out hardware-check-only items unless they need the author to act, and the optional
-    and no-change items. One closing line covers them: "Everything else I noted is either
-    covered by your follow-ups list or fine as it is."
+    and no-change items. The hardware-testing heading's framing covers them (see Tone).
 - **Tone**, from the maintainer's own edits of an earlier draft:
-  - Open by thanking the contributor for their own testing and write-up.
+  - Open the first response with thanks for the work itself, meaning what the PR adds and
+    not just its testing or write-up. For example: "Thank you for the Geos64
+    implementation, this is exciting!  :)"
   - "There are N things I'd like you to look at before merging." Not "I'd like changed".
   - "As always, feel free to push back on any that are inappropriate or would take
     significant memory/code to resolve."
-  - Hardware testing is shared and before release, not a merge gate: "One of us should
-    probably hit the following at some point before release:".
-  - Offer a hand before closing: "If any of these would be easier for me to take on, or
-    you'd like a hand with the testing, just let me know."
+  - The hardware-testing framing goes in the section's heading, not in a sentence under it:
+    "### Hardware testing still to do: for one of us to *possibly* get to before release,
+    not gates, and anything I noted that isn't listed is covered by your follow-ups or
+    fine as it is". Every hardware heading carries it. Later responses in a set use the
+    short form, which ends at "before release".
+  - No offer-of-a-hand line ("If any of these would be easier for me to take on…").
   - Close with "Thank you!"
   - The voice is the user's ("I reviewed…"), addressed to the author.
+  - **A set of related PRs reviewed together** (a stack, or a batch from one author): the
+    author reads every response at once, so say the shared lines only once, in the first
+    PR's response. Those are the review method, the push-back invitation, the long form of
+    the hardware-testing heading, and "Thank you!". Every other response opens with one
+    plain thank-you specific to that PR, and words its "nothing to change" or closing
+    line differently from the rest of the set.
 - **No private references.** This text is public.
   - No todo-list item numbers. GitHub also turns "#69" into a link to the repo's own PR or
     issue #69.
@@ -349,11 +362,17 @@ as a direct message to the author.
 The shape, with the details filled in from the review:
 
 ```markdown
-Thanks for <the author's testing and write-up>, much appreciated!  I reviewed this merged
-onto current main (<sha>) and built both targets. <One or two sentences of memory impact.>
+Thank you for <the work itself>, <a short, genuine reaction>!
 
-There are <N> things I'd like you to look at before merging.  As always, feel free to push
-back on any that are inappropriate or would take significant memory/code to resolve.
+I reviewed this merged onto current main (<sha>) and built both targets. As always, feel
+free to push back on anything that's inappropriate or would take significant memory/code
+to resolve.
+
+Memory: <one or two sentences of measured impact>. <N> improvement(s):
+
+- **<The lever>: <bytes saved, and which RAM>.** <Why it's safe.>
+
+There are <N> things I'd like you to look at before merging.
 
 ### 1. <What's wrong> (`<file>:<line>`)
 
@@ -363,19 +382,18 @@ Suggested fix: <the fix>.
 
 ### 2. ...
 
-### Hardware testing still to do
+### Hardware testing still to do: for one of us to *possibly* get to before release, not gates, and anything I noted that isn't listed is covered by your follow-ups or fine as it is
 
-Your <tests> cover <what they cover>.  One of us should probably hit the following at some
-point before release:
+Your <tests> cover <what they cover>.
 
 - <test>
 - <test>
 
-Everything else I noted is either covered by your follow-ups list or fine as it is.
-If any of these would be easier for me to take on, or you'd like a hand with the testing,
-just let me know.
 Thank you!
 ```
 
-Responses the user approved before, when there are any, are in this skill's `references/`
-folder.
+Responses the user approved and posted are in this skill's `references/` folder:
+`example-response-pr39.md` and `example-response-pr40.md`. Both predate the current tone
+rules: they thank the author for testing rather than for the work, frame hardware testing
+in a sentence instead of the heading, and end with an "everything else" line (PR #40 also
+with an offer of a hand). Where an example and the tone rules above differ, follow the rules.

.claude/skills/prrev/references/example-response-pr39.md

@@ -0,0 +1,47 @@
+Thanks for the thorough write-up and the NTSC runs, much appreciated!  I reviewed this merged onto current main (49b23c4, which merges cleanly) and built both targets. RAM1 variables, free-for-locals and RAM2 are unchanged on TR and TR+. ITCM code grows 576 B (TR+ main), 448 B (TR main) and 128–192 B (minimal), all in functions that were already in RAM, so no concerns there.
+
+There are three things I'd like you to look at before merging.  As always, feel free to push back on any that are inappropriate or would take significant memory/code to resolve.
+
+### 1. A failed TR remote launch still leaves the menu type switched (`RemoteControl.ino:202`)
+
+The TR branch now keeps the SD listing intact, which fixes the freed names, and your test confirms the listing stays readable. But by the time the TR lookups fail, `RemoteLaunched` is set, `IO1[rWRegCurrMenuWAIT]` is already `rmtTeensy`, and `VmRegistry` has been refreshed. The C64 still shows the SD listing, and picking anything from it goes down `HandleExecution`'s `rmtTeensy` case:
+
+- A file does `memcpy(RAM_Image, MenuSelCpy.Code_Image, MenuSelCpy.Size)` (`DriveDirLoad.ino:311`). `Code_Image` and `Size` are never set for card items.
+- `IOHandlerSelectInit` then reads an `IOHndlrAssoc` that's never set either.
+- A folder goes to `SetMenu((StructMenuItem*)Code_Image, …)`.
+
+Repro: browse SD, remote-launch `TR:/Games/NoSuchFile` (or press a hot key saved for a TR item that has since been renamed), then select any file in the listing.
+
+Suggested fix: both `FindTRMenuItem` lookups have no side effects, so resolve the TR folder and file right after splitting the path. Return from there, before `RemoteLaunched = true`, `IO1[rWRegCurrMenuWAIT] = MenuSourceID`, `SDFullInit` and `VmRegistry::refresh`.
+
+### 2. Paths of 256–357 characters are refused when they don't need to be, and silently (`RemoteControl.ino:193`)
+
+On main these overflowed `DriveDirPath`. In today's builds the overflow lands in `EZFlashRAM` (the next 256 bytes, in both the TR and TR+ main images), so in practice they launched. Now they're refused. `LaunchFile` has already sent its second `AckToken` (`SerUSBIO.ino:803`), so the app sees success and nothing happens. Your 363-byte test hit `ReceiveFileName`'s own limit, so this range wasn't covered.
+
+Only the folder part has to fit in `DriveDirPath`/`LaunchDir`. The file name can keep pointing into `FileNamePath`, and `LoadFile` builds the full path in a `MaxNamePathLength` buffer. So:
+
+- Check the folder length (`strrchr(FileNamePath, '/') - FileNamePath`) against `MaxPathLength` instead of the whole length, and copy just the folder into `LaunchDir`.
+- For anything still refused, have `LaunchFile` reply `FailToken` + "Path too long!" before the second ack, the way `ReceiveFileName` already does. The app already handles that reply.
+
+The consumers that need the whole path in 256 bytes (the CRT boot name and `SetLatestSIDLoaded`) already refuse on their own in this PR.
+
+### 3. Message wording (`FileParsers.ino:189`)
+
+"Path too long to boot / this crt from" reads as though something is missing. Maybe "CRT path too long / to boot from SD"?
+
+### Hardware testing still to do
+
+Your NTSC runs cover the long text file, the looping D64, menu browsing and a remote PRG launch.  One of us should probably hit the following at some point before release:
+
+- selecting on page 2+ of a big listing; the next/previous file-type keys; D64/D71/D81 browsing; the reload after a remote file change
+- a large CRT from SD (it frees the listing mid-load, which now redirects to the TR menu), one big enough for the MinimalBoot reboot, and REU / Swiftlink / Retro Replay / SSv5 launched from SD, each followed by a reset back to the menu
+- remote launch from SD root and a subfolder, USB, and TR root and a subfolder; hot keys; autolaunch at power-up; NFC tags (including random) if you have a reader
+- after fixes 1 and 2: the failed TR launch followed by selecting an SD item, and a path of 256–357 characters
+- a background SID set from SD and from the TR menu, then a power cycle
+- setting a hot key, auto launch, REU file, KERNAL file, NFC tag and Meatloaf mount with normal paths
+- TR BASIC TSAVE over an existing file, and TDIR of a big folder (the "Too many files" cutoff moved)
+- MinimalBoot EasyFlash bank swapping (e.g. SNKvsCAP)
+- PAL, and a plain TR board
+
+Everything else I noted is either covered by your follow-ups list or fine as it is.
+Thank you!

.claude/skills/prrev/references/example-response-pr40.md

@@ -0,0 +1,50 @@
+Thanks for the write-up and the before/after runs over USB, much appreciated!  I reviewed this merged onto current main (49b23c4, which merges cleanly) and built both targets. RAM1 and RAM2 are unchanged on TR and TR+, and flash code grows 88 B (TR+) and 128 B (TR), all in FLASHMEM functions, so no concerns there.
+
+There are three things I'd like you to look at before merging.  As always, feel free to push back on any that are inappropriate or would take significant memory/code to resolve.
+
+### 1. A failed upload leaves a truncated file that blocks the retry (`FileTransfer.ino:119`)
+
+When `ReceiveFileData` times out, it closes the partial file and returns. The checksum-failure path (`FileTransfer.ino:130`) does the same with a full-length but bad file. `GetFileStream` refuses a file that already exists (`FileTransfer.ino:54`), so once the drain has cleared the stream, the client's retry gets "File already exists." The truncated file also stays on the card, where it can be launched as a broken cart or program.
+
+Repro: post a file, stall partway through until the timeout, then post it again.
+
+Suggested fix: in `PostFileCommand`, remove the file when `ReceiveFileData` returns false. That covers both failure paths in one place. Since `GetFileStream` only ever creates a new file, it can't delete anything the user already had.
+
+### 2. The drain's ceiling counts from the start of the command (`FileTransfer.ino:120`, `SerUSBIO.ino:1255`)
+
+`DiscardCmdInput` stops at `SpansCeilingmS(Bytes)` + 2 s, measured from `Began`. Both callers set `Began` at the start of the command and pass the whole length as `Bytes`, so the ceiling is 7 s + len/2 mS from the start. A transfer that averaged under 2 bytes/mS and fails late is already past it. The drain then returns at once and the rest of the payload is parsed as commands, which is the case this PR fixes. It needs a slow link or many pauses just under 500 mS, so it's an edge case, but the fix is small.
+
+Suggested fix: at both call sites, pass `millis()` as `Began` and the bytes still owed as `Bytes`. The ceiling then becomes the remainder's time at 2 bytes/mS plus the slack, counted from the failure.
+
+While you're there: the comments over `SpansCeilingmS` and `DiscardCmdInput` (`SerUSBIO.ino:983-989`) still talk about "the command's deadline", which PostFile and WriteC64Mem no longer have.
+
+### 3. The title and description still describe the deadline
+
+The title is "Put a deadline on remote file and memory transfers", but the deadline was dropped in a84de2e, and the title ends up in the merge commit.
+
+The description also says the flushes are what hang `loop()` when a peer disappears. They were one way, but the writes themselves can block too:
+- `EthernetClass::socketSend` waits with no deadline for room in the send buffer.
+- `USBSerialBase::write` in USBHost_t36 does the same.
+
+So a reply bigger than the free buffer can still hang with a vanished peer. That seems a better fit for your overall-deadline follow-up than for this PR.
+
+Suggested fix: retitle to something like "Discard what a failed remote transfer still sends, and don't block on flush", and move the write-blocking case into the follow-ups.
+
+### Hardware testing still to do
+
+Your USB runs on a TR+ cover the drain for both commands, with a before/after against a build without this PR.  One of us should probably hit the following at some point before release:
+
+- the same upload stall test on a plain TR, which gets `DiscardCmdInput` for the first time with this PR
+- both stall tests over TCP
+- a large GetFile over TCP that completes with a matching checksum, since `SendFileData` no longer flushes there
+- pulling the Ethernet cable in the middle of a GetFile, to check that the menu recovers
+- an Ultimax 8k launch while a TCP client is still connected, on main and on this PR. I suspect this PR fixes a hang on main here, and if so it's worth adding to the description:
+  - The launch disables `IRQ_ENET` and `IRQ_PIT` (`DriveDirLoad.ino:386`).
+  - The old `CmdChannel->flush()` before the reset (now `Teensy.ino:302`) then waits for an ACK that FNET can't process.
+  - So I think main hangs there with the C64 held in reset.
+- normal large uploads and downloads with the TeensyROM app over USB and TCP, plus a failed upload followed by the app's retry. MetalHexx offered to test once this was rebased, and his app is the best check that nothing it sends right after a failure gets eaten by the drain.
+- after fixes 1 and 2: a failed upload leaves no file and the retry succeeds, and a sender paced under 2 bytes/mS that stalls late is still drained
+
+Everything else I noted is either covered by your follow-ups list or fine as it is.
+If any of these would be easier for me to take on, or you'd like a hand with the testing, just let me know.
+Thank you!