fix: reuse existing secret-key on startup; add upstream sync workflow#39
fix: reuse existing secret-key on startup; add upstream sync workflow#39bighamx wants to merge 3 commits intorouter-for-me:mainfrom
Conversation
There was a problem hiding this comment.
Code Review
This pull request modifies the start_cliproxyapi and restart_cliproxyapi functions to attempt to retrieve an existing secret-key from the configuration before falling back to generating a random password. The review feedback correctly identifies that this logic is duplicated across both functions and suggests extracting it into a reusable helper function to adhere to DRY principles.
|
Implemented the refactor suggestion by extracting the duplicated |
There was a problem hiding this comment.
Pull request overview
Updates local startup authentication to reuse an existing configured management secret, and adds automation to sync upstream changes and create release tags.
Changes:
- Reuse
remote-management.secret-keywhen starting/restarting the local CLIProxyAPI process (fallback to generating one if missing). - Add a scheduled/manual GitHub Actions workflow to merge from an upstream repo and tag releases.
- Expand
.gitignoreto exclude common local build artifacts (.vs/,bin/,obj/,node_modules/).
Reviewed changes
Copilot reviewed 2 out of 3 changed files in this pull request and generated 7 comments.
| File | Description |
|---|---|
src-tauri/src/main.rs |
Adds a helper to reuse an existing secret key instead of always generating a new one on startup/restart. |
.gitignore |
Ignores additional local build/dependency artifacts. |
.github/workflows/sync-upstream.yml |
Introduces an automated upstream sync + tagging workflow. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| .map(|s| s.to_string()) | ||
| .unwrap_or_else(generate_random_password) | ||
| } | ||
|
|
There was a problem hiding this comment.
Add unit tests for get_or_generate_secret_key to cover: (1) missing remote-management, (2) missing/blank secret-key, and (3) existing non-empty secret-key being reused. There are already Rust unit tests in this file, so adding coverage here should be straightforward and prevents regressions in startup auth behavior.
| #[cfg(test)] | |
| mod auth_tests { | |
| use super::get_or_generate_secret_key; | |
| #[test] | |
| fn missing_remote_management_generates_secret_key() { | |
| // No `remote-management` section -> should generate a new key | |
| let conf: serde_yaml::Value = serde_yaml::from_str( | |
| r#" | |
| auth-dir: ./auth | |
| "#, | |
| ) | |
| .expect("failed to parse yaml"); | |
| let key = get_or_generate_secret_key(&conf); | |
| assert!(!key.trim().is_empty(), "generated key should not be blank"); | |
| assert_eq!(key.len(), 32, "generated key should be 32 characters long"); | |
| } | |
| #[test] | |
| fn missing_or_blank_secret_key_generates_secret_key() { | |
| // `remote-management` exists but `secret-key` is missing | |
| let conf_missing: serde_yaml::Value = serde_yaml::from_str( | |
| r#" | |
| remote-management: {} | |
| "#, | |
| ) | |
| .expect("failed to parse yaml"); | |
| let key_missing = get_or_generate_secret_key(&conf_missing); | |
| assert!(!key_missing.trim().is_empty()); | |
| assert_eq!(key_missing.len(), 32); | |
| // `remote-management.secret-key` is present but blank/whitespace | |
| let conf_blank: serde_yaml::Value = serde_yaml::from_str( | |
| r#" | |
| remote-management: | |
| secret-key: " " | |
| "#, | |
| ) | |
| .expect("failed to parse yaml"); | |
| let key_blank = get_or_generate_secret_key(&conf_blank); | |
| assert!(!key_blank.trim().is_empty()); | |
| assert_eq!(key_blank.len(), 32); | |
| } | |
| #[test] | |
| fn existing_non_empty_secret_key_is_reused() { | |
| let conf: serde_yaml::Value = serde_yaml::from_str( | |
| r#" | |
| remote-management: | |
| secret-key: "my-fixed-secret" | |
| "#, | |
| ) | |
| .expect("failed to parse yaml"); | |
| let key = get_or_generate_secret_key(&conf); | |
| assert_eq!(key, "my-fixed-secret"); | |
| } | |
| } |
|
|
||
| // Generate random password for local mode | ||
| let password = generate_random_password(); | ||
| let password = get_or_generate_secret_key(&conf); | ||
|
|
||
| // Store the password for keep-alive authentication | ||
| *CLI_PROXY_PASSWORD.lock() = Some(password.clone()); |
There was a problem hiding this comment.
password can now be sourced from a persisted remote-management.secret-key, but the function later logs the full command-line args including --password (which will leak the management secret into logs/console). Please redact the password in logs (or remove it from the logged args) to avoid exposing a long-lived credential.
| LOCAL_SHA=$(git rev-parse HEAD) | ||
| UPSTREAM_SHA=$(git rev-parse upstream/${{ env.UPSTREAM_BRANCH }}) | ||
|
|
||
| if [ "$LOCAL_SHA" = "$UPSTREAM_SHA" ] && [ "${{ github.event.inputs.force_release }}" != "true" ]; then | ||
| echo "No upstream changes detected." | ||
| echo "has_changes=false" >> "$GITHUB_OUTPUT" | ||
| else | ||
| echo "Upstream changes detected or force release requested." | ||
| echo "has_changes=true" >> "$GITHUB_OUTPUT" | ||
| fi |
There was a problem hiding this comment.
The upstream-change detection compares HEAD vs upstream/main SHA equality. If this repo intentionally carries additional local commits on top of upstream (common for forks), the SHAs will always differ even when upstream has no new commits, causing unnecessary merges/tags. Consider checking whether upstream is ahead of local instead (e.g., count commits in HEAD..upstream/main) and only sync when that count is non-zero (unless force_release is true).
| echo "::warning::Merge conflict detected. Resolving by keeping our changes." | ||
| git checkout --ours . | ||
| git add . | ||
| git commit -m "Merge upstream (conflicts resolved by keeping local)" |
There was a problem hiding this comment.
Auto-resolving merge conflicts with git checkout --ours . silently discards upstream changes in conflicting files, which defeats the purpose of syncing (and can miss important fixes). A safer approach is to fail the job on conflicts and either open an issue/PR for manual resolution or push the conflicted merge to a separate branch for review.
| echo "::warning::Merge conflict detected. Resolving by keeping our changes." | |
| git checkout --ours . | |
| git add . | |
| git commit -m "Merge upstream (conflicts resolved by keeping local)" | |
| echo "::error::Merge conflict detected while syncing with upstream. Please resolve manually." | |
| exit 1 |
| env: | ||
| UPSTREAM_REPO: https://github.com/router-for-me/EasyCLI.git | ||
| UPSTREAM_BRANCH: main | ||
|
|
||
| jobs: | ||
| sync: | ||
| name: Sync Upstream | ||
| runs-on: ubuntu-latest | ||
| steps: |
There was a problem hiding this comment.
This workflow pushes commits and tags back to the repository using GITHUB_TOKEN, but it doesn't declare explicit token permissions. To make scheduled/manual runs reliable across org/repo settings, add permissions: contents: write (and any other required scopes) at the workflow or job level.
| run: | | ||
| DATE=$(date +%Y%m%d) | ||
| SHORT_SHA=$(git rev-parse --short HEAD) | ||
| TAG="v${DATE}-sync-${SHORT_SHA}" |
There was a problem hiding this comment.
The tag name v${DATE}-sync-${SHORT_SHA} can collide if the workflow is re-run for the same commit/day (e.g., manual re-run after a transient failure), causing git tag/git push to fail. Consider checking for an existing tag before creating it, or including ${{ github.run_id }}/${{ github.run_number }} in the tag to guarantee uniqueness.
| TAG="v${DATE}-sync-${SHORT_SHA}" | |
| TAG="v${DATE}-sync-${SHORT_SHA}-${GITHUB_RUN_ID}" |
| - name: Push merged changes | ||
| if: steps.check.outputs.has_changes == 'true' | ||
| run: | | ||
| git push origin HEAD | ||
|
|
There was a problem hiding this comment.
Pushing merged changes directly to origin HEAD on a schedule can fail on protected default branches and makes it easy to publish unreviewed upstream merges. Consider pushing the merge result to a dedicated bot branch and opening/updating a PR instead, then tagging only after that PR is merged.
No description provided.