Skip to content

Bound trajectory pointer file read with size cap#110555

Closed
cxbAsDev wants to merge 2 commits into
openclaw:mainfrom
cxbAsDev:fix/bound-trajectory-pointer-read
Closed

Bound trajectory pointer file read with size cap#110555
cxbAsDev wants to merge 2 commits into
openclaw:mainfrom
cxbAsDev:fix/bound-trajectory-pointer-read

Conversation

@cxbAsDev

@cxbAsDev cxbAsDev commented Jul 18, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

What Problem This Solves

readTrajectoryPointerFile calls fs.readFileSync(pointerPath, "utf8") on session trajectory pointer files with no size limit. A corrupted or malicious pointer file can OOM the process during session cleanup.

Why This Change Was Made

PR #101472 established readRegularFileSync with maxBytes as the standard bounded-read primitive. This applies the same pattern to trajectory pointer files.

User Impact

No user-visible change. Trajectory pointer files are typically <1 KB. A 1 MB cap is more than sufficient. An oversized pointer file gracefully returns null.

Evidence

Code Change

src/trajectory/cleanup.ts — readTrajectoryPointerFile

  • Added: import { readRegularFileSync } from "../infra/regular-file.js"
  • Added: const TRAJECTORY_POINTER_MAX_BYTES = 1 * 1024 * 1024
  • Old: JSON.parse(fs.readFileSync(pointerPath, "utf8"))
  • New: const { buffer } = readRegularFileSync({ filePath: pointerPath, maxBytes: TRAJECTORY_POINTER_MAX_BYTES }); JSON.parse(buffer.toString("utf8"))
  • Error handling: existing try-catch returns null on any error including RangeError

Fix Applied

  • 2026-07-18: Fixed ClawSweeper P1 finding — now correctly destructures { buffer } from readRegularFileSync result
  • Branch rebased onto current main

Provenance

Follows the same pattern as PR #101472 (merged) and PR #110516 (sibling PR).

@cxbAsDev

cxbAsDev commented Jul 18, 2026

Copy link
Copy Markdown
Contributor Author

Updated PR body with evidence and compatibility assessment. Branch rebased onto current main. @clawsweeper re-review

@clawsweeper clawsweeper Bot added rating: 🧂 unranked krab Not merge-ready due to missing proof or serious correctness/safety concerns. status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask. P2 Normal backlog priority with limited blast radius. merge-risk: 🚨 compatibility 🚨 May break existing users, config, migrations, defaults, or upgrade paths. labels Jul 18, 2026
@clawsweeper

clawsweeper Bot commented Jul 18, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Codex review: needs real behavior proof before merge. Reviewed July 18, 2026, 6:20 AM ET / 10:20 UTC.

Summary
The PR replaces the trajectory pointer file’s unbounded synchronous UTF-8 read with the shared bounded regular-file reader and a 1 MiB limit.

PR surface: Source +7. Total +7 across 1 file.

Reproducibility: no. high-confidence runtime reproduction was demonstrated in the PR. The source-level path is clear, but the review context lacks an inspectable live run showing normal and oversized pointer-file outcomes.

Review metrics: none identified.

Stored data model
Persistent data-model change detected: serialized state: src/trajectory/cleanup.ts. Confirm migration or upgrade compatibility proof before merge.

Merge readiness
Overall: 🦪 silver shellfish
Proof: 🦪 silver shellfish
Patch quality: 🐚 platinum hermit
Result: blocked until real behavior proof from a real setup is added.

Overall follows the weaker of proof and patch quality, so missing proof can cap an otherwise strong patch.

Rank-up moves:

  • [P2] Add redacted runtime evidence for a normal pointer file and a pointer exceeding 1 MiB, including the observed fallback result.
  • Coordinate with the overlapping broader PR so only one branch owns src/trajectory/cleanup.ts.

Proof guidance:

  • [P1] Needs real behavior proof before merge: The PR reports tests and typechecking but provides no inspectable after-fix runtime evidence; add a redacted terminal transcript, log, recording, or artifact showing ordinary and oversized pointer-file behavior before merge. After adding proof, update the PR body; ClawSweeper should re-review automatically. If it does not, the PR author or someone with repository write access can comment @clawsweeper re-review.

Risk before merge

  • [P1] An existing oversized trajectory pointer will now be treated as absent, so maintainers need after-fix evidence that cleanup fails safely without unexpected retention or deletion behavior.
  • [P1] Two active contributor branches modify the same trajectory-pointer read; merging both would create duplicate review and conflict risk unless one branch is narrowed.

Maintainer options:

  1. Consolidate the overlapping cleanup change (recommended)
    Select one branch to retain the cleanup.ts change, then provide redacted runtime evidence that ordinary pointers still parse and oversized pointers safely return the established fallback.

Next step before merge

  • [P1] A maintainer or the shared contributor must select one overlapping branch, and contributor-controlled real behavior proof is required before merge.

Maintainer decision needed

  • Question: Which active branch should own the src/trajectory/cleanup.ts bounded-read change: this focused PR or the broader overlapping PR?
  • Rationale: Both open branches from the same contributor implement the same path, while neither discussion provides proof sufficient to establish the broader branch as the safe canonical replacement.
  • Likely owner: cxbAsDev — They authored both overlapping branches and the earlier merged adjacent bounded-read work.
  • Options:
    • Retain the focused trajectory PR (recommended): Keep this PR for cleanup.ts, remove that duplicate hunk from the broader branch, and require redacted after-fix runtime proof before merge.
    • Consolidate into the broader branch: Close this PR only after the broader branch is narrowed, proves the same trajectory behavior, and becomes a viable proof-positive landing candidate.

Security
Cleared: The one-file diff adds no dependency, workflow, permission, secret, artifact-download, or supply-chain surface.

Review details

Best possible solution:

Keep one PR as the sole owner of the trajectory-pointer hardening, add redacted runtime proof for normal and oversized pointer files, and land only the selected focused change.

Do we have a high-confidence way to reproduce the issue?

No high-confidence runtime reproduction was demonstrated in the PR. The source-level path is clear, but the review context lacks an inspectable live run showing normal and oversized pointer-file outcomes.

Is this the best way to solve the issue?

Yes, reusing the shared bounded regular-file reader is the narrowest maintainable solution, provided the duplicate branch ownership is resolved and the fallback behavior is proven.

AGENTS.md: found and applied where relevant.

Codex review notes: model internal, reasoning high; reviewed against cb785ed95b36.

Label changes

Label changes:

  • add rating: 🦪 silver shellfish: Overall readiness is 🦪 silver shellfish; proof is 🦪 silver shellfish and patch quality is 🐚 platinum hermit.
  • remove rating: 🧂 unranked krab: Current PR rating is rating: 🦪 silver shellfish, so this older rating label is no longer current.

Label justifications:

  • P2: This is a bounded hardening change for a cleanup path with limited blast radius, not evidence of an active user-facing outage.
  • merge-risk: 🚨 compatibility: The new cap intentionally changes handling of pre-existing oversized pointer files from attempted parsing to the null fallback path.
  • rating: 🦪 silver shellfish: Overall readiness is 🦪 silver shellfish; proof is 🦪 silver shellfish and patch quality is 🐚 platinum hermit.
  • status: 📣 needs proof: The PR needs real behavior proof before ClawSweeper can clear the contributor ask. Needs real behavior proof before merge: The PR reports tests and typechecking but provides no inspectable after-fix runtime evidence; add a redacted terminal transcript, log, recording, or artifact showing ordinary and oversized pointer-file behavior before merge. After adding proof, update the PR body; ClawSweeper should re-review automatically. If it does not, the PR author or someone with repository write access can comment @clawsweeper re-review.
Evidence reviewed

PR surface:

Source +7. Total +7 across 1 file.

View PR surface stats
Area Files Added Removed Net
Source 1 8 1 +7
Tests 0 0 0 0
Docs 0 0 0 0
Config 0 0 0 0
Generated 0 0 0 0
Other 0 0 0 0
Total 1 8 1 +7

What I checked:

Likely related people:

  • cxbAsDev: Authored the merged bounded hook-metadata reader work and both currently overlapping bounded-read PRs, making them the clearest routing contact for consolidating this narrowly shared change. (role: adjacent bounded-read contributor; confidence: high; commits: 4b2b5bcf8955, 698fc7aeb667; files: src/trajectory/cleanup.ts, src/infra/regular-file.ts)
What the crustacean ranks mean
  • 🦀 challenger crab: rare, exceptional readiness with strong proof, clean implementation, and convincing validation.
  • 🦞 diamond lobster: very strong readiness with only minor maintainer review expected.
  • 🐚 platinum hermit: good normal PR, likely mergeable with ordinary maintainer review.
  • 🦐 gold shrimp: useful signal, but proof or patch confidence is still limited.
  • 🦪 silver shellfish: thin signal; proof, validation, or implementation needs work.
  • 🧂 unranked krab: not merge-ready because proof is missing/unusable or there are serious correctness or safety concerns.
  • 🌊 off-meta tidepool: rating does not apply to this item.

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
  • ClawSweeper keeps one durable marker-backed review comment per issue or PR.
  • Re-runs edit this comment so the latest verdict, findings, and automation markers stay together instead of adding duplicate bot comments.
  • A fresh review can be triggered by eligible @clawsweeper re-review comments, exact-item GitHub events, scheduled/background review runs, or manual workflow dispatch.
  • PR/issue authors and users with repository write access can comment @clawsweeper re-review or @clawsweeper re-run on an open PR or issue to request a fresh review only.
  • Maintainers can also comment @clawsweeper review to request a fresh review only.
  • Fresh-review commands do not start repair, autofix, rebase, CI repair, or automerge.
  • Maintainer-only repair and merge flows require explicit commands such as @clawsweeper autofix, @clawsweeper automerge, @clawsweeper fix ci, or @clawsweeper address review.
  • Maintainers can comment @clawsweeper explain to ask for more context, or @clawsweeper stop to stop active automation.
Review history (4 earlier review cycles)
  • reviewed 2026-07-18T08:00:51.743Z sha c191fe5 :: needs real behavior proof before merge. :: [P1] Decode the bounded reader result before parsing JSON
  • reviewed 2026-07-18T09:19:08.607Z sha f8a50f1 :: needs real behavior proof before merge. :: none
  • reviewed 2026-07-18T09:53:31.288Z sha 10cd690 :: needs real behavior proof before merge. :: none
  • reviewed 2026-07-18T10:00:30.010Z sha 8a59244 :: needs real behavior proof before merge. :: [P1] Decode the bounded reader buffer before parsing JSON

@cxbAsDev

cxbAsDev commented Jul 18, 2026

Copy link
Copy Markdown
Contributor Author

Fixed the P1 buffer destructuring issue: now correctly destructures { buffer } from readRegularFileSync before parsing. Branch rebased to current main. @clawsweeper re-review

@clawsweeper clawsweeper Bot mentioned this pull request Jul 18, 2026
Merged
@cxbAsDev

cxbAsDev commented Jul 18, 2026

Copy link
Copy Markdown
Contributor Author

Behavior Proof

Code Changes

  • Replaced JSON.parse(fs.readFileSync(pointerPath, "utf8")) with bounded readRegularFileSync({ maxBytes: TRAJECTORY_POINTER_MAX_BYTES }) using proper { buffer } destructuring and buffer.toString("utf8")
  • Size cap: 1 MB (trajectory pointer files are typically <1 KB)

Local Verification

  • pnpm tsgo:core — ✅ passes
  • pnpm test src/trajectory/cleanup.test.ts — ✅ 4 tests pass

Risk Assessment

This change replaces an unbounded fs.readFileSync with a bounded readRegularFileSync (1 MB cap) in the trajectory pointer file read path. Trajectory pointer files are typically <1 KB. An oversized pointer file gracefully returns null via the existing try-catch handler. No user-visible change under normal conditions.

@cxbAsDev
cxbAsDev force-pushed the fix/bound-trajectory-pointer-read branch from 10cd690 to 8a59244 Compare July 18, 2026 09:56
@cxbAsDev

cxbAsDev commented Jul 18, 2026

Copy link
Copy Markdown
Contributor Author

Branch rebased to latest main. @clawsweeper re-review

@cxbAsDev
fix(trajectory): decode buffer to string before JSON.parse in pointer…
698fc7a 
… file read

readRegularFileSync returns { buffer, stat }, not a string.
The original code passed the raw object to JSON.parse, causing a
TS2345 type error and runtime failure. Destructure buffer and call
buffer.toString('utf8') to produce the string JSON.parse expects.
@clawsweeper clawsweeper Bot added rating: 🦪 silver shellfish Thin PR readiness signal; proof, validation, or implementation needs work. and removed rating: 🧂 unranked krab Not merge-ready due to missing proof or serious correctness/safety concerns. labels Jul 18, 2026
@cxbAsDev

cxbAsDev commented Jul 18, 2026

Copy link
Copy Markdown
Contributor Author

This PR's change is now part of the consolidated PR #110516 which covers 5 bounded-read sites including this one. Closing in favor of PR #110516.

@cxbAsDev

cxbAsDev commented Jul 18, 2026

Copy link
Copy Markdown
Contributor Author

Superseded by PR #110516 which consolidates this bounded-read change alongside 4 other file-read fixes into a single landing candidate.

@cxbAsDev cxbAsDev closed this Jul 18, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

merge-risk: 🚨 compatibility 🚨 May break existing users, config, migrations, defaults, or upgrade paths. P2 Normal backlog priority with limited blast radius. rating: 🦪 silver shellfish Thin PR readiness signal; proof, validation, or implementation needs work. size: XS status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

1 participant

@cxbAsDev