feat(history): bulk & policy-driven history compaction (pm-f3pa/pm-yj9w/pm-0pnz)#288
Conversation
…9w/pm-0pnz)
Mature `pm history-compact` from a single-stream command into a full
compaction-maintenance surface, and add a config-driven policy that surfaces
oversized streams through `pm health`.
Bulk mode (pm-f3pa, pm-yj9w):
- `pm history-compact` now accepts bulk selectors (mutually exclusive with a
positional <id>): `--ids <a,b,c>` (explicit list), `--all-over <N>` (entry
threshold), `--scope closed|all-streams` (lifecycle filter), plus
`--min-entries <N>` (default 3) to skip already-compact streams.
- Pure selection lives in core/history/history-compact-bulk.ts
(selectHistoryCompactBulkTargets); runHistoryCompactBulk orchestrates by
reusing the single-item runHistoryCompact. One failing stream never aborts
the pass; the command exits non-zero only if any stream errored. --dry-run
supported. Result reports totals + per-stream rows
(compacted / skipped+skip_reason / errored).
- Orphan streams (history file, no item) are unbucketed and excluded from
scope=closed.
Compaction policy (pm-0pnz):
- New `history.compact_policy` settings block { enabled, max_entries(500),
trigger(health_warn|auto) } with validator, deterministic ordering, and
nested config keys (history_compact_policy_{enabled,max_entries,trigger}).
- `pm health` storage check raises an advisory (non-blocking) warning for
streams over max_entries, with a remediation_map (pm history-compact <id>,
rewritten to `--scope all-streams` when more than one stream is over). The
threshold scan only reads stream contents when the policy is enabled, so the
default path stays a cheap directory listing.
- When the policy is enabled and `--all-over` is omitted, bulk mode uses
max_entries as the default scan threshold.
Wiring: CLI flags + contracts (HISTORY_COMPACT_FLAG_CONTRACTS, parameter
contract, golden full.json), bash/zsh/fish completions, help content, remediation
registry, COMMANDS.md + CONFIGURATION.md. 100% test coverage; static + docs gates
green; CHANGELOG regenerated.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01TgxDnvgTbXNgUPV6ihKgTX
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
|
@coderabbitai full review |
|
@gemini-code-assist please review |
|
@codex review |
|
Warning Review limit reached
More reviews will be available in 10 minutes and 12 seconds. Learn how PR review limits work. Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable the review add-on in the billing tab to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file). ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based credits. 🚦 How do rate limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan refill rate. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, the refill rate gradually slows as usage increases. The highest same-day bursts are limited more strictly. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Repository UI (base), Organization UI (inherited) Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (33)
📝 WalkthroughWalkthroughAdds a config-driven ChangesBulk history-compact and compact_policy implementation
Sequence Diagram(s)sequenceDiagram
participant User
participant CLI as register-mutation
participant Validator as assertHistoryCompactTarget
participant Bulk as runHistoryCompactBulk
participant FS as history JSONL files
participant Selector as selectHistoryCompactBulkTargets
participant Compact as runHistoryCompact
User->>CLI: pm history-compact --all-over 50 --scope closed --min-entries 3
CLI->>Validator: assertHistoryCompactTarget(id=undefined, {allOver, scope})
Validator-->>CLI: ok
CLI->>Bulk: HistoryCompactBulkCommandOptions + GlobalOptions
Bulk->>FS: list streams, count entries, apply read hooks
Bulk->>Selector: candidates + criteria (allOver, scope, minEntries)
Selector-->>Bulk: HistoryCompactBulkSelectionRow array
loop each selected stream
Bulk->>Compact: dryRun, author, message, force
Compact-->>Bulk: result or caught per-stream error
end
Bulk-->>CLI: HistoryCompactBulkResult with totals and items
CLI->>User: print result table, non-zero exit when items_errored is positive
sequenceDiagram
participant User
participant runHealth
participant countHistoryStreams
participant FS as history JSONL files
participant RemediationRegistry
User->>runHealth: pm health
runHealth->>countHistoryStreams: pmRoot + settings.history.compact_policy
countHistoryStreams->>FS: read each stream entry count when policy enabled
countHistoryStreams-->>runHealth: count, over_threshold list, warnings, max_entries
runHealth->>runHealth: set storage status to warn when over_threshold non-empty
runHealth->>RemediationRegistry: look up history_stream_over_compact_threshold per id
RemediationRegistry-->>runHealth: pm history-compact id
runHealth->>runHealth: override to --scope all-streams when multiple streams over threshold
runHealth-->>User: health report with advisory warn, overall status stays ok
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
✅ Action performedFull review finished. |
|
To use Codex here, create a Codex account and connect to github. |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a config-driven auto-compact policy and bulk history compaction capabilities for the pm CLI tool, allowing operators to sweep and compact multiple history streams. Key additions include the history.compact_policy settings block, advisory warnings in pm health for over-threshold streams, and new bulk selectors (--ids, --all-over, and --scope) for pm history-compact. Feedback on the changes highlights opportunities to optimize memory usage in src/cli/commands/health.ts and src/cli/commands/history-compact.ts by replacing line-splitting string allocations with character-by-character scanning when counting history stream entries.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| const raw = await fs.readFile(streamPath, "utf8"); | ||
| let entries = 0; | ||
| for (const line of raw.split(/\r?\n/)) { | ||
| if (line.trim().length > 0) { | ||
| entries += 1; | ||
| } | ||
| } |
There was a problem hiding this comment.
Reading the entire history stream file and splitting it by line (raw.split(/\r?\n/)) creates a large array of strings in memory. For large history files (e.g., >19MB), this causes significant memory allocation overhead and garbage collection thrashing during health checks. Since we only need to count non-empty lines, we can scan the string character-by-character in
| const raw = await fs.readFile(streamPath, "utf8"); | |
| let entries = 0; | |
| for (const line of raw.split(/\r?\n/)) { | |
| if (line.trim().length > 0) { | |
| entries += 1; | |
| } | |
| } | |
| const raw = await fs.readFile(streamPath, "utf8"); | |
| let entries = 0; | |
| let hasChars = false; | |
| for (let i = 0; i < raw.length; i++) { | |
| const code = raw.charCodeAt(i); | |
| if (code === 10) { | |
| if (hasChars) { | |
| entries += 1; | |
| hasChars = false; | |
| } | |
| } else if (code !== 13 && code !== 32 && code !== 9) { | |
| hasChars = true; | |
| } | |
| } | |
| if (hasChars) { | |
| entries += 1; | |
| } |
| function countHistoryStreamEntries(raw: string): number { | ||
| let count = 0; | ||
| for (const line of raw.split(/\r?\n/)) { | ||
| if (line.trim().length > 0) { | ||
| count += 1; | ||
| } | ||
| } | ||
| return count; | ||
| } |
There was a problem hiding this comment.
Splitting the entire history stream file by line (raw.split(/\r?\n/)) to count entries allocates a large array of strings. For oversized history streams, this introduces unnecessary memory overhead and CPU cycles. We can optimize this to scan the string character-by-character in
function countHistoryStreamEntries(raw: string): number {
let count = 0;
let hasChars = false;
for (let i = 0; i < raw.length; i++) {
const code = raw.charCodeAt(i);
if (code === 10) {
if (hasChars) {
count += 1;
hasChars = false;
}
} else if (code !== 13 && code !== 32 && code !== 9) {
hasChars = true;
}
}
if (hasChars) {
count += 1;
}
return count;
}
There was a problem hiding this comment.
Code Review
This pull request introduces a config-driven history compaction policy and a bulk compaction mode for pm history-compact, allowing operators to automatically flag and sweep oversized history streams based on entry count thresholds or lifecycle status. It also integrates these checks into pm health as advisory storage warnings. The review feedback highlights a potential issue in src/cli/commands/health.ts where looping over and overwriting all keys in remediationMap could accidentally overwrite other unrelated storage remediations, and suggests targeting only the specific history_stream_over_compact_threshold key instead.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| if (check.name === "storage" && historySummary.over_threshold.length > 1) { | ||
| for (const code of Object.keys(remediationMap)) { | ||
| remediationMap[code] = "pm history-compact --scope all-streams"; | ||
| } | ||
| } |
There was a problem hiding this comment.
Looping over all keys of remediationMap and overwriting them with the bulk compaction command is risky. If other warnings are present in the storage check (or added in the future), their remediations will be incorrectly overwritten.
It is safer and more precise to target only the specific warning code history_stream_over_compact_threshold.
| if (check.name === "storage" && historySummary.over_threshold.length > 1) { | |
| for (const code of Object.keys(remediationMap)) { | |
| remediationMap[code] = "pm history-compact --scope all-streams"; | |
| } | |
| } | |
| if (check.name === "storage" && historySummary.over_threshold.length > 1) { | |
| if (remediationMap["history_stream_over_compact_threshold"] !== undefined) { | |
| remediationMap["history_stream_over_compact_threshold"] = "pm history-compact --scope all-streams"; | |
| } | |
| } |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/COMMANDS.md`:
- Line 594: The documentation for the history-compact command currently presents
the `--ids`, `--all-over`, and `--scope` options in a way that suggests they can
be combined (using "and/or"), but the CLI actually rejects mixing `--ids` with
the scan selectors. Clarify in the COMMANDS.md file that `--ids` is mutually
exclusive with `--all-over` and `--scope`, and that users can only select
streams using one of these three modes at a time. Restructure the wording to
make it explicit that these are three separate, mutually exclusive selection
approaches rather than options that can be mixed together.
In `@docs/CONFIGURATION.md`:
- Line 66: The documentation for `history.compact_policy.max_entries` currently
describes its behavior without clarifying that it only applies as the default
`--all-over` threshold when `history.compact_policy.enabled` is true. Revise the
configuration documentation entry for `history.compact_policy.max_entries` to
explicitly state this conditional dependency, making it clear that the default
threshold behavior is only active when the compact policy is enabled.
In `@src/cli/commands/history-compact.ts`:
- Around line 538-544: The operations on lines 540 and 541 (fs.readFile and
runActiveOnReadHooks) can throw exceptions that will terminate the entire
history-compact command instead of isolating the error to just that stream. Wrap
these operations in a try-catch block within the for loop that iterates over
historyFiles, and when an exception is caught, handle it gracefully by either
skipping that entry or adding it to candidates with error metadata, then
continue the loop to process remaining files instead of letting the exception
propagate.
In `@src/cli/register-mutation.ts`:
- Around line 1336-1340: The --before option is documented as single-id mode
only, but when id is omitted (bulk mode), the option is silently ignored rather
than throwing an error. This can lead to unintended data compaction. Add
validation logic near where the id parameter is checked (around line 1374) to
detect when --before is provided without an id and reject the operation with an
appropriate error message. Reference the option names like --before, --ids,
--all-over, and --scope in your validation to ensure the --before option
conflicts with bulk mode operations.
- Around line 1355-1358: The current implementation uses Number.parseInt for
parsing the allOver and minEntries options, which silently truncates invalid
input at the first non-numeric character (e.g., "10abc" becomes 10, "3.5"
becomes 3). Replace the Number.parseInt calls for both the allOver variable
assignment and the minEntries variable assignment with a strict integer parsing
approach that validates the entire input string is numeric. This should reject
partial numeric strings like "10abc" and decimal strings like "3.5" as invalid
rather than silently truncating them to pass validation.
In `@src/sdk/cli-contracts.ts`:
- Around line 1950-1954: The oneOfRequired constraint in the history-compact CLI
contract at line 1954 enforces that exactly one of id, ids, allOver, or scope
must be present, but the runtime assertHistoryCompactTarget allows allOver and
scope to be used together in scan mode, causing a schema-runtime mismatch.
Update the oneOfRequired array to include a valid combination option that allows
both allOver and scope together, in addition to the existing single-option
combinations, so the MCP schema validation matches the actual runtime behavior
for history-compact operations.
In `@tests/fixtures/contracts/full.json`:
- Around line 18012-18023: The scope enum values in the fixture are incorrect
for the history-compact command. Change the enum array from ["project",
"global"] to ["closed", "all-streams"] to match the PR objectives for
lifecycle-based filtering. Update the description to reflect that the scope
parameter filters by lifecycle state (closed or all-streams) rather than project
or global state. Update the examples array to use the correct valid values
("closed" and "all-streams") instead of the current project and global examples.
In `@tests/integration/history-compact-command.spec.ts`:
- Around line 730-733: The test "requires an initialized tracker" uses a
hardcoded path `/tmp/pm-compact-bulk-missing-root` that may already exist on
shared runners, causing nondeterministic test failures. Replace the hardcoded
path in the runHistoryCompactBulk call with a dynamically generated unique
temporary path using a method like generating a UUID or timestamp suffix,
ensuring a fresh path is used for each test execution.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: e0d695bb-06ca-4387-bdec-60a0fb68399f
📒 Files selected for processing (32)
.agents/pm/features/pm-0pnz.toon.agents/pm/features/pm-f3pa.toon.agents/pm/features/pm-yj9w.toon.agents/pm/history/pm-0pnz.jsonl.agents/pm/history/pm-f3pa.jsonl.agents/pm/history/pm-yj9w.jsonlCHANGELOG.mddocs/COMMANDS.mddocs/CONFIGURATION.mdsrc/cli/commands/completion.tssrc/cli/commands/health.tssrc/cli/commands/history-compact.tssrc/cli/help-content.tssrc/cli/register-mutation.tssrc/core/config/nested-settings.tssrc/core/diagnostics/remediation.tssrc/core/history/history-compact-bulk.tssrc/core/shared/constants.tssrc/core/store/settings-validator.tssrc/core/store/settings.tssrc/sdk/cli-contracts.tssrc/types.tstests/fixtures/contracts/full.jsontests/integration/history-compact-command.spec.tstests/integration/release-readiness-runtime.spec.tstests/unit/commands/completion-command.spec.tstests/unit/commands/config-key-aliases-search.spec.tstests/unit/commands/health-command.spec.tstests/unit/commands/register-commands.spec.tstests/unit/core/history/history-compact-bulk.spec.tstests/unit/core/store/settings-store.spec.tstests/unit/core/store/settings-validator.spec.ts
- Replace `--scope closed|all-streams` with `--closed` / `--all-streams` booleans. The MCP `pm` tool exposes a single shared `scope` param (enum project|global), so a history-compact `--scope` would advertise the wrong enum and reject closed|all-streams. Booleans are collision-free and mirror `history-repair --all`. New `closed`/`allStreams` (boolean) + `allOver`/`minEntries` (number) params are declared in the MCP property table so the generated schema is correctly typed (previously `allOver`/`minEntries` fell back to string). - Reject `--before` in bulk mode instead of silently ignoring it (it only applies to single-id compaction). - Strict non-negative-integer parsing for `--all-over`/`--min-entries`: reject "10abc"/"3.5"/"-3" that Number.parseInt would silently truncate. - Drop `oneOfRequired` from the history-compact parameter contract: scan selectors (`allOver` + `closed`/`allStreams`) legitimately combine, which a one-of rule rejects. Mode/exclusivity is enforced at runtime by assertHistoryCompactTarget (CLI + MCP). - Use a deterministic guaranteed-absent path in the not-initialized test instead of a hardcoded /tmp path. 100% coverage maintained; static + docs gates green; contract golden regenerated. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01TgxDnvgTbXNgUPV6ihKgTX
|
Thanks @coderabbitai — all five findings addressed in 3a1dde6:
100% coverage maintained, static + docs gates green, contract golden regenerated. @coderabbitai full review |
|
✅ Action performedFull review finished. |
#288) Address remaining CodeRabbit findings: - Bulk enumeration now wraps each stream read in try/catch: an unreadable stream (e.g. EISDIR) is isolated as an `errored` result row instead of aborting the whole pass, mirroring the per-stream compaction error handling. streams_considered + items_errored account for pre-selection failures. - COMMANDS.md: clarify that --ids is used on its own (not combined with the scan selectors) and that --closed/--all-streams are mutually exclusive. - CONFIGURATION.md: note that max_entries/trigger are inert unless the policy is enabled. 100% coverage maintained; static + docs gates green. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01TgxDnvgTbXNgUPV6ihKgTX
|
Addressed the remaining findings in 04e9244:
All 8 findings now resolved. 100% coverage, static + docs gates green. @coderabbitai full review |
|
✅ Action performedFull review finished. |
1 participant
Summary
Matures
pm history-compactfrom a single-stream command into a full compaction-maintenance surface, and adds a config-driven policy that surfaces oversized streams throughpm health. Combines three related storage-epic items (parentpm-o2kc) into one cohesive PR.pm-f3pa--all-over N/--idspm-yj9w--scope closed/all-streamscorpus GC sweeppm-0pnzpm healthadvisoryWhat shipped
Bulk mode (
pm-f3pa,pm-yj9w)pm history-compactaccepts bulk selectors (mutually exclusive with a positional<id>):--ids <a,b,c>— explicit list--all-over <N>— every stream with more than N entries--scope closed|all-streams— lifecycle filter--min-entries <N>(default 3) — skip already-compact streamscore/history/history-compact-bulk.ts(selectHistoryCompactBulkTargets);runHistoryCompactBulkorchestrates by reusing the single-itemrunHistoryCompact.--dry-runsupported.totals(streams_considered/selected/items_compacted/items_skipped/items_errored) + one row per stream (compacted/skipped+skip_reason/errored).scope=closed.Compaction policy (
pm-0pnz)history.compact_policyblock{ enabled, max_entries(500), trigger(health_warn|auto) }— validator, deterministic key ordering, and nested config keys (history_compact_policy_{enabled,max_entries,trigger}).pm healthstorage check raises an advisory (non-blocking) warning for streams overmax_entries, with aremediation_map(pm history-compact <id>, rewritten to--scope all-streamswhen >1 stream is over). The threshold scan only reads stream contents when the policy is enabled, so the default path stays a cheap directory listing.--all-overis omitted, bulk mode usesmax_entriesas the default scan threshold.Wiring & docs
CLI flags + contracts (
HISTORY_COMPACT_FLAG_CONTRACTS, parameter contract, goldenfull.json), bash/zsh/fish completions, help content, remediation registry,COMMANDS.md+CONFIGURATION.md.Validation
🤖 Generated with Claude Code