feat: add setup-repo-secrets script and document required PR-Agent secrets#2466
feat: add setup-repo-secrets script and document required PR-Agent secrets#2466bong-water-water-bong wants to merge 12 commits into
Conversation
…r OpenAI compat Closes lemonade-sdk#1370 — OpenCode / @ai-sdk/openai-compatible streaming crash Three fixes for reasoning model streaming: 1. Streaming proxy normalization (streaming_proxy.cpp): - Intercepts each SSE data: {...} line in forward_sse_stream() - Injects content: "" when reasoning_content is present without content - Injects role: "assistant" when null/missing on assistant deltas - Only applies to chat.completion.chunk objects (non-chat passthrough) 2. Non-streaming response (server.cpp): - Same content injection for REST chat completions response 3. thinking: false passthrough (server.cpp): - Replaced strip_handled_thinking_fields() (which erased enable_thinking/ thinking before forwarding) with normalize_thinking_fields() which renames thinking → enable_thinking and keeps it in the forwarded request. FLM/vLLM/cloud backends now see enable_thinking. - /no_think prefix retained for llama.cpp compatibility Tests: 11 unit tests covering role normalization, reasoning content normalization, carriage return, multi-choice, multi-line streams. 7/7 C++ tests pass (100%).
Closes lemonade-sdk#2371 — multi-GPU systems only got ROCm for the first GPU arch get_rocm_arch() iterates AMD GPUs (iGPU first, then dGPU) and returns only the first match. On systems with both an iGPU and dGPU with different architectures, TheRock was only installed for the iGPU. Fix: - Add get_rocm_arches() returning ALL detected AMD GPU architectures (deduplicated, iGPU-first ordering preserved) - Update install_therock_if_needed() to install TheRock for every arch - Keep get_rocm_arch() for backward compat (rocm_channel, display)
…dk#1364, lemonade-sdk#1546) lemonade-sdk#1364 — Large Prompts Timing Out - Add SSE keepalive heartbeat thread in forward_sse_stream() - Sends : keepalive\n\n every 10s during prefill while waiting for first token - Prevents client-side read timeouts on long-running prompt processing - Thread-safe via shared mutex with the libcurl write callback lemonade-sdk#1546 — Model Download Resilience - Add .completed sentinel written after all files are verified in download - is_checkpoint_path_complete() checks for .completed as authoritative marker - Prevents corrupt partially-downloaded files from appearing complete - Hardened recursive_directory_iterator with skip_permission_denied + error_code
Add a pre-load memory check in Router::load_model() that compares model_info.size (file size in GB) against get_available_memory_gb() for the target device. Logs a warning when the model may not fit. Chose warn-only (not block) because: 1. GGUF file size != load-time memory (mmap'd, paged) 2. Auto-tune ctx_size resolver will reduce context to fit 3. A hard block would frustrate users who know their setup
Completes fixes for lemonade-sdk#1364, lemonade-sdk#1546, lemonade-sdk#1804: ## lemonade-sdk#1364 — SSE heartbeat during long prefill Injects : keepalive\n\n every 10s during prefill to prevent client-side read timeouts on long prompts (15k tokens → 5 min prefill). ## #1546b — .completed sentinel for download verification Written after all files are verified in download_from_huggingface(). is_checkpoint_path_complete() checks for it, preventing corrupt partially- downloaded files from appearing complete after a crash. ## #1546a — Model-level download resume fast-path download_from_huggingface() now accepts do_not_upgrade flag. When set and .completed sentinel exists, skips the HF API call entirely. ## #1546c — Directory iterator hardening discover_extra_models() uses skip_permission_denied + error_code handling to prevent crashes on temp files from interrupted downloads. ## lemonade-sdk#1804 — Pre-load OOM guard Upgraded the pre-load memory check from warning to hard block when model size exceeds 2x available memory headroom. Prevents OOM killer crashes with a clear error message instead. ## CI — PR-Agent + Qodo dual review Added pr-agent-review.yml (DeepSeek) and qodo-merge.yml workflows.
Before fetching the HF API and rebuilding the file list, check for an existing .download_manifest.json with incomplete files. If found, resume downloading from the partial state instead of starting over. This avoids re-downloading already-completed files after a network interruption or Ctrl-C during model pull.
This reverts commit 11991bc.
…ection (lemonade-sdk#2414) The function used a std::lock_guard which caused a deadlock when build_recipes_info re-entered get_system_info_with_cache via get_rocm_arch(). Fix by switching to std::unique_lock, marking s_recipes_computed early, and unlocking during recipe computation. On failure the flag resets so the next call retries.
…ete (lemonade-sdk#2435) The draft checkpoint is optional — the model can run without MTP spec decoding. But are_required_checkpoints_complete() iterates over ALL checkpoint types, and if the draft model hasn't been downloaded yet the entire model gets marked as not-downloaded / 'unreadable'. Fix by skipping the draft type alongside npu_cache.
…crets Adds a setup script that reads API keys from ~/Documents/ and pushes them as GitHub secrets via \`gh secret set\`, plus comment headers in both PR-Agent workflows referencing how to configure secrets. Workflows already skip gracefully when secrets are missing, but the script and comments make it discoverable for new contributors. - .github/scripts/setup-repo-secrets.sh — idempotent, dry-run, --repo support - pr-agent-review.yml — header: DEEPSEEK_API_KEY required - qodo-merge.yml — header: QODO_API_KEY required
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 406e82d031
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| std::atomic<bool> heartbeat_running{true}; | ||
| std::thread heartbeat_thread([&]() { | ||
| while (!has_first_token.load() && heartbeat_running.load()) { | ||
| std::this_thread::sleep_for(KEEPALIVE_INTERVAL); |
There was a problem hiding this comment.
For normal streams where the first token arrives before 10 seconds, the heartbeat thread is usually still inside this sleep_for; after post_stream returns the code only flips heartbeat_running and then joins, but the sleep will not wake until the full interval elapses. That means fast SSE responses can sit for up to 10 seconds before the trailing line, [DONE], and sink.done() are flushed. Use an interruptible wait (for example a condition variable) or stop the thread as soon as the first token is observed.
Useful? React with 👍 / 👎.
| // indistinguishable from a complete download by other checks alone. | ||
| // The sentinel is written after all files are verified in | ||
| // download_from_huggingface(). | ||
| return safe_exists(marker_dir / ".completed"); |
There was a problem hiding this comment.
For directory-valued checkpoints, such as the Moonshine entries that resolve UsefulSensors/moonshine-streaming:onnx/tiny to snapshots/<sha>/onnx/tiny, this now requires .completed inside that resolved subdirectory. The downloader writes the sentinel at the snapshot root (snapshot_path/.completed), so those models are marked not downloaded immediately after a successful pull or cache rebuild. The check needs to resolve back to the HF snapshot root or the downloader needs to write a sentinel where directory checkpoints are validated.
Useful? React with 👍 / 👎.
| double headroom = available * SAFETY_MARGIN; | ||
| if (model_info.size > headroom * 2.0) { |
There was a problem hiding this comment.
get_available_memory_gb() returns 0.0 when no supported memory source is detected, but this code turns that into zero headroom and throws for any model with a nonzero size. On CPU-only or otherwise unsupported-GPU hosts, or transient hardware-detection failures, model loads are rejected even though the previous auto-tune path treated zero as unknown. Only enforce this new OOM gate when available > 0.
Useful? React with 👍 / 👎.
| if (entry.is_directory() && safe_exists(entry.path() / ".completed")) { | ||
| LOG(INFO, "ModelManager") << "Model already downloaded (found .completed sentinel), skipping API call" | ||
| << std::endl; | ||
| return; |
There was a problem hiding this comment.
When do_not_upgrade is true this returns as soon as any completed snapshot exists for the repo, without checking that the requested checkpoint/variant is present. If a user has one GGUF variant from a shared HF repo and then loads a sibling model from that same repo, is_model_downloaded() correctly falls through but this shortcut skips the download anyway, leaving the backend to fail on a missing file. The shortcut should validate the specific resolved checkpoint files, not just repo-level completion.
Useful? React with 👍 / 👎.
| s_recipes_computed = true; | ||
| lock.unlock(); |
There was a problem hiding this comment.
During startup or after invalidate_recipes(), this sets s_recipes_computed before recipes have been assigned and then unlocks the mutex. A concurrent caller can take the fast path and return s_cached_system_info without a recipes section, which also makes helpers like get_flm_status() report unsupported until a later call. Track an in-progress state or block readers until s_cached_system_info["recipes"] is written.
Useful? React with 👍 / 👎.
| bool enable = true; | ||
| if (thinking.is_boolean()) { | ||
| enable = thinking.get<bool>(); | ||
| request_json["enable_thinking"] = enable; |
There was a problem hiding this comment.
When a client sends both enable_thinking: true and thinking: false, should_disable_thinking() correctly treats enable_thinking as authoritative and avoids adding /no_think, but this assignment then overwrites the existing true value with false before forwarding. That reverses the documented precedence and makes backends disable reasoning for the case covered by test_007_enable_thinking_takes_precedence_over_thinking_false.
Useful? React with 👍 / 👎.
| message["reasoning_content"].is_string(); | ||
| const bool has_content = message.contains("content") && !message["content"].is_null(); | ||
| if (has_reasoning && !has_content) { | ||
| message["content"] = ""; |
There was a problem hiding this comment.
For non-streaming responses with multiple choices, this content normalization only runs inside the existing choices[0] block, so choices after the first can still contain reasoning_content without a content field. Those later choices hit the same OpenAI-compatible client failure the new normalization is trying to avoid; iterate over all choices before serializing the response.
Useful? React with 👍 / 👎.
| (void)checkpoint; | ||
|
|
||
| if (type == "npu_cache") continue; | ||
| if (type == "npu_cache" || type == "draft") continue; |
There was a problem hiding this comment.
For the MTP models in server_models.json, the draft checkpoint is the file passed to llama-server as --model-draft; skipping it here means a cache with only the main GGUF, for example after an interrupted or deleted auxiliary download, is reported as downloaded and load proceeds without the draft path while still adding the MTP flags. Treat draft like the other declared checkpoints or only skip it when the model is not using MTP.
Useful? React with 👍 / 👎.
| } | ||
| }); | ||
|
|
||
| auto result = utils::HttpClient::post_stream( |
There was a problem hiding this comment.
If post_stream throws before returning, such as on a backend connection failure or after the write callback returns false for a disconnected client, control skips the heartbeat_running=false/join() block below. The local std::thread is then destroyed while still joinable, which calls std::terminate and can bring down the server; wrap this call with RAII or a catch path that always stops and joins the heartbeat before rethrowing.
Useful? React with 👍 / 👎.
| for (const auto& rocm_arch : rocm_arches) { | ||
| backends::BackendUtils::install_therock(rocm_arch, version, progress_cb); |
There was a problem hiding this comment.
This loop only runs after the caller decides TheRock is needed, but that decision still uses is_therock_installed_for_current_arch() and therefore only checks the primary ROCm arch. On a mixed iGPU+dGPU system where the primary runtime is already installed and a secondary arch is missing, the new multi-arch loop is skipped entirely, leaving the secondary GPU without its TheRock runtime. The installed check should cover all get_rocm_arches() entries.
Useful? React with 👍 / 👎.
github-actions
Bot
added
area::ci
1 participant
Summary
Adds a setup script and workflow comment headers so contributors know how to configure the API keys needed by PR-Agent and Qodo Merge workflows.
Changes
.github/scripts/setup-repo-secrets.sh(new)Idempotent script that reads API keys from
~/Documents/deepseek api key.txtand~/Documents/qodo api key.txt, then pushes them as encrypted GitHub Actions secrets viagh secret set.--repo owner/repoand--dry-runghis installed and authenticated before writing anything.github/workflows/pr-agent-review.ymlAdded comment header documenting the required
DEEPSEEK_API_KEYsecret and how to set it up..github/workflows/qodo-merge.ymlAdded comment header documenting the required
QODO_API_KEYsecret and how to set it up.Why
Both workflows already handle missing secrets gracefully (they skip via
if: ${{ secrets.X != '' }}), but there was no discoverable way for a new contributor to know which secrets are needed or how to set them. This PR makes that path obvious.To test locally