Skip to content

fix(runtime-logs): preserve PEM context across polling pages - #759

Merged
freeqaz-openai merged 6 commits into
mainfrom
delivery/runtime-log-context-2025
Oct 1, 2026
Merged

freeqaz-openai merged 6 commits into
mainfrom
delivery/runtime-log-context-2025

Conversation

@freeqaz-openai

@freeqaz-openai freeqaz-openai commented Sep 30, 2026 •

Copy link
Copy Markdown
Contributor

Preserve PEM masking across runtime-log polling pages by carrying authenticated pemOpen and pemAfterTime context in the cursor. The reader validates the consumed prefix and compares delivered lines with the prior cursor frontier. An ordered line strictly newer than a reliable prior frontier can close a carried block. Ordered same-page BEGIN and END at that newer timestamp can close it; equality with the prior frontier does not establish forward progress. Missing, invalid or reordered times retain conservative open context, and undelivered lookahead cannot advance or close it. Cursor identity, expiry, stream and view resets and per-read authorization remain enforced. Ordinary operational text stays visible; existing JSON and bracket rules are unchanged.

The cursor authorization negative test also changes the first MAC character with an explicit decoded-byte precondition. The previous final-AA construction could leave a valid cursor unchanged; this was a test defect, not a production authentication bypass. The type-only follow-up permits the already-used undefined value in two optional declarations; its JavaScript output was independently verified unchanged. The guide and owning flow distinguish the prior frontier from ordered same-page boundaries and document conservative persistence and initial-tail limits.

This update merges pinned main 33a05c987de2d5c91e0243ceea011d7489abadf9 into the reviewed 48c3a9d9 branch. All eight feature-path blobs remain identical to 48c3a9d9, every other leaf of that pinned main is preserved, and published ancestry is retained. Main changed 116 leaves since the previously tested base 13cd2e81, with no overlap in these eight paths. Receiving those changes is not a claim that they fix the historical CI failure.

Validation and remaining limits:

  • On the composed source, the actual runtime-log content test file executed 44 cases with zero failures, skips or cancellations, including its three in-process controller content routes. An initial separate four-route selection passed those same three routes but could not import the API-security file because the bounded source view had omitted an unchanged helper. That failed attempt is retained. After receiving the exact hash-verified helper and verifying its existing Driver import closure, only the missing cursor principal/view-binding and per-poll reauthorization route was run: 1 pass, 0 failures/skips. All four named routes therefore have fresh local passes on this composition. The three content routes overlap the 44-case file result; they are not additional distinct cases. No replacement controller, helper stub, assertion change or real cluster operation was used.
  • A previous distinct local qualification on tested merge d4aaadad executed all four named controller routes successfully, with 52 top-level tests filtered. That is predecessor evidence, not a fourth current-composition route pass or a historical hosted result.
  • Local tests used the actual controller/Fastify injection, IAM, cursor codec and sanitizer over the repository's in-memory fixture. The selected source, lock, export maps and installed dependency closure were checked without installation or dependency copying. These tests do not establish a listener, PostgreSQL, provider, Kubernetes, model or production-runtime outcome.
  • Configured Prettier 3.9.6 passed all eight files, and scoped git diff --check passed. Full workspace lint, typecheck, documentation build and CI have not been rerun locally. Previous source reviews and scoped checks remain evidence only for their stated tuples and scope.
  • Hosted run 36795515059, attempt 1, on published 48c3a9d9 remains failed. Its tested merge was d4aaadad with parents 13cd2e81 and 48c3a9d9; the individual baseline failure cause is unresolved. The two previously denied result artifacts remain unreceived and were not retried, refreshed or accessed by an alternate route. Passing local routes does not erase that failure. This new merge needs its own hosted required checks and named-case receiving.
  • The exact composed branch received one normal helper invocation after a successful dry run covering the complete eight-path diff. With a 120-second engine limit, it returned reviewer_unavailable / engine_failed, reviewer exit 124, no report; the wrapper was terminal after 120.86 seconds. External descendant settlement is unproven. This is unavailable review evidence, not a helper PASS. Earlier d25 and 48c helper timeouts remain separate; the type-only correction had no new invocation. No historical helper request was retried.

This draft still requires final-head required CI, the named route evidence, review-conversation and Claw disposition, and applicable repository protection checks before landing. Unknown initial log tails without an observed boundary remain best-effort; this change is not a general secret-detection guarantee.

@clawsweeper

clawsweeper Bot commented Sep 30, 2026 •

Copy link
Copy Markdown
Contributor

🦞👀
ClawSweeper picked this up.

Pull request received. I will update this pull request when review starts.

ClawSweeper review complete

ClawSweeper finished reviewing this revision. The review result is being finalized.

View the workflow run.

@clawsweeper clawsweeper Bot added P2 Normal priority bug or improvement with limited blast radius. rating: 🐚 platinum hermit Good normal PR readiness with ordinary maintainer review expected. status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR. labels Sep 30, 2026
@clawsweeper

clawsweeper Bot commented Sep 30, 2026 •

Copy link
Copy Markdown
Contributor

Codex review: needs changes before merge. Reviewed October 1, 2026, 5:32 AM ET / 09:32 UTC (Revision 6).

ClawSweeper review

What this changes

The PR carries authenticated PEM key-block masking state between container-log polls and adds regression coverage and documentation for timestamp uncertainty and view resets.

Merge readiness

⛔ Needs changes before merge - 1 item remains

The fix remains useful because current main loses PEM masking context between polls. No actionable patch defects remain, and the earlier documentation finding is addressed. Enterprise policy retains this PR for maintainer triage.

Priority: P2
Reviewed head: d8abe9a90d8c2d64d9b8049619c6e74c50e48185

Review scores

Measure Result What it means
Overall readiness 🐚 platinum hermit (4/6) A focused repair with meaningful regression coverage, corrected documentation, and no remaining actionable findings.
Proof confidence 🌊 off-meta tidepool Not applicable: Verified repository-admin authorship exempts ordinary contributor runtime proof. Reported after-fix controller passes cover serialized polling with synthetic Driver pages, not live Kubernetes; cursor state is transient and changes no durable stored-data contract.
Patch quality 🐚 platinum hermit (4/6) No actionable review findings were identified.

Verification

Check Result Evidence
Real behavior Not applicable Not applicable: Verified repository-admin authorship exempts ordinary contributor runtime proof. Reported after-fix controller passes cover serialized polling with synthetic Driver pages, not live Kubernetes; cursor state is transient and changes no durable stored-data contract.
Evidence reviewed 10 items Pinned introduced change: Reviewed the eight-path merge-base-to-head delta. The checkout is the original PR head; the supplied stale test merge was not used to infer removal of current-main behavior.
Current main still needs the fix: Main's reader sanitizes each delivered page without carried PEM state, and its block masker initializes open=false on every call. A middle polling page containing only a short PEM body fragment therefore lacks the observed opening boundary.
Conservative polling boundary: The reader checks timestamp ordering through the last delivered line, compares closure against the incoming cursor frontier, and preserves uncertainty without consuming undelivered lookahead.
Findings None None.
Security None None.

How this fits together

OpenClaw Control Plane reads container output through the selected Compute Driver after authorizing the exact Agent. Its runtime-log reader removes overlap, masks sensitive text, and returns bounded records plus a signed cursor to console, CLI, and API callers.

flowchart TD
  A[Console or CLI log poll] --> B[Exact Agent authorization]
  B --> C[Compute Driver container logs]
  C --> D[Validate cursor and delivered order]
  D --> E[Carry PEM masking context]
  E --> F[Redacted records and signed cursor]
  F --> A
Loading

Before merge

  • Complete next step (P2) - Confirm the captured body’s final-head named-case receiving and review-conversation holds are settled before landing.
Agent review details

Security

None.

Review metrics

Metric Value Why it matters
Production and test LOC production +116/-12; tests +400/-6 Production growth implements cursor validation and conservative masking continuity; most added lines cover regression scenarios.

Technical review

Best possible solution:

Retain one cursor-backed masking implementation that conservatively handles uncertain chronology while keeping ordinary operational text visible.

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

Yes, source establishes the current-main failure: poll an observed PEM opening, then a middle page containing only a short body fragment. Main resets block state between those pages; this review did not execute the reproduction.

Is this the best way to solve the issue?

Yes, carrying authenticated masking context through the existing cursor is a focused repair, and the documented chronology rules match the implementation.

AGENTS.md: found and applied where relevant.

Codex review notes: model internal, reasoning medium; reviewed against cc521e90a6f9.

Labels

Label changes:

No label changes.

Label justifications:

  • P2: This is a bounded runtime-log redaction repair with documented best-effort limits and no introduced blocking defect.
  • rating: 🐚 platinum hermit: Overall readiness is 🐚 platinum hermit; proof is 🌊 off-meta tidepool and patch quality is 🐚 platinum hermit.
  • status: 👀 ready for maintainer look: ClawSweeper has no concrete contributor-facing blocker left for this PR. Not applicable: Verified repository-admin authorship exempts ordinary contributor runtime proof. Reported after-fix controller passes cover serialized polling with synthetic Driver pages, not live Kubernetes; cursor state is transient and changes no durable stored-data contract.

Evidence

What I checked:

  • Pinned introduced change: Reviewed the eight-path merge-base-to-head delta. The checkout is the original PR head; the supplied stale test merge was not used to infer removal of current-main behavior. (d8abe9a90d8c)
  • Current main still needs the fix: Main's reader sanitizes each delivered page without carried PEM state, and its block masker initializes open=false on every call. A middle polling page containing only a short PEM body fragment therefore lacks the observed opening boundary. (packages/occ/src/runtime-logs/redact.ts:302, cc521e90a6f9)
  • Conservative polling boundary: The reader checks timestamp ordering through the last delivered line, compares closure against the incoming cursor frontier, and preserves uncertainty without consuming undelivered lookahead. (packages/occ/src/runtime-logs/read.ts:285, d8abe9a90d8c)
  • Authorization remains independent of cursor state: Every controller poll still resolves the exact Agent and revision and authorizes read_logs/administer plus read before invoking the Driver. The new fields affect masking context, not permission to perform I/O. (packages/occ/src/index.ts:2465, d8abe9a90d8c)
  • Prior finding addressed: The guide now distinguishes timestamps equal to the incoming frontier from ordered same-page BEGIN and END at a newer timestamp, addressing fix(runtime-logs): preserve PEM context across polling pages #759 (comment). (docs/guides/topics/agent-logs.md:140, d8abe9a90d8c)
  • Regression evidence and its scope: The captured body reports 44 content cases and a separate successful principal/view-binding and reauthorization route on this composition. Reviewed tests exercise the real reader, codec, sanitizer, and controller with synthetic Driver pages; they do not prove live Kubernetes behavior. No tests or target helpers were executed during this review. (tests/conformance/runtime-logs-content.test.mjs:666, d8abe9a90d8c)

Likely related people:

  • freeqaz: Suggested for follow-up; no historical authorship or introduction is verified. (role: unverified routing candidate; confidence: low)
  • kevinlin-openai: Suggested for follow-up; no historical authorship or introduction is verified. (role: unverified routing candidate; confidence: low)

Rating scale

Score Internal tier Crab rank Meaning
6/6 S 🦀 challenger crab Exceptional readiness
5/6 A 🦞 diamond lobster Very strong readiness
4/6 B 🐚 platinum hermit Good normal PR; ordinary maintainer review
3/6 C 🦐 gold shrimp Useful, but confidence is limited
2/6 D 🦪 silver shellfish Proof or implementation needs work
1/6 F 🧂 unranked krab Not merge-ready
N/A NA 🌊 off-meta tidepool Rating does not apply

Overall follows the weaker of proof and patch quality.
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.

Workflow

  • 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.

History

Review history (5 earlier review cycles)
  • reviewed 2026-09-30T21:36:16.364Z sha d25f29b :: needs maintainer review before merge. :: none
  • reviewed 2026-09-30T21:48:52.057Z sha a4cddf2 :: needs maintainer review before merge. :: none
  • reviewed 2026-09-30T23:59:08.648Z sha a4cddf2 :: needs changes before merge. :: [P3] Narrow the equal-timestamp masking guarantee
  • reviewed 2026-10-01T00:24:09.172Z sha 48c3a9d :: needs maintainer review before merge. :: none
  • reviewed 2026-10-01T08:56:52.739Z sha d8abe9a :: needs changes before merge. :: none

Comment thread docs/guides/topics/agent-logs.md Outdated
Distinguish equality with the incoming cursor frontier from ordered same-page
BEGIN and END boundaries at a newer timestamp. Preserve masking limits.
@freeqaz-openai
freeqaz-openai marked this pull request as ready for review October 1, 2026 09:29
@freeqaz-openai
freeqaz-openai requested a review from a team as a code owner October 1, 2026 09:29
@freeqaz-openai
freeqaz-openai merged commit 2bb2e51 into main Oct 1, 2026
32 of 34 checks passed
@freeqaz-openai
freeqaz-openai deleted the delivery/runtime-log-context-2025 branch October 1, 2026 09:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

P2 Normal priority bug or improvement with limited blast radius. rating: 🐚 platinum hermit Good normal PR readiness with ordinary maintainer review expected. status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants