Skip to content

fix(agents): preserve compaction summary extra params#75085

Closed
jax-yu wants to merge 2 commits into
openclaw:mainfrom
jax-yu:fix/compaction-summary-extra-params
Closed

fix(agents): preserve compaction summary extra params#75085
jax-yu wants to merge 2 commits into
openclaw:mainfrom
jax-yu:fix/compaction-summary-extra-params

Conversation

@jax-yu

@jax-yu jax-yu commented Apr 30, 2026

Copy link
Copy Markdown

Summary

  • Preserve configured OpenAI-compatible extra payload params, including chat_template_kwargs and supported extra_body fields, when safeguard compaction generates built-in LLM summaries.
  • Thread resolved provider params and the active thinking level into compaction safeguard runtime so summary calls inherit the same vLLM/Qwen thinking behavior as normal assistant turns.
  • Add a small @mariozechner/pi-coding-agent patch so generateSummary() can receive an optional onPayload hook without changing default behavior.

Root Cause

The normal assistant stream path already patched OpenAI-compatible request bodies, but the compaction summary fallback called generateSummary() directly. That dependency function forced summary completion setup internally and exposed no request payload hook, so vLLM params such as:

{
  "chat_template_kwargs": {
    "enable_thinking": false,
    "preserve_thinking": true
  }
}

never reached summary requests during compaction.

Tests

  • pnpm test src/agents/compaction.summarize-fallback.test.ts
  • pnpm test src/agents/pi-hooks/compaction-safeguard.test.ts
  • pnpm test src/agents/pi-embedded-runner-extraparams.test.ts
  • pnpm check
  • pnpm build

@openclaw-barnacle openclaw-barnacle Bot added agents Agent runtime and tooling size: L labels Apr 30, 2026
@jax-yu jax-yu changed the title Preserve compaction summary extra params fix(agents): preserve compaction summary extra params Apr 30, 2026
@jax-yu
jax-yu marked this pull request as ready for review April 30, 2026 13:08
@clawsweeper

clawsweeper Bot commented Apr 30, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Thanks for the contribution. I reviewed the branch, and this PR is not a good landing base for OpenClaw.

Close: the reported compaction-summary payload gap is a useful bug direction, but this branch is no longer a viable landing candidate because it targets removed Pi runtime paths, adds an obsolete root dependency patch, is conflicting, and lacks real behavior proof.

So I’m closing this PR rather than keeping an unmergeable branch open. A new narrow PR that carries only the useful part is welcome.

Review details

Best possible solution:

Close this stale branch and take the useful idea into a fresh narrow PR against the current agent-core/session compaction streamFn seam, without a root dependency patch or changelog edit.

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

Yes from source inspection, but not from a live run. Current main applies OpenAI-compatible extra params on normal stream paths, while safeguard summary calls still do not carry the same resolved stream/payload context.

Is this the best way to solve the issue?

No. The behavior direction is useful, but the submitted fix is now the wrong layer because current main has a local agent-core streamFn compaction seam and no longer uses the patched Pi dependency.

Security review:

Security review needs attention: Security review needs attention because the diff adds a root patched dependency and lockfile patch hash that now appear unnecessary on current main.

  • [medium] Root patched dependency added — package.json:1736
    The PR adds a root @mariozechner/pi-coding-agent patch and patches package dist files, but current main no longer uses that dependency; dependency patches are supply-chain-sensitive and should be removed unless explicitly approved.
    Confidence: 0.88

AGENTS.md: found and applied where relevant.

What I checked:

  • Repository policy read: Root and scoped agents policy were read fully; dependency patches, proof requirements, and agent-runtime review guidance affected this review. (AGENTS.md:1, 462c076a24e2)
  • Live PR state: Live GitHub data shows the PR is open, external-authored, CONFLICTING, stale-labeled, and still at head 58d90ee0aed01529f4f80297741aaf9131478ae9 with no review comments. (58d90ee0aed0)
  • Obsolete dependency patch: The PR adds a root patchedDependencies entry for @mariozechner/[email protected]; current main no longer has @mariozechner/pi-coding-agent in package.json or pnpm-lock.yaml, so the patch is stale supply-chain surface. (package.json:1736, 58d90ee0aed0)
  • Current compaction seam: Current main already routes session compaction through the local agent-core bridge with optional thinkingLevel and streamFn parameters, so a fix should use this seam instead of patching the removed Pi dependency. (src/agents/sessions/compaction/compaction.ts:65, 462c076a24e2)
  • Source-reproducible remaining gap: The current safeguard LLM path calls summarizeInStages without a stream function, thinking level, or resolved extra params, so the central behavior still needs a current-main implementation. (src/agents/agent-hooks/compaction-safeguard.ts:1113, 462c076a24e2)
  • Removed/renamed runtime paths: The PR diff touches src/agents/pi-embedded-runner/* and src/agents/pi-hooks/*, while current main uses src/agents/embedded-agent-runner/* and src/agents/agent-hooks/*; nine touched runtime paths are no longer current-main paths. (462c076a24e2)

Likely related people:

  • Vincent Koc: Recent current-main history for the renamed agent-core/embedded-runner compaction bridge and release surface points here as the best current-path routing candidate. (role: recent adjacent owner; confidence: medium; commits: 8c802aa68351; files: packages/agent-core/src/harness/compaction/compaction.ts, src/agents/sessions/compaction/compaction.ts, src/agents/embedded-agent-runner/extra-params.ts)
  • DhruvBhatia0: Authored the merged pluggable compaction provider registry work that changed the safeguard summary owner boundary adjacent to this PR. (role: feature owner; confidence: medium; commits: 12331f04631f; files: src/agents/compaction.ts, src/agents/pi-embedded-runner/extensions.ts, src/agents/pi-hooks/compaction-safeguard.ts)
  • rodrigouroz: Authored the merged identifier-preservation compaction work, which is adjacent to summary prompt and safeguard behavior touched by this PR. (role: compaction behavior contributor; confidence: medium; commits: 0fe6cf06b226; files: src/agents/compaction.ts, src/agents/pi-embedded-runner/extensions.ts, src/agents/pi-extensions/compaction-safeguard.ts)

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

@jax-yu
jax-yu force-pushed the fix/compaction-summary-extra-params branch from d0d14c0 to 78230e9 Compare April 30, 2026 14:45
@jax-yu
jax-yu force-pushed the fix/compaction-summary-extra-params branch from 78230e9 to 58d90ee Compare April 30, 2026 16:48
@openclaw-barnacle

openclaw-barnacle Bot commented May 30, 2026

Copy link
Copy Markdown

This pull request has been automatically marked as stale due to inactivity.
Please add updates or it will be closed.

@openclaw-barnacle openclaw-barnacle Bot added stale Marked as stale due to inactivity triage: needs-real-behavior-proof Candidate: external PR needs after-fix proof from a real setup. labels May 30, 2026
@clawsweeper clawsweeper Bot added the rating: 🌊 off-meta tidepool PR readiness rating does not apply to this item. label May 30, 2026
@barnacle-openclaw barnacle-openclaw Bot removed the stale Marked as stale due to inactivity label May 30, 2026
@ai-hpc ai-hpc mentioned this pull request Jun 11, 2026
Open
@openclaw-barnacle

openclaw-barnacle Bot commented Jun 14, 2026

Copy link
Copy Markdown

This pull request has been automatically marked as stale due to inactivity.
Please add updates or it will be closed.

@openclaw-barnacle openclaw-barnacle Bot added the stale Marked as stale due to inactivity label Jun 14, 2026
@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. and removed rating: 🌊 off-meta tidepool PR readiness rating does not apply to this item. labels Jun 14, 2026
@clawsweeper

clawsweeper Bot commented Jun 14, 2026

Copy link
Copy Markdown
Contributor

ClawSweeper applied the proposed close for this PR.

@clawsweeper clawsweeper Bot closed this Jun 14, 2026
@clawsweeper clawsweeper Bot mentioned this pull request Jun 26, 2026
Open
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

agents Agent runtime and tooling merge-risk: 🚨 compatibility 🚨 May break existing users, config, migrations, defaults, or upgrade paths. P2 Normal backlog priority with limited blast radius. rating: 🧂 unranked krab Not merge-ready due to missing proof or serious correctness/safety concerns. size: L stale Marked as stale due to inactivity status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask. triage: needs-real-behavior-proof Candidate: external PR needs after-fix proof from a real setup.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

1 participant

@jax-yu