fix: stop gating local log parsing on a cloud credential#68
Merged
Conversation
The generated notify.cjs hook read a device token and refused to spawn anything without one. Parsing local logs and uploading them are two separate decisions, and the hook was making the second one to decide the first. A local-only install therefore refreshed its queue once at service startup and then went stale forever, no matter how many sessions ended. The throttle write lived inside that same credential branch, which is why sync.throttle was never created either — the symptom that made this look like a throttle bug rather than a gating bug. The hook now spawns `sync --auto --from-notify` on the 20s throttle alone. Nothing else changes: upload is still gated on a device token inside sync itself (src/commands/sync.js:936), so a tokenless install parses locally and transmits nothing. Verified that this path is not merely upload-free but network-free: ensurePricingLoaded — the only unconditional outbound call in the CLI — is reached from serve.js and local-api.js only, never from sync.js, and getModelPricing falls back to the bundled snapshot. Drops the now-unused configPath binding from the generated hook. Tests were confirmed to fail 0/3 against the unfixed template before being accepted: one pins that a tokenless notify creates the throttle stamp, one pins that the 20s throttle survived the change, and one pins that the generated hook contains no credential read at all — so the gate cannot grow back in a different shape. Fixes #67
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #67
Summary
The generated
notify.cjsread a device token and refused to spawn anything without one. Parsing local logs and uploading them are two separate decisions, and the hook was making the second to decide the first — so a local-only install refreshed its queue once at service startup and then went stale forever, however many sessions ended.The throttle write sat inside that same credential branch, which is why
sync.throttlewas never created either. That made it look like a throttle bug rather than a gating bug.The hook now spawns
sync --auto --from-notifyon the 20-second throttle alone. Also drops the now-unusedconfigPathbinding from the generated template.The privacy requirement in #67 is fully honored
syncitself —src/commands/sync.js:936if (runtime.deviceToken && runtime.baseUrl). All parsing and the cursor write complete at:923-929, before that gate, so a tokenless sync fillsqueue.jsonllocally and reaches no upload code.TOKENTRACKER_DEVICE_TOKENorconfig.jsonat all, and a test asserts the generated file contains no credential read in any form.ensurePricingLoaded— the one unconditional outbound call in the CLI — is reached only fromsrc/commands/serve.js:13andsrc/lib/local-api.js:1750, never fromsrc/commands/sync.js, andgetModelPricingfalls back to the bundled snapshot (src/lib/pricing/index.js:75). A tokenless notify → sync makes no network request whatsoever.Test plan
node --test test/notify-local-parse.test.js— 3/3main'sinit.jsand re-running. A regression guard that passes both before and after proves nothing.notify-debug-log1/1,codex-config-multiline-notify3/3,passive-mode8/8,init-uninstall18/18,grok-hook6/6,diagnostics3/3,status2/2npm test784/788; failing names diffed againstmainand byte-identicalsync.throttleappears →queue.jsonlgrows, with no device token configuredWhat the tests pin
Note on
src/commands/init.jsas a template literalThe hook is built as a template string, so a stray backtick or
${in the edit would corrupt every generatednotify.cjs. The added comment deliberately contains neither;git diffconfirms zero backtick/interpolation characters among the added lines, and the suites above exercise a realinitfollowed by running the generated file.Pre-existing CI failure
ci:localwill be red for the same 4parseKiroCliIncrementalfailures that are red onmain(see #65). This PR does not touch that path.