Skip to content

fix: use shared auto-compact threshold constants - #1193

Merged
will-lamerton merged 4 commits into
Nano-Collective:mainfrom
DeepamJha:fix/shared-auto-compact-threshold
Sep 15, 2026
Merged

will-lamerton merged 4 commits into
Nano-Collective:mainfrom
DeepamJha:fix/shared-auto-compact-threshold

Conversation

@DeepamJha

Copy link
Copy Markdown
Contributor

Summary

  • Use COMPRESSION_CONSTANTS as the single source of truth for auto-compact threshold clamping.
  • Update boundary tests to derive expected values from the shared constants.
  • Prevent threshold ranges from drifting between implementations.

Closes #1137

Validation

  • Focused AVA suite: 41 passing
  • Lint: passed
  • Format: passed
  • pnpm test:all: blocked by intermittent Corepack/pnpm signing-key verification.

@will-lamerton will-lamerton left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thanks for picking this up. The auto-compact.ts change is correct, and the tests are still honest because message-compression.spec.ts pins the literal 50/95 values.

One thing before merge: a third copy of the same clamp survives. source/config/index.ts:

// Validate and clamp threshold to valid range (50-95)
function validateThreshold(threshold: unknown): number {
	const num = typeof threshold === 'number' ? threshold : 60;
	return Math.max(50, Math.min(95, Math.round(num)));
}

That is the nanocoder.tune.threshold config path, so the drift #1137 describes still exists: change the constants and agents.config.json clamps differently from /context-max. Please import COMPRESSION_CONSTANTS there too and drop the stale range from the comment.

Optional: exporting a small clampThreshold(value) from message-compression.ts and using it in both call sites would make the single source of truth structural rather than by convention.

Also please run pnpm changeset --empty to clear the changeset bot comment, since this is behaviour-neutral.

@DeepamJha

Copy link
Copy Markdown
Contributor Author

Hey @will-lamerton, thanks for catching that. I’ve pushed the update in 0d03189.
The config threshold clamp now uses the same COMPRESSION_CONSTANTS values, and I removed the stale 50–95 comment. I also added the empty changeset as suggested.
I ran the affected config and auto-compact tests (83 passing), plus typecheck, lint, and format.

Whenever you have a moment, I’d appreciate another look.

Export clampThreshold/isThresholdInRange from message-compression so the
50-95 bounds are enforced in one place rather than copied. The remaining
literal copies were the settings panel validator and /compact --threshold,
both of which would have rejected values the config and session paths
accept once the constants change.

Also tidies the empty changeset and sorts an import in write-file.tsx that
biome check flags on main.
@github-actions github-actions Bot added area:tools Tool implementations and tool-calling area:tui Terminal UI labels Sep 15, 2026

@will-lamerton will-lamerton left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thanks for the follow-up - the config path is fixed as asked.

I pushed two commits on top: a merge of main to clear the conflict in source/config/index.ts (it was just an adjacent import line), and the structural version of the optional suggestion. clampThreshold/isThresholdInRange now live in message-compression.ts and every entry point calls them.

That last part mattered more than it looked - two more literal copies of the 50/95 bounds survived beyond the one I flagged: the settings panel validator in settings-auto-compact.tsx and --threshold in compact-handler.ts. Both would have rejected values the config and session paths accept once the constants move, which is the same bug #1137 describes. Added clamp coverage to message-compression.spec.ts too.

One unrelated thing rolled in: biome check fails on write-file.tsx on current main (unsorted import), so that one-line reorder is in here to get CI green.

Full suite passes.

@will-lamerton
will-lamerton merged commit 2d26843 into Nano-Collective:main Sep 15, 2026
16 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area:tools Tool implementations and tool-calling area:tui Terminal UI

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug] setAutoCompactThreshold clamp range defined in two places

2 participants