Route token counting via gateway#60
Conversation
Summary
Testing
|
Test & Lint Summary
|
noa-lucent
left a comment
There was a problem hiding this comment.
Requesting changes.
Main blockers:
- CI workflow: ensure the build step creates dist/ before go build; pin agynio/e2e action (no @main) and pass a pinned ref into agynio/bootstrap provision for determinism.
- grpc gateway stubs: the TokenCountingGateway ServiceDesc/handler boilerplate is duplicated across unit + e2e tests (and the client hardcodes the method separately). Please centralize into a single typed stub and reuse.
Non-blocking:
- README still mentions Go 1.24 while the repo/workflow are on 1.25.
Summary
Testing
|
Test & Lint Summary
|
noa-lucent
left a comment
There was a problem hiding this comment.
Re-review complete.
- E2E workflow is now deterministic (pinned action SHAs + pinned bootstrap/e2e refs) and the dist/agn build step creates dist/.
- TokenCountingGateway gRPC stub is centralized (gateway_stub.go) and reused by unit + e2e tests; client uses the shared method const.
Non-blocking: README still says Go 1.24.
Summary
Testing
|
Test & Lint Summary
|
noa-lucent
left a comment
There was a problem hiding this comment.
Re-review complete.
- e2e runner pin updated and still fully deterministic (action SHA + matching
ref). include-smoke: falseis now set as per issue #59 to run only the svc_agn_cli suite.
Non-blocking: README still mentions Go 1.24.
|
Summary:
Tests/Lint:
|
noa-lucent
left a comment
There was a problem hiding this comment.
Summary
Thanks for the substantial rework to in-process token counting — this is directionally aligned with issue #59 and agynio/architecture.
Blockers
-
BPE merges must not be downloaded at runtime. As implemented, tiktoken-go will fetch o200k_base.tiktoken from openaipublic unless env overrides are set. Please vendor/embed the merges into the agn binary and load them deterministically (no runtime config), ideally preloading at startup.
-
image_url should be HTTPS-only per the architecture spec.
-
Audio is currently silently under-counted (0 tokens) via input_audio and audio file extensions. Either implement correct audio counting or fail explicitly as unsupported.
-
Add unit tests for the new tokenizer package (parse shapes, BPE token counts, image tiling math, PDF path). E2E coverage alone won’t protect this new low-level logic.
Non-blocking
README still references Go 1.24 while CI/go.mod are on 1.25.
|
Summary:
Tests/Lint:
|
noa-lucent
left a comment
There was a problem hiding this comment.
Re-review complete.
Verified the previously requested blockers are addressed:
- BPE merges now load offline via tiktoken-go-loader (no runtime download).
- image_url is https-only.
- audio (input_audio + audio file extensions) now fails explicitly instead of silently counting 0.
- Added focused unit tests covering parse shapes, BPE token counts, image tiling, and PDF extraction.
Non-blocking: PR description and README still reference the older gateway-based approach / Go 1.24.
Summary
Tests
Lint
|
Summary
Tests
Lint
|
noa-lucent
left a comment
There was a problem hiding this comment.
Re-review complete.
Verified the workflow now pins agynio/e2e run-tests action to dbe74e80d6bbf8dde6cd4cac89dd15e8c72247ab and passes the matching ref input, keeping CI deterministic while using the no-mocks runner.
Summary
Tests
Lint
|
noa-lucent
left a comment
There was a problem hiding this comment.
Re-review complete.
Workflow-only change verified: agynio/e2e run-tests action is now pinned to 0a4f88cc9d2392234dd2faa88427d3051e2722d0 and the ref: input matches.
Summary
Tests
Lint
|
noa-lucent
left a comment
There was a problem hiding this comment.
Re-review complete.
Workflow-only change verified: the run-tests action input is include_smoke (underscore) in agynio/e2e, and the workflow now passes include_smoke: false accordingly.
Summary
Notes
Testing
Issue
Closes #59