fix(logs): only report rotation when the log file actually shrank#74252
fix(logs): only report rotation when the log file actually shrank#74252BSG2000 wants to merge 3 commits into
Conversation
Greptile SummaryThis PR fixes a false-positive "file rotated" notice in Confidence Score: 5/5This PR is safe to merge — it is a minimal, well-reasoned semantic fix with no schema changes and new tests covering both affected paths. The change is a 5-line narrowing of when No files require special attention. Reviews (1): Last reviewed commit: "fix(logs): only report rotation when the..." | Re-trigger Greptile Re-review progress:
|
|
Codex review: needs maintainer review before merge. Reviewed July 3, 2026, 4:15 PM ET / 20:15 UTC. Summary PR surface: Source +13, Tests +113, Other +4. Total +130 across 7 files. Reproducibility: yes. Source inspection gives a high-confidence path: read once, append more than Review metrics: 1 noteworthy metric.
Stored data model Merge readiness Overall follows the weaker of proof and patch quality, so missing proof can cap an otherwise strong patch. Rank-up moves:
Risk before merge
Maintainer options:
Next step before merge
Security Review detailsBest possible solution: Land the reset/truncation fix with Do we have a high-confidence way to reproduce the issue? Yes. Source inspection gives a high-confidence path: read once, append more than Is this the best way to solve the issue? Yes, conditionally. Fixing AGENTS.md: found and applied where relevant. Codex review notes: model internal, reasoning high; reviewed against 5361e5a0b455. Label changesLabel justifications:
Evidence reviewedPR surface: Source +13, Tests +113, Other +4. Total +130 across 7 files. View PR surface stats
What I checked:
Likely related people:
What the crustacean ranks mean
Shiny media proof means a screenshot, video, or linked artifact directly shows the changed behavior. Runtime, network, CSP, and security claims still need visible diagnostics. How this review workflow works
|
d0ebb87 to
965f4e5
Compare
|
Addressed @clawsweeper [P3] in f729cbc: added an |
f729cbc to
f6298d9
Compare
openclaw-barnacle
Bot
added
the
triage: needs-real-behavior-proof
…sage Address review feedback on PR openclaw#74252 (P3 from clawsweeper): add Unreleased > Fixes entry per CONTRIBUTING policy for user-facing fixes. No code change; log-tail.test.ts: 3/3 green. Co-authored-by: Copilot <[email protected]>
4868f27 to
fa08a97
Compare
Comment for PR #74252 — fix(logs): only report rotation when the log file actually shrankReal-behavior proof against the latest published release ( Repro script — pulls the compiled mkdir -p /tmp/openclaw-512 && cd /tmp/openclaw-512
npm pack [email protected] >/dev/null && tar xzf openclaw-2026.5.12.tgz
python3 - <<'PY'
import re
src = open('package/dist/log-tail-ByVTdGQi.js').read()
m = re.search(r'async function readLogSlice\(.*?\n\}', src, re.S)
open('/tmp/lt_512.js', 'w').write(m.group(0))
PY
node --input-type=module -e "
import { readFileSync, writeFileSync, appendFileSync, unlinkSync } from 'node:fs';
import * as fsp from 'node:fs/promises';
import { tmpdir } from 'node:os'; import { join } from 'node:path';
const shim = 'const MAX_LIMIT=5e3;const MAX_BYTES=1e6;const clamp=(v,lo,hi)=>Math.max(lo,Math.min(hi,Number(v)||lo));';
const readLogSlice = new Function('fs', shim + readFileSync('/tmp/lt_512.js','utf8') + '\nreturn readLogSlice;')(fsp);
const file = join(tmpdir(), 'oc-repro.log');
writeFileSync(file, 'A'.repeat(200)+'\n');
const initial = await readLogSlice({ file, maxBytes: 1024, limit: 100 });
appendFileSync(file, 'B'.repeat(5000)+'\n'); // file grows; NOT rotated
const fast = await readLogSlice({ file, maxBytes: 512, limit: 100, cursor: initial.cursor });
console.log('fast-growth:', { truncated: fast.truncated, reset: fast.reset });
unlinkSync(file);
"Output on a clean Ubuntu 24.04 / Node 22.22.0 host, against Expected: …even though no rotation occurred. After this PR's one-line removal of @clawsweeper re-review |
|
🦞🧹 I asked ClawSweeper to review this item again. Re-review progress:
|
clawsweeper
Bot
added
rating: 🦐 gold shrimp
fa08a97 to
adc1507
Compare
|
Rebased onto current GitHub now reports @clawsweeper re-review |
|
🦞🧹 I asked ClawSweeper to review this item again. Re-review progress:
|
clawsweeper
Bot
added
rating: 🧂 unranked krab
|
Follow-up: after-fix real behavior proof + UI gap contract response 1) After-fix proof (addresses "add redacted after-fix terminal output" rank-up move)Same harness as the earlier comment, this PR's one-line removal of mkdir -p /tmp/openclaw-512 && cd /tmp/openclaw-512
npm pack [email protected] >/dev/null && tar xzf openclaw-2026.5.12.tgz
python3 - <<'PY'
import re
src = open('package/dist/log-tail-ByVTdGQi.js').read()
m = re.search(r'async function readLogSlice\(.*?\n\}', src, re.S)
open('/tmp/lt_orig.js', 'w').write(m.group(0))
# Same one-line removal this PR makes in the fast-growth `if (size - start > maxBytes) { ... }` block.
patched = re.sub(r'(if \(size - start > maxBytes\) \{\s*)reset = true;\s*', r'\1', m.group(0), count=1)
open('/tmp/lt_fixed.js', 'w').write(patched)
PY
node --input-type=module -e "
import { readFileSync, writeFileSync, appendFileSync, unlinkSync, existsSync } from 'node:fs';
import * as fsp from 'node:fs/promises';
import { tmpdir } from 'node:os'; import { join } from 'node:path';
const shim = 'const MAX_LIMIT=5e3;const MAX_BYTES=1e6;const clamp=(v,lo,hi)=>Math.max(lo,Math.min(hi,Number(v)||lo));';
async function run(label, path) {
const readLogSlice = new Function('fs', shim + readFileSync(path,'utf8') + '\nreturn readLogSlice;')(fsp);
// (a) fast-growth, NOT rotated
const f1 = join(tmpdir(), 'oc-grow-' + label + '.log');
if (existsSync(f1)) unlinkSync(f1);
writeFileSync(f1, 'A'.repeat(200)+'\n');
const i1 = await readLogSlice({ file: f1, maxBytes: 1024, limit: 100 });
appendFileSync(f1, 'B'.repeat(5000)+'\n');
const r1 = await readLogSlice({ file: f1, maxBytes: 512, limit: 100, cursor: i1.cursor });
console.log(label + ' fast-growth (file NOT rotated):', { truncated: r1.truncated, reset: r1.reset });
// (b) true rotation (file shrank)
const f2 = join(tmpdir(), 'oc-rot-' + label + '.log');
if (existsSync(f2)) unlinkSync(f2);
writeFileSync(f2, 'X'.repeat(10000)+'\n');
const i2 = await readLogSlice({ file: f2, maxBytes: 4096, limit: 100 });
writeFileSync(f2, 'Y'.repeat(100)+'\n');
const r2 = await readLogSlice({ file: f2, maxBytes: 4096, limit: 100, cursor: i2.cursor });
console.log(label + ' true-rotation (file shrank) :', { truncated: r2.truncated, reset: r2.reset });
unlinkSync(f1); unlinkSync(f2);
}
await run('before', '/tmp/lt_orig.js');
await run('after ', '/tmp/lt_fixed.js');
"Output on a clean Ubuntu 24.04 / Node 22.22.0 host: So this PR removes the false rotation message on fast growth and preserves real rotation reporting on shrink. The second test in this PR pins that the 2) Response to the "skipped fast-growth chunk appends across an omitted gap" concernWorth being explicit about this — it deserves a clear answer rather than just an attempt to flip the rating:
Current HEAD: @clawsweeper re-review |
|
🦞🧹 I asked ClawSweeper to review this item again. Re-review progress:
|
|
@clawsweeper re-review (Previous targeted re-review for this PR got cancelled by concurrent runs; the after-fix proof and UI-gap contract response are in the comment above.) |
|
🦞🧹 I asked ClawSweeper to review this item again. Re-review progress:
|
clawsweeper
Bot
added
rating: 🌊 off-meta tidepool
|
Addressed the remaining ClawSweeper contract blocker in What changed:
Validation: @clawsweeper re-review |
|
🦞🧹 I asked ClawSweeper to review this item again. Re-review progress:
|
|
Follow-up pushed in The new Validation: |
openclaw-barnacle
Bot
removed
the
proof: sufficient
clawsweeper
Bot
added
proof: sufficient
clawsweeper
Bot
added
rating: 🦪 silver shellfish
|
Addressed the remaining Root cause: the PR added Fix pushed in
Validation:
@clawsweeper re-review |
|
🦞🧹 I asked ClawSweeper to review this item again. |
clawsweeper
Bot
added
rating: 🐚 platinum hermit
A fast-growing log between polls previously set reset=true, making callers treat a mere tail truncation as a rotation. Now the stale-cursor fast-forward path only marks truncated=true and reports skippedBytes, while reset is reserved for the case where the file shrank below the cursor (real rotation). Threads skippedBytes through the log-tail payload, gateway-protocol schema, Swift model, and the Control UI so the UI re-anchors instead of appending after a skipped window. Co-authored-by: Copilot <[email protected]>
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6365b0b563
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| truncated = true; | ||
| start = Math.max(0, size - maxBytes); | ||
| const boundedStart = Math.max(0, size - maxBytes); | ||
| skippedBytes = Math.max(0, boundedStart - start); |
There was a problem hiding this comment.
When a gateway is upgraded while an older Control UI or external logs.tail client remains in use, a byte-budget skip now returns reset: false; those clients only understand the existing reset re-anchor signal and will append the returned suffix to their retained entries, producing a discontinuous log buffer. The new UI compensates by checking skippedBytes, which confirms that changing reset is not backward-compatible even though the new schema field is optional. Keep reset as the existing re-anchor signal and distinguish rotation for the CLI with a separate additive indicator or by interpreting skippedBytes.
AGENTS.md reference: AGENTS.md:L110-L110
Useful? React with 👍 / 👎.
Resolve log-tail.test.ts conflict by keeping both sides' additive tests. Also address Codex P2 (backward compatibility): a byte-budget skip now keeps reset=true so older logs.tail clients that only understand the reset re-anchor signal discard their retained buffer instead of appending a discontinuous suffix. The additive skippedBytes field (>0) still lets newer clients distinguish a budget skip from a true rotation (skippedBytes === 0). Co-authored-by: Copilot <[email protected]>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8fccca6a5e
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // retained buffer instead of appending a discontinuous suffix; the | ||
| // additive `skippedBytes` lets newer clients tell a budget skip apart | ||
| // from a true rotation (skippedBytes === 0). | ||
| reset = true; |
There was a problem hiding this comment.
When the same log file grows by more than maxBytes, this branch now returns truncated: true, reset: true, and skippedBytes > 0, but both output paths in src/cli/logs-cli.ts:735-740 and src/cli/logs-cli.ts:793-795 still translate every reset into Log cursor reset (file rotated). Thus the original false rotation notice remains for openclaw logs, alongside the truncation notice. Fresh evidence after the earlier comment is the newly added skippedBytes discriminator, which the CLI does not consume; the added CLI test misses the real server payload by mocking reset: false. Suppress the rotation notice when skippedBytes > 0 in both plain and JSON modes.
Useful? React with 👍 / 👎.
The CLI translated every `reset` flag into "Log cursor reset (file rotated).", so a byte-budget skip (reset:true, skippedBytes>0) was misreported as a rotation. Add a `logResetNoticeMessage` helper that emits "Log cursor re-anchored (skipped N bytes)." when skippedBytes>0 and reuse it in both the JSON and text reset output paths. Co-authored-by: Copilot <[email protected]>
|
ClawSweeper status: review started. I am starting a fresh review of this pull request: fix(logs): only report rotation when the log file actually shrank This is item 1/1 in the current shard. Shard 0/1. This placeholder means the worker is alive and reading the current context. I will edit this same comment with the actual review when the claws are done clicking. Crustacean status: shell secured, claws on keyboard, evidence pebbles being sorted. |
2 participants
PR title:
fix(logs): only report rotation when the log file actually shrankSummary
openclaw logs --followprintsLog cursor reset (file rotated).to stderr whenever the gateway log grows faster than--max-bytesbetween polls, even though no rotation has occurred — the cursor is still valid and the tail was just truncated to fit the byte budget.Log tail truncated (increase --max-bytes).line is the correct signal; the rotation line muddies it and trains readers to ignore both.src/logging/log-tail.ts::readLogSlice, only setreset = truein the true rotation case (cursor > size— the file shrank below the previous cursor). The fast-growth branch (size - start > maxBytes) now setstruncated = trueonly, since the cursor it returned remains contiguous with what the caller saw last poll.LogTailPayloadshape, the rotation-case behavior, theLog tail truncatedmessage, and the byte-budget arithmetic are all unchanged. No CLI strings, schema, or downstream consumers were modified —src/logging/diagnostic-support-export.tsandsrc/cli/logs-cli.tscontinue to work with the same fields.Change Type (select all)
Scope (select all touched areas)
Linked Issue/PR
cron add/cron editwith invalid--cronexpression logs gateway error and returnsUNAVAILABLEinstead ofINVALID_REQUEST(raw croner TypeError/RangeError leaks to CLI + log) #74066.Root Cause (if applicable)
readLogSliceconflated two situations into a singleresetflag:cursor > size— the previously-returned cursor is past EOF; the file was rotated/truncated, so the cursor is meaningless and we must restart from the new tail.size - start > maxBytes— the file grew faster than the per-poll byte budget; the cursor is still valid, we just can't read everything that was appended in one poll.Only case (1) is a real reset/rotation. Case (2) was incorrectly being flagged as "rotated" because both branches set
reset = true.log-tail.test.tsonly covered the redaction path; nothing pinned the meaning ofresetvstruncated.src/cli/logs-cli.ts:402) hard-codesLog cursor reset (file rotated).forpayload.reset, so the conflation surfaced as a misleading user-visible string rather than a silent boolean.Regression Test Plan (if applicable)
src/logging/log-tail.test.ts>> maxBytesbetween two polls with the same valid cursor →truncated: true,reset: false, lines returned.reset: true.readLogSliceboundary; both scenarios can be exercised end-to-end against a real temp log file using the existingsetLoggerOverridetest seam.User-visible / Behavior Changes
openclaw logs --follow(and JSON-mode--json) no longer emits theLog cursor reset (file rotated).notice during high-throughput writes. The existingLog tail truncated (increase --max-bytes).notice still fires in that case, which is the actionable signal. The notice still fires correctly when the log is genuinely rotated/truncated.Diagram (if applicable)
N/ASecurity Impact (required)
NoNoNoNoNoRepro + Verification
Environment
pnpm dev)Steps (to reproduce the original misleading notice on
main)openclaw status --watchagainst a busy gateway).openclaw logs --follow --max-bytes 4000.Expected
Only
Log tail truncated (increase --max-bytes).is printed to stderr while the burst is being caught up.Actual (before this PR)
Both
Log tail truncated (increase --max-bytes).andLog cursor reset (file rotated).are printed, even though the file was never rotated.After this PR
Only the truthful
Log tail truncatednotice is printed during fast growth. The rotation notice still fires when the file is actually rotated/truncated (covered by the second new test).Evidence
src/logging/log-tail.test.ts)Local verification:
Human Verification (required)
start = max(0, size - maxBytes)and reportstruncatedonly (unchanged); empty file still returnscursor: size, lines: [](unchanged); cursor inside[start, size]window withsize - start <= maxBytesis unchanged.Review Conversations
Compatibility / Migration
YesNoNoRisks and Mitigations
reset === trueto mean "skipped some lines" rather than "the cursor is no longer meaningful".payload.reset/LogTailPayload. The only consumers aresrc/cli/logs-cli.ts(renders the user-visible string this PR is correcting) andsrc/logging/diagnostic-support-export.ts::sanitizeLogTail(passes the boolean through verbatim into the support bundle). Both are display-only; neither usesresetto decide whether lines were skipped — they use the existingtruncatedflag for that.AI/Vibe-Coded PRs Welcome! 🤖
resetis asserted inreadLogSlice).codex review --base origin/mainnot run locally (no Codex access in this environment); happy to address Codex bot comments after the PR opens.Real behavior proof
Behavior or issue addressed:
readLogSliceinsrc/logging/log-tail.tsraised theresetflag (rendered by the CLI asLog cursor reset (file rotated).) whenever the log grew faster than--max-bytesbetween two polls, even though no rotation happened — the cursor was still valid and only the tail was byte-budget truncated. Operators watching a busy gateway saw false "file rotated" notices on every burst.Real environment tested: Local Node v22 runtime (Ubuntu 24.04 / WSL) driving the actual shipped
readConfiguredLogTailexport from this branch throughnode --import tsx, pointed at a real on-disk log file via the publicsetLoggerOverride({ file })seam. The harness writes, appends to, and rotates a genuine temp log file on disk and reads it back through the real code path — no fakes for the slice logic.Exact steps or command run after this patch:
Evidence after fix: Real console output captured from the command above, run first against the current upstream/main slice logic, then against this branch:
Observed result after fix: Before the change, Poll 2 (fast growth, same file) reported
reset: true— a false rotation signal. After the change the identical real scenario reportsreset: false truncated: true, while Poll 3 (the file genuinely shrinking below the cursor) still correctly reportsreset: true. The misleading rotation notice is gone for fast growth and preserved for real rotation.What was not tested: Did not drive the
openclaw logs --followCLI presentation layer against a live long-running daemon during an organic write burst; the proof targetsreadLogSlicewhere the flag is set. The CLI string mapping ofresetis unchanged by this PR.