Assert unknown per-firm fields survive a store -> save -> load cycle

[michieldegezelle]

Summary

• Adds one regression test: an unrecognized per-firm field (e.g. one written by a future CLI version) must survive storeNewTokenPair -> saveCredentials -> loadCredentials.
• No production code change - the behavior already exists (loadCredentials assigns the parsed object directly, storeNewTokenPair merges into the existing per-firm record rather than replacing it). This just locks it in.

Why this instead of the larger corruption-handling change

This was split out of a much larger branch (see #270) that also made loadCredentials fail loudly on a malformed config file instead of resetting to {}. A second look at the CI auth root-cause doc found that failure mode had been specifically investigated across 36 incidents in 14 months and never observed - every incident was the revoke-on-use/concurrency class instead. The larger branch is parked, not deleted, in case that changes. This test is the one piece with a real, non-speculative driver: step 8 of the CI auth plan seeds a flag into CONFIG_JSON and needs to know the CLI won't strip it back out.

Test plan

• npx jest tests/lib/api/firmCredentials.test.js - new test passes

• Verified non-vacuous against three independent failure modes, each confirmed red before its fix and green after: a broken merge in storeNewTokenPair (replace instead of merge), a completely no-op loadCredentials() (the original assertion didn't actually depend on load reconstructing state - fixed by clearing in-memory data before the load and asserting on the serialized writtenData directly), and a skipped write

• npm test - 752 tests, 46 suites, all passing (verified with a fresh run just before this update)

• npm run lint - clean

• Skip bumping the CLI version

[coderabbitai]

Note

Reviews paused

It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

• @coderabbitai resume to resume automatic reviews.
• @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

• ▶️ Resume reviews
• 🔍 Trigger review

Note

Some coding-guideline sources were skipped; the reason is listed next to each one. Other guidelines were applied as usual. On a public repository, sources from your configuration are listed without their names.

Skipped guideline sources (1)

a configured source — invalid entry; expected repo:path or owner/repo:path with a relative file path

---

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

• Configuration used: Organization UI
• Review profile: CHILL
• Plan: Essentials
• Run ID: baaf9691-ab17-4e1d-b081-e844c42aefea

---

📥 Commits

Reviewing files that changed from the base of the PR and between 4729242 and e4878d3.

---

⛔ Files ignored due to path filters (1)

• package-lock.json is excluded by !**/package-lock.json

---

📒 Files selected for processing (2)

• CHANGELOG.md
• package.json

---

Included review availability: This review used your included allowance. 3 included reviews remain after this review. Your included PR review attempts over the past 7 days set your current allowance at 5 reviews per hour.

---

📜 Recent review details

⏰ Context from checks skipped due to timeout. (1)

• GitHub Check: test (22.x)

---

🧰 Additional context used

📚 Code guidelines (1)

CLAUDE.md — auto-discovered

---

📓 Path-based instructions (1)

Run `npm run lint && npm test` after code changes.

📄 CodeRabbit inference engine (CLAUDE.md)

Files:

• package.json
• CHANGELOG.md

---

🔇 Additional comments (2)

package.json (1)

3-3: LGTM!

---

CHANGELOG.md (1)

6-6: 📐 Maintainability & Code Quality

The “Dependencies updated” statement is accurate. package-lock.json changes multiple dependency versions, including adm-zip, @types/node, acorn, and yaml. Remove no text from the changelog.

---

---

Walkthrough

The change adds test coverage and documentation for preserving unknown per-firm credential fields across token storage, saving, and reloading. It also updates the changelog and package version to 1.60.1.

Changes

Firm credential persistence

Layer / File(s)	Summary
Credential round-trip coverage tests/lib/api/firmCredentials.test.js, tests/TESTS.md, CHANGELOG.md, package.json	The test checks that saving writes defined credential data and that unknown per-firm fields survive reloading. The test catalogue and changelog describe the test. The package version changes to 1.60.1.

Priority: ⬇️ Low

Estimated code review effort: 2 (Simple) | ~10 minutes

Merge Risk: ⚪ Minimal · up to e4878

No actionable merge-blocking risk is established by this test and release-metadata change.

Pre-merge checks | 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Title check		The title clearly identifies the main change: testing that unknown per-firm fields survive the store, save, and load cycle.
Description check		The description provides a clear summary, rationale, detailed test plan, test results, and author checklist. It uses equivalent headings instead of the template headings and omits the issue-reference …
Docstring Coverage		No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff. Analyzed 0 functions across 1…
Linked Issues check		Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check		Check skipped because no linked issues were found for this pull request.

---

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Commit to this branch
• Create a new PR

---

---

_{Comment @coderabbitai help to get the list of available commands.}

[michieldegezelle]

/review-pr (dry run)

Scope: 2 files, 1 commit at review time (now 3, after fixes below). No existing reviewer comments (fresh PR). 5 finders ran (correctness, regression, edgecases, architecture-cli, cli-conventions) — no security/error-handling signal in a test-only diff, so those two were skipped.

Fixed (2 Minor, both in the test itself):

• Didn't assert the write actually happened before feeding it back into the load — a regression that skips the save would've failed opaquely instead of pointing at the missing write.
• Used a persistent mockImplementation instead of mockImplementationOnce for a single expected call, inconsistent with this file's own convention; jest.clearAllMocks() doesn't clear a registered implementation.

Deliberately deferred (2 Suggestions): broaden the round-trip test to cover falsy/nested unknown-field shapes, not just a truthy boolean; restructure to mirror the sibling loadCredentials test's isolateModules scoping exactly. Both are reasonable but out of scope for a test meant to stay minimal and tied to one real driver (step 8 of the CI auth plan) — logged instead of implemented.

752/752 tests pass, lint clean.

[michieldegezelle]

Follow-up: a second review caught a real gap in the test itself

A second reviewer found the final assertion didn't actually depend on loadCredentials() doing anything — storeNewTokenPair's own in-place merge already produces the expected shape before load ever runs, so a stubbed no-op load still passed. Verified this empirically (spied loadCredentials as a no-op, confirmed the old test still went green) before fixing.

Fixed by: clearing the in-memory data before the load (so the assertion has to come from the reload, not leftover state) and adding a direct assertion on the serialized writtenData (the actual step-8 driver — what lands in CONFIG_JSON, independent of whether this instance ever reloads). Re-verified all three failure modes now go red before their respective fix: broken merge, no-op load, skipped write.

Squashed the three commits into one and force-pushed (this PR wasn't merged, no shared history to disrupt) — history now reads as a single clean commit rather than the review trail. PR body corrected to describe the actual verification. 752/752 tests pass, lint clean.