Repository navigation
emrg: the settings page can sign in to aitokenpool with a GitHub account - #2775
Conversation
The model tab only offered three text fields, so using aitokenpool meant registering on the web, copying a key and hand-filling all three — and any one of them wrong reads as "the model does not answer". The deployment already serves an OAuth device flow for GitHub identities (an account needs no email); nothing in EMRG reached it. - atp_login.js: the three HTTP round trips, the model-list reading, and the config.toml merge — pure, with fetch injected, so the whole chain runs against a fake upstream. Probe-before-write lives here in one place: a non-200 /api/models returns before writeConfig is ever called. - main.js holds the network and the write (the renderer is sandboxed with no network access), through four whitelisted channels; atp_login.js joins build.files or the packaged GUI could not require it. - The rendering code answers a 429 silently by switching interval, states which of the four failures happened and what to do next, and asks before replacing a key already in use — cancelling calls the write zero times. - writeConfig now delegates to writeConfigText (the login writes merged text, not an object); tests/test_gui_config_mode.py follows the rule to the function that writes the file and pins the delegation with it.
argszero
left a comment
There was a problem hiding this comment.
✅ LGTM — cycle cyc20261109-232101
Reviewed at head 59ad6b47, which is content-identical to the local cce11f83 the branch was refreshed to. The readings: base e4de3962 is master's tip (check-merge-freshness.py FRESH), MERGEABLE/CLEAN, both CI legs green (test 2m54s, test-windows 5m33s, run 42642395006), no review and no comment left unanswered.
The change. emrg/gui/atp_login.js carries the device flow, the model-list read and the config merge with fetch injected, so the whole chain runs against a fake upstream; main.js wires four whitelisted channels and delegates writeConfig to a new writeConfigText; SettingsPanel and AtpLoginDialog hold the UI; preload.js opens the channels and keeps no second copy of the service addresses, which is the right call — one producer for a value two places could disagree about.
Ordering is the requirement that carried the most risk, and it lives in one place. completeLogin probes GET /api/models and returns on a non-200 before readConfig and writeConfig are called at all, so "a failed probe writes nothing" is a property of the code rather than of the dialog that calls it. The merge touches base_url, api_key, model and one model entry; an existing entry of the same name has only its context_window updated rather than a second one appended; an unknown context_window is left unwritten instead of being defaulted, which is the difference between "we do not know" and "we know it is wrong".
The two moved guards are moved for the right reason. After the extraction the function that writes the file is writeConfigText, so tests/test_gui_config_mode.py pins the mode rule there and pins the delegation. A guard left watching writeConfig alone would have kept passing against a body that no longer writes anything — that is the failure this repository has a memory about, and this PR does not repeat it.
Two notes, neither a blocker. The body states 192 GUI / 773 renderer, which is what this branch measured before master was merged into it; on the merged tree the count writer reads 194 GUI / 774 renderer, and Agent.md states those, so the body outlives its own numbers — worth correcting so the next reader is not sent to a stale pair. And acceptance item 8 asks for three CI platforms while this repository's CI runs ubuntu and windows-2025, the macOS leg existing only in the tagged build: that item is met two leg of three by construction, not by an omission here.
argszero
left a comment
There was a problem hiding this comment.
✅ LGTM — cycle cyc20261109-233507
Voted on the tree this merge would land, not on the head's own CI: git merge-tree --write-tree a4ed6ad2 59ad6b47 → tree e76dc576, clean (rc 0). The head is one commit behind (base e4de3962, master a4ed6ad2), and what moved is #2777's single added line in emrg/server/open_source_prompt.md — a file this PR does not touch.
Readings taken on that landing tree, checked out detached at 7fcdea7e:
scripts/check-node-test-count.py --dry-run→OK: Agent.md documents 774 renderer + 194 GUI tests (both runners agree)— the counts this PR writes intoAgent.mdare measured by both runners, not arithmetic.tests/test_doc_counts.py+tests/test_gui_config_mode.py→ 89 passed; the five suites that read the prompt template (test_prompt_templates,test_language_policy_reach,test_rants_single_writer,test_boss_tool,test_host_blocked_browser_check) → 128 passed.- Both CI legs green on the head, run 42642395006:
test2m54s,test-windows5m33s.
What the code does, read in itself rather than from the body: atp_login.js keeps the protocol and the config merge in one module with fetch injected, and completeLogin probes the protected GET /api/models before writeConfig is reached — so a key that does not work cannot be written and then present as "the model does not answer". main.js owns the network and the write because the renderer is sandboxed; writeConfig delegates to writeConfigText instead of re-implementing the merge there. The overwrite confirmation precedes replacing a key already in use, and the merge preserves every other key and section. The two guards that moved did so with the extraction, and both directions are armed by the mutation record.
One prose defect, no code defect: the body stated 192 GUI / 773 renderer against the 194 / 774 its own Agent.md documents, and its last line still called CI pending. Both are corrected in the body now, because the file — the thing the guard measures — was the right side of that disagreement.
2 of 3 votes.
argszero
left a comment
There was a problem hiding this comment.
✅ LGTM — cycle cyc20261109-234620
Third vote, on a landing tree that is the same object the previous cycle measured, not a re-reading of a similar one: git merge-tree --write-tree a4ed6ad2 59ad6b47 answers e76dc576fce78f43399db92b38a618eefc87f09c — byte-for-byte the tree this cycle's predecessor checked out and measured, because master has not moved since (a4ed6ad2) and the head has not moved since (59ad6b47, pushed 15:13:01Z). Content addressing is what makes that a reading rather than an assumption.
On that tree the guards answered 89 passed (tests/test_doc_counts.py + tests/test_gui_config_mode.py), 128 passed (the five suites that read emrg/server/open_source_prompt.md), and scripts/check-node-test-count.py --dry-run → OK: Agent.md documents 774 renderer + 194 GUI tests (both runners agree).
Also read, on this head: both CI legs green (run 42642395006 — test 2m54s, test-windows 5m33s), MERGEABLE/CLEAN, the base a4ed6ad2's own master CI concluded success (run 42644881374), and the link reading ok both ways — the PR declares #2774 and the issue names #2775 back.
The change is sound on its own terms: atp_login.js holds the protocol and the config merge with fetch injected, and probes the protected GET /api/models before writeConfig is reachable, so a key that does not work cannot be written and then present as "the model does not answer"; main.js owns the network and the write because the renderer is sandboxed; the merge preserves every other [llm] key and section. The two guards that moved (test_gui_config_mode.py, preload-api.test.js) moved with the extraction they describe, and the mutation record arms both directions.
The body's own two defects were corrected last cycle rather than counted against the code: its Verification section stated 192 GUI / 773 renderer against the 194 / 774 its Agent.md documents, and its closing line still called CI pending. Body patched by API, head unmoved, both earlier votes intact.
3 of 3 votes.
Closes #2774
The model tab offered exactly three text fields, so using aitokenpool meant registering on its website, copying a key and hand-filling all three. Any one of them wrong — a trailing
/v1, a stale key, a model the endpoint does not serve — presents as "the model does not answer", which costs more to diagnose than to configure. The deployment already serves an OAuth device flow for GitHub identities (an account needs no email address); nothing in EMRG reached it.What changed
emrg/gui/atp_login.js(new) — the protocol and the config merge, withfetchinjected, so the whole chain runs against a fake upstream and never leaves the machine. This module is also the single place the ordering rule lives:completeLoginprobes the protectedGET /api/modelsfirst and returns on a non-200, sowriteConfigis not called at all.emrg/gui/main.js— four whitelisted channels (atpBases,atpDeviceStart,atpDevicePoll,atpApply). The network and the write are here because the renderer is sandboxed withcontextIsolationand has no network access.writeConfignow delegates to a newwriteConfigText: the login produces merged text, and serializing an object on that path would have re-implemented the merge.emrg/gui/preload.js— the four channels; the service addresses are named once, inatp_login.js, and the renderer renders what main reports rather than keeping a second copy.SettingsPanel.tsx+AtpLoginDialog.tsx(new) — an account block above the model fields; a dialog with a copyable one-time code, "open browser", a waiting state and a copy path; polling at the interval the server returns, with a429changing the interval silently; the overwrite confirmation before a key already in use is replaced; on success the three fields are filled into the existing form. No signed-in indicator and no disconnect: there is no reliable way to derive an identity from a key, and a fabricated state is worse than none.Verification
emrg/gui npm test— 194 tests (was 175;+19 atp_login), 0 fail.emrg/gui/renderer npm run typecheck && npm test— 774 passed (was 765;SettingsPanel26 → 35).scripts/check-node-test-count.pyanswersOK: Agent.md documents 774 renderer + 194 GUI tests (both runners agree).uv run pytest tests/— 6187 passed, 36 skipped, on this tree.uv run python -c "from emrg.client.app import run_client"anduv run python -m emrg --help— OK.Mutation arms, each reverted and re-verified byte-identical:
finishAtpLogin→ the two confirmation cases redden (2 failed / 33 passed);applyAtpConfigrewritten to start from{}instead of the parsed file → the preservation case reddens (4 failed / 15 passed in the Node suite);writeConfigno longer delegating towriteConfigText→ the permission guard reddens; andwriteConfigTextdroppingmakeConfigPrivate→ the same guard reddens on the other assertion.Two guards moved, and why
tests/test_gui_config_mode.pywatchedwriteConfig's body for themakeConfigPrivatestatement. After the extraction the function that writes the file iswriteConfigText, so the guard now pins the rule there and pins the delegation with it. A guard naming onlywriteConfigwould pass on a body that no longer writes anything; both directions are covered by the arms above.test/preload-api.test.js's frozen list gains the four channels, andi18n.test.ts's key count moves 444 → 465 (21 new keys, both dictionaries in step).A limit worth naming
The merge parses and re-serializes the file, so comment lines are not preserved — the freshly generated config template carries some. This is the pre-existing behaviour of
emrg:saveSettings, which has written config.toml this way since G60; every other key and section survives verbatim (asserted). Making the round trip comment-preserving is a separate change to both writers, not something this PR should smuggle in.Route
git-over-https is down on this host (the configured proxy refuses connections), so the branch was pushed with
scripts/push-branch-from-api.py; the pushed commit is content-identical, its sha differing because the API recreates it. CI on this head: both legs green (run 42642395006 —test2m54s,test-windows5m33s).