fix(server): a duplicate variable row no longer deletes a stored secret#31337

The server now preserves stored secrets when duplicate provider environment rows include a redacted row. Tests cover saving and reloading duplicate rows, explicit new values, and secret removal after all rows with a name are deleted.

Changes: Duplicate secret preservation

Layer / File(s)	Summary
Retain secrets claimed by redacted rows apps/server/src/serverSettings.ts, apps/server/src/serverSettings.test.ts	persistSettingsSecrets tracks secrets claimed by redacted rows, defers inline writes when needed, and filters out conflicting removals. Tests cover duplicate values and ordering, secret references, reload behavior, and removal after all matching rows are deleted.

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

Change: Bug fix · Severity of issue fixed: Medium

Possibly related PRs

• pingdotgg/t3code#27115: This PR changes how provider environment values represented as SecretReference objects pass through the secret persistence path.

Suggested reviewers: yordis, maria-rcks

The change stops a duplicate variable row from deleting a stored provider secret, and it keeps the cleanup and legacy-secret paths working. The earlier review concerns were addressed and no actionable merge risk remains.

^Failed Failed checks: 2 warnings

Check name	Status	Explanation	Resolution
Out of Scope Changes check	Warning	The incremental range 9984ae9e to ded4c749 adds changes that do not connect to #31328, which concerns duplicate environment-variable rows. (1) apps/server/src/serverSettings.ts: updateSettings now cal…	Remove the secret-provider removal and older-client reference echo changes from this PR. This includes their call sites in updateSettings, the new secret-provider and cloudSpeechCredentials/simlockDeviceHosts test additions, and the strictS…
Description check	Warning	The description states the bug, reproduction steps on main, the root cause and fix in persistSettingsSecrets, legacy inline handling, stale-secret cleanup, and a detailed verification section with tes…	Strengthen the Scope and approval section. Link the triaged bug issue #31328 and include the maintainer approval comment or discussion that sets direction and scope, or explain why this focused fix of an obvious bug qualifies without a prio…

^Passed Passed checks: 2 passed

Check name	Status	Explanation
Title check	Passed	The title names the main change: a duplicate variable row no longer deletes a stored secret. It follows the conventional commit form and matches the server-side secret-retention changes in serverSetti…
Linked Issues check	Passed	[#31328] requires that saving an empty sensitive duplicate of a provider environment variable does not delete a stored secret that a redacted row still claims. Scope: whole PR, merge base 67eebaca23 t…

Full details: Title check

Explanation

The title names the main change: a duplicate variable row no longer deletes a stored secret. It follows the conventional commit form and matches the server-side secret-retention changes in serverSettings.ts and its tests. It is specific and clear, with no vague terms.

Full details: Linked Issues check

Explanation

[#31328] requires that saving an empty sensitive duplicate of a provider environment variable does not delete a stored secret that a redacted row still claims. Scope: whole PR, merge base 67eebaca23 to head ded4c749. The incremental range 9984ae9e to ded4c749 does not change persistSettingsSecrets in apps/server/src/serverSettings.ts. The kept-slot filter, the inline-write fallback, and the sensitive-only stale cleanup from 9984ae9e are unchanged, so the earlier PASS basis still holds. The incremental change to the update path adds withEchoedSecretReferences before vetting and withoutRemovedSecretProviderReferences before persistSettingsSecrets. These calls do not alter the duplicate-row logic in the code shown. The test file adds two duplicate-row cases to the existing table: 'plain then redacted' and 'secret reference then redacted'. Limits: tests were not run, so the pass results are author-reported. The bodies of withEchoedSecretReferences and withoutRemovedSecretProviderReferences were not read, so their effect on the env-row path is not verified from code. Re-delivery reconciliation (#30115) is not changed.

Full details: Out of Scope Changes check

Explanation

The incremental range 9984ae9e to ded4c749 adds changes that do not connect to #31328, which concerns duplicate environment-variable rows. (1) apps/server/src/serverSettings.ts: updateSettings now calls withEchoedSecretReferences on the update result before vetting. It also calls withoutRemovedSecretProviderReferences before persistSettingsSecrets. The code comment describes older clients that echo a reference as the redacted marker, and clearing references of a removed secret provider. Neither behavior concerns duplicate environment-variable rows. (2) apps/server/src/serverSettings.test.ts: adds the test 'removing a secret provider unsets every field that read from it'. It extends the legacy echo test with cloudSpeechCredentials, simlockDeviceHosts, and providerInstances apiKey and serverPassword fields. It adds cloudSpeechCredentials and simlockDeviceHosts to strictSecretReferencePaths. It adds a policy-hidden variable case for secret-provider removal. None of these checks a duplicate-name row. The PR description states a server-only change that fixes duplicate-row deletion. It does not mention secret providers, older-client reference echo, cloudSpeechCredentials, or simlock. The changes in persistSettingsSecrets and its duplicate-row tests remain connected to #31328. Limit: the definition of withEchoedSecretReferences is not in the reviewed hunks, so whether it predates this PR is not established.

Resolution

Remove the secret-provider removal and older-client reference echo changes from this PR. This includes their call sites in updateSettings, the new secret-provider and cloudSpeechCredentials/simlockDeviceHosts test additions, and the strictSecretReferencePaths additions. Move them to a separate PR that links its own issue. Keep the persistSettingsSecrets changes and the duplicate-row tests. If these changes must stay, add a linked issue that requires them and update the PR description to list each change and its test. Confirm whether withEchoedSecretReferences exists at the merge base.

Full details: Description check

Explanation

The description states the bug, reproduction steps on main, the root cause and fix in persistSettingsSecrets, legacy inline handling, stale-secret cleanup, and a detailed verification section with test counts, a check:changed run, and a before/after runtime trace. It explicitly states what was not checked (mobile and desktop clients) and justifies no screenshots because the change is server-only with no visible UI difference. The template's Scope and approval section is weak: it only says 'Fixes pingdotgg/t3code#31328' and does not link a triaged issue with explicit maintainer approval of direction and scope, nor explain why a very small focused fix qualifies without prior approval. The template's conventional commit title guidance is not addressed in the body, and the description does not state the model and harness in a required form, though it does name Claude Opus 5.5 in Claude Code. Verification test count is also inconsistent with the objectives (75/75 in the description vs 72/72 in the objectives summary), which the author should reconcile.

Resolution

Strengthen the Scope and approval section. Link the triaged bug issue #31328 and include the maintainer approval comment or discussion that sets direction and scope, or explain why this focused fix of an obvious bug qualifies without a prior issue or discussion. Reconcile the test count: the description reports 75/75 passing, while the PR objectives report 72/72. Confirm the correct number and update the description. Optionally, add a conventional commit style title as the template suggests.

Generate docstrings

• Commit to this branch
• Create a new PR

Generate unit tests (beta)

• Commit to this branch
• Create a new PR

10 historical turns, 5 command tools per turn, 878.9 KiB retained MCP result per historical turn, and a 1.05 MiB retained result in the measured turn.

• Codex decoded thread snapshot: 108.5 KiB
• Claude decoded thread snapshot: 108.8 KiB

Script executed:

shell

1	#!/bin/bash
2	set +e
3
4	echo '=== Current PR metadata ==='
5	gh pr view 31337 --repo pingdotgg/t3code --json title,body,baseRefName,baseRefOid,headRefName,headRefOid,mergeStateStatus
6
7	echo '=== Current PR changed files and patches ==='
8	gh api --paginate repos/pingdotgg/t3code/pulls/31337/files --jq '.[] | {filename,status,additions,deletions,patch}'
9
10	echo '=== Referenced PR metadata ==='
11	for n in 31651 31646; do
12	gh pr view "$n" --repo pingdotgg/t3code --json number,title,state,mergedAt,mergeCommit,baseRefName
13	done
14
15	echo '=== Local branch and ancestry ==='
16	git status --short --branch
17	git log -8 --format='%h %p %s'
18	git log --format='%h %p %s' 9984ae9e..ded4c749 2>&1
19
20	echo '=== Local base comparison when available ==='
21	for base in refs/remotes/origin/main refs/heads/main; do
22	if git show-ref --verify --quiet "$base"; then
23	echo "Base: $base"
24	git merge-base "$base" HEAD
25	git diff --stat "$base"...HEAD
26	git diff "$base"...HEAD -- apps/server/src/serverSettings.ts apps/server/src/serverSettings.test.ts
27	break
28	fi
29	done
30
31	echo '=== Referenced source outlines ==='
32	for f in apps/server/src/serverSettings.ts apps/server/src/serverSettings.test.ts; do
33	if test -f "$f"; then
34	echo "--- $f ---"
35	ast-grep outline "$f" --items all --match 'persistSettingsSecrets|updateSettings|withEchoedSecretReferences|withoutRemovedSecretProviderReferences|duplicate|secret' --view expanded
36	rg -n -C 3 'persistSettingsSecrets|withEchoedSecretReferences|withoutRemovedSecretProviderReferences|strictSecretReferencePaths|removing a secret provider|legacy echo|duplicate name' "$f"
37	else
38	echo "File unavailable: $f"
39	fi
40	done

Length of output: 64268

---