fix(login): preserve runpodctl credentials in ~/.runpod/config.toml

[vaskoz]

Summary

• flash login was clobbering the top-level apikey/apiurl values that runpodctl writes to ~/.runpod/config.toml. The cause: save_api_key() delegated to runpod-python's set_credentials(overwrite=True), which opens the file in "w" mode and rewrites it with only a [default] section.
• save_api_key() now does a line-based merge that updates only the [default].api_key field and preserves everything else — runpodctl's top-level keys, other profile sections, and any unrelated fields within [default].

Test plan

• New unit tests cover: preserving runpodctl top-level keys, adding [default] when missing, preserving other profile sections, fresh-file creation
• Existing test_credentials.py and test_login.py still pass (26/26)
• Smoke-tested merger against real-world TOML shapes (file with both formats coexisting, only top-level keys, empty file, [default] without api_key, blank lines between sections)

🤖 Generated with Claude Code

[Copilot]

Pull request overview

This PR updates flash login credential persistence to avoid clobbering runpodctl’s top-level apikey/apiurl keys in ~/.runpod/config.toml by performing a targeted text merge that updates only [default].api_key.

Changes:

• Replace runpod-python’s set_credentials(overwrite=True) usage with a line-based upsert that preserves unrelated TOML content.
• Add unit tests covering mixed runpodctl + [default] formats, missing [default], preserving other profiles, and fresh-file creation.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.

File	Description
src/runpod_flash/core/credentials.py	Implements _upsert_default_api_key() and updates save_api_key() to preserve non-flash TOML content.
tests/unit/test_credentials.py	Adds unit tests to validate config preservation behavior when saving API keys.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[deanq]

Inline review — see comments. Bottom line: the manual TOML merge has several edge cases (inline comments on [default], CRLF, untested escaping branches) that a tomlkit round-trip would eliminate. Falls back to specific edge-case requests if the manual approach stays.

[runpod-Henrik]

Henrik's AI-Powered Bug Finder — PR #333 Review

Verdict: NEEDS WORK

The bug being fixed is real — set_credentials(overwrite=True) from runpod-python opens the file in "w" mode and obliterates runpodctl's top-level apikey/apiurl keys. The fix is in the right direction. CI green on 3.10/3.11/3.12.

The implementation reaches for hand-rolled TOML parsing via regex, and the section-boundary detection has at least one edge case that silently destroys an unrelated profile's credentials. Worth addressing before merge.

---

1. Bug: a [default] # comment or [staging] # comment section header makes the merger overwrite the wrong section's api_key

User scenario: a user has a config file someone (or another tool) has annotated with inline comments on the section headers — common in shared dev environments and pasted snippets:

```toml
[default]
some_other_field = "x"

[staging] # production environment
api_key = "staging-key-here"
```

User runs flash login. Expected: [default].api_key is set to the new key, [staging] untouched. Actual: [staging].api_key is overwritten with the new flash key, and [default] never gets an api_key line.

Walk through _upsert_default_api_key:

1. _DEFAULT_HEADER_RE.match on [default] → matches at line 0. default_start = 0.
2. Inner loop scans for the next section header:
   • line 1 (some_other_field = "x") → no match
   • line 2 ([staging] # production environment) — _SECTION_HEADER_RE = r"^\s*\[[^\]]+\]\s*$" requires the line to end after the closing bracket and optional whitespace. The inline comment breaks the match. → no match
   • line 3 (api_key = "staging-key-here") → no match
3. Inner loop exits without finding a section boundary. default_end = len(lines).
4. Replacement scan from default_start+1 to default_end: line 3 matches _API_KEY_LINE_RE → gets replaced with api_key = "<new flash key>".
5. The user's staging credentials are silently destroyed. No warning, no log.

Repro test (suggest adding):

```python
def test_inline_comment_on_section_header_does_not_overwrite_other_profile(
self, isolate_credentials_file
):
isolate_credentials_file.parent.mkdir(parents=True, exist_ok=True)
isolate_credentials_file.write_text(
"[default]\n"
"some_other_field = "x"\n"
"\n"
"[staging] # production environment\n"
'api_key = "staging-key"\n'
)
save_api_key("new-flash-key")
parsed = tomllib.loads(isolate_credentials_file.read_text())
assert parsed["default"]["api_key"] == "new-flash-key"
assert parsed["staging"]["api_key"] == "staging-key" # currently FAILS
```

The cleanest fix is the one another reviewer already suggested: use tomlkit (round-trips comments + structure), or use stdlib tomllib/tomli to parse + Python serializer to write. The hand-rolled regex approach will keep producing edge cases like this.

If staying with regex for some reason: tighten the section-header regex to accept inline comments, e.g.

```python
_SECTION_HEADER_RE = re.compile(r"^\s*[[^\]]+]\s*(#.)?$")
_DEFAULT_HEADER_RE = re.compile(r"^\s[default]\s*(#.*)?$")
```

And add a test for [default] # comment too.

---

2. Issue: incomplete TOML string escaping

_toml_quote escapes only \ and ". The TOML spec requires escaping control characters (U+0000–U+001F and U+007F) inside basic strings. User scenario: a future deploy or auth path passes an api_key containing a tab or newline (unlikely for prod API keys, but plausible for trimmed-from-clipboard edge cases — and PR #337 ships a new copy-paste flow where the user could paste extra whitespace). The written file becomes invalid TOML, and the next flash login or flash deploy fails at credential load with an opaque parser error.

Two cheap mitigations:

• Use TOML literal strings ('...' single quotes — no escape sequences allowed, but no control chars either). Safer for keys that are alphanumeric anyway.
• Reject the api_key with a clear error if it contains non-printable chars before writing.

Or — again — let tomlkit/tomllib handle serialization.

---

3. Issue: line endings drift on Windows / CRLF files

content.splitlines(keepends=True) preserves the original \r\n for existing lines, but the new api_key = ... line is appended with only \n. A Windows-edited config will get a \n-terminated line in a sea of \r\n lines after running flash login.

Most editors and tools handle mixed line endings, so this is cosmetic. But it'll show up as a single-line diff in version control or in editors that surface line-ending mismatches. Worth detecting the predominant ending and matching it, or normalizing on write.

---

4. Question: behavior when [default] exists with no api_key and the section has trailing blank lines

The insert path:

```python
insert_idx = default_end
while insert_idx > default_start + 1 and lines[insert_idx - 1].strip() == "":
insert_idx -= 1
lines.insert(insert_idx, new_line + "\n")
```

This walks back over blank lines and inserts just below the last non-blank line in [default]. Looks correct, but no test covers it. Worth one:

```python
def test_inserts_api_key_above_trailing_blank_lines_in_default(...):
text = "[default]\nname = "primary"\n\n\n[staging]\n..."
# After: api_key sits next to name, blank lines preserved between [default] and [staging]
```

Not a bug claim — just untested behavior worth pinning down.

---

5. Nit: line-based regex on TOML doesn't survive multi-line strings

If [default] ever contains a multi-line basic string (description = """\n...\n""") whose content happens to include a line matching ^\s*api_key\s*=, the merger would replace inside the string literal. Vanishingly unlikely for the credentials file but worth noting that a real TOML parser would eliminate the class of edge cases entirely. Same recommendation as #1.

---

Nits

• Test coverage is good for the documented cases — preserve top-level keys, missing [default], other profile sections, fresh file. Just missing the comment-on-section-header case (above) and the trailing-blank-lines case (above).
• The _OLD_XDG_PATH migration path is untouched — separate concern, but worth confirming the migration helper still works correctly when the source file has the same multi-format shape (runpodctl top-level + flash [default]).
• 0600 perms set after write — good. Worth confirming this works on Windows (where os.chmod is a partial no-op); current try/except OSError swallows it correctly, so Windows users just won't have restrictive perms, which is the existing behavior.

---

🤖 Reviewed by Henrik's AI-Powered Bug Finder

[deanq]

Pushed dfbc9ca addressing the review feedback by replacing the regex merge with a tomlkit round-trip — the approach Copilot, and Henrik's bug-finder all converged on. This resolves the edge cases structurally rather than patching them one regex at a time.

Implementation

• _upsert_default_api_key now parses with tomlkit, sets [default].api_key, and dumps. Foreign top-level keys (runpodctl's apikey/apiurl), sibling profiles, comments, and escaping are preserved by construction.
• save_api_key reads/writes with newline="" so CRLF files keep their endings (read_text was collapsing \r\n → \n).
• Dropped the three header/key regexes and _toml_quote; added _DEFAULT_SECTION/_API_KEY_FIELD constants.
• Declared tomlkit as a direct dependency (was transitive via runpod).

How each item is covered (all with regression tests)

• Inline-comment section header overwriting another profile → fixed; [staging] left untouched.
• [default] # comment → updated in place, no duplicate table.
• [default] with no trailing newline → valid TOML output.
• Backslash / double-quote / control-char keys → round-trip through tomllib.
• CRLF preservation → byte-level assertion.
• [default] without an api_key field → key inserted, siblings preserved.
• Dropped the brittle substring assertions (kept the tomllib-parsed checks).

One behavior change worth noting: tomlkit.parse raises on malformed existing TOML, where the old regex appended regardless. I kept it fail-loud rather than reintroduce clobber-on-corrupt, since silently overwriting is exactly the data loss this PR fixes. Happy to add a guarded fallback if reviewers prefer.

make quality-check green: 2574 + 53 tests pass, coverage 85.94%.

[Copilot]

Pull request overview

Copilot reviewed 3 out of 4 changed files in this pull request and generated 1 comment.