fix(github-pat): level every GITHUB_PAT line, not just the first (#2016)

[dolho]

Summary

_patch_env_github_pat replaced the first matching line (count=1) while the agent's own .env reader is last-wins. On a file carrying a duplicate, the rotation wrote the new token to line 1, the revoked token survived below it, and the agent kept authenticating with the revoked one — while propagate_pat_to_all_agents reported that agent as updated.

in:  GITHUB_PAT="old-token"          out: GITHUB_PAT="ghp_NEW"
     FOO=1                                FOO=1
     GITHUB_PAT="old-token"               GITHUB_PAT="old-token"   <-- survives, and wins

agent reads GITHUB_PAT = 'old-token'

Same silent-success failure #1967 exists to close, reached from the other side: there the rotation never reached the agent; here it reaches the agent and the agent ignores the result.

Where the duplicate comes from

Not from this function — it appends only when the key is absent, and substitutes when present. It arrives from the other writers of that file: an agent editing its own .env (#1999), an operator appending over SSH or docker exec, or a restored/hand-merged file. Uncommon, which is why this is P3; silent and credential-shaped, which is why it's worth the seven-character fix.

Levelled, not de-duplicated

Both were on the table. After levelling, every copy carries the same value, so last-wins reads the right token whichever line it lands on — and the file keeps whatever structure the operator gave it. Removing lines would be a second behaviour change for no correctness gain, so I didn't.

Test plan

• tests/unit/test_2016_duplicate_pat_line.py — 21 tests. Every assertion goes through what the agent reads, not what the file contains; that distinction is the bug, since the file did contain the new token, on a line nothing read
• Covers: the reproduction; the revoked token surviving nowhere in the file (shells and greps read .env too); 2/3/5 copies; each of the three Wire the agent GitHub PAT for the gh CLI + REST API (not just git) #1574 mirrored keys independently, since each runs its own substitution and a cap left on one reopens the bug for that key alone; interleaved duplicates of different keys; the messy real-world duplicate shapes (indented, tabbed, unquoted, single-quoted, empty-value); a commented duplicate deliberately left alone; lookalike keys untouched; and the single-line path unchanged including idempotence
• Mutation-verified against both spellings of the cap — count=1 and a positional third argument — 15 of 21 fail on each
• 1101 adjacent tests green

One test-quality note worth recording: the structural guard reads the .sub() call via ast, not the source text. This function's own docstring explains the bug and therefore contains the string count=1, so a textual scan passes on the prose with the cap restored. My first draft did exactly that and failed here — the same trap the ledger records for #1871 and ent#314, and that ent#237's auth guard hit last week when ast.dump rendered a docstring.

Merge order

⚠️ Conflicts with #2024 (issue #2017) on one line — both change line_re.sub(...). The resolution is to keep both edits:

out = line_re.sub(lambda _match: new_line, out)     # #2017 callable + #2016 no cap

Whichever lands second takes that line. Kept as separate PRs because they are separate defects with separate tests; branched off dev rather than stacked so both get the full pytest/CodeQL matrix, which a feature-branch base would skip.

Observed once, not reproduced

During adjacent-suite runs I saw a single collection error in test_subscription_auto_switch_pingpong.py — a file this PR doesn't touch. It did not reproduce in five subsequent runs including three fixed seeds, and my new file passes when run directly alongside it. It looks like the pre-existing order-dependent sys.modules interference this suite already has, perturbed by adding a file. Flagging rather than omitting, since I can't prove it isn't mine.

Closes #2016

[obasilakis]

Approving. The fix is right, the reasoning about why it is a bug is right, and the test that pins it is the best-constructed guard in this batch — I tried to defeat it two different ways and could not.

The core insight is the part worth keeping: the file did contain the new token, on a line nothing read. Asserting through the agent's own last-wins parser instead of through the file contents is what makes these tests mean anything, and the module docstring says so explicitly.

Verified

Fix behaves as described — 21 tests pass on the branch.

The guard survives mutation, both spellings. I reverted the fix two ways on the merged tree and re-ran test_the_substitution_is_not_capped_at_one:

restore `count=1` keyword       -> FAIL (red)   correct
restore positional `sub(x, y, 1)` -> FAIL (red)   correct

The second one is the reason this guard is good. assert len(call.args) <= 2 catches the cap written positionally, which a keyword-only check would have missed — and that is exactly how a cap tends to come back during a refactor. The AST approach also sidesteps the trap your own docstring records:

this function's own docstring explains the bug and therefore contains the string count=1, so a textual scan passes on the prose with the cap restored. My first draft did exactly that and failed here.

Worth flagging that the sibling PR #2024 did not clear that trap — its test_the_replacement_is_a_callable_not_a_string substring-matches "lambda" against inspect.getsource(...), and its own added comment contains the word lambda, so it passes with the fix reverted and the bug live. I have asked for it to adopt the shape you used here. Same function, same batch, one got it and one didn't — which is itself the argument for the AST form being the house pattern.

Level rather than de-duplicate is the right call and the docstring justifies it properly: after the change every copy carries the same value, so last-wins reads correctly whichever line it lands on, and the operator's file structure is preserved. Removing lines would be a second behaviour change buying nothing. The tests cover the shapes that actually occur — differently-formatted duplicate, commented duplicate left alone, lookalike keys (MY_GITHUB_PAT, GITHUB_PATX) untouched, each of the three #1574 mirror keys levelled independently.

agent_reads is byte-faithful to docker/base-image/agent_server/routers/credentials.py:379-386 — checked line by line (strip, skip blank/#/no-=, partition("="), key.strip(), value.strip().strip('"').strip("'"), last write wins).

Composition with #2024 — they conflict, and neither side is correct alone

Both PRs rewrite the same three lines. The conflict is:

<<<<<<< HEAD
            out = line_re.sub(lambda _match: new_line, out, count=1)
=======
            out = line_re.sub(new_line, out)
>>>>>>> origin/fix/2016-duplicate-pat-line

Correct resolution is the composition of both:

out = line_re.sub(lambda _match: new_line, out)

Taking your side wholesale drops #2024's callable and reopens the backslash crash; taking theirs restores count=1 and reopens this bug. Your guard catches the second direction, which is the more dangerous one — so if you land first, CI will protect you. I confirmed the two fixes compose: with the line above, all of #2024's, #2025's and #2018's PAT tests behave as expected on the merged tree.

Whichever of you lands second: also delete the matching xfail(strict=True) from tests/unit/test_pat_propagation_properties.py (#2018) — for this PR that is test_a_duplicated_pat_line_still_rotates, which otherwise turns the suite red by passing.

Note, not blocking

tests/registry.json conflicts against dev. Re-serializing from parsed JSON rather than splicing the conflict markers avoids losing the separating comma. Noise, not a finding.

No credential values in the diff or the fixtures.

[github-actions]

⚠️ Nightly unit-suite check skipped — merge conflict against dev.

Resolve by running git merge dev locally and pushing the result. The next nightly run will re-test once the conflict is gone.

[vybe]

Rebased onto dev. Only tests/registry.json conflicted (this branch's entry against the ones that landed today); resolved from the merge stages rather than by splicing markers — 131 dev entries + this branch's one = 132, no duplicates, valid JSON. github_pat_propagation_service.py merged clean. 21/21 tests pass locally after the rebase.

The reasoning for levelling duplicates rather than de-duplicating them is the right call and worth keeping in the docstring: the agent's .env reader is last-wins, so making every copy carry the same value fixes the read whichever line wins, while deleting lines would be a second behaviour change with no correctness gain on a file an operator may have structured deliberately.

Merging on the existing approval.