Skip to content

fix(session-start): support ECC_SESSION_RETENTION_DAYS opt-out + document env var (#2151) - #2163

Merged
affaan-m merged 2 commits into
affaan-m:mainfrom
gaurav0107:fix/2151-session-tmp-files-accumulate-retention-o
Jun 7, 2026
Merged

affaan-m merged 2 commits into
affaan-m:mainfrom
gaurav0107:fix/2151-session-tmp-files-accumulate-retention-o

Conversation

@gaurav0107

@gaurav0107 gaurav0107 commented Jun 5, 2026 •

Copy link
Copy Markdown
Contributor

Summary

  • Extend getSessionRetentionDays() so ECC_SESSION_RETENTION_DAYS=0|off|false|disabled|never|none disables session-tmp pruning entirely. Issue [BUG]: Session tmp files accumulate indefinitely #2151 follow-up — the underlying retention pass landed previously; this closes the configurability + discoverability gaps the original report asked for. Default behavior is unchanged.
  • Document ECC_SESSION_RETENTION_DAYS in the README "Hook Runtime Controls" section alongside the other ECC_SESSION_* knobs (was previously undocumented, so users could not discover the setting).
  • Add three regression tests covering opt-out via 0, opt-out via off, and garbage-value fallback to default 30.

Fixes #2151

Verification

  • node tests/hooks/hooks.test.js — 240/240 green (incl. 3 new retention tests).
  • node tests/run-all.js — 2622/2622 green.
  • npx eslint scripts/hooks/session-start.js tests/hooks/hooks.test.js — clean.
  • node scripts/ci/validate-no-personal-paths.js — clean.
  • node scripts/ci/check-unicode-safety.js — clean.
  • node scripts/ci/validate-hooks.js — 28 matchers validated.
  • node scripts/ci/validate-rules.js — 115 files validated.
  • Manual: ECC_SESSION_RETENTION_DAYS=0 … skips the prune pass; ECC_SESSION_RETENTION_DAYS=14 … prunes at 14 days; ECC_SESSION_RETENTION_DAYS=garbage … falls back to 30-day default.

Summary by cubic

Adds opt-out support for session-tmp pruning via ECC_SESSION_RETENTION_DAYS and documents the env var. Default 30-day behavior stays the same with clear logging when pruning is disabled. Fixes #2151.

  • Bug Fixes
    • Treat 0/off/false/disabled/never/none as opt-out; skip pruning and log "Pruning disabled via ECC_SESSION_RETENTION_DAYS". Garbage values fall back to 30 days.
    • Document ECC_SESSION_RETENTION_DAYS in README with default, all six opt-out values, and a Windows PowerShell example.
    • Add regression tests for opt-out (0, off) and default fallback.

Written for commit a75130d. Summary will update on new commits.

Review in cubic

Summary by CodeRabbit

  • New Features

    • Added environment variable to configure temporary session file retention (default: 30 days) with explicit options to disable pruning via common values (e.g., 0/off/false/disabled/never/none).
  • Documentation

    • README updated with configuration details, examples, and a Windows PowerShell snippet showing a 14-day example.
  • Tests

    • Added tests covering disabled retention and fallback behavior for invalid values.

…ment env var

The retention pass for *-session.tmp files (issue affaan-m#2151) landed previously,
but the env var that controls it was undocumented in the README and rejected
falsy values (0, off, disabled), silently falling back to the 30-day default.
Users who want to keep all sessions for forensic or research workflows had no
way to opt out.

This patch:

- Extends getSessionRetentionDays() so 0|off|false|disabled|never|none disables
  pruning entirely (returns null sentinel; default behavior unchanged).
- Updates the call site in main() to skip pruneExpiredSessions when retention
  is null and emits a clear "[SessionStart] Pruning disabled via
  ECC_SESSION_RETENTION_DAYS" log line so the operator can tell pruning is off.
- Documents ECC_SESSION_RETENTION_DAYS in the README "Hook Runtime Controls"
  section alongside the other ECC_SESSION_* knobs.
- Adds three regression tests in tests/hooks/hooks.test.js covering opt-out
  via 0, opt-out via off, and garbage-value fallback to default 30.

Verification:
- node tests/hooks/hooks.test.js  — 240/240 green (incl. 3 new retention tests)
- node tests/run-all.js           — 2622/2622 green
- npx eslint scripts/hooks/session-start.js tests/hooks/hooks.test.js — clean
- node scripts/ci/validate-no-personal-paths.js — clean
- node scripts/ci/check-unicode-safety.js       — clean
- node scripts/ci/validate-hooks.js — 28 matchers validated
- node scripts/ci/validate-rules.js — 115 files validated

Fixes affaan-m#2151
@coderabbitai

coderabbitai Bot commented Jun 5, 2026 •

Copy link
Copy Markdown
Contributor

Lost in the diff? Review this PR in Change Stack to follow the change map from intent to exact ranges.

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 04b16c39-66b5-4aea-be4f-40d498ac1504

📥 Commits

Reviewing files that changed from the base of the PR and between 5c8caec and a75130d.

📒 Files selected for processing (1)
  • README.md
✅ Files skipped from review due to trivial changes (1)
  • README.md

📝 Walkthrough

Walkthrough

Session retention is now configurable via ECC_SESSION_RETENTION_DAYS, allowing users to set a custom retention window or disable automatic cleanup entirely. The implementation adds env var parsing with opt-out support and conditionally gates the session-tmp file pruning logic in the startup hook.

Changes

Session Retention Configuration

Layer / File(s) Summary
Session retention configuration and parsing
README.md, scripts/hooks/session-start.js
ECC_SESSION_RETENTION_DAYS env var is documented (default 30 days, example 14) with opt-out values (0/off/disabled/false/never/none). getSessionRetentionDays() normalizes the env var, returning null for disable-like values and falling back to default on invalid input.
Conditional pruning logic and test coverage
scripts/hooks/session-start.js, tests/hooks/hooks.test.js
Pruning in main() is gated by the retention value: when null, pruning is skipped with a disabled log; otherwise it proceeds and logs the number of pruned sessions. Tests verify pruning is disabled for "0" and "off", and that invalid values fall back to the default 30-day behavior.

🎯 2 (Simple) | ⏱️ ~12 minutes

Hop, hop! A retention window so fine,
Sessions now prune on your timeline.
Set it to zero, let them stay and grow,
Or pick your own days—the choice, you know! 🐰✨

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The PR title clearly summarizes the main changes: adding opt-out support for session retention configuration and documenting the environment variable.
Linked Issues check ✅ Passed All coding requirements from issue #2151 are met: configurable retention via ECC_SESSION_RETENTION_DAYS, async cleanup without blocking session startup, conservative 30-day default, and documentation.
Out of Scope Changes check ✅ Passed All changes are directly related to implementing session retention cleanup and documentation as specified in issue #2151; no unrelated modifications are present.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@ecc-tools

ecc-tools Bot commented Jun 5, 2026

Copy link
Copy Markdown
Contributor

ECC bundle files are already tracked in this repository. Skipping generation of another bundle PR.

@gaurav0107
gaurav0107 marked this pull request as ready for review June 5, 2026 21:10
@gaurav0107
gaurav0107 requested a review from affaan-m as a code owner June 5, 2026 21:10

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🤖 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 `@README.md`:
- Around line 482-484: Update the README line for ECC_SESSION_RETENTION_DAYS to
document all supported opt-out values; specifically mention that setting
ECC_SESSION_RETENTION_DAYS to 0 or any of the string values "off", "false",
"disabled", "never", or "none" will disable session retention (keep all
sessions). Ensure the text around the variable name ECC_SESSION_RETENTION_DAYS
clearly lists these accepted values and their effect.
🪄 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: defaults

Review profile: CHILL

Plan: Pro

Run ID: aded40d5-f8af-4ffc-a5d8-65e5b309e39e

📥 Commits

Reviewing files that changed from the base of the PR and between 7113b5b and 5c8caec.

📒 Files selected for processing (3)
  • README.md
  • scripts/hooks/session-start.js
  • tests/hooks/hooks.test.js

Comment thread README.md Outdated

@cubic-dev-ai cubic-dev-ai Bot left a comment •

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

1 issue found across 3 files

Reply with feedback, questions, or to request a fix.

Re-trigger cubic

Comment thread README.md Outdated
…d Windows example

Address reviewer feedback on PR affaan-m#2163:
- CodeRabbit and cubic both flagged that the README docs only listed 3 of 6
  opt-out values accepted by getSessionRetentionDays() (0, off, disabled),
  while the implementation also accepts false, never, none.
- cubic also flagged the missing Windows PowerShell example for the new
  variable, breaking the parallel structure of the existing
  ECC_CONTEXT_MONITOR_COST_WARNINGS example block.

Updated the README to:
- Spell out all six opt-out values (0, off, false, disabled, never, none)
  and clarify they "keep all sessions (disable pruning)".
- Add an ECC_SESSION_RETENTION_DAYS line to the Windows PowerShell example.

No behavior change. README only.

Verification:
- npx markdownlint README.md — clean
- npx eslint scripts/hooks/session-start.js tests/hooks/hooks.test.js — clean
@ecc-tools

ecc-tools Bot commented Jun 5, 2026

Copy link
Copy Markdown
Contributor

ECC bundle files are already tracked in this repository. Skipping generation of another bundle PR.

@affaan-m
affaan-m merged commit 8dc43e5 into affaan-m:main Jun 7, 2026
40 checks passed
syarfandi pushed a commit to syarfandi/ECC that referenced this pull request Jun 9, 2026
…ment env var (affaan-m#2151) (affaan-m#2163)

* fix(session-start): support ECC_SESSION_RETENTION_DAYS opt-out + document env var

The retention pass for *-session.tmp files (issue affaan-m#2151) landed previously,
but the env var that controls it was undocumented in the README and rejected
falsy values (0, off, disabled), silently falling back to the 30-day default.
Users who want to keep all sessions for forensic or research workflows had no
way to opt out.

This patch:

- Extends getSessionRetentionDays() so 0|off|false|disabled|never|none disables
  pruning entirely (returns null sentinel; default behavior unchanged).
- Updates the call site in main() to skip pruneExpiredSessions when retention
  is null and emits a clear "[SessionStart] Pruning disabled via
  ECC_SESSION_RETENTION_DAYS" log line so the operator can tell pruning is off.
- Documents ECC_SESSION_RETENTION_DAYS in the README "Hook Runtime Controls"
  section alongside the other ECC_SESSION_* knobs.
- Adds three regression tests in tests/hooks/hooks.test.js covering opt-out
  via 0, opt-out via off, and garbage-value fallback to default 30.

Verification:
- node tests/hooks/hooks.test.js  — 240/240 green (incl. 3 new retention tests)
- node tests/run-all.js           — 2622/2622 green
- npx eslint scripts/hooks/session-start.js tests/hooks/hooks.test.js — clean
- node scripts/ci/validate-no-personal-paths.js — clean
- node scripts/ci/check-unicode-safety.js       — clean
- node scripts/ci/validate-hooks.js — 28 matchers validated
- node scripts/ci/validate-rules.js — 115 files validated

Fixes affaan-m#2151

* docs(readme): list all ECC_SESSION_RETENTION_DAYS opt-out values + add Windows example

Address reviewer feedback on PR affaan-m#2163:
- CodeRabbit and cubic both flagged that the README docs only listed 3 of 6
  opt-out values accepted by getSessionRetentionDays() (0, off, disabled),
  while the implementation also accepts false, never, none.
- cubic also flagged the missing Windows PowerShell example for the new
  variable, breaking the parallel structure of the existing
  ECC_CONTEXT_MONITOR_COST_WARNINGS example block.

Updated the README to:
- Spell out all six opt-out values (0, off, false, disabled, never, none)
  and clarify they "keep all sessions (disable pruning)".
- Add an ECC_SESSION_RETENTION_DAYS line to the Windows PowerShell example.

No behavior change. README only.

Verification:
- npx markdownlint README.md — clean
- npx eslint scripts/hooks/session-start.js tests/hooks/hooks.test.js — clean
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.

[BUG]: Session tmp files accumulate indefinitely

2 participants