Skip to content

feat: add compaction worker extension#705

Open
gugu91 wants to merge 3 commits intomainfrom
feat/custom-compaction
Open

feat: add compaction worker extension#705
gugu91 wants to merge 3 commits intomainfrom
feat/custom-compaction

Conversation

@gugu91
Copy link
Copy Markdown
Owner

@gugu91 gugu91 commented May 3, 2026

Summary

  • add a new opt-in @gugu910/pi-compaction-worker Pi package for custom/model-aware compaction
  • support exact/glob profile matching, prepare/trigger thresholds, side summary preparation, live custom fallback, stale prepared-summary validation, and /compaction-worker-status
  • rebuild compaction preparation with exported Pi utilities (findCutPoint, buildSessionContext) so per-profile keepRecentTokens can control the kept boundary without private Pi imports

Testing

  • pnpm prepush
  • pnpm --filter @gugu910/pi-compaction-worker build

Refs #595

@gugu91
Copy link
Copy Markdown
Owner Author

gugu91 commented May 3, 2026

Will's reviewer agent Token Marmot

Verdict

  • Block
  • The public Pi exports and workspace/build wiring look sound, but I found one correctness bug that drops cumulative file-operation context after the first extension-driven compaction, plus one public-API compatibility issue around ignored custom compaction instructions.

Top findings

  • compaction-worker/index.ts:290-308, compaction-worker/retention.ts:98-110buildDetails() wraps Pi's {readFiles, modifiedFiles} payload under details, while retention only reloads top-level details.readFiles / details.modifiedFiles. After the first hook-generated compaction, later compactions stop carrying forward earlier file-op history.
  • compaction-worker/index.ts:164-179, compaction-worker/index.ts:367-417session_before_compact ignores event.customInstructions and always uses the fixed worker prompt. That overrides Pi's public compaction API semantics, and a prepared generic summary can be reused even when a caller explicitly requested different compaction instructions.

Verification

  • pnpm --filter @gugu910/pi-compaction-worker test
  • pnpm --filter @gugu910/pi-compaction-worker typecheck
  • pnpm --filter @gugu910/pi-compaction-worker lint
  • pnpm --filter @gugu910/pi-compaction-worker build
  • pnpm prepush
  • inspected published @mariozechner/pi-coding-agent exports via npm view and tarball inspection

@gugu91 gugu91 force-pushed the feat/custom-compaction branch from 73ed126 to d2ff16f Compare May 3, 2026 10:25
@gugu91
Copy link
Copy Markdown
Owner Author

gugu91 commented May 3, 2026

Will's reviewer agent Trace Platypus

Verdict

  • Block
  • The earlier blockers around top-level file-op details, live custom-instruction threading, and zero skip-margin handling look fixed, and the targeted package checks pass. I'm still blocking because the custom-instructions bypass path leaves any existing prepared summary marked ready, which suppresses the next prepare cycle after that compaction.

Review Scope

  • PR reviewed: #705 https://github.com/gugu91/extensions/pull/705
  • Files examined: compaction-worker/{index.ts,helpers.ts,helpers.test.ts,retention.ts,config.ts,README.md,package.json,tsconfig.json}, types/pi-coding-agent.d.ts, scripts/build-package.mjs, pnpm-workspace.yaml
  • Commands run: gh pr view 705 --json ..., git diff origin/main...HEAD -- compaction-worker/index.ts compaction-worker/retention.ts compaction-worker/helpers.ts compaction-worker/helpers.test.ts types/pi-coding-agent.d.ts, pnpm --filter @gugu910/pi-compaction-worker test, pnpm --filter @gugu910/pi-compaction-worker typecheck, pnpm --filter @gugu910/pi-compaction-worker lint
  • PiComms: pr-705-review (posting in this review run)
  • GitHub: PR summary comment posted

Findings

Critical (must fix)

  • compaction-worker/index.ts:418, compaction-worker/index.ts:469, compaction-worker/helpers.ts:238 — custom-instruction compactions leave stale prepared summaries in ready state
    • Why it matters: after a compaction that bypasses the prepared summary because the caller supplied custom instructions, the compaction boundary has changed and the old prepared summary is no longer reusable. But session_compact never clears it, so decideRuntimeAction() keeps treating it as a valid ready summary and refuses to start a fresh prepare job once usage crosses the prepare threshold.
    • Suggested fix: clear or mark runtime.prepared stale/used on session_compact (or otherwise when the bypassed compaction completes), then add a regression test that covers ready prepared -> external custom compaction -> next prepare threshold.

Warnings (should fix)

  • None.

Suggestions (consider)

  • None.

Checklist

  • Correctness: fail — the custom-instruction path still leaves the runtime state inconsistent for the next compaction cycle.
  • Architecture & maintainability: fail — compaction completion updates differ between trigger-driven and externally instructed flows, which makes the state machine easy to desynchronize.
  • Security & reliability: fail — no security issue found, but the stale ready state weakens the reliability of the prepare/trigger workflow.
  • Tests & verification: fail — test, typecheck, and lint passed for @gugu910/pi-compaction-worker, but there is no regression coverage for this lifecycle gap.

Summary

  • Top-level readFiles / modifiedFiles are now emitted and read compatibly, so the earlier file-op carry-forward bug looks fixed.
  • External custom instructions now bypass prepared summaries and are threaded into live summary generation.
  • Explicit builtinSkipMarginPercent: 0 is now preserved and covered by a targeted unit test.
  • One blocker remains: after a custom-instruction compaction, the old prepared summary stays ready and suppresses the next proactive prepare cycle.

@gugu91 gugu91 force-pushed the feat/custom-compaction branch from d2ff16f to a155449 Compare May 3, 2026 10:32
@gugu91
Copy link
Copy Markdown
Owner Author

gugu91 commented May 3, 2026

Will's reviewer agent Spline Wombat

Verdict

  • Approve
  • The remaining stale-ready-state blocker looks resolved. compaction-worker/index.ts:289-293 clears prepared state after worker-triggered compactions, and compaction-worker/index.ts:469-475 now clears it on any successful session_compact, so external custom-instruction compactions no longer leave a ready summary behind to suppress the next prepare cycle.

Verification

  • pnpm --filter @gugu910/pi-compaction-worker test
  • pnpm --filter @gugu910/pi-compaction-worker typecheck
  • pnpm --filter @gugu910/pi-compaction-worker lint

Summary

@gugu91 gugu91 force-pushed the feat/custom-compaction branch from a155449 to 07b645a Compare May 3, 2026 10:41
@gugu91
Copy link
Copy Markdown
Owner Author

gugu91 commented May 3, 2026

Will's reviewer agent Patch Puffin

Verdict

  • Block
  • The package wiring and CI are in good shape, but two logic issues still make the model-aware policy unreliable: the default built-in skip margin suppresses legitimate late triggers on large-context models, and cooldown state leaks across session/tree resets.

Review Scope

  • PR reviewed: #705 https://github.com/gugu91/extensions/pull/705
  • Files examined: compaction-worker/{README.md,config.ts,helpers.ts,helpers.test.ts,index.ts,package.json,retention.ts}, types/pi-coding-agent.d.ts, scripts/build-package.mjs, pnpm-workspace.yaml
  • Commands run: gh issue view 595 --json ...; gh pr view 705 --json ...; gh pr diff 705 --name-only; git diff --stat origin/main...HEAD; pnpm --filter @gugu910/pi-compaction-worker test; pnpm --filter @gugu910/pi-compaction-worker typecheck; pnpm --filter @gugu910/pi-compaction-worker lint; pnpm --filter @gugu910/pi-compaction-worker build; pnpm prepush; node --experimental-strip-types --input-type=module ... (skip-margin and profile-match reproductions); grep -n "lastCompactAtMs" compaction-worker/index.ts compaction-worker/helpers.ts
  • PiComms: pr-705-review — comment posted
  • GitHub: summary comment posted

Findings

Critical (must fix)

  • compaction-worker/helpers.ts:8-9, 227-233, 426-431 — the default 5% built-in skip margin can nullify valid custom trigger/reserve settings on large-context models

    • Why it matters: this worker is supposed to make per-model budgets effective, but with a 1M-token model the default skip margin is 50k tokens. A reasonable late trigger such as reserveTokens: 65536 resolves to 934,464, yet decideRuntimeAction() returns near-built-in-threshold instead of compacting. That means the profile override never takes effect unless the user already knows to tune an undocumented internal knob.
    • Suggested fix: base the race-avoidance margin on a small absolute token buffer or on the actual gap to Pi's built-in threshold, not on a percentage of the entire context window; document it; and add a regression test using a large-context model plus a late trigger / reserveTokens: 65536 override.
  • compaction-worker/index.ts:291, 473, 478-493; compaction-worker/helpers.ts:221-223 — cooldown survives session_start / session_tree, so one compaction can suppress the next session or branch

    • Why it matters: the worker clears prepared state when sessions/trees change, but it keeps lastCompactAtMs. If you compact session A and then immediately open a different long session or branch, decideRuntimeAction() still returns cooldown for up to cooldownMs (60s by default). That breaks session isolation in the state machine and can skip the exact prepare/compact work the new branch needs.
    • Suggested fix: scope cooldown to session identity/branch, or at minimum clear lastCompactAtMs on session_start and session_tree, then add a regression test for compact -> session switch -> agent_end.

Warnings (should fix)

  • compaction-worker/helpers.ts:369-377 — overlapping profiles are matched by raw object insertion order, so a broad glob can shadow a more specific profile
    • Why it matters: users configuring both a general provider profile and a model-specific profile can silently get the wrong budgets and thresholds. I reproduced anthropic/* beating anthropic/claude-opus-* when declared first, which makes config behavior brittle and hard to debug.
    • Suggested fix: implement deterministic precedence (exact > more specific glob > broad glob) or reject/diagnose overlapping matches, and add tests for overlapping profiles.

Suggestions (consider)

  • compaction-worker/README.md:17-40 — document the extra guardrail knobs that materially affect when the worker will actually compact
    • Why it may help: builtinReserveTokens and especially builtinSkipMarginPercent change runtime behavior today, but they are not surfaced in the config docs, so misfires look like broken profile matching instead of an intentional safety margin.

Checklist

  • Correctness: fail — the default skip margin can disable intended per-model triggers, and cooldown is not reset on session/tree changes.
  • Architecture & maintainability: fail — runtime state is partly session-scoped and partly global, and profile precedence is implicit rather than deterministic.
  • Security & reliability: fail — no direct security issue found, but the state machine can silently skip expected compaction on new sessions and branches.
  • Tests & verification: fail — package tests/lint/typecheck/build and root prepush passed, but there is no regression coverage for the failing lifecycle/config paths above.

Summary

  • quality is green and the package/test wiring looks healthy, so the remaining concerns are logic issues rather than build plumbing.
  • The default built-in skip margin is too large for large-context models and can prevent configured late-trigger or reserve-based policies from ever firing.
  • Cooldown state currently leaks across session_start / session_tree, which breaks session/branch isolation in the compaction state machine.
  • Overlapping profile matches still need explicit precedence or validation before this is predictable to users.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant