Repository navigation
Conversation
162c63c to
4279ff1
Compare
fredheir
left a comment
There was a problem hiding this comment.
Blocking correctness issue: CopilotSummarizer defines CopilotTokenManager, but the client path sends the long-lived GitHub OAuth token directly to https://api.githubcopilot.com. That bypasses the /copilot_internal/v2/token exchange and will fail for the intended Copilot API flow.
I prepared and tested a local patch that changes _get_client() to call CopilotTokenManager.get_token(), invalidates that session token on retry, removes the setup_cloud.sh TODO cost text, and adds focused tests proving the OpenAI-compatible client receives the Copilot session token rather than the GitHub OAuth token. Proof run locally: uv run --extra dev --extra copilot -m pytest -q tests/test_summarizers.py -> 10 passed; uv run --extra dev --extra copilot ruff check meeting_notes/copilot_auth.py meeting_notes/ai_summarizer.py tests/conftest.py tests/test_summarizers.py -> passed.
I cannot push to mathstuf/meeting-notes-tui from this token despite maintainerCanModify, so the branch cannot be updated in place from here.
|
Reference branch with the tested fix, since I cannot push to the contributor fork directly: https://github.com/Markolo-Research/meeting-notes-streaming/tree/codex/pr10-copilot-session-token-fix Incremental commit range in my worktree: |
|
Your fork is private and I cannot access it. |
887741a to
dd089c9
Compare
|
I have a diff assisted by DeepSeek applied now. @fredheir is this similar to what you have? |
|
@mathstuf my apologies, my agent went rogue. |
dd089c9 to
913d7aa
Compare
|
Thanks; I've integrated the changes in (except the spurious whitespace diffs). |
913d7aa to
7eb5a1b
Compare
|
Rebased on |
Fixes-by: Rolf Fredheim <[email protected]> (agent-assisted) Co-authored-by: GitHub Copilot (claude-opus-4.6) Fixes: jamespember#2
7eb5a1b to
afd9239
Compare
…way and DeepInfra OpenAICompatibleSummarizer runs the shared prompt and parser against any OpenAI chat-completions endpoint: a subclass sets BASE_URL, ENV_VAR, LABEL and a tier -> model-id table. Two providers use it, both serving Claude with ai_model haiku | sonnet | opus: - assemblyai: AssemblyAI's LLM Gateway, authenticated with the same key as AssemblyAI transcription, so one key covers the whole pipeline. - deepinfra: DeepInfra's OpenAI-compatible API. Choices that matter for current models: - no temperature: Claude 5 models on the gateway reject it; - MAX_TOKENS = 16000: reasoning models spend output tokens before writing text (a 34-minute meeting needed ~5,400 for ~1,100 visible); - finish_reason "length" raises SummaryTruncated rather than parsing a partial response. The same base would also cover other OpenAI-compatible services, e.g. the Copilot request in jamespember#2 / jamespember#10. Config validation, provider_api_key(), Settings (shared Claude tier buttons, key inputs) and NoteMaker are wired for both; tests use fake clients only. Co-Authored-By: Claude Opus 5 (1M context) <[email protected]>
|
Great contribution @mathstuf — a well-scoped implementation of the GitHub Device Flow with token caching, and it fits cleanly alongside the other providers. What I like:
Rebase notes (merged locally on top of #16–#20): The PR conflicted with the AssemblyAI/DeepInfra provider additions from #19/#20 because both series added new elements to the same enumerations. I resolved them by keeping both:
Two small fixes I made on top during the rebase:
One thing worth flagging (not blocking): The default Local: 162 tests pass, ruff clean. Merging via local squash (workflow scope again — same reason as #19/#20). |
Co-authored-by: GitHub Copilot(claude-opus-4.6)
Fixes: #2
Based on #9 and #5.