Skip to content

fix(cli): declare tiktoken for tool token estimates - #33350

Closed
hanzckernel wants to merge 1 commit into
NousResearch:mainfrom
hanzckernel:fix/cli-tiktoken-extra-fallback
Closed

fix(cli): declare tiktoken for tool token estimates#33350
hanzckernel wants to merge 1 commit into
NousResearch:mainfrom
hanzckernel:fix/cli-tiktoken-extra-fallback

Conversation

@hanzckernel

@hanzckernel hanzckernel commented May 27, 2026

Copy link
Copy Markdown
Contributor

Summary

  • Add tiktoken==0.12.0 to the cli extra so fresh CLI installs include the dependency used by hermes tools token estimation.
  • Regenerate uv.lock from current main so [cli], [all], termux, and termux-all resolve consistently through the existing extra graph.
  • Add a packaging regression that parses PEP 508 requirements and canonicalizes the dependency name, keeping the assertion version- and spelling-normalization-agnostic.

Behavior

This does not make token estimation a hard runtime requirement. _estimate_tool_tokens() still imports tiktoken lazily and returns {} when it is unavailable, so base/fallback installs can still open the tool configuration UI without token estimates.

Related: #13481 touches the same dependency area, but this PR is intentionally scoped to the CLI token-estimation dependency declaration and preserves the existing fallback behavior.

Current-main refresh

  • Replayed as one commit; stale merge-only history and old lockfile chunks were not carried forward.
  • Regenerated uv.lock with current uv lock output.
  • Revalidated after current main changed hermes_cli/tools_config.py; the lazy no-tiktoken fallback remains intact.

Verification

  • Exact current-main matrix: 29 passed, 6 skipped.
  • Tool-token tests with a temporary tiktoken==0.12.0 install: 13 passed.
  • Packaging/project metadata tests: 22 passed.
  • uv lock --check: passed.
  • Ruff and git diff --check: passed.
  • One-commit invariant and clean worktree verified before push.
  • Exact-SHA independent review and post-fix closure: clean.

@hanzckernel
hanzckernel requested a review from a team May 27, 2026 16:29
@alt-glitch alt-glitch added type/bug Something isn't working P3 Low — cosmetic, nice to have comp/cli CLI entry point, hermes_cli/, setup wizard labels May 27, 2026

@lordbuffcloud lordbuffcloud left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Looks right to me. This keeps the graceful runtime fallback for base installs, but makes the CLI extra actually ship tiktoken for tool-token estimates and locks that expectation in with a packaging test.\n\nI reran:\n- python3 -m pytest -q tests/hermes_cli/test_tool_token_estimation.py\n\nPassed locally.

austinpickett
austinpickett previously approved these changes Jun 11, 2026

@austinpickett austinpickett left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

✅ Approved

Dep gap confirmed on main: tiktoken appears nowhere in pyproject.toml — fresh pip install hermes-agent[cli] installs silently lack it, causing _estimate_tool_tokens() to always return {}.

Fix is surgical: "tiktoken==0.12.0" added to the cli extra; uv.lock updated (+164 lines resolving transitive deps); ImportError guard retained for base installs. Test parses pyproject.toml via tomllib — proper packaging regression guard.

CI: all 23 checks green including uv lock --check, osv-scanner, all 6 test shards, both platform builds.

@teknium1 teknium1 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.

Thanks for isolating the missing CLI dependency while preserving the base-install fallback. The premise is still present on current main: hermes_cli/tools_config.py:2088-2094 lazily imports tiktoken, but pyproject.toml:171 declares the cli extra with only simple-term-menu.

Problems

  • tests/hermes_cli/test_tool_token_estimation.py:28 snapshots "tiktoken==0.12.0". A normal dependency-version update would break this test even if the CLI extra continues to declare tiktoken. This conflicts with the repository's change-detector guidance in AGENTS.md:1309-1355.

Suggested changes

  • Make the new test assert that the cli extra contains a tiktoken requirement without hard-coding its version; retain version/pin-policy checks in the general packaging metadata tests.
  • During salvage, regenerate uv.lock against current main rather than reusing the June lockfile resolution.

This is an automated hermes-sweeper review.


cli_extra = pyproject["project"]["optional-dependencies"]["cli"]

assert "tiktoken==0.12.0" in cli_extra

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.

Please make this assertion version-agnostic (for example, assert that a CLI-extra requirement has package name tiktoken). The exact version is expected to change during dependency/security updates, so this becomes a change-detector rather than a packaging-contract test.

@teknium1 teknium1 added sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades sweeper:risk-platform-windows Sweeper risk: may break or behave differently on native Windows sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform labels Jul 13, 2026
@hanzckernel
hanzckernel force-pushed the fix/cli-tiktoken-extra-fallback branch from 56646de to 2633981 Compare July 13, 2026 21:52
@hanzckernel
hanzckernel deleted the fix/cli-tiktoken-extra-fallback branch July 20, 2026 00:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

comp/cli CLI entry point, hermes_cli/, setup wizard P3 Low — cosmetic, nice to have sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades sweeper:risk-platform-windows Sweeper risk: may break or behave differently on native Windows type/bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants