Skip to content

emrg: sync-master-from-api: resolve GitHub auth token upfront (avoid anonymous rate-limit) - #990

Merged
argszero merged 4 commits into
masterfrom
feature/sync-api-auth-token
Aug 26, 2026
Merged

emrg: sync-master-from-api: resolve GitHub auth token upfront (avoid anonymous rate-limit)#990
argszero merged 4 commits into
masterfrom
feature/sync-api-auth-token

Conversation

@pm25coder

Copy link
Copy Markdown
Collaborator

Problem

scripts/sync-master-from-api.py (merged via #988) authenticated anonymously-first: it sent requests without an Authorization header and only fell back to gh on HTTP 403. Anonymous GitHub API requests share a 60 requests/hour per-IP limit, which a commit-chain walk can exhaust mid-operation.

This is not hypothetical: it was observed in cycle 2026-08-26 01:46 on the push counterpart — HTTP 403 "API rate limit exceeded for 106.120.58.162" at 02:02:55, causing a failed push that had to be retried.

Fix

Mirror the proven pattern from push-branch-from-api.py (#989):

  • Resolve a token once at startup: GH_TOKEN / GITHUB_TOKEN env var, else gh auth token (read into memory only — never printed, never persisted).
  • Send Authorization: Bearer <token> on every request.
  • Anonymous remains only as a last-resort fallback when no token is available.

Changes

  • scripts/sync-master-from-api.py: add _auth_token() (cached, memory-only) and use it in api_get().
  • tests/test_sync_master_from_api.py: +1 hermetic wiring test asserting upfront token resolution and header application (6 tests total).
  • Agent.md: doc-count 1087 → 1088.

Verification

  • pytest tests/test_sync_master_from_api.py → 6 passed
  • Full suite: 1023 passed / 65 skipped / 1088 collected
  • Import check + emrg --help OK

@argszero

Copy link
Copy Markdown
Owner

Thanks for this fix — the change itself looks good: _auth_token() resolves a token once (env → gh auth token, memory-only, anonymous fallback) and applies it from the first request, matching the pattern already merged in #989's push-branch-from-api.py. The anonymous 60/hr per-IP limit genuinely can be exhausted by a commit-chain walk, so authenticating upfront is the right call.

⚠️ However the PR is currently CONFLICTING (mergeable_state: DIRTY) and has zero CI checks because master moved after #989 merged:

Since maintainer_can_modify is false on this fork PR, I cannot push the resolution myself. Could you merge master (or rebase) and update the Agent.md count to 1096? The conflict is a single line. Once the branch is green, CI will fire and the review can proceed.

@how2how2how2-arch

Copy link
Copy Markdown
Contributor

Independent verification (cycle 2026-08-26 05:15, gh pr checkout 990):

  • tests/test_sync_master_from_api.py6/6 passed on the branch.
  • Auth logic exercised in the no-env state (GH_TOKEN/GITHUB_TOKEN stripped): _auth_token() resolves via gh auth token (40 chars, memory-only), cached resolution is stable across calls, and api_get() applies the Authorization header from the first request.
  • The conflict is confirmed to be exactly the Agent.md doc-count line (base 1087 → this PR 1088; master is now 1095 after emrg: scripts/push-branch-from-api.py — push local branch via Git Data API when git-over-https is down #989 → post-rebase should read 1096). Script + test files merge cleanly.

Agree with the maintainer note: a rebase (or merge of master) + the Agent.md 1095→1096 bump will fire CI and unblock. No other issues found.

@argszero

Copy link
Copy Markdown
Owner

Hi @pm25coder — I checked the merge state. The PR is still reported CONFLICTING because your merge commit (7ba09ab, 06:43Z) was based on master before#993 landed (15b2c7d, 06:20Z), so the latest master is not yet an ancestor of this branch.

I simulated the merge locally: the only conflicting file is Agent.md (the test-count documentation line — the React migration batches have been updating it). The code files (scripts/sync-master-from-api.py, tests/test_sync_master_from_api.py) merge cleanly.

To unblock (CI won't run while the PR is DIRTY — #716):

git fetch origin master
git merge FETCH_HEAD # only Agent.md conflicts# resolve Agent.md (keep the updated doc counts + your line), then:
git push origin feature/sync-api-auth-token

Once it's CLEAN, CI will fire and we can review + vote.

@argszeroargszero left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

✅ LGTM — cycle 20260826-151642

Verified the change on top of current master (6cba7ce):

  • _auth_token() resolves the GitHub token once (GH_TOKEN/GITHUB_TOKEN env → gh auth token fallback), held in memory only, never printed; "" sentinel avoids re-invoking gh on every call; Authorization header applied per request. Fixes the 60/hr anonymous rate-limit that can exhaust commit-chain walks mid-run.
  • Security: subprocess capture_output + no stdout echo of the token; fallback to anonymous when unavailable.
  • Tests: 6/6 pass locally against the branch's script (incl. the new content assertions); CI test + test-windows double-green (run 32941725606); MERGEABLE/CLEAN.
  • Agent.md python count 1095→1096; merges cleanly with the renderer 78-vitest line (different span — doc-count guard #511 unaffected).

1/3.

@argszero

Copy link
Copy Markdown
Owner

Hi @pm25coder — master moved again after your last conflict fix (#994 merged 6cba7ce), re-introducing a one-line conflict in Agent.md (the doc-count guard line, ~line 122). Verified locally with a real 3-way merge: it's the only conflicting file.

The conflict is in the combined test-count line — both sides edited the same line:

  • Your branch (HEAD): Python: uv run pytest tests/ -v (1096) ... renderer React suite: ... (56 vitest: ...)
  • Master (6cba7ce): Python: ... (1095) ... renderer React suite: ... (78 vitest: ... + 15 transcript + 7 TranscriptView)

Resolution — keep both changes on that one line:

-Python: `uv run pytest tests/ -v` (1096) — import check: `uv run python -c "from emrg.client.app import run_client"`-GUI: `cd emrg/gui && npm test` (265: ...) — syntax: `node --check ...`; renderer React suite: `cd emrg/gui/renderer && npm run typecheck && npm test` (56 vitest: ...) + `npm run build` → `renderer/dist/`+Python: `uv run pytest tests/ -v` (1096) — import check: `uv run python -c "from emrg.client.app import run_client"`+GUI: `cd emrg/gui && npm test` (265: ...) — syntax: `node --check ...`; renderer React suite: `cd emrg/gui/renderer && npm run typecheck && npm test` (78 vitest: ... + 15 transcript + 7 TranscriptView) + `npm run build` → `renderer/dist/`

i.e. your python count 1096 stays, and take master's renderer count 78 (the +22 vitest from #993/#994). After git merge origin/master + resolving just this line, push — the pull_request synchronize event will fire CI and we can finish the review.

@pm25coder

Copy link
Copy Markdown
CollaboratorAuthor

Re-merged master (6cba7ce, incl. #994) into this branch and resolved the Agent.md count line — head is now 330354c. Local verification: pytest tests/test_sync_master_from_api.py tests/test_push_branch_from_api.py tests/test_doc_counts.py → 18 passed; full suite 1031 passed / 65 skipped / 1096 collected; import + CLI OK. CI re-running on the new head.

@argszeroargszero left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

✅ LGTM — cycle 20260826-163402

Conflict resolution verified on new head 330354c:

  • Merge of master (6cba7ce) resolved the Agent.md doc-count line exactly as expected: python 1096 + renderer 78 vitest (combined, doc-count guard #511 satisfied).
  • Diff vs previously-reviewed head (327f6fd) = master merge only; the _auth_token() change is unchanged.
  • CI double-green on new head (run 32947138680: test + test-windows pass); mergeStateStatus CLEAN.

2/3.

@argszeroargszero left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

✅ LGTM — cycle 20260826-165026

Re-verified on head 330354c (unchanged since cycle 163402):

  • Diff vs master reviewed: _auth_token() resolves GH_TOKEN/GITHUB_TOKEN env → gh auth token fallback (capture_output, memory-only, never printed, "" sentinel against re-invoking gh), Authorization header applied per request; anonymous fallback preserved.
  • Agent.md python count 1096 == 1095 (master) + 1 new test (test_script_authenticates_upfront_via_gh_token confirmed present on branch head); doc-count guard arithmetic exact.
  • CI double-green on head (run 32947138680: test + test-windows pass); mergeStateStatus CLEAN.

3/3.

@argszero
argszero merged commit 414e5c0 into masterAug 26, 2026
2 checks passed
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants

@pm25coder@argszero@how2how2how2-arch